From 355f9cce0102e025ccf43c94f4cfa0831abd48d9 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 12:09:44 +0200 Subject: [PATCH] Ask before minting a package price Same defect, same fix, one difference: a family Product carries every version and every term, so the amount alone does not say which row a Price belongs to. plan_price_id in the metadata does, and it is what has to agree before a Price is taken over. Co-Authored-By: Claude Opus 5 --- app/Services/Billing/PlanPrices.php | 62 +++++++++++++------ .../Billing/StripePriceAdoptionTest.php | 54 ++++++++++++++++ 2 files changed, 96 insertions(+), 20 deletions(-) diff --git a/app/Services/Billing/PlanPrices.php b/app/Services/Billing/PlanPrices.php index ed8d631..03c93af 100644 --- a/app/Services/Billing/PlanPrices.php +++ b/app/Services/Billing/PlanPrices.php @@ -40,7 +40,10 @@ use App\Services\Stripe\StripeClient; */ final class PlanPrices { - public function __construct(private readonly StripeClient $stripe) {} + public function __construct( + private readonly StripeClient $stripe, + private readonly AdoptStripePrice $adopt, + ) {} /** What Stripe is asked to take for this row from a customer treated so. */ public static function chargedCents(PlanPrice $price, TaxTreatment $treatment): int @@ -131,30 +134,49 @@ final class PlanPrices $existing = $this->registered($price, $treatment, includeArchived: true); $priceId = $existing?->stripe_price_id; + $interval = $price->term === Subscription::TERM_YEARLY ? 'year' : 'month'; if ($priceId === null) { - $priceId = $this->stripe->createPrice( + $metadata = [ + 'plan_family' => $family->key, + 'plan_version' => (string) $version->version, + 'plan_version_id' => (string) $version->id, + 'plan_price_id' => (string) $price->id, + // Read back by anything that has a Price id and needs to know + // what kind of sale it was — and by a person looking at + // Stripe's own dashboard, where two Prices on one Product + // would otherwise differ only by an amount. + 'tax_treatment' => $treatment->reverseCharge ? 'reverse_charge' : 'domestic', + ]; + + // Asked BEFORE minting: a run that died between Stripe's create and + // our insert left a Price the register does not know, and the key + // below stops protecting it after twenty-four hours. `plan_price_id` + // is what proves such a Price is this row's — one Product carries + // every version and term of a family, so the amount alone would not. + $priceId = ($this->adopt)( productId: (string) $productId, amountCents: $charged, currency: (string) $price->currency, - interval: $price->term === Subscription::TERM_YEARLY ? 'year' : 'month', - metadata: [ - 'plan_family' => $family->key, - 'plan_version' => (string) $version->version, - 'plan_version_id' => (string) $version->id, - 'plan_price_id' => (string) $price->id, - // Read back by anything that has a Price id and needs to know - // what kind of sale it was — and by a person looking at - // Stripe's own dashboard, where two Prices on one Product - // would otherwise differ only by an amount. - 'tax_treatment' => $treatment->reverseCharge ? 'reverse_charge' : 'domestic', - ], - // The CHARGED amount is part of the key, so a run after a rate - // change cannot replay the Price minted at the old figure. So is - // the treatment, and it has to be: at a rate of nought the two - // Prices are the same amount, and one key would have Stripe hand - // back the same object for both — which the register, where a - // Price id is unique, would refuse to record twice. + interval: $interval, + metadata: $metadata, + identifying: ['plan_price_id'], + claimed: fn (string $id) => StripePlanPrice::query() + ->where('stripe_price_id', $id) + ->exists(), + ); + + $priceId ??= $this->stripe->createPrice( + productId: (string) $productId, + amountCents: $charged, + currency: (string) $price->currency, + interval: $interval, + metadata: $metadata, + // Says "I have already sent this call", nothing more — the + // metadata is folded in by IdempotencyKey inside the client, so + // changing the format above can never again refuse the call for + // a day. What stops a second Price for one figure is the + // adoption step above; Stripe forgets a key after 24 hours. idempotencyKey: "clupilot-price-{$price->id}-{$charged}" .($treatment->reverseCharge ? '-rc' : ''), ); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index cfeabf3..e552943 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -1,10 +1,13 @@ toBe($sold); }); + +it('takes over an orphaned package price', function () { + // A catalogue mirrored once, so families have Products and rows have Prices. + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + + // The register loses its row and Stripe keeps the Price: a run that died + // between the two, seen from the next run's point of view. + $orphan = (string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id'); + StripePlanPrice::query()->where('stripe_price_id', $orphan)->delete(); + + $before = count($this->stripe->prices); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->toBe($orphan) + ->and(count($this->stripe->prices))->toBe($before) + ->and(StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->where('charged_cents', $charged) + ->value('stripe_price_id'))->toBe($orphan) + // The pointer for the ordinary domestic sale is written as before. + ->and((string) $row->refresh()->stripe_price_id)->toBe($orphan); +}); + +it('does not take a package price belonging to another catalogue row', function () { + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $product = (string) $row->version->family->stripe_product_id; + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + $interval = $row->term === Subscription::TERM_YEARLY ? 'year' : 'month'; + + StripePlanPrice::query()->where('plan_price_id', $row->id)->delete(); + + // Same product, same money, same interval — and plan_price_id says it is a + // DIFFERENT row's Price. One Product carries every version and term of a + // family, so this is the ordinary case, not an exotic one. + $this->stripe->plantPrice('price_other_row', $product, $charged, (string) $row->currency, + $interval, ['plan_price_id' => (string) ($row->id + 1000), 'tax_treatment' => 'domestic']); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->not->toBe('price_other_row'); +});