diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index f223f88..ad98c98 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -8,7 +8,14 @@ export type { ItemStatus }; export class FilterError extends Error {} export interface ItemFilters { - categoryId: number | null; + // Several categories, combined as OR — a customer picking Furniture and + // Decor wants both, not the empty intersection. Deliberately the opposite of + // how tagIds combine below, which is AND; the two controls say so in the UI + // rather than leaving it to be discovered. + // + // Each selected id still expands to its descendants, so choosing two parents + // means "anything filed under either of them". + categoryIds: number[]; tagIds: number[]; minPriceCents: number | null; maxPriceCents: number | null; @@ -110,12 +117,33 @@ function parsePrice(value: unknown, name: string): number | null { return parseNonNegativeInteger(raw, name); } -function parseCategoryId(value: unknown): number | null { +/** + * Comma-separated, the same shape `tags` and `status` already use. + * + * The parameter keeps its singular name so that every `?category=1` link, + * bookmark and shared URL written before this went multi-valued still parses — + * as a list of one, needing no alias and leaving no way to give the filter + * twice with different meanings. + */ +function parseCategoryIds(value: unknown): number[] { const raw = singleValue(value, 'category'); - if (raw === null || raw === '') { - return null; + const categoryIds: number[] = []; + if (!raw) { + return categoryIds; } - return parseId(raw, 'category'); + for (const part of raw.split(',')) { + const trimmed = part.trim(); + if (trimmed === '') { + continue; + } + const id = parseId(trimmed, 'category'); + // Duplicates are harmless to the OR below, but they would show twice in + // any caller that renders the parsed filter back. + if (!categoryIds.includes(id)) { + categoryIds.push(id); + } + } + return categoryIds; } function parseTagIds(value: unknown): number[] { @@ -193,7 +221,7 @@ function parseFavoritesOnly(value: unknown): boolean { // deliberate and observable: a query wrong in two ways reports the first // field, so moving these lines around changes which error a caller sees. export function parseItemFilters(query: Record): ItemFilters { - const categoryId = parseCategoryId(query.category); + const categoryIds = parseCategoryIds(query.category); const tagIds = parseTagIds(query.tags); const minPriceCents = parsePrice(query.min_price, 'min_price'); @@ -205,7 +233,7 @@ export function parseItemFilters(query: Record): ItemFilters { const status = parseStatus(query.status); const favoritesOnly = parseFavoritesOnly(query.favorites); - return { categoryId, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; + return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; } // Returns WHERE fragments plus their parameters, with placeholders numbered @@ -226,15 +254,20 @@ export function buildItemFilterSql( const params: unknown[] = []; let next = startIndex; - if (filters.categoryId !== null) { - params.push(filters.categoryId); + if (filters.categoryIds.length) { + params.push(filters.categoryIds); // Selecting a category means "and everything filed beneath it", so walk the - // tree down from the chosen node. A recursive CTE keeps the tree + // tree down from each chosen node. A recursive CTE keeps the tree // un-denormalized: reparenting stays a single UPDATE with no stored paths // to rewrite. + // + // Seeded with `= ANY(...)` rather than one id, so every selected root is + // walked in the same recursion. That also gives the OR for free: the union + // of the subtrees is exactly "filed under any of these", and an item filed + // under two selected branches appears once because IN is a set test. clauses.push(`i.category_id IN ( WITH RECURSIVE subtree AS ( - SELECT id FROM categories WHERE id = $${next} + SELECT id FROM categories WHERE id = ANY($${next}::int[]) UNION ALL SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id ) diff --git a/backend/tests/integration/categoriesTags.integration.test.ts b/backend/tests/integration/categoriesTags.integration.test.ts index bbbd16f..161d791 100644 --- a/backend/tests/integration/categoriesTags.integration.test.ts +++ b/backend/tests/integration/categoriesTags.integration.test.ts @@ -371,6 +371,34 @@ describe('GET /api/items filtering', () => { expect(res.body.map((i: { name: string }) => i.name).sort()).toEqual(['Deep', 'Mid', 'Top']); }); + it('combines several categories as any-of rather than all-of', async () => { + const furniture = await createCategory('Furniture'); + const tables = await createCategory('Tables', furniture); + const decor = await createCategory('Decor'); + const art = await createCategory('Art'); + + await createItem('Nested', 1000, tables); + await createItem('Elsewhere', 1000, decor); + await createItem('Unrelated', 1000, art); + + // Two branches sharing no items, so an AND would answer with nothing at + // all — which is the failure this exists to catch. + const res = await request(app).get(`/api/items?category=${furniture},${decor}`); + expect(res.body.map((i: { name: string }) => i.name).sort()).toEqual(['Elsewhere', 'Nested']); + }); + + it('lists an item once when its category sits under two selected branches', async () => { + const furniture = await createCategory('Furniture'); + const tables = await createCategory('Tables', furniture); + await createItem('Nested', 1000, tables); + + // The subtrees overlap: Tables is walked from both seeds, so the recursion + // yields its id twice. Selecting on IN makes that a set test rather than a + // join, so the item still appears once. + const res = await request(app).get(`/api/items?category=${furniture},${tables}`); + expect(res.body.map((i: { name: string }) => i.name)).toEqual(['Nested']); + }); + it('excludes uncategorized items from a category filter', async () => { const furniture = await createCategory('Furniture'); await createItem('Filed', 1000, furniture); @@ -444,6 +472,16 @@ describe('GET /api/items filtering', () => { expect(res.status).toBe(400); }); + it('rejects a category list with one unreadable entry rather than honouring the rest', async () => { + const furniture = await createCategory('Furniture'); + await createItem('A', 1000, furniture); + + // Filtering on the readable half would answer a narrower question than the + // one asked, and look indistinguishable from a filter that worked. + const res = await request(app).get(`/api/items?category=${furniture},furniture`); + expect(res.status).toBe(400); + }); + it('rejects an inverted price range', async () => { const res = await request(app).get('/api/items?min_price=5000&max_price=1000'); expect(res.status).toBe(400); diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 7c5eb5a..845aaf6 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -3,7 +3,7 @@ import { parseItemFilters, FilterError, buildItemFilterSql } from '../../src/ite describe('parseItemFilters', () => { it('returns empty filters for an empty query', () => { expect(parseItemFilters({})).toEqual({ - categoryId: null, + categoryIds: [], tagIds: [], minPriceCents: null, maxPriceCents: null, @@ -12,8 +12,30 @@ describe('parseItemFilters', () => { }); }); - it('parses a category id', () => { - expect(parseItemFilters({ category: '7' }).categoryId).toBe(7); + // The parameter stayed singular when it went multi-valued (#139), so every + // link written before that still parses — as a list of one. + it('parses a single category id, as older links still send it', () => { + expect(parseItemFilters({ category: '7' }).categoryIds).toEqual([7]); + }); + + it('parses a comma-separated category list', () => { + expect(parseItemFilters({ category: '4,9,2' }).categoryIds).toEqual([4, 9, 2]); + }); + + it('collapses duplicate category ids', () => { + expect(parseItemFilters({ category: '3,3,8' }).categoryIds).toEqual([3, 8]); + }); + + it('treats an empty category list as no category filter', () => { + expect(parseItemFilters({ category: '' }).categoryIds).toEqual([]); + }); + + // Decision 9: a malformed filter shows itself rather than quietly returning + // the whole catalogue, and going multi-valued must not weaken that. + it('refuses a category list containing anything unreadable', () => { + expect(() => parseItemFilters({ category: '4,nope' })).toThrow(FilterError); + expect(() => parseItemFilters({ category: '0' })).toThrow(FilterError); + expect(() => parseItemFilters({ category: '-1,2' })).toThrow(FilterError); }); it('parses a comma-separated tag list', () => { @@ -158,7 +180,19 @@ describe('buildItemFilterSql', () => { it('matches a category and all of its descendants', () => { const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 1, null); expect(built.clauses.join(' ')).toContain('RECURSIVE'); - expect(built.params).toEqual([4]); + // One array parameter rather than one id: the CTE is seeded with ANY so + // several selected roots are walked in the same recursion. + expect(built.params).toEqual([[4]]); + }); + + it('seeds the descendant walk with every selected category', () => { + const built = buildItemFilterSql(parseItemFilters({ category: '4,9' }), 1, null); + const sql = built.clauses.join(' '); + expect(sql).toContain('RECURSIVE'); + // ANY over the seeds is what makes several categories combine as OR: the + // result is the union of their subtrees. + expect(sql).toContain('= ANY($1::int[])'); + expect(built.params).toEqual([[4, 9]]); }); it('requires every listed tag rather than any of them', () => { @@ -215,7 +249,7 @@ describe('buildItemFilterSql', () => { 1, null ); - expect(built.params).toEqual([4, 100, 900]); + expect(built.params).toEqual([[4], 100, 900]); const sql = built.clauses.join(' '); expect(sql).toContain('$1'); expect(sql).toContain('$2'); diff --git a/frontend/src/admin/InventoryFilters.tsx b/frontend/src/admin/InventoryFilters.tsx index a2a57fe..44f898a 100644 --- a/frontend/src/admin/InventoryFilters.tsx +++ b/frontend/src/admin/InventoryFilters.tsx @@ -65,8 +65,16 @@ export default function InventoryFilters({ categories, tags, filters, onChange, aria-label="Filter by category" style={{ minWidth: 200 }} treeData={treeData} - value={filters.categoryId ?? undefined} - onChange={(value) => onChange({ ...filters, categoryId: value ?? null })} + // The filter shape went multi-valued for the storefront (#139). This + // control stays single-select — the admin asks "what is in this + // category", not "in any of these" — so it reads and writes a list of + // at most one rather than growing a second shape. + value={filters.categoryIds[0] ?? undefined} + // Annotated nullable because allowClear hands back undefined, which + // the control's own onChange type does not admit. + onChange={(value: number | undefined) => + onChange({ ...filters, categoryIds: value === undefined ? [] : [value] }) + } /> onChange({ ...filters, tagIds })} + options={tags.map((tag) => ({ value: tag.id, label: tag.name }))} + tagRender={({ value, label, closable, onClose }) => ( + { + event.preventDefault(); + event.stopPropagation(); + }} + style={{ marginInlineEnd: 4 }} + > + {label} + + )} + /> ) : ( )} diff --git a/frontend/src/filters.ts b/frontend/src/filters.ts index 1b984f9..dbd86f1 100644 --- a/frontend/src/filters.ts +++ b/frontend/src/filters.ts @@ -3,7 +3,14 @@ import type { Category } from './api'; export type ItemStatus = 'pending' | 'available' | 'reserved' | 'sold'; export interface ItemFilters { - categoryId: number | null; + // Several categories, combined as OR — picking Furniture and Decor means + // either, not the empty intersection. Deliberately the opposite of tagIds + // below, which is AND, and both controls label their rule so the difference + // is stated rather than discovered. + // + // The admin's inventory filter is single-select and holds a list of one; the + // type is shared, and one shape is better than two that drift. + categoryIds: number[]; tagIds: number[]; minPriceCents: number | null; maxPriceCents: number | null; @@ -69,7 +76,7 @@ export function saleStateFromStatuses( } export const EMPTY_FILTERS: ItemFilters = { - categoryId: null, + categoryIds: [], tagIds: [], minPriceCents: null, maxPriceCents: null, @@ -82,7 +89,9 @@ export const EMPTY_FILTERS: ItemFilters = { // what GET /api/items accepts, so the same object serializes for both. export function filtersToSearchParams(filters: ItemFilters): URLSearchParams { const params = new URLSearchParams(); - if (filters.categoryId !== null) params.set('category', String(filters.categoryId)); + // Comma-separated under the singular name it has always had, so a link + // written before this went multi-valued still means what it meant. + if (filters.categoryIds.length) params.set('category', filters.categoryIds.join(',')); if (filters.tagIds.length) params.set('tags', filters.tagIds.join(',')); if (filters.minPriceCents !== null) params.set('min_price', String(filters.minPriceCents)); if (filters.maxPriceCents !== null) params.set('max_price', String(filters.maxPriceCents)); @@ -98,6 +107,11 @@ function readInt(raw: string | null): number | null { } export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { + const categories = (params.get('category') || '') + .split(',') + .map((part) => readInt(part)) + .filter((id): id is number => id !== null && id > 0); + const tags = (params.get('tags') || '') .split(',') .map((part) => readInt(part)) @@ -127,7 +141,7 @@ export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { const favorites = params.get('favorites'); return { - categoryId: readInt(params.get('category')), + categoryIds: categories, tagIds: tags, minPriceCents: readInt(params.get('min_price')), maxPriceCents: readInt(params.get('max_price')), @@ -145,7 +159,7 @@ export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { // displays its own position. export function activeFilterCount(filters: ItemFilters): number { let count = 0; - if (filters.categoryId !== null) count++; + count += filters.categoryIds.length; count += filters.tagIds.length; if (filters.minPriceCents !== null || filters.maxPriceCents !== null) count++; if (filters.favoritesOnly) count++; diff --git a/frontend/tests/e2e/filters.spec.ts b/frontend/tests/e2e/filters.spec.ts index c9f1a4d..c8dabe3 100644 --- a/frontend/tests/e2e/filters.spec.ts +++ b/frontend/tests/e2e/filters.spec.ts @@ -89,6 +89,41 @@ test.describe('Storefront filters', () => { await expect(storefront.card(NAMES.midItem)).toBeHidden(); }); + test('several categories combine as any-of rather than all-of', async ({ + page, + storefront, + filterDrawer + }) => { + await storefront.goto(); + await storefront.openFilters(); + + // Different branches with no items in common, so an AND would show nothing. + await filterDrawer.chooseCategories(NAMES.tables, NAMES.decor); + + await expect(storefront.card(NAMES.deepItem)).toBeVisible(); + await expect(storefront.card(NAMES.otherItem)).toBeVisible(); + // Filed directly in Furniture, which was not among the selections. + await expect(storefront.card(NAMES.midItem)).toBeHidden(); + + // Both ride in the one parameter the filter has always used, so links + // written before it went multi-valued still mean what they meant. + await expect(page).toHaveURL(/category=\d+,\d+/); + }); + + test('each selected category gets its own removable chip', async ({ storefront, filterDrawer }) => { + await storefront.goto(); + await storefront.openFilters(); + await filterDrawer.chooseCategories(NAMES.tables, NAMES.decor); + await filterDrawer.close(); + + await storefront.removeFilterChip(NAMES.decor).click(); + + // Removing one chip narrows the filter to the other rather than clearing + // the category filter outright. + await expect(storefront.card(NAMES.deepItem)).toBeVisible(); + await expect(storefront.card(NAMES.otherItem)).toBeHidden(); + }); + test('requires every selected tag rather than any of them', async ({ storefront, filterDrawer }) => { await storefront.goto(); await storefront.openFilters(); diff --git a/frontend/tests/e2e/pages/FilterDrawer.ts b/frontend/tests/e2e/pages/FilterDrawer.ts index 63077e6..b4fc4bb 100644 --- a/frontend/tests/e2e/pages/FilterDrawer.ts +++ b/frontend/tests/e2e/pages/FilterDrawer.ts @@ -3,13 +3,17 @@ import { Locator, Page, expect } from '@playwright/test'; /** * The storefront's filter drawer. * - * Categories are a tree rather than a list, because a category filter matches - * the node and everything filed beneath it — so the locator is `treeitem`, and - * a nested category is only reachable once the tree has loaded its data. + * Both taxonomy filters are searchable multi-selects whose options only exist + * while the list is open, so every choose/toggle here opens the list, acts, and + * closes it again. * - * Tags are buttons that toggle. The rule they follow is AND, not OR: selecting - * two tags means "must have both", which is deliberately different from how - * categories combine and is the thing several tests exist to pin down. + * Categories keep their hierarchy — a category filter matches the node and + * everything filed beneath it — so their options are `treeitem`. Tags are flat, + * so theirs are `option`. + * + * The two follow opposite rules, which is the thing several tests exist to pin + * down: categories are OR (any of the selected branches), tags are AND (the + * item must carry all of them). */ export class FilterDrawer { readonly minimumPrice: Locator; @@ -17,6 +21,9 @@ export class FilterDrawer { readonly closeButton: Locator; readonly clearAllButton: Locator; readonly favoritesOnlySwitch: Locator; + readonly categorySelect: Locator; + readonly tagSelect: Locator; + readonly title: Locator; constructor(private readonly page: Page) { this.minimumPrice = page.getByLabel('Minimum price'); @@ -24,6 +31,9 @@ export class FilterDrawer { this.closeButton = page.getByRole('button', { name: 'Close' }); this.clearAllButton = page.getByRole('button', { name: 'Clear all' }); this.favoritesOnlySwitch = page.getByRole('switch', { name: 'Only my favorites' }); + this.categorySelect = page.getByRole('combobox', { name: 'Filter by category' }); + this.tagSelect = page.getByRole('combobox', { name: 'Filter by tags' }); + this.title = page.getByRole('heading', { name: 'Filters' }); } category(name: string): Locator { @@ -31,15 +41,50 @@ export class FilterDrawer { } tag(name: string): Locator { - return this.page.getByRole('button', { name }); + return this.page.getByRole('option', { name }); + } + + /** + * Opens the category list and leaves it open. + * + * Separate from choosing, so a test that picks two categories opens once — + * which is also what a customer does, the list staying open being the point + * of a multi-select. + */ + async openCategoryList(): Promise { + await this.categorySelect.click(); + } + + /** + * Closes whichever option list is open, without closing the drawer. + * + * Escape would do both — the drawer listens for it too — so this clicks off + * the control instead, onto the one part of the drawer nothing overlaps. + */ + async closeOptionList(): Promise { + await this.title.click(); } async chooseCategory(name: string): Promise { + await this.openCategoryList(); await this.category(name).click(); + await this.closeOptionList(); } + /** Picks several categories from one opening of the list, as the UI intends. */ + async chooseCategories(...names: string[]): Promise { + await this.openCategoryList(); + for (const name of names) { + await this.category(name).click(); + } + await this.closeOptionList(); + } + + /** Clicking a selected option in a multi-select deselects it, so this toggles. */ async toggleTag(name: string): Promise { + await this.tagSelect.click(); await this.tag(name).click(); + await this.closeOptionList(); } async setPriceRange(minimum?: string, maximum?: string): Promise {