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/App.tsx b/frontend/src/App.tsx index f87ddda..cad4bdb 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -230,7 +230,8 @@ export default function App() { Filters{activeCount ? ` (${activeCount})` : ''} setDrawerOpen(false)} - options={options} + categories={options?.categories ?? []} + tags={options?.tags ?? []} + priceRange={options?.priceRange ?? null} filters={filters} onChange={applyFilters} onClear={clearFilters} resultCount={items.length} + showFavorites /> {/* The same prompt the heart button and Add to Cart use. Signing in diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx index 0e95270..7bcb6b9 100755 --- a/frontend/src/admin/Admin.tsx +++ b/frontend/src/admin/Admin.tsx @@ -275,6 +275,7 @@ function Inventory() { filters={filters} onChange={applyFilters} onClear={clearFilters} + resultCount={items.length} /> diff --git a/frontend/src/admin/InventoryFilters.tsx b/frontend/src/admin/InventoryFilters.tsx index a2a57fe..36f83d7 100644 --- a/frontend/src/admin/InventoryFilters.tsx +++ b/frontend/src/admin/InventoryFilters.tsx @@ -1,40 +1,10 @@ -import { useMemo } from 'react'; -import TreeSelect from 'antd/es/tree-select'; -import Select from 'antd/es/select'; -import InputNumber from 'antd/es/input-number'; +import { useState } from 'react'; import Button from 'antd/es/button'; +import { FilterOutlined } from '@ant-design/icons'; import type { Category, Tag } from '../api'; -import { - ItemFilters, - ItemStatus, - buildCategoryTree, - CategoryNode, - hasActiveFilters -} from '../filters'; - -// Named individually rather than grouped, because grouping is what the preset -// this replaces did. Pending is listed first: "what is waiting to be published" -// is the question that prompted #132. -const STATUS_OPTIONS: { value: ItemStatus; label: string }[] = [ - { value: 'pending', label: 'Pending' }, - { value: 'available', label: 'Available' }, - { value: 'reserved', label: 'Reserved' }, - { value: 'sold', label: 'Sold' } -]; - -interface CategoryTreeOption { - value: number; - title: string; - children?: CategoryTreeOption[]; -} - -function toTreeData(nodes: CategoryNode[]): CategoryTreeOption[] { - return nodes.map((node) => ({ - value: node.id, - title: node.name, - children: node.children.length ? toTreeData(node.children) : undefined - })); -} +import { ItemFilters, activeFilterCount } from '../filters'; +import FilterDrawer from '../components/FilterDrawer'; +import ActiveFilterChips from '../components/ActiveFilterChips'; type Props = Readonly<{ categories: Category[]; @@ -42,95 +12,67 @@ type Props = Readonly<{ filters: ItemFilters; onChange: (filters: ItemFilters) => void; onClear: () => void; + resultCount: number; }>; -const centsToDollars = (cents: number | null): number | null => (cents === null ? null : cents / 100); -const dollarsToCents = (dollars: number | null): number | null => - dollars === null || Number.isNaN(dollars) ? null : Math.round(dollars * 100); +// The same flyout the storefront uses, rather than the row of controls this +// used to be (#169). +// +// That row was deliberate — it carried a comment arguing that hiding the +// controls above a data table costs more than the space it saves, and that a +// drawer overlays the very rows being filtered. Both are true, and both are +// traded for the two screens asking the same questions through the same UI. +// The chips are what makes the trade bearable: the active filter stays readable +// beside the button without opening anything, which is what the always-visible +// row was really protecting. +export default function InventoryFilters({ + categories, + tags, + filters, + onChange, + onClear, + resultCount +}: Props) { + const [drawerOpen, setDrawerOpen] = useState(false); -// An always-visible row rather than the storefront's drawer: this sits above a -// data table, where hiding the controls behind a click costs more than the -// space it saves, and a drawer would overlay the very rows being filtered. -export default function InventoryFilters({ categories, tags, filters, onChange, onClear }: Props) { - const treeData = useMemo(() => toTreeData(buildCategoryTree(categories)), [categories]); + // Status lives in the drawer here, unlike on the storefront, so it belongs in + // the button's tally — activeFilterCount leaves it out precisely because the + // storefront filters status outside the drawer. + const activeCount = activeFilterCount(filters) + (filters.status === null ? 0 : 1); return (
- onChange({ ...filters, categoryId: value ?? null })} + + + - - // Empty means no filter, not "no statuses". A multi-select cleared - // back to nothing should show everything rather than an empty table. - onChange({ ...filters, status: value.length ? value : null }) - } - options={STATUS_OPTIONS} - /> - - {hasActiveFilters(filters) && }
); } diff --git a/frontend/src/components/ActiveFilterChips.tsx b/frontend/src/components/ActiveFilterChips.tsx index 0c71fd8..32a5afe 100644 --- a/frontend/src/components/ActiveFilterChips.tsx +++ b/frontend/src/components/ActiveFilterChips.tsx @@ -1,21 +1,30 @@ import Tag from 'antd/es/tag'; import Button from 'antd/es/button'; -import type { FilterOptions } from '../api'; -import { ItemFilters, categoryPath, formatPriceRange, hasActiveFilters } from '../filters'; +import type { Category, Tag as ItemTag } from '../api'; +import { ItemFilters, categoryPath, formatPriceRange, hasActiveFilters, statusLabel } from '../filters'; type Props = Readonly<{ - options: FilterOptions | null; + categories: Category[]; + tags: ItemTag[]; filters: ItemFilters; onChange: (filters: ItemFilters) => void; onClear: () => void; + // Where status is one of the drawer's controls rather than a preset beside + // it (#169), it needs a chip too — otherwise the one filter most likely to + // empty a table is the one filter invisible without opening the drawer. + showStatus?: boolean; }>; -export default function ActiveFilterChips({ options, filters, onChange, onClear }: Props) { +export default function ActiveFilterChips({ + categories, + tags, + filters, + onChange, + onClear, + showStatus = false +}: Props) { if (!hasActiveFilters(filters)) return null; - const categories = options?.categories ?? []; - const tags = options?.tags ?? []; - const chips: { key: string; label: string; onRemove: () => void }[] = []; // Listed first so it matches the drawer's ordering, and because it is the @@ -28,17 +37,20 @@ export default function ActiveFilterChips({ options, filters, onChange, onClear }); } - if (filters.categoryId !== null) { - const path = categoryPath(categories, filters.categoryId); + // One chip per selected category, each removable on its own — removing the + // whole set at once is what Clear all is for. + for (const categoryId of filters.categoryIds) { + const path = categoryPath(categories, categoryId); // Falls back to the raw id while /api/filters is still loading, so the chip // never renders as an empty box. - const label = path || `Category ${filters.categoryId}`; + const label = path || `Category ${categoryId}`; chips.push({ - key: `category-${filters.categoryId}`, - // The removable name is the leaf, matching what the user clicked in the - // tree, while the chip itself shows the full path for context. + key: `category-${categoryId}`, + // The chip shows the full path for context, since two categories can + // share a leaf name under different parents. label, - onRemove: () => onChange({ ...filters, categoryId: null }) + onRemove: () => + onChange({ ...filters, categoryIds: filters.categoryIds.filter((id) => id !== categoryId) }) }); } @@ -51,6 +63,21 @@ export default function ActiveFilterChips({ options, filters, onChange, onClear }); } + if (showStatus && filters.status !== null) { + for (const status of filters.status) { + chips.push({ + key: `status-${status}`, + label: statusLabel(status), + onRemove: () => { + const rest = (filters.status ?? []).filter((value) => value !== status); + // Back to null rather than an empty list: emptying the control means + // "no status filter", not "no statuses", which would empty the table. + onChange({ ...filters, status: rest.length ? rest : null }); + } + }); + } + } + if (filters.minPriceCents !== null || filters.maxPriceCents !== null) { chips.push({ key: 'price', diff --git a/frontend/src/components/FilterDrawer.tsx b/frontend/src/components/FilterDrawer.tsx index 673e02d..efb7b03 100644 --- a/frontend/src/components/FilterDrawer.tsx +++ b/frontend/src/components/FilterDrawer.tsx @@ -1,34 +1,67 @@ import Drawer from 'antd/es/drawer'; import Button from 'antd/es/button'; -import Tree from 'antd/es/tree'; +import TreeSelect from 'antd/es/tree-select'; +import Select from 'antd/es/select'; import Tag from 'antd/es/tag'; import Slider from 'antd/es/slider'; import InputNumber from 'antd/es/input-number'; import Empty from 'antd/es/empty'; import Switch from 'antd/es/switch'; import Grid from 'antd/es/grid'; -import type { DataNode } from 'antd/es/tree'; -import type { FilterOptions } from '../api'; -import { ItemFilters, buildCategoryTree, CategoryNode } from '../filters'; +import type { Category, Tag as ItemTag } from '../api'; +import { ItemFilters, ItemStatus, STATUS_OPTIONS, buildCategoryTree, CategoryNode } from '../filters'; +// One drawer for the storefront and the admin, with the sections that differ +// driven by props rather than by a second component that would drift (#169). +// What is shared is not just the markup but the phrasing of the rules — that +// categories are OR and tags are AND has to read the same on both screens or it +// stops being one rule. type Props = Readonly<{ open: boolean; onClose: () => void; - options: FilterOptions | null; + categories: Category[]; + tags: ItemTag[]; + // Bounds for the price slider, or null on a screen with no catalogue-wide + // range to draw one from, where the two number inputs stand alone. A slider + // needs real bounds: invented ones would misreport where the prices are. + priceRange: { min_cents: number; max_cents: number } | null; filters: ItemFilters; onChange: (filters: ItemFilters) => void; onClear: () => void; resultCount: number; + // Storefront only — signing in is what makes favorites mean anything. + showFavorites?: boolean; + // Admin only. The storefront keeps its three-way preset outside the drawer: + // pending is excluded from every public read, so Published and Unpublished + // are not distinctions a customer can draw. + showStatus?: boolean; }>; -function toTreeData(nodes: CategoryNode[]): DataNode[] { +// `value` rather than `key`: this fed an antd `Tree`, which identifies nodes by +// key, and now feeds a `TreeSelect`, which selects and searches by value. The +// shape matches the admin's CategoryTreeSelect so the two stay comparable. +interface CategoryTreeOption { + value: number; + title: string; + children?: CategoryTreeOption[]; +} + +function toTreeData(nodes: CategoryNode[]): CategoryTreeOption[] { return nodes.map((node) => ({ - key: node.id, + value: node.id, title: node.name, children: node.children.length ? toTreeData(node.children) : undefined })); } +const sectionHeading: React.CSSProperties = { + margin: '0 0 8px', + fontSize: 12, + letterSpacing: '.06em', + textTransform: 'uppercase', + opacity: 0.65 +}; + const centsToDollars = (cents: number | null): number | null => (cents === null ? null : cents / 100); const dollarsToCents = (dollars: number | null): number | null => dollars === null || Number.isNaN(dollars) ? null : Math.round(dollars * 100); @@ -36,30 +69,26 @@ const dollarsToCents = (dollars: number | null): number | null => export default function FilterDrawer({ open, onClose, - options, + categories, + tags, + priceRange, filters, onChange, onClear, - resultCount + resultCount, + showFavorites = false, + showStatus = false }: Props) { const screens = Grid.useBreakpoint(); - const categories = options?.categories ?? []; - const tags = options?.tags ?? []; - const bounds = options?.priceRange ?? { min_cents: 0, max_cents: 0 }; + const bounds = priceRange ?? { min_cents: 0, max_cents: 0 }; - function toggleTag(tagId: number) { - const next = filters.tagIds.includes(tagId) - ? filters.tagIds.filter((id) => id !== tagId) - : [...filters.tagIds, tagId]; - onChange({ ...filters, tagIds: next }); + function selectCategories(ids: number[]) { + onChange({ ...filters, categoryIds: ids }); } - // Selecting the already-selected node clears the filter, so the tree doubles - // as its own "all items" control. - function selectCategory(keys: React.Key[]) { - const picked = keys.length ? Number(keys[0]) : null; - onChange({ ...filters, categoryId: picked === filters.categoryId ? null : picked }); - } + // The selected pills are rendered by the Select, which is handed ids rather + // than tags, so the colour has to be looked up rather than carried along. + const tagColors = new Map(tags.map((tag) => [tag.id, tag.color])); const sliderMax = Math.max(bounds.max_cents, bounds.min_cents + 100); @@ -86,8 +115,9 @@ export default function FilterDrawer({ here for their favorites should not have to scroll past the catalogue controls to find it. Shown to signed-out visitors too: switching it on prompts them to sign in, which is how they learn favorites exist. */} + {showFavorites && (
-

+

Favorites

{/* Deliberately not wrapped in a
+ )}
-

- Category +

+ Categories — any of these

{categories.length ? ( - ) : ( @@ -122,42 +165,55 @@ export default function FilterDrawer({
-

- Tags — must have all +

+ Tags — must have all of these

{tags.length ? ( -
- {tags.map((tag) => { - const selected = filters.tagIds.includes(tag.id); - return ( - // A real button rather than a styled span, so the pills are - // reachable by keyboard and announce their on/off state. - - ); - })} -
+ // Was a wall of every tag in the system, which read fine at a dozen + // and not at a hundred. A searchable multi-select scales with the + // taxonomy and, like the category control above it, states its + // selection inside the control instead of in chip colouring. + // + // The colours survive as the selected pills, since that is the only + // place a tag's colour was ever load-bearing. + + // Empty means no filter, not "no statuses". A multi-select cleared + // back to nothing should show everything rather than an empty table. + onChange({ ...filters, status: value.length ? value : null }) + } + options={STATUS_OPTIONS} + /> +
+ )} ); } diff --git a/frontend/src/filters.ts b/frontend/src/filters.ts index 1b984f9..0395058 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')), @@ -143,9 +157,27 @@ export function filtersFromSearchParams(params: URLSearchParams): ItemFilters { // beside this button rather than living in the drawer, so counting it would put // a number on a button whose drawer shows nothing set — and the control already // displays its own position. +// Named individually rather than grouped, because grouping is what the preset +// this replaced did. Pending is listed first: "what is waiting to be published" +// is the question that prompted #132. +// +// Here rather than in the admin screen because the drawer and the active-filter +// chips both need to turn a status into a label, and a second copy of this list +// is a second place for a new status to be forgotten. +export const STATUS_OPTIONS: { value: ItemStatus; label: string }[] = [ + { value: 'pending', label: 'Pending' }, + { value: 'available', label: 'Available' }, + { value: 'reserved', label: 'Reserved' }, + { value: 'sold', label: 'Sold' } +]; + +export function statusLabel(status: ItemStatus): string { + return STATUS_OPTIONS.find((option) => option.value === status)?.label ?? status; +} + 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/admin-inventory-filters.spec.ts b/frontend/tests/e2e/admin-inventory-filters.spec.ts index e579d3f..29a3338 100644 --- a/frontend/tests/e2e/admin-inventory-filters.spec.ts +++ b/frontend/tests/e2e/admin-inventory-filters.spec.ts @@ -43,8 +43,7 @@ test.describe('Admin inventory filters', () => { await admin.goto(); await adminInventory.filterByCategory(NAMES.category, NAMES.cheap); - await adminInventory.minimumPrice.fill('100'); - await adminInventory.maximumPrice.fill('500'); + await adminInventory.setPriceRange('100', '500'); await expect(adminInventory.row(NAMES.mid)).toBeVisible(); await expect(adminInventory.row(NAMES.cheap)).toHaveCount(0); @@ -111,16 +110,20 @@ test.describe('Admin inventory filters', () => { test('combines filters, and clearing restores them', async ({ admin, adminInventory }) => { await admin.goto(); await adminInventory.filterByCategory(NAMES.category, NAMES.cheap); - await adminInventory.minimumPrice.fill('800'); + await adminInventory.setPriceRange('800'); await expect(adminInventory.row(NAMES.cheap)).toHaveCount(0); await expect(adminInventory.row(NAMES.dear)).toBeVisible(); - await adminInventory.clearFiltersButton.click(); + await adminInventory.clearFilters(); // Asserting on the controls rather than on the rows: with the filters gone // the table is the whole paginated catalogue again, so a given fixture is // not reliably on the first page. + // + // The chip row renders only while something is filtered, and the button's + // tally is the other half of the same claim — nothing is filtered, and the + // screen says so without the drawer being opened to check. await expect(adminInventory.clearFiltersButton).toHaveCount(0); - await expect(adminInventory.minimumPrice).toHaveValue(''); + await expect(adminInventory.filtersButton).toHaveText('Filters'); }); }); diff --git a/frontend/tests/e2e/filters.spec.ts b/frontend/tests/e2e/filters.spec.ts index c9f1a4d..e37686a 100644 --- a/frontend/tests/e2e/filters.spec.ts +++ b/frontend/tests/e2e/filters.spec.ts @@ -89,6 +89,46 @@ 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. + // + // The separator arrives percent-encoded because URLSearchParams encodes a + // comma, which is how the `tags` parameter has always looked too. Either + // spelling parses, so the assertion accepts both rather than pinning the + // encoding. + await expect(page).toHaveURL(/category=\d+(,|%2C)\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/AdminInventory.ts b/frontend/tests/e2e/pages/AdminInventory.ts index fba88bd..6325a83 100644 --- a/frontend/tests/e2e/pages/AdminInventory.ts +++ b/frontend/tests/e2e/pages/AdminInventory.ts @@ -81,6 +81,31 @@ export class AdminInventory { } // ---- The inventory filter bar ---- + // + // The controls moved into the storefront's flyout (#169), so each method here + // opens the drawer, acts, and closes it again. Closing matters: the drawer + // overlays the table, and every assertion in these specs is about rows. + + get filtersButton(): Locator { + return this.page.getByRole('button', { name: 'Filters' }); + } + + get filterDrawer(): Locator { + return this.page.getByRole('dialog', { name: 'Filters' }); + } + + /** + * The chip row's own "Clear all", scoped to the group so it stays distinct + * from the identically-labelled button in the drawer's footer. + * + * It exists only while something is filtered, which is what lets a spec assert + * that clearing worked by its absence. + */ + get clearFiltersButton(): Locator { + return this.page + .getByRole('group', { name: 'Active filters' }) + .getByRole('button', { name: 'Clear all' }); + } get categoryFilter(): Locator { return this.page.getByRole('combobox', { name: 'Filter by category' }); @@ -98,30 +123,37 @@ export class AdminInventory { return this.page.getByLabel('Maximum price'); } - get clearFiltersButton(): Locator { - return this.page.getByRole('button', { name: 'Clear filters' }); + /** Opens the flyout, or leaves it open if it already is. */ + async openFilters(): Promise { + if (await this.filterDrawer.isVisible().catch(() => false)) return; + await this.filtersButton.click(); + await expect(this.filterDrawer).toBeVisible(); + } + + /** Closes it through the footer button, which is what a person would click. */ + async closeFilters(): Promise { + await this.filterDrawer.getByRole('button', { name: /^Show / }).click(); + await expect(this.filterDrawer).toBeHidden(); } /** * Toggles one status in the multi-select. Clicking a selected option removes * it, which is what the clearing test relies on. * - * Two antd details decide this locator. It renders an invisible role="listbox" - * shim beside the real list for accessibility, so getByRole('option') finds - * something zero-sized that cannot be clicked. And once a status is selected - * it also renders as a tag carrying the same title as the option, so an - * unscoped getByTitle becomes ambiguous. Matching the visible option class - * avoids both. - * - * The dropdown is opened only when it is not already open: antd keeps it open - * after a selection in multiple mode, so clicking the box again would close it. + * The option is matched by class rather than by role because antd renders an + * invisible role="listbox" shim beside the real list for accessibility, so + * getByRole('option') finds something zero-sized that cannot be clicked; and + * once a status is selected it also renders as a tag carrying the same title, + * so an unscoped getByTitle becomes ambiguous. */ async toggleStatus(label: string): Promise { + await this.openFilters(); const option = this.page.locator(`.ant-select-item-option[title="${label}"]`); if (!(await option.isVisible().catch(() => false))) { await this.statusFilter.click(); } await option.click(); + await this.closeFilters(); } /** @@ -131,14 +163,32 @@ export class AdminInventory { * unfiltered page 1 is not a reliable place to look for a fixture. Waiting for * a known row is the action's contract — the filter has been applied when the * table has re-rendered under it. + * + * Typing the name before clicking it is not for realism: the tree is + * virtualized, so against a database holding hundreds of categories the wanted + * row is never rendered until a search narrows to it. */ async filterByCategory(categoryName: string, expectedRow: string): Promise { + await this.openFilters(); await this.categoryFilter.click(); await this.categoryFilter.fill(categoryName); - await this.page.getByTitle(categoryName, { exact: true }).click(); + await this.page.getByRole('treeitem', { name: categoryName }).click(); + await this.closeFilters(); await expect(this.row(expectedRow)).toBeVisible(); } + /** Sets either end of the price range, leaving an omitted end untouched. */ + async setPriceRange(minimum?: string, maximum?: string): Promise { + await this.openFilters(); + if (minimum !== undefined) await this.minimumPrice.fill(minimum); + if (maximum !== undefined) await this.maximumPrice.fill(maximum); + await this.closeFilters(); + } + + async clearFilters(): Promise { + await this.clearFiltersButton.click(); + } + async openItemForm(): Promise { await this.addItemButton.click(); } diff --git a/frontend/tests/e2e/pages/FilterDrawer.ts b/frontend/tests/e2e/pages/FilterDrawer.ts index 63077e6..e26fdf0 100644 --- a/frontend/tests/e2e/pages/FilterDrawer.ts +++ b/frontend/tests/e2e/pages/FilterDrawer.ts @@ -3,13 +3,26 @@ 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. + * Each one types the name before clicking it. Not for realism — the lists are + * virtualized, so against a database holding hundreds of categories the wanted + * row is never rendered until a search narrows to it. Scrolling to it would be + * testing the virtual list rather than the filter. + * + * Categories keep their hierarchy — a category filter matches the node and + * everything filed beneath it — so their options are `treeitem`. + * + * Tags are flat, and their options are matched by class rather than by role, + * for the reason AdminInventory.toggleStatus records: antd renders an invisible + * role="listbox" shim beside the real list, so getByRole('option') resolves to + * something zero-sized that can never be clicked. + * + * 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 +30,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 +40,11 @@ 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' }); + // Somewhere inside the drawer that is inert and never covered by an + // option list, which opens downward from the controls below it. + this.title = page.getByRole('heading', { name: 'Favorites' }); } category(name: string): Locator { @@ -31,15 +52,62 @@ export class FilterDrawer { } tag(name: string): Locator { - return this.page.getByRole('button', { name }); + return this.page.locator(`.ant-select-item-option[title="${name}"]`); } - async chooseCategory(name: string): Promise { + /** + * 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(); + } + + /** + * Narrows to one category and clicks it, with the list already open. + * + * Selecting clears the search box, so consecutive calls each start from the + * unfiltered list. + */ + private async pickCategory(name: string): Promise { + await this.categorySelect.fill(name); await this.category(name).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.pickCategory(name); + 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.pickCategory(name); + } + 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.tagSelect.fill(name); await this.tag(name).click(); + await this.closeOptionList(); } async setPriceRange(minimum?: string, maximum?: string): Promise {