diff --git a/docs/modules.md b/docs/modules.md index 2a8433f..7d05580 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -49,7 +49,8 @@ boboko-test/ app/ Models/ Customer.php ← app-level model, extends Modules\Core\Customer\Models\Customer - User.php ← app-level model, dispatches Modules\Core\Auth\Events\UserCreated + User.php ← app-level model, no $dispatchesEvents needed — core dispatches + UserCreated itself (Modules\Core\Auth\Services\UserOtpService) Staff.php ← app-level model, extends Modules\Core\Auth\Models\Staff Lunar/ Extensions/ ← app's own Filament resource extensions (source of truth, wired in PanelServiceProvider) @@ -264,7 +265,14 @@ php artisan vendor:publish --tag=core-config 'auto_create_customer_for_user' => false, ``` -Both listeners guard against the other direction re-triggering: they call `User::withoutEvents(...)` around `firstOrCreate`/save, so pairing a `Customer` never spuriously fires `UserCreated` (and vice versa) even if both directions are somehow active at once. +A guard against the other direction re-triggering is only needed where a real risk exists: +`App\Listeners\CreateUserForCustomerListener` (`boboko-test`, app-level) wraps its +`firstOrCreate` in `User::withoutEvents(...)`, since finding-or-creating a `User` there could +itself fire `UserCreated` and loop back into `CreateCustomerForUser`. `Modules\Core\Customer\ +Listeners\CreateCustomerForUser` (core) needs no such guard — it calls a plain +`$model::create([])` on `Customer`, which has no `$dispatchesEvents`/model hooks of its own in +core that could re-trigger anything; the guard belongs only on the side that actually creates a +`User`. --- diff --git a/src/Auth/Services/UserOtpService.php b/src/Auth/Services/UserOtpService.php index 0b535b1..393d6b6 100644 --- a/src/Auth/Services/UserOtpService.php +++ b/src/Auth/Services/UserOtpService.php @@ -10,6 +10,7 @@ use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Mail; use Illuminate\Support\Facades\RateLimiter; use Modules\Core\Auth\Events\UserAuthenticated; +use Modules\Core\Auth\Events\UserCreated; use Modules\Core\Auth\Exceptions\OtpThrottledException; use Modules\Core\Auth\Mail\UserOtpMail; @@ -75,6 +76,18 @@ class UserOtpService $model = config('auth.providers.users.model'); $user = $model::firstOrCreate(['email' => $email]); + // wasRecentlyCreated is Eloquent's own "did firstOrCreate() just + // INSERT, or did it find an existing row" flag — the only reliable + // way to tell them apart from firstOrCreate()'s return value alone. + // Without this check, a genuinely new signup never fired + // UserCreated at all (this class's own docblock claimed the + // Customer/User pairing cascade "already triggers" here, which was + // false as written — see Modules\Core\Customer\Listeners\ + // CreateCustomerForUser, which depends entirely on this event). + if ($user->wasRecentlyCreated) { + Event::dispatch(new UserCreated($user)); + } + $code = str_pad((string) random_int(0, 999999), self::CODE_LENGTH, '0', STR_PAD_LEFT); $user->otp_code = $code; diff --git a/src/Catalog/Listeners/ReindexProductsRecommendingProduct.php b/src/Catalog/Listeners/ReindexProductsRecommendingProduct.php index 957cf2d..5358204 100644 --- a/src/Catalog/Listeners/ReindexProductsRecommendingProduct.php +++ b/src/Catalog/Listeners/ReindexProductsRecommendingProduct.php @@ -2,11 +2,17 @@ namespace Modules\Core\Catalog\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Lunar\Models\Product; use Modules\Core\Catalog\Events\ProductDeleted; use Modules\Core\Catalog\Events\ProductSaved; /** + * Queued — a Meilisearch filter query plus N reindex calls with no + * same-request reader; a few seconds of stale `recommendations` on a + * referencing product's storefront page is a cosmetic, not correctness, + * concern (see the class's own docblock below). + * * Keeps every product's embedded `recommendations` field (see * ProductIndexer) in sync when a product they recommend changes or is * removed. Unlike Modules\Core\Catalog\Observers\ProductOptionReindexObserver's @@ -27,7 +33,7 @@ use Modules\Core\Catalog\Events\ProductSaved; * SCOUT_QUEUE is configured) reindex job per matched product — this * listener itself does no synchronous Meilisearch writing. */ -class ReindexProductsRecommendingProduct +class ReindexProductsRecommendingProduct implements ShouldQueue { public function handleSaved(ProductSaved $event): void { diff --git a/src/Catalog/Services/StockService.php b/src/Catalog/Services/StockService.php new file mode 100644 index 0000000..edac9d3 --- /dev/null +++ b/src/Catalog/Services/StockService.php @@ -0,0 +1,71 @@ + Modules\Core\Order\Services\TransactionRecorder). + * + * Only decrements for `purchasable === 'in_stock'` variants — 'always' and + * 'backorder' variants are deliberately allowed to sell past (or without + * regard to) their stock count already (see ProductVariant:: + * canBeFulfilledAtQuantity()), so decrementing their stock would just make + * that column an inaccurate, decreasingly-negative number with no purchasing + * consequence. Only `OrderLine::type === 'physical'` lines are considered — + * a digital line has no stock to decrement (ProductVariant::getType()). + * + * A single UPDATE per variant (`DB::table(...)->update()` with a raw + * expression), not a read-then-write on the Eloquent model — avoids a + * lost-update race between two orders decrementing the same variant + * concurrently, and skips Modules\Core\Catalog\Services\ProductIndexer:: + * stock's staleness gap for the DB value itself even though the search + * index still only refreshes on the next reindex event/nightly job (see + * that class's own docblock). + * + * Never lets stock go negative (`GREATEST(stock - qty, 0)` via a raw + * expression) — an order can still be placed against a variant whose stock + * was already fully consumed by another concurrent order (Lunar has no + * stock-reservation step at cart/checkout time), so this is a best-effort + * count, not a hard inventory guarantee. + */ +class StockService +{ + public function decrementForOrder(Order $order): void + { + $lines = $order->lines() + ->where('type', 'physical') + ->where('purchasable_type', ProductVariant::morphName()) + ->get(['purchasable_id', 'quantity']); + + if ($lines->isEmpty()) { + return; + } + + foreach ($lines as $line) { + DB::table((new ProductVariant())->getTable()) + ->where('id', $line->purchasable_id) + ->where('purchasable', 'in_stock') + ->update([ + 'stock' => DB::raw('GREATEST(stock - '.(int) $line->quantity.', 0)'), + ]); + } + + $productIds = ProductVariant::whereIn('id', $lines->pluck('purchasable_id')) + ->pluck('product_id') + ->unique(); + + Product::whereIn('id', $productIds)->get()->each->searchable(); + } +} diff --git a/src/Customer/Listeners/CreateCustomerForUser.php b/src/Customer/Listeners/CreateCustomerForUser.php index 64adf0d..46b6338 100644 --- a/src/Customer/Listeners/CreateCustomerForUser.php +++ b/src/Customer/Listeners/CreateCustomerForUser.php @@ -6,6 +6,19 @@ use Lunar\Facades\ModelManifest; use Lunar\Models\Contracts\Customer as CustomerContract; use Modules\Core\Auth\Events\UserCreated; +/** + * Deliberately NOT queued, even though UserCreated (requesting an OTP + * code) and the login that follows it (submitting the code) are normally + * separate requests with a real time gap between them — that gap is not + * a guarantee this code controls. A busy/backed-up queue (a deploy in + * progress, a crashed worker, a traffic spike) could make this job run + * AFTER the shopper has already logged in and something has read + * $user->latestCustomer() (Modules\Core\Customer\Services\ + * CustomerAccountService), silently returning null for a legitimately + * paired user with no retry anywhere to catch it. Kept synchronous so the + * Customer always exists by the time UserCreated's dispatch call returns, + * regardless of queue health. + */ class CreateCustomerForUser { public function handle(UserCreated $event): void diff --git a/src/Customer/Listeners/LogCustomerAccountActivity.php b/src/Customer/Listeners/LogCustomerAccountActivity.php index 1a7e4d0..68da2fc 100644 --- a/src/Customer/Listeners/LogCustomerAccountActivity.php +++ b/src/Customer/Listeners/LogCustomerAccountActivity.php @@ -2,6 +2,7 @@ namespace Modules\Core\Customer\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Lunar\Models\Address; use Modules\Core\Customer\Events\CustomerAddressCreated; use Modules\Core\Customer\Events\CustomerAddressDeleted; @@ -18,8 +19,11 @@ use Modules\Core\Logging\ActivityLogService; * passed through explicitly on every call, since these events are * `web`-guard-caused, not `staff`-guard — see ActivityLogService's own * docblock for why that parameter exists. + * + * Queued — a pure audit-log write with no same-request reader; the + * shopper's own request doesn't need this to complete before responding. */ -class LogCustomerAccountActivity +class LogCustomerAccountActivity implements ShouldQueue { public function __construct( private readonly ActivityLogService $activityLog, diff --git a/src/Localization/Listeners/FlushLanguageCache.php b/src/Localization/Listeners/FlushLanguageCache.php index e79016c..8b01dc2 100644 --- a/src/Localization/Listeners/FlushLanguageCache.php +++ b/src/Localization/Listeners/FlushLanguageCache.php @@ -2,12 +2,21 @@ namespace Modules\Core\Localization\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Modules\Core\Localization\Events\LanguageCreated; use Modules\Core\Localization\Events\LanguageDeleted; use Modules\Core\Localization\Events\LanguageUpdated; use Modules\Core\Localization\Services\LanguageCache; -class FlushLanguageCache +/** + * Queued — the only reader of this cache is Modules\Core\Localization\ + * Middleware\LocaleMiddleware on a LATER storefront request, never the + * same admin request that edited/created/deleted the Language row (that + * request redirects to a fresh page read straight from the DB, not this + * cache). A few seconds of eventual consistency before the queue worker + * picks this up is an acceptable trade for not blocking the admin save. + */ +class FlushLanguageCache implements ShouldQueue { public function __construct(private readonly LanguageCache $languages) {} diff --git a/src/Localization/Listeners/FlushTranslationCache.php b/src/Localization/Listeners/FlushTranslationCache.php index 4b9d6ca..0ed81f0 100644 --- a/src/Localization/Listeners/FlushTranslationCache.php +++ b/src/Localization/Listeners/FlushTranslationCache.php @@ -2,6 +2,7 @@ namespace Modules\Core\Localization\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Support\Facades\Cache; use Modules\Core\Localization\Events\TranslationCreated; use Modules\Core\Localization\Events\TranslationDeleted; @@ -15,8 +16,13 @@ use Spatie\TranslationLoader\LanguageLine; * `group`/`key` (the old group's cached array never gets told a row left it). * This listener flushes every group+locale combination touched by either the * old or new state so nothing can remain stale. + * + * Queued — this cache backs `__('storefront.*')` lookups on a LATER + * storefront request, never the same admin request that just edited the + * translation (Filament redirects to a fresh index read straight from the + * DB, not this cache). Safe to let a queue worker pick up. */ -class FlushTranslationCache +class FlushTranslationCache implements ShouldQueue { public function handle(TranslationCreated|TranslationUpdated|TranslationDeleted $event): void { diff --git a/src/Localization/Listeners/LogTranslationActivity.php b/src/Localization/Listeners/LogTranslationActivity.php index 3a83b7b..31129b4 100644 --- a/src/Localization/Listeners/LogTranslationActivity.php +++ b/src/Localization/Listeners/LogTranslationActivity.php @@ -2,6 +2,7 @@ namespace Modules\Core\Localization\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Support\Arr; use Modules\Core\Localization\Events\TranslationCreated; use Modules\Core\Localization\Events\TranslationDeleted; @@ -9,7 +10,12 @@ use Modules\Core\Localization\Events\TranslationUpdated; use Modules\Core\Logging\ActivityLogService; use Spatie\TranslationLoader\LanguageLine; -class LogTranslationActivity +/** + * Queued — a pure audit-log write with no same-request reader (Filament + * redirects to a fresh index page after save, which doesn't read the + * activity log at all). + */ +class LogTranslationActivity implements ShouldQueue { public function __construct( private readonly ActivityLogService $activityLog, diff --git a/src/Localization/Listeners/MigrateTranslationsForRenamedLanguage.php b/src/Localization/Listeners/MigrateTranslationsForRenamedLanguage.php index 046fd23..ca90713 100644 --- a/src/Localization/Listeners/MigrateTranslationsForRenamedLanguage.php +++ b/src/Localization/Listeners/MigrateTranslationsForRenamedLanguage.php @@ -2,6 +2,7 @@ namespace Modules\Core\Localization\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Support\Facades\Cache; use Modules\Core\Localization\Events\LanguageUpdated; use Spatie\TranslationLoader\LanguageLine; @@ -12,8 +13,15 @@ use Spatie\TranslationLoader\LanguageLine; * getTranslationsForGroup($newCode, ...) would silently return nothing for * that locale even though the translated content still exists. Move the * text.{oldCode} key to text.{newCode} on every affected row instead. + * + * Queued — this walks every LanguageLine row containing the old locale key + * with no upper bound, and nothing in the same request needs the migration + * to have completed before responding (a rename is a rare admin action; + * the affected storefront locale is briefly unavailable until the queue + * worker finishes, the same window that already exists before this + * listener runs at all). */ -class MigrateTranslationsForRenamedLanguage +class MigrateTranslationsForRenamedLanguage implements ShouldQueue { public function handle(LanguageUpdated $event): void { diff --git a/src/Order/Listeners/AdvanceFulfillmentOnCarrierCheckpoint.php b/src/Order/Listeners/AdvanceFulfillmentOnCarrierCheckpoint.php index e5451bc..641a481 100644 --- a/src/Order/Listeners/AdvanceFulfillmentOnCarrierCheckpoint.php +++ b/src/Order/Listeners/AdvanceFulfillmentOnCarrierCheckpoint.php @@ -2,12 +2,19 @@ namespace Modules\Core\Order\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Modules\Core\Order\Events\OrderDispatched; +use Modules\Core\Order\Services\OrderStatusFlow; use Modules\Core\Order\Services\OrderStatusWriter; use Modules\Core\Shipping\Enums\TrackingStatus; use Modules\Core\Shipping\Events\ShipmentStatusUpdatedByCarrier; /** + * Queued — see Modules\Core\Order\Listeners\DeriveOrderDeliveredFromShipment's + * own docblock: ShipmentStatusUpdatedByCarrier comes from a scheduled + * polling job, not a webhook, so nothing needs this to complete before a + * request returns. + * * The automatic half of "Dispatched" — the manual fallback is the staff * "Update Status" action (Modules\Core\Shipping\Extensions\ * OrderViewExtension). Listens to ShipmentStatusUpdatedByCarrier directly, @@ -19,14 +26,17 @@ use Modules\Core\Shipping\Events\ShipmentStatusUpdatedByCarrier; * carrier that skips straight there without a distinct collection * checkpoint. * - * Guarded to only fire from 'ready_for_dispatch' — a late/duplicate - * checkpoint, or an order the manual action already advanced, is a - * silent no-op. + * Guarded by OrderStatusFlow::isValidTransition() rather than a hardcoded + * "only fire from 'ready_for_dispatch'" comparison — the single source of + * truth for the status graph lives there, not duplicated here. A + * late/duplicate checkpoint, or an order the manual action already + * advanced, is a silent no-op either way. */ -class AdvanceFulfillmentOnCarrierCheckpoint +class AdvanceFulfillmentOnCarrierCheckpoint implements ShouldQueue { public function __construct( private readonly OrderStatusWriter $writer, + private readonly OrderStatusFlow $flow, ) {} public function handle(ShipmentStatusUpdatedByCarrier $event): void @@ -38,7 +48,7 @@ class AdvanceFulfillmentOnCarrierCheckpoint $order = $event->shipmentInfo->shipment->order; - if (! $order || $order->status !== 'ready_for_dispatch') { + if (! $order || ! $this->flow->isValidTransition($order, 'dispatched')) { return; } diff --git a/src/Order/Listeners/AdvanceFulfillmentOnDelivered.php b/src/Order/Listeners/AdvanceFulfillmentOnDelivered.php index 1ca6e9b..cebfb52 100644 --- a/src/Order/Listeners/AdvanceFulfillmentOnDelivered.php +++ b/src/Order/Listeners/AdvanceFulfillmentOnDelivered.php @@ -2,10 +2,18 @@ namespace Modules\Core\Order\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Modules\Core\Order\Events\OrderDelivered; +use Modules\Core\Order\Services\OrderStatusFlow; use Modules\Core\Order\Services\OrderStatusWriter; /** + * Queued — OrderDelivered is only ever dispatched from Modules\Core\Order\ + * Listeners\DeriveOrderDeliveredFromShipment, itself queued (see that + * class's own docblock: the triggering ShipmentStatusUpdatedByCarrier + * comes from a scheduled polling job, not a request with a page waiting + * on the result). + * * Writes `status` to 'delivered' once a carrier confirms delivery, rather * than jumping straight to 'completed'. Carrier orders get a return * window between delivery and completion (see Modules\Core\Order\ @@ -20,21 +28,23 @@ use Modules\Core\Order\Services\OrderStatusWriter; * OrderDelivered — deriving "was this delivered" and acting on it by * writing `status` are deliberately two different listeners. * - * Guarded to only fire from 'dispatched' — a duplicate/late Delivered + * Guarded by OrderStatusFlow::isValidTransition() rather than a hardcoded + * "only fire from 'dispatched'" comparison. A duplicate/late Delivered * checkpoint, or an order a manual action already moved past, is a - * silent no-op. + * silent no-op either way. */ -class AdvanceFulfillmentOnDelivered +class AdvanceFulfillmentOnDelivered implements ShouldQueue { public function __construct( private readonly OrderStatusWriter $writer, + private readonly OrderStatusFlow $flow, ) {} public function handle(OrderDelivered $event): void { $order = $event->order; - if ($order->status !== 'dispatched') { + if (! $this->flow->isValidTransition($order, 'delivered')) { return; } diff --git a/src/Order/Listeners/ApplyResolvedPaymentStatus.php b/src/Order/Listeners/ApplyResolvedPaymentStatus.php index 28d6524..9683e54 100644 --- a/src/Order/Listeners/ApplyResolvedPaymentStatus.php +++ b/src/Order/Listeners/ApplyResolvedPaymentStatus.php @@ -2,13 +2,8 @@ namespace Modules\Core\Order\Listeners; -use Illuminate\Support\Facades\Event; use Lunar\Models\Order; -use Modules\Core\Checkout\Events\OrderPlaced; -use Modules\Core\Order\Enums\PaymentStatus; -use Modules\Core\Order\Services\OrderStatusFlow; -use Modules\Core\Order\Services\OrderStatusWriter; -use Modules\Core\Order\Support\OrderStatus; +use Modules\Core\Order\Services\OrderPaymentResolutionService; use Modules\Core\Payment\Events\PaymentAuthorized; use Modules\Core\Payment\Events\PaymentCaptured; use Modules\Core\Payment\Events\PaymentRefunded; @@ -17,38 +12,24 @@ use Modules\Core\Payment\Events\PaymentRefunded; * Registered against PaymentCaptured, PaymentAuthorized, AND * PaymentRefunded (see OrderServiceProvider). * - * PaymentCaptured writes both Order::paid/paid_at (via - * OrderStatusWriter::markPaid()) AND advances `status` out of - * 'awaiting_payment' to the next step in the order's flow (see - * OrderStatusFlow::nextOptions()) — re-confirmed with the user: a - * captured payment, manual or via Stripe's webhook, should never leave an - * order sitting at 'awaiting_payment'. Only fires when status is still - * exactly 'awaiting_payment', so a duplicate/delayed capture event never - * regresses an order staff already advanced further. PaymentAuthorized - * only marks paid — an authorization is not yet captured funds, so - * status stays put until the actual capture. - * - * A refund still moves `status` (returned -> refunded/partially_refunded) - * — refunds are a normal step in Modules\Core\Order\Services\ - * OrderStatusFlow's own sequence, unlike captures. Derives - * Refunded/PartialRefund from Modules\Core\Order\Support\OrderStatus:: - * payment() — the existing, unchanged derived-enum logic, reused rather - * than reimplemented. - * - * Reads $event->context['order_id'] to find which Order this outcome - * belongs to — Payment has no concept of an Order. - * - * Dispatches Checkout\Events\OrderPlaced itself, once placed_at is set. - * Never fires from the PaymentRefunded path — a refund can only ever - * happen after an order was already placed. + * A thin reactor — resolves which Order this outcome belongs to (Payment + * has no concept of an Order, so this reads $event->context['order_id']) + * and hands off to Modules\Core\Order\Services\ + * OrderPaymentResolutionService for the actual decisions: whether to mark + * the order paid, whether/how far to advance `status`, and what a refund + * does to it. See that service's own docblock, and its methods' own + * docblocks, for the full business reasoning (re-confirmed with the + * user): a captured payment, manual or via Stripe's webhook, should + * never leave an order sitting at 'awaiting_payment'; an authorization + * only marks paid, since it isn't yet captured funds; a refund is a + * normal step in the order's own status sequence, unlike a capture. * * Deliberately does NOT react to PaymentVoided. */ class ApplyResolvedPaymentStatus { public function __construct( - private readonly OrderStatusWriter $writer, - private readonly OrderStatusFlow $flow, + private readonly OrderPaymentResolutionService $resolution, ) {} public function handle(PaymentCaptured|PaymentAuthorized|PaymentRefunded $event): void @@ -62,58 +43,11 @@ class ApplyResolvedPaymentStatus $order = Order::findOrFail($orderId); if ($event instanceof PaymentRefunded) { - $this->applyRefund($order, $event); + $this->resolution->resolveRefund($order, $event::class); return; } - $wasPlaced = ! blank($order->placed_at); - - $this->writer->markPaid($order, $event::class); - - if ($event instanceof PaymentCaptured) { - $this->advancePastAwaitingPayment($order, $event); - } - - if (! $wasPlaced) { - $order->update(['placed_at' => $order->placed_at ?? now()]); - Event::dispatch(new OrderPlaced($order)); - } - } - - private function advancePastAwaitingPayment(Order $order, PaymentCaptured $event): void - { - if ($order->status !== 'awaiting_payment') { - return; - } - - $next = $this->flow->nextOptions($order); - $target = array_key_first($next); - - if ($target !== null) { - $this->writer->write($order, $target, $event::class); - } - } - - /** - * Requires the refund Transaction row to already exist (Modules\Core\ - * Order\Listeners\RecordPaymentTransaction must run first — see - * OrderServiceProvider's listener registration order for - * PaymentRefunded), so the relation is refreshed here rather than - * trusted from a possibly-stale $order instance. - */ - private function applyRefund(Order $order, PaymentRefunded $event): void - { - $order->load('transactions'); - - $target = match (OrderStatus::payment($order)) { - PaymentStatus::Refunded => 'refunded', - PaymentStatus::PartialRefund => 'partially_refunded', - default => null, - }; - - if ($target !== null && $order->status !== $target) { - $this->writer->write($order, $target, $event::class); - } + $this->resolution->resolveCaptureOrAuthorization($order, $event::class, isCapture: $event instanceof PaymentCaptured); } } diff --git a/src/Order/Listeners/CompleteOrderOnPickedUp.php b/src/Order/Listeners/CompleteOrderOnPickedUp.php index 581a9e3..911d3c9 100644 --- a/src/Order/Listeners/CompleteOrderOnPickedUp.php +++ b/src/Order/Listeners/CompleteOrderOnPickedUp.php @@ -4,9 +4,19 @@ namespace Modules\Core\Order\Listeners; use Modules\Core\Order\Events\OrderCompleted; use Modules\Core\Order\Events\OrderPickedUp; +use Modules\Core\Order\Services\OrderStatusFlow; use Modules\Core\Order\Services\OrderStatusWriter; /** + * Deliberately NOT queued — OrderPickedUp is dispatched from a staff + * Filament action (see OrderFulfillmentService::markPickedUp()), and the + * page staff are looking at needs to show `status` as 'completed' + * immediately after they click, not still 'picked_up' until a queue + * worker catches up. Unlike ShipmentStatusUpdatedByCarrier's listeners + * (queued — dispatched from a scheduled polling job with no page waiting + * on the result), this one has a real same-request/same-page-load + * dependency. + * * The store-pickup mirror of AdvanceFulfillmentOnDelivered — reacts to * OrderPickedUp (dispatched by Modules\Core\Order\Services\ * OrderFulfillmentService::markPickedUp() the moment staff confirm the @@ -15,20 +25,22 @@ use Modules\Core\Order\Services\OrderStatusWriter; * business design — unlike the carrier branch, there is no 'delivered' * intermediate value on this path. * - * Guarded to only fire from 'picked_up' — a duplicate dispatch (e.g. a - * stale page re-submitting the action) is a silent no-op. + * Guarded by OrderStatusFlow::isValidTransition() rather than a hardcoded + * "only fire from 'picked_up'" comparison. A duplicate dispatch (e.g. a + * stale page re-submitting the action) is a silent no-op either way. */ class CompleteOrderOnPickedUp { public function __construct( private readonly OrderStatusWriter $writer, + private readonly OrderStatusFlow $flow, ) {} public function handle(OrderPickedUp $event): void { $order = $event->order; - if ($order->status !== 'picked_up') { + if (! $this->flow->isValidTransition($order, 'completed')) { return; } diff --git a/src/Order/Listeners/DecrementStockOnOrderPlaced.php b/src/Order/Listeners/DecrementStockOnOrderPlaced.php index 67e497f..6429200 100644 --- a/src/Order/Listeners/DecrementStockOnOrderPlaced.php +++ b/src/Order/Listeners/DecrementStockOnOrderPlaced.php @@ -2,63 +2,38 @@ namespace Modules\Core\Order\Listeners; -use Illuminate\Support\Facades\DB; -use Lunar\Models\Product; -use Lunar\Models\ProductVariant; +use Modules\Core\Catalog\Services\StockService; use Modules\Core\Checkout\Events\OrderPlaced; /** - * The only place ProductVariant::stock is written as a result of an order — - * fires once per order regardless of capture_mode/driver, same reasoning as - * Modules\Core\Order\Notifications\OrderPlacedNotification: OrderPlaced is - * dispatched exactly once, from the one place an order's placed_at - * actually gets set (Modules\Core\Order\Listeners\ApplyResolvedPaymentStatus), - * so this can't double-decrement across a capture/authorize/refund sequence - * the way listening to PaymentCaptured directly could. + * Deliberately NOT queued — unlike this codebase's other queued side + * effects (cache flushes, audit logs, search reindexes), a stalled queue + * here isn't just cosmetic staleness: it widens the window in which + * another order can be accepted against stock this order already + * committed (Lunar has no stock-reservation step at checkout time to + * begin with — see StockService's own "Never lets stock go negative" + * note — so some oversell race already exists, but a queue stall of + * minutes/hours extends that window far past the sub-millisecond one a + * synchronous write leaves open). StockService's atomic `GREATEST(stock - + * qty, 0)` SQL still protects against a LOST update between two orders + * decrementing the same variant concurrently; running it synchronously + * keeps the exposure window as small as possible on top of that. * - * Only decrements for `purchasable === 'in_stock'` variants — 'always' and - * 'backorder' variants are deliberately allowed to sell past (or without - * regard to) their stock count already (see ProductVariant:: - * canBeFulfilledAtQuantity()), so decrementing their stock would just make - * that column an inaccurate, decreasingly-negative number with no purchasing - * consequence. Only `OrderLine::type === 'physical'` lines are considered — - * a digital line has no stock to decrement (ProductVariant::getType()). - * - * A single UPDATE per variant (`DB::table(...)->decrement()`), not a - * read-then-write on the Eloquent model — avoids a lost-update race between - * two orders decrementing the same variant concurrently, and skips - * Modules\Core\Catalog\Services\ProductIndexer::stock's staleness gap for - * the DB value itself even though the search index still only refreshes on - * the next reindex event/nightly job (see that class's own docblock). - * - * Never lets stock go negative (`GREATEST(stock - qty, 0)` via a raw - * expression) — an order can still be placed against a variant whose stock - * was already fully consumed by another concurrent order (Lunar has no - * stock-reservation step at cart/checkout time), so this is a best-effort - * count, not a hard inventory guarantee. + * The actual decrement logic lives in Modules\Core\Catalog\Services\ + * StockService — stock (the column, its invariants) is a Catalog concern, + * not an Order one; this listener is just the "an order was placed" + * trigger. Fires once per order regardless of capture_mode/driver, same + * reasoning as Modules\Core\Order\Notifications\OrderPlacedNotification: + * OrderPlaced is dispatched exactly once, from the one place an order's + * placed_at actually gets set (Modules\Core\Order\Listeners\ + * ApplyResolvedPaymentStatus), so this can't double-decrement across a + * capture/authorize/refund sequence the way listening to PaymentCaptured + * directly could. */ class DecrementStockOnOrderPlaced { public function handle(OrderPlaced $event): void { - $lines = $event->order->lines() - ->where('type', 'physical') - ->where('purchasable_type', ProductVariant::morphName()) - ->get(['purchasable_id', 'quantity']); - - foreach ($lines as $line) { - DB::table((new ProductVariant())->getTable()) - ->where('id', $line->purchasable_id) - ->where('purchasable', 'in_stock') - ->update([ - 'stock' => DB::raw('GREATEST(stock - '.(int) $line->quantity.', 0)'), - ]); - } - - $productIds = ProductVariant::whereIn('id', $lines->pluck('purchasable_id')) - ->pluck('product_id') - ->unique(); - - Product::whereIn('id', $productIds)->get()->each->searchable(); + app(StockService::class)->decrementForOrder($event->order); } } diff --git a/src/Order/Listeners/DeriveOrderDeliveredFromShipment.php b/src/Order/Listeners/DeriveOrderDeliveredFromShipment.php index 582431f..d7d5672 100644 --- a/src/Order/Listeners/DeriveOrderDeliveredFromShipment.php +++ b/src/Order/Listeners/DeriveOrderDeliveredFromShipment.php @@ -2,17 +2,23 @@ namespace Modules\Core\Order\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Modules\Core\Order\Events\OrderDelivered; use Modules\Core\Shipping\Enums\TrackingStatus; use Modules\Core\Shipping\Events\ShipmentStatusUpdatedByCarrier; /** + * Queued — ShipmentStatusUpdatedByCarrier is dispatched from + * Modules\Core\Shipping\Jobs\PollShipmentTrackingJob, a scheduled job with + * no HTTP request waiting on a response, so there is no same-request + * timing pressure for any of this event's listeners (unlike a webhook). + * * Translates a carrier tracking checkpoint into OrderDelivered — the event * OrderDeliveredNotification (via NotificationRegistry) actually listens * to. Kept separate from the notification itself so the "is this checkpoint * a delivery" filtering doesn't leak into notification code. */ -class DeriveOrderDeliveredFromShipment +class DeriveOrderDeliveredFromShipment implements ShouldQueue { public function handle(ShipmentStatusUpdatedByCarrier $event): void { diff --git a/src/Order/Listeners/MarkDeliveryFailedOnCarrierCheckpoint.php b/src/Order/Listeners/MarkDeliveryFailedOnCarrierCheckpoint.php index 96cab9f..460be03 100644 --- a/src/Order/Listeners/MarkDeliveryFailedOnCarrierCheckpoint.php +++ b/src/Order/Listeners/MarkDeliveryFailedOnCarrierCheckpoint.php @@ -2,20 +2,28 @@ namespace Modules\Core\Order\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; +use Modules\Core\Order\Services\OrderStatusFlow; use Modules\Core\Order\Services\OrderStatusWriter; use Modules\Core\Shipping\Enums\TrackingStatus; use Modules\Core\Shipping\Events\ShipmentStatusUpdatedByCarrier; /** + * Queued — see Modules\Core\Order\Listeners\DeriveOrderDeliveredFromShipment's + * own docblock: ShipmentStatusUpdatedByCarrier comes from a scheduled + * polling job, not a webhook. + * * Wires TrackingStatus::Failed to the 'delivery_failed' status for the - * first time — previously an unused enum case. Guarded to only fire from - * 'dispatched': a stale/duplicate checkpoint, or an order a manual action - * already moved past, is a silent no-op. + * first time — previously an unused enum case. Guarded by + * OrderStatusFlow::isValidTransition() rather than a hardcoded "only fire + * from 'dispatched'" comparison. A stale/duplicate checkpoint, or an + * order a manual action already moved past, is a silent no-op either way. */ -class MarkDeliveryFailedOnCarrierCheckpoint +class MarkDeliveryFailedOnCarrierCheckpoint implements ShouldQueue { public function __construct( private readonly OrderStatusWriter $writer, + private readonly OrderStatusFlow $flow, ) {} public function handle(ShipmentStatusUpdatedByCarrier $event): void @@ -26,7 +34,7 @@ class MarkDeliveryFailedOnCarrierCheckpoint $order = $event->shipmentInfo->shipment->order; - if (! $order || $order->status !== 'dispatched') { + if (! $order || ! $this->flow->isValidTransition($order, 'delivery_failed')) { return; } diff --git a/src/Order/Listeners/RecordPaymentTransaction.php b/src/Order/Listeners/RecordPaymentTransaction.php index ae88025..c75390d 100644 --- a/src/Order/Listeners/RecordPaymentTransaction.php +++ b/src/Order/Listeners/RecordPaymentTransaction.php @@ -10,6 +10,17 @@ use Modules\Core\Payment\Events\PaymentRefunded; use Modules\Core\Payment\Events\PaymentVoided; /** + * Deliberately NOT queued, despite looking like a pure audit-trail write + * with no same-request reader — Modules\Core\Providers\ + * OrderServiceProvider registers this to run BEFORE + * Modules\Core\Order\Listeners\ApplyResolvedPaymentStatus for + * PaymentRefunded specifically, because that listener's refund-status + * resolution reads the Transaction row this listener just wrote. Queueing + * this would run it asynchronously while ApplyResolvedPaymentStatus (sync) + * proceeds immediately, almost certainly executing before the queued job + * and silently breaking that read. See OrderServiceProvider's own + * registration-order comment. + * * Writes the Transaction row for a successful payment outcome — the * "record what happened" half of reacting to Payment's events, separate * from Modules\Core\Order\Listeners\ApplyResolvedPaymentStatus's "update diff --git a/src/Order/Listeners/RecordStatusTransition.php b/src/Order/Listeners/RecordStatusTransition.php index bb70d21..19bbc51 100644 --- a/src/Order/Listeners/RecordStatusTransition.php +++ b/src/Order/Listeners/RecordStatusTransition.php @@ -2,11 +2,16 @@ namespace Modules\Core\Order\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Modules\Core\Order\Events\OrderPaidChanged; use Modules\Core\Order\Events\OrderStatusChanged; use Modules\Core\Order\Services\OrderStatusTransitionRecorder; /** + * Queued — a pure history-log write with no same-request reader anywhere + * in the codebase (no Filament page renders order_status_transitions + * immediately after a status change; it's browsed later, if at all). + * * The one place order_status_transitions rows actually get written — * listens to OrderStatusChanged (every write of the single `status` * column, via Modules\Core\Order\Services\OrderStatusWriter::write()) and @@ -15,7 +20,7 @@ use Modules\Core\Order\Services\OrderStatusTransitionRecorder; * one consistent audit trail entry ('paid', with a null from_status) * rather than a second, separate table. */ -class RecordStatusTransition +class RecordStatusTransition implements ShouldQueue { public function __construct( private readonly OrderStatusTransitionRecorder $recorder, diff --git a/src/Order/Services/OrderPaymentResolutionService.php b/src/Order/Services/OrderPaymentResolutionService.php new file mode 100644 index 0000000..5efdb27 --- /dev/null +++ b/src/Order/Services/OrderPaymentResolutionService.php @@ -0,0 +1,90 @@ +context, load the Order, call this service, done. + * + * See ApplyResolvedPaymentStatus's own docblock for the full business + * reasoning (re-confirmed with the user) behind each rule enforced here — + * this class only re-documents what's specific to the decision logic + * itself, not the "why" already recorded there. + */ +class OrderPaymentResolutionService +{ + public function __construct( + private readonly OrderStatusWriter $writer, + private readonly OrderStatusFlow $flow, + ) {} + + /** + * A captured or authorized payment: marks the order paid (capture + * only — an authorization is not yet captured funds), advances status + * out of 'awaiting_payment' (capture only), and marks the order + * placed if this is the first payment outcome it's seen. + */ + public function resolveCaptureOrAuthorization(Order $order, string $causeClass, bool $isCapture): void + { + $wasPlaced = ! blank($order->placed_at); + + $this->writer->markPaid($order, $causeClass); + + if ($isCapture) { + $this->advancePastAwaitingPayment($order, $causeClass); + } + + if (! $wasPlaced) { + $order->update(['placed_at' => $order->placed_at ?? now()]); + Event::dispatch(new OrderPlaced($order)); + } + } + + /** + * Requires the refund Transaction row to already exist (Modules\Core\ + * Order\Listeners\RecordPaymentTransaction must run first — see + * OrderServiceProvider's listener registration order for + * PaymentRefunded), so the relation is refreshed here rather than + * trusted from a possibly-stale $order instance. + */ + public function resolveRefund(Order $order, string $causeClass): void + { + $order->load('transactions'); + + $target = match (OrderStatus::payment($order)) { + PaymentStatus::Refunded => 'refunded', + PaymentStatus::PartialRefund => 'partially_refunded', + default => null, + }; + + if ($target !== null && $order->status !== $target) { + $this->writer->write($order, $target, $causeClass); + } + } + + private function advancePastAwaitingPayment(Order $order, string $causeClass): void + { + if ($order->status !== 'awaiting_payment') { + return; + } + + $next = $this->flow->nextOptions($order); + $target = array_key_first($next); + + if ($target !== null) { + $this->writer->write($order, $target, $causeClass); + } + } +} diff --git a/src/Order/Services/OrderStatusFlow.php b/src/Order/Services/OrderStatusFlow.php index daa4ed5..763289c 100644 --- a/src/Order/Services/OrderStatusFlow.php +++ b/src/Order/Services/OrderStatusFlow.php @@ -133,6 +133,22 @@ class OrderStatusFlow return ! $order->paid && $this->isCod($order); } + /** + * Whether moving $order to $to is a valid transition from its CURRENT + * status — the single source of truth for "is this a legal next step," + * so a caller reacting to an external event (a carrier tracking + * checkpoint, a staff action) doesn't need to hardcode its own "only + * fire from status X" guard duplicating what nextOptions() already + * knows. See e.g. Modules\Core\Order\Listeners\ + * AdvanceFulfillmentOnCarrierCheckpoint, which used to compare + * $order->status to a literal 'ready_for_dispatch' inline instead of + * asking this class. + */ + public function isValidTransition(Order $order, string $to): bool + { + return array_key_exists($to, $this->nextOptions($order)); + } + private function label(string $status): string { return (string) str($status)->replace('_', ' ')->title(); diff --git a/src/Payment/Listeners/LogPaymentMethodActivity.php b/src/Payment/Listeners/LogPaymentMethodActivity.php index dea4871..ba34704 100644 --- a/src/Payment/Listeners/LogPaymentMethodActivity.php +++ b/src/Payment/Listeners/LogPaymentMethodActivity.php @@ -2,6 +2,7 @@ namespace Modules\Core\Payment\Listeners; +use Illuminate\Contracts\Queue\ShouldQueue; use Modules\Core\Logging\ActivityLogService; use Modules\Core\Payment\Events\PaymentMethodCreated; use Modules\Core\Payment\Events\PaymentMethodDeleted; @@ -9,6 +10,8 @@ use Modules\Core\Payment\Events\PaymentMethodUpdated; use Modules\Core\Payment\Models\PaymentMethod; /** + * Queued — a pure audit-log write with no same-request reader. + * * Same pattern as Localization\Listeners\LogTranslationActivity — routes * PaymentMethodService's own events through the existing * Logging\ActivityLogService instead of PaymentMethod separately opting @@ -29,7 +32,7 @@ use Modules\Core\Payment\Models\PaymentMethod; * forcing into a one-subject shape or adding a new method to the shared * service for. */ -class LogPaymentMethodActivity +class LogPaymentMethodActivity implements ShouldQueue { public function __construct( private readonly ActivityLogService $activityLog, diff --git a/src/Review/Events/ReviewReplied.php b/src/Review/Events/ReviewReplied.php new file mode 100644 index 0000000..5ec2099 --- /dev/null +++ b/src/Review/Events/ReviewReplied.php @@ -0,0 +1,26 @@ +fillForm(fn (ProductReview $record) => ['reply' => $record->reply]) ->action(function (ProductReview $record, array $data) { - $record->update([ - 'reply' => $data['reply'], - 'replied_at' => Carbon::now(), - ]); + app(ReviewService::class)->reply($record, $data['reply'], auth('staff')->user()); }), DeleteAction::make(), ]) diff --git a/src/Review/Services/ReviewService.php b/src/Review/Services/ReviewService.php new file mode 100644 index 0000000..49360c3 --- /dev/null +++ b/src/Review/Services/ReviewService.php @@ -0,0 +1,35 @@ +replied_at === null; + + $review->update([ + 'reply' => $reply, + 'replied_at' => now(), + ]); + + Event::dispatch(new ReviewReplied($review, $repliedBy, $wasReply)); + + return $review; + } +}