Subtract only what was counted, not everything a singleton adopted
A figure may only subtract what it counted as an intent in the first place. The family loop counts a Product before knowing whether it will be adopted or minted, so an adoption there could honestly be subtracted back out — but syncModules() has never counted a module's own Product as an intent at all; it only counts a module's Prices. Reading AdoptStripeProduct::adoptions as one run-wide delta could not tell the two apart, so a module Product adopted from an interrupted run silently inflated "adopted" and shrank "created" by exactly one, for a Product the command never claimed to have made in the first place. The fix counts the family side locally, at the one call site that already counts the intent, and leaves a module's Product out of both figures entirely — adopted or minted, it was never counted, so neither number may move for it. AdoptStripeProduct::$adoptions is gone with it: nothing reads a singleton-wide total that cannot be attributed to one side or the other, and keeping it around unread would be exactly the kind of state this codebase does not leave lying about. The duplicate-product report had the same shape of bug one line down: it read straight off the singleton's list with no before/after snapshot, so a second handle() call in one process would reprint a duplicate an earlier run already named. Sliced to what this run itself added, the same way the counters beside it already were. Also: the unread `$charged` line in the adoption test that TDD had already exercised as dead weight, and its now-unused PlanPrices import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>feature/host-bootstrap
nexxo
parent
df7da334ee
commit
048e5ba81f
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
||||
|
|
|
|||
|
|
@ -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'];
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue