feat(deleter): check all org delete blockers up front and return them together - #1857
feat(deleter): check all org delete blockers up front and return them together#1857whoAbhishekSah wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughOrganization deletion now performs billing preflight checks, aggregates structured blockers, and accepts token-forfeit acknowledgement. Billing-account deletion audits forfeited tokens. The Connect API returns structured failed-precondition details for blocked deletions. ChangesOrganization deletion workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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. Comment |
29a0e18 to
783da24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51d78c26-5a73-43e9-8050-b9aec5ac46cb
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (13)
Makefilebilling/invoice/invoice.gocore/audit/audit.gocore/deleter/deleter.gocore/deleter/mocks/credit_service.gocore/deleter/mocks/subscription_service.gocore/deleter/service.gocore/deleter/service_test.gogo.modinternal/api/v1beta1connect/deleter.gointernal/api/v1beta1connect/deleter_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/cascade_deleter.go
| // the ack flag cannot bypass a debt | ||
| err = m.build().DeleteOrganization(context.Background(), "org-1", true) | ||
| assert.ErrorAs(t, err, &blocked) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the blocker type on the acknowledged retry.
The second call only checks that the error is a BlockedError. A regression that returns a different blocker type for ackTokenForfeit=true would still pass. Assert the type to close the gap.
💚 Proposed test assertion
// the ack flag cannot bypass a debt
err = m.build().DeleteOrganization(context.Background(), "org-1", true)
assert.ErrorAs(t, err, &blocked)
+ assert.Len(t, blocked.Blockers, 1)
+ assert.Equal(t, deleter.BlockerNegativeTokenBalance, blocked.Blockers[0].Type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // the ack flag cannot bypass a debt | |
| err = m.build().DeleteOrganization(context.Background(), "org-1", true) | |
| assert.ErrorAs(t, err, &blocked) | |
| // the ack flag cannot bypass a debt | |
| err = m.build().DeleteOrganization(context.Background(), "org-1", true) | |
| assert.ErrorAs(t, err, &blocked) | |
| assert.Len(t, blocked.Blockers, 1) | |
| assert.Equal(t, deleter.BlockerNegativeTokenBalance, blocked.Blockers[0].Type) |
… together Replace the blanket "any invoice ever" gate with a pre-flight check that runs before any deletion starts and collects every blocker: - active or trialing subscription: cancel it, then retry - open or uncollectible invoice: pay it, then retry; paid invoices stop blocking since the billing provider keeps its own copy - negative token balance: buy tokens to clear the debt, then retry - unused tokens: retry with acknowledge_token_forfeit set; the forfeited amount is written to an audit record during the delete Accounts without a billing provider only get the token checks, since their subscription and invoice rows have nothing the caller could cancel or pay. The handler returns the blockers as a failed_precondition error with one PreconditionFailure violation per blocker, so a caller sees the whole checklist in one response instead of one blocker per retry. Unexpected failures stay internal. Closes #1837 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
783da24 to
3c17128
Compare
1 participant
Closes #1837. Top of a 3-PR stack (#1866): #1864 (proto) → #1865 (mocks) → this. Depends on raystack/proton#497 for the new request field.
What changed
Deleting an organization used to be gated by one rule: if the billing account ever had an invoice, the delete was refused — and the caller saw a bare
internal server error. That rule was too strict (a fully paid invoice blocked the org forever) and too loose at the same time (an active subscription didn't block anything; the delete silently cancelled it mid-flight, and token balances were never looked at).This PR replaces that gate with a pre-flight check that runs before any deletion starts, collects all blockers, and returns them together as a
failed_preconditionerror with one standardPreconditionFailureviolation per blocker:ACTIVE_SUBSCRIPTIONUNPAID_INVOICENEGATIVE_TOKEN_BALANCEUNUSED_TOKENSacknowledge_token_forfeitsetPaid, void, draft, and zero-amount invoices no longer block: the billing provider keeps its own permanent copy of every invoice, so deleting our rows loses nothing.
The token forfeit acknowledgment
Unused prepaid tokens have no clean-up action — the user already paid for them. What the server needs is informed consent, and a UI popup alone can't provide that because the API is public (SDKs, scripts, curl). So
DeleteOrganizationRequestgains abool acknowledge_token_forfeitfield (added in raystack/proton#497; pulled in by #1864). Without the flag, a positive balance returns anUNUSED_TOKENSviolation naming the amount at stake. With it, the delete proceeds and the forfeited amount is written to a newapp.billing.tokens.forfeitedaudit record during the credit teardown. The flag cannot bypass a debt.Accounts without a billing provider (offline accounts) only get the token checks: their subscription and invoice rows have nothing behind them the caller could cancel or pay, and the teardown already skips the provider for them.
Error mapping
The handler maps
deleter.BlockedErrortofailed_preconditionand attaches the violations aserrdetails.PreconditionFailure. The error sanitizer interceptor passes non-internal codes through untouched, so both the message and the structured details reach the caller. Everything unexpected staysinternal, same as before.Example response:
{ "code": "failed_precondition", "message": "organization cannot be deleted yet: subscription[...] is active: cancel it, then retry the delete; billing account[...] has 500 unused tokens that deleting the organization forfeits: retry the delete with acknowledge_token_forfeit set to proceed", "details": [{ "type": "google.rpc.PreconditionFailure", "value": { "violations": [ { "type": "ACTIVE_SUBSCRIPTION", "subject": "<subscription-id>", "description": "..." }, { "type": "UNUSED_TOKENS", "subject": "<billing-account-id>", "description": "..." } ]} }] }Notes for review
google.golang.org/genproto/googleapis/rpcmoves from indirect to direct in go.mod for theerrdetailsimport; no version change. This is the first use oferrdetailsin the codebase.🤖 Generated with Claude Code