From d1160eb65a3569380c2ac1dabebca03bc4372978 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Sun, 30 Aug 2026 11:48:52 +0200 Subject: [PATCH] fix(housekeeping): address task 14 review round 3 --- src/actions/content-legacy-parity.test.ts | 39 +++++++++ src/actions/save-favicon.ts | 23 ++++- src/actions/save-logo.ts | 16 +++- .../content/pages/content-command-form.tsx | 27 +++++- .../content/pages/content-pages.test.tsx | 68 ++++++++++++++- .../domains/content/pages/engagement.tsx | 23 ++++- .../services/mutations-production.test.ts | 87 +++++++++++++------ 7 files changed, 243 insertions(+), 40 deletions(-) diff --git a/src/actions/content-legacy-parity.test.ts b/src/actions/content-legacy-parity.test.ts index 43421653..01ea1f6a 100644 --- a/src/actions/content-legacy-parity.test.ts +++ b/src/actions/content-legacy-parity.test.ts @@ -265,6 +265,45 @@ describe("Content legacy wrappers", () => { expect(auditedBrandExecute).not.toHaveBeenCalled(); }); + it("maps a favicon audit partial to the truthful legacy result shape", async () => { + auditedBrandExecute.mockResolvedValueOnce({ + before: { value: "/old.ico" }, + after: { value: "/api/media/favicon/new.ico" }, + output: { url: "/api/media/favicon/new.ico" }, + completion: { + status: "partial", + external: "completed", + audit: "unavailable", + }, + }); + const file = new File(["bytes"], "favicon.png", { type: "image/png" }); + await expect(saveFavicon(form({ file }) as FormData)).resolves.toEqual({ + success: false, + url: "/api/media/favicon/new.ico", + error: + "Favicon change completed partially; verify storage and audit state", + }); + }); + + it("maps a logo audit partial to the truthful legacy result shape", async () => { + auditedBrandExecute.mockResolvedValueOnce({ + before: { value: "/old.png" }, + after: { value: "/api/media/logo/new.png" }, + output: { url: "/api/media/logo/new.png" }, + completion: { + status: "partial", + external: "completed", + audit: "unavailable", + }, + }); + const file = new File(["bytes"], "logo.png", { type: "image/png" }); + await expect(saveLogo(form({ file }) as FormData)).resolves.toEqual({ + success: false, + url: "/api/media/logo/new.png", + error: "Logo change completed partially; verify storage and audit state", + }); + }); + it("keeps every listed legacy action as a thin shared-service wrapper", () => { for (const path of [ "src/actions/admin-ads.ts", diff --git a/src/actions/save-favicon.ts b/src/actions/save-favicon.ts index a1b25ed1..63178019 100644 --- a/src/actions/save-favicon.ts +++ b/src/actions/save-favicon.ts @@ -15,6 +15,8 @@ const ALLOWED = [ "image/x-icon", "image/svg+xml", ]; +const PARTIAL_ERROR = + "Favicon change completed partially; verify storage and audit state"; async function executeAuditedFaviconMutation( staff: StaffUser, @@ -59,11 +61,18 @@ export async function saveFavicon( file, }); siteRevalidate(); + const url = + typeof result.output?.url === "string" ? result.output.url : undefined; + if (result.completion?.status === "partial") { + return { + success: false, + ...(url ? { url } : {}), + error: PARTIAL_ERROR, + }; + } return { success: true, - ...(typeof result.output?.url === "string" - ? { url: result.output.url } - : {}), + ...(url ? { url } : {}), }; } catch (error) { return { @@ -79,8 +88,14 @@ export async function deleteFavicon(): Promise<{ }> { const staff = await requirePermission(PERMS.SETTINGS_VIEW); try { - await executeAuditedFaviconMutation(staff, "favicon.delete", {}); + const result = await executeAuditedFaviconMutation( + staff, + "favicon.delete", + {}, + ); siteRevalidate(); + if (result.completion?.status === "partial") + return { success: false, error: PARTIAL_ERROR }; return { success: true }; } catch (error) { return { diff --git a/src/actions/save-logo.ts b/src/actions/save-logo.ts index 04479c9a..e127dd87 100644 --- a/src/actions/save-logo.ts +++ b/src/actions/save-logo.ts @@ -5,6 +5,9 @@ import type { ContentMutationSnapshot } from "@/features/housekeeping/domains/co import { createCorrelationId } from "@/features/housekeeping/foundation/contracts"; import { requireStaff, type StaffUser } from "@/lib/admin/guard"; +const PARTIAL_ERROR = + "Logo change completed partially; verify storage and audit state"; + async function executeAuditedLogoMutation( staff: StaffUser, input: unknown, @@ -37,11 +40,18 @@ export async function saveLogo( if (!file) return { success: false, error: "No file provided" }; const result = await executeAuditedLogoMutation(staff, { file }); revalidatePath("/", "layout"); + const url = + typeof result.output?.url === "string" ? result.output.url : undefined; + if (result.completion?.status === "partial") { + return { + success: false, + ...(url ? { url } : {}), + error: PARTIAL_ERROR, + }; + } return { success: true, - ...(typeof result.output?.url === "string" - ? { url: result.output.url } - : {}), + ...(url ? { url } : {}), }; } catch (error) { return { diff --git a/src/features/housekeeping/domains/content/pages/content-command-form.tsx b/src/features/housekeeping/domains/content/pages/content-command-form.tsx index 8d587784..e511efaa 100644 --- a/src/features/housekeeping/domains/content/pages/content-command-form.tsx +++ b/src/features/housekeeping/domains/content/pages/content-command-form.tsx @@ -20,6 +20,7 @@ export interface ContentCommandField { readonly max?: number; readonly maxLength?: number; readonly defaultValue?: string | number | boolean; + readonly allowUnchanged?: boolean; readonly options?: readonly Readonly<{ value: string | number; label: string; @@ -41,7 +42,15 @@ const OMIT_FIELD = Symbol("omit optional Content command field"); function parseField(field: ContentCommandField, formData: FormData): unknown { const rawValue = formData.get(field.name); - if (field.type === "checkbox") return rawValue === "on"; + if (field.type === "checkbox") { + if (field.allowUnchanged) { + if (rawValue === null || rawValue === "") return OMIT_FIELD; + if (rawValue === "true") return true; + if (rawValue === "false") return false; + return OMIT_FIELD; + } + return rawValue === "on"; + } if (rawValue === null && !field.required) return OMIT_FIELD; if (field.type === "file") return rawValue instanceof File ? rawValue : null; const raw = String(rawValue ?? "") @@ -130,7 +139,21 @@ export function ContentCommandForm({ htmlFor={`${commandId}-${field.name}`} className="block text-sm" > - {field.type === "checkbox" ? ( + {field.type === "checkbox" && field.allowUnchanged ? ( + <> + {field.label} + + + ) : field.type === "checkbox" ? ( <> { }); }); - it("omits untouched optional text but submits a declared unchecked checkbox as false", async () => { + it("omits untouched optional text and tri-state checkbox fields during updates", async () => { vi.mocked(executeHousekeepingCommand).mockResolvedValue( ok({ before: null, after: { id: "7" } }, "partial-form"), ); @@ -168,7 +168,12 @@ describe("Content actionable form wiring", () => { { name: "id", label: "ID", type: "identifier", required: true }, { name: "title", label: "Title", type: "text" }, { name: "image", label: "Image", type: "text" }, - { name: "isActive", label: "Active", type: "checkbox" }, + { + name: "isActive", + label: "Active", + type: "checkbox", + allowUnchanged: true, + }, ], }, null, @@ -180,12 +185,47 @@ describe("Content actionable form wiring", () => { input: { action: "update", id: "7", - isActive: false, title: "Renamed", }, }); }); + it.each([ + ["false", false], + ["true", true], + ] as const)( + "submits an explicit tri-state checkbox value %s as %s", + async (submitted, expected) => { + vi.mocked(executeHousekeepingCommand).mockResolvedValue( + ok({ before: null, after: { id: "7" } }, "explicit-checkbox"), + ); + const formData = new FormData(); + formData.set("id", "7"); + formData.set("isActive", submitted); + await submitContentCommandForm( + { + commandId: "content.engagement.event-type.change", + input: { action: "update" }, + fields: [ + { name: "id", label: "ID", type: "identifier", required: true }, + { + name: "isActive", + label: "Active", + type: "checkbox", + allowUnchanged: true, + }, + ], + }, + null, + formData, + ); + expect(executeHousekeepingCommand).toHaveBeenCalledWith({ + commandId: "content.engagement.event-type.change", + input: { action: "update", id: "7", isActive: expected }, + }); + }, + ); + it("renders poll creation with show-results checked by default", () => { const html = renderToStaticMarkup( { />, ); expect(html).toMatch(/name="showResults"[^>]*checked=""/u); + const multipleChoice = html.match( + /]*name="multipleChoice"[^>]*>/u, + )?.[0]; + expect(multipleChoice).toBeDefined(); + expect(multipleChoice).not.toContain("checked"); + }); + + it("renders poll updates with explicit unchanged, enabled, and disabled choices", () => { + const html = renderToStaticMarkup( + , + ); + expect(html).toMatch(/]*name="showResults"/u); + expect(html).toContain("No change"); + expect(html).toContain('value="true"'); + expect(html).toContain('value="false"'); }); it("renders mutation forms only with the exact capability", () => { diff --git a/src/features/housekeeping/domains/content/pages/engagement.tsx b/src/features/housekeeping/domains/content/pages/engagement.tsx index b988b223..3c68ce69 100644 --- a/src/features/housekeeping/domains/content/pages/engagement.tsx +++ b/src/features/housekeeping/domains/content/pages/engagement.tsx @@ -58,7 +58,12 @@ export function ContentEngagementPage({ }, { name: "endsAt", label: "Ends at", type: "text", maxLength: 50 }, { name: "maxPlayers", label: "Maximum players", type: "number", min: 1 }, - { name: "isRecurring", label: "Recurring", type: "checkbox" }, + { + name: "isRecurring", + label: "Recurring", + type: "checkbox", + allowUnchanged: !creatingEvent, + }, { name: "recurrenceRule", label: "Recurrence rule", @@ -113,11 +118,13 @@ export function ContentEngagementPage({ label: "Show results", type: "checkbox", defaultValue: creatingPoll, + allowUnchanged: !creatingPoll, }, { name: "multipleChoice", label: "Allow multiple choices", type: "checkbox", + allowUnchanged: !creatingPoll, }, { name: "startsAt", label: "Starts at", type: "text", maxLength: 50 }, { name: "endsAt", label: "Ends at", type: "text", maxLength: 50 }, @@ -151,7 +158,12 @@ export function ContentEngagementPage({ }, { name: "color", label: "Color", type: "text", maxLength: 20 }, { name: "icon", label: "Icon", type: "text", maxLength: 50 }, - { name: "isActive", label: "Active", type: "checkbox" }, + { + name: "isActive", + label: "Active", + type: "checkbox", + allowUnchanged: true, + }, { name: "minRank", label: "Minimum rank", @@ -354,7 +366,12 @@ export function ContentEngagementPage({ type: "text", maxLength: 255, }, - { name: "active", label: "Active", type: "checkbox" }, + { + name: "active", + label: "Active", + type: "checkbox", + allowUnchanged: true, + }, ]} /> { expect(JSON.stringify(deps.writeAudit.mock.calls)).not.toContain("Hello"); }); - it("routes a logo write through correlated intent and outcome audit", async () => { - const deps = dependencies(); - await deps.adapter.execute( - "logo.save", - { file: { name: "logo.png" } }, - mutationContext, - ); - expect(deps.executeOperation).toHaveBeenCalledWith( - "logo.save", - expect.anything(), - mutationContext, - ); - expect(deps.writeAudit.mock.calls.map(([entry]) => entry)).toEqual([ - expect.objectContaining({ - action: "content.logo.save", - outcome: "intent", - correlationId: "production-matrix", - }), - expect.objectContaining({ - action: "content.logo.save", - outcome: "success", - correlationId: "production-matrix", - }), - ]); - }); + it.each(["favicon.save", "logo.save"] as const)( + "routes %s through correlated intent and outcome audit", + async (operation) => { + const deps = dependencies(); + await deps.adapter.execute( + operation, + { file: { name: "brand.png" } }, + mutationContext, + ); + expect(deps.executeOperation).toHaveBeenCalledWith( + operation, + expect.anything(), + mutationContext, + ); + expect(deps.writeAudit.mock.calls.map(([entry]) => entry)).toEqual([ + expect.objectContaining({ + action: `content.${operation}`, + outcome: "intent", + correlationId: "production-matrix", + }), + expect.objectContaining({ + action: `content.${operation}`, + outcome: "success", + correlationId: "production-matrix", + }), + ]); + }, + ); + + it.each(["favicon.save", "logo.save"] as const)( + "returns a typed partial for %s when its outcome audit is unavailable", + async (operation) => { + const deps = dependencies(); + deps.writeAudit + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("audit offline")); + const snapshot = await deps.adapter.execute( + operation, + { file: { name: "brand.png" } }, + mutationContext, + ); + expect(snapshot.completion).toEqual({ + status: "partial", + external: "completed", + audit: "unavailable", + }); + expect(deps.writeAudit.mock.calls).toEqual([ + [ + expect.objectContaining({ + action: `content.${operation}`, + outcome: "intent", + }), + ], + [ + expect.objectContaining({ + action: `content.${operation}`, + outcome: "success", + }), + ], + ]); + }, + ); it("returns typed partial when database committed but the external effect failed", async () => { const deps = dependencies();