feat(admin): gate org deletion on credit balance - #3368
Merged
Merged
Conversation
Deleting an organization from the admin panel is a fallback for abuse
handling, not an account-closure tool. An organization that still holds
credits is a paying customer, so it must never be swept up by an admin
delete — the balance has to be spent, refunded, or zeroed deliberately
first, and the account then handled manually.
Every path that marks an organization `deleted` now funnels through a
single `assertOrganizationDeletable` guard and rejects with HTTP 409
when credits are positive:
- single block (POST /admin/organizations/{orgId}/block), checked before
any Stripe call so subscriptions are not cancelled on a refusal
- status toggle (PATCH /admin/organizations/{orgId}/status) in the
`deleted` direction only; re-enabling stays unconditional
- bulk block, which additionally excludes those organizations from the
candidate query so the preview count and the mutation agree
The admin dashboard mirrors the rule by disabling the affected buttons
with the reason as their tooltip, but enforcement is server-side.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KcFa59wVqaes2t8bXZEjNN
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
WalkthroughOrganization blocking and deletion now require non-positive, parseable credit balances. Bulk-block operations exclude credited organizations. Admin controls show blocking reasons, and integration tests cover API and bulk behavior. ChangesCredit-aware organization deletion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant AdminAPI
participant CreditGuard
participant Stripe
Admin->>AdminAPI: Request block or deletion
AdminAPI->>CreditGuard: Check organization credits
CreditGuard-->>AdminAPI: Allow or return HTTP 409
AdminAPI->>Stripe: Cancel subscription after approval
AdminAPI-->>Admin: Return organization status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds a server-enforced guard to prevent admin org deletion/blocking when an organization still has a positive credit balance, and mirrors that rule in the Enterprise admin UI for clearer operator UX.
Changes:
- Centralized a credit-balance deletion guard (
assertOrganizationDeletable) inapps/api/src/routes/admin.tsand applied it to single-block, status-toggle-to-deleted, and bulk-block candidate selection. - Updated the admin dashboard to disable destructive org actions when credits are positive, surfacing the reason via button titles/tooltips.
- Added/updated Vitest coverage for single and bulk flows to ensure credit-holding orgs are refused and/or excluded as intended.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ee/admin/src/lib/org-deletion.ts | Adds UI helper to compute a “deletion blocked” reason based on credits. |
| ee/admin/src/components/org-status-toggle-button.tsx | Disables the “disable org” direction and surfaces a blocked reason via title. |
| ee/admin/src/components/bulk-block-orgs-button.tsx | Updates bulk-block dialog copy to clarify credit-holding orgs are excluded. |
| ee/admin/src/components/block-org-button.tsx | Adds optional disabled reason for the block button (tooltip/title). |
| ee/admin/src/app/organizations/page.tsx | Wires the deletion-blocked reason into list-page toggle/block controls. |
| ee/admin/src/app/organizations/[orgId]/page.tsx | Wires the deletion-blocked reason into org detail page block control. |
| apps/api/src/routes/admin.ts | Implements assertOrganizationDeletable, applies it to status toggle + single block, and excludes positive-credit orgs from bulk-block candidates. |
| apps/api/src/routes/admin-org-delete-credits.spec.ts | New test suite covering 409 refusal on positive credits and success on non-positive credits. |
| apps/api/src/routes/admin-bulk-block.spec.ts | Adds tests ensuring bulk preview/run exclude and never block positive-credit orgs. |
Suppressed comments (1)
ee/admin/src/components/block-org-button.tsx:100
- Same issue as the full variant: if
disabledis true anddisabledReasonis not provided,titlebecomesundefinedand the tooltip is lost. Use the default title as a fallback.
disabled={disabled}
title={disabled ? disabledReason : "Block account"}
>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+87
to
+89
| disabled={disabled} | ||
| title={disabled ? disabledReason : "Block account"} | ||
| > |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/admin-org-delete-credits.spec.ts`:
- Around line 62-104: Extend the credit test matrices covering block and
setStatus(..., "deleted", ...) with an unparseable credit value. For each
endpoint, assert a 409 response and verify getStatus still returns "active",
preserving the existing positive-credit rejection behavior.
In `@apps/api/src/routes/admin.ts`:
- Around line 6101-6124: The pre-delete credit validation in
assertOrganizationDeletable is race-prone because giftCreditsRoute can increase
credits before the destructive status or block write. Make each deletion path
perform an atomic conditional state transition using a zero-or-negative balance
predicate, or reserve the organization with that predicate before Stripe
cancellation and local deletion, and handle a failed condition as a conflict
instead of deleting.
In `@ee/admin/src/app/organizations/page.tsx`:
- Around line 434-448: In ee/admin/src/app/organizations/page.tsx#L434-L448 and
ee/admin/src/app/organizations/[orgId]/page.tsx#L305-L311, compute one
status-aware deletion reason per organization, giving deleted status precedence
over credit blocking, and reuse it for both BlockOrgButton.disabled and
disabledReason. Apply the same logic at both sites to avoid repeated
getOrgDeletionBlockedReason calls and ensure every disabled state has the
correct reason.
In `@ee/admin/src/components/bulk-block-orgs-button.tsx`:
- Around line 178-179: Update both bulk-block messages in the organization
deletion flow to explicitly mention organizations with positive or unparseable
credit balances, including the corresponding message near the other active
organization. Preserve the existing manual-handling guidance while ensuring
invalid credit data is clearly covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ce8964f-66c1-456b-9c76-65e815b33425
📒 Files selected for processing (9)
apps/api/src/routes/admin-bulk-block.spec.tsapps/api/src/routes/admin-org-delete-credits.spec.tsapps/api/src/routes/admin.tsee/admin/src/app/organizations/[orgId]/page.tsxee/admin/src/app/organizations/page.tsxee/admin/src/components/block-org-button.tsxee/admin/src/components/bulk-block-orgs-button.tsxee/admin/src/components/org-status-toggle-button.tsxee/admin/src/lib/org-deletion.ts
Comment on lines
+62
to
+104
| it("refuses to block an organization with positive credits", async () => { | ||
| await insertOrg("credit-positive", "12.34"); | ||
|
|
||
| const res = await block("credit-positive", cookie); | ||
| expect(res.status).toBe(409); | ||
| const json = (await res.json()) as { message: string }; | ||
| expect(json.message).toContain("positive credit balance"); | ||
|
|
||
| expect(await getStatus("credit-positive")).toBe("active"); | ||
| }); | ||
|
|
||
| it("refuses to disable an organization with positive credits", async () => { | ||
| await insertOrg("credit-positive", "0.01"); | ||
|
|
||
| const res = await setStatus("credit-positive", "deleted", cookie); | ||
| expect(res.status).toBe(409); | ||
|
|
||
| expect(await getStatus("credit-positive")).toBe("active"); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ["zero credits", "0"], | ||
| ["negative credits", "-5.00"], | ||
| ])("blocks an organization with %s", async (_label, credits) => { | ||
| await insertOrg("credit-empty", credits); | ||
|
|
||
| const res = await block("credit-empty", cookie); | ||
| expect(res.status).toBe(200); | ||
|
|
||
| expect(await getStatus("credit-empty")).toBe("deleted"); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ["zero credits", "0"], | ||
| ["negative credits", "-1.50"], | ||
| ])("disables an organization with %s", async (_label, credits) => { | ||
| await insertOrg("credit-empty", credits); | ||
|
|
||
| const res = await setStatus("credit-empty", "deleted", cookie); | ||
| expect(res.status).toBe(200); | ||
|
|
||
| expect(await getStatus("credit-empty")).toBe("deleted"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add unparseable-credit cases.
The test matrix covers positive, zero, and negative balances only. It does not verify that block and setStatus(..., "deleted", ...) return 409 for an unparseable balance.
Add an invalid credit value for both endpoints. Assert that the organization remains active. The PR objective requires this behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/admin-org-delete-credits.spec.ts` around lines 62 - 104,
Extend the credit test matrices covering block and setStatus(..., "deleted",
...) with an unparseable credit value. For each endpoint, assert a 409 response
and verify getStatus still returns "active", preserving the existing
positive-credit rejection behavior.
Comment on lines
+6101
to
+6124
| /** | ||
| * Admin deletion is a fallback for abuse handling, not an account-closure tool. | ||
| * An organization that still holds credits is a paying customer, so wiping it | ||
| * from the admin panel is never the right move — the balance has to be spent, | ||
| * refunded, or zeroed deliberately first, and the account then handled manually. | ||
| * | ||
| * Every path that marks an organization `deleted` (status toggle, single block, | ||
| * bulk block) funnels through this guard so no single entry point can bypass it. | ||
| */ | ||
| function assertOrganizationDeletable(org: { | ||
| name: string; | ||
| credits: string | null; | ||
| }): void { | ||
| const credits = Number(org.credits ?? "0"); | ||
|
|
||
| // Written as `!(credits <= 0)` rather than `credits > 0` so an unparseable | ||
| // balance (NaN) also refuses the delete: not deleting is always the safe | ||
| // direction here. | ||
| if (!(credits <= 0)) { | ||
| throw new HTTPException(409, { | ||
| message: `"${org.name}" still has a positive credit balance (${org.credits ?? "0"}). Deleting is only allowed for organizations with zero or negative credits — zero out or refund the balance first, then handle this account manually.`, | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the credit check atomic with deletion.
assertOrganizationDeletable evaluates an org fetched before the destructive write. giftCreditsRoute can add credits after this check and before the status write at Line 6206 or the block write at Line 6436. The request can then delete an organization with a positive credit balance.
Use a conditional state transition or reserve the organization with the balance predicate before Stripe cancellation and local deletion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/admin.ts` around lines 6101 - 6124, The pre-delete credit
validation in assertOrganizationDeletable is race-prone because giftCreditsRoute
can increase credits before the destructive status or block write. Make each
deletion path perform an atomic conditional state transition using a
zero-or-negative balance predicate, or reserve the organization with that
predicate before Stripe cancellation and local deletion, and handle a failed
condition as a conflict instead of deleting.
Comment on lines
+434
to
+448
| disableBlockedReason={getOrgDeletionBlockedReason( | ||
| org.credits, | ||
| )} | ||
| onToggle={handleToggleOrgStatus} | ||
| /> | ||
| <BlockOrgButton | ||
| orgId={org.id} | ||
| orgName={org.name} | ||
| disabled={org.status === "deleted"} | ||
| disabled={ | ||
| org.status === "deleted" || | ||
| getOrgDeletionBlockedReason(org.credits) !== null | ||
| } | ||
| disabledReason={ | ||
| getOrgDeletionBlockedReason(org.credits) ?? undefined | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Derive one status-aware deletion reason for both disabled props.
Both pages disable blocking for deleted or credit-blocked organizations, but derive disabledReason only from credits. This produces no reason for some deleted organizations and a misleading credit reason for others.
ee/admin/src/app/organizations/page.tsx#L434-L448: compute the helper once per row, give the deleted-state reason precedence, and reuse it fordisabledanddisabledReason.ee/admin/src/app/organizations/[orgId]/page.tsx#L305-L311: apply the same single status-aware reason before passing both props.
As per coding guidelines, “Apply DRY principles for reusable code.”
📍 Affects 2 files
ee/admin/src/app/organizations/page.tsx#L434-L448(this comment)ee/admin/src/app/organizations/[orgId]/page.tsx#L305-L311
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/admin/src/app/organizations/page.tsx` around lines 434 - 448, In
ee/admin/src/app/organizations/page.tsx#L434-L448 and
ee/admin/src/app/organizations/[orgId]/page.tsx#L305-L311, compute one
status-aware deletion reason per organization, giving deleted status precedence
over credit blocking, and reuse it for both BlockOrgButton.disabled and
disabledReason. Apply the same logic at both sites to avoid repeated
getOrgDeletionBlockedReason calls and ensure every disabled state has the
correct reason.
Source: Coding guidelines
Comment on lines
+178
to
+179
| other active organization. Organizations with a positive credit | ||
| balance are never included — handle those manually. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mention unparseable credit balances in the bulk-block messages.
The deletion policy excludes unparseable balances as well as positive balances. These strings mention only positive balances and “still holding credits”, so an organization with invalid credit data can be skipped without a clear explanation. Update both messages to mention positive or unparseable credit balances.
Also applies to: 205-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/admin/src/components/bulk-block-orgs-button.tsx` around lines 178 - 179,
Update both bulk-block messages in the organization deletion flow to explicitly
mention organizations with positive or unparseable credit balances, including
the corresponding message near the other active organization. Preserve the
existing manual-handling guidance while ensuring invalid credit data is clearly
covered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deleting an organization from the admin panel is a fallback for abuse handling, not an account-closure tool. An organization that still holds credits is a paying customer, so it must never be swept up by an admin delete — the balance has to be spent, refunded, or zeroed deliberately first, and the account then handled manually.
What changed
Every path that marks an organization
deletednow funnels through a singleassertOrganizationDeletableguard inapps/api/src/routes/admin.tsand rejects with HTTP 409 when the organization's credit balance is positive. Zero or negative balances are unaffected.POST /admin/organizations/{orgId}/blockPATCH /admin/organizations/{orgId}/statusdeleteddirection only. Re-enabling (active) stays unconditional, so an org that is gifted credits after being disabled can still be restored.POST /admin/organizations/bulk-blockresolveBulkBlockCandidates, so they count as skipped in the preview and the confirm-count check agrees with the mutation. The per-org guard still runs during the block loop, so a top-up landing between preview and confirm fails that one org intofailedrather than deleting it.The balance check is written as
!(credits <= 0)rather thancredits > 0so an unparseable balance also refuses the delete — not deleting is the safe direction.Scope note
The status toggle wasn't named in the request, but it sets the same
status: "deleted"(and logsorganization.delete), so leaving it open would have made the guard bypassable with the button right next to it. Happy to drop that part if you'd rather keep the toggle unrestricted.Admin dashboard
The dashboard mirrors the rule as a UX convenience — enforcement is server-side regardless of what the UI renders:
ee/admin/src/lib/org-deletion.tshelper returns the reason deletion is blocked, or null.Testing
apps/api/src/routes/admin-org-delete-credits.spec.ts— 8 tests covering refusal on positive credits for both single block and status toggle, success at zero and negative balances, that re-enabling still works after a post-block credit gift, and that a refused block leaves members active.apps/api/src/routes/admin-bulk-block.spec.ts— 2 new tests: credit-holding orgs are excluded from the preview and are never blocked by a bulk run.pnpm exec vitest run --no-file-parallelism apps/api/src/routes→ 377 passed, 1 skipped.pnpm buildandpnpm lintclean.Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes