diff --git a/app/Services/Billing/PlanPrices.php b/app/Services/Billing/PlanPrices.php index 03c93af..5d58ce7 100644 --- a/app/Services/Billing/PlanPrices.php +++ b/app/Services/Billing/PlanPrices.php @@ -172,11 +172,18 @@ final class PlanPrices 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. + // Says "I have already sent this call", nothing more. 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. 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, not this key — + // Stripe forgets a key after twenty-four 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 6066ed5..f5c3caa 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -401,3 +401,52 @@ it('does not take a package price belonging to another catalogue row', function // exactly the failure a wrong plan_price_id must never cause. ->and($this->stripe->archived)->not->toContain('price_other_row'); }); + +it('does not adopt a package price that only proves the family, not this row', function () { + // The whole reason the plan side needs `identifying: ['plan_price_id']` + // rather than the module side's simpler key: one Product carries a Price + // for every version, term AND treatment of a family, so a Price that + // proves only the FAMILY could be any of them. If `identifying` were ever + // loosened to ['plan_family'], a Price like the one planted below would + // wrongly confirm — nothing else here would catch it. + 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'; + + // This row's OWN domestic Price is a legitimate orphan the moment its + // register row is gone — same reason as the two tests above — and would + // otherwise be correctly re-adopted before the planted Price below is + // ever weighed, masking the very rejection this test means to prove. + $this->stripe->archivePrice((string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id')); + + StripePlanPrice::query()->where('plan_price_id', $row->id)->delete(); + + // Right product, right money, right currency, right interval, and the + // right FAMILY — the one thing missing is plan_price_id, which is the + // only key that says which of the family's many rows a Price belongs to. + $this->stripe->plantPrice('price_family_only', $product, $charged, (string) $row->currency, + $interval, ['plan_family' => $row->version->family->key]); + + // Same reason as the two tests above: with the sync's own idempotency + // key still in force, createPrice() would replay this row's own previous + // Price rather than genuinely minting, leaving nothing for the + // assertions below to catch. + $this->stripe->keys = []; + + $before = count($this->stripe->prices); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->not->toBe('price_family_only') + // A fresh Price was minted rather than the family-only Price being + // handed out — this is the assertion `identifying: ['plan_price_id']` + // exists to keep true; swap it for ['plan_family'] and this fails. + ->and(count($this->stripe->prices))->toBe($before + 1) + ->and($this->stripe->archived)->not->toContain('price_family_only'); +});