Commit ce92f8e9 authored by DevPilot's avatar DevPilot

fix(cash-sessions): give every branch its own till instead of one per person

A cashier could hold only one open drawer across the whole academy, and the
three screens that ask about it did not agree on which one. Opening a shift
checked every branch (withoutBranchScope), while the POS terminal and the
manage screen looked only at the branch being worked in. So a drawer left open
at one branch locked every other branch out of selling: the terminal said "open
a shift first", the open form answered "there is already one open", and the
manage screen offered nothing to close, because the shift it was refusing over
was in a branch that screen will not show. On OC-Sport three accounts had
drawers open at ZSC since July, which is every account the other seven branches
sell through.

A drawer is a physical box standing in one branch — its float, its cash in and
its variance at close all belong to that branch's reconciliation, and
POSService already refuses to ring a sale against another branch's session. So
the invariant it can actually carry is one open drawer per cashier per branch,
which is also what the desks need: each branch opens its own shift and collects
normally, and closing one is never a precondition for another.

Also fixed, because more than one session per user can now be open at once:

- getOpenSession() resolves a named branch through forBranch() rather than
  filtering on top of the request scope. RefundService asks it for the drawer of
  the branch whose money is going back out; from any other branch that returned
  null and the refund silently skipped the cash count.
- UpdateCashSessionTotals prefers the drawer the payment names, and narrows its
  fallback to the payment's own branch. It runs on a queue where the branch
  scope is off, so an unqualified first() would have counted one branch's cash
  into another branch's box.
- Both screens now name the branches where the cashier still has a drawer open,
  so "there is already an open shift" is something the desk can act on.
