From e810243dd2696bc344799d9249d75e356f1b9d79 Mon Sep 17 00:00:00 2001 From: Joshua Date: Sat, 26 Sep 2026 08:54:54 -0400 Subject: [PATCH] Admin: omit an unchanged icon from the definition PATCH (NQ-ICON-02) A definition stored with a legacy icon (e.g. mdi:tooth) reads back as lucide:mdi:tooth, which the API's strict validate_icon rejects, so any edit that echoed the icon back failed. Mirror the keepsAssignees pattern: keep the loaded icon and delete `icon` from the PATCH body while the draft icon is unchanged. The edit route dumps the body with exclude_unset, so an absent icon leaves the stored value untouched; a new choice is sent and No icon still sends null. Create always sends icon. Co-Authored-By: Claude Opus 5.5 --- admin/src/definitions/DefinitionsTab.test.tsx | 119 +++++++++++++++++- admin/src/definitions/DefinitionsTab.tsx | 5 + 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/admin/src/definitions/DefinitionsTab.test.tsx b/admin/src/definitions/DefinitionsTab.test.tsx index 2f6d818..7ee2a0d 100644 --- a/admin/src/definitions/DefinitionsTab.test.tsx +++ b/admin/src/definitions/DefinitionsTab.test.tsx @@ -161,6 +161,10 @@ function fakeApi(initial: QuestDefinition[]) { } const match = /^\/api\/v1\/admin\/quest-definitions\/(\d+)$/.exec(path); if (method === "PATCH" && match) { + // Like the API's strict validate_icon: a Lucide name never has a colon. + if (typeof body.icon === "string" && /^lucide:.*:/.test(body.icon)) { + return jsonResponse({ detail: "icon is not a valid icon key" }, 422); + } const id = Number(match[1]); definitions = definitions.map((d) => d.id === id @@ -463,7 +467,6 @@ describe("DefinitionsTab", () => { path: "/api/v1/admin/quest-definitions/11", body: { title: "Take out the bins", - icon: null, rule: { rule_type: "monthly_day", interval: 1, @@ -1087,7 +1090,7 @@ describe("DefinitionsTab icon picker", () => { await waitFor(() => expect(rowIcon(screen.getByTestId("definition-11"))).toBe("trash-2")); }); - it("an unchanged icon is still sent on edit and shows as selected", async () => { + it("an unchanged icon is omitted on edit and shows as selected", async () => { await renderReady(); expect(rowIcon(screen.getByTestId("definition-10"))).toBe("smile"); fireEvent.click(screen.getByTestId("definition-10")); @@ -1101,7 +1104,7 @@ describe("DefinitionsTab icon picker", () => { fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); }); - expect((api.writes()[0].body as { icon: string }).icon).toBe("smile"); + expect(api.writes()[0].body).not.toHaveProperty("icon"); }); it("an unknown legacy icon falls back to the generic glyph and is kept on save", async () => { @@ -1121,7 +1124,7 @@ describe("DefinitionsTab icon picker", () => { fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); }); - expect((api.writes()[0].body as { icon: string }).icon).toBe("rocket-ship-9000"); + expect(api.writes()[0].body).not.toHaveProperty("icon"); await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); expect(rowIcon(screen.getByTestId("definition-20"))).toBeNull(); }); @@ -1408,7 +1411,7 @@ describe("DefinitionsTab Font Awesome and emoji icons", () => { await waitFor(() => expect(rowGlyph(screen.getByTestId("definition-52"))).toBe("fa:paw")); }); - it("an unchanged emoji is sent as-is on edit", async () => { + it("an unchanged emoji is omitted on edit", async () => { await renderReady([{ ...BRUSH, id: 53, icon: "emoji:🐶" }]); fireEvent.click(screen.getByTestId("definition-53")); const s = sheet(); @@ -1416,7 +1419,7 @@ describe("DefinitionsTab Font Awesome and emoji icons", () => { target: { value: "Feed the dog" }, }); await save(s); - expect((api.writes()[0].body as { icon: string }).icon).toBe("emoji:🐶"); + expect(api.writes()[0].body).not.toHaveProperty("icon"); }); it("No icon and an emptied emoji field both clear the icon to null", async () => { @@ -1442,6 +1445,110 @@ describe("DefinitionsTab Font Awesome and emoji icons", () => { }); }); +/* ── Legacy icons ───────────────────────────────────────────────────── */ + +describe("DefinitionsTab legacy icon edits", () => { + // The API reads a stored legacy `mdi:tooth` back as `lucide:mdi:tooth`, + // which its strict write validation rejects if sent back. + const LEGACY: QuestDefinition = { ...BRUSH, id: 60, icon: "lucide:mdi:tooth" }; + + beforeEach(() => { + sessionStorage.clear(); + storeTokens({ access_token: "at-123", expires_in: 600 }); + }); + + afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + vi.useRealTimers(); + }); + + function radio(s: HTMLElement, group: "Icon" | "Font Awesome", label: string): HTMLElement { + return within(within(s).getByRole("radiogroup", { name: group })).getByRole("radio", { + name: label, + }); + } + + async function save(s: HTMLElement) { + await act(async () => { + fireEvent.click(within(s).getByRole("button", { name: "Save changes" })); + }); + } + + it("editing only the title omits the legacy icon and saves", async () => { + await renderReady([LEGACY]); + fireEvent.click(screen.getByTestId("definition-60")); + const s = sheet(); + fireEvent.change(within(s).getByRole("textbox", { name: "Title" }), { + target: { value: "Brush teeth well" }, + }); + await save(s); + + expect(api.writes()).toHaveLength(1); + expect(api.writes()[0].method).toBe("PATCH"); + expect(api.writes()[0].body).not.toHaveProperty("icon"); + expect((api.writes()[0].body as { title: string }).title).toBe("Brush teeth well"); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + expect(await screen.findByText("Brush teeth well")).toBeTruthy(); + }); + + for (const [label, choose, sent] of [ + ["a Lucide", (s: HTMLElement) => fireEvent.click(radio(s, "Icon", "Dog")), "lucide:dog"], + ["a Font Awesome", (s: HTMLElement) => fireEvent.click(radio(s, "Font Awesome", "Dog")), "fa:dog"], + [ + "an emoji", + (s: HTMLElement) => + fireEvent.change(within(s).getByRole("textbox", { name: "Emoji" }), { + target: { value: "🦷" }, + }), + "emoji:🦷", + ], + ["No icon", (s: HTMLElement) => fireEvent.click(radio(s, "Icon", "No icon")), null], + ] as const) { + it(`choosing ${label} replacement for the legacy icon sends ${JSON.stringify(sent)}`, async () => { + await renderReady([LEGACY]); + fireEvent.click(screen.getByTestId("definition-60")); + const s = sheet(); + choose(s); + await save(s); + + expect(api.writes()).toHaveLength(1); + expect(api.writes()[0].body).toHaveProperty("icon", sent); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + }); + } + + it("an invalid emoji replacing the legacy icon is still rejected before the request", async () => { + await renderReady([LEGACY]); + fireEvent.click(screen.getByTestId("definition-60")); + const s = sheet(); + fireEvent.change(within(s).getByRole("textbox", { name: "Emoji" }), { + target: { value: "a b" }, + }); + await save(s); + + expect(api.writes()).toHaveLength(0); + expect(within(s).getByText(/Enter a single emoji/)).toBeTruthy(); + }); + + it("creating a task still sends icon, even when none is chosen", async () => { + await renderReady([LEGACY]); + fireEvent.click(screen.getByRole("button", { name: "New" })); + const s = sheet(); + fireEvent.change(within(s).getByRole("textbox", { name: "Title" }), { + target: { value: "Floss" }, + }); + fireEvent.click(within(s).getByRole("button", { name: /Assigned children/ })); + fireEvent.click(within(s).getByRole("checkbox", { name: "Declan" })); + fireEvent.click(within(s).getByRole("button", { name: "Morning" })); + await save(s); + + expect(api.writes()).toHaveLength(1); + expect(api.writes()[0].method).toBe("POST"); + expect(api.writes()[0].body).toHaveProperty("icon", null); + }); +}); + /* ── Start date ─────────────────────────────────────────────────────── */ function startDateInput(s: HTMLElement): HTMLInputElement { diff --git a/admin/src/definitions/DefinitionsTab.tsx b/admin/src/definitions/DefinitionsTab.tsx index fcd174a..4ac7748 100644 --- a/admin/src/definitions/DefinitionsTab.tsx +++ b/admin/src/definitions/DefinitionsTab.tsx @@ -462,6 +462,10 @@ function EditSheet({ // assignee_child_ids, because the API replaces the whole set and inactive // children cannot be sent back. const [originalActiveIds] = useState(() => draft.assigneeIds); + // The icon as loaded: while it is unchanged, the PATCH omits `icon` (absent + // means unchanged), because a legacy value read back as `lucide:` would + // fail the API's strict icon validation if sent back. + const [originalIcon] = useState(() => draft.icon); const inactiveAssignees = target === "new" ? [] @@ -544,6 +548,7 @@ function EditSheet({ } else { const edit: DefinitionEditBody = { ...body }; if (keepsAssignees) delete edit.assignee_child_ids; + if (draft.icon === originalIcon) delete edit.icon; await updateDefinition(target.id, edit); } onSaved(); -- 2.55.0