Skip to content

chore(deleter): extend deleter billing mocks for the delete checks - #1865

Open
whoAbhishekSah wants to merge 1 commit into
mainfrom
org-delete-preflight-mocks
Open

chore(deleter): extend deleter billing mocks for the delete checks#1865
whoAbhishekSah wants to merge 1 commit into
mainfrom
org-delete-preflight-mocks

Conversation

@whoAbhishekSah

@whoAbhishekSah whoAbhishekSah commented Aug 10, 2026

Copy link
Copy Markdown
Member

Part of #1837. Base of the stack; the delete-blocker change sits on top.

Extends the deleter's billing mocks for the delete's up-front checks:

  • credit service mock gets GetBalance
  • subscription service mock gets List and Cancel
  • invoice service mock gets SyncWithProvider
  • new plan service mock (GetByID, used to resolve the standard plan)

The matching interface methods land with the next PR in the stack; the mocks compile standalone.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview Aug 21, 2026 8:15am

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ee5c7a4-e347-433c-9edd-c5808cb3f439

📥 Commits

Reviewing files that changed from the base of the PR and between 338eeb6 and 908fb91.

📒 Files selected for processing (1)
  • core/deleter/mocks/invoice_service.go

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.


📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Expanded service test support for credit balance and transaction operations, payable invoice retrieval, subscription cancellation and listing, and plan lookup.
    • Added flexible configuration for expected calls, return values, callbacks, and error scenarios.
    • Added automatic verification of configured expectations during test cleanup.
    • Improved consistency and reliability when validating service behavior across common success and failure cases.

Walkthrough

The change adds generated Testify mocks for plan, credit, invoice, and subscription service methods. The mocks support invocation handling, configured returns, callbacks, typed expectation helpers, and cleanup assertions where applicable.

Changes

Deleter mock extensions

Layer / File(s) Summary
Plan service mock support
core/deleter/mocks/plan_service.go
Adds the PlanService mock, GetByID handling, typed expectation helpers, and cleanup-based assertion registration.
Credit service mock support
core/deleter/mocks/credit_service.go
Adds GetBalance and List handling with typed Run, Return, RunAndReturn, and expectation helpers.
Invoice payable-list mock support
core/deleter/mocks/invoice_service.go
Adds ListPayableOnProvider handling with typed returns, callbacks, and expectation helpers.
Subscription method mock support
core/deleter/mocks/subscription_service.go
Adds generated mock support for Cancel and List, including typed returns, callbacks, expectations, and the required package import.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 908fb

This change only extends billing-service mocks used by deleter checks, with no actionable merge-blocking risk remaining beyond normal checks and review.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Aug 10, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 32462224935

Coverage remained the same at 48.804%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 40101
Covered Lines: 19571
Line Coverage: 48.8%
Coverage Strength: 15.73 hits per line

💛 - Coveralls

@whoAbhishekSah
whoAbhishekSah force-pushed the org-delete-preflight-mocks branch from 16f57af to 0ba1676 Compare August 10, 2026 06:27
@whoAbhishekSah
whoAbhishekSah changed the base branch from org-delete-ack-proto to main August 13, 2026 10:57
@whoAbhishekSah
whoAbhishekSah force-pushed the org-delete-preflight-mocks branch from 0ba1676 to 2f3076f Compare August 13, 2026 10:57
@whoAbhishekSah whoAbhishekSah changed the title chore(deleter): add GetBalance and subscription List to deleter mocks chore(deleter): extend deleter billing mocks for the pre-flight check Aug 13, 2026
@whoAbhishekSah
whoAbhishekSah force-pushed the org-delete-preflight-mocks branch from 2f3076f to 3f71c2d Compare August 13, 2026 11:15
@whoAbhishekSah
whoAbhishekSah force-pushed the org-delete-preflight-mocks branch from 3f71c2d to 5a8e3f8 Compare August 18, 2026 04:48
@whoAbhishekSah whoAbhishekSah changed the title chore(deleter): extend deleter billing mocks for the pre-flight check chore(deleter): extend deleter billing mocks for the delete checks Aug 18, 2026
@whoAbhishekSah

whoAbhishekSah commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