Co-Authored-By: 's avatarClaude Opus 5 <noreply@anthropic.com>
parent 5e1d9b4b
......@@ -18,8 +18,26 @@ public function handle(PaymentReceived $event): void
return;
}
$cashSession = CashSession::where('user_id', $event->actor->id)
// The drawer the money actually went into. The payment names it
// whenever the collecting screen knew it, and that is the only
// answer that cannot be wrong.
//
// Falling back to "this user's open session" has to be narrowed to
// the payment's own branch: a cashier may now hold a drawer open in
// more than one branch, and this listener runs on a queue where the
// branch scope is off, so an unqualified first() would count one
// branch's cash into another branch's box.
$cashSession = $payment->cash_session_id
? CashSession::withoutBranchScope()
->where('id', $payment->cash_session_id)
->where('status', 'open')
->first()
: null;
$cashSession ??= CashSession::forBranch($payment->branch_id ? (int) $payment->branch_id : null)
->where('user_id', $event->actor->id)
->where('status', 'open')
->orderByDesc('opened_at')
->first();
if (!$cashSession) {
......
......@@ -13,18 +13,30 @@ class CashSessionService
public function open(User $user, int $branchId, int $openingBalance, ?string $notes = null): CashSession
{
return DB::transaction(function () use ($user, $branchId, $openingBalance, $notes) {
// Guard: user must not have another open session — in ANY branch.
// One person cannot stand at two tills, so this is a person-level
// invariant and the branch scope would quietly weaken it: a cashier
// who left a drawer open at one branch would just open a second one
// after switching, and neither would ever reconcile.
$existingOpen = CashSession::withoutBranchScope()
// Guard: one open drawer per cashier, per branch.
//
// This used to be per person across every branch, reasoning that
// one person cannot stand at two tills. Every branch is somewhere
// else and they share logins, so what it actually produced was a
// deadlock that stopped the tills selling: a drawer left open at
// one branch, and at every other branch the POS asked for a shift
// (its lookup is branch-scoped, so it saw none here), opening one
// was refused (this check was not, so it saw the one over there),
// and the open drawer could not be closed from here either because
// the manage screen also only ever shows this branch's.
//
// A drawer is a physical box standing in one branch, and every
// amount on it — opening float, cash in, the variance at close —
// belongs to that branch's reconciliation. So the invariant it can
// carry is one drawer per cashier per branch; across branches the
// sessions are separate boxes and never mix.
$existingOpen = CashSession::forBranch($branchId)
->where('user_id', $user->id)
->where('status', 'open')
->exists();
if ($existingOpen) {
throw new DomainException('يوجد وردية مفتوحة بالفعل. يجب إغلاقها أولاً');
throw new DomainException('يوجد وردية مفتوحة بالفعل في هذا الفرع. يجب إغلاقها أولاً');
}
return CashSession::create([
......@@ -117,9 +129,40 @@ public function recordCashOut(CashSession $session, int $amount): void
*/
public function getOpenSession(User $user, ?int $branchId = null): ?CashSession
{
return CashSession::where('user_id', $user->id)
// A named branch is answered from that branch, not through whichever
// one the request happens to be in: the caller that matters here is a
// refund, which asks for the drawer of the branch whose money is going
// back out. Filtering on top of the request scope answered "no drawer"
// whenever those two differed, and the refund then silently missed the
// cash count instead of reducing it.
$query = $branchId !== null
? CashSession::forBranch($branchId)
: CashSession::query();
return $query->where('user_id', $user->id)
->where('status', 'open')
->when($branchId, fn ($q) => $q->where('branch_id', $branchId))
->orderByDesc('opened_at')
->first();
}
/**
* This cashier's drawers left open in other branches.
*
* Purely so a screen can say where they are. A shift that is open
* somewhere else no longer blocks anything, but it is still money nobody
* has counted, and the person standing at this till is usually the one who
* can go and close it.
*
* @return \Illuminate\Database\Eloquent\Collection<int, CashSession>
*/
public function getOpenSessionsInOtherBranches(User $user, int $branchId)
{
return CashSession::withoutBranchScope()
->with('branch')
->where('user_id', $user->id)
->where('status', 'open')
->where('branch_id', '!=', $branchId)
->orderBy('opened_at')
->get();
}
}
......@@ -7,6 +7,7 @@
use App\Domain\Shared\Exceptions\DomainException;
use App\Domain\Shared\Traits\UsesBranchScope;
use Livewire\Attributes\Layout;
use Livewire\Attributes\Locked;
use Livewire\Attributes\Title;
use Livewire\Component;
......@@ -18,6 +19,17 @@ class CashSessionManage extends Component
public ?CashSession $currentSession = null;
/**
* Drawers this cashier has open in other branches — branch name and when.
* Display only, and locked: nothing here is read back into a query, but a
* plain public array is settable from the browser and this one is about
* where money is sitting.
*
* @var array<int, array{branch: string, opened_at: string}>
*/
#[Locked]
public array $otherBranchSessions = [];
// Open form
public float $opening_balance = 0;
public string $opening_notes = '';
......@@ -26,13 +38,35 @@ class CashSessionManage extends Component
public float $closing_balance = 0;
public string $closing_notes = '';
public function mount(): void
public function mount(CashSessionService $service): void
{
$this->authorize('cash_sessions.manage');
$this->currentSession = CashSession::where('user_id', auth()->id())
->where('status', 'open')
->first();
$this->loadSession($service);
}
/**
* The drawer for the branch being worked in — asked for by branch rather
* than left to the global scope, so this screen and the guard in
* CashSessionService::open() are answering the same question. They were
* not: this one saw only the current branch, the guard saw every branch,
* and between them a shift open elsewhere left the desk with a form that
* refused to submit and no drawer it could close.
*/
private function loadSession(CashSessionService $service): void
{
$branchId = $this->getActiveBranchId();
$this->currentSession = $service->getOpenSession(auth()->user(), $branchId);
$this->otherBranchSessions = $branchId
? $service->getOpenSessionsInOtherBranches(auth()->user(), $branchId)
->map(fn ($s) => [
'branch' => $s->branch?->name_ar ?? $s->branch?->name ?? '—',
'opened_at' => $s->opened_at?->format('Y-m-d H:i') ?? '—',
])
->all()
: [];
if ($this->currentSession) {
$expected = ($this->currentSession->opening_balance + $this->currentSession->total_cash_in - $this->currentSession->total_cash_out) / 100;
......
......@@ -72,6 +72,15 @@ class POSTerminal extends Component
// Cash session
public bool $hasOpenSession = false;
/**
* Branch names where this cashier still has a drawer open. Display only —
* locked because nothing the browser sends should be able to write it.
*
* @var array<int, string>
*/
#[Locked]
public array $otherBranchSessions = [];
// Participant search results
public array $searchResults = [];
......@@ -92,8 +101,23 @@ public function mount(CashSessionService $cashSessionService): void
{
$this->authorize('pos.sell');
$session = $cashSessionService->getOpenSession(auth()->user());
// The drawer this till sells from is the one open in the branch the
// cashier is working in — asked for by branch rather than left to the
// request scope, so the answer is the same one processTransaction()
// will insist on when the sale is rung up.
$branchId = $this->getActiveBranchId();
$session = $cashSessionService->getOpenSession(auth()->user(), $branchId);
$this->hasOpenSession = $session !== null;
// When there is no drawer here but one is open elsewhere, say so.
// "افتح وردية" answered by "there is already one open" with no hint of
// where is what left the desk stuck, unable to sell and unable to guess
// which branch to go and close.
$this->otherBranchSessions = $branchId
? $cashSessionService->getOpenSessionsInOtherBranches(auth()->user(), $branchId)
->map(fn ($s) => $s->branch?->name_ar ?? $s->branch?->name ?? '—')
->all()
: [];
}
public function updatedParticipantSearch(): void
......
......@@ -20,6 +20,28 @@ class="inline-flex items-center gap-2 px-4 py-2 bg-gray-100 text-gray-700 rounde
</div>
@endif
{{-- Drawers still open in other branches. Each branch's shift stands on
its own, so these block nothing here — they are shown because money
nobody has counted is otherwise invisible from this screen, which is
what made "there is already an open shift" impossible to act on. --}}
@if(count($otherBranchSessions))
<div class="mb-4 p-4 bg-blue-50 border border-blue-200 rounded-lg">
<p class="text-sm font-semibold text-blue-800 mb-2">{{ __('ورديات مفتوحة لك في فروع أخرى') }}</p>
<ul class="space-y-1">
@foreach($otherBranchSessions as $other)
<li class="text-sm text-blue-700 flex items-center gap-2">
<span class="w-1.5 h-1.5 bg-blue-500 rounded-full"></span>
<span class="font-medium">{{ $other['branch'] }}</span>
<span class="text-blue-600" dir="ltr">{{ $other['opened_at'] }}</span>
</li>
@endforeach
</ul>
<p class="text-xs text-blue-600 mt-2">
{{ __('لإغلاق أي منها اختر فرعها من القائمة العلوية. لا تمنعك من فتح وردية في هذا الفرع.') }}
</p>
</div>
@endif
@if($currentSession)
{{-- Current Open Session Info --}}
<div class="bg-white rounded-xl shadow-sm border border-green-200 p-4 sm:p-6 mb-4 sm:mb-6">
......
......@@ -13,6 +13,13 @@
</svg>
<h3 class="text-base sm:text-lg font-bold text-amber-800 mb-2">{{ __('يجب فتح وردية أولاً') }}</h3>
<p class="text-amber-700 text-xs sm:text-sm mb-4">{{ __('لا يمكن إجراء عمليات بيع بدون وردية مفتوحة') }}</p>
@if(count($otherBranchSessions))
<p class="text-amber-700 text-xs sm:text-sm mb-4 text-start bg-amber-100/70 border border-amber-200 rounded-lg p-3">
{{ __('لديك وردية مفتوحة في:') }}
<span class="font-semibold">{{ implode('، ', $otherBranchSessions) }}</span>
— {{ __('وردية كل فرع مستقلة، ويمكنك فتح وردية هنا دون إغلاقها.') }}
</p>
@endif
<a href="{{ route('cash-sessions.manage') }}"
class="inline-flex items-center gap-2 px-4 py-2.5 min-h-[44px] bg-amber-600 text-white rounded-lg hover:bg-amber-700 text-xs sm:text-sm font-medium transition-colors">
<svg class="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24">
......
<?php
namespace Tests\Feature;
use App\Domain\Financial\Events\PaymentReceived;
use App\Domain\Financial\Listeners\UpdateCashSessionTotals;
use App\Domain\Financial\Models\CashSession;
use App\Domain\Financial\Models\Payment;
use App\Domain\Financial\Services\CashSessionService;
use App\Domain\Shared\Context\BranchContext;
use App\Domain\Shared\Context\BranchScopeState;
use App\Domain\Shared\Exceptions\DomainException;
use App\Domain\Shared\Models\Academy;
use App\Models\User;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Str;
use Tests\TestCase;
/**
* A cash drawer belongs to a branch, so every branch can have one open.
*
* DB_CONNECTION=pgsql DB_DATABASE=oc_sport_test ./vendor/bin/phpunit --filter PerBranchCashSessionTest
*
* The bug this pins deadlocked every till but one. The guard on opening a
* shift looked across all branches; the POS and the manage screen looked only
* at the current one. A cashier with a drawer left open at branch A therefore
* arrived at branch B to find the terminal asking for a shift, the open form
* refusing with "there is already one open", and nothing on screen to close —
* the shift it meant was in a branch the screen would not show. On OC-Sport
* three users had drawers sitting open at ZSC since July, which is every
* account the other seven branches sell through.
*
* Everything runs inside a transaction that is rolled back: the tenant is a
* restored copy, but it is a copy of real money and these tests open tills.
*/
class PerBranchCashSessionTest extends TestCase
{
private User $cashier;
private int $branchA;
private int $branchB;
private CashSessionService $service;
protected function setUp(): void
{
parent::setUp();
if (config('database.default') !== 'pgsql') {
$this->markTestSkipped('Needs a restored Postgres tenant; see the class comment.');
}
if (! $academy = Academy::first()) {
$this->markTestSkipped('No academy in the restored tenant.');
}
app()->instance('current_academy', $academy);
$branches = DB::table('branches')
->where('academy_id', $academy->id)
->whereNull('deleted_at')
->orderBy('id')
->pluck('id')
->map(fn ($id) => (int) $id)
->all();
if (count($branches) < 2) {
$this->markTestSkipped('Needs at least two branches to have a second till.');
}
[$this->branchA, $this->branchB] = $branches;
$user = User::where('academy_id', $academy->id)->orderBy('id')->first();
if (! $user) {
$this->markTestSkipped('No user in the restored tenant.');
}
$this->cashier = $user;
$this->service = app(CashSessionService::class);
DB::beginTransaction();
// Start from a clean drawer for this cashier, inside the transaction —
// the restored tenant carries the very sessions that caused the bug.
CashSession::withoutBranchScope()
->where('user_id', $this->cashier->id)
->where('status', 'open')
->update(['status' => 'closed', 'closed_at' => now()]);
}
protected function tearDown(): void
{
DB::rollBack();
parent::tearDown();
}
/** Put the request in a branch the way the topbar switcher does. */
private function inBranch(int $branchId): void
{
session([BranchContext::KEY => $branchId]);
app()->forgetInstance(BranchContext::class);
app(BranchScopeState::class)->activate($branchId);
}
public function test_a_drawer_open_in_another_branch_does_not_block_this_one(): void
{
$this->actingAs($this->cashier);
$this->inBranch($this->branchA);
$first = $this->service->open($this->cashier, $this->branchA, 10000);
$this->inBranch($this->branchB);
$second = $this->service->open($this->cashier, $this->branchB, 5000);
$this->assertNotSame($first->id, $second->id);
$this->assertSame($this->branchA, (int) $first->branch_id);
$this->assertSame($this->branchB, (int) $second->branch_id);
$open = CashSession::withoutBranchScope()
->where('user_id', $this->cashier->id)
->where('status', 'open')
->pluck('branch_id')
->map(fn ($id) => (int) $id)
->all();
sort($open);
$this->assertSame([$this->branchA, $this->branchB], $open);
}
public function test_a_second_drawer_in_the_same_branch_is_still_refused(): void
{
$this->actingAs($this->cashier);
$this->inBranch($this->branchA);
$this->service->open($this->cashier, $this->branchA, 10000);
$this->expectException(DomainException::class);
$this->service->open($this->cashier, $this->branchA, 2000);
}
public function test_each_branch_resolves_its_own_drawer(): void
{
$this->actingAs($this->cashier);
$this->inBranch($this->branchA);
$atA = $this->service->open($this->cashier, $this->branchA, 10000);
// Standing in B, the drawer in A is not this till's drawer...
$this->inBranch($this->branchB);
$this->assertNull($this->service->getOpenSession($this->cashier, $this->branchB));
// ...but it is still findable by name, from anywhere. This is the
// lookup a refund uses to put cash back in the branch it came from.
$this->assertSame(
$atA->id,
$this->service->getOpenSession($this->cashier, $this->branchA)?->id
);
$elsewhere = $this->service->getOpenSessionsInOtherBranches($this->cashier, $this->branchB);
$this->assertCount(1, $elsewhere);
$this->assertSame($this->branchA, (int) $elsewhere->first()->branch_id);
}
public function test_the_till_and_the_shift_screen_agree_and_the_shift_opens(): void
{
$this->actingAs($this->cashier);
$this->inBranch($this->branchA);
$this->service->open($this->cashier, $this->branchA, 10000);
// At branch B: the POS says there is no shift here, the manage screen
// offers the open form rather than a close form, and opening works.
// Before the fix the first two held and the third threw.
$this->inBranch($this->branchB);
$manage = \Livewire\Livewire::test(\App\Livewire\CashSessions\CashSessionManage::class);
$manage->assertSet('currentSession', null);
$this->assertNotEmpty($manage->get('otherBranchSessions'));
$manage->set('opening_balance', 50)
->call('openSession')
->assertSet('currentSession.branch_id', $this->branchB);
$terminal = \Livewire\Livewire::test(\App\Livewire\POS\POSTerminal::class);
$terminal->assertSet('hasOpenSession', true);
}
public function test_cash_counts_into_the_drawer_of_the_branch_that_took_it(): void
{
$this->actingAs($this->cashier);
$this->inBranch($this->branchA);
$atA = $this->service->open($this->cashier, $this->branchA, 0);
$this->inBranch($this->branchB);
$atB = $this->service->open($this->cashier, $this->branchB, 0);
// The listener runs on a queue, where branch enforcement is off — the
// condition that let it credit whichever drawer sorted first.
app(BranchScopeState::class)->deactivate();
$payment = Payment::create([
'uuid' => (string) Str::uuid(),
'academy_id' => $this->cashier->academy_id,
'branch_id' => $this->branchB,
'direction' => 'inbound',
'method' => 'cash',
'status' => 'confirmed',
'amount' => 7500,
'payment_date' => now()->toDateString(),
'received_by' => $this->cashier->id,
]);
app(UpdateCashSessionTotals::class)->handle(
new PaymentReceived($payment, $this->cashier)
);
$this->assertSame(7500, (int) $atB->fresh()->total_cash_in);
$this->assertSame(0, (int) $atA->fresh()->total_cash_in);
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment