fix(housekeeping): address task 14 review round 3

This commit is contained in:
Simo committed 2026-08-30 11:48:52 +02:00
1 parent c524b305d7
commit d1160eb65a
7 files changed
+243 -40

No files matched your search

+39
View File
@@ -265,6 +265,45 @@ describe("Content legacy wrappers", () => {
expect(auditedBrandExecute).not.toHaveBeenCalled(); 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", () => { it("keeps every listed legacy action as a thin shared-service wrapper", () => {
for (const path of [ for (const path of [
"src/actions/admin-ads.ts", "src/actions/admin-ads.ts",
+19 -4
View File
@@ -15,6 +15,8 @@ const ALLOWED = [
"image/x-icon", "image/x-icon",
"image/svg+xml", "image/svg+xml",
]; ];
const PARTIAL_ERROR =
"Favicon change completed partially; verify storage and audit state";
async function executeAuditedFaviconMutation( async function executeAuditedFaviconMutation(
staff: StaffUser, staff: StaffUser,
@@ -59,11 +61,18 @@ export async function saveFavicon(
file, file,
}); });
siteRevalidate(); 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 { return {
success: true, success: true,
...(typeof result.output?.url === "string" ...(url ? { url } : {}),
? { url: result.output.url }
: {}),
}; };
} catch (error) { } catch (error) {
return { return {
@@ -79,8 +88,14 @@ export async function deleteFavicon(): Promise<{
}> { }> {
const staff = await requirePermission(PERMS.SETTINGS_VIEW); const staff = await requirePermission(PERMS.SETTINGS_VIEW);
try { try {
await executeAuditedFaviconMutation(staff, "favicon.delete", {}); const result = await executeAuditedFaviconMutation(
staff,
"favicon.delete",
{},
);
siteRevalidate(); siteRevalidate();
if (result.completion?.status === "partial")
return { success: false, error: PARTIAL_ERROR };
return { success: true }; return { success: true };
} catch (error) { } catch (error) {
return { return {
+13 -3
View File
@@ -5,6 +5,9 @@ import type { ContentMutationSnapshot } from "@/features/housekeeping/domains/co
import { createCorrelationId } from "@/features/housekeeping/foundation/contracts"; import { createCorrelationId } from "@/features/housekeeping/foundation/contracts";
import { requireStaff, type StaffUser } from "@/lib/admin/guard"; import { requireStaff, type StaffUser } from "@/lib/admin/guard";
const PARTIAL_ERROR =
"Logo change completed partially; verify storage and audit state";
async function executeAuditedLogoMutation( async function executeAuditedLogoMutation(
staff: StaffUser, staff: StaffUser,
input: unknown, input: unknown,
@@ -37,11 +40,18 @@ export async function saveLogo(
if (!file) return { success: false, error: "No file provided" }; if (!file) return { success: false, error: "No file provided" };
const result = await executeAuditedLogoMutation(staff, { file }); const result = await executeAuditedLogoMutation(staff, { file });
revalidatePath("/", "layout"); 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 { return {
success: true, success: true,
...(typeof result.output?.url === "string" ...(url ? { url } : {}),
? { url: result.output.url }
: {}),
}; };
} catch (error) { } catch (error) {
return { return {
@@ -20,6 +20,7 @@ export interface ContentCommandField {
readonly max?: number; readonly max?: number;
readonly maxLength?: number; readonly maxLength?: number;
readonly defaultValue?: string | number | boolean; readonly defaultValue?: string | number | boolean;
readonly allowUnchanged?: boolean;
readonly options?: readonly Readonly<{ readonly options?: readonly Readonly<{
value: string | number; value: string | number;
label: string; label: string;
@@ -41,7 +42,15 @@ const OMIT_FIELD = Symbol("omit optional Content command field");
function parseField(field: ContentCommandField, formData: FormData): unknown { function parseField(field: ContentCommandField, formData: FormData): unknown {
const rawValue = formData.get(field.name); 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 (rawValue === null && !field.required) return OMIT_FIELD;
if (field.type === "file") return rawValue instanceof File ? rawValue : null; if (field.type === "file") return rawValue instanceof File ? rawValue : null;
const raw = String(rawValue ?? "") const raw = String(rawValue ?? "")
@@ -130,7 +139,21 @@ export function ContentCommandForm({
htmlFor={`${commandId}-${field.name}`} htmlFor={`${commandId}-${field.name}`}
className="block text-sm" className="block text-sm"
> >
{field.type === "checkbox" ? ( {field.type === "checkbox" && field.allowUnchanged ? (
<>
{field.label}
<select
id={`${commandId}-${field.name}`}
name={field.name}
defaultValue=""
className="mt-1 block w-full"
>
<option value="">No change</option>
<option value="true">Enabled</option>
<option value="false">Disabled</option>
</select>
</>
) : field.type === "checkbox" ? (
<> <>
<input <input
id={`${commandId}-${field.name}`} id={`${commandId}-${field.name}`}
@@ -151,7 +151,7 @@ describe("Content actionable form wiring", () => {
}); });
}); });
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( vi.mocked(executeHousekeepingCommand).mockResolvedValue(
ok({ before: null, after: { id: "7" } }, "partial-form"), 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: "id", label: "ID", type: "identifier", required: true },
{ name: "title", label: "Title", type: "text" }, { name: "title", label: "Title", type: "text" },
{ name: "image", label: "Image", type: "text" }, { name: "image", label: "Image", type: "text" },
{ name: "isActive", label: "Active", type: "checkbox" }, {
name: "isActive",
label: "Active",
type: "checkbox",
allowUnchanged: true,
},
], ],
}, },
null, null,
@@ -180,12 +185,47 @@ describe("Content actionable form wiring", () => {
input: { input: {
action: "update", action: "update",
id: "7", id: "7",
isActive: false,
title: "Renamed", 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", () => { it("renders poll creation with show-results checked by default", () => {
const html = renderToStaticMarkup( const html = renderToStaticMarkup(
<ContentEngagementPage <ContentEngagementPage
@@ -198,6 +238,28 @@ describe("Content actionable form wiring", () => {
/>, />,
); );
expect(html).toMatch(/name="showResults"[^>]*checked=""/u); expect(html).toMatch(/name="showResults"[^>]*checked=""/u);
const multipleChoice = html.match(
/<input[^>]*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(
<ContentEngagementPage
context={context([PERMS.POLLS_EDIT])}
result={ok(
{ kind: "engagement", items: [], total: 0, partialDependencies: [] },
"poll-update-checkbox",
)}
routeId="content.engagement.poll-detail"
/>,
);
expect(html).toMatch(/<select[^>]*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", () => { it("renders mutation forms only with the exact capability", () => {
@@ -58,7 +58,12 @@ export function ContentEngagementPage({
}, },
{ name: "endsAt", label: "Ends at", type: "text", maxLength: 50 }, { name: "endsAt", label: "Ends at", type: "text", maxLength: 50 },
{ name: "maxPlayers", label: "Maximum players", type: "number", min: 1 }, { 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", name: "recurrenceRule",
label: "Recurrence rule", label: "Recurrence rule",
@@ -113,11 +118,13 @@ export function ContentEngagementPage({
label: "Show results", label: "Show results",
type: "checkbox", type: "checkbox",
defaultValue: creatingPoll, defaultValue: creatingPoll,
allowUnchanged: !creatingPoll,
}, },
{ {
name: "multipleChoice", name: "multipleChoice",
label: "Allow multiple choices", label: "Allow multiple choices",
type: "checkbox", type: "checkbox",
allowUnchanged: !creatingPoll,
}, },
{ name: "startsAt", label: "Starts at", type: "text", maxLength: 50 }, { name: "startsAt", label: "Starts at", type: "text", maxLength: 50 },
{ name: "endsAt", label: "Ends 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: "color", label: "Color", type: "text", maxLength: 20 },
{ name: "icon", label: "Icon", type: "text", maxLength: 50 }, { name: "icon", label: "Icon", type: "text", maxLength: 50 },
{ name: "isActive", label: "Active", type: "checkbox" }, {
name: "isActive",
label: "Active",
type: "checkbox",
allowUnchanged: true,
},
{ {
name: "minRank", name: "minRank",
label: "Minimum rank", label: "Minimum rank",
@@ -354,7 +366,12 @@ export function ContentEngagementPage({
type: "text", type: "text",
maxLength: 255, maxLength: 255,
}, },
{ name: "active", label: "Active", type: "checkbox" }, {
name: "active",
label: "Active",
type: "checkbox",
allowUnchanged: true,
},
]} ]}
/> />
<ContentCommandForm <ContentCommandForm
@@ -122,31 +122,68 @@ describe("Content production mutation adapter", () => {
expect(JSON.stringify(deps.writeAudit.mock.calls)).not.toContain("Hello"); expect(JSON.stringify(deps.writeAudit.mock.calls)).not.toContain("Hello");
}); });
it("routes a logo write through correlated intent and outcome audit", async () => { it.each(["favicon.save", "logo.save"] as const)(
const deps = dependencies(); "routes %s through correlated intent and outcome audit",
await deps.adapter.execute( async (operation) => {
"logo.save", const deps = dependencies();
{ file: { name: "logo.png" } }, await deps.adapter.execute(
mutationContext, operation,
); { file: { name: "brand.png" } },
expect(deps.executeOperation).toHaveBeenCalledWith( mutationContext,
"logo.save", );
expect.anything(), expect(deps.executeOperation).toHaveBeenCalledWith(
mutationContext, operation,
); expect.anything(),
expect(deps.writeAudit.mock.calls.map(([entry]) => entry)).toEqual([ mutationContext,
expect.objectContaining({ );
action: "content.logo.save", expect(deps.writeAudit.mock.calls.map(([entry]) => entry)).toEqual([
outcome: "intent", expect.objectContaining({
correlationId: "production-matrix", action: `content.${operation}`,
}), outcome: "intent",
expect.objectContaining({ correlationId: "production-matrix",
action: "content.logo.save", }),
outcome: "success", expect.objectContaining({
correlationId: "production-matrix", 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 () => { it("returns typed partial when database committed but the external effect failed", async () => {
const deps = dependencies(); const deps = dependencies();