diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 4a952e644..1a28a2620 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -2225,3 +2225,261 @@ describe('users bulk-actions toolbar', () => { }); }); }); + +// ============================================================================ +// GROUP ASSIGNMENT UX TESTS (issue #998) +// ============================================================================ +describe('group assignment UX (issue #998)', () => { + const mockUsers = [ + { id: '1', email: 'alice@test.com', groups: ['g1'], mfa_enabled: false }, + { id: '2', email: 'bob@test.com', groups: [], mfa_enabled: false }, + ]; + const mockGroups = [ + { id: 'g1', name: 'Admins', permissions: [], description: '', allowed_accounts: [] }, + { id: 'g2', name: 'Editors', permissions: [], description: '', allowed_accounts: [] }, + ]; + + function buildDom(): void { + document.body.innerHTML = ` +
+
+ + `; + } + + beforeEach(() => { + buildDom(); + userState.setAllUsers(mockUsers as any); + userState.setFilteredUsers(mockUsers as any); + userState.setAvailableGroups(mockGroups as any); + userState.clearSelectedUserIds(); + (api.updateUser as jest.Mock).mockResolvedValue({}); + jest.clearAllMocks(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + // ----------------------------------------------------------------------- + // 1. Single toast per group update + // ----------------------------------------------------------------------- + describe('single toast per group update', () => { + it('emits exactly one success toast when a group checkbox is toggled once', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + // Expand user "1" so the panel + checkboxes are in the DOM. + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expect(expandBtn).toBeTruthy(); + expandBtn!.click(); + + // Fire the change event on the group-assign checkbox. + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="g2"]', + ); + expect(cb).toBeTruthy(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + // Flush the async toggleUserGroup promise. + await Promise.resolve(); + await Promise.resolve(); + + const toasts = document.querySelectorAll('.toast-success'); + expect(toasts.length).toBe(1); + }); + + it('still emits exactly one toast even after multiple renderUsers calls (listener-stacking regression)', async () => { + // Pre-fix: each renderUsers call stacked a new document-level listener, + // so N renders produced N toasts for a single toggle. + jest.useFakeTimers(); + + // Simulate three renderUsers calls (as would happen after 3 prior group + // toggles that called loadUsers -> renderUsers). + userList.renderUsers(mockUsers as any); + userList.renderUsers(mockUsers as any); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expandBtn!.click(); + + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="g2"]', + ); + expect(cb).toBeTruthy(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + const toasts = document.querySelectorAll('.toast-success'); + // Must be exactly 1, not 3 (one per stacked listener). + expect(toasts.length).toBe(1); + }); + }); + + // ----------------------------------------------------------------------- + // 2. Expand panel stays open after a group checkbox toggle + // ----------------------------------------------------------------------- + describe('expand panel stays open after checkbox toggle', () => { + it('does not collapse the expand row after a group toggle', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expandBtn!.click(); + + // Verify it is open. + const expandRow = document.querySelector( + 'tr.user-expand-row[data-user-id="1"]', + ); + expect(expandRow?.classList.contains('hidden')).toBe(false); + + // Toggle a group checkbox. + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="g2"]', + ); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + // The row must still be visible - the fix does NOT call renderUsers (which + // would replace the whole table and reset all rows to hidden). + expect(expandRow?.classList.contains('hidden')).toBe(false); + }); + + it('keeps the expand panel in the DOM after a successful toggle', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expandBtn!.click(); + + const panelBefore = document.querySelector('.user-expand-panel[data-user-id="1"]'); + expect(panelBefore).toBeTruthy(); + + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="g2"]', + ); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + // The panel element must still exist (not destroyed by a table re-render). + const panelAfter = document.querySelector('.user-expand-panel[data-user-id="1"]'); + expect(panelAfter).toBeTruthy(); + }); + }); + + // ----------------------------------------------------------------------- + // 3. Expand button reacts reliably (single listener bound per render) + // ----------------------------------------------------------------------- + describe('expand button listener bound once', () => { + it('expand button opens the panel on the first click after renderUsers', () => { + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expect(expandBtn).toBeTruthy(); + + // First click must open the panel. + expandBtn!.click(); + + const expandRow = document.querySelector( + 'tr.user-expand-row[data-user-id="1"]', + ); + expect(expandRow?.classList.contains('hidden')).toBe(false); + expect(expandBtn!.getAttribute('aria-expanded')).toBe('true'); + }); + + it('expand button toggles correctly after multiple renderUsers calls', () => { + // Each renderUsers replaces the table innerHTML, so each new expand + // button element gets exactly one click listener. + userList.renderUsers(mockUsers as any); + userList.renderUsers(mockUsers as any); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="2"]', + ); + expandBtn!.click(); + + const expandRow = document.querySelector( + 'tr.user-expand-row[data-user-id="2"]', + ); + expect(expandRow?.classList.contains('hidden')).toBe(false); + + // Second click must collapse. + expandBtn!.click(); + expect(expandRow?.classList.contains('hidden')).toBe(true); + }); + }); + + // ----------------------------------------------------------------------- + // 4. In-memory state is patched after toggle (no full round-trip) + // ----------------------------------------------------------------------- + describe('in-memory state updated after toggle', () => { + it('patches allUsers so the toggled group appears in user.groups', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="2"]', + ); + expandBtn!.click(); + + // Bob starts with no groups; add g1. + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="2"][data-group-id="g1"]', + ); + cb!.checked = true; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + const updated = userState.allUsers.find(u => u.id === '2'); + expect(updated?.groups).toContain('g1'); + }); + + it('patches allUsers so the removed group disappears from user.groups', async () => { + jest.useFakeTimers(); + userList.renderUsers(mockUsers as any); + + const expandBtn = document.querySelector( + '.user-expand-btn[data-user-id="1"]', + ); + expandBtn!.click(); + + // Alice starts with ['g1']; remove g1. + const cb = document.querySelector( + '.group-assign-checkbox[data-user-id="1"][data-group-id="g1"]', + ); + cb!.checked = false; + cb!.dispatchEvent(new Event('change', { bubbles: true })); + + await Promise.resolve(); + await Promise.resolve(); + + const updated = userState.allUsers.find(u => u.id === '1'); + expect(updated?.groups).not.toContain('g1'); + }); + }); +}); diff --git a/frontend/src/users/userList.ts b/frontend/src/users/userList.ts index e1e71acaf..941fa3fe8 100644 --- a/frontend/src/users/userList.ts +++ b/frontend/src/users/userList.ts @@ -11,12 +11,20 @@ import { availableGroups, addSelectedUserId, removeSelectedUserId, - clearSelectedUserIds + clearSelectedUserIds, + setAllUsers, } from './state'; import { escapeHtml, formatRelativeTime, formatDate, showSuccess, showError } from './utils'; -import { openEditUserModal, deleteUser, loadUsers } from './userActions'; +import { openEditUserModal, deleteUser } from './userActions'; import { ADMINISTRATORS_GROUP_ID } from '../permissions'; +/** + * Single AbortController for the document-level group-assign-checkbox + * change listener. Replaced on every renderUsers() call so only one + * listener is ever active, preventing duplicate toasts (issue #998). + */ +let groupAssignListenerCtrl: AbortController | null = null; + /** * Render user statistics */ @@ -200,6 +208,11 @@ function renderUserExpandPanel(user: APIUser): string { /** * Toggle a group membership for a user inline (no modal). + * + * After a successful API call, patch state in-memory and refresh only the + * affected expand panel rather than re-rendering the whole table. This + * prevents the panel from collapsing on every toggle (issue #998) and + * keeps the user in context for subsequent group changes. */ async function toggleUserGroup(userId: string, groupId: string, checked: boolean): Promise { const user = allUsers.find(u => u.id === userId); @@ -214,14 +227,36 @@ async function toggleUserGroup(userId: string, groupId: string, checked: boolean try { await api.updateUser(userId, { groups: next }); + + // Patch the user in-memory so subsequent renders and effectivePermissions + // calculations reflect the change without a full round-trip re-render. + const updatedUser: APIUser = { ...user, groups: next }; + setAllUsers(allUsers.map(u => (u.id === userId ? updatedUser : u))); + + // Re-render the expand panel in place so the checkboxes and permissions + // summary stay current without collapsing the row or re-rendering the + // whole table (which would tear down all open panels). + const panel = document.querySelector( + `.user-expand-panel[data-user-id="${userId}"]`, + ); + // eslint-disable-next-line no-unsanitized/property + if (panel) panel.innerHTML = renderUserExpandPanel(updatedUser); + + // Refresh the group-badge cell in the main row so it reflects the new + // membership without a full re-render. + const groupsCell = document.querySelector( + `tr.user-row[data-user-id="${userId}"] .groups-cell`, + ); + if (groupsCell) { + groupsCell.innerHTML = updatedUser.groups.length > 0 + ? updatedUser.groups.map(g => `${escapeHtml(groupName(g))}`).join(' ') + : 'No groups'; + } + // Surface inline-save confirmation so users know the toggle stuck. - // Addresses the audit's "unclear if edits persist on collapse" - // concern by showing a toast instead of requiring an explicit - // Save/Cancel flow. showSuccess(checked ? `Added ${user.email} to "${groupLabel}"` : `Removed ${user.email} from "${groupLabel}"`); - await loadUsers(); } catch (error) { const err = error as Error; console.error('Failed to update user groups:', err); @@ -317,6 +352,12 @@ function setupUserTableListeners(): void { }); // Inline group assignment checkboxes inside the expand panel. + // Use AbortController so only one listener is ever registered on document, + // regardless of how many times renderUsers() is called. Without this, + // every render stacks another listener and triggers multiple toasts per + // toggle (issue #998). + groupAssignListenerCtrl?.abort(); + groupAssignListenerCtrl = new AbortController(); document.addEventListener('change', (e) => { const target = e.target as HTMLElement; if (!target.classList.contains('group-assign-checkbox')) return; @@ -324,7 +365,7 @@ function setupUserTableListeners(): void { const groupId = target.dataset.groupId; if (!userId || !groupId) return; void toggleUserGroup(userId, groupId, (target as HTMLInputElement).checked); - }); + }, { signal: groupAssignListenerCtrl.signal }); } /**