diff --git a/admin/src/definitions/DefinitionsTab.css b/admin/src/definitions/DefinitionsTab.css index 0c115ba..1b9f425 100644 --- a/admin/src/definitions/DefinitionsTab.css +++ b/admin/src/definitions/DefinitionsTab.css @@ -353,6 +353,16 @@ color: var(--nq-brand-blue); } +.defs-picker-option--locked { + color: var(--nq-a-ink-secondary); + cursor: default; +} + +.defs-picker-note { + font-size: 12px; + font-weight: 600; +} + .defs-repeats { display: flex; gap: 8px; @@ -493,6 +503,24 @@ margin-top: 16px; } +.defs-confirm { + margin-top: 12px; + padding: 10px 12px; + border-radius: var(--nq-a-density-radius); + background: var(--nq-a-danger-bg); + color: var(--nq-a-danger-strong); + font-size: 12.5px; + font-weight: 700; +} + +.defs-confirm p { + margin: 0; +} + +.defs-confirm .defs-sheet-footer { + margin-top: 10px; +} + .defs-cancel, .defs-save { padding: 13px 0; diff --git a/admin/src/definitions/DefinitionsTab.test.tsx b/admin/src/definitions/DefinitionsTab.test.tsx index 96c0654..1ae0d64 100644 --- a/admin/src/definitions/DefinitionsTab.test.tsx +++ b/admin/src/definitions/DefinitionsTab.test.tsx @@ -152,6 +152,14 @@ function fakeApi(initial: QuestDefinition[]) { title: body.title ?? d.title, skip_on_away: body.skip_on_away ?? d.skip_on_away, rule: body.rule ?? d.rule, + // Like the API: a sent list replaces the whole set; an absent one keeps it. + assignees: + "assignee_child_ids" in body + ? CHILDREN.filter((c) => body.assignee_child_ids.includes(c.id)).map((c) => ({ + id: c.id, + display_name: c.display_name, + })) + : d.assignees, } : d, ); @@ -360,9 +368,13 @@ describe("DefinitionsTab", () => { expect((within(s).getByLabelText("Afternoon due time") as HTMLInputElement).value).toBe( "16:00", ); - // The inactive child "Old" is not offered and not submitted. + // The inactive child "Old" is shown locked, not offered as a checkbox. fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); expect(within(s).queryByRole("checkbox", { name: "Old" })).toBeNull(); + const locked = within(s).getByTestId("inactive-assignee-4"); + expect(locked.textContent).toContain("Old"); + expect(locked.textContent).toContain("Inactive"); + expect(locked.getAttribute("aria-disabled")).toBe("true"); fireEvent.click(within(s).getByRole("checkbox", { name: "Maeve" })); fireEvent.change(within(s).getByRole("textbox", { name: "Title" }), { @@ -382,8 +394,11 @@ describe("DefinitionsTab", () => { }); fireEvent.click(within(s).getByRole("switch", { name: "Skip on away days" })); + // Changing the selection would drop "Old": confirm before sending. + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + expect(api.writes()).toHaveLength(0); await act(async () => { - fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + fireEvent.click(within(s).getByRole("button", { name: "Unassign and save" })); }); expect(api.writes()).toEqual([ @@ -417,6 +432,125 @@ describe("DefinitionsTab", () => { expect(screen.queryByRole("dialog")).toBeNull(); }); + it("editing only the title omits assignee_child_ids and keeps an inactive assignee", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-11")); + const s = sheet(); + expect(within(s).getByText(/Also assigned \(inactive\): Old\./)).toBeTruthy(); + fireEvent.change(within(s).getByRole("textbox", { name: "Title" }), { + target: { value: "Bins out" }, + }); + + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + + expect(screen.queryByTestId("unassign-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(1); + const body = api.writes()[0].body as Record; + expect(api.writes()[0].method).toBe("PATCH"); + expect(body.title).toBe("Bins out"); + expect(body).not.toHaveProperty("assignee_child_ids"); + + await screen.findByText("Bins out"); + expect(meta(screen.getByTestId("definition-11"))).toBe( + "Declan, Old · Every 2 weeks on Mon, Thu · Afternoon", + ); + }); + + it("changing the active selection with an inactive assignee asks first", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-11")); + const s = sheet(); + fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Rory" })); + + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + const confirm = within(s).getByTestId("unassign-confirm"); + expect(confirm.textContent).toContain("Saving will unassign Old."); + expect(document.activeElement).toBe(confirm); + expect(api.writes()).toHaveLength(0); + + // Going back sends nothing and restores the normal footer. + fireEvent.click(within(confirm).getByRole("button", { name: "Go back" })); + expect(within(s).queryByTestId("unassign-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(0); + + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Unassign and save" })); + }); + expect(api.writes()).toHaveLength(1); + expect((api.writes()[0].body as { assignee_child_ids: number[] }).assignee_child_ids).toEqual([ + 1, 3, + ]); + }); + + it("reverting the selection to the original omits assignee_child_ids without asking", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-11")); + const s = sheet(); + fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Rory" })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Rory" })); + + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + expect(screen.queryByTestId("unassign-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(1); + expect(api.writes()[0].body).not.toHaveProperty("assignee_child_ids"); + }); + + it("focus moves into the sheet on open and returns to the row on Escape", async () => { + await renderReady(); + const row = screen.getByTestId("definition-10"); + row.focus(); + fireEvent.click(row); + const s = sheet(); + expect(document.activeElement).toBe(within(s).getByRole("heading", { name: "Edit task" })); + + fireEvent.keyDown(s, { key: "Escape" }); + expect(screen.queryByRole("dialog")).toBeNull(); + expect(document.activeElement).toBe(row); + expect(api.writes()).toHaveLength(0); + }); + + it("Tab and Shift+Tab wrap within the sheet", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-10")); + const s = sheet(); + const title = within(s).getByRole("textbox", { name: "Title" }); + const save = within(s).getByRole("button", { name: "Save changes" }); + + save.focus(); + fireEvent.keyDown(s, { key: "Tab" }); + expect(document.activeElement).toBe(title); + + fireEvent.keyDown(s, { key: "Tab", shiftKey: true }); + expect(document.activeElement).toBe(save); + }); + + it("Escape does not close the sheet while saving", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-10")); + let release: (response: Response) => void = () => {}; + api.fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (release = resolve)), + ); + const s = sheet(); + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + fireEvent.keyDown(s, { key: "Escape" }); + expect(screen.getByRole("dialog")).toBeTruthy(); + + await act(async () => { + release(jsonResponse(BRUSH)); + }); + await vi.waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + }); + it("the skip-on-away toggle reflects and persists a false value", async () => { await renderReady(); fireEvent.click(screen.getByTestId("definition-11")); diff --git a/admin/src/definitions/DefinitionsTab.tsx b/admin/src/definitions/DefinitionsTab.tsx index b136446..2bb80e6 100644 --- a/admin/src/definitions/DefinitionsTab.tsx +++ b/admin/src/definitions/DefinitionsTab.tsx @@ -4,7 +4,13 @@ * The icon picker (§3.3) and the occurrence preview are separate tasks: an * existing definition's icon is carried through unchanged by never sending it. */ -import { useEffect, useRef, useState, type ReactNode } from "react"; +import { + useEffect, + useRef, + useState, + type KeyboardEvent as ReactKeyboardEvent, + type ReactNode, +} from "react"; import { ApiForbiddenError } from "../api/client"; import { ApiRequestError, @@ -14,6 +20,7 @@ import { updateDefinition, type AdminChild, type DefinitionCreateBody, + type DefinitionEditBody, type DefinitionRule, type QuestDefinition, type WindowEntry, @@ -202,10 +209,14 @@ function buildRule(draft: Draft): DefinitionRule | string { } } -function buildBody(draft: Draft): DefinitionCreateBody | string { +/** + * `keepsAssignees`: an edit whose active selection is unchanged sends no + * assignee list, so an empty selection is fine when inactive children remain. + */ +function buildBody(draft: Draft, keepsAssignees = false): DefinitionCreateBody | string { const title = draft.title.trim(); if (!title) return "Give the task a title."; - if (draft.assigneeIds.length === 0) return "Assign at least one child."; + if (draft.assigneeIds.length === 0 && !keepsAssignees) return "Assign at least one child."; if (draft.windows.length === 0) return "Pick at least one window."; const rule = buildRule(draft); if (typeof rule === "string") return rule; @@ -231,6 +242,13 @@ function toggle(list: T[], value: T): T[] { return list.includes(value) ? list.filter((item) => item !== value) : [...list, value]; } +function sameIds(a: number[], b: number[]): boolean { + return a.length === b.length && a.every((id) => b.includes(id)); +} + +const FOCUSABLE = + 'button:not([disabled]), input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [href], [tabindex]:not([tabindex="-1"])'; + /* ── List ───────────────────────────────────────────────────────────── */ function FilterChips({ @@ -334,11 +352,58 @@ function EditSheet({ const [draft, setDraft] = useState(() => target === "new" ? newDraft(activeChildren) : draftFrom(target, activeChildren), ); + // The active selection as loaded: while it is unchanged, the PATCH omits + // assignee_child_ids, because the API replaces the whole set and inactive + // children cannot be sent back. + const [originalActiveIds] = useState(() => draft.assigneeIds); + const inactiveAssignees = + target === "new" + ? [] + : target.assignees.filter((a) => !activeChildren.some((child) => child.id === a.id)); const [pickerOpen, setPickerOpen] = useState(false); const [saving, setSaving] = useState(false); + const [confirming, setConfirming] = useState(false); const [error, setError] = useState(null); // A ref, not state: two taps in one frame must not both see `saving === false`. const inFlight = useRef(false); + const sheetRef = useRef(null); + const titleRef = useRef(null); + const confirmRef = useRef(null); + + // Modal focus: move in on open, hand back to the trigger on close. + useEffect(() => { + const trigger = document.activeElement as HTMLElement | null; + titleRef.current?.focus(); + return () => { + if (trigger && trigger.isConnected) trigger.focus(); + }; + }, []); + + useEffect(() => { + if (confirming) confirmRef.current?.focus(); + }, [confirming]); + + function onKeyDown(event: ReactKeyboardEvent) { + if (event.key === "Escape") { + event.preventDefault(); + if (!inFlight.current) onCancel(); + return; + } + if (event.key !== "Tab" || !sheetRef.current) return; + const focusable = Array.from(sheetRef.current.querySelectorAll(FOCUSABLE)); + if (focusable.length === 0) return; + const first = focusable[0]; + const last = focusable[focusable.length - 1]; + const current = document.activeElement; + const outside = !focusable.includes(current as HTMLElement); + if (event.shiftKey && (current === first || outside)) { + event.preventDefault(); + last.focus(); + } else if (!event.shiftKey && (current === last || outside)) { + event.preventDefault(); + first.focus(); + } + } const update = (patch: Partial) => setDraft((d) => ({ ...d, ...patch })); @@ -347,19 +412,32 @@ function EditSheet({ .map((child) => ({ id: child.id, display_name: child.display_name })); const orderedWindows = WINDOW_ORDER.filter((w) => draft.windows.includes(w)); - async function save() { + async function save(confirmed = false) { if (inFlight.current) return; - const body = buildBody(draft); + const keepsAssignees = target !== "new" && sameIds(draft.assigneeIds, originalActiveIds); + const body = buildBody(draft, keepsAssignees); if (typeof body === "string") { setError(body); + setConfirming(false); return; } + if (target !== "new" && !keepsAssignees && inactiveAssignees.length > 0 && !confirmed) { + setError(null); + setConfirming(true); + return; + } + setConfirming(false); inFlight.current = true; setSaving(true); setError(null); try { - if (target === "new") await createDefinition(body); - else await updateDefinition(target.id, body); + if (target === "new") { + await createDefinition(body); + } else { + const edit: DefinitionEditBody = { ...body }; + if (keepsAssignees) delete edit.assignee_child_ids; + await updateDefinition(target.id, edit); + } onSaved(); } catch (err) { setError(saveErrorMessage(err)); @@ -378,13 +456,15 @@ function EditSheet({ }} />
);