From 40532c6e0267aaf040104d42da2b08a52aebde20 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:04:25 +0200 Subject: [PATCH] Put the money gate in the class that promises it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdoptStripePrice says in its own docblock that no adoption can move money, and could compare three properties. The four that also decide what a Price charges were filtered in HttpStripeClient — where FakeStripeClient cannot express them, so no test of the class could reach the promise it makes, and a third client implementation would drop the check in silence. The client reports them now and the caller decides. Absent keys stay Stripe's defaults, which is the direction that matters: read the other way round the gate refuses every legitimate price and recognition becomes a no-op nothing alerts on. And the unexplained-orphan warning is a state, not an event. It fired on every sweep and every customer booking for as long as the orphan existed; once a day per price is enough to act on. Co-Authored-By: Claude Opus 5 --- app/Services/Billing/AdoptStripePrice.php | 39 +++++++-- app/Services/Stripe/FakeStripeClient.php | 33 +++++--- app/Services/Stripe/HttpStripeClient.php | 45 +++------- app/Services/Stripe/StripeClient.php | 45 ++++------ .../Billing/StripeIdempotencyKeyTest.php | 82 +++++++------------ .../Billing/StripePriceAdoptionTest.php | 58 +++++++++++++ 6 files changed, 171 insertions(+), 131 deletions(-) diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 41461d5..1e7d4b7 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -3,6 +3,7 @@ namespace App\Services\Billing; use App\Services\Stripe\StripeClient; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; /** @@ -68,9 +69,22 @@ final class AdoptStripePrice $candidates = []; foreach ($this->stripe->activePricesFor($productId) as $price) { + // The money gate, and the whole of it — this class promises that no + // adoption can move money, and until these four moved here it could + // compare only the first three. A Price at our figure with + // interval_count 3 bills quarterly; one with transform_quantity + // divide_by 10 charges a customer holding three packs for one; a + // metered or tiered one does not charge an amount at all. Stripe's + // dashboard copies metadata when it duplicates a Price, so such a + // Price can carry our own identifying key and be indistinguishable + // from ours by every other test here. if ($price['unit_amount'] !== $amountCents || $price['currency'] !== strtoupper($currency) - || $price['interval'] !== $interval) { + || $price['interval'] !== $interval + || $price['interval_count'] !== 1 + || $price['usage_type'] !== 'licensed' + || $price['transform_quantity'] + || $price['billing_scheme'] !== 'per_unit') { continue; } @@ -86,13 +100,22 @@ final class AdoptStripePrice } if (! $this->confirms($price['metadata'], $metadata, $identifying)) { - Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ - 'price' => $price['id'], - 'product' => $productId, - 'amount_cents' => $amountCents, - 'currency' => strtoupper($currency), - 'interval' => $interval, - ]); + // Once a day per Price. An orphan nobody can adopt is a STATE, + // not an event, and this runs in a customer's module booking as + // well as in the sweep — without a throttle it writes a line + // every time anybody books anything, for as long as the orphan + // exists, and the clean-up command that would end it is a + // separate one. Cache::add is this repo's throttle; see + // App\Livewire\Billing. + if (Cache::add('stripe:unexplained-price:'.$price['id'], true, now()->addDay())) { + Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ + 'price' => $price['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + } continue; } diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 25d4ebc..262f8d7 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -19,7 +19,7 @@ class FakeStripeClient implements StripeClient /** @var array */ public array $products = []; - /** @var array */ + /** @var array */ public array $prices = []; /** @var array */ @@ -218,6 +218,13 @@ class FakeStripeClient implements StripeClient 'amount' => $amountCents, 'currency' => $currency, 'interval' => $interval, + // Stripe's defaults, and the only thing createPrice() can produce: + // it sends none of these four, so every Price this platform mints + // carries exactly them. + 'interval_count' => 1, + 'usage_type' => 'licensed', + 'transform_quantity' => false, + 'billing_scheme' => 'per_unit', 'metadata' => $metadata, // Stripe stamps every object with its creation time and // AdoptStripePrice takes the OLDEST of several orphans. Counted @@ -258,15 +265,6 @@ class FakeStripeClient implements StripeClient // No failIfAsked(): createPrice(), the call this one stands in front of, // does not fail either, and a test that scripts an outage for a refund // must not have its catalogue sync change behaviour underneath it. - // - // None of HttpStripeClient's charging-property filter either — the four - // fields it names (interval_count, usage_type, transform_quantity, - // billing_scheme). This fake only ever holds prices its own - // createPrice()/plantPrice() put here, and neither takes an argument for - // any of them: there is nothing non-standard a test could plant, so the - // filter would have nothing to do. Not an oversight. What that filter - // does is proven where it lives, over Http::fake — see - // tests/Feature/Billing/StripeIdempotencyKeyTest.php. $found = []; foreach ($this->prices as $id => $price) { @@ -279,6 +277,10 @@ class FakeStripeClient implements StripeClient 'unit_amount' => $price['amount'], 'currency' => strtoupper($price['currency']), 'interval' => $price['interval'], + 'interval_count' => $price['interval_count'], + 'usage_type' => $price['usage_type'], + 'transform_quantity' => $price['transform_quantity'], + 'billing_scheme' => $price['billing_scheme'], 'created' => $price['created'], 'metadata' => $price['metadata'], ]; @@ -312,12 +314,23 @@ class FakeStripeClient implements StripeClient string $interval, array $metadata = [], int $created = 1, + int $intervalCount = 1, + string $usageType = 'licensed', + bool $transformQuantity = false, + string $billingScheme = 'per_unit', ): void { $this->prices[$id] = [ 'product' => $productId, 'amount' => $amountCents, 'currency' => $currency, 'interval' => $interval, + // Defaulted to what we could have minted ourselves, so a test that + // does not care says nothing — and a test about the money gate says + // exactly which property it is about. + 'interval_count' => $intervalCount, + 'usage_type' => $usageType, + 'transform_quantity' => $transformQuantity, + 'billing_scheme' => $billingScheme, 'metadata' => $metadata, 'created' => $created, ]; diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index fc90f32..b90cbe9 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -170,43 +170,24 @@ class HttpStripeClient implements StripeClient foreach ($data as $price) { $recurring = (array) ($price['recurring'] ?? []); - // Only prices we could have minted ourselves. createPrice() sends - // product, unit_amount, currency, recurring[interval] and metadata - // and nothing else, so Stripe defaults every other field that - // decides what a Price CHARGES — and anything but those defaults - // is a Price this platform did not write. Skipped here rather than - // returned, because AdoptStripePrice compares amount, currency and - // interval only and trusts that triple as the whole of what a - // Price charges: - // - // - interval_count multiplies the period — 3 bills every three - // months — while `interval` still reads 'month'; - // - usage_type other than 'licensed' bills metered usage; - // - a transform_quantity divides the quantity before charging, - // and modules are billed BY quantity — SyncStripeAddonItems - // sums a pack into one item at quantity n — so divide_by 10 - // would charge a customer holding three for one; - // - billing_scheme 'tiered' keeps the money in tiers, leaving - // unit_amount null. Read as 0 below, so such a Price fails the - // amount match only while the caller's own figure is not 0 — - // and PlanPrices::ensure() has no zero guard, so that is not a - // condition to rest on. - // - // An ABSENT key is Stripe's default and no reason to reject. - // Read the other way round this filter would refuse every - // legitimate price and turn recognition into a permanent no-op. - if ((int) ($recurring['interval_count'] ?? 1) !== 1 - || ($recurring['usage_type'] ?? 'licensed') !== 'licensed' - || (array) ($price['transform_quantity'] ?? []) !== [] - || ($price['billing_scheme'] ?? 'per_unit') !== 'per_unit') { - continue; - } - $prices[] = [ 'id' => (string) ($price['id'] ?? ''), 'unit_amount' => (int) ($price['unit_amount'] ?? 0), 'currency' => strtoupper((string) ($price['currency'] ?? '')), 'interval' => (string) ($recurring['interval'] ?? ''), + // The four properties that decide what a Price CHARGES + // beyond its amount. createPrice() sends none of them, so + // Stripe defaults every one — and an ABSENT key is that + // default, never a reason to reject. Reported rather than + // filtered: the class that promises adoption cannot move + // money is AdoptStripePrice, and it could not keep that + // promise while these lived here, where FakeStripeClient + // cannot express them and no test of that class could reach + // them. + 'interval_count' => (int) ($recurring['interval_count'] ?? 1), + 'usage_type' => (string) ($recurring['usage_type'] ?? 'licensed'), + 'transform_quantity' => (array) ($price['transform_quantity'] ?? []) !== [], + 'billing_scheme' => (string) ($price['billing_scheme'] ?? 'per_unit'), 'created' => (int) ($price['created'] ?? 0), 'metadata' => array_map( fn ($value) => (string) $value, diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index 0ffed51..ab7908d 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -125,39 +125,30 @@ interface StripeClient ): string; /** - * Every active recurring Price of a Product, as Stripe holds them. + * Every active recurring Price of a Product, as Stripe holds them, with the + * properties that decide what each one CHARGES. * - * The half of the catalogue mirror that was missing: nothing here ever asked - * Stripe what it already had. A run that created a Price and died before the - * row was written left an orphan our table never learned about, and once the - * idempotency key expired the next run made a SECOND live Price for the same - * money — the exact duplicate the key exists to prevent. See - * App\Services\Billing\AdoptStripePrice, which is the only caller. + * `interval_count` multiplies the period — 3 bills every three months while + * `interval` still reads 'month'. `usage_type` other than 'licensed' bills + * metered usage. `transform_quantity` divides the quantity before charging, + * and modules are billed BY quantity, so a divide_by of 10 would charge a + * customer holding three for one. `billing_scheme` 'tiered' keeps the money + * in tiers and leaves `unit_amount` null, which reads as 0 here. + * + * All four are REPORTED, not filtered. Which of them is acceptable is the + * caller's decision, and the caller is App\Services\Billing\AdoptStripePrice + * — the class whose docblock promises that adoption cannot move money. That + * promise has to be testable in the class that makes it, and it was not + * while these fields were dropped here: FakeStripeClient cannot express + * them. + * + * An absent key is Stripe's own default — 1, 'licensed', none, 'per_unit'. * * Active ones only, because the question being asked is "is there something * here I can sell on?". `currency` comes back UPPER CASE, the way our own * tables hold it. * - * FOUR properties are checked, and they are named because the list is what - * the contract is: `recurring.interval_count` is 1, `recurring.usage_type` - * is `licensed`, there is no `transform_quantity`, and `billing_scheme` is - * `per_unit`. Those are Stripe's own defaults, and createPrice() sets none of - * the four — so anything else is a Price this platform did not write, and is - * filtered out here rather than returned. - * - * The reason it is filtered at all: AdoptStripePrice compares the amount, - * currency and interval of the shape below and trusts that triple as the - * WHOLE of what a Price charges. A Price billed every three months, on - * metered usage, dividing the quantity before charging, or pricing in tiers - * would otherwise pass that triple on `interval => 'month'` alone and be - * adopted as if it charged our figure per unit per month — which is adoption - * moving money, the one thing nothing here may do. - * - * Not a list of everything Stripe can put on a Price: it is the list of - * fields known to change what one CHARGES. A fifth would have to be added - * here, at the boundary, before the shape below is handed on. - * - * @return array}> + * @return array}> */ public function activePricesFor(string $productId): array; diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index ead4498..47686e0 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -155,6 +155,13 @@ it('pages through every active price of a product', function () { // side is which. 'currency' => 'EUR', 'interval' => 'month', + // Stripe's own defaults, absent from every price in this response — + // reported here rather than filtered, which is the whole point of + // this test's neighbour below. + 'interval_count' => 1, + 'usage_type' => 'licensed', + 'transform_quantity' => false, + 'billing_scheme' => 'per_unit', 'created' => 100, 'metadata' => ['addon' => 'priority_support'], ]) @@ -170,73 +177,40 @@ it('pages through every active price of a product', function () { Http::assertSent(fn ($request) => str_contains($request->url(), 'active=true')); }); -it('skips a price it could not have created itself, so the money gate never trusts a partial recurrence', function () { +it('reports the properties that decide what a price charges, and filters none of them', function () { Http::fake([ 'api.stripe.com/*' => Http::response([ 'data' => [ - ['id' => 'price_monthly', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 100, 'recurring' => ['interval' => 'month'], - 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], 'metadata' => []], ['id' => 'price_quarterly', 'unit_amount' => 3480, 'currency' => 'eur', 'created' => 101, 'recurring' => ['interval' => 'month', 'interval_count' => 3], - 'metadata' => ['addon' => 'priority_support']], - ['id' => 'price_metered', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 102, 'recurring' => ['interval' => 'month', 'usage_type' => 'metered'], - 'metadata' => ['addon' => 'priority_support']], - ], - 'has_more' => false, - ]), - ]); - - $prices = (new HttpStripeClient)->activePricesFor('prod_1'); - - // A hand-duplicated dashboard price keeps our metadata, so nothing - // downstream could tell it apart, and the module would bill quarterly. - // - // None of the three planted prices carries `transform_quantity` or - // `billing_scheme` at all, and the ordinary one still comes back — so this - // also holds the other direction of the next test's filter: an absent key is - // Stripe's default, not a reason to reject. - expect(collect($prices)->pluck('id')->all())->toBe(['price_monthly']); -}); - -it('skips a price that would charge our figure for the wrong quantity', function () { - Http::fake([ - 'api.stripe.com/*' => Http::response([ - 'data' => [ - // Spelled out as Stripe actually sends them for an ordinary - // price: transform_quantity null, billing_scheme per_unit. - ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 100, 'recurring' => ['interval' => 'month'], - 'transform_quantity' => null, 'billing_scheme' => 'per_unit', - 'metadata' => ['addon' => 'priority_support']], + 'metadata' => []], ['id' => 'price_divided', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 101, 'recurring' => ['interval' => 'month'], - 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], - 'billing_scheme' => 'per_unit', - 'metadata' => ['addon' => 'priority_support']], - ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', 'created' => 102, 'recurring' => ['interval' => 'month'], - 'billing_scheme' => 'tiered', - 'metadata' => ['addon' => 'priority_support']], + 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], 'metadata' => []], + ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', + 'created' => 103, 'recurring' => ['interval' => 'month', 'usage_type' => 'metered'], + 'billing_scheme' => 'tiered', 'metadata' => []], ], 'has_more' => false, ]), ]); - $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + $prices = collect((new HttpStripeClient)->activePricesFor('prod_1'))->keyBy('id'); - // The concrete harm: modules are billed BY quantity — SyncStripeAddonItems - // sums a pack into ONE item at quantity n — so price_divided is our exact - // figure, on our Product, carrying our `addon` key, and adoption would take - // it. A customer holding three would then be charged ceil(3/10) = 1. - // - // price_tiered is refused by name rather than by luck: Stripe reports - // unit_amount null for a tiered price, activePricesFor() casts that to 0, and - // the amount match alone only rejects it while the caller's own figure is not - // 0 — which PlanPrices::ensure(), unlike AddonPrices::ensure(), does not - // guarantee. - expect(collect($prices)->pluck('id')->all())->toBe(['price_ordinary']); + // All four come back. Deciding which are usable is AdoptStripePrice's job — + // it is the class that promises adoption cannot move money, and it could not + // keep that promise while the fields lived only here. + expect($prices)->toHaveCount(4) + ->and($prices['price_ordinary'])->toMatchArray([ + 'interval_count' => 1, 'usage_type' => 'licensed', + 'transform_quantity' => false, 'billing_scheme' => 'per_unit', + ]) + ->and($prices['price_quarterly']['interval_count'])->toBe(3) + ->and($prices['price_divided']['transform_quantity'])->toBeTrue() + ->and($prices['price_tiered']['usage_type'])->toBe('metered') + ->and($prices['price_tiered']['billing_scheme'])->toBe('tiered'); }); it('writes metadata onto a price that already exists', function () { diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index df68ab1..ca10ff2 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -610,3 +610,61 @@ it('keeps only the lowest-id row in every group of duplicates, and leaves the re 'created_at' => now(), 'updated_at' => now(), ]))->toThrow(UniqueConstraintViolationException::class); }); + +it('refuses a price whose recurrence is not one we could have minted', function () { + // The money gate, asked of AdoptStripePrice itself rather than of the + // client's filter. Until this moved, these four properties were rejected + // inside HttpStripeClient — and FakeStripeClient could not express them, so + // no test of this class could reach the promise its docblock makes. + $this->stripe->plantPrice('price_quarterly', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), intervalCount: 3); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that meters usage instead of selling a licence', function () { + $this->stripe->plantPrice('price_metered', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), usageType: 'metered'); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that divides the quantity before charging', function () { + // Modules are billed BY quantity — SyncStripeAddonItems sums a pack into one + // item at quantity n — so a divide_by price would charge a customer holding + // three for one. + $this->stripe->plantPrice('price_divided', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), transformQuantity: true); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that keeps its money in tiers', function () { + $this->stripe->plantPrice('price_tiered', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), billingScheme: 'tiered'); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('still adopts a price that carries every one of Stripe\'s defaults', function () { + // The direction that matters more than the four above: read the wrong way + // round, the gate refuses every legitimate price and recognition becomes a + // permanent no-op that nothing would ever alert on. + $this->stripe->plantPrice('price_ordinary', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBe('price_ordinary'); +}); + +it('warns about an unexplained price once a day, not once a booking', function () { + Log::spy(); + + $this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []); + + // ensure() runs inside a customer's module booking, so without a throttle + // this line is written every time anybody books anything. + adoptModulePrice(); + adoptModulePrice(); + adoptModulePrice(); + + Log::shouldHaveReceived('warning')->once(); +});