Skip to content

fix(user-mgmt): single toast + stable editor on group update (closes #998) #1002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Jun 6, 2026
Merged

fix(user-mgmt): single toast + stable editor on group update (closes #998) #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
258 changes: 258 additions & 0 deletions frontend/src/__tests__/users.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = `
<div id="users-list"></div>
<div id="user-stats"></div>
<div id="bulk-actions-bar" class="hidden">
<span id="selected-count">0</span>
</div>
`;
}

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<HTMLButtonElement>(
'.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<HTMLInputElement>(
'.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<HTMLButtonElement>(
'.user-expand-btn[data-user-id="1"]',
);
expandBtn!.click();

const cb = document.querySelector<HTMLInputElement>(
'.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<HTMLButtonElement>(
'.user-expand-btn[data-user-id="1"]',
);
expandBtn!.click();

// Verify it is open.
const expandRow = document.querySelector<HTMLElement>(
'tr.user-expand-row[data-user-id="1"]',
);
expect(expandRow?.classList.contains('hidden')).toBe(false);

// Toggle a group checkbox.
const cb = document.querySelector<HTMLInputElement>(
'.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<HTMLButtonElement>(
'.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<HTMLInputElement>(
'.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<HTMLButtonElement>(
'.user-expand-btn[data-user-id="1"]',
);
expect(expandBtn).toBeTruthy();

// First click must open the panel.
expandBtn!.click();

const expandRow = document.querySelector<HTMLElement>(
'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<HTMLButtonElement>(
'.user-expand-btn[data-user-id="2"]',
);
expandBtn!.click();

const expandRow = document.querySelector<HTMLElement>(
'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<HTMLButtonElement>(
'.user-expand-btn[data-user-id="2"]',
);
expandBtn!.click();

// Bob starts with no groups; add g1.
const cb = document.querySelector<HTMLInputElement>(
'.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<HTMLButtonElement>(
'.user-expand-btn[data-user-id="1"]',
);
expandBtn!.click();

// Alice starts with ['g1']; remove g1.
const cb = document.querySelector<HTMLInputElement>(
'.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');
});
});
});
55 changes: 48 additions & 7 deletions frontend/src/users/userList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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<void> {
const user = allUsers.find(u => u.id === userId);
Expand All @@ -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<HTMLElement>(
`.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<HTMLElement>(
`tr.user-row[data-user-id="${userId}"] .groups-cell`,
);
if (groupsCell) {
groupsCell.innerHTML = updatedUser.groups.length > 0
? updatedUser.groups.map(g => `<span class="badge badge-group">${escapeHtml(groupName(g))}</span>`).join(' ')
: '<span class="text-muted">No groups</span>';
}

// 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);
Expand Down Expand Up @@ -317,14 +352,20 @@ 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;
const userId = target.dataset.userId;
const groupId = target.dataset.groupId;
if (!userId || !groupId) return;
void toggleUserGroup(userId, groupId, (target as HTMLInputElement).checked);
});
}, { signal: groupAssignListenerCtrl.signal });
}

/**
Expand Down
Loading