Make the money gate compare the whole recurrence
activePricesFor() reported interval alone, never interval_count or usage_type. A Price we could not have created ourselves — billed every three months, or on metered usage — could carry our own metadata (Stripe's dashboard "duplicate price" copies it) and pass AdoptStripePrice's amount/currency/interval check on 'month' alone. Adopting it would move money onto a different recurrence, which is exactly what nothing here may do. Filtered at the boundary instead of in the predicate: HttpStripeClient now skips anything besides Stripe's own defaults (interval_count 1, usage_type licensed), so activePricesFor() can promise callers it returns only prices we could have minted. FakeStripeClient needed no filter — it only ever holds prices its own createPrice()/plantPrice() made, all standard — but says so, so the omission doesn't read as forgotten. Two more silent-failure paths, closed with the same three lines each: - The metadata-differs check was a bare !==, which is key-order sensitive and blind to Stripe's own merge. A Price carrying an extra key of its own, or metadata handed back in a different order, could never compare equal and would be rewritten every sweep forever. Compared through array_intersect_key() plus ksort() on both sides instead, so only what WE would send is what gets compared. - Metadata is normalised to strings once, up front, so an un-cast value from a caller can't make confirms() fail forever and silently turn adoption off for that Price. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>feature/betriebsmodus^2
parent
4de44e7ab0
commit
06c214e597
|
|
@ -59,6 +59,12 @@ final class AdoptStripePrice
|
|||
array $identifying,
|
||||
callable $claimed,
|
||||
): ?string {
|
||||
// Stripe hands metadata back as strings. An un-cast int here would then
|
||||
// never satisfy confirms()'s strict ===, silently disabling adoption for
|
||||
// that Price forever and warning on every sweep and every booking. One
|
||||
// cast here makes the class independent of what a caller happens to pass.
|
||||
$metadata = array_map(fn ($value) => (string) $value, $metadata);
|
||||
|
||||
$candidates = [];
|
||||
|
||||
foreach ($this->stripe->activePricesFor($productId) as $price) {
|
||||
|
|
@ -114,8 +120,20 @@ final class AdoptStripePrice
|
|||
}
|
||||
|
||||
// Only when it differs, so a sweep over a healthy catalogue makes no
|
||||
// writes at Stripe at all.
|
||||
if ($adopted['metadata'] !== $metadata) {
|
||||
// writes at Stripe at all. Compared through array_intersect_key() rather
|
||||
// than a bare !==, for two reasons: Stripe MERGES a metadata write rather
|
||||
// than replacing it, so a Price can carry a key of its own that a write
|
||||
// would never remove and a bare !== would then never call equal — and
|
||||
// PHP's array !== is key-order sensitive, so Stripe handing the same
|
||||
// values back in a different order would trigger a write every single
|
||||
// sweep. ksort() on both sides settles the order; the intersect settles
|
||||
// the extra key, by looking only at what WE would send.
|
||||
$overlap = array_intersect_key($adopted['metadata'], $metadata);
|
||||
$wanted = $metadata;
|
||||
ksort($overlap);
|
||||
ksort($wanted);
|
||||
|
||||
if ($overlap !== $wanted) {
|
||||
$this->stripe->updatePriceMetadata($adopted['id'], $metadata);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -253,6 +253,12 @@ 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.
|
||||
//
|
||||
// No interval_count/usage_type filter either, unlike HttpStripeClient's:
|
||||
// this fake only ever holds prices its own createPrice()/plantPrice() put
|
||||
// here, and neither lets a caller set anything but a standard monthly or
|
||||
// yearly recurrence — there is nothing non-standard a test could plant,
|
||||
// so the filter would have nothing to do. Not an oversight.
|
||||
$found = [];
|
||||
|
||||
foreach ($this->prices as $id => $price) {
|
||||
|
|
|
|||
|
|
@ -168,11 +168,25 @@ class HttpStripeClient implements StripeClient
|
|||
$data = (array) ($page['data'] ?? []);
|
||||
|
||||
foreach ($data as $price) {
|
||||
$recurring = (array) ($price['recurring'] ?? []);
|
||||
|
||||
// Only prices we could have minted ourselves: createPrice() never
|
||||
// sets either field, so Stripe defaults both, and anything else is
|
||||
// not a standard monthly/yearly Price. Skipped here rather than
|
||||
// returned, because AdoptStripePrice trusts the amount/currency/
|
||||
// interval triple as the WHOLE recurrence — a Price billed every
|
||||
// three months, or on metered usage, would otherwise pass that
|
||||
// triple on 'month' alone and be adopted as if it billed monthly.
|
||||
if ((int) ($recurring['interval_count'] ?? 1) !== 1
|
||||
|| ($recurring['usage_type'] ?? 'licensed') !== 'licensed') {
|
||||
continue;
|
||||
}
|
||||
|
||||
$prices[] = [
|
||||
'id' => (string) ($price['id'] ?? ''),
|
||||
'unit_amount' => (int) ($price['unit_amount'] ?? 0),
|
||||
'currency' => strtoupper((string) ($price['currency'] ?? '')),
|
||||
'interval' => (string) ($price['recurring']['interval'] ?? ''),
|
||||
'interval' => (string) ($recurring['interval'] ?? ''),
|
||||
'created' => (int) ($price['created'] ?? 0),
|
||||
'metadata' => array_map(
|
||||
fn ($value) => (string) $value,
|
||||
|
|
|
|||
|
|
@ -138,6 +138,16 @@ interface StripeClient
|
|||
* here I can sell on?". `currency` comes back UPPER CASE, the way our own
|
||||
* tables hold it.
|
||||
*
|
||||
* Only prices WE could have created ourselves. createPrice() never sets
|
||||
* `recurring.interval_count` or `recurring.usage_type`, so anything besides
|
||||
* Stripe's own defaults for them — one, and `licensed` — is filtered out
|
||||
* here rather than returned. AdoptStripePrice trusts the amount/currency/
|
||||
* interval triple in the shape below as the WHOLE of a Price's recurrence;
|
||||
* a Price billed every three months, or on metered usage, would otherwise
|
||||
* pass that triple on `interval => 'month'` alone and be adopted as if it
|
||||
* billed monthly — which is adoption moving money, the one thing nothing
|
||||
* here may do.
|
||||
*
|
||||
* @return array<int, array{id: string, unit_amount: int, currency: string, interval: string, created: int, metadata: array<string, string>}>
|
||||
*/
|
||||
public function activePricesFor(string $productId): array;
|
||||
|
|
|
|||
|
|
@ -170,6 +170,31 @@ 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 () {
|
||||
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_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.
|
||||
expect(collect($prices)->pluck('id')->all())->toBe(['price_monthly']);
|
||||
});
|
||||
|
||||
it('writes metadata onto a price that already exists', function () {
|
||||
Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_a'])]);
|
||||
|
||||
|
|
|
|||
|
|
@ -72,6 +72,21 @@ it('leaves the metadata alone when it already says the right thing', function ()
|
|||
->and($this->stripe->metadataUpdates)->toBe([]);
|
||||
});
|
||||
|
||||
it('leaves the metadata alone when Stripe merged it in a different order plus a key of its own', function () {
|
||||
// plantPrice() stores our own array in our own order, so the test above
|
||||
// cannot catch an order- or merge-sensitive comparison: Stripe returns
|
||||
// metadata as the result of a MERGE, never a replace, in whatever key
|
||||
// order it likes — and a write never removes a key it did not send.
|
||||
$this->stripe->plantPrice('price_ok', 'prod_support', 3480, 'EUR', 'month', [
|
||||
'tax_treatment' => 'domestic',
|
||||
'addon' => 'priority_support',
|
||||
'internal_note' => 'added by hand in the Stripe dashboard',
|
||||
]);
|
||||
|
||||
expect(adoptModulePrice())->toBe('price_ok')
|
||||
->and($this->stripe->metadataUpdates)->toBe([]);
|
||||
});
|
||||
|
||||
it('adopts nothing when the amount, currency or interval differ', function () {
|
||||
$this->stripe->plantPrice('price_cheaper', 'prod_support', 2900, 'EUR', 'month', moduleMetadata());
|
||||
$this->stripe->plantPrice('price_yearly', 'prod_support', 3480, 'EUR', 'year', moduleMetadata());
|
||||
|
|
|
|||
Loading…
Reference in New Issue