From d43e2d5871757280eae30055ba424f1df72bd01d Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 24 Aug 2026 14:15:27 -0500 Subject: [PATCH] refactor(backend): type the admin item queries, and fix the stale status union (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin.ts has no untyped reads left. Typed sites go from 43 to 49. New ItemRecord in itemSelect.ts for the bare `items` row that `RETURNING *` gives back. Deliberately not AdminItemRow: that describes a select which joins the category and adds images and tags as subqueries, so typing a RETURNING * as it would promise three fields the result does not contain. Three shapes for one table, because three different queries return three different things. The typing found a real defect on its first run, which is the case for doing this at all. `ItemStatus` in types.ts was `'available' | 'reserved' | 'sold'`. The database has four values and defaults to 'pending' — items have arrived pending since #90. itemFilters.ts declared its own copy that had all four and was correct. Two declarations of one union with nothing connecting them: one went stale and nothing said so. It was invisible while query rows were `any`. Typing them turned `if (status === 'pending')` in admin.ts into TS2367, "this comparison appears to be unintentional because the types 'ItemStatus' and '\"pending\"' have no overlap" — a compiler telling us the unpublish route's guard could never be true, against a type that was simply wrong. Confirmed against the database rather than by picking the more plausible of the two declarations: `SELECT DISTINCT status FROM items` returns pending, available, reserved and sold. Fixed by removing the duplication rather than by patching both copies. types.ts now holds the only declaration and itemFilters.ts imports it, re-exporting so its existing importers are unaffected. Patching both would have left the next drift free to happen the same way. Verified: tsc clean, unit 254/254, integration 238/238, and backend lint unchanged — the four warnings it reports are identical to those on main with these changes stashed, so none of them are new. Refs #159 --- backend/src/itemFilters.ts | 5 +++-- backend/src/itemSelect.ts | 21 +++++++++++++++++++++ backend/src/routes/admin.ts | 25 ++++++++++++++++++------- backend/src/types.ts | 14 +++++++++++++- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index 1d2df9f..f223f88 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -2,6 +2,9 @@ // filters. Kept apart from the route so the rules can be unit-tested without a // database, and so items.ts stays a thin handler. +import { ItemStatus } from './types'; +export type { ItemStatus }; + export class FilterError extends Error {} export interface ItemFilters { @@ -26,8 +29,6 @@ export interface ItemFilters { favoritesOnly: boolean; } -export type ItemStatus = 'pending' | 'available' | 'reserved' | 'sold'; - // Matched exactly, not case-insensitively: `items.status` only ever holds these // lowercase values, so accepting 'Reserved' would quietly return nothing rather // than reporting that the filter was wrong. diff --git a/backend/src/itemSelect.ts b/backend/src/itemSelect.ts index 253d8e2..bea2280 100644 --- a/backend/src/itemSelect.ts +++ b/backend/src/itemSelect.ts @@ -87,3 +87,24 @@ export interface AdminItemRow extends ItemRowBase { sold_at: Date | null; paypal_order_id: string | null; } + +/** + * A bare `items` row, as `RETURNING *` gives it back. + * + * Distinct from the two select rows above and not interchangeable with them: + * this is the table, so it has no category_name, no images and no tags. Those + * come from the joins and subqueries the selects add, and typing a RETURNING * + * as AdminItemRow would promise three fields that are not in the result. + */ +export interface ItemRecord { + id: number; + name: string; + description: string | null; + price_cents: number; + status: ItemStatus; + reserved_until: Date | null; + sold_at: Date | null; + paypal_order_id: string | null; + created_at: Date; + category_id: number | null; +} diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index d8a8431..df70e65 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -4,7 +4,8 @@ import { promises as fs } from 'fs'; import { randomUUID } from 'crypto'; import { PoolClient } from 'pg'; import { pool } from '../db'; -import { ADMIN_ITEM_SELECT, AdminItemRow } from '../itemSelect'; +import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect'; +import { ItemStatus } from '../types'; import { asyncRoute } from '../asyncRoute'; import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; import { tagColorFor } from '../utils'; @@ -19,6 +20,16 @@ import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecip const router = Router(); +/** The next image slot, from a COALESCE'd MAX so it is never null. */ +interface MaxSortRow { + max_sort: number; +} + +/** Just the status column, read before deciding whether a transition is legal. */ +interface ItemStatusRow { + status: ItemStatus; +} + const UPLOADS_DIR = process.env.UPLOADS_DIR || '/app/uploads'; // Multer writes to disk with no size cap unless one is given, so a single @@ -255,7 +266,7 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons const client = await pool.connect(); try { await client.query('BEGIN'); - const { rows } = await client.query( + const { rows } = await client.query( `INSERT INTO items (name, description, price_cents, category_id) VALUES ($1, $2, $3, $4) RETURNING *`, [name, description, Math.round(parseFloat(price) * 100), categoryId ?? null] ); @@ -310,7 +321,7 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp await setItemTags(client, Number(req.params.id), await resolveTagIds(client, tagNames)); } if (files.length) { - const { rows: existing } = await client.query( + const { rows: existing } = await client.query( `SELECT COALESCE(MAX(sort_order), -1) AS max_sort FROM item_images WHERE item_id = $1`, [req.params.id] ); @@ -356,7 +367,7 @@ router.delete('/items/:id/images/:imageId', asyncRoute(async (req: Request, res: })); router.post('/items/:id/mark-sold', asyncRoute(async (req: Request, res: Response) => { - const { rows } = await pool.query( + const { rows } = await pool.query( `UPDATE items SET status='sold', sold_at=now() WHERE id=$1 RETURNING *`, [req.params.id] ); @@ -372,7 +383,7 @@ router.post('/items/:id/mark-sold', asyncRoute(async (req: Request, res: Respons // duplication, so the admin UI labels that button "Publish" when the item is // pending. This is the reverse, and it is not symmetrical — see the guard. router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Response) => { - const { rows } = await pool.query(`SELECT status FROM items WHERE id = $1`, [req.params.id]); + const { rows } = await pool.query(`SELECT status FROM items WHERE id = $1`, [req.params.id]); if (!rows.length) { return res.status(404).json({ error: 'not found' }); } @@ -393,7 +404,7 @@ router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Respons return res.status(400).json({ error: 'a sold item cannot be unpublished' }); } - const { rows: updated } = await pool.query( + const { rows: updated } = await pool.query( `UPDATE items SET status='pending' WHERE id=$1 RETURNING *`, [req.params.id] ); @@ -401,7 +412,7 @@ router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Respons })); router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Response) => { - const { rows } = await pool.query( + const { rows } = await pool.query( `UPDATE items SET status='available', sold_at=NULL, reserved_until=NULL, paypal_order_id=NULL WHERE id=$1 RETURNING *`, [req.params.id] diff --git a/backend/src/types.ts b/backend/src/types.ts index 545c7c2..cc43625 100755 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -1,4 +1,16 @@ -export type ItemStatus = 'available' | 'reserved' | 'sold'; +/** + * Every value `items.status` can hold. + * + * 'pending' was missing here from the moment items started arriving pending, + * while itemFilters.ts declared its own copy that had it. Two declarations of + * one union is how that happens: nothing connects them, so one goes stale and + * nothing says so. The stale one was harmless only because query rows were + * `any` — typing them turned `status === 'pending'` in admin.ts into a compile + * error about a comparison with no overlap, which is how it was found. + * + * This is now the single declaration. itemFilters.ts imports it. + */ +export type ItemStatus = 'pending' | 'available' | 'reserved' | 'sold'; export interface ItemImage { id: number; -- 2.54.0