diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 1e7d4b7..3c1e8ce 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -127,7 +127,9 @@ final class AdoptStripePrice return null; } - usort($candidates, fn (array $a, array $b) => $a['created'] <=> $b['created']); + // Oldest first, and on a tie the lower id — so the same two orphans give + // the same answer on every run, whatever order Stripe lists them in. + usort($candidates, fn (array $a, array $b) => [$a['created'], $a['id']] <=> [$b['created'], $b['id']]); $adopted = array_shift($candidates); diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 262f8d7..1db81fa 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -231,9 +231,11 @@ class FakeStripeClient implements StripeClient // rather than clocked, because two Prices minted in the same second // would share a timestamp — but the two counters are independent: // plantPrice() stamps `created: 1` of its own accord, which ties with - // a Price minted here while this list was still empty. Any test whose - // outcome depends on which of two prices is older passes `created:` - // itself, as the adoption tests do. + // a Price minted here while this list was still empty. A tie is + // possible, not merely theoretical — which is why AdoptStripePrice's + // usort breaks one by price id rather than leaving it to Stripe's + // list order, and a test can plant two prices at the same `created` + // on purpose to prove it. 'created' => count($this->prices) + 1, ]; @@ -282,7 +284,10 @@ class FakeStripeClient implements StripeClient 'transform_quantity' => $price['transform_quantity'], 'billing_scheme' => $price['billing_scheme'], 'created' => $price['created'], - 'metadata' => $price['metadata'], + // Cast like the wire does: Stripe hands metadata back as + // strings, and an un-cast int here would let a test pass where + // production's strict === fails. + 'metadata' => array_map(fn ($value) => (string) $value, $price['metadata']), ]; } @@ -292,7 +297,14 @@ class FakeStripeClient implements StripeClient public function updatePriceMetadata(string $priceId, array $metadata): void { if (isset($this->prices[$priceId])) { - $this->prices[$priceId]['metadata'] = $metadata; + // Merged, not replaced — Stripe leaves a key you do not send where + // it is. A fake that replaced would let a test prove the opposite of + // production, and "write only when it differs" in AdoptStripePrice + // is built around exactly this merging. + $this->prices[$priceId]['metadata'] = [ + ...$this->prices[$priceId]['metadata'], + ...$metadata, + ]; } $this->metadataUpdates[] = ['price' => $priceId, 'metadata' => $metadata]; diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index b90cbe9..dc08835 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -197,7 +197,17 @@ class HttpStripeClient implements StripeClient } $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); - } while (($page['has_more'] ?? false) === true && $after !== null); + + // invoiceLines() stops here, and there that costs a short document. + // Here it costs an unseen orphan and a second live Price for one + // figure — the incident. So it is an error, not a stopping point. + if (($page['has_more'] ?? false) === true && $after === null) { + throw new RuntimeException( + 'Stripe reported another page of prices and the last item of this one carried no id; ' + .'refusing to stop half-read.' + ); + } + } while (($page['has_more'] ?? false) === true); return $prices; } diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index 47686e0..1f8a9df 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -238,3 +238,45 @@ it('lets the fake answer with the prices it holds, minus the archived ones', fun expect($found)->toContain($kept, 'price_orphan') ->and($found)->not->toContain($gone, $other); }); + +it('merges metadata onto a price the way Stripe does, rather than replacing it', function () { + $fake = new FakeStripeClient; + $fake->plantPrice('price_a', 'prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'internal_note' => 'kept']); + + $fake->updatePriceMetadata('price_a', ['tax_treatment' => 'domestic']); + + // Stripe merges: a key you do not send stays. A fake that replaced would let + // a test prove the opposite of production — and it is exactly this merging + // that AdoptStripePrice's "write only when it differs" comparison is built + // around. + expect($fake->activePricesFor('prod_1')[0]['metadata'])->toBe([ + 'addon' => 'priority_support', + 'internal_note' => 'kept', + 'tax_treatment' => 'domestic', + ]); +}); + +it('hands metadata back as strings, the way the wire does', function () { + $fake = new FakeStripeClient; + $fake->plantPrice('price_a', 'prod_1', 3480, 'EUR', 'month', ['plan_price_id' => 42]); + + // HttpStripeClient casts; the fake did not. An un-cast int is what makes + // confirms()'s strict === fail forever for that price. + expect($fake->activePricesFor('prod_1')[0]['metadata']['plan_price_id'])->toBe('42'); +}); + +it('refuses to stop half-read when Stripe says there is another page', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + // has_more, and the last item carries no id to page after. Stopping + // here leaves an orphan unseen — which is a second live Price for one + // figure, the incident this whole feature exists to end. + 'data' => [['unit_amount' => 3480, 'currency' => 'eur', 'recurring' => ['interval' => 'month']]], + 'has_more' => true, + ]), + ]); + + expect(fn () => (new HttpStripeClient)->activePricesFor('prod_1')) + ->toThrow(RuntimeException::class, 'half-read'); +}); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index ca10ff2..6a6f75f 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -668,3 +668,16 @@ it('warns about an unexplained price once a day, not once a booking', function ( Log::shouldHaveReceived('warning')->once(); }); + +it('breaks a tie on created by the price id, so two runs agree', function () { + // plantPrice defaults created to 1 and createPrice stamps count+1, so a + // planted orphan can tie with a minted price. A tie decided by Stripe's list + // order would adopt one price today and the other tomorrow. + $this->stripe->plantPrice('price_bbb', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), created: 7); + $this->stripe->plantPrice('price_aaa', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), created: 7); + + expect(adoptModulePrice())->toBe('price_aaa') + ->and($this->stripe->archived)->toBe(['price_bbb']); +});