From cfd13a02acd965dd03bb62bcb6b48ed658512a1c Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Tue, 25 Aug 2026 21:00:27 +0500 Subject: [PATCH 1/2] Memoise the discounts a cart has already consumed --- .../core/src/Actions/Carts/CreateOrder.php | 2 +- packages/core/src/Models/Cart.php | 28 ++++++- .../Unit/Actions/Carts/CreateOrderTest.php | 80 +++++++++++++++++++ 3 files changed, 107 insertions(+), 3 deletions(-) diff --git a/packages/core/src/Actions/Carts/CreateOrder.php b/packages/core/src/Actions/Carts/CreateOrder.php index d73ede1b7b..a788ab34e6 100644 --- a/packages/core/src/Actions/Carts/CreateOrder.php +++ b/packages/core/src/Actions/Carts/CreateOrder.php @@ -31,7 +31,7 @@ public function execute( // Read before the creation pipeline runs: MapDiscountBreakdown // rewrites the order's breakdown, so afterwards every discount would // look as though it had already been consumed. - $alreadyConsumed = $cart->consumedDiscountIds(); + $alreadyConsumed = $cart->consumedDiscountIds(fresh: true); if ($cart->hasCompletedOrders() && ! $allowMultipleOrders) { throw new DisallowMultipleCartOrdersException; diff --git a/packages/core/src/Models/Cart.php b/packages/core/src/Models/Cart.php index 3b0386f141..195d2b9bb6 100644 --- a/packages/core/src/Models/Cart.php +++ b/packages/core/src/Models/Cart.php @@ -284,6 +284,11 @@ public function draftOrder(?int $draftOrderId = null): HasOne })->whereNull('placed_at'); } + /** + * Memoised result of {@see self::consumedDiscountIds()}. + */ + protected ?Collection $consumedDiscountIds = null; + /** * The ids of any discounts this cart has already consumed. * @@ -291,14 +296,33 @@ public function draftOrder(?int $draftOrderId = null): HasOne * checkout that runs it a second time - a declined card and a retry - would * otherwise find its own coupon exhausted and re-price that same order * without it. A cart's own consumption must not count against it. + * + * Memoised per instance, because this is read once when the discount set is + * rebuilt and once per discount while conditions are checked - a fresh query + * each time costs a single-row lookup per discount on every calculate, + * including for carts that never reach a checkout. + * + * @param bool $fresh Re-read rather than use the memoised set. Order + * creation writes the breakdown this reads, so it must + * pass true: a set memoised before the first order + * existed is still empty on a same-request retry, and + * the discount would be consumed a second time. */ - public function consumedDiscountIds(): Collection + public function consumedDiscountIds(bool $fresh = false): Collection { + if ($fresh) { + $this->consumedDiscountIds = null; + } + + if ($this->consumedDiscountIds !== null) { + return $this->consumedDiscountIds; + } + // Read the raw column: the cast hydrates an OrderLine per breakdown // line, which is a lot of work to reach an id. $breakdown = $this->draftOrder()->first()?->getRawOriginal('discount_breakdown'); - return collect(json_decode($breakdown ?: '[]', true) ?: []) + return $this->consumedDiscountIds = collect(json_decode($breakdown ?: '[]', true) ?: []) ->pluck('discount_id') ->filter() ->unique() diff --git a/tests/core/Unit/Actions/Carts/CreateOrderTest.php b/tests/core/Unit/Actions/Carts/CreateOrderTest.php index bda747f239..68d4248889 100644 --- a/tests/core/Unit/Actions/Carts/CreateOrderTest.php +++ b/tests/core/Unit/Actions/Carts/CreateOrderTest.php @@ -618,3 +618,83 @@ function can_update_draft_order() expect($orderB->sub_total->value)->toEqual(1000); expect($orderB->discount_total->value)->toEqual(0); }); + +test('can not consume a discount twice on one cart instance', function () { + TaxClass::factory()->create([ + 'default' => true, + ]); + + $customerGroup = CustomerGroup::factory()->create([ + 'default' => true, + ]); + + $channel = Channel::factory()->create([ + 'default' => true, + ]); + + $currency = Currency::factory()->create([ + 'decimal_places' => 2, + ]); + + $cart = Cart::factory()->create([ + 'currency_id' => $currency->id, + 'channel_id' => $channel->id, + 'coupon_code' => 'SAVE10', + ]); + + $purchasable = ProductVariant::factory()->create(); + + Price::factory()->create([ + 'price' => 1000, + 'min_quantity' => 1, + 'currency_id' => $currency->id, + 'priceable_type' => $purchasable->getMorphClass(), + 'priceable_id' => $purchasable->id, + ]); + + $cart->lines()->create([ + 'purchasable_type' => $purchasable->getMorphClass(), + 'purchasable_id' => $purchasable->id, + 'quantity' => 2, + ]); + + $discount = Discount::factory()->create([ + 'type' => AmountOff::class, + 'name' => 'Ten off', + 'coupon' => 'SAVE10', + 'uses' => 0, + 'max_uses' => 1, + 'data' => [ + 'fixed_value' => true, + 'fixed_values' => [ + $currency->code => 500, + ], + ], + ]); + + $discount->customerGroups()->sync([ + $customerGroup->id => ['enabled' => true, 'starts_at' => now()->subHour()], + ]); + + $discount->channels()->sync([ + $channel->id => ['enabled' => true, 'starts_at' => now()->subHour()], + ]); + + // The same instance throughout: no reload between the two attempts. This is + // what a checkout that retries in one request looks like, and it is the case + // any memoisation of consumedDiscountIds() has to survive - a set cached + // before the first order exists would still be empty for the second. + $cart->calculate(); + + $orderA = (new CreateOrder)->execute($cart)->then(fn ($order) => $order->refresh()); + + expect($orderA->discount_total->value)->toEqual(500); + expect($discount->refresh()->uses)->toEqual(1); + + $cart->calculate(); + + $orderB = (new CreateOrder)->execute($cart)->then(fn ($order) => $order->refresh()); + + expect($orderB->id)->toEqual($orderA->id); + expect($discount->refresh()->uses)->toEqual(1); +}); From 109398fdcdf0d012b63fee39f63e68ae4d13bad7 Mon Sep 17 00:00:00 2001 From: Glenn Jacobs Date: Wed, 26 Aug 2026 13:35:42 +0100 Subject: [PATCH 2/2] Forget the consumed discount memo when an order is created The memo was primed before the creation pipeline wrote the breakdown it reads, so it stayed empty for the rest of the request: a same-request retry read the cart's own coupon as unconsumed in CreateOrder, and as exhausted in the discount conditions, re-pricing the order without it. Invalidating on the one event that changes the answer covers both, and removes the need for the fresh flag on a public model method. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/src/Actions/Carts/CreateOrder.php | 6 +- packages/core/src/Models/Cart.php | 39 +++++---- .../Unit/Actions/Carts/CreateOrderTest.php | 83 +++++++++++++++++++ 3 files changed, 111 insertions(+), 17 deletions(-) diff --git a/packages/core/src/Actions/Carts/CreateOrder.php b/packages/core/src/Actions/Carts/CreateOrder.php index a788ab34e6..30f7cc24c0 100644 --- a/packages/core/src/Actions/Carts/CreateOrder.php +++ b/packages/core/src/Actions/Carts/CreateOrder.php @@ -31,7 +31,7 @@ public function execute( // Read before the creation pipeline runs: MapDiscountBreakdown // rewrites the order's breakdown, so afterwards every discount would // look as though it had already been consumed. - $alreadyConsumed = $cart->consumedDiscountIds(fresh: true); + $alreadyConsumed = $cart->consumedDiscountIds(); if ($cart->hasCompletedOrders() && ! $allowMultipleOrders) { throw new DisallowMultipleCartOrdersException; @@ -60,6 +60,10 @@ public function execute( $discount->markAsUsed($cart)->discount->save(); }); + // The breakdown has been rewritten, so anything still holding this + // cart must not read a set memoised before the order existed. + $cart->forgetConsumedDiscountIds(); + $cart->save(); MarkAsNewCustomer::dispatch($order->id); diff --git a/packages/core/src/Models/Cart.php b/packages/core/src/Models/Cart.php index 0781c389eb..f9bcee2d66 100644 --- a/packages/core/src/Models/Cart.php +++ b/packages/core/src/Models/Cart.php @@ -212,6 +212,11 @@ protected static function newFactory() 'coupon_code' => CouponString::class, ]; + /** + * Memoised result of {@see self::consumedDiscountIds()}. + */ + protected ?Collection $consumedDiscountIds = null; + public function lines(): HasMany { return $this->hasMany(CartLine::modelClass(), 'cart_id', 'id')->orderBy('id'); @@ -284,11 +289,6 @@ public function draftOrder(?int $draftOrderId = null): HasOne })->whereNull('placed_at'); } - /** - * Memoised result of {@see self::consumedDiscountIds()}. - */ - protected ?Collection $consumedDiscountIds = null; - /** * The ids of any discounts this cart has already consumed. * @@ -300,20 +300,13 @@ public function draftOrder(?int $draftOrderId = null): HasOne * Memoised per instance, because this is read once when the discount set is * rebuilt and once per discount while conditions are checked - a fresh query * each time costs a single-row lookup per discount on every calculate, - * including for carts that never reach a checkout. + * including for carts that never reach a checkout. Order creation is the + * only thing that changes the answer, so it forgets the memo. * - * @param bool $fresh Re-read rather than use the memoised set. Order - * creation writes the breakdown this reads, so it must - * pass true: a set memoised before the first order - * existed is still empty on a same-request retry, and - * the discount would be consumed a second time. + * @see self::forgetConsumedDiscountIds() */ - public function consumedDiscountIds(bool $fresh = false): Collection + public function consumedDiscountIds(): Collection { - if ($fresh) { - $this->consumedDiscountIds = null; - } - if ($this->consumedDiscountIds !== null) { return $this->consumedDiscountIds; } @@ -329,6 +322,20 @@ public function consumedDiscountIds(bool $fresh = false): Collection ->values(); } + /** + * Forget the memoised consumed discount ids. + * + * Order creation writes the breakdown consumedDiscountIds() reads, so a set + * memoised before it ran is stale afterwards: on a same-request retry the + * cart's own coupon would look unconsumed to CreateOrder and be consumed + * twice, and exhausted to the discount conditions, re-pricing the order + * without it. + */ + public function forgetConsumedDiscountIds(): void + { + $this->consumedDiscountIds = null; + } + public function currentDraftOrder(?int $draftOrderId = null) { return $this->calculate() diff --git a/tests/core/Unit/Actions/Carts/CreateOrderTest.php b/tests/core/Unit/Actions/Carts/CreateOrderTest.php index 68d4248889..ba78068991 100644 --- a/tests/core/Unit/Actions/Carts/CreateOrderTest.php +++ b/tests/core/Unit/Actions/Carts/CreateOrderTest.php @@ -696,5 +696,88 @@ function can_update_draft_order() $orderB = (new CreateOrder)->execute($cart)->then(fn ($order) => $order->refresh()); expect($orderB->id)->toEqual($orderA->id); + expect($orderB->discount_total->value)->toEqual(500); + expect($discount->refresh()->uses)->toEqual(1); +}); + +test('keeps its own discount when a cart is priced again after order creation', function () { + TaxClass::factory()->create([ + 'default' => true, + ]); + + $customerGroup = CustomerGroup::factory()->create([ + 'default' => true, + ]); + + $channel = Channel::factory()->create([ + 'default' => true, + ]); + + $currency = Currency::factory()->create([ + 'decimal_places' => 2, + ]); + + $cart = Cart::factory()->create([ + 'currency_id' => $currency->id, + 'channel_id' => $channel->id, + 'coupon_code' => 'SAVE10', + ]); + + $purchasable = ProductVariant::factory()->create(); + + Price::factory()->create([ + 'price' => 1000, + 'min_quantity' => 1, + 'currency_id' => $currency->id, + 'priceable_type' => $purchasable->getMorphClass(), + 'priceable_id' => $purchasable->id, + ]); + + $cart->lines()->create([ + 'purchasable_type' => $purchasable->getMorphClass(), + 'purchasable_id' => $purchasable->id, + 'quantity' => 2, + ]); + + $discount = Discount::factory()->create([ + 'type' => AmountOff::class, + 'name' => 'Ten off', + 'coupon' => 'SAVE10', + 'uses' => 0, + 'max_uses' => 1, + 'data' => [ + 'fixed_value' => true, + 'fixed_values' => [ + $currency->code => 500, + ], + ], + ]); + + $discount->customerGroups()->sync([ + $customerGroup->id => ['enabled' => true, 'starts_at' => now()->subHour()], + ]); + + $discount->channels()->sync([ + $channel->id => ['enabled' => true, 'starts_at' => now()->subHour()], + ]); + + $cart->calculate(); + + $orderA = (new CreateOrder)->execute($cart)->then(fn ($order) => $order->refresh()); + + expect($orderA->discount_total->value)->toEqual(500); + expect($discount->refresh()->uses)->toEqual(1); + + // Priced again on the same instance, with the discount set rebuilt: the + // cart's own use must not read as an exhausted coupon, or the retry is + // re-priced without the discount the shopper was quoted. + Discounts::resetDiscounts(); + + $cart->recalculate(); + + $orderB = (new CreateOrder)->execute($cart)->then(fn ($order) => $order->refresh()); + + expect($orderB->id)->toEqual($orderA->id); + expect($orderB->discount_total->value)->toEqual(500); expect($discount->refresh()->uses)->toEqual(1); });