refactor: remove the duplicated blocks SonarQube found (#182)
Three of the four candidates were real. The fourth was my mistake in the issue. **The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation. `Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly. **The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string. Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did. **The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once. **`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves. Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation. Closes #182
This commit is contained in:
+67
-29
@@ -266,6 +266,60 @@ async function setItemTags(client: PoolClient, itemId: number, tagIds: number[])
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Records uploaded files as an item's images.
|
||||
*
|
||||
* Create and update wrote this loop separately, differing only in where the id
|
||||
* came from and where the sort order started — zero for a new item, one past
|
||||
* the current maximum for an existing one. Both are parameters now.
|
||||
*
|
||||
* It also means the `/uploads/` prefix is written once. That matters more than
|
||||
* it looks: #103 made the stored value the path `uploadUrl` joins an origin
|
||||
* onto, so it is a contract rather than a string, and two places to change it
|
||||
* is one place to forget.
|
||||
*/
|
||||
async function insertItemImages(
|
||||
client: PoolClient,
|
||||
itemId: number,
|
||||
files: Express.Multer.File[],
|
||||
firstSortOrder: number
|
||||
): Promise<void> {
|
||||
// Iterated by entry rather than by index, so there is no possibly-undefined
|
||||
// element to guard — the create path used to fall back to an empty filename,
|
||||
// which would have stored a path pointing at the uploads directory itself.
|
||||
for (const [offset, file] of files.entries()) {
|
||||
await client.query(
|
||||
`INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`,
|
||||
[itemId, `/uploads/${file.filename}`, firstSortOrder + offset]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The two optional fields the item form submits as multipart text.
|
||||
*
|
||||
* Both routes parsed them and refused them identically, eight lines each. The
|
||||
* distinction being preserved is that `undefined` means "not submitted", which
|
||||
* update reads as "leave as-is" — so an unparseable value has to be told apart
|
||||
* from an absent one, which is what makes this more than a null check and worth
|
||||
* having in one place.
|
||||
*/
|
||||
type ParsedItemFields =
|
||||
| { ok: true; categoryId: number | null | undefined; tagNames: string[] | undefined }
|
||||
| { ok: false; error: string };
|
||||
|
||||
function readOptionalItemFields(body: Record<string, unknown>): ParsedItemFields {
|
||||
const categoryId = readCategoryId(body.category_id);
|
||||
if (categoryId === undefined && body.category_id !== undefined) {
|
||||
return { ok: false, error: 'invalid category_id' };
|
||||
}
|
||||
const tagNames = readTagNames(body.tags);
|
||||
if (tagNames === undefined && body.tags !== undefined) {
|
||||
return { ok: false, error: 'invalid tags' };
|
||||
}
|
||||
return { ok: true, categoryId, tagNames };
|
||||
}
|
||||
|
||||
router.get('/items', asyncRoute(async (req: Request, res: Response) => {
|
||||
// Same parser and query builder as the storefront, so admin filtering cannot
|
||||
// drift from what customers see. The one addition is `status`, which is how
|
||||
@@ -295,14 +349,9 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => {
|
||||
router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Response) => {
|
||||
const { name, description, price } = req.body;
|
||||
|
||||
const categoryId = readCategoryId(req.body.category_id);
|
||||
if (categoryId === undefined && req.body.category_id !== undefined) {
|
||||
return res.status(400).json({ error: 'invalid category_id' });
|
||||
}
|
||||
const tagNames = readTagNames(req.body.tags);
|
||||
if (tagNames === undefined && req.body.tags !== undefined) {
|
||||
return res.status(400).json({ error: 'invalid tags' });
|
||||
}
|
||||
const parsed = readOptionalItemFields(req.body);
|
||||
if (!parsed.ok) return res.status(400).json({ error: parsed.error });
|
||||
const { categoryId, tagNames } = parsed;
|
||||
|
||||
const files = (req.files as Express.Multer.File[]) || [];
|
||||
const client = await pool.connect();
|
||||
@@ -313,12 +362,7 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons
|
||||
[name, description, Math.round(parseFloat(price) * 100), categoryId ?? null]
|
||||
);
|
||||
const item = requireRow(rows, 'the item INSERT');
|
||||
for (let i = 0; i < files.length; i++) {
|
||||
await client.query(
|
||||
`INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`,
|
||||
[item.id, `/uploads/${files[i]?.filename ?? ''}`, i]
|
||||
);
|
||||
}
|
||||
await insertItemImages(client, item.id, files, 0);
|
||||
if (tagNames) {
|
||||
await setItemTags(client, item.id, await resolveTagIds(client, tagNames));
|
||||
}
|
||||
@@ -337,14 +381,9 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons
|
||||
router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Response) => {
|
||||
const { name, description, price } = req.body;
|
||||
|
||||
const categoryId = readCategoryId(req.body.category_id);
|
||||
if (categoryId === undefined && req.body.category_id !== undefined) {
|
||||
return res.status(400).json({ error: 'invalid category_id' });
|
||||
}
|
||||
const tagNames = readTagNames(req.body.tags);
|
||||
if (tagNames === undefined && req.body.tags !== undefined) {
|
||||
return res.status(400).json({ error: 'invalid tags' });
|
||||
}
|
||||
const parsed = readOptionalItemFields(req.body);
|
||||
if (!parsed.ok) return res.status(400).json({ error: parsed.error });
|
||||
const { categoryId, tagNames } = parsed;
|
||||
|
||||
const files = (req.files as Express.Multer.File[]) || [];
|
||||
const client = await pool.connect();
|
||||
@@ -368,13 +407,12 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
|
||||
[req.params.id]
|
||||
);
|
||||
// COALESCE'd MAX, so the aggregate always returns exactly one row.
|
||||
let nextSort = requireRow(existing, 'the MAX(sort_order) aggregate').max_sort + 1;
|
||||
for (const file of files) {
|
||||
await client.query(
|
||||
`INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`,
|
||||
[req.params.id, `/uploads/${file.filename}`, nextSort++]
|
||||
);
|
||||
}
|
||||
const nextSort = requireRow(existing, 'the MAX(sort_order) aggregate').max_sort + 1;
|
||||
// Number(), as the setItemTags call above already does: a matched route
|
||||
// always has this param, but noUncheckedIndexedAccess cannot know that,
|
||||
// and the helper's typed parameter surfaces what the old inline query's
|
||||
// unknown[] hid.
|
||||
await insertItemImages(client, Number(req.params.id), files, nextSort);
|
||||
}
|
||||
await client.query('COMMIT');
|
||||
const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]);
|
||||
|
||||
Reference in New Issue
Block a user