diff --git a/app/Actions/ApplyPlanChange.php b/app/Actions/ApplyPlanChange.php index 6dca884..1d6422d 100644 --- a/app/Actions/ApplyPlanChange.php +++ b/app/Actions/ApplyPlanChange.php @@ -4,6 +4,7 @@ namespace App\Actions; use App\Models\Instance; use App\Models\Order; +use App\Models\PlanFamily; use App\Models\ProvisioningRun; use App\Models\Subscription; use App\Models\SubscriptionRecord; @@ -108,8 +109,8 @@ class ApplyPlanChange return null; } - // The gate, and the only one. An upgrade with the period already over, - // a downgrade before the term the customer paid for has run out, and any + // The eligibility gate. An upgrade with the period already over, a + // downgrade before the term the customer paid for has run out, and any // move on a contract that is no longer active all stop here — see // PlanChange::evaluate() for why each of them is refused. $change = PlanChange::evaluate($subscription, $targetPlan, $at); @@ -118,6 +119,50 @@ class ApplyPlanChange return null; } + // The other gate: can this contract actually be BILLED for what it is + // about to become? A contract with a live Stripe subscription behind it + // has to end this call still agreeing with Stripe about the price, and + // an INTERNAL family (`plan_families.internal`) never gets one — + // stripe:sync-catalogue skips it on purpose, permanently, because a + // Stripe Price cannot be deleted once minted and a package nobody is + // ever charged for is not worth leaving one behind. Left to run, + // MoveStripeSubscriptionPrice below would find nothing to move onto, + // throw, and park the swap on stripe_price_sync — where + // clupilot:sync-stripe-subscriptions retries it every hour forever, + // because no future sync run can ever fill in a price this family will + // never receive. + // + // Checked on the FLAG rather than on whether a price happens to resolve + // right now, even though the latter is more general and closer to the + // actual damage: a perfectly ordinary, still-sellable family can be + // between publish and its first stripe:sync-catalogue run too, and that + // gap is exactly what the park-and-retry path exists for — one sync run + // closes it (see the test right above this guard in + // StripePlanChangeTest). Refusing on unresolvability alone would treat + // that transient, self-healing gap the same as a permanent one and + // reject a change that would have gone through fine an hour later. The + // flag is the only signal that tells "will never be fixed by syncing + // again" apart from "has simply not been synced yet". + // + // A granted contract carries no `stripe_subscription_id` at all — see + // MoveStripeSubscriptionPrice's own early return — so there is nothing + // for it to disagree with Stripe about, and moving one onto an internal + // family (which is, after all, how such a package is normally handed + // out) is left alone here. + // + // Checked BEFORE the transaction below, not after: refusing once the + // contract, the register and the machine have already moved would + // leave the change half-done, which is worse than the sweep this + // replaces. + if ($subscription->stripe_subscription_id !== null + && PlanFamily::query()->where('key', $targetPlan)->value('internal')) { + Log::error('Cannot move a Stripe-billed contract onto an internal package: Stripe has no price for it and never will while it stays internal.', [ + 'subscription' => $subscription->uuid, 'plan' => $targetPlan, + ]); + + return null; + } + $instance = $this->instanceOf($subscription); // Read BEFORE the snapshot moves: the resize step has to know what the diff --git a/tests/Feature/Billing/StripePlanChangeTest.php b/tests/Feature/Billing/StripePlanChangeTest.php index be9939e..a4168ef 100644 --- a/tests/Feature/Billing/StripePlanChangeTest.php +++ b/tests/Feature/Billing/StripePlanChangeTest.php @@ -191,6 +191,34 @@ it('parks the change when the catalogue has never been mirrored into Stripe', fu ->and($contract->fresh()->stripe_price_sync['error'])->toContain('stripe:sync-catalogue'); }); +it('refuses to move a Stripe-billed contract onto an internal package instead of parking it forever', function () { + fakeServices(); + Queue::fake(); + + // A full, successful sync — unlike the test above. Enterprise is + // `internal` (see plan_families.internal) and stripe:sync-catalogue + // skips every internal family on purpose (its own docblock: a Price + // cannot be deleted once minted, so a package nobody is ever charged + // for stays out of the mirror entirely). So even after a clean run its + // price carries no Stripe id — and no later run will ever add one while + // it stays internal, which is what tells this apart from the ordinary + // "not synced YET" case above that one sync run fixes. + $stripe = stripeCatalogue(); + $contract = stripeContractOn('team'); + + app(ApplyPlanChange::class)($contract, 'enterprise'); + + // Refused before anything moved: the contract, the register and Stripe + // all stay exactly where they were, and nothing sits in + // stripe_price_sync for clupilot:sync-stripe-subscriptions to retry + // hourly, forever, because the price it is retrying for will never + // exist. + expect($stripe->priceChanges)->toBeEmpty() + ->and($contract->fresh()->plan)->toBe('team') + ->and($contract->fresh()->stripe_price_sync)->toBeNull() + ->and(SubscriptionRecord::query()->where('event', SubscriptionRecord::EVENT_UPGRADE)->count())->toBe(0); +}); + it('learns the subscription item from Stripe’s own events', function () { $contract = stripeContractOn('team', itemId: null);