Feature review remediation: editing a legacy unknown icon is rejected #250

2 changed files with 118 additions and 6 deletions
+113 -6
View File
@@ -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 {
+5
View File
@@ -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:<old>` 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();