diff --git a/packages/core/src/Actions/Carts/CreateOrder.php b/packages/core/src/Actions/Carts/CreateOrder.php index d73ede1b7b..30f7cc24c0 100644 --- a/packages/core/src/Actions/Carts/CreateOrder.php +++ b/packages/core/src/Actions/Carts/CreateOrder.php @@ -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 860dd3b837..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'); @@ -291,20 +296,46 @@ 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. Order creation is the + * only thing that changes the answer, so it forgets the memo. + * + * @see self::forgetConsumedDiscountIds() */ public function consumedDiscountIds(): Collection { + 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() ->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 bda747f239..ba78068991 100644 --- a/tests/core/Unit/Actions/Carts/CreateOrderTest.php +++ b/tests/core/Unit/Actions/Carts/CreateOrderTest.php @@ -618,3 +618,166 @@ 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($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); +});