From e0efbef30d79d26a99ca7eae21abdfe8e37fcf23 Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 30 Sep 2026 19:17:20 +0200 Subject: [PATCH] fix(catalog): make the Builder Club catalog read and write its own offers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit taught bulk editing and delete-with-restore about the BC catalog. Neither actually worked, and one of them was destructive. `catalog_items_bc` has six columns: id, item_ids, page_id, catalog_name, order_number, extradata. There is no price, points, currency, offer_id, limit or membership column on it. The bulk path read and wrote columns that do not exist, and the UPDATE was aimed at catalog_items while the SELECT came from catalog_items_bc — so a BC category move wrote into the normal catalog. Two tests now pin that pairing: reads and writes have to stay in the same table. Underneath it the BC table was never being read at all. The inline editor fetched `/api/admin/catalog/items?pageId=N` without the catalog, so opening a BC category showed the normal catalog's offers, and the route selected BC rows directly instead of going through the loader, skipping the furni enrichment the table needs to render anything but a bare caption. Both catalogs now take the same path, and the catalog is in the fetch callback's dependencies — without that, a switch keeps reading the previous catalog's rows through a stale closure. Because a BC offer has no price, the editor no longer offers one. The server refuses price, points and currency changes with a readable message instead of letting them reach the database as an unknown-column error, and a BC bulk edit is what it can actually be: a category move. BC deletions also went through a bare DELETE, which made them the one catalog mutation with no way back. They now keep their rows and hand back a restoreId like the normal ones. The catalog is recorded in the audit target rather than in the payload, so a restore can never put a BC row into the normal offers table. --- src/actions/catalog-bc.test.ts | 41 ++- src/actions/catalog-bc.ts | 45 ++- src/actions/catalog-items.ts | 14 +- src/app/api/admin/catalog/items/route.ts | 30 +- .../admin/catalog-manager/inline-editor.tsx | 12 +- .../catalog-items-table.tsx | 20 +- .../catalog/components/bulk-offer-editor.tsx | 261 ++++++++++-------- .../catalog/server/bulk-offers.test.ts | 45 +++ src/features/catalog/server/bulk-offers.ts | 41 ++- .../catalog/server/item-deletes.test.ts | 125 ++++++++- src/features/catalog/server/item-deletes.ts | 112 ++++++-- src/lib/services/catalog-items-loader.ts | 20 +- 12 files changed, 548 insertions(+), 218 deletions(-) diff --git a/src/actions/catalog-bc.test.ts b/src/actions/catalog-bc.test.ts index 5fcd8497..c5d3fe5f 100644 --- a/src/actions/catalog-bc.test.ts +++ b/src/actions/catalog-bc.test.ts @@ -1,5 +1,4 @@ // @ts-nocheck -import { eq } from "drizzle-orm"; import { beforeEach, describe, expect, it, vi } from "vitest"; const state = vi.hoisted(() => ({ @@ -50,6 +49,13 @@ vi.mock("@/features/catalog/server/page-commands", () => ({ togglePageCommand: mockTogglePageCommand, })); +const mockDeleteCatalogItemsCommand = vi.hoisted(() => + vi.fn(async () => ({ deleted: 1, restoreId: 55 })), +); +vi.mock("@/features/catalog/server/item-deletes", () => ({ + deleteCatalogItemsCommand: mockDeleteCatalogItemsCommand, +})); + const mockSendCatalogUpdate = vi.hoisted(() => vi.fn()); vi.mock("@/features/catalog/server/sync-status", () => ({ sendCatalogUpdate: mockSendCatalogUpdate, @@ -86,7 +92,6 @@ vi.mock("@/lib/db", async () => { }; }); -import { CatalogItemsBc } from "@/lib/db"; import { createBcItem, createBcPage, @@ -179,16 +184,25 @@ describe("updateBcPage", () => { }); describe("deleteBcItem", () => { - it("deletes the bc item and logs activity", async () => { + it("deletes through the restorable path and logs activity", async () => { const result = await deleteBcItem({ id: 7 }); - expect(result).toEqual({ ok: true }); - expect(state.deletes).toHaveLength(1); - expect(state.deletes[0].table).toBe(CatalogItemsBc); - expect(state.deletes[0].where).toEqual(eq(CatalogItemsBc.id, 7)); + expect(result).toEqual({ + ok: true, + data: { deleted: 1, restoreId: 55 }, + }); + // Not a raw DELETE: BC rows are kept so the delete can be undone, the same + // as the normal catalog. + expect(mockDeleteCatalogItemsCommand).toHaveBeenCalledWith( + [7], + expect.anything(), + undefined, + "bc", + ); + expect(state.deletes).toHaveLength(0); expect(mockLogStaffActivity).toHaveBeenCalledWith( expect.objectContaining({ action: "bc_item_delete", - description: "Deleted BC catalog item #7", + description: "Deleted BC catalog offer #7", }), ); expect(mockSendCatalogUpdate).toHaveBeenCalled(); @@ -197,9 +211,14 @@ describe("deleteBcItem", () => { ); }); - it("propagates delete failures", async () => { - state.deleteError = new Error("db down"); - await expect(deleteBcItem({ id: 7 })).rejects.toThrow("db down"); + it("reports a failed delete instead of pretending it worked", async () => { + mockDeleteCatalogItemsCommand.mockRejectedValueOnce(Error("db down")); + mockCatalogFailure.mockReturnValueOnce({ message: "Delete failed" }); + + const result = await deleteBcItem({ id: 7 }); + + expect(result).toEqual({ ok: false, error: "Delete failed" }); + expect(mockSendCatalogUpdate).not.toHaveBeenCalled(); }); }); diff --git a/src/actions/catalog-bc.ts b/src/actions/catalog-bc.ts index 85db33c6..d8e3d8b3 100644 --- a/src/actions/catalog-bc.ts +++ b/src/actions/catalog-bc.ts @@ -1,8 +1,8 @@ "use server"; -import { eq } from "drizzle-orm"; import { revalidatePath } from "next/cache"; import { catalogFailure } from "@/features/catalog/server/errors"; +import { deleteCatalogItemsCommand } from "@/features/catalog/server/item-deletes"; import { createBcOfferCommand, updateBcOfferCommand, @@ -16,7 +16,6 @@ import { } from "@/features/catalog/server/page-commands"; import { sendCatalogUpdate } from "@/features/catalog/server/sync-status"; import { requirePermission } from "@/lib/admin/guard"; -import { CatalogItemsBc, db } from "@/lib/db"; import { PERMS } from "@/lib/permissions"; import { withCatalogExport } from "@/lib/services/catalog-git-queue"; import { logStaffActivity } from "@/lib/services/staff-activity"; @@ -92,21 +91,37 @@ export async function updateBcPage({ }); } -export async function deleteBcItem({ id }: { id: number }) { +/** + * BC offers are deleted through the same keep-and-restore path as normal ones. + * It used to be a bare `DELETE` here, so a BC deletion was the one catalog + * mutation with no way back. + */ +export async function deleteBcItem({ + id, + requestKey, +}: { + id: number; + requestKey?: string; +}) { const staff = await requirePermission(PERMS.CATALOG_EDIT); - return await withCatalogExport(async () => { - await db.delete(CatalogItemsBc).where(eq(CatalogItemsBc.id, id)); - await sendCatalogUpdate(); - await logStaffActivity({ - staffId: staff.id, - action: "bc_item_delete", - description: `Deleted BC catalog item #${id}`, - targetType: "catalog_item_bc", - targetId: id, + try { + return await withCatalogExport(async () => { + const data: { deleted: number; restoreId: number } = + await deleteCatalogItemsCommand([id], staff.id, requestKey, "bc"); + await sendCatalogUpdate(); + await logStaffActivity({ + staffId: staff.id, + action: "bc_item_delete", + description: `Deleted BC catalog offer #${id}`, + targetType: "catalog_item_bc", + targetId: id, + }); + revalidatePath("/admin/catalog/builder-club"); + return { ok: true as const, data }; }); - revalidatePath("/admin/catalog/builder-club"); - return { ok: true as const }; - }); + } catch (error) { + return { ok: false as const, error: catalogFailure(error).message }; + } } export async function updateBcItem({ diff --git a/src/actions/catalog-items.ts b/src/actions/catalog-items.ts index 07b4d560..0a370c08 100644 --- a/src/actions/catalog-items.ts +++ b/src/actions/catalog-items.ts @@ -210,9 +210,11 @@ export async function bulkCreateCatalogItems({ export async function deleteCatalogItems({ ids, requestKey, + catalog = "normal", }: { ids: number[]; requestKey?: string; + catalog?: "normal" | "bc"; }) { const staff = await requirePermission(PERMS.CATALOG_EDIT); try { @@ -220,15 +222,21 @@ export async function deleteCatalogItems({ const data: { deleted: number; restoreId: number; - } = await deleteCatalogItemsCommand(ids, staff.id, requestKey); + } = await deleteCatalogItemsCommand( + ids, + staff.id, + requestKey, + catalog === "bc" ? "bc" : "normal", + ); await sendCatalogUpdate(); await logStaffActivity({ staffId: staff.id, action: "catalog_items_delete", - description: `Deleted ${data.deleted} catalog offer(s): ${ids.join(", ")}`, - targetType: "catalog_item", + description: `Deleted ${data.deleted} ${catalog === "bc" ? "BC " : ""}catalog offer(s): ${ids.join(", ")}`, + targetType: catalog === "bc" ? "catalog_item_bc" : "catalog_item", }); revalidatePath("/admin/catalog"); + if (catalog === "bc") revalidatePath("/admin/catalog/builder-club"); return { ok: true as const, data }; }); } catch (error) { diff --git a/src/app/api/admin/catalog/items/route.ts b/src/app/api/admin/catalog/items/route.ts index 3259b184..ef86636b 100644 --- a/src/app/api/admin/catalog/items/route.ts +++ b/src/app/api/admin/catalog/items/route.ts @@ -1,16 +1,8 @@ -import { asc, eq } from "drizzle-orm"; import { apiError, apiOk } from "@/lib/api"; import { withAdmin } from "@/lib/api-handler"; -import { CatalogItemsBc, db } from "@/lib/db"; import { PERMS } from "@/lib/permissions"; import { loadCatalogItemsData } from "@/lib/services/catalog-items-loader"; -function jsonSafe(data: T): T { - return JSON.parse( - JSON.stringify(data, (_k, v) => (typeof v === "bigint" ? Number(v) : v)), - ) as T; -} - export const GET = withAdmin( { permission: PERMS.CATALOG_VIEW }, async (request) => { @@ -18,16 +10,16 @@ export const GET = withAdmin( if (!Number.isFinite(pageId) || pageId <= 0) { return apiError("Invalid pageId"); } - const isBc = request.nextUrl.searchParams.get("catalog") === "bc"; - if (isBc) { - const items = await db - .select() - .from(CatalogItemsBc) - .where(eq(CatalogItemsBc.pageId, pageId)) - .orderBy(asc(CatalogItemsBc.orderNumber)); - return apiOk({ items: jsonSafe(items) }); - } - const data = await loadCatalogItemsData(pageId); - return apiOk(jsonSafe(data) as unknown as Record); + const catalog = + request.nextUrl.searchParams.get("catalog") === "bc" ? "bc" : "normal"; + // Both catalogs go through the loader. The BC branch used to select the + // rows directly, which returned them without the furni enrichment the + // table needs to render anything but a bare caption. + return apiOk( + (await loadCatalogItemsData(pageId, catalog)) as unknown as Record< + string, + unknown + >, + ); }, ); diff --git a/src/components/admin/catalog-manager/inline-editor.tsx b/src/components/admin/catalog-manager/inline-editor.tsx index 7cee005b..07c44036 100644 --- a/src/components/admin/catalog-manager/inline-editor.tsx +++ b/src/components/admin/catalog-manager/inline-editor.tsx @@ -233,9 +233,10 @@ function InlineEditorSession({ setItemsError(false); setItemsLoading(true); try { - const res = await fetch(`/api/admin/catalog/items?pageId=${id}`, { - signal: request.signal, - }); + const res = await fetch( + `/api/admin/catalog/items?pageId=${id}${catQs}`, + { signal: request.signal }, + ); if (!res.ok) throw new Error(); const data = await res.json(); if (!request.isCurrent()) return; @@ -250,7 +251,10 @@ function InlineEditorSession({ if (request.isCurrent()) setItemsLoading(false); } }, - [itemRequests], + // `catQs` selects which catalog the offers come from, so it has to be part + // of this callback's identity: without it a switch keeps reading the + // previous catalog's rows. + [itemRequests, catQs], ); const reloadItems = useCallback(() => { diff --git a/src/components/admin/catalog/catalog-items-table/catalog-items-table.tsx b/src/components/admin/catalog/catalog-items-table/catalog-items-table.tsx index 569477e8..0b493b6c 100644 --- a/src/components/admin/catalog/catalog-items-table/catalog-items-table.tsx +++ b/src/components/admin/catalog/catalog-items-table/catalog-items-table.tsx @@ -564,15 +564,19 @@ export function CatalogItemsTable({ if (!ok) return; const ids = [...selected]; - run(() => deleteCatalogItems({ ids, requestKey: crypto.randomUUID() }), { - successMessage: `Deleted ${ids.length} item(s).`, - errorMessage: "Failed to delete items.", - onSuccess: (data) => { - setSelected(new Set()); - onRefresh(); - offerUndoDelete(Number(data.restoreId), ids.length); + run( + () => + deleteCatalogItems({ ids, requestKey: crypto.randomUUID(), catalog }), + { + successMessage: `Deleted ${ids.length} item(s).`, + errorMessage: "Failed to delete items.", + onSuccess: (data) => { + setSelected(new Set()); + onRefresh(); + offerUndoDelete(Number(data.restoreId), ids.length); + }, }, - }); + ); } // ── Move selected items to another page ───────────────────────── diff --git a/src/features/catalog/components/bulk-offer-editor.tsx b/src/features/catalog/components/bulk-offer-editor.tsx index 00db4a15..10e72749 100644 --- a/src/features/catalog/components/bulk-offer-editor.tsx +++ b/src/features/catalog/components/bulk-offer-editor.tsx @@ -288,104 +288,120 @@ export function BulkOfferEditor({ {!preview ? (

{t("fieldHint")}

- {(["costCredits", "costPoints"] as const).map((field) => ( -
- - {prices[field].enabled && ( -
-
- - -
-
- - - updatePrice(field, { value: event.target.value }) + {/* A BC offer has no price, points or currency column to + write. The server refuses those changes too; hiding the + fields keeps the dialog from offering what cannot be done. */} + {catalog !== "bc" && ( + <> + {(["costCredits", "costPoints"] as const).map((field) => ( +
+
-
- )} -
- ))} -
- - {currencyEnabled && ( - + updatePrice(field, { mode: value as PriceMode }) + } + disabled={busy} + > + + + + + {(["set", "add", "percent"] as const).map( + (mode) => ( + + {t(`modes.${mode}`)} + + ), + )} + + +
+
+ + + updatePrice(field, { + value: event.target.value, + }) + } + disabled={busy} /> - {currencyName(Number(type))} - - - ))} - - - )} -
+
+
+ )} + + ))} +
+ + {currencyEnabled && ( + + )} +
+ + )}