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(); +});