backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
113 lines
4.6 KiB
TypeScript
Executable File
113 lines
4.6 KiB
TypeScript
Executable File
import { Router, Request, Response } from 'express';
|
|
import { asyncRoute } from '../asyncRoute';
|
|
import { publicItemQuery, PublicItemRow, ItemContext } from '../itemSelect';
|
|
import {
|
|
parseItemFilters,
|
|
itemFilterExpressions,
|
|
FilterError,
|
|
NON_PUBLIC_STATUSES,
|
|
STOREFRONT_DEFAULT_STATUSES,
|
|
STOREFRONT_ALL_STATUSES
|
|
} from '../itemFilters';
|
|
|
|
/**
|
|
* Pending items are excluded everywhere, not only from the list. A pending item
|
|
* that stayed fetchable by id would be hidden from the catalogue and still
|
|
* reachable by anyone who guessed or kept a link.
|
|
*
|
|
* An expression rather than the SQL literal this was until #308, so it composes
|
|
* with the filter clauses through `eb.and` instead of being joined into a
|
|
* string. That join used to need its own argument about why AND could not
|
|
* weaken it; `and` cannot re-associate anything.
|
|
*/
|
|
function notPending(eb: ItemContext) {
|
|
return eb('i.status', '!=', 'pending');
|
|
}
|
|
|
|
const router = Router();
|
|
|
|
router.get('/', asyncRoute(async (req: Request, res: Response) => {
|
|
let filters;
|
|
try {
|
|
filters = parseItemFilters(req.query as Record<string, unknown>);
|
|
} catch (err) {
|
|
// A malformed filter is returned as an error rather than ignored, so a
|
|
// broken link shows itself instead of quietly listing the whole catalogue.
|
|
if (err instanceof FilterError) {
|
|
return res.status(400).json({ error: err.message });
|
|
}
|
|
throw err;
|
|
}
|
|
|
|
// 401 rather than an empty list: a signed-out visitor asking for "my
|
|
// favorites" has no favorites to be empty of, and answering with [] would
|
|
// render as "no items match these filters" — a plausible-looking lie. The
|
|
// storefront prompts for sign-in instead of sending this, so reaching here
|
|
// means a bookmarked link outlived its session.
|
|
if (filters.favoritesOnly && !req.customerId) {
|
|
return res.status(401).json({ error: 'sign in to filter by favorites' });
|
|
}
|
|
|
|
// Refused rather than quietly answered. The filter parser is shared with the
|
|
// admin routes, where 'pending' is valid, so it parses here too — and with
|
|
// the exclusion below it would return an empty list, which reads as "no items
|
|
// match" rather than "you may not ask that".
|
|
//
|
|
// Checked across every requested status, not just a single one: `?status=
|
|
// available,pending` must be refused for naming pending at all, rather than
|
|
// quietly answered because the first name in the list happened to be allowed.
|
|
if (filters.status?.some((status) => NON_PUBLIC_STATUSES.includes(status))) {
|
|
return res.status(400).json({ error: 'invalid status' });
|
|
}
|
|
|
|
// No preference means Not Sold rather than everything. Applied here rather
|
|
// than in the parser, which is shared with the admin, where the same absence
|
|
// has to go on meaning "every status including pending".
|
|
//
|
|
// Except when the customer asked for their own favorites, where the default
|
|
// stays everything. A favorite that has just sold is often exactly what the
|
|
// customer came to look at — they were emailed to say so — and hiding it
|
|
// would make an item they curated vanish without explanation. That was a
|
|
// deliberate decision before this filter existed, and defaulting favorites to
|
|
// Not Sold would have quietly reversed it. An explicit ?status= still wins,
|
|
// so the choice remains theirs.
|
|
const defaultStatuses = filters.favoritesOnly
|
|
? STOREFRONT_ALL_STATUSES
|
|
: STOREFRONT_DEFAULT_STATUSES;
|
|
const effectiveFilters = {
|
|
...filters,
|
|
status: filters.status ?? [...defaultStatuses]
|
|
};
|
|
|
|
const rows: PublicItemRow[] = await publicItemQuery()
|
|
.where((eb) =>
|
|
eb.and([
|
|
notPending(eb),
|
|
...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null)
|
|
])
|
|
)
|
|
.orderBy('i.created_at', 'desc')
|
|
.execute();
|
|
|
|
res.json(rows);
|
|
}));
|
|
|
|
router.get('/:id', asyncRoute(async (req: Request, res: Response) => {
|
|
// Number() rather than readId(), and that is deliberate rather than an
|
|
// oversight. readId would be stricter and would match every other id-taking
|
|
// route (#207) — but errorHandling.integration.test.ts drives this exact
|
|
// route with a non-numeric id to prove that asyncRoute plus the error
|
|
// middleware answer 500 rather than leaving the request hanging, and a
|
|
// stricter parse here would leave that test green while removing the thing
|
|
// it tests. Switching this over means giving that test another trigger in
|
|
// the same change. See #307.
|
|
const rows = await publicItemQuery()
|
|
.where('i.id', '=', Number(req.params.id))
|
|
.where((eb) => notPending(eb))
|
|
.execute();
|
|
if (!rows.length) return res.status(404).json({ error: 'not found' });
|
|
res.json(rows[0]);
|
|
}));
|
|
|
|
export default router;
|