diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index 7f32e41..c18f3b3 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -8,6 +8,7 @@ use App\Models\PlanVersion; use App\Models\Subscription; use App\Services\Billing\AddonCatalogue; use App\Services\Billing\AddonPrices; +use App\Services\Billing\AdoptStripePrice; use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; @@ -84,6 +85,14 @@ class SyncStripeCatalogue extends Command return self::FAILURE; } + // Resolved once, as singletons: each counts across the WHOLE run, and a + // count taken before ensure()/the adoption call runs can never itself + // distinguish "created" from "adopted" — see AdoptStripePrice. + $adoptPrices = app(AdoptStripePrice::class); + $adoptProducts = app(AdoptStripeProduct::class); + $adoptedPricesBefore = $adoptPrices->adoptions; + $adoptedProductsBefore = $adoptProducts->adoptions; + $created = 0; foreach (PlanFamily::query()->with('versions.prices')->orderBy('tier')->get() as $family) { @@ -110,7 +119,7 @@ class SyncStripeCatalogue extends Command // below stops protecting it after twenty-four hours. // plan_family_id is what proves such a Product is this // family's. - $productId = app(AdoptStripeProduct::class)($metadata, ['plan_family_id']); + $productId = $adoptProducts($metadata, ['plan_family_id']); $productId ??= $stripe->createProduct( $family->name, @@ -136,22 +145,33 @@ 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; + $minted = max(0, $created - $adopted); + + foreach ($adoptProducts->duplicates 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. + $this->warn(" duplicate product {$duplicate} — left active, a product's prices become unsellable if it is deactivated"); + } + if ($created === 0) { $this->info('Stripe is already in step with the catalogue.'); return self::SUCCESS; } - // "or adopted", because the count is taken BEFORE ensure() runs and - // ensure() may find the Price already at Stripe and adopt it instead of - // creating anything — see App\Services\Billing\AdoptStripePrice. Telling - // which of the two happened is the whole purpose of that step, so the - // first thing an operator reads after an interrupted run must not assert - // a creation that did not take place. The log entry adoption writes names - // the Price it took over. $this->info($dryRun - ? "{$created} object(s) would be created or adopted. Run without --dry-run to do it." - : "{$created} object(s) created or adopted in Stripe."); + ? "{$created} object(s) would be created. Run without --dry-run to create them." + : "{$minted} object(s) created, {$adopted} adopted in Stripe."); return self::SUCCESS; } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 1c5d75f..865a1df 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -13,6 +13,7 @@ use App\Mail\Transport\MailboxTransport; use App\Models\Customer; use App\Models\MaintenanceNotification; use App\Provisioning\PipelineRegistry; +use App\Services\Billing\AdoptStripePrice; use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\CustomDomainAccess; use App\Services\Dns\FileHostDnsDirectory; @@ -58,12 +59,15 @@ class AppServiceProvider extends ServiceProvider config('provisioning.pipelines', []), )); - // Singletons because both carry a record across one run that the caller - // reads afterwards: which products were duplicates, and how many prices - // were adopted rather than created. Resolved per call they would each - // start empty — SyncStripeCatalogue asks the container for PlanPrices - // once per catalogue row. + // 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. $this->app->singleton(AdoptStripeProduct::class); + $this->app->singleton(AdoptStripePrice::class); // Real I/O implementations; tests swap in fakes via app()->instance(). $this->app->bind(RemoteShell::class, PhpseclibRemoteShell::class); diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 3c1e8ce..2b547a3 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -42,6 +42,16 @@ use Illuminate\Support\Facades\Log; */ final class AdoptStripePrice { + /** + * How many Prices this run took over rather than minted. + * + * Read by stripe:sync-catalogue, which counts an intent BEFORE calling + * ensure() and so cannot tell the two apart on its own — comparing the Price + * id before and after does not distinguish them either, because there is no + * id before in either case. + */ + public int $adoptions = 0; + public function __construct(private readonly StripeClient $stripe) {} /** @@ -170,6 +180,8 @@ final class AdoptStripePrice 'interval' => $interval, ]); + $this->adoptions++; + return $adopted['id']; } diff --git a/app/Services/Billing/AdoptStripeProduct.php b/app/Services/Billing/AdoptStripeProduct.php index 4f5f46a..3145fd5 100644 --- a/app/Services/Billing/AdoptStripeProduct.php +++ b/app/Services/Billing/AdoptStripeProduct.php @@ -44,6 +44,16 @@ 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) {} /** @@ -98,6 +108,8 @@ 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 3379629..2820bc0 100644 --- a/tests/Feature/Billing/StripeProductAdoptionTest.php +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -1,10 +1,13 @@ once(); }); + +it('says it adopted, not that it created', function () { + // The one signal a human reads after an incident. The count is taken before + // ensure() runs, so without this the run reports objects created in Stripe + // when it made none — and telling those two apart is the whole point of the + // recognition step. + 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'); + StripePlanPrice::query()->where('stripe_price_id', $orphan)->delete(); + $this->stripe->keys = []; + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain('1 adopted') + ->assertSuccessful(); +}); + +it('names a duplicate product in its report, without touching it', function () { + // Nothing has been synced yet, same as the orphan test above — the family + // has no product of its own, and two stray ones claiming to be it are the + // whole of what Stripe has for it. + $family = PlanFamily::query()->orderBy('tier')->firstOrFail(); + $family->update(['stripe_product_id' => null]); + + $metadata = ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id]; + $this->stripe->plantProduct('prod_first', $family->name, $metadata, created: 100); + $this->stripe->plantProduct('prod_second', $family->name, $metadata, created: 200); + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain('prod_second') + ->assertSuccessful(); + + expect($family->refresh()->stripe_product_id)->toBe('prod_first'); +});