diff --git a/CLAUDE.md b/CLAUDE.md index 5fed282f..8a5e5871 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,6 +105,7 @@ Also supported (`products.payment_provider`): `paypal`, `lemonsqueezy`, `solana` **Key invariants:** - Transaction `status`: `pending`, `successful`, `failed`, `archived`, `canceled`, `refunded` +- **`subscriptions.cancel_at_period_end` is the ONLY signal that a cancel is scheduled** (since `20260726120000`, issue #545). `cancel_at` is nullable and purely informational — the CHECK `subscriptions_cancel_at_requires_schedule` allows it to be non-NULL only while the flag is set, so clear both together (both reactivate actions do). It shipped as `NOT NULL DEFAULT now()` with no writer setting it, which made the Solana crank cancel every subscription at its first rollover. `subscription_status` is `active`/`canceled`/`expired`/`renewed`/`past_due`; `renewed` and `past_due` both count as LIVE (parallel-subscription guards, billing UI, plan change), and cancelling must never *improve* a status - **Access control lives in `entitlements`, not `enrollments`** (since migration `20260516150000`): `entitlements` (`user_id`, `course_id`, `tenant_id`, `source_type`, `source_id`, `status`, `expires_at`) is the polymorphic source of truth for product/subscription access. `enrollments.product_id`/`subscription_id` and their old CHECK constraint were dropped — `enrollments` is now a learning-progress record only (`user_id`, `course_id`, `status`, `tenant_id`, `enrollment_date`) - `enroll_user()` RPC loops through ALL courses for a product (a product can map to multiple courses via `product_courses`) and writes to `entitlements` - **Subscriptions grant access, not auto-enrollment** — students self-enroll via `/dashboard/student/browse` (`useEnrollment()` hook); `plan_courses` defines which courses a plan covers diff --git a/app/[locale]/(public)/checkout/actions.ts b/app/[locale]/(public)/checkout/actions.ts index 71223511..42875a94 100644 --- a/app/[locale]/(public)/checkout/actions.ts +++ b/app/[locale]/(public)/checkout/actions.ts @@ -249,9 +249,33 @@ function mapPlanChangeError(raw: string): string { if (raw.includes('no_active_subscription')) return "You don't have an active subscription to change."; if (raw.includes('invalid_plan') || raw.includes('cross_tenant_plan')) return "Plan not found."; if (raw.includes('plan_deleted')) return "That plan is no longer available."; + if (raw.includes('upgrade_requires_payment')) { + return "This plan costs more than your current one. Contact your school to arrange the payment for the upgrade."; + } + // 23502 (not-null) / 23514 (check) on the subscriptions cancel columns. The + // #545 schema change should make these unreachable; before it, the + // reactivate branch of the RPC wrote NULL into a NOT NULL `cancel_at` and + // every switch back to a previously-held plan died here behind the generic + // message below, with no clue for the student or the logs. + if (raw.includes('23502') || raw.includes('23514') || raw.includes('cancel_at')) { + return "We couldn't switch your plan because of a problem on our side. Please contact your school."; + } return "Could not change your plan. Please try again."; } +/** + * RPC errors raised BEFORE the function writes anything, where our DB is + * already in the state the caller wanted (or never had a subscription to + * change). Compensating a native provider swap on one of these actively breaks + * the pairing: on a double-click, call A swaps the provider to plan B and + * commits the DB; call B swaps to B again, gets `same_plan`, and a blind revert + * would put the provider back on plan A's price with `proration_behavior: + * 'none'` — DB on B, billing A, no error the student ever sees (#545). + */ +function isNoOpPlanChangeError(raw: string): boolean { + return raw.includes('same_plan') || raw.includes('no_active_subscription'); +} + /** * Switch the student's current subscription to a different plan of the SAME * school + payment provider (supersession — never two live subs; issue #463). @@ -360,8 +384,12 @@ export async function changePlan(newPlanId?: string) { }); if (rpcError) { // Compensate: put the provider subscription back on the old price - // so billing and our DB stay consistent. - if (currentPlan?.provider_price_id) { + // so billing and our DB stay consistent — but only when the RPC + // genuinely failed to apply the switch. `same_plan` / + // `no_active_subscription` mean our DB is already where the + // student wanted it (or never had a subscription to move), so a + // revert would DESYNC the provider rather than restore it. + if (currentPlan?.provider_price_id && !isNoOpPlanChangeError(rpcError.message)) { try { await provider.updateSubscription(current.provider_subscription_id, { newProviderPriceId: currentPlan.provider_price_id, diff --git a/app/[locale]/dashboard/student/billing/page.tsx b/app/[locale]/dashboard/student/billing/page.tsx index ed8ea993..5e1c6aa1 100644 --- a/app/[locale]/dashboard/student/billing/page.tsx +++ b/app/[locale]/dashboard/student/billing/page.tsx @@ -51,8 +51,20 @@ interface Subscription { cancel_at_period_end: boolean | null plan_id: number payment_provider: string + provider_subscription_id: string | null } +/** + * Statuses whose subscription is the student's LIVE one — the row this page + * renders and offers cancel / switch controls for. + * + * `past_due` belongs here (#545). It used to be excluded, so a student mid + * dunning got the "no subscription" empty state: neither ChangePlanDialog nor + * ManageSubscription mounted and they could neither cancel nor switch while + * still being billed, even though both server actions accept the status. + */ +const LIVE_SUBSCRIPTION_STATUSES: SubscriptionStatus[] = ['active', 'renewed', 'past_due'] + interface PaymentRequest { request_id: number status: string @@ -197,7 +209,7 @@ export default async function StudentBillingPage() { // ── 2. Subscriptions ──────────────────────────────────────────────────────── const { data: rawSubs } = await supabase .from('subscriptions') - .select('subscription_id, subscription_status, current_period_end, end_date, cancel_at_period_end, plan_id, payment_provider') + .select('subscription_id, subscription_status, current_period_end, end_date, cancel_at_period_end, plan_id, payment_provider, provider_subscription_id') .eq('user_id', userId) .eq('tenant_id', tenantId) .order('created', { ascending: false }) @@ -235,9 +247,10 @@ export default async function StudentBillingPage() { } // Derived - const activeSubscription = subscriptions.find( - (s) => s.subscription_status === 'active' || s.subscription_status === 'renewed' + const activeSubscription = subscriptions.find((s) => + LIVE_SUBSCRIPTION_STATUSES.includes(s.subscription_status) ) + const isPastDue = activeSubscription?.subscription_status === 'past_due' // ── Plan-change (switch) support ──────────────────────────────────────────── // Switchable when the provider does an in-place swap (Stripe/LS) OR is @@ -250,14 +263,44 @@ export default async function StudentBillingPage() { !!activeCaps && (activeCaps.supportsPlanChange || !activeCaps.supportsNativeSubscriptions) + // Does this subscription settle a price difference by itself? Only an + // in-place provider swap with proration does (Stripe / Lemon Squeezy, and + // only when we actually hold a provider subscription to swap). Mirrors + // `_settles_natively` in change_subscription_plan. + const settlesNatively = + !!activeCaps?.supportsPlanChange && !!activeSubscription?.provider_subscription_id + let switchablePlans: SwitchablePlan[] = [] - if (canSwitchPlan) { - const { data: rawPlans } = await supabase - .from('plans') - .select('plan_id, plan_name, price, payment_provider') - .eq('tenant_id', tenantId) - .is('deleted_at', null) - switchablePlans = (rawPlans ?? []) as SwitchablePlan[] + if (canSwitchPlan && activeSubscription) { + const [{ data: rawPlans }, { data: currentPlanRow }] = await Promise.all([ + supabase + .from('plans') + .select('plan_id, plan_name, price, payment_provider') + .eq('tenant_id', tenantId) + .is('deleted_at', null), + supabase.from('plans').select('price').eq('plan_id', activeSubscription.plan_id).maybeSingle(), + ]) + const allPlans = (rawPlans ?? []) as SwitchablePlan[] + + // Price gate, server-side (#545). This list used to be every non-deleted + // tenant plan, so a student on a one-click FREE plan could pick the + // school's paid bank-transfer plan and walk away with its entitlements — + // no transaction, no payment_requests row, nothing for an admin to + // reconcile. Anything that does not settle natively may only move to a + // plan costing the same or less; the RPC refuses the rest with + // `upgrade_requires_payment` regardless of what the UI offers. + // + // The current plan is read WITHOUT the `deleted_at` filter on purpose: a + // school can soft-delete a plan students are still subscribed to, and the + // gate needs its real price. Students can read those rows — "Students can + // view active plans" (20260330200000) is permissive on `tenant_id` alone, + // so it ORs past the `deleted_at IS NULL` policy. If the row is genuinely + // gone the price reads 0 and the list collapses to free-only, which fails + // closed rather than opening the gate. + const currentPrice = Number(currentPlanRow?.price ?? 0) + switchablePlans = settlesNatively + ? allPlans + : allPlans.filter((p) => Number(p.price) <= currentPrice) } return ( @@ -298,13 +341,24 @@ export default async function StudentBillingPage() { + {/* Dunning notice (#545): a past_due subscription still bills and + still grants access during the provider's retry window, so say + so instead of rendering it as if nothing were wrong. */} + {isPastDue && ( +
+ + {t('subscription.pastDueNotice')} +
+ )} {/* Renewal / expiry date */} {(activeSubscription.current_period_end ?? activeSubscription.end_date) && (
{activeSubscription.cancel_at_period_end ? t('subscription.cancelsOn') - : t('subscription.renewsOn')} + : isPastDue + ? t('subscription.paymentDueSince') + : t('subscription.renewsOn')} {formatDate(activeSubscription.current_period_end ?? activeSubscription.end_date)} @@ -340,7 +394,7 @@ export default async function StudentBillingPage() { )} diff --git a/app/actions/admin/subscriptions.ts b/app/actions/admin/subscriptions.ts index 69aeaac6..1e0a1c5d 100644 --- a/app/actions/admin/subscriptions.ts +++ b/app/actions/admin/subscriptions.ts @@ -62,9 +62,17 @@ export async function cancelSubscription( } } + // A period-end cancel keeps whatever live status the row already had — + // cancelling must never IMPROVE it (#545). Writing 'active' unconditionally + // erased a `past_due` delinquency from billing health and the admin views + // the moment anyone scheduled a cancel. Only the legacy `renewed` status is + // normalized to 'active'; nothing writes it any more. + const liveStatus = + subscription.subscription_status === 'renewed' ? 'active' : subscription.subscription_status + const updates: any = { - // Use 'canceled' (correct enum value) for immediate; keep 'active' for period-end - subscription_status: immediate ? 'canceled' : 'active', + // Use 'canceled' (correct enum value) for immediate; keep the live status for period-end + subscription_status: immediate ? 'canceled' : liveStatus, canceled_at: new Date().toISOString(), } @@ -160,18 +168,16 @@ export async function reactivateSubscription(subscriptionId: number) { } } - // Reactivate the subscription. NB: `cancel_at` is NOT NULL in the schema — - // setting it to null here would fail the update (a latent bug that silently - // broke admin reactivate). Clearing `cancel_at_period_end` un-schedules the - // subscription-expiry crons, but the Solana auto-pull crank - // (lib/payments/solana-pull-decision.ts) reads `cancel_at` on its own, so we - // push it forward to the live period end instead of leaving a past value that - // would re-cancel the row. `canceled_at` is cleared. + // Reactivate the subscription. `cancel_at` is cleared WITH the flag: since + // #545 it is nullable and the CHECK + // `subscriptions_cancel_at_requires_schedule` rejects a cancel date that + // outlives its schedule, so a leftover date can no longer be read as a live + // cancel by the Solana auto-pull crank. `canceled_at` is cleared too. const { error: updateError } = await supabase .from('subscriptions') .update({ cancel_at_period_end: false, - cancel_at: subscription.current_period_end || subscription.end_date, + cancel_at: null, canceled_at: null, }) .eq('subscription_id', subscriptionId) diff --git a/app/actions/subscriptions.ts b/app/actions/subscriptions.ts index 68204fde..398c0007 100644 --- a/app/actions/subscriptions.ts +++ b/app/actions/subscriptions.ts @@ -75,15 +75,26 @@ export async function cancelMySubscription(subscriptionId: number): Promise p.plan_id !== currentPlanId && (p.payment_provider ?? 'manual') === currentProvider, ) diff --git a/components/student/manage-subscription.tsx b/components/student/manage-subscription.tsx index 8a8ee3dd..e85bac0e 100644 --- a/components/student/manage-subscription.tsx +++ b/components/student/manage-subscription.tsx @@ -77,6 +77,7 @@ export function ManageSubscription({ variant="outline" size="sm" className="gap-1.5" + data-testid="resume-subscription-button" onClick={handleReactivate} disabled={pending} > @@ -92,6 +93,7 @@ export function ManageSubscription({ variant="outline" size="sm" className="gap-1.5 text-destructive" + data-testid="cancel-subscription-button" onClick={() => setOpen(true)} > diff --git a/docs/DATABASE_SCHEMA.md b/docs/DATABASE_SCHEMA.md index 36067127..ea2dce8f 100644 --- a/docs/DATABASE_SCHEMA.md +++ b/docs/DATABASE_SCHEMA.md @@ -525,8 +525,8 @@ Student subscriptions to a school's plans. Tenant-scoped. | `start_date` / `end_date` | TIMESTAMPTZ | | | `current_period_start` / `current_period_end` | TIMESTAMPTZ | | | `trial_start` / `trial_end` | TIMESTAMPTZ | | -| `cancel_at` | TIMESTAMPTZ | NOT NULL — must be advanced on renewal, or reactivation breaks | -| `cancel_at_period_end` | BOOLEAN | Student self-cancel flow | +| `cancel_at` | TIMESTAMPTZ | NULLABLE. When the subscription terminates, or NULL when no cancel is scheduled — informational, never a signal. CHECK `subscriptions_cancel_at_requires_schedule`: non-NULL only while `cancel_at_period_end` (#545) | +| `cancel_at_period_end` | BOOLEAN | NOT NULL DEFAULT false. **The single source of truth for "a cancel is scheduled"** — read by the Solana crank, the expiry crons and the billing UI. It used to be OR'd with `cancel_at <= now()`, and since `cancel_at` defaulted to `now()` on every INSERT, that canceled every `solana_subs` subscription at its first rollover (#545) | | `canceled_at` / `ended_at` | TIMESTAMPTZ | | | `superseded_by` | INTEGER | Set on plan change — points at the replacement subscription | | `payment_provider` | VARCHAR | | diff --git a/lib/database.types.ts b/lib/database.types.ts index 2c00d510..507be5dd 100644 --- a/lib/database.types.ts +++ b/lib/database.types.ts @@ -5466,8 +5466,8 @@ export type Database = { } subscriptions: { Row: { - cancel_at: string - cancel_at_period_end: boolean | null + cancel_at: string | null + cancel_at_period_end: boolean canceled_at: string | null created: string current_period_end: string @@ -5489,8 +5489,8 @@ export type Database = { user_id: string } Insert: { - cancel_at?: string - cancel_at_period_end?: boolean | null + cancel_at?: string | null + cancel_at_period_end?: boolean canceled_at?: string | null created?: string current_period_end?: string @@ -5512,8 +5512,8 @@ export type Database = { user_id: string } Update: { - cancel_at?: string - cancel_at_period_end?: boolean | null + cancel_at?: string | null + cancel_at_period_end?: boolean canceled_at?: string | null created?: string current_period_end?: string @@ -6323,8 +6323,8 @@ export type Database = { change_subscription_plan: { Args: { _new_plan_id: number } Returns: { - cancel_at: string - cancel_at_period_end: boolean | null + cancel_at: string | null + cancel_at_period_end: boolean canceled_at: string | null created: string current_period_end: string diff --git a/lib/payments/solana-pull-decision.ts b/lib/payments/solana-pull-decision.ts index de63321d..21f9c881 100644 --- a/lib/payments/solana-pull-decision.ts +++ b/lib/payments/solana-pull-decision.ts @@ -14,8 +14,26 @@ /** The subscription-row fields the decision depends on. */ export interface PullDecisionRow { subscription_status: string + /** + * The ONLY signal that a cancel is scheduled (issue #545). Everything else + * about the cancel — including when it takes effect — is derived from the + * period, never from a stored date. + */ cancel_at_period_end: boolean | null - /** ISO timestamp the row is scheduled to cancel at (period-end cancel). */ + /** + * When the row is scheduled to terminate, or null when no cancel is + * scheduled. Reported in the decision's `reason`, and DELIBERATELY not part + * of the decision itself. + * + * This column used to be `NOT NULL DEFAULT now()` with no writer setting it + * at creation, so every subscription was born with a cancel date in the past + * and this function's old `cancelDue` branch canceled every `solana_subs` + * subscription at its first rollover instead of renewing it (#545 bug 1). + * `20260726120000_subscription_cancel_at_contract.sql` made the column + * nullable and pinned `cancel_at IS NULL OR cancel_at_period_end`, but the + * crank does not depend on that repair having reached any given database: + * a stale or garbage `cancel_at` can no longer cost a school a renewal. + */ cancel_at: string | null } @@ -79,11 +97,19 @@ export function decidePullAction(params: { // Without this the crank would renew a subscription the admin already // canceled (the money leak in #460 — the on-chain delegation is still live, // so a pull would succeed and re-bill the student). - const cancelDue = row.cancel_at - ? BigInt(Math.floor(new Date(row.cancel_at).getTime() / 1000)) <= now - : false - if (row.cancel_at_period_end || cancelDue) { - return { action: 'cancel', reason: 'scheduled to cancel at period end' } + // + // `cancel_at_period_end` is the whole test (#545). It used to be OR'd with a + // `cancel_at <= now` check, and because `cancel_at` defaulted to now() on + // every INSERT that OR was always true at the first rollover — no + // `solana_subs` subscription could ever renew. A cancel date is a + // consequence of the schedule, not evidence of one. + if (row.cancel_at_period_end === true) { + return { + action: 'cancel', + reason: row.cancel_at + ? `scheduled to cancel at period end (${row.cancel_at})` + : 'scheduled to cancel at period end', + } } // (5) Genuinely active and due — charge it. diff --git a/lib/payments/subscription-guard.ts b/lib/payments/subscription-guard.ts index 15ff9a9b..666eadcb 100644 --- a/lib/payments/subscription-guard.ts +++ b/lib/payments/subscription-guard.ts @@ -15,8 +15,19 @@ import type { SupabaseClient } from '@supabase/supabase-js' -/** Statuses that mean "this subscription still bills / grants access". */ -export const BLOCKING_SUBSCRIPTION_STATUSES = ['active', 'past_due'] +/** + * Statuses that mean "this subscription still bills / grants access". + * + * `renewed` belongs here (#545): it grants access and keeps billing exactly + * like `active`, and six other places treat it as live — including + * `change_subscription_plan`'s own supersession SELECT. Omitting it let a + * student holding a `renewed` subscription pass this guard AND the DB backstop + * in `handle_new_subscription`, and check out a second plan — the parallel + * double-billing #459 exists to prevent. Rows carry that status if they were + * renewed before `20260516140000` stopped writing it, i.e. the long-lived + * paying subscribers. + */ +export const BLOCKING_SUBSCRIPTION_STATUSES = ['active', 'renewed', 'past_due'] /** English fallback shown when an API path is hit directly; the checkout pages * render a translated notice before the user ever reaches these routes. */ diff --git a/lib/payments/webhook-dispatch.ts b/lib/payments/webhook-dispatch.ts index dfb35fd6..1ecf46eb 100644 --- a/lib/payments/webhook-dispatch.ts +++ b/lib/payments/webhook-dispatch.ts @@ -9,12 +9,9 @@ * Subscriptions are matched by (provider_subscription_id, payment_provider). * Writing `subscription_status` fires the DB trigger * `handle_subscription_status_change`, which disables linked enrollments on - * canceled/expired and re-enables them on a return to active. - * - * IMPORTANT: the `subscription_status` enum is - * ('active','canceled','expired','renewed') — there is NO 'past_due'. past_due - * events are logged only (access continues during the provider's retry window) - * rather than written to an invalid enum value. + * canceled/expired and re-enables them on a return to active. It matches + * neither branch for `past_due`, so recording a dunning subscription leaves + * access untouched — which is what we want during the provider's retry window. */ import type { SupabaseClient } from '@supabase/supabase-js' @@ -149,11 +146,23 @@ export async function dispatchBillingEvent( break } - case 'subscription.past_due': - // No 'past_due' enum value; log only. Access continues during the - // provider's retry window — a later canceled/expired event ends it. - console.log(`[webhook] past_due for ${provider} sub ${subId ?? 'unknown'} — no status change`) + case 'subscription.past_due': { + // `past_due` HAS been a valid enum value since + // 20260530140000_add_past_due_subscription_status.sql; this branch used + // to log and drop the event on a stale "the enum has no past_due" + // comment, so a student mid-dunning looked perfectly healthy to billing + // health, the admin subscription list and the student's own billing page + // (#545). Record it: access continues (the status-change trigger ignores + // past_due) and a later renewed/canceled/expired event moves it on. + if (!subId) break + const { error } = await admin + .from('subscriptions') + .update({ subscription_status: 'past_due' }) + .eq('provider_subscription_id', subId) + .eq('payment_provider', provider) + if (error) throw new Error(`dispatch ${event.type} failed: ${error.message}`) break + } case 'payment.succeeded': { // One-time purchase confirmation for hosted-checkout / Merchant-of-Record diff --git a/messages/en.json b/messages/en.json index 867f3bd6..ebc15fba 100644 --- a/messages/en.json +++ b/messages/en.json @@ -255,6 +255,8 @@ "renewsOn": "Renews on", "cancelsOn": "Cancels on", "cancelAtPeriodEndNotice": "This subscription will not renew. Access ends on {date}.", + "pastDueNotice": "Your last payment didn't go through. You still have access while we retry — please update your payment method or contact your school to keep your subscription.", + "paymentDueSince": "Payment due since", "cancelNote": "To cancel your subscription, please contact your school.", "manage": { "cancelHint": "You can cancel anytime — you keep access until your billing period ends.", diff --git a/messages/es.json b/messages/es.json index a159a57b..601e6d69 100644 --- a/messages/es.json +++ b/messages/es.json @@ -255,6 +255,8 @@ "renewsOn": "Se renueva el", "cancelsOn": "Se cancela el", "cancelAtPeriodEndNotice": "Esta suscripción no se renovará. El acceso termina el {date}.", + "pastDueNotice": "Tu último pago no se procesó. Mantienes el acceso mientras lo reintentamos: actualiza tu método de pago o contacta a tu escuela para conservar tu suscripción.", + "paymentDueSince": "Pago pendiente desde", "cancelNote": "Para cancelar tu suscripción, comunícate con tu institución.", "manage": { "cancelHint": "Puedes cancelar cuando quieras: conservas el acceso hasta que termine tu periodo de facturación.", diff --git a/supabase/migrations/20260726120000_subscription_cancel_at_contract.sql b/supabase/migrations/20260726120000_subscription_cancel_at_contract.sql new file mode 100644 index 00000000..4fc77bb6 --- /dev/null +++ b/supabase/migrations/20260726120000_subscription_cancel_at_contract.sql @@ -0,0 +1,96 @@ +-- Subscription cancel-state contract — issue #545 (EPIC #540 §2.2), bug 1. +-- +-- `subscriptions.cancel_at` shipped as +-- +-- cancel_at | timestamptz | NOT NULL | DEFAULT timezone('utc', now()) +-- +-- and NO writer sets it at creation (handle_new_subscription and +-- change_subscription_plan both omit it), so every row was born with a cancel +-- date permanently in the past. The Solana auto-pull crank +-- (lib/payments/solana-pull-decision.ts) read that column as "is this row +-- scheduled to cancel?", so the first crank run after a period rolled took the +-- cancel branch: subscription_status → 'canceled', entitlements expired by +-- handle_subscription_status_change, no pull submitted. Native `solana_subs` +-- recurring billing could therefore never renew — the student lost access at +-- period end, the school was never paid for period 2 onward, and the on-chain +-- delegation stayed live. +-- +-- The contract from here on: +-- +-- cancel_at_period_end NOT NULL DEFAULT false. The ONLY signal that a cancel +-- is scheduled. Every decision (crank, cron, UI) reads +-- this and nothing else. +-- cancel_at NULL when no cancel is scheduled; otherwise the +-- instant the subscription terminates. Informational — +-- a date, never a signal. +-- +-- The CHECK below makes the pair inseparable: a stale cancel date cannot be +-- left behind by clearing the flag, and a date cannot be written without +-- scheduling the cancel. That is exactly the invariant the old column default +-- violated on every INSERT. +-- +-- `canceled_at` and `ended_at` carried the same `DEFAULT now()` lie — a freshly +-- created subscription was born "canceled at now, ended at now". Both are +-- already worked around by writers that explicitly send NULL +-- (handle_new_subscription's ON CONFLICT, change_subscription_plan); drop the +-- defaults so the workaround is no longer needed, and repair live rows. +-- +-- No real users hold subscriptions (#545: "prefer the clean schema change over +-- a compatible one"), so this changes the column contract outright rather than +-- layering a compatibility shim on top of it. + +-- ── 1. cancel_at becomes nullable with no default ─────────────────────────── +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at DROP DEFAULT; +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at DROP NOT NULL; + +-- ── 2. cancel_at_period_end becomes the authoritative, always-present flag ── +UPDATE public.subscriptions SET cancel_at_period_end = false WHERE cancel_at_period_end IS NULL; +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at_period_end SET DEFAULT false; +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at_period_end SET NOT NULL; + +-- ── 3. Repair every row born with the bogus defaults ──────────────────────── +-- A row that is not scheduled to cancel must not carry a cancel date. A row +-- that IS scheduled cancels at its period end, which is what both cancel +-- actions write today. +UPDATE public.subscriptions + SET cancel_at = CASE + WHEN cancel_at_period_end THEN COALESCE(current_period_end, end_date, cancel_at) + ELSE NULL + END; + +-- `canceled_at` / `ended_at` only mean something once the subscription actually +-- ended. Clear the default-injected values on every still-live row. +ALTER TABLE public.subscriptions ALTER COLUMN canceled_at DROP DEFAULT; +ALTER TABLE public.subscriptions ALTER COLUMN ended_at DROP DEFAULT; + +UPDATE public.subscriptions + SET canceled_at = NULL + WHERE canceled_at IS NOT NULL + AND NOT cancel_at_period_end + AND subscription_status NOT IN ('canceled', 'expired'); + +UPDATE public.subscriptions + SET ended_at = NULL + WHERE ended_at IS NOT NULL + AND subscription_status NOT IN ('canceled', 'expired'); + +-- ── 4. Pin the invariant ──────────────────────────────────────────────────── +-- Validated (not NOT VALID): step 3 guarantees every existing row satisfies it, +-- and a silently-unenforced constraint would reproduce this whole bug class. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'subscriptions_cancel_at_requires_schedule' + AND conrelid = 'public.subscriptions'::regclass + ) THEN + ALTER TABLE public.subscriptions + ADD CONSTRAINT subscriptions_cancel_at_requires_schedule + CHECK (cancel_at IS NULL OR cancel_at_period_end); + END IF; +END $$; + +COMMENT ON COLUMN public.subscriptions.cancel_at IS + 'When this subscription terminates, or NULL when no cancel is scheduled. Informational only — cancel_at_period_end is the signal (issue #545). Enforced by subscriptions_cancel_at_requires_schedule.'; +COMMENT ON COLUMN public.subscriptions.cancel_at_period_end IS + 'The single source of truth for "a cancel is scheduled". Read by the Solana auto-pull crank, the expiry crons and the billing UI (issue #545).'; diff --git a/supabase/migrations/20260726120100_handle_new_subscription_state_machine.sql b/supabase/migrations/20260726120100_handle_new_subscription_state_machine.sql new file mode 100644 index 00000000..b55ae457 --- /dev/null +++ b/supabase/migrations/20260726120100_handle_new_subscription_state_machine.sql @@ -0,0 +1,115 @@ +-- handle_new_subscription state-machine fixes — issue #545 (EPIC #540 §2.2). +-- +-- Two changes on top of 20260719120000_guard_parallel_subscriptions.sql: +-- +-- 1. `renewed` joins the #459 parallel-subscription backstop. The guard checked +-- `IN ('active','past_due')` while six other places treat `renewed` as live +-- (change_subscription_plan's own supersession SELECT among them), so a +-- student holding a `renewed` subscription passed both the app guard +-- (lib/payments/subscription-guard.ts) and this DB backstop and could check +-- out a second plan — the exact parallel double-billing #459 exists to stop. +-- `renewed` rows are the long-lived paying subscribers renewed before +-- 20260516140000 stopped writing that status, so this is the population it +-- matters most for. +-- +-- 2. Re-purchasing a plan that is scheduled to cancel un-schedules the cancel. +-- The ON CONFLICT branch (the renewal path) flipped the row back to 'active' +-- and extended the period but left `cancel_at_period_end = true` behind, so +-- the subscription the student had just paid for was still torn down at +-- period end. With the #545 contract (20260726120000) `cancel_at` must be +-- cleared with the flag, so both move together here. +-- +-- Body otherwise identical to 20260719120000_guard_parallel_subscriptions.sql. + +CREATE OR REPLACE FUNCTION public.handle_new_subscription( + _user_id uuid, + _plan_id integer, + _transaction_id integer, + _start_date timestamp with time zone DEFAULT now() +) +RETURNS void +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path TO 'public' +AS $function$ +DECLARE + _duration INTERVAL; + _end_date TIMESTAMP WITH TIME ZONE; + _existing_end TIMESTAMP WITH TIME ZONE; + _is_renewal BOOLEAN; + _tenant_id UUID; + _subscription_id INTEGER; + _provider TEXT; + _provider_sub_id TEXT; + _rec RECORD; +BEGIN + SELECT make_interval(days => p.duration_in_days), p.tenant_id INTO _duration, _tenant_id + FROM plans p WHERE p.plan_id = _plan_id; + IF _tenant_id IS NULL THEN + RAISE EXCEPTION 'Plan % not found', _plan_id; + END IF; + + -- Provider info recorded on the transaction at checkout (native sub id). + SELECT t.payment_provider, t.provider_subscription_id + INTO _provider, _provider_sub_id + FROM transactions t WHERE t.transaction_id = _transaction_id; + + SELECT end_date INTO _existing_end FROM subscriptions WHERE user_id = _user_id AND plan_id = _plan_id; + _is_renewal := FOUND; + + -- Parallel-subscription backstop (#459): refuse to CREATE a subscription + -- while another live one exists for the same user + tenant. Renewals of an + -- existing row are exempt. `renewed` counts as live (#545) — it grants + -- access and still bills, so letting it through re-opened #459. + IF NOT _is_renewal THEN + PERFORM 1 FROM subscriptions s + WHERE s.user_id = _user_id + AND s.tenant_id = _tenant_id + AND s.plan_id <> _plan_id + AND s.subscription_status IN ('active', 'renewed', 'past_due'); + IF FOUND THEN + RAISE EXCEPTION 'parallel_subscription: user % already has a live subscription in tenant %; refusing to create a second one for plan % (issue #459)', + _user_id, _tenant_id, _plan_id; + END IF; + END IF; + + _end_date := GREATEST(COALESCE(_existing_end, _start_date), _start_date) + _duration; + + INSERT INTO subscriptions ( + user_id, plan_id, start_date, end_date, current_period_end, transaction_id, + subscription_status, tenant_id, payment_provider, provider_subscription_id + ) + VALUES ( + _user_id, _plan_id, _start_date, _end_date, _end_date, _transaction_id, + 'active', _tenant_id, COALESCE(_provider, 'manual'), _provider_sub_id + ) + ON CONFLICT (user_id, plan_id) DO UPDATE SET + end_date = EXCLUDED.end_date, + current_period_end = EXCLUDED.current_period_end, + transaction_id = EXCLUDED.transaction_id, + subscription_status = 'active', + tenant_id = EXCLUDED.tenant_id, + ended_at = NULL, + -- Paying for this plan again un-schedules a pending cancel (#545): + -- leaving the flag set tore down the period the student just bought. + -- cancel_at moves with the flag (subscriptions_cancel_at_requires_schedule). + cancel_at_period_end = false, + cancel_at = NULL, + canceled_at = NULL, + -- COALESCE so a manual/legacy transaction with NULL provider info never + -- wipes an existing native subscription id. + payment_provider = COALESCE(EXCLUDED.payment_provider, subscriptions.payment_provider), + provider_subscription_id = COALESCE(EXCLUDED.provider_subscription_id, subscriptions.provider_subscription_id) + RETURNING subscription_id INTO _subscription_id; + + -- Grant access only. Enrollment is the student's explicit choice (self-enroll + -- via /browse), NOT auto-created here. + FOR _rec IN SELECT pc.course_id FROM plan_courses pc WHERE pc.plan_id = _plan_id + LOOP + INSERT INTO entitlements (user_id, course_id, tenant_id, source_type, source_id, status, expires_at) + VALUES (_user_id, _rec.course_id, _tenant_id, 'subscription', _subscription_id, 'active', _end_date) + ON CONFLICT (user_id, course_id, source_type, source_id) DO UPDATE SET + status = 'active', expires_at = EXCLUDED.expires_at, revoked_at = NULL, tenant_id = EXCLUDED.tenant_id; + END LOOP; +END; +$function$; diff --git a/supabase/migrations/20260726120200_change_subscription_plan_hardening.sql b/supabase/migrations/20260726120200_change_subscription_plan_hardening.sql new file mode 100644 index 00000000..4e3f03a9 --- /dev/null +++ b/supabase/migrations/20260726120200_change_subscription_plan_hardening.sql @@ -0,0 +1,234 @@ +-- change_subscription_plan hardening — issue #545 (EPIC #540 §2.2), bugs 2 + 3. +-- +-- Replaces the body shipped in 20260719170000_change_subscription_plan.sql. +-- The supersession semantics (cancel the old sub, activate the new one, carry +-- the period, re-grant entitlements) are unchanged; four defects are fixed. +-- +-- 1. `cancel_at = NULL` on a NOT NULL column (`20260719170000:124`). +-- The reactivate-existing-row branch is taken whenever a row already exists +-- for (user, new_plan) in ANY status — which is exactly what step 1 of this +-- same function leaves behind (old row set to 'canceled', never deleted). So +-- EVERY switch back to a previously-held plan failed with 23502 and the +-- canonical Basic → Pro → Basic downgrade always aborted. `cancel_at` is +-- nullable as of 20260726120000, which makes NULL the correct value here. +-- +-- 2. No advisory lock. Two concurrent calls (a double-click; the dialog's +-- `pending` disable is per-tab only) could both read the same live +-- subscription and race. `grant_free_subscription` already takes +-- pg_advisory_xact_lock for the same reason; this takes one keyed on +-- (user, tenant) so the second call blocks, then sees the state the first +-- committed and raises `same_plan` — and the server action no longer +-- compensates on that, so the provider and our DB end up agreeing. +-- +-- 3. A free plan could be switched to a paid one with no payment. Nothing +-- compared prices — not this function, not the checkout action, not the +-- billing page's plan list. Activate a free plan (one click, subscribeFree, +-- which sets current_period_end = 2126-01-01), open Billing → Change plan, +-- pick the school's paid manual/bank-transfer plan, and this function wrote +-- `entitlements … expires_at = 2126-01-01` for every course in that plan +-- with no transaction and no payment_requests row for an admin to +-- reconcile. Manual/offline is the primary LATAM flow, so this was not a +-- corner tenant. An upgrade is now refused unless the provider settles the +-- difference itself (Stripe / Lemon Squeezy prorate the in-place swap the +-- server action performs before calling this). +-- +-- The comparison is on raw `plans.price`, deliberately NOT price-per-day: +-- the period is CARRIED from the old subscription, never extended, so +-- switching a $19/30d subscription onto a $190/365d plan hands over the +-- pricier plan's courses for the remaining prepaid days. Raw price is the +-- right conservative test for "is the student asking for more than they +-- paid for". +-- +-- 4. A scheduled cancel was silently discarded. The new row was written with +-- `cancel_at_period_end = false, canceled_at = NULL` regardless of the old +-- row's state, while `lib/payments/stripe-provider.ts` swaps the item and +-- proration only and never clears Stripe's own cancel_at_period_end. Cancel +-- → switch therefore left our DB showing "Renews on" while Stripe still +-- terminated the subscription, and the reactivate affordance was hidden +-- because the flag read false. The pending cancel is now carried onto the +-- new row, so the two agree and the student can still resume. + +CREATE OR REPLACE FUNCTION public.change_subscription_plan(_new_plan_id integer) +RETURNS public.subscriptions +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path TO 'public' +AS $function$ +DECLARE + _uid uuid := auth.uid(); + _now timestamptz := now(); + _old subscriptions; + _new subscriptions; + _tenant uuid; + _plan_tenant uuid; + _plan_deleted timestamptz; + _duration integer; + _new_price numeric; + _old_price numeric; + _settles_natively boolean; + _carry_cancel boolean; + _end timestamptz; + _rec RECORD; + -- Free plans never expire (mirrors grant_free_subscription's horizon). + _far_future CONSTANT timestamptz := timestamptz '2126-01-01 00:00:00+00'; +BEGIN + IF _uid IS NULL THEN + RAISE EXCEPTION 'not_authenticated'; + END IF; + + -- Resolve the target plan FIRST — its tenant is authoritative. Deriving the + -- tenant from the plan (not from an arbitrary "most recent" sub) keeps this + -- correct for users who are members of more than one school: the sub we + -- supersede is the caller's live one IN THE TARGET PLAN'S TENANT, matching + -- what the server action validated. + SELECT p.tenant_id, p.deleted_at, p.duration_in_days, p.price + INTO _plan_tenant, _plan_deleted, _duration, _new_price + FROM plans p + WHERE p.plan_id = _new_plan_id; + + IF _plan_tenant IS NULL THEN + RAISE EXCEPTION 'invalid_plan'; + END IF; + IF _plan_deleted IS NOT NULL THEN + RAISE EXCEPTION 'plan_deleted'; + END IF; + _tenant := _plan_tenant; + + -- Serialize concurrent plan changes for this (user, tenant) — a double + -- click must not have two calls reading the same live subscription. Taken + -- BEFORE the read so the loser sees the winner's committed state. + PERFORM pg_advisory_xact_lock(hashtext(_uid::text || ':' || _tenant::text)); + + -- Current live subscription (the one being switched away from), scoped to the + -- caller AND the target plan's tenant — this RPC can never touch another + -- user's subscription, nor a live sub the caller holds in a different school. + SELECT * INTO _old + FROM subscriptions + WHERE user_id = _uid + AND tenant_id = _tenant + AND subscription_status IN ('active', 'renewed', 'past_due') + ORDER BY created DESC + LIMIT 1; + + IF _old.subscription_id IS NULL THEN + RAISE EXCEPTION 'no_active_subscription'; + END IF; + + IF _old.plan_id = _new_plan_id THEN + RAISE EXCEPTION 'same_plan'; + END IF; + + -- Price authority. Only a provider that settles the mid-period difference + -- itself may be moved onto a more expensive plan by a supersession: the + -- server action swaps the item on the SAME provider subscription with + -- proration before calling us, so the student is actually charged. Every + -- other provider (manual / bank transfer / one-time crypto) settles + -- offline, and nothing in this path creates a transaction or a + -- payment_requests row — so an upgrade there is an unpaid grant. + SELECT p.price INTO _old_price FROM plans p WHERE p.plan_id = _old.plan_id; + _settles_natively := _old.payment_provider IN ('stripe', 'lemonsqueezy') + AND _old.provider_subscription_id IS NOT NULL; + + IF NOT _settles_natively AND COALESCE(_new_price, 0) > COALESCE(_old_price, 0) THEN + RAISE EXCEPTION 'upgrade_requires_payment'; + END IF; + + -- Period-aligned: carry the old sub's remaining period. Never trust a + -- client-supplied period (this function is callable by any authenticated + -- user). A native in-place swap keeps the provider's billing cycle, so this + -- matches; a later renewal webhook corrects it if it ever diverges. + _end := COALESCE(_old.current_period_end, _old.end_date, + _now + make_interval(days => COALESCE(_duration, 30))); + + -- A pending cancel is the student's standing intent and Stripe still holds + -- it (the in-place swap does not clear cancel_at_period_end there), so it + -- rides along to the new plan rather than being silently dropped. + _carry_cancel := COALESCE(_old.cancel_at_period_end, false); + + -- Switching ONTO a free plan: nothing bills, so there is nothing to cancel, + -- and free subscriptions never expire (same horizon grant_free_subscription + -- uses) — otherwise the student's free plan would lapse when the prepaid + -- period of the plan they left runs out, with no way to re-activate it. + IF COALESCE(_new_price, 0) = 0 THEN + _end := _far_future; + _carry_cancel := false; + END IF; + + -- 1) Supersede the old subscription. Its cancel schedule goes with it — + -- cancel_at must not outlive the flag (#545 contract). + UPDATE subscriptions + SET subscription_status = 'canceled', + cancel_at_period_end = false, + cancel_at = NULL, + canceled_at = _now, + ended_at = _now, + provider_subscription_id = NULL + WHERE subscription_id = _old.subscription_id; + + -- 2) Activate the new plan (reactivate existing row or insert), carrying the + -- old sub's provider fields + transaction_id. + SELECT * INTO _new + FROM subscriptions + WHERE user_id = _uid AND plan_id = _new_plan_id + LIMIT 1; + + IF _new.subscription_id IS NOT NULL THEN + UPDATE subscriptions + SET subscription_status = 'active', + tenant_id = _tenant, + start_date = _now, + current_period_start = _now, + current_period_end = _end, + end_date = _end, + cancel_at_period_end = _carry_cancel, + cancel_at = CASE WHEN _carry_cancel THEN _end ELSE NULL END, + canceled_at = CASE WHEN _carry_cancel THEN COALESCE(_old.canceled_at, _now) ELSE NULL END, + ended_at = NULL, + payment_provider = _old.payment_provider, + provider_subscription_id = _old.provider_subscription_id, + transaction_id = _old.transaction_id, + superseded_by = NULL + WHERE subscription_id = _new.subscription_id + RETURNING * INTO _new; + ELSE + INSERT INTO subscriptions ( + user_id, plan_id, tenant_id, start_date, current_period_start, + current_period_end, end_date, subscription_status, transaction_id, + payment_provider, provider_subscription_id, created, ended_at, + cancel_at_period_end, cancel_at, canceled_at + ) + VALUES ( + _uid, _new_plan_id, _tenant, _now, _now, + _end, _end, 'active', _old.transaction_id, + _old.payment_provider, _old.provider_subscription_id, _now, NULL, + _carry_cancel, + CASE WHEN _carry_cancel THEN _end ELSE NULL END, + CASE WHEN _carry_cancel THEN COALESCE(_old.canceled_at, _now) ELSE NULL END + ) + RETURNING * INTO _new; + END IF; + + -- 3) Record supersession lineage on the old row. + UPDATE subscriptions + SET superseded_by = _new.subscription_id + WHERE subscription_id = _old.subscription_id; + + -- 4) Grant entitlements for the new plan's courses (mirror + -- handle_new_subscription). Shared courses keep access; old-only courses + -- were expired by the status-change trigger in step 1. + FOR _rec IN SELECT pc.course_id FROM plan_courses pc WHERE pc.plan_id = _new_plan_id + LOOP + INSERT INTO entitlements (user_id, course_id, tenant_id, source_type, source_id, status, expires_at) + VALUES (_uid, _rec.course_id, _tenant, 'subscription', _new.subscription_id, 'active', _end) + ON CONFLICT (user_id, course_id, source_type, source_id) DO UPDATE SET + status = 'active', expires_at = EXCLUDED.expires_at, revoked_at = NULL, tenant_id = EXCLUDED.tenant_id; + END LOOP; + + RETURN _new; +END; +$function$; + +-- Callable by the subscription owner (SECURITY DEFINER scopes writes to +-- auth.uid()); service_role for admin/testing. Never anon. +REVOKE ALL ON FUNCTION public.change_subscription_plan(integer) FROM public, anon; +GRANT EXECUTE ON FUNCTION public.change_subscription_plan(integer) TO authenticated, service_role; diff --git a/supabase/migrations/rollback/20260726120000_subscription_cancel_at_contract.down.sql b/supabase/migrations/rollback/20260726120000_subscription_cancel_at_contract.down.sql new file mode 100644 index 00000000..7eb1aa86 --- /dev/null +++ b/supabase/migrations/rollback/20260726120000_subscription_cancel_at_contract.down.sql @@ -0,0 +1,41 @@ +-- Rollback for 20260726120000_subscription_cancel_at_contract.sql (issue #545). +-- +-- NOT APPLIED BY DEFAULT. Running this restores the column contract that CAUSED +-- the bug: `cancel_at` back to NOT NULL DEFAULT now(), which means every row +-- inserted afterwards is born with a cancel date permanently in the past. +-- +-- On its own that is inert — `lib/payments/solana-pull-decision.ts` no longer +-- reads `cancel_at` for the cancel decision — but it also breaks every writer +-- shipped alongside the forward migration: both reactivate actions and +-- `change_subscription_plan`'s reactivate branch now write `cancel_at = NULL` +-- and would fail with 23502, taking the A → B → A plan switch down with them. +-- So this only makes sense together with reverting the TypeScript and the two +-- function migrations (20260726120100 / 20260726120200) in the same commit. +-- +-- Only run it if the new contract has broken a legitimate write path and the +-- fix has to be re-cut. + +BEGIN; + +ALTER TABLE public.subscriptions + DROP CONSTRAINT IF EXISTS subscriptions_cancel_at_requires_schedule; + +-- NOT NULL needs every row populated first; the forward migration deliberately +-- NULLed the rows that are not scheduled to cancel. +UPDATE public.subscriptions + SET cancel_at = COALESCE(cancel_at, current_period_end, end_date, timezone('utc', now())) + WHERE cancel_at IS NULL; + +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at SET DEFAULT timezone('utc'::text, now()); +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at SET NOT NULL; + +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at_period_end DROP NOT NULL; +ALTER TABLE public.subscriptions ALTER COLUMN cancel_at_period_end DROP DEFAULT; + +ALTER TABLE public.subscriptions ALTER COLUMN canceled_at SET DEFAULT timezone('utc'::text, now()); +ALTER TABLE public.subscriptions ALTER COLUMN ended_at SET DEFAULT timezone('utc'::text, now()); + +COMMENT ON COLUMN public.subscriptions.cancel_at IS NULL; +COMMENT ON COLUMN public.subscriptions.cancel_at_period_end IS NULL; + +COMMIT; diff --git a/supabase/migrations/rollback/20260726120100_handle_new_subscription_state_machine.down.sql b/supabase/migrations/rollback/20260726120100_handle_new_subscription_state_machine.down.sql new file mode 100644 index 00000000..4508f55a --- /dev/null +++ b/supabase/migrations/rollback/20260726120100_handle_new_subscription_state_machine.down.sql @@ -0,0 +1,14 @@ +-- Rollback for 20260726120100_handle_new_subscription_state_machine.sql (#545). +-- +-- NOT APPLIED BY DEFAULT. Re-applies the previous definition verbatim, which: +-- * drops `renewed` from the #459 parallel-subscription backstop, so a +-- `renewed` subscriber can create a second live subscription and be billed +-- for both (pair this with reverting BLOCKING_SUBSCRIPTION_STATUSES); +-- * stops clearing a pending cancel on renewal, so re-purchasing a plan that +-- is scheduled to cancel still tears it down at period end. +-- +-- Re-run the shipped migration to restore it: +-- psql < supabase/migrations/20260719120000_guard_parallel_subscriptions.sql +-- +-- The previous body lives in that file unchanged; this rollback intentionally +-- does NOT inline a copy, so the two can never drift. diff --git a/supabase/migrations/rollback/20260726120200_change_subscription_plan_hardening.down.sql b/supabase/migrations/rollback/20260726120200_change_subscription_plan_hardening.down.sql new file mode 100644 index 00000000..08d57332 --- /dev/null +++ b/supabase/migrations/rollback/20260726120200_change_subscription_plan_hardening.down.sql @@ -0,0 +1,14 @@ +-- Rollback for 20260726120200_change_subscription_plan_hardening.sql (#545). +-- +-- NOT APPLIED BY DEFAULT. Re-applying the previous definition restores all four +-- defects: the `cancel_at = NULL` write (harmless only while 20260726120000 is +-- still applied — with the schema rolled back too it fails every A → B → A +-- switch with 23502), no advisory lock, no price authority (a free plan can be +-- switched to a paid one and receive its entitlements with no transaction), and +-- a silently discarded pending cancel that desyncs Stripe. +-- +-- Restore it with: +-- psql < supabase/migrations/20260719170000_change_subscription_plan.sql +-- +-- The previous body lives in that file unchanged; this rollback intentionally +-- does NOT inline a copy, so the two can never drift. diff --git a/tests/playwright/plan-change.spec.ts b/tests/playwright/plan-change.spec.ts index df7e83b5..c6878025 100644 --- a/tests/playwright/plan-change.spec.ts +++ b/tests/playwright/plan-change.spec.ts @@ -1,7 +1,18 @@ import { test, expect } from '@playwright/test' +import { createClient as createSupabaseClient } from '@supabase/supabase-js' import { loginAsTenantStudent } from './utils/auth' import { TENANT_BASE } from './utils/constants' +const ALICE_ID = 'a1000000-0000-0000-0000-000000000004' + +function getAdmin() { + return createSupabaseClient( + process.env.NEXT_PUBLIC_SUPABASE_URL!, + process.env.SUPABASE_SERVICE_ROLE_KEY!, + { auth: { autoRefreshToken: false, persistSession: false } } + ) +} + /** * Student plan-change (switch) UX — issue #463 (part of #458). * @@ -13,14 +24,20 @@ import { TENANT_BASE } from './utils/constants' * dead-end "contact your school" note. * * These are render-level assertions of the plan-change integration — the parts - * that are deterministic in the E2E harness. The actual supersession mutation is - * exercised directly against the DB primitive `change_subscription_plan` - * (old → canceled, new → active, shared entitlements kept / old-only revoked, - * plus the same_plan / no_active_subscription / cross-tenant guards) and the - * `changePlan` server-action orchestration + provider swaps are pinned by the - * vitest suites (`tests/unit/plan-change-{action,provider}.test.ts`). The - * in-dialog "Switch" click is intentionally not driven here: base-ui dialogs are - * not reliably automatable in this harness (see PRs #460/#467). + * that are deterministic in the E2E harness. The in-dialog "Switch" click is + * intentionally not driven here: base-ui dialogs are not reliably automatable in + * this harness (see PRs #460/#467). + * + * The supersession mutation itself lives in + * `subscription-plan-change-rpc.spec.ts`, which drives + * `change_subscription_plan` against a seeded DB as a real authenticated + * student: A → B, A → B → A, cancel-then-switch, same_plan, cross-tenant, the + * price gate and the advisory lock, asserting the entitlement delta per course. + * This file used to CLAIM that coverage while nothing exercised the RPC at all, + * which is how two of the three #545 bugs shipped — including a `cancel_at = + * NULL` write into a NOT NULL column that failed every A → B → A round trip. + * The `changePlan` server-action orchestration + provider swaps are pinned by + * the vitest suites (`tests/unit/plan-change-{action,provider}.test.ts`). */ test.describe('Student plan-change (switch) UX', () => { @@ -49,3 +66,49 @@ test.describe('Student plan-change (switch) UX', () => { await expect(page.getByTestId('change-plan-button')).toBeVisible() }) }) + +/** + * A `past_due` subscription is still LIVE — it bills and it grants access — but + * the billing page only treated `active`/`renewed` as the current subscription, + * so a student mid-dunning got the "no subscription" empty state: no cancel, no + * switch, no explanation, while the school kept charging them (#545). Both + * server actions accept `past_due`; the UI simply never offered it. + */ +test.describe('Student billing — past_due subscription', () => { + test('renders the subscription with a dunning notice and full management actions', async ({ + page, + }) => { + const admin = getAdmin() + const { data: before } = await admin + .from('subscriptions') + .select('subscription_id, subscription_status') + .eq('user_id', ALICE_ID) + .eq('plan_id', 2001) + .single() + expect(before).not.toBeNull() + + await admin + .from('subscriptions') + .update({ subscription_status: 'past_due' }) + .eq('subscription_id', before!.subscription_id) + + try { + await loginAsTenantStudent(page) + await page.goto(`${TENANT_BASE}/en/dashboard/student/billing`) + + // Not the empty state: the real card, badged past due. + await expect(page.getByText('Past due', { exact: true })).toBeVisible() + await expect(page.getByText(/last payment didn't go through/i)).toBeVisible() + + // …and the student can still act on it. + await expect(page.getByTestId('change-plan-button')).toBeVisible() + await expect(page.getByTestId('cancel-subscription-button')).toBeVisible() + } finally { + // Restore the seeded state for every spec that depends on it. + await admin + .from('subscriptions') + .update({ subscription_status: before!.subscription_status }) + .eq('subscription_id', before!.subscription_id) + } + }) +}) diff --git a/tests/playwright/subscription-plan-change-rpc.spec.ts b/tests/playwright/subscription-plan-change-rpc.spec.ts new file mode 100644 index 00000000..4a4455bd --- /dev/null +++ b/tests/playwright/subscription-plan-change-rpc.spec.ts @@ -0,0 +1,534 @@ +/** + * `change_subscription_plan` against a seeded database — issue #545 (EPIC #540 §2.2). + * + * The supersession primitive had never been exercised end to end. `plan-change.spec.ts` + * claimed this coverage existed ("exercised directly against the DB primitive + * change_subscription_plan"); it did not, and two of the three bugs #545 lists + * were sitting in the parts nothing ran: + * + * * the reactivate-an-existing-row branch wrote `cancel_at = NULL` into a + * NOT NULL column, so EVERY switch back to a previously-held plan died with + * 23502 — the canonical A → B → A downgrade could never complete; + * * nothing anywhere compared prices, so a student on a one-click FREE plan + * could switch to the school's paid manual plan and receive its entitlements + * with no transaction and no payment_requests row to reconcile. + * + * These drive the RPC as a REAL authenticated student (role `authenticated`, + * `auth.uid()` = the caller) because that is the surface it is granted on, and + * assert the ENTITLEMENT DELTA PER COURSE — shared courses kept, old-only + * courses revoked — which is the thing a student actually feels. + * + * Fixtures are private to this spec: a dedicated QA user and QA plans, created + * and torn down here, so the shared seed rows (alice's plan-2001 subscription, + * which half the suite depends on) are never touched. + */ +import { test, expect } from '@playwright/test' +import { createClient as createSupabaseClient } from '@supabase/supabase-js' +import type { SupabaseClient } from '@supabase/supabase-js' + +const CODE_ACADEMY = '00000000-0000-0000-0000-000000000002' +const DEFAULT_TENANT = '00000000-0000-0000-0000-000000000001' + +/** Code Academy courses — SHARED sits in both QA plans, ALPHA_ONLY in one. */ +const COURSE_SHARED = 2001 +const COURSE_ALPHA_ONLY = 2002 +/** A Default School course, for the cross-tenant plan. */ +const COURSE_OTHER_TENANT = 1001 + +const QA_EMAIL = 'qa-545-plan-change@e2etest.com' +const QA_PASSWORD = 'password123' +const QA_PLAN_PREFIX = '[E2E 545]' + +const FAR_FUTURE_YEAR = 2126 + +function getAdmin(): SupabaseClient { + return createSupabaseClient( + process.env.NEXT_PUBLIC_SUPABASE_URL!, + process.env.SUPABASE_SERVICE_ROLE_KEY!, + { auth: { autoRefreshToken: false, persistSession: false } } + ) +} + +const admin = getAdmin() + +/** The plan ids created in beforeAll. */ +const plans = { + /** price 19 · courses SHARED + ALPHA_ONLY — the starting plan. */ + alpha: 0, + /** price 19 · courses SHARED — same price, narrower course set. */ + beta: 0, + /** price 99 · an upgrade nothing settles. */ + premium: 0, + /** price 0 · the one-click free plan. */ + free: 0, + /** Another school's plan entirely. */ + foreign: 0, +} + +let qaUserId = '' +/** User-scoped client for the QA student — role `authenticated`. */ +let qa: SupabaseClient + +async function createPlan(opts: { + name: string + price: number + tenantId: string + courseIds: number[] +}): Promise { + const { data, error } = await admin + .from('plans') + .insert({ + plan_name: `${QA_PLAN_PREFIX} ${opts.name}`, + price: opts.price, + duration_in_days: 30, + currency: 'usd', + payment_provider: 'manual', + tenant_id: opts.tenantId, + }) + .select('plan_id') + .single() + expect(error, `create plan ${opts.name}`).toBeNull() + const planId = data!.plan_id as number + + if (opts.courseIds.length > 0) { + const { error: linkError } = await admin + .from('plan_courses') + .insert(opts.courseIds.map((course_id) => ({ plan_id: planId, course_id }))) + expect(linkError, `link courses for ${opts.name}`).toBeNull() + } + return planId +} + +/** + * Wipe the QA student's billing state. Deleting the transactions cascades the + * subscriptions (subscriptions_transaction_id_fkey ON DELETE CASCADE), which + * also clears the #459 backstop so the next seed can create a fresh one. + */ +async function resetBilling() { + await admin.from('entitlements').delete().eq('user_id', qaUserId) + await admin.from('transactions').delete().eq('user_id', qaUserId) +} + +/** + * Put the student on `planId` the normal way: a settled transaction, whose + * after_transaction_insert trigger runs handle_new_subscription (subscription + * row + plan_courses entitlements). + */ +async function subscribeTo(planId: number, amount: number) { + const { error } = await admin.from('transactions').insert({ + user_id: qaUserId, + tenant_id: CODE_ACADEMY, + plan_id: planId, + amount, + currency: 'usd', + payment_method: 'manual', + status: 'successful', + }) + expect(error, `seed subscription for plan ${planId}`).toBeNull() +} + +interface SubSnapshot { + subscription_id: number + plan_id: number + subscription_status: string + cancel_at: string | null + cancel_at_period_end: boolean + current_period_end: string + superseded_by: number | null + payment_provider: string +} + +async function subscriptions(): Promise { + const { data, error } = await admin + .from('subscriptions') + .select( + 'subscription_id, plan_id, subscription_status, cancel_at, cancel_at_period_end, current_period_end, superseded_by, payment_provider' + ) + .eq('user_id', qaUserId) + .order('subscription_id', { ascending: true }) + expect(error).toBeNull() + return (data ?? []) as SubSnapshot[] +} + +interface CourseAccess { + /** Any ACTIVE entitlement for this course — what has_course_access answers. */ + active: boolean + /** The subscription granting the active entitlement, if there is one. */ + sourceId: number | null + expiresAt: string | null +} + +/** + * course_id → effective access, across every entitlement row for that course. + * + * A supersession leaves MORE THAN ONE row per course: the unique key is + * (user_id, course_id, source_type, source_id), so a course shared by the old + * and new plan ends up with the old subscription's row expired and the new + * subscription's row active. Reading "the" row for a course would pick one of + * them arbitrarily; access is the OR over all of them. + */ +async function accessByCourse(): Promise> { + const { data, error } = await admin + .from('entitlements') + .select('course_id, status, source_id, expires_at') + .eq('user_id', qaUserId) + .eq('source_type', 'subscription') + expect(error).toBeNull() + + const map: Record = {} + for (const row of data ?? []) { + const courseId = row.course_id as number + map[courseId] ??= { active: false, sourceId: null, expiresAt: null } + if (row.status === 'active') { + map[courseId] = { + active: true, + sourceId: row.source_id as number | null, + expiresAt: row.expires_at as string | null, + } + } + } + return map +} + +/** The DB's own access oracle — the check every gated page/route runs. */ +async function hasCourseAccess(courseId: number): Promise { + const { data, error } = await admin.rpc('has_course_access', { + _user_id: qaUserId, + _course_id: courseId, + }) + expect(error, error?.message).toBeNull() + return data === true +} + +async function switchTo(planId: number) { + return qa.rpc('change_subscription_plan', { _new_plan_id: planId }) +} + +/** The single live subscription, asserted to be unique. */ +function soleLive(rows: SubSnapshot[]): SubSnapshot { + const live = rows.filter((r) => ['active', 'renewed', 'past_due'].includes(r.subscription_status)) + expect(live, 'exactly one live subscription').toHaveLength(1) + return live[0] +} + +test.describe.configure({ mode: 'serial' }) + +test.beforeAll(async () => { + // A dedicated student — created through the auth admin API so + // handle_new_user() fires and the profile exists. + const existing = await admin.auth.admin.listUsers({ page: 1, perPage: 1000 }) + const previous = existing.data?.users?.find((u) => u.email === QA_EMAIL) + if (previous) await admin.auth.admin.deleteUser(previous.id) + + const { data: created, error: createError } = await admin.auth.admin.createUser({ + email: QA_EMAIL, + password: QA_PASSWORD, + email_confirm: true, + }) + expect(createError, 'create QA user').toBeNull() + qaUserId = created!.user!.id + + const { error: memberError } = await admin + .from('tenant_users') + .insert({ tenant_id: CODE_ACADEMY, user_id: qaUserId, role: 'student', status: 'active' }) + expect(memberError, 'add QA user to Code Academy').toBeNull() + + plans.alpha = await createPlan({ + name: 'Alpha', + price: 19, + tenantId: CODE_ACADEMY, + courseIds: [COURSE_SHARED, COURSE_ALPHA_ONLY], + }) + // Same price as Alpha on purpose: an A → B → A round trip must not be + // refused by the price gate, so the reactivate-existing-row branch (the + // 23502 crash site) is what gets exercised. + plans.beta = await createPlan({ + name: 'Beta', + price: 19, + tenantId: CODE_ACADEMY, + courseIds: [COURSE_SHARED], + }) + plans.premium = await createPlan({ + name: 'Premium', + price: 99, + tenantId: CODE_ACADEMY, + courseIds: [COURSE_SHARED, COURSE_ALPHA_ONLY], + }) + plans.free = await createPlan({ + name: 'Free', + price: 0, + tenantId: CODE_ACADEMY, + courseIds: [COURSE_SHARED], + }) + plans.foreign = await createPlan({ + name: 'Foreign', + price: 19, + tenantId: DEFAULT_TENANT, + courseIds: [COURSE_OTHER_TENANT], + }) + + qa = createSupabaseClient( + process.env.NEXT_PUBLIC_SUPABASE_URL!, + process.env.NEXT_PUBLIC_SUPABASE_PUBLISHABLE_OR_ANON_KEY!, + { auth: { autoRefreshToken: false, persistSession: false } } + ) + const { error: signInError } = await qa.auth.signInWithPassword({ + email: QA_EMAIL, + password: QA_PASSWORD, + }) + expect(signInError, 'sign in as QA student').toBeNull() +}) + +test.afterAll(async () => { + if (qaUserId) { + await resetBilling() + await admin.auth.admin.deleteUser(qaUserId) + } + const planIds = Object.values(plans).filter(Boolean) + if (planIds.length > 0) { + await admin.from('plan_courses').delete().in('plan_id', planIds) + await admin.from('plans').delete().in('plan_id', planIds) + } +}) + +test.beforeEach(async () => { + await resetBilling() +}) + +test.describe('change_subscription_plan — supersession', () => { + test('A → B: old subscription superseded, shared course kept, old-only course revoked', async () => { + await subscribeTo(plans.alpha, 19) + + const before = await accessByCourse() + expect(before[COURSE_SHARED].active).toBe(true) + expect(before[COURSE_ALPHA_ONLY].active).toBe(true) + + const { error } = await switchTo(plans.beta) + expect(error, error?.message).toBeNull() + + const rows = await subscriptions() + const live = soleLive(rows) + expect(live.plan_id).toBe(plans.beta) + + const old = rows.find((r) => r.plan_id === plans.alpha)! + expect(old.subscription_status).toBe('canceled') + expect(old.superseded_by).toBe(live.subscription_id) + + // The entitlement delta is the whole point: Beta covers SHARED only. + const after = await accessByCourse() + expect(after[COURSE_SHARED].active).toBe(true) + expect(after[COURSE_SHARED].sourceId).toBe(live.subscription_id) + expect(after[COURSE_ALPHA_ONLY].active).toBe(false) + // Cross-checked against the oracle every gated route uses. + expect(await hasCourseAccess(COURSE_SHARED)).toBe(true) + expect(await hasCourseAccess(COURSE_ALPHA_ONLY)).toBe(false) + }) + + test('A → B → A round trip succeeds and restores the old-only course', async () => { + // The regression that shipped: the return leg reactivates the existing + // (user, Alpha) row — the branch that wrote NULL into a NOT NULL cancel_at + // and failed with 23502 every single time. + await subscribeTo(plans.alpha, 19) + + const out = await switchTo(plans.beta) + expect(out.error, out.error?.message).toBeNull() + + const back = await switchTo(plans.alpha) + expect(back.error, back.error?.message).toBeNull() + + const rows = await subscriptions() + expect(rows).toHaveLength(2) // reactivated, never duplicated + const live = soleLive(rows) + expect(live.plan_id).toBe(plans.alpha) + expect(live.cancel_at).toBeNull() + expect(live.cancel_at_period_end).toBe(false) + + const beta = rows.find((r) => r.plan_id === plans.beta)! + expect(beta.subscription_status).toBe('canceled') + expect(beta.superseded_by).toBe(live.subscription_id) + + const after = await accessByCourse() + expect(after[COURSE_SHARED].active).toBe(true) + expect(after[COURSE_ALPHA_ONLY].active).toBe(true) + expect(after[COURSE_ALPHA_ONLY].sourceId).toBe(live.subscription_id) + expect(await hasCourseAccess(COURSE_SHARED)).toBe(true) + expect(await hasCourseAccess(COURSE_ALPHA_ONLY)).toBe(true) + }) + + test('cancel → switch carries the pending cancel to the new plan', async () => { + await subscribeTo(plans.alpha, 19) + const [initial] = await subscriptions() + await admin + .from('subscriptions') + .update({ + cancel_at_period_end: true, + cancel_at: initial.current_period_end, + canceled_at: new Date().toISOString(), + }) + .eq('subscription_id', initial.subscription_id) + + const { error } = await switchTo(plans.beta) + expect(error, error?.message).toBeNull() + + // The student asked to stop at period end and Stripe still holds that + // instruction (the in-place swap never clears it), so our row must agree — + // it used to be reset to "renews", hiding the resume affordance too. + const live = soleLive(await subscriptions()) + expect(live.plan_id).toBe(plans.beta) + expect(live.cancel_at_period_end).toBe(true) + expect(live.cancel_at).not.toBeNull() + expect(new Date(live.cancel_at!).getTime()).toBe(new Date(live.current_period_end).getTime()) + }) + + test('same_plan is refused and writes nothing', async () => { + await subscribeTo(plans.alpha, 19) + const before = await subscriptions() + + const { error } = await switchTo(plans.alpha) + expect(error?.message).toContain('same_plan') + + expect(await subscriptions()).toEqual(before) + }) + + test('a plan in another school is unreachable', async () => { + await subscribeTo(plans.alpha, 19) + + // The RPC derives the tenant from the TARGET plan, so the caller's Code + // Academy subscription is not even a candidate to supersede. + const { error } = await switchTo(plans.foreign) + expect(error?.message).toContain('no_active_subscription') + + const rows = await subscriptions() + expect(rows).toHaveLength(1) + expect(rows[0].plan_id).toBe(plans.alpha) + expect(rows[0].subscription_status).toBe('active') + + const after = await accessByCourse() + expect(after[COURSE_OTHER_TENANT]).toBeUndefined() + }) +}) + +test.describe('change_subscription_plan — price authority', () => { + test('a free plan cannot be switched to a paid one without a settled transaction', async () => { + // The exact #545 bug 3 path: one-click free activation, then Billing → + // Change plan → the school's paid manual plan. + const { error: grantError } = await qa.rpc('grant_free_subscription', { + _user_id: qaUserId, + _plan_id: plans.free, + }) + expect(grantError, grantError?.message).toBeNull() + + const freeSub = soleLive(await subscriptions()) + expect(freeSub.plan_id).toBe(plans.free) + expect(new Date(freeSub.current_period_end).getUTCFullYear()).toBe(FAR_FUTURE_YEAR) + + const { error } = await switchTo(plans.alpha) + expect(error?.message).toContain('upgrade_requires_payment') + + // Nothing moved: no Alpha subscription, no Alpha-only entitlement, and the + // free plan is exactly as it was. + const rows = await subscriptions() + expect(rows).toHaveLength(1) + expect(rows[0].plan_id).toBe(plans.free) + expect(rows[0].subscription_status).toBe('active') + + const after = await accessByCourse() + expect(after[COURSE_ALPHA_ONLY]).toBeUndefined() + expect(await hasCourseAccess(COURSE_ALPHA_ONLY)).toBe(false) + + // And no money was recorded for the upgrade that did not happen. + const { data: txns } = await admin + .from('transactions') + .select('transaction_id, plan_id, status') + .eq('user_id', qaUserId) + expect((txns ?? []).some((t) => t.plan_id === plans.alpha)).toBe(false) + }) + + test('a self-managed subscription cannot be upgraded to a pricier plan', async () => { + await subscribeTo(plans.alpha, 19) + + const { error } = await switchTo(plans.premium) + expect(error?.message).toContain('upgrade_requires_payment') + + const live = soleLive(await subscriptions()) + expect(live.plan_id).toBe(plans.alpha) + }) + + test('a same-price switch is allowed (the gate is about paying more, not about switching)', async () => { + await subscribeTo(plans.alpha, 19) + const { error } = await switchTo(plans.beta) + expect(error, error?.message).toBeNull() + expect(soleLive(await subscriptions()).plan_id).toBe(plans.beta) + }) + + test('switching down to a free plan grants the never-expiring period', async () => { + await subscribeTo(plans.alpha, 19) + + const { error } = await switchTo(plans.free) + expect(error, error?.message).toBeNull() + + const live = soleLive(await subscriptions()) + expect(live.plan_id).toBe(plans.free) + // Free subscriptions never expire — same horizon grant_free_subscription + // uses, so the plan does not silently lapse when the prepaid period of the + // plan they left runs out. + expect(new Date(live.current_period_end).getUTCFullYear()).toBe(FAR_FUTURE_YEAR) + + const after = await accessByCourse() + expect(after[COURSE_SHARED].active).toBe(true) + expect(new Date(after[COURSE_SHARED].expiresAt!).getUTCFullYear()).toBe(FAR_FUTURE_YEAR) + expect(after[COURSE_ALPHA_ONLY].active).toBe(false) + }) +}) + +test.describe('change_subscription_plan — concurrency', () => { + test('a double submit leaves exactly one live subscription (advisory lock)', async () => { + await subscribeTo(plans.alpha, 19) + + // Two calls in flight at once — the dialog's `pending` disable is per-tab, + // so this is a plain double click across two tabs. Without + // pg_advisory_xact_lock both read the same live Alpha row and race. + const [first, second] = await Promise.all([switchTo(plans.beta), switchTo(plans.beta)]) + const results = [first, second] + + const succeeded = results.filter((r) => !r.error) + const failed = results.filter((r) => r.error) + expect(succeeded).toHaveLength(1) + expect(failed).toHaveLength(1) + // The loser blocked on the lock, then saw the committed state: it is + // already on Beta. `same_plan` is precisely the error the checkout action + // must NOT compensate a provider swap for. + expect(failed[0].error!.message).toContain('same_plan') + + const rows = await subscriptions() + expect(rows).toHaveLength(2) + const live = soleLive(rows) + expect(live.plan_id).toBe(plans.beta) + expect(rows.find((r) => r.plan_id === plans.alpha)!.subscription_status).toBe('canceled') + + const after = await accessByCourse() + expect(after[COURSE_SHARED].active).toBe(true) + expect(after[COURSE_ALPHA_ONLY].active).toBe(false) + }) +}) + +test.describe('subscriptions cancel-state contract', () => { + test('no live subscription carries a cancel date it has not scheduled', async () => { + await subscribeTo(plans.alpha, 19) + + // The acceptance criterion for #545 bug 1, over the whole table: a row born + // with cancel_at = now() is what made the Solana crank cancel every + // subscription at its first rollover instead of renewing it. + const { data, error } = await admin + .from('subscriptions') + .select('subscription_id') + .eq('subscription_status', 'active') + .lte('cancel_at', new Date().toISOString()) + expect(error).toBeNull() + expect(data ?? []).toHaveLength(0) + + const fresh = soleLive(await subscriptions()) + expect(fresh.cancel_at).toBeNull() + expect(fresh.cancel_at_period_end).toBe(false) + }) +}) diff --git a/tests/unit/plan-change-action.test.ts b/tests/unit/plan-change-action.test.ts index aa094062..237783c2 100644 --- a/tests/unit/plan-change-action.test.ts +++ b/tests/unit/plan-change-action.test.ts @@ -139,6 +139,43 @@ describe('changePlan — native providers (Stripe / Lemon Squeezy)', () => { prorationBehavior: 'none', })) }) + + // #545: the compensating revert used to fire on ANY rpcError, including the + // two the RPC raises before writing anything. On a double-click that meant: + // call A swaps Stripe to plan B and commits the DB; call B swaps to B again, + // gets `same_plan`, and reverts Stripe to plan A's price with + // proration_behavior 'none' — DB on B, Stripe billing A, and the only thing + // the student sees is "You are already on this plan." + for (const benign of ['same_plan', 'no_active_subscription']) { + it(`does NOT revert the provider swap on ${benign} (DB already where the student wanted it)`, async () => { + state.targetPlan = { plan_id: 2, payment_provider: 'stripe', provider_price_id: 'price_new', tenant_id: 't1', deleted_at: null } + state.current = { subscription_id: 10, plan_id: 1, payment_provider: 'stripe', provider_subscription_id: 'sub_x' } + state.currentPlan = { provider_price_id: 'price_old' } + state.rpcError = { message: `${benign}: raised by change_subscription_plan` } + + await expect(changePlan('2')).rejects.toThrow() + // Only the forward swap — the provider stays on the new plan's price. + expect(state.updateSub).toHaveBeenCalledTimes(1) + expect(state.updateSub.mock.calls[0][1]).toEqual(expect.objectContaining({ + newProviderPriceId: 'price_new', + })) + }) + } + + it('surfaces upgrade_requires_payment as an actionable message, without reverting blindly', async () => { + state.targetPlan = { plan_id: 2, payment_provider: 'stripe', provider_price_id: 'price_new', tenant_id: 't1', deleted_at: null } + state.current = { subscription_id: 10, plan_id: 1, payment_provider: 'stripe', provider_subscription_id: 'sub_x' } + state.currentPlan = { provider_price_id: 'price_old' } + state.rpcError = { message: 'upgrade_requires_payment' } + + // The switch genuinely did not apply, so the provider IS put back. + await expect(changePlan('2')).rejects.toThrow(/costs more than your current one/) + expect(state.updateSub).toHaveBeenCalledTimes(2) + expect(state.updateSub.mock.calls[1][1]).toEqual(expect.objectContaining({ + newProviderPriceId: 'price_old', + prorationBehavior: 'none', + })) + }) }) describe('changePlan — self-managed providers (manual)', () => { diff --git a/tests/unit/solana-pull-decision.test.ts b/tests/unit/solana-pull-decision.test.ts index dcaada40..3e0ddfaf 100644 --- a/tests/unit/solana-pull-decision.test.ts +++ b/tests/unit/solana-pull-decision.test.ts @@ -3,11 +3,23 @@ import { decidePullAction, type PullDecisionRow, type PullDecisionState } from ' /** * The crank must NEVER charge a subscription the DB no longer considers active, - * and must finalize (not renew) a subscription that reached its period-end - * cancel (issue #460). These tests pin that gate. + * must finalize (not renew) a subscription that reached its period-end cancel + * (issue #460) — and must RENEW everything else (issue #545). + * + * These fixtures use values the `subscriptions` table can actually hold. The + * suite previously fixed `cancel_at: null` on a column that was + * `NOT NULL DEFAULT timezone('utc', now())`, so it tested a row shape that + * could not exist: every real row carried a cancel date in the past, the + * `cancel_at <= now` branch fired at the first rollover, and native + * `solana_subs` recurring billing could never renew — green tests the whole + * time. `20260726120000_subscription_cancel_at_contract.sql` made `cancel_at` + * nullable with the invariant `cancel_at IS NULL OR cancel_at_period_end`, so + * the three shapes below are now the only ones the schema permits, plus the + * legacy poisoned shape that used to be universal. */ const NOW = 1_700_000_000 // fixed unix seconds +const iso = (unixSec: number) => new Date(unixSec * 1000).toISOString() // An active, freshly-started period: started 2h ago, 24h period → not due. const activeState: PullDecisionState = { @@ -16,19 +28,38 @@ const activeState: PullDecisionState = { periodHours: BigInt(24), } -// A period that has rolled over: started 25h ago, 24h period → due. +// A period that has rolled over: started 25h ago, 24h period → due 1h ago. const dueState: PullDecisionState = { expiresAtTs: BigInt(0), currentPeriodStartTs: BigInt(NOW - 25 * 3600), periodHours: BigInt(24), } +/** Renewing normally: no cancel scheduled, so no cancel date (#545 contract). */ const activeRow: PullDecisionRow = { subscription_status: 'active', cancel_at_period_end: false, cancel_at: null, } +/** Scheduled to cancel: the flag is set and the date is this period's end. */ +const cancelScheduledRow: PullDecisionRow = { + subscription_status: 'active', + cancel_at_period_end: true, + cancel_at: iso(NOW - 3600), +} + +/** + * What every row looked like before the #545 migration: `cancel_at` defaulted + * to the row's creation time and no writer ever cleared it, while nobody had + * scheduled anything. This shape must renew. + */ +const legacyPoisonedRow: PullDecisionRow = { + subscription_status: 'active', + cancel_at_period_end: false, + cancel_at: iso(NOW - 30 * 24 * 3600), +} + describe('decidePullAction', () => { it('pulls when active and the period has rolled over', () => { const d = decidePullAction({ row: activeRow, state: dueState, nowSec: NOW }) @@ -52,17 +83,36 @@ describe('decidePullAction', () => { }) it('finalizes to canceled (does NOT pull) when set to cancel_at_period_end and due', () => { + const d = decidePullAction({ row: cancelScheduledRow, state: dueState, nowSec: NOW }) + expect(d.action).toBe('cancel') + }) + + // ── #545: a cancel DATE is not a cancel SCHEDULE ────────────────────────── + it('RENEWS a due subscription whose cancel_at is in the past but is not scheduled to cancel', () => { + // The shipped bug: `cancel_at` defaulted to now() at INSERT, so this was + // EVERY solana_subs row. The crank canceled instead of pulling — the + // student lost access at period end and the school was never paid again. + const d = decidePullAction({ row: legacyPoisonedRow, state: dueState, nowSec: NOW }) + expect(d.action).toBe('pull') + if (d.action === 'pull') { + expect(d.periodEndSec).toBe(BigInt(NOW - 3600)) + } + }) + + it('renews a due subscription whose cancel_at was left behind by a reactivate', () => { + // Both reactivate paths now clear cancel_at with the flag, but a row that + // predates that (or any future writer that forgets) must still renew. const d = decidePullAction({ - row: { ...activeRow, cancel_at_period_end: true }, + row: { ...activeRow, cancel_at: iso(NOW + 30 * 24 * 3600) }, state: dueState, nowSec: NOW, }) - expect(d.action).toBe('cancel') + expect(d.action).toBe('pull') }) - it('finalizes to canceled when cancel_at has passed and the period is due', () => { + it('cancels on the flag alone, even with no cancel_at recorded', () => { const d = decidePullAction({ - row: { ...activeRow, cancel_at: new Date((NOW - 3600) * 1000).toISOString() }, + row: { ...cancelScheduledRow, cancel_at: null }, state: dueState, nowSec: NOW, }) @@ -71,11 +121,7 @@ describe('decidePullAction', () => { it('does not finalize a cancel-scheduled row until the period actually rolls', () => { // cancel_at_period_end is set, but the period is not due yet → still just skip. - const d = decidePullAction({ - row: { ...activeRow, cancel_at_period_end: true }, - state: activeState, - nowSec: NOW, - }) + const d = decidePullAction({ row: cancelScheduledRow, state: activeState, nowSec: NOW }) expect(d.action).toBe('skip') }) @@ -106,4 +152,23 @@ describe('decidePullAction', () => { }) expect(d.action).toBe('skip') }) + + it('renews period after period — a subscription is never spuriously finalized', () => { + // Three consecutive rollovers of the same never-canceled subscription. The + // acceptance criterion for #545: a solana_subs subscription RENEWS at + // period end, with the column values a real row carries. + const periodHours = BigInt(24) + let periodStart = BigInt(NOW - 25 * 3600) + for (let period = 0; period < 3; period++) { + const at = Number(periodStart + periodHours * BigInt(3600)) + 60 + const d = decidePullAction({ + row: activeRow, + state: { expiresAtTs: BigInt(0), currentPeriodStartTs: periodStart, periodHours }, + nowSec: at, + }) + expect(d.action).toBe('pull') + if (d.action !== 'pull') return + periodStart = d.periodEndSec + } + }) }) diff --git a/tests/unit/subscription-state-machine.test.ts b/tests/unit/subscription-state-machine.test.ts new file mode 100644 index 00000000..0d5e00a6 --- /dev/null +++ b/tests/unit/subscription-state-machine.test.ts @@ -0,0 +1,243 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +/** + * Pins the subscription STATE MACHINE around cancel / reactivate / parallel + * guards (issue #545, EPIC #540 §2.2). Three rules, each of which shipped + * broken: + * + * 1. Cancelling never IMPROVES a status. Both cancel actions wrote + * `subscription_status: 'active'` unconditionally alongside the cancel + * fields, so a student mid-dunning who cancelled had their `past_due` row + * rewritten as healthy and the delinquency vanished from billing health + * and the admin views. + * 2. Reactivating clears `cancel_at` WITH the flag. It used to push the date + * forward instead (the column was NOT NULL), leaving a live-looking cancel + * date behind; the CHECK added in 20260726120000 now rejects that. + * 3. `renewed` blocks a parallel checkout. It grants access and still bills, + * but neither #459 guard listed it, so a `renewed` subscriber could buy a + * second plan and be billed twice. + * + * The actions build their Supabase client from module imports, so those are + * mocked; the assertions are on the exact patch each action writes. + */ + +interface SubRow { + subscription_id: number + user_id: string + tenant_id: string + plan_id: number + subscription_status: string + cancel_at_period_end: boolean | null + current_period_end: string | null + end_date: string + payment_provider: string + provider_subscription_id: string | null +} + +const TENANT = 't1' +const USER = 'u1' + +const state: { + subscription: SubRow | null + updates: { table: string; values: Record }[] +} = { subscription: null, updates: [] } + +function subRow(overrides: Partial = {}): SubRow { + return { + subscription_id: 10, + user_id: USER, + tenant_id: TENANT, + plan_id: 1, + subscription_status: 'active', + cancel_at_period_end: false, + current_period_end: '2026-09-01T00:00:00.000Z', + end_date: '2026-09-01T00:00:00.000Z', + payment_provider: 'manual', + provider_subscription_id: null, + ...overrides, + } +} + +function makeAdminClient() { + function builder(table: string) { + const b: Record = { + select: () => b, + eq: () => b, + insert: () => b, + update: (values: Record) => { + state.updates.push({ table, values }) + return b + }, + single: () => + Promise.resolve( + table === 'subscriptions' + ? { data: state.subscription, error: state.subscription ? null : { message: 'not found' } } + : { data: null, error: null }, + ), + maybeSingle: () => Promise.resolve({ data: null, error: null }), + // Awaitable terminal so `await admin.from(x).update(y).eq().eq()` resolves. + then: (resolve: (v: { data: null; error: null }) => unknown) => + Promise.resolve({ data: null, error: null }).then(resolve), + } + return b + } + return { from: (t: string) => builder(t) } +} + +vi.mock('next/cache', () => ({ revalidatePath: vi.fn() })) +vi.mock('@/lib/supabase/admin', () => ({ + createAdminClient: () => makeAdminClient(), + verifyAdminAccess: () => Promise.resolve(true), +})) +vi.mock('@/lib/supabase/get-user-role', () => ({ isSuperAdmin: () => Promise.resolve(false) })) +vi.mock('@/lib/supabase/tenant', () => ({ + getCurrentUserId: () => Promise.resolve(USER), + getCurrentTenantId: () => Promise.resolve(TENANT), +})) +vi.mock('@/lib/payments', async (importActual) => { + const actual = await importActual() + return { ...actual, getPaymentProvider: () => ({}) } +}) + +import { cancelMySubscription, reactivateMySubscription } from '@/app/actions/subscriptions' +import { cancelSubscription as adminCancelSubscription } from '@/app/actions/admin/subscriptions' +import { + BLOCKING_SUBSCRIPTION_STATUSES, + findConflictingSubscription, +} from '@/lib/payments/subscription-guard' + +/** The one `subscriptions` patch the action under test wrote. */ +function soleSubscriptionPatch() { + const subs = state.updates.filter((u) => u.table === 'subscriptions') + expect(subs).toHaveLength(1) + return subs[0].values +} + +beforeEach(() => { + state.subscription = null + state.updates = [] +}) + +describe('cancelMySubscription — cancelling never improves a status', () => { + it('leaves a past_due subscription past_due', async () => { + state.subscription = subRow({ subscription_status: 'past_due' }) + + const result = await cancelMySubscription(10) + + expect(result).toEqual({ success: true, providerWarning: undefined }) + const patch = soleSubscriptionPatch() + expect(patch.subscription_status).toBe('past_due') + expect(patch.cancel_at_period_end).toBe(true) + expect(patch.cancel_at).toBe('2026-09-01T00:00:00.000Z') + }) + + it('keeps an active subscription active', async () => { + state.subscription = subRow({ subscription_status: 'active' }) + await cancelMySubscription(10) + expect(soleSubscriptionPatch().subscription_status).toBe('active') + }) + + it("normalizes the legacy 'renewed' status to active", async () => { + // `renewed` means exactly what `active` means and nothing writes it any + // more — this is the one status the cancel is allowed to rewrite. + state.subscription = subRow({ subscription_status: 'renewed' }) + await cancelMySubscription(10) + expect(soleSubscriptionPatch().subscription_status).toBe('active') + }) + + it('refuses to cancel a subscription that is not live', async () => { + state.subscription = subRow({ subscription_status: 'expired' }) + await expect(cancelMySubscription(10)).resolves.toEqual({ + success: false, + error: 'not_cancelable', + }) + expect(state.updates.filter((u) => u.table === 'subscriptions')).toHaveLength(0) + }) +}) + +describe('reactivateMySubscription — the cancel date dies with the schedule', () => { + it('clears cancel_at together with cancel_at_period_end', async () => { + state.subscription = subRow({ cancel_at_period_end: true }) + + await reactivateMySubscription(10) + + const patch = soleSubscriptionPatch() + expect(patch).toEqual({ + cancel_at_period_end: false, + cancel_at: null, + canceled_at: null, + }) + }) +}) + +describe('admin cancelSubscription — same rule, admin surface', () => { + it('a period-end cancel leaves a past_due subscription past_due', async () => { + state.subscription = subRow({ subscription_status: 'past_due' }) + + await adminCancelSubscription(10, false) + + const patch = soleSubscriptionPatch() + expect(patch.subscription_status).toBe('past_due') + expect(patch.cancel_at_period_end).toBe(true) + }) + + it('an immediate cancel is still a cancel', async () => { + state.subscription = subRow({ subscription_status: 'past_due' }) + + await adminCancelSubscription(10, true) + + const patch = soleSubscriptionPatch() + expect(patch.subscription_status).toBe('canceled') + expect(patch.cancel_at_period_end).toBeUndefined() + }) +}) + +describe('parallel-subscription guard — renewed is live', () => { + it("lists 'renewed' as blocking", () => { + expect(BLOCKING_SUBSCRIPTION_STATUSES).toEqual( + expect.arrayContaining(['active', 'renewed', 'past_due']), + ) + }) + + it('blocks a second plan checkout while a renewed subscription is held', async () => { + const queried: { statuses?: string[] } = {} + const supabase = { + from: () => { + const b: Record = { + select: () => b, + eq: () => b, + in: (_col: string, values: string[]) => { + queried.statuses = values + // The DB only returns rows whose status is in the filter, so a + // `renewed` row is visible here only because the guard asks for it. + return Promise.resolve({ + data: values.includes('renewed') + ? [ + { + subscription_id: 7, + plan_id: 1, + end_date: '2026-09-01', + payment_provider: 'stripe', + plan: { plan_name: 'Pro' }, + }, + ] + : [], + error: null, + }) + }, + } + return b + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any + + const conflict = await findConflictingSubscription(supabase, { + userId: USER, + tenantId: TENANT, + planId: 2, // a DIFFERENT plan — this is the parallel checkout + }) + + expect(queried.statuses).toContain('renewed') + expect(conflict).toMatchObject({ subscription_id: 7, plan_id: 1, plan_name: 'Pro' }) + }) +}) diff --git a/tests/unit/webhook-dispatch.test.ts b/tests/unit/webhook-dispatch.test.ts index cc9633a0..6d7cfc34 100644 --- a/tests/unit/webhook-dispatch.test.ts +++ b/tests/unit/webhook-dispatch.test.ts @@ -72,14 +72,28 @@ function event(type: BillingEventType, extra: Partial = const PROVIDER = 'lemonsqueezy' describe('dispatchBillingEvent', () => { - it('past_due → no subscriptions write and no rpc (log-only)', async () => { + // The dispatcher used to log past_due and drop it, on a stale comment + // claiming the enum had no such value — + // 20260530140000_add_past_due_subscription_status.sql added it, so a student + // mid-dunning looked healthy everywhere (#545). + it('past_due → writes subscription_status="past_due" (access untouched)', async () => { const { admin, calls } = makeFakeAdmin() await dispatchBillingEvent(event('subscription.past_due', { providerSubscriptionId: 'sub_1' }), { provider: PROVIDER, admin, }) - expect(calls.updates).toHaveLength(0) + expect(calls.updates).toHaveLength(1) + expect(calls.updates[0].table).toBe('subscriptions') + expect(calls.updates[0].values).toEqual({ subscription_status: 'past_due' }) + // No ended_at / entitlement change — handle_subscription_status_change + // matches neither branch for past_due, so access rides out the retry window. expect(calls.rpc).toHaveLength(0) + }) + + it('past_due without a provider subscription id → no write', async () => { + const { admin, calls } = makeFakeAdmin() + await dispatchBillingEvent(event('subscription.past_due'), { provider: PROVIDER, admin }) + expect(calls.updates).toHaveLength(0) expect(calls.from).toHaveLength(0) })