From 314936e495c3e76b02a880c9e7799757ff22770b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:14:51 +0000 Subject: [PATCH] fix(frontend): show group form save refusals under the field, not only as a toast Issue #727 moved the user form and the group form's blank-field checks to inline messages, but three group form refusals stayed toast-only: a Max Amount of 0 or less, a constraint list the form cannot carry back unchanged (stored or typed), and a group with no permissions. The toast fades, sits away from the field and gives a screen-reader user no cue on the control. Each refusal now also puts its message under the input to fix (or under Add Permission) through lib/field-errors, with aria-invalid, aria-describedby and focus on the first one. The toast stays as the row-naming summary. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/group-form-refusals-727.test.ts | 185 ++++++++++++++++++ frontend/src/groups/groupFormValidation.ts | 41 ++++ frontend/src/groups/groupModals.ts | 63 ++++-- 3 files changed, 274 insertions(+), 15 deletions(-) create mode 100644 frontend/src/__tests__/group-form-refusals-727.test.ts diff --git a/frontend/src/__tests__/group-form-refusals-727.test.ts b/frontend/src/__tests__/group-form-refusals-727.test.ts new file mode 100644 index 000000000..f92998aa0 --- /dev/null +++ b/frontend/src/__tests__/group-form-refusals-727.test.ts @@ -0,0 +1,185 @@ +/** + * Issue #727 (UX-33), group form: the refusals that follow the blank-field + * checks (a bad Max Amount, an unrepresentable constraint, no permissions at + * all) used to be toast-only. Each now also shows under the field to fix, + * through lib/field-errors: aria-invalid, aria-describedby, focus on the + * first problem, and the next edit clears it. The toast stays as a summary. + */ +import './setup'; + +jest.mock('../api', () => ({ + getGroup: jest.fn(), + updateGroup: jest.fn(), + createGroup: jest.fn(), +})); + +jest.mock('../users/userActions', () => ({ + loadUsers: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn().mockResolvedValue(true), +})); + +jest.mock('../users/utils', () => ({ + ...jest.requireActual('../users/utils'), + showError: jest.fn(), + showSuccess: jest.fn(), +})); + +import * as api from '../api'; +import * as groupState from '../groups/state'; +import * as groupModals from '../groups/groupModals'; +import { wireGroupForm } from '../groups/groupFormValidation'; +import { showError } from '../users/utils'; + +function setUpModalDom(): void { + // Mirrors index.html's #group-modal, including the Add Permission button. + document.body.innerHTML = ` + + `; + wireGroupForm(e => { void groupModals.saveGroup(e); }); +} + +async function openWith(permissions: api.Permission[]): Promise { + (api.getGroup as jest.Mock).mockResolvedValue({ + id: 'g1', name: 'Finance', description: '', permissions, created_at: '2024-01-01T00:00:00Z', + }); + await groupModals.openEditGroupModal('g1'); +} + +async function clickSave(): Promise { + (document.getElementById('group-form') as HTMLFormElement).requestSubmit(); + await new Promise(r => setTimeout(r, 0)); +} + +function input(cls: string, row = 0): HTMLInputElement { + return document.querySelectorAll('#permissions-list .permission-item')[row]!.querySelector(`.${cls}`)!; +} + +/** The inline message linked to `control`, or null when it has none. */ +function inlineError(control: HTMLElement): string | null { + if (control.getAttribute('aria-invalid') !== 'true') return null; + const ids = (control.getAttribute('aria-describedby') ?? '').split(/\s+/); + const note = ids.map(id => document.getElementById(id)).find(el => el?.classList.contains('field-error')); + return note?.textContent ?? null; +} + +function type(control: HTMLInputElement, value: string): void { + control.value = value; + control.dispatchEvent(new Event('input', { bubbles: true })); +} + +describe('group form refusals show under the field (issue #727)', () => { + beforeEach(() => { + setUpModalDom(); + groupState.setCurrentEditingGroup(null); + jest.clearAllMocks(); + }); + + test('a Max Amount of 0 is marked on that input, focused, and cleared by the next edit', async () => { + await openWith([ + { action: 'view', resource: 'plans' }, + { action: 'update-any', resource: 'purchases', constraints: { max_amount: 5000 } }, + ]); + const amount = input('perm-max-amount', 1); + amount.value = '0'; + await clickSave(); + + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(inlineError(amount)).toBe('Enter an amount greater than 0, or leave it blank for no limit.'); + expect(inlineError(input('perm-max-amount', 0))).toBeNull(); + expect(document.activeElement).toBe(amount); + // The note sits below the flex .form-row, not beside the input. + const note = document.getElementById(amount.getAttribute('aria-describedby')!)!; + expect(note.previousElementSibling).toBe(amount.closest('.form-row')); + // The toast summary still names the row. + expect(showError).toHaveBeenCalledWith(expect.stringContaining('(permission 1)')); + + type(amount, '250'); + expect(inlineError(amount)).toBeNull(); + expect(document.querySelectorAll('.field-error')).toHaveLength(0); + }); + + test('a typed constraint with no usable entries is marked on that input', async () => { + await openWith([{ action: 'view', resource: 'history', constraints: { regions: ['us-east-1'] } }]); + const regions = input('perm-regions'); + regions.value = ' , '; + await clickSave(); + + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(inlineError(regions)).toBe('This list has no usable entries. Enter at least one value, or clear the box for no restriction.'); + expect(inlineError(input('perm-accounts'))).toBeNull(); + expect(document.activeElement).toBe(regions); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('"regions" value with no usable entries')); + }); + + test('a stored constraint value the form cannot represent is marked on each affected input', async () => { + await openWith([{ action: 'view', resource: 'history', constraints: { accounts: [' acct A '], services: [''] } }]); + await clickSave(); + + expect(api.updateGroup).not.toHaveBeenCalled(); + for (const cls of ['perm-accounts', 'perm-services']) { + expect(inlineError(input(cls))).toMatch(/^This stored value cannot be shown in this form/); + } + expect(inlineError(input('perm-regions'))).toBeNull(); + // One toast entry for the row, not one per input. + const toast = (showError as jest.Mock).mock.calls[0][0] as string; + expect(toast.match(/permission 0/g)).toHaveLength(1); + expect(toast).toContain('"accounts, services"'); + }); + + test('no permissions at all is marked at Add Permission, and adding a row clears it', async () => { + await openWith([{ action: 'admin', resource: '*' }]); + document.querySelector('.remove-permission-btn')!.click(); + await clickSave(); + + const add = document.getElementById('add-permission-btn')!; + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(inlineError(add)).toBe('Add at least one permission. To remove all privileges, delete the group instead.'); + expect(document.activeElement).toBe(add); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + + add.addEventListener('click', () => groupModals.addPermission()); + add.click(); + expect(inlineError(add)).toBeNull(); + expect(document.querySelectorAll('.field-error')).toHaveLength(0); + }); + + test('a fixed form saves and leaves no stale field errors behind', async () => { + await openWith([{ action: 'update-any', resource: 'purchases', constraints: { max_amount: 5000 } }]); + const amount = input('perm-max-amount'); + amount.value = '-1'; + await clickSave(); + expect(inlineError(amount)).not.toBeNull(); + + amount.value = '100'; + await clickSave(); + expect(api.updateGroup).toHaveBeenCalledTimes(1); + expect(document.querySelectorAll('.field-error')).toHaveLength(0); + expect(amount.hasAttribute('aria-invalid')).toBe(false); + }); + + test('reopening the modal clears field errors from a refused save', async () => { + await openWith([{ action: 'update-any', resource: 'purchases', constraints: { max_amount: 5000 } }]); + input('perm-max-amount').value = '0'; + await clickSave(); + expect(document.querySelectorAll('.field-error')).toHaveLength(1); + + groupModals.openCreateGroupModal(); + expect(document.querySelectorAll('.field-error')).toHaveLength(0); + expect(document.getElementById('add-permission-btn')!.hasAttribute('aria-invalid')).toBe(false); + }); +}); diff --git a/frontend/src/groups/groupFormValidation.ts b/frontend/src/groups/groupFormValidation.ts index 0bac5213e..4e4c93dbb 100644 --- a/frontend/src/groups/groupFormValidation.ts +++ b/frontend/src/groups/groupFormValidation.ts @@ -22,6 +22,8 @@ * so re-selecting the placeholder brings the note back. */ +import { clearFieldError, clearFieldErrors, showFieldError } from '../lib/field-errors'; + const NOTE_CLASS = 'group-form-note'; const NAME_NOTE_ID = 'group-name-error'; @@ -67,11 +69,50 @@ function rowMessage(actionBlank: boolean, resourceBlank: boolean): string { export function clearGroupFormValidation(): void { const f = form(); if (!f) return; + clearFieldErrors(f); f.querySelectorAll(`.${NOTE_CLASS}`).forEach(n => n.remove()); f.querySelectorAll('[aria-invalid="true"]').forEach(el => setInvalid(el as HTMLElement, null)); delete f.dataset['validated']; } +/** + * A reason saveGroup refuses after the blank-field checks (issue #727): an + * unrepresentable constraint, a bad Max Amount, or no permissions at all, + * tied to the control that has to change. + */ +export interface GroupFieldProblem { + control: HTMLElement; + /** Shown under the field. */ + message: string; + /** The same reason for the toast summary, naming the permission row. */ + summary: string; +} + +/** + * Show each problem under its field through lib/field-errors (aria-invalid, + * aria-describedby, cleared by the next edit) and focus the first. Inside a + * permission row the note goes after the flex .form-row, not beside the + * input. The caller keeps its toast as the summary. + */ +export function showGroupFieldProblems(problems: readonly GroupFieldProblem[]): void { + for (const p of problems) { + showFieldError(p.control, p.message, p.control.closest('.form-row') ?? undefined); + } + problems[0]?.control.focus(); +} + +/** Drop the field problems of an earlier refused save before re-checking. */ +export function clearGroupFieldProblems(): void { + const f = form(); + if (f) clearFieldErrors(f); +} + +/** A newly added permission row answers the "no permissions" refusal. */ +export function clearNoPermissionsProblem(): void { + const add = document.getElementById('add-permission-btn'); + if (add) clearFieldError(add); +} + /** * Check the name and every permission row, render the notes, and return the * first invalid control (null when the form can be saved). `focus` moves diff --git a/frontend/src/groups/groupModals.ts b/frontend/src/groups/groupModals.ts index 9adf77bd0..614cd72b1 100644 --- a/frontend/src/groups/groupModals.ts +++ b/frontend/src/groups/groupModals.ts @@ -10,9 +10,24 @@ import { escapeHtml, showError, showSuccess } from '../users/utils'; import { loadUsers } from '../users/userActions'; import { openModal, closeModal } from '../modal'; import { iconSvg } from '../icons'; -import { clearGroupFormValidation, validateGroupForm } from './groupFormValidation'; +import { + clearGroupFieldProblems, clearGroupFormValidation, clearNoPermissionsProblem, + showGroupFieldProblems, validateGroupForm, type GroupFieldProblem, +} from './groupFormValidation'; import { buildActionOptions, buildResourceOptions } from './permissionOptions'; +// Messages shown under the field for the saveGroup refusals that follow the +// blank-field checks (issue #727). The toasts keep the row-naming summary. +const NO_PERMISSIONS_FIELD_MESSAGE = 'Add at least one permission. To remove all privileges, delete the group instead.'; +const STORED_VALUE_FIELD_MESSAGE = 'This stored value cannot be shown in this form, so saving would drop the restriction. ' + + 'Repair it through the API or in the database, then reload this page.'; +const EMPTY_LIST_FIELD_MESSAGE = 'This list has no usable entries. Enter at least one value, or clear the box for no restriction.'; +const MAX_AMOUNT_FIELD_MESSAGE = 'Enter an amount greater than 0, or leave it blank for no limit.'; + +function constraintInput(item: Element, dimension: string): HTMLInputElement | null { + return item.querySelector(`.perm-${dimension}`); +} + // Module-level state for the duplicate modal — holds the source group so // saveDuplicateGroup doesn't need another lookup. let duplicateSourceGroup: APIGroup | null = null; @@ -93,6 +108,7 @@ export async function saveGroup(e: Event): Promise { // with a note under the field, never dropped or widened (#group-form is // novalidate, so this is the only gate). if (validateGroupForm()) return; + clearGroupFieldProblems(); // Refuse rather than widen. A stored constraint value this form cannot // carry unchanged is flagged when the row is rendered; saving anyway would @@ -100,10 +116,12 @@ export async function saveGroup(e: Event): Promise { // for a constraint list that means an unintended widening (an empty list is // "no restriction" at enforcement). The group stays uneditable through the // form until the value is repaired via the API or in the database. + // Issue #727: each refusal below also shows under the field to fix. const blocked = unrepresentablePermissionErrors(); if (blocked.length > 0) { + showGroupFieldProblems(blocked); showError( - `Cannot save: ${blocked.join('; ')}. ` + + `Cannot save: ${blocked.map(p => p.summary).filter(Boolean).join('; ')}. ` + 'Saving would silently drop the restriction, so the edit is refused. ' + 'Repair the value through the API or in the database, then reload this page.' ); @@ -112,8 +130,9 @@ export async function saveGroup(e: Event): Promise { const badAmounts = maxAmountInputErrors(); if (badAmounts.length > 0) { + showGroupFieldProblems(badAmounts); showError( - `Max Amount must be greater than 0; leave it blank for no limit (${badAmounts.join(', ')}).` + `Max Amount must be greater than 0; leave it blank for no limit (${badAmounts.map(p => p.summary).join(', ')}).` ); return; } @@ -130,6 +149,10 @@ export async function saveGroup(e: Event): Promise { // keeping the group's old privileges -- there is no way for this form to // express "clear all permissions". if (permissions.length === 0) { + const add = document.getElementById('add-permission-btn'); + if (add) { + showGroupFieldProblems([{ control: add, message: NO_PERMISSIONS_FIELD_MESSAGE, summary: '' }]); + } showError( 'A group must grant at least one permission. ' + 'To remove all privileges, delete the group instead.' @@ -227,6 +250,7 @@ export function addPermission(permission?: Permission): void { `; permissionsList.appendChild(permDiv); + clearNoPermissionsProblem(); // Add event listener for remove button const removeBtn = permDiv.querySelector('.remove-permission-btn'); @@ -312,11 +336,11 @@ function unrepresentableDimensions(constraints: Permission['constraints']): stri // which is ordinary. Blank is measured after trimming, so a box holding only // spaces counts as blank -- it is indistinguishable from empty on screen, and // erroring on a field that looks empty would be unfixable from the UI. -function unrepresentablePermissionErrors(): string[] { +function unrepresentablePermissionErrors(): GroupFieldProblem[] { const permissionsList = document.getElementById('permissions-list'); if (!permissionsList) return []; - const errors: string[] = []; + const errors: GroupFieldProblem[] = []; permissionsList.querySelectorAll('.permission-item').forEach((item, index) => { const action = (item.querySelector('.perm-action') as HTMLSelectElement | null)?.value || ''; const resource = (item.querySelector('.perm-resource') as HTMLSelectElement | null)?.value || ''; @@ -324,15 +348,22 @@ function unrepresentablePermissionErrors(): string[] { const stored = (item.getAttribute('data-unrepresentable') || '').split(', ').filter(d => d); if (stored.length > 0) { - errors.push(`${label} has a stored "${stored.join(', ')}" constraint value this form cannot represent`); + const summary = `${label} has a stored "${stored.join(', ')}" constraint value this form cannot represent`; + // Every affected input gets the inline note; the toast names the row once. + stored.forEach((dimension, i) => errors.push({ + control: constraintInput(item, dimension) ?? (item as HTMLElement), + message: STORED_VALUE_FIELD_MESSAGE, + summary: i === 0 ? summary : '', + })); } for (const dimension of LIST_CONSTRAINT_DIMENSIONS) { // Already refused for this row on the stored value; do not say it twice. if (stored.includes(dimension)) continue; - const raw = (item.querySelector(`.perm-${dimension}`) as HTMLInputElement | null)?.value ?? ''; - if (raw.trim() !== '' && parseConstraintList(raw).length === 0) { - errors.push(`${label} has a "${dimension}" value with no usable entries`); + const control = constraintInput(item, dimension); + const raw = control?.value ?? ''; + if (control && raw.trim() !== '' && parseConstraintList(raw).length === 0) { + errors.push({ control, message: EMPTY_LIST_FIELD_MESSAGE, summary: `${label} has a "${dimension}" value with no usable entries` }); } } }); @@ -342,21 +373,23 @@ function unrepresentablePermissionErrors(): string[] { // Max Amount is typed by the admin, unlike the stored-value problems above, so // its message tells them how to fix the field instead of pointing at the API. // 0 would read as "no cap" at enforcement, so it is refused rather than sent. -function maxAmountInputErrors(): string[] { - const errors: string[] = []; +function maxAmountInputErrors(): GroupFieldProblem[] { + const errors: GroupFieldProblem[] = []; document.querySelectorAll('#permissions-list .permission-item').forEach((item, index) => { const input = item.querySelector('.perm-max-amount') as HTMLInputElement | null; + if (!input) return; + const problem = { control: input, message: MAX_AMOUNT_FIELD_MESSAGE, summary: `permission ${index}` }; // A number input reports unparsable text ("abc", "1e") as value "", which // would read as blank and save with no cap; badInput is the only signal. - if (input?.validity?.badInput) { - errors.push(`permission ${index}`); + if (input.validity?.badInput) { + errors.push(problem); return; } - const raw = input?.value ?? ''; + const raw = input.value; if (raw.trim() === '') return; const amount = Number(raw); if (!Number.isFinite(amount) || amount <= 0) { - errors.push(`permission ${index}`); + errors.push(problem); } }); return errors;