diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index c18f3b3..f880966 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -91,9 +91,21 @@ class SyncStripeCatalogue extends Command $adoptPrices = app(AdoptStripePrice::class); $adoptProducts = app(AdoptStripeProduct::class); $adoptedPricesBefore = $adoptPrices->adoptions; - $adoptedProductsBefore = $adoptProducts->adoptions; + // Duplicates accumulate on the SAME singleton across the whole process, + // not only this run — without this, a second handle() in one process + // would print a duplicate a previous run already reported. + $duplicatesBefore = count($adoptProducts->duplicates); $created = 0; + // Counted here, locally, rather than on AdoptStripeProduct itself: the + // very same class also adopts a MODULE's Product, from + // AddonPrices::product() inside syncModules() below, and a module's + // Product is never counted as an intent at all — unlike a family's, + // which gets its own "product …" line and $created++ right where this + // is incremented. A single counter on the class could not tell the two + // apart; counting only at the one call site that also counts the + // intent avoids the question entirely. + $adoptedFamilyProducts = 0; foreach (PlanFamily::query()->with('versions.prices')->orderBy('tier')->get() as $family) { $published = $family->versions->filter(fn (PlanVersion $version) => $version->isPublished()); @@ -121,6 +133,13 @@ class SyncStripeCatalogue extends Command // family's. $productId = $adoptProducts($metadata, ['plan_family_id']); + if ($productId !== null) { + // The intent was counted three lines up, so — unlike a + // module's Product — an adoption here may honestly be + // subtracted back out of it below. + $adoptedFamilyProducts++; + } + $productId ??= $stripe->createProduct( $family->name, $metadata, @@ -145,18 +164,24 @@ class SyncStripeCatalogue extends Command $this->newLine(); - // Both counted the same way and for the same reason: $created is an - // INTENT taken before we know whether ensure()/the adoption call will - // mint or adopt, for a price as well as for a family's product — see - // AdoptStripePrice. Combined into one "adopted" figure because an - // operator reading the summary line cares whether Stripe made anything - // NEW, not which kind of object it would have been. - $adoptedPrices = $adoptPrices->adoptions - $adoptedPricesBefore; - $adoptedProducts = $adoptProducts->adoptions - $adoptedProductsBefore; - $adopted = $adoptedPrices + $adoptedProducts; + // $adopted may only subtract what $created actually counted as an + // intent. Every price is one, family or module — syncPrice() and + // syncModules() both increment $created before knowing whether ensure() + // will mint or adopt, so the full price-adoption delta is honest. A + // family's Product is one too, counted above, so $adoptedFamilyProducts + // is honest for the same reason. A module's Product is NOT one — it + // prints no line of its own and increments nothing in syncModules(), + // however AdoptStripeProduct answers it — so its adoption stays out of + // both figures entirely. Subtracting it would report fewer objects + // created than Stripe actually gained. + $adopted = ($adoptPrices->adoptions - $adoptedPricesBefore) + $adoptedFamilyProducts; $minted = max(0, $created - $adopted); - foreach ($adoptProducts->duplicates as $duplicate) { + // Sliced to what THIS run added — the list lives on the same + // process-wide singleton as the count above, so without the same + // before/after care a second handle() in one process would print a + // duplicate a previous run already reported. + foreach (array_slice($adoptProducts->duplicates, $duplicatesBefore) as $duplicate) { // Named here as well as in the log: an operator running the sweep // reads this, and a second Product for one family is something only // a person can resolve — we deliberately do not deactivate it. diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 865a1df..5bdc8fb 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -61,11 +61,12 @@ class AppServiceProvider extends ServiceProvider // Both singletons because each carries a record across one run that the // caller reads only after it finishes: which products were duplicates, - // and how many products and prices were adopted rather than created. - // Resolved per call they would each start empty on every read. PlanPrices - // is deliberately NOT a singleton — SyncStripeCatalogue asks the container - // for it once per catalogue row — which is exactly why the counter has to - // live here instead: on a per-call instance it could never pass one. + // and how many prices were adopted rather than created. Resolved per + // call they would each start empty on every read. PlanPrices is + // deliberately NOT a singleton — SyncStripeCatalogue asks the container + // for it once per catalogue row — which is exactly why a record like + // this has to live on the adoption classes instead: on a per-call + // instance it could never carry across the row it started on. $this->app->singleton(AdoptStripeProduct::class); $this->app->singleton(AdoptStripePrice::class); diff --git a/app/Services/Billing/AdoptStripeProduct.php b/app/Services/Billing/AdoptStripeProduct.php index 3145fd5..4f5f46a 100644 --- a/app/Services/Billing/AdoptStripeProduct.php +++ b/app/Services/Billing/AdoptStripeProduct.php @@ -44,16 +44,6 @@ final class AdoptStripeProduct */ public array $duplicates = []; - /** - * How many Products this run took over rather than minted. - * - * Mirrors AdoptStripePrice::$adoptions, and for the same reason: - * stripe:sync-catalogue counts an intent to create a Product BEFORE it - * knows whether this class will hand back an existing one, so a Product - * that was adopted must not silently count as one that was created. - */ - public int $adoptions = 0; - public function __construct(private readonly StripeClient $stripe) {} /** @@ -108,8 +98,6 @@ final class AdoptStripeProduct 'metadata' => $metadata, ]); - $this->adoptions++; - return $adopted['id']; } diff --git a/tests/Feature/Billing/StripeProductAdoptionTest.php b/tests/Feature/Billing/StripeProductAdoptionTest.php index 2820bc0..9ea9275 100644 --- a/tests/Feature/Billing/StripeProductAdoptionTest.php +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -7,7 +7,6 @@ use App\Models\StripePlanPrice; use App\Models\Subscription; use App\Services\Billing\AddonPrices; use App\Services\Billing\AdoptStripeProduct; -use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; use App\Services\Stripe\FakeStripeClient; use App\Services\Stripe\StripeClient; @@ -123,7 +122,6 @@ it('says it adopted, not that it created', function () { app(Kernel::class)->call('stripe:sync-catalogue'); $row = PlanPrice::query()->firstOrFail(); - $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); $orphan = (string) StripePlanPrice::query() ->where('plan_price_id', $row->id)->where('reverse_charge', false) ->value('stripe_price_id'); @@ -152,3 +150,35 @@ it('names a duplicate product in its report, without touching it', function () { expect($family->refresh()->stripe_product_id)->toBe('prod_first'); }); + +it('does not fold a module Product adoption into the created/minted count', function () { + // A module's prices are all genuinely new, and its Product exists at + // Stripe already — unlinked from our own register, the same shape as a + // family's orphaned Product one level up (AddonPrices::product() asks + // AdoptStripeProduct exactly as the family loop does). The difference that + // makes this dangerous: syncModules() below counts a module's PRICES as + // intents, but it has never counted a module's PRODUCT as one — adopted or + // minted, that Product prints no line and increments nothing — so + // adopting it here must neither inflate "adopted" nor shrink "created". + $this->stripe->plantProduct('prod_priority_support_orphan', 'Priority Support', [ + 'addon' => 'priority_support', + ]); + + // Computed from the catalogue rather than hardcoded, the way + // ReverseChargePriceTest sizes a rate-change sweep: one Product per family, + // two treatments per priced row, and monthly+yearly at both treatments for + // every module — all of it newly minted here, nothing pre-existing to + // adopt except the one Product just planted above. + $modules = count(array_merge(array_keys((array) config('provisioning.addons')), ['storage'])); + $expectedCreated = PlanFamily::query()->count() + PlanPrice::query()->count() * 2 + $modules * 4; + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain("{$expectedCreated} object(s) created, 0 adopted in Stripe.") + ->assertSuccessful(); + + // And the orphan really was taken over rather than merely ignored — the + // Product this test is about, not a no-op that would pass it by accident. + expect(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'))->toBe('prod_priority_support_orphan'); +});