From a55697ce8207eb2c92f058b0e6a7436b5d4d2622 Mon Sep 17 00:00:00 2001 From: Konstantinos Arvanitakis Date: Wed, 16 Sep 2026 23:24:02 +0300 Subject: [PATCH 1/4] Feature: Updating Listreners, Separating Logic from listeners, Queuing Policies --- docs/modules.md | 12 ++- src/Auth/Services/UserOtpService.php | 13 +++ .../ReindexProductsRecommendingProduct.php | 8 +- src/Catalog/Services/StockService.php | 71 ++++++++++++++ .../Listeners/CreateCustomerForUser.php | 13 +++ .../Listeners/LogCustomerAccountActivity.php | 6 +- .../Listeners/FlushLanguageCache.php | 11 ++- .../Listeners/FlushTranslationCache.php | 8 +- .../Listeners/LogTranslationActivity.php | 8 +- .../MigrateTranslationsForRenamedLanguage.php | 10 +- .../AdvanceFulfillmentOnCarrierCheckpoint.php | 20 +++- .../AdvanceFulfillmentOnDelivered.php | 18 +++- .../Listeners/ApplyResolvedPaymentStatus.php | 96 +++---------------- .../Listeners/CompleteOrderOnPickedUp.php | 18 +++- .../Listeners/DecrementStockOnOrderPlaced.php | 73 +++++--------- .../DeriveOrderDeliveredFromShipment.php | 8 +- .../MarkDeliveryFailedOnCarrierCheckpoint.php | 18 +++- .../Listeners/RecordPaymentTransaction.php | 11 +++ .../Listeners/RecordStatusTransition.php | 7 +- .../OrderPaymentResolutionService.php | 90 +++++++++++++++++ src/Order/Services/OrderStatusFlow.php | 16 ++++ .../Listeners/LogPaymentMethodActivity.php | 5 +- src/Review/Events/ReviewReplied.php | 26 +++++ .../Filament/Pages/ManageProductReviews.php | 7 +- src/Review/Services/ReviewService.php | 35 +++++++ 25 files changed, 445 insertions(+), 163 deletions(-) create mode 100644 src/Catalog/Services/StockService.php create mode 100644 src/Order/Services/OrderPaymentResolutionService.php create mode 100644 src/Review/Events/ReviewReplied.php create mode 100644 src/Review/Services/ReviewService.php 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; + } +} From dcdc998eee0de65a7d52b7368c2419ff2b1a4c2c Mon Sep 17 00:00:00 2001 From: Konstantinos Arvanitakis Date: Fri, 18 Sep 2026 00:52:38 +0300 Subject: [PATCH 2/4] Feat: Updating Shipping Method with new variables, for correct box env vars --- config/shippingCarriers/boxnow.php | 14 +++++++ .../ShippingMethodListExtension.php | 16 ++++--- .../ShippingMethodResourceExtension.php | 22 ++++++++-- .../Filament/Pages/ManageShippingRates.php | 42 ++++++++++++++++++- 4 files changed, 83 insertions(+), 11 deletions(-) diff --git a/config/shippingCarriers/boxnow.php b/config/shippingCarriers/boxnow.php index 672b95f..9a011ad 100644 --- a/config/shippingCarriers/boxnow.php +++ b/config/shippingCarriers/boxnow.php @@ -13,12 +13,25 @@ | | Set these via environment variables — never commit real values. | +| Box Now has two environments (see their Partner API manual, section 2): +| Stage/Sandbox for testing, Production once live. Each has its own +| client_id/client_secret pair and its own base_url/location_api_url — +| there is no shared "switch an env var" flag, since stage credentials +| don't work against the production host or vice versa. +| | BOXNOW_BASE_URL Root REST endpoint for delivery-requests/parcels. | BOXNOW_LOCATION_API_URL Separate, faster endpoint for origins/destinations | lookups (Box Now recommends this over the main | base URL for those two calls specifically). | BOXNOW_CLIENT_ID OAuth2 client id. | BOXNOW_CLIENT_SECRET OAuth2 client secret. +| BOXNOW_PARTNER_ID Numeric partnerId Box Now issues alongside your +| credentials. NOT used for REST API authentication +| (BoxNowClient authenticates with client_id/ +| client_secret alone) — this is only consumed by +| the client-side Destination Map widget config +| (_bn_map_widget_config.partnerId), confirmed +| against Box Now's own WooCommerce plugin source. | BOXNOW_ORIGIN_LOCATION_ID Your warehouse's Box Now locationId, used as | the pickup origin on every delivery request. | BOXNOW_SENDER_* Static sender contact details reused on every @@ -33,6 +46,7 @@ return [ 'client_id' => env('BOXNOW_CLIENT_ID'), 'client_secret' => env('BOXNOW_CLIENT_SECRET'), + 'partner_id' => env('BOXNOW_PARTNER_ID'), 'origin_location_id' => env('BOXNOW_ORIGIN_LOCATION_ID'), diff --git a/src/Shipping/Extensions/ShippingMethodListExtension.php b/src/Shipping/Extensions/ShippingMethodListExtension.php index 3eb5b4b..61ad91c 100644 --- a/src/Shipping/Extensions/ShippingMethodListExtension.php +++ b/src/Shipping/Extensions/ShippingMethodListExtension.php @@ -12,11 +12,15 @@ use Lunar\Shipping\Filament\Resources\ShippingMethodResource; /** * ListShippingMethod::getDefaultHeaderActions() builds its CreateAction's - * form inline (calling ShippingMethodResource::getDriverFormComponent() - * directly, a hardcoded 2-option Select) rather than through the resource's - * own extendForm() pipeline, so ShippingMethodResourceExtension's driver - * fix never reaches it. Re-declares the same create-action form with a - * dynamic driver Select instead. + * form inline (calling ShippingMethodResource::getDriverFormComponent()/ + * getNameFormComponent() directly, hardcoded vendor components) rather + * than through the resource's own extendForm() pipeline, so neither + * ShippingMethodResourceExtension's driver Select nor its translated + * `name` field ever reached this action — `name`'s plain-string TextInput + * in particular used to insert a raw string into the now-JSON `name` + * column, crashing with a Postgres "invalid input syntax for type json" + * error on every create. Re-declares the same create-action form with + * both fixes reapplied instead. */ class ShippingMethodListExtension extends BaseExtension { @@ -25,7 +29,7 @@ class ShippingMethodListExtension extends BaseExtension foreach ($actions as $action) { if ($action instanceof CreateAction) { $action->schema([ - ShippingMethodResource::getNameFormComponent(), + ShippingMethodResourceExtension::translatedNameField(), Group::make([ ShippingMethodResource::getCodeFormComponent(), $this->driverSelect(), diff --git a/src/Shipping/Extensions/ShippingMethodResourceExtension.php b/src/Shipping/Extensions/ShippingMethodResourceExtension.php index 3506fa9..1da2c8a 100644 --- a/src/Shipping/Extensions/ShippingMethodResourceExtension.php +++ b/src/Shipping/Extensions/ShippingMethodResourceExtension.php @@ -43,7 +43,7 @@ class ShippingMethodResourceExtension extends ResourceExtension { return array_map(function (Component $component) { if (method_exists($component, 'getName') && $component->getName() === 'name') { - return $this->translatedNameField(); + return self::translatedNameField(); } if (in_array(HasChildComponents::class, class_uses_recursive($component), true)) { @@ -68,14 +68,28 @@ class ShippingMethodResourceExtension extends ResourceExtension * the model attribute is a string on the way in and out, only ever * an array while Filament's schema state holds it. */ - private function translatedNameField(): TranslatedText + /** + * Also called directly by Modules\Core\Shipping\Extensions\ + * ShippingMethodListExtension — the create action's form is built + * inline by the vendor's ListShippingMethod page rather than through + * this class's own extendForm() pipeline, so it needs the same + * translated field wired in separately. + */ + public static function translatedNameField(): TranslatedText { $field = TranslatedText::make('name') ->label('Name') ->required() ->afterStateHydrated(function (TranslatedText $component, $state) { - $decoded = json_decode((string) $state, true); - $component->state(is_array($decoded) ? $decoded : []); + // On create there is no record yet, so Filament hydrates + // this from the field's own default/current state — already + // an array (or null), never the raw JSON string edit gets + // from the model attribute. Only decode when it's a string. + if (is_string($state)) { + $state = json_decode($state, true); + } + + $component->state(is_array($state) ? $state : []); }) ->dehydrateStateUsing(fn ($state) => json_encode(is_array($state) ? $state : [])); diff --git a/src/Shipping/Filament/Pages/ManageShippingRates.php b/src/Shipping/Filament/Pages/ManageShippingRates.php index 000c2cf..b0fa633 100644 --- a/src/Shipping/Filament/Pages/ManageShippingRates.php +++ b/src/Shipping/Filament/Pages/ManageShippingRates.php @@ -4,6 +4,7 @@ namespace Modules\Core\Shipping\Filament\Pages; use Filament\Schemas\Schema; use Filament\Schemas\Components\Utilities\Get; +use Filament\Forms\Components\Select; use Filament\Forms\Components\TextInput; use Filament\Tables\Columns\TextColumn; use Filament\Tables\Table; @@ -11,6 +12,7 @@ use Illuminate\Database\Eloquent\Model; use Lunar\Shipping\Filament\Resources\ShippingZoneResource\Pages\ManageShippingRates as BaseManageShippingRates; use Lunar\Shipping\Models\ShippingMethod; use Lunar\Shipping\Models\ShippingRate; +use Modules\Core\Shipping\Support\ShippingMethodName; /** * Bound in place of the vendor ManageShippingRates page via the container @@ -33,6 +35,16 @@ use Lunar\Shipping\Models\ShippingRate; * null-guard, which crashes on any rate with no basePrices row — routine * for a live rate that has never had a fallback price configured. Same * logic, just null-safe. + * + * Also replaces the vendor's `shipping_method_id` Select, which uses + * ->relationship(titleAttribute: 'name') — Filament builds that option + * list with `orderBy('name')`/`pluck('name', ...)` against the DB, but + * ShippingMethod.name is now a locale-keyed JSON column (see database/ + * migrations/..._make_shipping_methods_name_translatable.php) that + * Postgres has no default ordering operator for, crashing with + * "could not identify an ordering operator for type json" the moment + * this page loads. Resolved app-side instead via ShippingMethodName, + * same as every other read site for this column. */ class ManageShippingRates extends BaseManageShippingRates { @@ -41,10 +53,30 @@ class ManageShippingRates extends BaseManageShippingRates $schema = parent::form($schema); return $schema->components( - $this->labelPriceFieldsAsFallbackWhenLive($schema->getComponents()) + $this->labelPriceFieldsAsFallbackWhenLive( + $this->replaceShippingMethodField($schema->getComponents()) + ) ); } + private function replaceShippingMethodField(array $components): array + { + return array_map(function ($component) { + if (method_exists($component, 'getName') && $component->getName() === 'shipping_method_id') { + return Select::make('shipping_method_id') + ->label($component->getLabel()) + ->required() + ->live() + ->options(fn () => ShippingMethod::all() + ->mapWithKeys(fn (ShippingMethod $method) => [$method->id => ShippingMethodName::resolve($method)])) + ->searchable() + ->columnSpan(2); + } + + return $component; + }, $components); + } + private function labelPriceFieldsAsFallbackWhenLive(array $components): array { $isLive = fn (Get $get) => static::methodChargeBy($get('shipping_method_id')) === 'live'; @@ -86,6 +118,14 @@ class ManageShippingRates extends BaseManageShippingRates return $table->columns( array_map(function ($column) { + if (method_exists($column, 'getName') && $column->getName() === 'shippingMethod.name') { + return TextColumn::make('shippingMethod.name') + ->label(__('lunarpanel.shipping::relationmanagers.shipping_rates.table.shipping_method.label')) + ->state(fn (ShippingRate $record) => $record->shippingMethod + ? ShippingMethodName::resolve($record->shippingMethod) + : null); + } + if (method_exists($column, 'getName') && $column->getName() === 'basePrices.0') { return TextColumn::make('basePrices.0') ->label(__('lunarpanel.shipping::relationmanagers.shipping_rates.table.price.label')) From 12aaa43f1032f6e5a66e166fd1dba369655fe68d Mon Sep 17 00:00:00 2001 From: Konstantinos Arvanitakis Date: Fri, 18 Sep 2026 00:54:43 +0300 Subject: [PATCH 3/4] Fix: Updates to OrderFullfilmentServices and box now clients, order views and checkout services --- .../Exceptions/NoShippingAddressException.php | 18 ++++ src/Checkout/Services/CheckoutService.php | 85 ++++++++++++++++++- .../MarkOrderPlacedOnDeferredPayment.php | 66 ++++++++++++++ .../Services/OrderFulfillmentService.php | 36 ++++++++ .../OrderPaymentResolutionService.php | 18 ++++ .../Drivers/CashOnDeliveryPaymentDriver.php | 18 +++- src/Payment/Events/PaymentDeferred.php | 46 ++++++++++ .../Resources/PaymentMethodResource.php | 46 +++++++++- .../Pages/ListPaymentMethods.php | 10 +++ src/Providers/OrderServiceProvider.php | 4 + src/Shipping/Carriers/BoxNow/BoxNowClient.php | 14 +++ .../BoxNow/BoxNowFulfillmentService.php | 33 ++++++- .../Extensions/OrderShipmentsExtension.php | 26 ++++++ .../Extensions/OrderViewExtension.php | 35 +++++--- src/Shipping/Jobs/PollShipmentTrackingJob.php | 13 ++- 15 files changed, 450 insertions(+), 18 deletions(-) create mode 100644 src/Checkout/Exceptions/NoShippingAddressException.php create mode 100644 src/Order/Listeners/MarkOrderPlacedOnDeferredPayment.php create mode 100644 src/Payment/Events/PaymentDeferred.php diff --git a/src/Checkout/Exceptions/NoShippingAddressException.php b/src/Checkout/Exceptions/NoShippingAddressException.php new file mode 100644 index 0000000..4a90b0b --- /dev/null +++ b/src/Checkout/Exceptions/NoShippingAddressException.php @@ -0,0 +1,18 @@ +cart->currentOrCreate()->setShippingAddress($address); + $cartBefore = $this->cart->currentOrCreate(); + $boxNowLocker = $cartBefore->shippingAddress?->meta['box_now_locker'] ?? null; + + $cart = $cartBefore->setShippingAddress($address); + + if ($boxNowLocker !== null) { + $newAddress = $cart->shippingAddress; + $newAddress->meta = [...($newAddress->meta?->toArray() ?? []), 'box_now_locker' => $boxNowLocker]; + $newAddress->save(); + } Event::dispatch(new ShippingAddressSet($cart, $address)); @@ -141,11 +168,67 @@ class CheckoutService $cart = $cartBefore->setShippingOption($option); + // Switching away from Box Now leaves a stale box_now_locker on the + // address's meta (see setShippingAddress()'s own docblock for why + // it survives address-row recreation) — irrelevant while a + // different method is selected, but wrong if the shopper later + // switches BACK to Box Now and it resurfaces as if still chosen, + // possibly for a locker that no longer exists/fits. Cleared here, + // the one place that knows the method just changed. + if ($identifier !== 'box-now') { + $address = $cart->shippingAddress; + + if ($address && isset($address->meta['box_now_locker'])) { + $meta = $address->meta->toArray(); + unset($meta['box_now_locker']); + $address->meta = $meta; + $address->save(); + } + } + Event::dispatch(new ShippingOptionSelected($cart, $option)); return $cart; } + /** + * Records the shopper's chosen Box Now locker on the cart's shipping + * address (Cart\Addresses::shippingAddress()->meta['box_now_locker']), + * not on the cart itself — Lunar\Pipelines\Order\Creation\ + * CreateOrderAddresses copies every cart address's full attributes + * (meta included) onto the new order address when the order is placed, + * so this is what Modules\Core\Shipping\Carriers\BoxNow\ + * BoxNowFulfillmentService and Modules\Core\Shipping\Extensions\ + * OrderViewExtension already expect to find at + * $order->shippingAddress->meta['box_now_locker']['locationId']. + * + * No validation against Box Now's own /destinations list here — this + * mirrors setShippingAddress()'s leniency (see its own docblock/the + * class-level note on required-field enforcement happening at the + * payment gate, not mid-checkout). An invalid/stale locationId still + * surfaces later, at BoxNowFulfillmentService::createShipment() time. + * + * @throws NoShippingAddressException if the cart has no shipping + * address yet + */ + public function selectBoxNowLocker(array $locker): Cart + { + $cart = $this->cart->currentOrCreate(); + $address = $cart->shippingAddress; + + if (! $address) { + throw new NoShippingAddressException(); + } + + $address->meta = [ + ...($address->meta?->toArray() ?? []), + 'box_now_locker' => $locker, + ]; + $address->save(); + + return $cart; + } + /** * Every payment method currently offered to the storefront, ordered by * Modules\Core\Payment\Models\PaymentMethod::position — a row is diff --git a/src/Order/Listeners/MarkOrderPlacedOnDeferredPayment.php b/src/Order/Listeners/MarkOrderPlacedOnDeferredPayment.php new file mode 100644 index 0000000..90ef307 --- /dev/null +++ b/src/Order/Listeners/MarkOrderPlacedOnDeferredPayment.php @@ -0,0 +1,66 @@ + DecrementStockOnOrderPlaced), + * so queueing this one would just move the same stock-oversell risk one + * hop earlier. + * + * A thin reactor, same shape as ApplyResolvedPaymentStatus — the actual + * decisions ("this order counts as placed the moment a deferred-payment + * driver resolves, independent of Order::paid" and "such an order also + * has nothing to sit at awaiting_payment for") live in PaymentDeferred's + * and OrderPaymentResolutionService::resolveDeferredPayment()'s own + * docblocks, re-confirmed with the user; this only extracts the order id + * and applies both, guarded against a duplicate/replayed event the same + * way OrderPaymentResolutionService::resolveCaptureOrAuthorization() is. + * + * Without the status advance below, a COD order was left sitting at + * 'awaiting_payment' forever — placed_at/OrderPlaced alone fixed order + * visibility and stock decrement, but nothing ever moved `status` off its + * initial value, since resolveCaptureOrAuthorization() only does that for + * an actual capture. Caught and fixed after the fact. + */ +class MarkOrderPlacedOnDeferredPayment +{ + public function __construct( + private readonly OrderPaymentResolutionService $resolution, + ) {} + + public function handle(PaymentDeferred $event): void + { + $orderId = $event->context['order_id'] ?? null; + + if ($orderId === null) { + return; + } + + $order = Order::findOrFail($orderId); + + $this->resolution->resolveDeferredPayment($order, self::class); + + if (! blank($order->placed_at)) { + return; + } + + $order->update(['placed_at' => now()]); + + Event::dispatch(new OrderPlaced($order)); + } +} diff --git a/src/Order/Services/OrderFulfillmentService.php b/src/Order/Services/OrderFulfillmentService.php index 1874883..70eec8d 100644 --- a/src/Order/Services/OrderFulfillmentService.php +++ b/src/Order/Services/OrderFulfillmentService.php @@ -8,6 +8,8 @@ use Modules\Core\Order\DTOs\OrderFulfillmentResult; use Modules\Core\Order\Events\OrderPickedUp; use Modules\Core\Order\Events\OrderReadyForDispatch; use Modules\Core\Order\Events\OrderReadyForPickup; +use Modules\Core\Payment\DTOs\PaymentResult; +use Modules\Core\Payment\Enums\PaymentResultStatus; use Modules\Core\Shipping\Contracts\CarrierFulfillmentInterface; use Modules\Core\Shipping\DTOs\ShipmentRequest; use Throwable; @@ -31,6 +33,7 @@ class OrderFulfillmentService public function __construct( private readonly OrderStatusWriter $writer, private readonly OrderStatusFlow $flow, + private readonly TransactionRecorder $transactions, ) {} public function markReady(Order $order): OrderFulfillmentResult @@ -121,6 +124,39 @@ class OrderFulfillmentService return OrderFulfillmentResult::failure('This order cannot be marked paid right now.'); } + // canMarkPaid() only ever returns true for an order whose payment + // method resolves to the cash-on-delivery DRIVER (see + // OrderStatusFlow::isCod(), which checks PaymentMethod::driver, + // never the merchant-chosen `type` slug directly — a store could + // name that method "cod", "pay-on-delivery", anything). Such an + // order never runs through Payment's pay()/authorize() flow at + // checkout, so nothing else records a Transaction for it. Money + // changes hands right here, at this click, so this is the one + // place that write can happen; there is no earlier Payment event + // to hang it off of the way Modules\Core\Order\Listeners\ + // RecordPaymentTransaction does for a gateway driver. See + // TransactionRecorder's own docblock — it already anticipated + // exactly this "manually-triggered ... from Filament" call site. + // + // $driver below is the payment method's own `type` slug (whatever + // the merchant named it, e.g. 'cash-on-delivery' or 'cod') — + // Transaction.driver's established meaning everywhere else in this + // codebase (see RecordPaymentTransaction/TransactionRecorder's own + // docblocks) is that type key, never the underlying driver CLASS. + // No fallback guess here: CheckoutService::initiatePayment() always + // writes Order.meta['payment_method'] before charging, and + // canMarkPaid() already guarantees this order got that far. + $this->transactions->record( + $order, + type: 'capture', + driver: (string) $order->meta['payment_method'], + result: new PaymentResult( + status: PaymentResultStatus::Succeeded, + reference: 'cod-manual-'.$order->id, + amount: $order->total, + ), + ); + $this->writer->markPaid($order, self::class.'::markPaid'); return OrderFulfillmentResult::success('Order marked as paid.'); diff --git a/src/Order/Services/OrderPaymentResolutionService.php b/src/Order/Services/OrderPaymentResolutionService.php index 5efdb27..d990382 100644 --- a/src/Order/Services/OrderPaymentResolutionService.php +++ b/src/Order/Services/OrderPaymentResolutionService.php @@ -52,6 +52,24 @@ class OrderPaymentResolutionService } } + /** + * A deferred-capture payment (currently only cash-on-delivery — see + * Payment\Events\PaymentDeferred's own docblock): no money has moved, + * so unlike resolveCaptureOrAuthorization() this never calls + * $writer->markPaid() — Order::paid stays false until staff explicitly + * mark it received. But per OrderStatusFlow's own docblock, payment + * method never affects the status SEQUENCE at all — a COD order has + * nothing to "await" at checkout (no payment attempt happens), so + * 'awaiting_payment' is simply the wrong first status for it. Reuses + * the exact same advancePastAwaitingPayment() a capture uses, since + * the status-sequence logic itself doesn't differ by payment method, + * only whether `paid` also flips alongside it. + */ + public function resolveDeferredPayment(Order $order, string $causeClass): void + { + $this->advancePastAwaitingPayment($order, $causeClass); + } + /** * Requires the refund Transaction row to already exist (Modules\Core\ * Order\Listeners\RecordPaymentTransaction must run first — see diff --git a/src/Payment/Drivers/CashOnDeliveryPaymentDriver.php b/src/Payment/Drivers/CashOnDeliveryPaymentDriver.php index 866167b..ea568bf 100644 --- a/src/Payment/Drivers/CashOnDeliveryPaymentDriver.php +++ b/src/Payment/Drivers/CashOnDeliveryPaymentDriver.php @@ -8,6 +8,7 @@ use Modules\Core\Payment\Contracts\Configurable; use Modules\Core\Payment\Contracts\SupportsPay; use Modules\Core\Payment\DTOs\PaymentResult; use Modules\Core\Payment\Enums\PaymentResultStatus; +use Modules\Core\Payment\Events\PaymentDeferred; /** * Cash-on-delivery/cash-on-pickup — the shopper pays staff in person, at @@ -31,6 +32,17 @@ use Modules\Core\Payment\Enums\PaymentResultStatus; * marking it received (Modules\Core\Order\Services\ * OrderFulfillmentService::markPaid()), offered by the single "Update * Status" action at any time, independent of status. + * + * Despite returning Pending, this order IS fully placed the moment pay() + * returns — unlike a Stripe 3-D Secure Pending, nothing will ever resolve + * this into a later PaymentCaptured/PaymentAuthorized (COD has no gateway + * callback at all). Without PaymentDeferred, no listener ever set + * Order::placed_at for a COD order: invisible in customer order history, + * no stock decrement (Modules\Core\Order\Listeners\ + * DecrementStockOnOrderPlaced only reacts to Checkout\Events\OrderPlaced), + * and the storefront's own post-checkout confirmation could never find it + * — a real bug, not a hypothetical, caught and fixed after the fact. See + * PaymentDeferred's own docblock for the full reasoning. */ class CashOnDeliveryPaymentDriver implements Configurable, SupportsPay { @@ -41,10 +53,14 @@ class CashOnDeliveryPaymentDriver implements Configurable, SupportsPay public function pay(string $type, Price $amount, array $data = [], array $context = []): PaymentResult { - return new PaymentResult( + $result = new PaymentResult( status: PaymentResultStatus::Pending, reference: 'cod-'.Str::uuid(), amount: $amount, ); + + PaymentDeferred::dispatch($type, $result, $context); + + return $result; } } diff --git a/src/Payment/Events/PaymentDeferred.php b/src/Payment/Events/PaymentDeferred.php new file mode 100644 index 0000000..b9df3f2 --- /dev/null +++ b/src/Payment/Events/PaymentDeferred.php @@ -0,0 +1,46 @@ + $context + */ + public function __construct( + public readonly string $type, + public readonly PaymentResult $result, + public readonly array $context = [], + ) {} +} diff --git a/src/Payment/Filament/Resources/PaymentMethodResource.php b/src/Payment/Filament/Resources/PaymentMethodResource.php index 2cfbc2c..2ea9eed 100644 --- a/src/Payment/Filament/Resources/PaymentMethodResource.php +++ b/src/Payment/Filament/Resources/PaymentMethodResource.php @@ -3,10 +3,13 @@ namespace Modules\Core\Payment\Filament\Resources; use Filament\Actions\Action; +use Filament\Forms\Components\Hidden; use Filament\Forms\Components\Select; use Filament\Forms\Components\TextInput; use Filament\Resources\Resource; use Filament\Schemas\Components\Component; +use Filament\Schemas\Components\Utilities\Get; +use InvalidArgumentException; use Filament\Tables\Columns\IconColumn; use Filament\Tables\Columns\TextColumn; use Filament\Tables\Columns\ToggleColumn; @@ -14,6 +17,7 @@ use Filament\Tables\Table; use Illuminate\Support\Facades\Event; use Lunar\Admin\Support\Forms\Components\TranslatedText; use Modules\Core\Payment\Contracts\Configurable; +use Modules\Core\Payment\Contracts\SupportsAuthorization; use Modules\Core\Payment\Events\PaymentMethodsReordered; use Modules\Core\Payment\Filament\Resources\PaymentMethodResource\Pages\ListPaymentMethods; use Modules\Core\Payment\Models\PaymentMethod; @@ -144,7 +148,31 @@ class PaymentMethodResource extends Resource ]) ->default('pay') ->live() - ->required(), + // Only meaningful for a driver that actually implements + // SupportsAuthorization — CheckoutService::initiatePayment() + // calls $driver->authorize() when capture_mode is + // "authorize", which fatals on a driver missing that method + // entirely (e.g. CashOnDeliveryPaymentDriver, which only + // ever implements SupportsPay: the shopper pays staff in + // person, at an unknown future moment — there is no + // "hold now, settle later" operation to offer for that at + // all). Hidden rather than merely disabled, since a + // hidden field is also excluded from validation/dehydration + // — required() below would otherwise still block saving. + ->visible(fn (Get $get) => static::driverSupportsAuthorization($get('driver'))) + ->required(fn (Get $get) => static::driverSupportsAuthorization($get('driver'))), + // Every OTHER fillForm() value not backed by a real component + // here is silently dropped — an Action::schema() modal only + // dehydrates fields present in its own schema, unlike a + // resource's form(); ListPaymentMethods::getHeaderActions()'s + // CreateAction::fillForm() used to set 'position' this same + // way and it never reached PaymentMethodService::create(), + // so every new method saved with the column's raw DB default + // (0) regardless of what fillForm() computed. Hidden here + // purely so it actually dehydrates; the table's own + // reorderable('position') drag-and-drop remains the real + // staff-facing way to change it afterward. + Hidden::make('position'), ]; } @@ -153,9 +181,23 @@ class PaymentMethodResource extends Resource return Select::make('driver') ->label('Driver') ->options(fn () => app(PaymentDriverRegistry::class)->labels()) + ->live() ->required(); } + private static function driverSupportsAuthorization(?string $driver): bool + { + if (! $driver) { + return true; + } + + try { + return app(PaymentDriverRegistry::class)->resolve($driver) instanceof SupportsAuthorization; + } catch (InvalidArgumentException) { + return true; + } + } + public static function getPages(): array { return [ @@ -180,7 +222,7 @@ class PaymentMethodResource extends Resource ->icon('heroicon-o-pencil-square') ->schema(static::getFormComponents()) ->fillForm(fn (PaymentMethod $record) => $record->only([ - 'name', 'type', 'driver', 'capture_mode', + 'name', 'type', 'driver', 'capture_mode', 'position', ])) ->action(fn (PaymentMethod $record, array $data) => app(PaymentMethodService::class)->update($record, $data)); } diff --git a/src/Payment/Filament/Resources/PaymentMethodResource/Pages/ListPaymentMethods.php b/src/Payment/Filament/Resources/PaymentMethodResource/Pages/ListPaymentMethods.php index 9d170b1..663d654 100644 --- a/src/Payment/Filament/Resources/PaymentMethodResource/Pages/ListPaymentMethods.php +++ b/src/Payment/Filament/Resources/PaymentMethodResource/Pages/ListPaymentMethods.php @@ -5,6 +5,7 @@ namespace Modules\Core\Payment\Filament\Resources\PaymentMethodResource\Pages; use Filament\Actions\CreateAction; use Filament\Actions; use Filament\Resources\Pages\ListRecords; +use Lunar\Models\Language; use Modules\Core\Payment\Filament\Resources\PaymentMethodResource; use Modules\Core\Payment\Models\PaymentMethod; use Modules\Core\Payment\Services\PaymentMethodService; @@ -22,6 +23,15 @@ class ListPaymentMethods extends ListRecords 'position' => (PaymentMethod::max('position') ?? 0) + 1, 'enabled' => false, 'data' => [], + // TranslatedText's own default() (getLanguageDefaults()) + // never reaches this mounted action's initial state — + // unlike a resource's own form(), an Action::schema() + // modal starts from exactly what fillForm() returns, so + // `name` was landing as null rather than the expected + // per-locale array, and every locale's sub-input + // silently failed to bind to it (required() on the + // default locale then correctly rejected the null). + 'name' => Language::pluck('code')->mapWithKeys(fn (string $code) => [$code => ''])->all(), ]) // Every PaymentMethod write goes through PaymentMethodService // — see PaymentMethodResource's own docblock — so this diff --git a/src/Providers/OrderServiceProvider.php b/src/Providers/OrderServiceProvider.php index b5b57e5..4e1cdc3 100644 --- a/src/Providers/OrderServiceProvider.php +++ b/src/Providers/OrderServiceProvider.php @@ -21,6 +21,7 @@ use Modules\Core\Order\Listeners\CompleteOrderOnPickedUp; use Modules\Core\Order\Listeners\DecrementStockOnOrderPlaced; use Modules\Core\Order\Listeners\DeriveOrderDeliveredFromShipment; use Modules\Core\Order\Listeners\MarkDeliveryFailedOnCarrierCheckpoint; +use Modules\Core\Order\Listeners\MarkOrderPlacedOnDeferredPayment; use Modules\Core\Order\Listeners\RecordPaymentTransaction; use Modules\Core\Order\Listeners\RecordStatusTransition; use Modules\Core\Order\Models\OrderStatusTransition; @@ -37,6 +38,7 @@ use Modules\Core\Order\Observers\TransactionObserver; use Modules\Core\Order\Support\OrderStatus; use Modules\Core\Payment\Events\PaymentAuthorized; use Modules\Core\Payment\Events\PaymentCaptured; +use Modules\Core\Payment\Events\PaymentDeferred; use Modules\Core\Payment\Events\PaymentRefunded; use Modules\Core\Payment\Events\PaymentVoided; use Modules\Core\Shipping\Events\ShipmentStatusUpdatedByCarrier; @@ -74,6 +76,8 @@ class OrderServiceProvider extends ServiceProvider Event::listen(PaymentRefunded::class, RecordPaymentTransaction::class); Event::listen(PaymentRefunded::class, ApplyResolvedPaymentStatus::class); + Event::listen(PaymentDeferred::class, MarkOrderPlacedOnDeferredPayment::class); + Event::listen(OrderPlaced::class, DecrementStockOnOrderPlaced::class); Event::listen(OrderStatusChanged::class, [RecordStatusTransition::class, 'handleStatusChanged']); diff --git a/src/Shipping/Carriers/BoxNow/BoxNowClient.php b/src/Shipping/Carriers/BoxNow/BoxNowClient.php index dd8bbea..2507249 100644 --- a/src/Shipping/Carriers/BoxNow/BoxNowClient.php +++ b/src/Shipping/Carriers/BoxNow/BoxNowClient.php @@ -61,6 +61,20 @@ class BoxNowClient return $response->json() ?? []; } + /** + * List available APM (locker) destinations — the data behind Box Now's + * own Destination Map widget, which only works against their + * Production environment (not Stage/sandbox). Used to build a plain + * locker picker on the storefront checkout instead, working against + * whichever environment is configured. + * + * @return array> + */ + public function destinations(array $query = []): array + { + return $this->locationRequest('/destinations', $query)['data'] ?? []; + } + /** * Fetch raw bytes (e.g. a PDF label) rather than JSON. */ diff --git a/src/Shipping/Carriers/BoxNow/BoxNowFulfillmentService.php b/src/Shipping/Carriers/BoxNow/BoxNowFulfillmentService.php index 971df90..7dbae9a 100644 --- a/src/Shipping/Carriers/BoxNow/BoxNowFulfillmentService.php +++ b/src/Shipping/Carriers/BoxNow/BoxNowFulfillmentService.php @@ -67,7 +67,7 @@ class BoxNowFulfillmentService implements CarrierFulfillmentInterface, SupportsT 'locationId' => config('boxnow.origin_location_id'), ], 'destination' => [ - 'contactNumber' => $address->contact_phone, + 'contactNumber' => $this->internationalPhone($address->contact_phone), 'contactEmail' => $address->contact_email, 'contactName' => trim("{$address->first_name} {$address->last_name}"), 'locationId' => $destinationLocationId, @@ -105,6 +105,37 @@ class BoxNowFulfillmentService implements CarrierFulfillmentInterface, SupportsT return $shipments->first(); } + /** + * Box Now rejects any contactNumber not in full international format + * (error P405 — confirmed in practice: a plain Greek mobile like + * "6955994563" 400s with {"code":"P405"}). Checkout collects phone + * numbers in local format, with no international-format enforcement of + * its own — this store is Greece-only (see CheckoutController:: + * STORE_COUNTRY_ISO3), so a bare local number is assumed Greek and + * prefixed accordingly, same convention ACS's own sender config already + * uses (config('boxnow.sender.phone') is documented as "+30..." there + * too). A number already carrying a country code (leading "+" or "00") + * is passed through unchanged. + */ + private function internationalPhone(?string $phone): ?string + { + if ($phone === null) { + return null; + } + + $digitsOnly = preg_replace('/[^\d+]/', '', $phone); + + if (str_starts_with($digitsOnly, '+')) { + return $digitsOnly; + } + + if (str_starts_with($digitsOnly, '00')) { + return '+'.substr($digitsOnly, 2); + } + + return '+30'.ltrim($digitsOnly, '0'); + } + public function printLabel(Shipment $shipment): string { $bytes = $this->client->requestRaw("/parcels/{$shipment->tracking_reference}/label.pdf"); diff --git a/src/Shipping/Extensions/OrderShipmentsExtension.php b/src/Shipping/Extensions/OrderShipmentsExtension.php index 0298812..a321642 100644 --- a/src/Shipping/Extensions/OrderShipmentsExtension.php +++ b/src/Shipping/Extensions/OrderShipmentsExtension.php @@ -5,6 +5,8 @@ namespace Modules\Core\Shipping\Extensions; use Filament\Actions\Action; use Filament\Infolists\Components\RepeatableEntry; use Filament\Infolists\Components\TextEntry; +use Illuminate\Support\Collection; +use Modules\Core\Shipping\Models\ShipmentInfo; use Filament\Notifications\Notification; use Filament\Schemas\Components\Section; use Illuminate\Support\Facades\URL; @@ -112,10 +114,34 @@ class OrderShipmentsExtension extends ViewPageExtension ->action(fn (Shipment $record) => $this->cancel($record)) ->visible(fn (Shipment $record) => ! $record->cancelled_at), ]), + RepeatableEntry::make('shipmentInfo') + ->label('Tracking history') + ->state(fn (Shipment $record) => $this->orderedCheckpoints($record)) + ->hidden(fn (Shipment $record) => $record->shipmentInfo->isEmpty()) + ->schema([ + TextEntry::make('status') + ->label(fn (ShipmentInfo $record) => $record->occurred_at->format('Y-m-d H:i')) + ->inlineLabel() + ->state(fn (ShipmentInfo $record) => (string) str($record->status->value)->replace('_', ' ')->title()) + ->helperText(fn (ShipmentInfo $record) => $record->location), + ]), ]), ]); } + /** + * Oldest first — a delivery journey (Collected → In Transit → + * Delivered) reads naturally top-to-bottom in that order, unlike + * statusLabel()/statusColor() above which only ever need the single + * latest checkpoint and so use latestShipmentInfo() directly instead. + * + * @return Collection + */ + private function orderedCheckpoints(Shipment $record): Collection + { + return $record->shipmentInfo->sortBy('occurred_at')->values(); + } + private function carrierLabel(Shipment $record): string { return match ($record->carrier) { diff --git a/src/Shipping/Extensions/OrderViewExtension.php b/src/Shipping/Extensions/OrderViewExtension.php index f792860..0685b26 100644 --- a/src/Shipping/Extensions/OrderViewExtension.php +++ b/src/Shipping/Extensions/OrderViewExtension.php @@ -56,10 +56,11 @@ use Modules\Core\Shipping\Support\WeightCalculator; * each with its own S/M/L size) instead — see * Modules\Core\Shipping\Carriers\BoxNow\BoxNowFulfillmentService for how * multiple boxes become multiple Shipment rows from one delivery request. - * Box Now's locker is locked to read-only once the shopper's own checkout - * selection ($order->shippingAddress->meta['box_now_locker']) is present - * — staff can only fill it in manually for the (current, checkout-UI-less) - * case where nothing set it yet. + * Box Now's locker field defaults from the shopper's own checkout + * selection ($order->shippingAddress->meta['box_now_locker']) when present, + * but stays editable — staff can override to a different locker (e.g. the + * customer's choice turns out to be unavailable) or fill it in manually for + * an order placed before the checkout locker picker existed. * * "Mark Paid" is a third, separate header action — Order::paid is * independent of `status` (see OrderStatusFlow's own docblock), so it @@ -124,16 +125,9 @@ class OrderViewExtension extends ViewPageExtension TextInput::make('destination_location_id') ->label('Box Now locker ID') ->default($lockerId) - // Locked once the shopper's own checkout selection is - // known — staff should not be able to redirect a - // parcel to a different locker than the one the - // customer picked. Only editable for the (current, - // checkout-UI-less) case where nothing set it yet. - ->disabled(filled($lockerId)) - ->dehydrated() ->required() ->helperText($lockerId - ? 'Set by the customer at checkout.' + ? 'Set by the customer at checkout — override if the parcel needs to go to a different locker.' : 'No locker was selected at checkout — enter it manually.'), Repeater::make('boxes') ->label('Boxes') @@ -152,12 +146,29 @@ class OrderViewExtension extends ViewPageExtension ]; }) ->action(function (Order $record, array $data, Action $action) { + // Derived from the order itself, never from staff input — + // whether a shipment collects cash on delivery is a fact + // about the order (which PaymentMethod it was placed + // against), not a choice to make again at dispatch time. + // Previously this was never set at all (defaulted to + // ShipmentRequest::$paymentMode's own null), which silently + // made AcsFulfillmentService::createShipment()'s + // `$request->paymentMode === 'cod'` branch (sends + // Cod_Ammount/Cod_Payment_Way to ACS) permanently + // unreachable, and BoxNowFulfillmentService::createShipment() + // always ship 'prepaid' with amountToBeCollected '0.00' — + // a real COD order would arrive with the carrier believing + // full payment was already settled, and never collect it. + $isCod = app(OrderStatusFlow::class)->isCod($record); + $result = $this->service()->createShipmentAndDispatch( $record, new ShipmentRequest( weight: filled($data['weight'] ?? null) ? (float) $data['weight'] : null, packageCount: (int) ($data['package_count'] ?? 1), destinationLocationId: $data['destination_location_id'] ?? null, + paymentMode: $isCod ? 'cod' : 'prepaid', + amountToCollect: $isCod ? $record->total->decimal : null, boxes: collect($data['boxes'] ?? [])->pluck('size')->all(), ), ); diff --git a/src/Shipping/Jobs/PollShipmentTrackingJob.php b/src/Shipping/Jobs/PollShipmentTrackingJob.php index 7714e22..7090aac 100644 --- a/src/Shipping/Jobs/PollShipmentTrackingJob.php +++ b/src/Shipping/Jobs/PollShipmentTrackingJob.php @@ -14,12 +14,19 @@ use Modules\Core\Shipping\Enums\TrackingStatus; use Modules\Core\Shipping\Events\ShipmentStatusUpdatedByCarrier; use Modules\Core\Shipping\Models\Shipment; use Modules\Core\Shipping\Models\ShipmentInfo; +use Throwable; /** * Carrier-agnostic: polls every Shipment not yet in a terminal state, * skipping carriers whose fulfillment service doesn't implement * SupportsTracking. New checkpoints are recorded in shipment_info and * dispatch ShipmentStatusUpdatedByCarrier — one event per new checkpoint. + * + * Each shipment's trackShipment() call is individually try/caught in + * pollCarrierShipments() — one shipment's tracking lookup failing (a + * carrier 500, a malformed parcel response) must not stop the rest of that + * carrier's shipments in the same batch from being polled. The failure is + * reported and the loop continues. */ class PollShipmentTrackingJob implements ShouldQueue { @@ -68,7 +75,11 @@ class PollShipmentTrackingJob implements ShouldQueue } foreach ($shipments as $shipment) { - $this->recordNewCheckpoints($shipment, $service->trackShipment($shipment)); + try { + $this->recordNewCheckpoints($shipment, $service->trackShipment($shipment)); + } catch (Throwable $e) { + report($e); + } } } From 609a63c2f4d0c7e57725ee6f104873d3f07e295a Mon Sep 17 00:00:00 2001 From: Konstantinos Arvanitakis Date: Fri, 18 Sep 2026 01:23:50 +0300 Subject: [PATCH 4/4] Feat: Tying Specific Methods with Carrier Drivers --- src/Checkout/Services/CheckoutService.php | 51 ++++++++++++++++++- .../Contracts/RequiresFulfillmentType.php | 38 ++++++++++++++ .../Drivers/CashOnDeliveryPaymentDriver.php | 14 ++++- src/Payment/Drivers/OfflinePaymentDriver.php | 13 ++++- 4 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 src/Payment/Contracts/RequiresFulfillmentType.php diff --git a/src/Checkout/Services/CheckoutService.php b/src/Checkout/Services/CheckoutService.php index 66eaf64..1d33b24 100644 --- a/src/Checkout/Services/CheckoutService.php +++ b/src/Checkout/Services/CheckoutService.php @@ -10,6 +10,7 @@ use Lunar\Base\Addressable; use Lunar\DataTypes\ShippingOption; use Lunar\Facades\ShippingManifest; use Lunar\Models\Cart; +use Lunar\Shipping\Models\ShippingMethod; use Modules\Core\Cart\Services\CartService; use Modules\Core\Checkout\Events\BillingAddressSet; use Modules\Core\Checkout\Events\PaymentMethodSelected; @@ -20,10 +21,12 @@ use Modules\Core\Checkout\Exceptions\InvalidShippingOptionException; use Modules\Core\Checkout\Exceptions\NoShippingAddressException; use Modules\Core\Checkout\Exceptions\TermsNotAcceptedException; use Modules\Core\Checkout\Exceptions\UnknownPaymentTypeException; +use Modules\Core\Payment\Contracts\RequiresFulfillmentType; use Modules\Core\Payment\DTOs\PaymentResult; use Modules\Core\Payment\Models\PaymentMethod; use Modules\Core\Payment\Services\PaymentDriverRegistry; use Modules\Core\Payment\Services\PaymentMethodCache; +use Modules\Core\Shipping\Support\FulfillmentType; /** * Storefront-facing checkout operations, mirroring @@ -232,7 +235,7 @@ class CheckoutService /** * Every payment method currently offered to the storefront, ordered by * Modules\Core\Payment\Models\PaymentMethod::position — a row is - * offered only when ALL three checks pass, each meaning something + * offered only when ALL four checks pass, each meaning something * different to an admin diagnosing why a method isn't showing up (see * docs/payments.md): * 1. `enabled` — an admin turned it on. @@ -243,17 +246,61 @@ class CheckoutService * vanished driver can never silently look "available"). * 3. the resolved driver reports Configurable::isConfigured() — its * own runtime requirements (e.g. an API key) are met. + * 4. its driver's RequiresFulfillmentType (if it declares one) + * agrees with the cart's currently selected shipping method's own + * fulfillment type (Modules\Core\Shipping\Support\ + * FulfillmentType::resolve()) — "Pay in store" offered alongside + * a courier delivery makes no sense (no staff member present at + * handoff to take cash), and cash-on-delivery alongside store + * pickup is equally meaningless (OfflinePaymentDriver already + * covers that in-person moment). A cart with no shipping option + * selected yet imposes no constraint here — every method is + * offered until a fulfillment type is actually known, the same + * leniency setShippingAddress()'s own docblock describes for + * required-field enforcement happening at the payment gate, not + * mid-checkout. * * @return Collection */ public function getPaymentMethods(): Collection { + $fulfillmentType = $this->currentFulfillmentType(); + return $this->paymentMethods->all() ->filter(fn (PaymentMethod $method) => $method->enabled && $method->driver_missing_at === null) - ->filter(fn (PaymentMethod $method) => $this->paymentDrivers->resolve($method->driver)?->isConfigured() ?? false) + ->filter(function (PaymentMethod $method) use ($fulfillmentType) { + $driver = $this->paymentDrivers->resolve($method->driver); + + if (! $driver?->isConfigured()) { + return false; + } + + if ($fulfillmentType === null || ! $driver instanceof RequiresFulfillmentType) { + return true; + } + + return $driver->requiredFulfillmentType() === $fulfillmentType; + }) ->values(); } + /** + * @return 'carrier'|'store_pickup'|null null when the cart has no + * shipping option selected yet + */ + private function currentFulfillmentType(): ?string + { + $identifier = $this->cart->currentOrCreate()->shippingAddress?->shipping_option; + + if ($identifier === null) { + return null; + } + + $method = ShippingMethod::where('code', $identifier)->first(); + + return $method ? FulfillmentType::resolve($method) : null; + } + /** * Records which payment type the shopper picked (Cart::meta * ['payment_method']) — read by Modules\Core\Payment\Pipelines\ diff --git a/src/Payment/Contracts/RequiresFulfillmentType.php b/src/Payment/Contracts/RequiresFulfillmentType.php new file mode 100644 index 0000000..2bd143d --- /dev/null +++ b/src/Payment/Contracts/RequiresFulfillmentType.php @@ -0,0 +1,38 @@ +