fix(frontend): accept org RBAC delete role#1858
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralized role checks into store helpers ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Component
participant Store as OrganizationStore
participant DB as Supabase
Client->>Store: request deleteOrganization(orgId)
Store->>Store: canDeleteOrganization(orgId)? (uses isAdminRole / isSuperAdminRole / roleHasLegacyMinRight)
alt authorized
Store->>DB: DELETE from "orgs" where id=orgId
DB-->>Store: {data, error}
Store-->>Client: {data, error:null}
else unauthorized
Store-->>Client: {data:null, error: "Insufficient permissions"}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/organization-store-delete.unit.test.ts (2)
51-56: Consider usingit.concurrent()for parallelism per coding guidelines.The coding guidelines specify using
it.concurrent()instead ofit()in test files to maximize parallelism. Since these tests are stateless (each creates its own Pinia instance), they are good candidates for concurrent execution.♻️ Suggested change
describe('organization store deleteOrganization', () => { beforeEach(() => { vi.clearAllMocks() setActivePinia(createPinia()) mockEq.mockResolvedValue({ data: null, error: null }) }) - it('allows org deletion for org_super_admin roles', async () => { + it.concurrent('allows org deletion for org_super_admin roles', async () => {Apply the same change to the other test case.
As per coding guidelines: "Use
it.concurrent()instead ofit()to maximize parallelism and run tests in parallel within the same file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/organization-store-delete.unit.test.ts` around lines 51 - 56, Replace standard synchronous test declarations with concurrent tests in this suite: change any occurrences of it(...) to it.concurrent(...) inside the describe('organization store deleteOrganization', ...) block (and the sibling test case mentioned) so both test cases run in parallel; ensure the tests still call setActivePinia(createPinia()) in beforeEach and keep mockEq.mockResolvedValue(...) as-is to preserve isolation while switching to it.concurrent.
58-74: Good test case, but consider adding coverage for legacy roles.This test validates the new
org_super_adminRBAC role correctly. However, the PR objective states the change should still support legacy roles (super_admin,owner). Consider adding test cases to ensure these legacy roles continue to work:
- Legacy
super_adminrole (withuse_new_rbac: false)- Legacy
ownerrole (withuse_new_rbac: false)This would provide regression coverage ensuring the fix doesn't break existing legacy role users. Based on learnings, the
ownerrole exists only at the organization level, so this is the appropriate place to test it.💡 Suggested additional test cases
it.concurrent('allows org deletion for legacy super_admin role', async () => { const { useOrganizationStore } = await import('../src/stores/organization.ts') const store = useOrganizationStore() const orgId = 'org-legacy-super-admin' store.getAllOrgs().set(orgId, { gid: orgId, role: 'super_admin', use_new_rbac: false, } as any) const result = await store.deleteOrganization(orgId) expect(result.error).toBeNull() expect(mockFrom).toHaveBeenCalledWith('orgs') }) it.concurrent('allows org deletion for legacy owner role', async () => { const { useOrganizationStore } = await import('../src/stores/organization.ts') const store = useOrganizationStore() const orgId = 'org-legacy-owner' store.getAllOrgs().set(orgId, { gid: orgId, role: 'owner', use_new_rbac: false, } as any) const result = await store.deleteOrganization(orgId) expect(result.error).toBeNull() expect(mockFrom).toHaveBeenCalledWith('orgs') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/organization-store-delete.unit.test.ts` around lines 58 - 74, Add regression tests covering legacy RBAC roles by duplicating the existing org_super_admin test but setting role to 'super_admin' and 'owner' with use_new_rbac: false: call useOrganizationStore(), populate store.getAllOrgs().set(orgId, { gid: orgId, role: 'super_admin', use_new_rbac: false }/ { role: 'owner', use_new_rbac: false } as any), invoke deleteOrganization(orgId) and assert result.error is null and mockFrom was calledWith('orgs') (same expectation as the current test); use it.concurrent for parity with other tests and unique orgId values like 'org-legacy-super-admin' and 'org-legacy-owner'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/organization-store-delete.unit.test.ts`:
- Around line 51-56: Replace standard synchronous test declarations with
concurrent tests in this suite: change any occurrences of it(...) to
it.concurrent(...) inside the describe('organization store deleteOrganization',
...) block (and the sibling test case mentioned) so both test cases run in
parallel; ensure the tests still call setActivePinia(createPinia()) in
beforeEach and keep mockEq.mockResolvedValue(...) as-is to preserve isolation
while switching to it.concurrent.
- Around line 58-74: Add regression tests covering legacy RBAC roles by
duplicating the existing org_super_admin test but setting role to 'super_admin'
and 'owner' with use_new_rbac: false: call useOrganizationStore(), populate
store.getAllOrgs().set(orgId, { gid: orgId, role: 'super_admin', use_new_rbac:
false }/ { role: 'owner', use_new_rbac: false } as any), invoke
deleteOrganization(orgId) and assert result.error is null and mockFrom was
calledWith('orgs') (same expectation as the current test); use it.concurrent for
parity with other tests and unique orgId values like 'org-legacy-super-admin'
and 'org-legacy-owner'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9757928-7403-4df5-8f51-04ca7f81f5a5
📒 Files selected for processing (3)
src/pages/settings/organization/index.vuesrc/stores/organization.tstests/organization-store-delete.unit.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/settings/account/index.vue (1)
662-666:⚠️ Potential issue | 🟡 MinorUse translation keys instead of hardcoded English text.
These strings should use the
t()function with translation keys instead of hardcoded English text. As per coding guidelines, all user-facing text must use translation keys.🔧 Suggested fix
<div class="text-base text-gray-500 dark:text-gray-400"> <p class="mb-4"> - This action cannot be undone. Your account and all associated data will be permanently deleted. + {{ t('delete-account-warning') }} </p> <p class="font-medium text-gray-700 dark:text-gray-300"> - Your account will be deleted after 30 days + {{ t('delete-account-delay-notice') }} </p>Add corresponding keys to
messages/en.json:{ "delete-account-warning": "This action cannot be undone. Your account and all associated data will be permanently deleted.", "delete-account-delay-notice": "Your account will be deleted after 30 days" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/index.vue` around lines 662 - 666, Replace the hardcoded English paragraphs in the template with calls to the translation helper (t) using new keys (e.g., "delete-account-warning" and "delete-account-delay-notice") so the template uses t('delete-account-warning') and t('delete-account-delay-notice') instead of raw strings; then add those same keys and English values to the messages/en.json locale file so the translations are available at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pages/settings/account/index.vue`:
- Around line 662-666: Replace the hardcoded English paragraphs in the template
with calls to the translation helper (t) using new keys (e.g.,
"delete-account-warning" and "delete-account-delay-notice") so the template uses
t('delete-account-warning') and t('delete-account-delay-notice') instead of raw
strings; then add those same keys and English values to the messages/en.json
locale file so the translations are available at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58183400-2f79-4633-8bfc-ab8d7d8f4b80
📒 Files selected for processing (6)
src/components/AdminOnlyModal.vuesrc/pages/settings/account/index.vuesrc/pages/settings/organization/Members.vuesrc/pages/settings/organization/Plans.vuesrc/stores/organization.tstests/organization-store-delete.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/organization-store-delete.unit.test.ts
- src/stores/organization.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/stores/main.ts (1)
9-9: Use~/alias for imports per coding guidelines.The import should use the path alias instead of a relative path.
Suggested fix
-import { createDeferredPromise } from '../utils/promise' +import { createDeferredPromise } from '~/utils/promise'As per coding guidelines: "Import using
~/alias forsrc/directory in frontend code instead of relative paths"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/main.ts` at line 9, Update the import in src/stores/main.ts to use the project alias for src instead of a relative path: replace the relative import that imports createDeferredPromise (symbol: createDeferredPromise) from '../utils/promise' with the aliased path using '~/utils/promise' so it follows the frontend coding guideline of using the `~/` alias for imports rooted at src.src/stores/organization.ts (1)
8-8: Use~/alias for imports per coding guidelines.The import should use the path alias instead of a relative path.
Suggested fix
-import { createDeferredPromise } from '../utils/promise' +import { createDeferredPromise } from '~/utils/promise'As per coding guidelines: "Import using
~/alias forsrc/directory in frontend code instead of relative paths"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/organization.ts` at line 8, The import of createDeferredPromise should use the project alias for the src directory; replace the relative import "import { createDeferredPromise } from '../utils/promise'" with an aliased import that starts with "~/" (e.g., import { createDeferredPromise } from '~/utils/promise') so the store module references the promise utility via the codebase alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/stores/main.ts`:
- Line 9: Update the import in src/stores/main.ts to use the project alias for
src instead of a relative path: replace the relative import that imports
createDeferredPromise (symbol: createDeferredPromise) from '../utils/promise'
with the aliased path using '~/utils/promise' so it follows the frontend coding
guideline of using the `~/` alias for imports rooted at src.
In `@src/stores/organization.ts`:
- Line 8: The import of createDeferredPromise should use the project alias for
the src directory; replace the relative import "import { createDeferredPromise }
from '../utils/promise'" with an aliased import that starts with "~/" (e.g.,
import { createDeferredPromise } from '~/utils/promise') so the store module
references the promise utility via the codebase alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c15ba05-a68a-4b11-b442-620327b48c25
📒 Files selected for processing (3)
src/stores/main.tssrc/stores/organization.tssrc/utils/promise.ts
✅ Files skipped from review due to trivial changes (1)
- src/utils/promise.ts
|
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
1 participant
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.
Summary (AI generated)
org_super_adminrole instead of only legacy role namesorg_super_adminand denied deletion for lower org rolesMotivation (AI generated)
Organization deletion was blocked for users who legitimately had the new RBAC org-level super admin role because the frontend action still compared against legacy
super_adminandownerstrings directly.Business Impact (AI generated)
This restores a critical org-management flow for RBAC-enabled customers and reduces support/debugging time caused by false permission denials in settings.
Test Plan (AI generated)
bunx vitest run tests/organization-store-delete.unit.test.tsbunx eslint src/stores/organization.ts src/pages/settings/organization/index.vue tests/organization-store-delete.unit.test.tsbun run typecheckGenerated with AI
Summary by CodeRabbit