diff --git a/app/Services/Billing/AddonPrices.php b/app/Services/Billing/AddonPrices.php index e1d4610..acabe91 100644 --- a/app/Services/Billing/AddonPrices.php +++ b/app/Services/Billing/AddonPrices.php @@ -315,11 +315,21 @@ final class AddonPrices } catch (UniqueConstraintViolationException) { // Two bookings of the same module landed together. Stripe replayed // one Price for both — the idempotency key saw to that — so there is - // nothing to correct here beyond letting the first row stand. The - // same catch now also covers a Price id another row already claims, - // which the unique index refuses: the caller is handed the id - // regardless, because refusing would fail a customer's booking over - // a register that is one row short. + // nothing to correct here beyond letting the first row stand. + // + // What keeps two ROWS off one Price id is the `claimed` callback in + // ensure(), asked before minting — not this table. Today + // `stripe_addon_prices` is unique on (addon_key, reverse_charge, + // amount_cents, currency, interval) only; nothing here refuses a + // second row for a Price id another row already claims. (That unique + // index is `stripe_plan_prices.stripe_price_id`, on the PLAN side — + // easy to mistake for a guarantee this table shares, and it does + // not.) 2026_07_31_210000_one_row_per_stripe_price adds a matching + // index here, as a net under `claimed` rather than instead of it: + // two rows sharing one Price is worse than a duplicate, because + // archiving one — the ordinary response to a superseded figure — + // would then withdraw the very Price the other row is still + // selling. } } } diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index 4c06251..9fe847c 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -201,6 +201,19 @@ it('does not block a booking because the metadata format moved', function () { }); it('leaves a frozen booking on the price it was sold at', function () { + // Seeds the product id AddonPrices::product() will find and reuse — same + // reason as the first two tests in this file. Without it, the sync below + // mints its own Stripe Product with a generated id, and the orphan planted + // further down (deliberately at the literal 'prod_support' Stripe uses + // throughout this file) would sit on a product nothing ever asks about, + // making it inert rather than the competing figure the test needs. + StripeAddonPrice::query()->create([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 41760, 'net_cents' => 34800, 'currency' => 'EUR', + 'interval' => 'year', 'stripe_product_id' => 'prod_support', + 'stripe_price_id' => 'price_yearly_already_known', + ]); + app(Kernel::class)->call('stripe:sync-catalogue'); $sold = app(AddonPrices::class)->liveFor( @@ -229,6 +242,35 @@ it('leaves a frozen booking on the price it was sold at', function () { app(Kernel::class)->call('stripe:sync-catalogue'); + // The two assertions below only say the booking ROW was not rewritten and + // that ITS OWN Price was not archived — true in this exact path even with + // adoption removed altogether, because stripe:sync-catalogue never writes + // subscription_addons.stripe_price_id (that is SyncStripeAddonItems, a + // separate write path) and the orphan planted above sits at a different + // amount from $sold, so archiveSuperseded's net_cents scoping was already + // never going to reach it. They stay because the untouched row is still + // worth stating, but they are not the guard. The three assertions after + // them are: they fail the moment adoption reaches for a figure other than + // the one it was asked for — the exact way a defect here would move money + // on an already-frozen booking. expect($booking->refresh()->stripe_price_id)->toBe($sold) - ->and($this->stripe->archived)->not->toContain($sold); + ->and($this->stripe->archived)->not->toContain($sold) + // The OLD figure's Price is still what a checkout for it would use — + // adoption of the orphan at the NEW figure must not have reached + // backwards and pulled the frozen figure's Price out of circulation. + ->and(app(AddonPrices::class)->liveFor( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ))->toBe($sold) + ->and(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('reverse_charge', false) + ->where('net_cents', 2900) + ->where('interval', 'month') + ->value('archived_at'))->toBeNull() + // And asking for the OLD figure again — the same call a renewal on + // this very booking would make — must still be handed $sold, not the + // orphan sitting at the new figure's amount. + ->and(app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ))->toBe($sold); });