diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index fd2dd1d..41461d5 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -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); } diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 47efe93..311b188 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -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) { diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index 8926f39..2058442 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -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, diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index 1e06947..45e6d30 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -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}> */ public function activePricesFor(string $productId): array; diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index ef93691..3ea6478 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -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'])]); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index 68690d1..c3d47a1 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -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());