diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index 44f2ebc..1d2df9f 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -9,7 +9,17 @@ export interface ItemFilters { tagIds: number[]; minPriceCents: number | null; maxPriceCents: number | null; - status: ItemStatus | null; + // Several statuses rather than one, because the control this exists to serve + // is not a status filter. "Not sold" is available-or-reserved on the + // storefront and available-or-reserved-or-pending in the admin, so it cannot + // be expressed as equality against a single value. A single-status filter is + // still expressible: it arrives as a list of one, which is how the admin's + // old `?status=sold` keeps working unchanged. + // + // Null means the caller expressed no preference, which is distinct from + // asking for every status — the storefront turns the first into its default + // and the second into an explicit list. + status: ItemStatus[] | null; // Storefront only: "just the items I have favorited". Which customer that // means is not part of the parsed filter — it comes from the session at build // time, so a query string can never name someone else's favorites. @@ -28,6 +38,19 @@ const ITEM_STATUSES: readonly string[] = ['pending', 'available', 'reserved', 's // to refuse it themselves rather than the parser refusing it for everyone. export const NON_PUBLIC_STATUSES: readonly ItemStatus[] = ['pending']; +// What the storefront lists when the caller expressed no preference. Named here +// rather than implied by the absence of a parameter, because the absence is now +// meaningful: before this change no status meant every status, and afterwards it +// means these two. Anything reading a shared link from before will get the new +// meaning, which is the accepted cost of the default changing. +export const STOREFRONT_DEFAULT_STATUSES: readonly ItemStatus[] = ['available', 'reserved']; + +// What "All" can mean on the storefront, which is not all of them. Pending items +// are excluded from every public read unconditionally, so a filter labelled All +// must not promise the fourth — a label that delivers less than it says is the +// shape this codebase keeps designing against. +export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold']; + export interface BuiltFilter { clauses: string[]; params: unknown[]; @@ -115,15 +138,39 @@ function parseTagIds(value: unknown): number[] { return tagIds; } -function parseStatus(value: unknown): ItemStatus | null { +// Comma-separated, matching how `tags` already works, so the two multi-value +// parameters in this parser read the same way in a URL. +// +// An unrecognised name is refused rather than dropped. Silently ignoring one +// would turn `?status=available,sold_out` into "available only" — narrower than +// what was asked for, and indistinguishable from a filter that worked. +function parseStatus(value: unknown): ItemStatus[] | null { const raw = singleValue(value, 'status'); if (raw === null || raw === '') { return null; } - if (!ITEM_STATUSES.includes(raw)) { + const statuses: ItemStatus[] = []; + for (const part of raw.split(',')) { + const trimmed = part.trim(); + if (trimmed === '') { + continue; + } + if (!ITEM_STATUSES.includes(trimmed)) { + throw new FilterError('invalid status'); + } + // Duplicates are harmless in `= ANY(...)`, but removing them keeps the + // parsed filter a faithful description of what was asked for. + if (!statuses.includes(trimmed as ItemStatus)) { + statuses.push(trimmed as ItemStatus); + } + } + // `?status=,,` asked for something and named nothing. Returning null would + // silently mean "no status filter", which on the storefront now means the + // default rather than everything — a different answer from the one requested. + if (statuses.length === 0) { throw new FilterError('invalid status'); } - return raw as ItemStatus; + return statuses; } function parseFavoritesOnly(value: unknown): boolean { @@ -221,7 +268,10 @@ export function buildItemFilterSql( if (filters.status !== null) { params.push(filters.status); - clauses.push(`i.status = $${next}`); + // ANY rather than equality, so one status and several use the same clause. + // The ::text[] cast is explicit because `status` is a text column and the + // driver would otherwise have to infer the array's element type. + clauses.push(`i.status = ANY($${next}::text[])`); next++; } diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index c7f673c..bfbab42 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -2,7 +2,14 @@ import { Router, Request, Response } from 'express'; import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; import { PUBLIC_ITEM_SELECT } from '../itemSelect'; -import { parseItemFilters, buildItemFilterSql, FilterError, NON_PUBLIC_STATUSES } from '../itemFilters'; +import { + parseItemFilters, + buildItemFilterSql, + FilterError, + NON_PUBLIC_STATUSES, + STOREFRONT_DEFAULT_STATUSES, + STOREFRONT_ALL_STATUSES +} from '../itemFilters'; // Applied to every public read, unconditionally. This route has never had a // status filter of its own — sold items are listed and rendered with a Sold @@ -38,11 +45,34 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { // 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". - if (filters.status && NON_PUBLIC_STATUSES.includes(filters.status)) { + // + // 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' }); } - const { clauses, params } = buildItemFilterSql(filters, 1, req.customerId ?? null); + // 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 { clauses, params } = buildItemFilterSql(effectiveFilters, 1, req.customerId ?? null); const where = [EXCLUDE_PENDING, ...clauses].join(' AND '); const { rows } = await pool.query( `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`, diff --git a/backend/tests/integration/soldFilter.integration.test.ts b/backend/tests/integration/soldFilter.integration.test.ts new file mode 100644 index 0000000..3f6e36b --- /dev/null +++ b/backend/tests/integration/soldFilter.integration.test.ts @@ -0,0 +1,196 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +// Direct insert so a test can put an item in a specific state without going +// through the transitions that put it there. +async function insertItem(name: string, status: string): Promise { + const { rows } = await pool.query( + `INSERT INTO items (name, price_cents, status) VALUES ($1, $2, $3) RETURNING id`, + [name, 10000, status] + ); + return rows[0].id; +} + +async function seedOneOfEach() { + await insertItem('Pending piece', 'pending'); + await insertItem('Available piece', 'available'); + await insertItem('Reserved piece', 'reserved'); + await insertItem('Sold piece', 'sold'); +} + +const namesFrom = (body: { name: string }[]) => body.map((i) => i.name).sort(); + +describe('the storefront status filter', () => { + // The product decision this change carries: with no filter the storefront no + // longer lists sold items. Asserted on the default rather than on the + // parameter, because the default is the part that changed for everyone. + it('lists neither sold nor pending items by default', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/items'); + + expect(res.status).toBe(200); + expect(namesFrom(res.body)).toEqual(['Available piece', 'Reserved piece']); + }); + + it('lists only sold items when asked for them', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/items?status=sold'); + + expect(namesFrom(res.body)).toEqual(['Sold piece']); + }); + + it('brings sold items back for an explicit All', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/items?status=available,reserved,sold'); + + expect(namesFrom(res.body)).toEqual(['Available piece', 'Reserved piece', 'Sold piece']); + }); + + it('lists not-sold items for an explicit Not Sold, matching the default', async () => { + await seedOneOfEach(); + + const explicit = await request(app).get('/api/items?status=available,reserved'); + const implied = await request(app).get('/api/items'); + + expect(namesFrom(explicit.body)).toEqual(namesFrom(implied.body)); + }); + + // The guarantee that must survive the filter being generalised. Pending is + // excluded from every public read by a clause the caller cannot opt out of, + // and these assert it directly rather than trusting the parser to go on + // refusing the name. + describe('pending stays unreachable from the storefront', () => { + it('refuses a request naming pending on its own', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/items?status=pending'); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid status'); + }); + + // The case a single-value check would have let through: the list is refused + // for naming pending at all, not accepted because the first name is fine. + it('refuses a request naming pending among allowed statuses', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/items?status=available,pending'); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid status'); + }); + + it('never returns a pending item under any accepted combination', async () => { + await seedOneOfEach(); + + for (const query of ['', '?status=sold', '?status=available,reserved,sold', '?status=reserved']) { + const res = await request(app).get(`/api/items${query}`); + expect(res.status).toBe(200); + expect(res.body.map((i: { name: string }) => i.name)).not.toContain('Pending piece'); + } + }); + }); + + it('refuses an unknown status rather than ignoring it', async () => { + const res = await request(app).get('/api/items?status=available,sold_out'); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid status'); + }); +}); + +describe('the admin inventory status filter', () => { + // All means all four here, unlike the storefront. Same word, two meanings, + // which is why each is asserted where it applies rather than assumed shared. + it('lists every status including pending when none is named', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/admin/items'); + + expect(namesFrom(res.body)).toEqual([ + 'Available piece', + 'Pending piece', + 'Reserved piece', + 'Sold piece' + ]); + }); + + it('still accepts a single status, which is what the old dropdown sent', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/admin/items?status=pending'); + + expect(namesFrom(res.body)).toEqual(['Pending piece']); + }); + + // Not Sold in the admin includes pending, which it does not on the storefront. + it('accepts a multi-status Not Sold that includes pending', async () => { + await seedOneOfEach(); + + const res = await request(app).get('/api/admin/items?status=available,reserved,pending'); + + expect(namesFrom(res.body)).toEqual(['Available piece', 'Pending piece', 'Reserved piece']); + }); +}); + +describe('the favorites view keeps its own default', () => { + // Favorites defaulted to showing sold items before this filter existed, on + // the reasoning that a favorite which has just sold is often exactly what the + // customer came to look at — they were emailed to say so. Defaulting them to + // Not Sold alongside everything else would have quietly reversed that. + it('shows a sold favorite with no status named', async () => { + const agent = request.agent(app); + expect( + (await agent.post('/api/customers/register').send({ + email: 'favdefault@example.com', + password: 'supersecret123', + firstName: 'Thom', + lastName: 'Lamb' + })).status + ).toBe(200); + + const soldId = await insertItem('Sold favorite', 'sold'); + const availableId = await insertItem('Available favorite', 'available'); + expect((await agent.post(`/api/customers/me/favorites/${soldId}`)).status).toBeLessThan(300); + expect((await agent.post(`/api/customers/me/favorites/${availableId}`)).status).toBeLessThan(300); + + const res = await agent.get('/api/items?favorites=1'); + + expect(res.status).toBe(200); + expect(namesFrom(res.body)).toEqual(['Available favorite', 'Sold favorite']); + }); + + // The default is a default, not an override: naming a status still wins. + it('honours an explicit Not Sold within the favorites view', async () => { + const agent = request.agent(app); + await agent.post('/api/customers/register').send({ + email: 'favexplicit@example.com', + password: 'supersecret123', + firstName: 'Thom', + lastName: 'Lamb' + }); + + const soldId = await insertItem('Sold favorite', 'sold'); + const availableId = await insertItem('Available favorite', 'available'); + await agent.post(`/api/customers/me/favorites/${soldId}`); + await agent.post(`/api/customers/me/favorites/${availableId}`); + + const res = await agent.get('/api/items?favorites=1&status=available,reserved'); + + expect(namesFrom(res.body)).toEqual(['Available favorite']); + }); +}); diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 0f98cbe..7c5eb5a 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -75,10 +75,41 @@ describe('parseItemFilters', () => { expect(() => parseItemFilters({ category: ['1', '2'] })).toThrow(FilterError); }); + // A single status parses to a list of one, which is what lets the admin's + // existing ?status=sold keep working unchanged against the multi-value shape. it('parses each of the item statuses', () => { - expect(parseItemFilters({ status: 'available' }).status).toBe('available'); - expect(parseItemFilters({ status: 'reserved' }).status).toBe('reserved'); - expect(parseItemFilters({ status: 'sold' }).status).toBe('sold'); + expect(parseItemFilters({ status: 'available' }).status).toEqual(['available']); + expect(parseItemFilters({ status: 'reserved' }).status).toEqual(['reserved']); + expect(parseItemFilters({ status: 'sold' }).status).toEqual(['sold']); + expect(parseItemFilters({ status: 'pending' }).status).toEqual(['pending']); + }); + + it('parses several statuses from one comma-separated value', () => { + expect(parseItemFilters({ status: 'available,reserved' }).status).toEqual([ + 'available', + 'reserved' + ]); + expect(parseItemFilters({ status: ' available , sold ' }).status).toEqual([ + 'available', + 'sold' + ]); + }); + + it('drops a repeated status rather than listing it twice', () => { + expect(parseItemFilters({ status: 'sold,sold' }).status).toEqual(['sold']); + }); + + // Refused rather than dropped. Ignoring the unknown name would turn this into + // "available only" - narrower than what was asked for, and indistinguishable + // from a filter that worked. + it('rejects a list containing an unknown status', () => { + expect(() => parseItemFilters({ status: 'available,sold_out' })).toThrow(FilterError); + }); + + // Asked for something, named nothing. Answering null would mean "no status + // filter", which on the storefront is the default rather than everything. + it('rejects a list that names nothing', () => { + expect(() => parseItemFilters({ status: ',,' })).toThrow(FilterError); }); it('treats an absent or empty status as no status filter', () => { @@ -145,8 +176,17 @@ describe('buildItemFilterSql', () => { it('filters on status', () => { const built = buildItemFilterSql(parseItemFilters({ status: 'reserved' }), 1, null); - expect(built.clauses.join(' ')).toContain('i.status'); - expect(built.params).toEqual(['reserved']); + expect(built.clauses.join(' ')).toContain('i.status = ANY'); + expect(built.params).toEqual([['reserved']]); + }); + + // One clause for one status and for several, which is the whole reason the + // filter was generalised rather than joined by a second dimension. + it('filters on several statuses with the same single clause', () => { + const built = buildItemFilterSql(parseItemFilters({ status: 'available,reserved' }), 1, null); + expect(built.clauses).toHaveLength(1); + expect(built.clauses[0]).toContain('i.status = ANY'); + expect(built.params).toEqual([['available', 'reserved']]); }); it('restricts to the favorites of the given customer', () => { diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 75b509e..e82bec4 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -10,6 +10,7 @@ import theme from 'antd/es/theme'; import Badge from 'antd/es/badge'; import Empty from 'antd/es/empty'; import Alert from 'antd/es/alert'; +import Segmented from 'antd/es/segmented'; import { ShoppingCartOutlined, FilterOutlined } from '@ant-design/icons'; import { Link, useLocation, useSearchParams } from 'react-router-dom'; import { Item, FilterOptions, fetchItems, fetchFilterOptions } from './api'; @@ -19,10 +20,13 @@ import FilterDrawer from './components/FilterDrawer'; import ActiveFilterChips from './components/ActiveFilterChips'; import { ItemFilters, + SaleState, + STOREFRONT_SALE_STATUSES, activeFilterCount, filtersFromSearchParams, filtersToSearchParams, - hasActiveFilters + hasActiveFilters, + saleStateFromStatuses } from './filters'; import AuthPromptModal from './customer/AuthPromptModal'; import { useThemeMode } from './theme/ThemeContext'; @@ -248,6 +252,37 @@ export default function App() {
+ {/* In the bar rather than inside the drawer, deliberately. The default + now hides sold pieces, so a customer who never opens the drawer + would otherwise have no way to know sold items exist — and on a + one-of-a-kind catalogue the sold pieces are part of the story. */} + { + const state = value as SaleState; + applyFilters({ + ...filters, + // Not Sold is the default, so it is stored as "no preference" + // rather than as an explicit list. That keeps it out of the URL + // and out of the Filters (N) count, where it would otherwise + // show as an active filter nobody chose. + status: state === 'not-sold' ? null : STOREFRONT_SALE_STATUSES[state] + }); + }} + options={[ + { label: 'Not sold', value: 'not-sold' }, + { label: 'Sold', value: 'sold' }, + { label: 'All', value: 'all' } + ]} + />