Tested the whole stack live on the tip branch (#1865#1857#1880#1893#1894), so these results cover all five server PRs combined.

Setup

  • Local server built from org-delete-check-api, Stripe in test mode, real Stripe customers/subs/invoices behind every case.
  • Two test users: an org owner (has delete on the org) and an org viewer (does not). Superadmin via the bootstrap service account.
  • Fixture orgs, one per case: no billing account at all; active pro sub; open $42 Stripe invoice; token balance −500; token balance +100; pro sub + open invoice + debt combined; a disabled org.
  • Subscriptions created through AdminService/DelegatedCheckout with a test card. Invoices through the Stripe API (send_invoice, finalized to open). Debt through UpdateBillingAccountLimits (overdraft floor) + CreateBillingUsage. Credits through a credits-behavior product.
  • Two config rounds: first with billing.customer.default_plan: "", then with default_plan: "starter-e96ccd" (server restarted in between).

CheckOrganizationDelete

Case Expected can_delete observed Result
Clean org, no billing account can_delete: true true
Active paid-plan sub ACTIVE_SUBSCRIPTION, subject = sub id, billing_subscription false ✅ message names the fix ("downgrade to the standard plan")
Open invoice UNPAID_INVOICE, subject = invoice id false
Token balance −500 NEGATIVE_TOKEN_BALANCE, subject = billing account id false ✅ "owes 500 tokens: contact support"
Token balance +100 can_delete: true — tokens never block true
Sub + invoice + debt at once all three blockers in one response false
Paid and $0 invoices ignored false — only the open invoice blocked, the paid/$0 rows emitted nothing
No side effects sub still active locally and in Stripe after the check false
Non-UUID id not_found — (error, no body)
Owner / viewer / unauthenticated / superadmin ok / permission_denied / unauthenticated / ok false for owner and superadmin; errors carry no body

DeleteOrganization

The can_delete column here is what a check returned at that same moment, next to what the delete did.

Case Expected can_delete observed Result
Blocked delete failed_precondition + one PreconditionFailure violation per blocker false ✅ org, projects, and sub untouched afterwards
All three blockers at once every violation in one error false
Invoice paid on Stripe, local row still open delete syncs fresh and succeeds false (stale) — yet the delete passed ✅ advisory contract works
Token balance +100 delete succeeds, tokens forfeited true ✅ audit rows app.billing.tokens.forfeited (amount 100) + app.organization.deleted; forfeit mail attempt hit the mailer
Debt settled (revert usage) check flips to yes, delete passes true
Delete an already-deleted org rejected — (error, no body) permission_denied for every caller — see notes
Full cleanup org, billing account, invoices, transactions, checkouts all gone — (org gone, check errors) ✅ verified in DB
Viewer tries delete permission_denied — (denied, no body)

Standard-plan flow (default_plan: "starter-e96ccd")

Case Expected can_delete observed Result
Check with active starter sub no ACTIVE_SUBSCRIPTION blocker, sub not canceled by the check true
Delete with starter sub delete cancels the sub itself, immediately true ✅ sub canceled in Stripe right after
Post-cancel invoice re-check a final invoice from the cancel still blocks — (internal to the delete) ✅ path ran; final invoice was $0 here so it correctly did not block
Pro sub still blocks false
Downgrade pro → starter, then delete passes once the downgrade lands false until the sub sync landed, then true ✅ blocked → downgrade → check yes → delete ok, sub canceled in Stripe

Notes

  • A deleted or unknown org returns permission_denied for every caller, superadmin included: the authz interceptor asks SpiceDB before the handler runs, and a deleted org has no tuples left. The handler's not_found mapping only surfaces for non-UUID ids. Not a leak — it hides existence — just worth knowing when reading the handler code.
  • Subscription state is judged from local rows, invoices from fresh provider data. A delete fired right after a downgrade can bounce with a stale ACTIVE_SUBSCRIPTION blocker until the next subscription sync; the retry then passes. Invoices don't have this gap since the delete syncs them itself.
  • Disabled orgs are deletable by superadmins only — the owner is rejected earlier by the generic "org is disabled" gate. Pre-existing behavior, not from this stack.
  • can_delete: false is omitted in raw JSON (proto3 zero value). Fine for generated clients; raw-JSON consumers must not read absence as unknown.

@whoAbhishekSah

Copy link
Copy Markdown
Member Author

Round 2: token-forfeit purchased-share changes (#1880). Round 1 covered the blocker and delete flows: #1865 (comment)

Built from the stack tip (#1865#1857#1880#1893#1894, head 7f1c580), so the rebased review fixes are included. Every email assertion below is from an actual message body captured by a local SMTP server, not from log lines; every audit assertion is from the auditlogs rows.

Setup

  • Local server, offline billing accounts (balance logic is local, no provider needed for these cases).
  • Token balances staged as direct billing_transactions rows with real sources: system.buy, system.awarded, system.starter, plus debit rows for spend.
  • Mailer pointed at a local SMTP catcher; full message bodies inspected.
  • Deletes run as the org owner's session; one case as the bootstrap superuser service account where noted.
  • Expected purchased value everywhere: per account, min(balance, sum of credit rows with source system.buy), then summed across the org's accounts.

Purchased-share math

Case Setup Expected Result
Mixed sources starter 100 + buy 60 + debit 50 (balance 110) email "110 unused tokens remaining, of which 60 came from purchases"; audit amount=110 purchased=60
Complimentary only awarded 500 email "500 … of which 0 came from purchases"; audit purchased=0 ✅ (see notes on the 0 wording)
All purchased buy 200 split line absent (purchased == amount); audit amount=200 purchased=200
Clamp binds starter 10 + buy 100 + debit 80 (balance 30) purchased = min(30, 100) = 30, not 100 ✅ audit amount=30 purchased=30
Multiple buy rows buy 30 + buy 30 + awarded 40 (balance 100) buys sum: purchased 60 ✅ email "of which 60"
Two billing accounts acct A: buy 50 + debit 40 (bal 10); acct B (disabled): awarded 100 per-account clamp then sum: purchased 10 (a global min(110, 50) would say 50) ✅ email "110 … of which 10"; audit is per account (100/0 and 10/10)
Refund-style rows buy 100 + debit 100 with source system.buy + awarded 50 (balance 50) documents the formula's limit: debit sources are ignored, purchased reports 50 though the purchase was fully refunded ✅ behaves as implemented — see notes

Recipients and deleted-by

Case Setup Expected Result
Two owners forfeit org with two owners both owners get the mail ✅ exactly 2 mails, one per owner
Non-owner member same org has a viewer viewer gets nothing
Deleted by owner delete with the owner's session body names the caller's email ✅ "deleted by delowner@…" in every owner-run case
Deleted by service account delete as the bootstrap superuser SA DeletedBy falls back to the SA title, never an empty "deleted by" ✅ "deleted by GitOps Bootstrap Superuser"
Duplicate owner policy second owner policy for the same user one mail, not two ✅ policy create upserts, so the duplicate can't exist; single mail confirmed

Template and regressions

Case Setup Expected Result
Custom template config billing.token_forfeit_notice using {{.Amount}}, {{.Purchased}}, {{.DeletedBy}}, {{.Org.Title}} rendered mail uses the custom subject and body with correct values ✅ subject and body both rendered, all four variables correct
Zero balance org with no transactions delete succeeds, no mail, no forfeit audit row ✅ only app.organization.deleted written
Negative balance balance −500 still blocks with NEGATIVE_TOKEN_BALANCE, no mail, org intact

Notes

  • The audit trail is per billing account while the email carries org totals. In the two-account case the mail says 110/10 and the audit has two app.billing.tokens.forfeited rows (100 purchased 0, and 10 purchased 10). Consistent, just worth knowing when reading either side alone.
  • When purchased is 0 the mail still renders "of which 0 came from purchases" and closes with "Contact support to get the purchased amount transferred" — an invitation about nothing. Suppressing the split line and the support sentence at purchased 0 may read better; product call.
  • Known limit, now documented by a test: purchasedTokens sums only credit rows with source system.buy and ignores debit sources. A refunded purchase therefore still counts as transferable up to the balance clamp. Fine if refunds never land as system.buy debits; worth revisiting if they do.
  • Owner de-dup can't be exercised through the API — CreatePolicy upserts, so a principal can't hold the owner role twice. The straight append in collectForfeitNotice is safe under that invariant.

@whoAbhishekSah

Copy link
Copy Markdown
Member Author

Follow-ups from the round 2 notes, landed on the stack (email PR commit, stack rebased):

  • Zero-purchased wording: when nothing in the balance was bought, the mail no longer says "of which 0 came from purchases" or invites a support request. It now closes with "These were complimentary tokens, so there is no amount to transfer." The split line and the support sentence only appear when there is a purchased share.
  • Refunded purchases: purchasedTokens now subtracts debit rows recorded against the system.buy source, so a refunded purchase stops counting as transferable (floored at zero, still clamped to the balance). The documented limit from the notes is closed; covered by a body-level assertion in the unit test.

No change for the other two notes: the per-account audit vs org-total email split stays as designed, and the owner de-dup stays a straight append since CreatePolicy upserts.

@whoAbhishekSah

Copy link
Copy Markdown
Member Author

Round 3 (retest of the review-round behavior changes: amount-remaining invoice gating, dangling-plan blocker, two-phase cancel, audit-record notice recovery) is on #1857: #1857 (comment). All five cases passed.

Adds GetBalance and List to the credit service mock, List and Cancel to
the subscription service mock, ListPayableOnProvider to the invoice
service mock, and a new plan service mock. The interface methods land
with the next PRs in the stack; the mocks compile standalone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants