boban
/
CluPilotCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Put the money gate in the class that promises it 

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 <noreply@anthropic.com>
feature/host-bootstrap
nexxo 2026-07-30 17:04:25 +02:00
parent 7d66a01a40
commit 40532c6e02
6 changed files with 171 additions and 131 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
app/Services/Billing/AdoptStripePrice.php
View File

@ -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;
}

33
app/Services/Stripe/FakeStripeClient.php
View File

@ -19,7 +19,7 @@ class FakeStripeClient implements StripeClient
/** @var array<int, array{name: string, metadata: array}> */
public array $products = [];
/** @var array<int, array{product: string, amount: int, currency: string, interval: string, metadata: array, created: int}> */
/** @var array<int, array{product: string, amount: int, currency: string, interval: string, interval_count: int, usage_type: string, transform_quantity: bool, billing_scheme: string, metadata: array, created: int}> */
public array $prices = [];
/** @var array<int, string> */
@ -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,
];

45
app/Services/Stripe/HttpStripeClient.php
View File

@ -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,

45
app/Services/Stripe/StripeClient.php
View File

@ -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<int, array{id: string, unit_amount: int, currency: string, interval: string, created: int, metadata: array<string, string>}>
* @return array<int, array{id: string, unit_amount: int, currency: string, interval: string, interval_count: int, usage_type: string, transform_quantity: bool, billing_scheme: string, created: int, metadata: array<string, string>}>
*/
public function activePricesFor(string $productId): array;

82
tests/Feature/Billing/StripeIdempotencyKeyTest.php
View File

@ -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 () {

58
tests/Feature/Billing/StripePriceAdoptionTest.php
View File

@ -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();
});