fix(billing): make school billing lifecycle reversible, bounded and consistently counted (#546) - #557
Merged
Conversation
…onsistently counted (#546) Five independent defects in the school→platform billing lifecycle, none of which had any test coverage. 1. Cancel was irreversible. PostgREST's ON CONFLICT DO UPDATE only writes the columns you name, so confirmManualPayment's upsert left a stale cancel_at_period_end = true: a school could cancel, change its mind, pay for a full year and still be dropped to free at the end of it, with no reminder and no grace (both cron phases filter on cancel_at_period_end = false). The confirm upsert and changePlan now clear the flag (and pass cancel_at_period_end: false to Stripe), and a new reactivateSubscription() action is wired into the billing page — there was no reactivate action in the UI at all. 2. An unpaid renewal request paused the expiry downgrade forever. Requests now carry expires_at (created + 14 days); a new cron phase sweeps lapsed ones to 'expired' with an admin email, and the pause only honours open requests. Both duplicate guards moved off .single() — with two matching rows PostgREST returns PGRST116 / data: null, so the guards PASSED — and the renewal window guard is no longer bypassed by any non-active subscription status. 3. forceTenantPlanChange on a live Stripe subscriber billed the school for the comped plan: the next customer.subscription.updated read Stripe's real price as a downgrade, found the tenant over the real plan's limits (the reason it was comped) and repriced Stripe onto the comped plan. The override is now stamped on the subscription and applyPortalPlanChange ignores overridden tenants; clearTenantPlanOverride, a confirmed payment or an in-app plan change lift it. 4. The grace window collapsed to zero after a cron outage — graceEnd came from the subscription's period end, so one pass could send both "overdue" and "downgraded". It is now max(period_end, now) + GRACE_DAYS, and phase 3 skips tenants that entered grace in the same run. 5. Four definitions of "how many courses does this tenant have" collapsed to one: the billing page, the downgrade pre-flight and creation enforcement all route through countTenantUsage, and the hardcoded PLAN_LIMITS_FALLBACK map is gone in favour of platform_plans. Tests: 411 → 447. New cron, action, TTL and count-parity suites; each new assertion was verified to fail with the corresponding fix patched back out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U63Vu8MHB1MzE3e9oFk2kc
guillermoscript
marked this pull request as ready for review
July 26, 2026 17:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes all five defects in the school→platform billing lifecycle from #546: cancellation is reversible and a confirmed payment un-cancels, an unpaid renewal request can no longer pause the expiry downgrade forever, a super-admin comp no longer causes the webhook to bill the school for its own comp, the grace window survives a cron outage, and the four disagreeing course counts collapse to one.
Closes #546
Why
Each item is a real money/access bug, and
grep -rl "platform-webhook\|expire-platform-subscriptions" tests/returned nothing, so none of them could regress loudly.§1 — cancel was irreversible, and paying again did not undo it. PostgREST's
ON CONFLICT DO UPDATEonly writes the columns you name, soconfirmManualPayment's upsert left a stalecancel_at_period_end = trueon the row. Phases 1 and 2 of the cron both filter oncancel_at_period_end = false, phase 4 downgrades on it — so a school could cancel, change its mind, pay for a full year, have it confirmed, and be silently dropped to free at the end of that year having received no warning at all. Same shape on Stripe viachangePlan, and the billing UI imported no reactivate action whatsoever whilecheckout-sessionblocks re-checkout for a still-activesubscription.confirmManualPaymentandchangePlannow clearcancel_at_period_end/canceled_at, andchangePlansendscancel_at_period_end: falseto Stripe.reactivateSubscription()action + button on the billing page (Stripe first, then the local mirror; refuses when nothing is scheduled or the period already lapsed). Strings added toen.jsonandes.json.§2 — a never-paid renewal request paused the downgrade forever. Nothing ever moved a request out of an open status except a super admin, so "click request renewal, never pay" kept the paid plan, its limits and its reduced platform fee indefinitely — #462's revenue leak reopened through the pause. Both duplicate guards used
.single(), which returnsPGRST116 / data: nullonce two rows match, so the guards passed; and because the two guards had different filters, creating a renewal alongside a pending upgrade was the ordinary way to get there.platform_payment_requests.expires_at(created + 14 days,NOT NULL, defaulted) — migration20260726130000.expiredwith an admin email, and the downgrade pause only honours open requests (a lapsed one stops counting the instant it lapses, not when the sweep runs — tying it to the sweep would hand the leak back during any cron outage).activestatus; apast_dueschool can still pay, acanceledone 200 days from its period end cannot.§3 —
forceTenantPlanChangeon a live Stripe subscriber billed the school for the comped plan. The override never calls Stripe, so the nextcustomer.subscription.updatedcarried Stripe's real (unchanged) price, missed the no-op guard, read it as a downgrade, found the tenant over the real plan's limits — which is why it was comped — and "reverted" Stripe onto the forced plan's price. Masked by the dead webhook until #544 landed.platform_subscriptions.plan_override_by/plan_override_at— migration20260726130100.applyPortalPlanChangereturnsaction: 'ignored'for an overridden tenant.confirmManualPayment,changePlan, and a new super-adminclearTenantPlanOverride().§4 — the grace window collapsed to zero when the cron missed a week.
graceEndcame from the subscription'scurrent_period_end, and phase 3 re-queries in the same request, so after an outage longer thanGRACE_DAYSone pass sent "your payment is overdue" and "you have been downgraded". Silent: both counters incremented, so the response looked healthy.graceEnd = max(period_end, now) + GRACE_DAYS, plus phase 3 skips any tenant that entered grace in the same run.§5 — four definitions of "how many courses does this tenant have". The pre-flight and cutoff reconciler counted non-archived; creation enforcement and the number shown to the admin counted all courses; and
checkCourseLimitresolved the limit itself through a hardcodedPLAN_LIMITS_FALLBACKmap. A school with 30 courses (20 archived) onprosaw 30/100, was approved to downgrade tostarter(10 active ≤ 15), and then could not create a single course — with an error telling it to archive courses it had already archived.countTenantUsage;PLAN_LIMITS_FALLBACKdeleted in favour of a newgetTenantPlanLimits()readingplatform_plans(nois_activefilter — retiring a plan must not change what its existing subscribers may do; a missing row fails open to unlimited, matching every other caller in that module).Tenants affected by the §5 loosening: 0
Counted on the production project before writing the change:
No tenant's enforced limit changes today; the fix is about the state the pre-flight can put a school into tomorrow.
How to QA
Requires the two new migrations (
supabase db push/npm run db:reset). They have not been applied to cloud — that is a deploy step for this PR.owner@e2etest.comondefault.lvh.me:3000, put the tenant on a manual-transfer plan, cancel it from/dashboard/admin/billing, then confirm a renewal request as super admin at/platform/billing. Checkplatform_subscriptions:cancel_at_period_endisfalseandcanceled_atisNULL.UPDATE platform_payment_requests SET expires_at = now() - interval '1 day'. HitGET /api/cron/expire-platform-subscriptionswithAuthorization: Bearer $CRON_SECRET: the response reportsrequestsExpired: 1, the row isexpired, an email goes to the tenant admins, and apast_duesubscription past its grace window is no longer paused.current_period_end20 days in the past and run the cron:graceStarted: 1,downgraded: 0, andgrace_period_end ≈ now + 7 days./platform/tenants;plan_override_atis stamped and any subsequentcustomer.subscription.updatedreturnsignoredwithout touching Stripe./dashboard/admin/billingwith what course creation enforces — they match.Automated:
npx vitest run tests/unit/expire-platform-subscriptions.test.ts tests/unit/platform-billing-lifecycle.test.ts tests/unit/plan-limit-count-parity.test.ts tests/unit/payment-request-ttl.test.ts tests/unit/platform-plan-change.test.tsScreenshots / GIF
The only visible change is a Reactivate plan button that appears in the billing header when a subscription is scheduled for cancellation (previously that state had no exit in the UI). No GIF recorded — the flow needs a manual-transfer subscription mid-cancellation, which is faster to reach via step 2 above than to stage for a recording.
Tests
411 → 447 (37 files). Every new assertion was verified to fail with the corresponding fix patched back out:
max(period_end, now)→period_end.single()applyPortalPlanChangerevertedinstead ofignored— Stripe repriced onto the comped plan)tests/unit/support/fake-supabase.tsis a small in-memory PostgREST stand-in — these bugs are about what a query returns for a given table state (a.single()that yieldsPGRST116once two rows match, an upsert that leaves an unnamed column untouched, a count that does or doesn't include archived rows), which canned per-call replies cannot express.Checklist
npm run typecheckandnpm run test:unitpass (447/447)npm run buildpassestenant_idCRON_SECRET)messages/en.jsonandmessages/es.jsonNOT NULLbackfill and the 14-day default)lib/database.types.tsregenerated — deliberately not, since it is generated from the linked cloud project and these migrations are not applied there yet.createAdminClient()is untyped, so nothing depends on it.Lint: repo-wide
npm run lintkeeps its large pre-existing error baseline;npx eslintover every file this PR touches reports 0 errors (4 warnings, all pre-existing unused-variable ones on lines this PR did not introduce).🤖 Generated with Claude Code
https://claude.ai/code/session_01U63Vu8MHB1MzE3e9oFk2kc