fix(frontend): show group form save refusals under the field, not only as a toast

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 = `
+    <div id="group-modal" class="hidden">
+      <span id="group-modal-title"></span>
+      <form id="group-form" novalidate>
+        <input type="hidden" id="group-id" />
+        <div class="form-group"><input type="text" id="group-name" required /></div>
+        <textarea id="group-description"></textarea>
+        <div class="form-group">
+          <div id="permissions-list"></div>
+          <button type="button" id="add-permission-btn">Add Permission</button>
+        </div>
+        <button type="submit">Save Group</button>
+      </form>
+    </div>
+  `;
+  wireGroupForm(e => { void groupModals.saveGroup(e); });
+}
+
+async function openWith(permissions: api.Permission[]): Promise<void> {
+  (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<void> {
+  (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<HTMLInputElement>(`.${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<HTMLButtonElement>('.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);
+  });
+});

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

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<HTMLInputElement>(`.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,17 +108,20 @@ export async function saveGroup(e: Event): Promise<void> {
   // 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
   // re-submit a materially different permission set than the one loaded, and
   // 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<void> {
 
   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<void> {
   // 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,27 +336,34 @@ 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 || '';
     const label = `permission ${index} (${action}:${resource})`;
 
     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;