Skip to content

fix(payments): claw back refunded subscription payments (#515) - #522

Merged
guillermoscript merged 1 commit into
masterfrom
fix/subscription-refund-clawback-515
Jul 25, 2026
Merged

fix(payments): claw back refunded subscription payments (#515)#522
guillermoscript merged 1 commit into
masterfrom
fix/subscription-refund-clawback-515

Conversation

@guillermoscript

Copy link
Copy Markdown
Owner

Closes #515.

Description

The shared refund.succeeded handler in lib/payments/webhook-dispatch.ts required a one-time product purchase before it would do anything:

if (!tx || tx.plan_id || !tx.product_id || tx.status !== 'successful') break

Two of those conditions exclude subscriptions — tx.plan_id truthy exits, and a plan transaction has product_id IS NULL so !tx.product_id exits as well. A refund event whose reference resolved to a subscription transaction therefore returned without writing anything.

The guard's comment ("subscription refunds are owned by subscription.canceled/expired") was accurate about entitlements, but those handlers only write subscriptions.subscription_status and ended_at. Nothing on the unified webhook path ever wrote transactions.status = 'refunded' for a plan row.

All three platform-settled providers do deliver the event with a usable reference, so it arrived and was dropped downstream:

  • lib/payments/paypal-provider.tsPAYMENT.CAPTURE.REFUNDED
  • lib/payments/binance-provider.tsPAY_REFUND / REFUND_SUCCESS
  • lib/payments/lemonsqueezy-provider.tsorder_refunded (Lemon Squeezy raises this for a subscription's first order too)

The legacy Stripe Connect route was already the counter-example: app/api/stripe/webhook/route.ts (charge.refunded) flips the transaction for product and plan rows alike. The unified dispatcher was the outlier.

Impact

getPayoutsOwed() selects transactions with status IN ('successful','refunded') and computeOwedBalances leaves refunded sales out of grossOwed. A subscription refund that never reached refunded therefore stayed inside grossOwed: the platform kept showing the school's share of a payment it had already given back, and paid it out on the next cycle. clawback under-reported for the same reason, since it is derived from the same refunded rows.

Note that the issue was filed before #511 (PR #521) changed the payout arithmetic. The bug is unchanged, but the mechanism is now the grossOwed exclusion rather than a clawback subtraction — this PR is written against current master.

Changes made

lib/payments/webhook-dispatch.ts — split the guard into the two decisions it was conflating:

  1. Record the money, for both kinds of purchase. transactions.statusrefunded for product and plan rows, keeping the .eq('status','successful') guard so a webhook redelivery is a no-op. This is the value the payout computation reads.
  2. Revoke access, for products only — unchanged. No trigger revokes product entitlements (trigger_manage_transactions acts on successful/failed only), so it stays explicit here. Subscription access remains owned by subscription.canceled/subscription.expired, exactly as before.

Rows in a status other than successful, and rows with neither product_id nor plan_id, still break out without writing.

refunded is an existing value of the transaction_status enum and is already written by the legacy Stripe route, so no migration is needed.

lib/payments/lemonsqueezy-provider.ts — comment only. The order_refunded docblock claimed subscription refunds were handled entirely by subscription_cancelled/expired, which is now only true of access, not of the money.

tests/unit/webhook-dispatch.test.ts — five refund.succeeded cases replacing the single "no writes" case.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change
  • Documentation update

Testing

  • npm run test:unit318 passed, 27 files, 0 failures
  • npx vitest run tests/unit/webhook-dispatch.test.ts tests/unit/payouts-owed.test.ts48 passed (22 dispatcher + 26 payout-arithmetic; the Payout clawback double-subtracts refunds and claws back sales never paid out (#498 follow-up) #511 arithmetic is untouched by this change)
  • npm run typecheck — clean, no errors
  • npm run build — production build succeeded
  • npx eslint on the three changed files — 0 errors. 13 warnings, all pre-existing no-unused-vars on underscore-prefixed interface stubs in lemonsqueezy-provider.ts and the test's _cols parameter; none on changed lines.

The regression test was confirmed to fail against the old code

The new #515 case is not a test that would pass either way. With the previous guard restored in place, the suite reports exactly one failure — that case — and passes again once the fix is reapplied:

Tests  1 failed | 21 passed (22)

Live database verification

The unit tests use a fake Supabase client, so the one claim they cannot prove is what the database does when a plan row flips to refunded. Verified directly against the local database inside a transaction that was rolled back:

Check Result
UPDATE transactions SET status='refunded' on a plan row UPDATE 1 — no trigger error
subscriptions.subscription_status afterwards unchanged (active)
entitlements for that user afterwards unchanged (all active)
Re-subscribing to the same plan afterwards now permitted — a new pending row inserts

The first three confirm the flip is inert: after_transaction_updatetrigger_manage_transactions branches on status = 'successful' and status = 'failed', and matches neither. The fourth is a welcome side effect — the row leaves the partial unique index transactions_unique_plan (WHERE product_id IS NULL AND status IN ('pending','successful')), so a refunded student can re-subscribe to that plan. Previously the refunded-but-still-successful row blocked that purchase permanently. The product path already behaved this way.

QA verification steps

No UI changed in this PR, so there are no screenshots. To verify end to end:

  1. Pick (or seed) a subscription transaction on a platform-settled provider — payment_provider in paypal, binance or lemonsqueezy — with plan_id set, product_id null, and status = 'successful'. It also needs school_percentage_snapshot set for the payout figures to be meaningful.
  2. Sign in as a super admin and open /platform/payouts. Note the school's grossOwed and netOwed for that currency.
  3. POST that provider's refund webhook payload to /api/payments/webhook/<provider>, with the transaction id round-tripped as reference (custom_id for PayPal, merchantTradeNo/passthrough for Binance, custom_data for Lemon Squeezy).
  4. Confirm the transaction now reads status = 'refunded'.
  5. Reload /platform/payouts — the school's share of that sale is gone from grossOwed, and netOwed drops by the same amount.
  6. Confirm the student's subscriptions row and entitlements are unchanged — this PR deliberately does not touch access.
  7. Redeliver the same webhook. Nothing changes (the status = 'successful' guard makes it a no-op).
  8. Sanity-check the unchanged path: refund a one-time product purchase and confirm the transaction flips and its entitlements are revoked, as before.

Known limitations (deliberate, not regressions)

Both are pre-existing and unchanged by this PR; recording them so a reviewer does not have to rediscover them:

  • Access is not revoked on refund. Per the issue's scope, entitlement handling stays with subscription.canceled/expired. A refund arriving without an accompanying cancel event books the refund while the student keeps access until the period ends. This PR fixes the money side only.
  • Renewal refunds attach to the first charge. A renewal does not insert a new transaction row (transactions_unique_plan blocks it; extend_subscription_period extends the existing row instead), so a refund of a renewal resolves to the original first-charge transaction and flips that one. The amount is correct for a fixed-price plan, but the row it lands on is the first charge. Fixing this properly needs per-period transaction rows and is out of scope here.

Checklist

  • Tenant filter applied where relevant (this path resolves a single transaction by primary key from a signed provider event; the existing owner-binding guards on payment.succeeded/subscription.activated are untouched)
  • Tested with all relevant roles (payout figures are super-admin only)
  • Loading and error states handled (webhook errors continue to throw with a dispatch <event> prefix)
  • No new migration required

The shared `refund.succeeded` handler required a one-time product purchase
(`if (!tx || tx.plan_id || !tx.product_id || tx.status !== 'successful') break`),
so a refund resolving to a subscription transaction exited without writing
anything. Nothing else writes `transactions.status = 'refunded'` for a plan row
on the unified webhook path, so `getPayoutsOwed()` kept counting the sale inside
`grossOwed` and the school was still paid its share of money the platform had
already given back.

Split the guard into the two decisions it was conflating:

- Record the money for BOTH kinds — flip `transactions.status` to `refunded`,
  status-guarded so a webhook redelivery is a no-op. This is what the payout
  computation reads.
- Revoke access for products ONLY, unchanged. Subscription access stays owned by
  `subscription.canceled`/`subscription.expired`.

Verified against the local database that flipping a plan row is inert: the
`after_transaction_update` trigger branches on `successful`/`failed` only, so
`subscriptions` and `entitlements` are untouched, and the row leaving the
partial unique index `transactions_unique_plan` frees the buyer to re-subscribe
to that plan later.

Adds five `refund.succeeded` cases to `tests/unit/webhook-dispatch.test.ts`,
including the #515 regression (which fails against the previous guard) and a
product case pinning the unchanged entitlement-revoke path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KZ7dtT3dqVphasQxZLFBaZ
@guillermoscript guillermoscript added bug Something isn't working Sub-task severity:medium Medium severity security finding payments Payment provider / billing related labels Jul 25, 2026
@guillermoscript guillermoscript self-assigned this Jul 25, 2026
@guillermoscript
guillermoscript marked this pull request as ready for review July 25, 2026 02:48
@guillermoscript
guillermoscript merged commit 69613aa into master Jul 25, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working payments Payment provider / billing related severity:medium Medium severity security finding Sub-task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Subscription refunds never flip transaction status, so they are never clawed back (#498 follow-up)

1 participant