From 53bdb80fe36a1c408a4354ffd2aaa90a1c31e34f Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 23:31:11 +0000 Subject: [PATCH] fix(frontend): hide Edit and Delete on system-managed groups The API refuses edits and deletes of system-managed groups (seeded by migrations: Administrators, Purchaser, RI Exchanger, Standard Users, Read-Only Users, ...) with ErrSystemManagedGroup / 403, but every group card offered Edit and Delete, so admins were handed actions that always fail. When Delete failed, the toast also dropped the server's reason and only said "Failed to delete group", while Edit already shows it. Cards for groups with system_managed: true now show a muted "System group" badge (tooltip plus screen-reader text explaining it cannot be edited or deleted, and that membership is changed from the Users list) in place of Edit and Delete. Duplicate stays, since it makes a separate, editable copy. deleteGroup refuses such a group up front instead of asking for confirmation, and a failed delete now reads "Failed to delete group: ". The list endpoint does not serialize system_managed yet (auth.APIGroup has no such field), so the cards only change once the backend sends it; the Delete message fix works today. Refs #919 Co-Authored-By: Claude Opus 5.5 --- .../groups-system-managed-919.test.ts | 107 ++++++++++++++++++ frontend/src/groups/groupActions.ts | 12 +- frontend/src/groups/groupList.ts | 8 +- frontend/src/groups/systemGroups.ts | 16 +++ .../tests-e2e/system-managed-groups.spec.ts | 27 +++++ 5 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 frontend/src/__tests__/groups-system-managed-919.test.ts create mode 100644 frontend/src/groups/systemGroups.ts create mode 100644 frontend/tests-e2e/system-managed-groups.spec.ts diff --git a/frontend/src/__tests__/groups-system-managed-919.test.ts b/frontend/src/__tests__/groups-system-managed-919.test.ts new file mode 100644 index 000000000..c0e3149ab --- /dev/null +++ b/frontend/src/__tests__/groups-system-managed-919.test.ts @@ -0,0 +1,107 @@ +/** + * Issue #919: system-managed groups must not offer Edit or Delete (the API + * refuses both with 403), and a refused Delete must show the server's + * reason rather than a generic "Failed to delete group". + */ +import './setup'; + +jest.mock('../api', () => ({ + deleteGroup: jest.fn(), + getGroup: jest.fn(), +})); + +jest.mock('../users/userActions', () => ({ + loadUsers: jest.fn().mockResolvedValue(undefined), +})); + +const mockConfirmDialog = jest.fn, [unknown]>(() => Promise.resolve(true)); +jest.mock('../confirmDialog', () => ({ + confirmDialog: (opts: unknown) => mockConfirmDialog(opts), +})); + +import * as api from '../api'; +import type { APIGroup } from '../api'; +import * as userState from '../users/state'; +import { renderGroups } from '../groups/groupList'; +import { deleteGroup } from '../groups/groupActions'; +import { SYSTEM_GROUP_NOTE } from '../groups/systemGroups'; + +const SYSTEM: APIGroup = { + id: '00000000-0000-5000-8000-000000000001', name: 'Administrators', description: '', + permissions: [{ action: 'admin', resource: '*' }], system_managed: true, +}; +const CUSTOM: APIGroup = { + id: 'g-custom', name: 'Finance', description: '', + permissions: [{ action: 'view', resource: 'plans' }], +}; + +function card(name: string): HTMLElement { + return Array.from(document.querySelectorAll('.group-card')) + .find(c => c.querySelector('h4')?.textContent === name)!; +} + +beforeEach(() => { + document.body.innerHTML = '
'; + userState.setAllUsers([]); + userState.setAvailableGroups([SYSTEM, CUSTOM]); + jest.clearAllMocks(); + mockConfirmDialog.mockResolvedValue(true); + jest.spyOn(console, 'error').mockImplementation(() => undefined); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +describe('group cards (#919)', () => { + it('hides Edit and Delete on a system-managed group and says why', () => { + renderGroups([SYSTEM, CUSTOM]); + const sys = card('Administrators'); + expect(sys.querySelector('.edit-group-btn')).toBeNull(); + expect(sys.querySelector('.delete-group-btn')).toBeNull(); + // Duplicate still works: it makes an editable copy. + expect(sys.querySelector('.duplicate-group-btn')).not.toBeNull(); + const badge = sys.querySelector('.system-group-badge') as HTMLElement; + expect(badge.getAttribute('title')).toBe(SYSTEM_GROUP_NOTE); + expect(badge.querySelector('.sr-only')?.textContent).toContain(SYSTEM_GROUP_NOTE); + }); + + it('keeps Edit and Delete on custom groups', () => { + renderGroups([SYSTEM, CUSTOM]); + const custom = card('Finance'); + expect(custom.querySelector('.edit-group-btn')).not.toBeNull(); + expect(custom.querySelector('.delete-group-btn')).not.toBeNull(); + expect(custom.querySelector('.system-group-badge')).toBeNull(); + }); + + it('treats a missing or false system_managed as a custom group', () => { + renderGroups([{ ...CUSTOM, system_managed: false }]); + expect(document.querySelector('.edit-group-btn')).not.toBeNull(); + expect(document.querySelector('.delete-group-btn')).not.toBeNull(); + }); +}); + +describe('deleteGroup (#919)', () => { + it('refuses a system-managed group without asking or calling the API', async () => { + await deleteGroup(SYSTEM.id); + expect(mockConfirmDialog).not.toHaveBeenCalled(); + expect(api.deleteGroup).not.toHaveBeenCalled(); + expect(document.querySelector('.toast-error')?.textContent).toContain(SYSTEM_GROUP_NOTE); + }); + + it('shows the server reason when the API refuses a delete', async () => { + (api.deleteGroup as jest.Mock).mockRejectedValue( + new Error('system-managed group cannot be modified: "Finance" is seeded and maintained by migrations'), + ); + await deleteGroup(CUSTOM.id); + expect(document.querySelector('.toast-error')?.textContent).toContain( + 'Failed to delete group: system-managed group cannot be modified: "Finance" is seeded and maintained by migrations', + ); + }); + + it('falls back to the generic message when the error has no text', async () => { + (api.deleteGroup as jest.Mock).mockRejectedValue(new Error('')); + await deleteGroup(CUSTOM.id); + expect(document.querySelector('.toast-error')?.textContent).toContain('Failed to delete group'); + }); +}); diff --git a/frontend/src/groups/groupActions.ts b/frontend/src/groups/groupActions.ts index 9a0320f7d..ef74b1099 100644 --- a/frontend/src/groups/groupActions.ts +++ b/frontend/src/groups/groupActions.ts @@ -7,6 +7,7 @@ import { availableGroups } from '../users/state'; import { showError, showSuccess } from '../users/utils'; import { loadUsers } from '../users/userActions'; import { confirmDialog } from '../confirmDialog'; +import { isSystemManagedGroup, SYSTEM_GROUP_NOTE } from './systemGroups'; /** * Delete group @@ -14,6 +15,12 @@ import { confirmDialog } from '../confirmDialog'; export async function deleteGroup(groupId: string): Promise { const group = availableGroups.find(g => g.id === groupId); if (!group) return; + // The card hides Delete for these (#919); refuse here too rather than + // ask for confirmation of a delete the API will reject. + if (isSystemManagedGroup(group)) { + showError(SYSTEM_GROUP_NOTE); + return; + } const ok = await confirmDialog({ title: `Delete group "${group.name}"?`, @@ -29,7 +36,10 @@ export async function deleteGroup(groupId: string): Promise { showSuccess('Group deleted successfully'); } catch (error) { console.error('Failed to delete group:', error); - showError('Failed to delete group'); + // Surface the server's reason (e.g. the system-managed refusal), as the + // Edit path already does (#919 review comment). + const err = error as Error; + showError(err?.message ? `Failed to delete group: ${err.message}` : 'Failed to delete group'); } } diff --git a/frontend/src/groups/groupList.ts b/frontend/src/groups/groupList.ts index 75cf12152..22e7af137 100644 --- a/frontend/src/groups/groupList.ts +++ b/frontend/src/groups/groupList.ts @@ -7,6 +7,7 @@ import { allUsers } from '../users/state'; import { escapeHtml } from '../users/utils'; import { openEditGroupModal, deleteGroup } from './groupActions'; import { openDuplicateGroupModal } from './groupModals'; +import { isSystemManagedGroup, SYSTEM_GROUP_NOTE } from './systemGroups'; /** * Format permission constraints as a readable string for tooltips @@ -49,6 +50,7 @@ export function renderGroups(groups: APIGroup[]): void { ? members.map(m => `${escapeHtml(m.email)}`).join('') : 'No members'; + const locked = isSystemManagedGroup(group); const description = group.description ? `

${escapeHtml(group.description)}

` : ''; @@ -62,9 +64,11 @@ export function renderGroups(groups: APIGroup[]): void {
${memberCount} member${memberCount !== 1 ? 's' : ''} - + ${locked + ? `System group. ${escapeHtml(SYSTEM_GROUP_NOTE)}` + : ``} - + ${locked ? '' : ``}
diff --git a/frontend/src/groups/systemGroups.ts b/frontend/src/groups/systemGroups.ts new file mode 100644 index 000000000..7c0f115be --- /dev/null +++ b/frontend/src/groups/systemGroups.ts @@ -0,0 +1,16 @@ +/** + * System-managed groups (issue #919). + */ + +import type { APIGroup } from '../api'; + +// Issue #919: the API refuses edits and deletes of system-managed groups +// (seeded by migrations: Administrators, Purchaser, Standard Users, ...) +// with ErrSystemManagedGroup / 403, so offering Edit and Delete there only +// leads to an error. Duplicate stays: it creates a separate, editable copy. +export const SYSTEM_GROUP_NOTE = + 'Built-in group maintained by CUDly. It cannot be edited or deleted; change its members from the Users list.'; + +export function isSystemManagedGroup(group: Pick | undefined): boolean { + return group?.system_managed === true; +} diff --git a/frontend/tests-e2e/system-managed-groups.spec.ts b/frontend/tests-e2e/system-managed-groups.spec.ts new file mode 100644 index 000000000..fdcddc150 --- /dev/null +++ b/frontend/tests-e2e/system-managed-groups.spec.ts @@ -0,0 +1,27 @@ +import { test, expect } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +// Issue #919: system-managed groups are refused by the API on edit and +// delete, so their cards must not offer those actions. +test('system-managed group cards hide Edit and Delete', async ({ page }) => { + await page.addInitScript(() => localStorage.setItem('cudly:purchaser-prompt-dismissed', '1')); + await seedAuth(page); + await mockApi(page); + await page.route('**/api/users', route => route.fulfill({ json: { users: [] } })); + await page.route('**/api/groups', route => route.fulfill({ json: { groups: [ + { id: '00000000-0000-5000-8000-000000000001', name: 'Administrators', description: '', + permissions: [{ action: 'admin', resource: '*' }], allowed_accounts: [], system_managed: true }, + { id: 'g-custom', name: 'Finance', description: '', + permissions: [{ action: 'view', resource: 'plans' }], allowed_accounts: [] }, + ] } })); + + await page.goto('/admin/users'); + const system = page.locator('.group-card', { has: page.locator('h4', { hasText: 'Administrators' }) }); + const custom = page.locator('.group-card', { has: page.locator('h4', { hasText: 'Finance' }) }); + await expect(system.locator('.system-group-badge')).toBeVisible(); + await expect(system.locator('.edit-group-btn')).toHaveCount(0); + await expect(system.locator('.delete-group-btn')).toHaveCount(0); + await expect(system.locator('.duplicate-group-btn')).toBeVisible(); + await expect(custom.locator('.edit-group-btn')).toBeVisible(); + await expect(custom.locator('.delete-group-btn')).toBeVisible(); +});