From 40f78187571b77b21119751e85b86af61690d7da Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 11:45:50 +0200 Subject: [PATCH] Fix round 1: give the frozen-booking test real teeth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Important findings from review, both against the plan's own text. The frozen-booking test could not fail for a defect this diff introduces. Digging into why turned up two problems, not one. The two assertions the review named were never exercised: sync-catalogue never writes subscription_addons.stripe_price_id, and the orphan planted for the new figure sits at a different amount than the booking's own price, so archiveSuperseded's net_cents scoping was never going to reach it either. Underneath that, the test's own orphan was inert regardless of any of that: with no StripeAddonPrice row seeded first, AddonPrices::product() minted its own Stripe Product instead of reusing 'prod_support', so the competing orphan planted under that literal id sat on a product nothing ever asked about. Fixed both — seeded the product the way the first two tests in this file already do, added the three assertions the review specified, and a comment on what the two original assertions do and do not prove. Verified the fix actually bites, since a test whose teeth are asserted but never demonstrated is what produced this finding in the first place. With just the amount check in AdoptStripePrice disabled, the booking still survived: the `claimed` callback this diff wires into AddonPrices::ensure() caught it as a second, independent guard. Only disabling both together reproduced the failure — the honest demonstration, not the single-line sabotage first suggested, which this diff's own defense-in-depth absorbs. The remember() comment credited a unique index stripe_addon_prices does not have — that index sits on stripe_plan_prices, the plan side. Reworded to name what actually keeps two rows off one Price id today (the `claimed` callback), what 2026_07_31_210000_one_row_per_stripe_price adds as a net under it, and why two rows sharing one Price is dangerous: archiving one — the ordinary response to a superseded figure — would withdraw the very Price the other row is still selling. Co-Authored-By: Claude Opus 5 --- app/Services/Billing/AddonPrices.php | 20 ++++++--- .../Billing/StripePriceAdoptionTest.php | 44 ++++++++++++++++++- 2 files changed, 58 insertions(+), 6 deletions(-) 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); });