diff --git a/src/app/(app)/delivery-notes/page.tsx b/src/app/(app)/delivery-notes/page.tsx index 94ffd20..7b6dc67 100644 --- a/src/app/(app)/delivery-notes/page.tsx +++ b/src/app/(app)/delivery-notes/page.tsx @@ -28,6 +28,10 @@ import { deriveDnStage, dnStageTint, dnTabForStatus, + isDoneStatus, + matchesDeliveryNoteSearch, + matchesDoneFilter, + type DoneFilter, type DnStage, } from "@/lib/delivery-note-stage"; import { @@ -64,67 +68,48 @@ import type { } from "@/lib/types"; /** - * Tab key for the list-page filter strip. The four buckets cover - * the full lifecycle without forcing the operator to scroll: + * Tab key for the list-page filter strip. + * * - "active" → draft + sent + loaded + po_received (anything the * operator still has to act on) - * - "completed" → invoiced + completed (terminal, visible but inert) - * - "cancelled" → cancelled only (kept separate so old cancelled DNs - * don't pollute the working list, and so the - * Completed count isn't a lie about what shipped) + * - "completed" → invoiced + completed + CANCELLED, narrowed by the + * `doneFilter` sub-control below * - "all" → no filter (escape hatch when the operator * doesn't know the status) * - * `invoiced` deliberately is NOT in "active". It used to be, which is - * why the active list never drained — finished work stayed in the - * operator's face all day. The buckets are derived through - * `dnTabForStatus` so the tab and the colour ladder can never disagree - * about what "active" means. + * `invoiced` deliberately is NOT in "active" — that is why the active + * list drains instead of being a graveyard. + * + * Cancelled used to have its own tab. It doesn't any more: the + * operator asked for it to sit under the done heading with a filter + * between the two, rather than spending a tab on a small, cold set + * that nobody visits. See `matchesDoneFilter`. */ -type DnTab = "active" | "cancelled" | "completed" | "all"; +type DnTab = "active" | "completed" | "all"; const TAB_LABELS: Record = { active: "Active", - cancelled: "Cancelled", completed: "Completed", all: "All", }; -const ALL_STATUSES: DeliveryNoteStatus[] = [ - "draft", - "sent", - "loaded", - "po_received", - "invoiced", - "completed", - "cancelled", -]; - -const TAB_STATUSES: Record = { - active: ["draft", "sent", "loaded", "po_received"], - cancelled: ["cancelled"], - completed: ["invoiced", "completed"], - all: ALL_STATUSES, -}; - /** - * Status → badge treatment. Every status is deliberately NEUTRAL. + * Status → badge treatment. * - * The badge's job is to name the state; the row's tint is what tells - * the operator where they are in the workflow. Colouring both meant - * "Invoiced", "PO received" and "Completed" all mapped to `default` — - * byte-for-byte identical rendering — while the three rows that - * genuinely look different sat underneath them. One signal, one - * place: the row (see `rowStyle`). + * `cancelled` is the one status that keeps a colour. It has to: it's + * now inside the Completed tab, so "Completed" and "Cancelled" rows + * sit next to each other and the operator needs to tell them apart + * without opening each one. Everything else is neutral, because the + * ROW tint is what carries progress — see `rowStyle`. */ -const statusColors: Record = { +const statusColors: Record = { draft: "outline", sent: "outline", loaded: "outline", po_received: "outline", invoiced: "secondary", completed: "secondary", - cancelled: "outline", + cancelled: "destructive", }; const statusLabel: Record = { @@ -151,15 +136,28 @@ export default function DeliveryNotesPage() { ); const [loading, setLoading] = useState(true); const [error, setError] = useState(""); - const [page, setPage] = useState(1); - const [totalPages, setTotalPages] = useState(1); + // No pagination state. This screen fetches a single wide page (500 + // rows, well over the real DN count) and groups it by company, so + // there is only ever one page. It used to hold `page`/`setPage`/ + // `totalPages` and hand the DataTable `page={1} totalPages={1}` with + // a no-op `onPageChange` — three pieces of state that could never + // do anything. Search and the status filters are client-side over + // the fetched set for the same reason. + // Search term. Applied CLIENT-SIDE over the already-fetched notes + // (which carry customer / company / order expanded), so it covers + // every field the row shows. See `getDeliveryNotes` for why the + // server-side version was a dead end. const [search, setSearch] = useState(""); // List-page filter strip. Defaults to "active" so the operator - // opens the page to a working list; cancelled / completed / - // all are opt-in. Status filter is applied client-side over - // the current page of notes (the server still returns the full - // recent set so search/tab switching doesn't have to re-fetch). + // opens the page to a working list; completed / all are opt-in. + // Applied client-side over the fetched set so switching tabs never + // re-fetches. const [tab, setTab] = useState("active"); + // Sub-filter inside the Completed tab, which holds invoiced + + // completed + cancelled together. Defaults to "both" so the tab's + // count and its rows always agree — a tab promising 40 and showing + // 29 is how a list stops being trusted. + const [doneFilter, setDoneFilter] = useState("both"); // Per-company COLLAPSED state — note the inversion. // // This used to be `openGroups`, initialised to an empty Set, with @@ -336,19 +334,30 @@ export default function DeliveryNotesPage() { const tabCounts = useMemo(() => { const counts: Record = { active: 0, - cancelled: 0, completed: 0, all: notes.length, }; for (const note of notes) { - const bucket = dnTabForStatus(note.status); - if (bucket === "cancelled") counts.cancelled += 1; - else if (bucket === "invoiced") counts.completed += 1; - else counts.active += 1; + if (dnTabForStatus(note.status) === "active") counts.active += 1; + else counts.completed += 1; } return counts; }, [notes]); + // Split inside the done bucket, for the sub-filter's own labels. + // Computed against the full set (not the search-filtered view) so + // the counts describe what's on the server, not what happens to be + // visible right now. + const doneCounts = useMemo(() => { + let completed = 0; + let cancelled = 0; + for (const note of notes) { + if (note.status === "cancelled") cancelled += 1; + else if (isDoneStatus(note.status)) completed += 1; + } + return { completed, cancelled, both: completed + cancelled }; + }, [notes]); + // ── Expandable row: fetch a note's line items on first open. // // The list endpoint returns headers only, so the items live behind @@ -433,12 +442,26 @@ export default function DeliveryNotesPage() { } }, [itemsByDnId, loadingIds]); - // Apply the tab filter. Selection / bulk actions keep working - // because we filter the visible array but keep the underlying - // `notes` untouched. + // The one filtering pass: search, then tab, then the done + // sub-filter. Order matters — search runs against the full set so + // the tab counts stay truthful, and the sub-filter is a no-op + // outside the Completed tab. const visibleNotes = useMemo( - () => notes.filter((note) => TAB_STATUSES[tab].includes(note.status)), - [notes, tab], + () => + notes.filter((note) => { + if (!matchesDeliveryNoteSearch(note, search)) return false; + const bucket = dnTabForStatus(note.status); + if (bucket === "done") { + if (tab === "active") return false; + if (tab === "completed" && !matchesDoneFilter(note.status, doneFilter)) { + return false; + } + } else if (tab === "completed") { + return false; + } + return true; + }), + [notes, search, tab, doneFilter], ); // Derived progress stage per note. Memoised because it feeds both @@ -487,15 +510,17 @@ export default function DeliveryNotesPage() { try { const userType = (user as { user_type?: string[] } | null)?.user_type; const repId = user && isRepUser(userType) ? user.id : null; - // Fetch a wide page so the client-side tab filter sees the - // full set — the previous `perPage=20` made the active - // tab silently drop records past rank 20 (the operator - // saw "no DN" when the row existed but was buried). 500 - // is well over the production DN count (≈30) and still - // cheap for PB. - const result = await getDeliveryNotes(page, 500, search, repId); + // Fetch a wide page so the client-side search / tab / sub-filter + // all see the full set — the previous `perPage=20` made the active + // tab silently drop records past rank 20 (the operator saw "no DN" + // when the row existed but was buried). 500 is well over the + // production DN count (≈30) and still cheap for PB. + // + // No search term is sent: search is client-side now, because the + // server-side version could only see two fields and would have to + // re-fetch on every keystroke to add the rest. + const result = await getDeliveryNotes(1, 500, repId); setNotes(result.items); - setTotalPages(result.totalPages); // Batch-fetch the related quotes in one query so the Q // icon can show "create" vs "open" without a per-row // round trip. Failures here are non-fatal — the Q icon @@ -517,7 +542,7 @@ export default function DeliveryNotesPage() { } finally { setLoading(false); } - }, [page, search, user]); + }, [user]); useEffect(() => { fetchNotes(); @@ -1245,65 +1270,109 @@ export default function DeliveryNotesPage() { {/* Status filter strip. Active is the default landing tab so - the operator opens the page to the working list; - cancelled / completed live one click away and never - collide with the active workflow. Each tab carries a - count badge so the operator can tell at a glance how - many DNs sit in each bucket. */} -
- {(Object.keys(TAB_LABELS) as DnTab[]).map((key) => { - const isActive = key === tab; - return ( - - ); - })} + {TAB_LABELS[key]} + + {tabCounts[key]} + + + ); + })} +
+ + {/* Sub-filter for the done bucket. ALWAYS rendered when the + Completed tab is active, never hidden behind a click — + an invisible control on this screen was the "what does + that do?" complaint in the first place. Each option + carries its own count so the tab total always reconciles + with the sum of the options. */} + {tab === "completed" && ( +
+ {( + [ + { key: "completed", label: "Completed", n: doneCounts.completed }, + { key: "cancelled", label: "Cancelled", n: doneCounts.cancelled }, + { key: "both", label: "Both", n: doneCounts.both }, + ] as const + ).map((opt) => { + const isOn = doneFilter === opt.key; + return ( + + ); + })} +
+ )} - {/* Global search. One input filters across every company - below — operators usually know the DN number / customer - / order, not the company. We use a plain Input here - instead of a full DataTable so the search input doesn't - render its own empty-state banner ("No records found") - that hides the actual grouped list. */} + {/* Search. Filters the already-fetched notes client-side, so it + covers every field the row shows — DN number, order number, + customer, company, the customer's own PO reference, and + notes. The placeholder says so, because the previous one + suggested it only understood a DN number, and it only + searched two fields. Multiple words narrow (every word must + match somewhere on the row). + + Deliberately not a DataTable: that renders its own + "No records found" empty state which would hide the grouped + list. A plain Input also means no round trip per keystroke. */}
{ - setSearch(e.target.value); - setPage(1); - }} + onChange={(e) => setSearch(e.target.value)} className="pl-9 border-warn/40 bg-warn/5 focus-visible:border-warn focus-visible:ring-warn/30" data-testid="dn-search" /> @@ -1318,8 +1387,29 @@ export default function DeliveryNotesPage() { about right now. */} {groupedNotes.length === 0 && !loading ? (
-

No delivery notes on this tab.

-

Switch tabs above or create a new delivery note.

+ {search.trim() ? ( + <> +

+ Nothing matches “{search.trim()}”. +

+

+ Search covers the DN number, order number, customer, + company, customer PO and notes. Try a shorter term, or + switch to the All tab. +

+ + ) : ( + <> +

+ No delivery notes on this tab. +

+

+ {tab === "completed" && doneCounts.cancelled > 0 && doneCounts.completed === 0 + ? "Only cancelled notes are here — try the Cancelled option above." + : "Switch tabs above or create a new delivery note."} +

+ + )}
) : (
diff --git a/src/lib/delivery-note-stage.test.ts b/src/lib/delivery-note-stage.test.ts index 98da50c..091ea89 100644 --- a/src/lib/delivery-note-stage.test.ts +++ b/src/lib/delivery-note-stage.test.ts @@ -4,6 +4,9 @@ import { dnStageTint, dnTabForStatus, hasSignedCopy, + isDoneStatus, + matchesDeliveryNoteSearch, + matchesDoneFilter, } from "./delivery-note-stage"; import type { DeliveryNote, DeliveryNoteStatus } from "./types"; @@ -123,8 +126,138 @@ describe("dnTabForStatus", () => { expect(dnTabForStatus("sent")).toBe("active"); expect(dnTabForStatus("loaded")).toBe("active"); expect(dnTabForStatus("po_received")).toBe("active"); - expect(dnTabForStatus("invoiced")).toBe("invoiced"); - expect(dnTabForStatus("completed")).toBe("invoiced"); - expect(dnTabForStatus("cancelled")).toBe("cancelled"); + }); + + it("folds invoiced, completed AND cancelled into one done bucket", () => { + // Cancelled used to have its own tab. The operator asked for it + // to live under the done heading with a filter between them. + expect(dnTabForStatus("invoiced")).toBe("done"); + expect(dnTabForStatus("completed")).toBe("done"); + expect(dnTabForStatus("cancelled")).toBe("done"); + }); +}); + +describe("matchesDoneFilter", () => { + it("'both' shows every done note", () => { + expect(matchesDoneFilter("invoiced", "both")).toBe(true); + expect(matchesDoneFilter("completed", "both")).toBe(true); + expect(matchesDoneFilter("cancelled", "both")).toBe(true); + }); + + it("'completed' hides cancelled but keeps both finished states", () => { + expect(matchesDoneFilter("invoiced", "completed")).toBe(true); + expect(matchesDoneFilter("completed", "completed")).toBe(true); + expect(matchesDoneFilter("cancelled", "completed")).toBe(false); + }); + + it("'cancelled' shows only cancelled", () => { + expect(matchesDoneFilter("cancelled", "cancelled")).toBe(true); + expect(matchesDoneFilter("completed", "cancelled")).toBe(false); + expect(matchesDoneFilter("invoiced", "cancelled")).toBe(false); + }); + + it("never lets an in-flight status through, whatever the filter", () => { + // Guard: the filter lives INSIDE the done tab, so this should be + // impossible in the UI. If it ever is, we'd show work-in-progress + // in a tab labelled Completed. + for (const f of ["both", "completed", "cancelled"] as const) { + expect(matchesDoneFilter("draft", f)).toBe(false); + expect(matchesDoneFilter("po_received", f)).toBe(false); + } + }); +}); + +describe("isDoneStatus", () => { + it("treats cancelled as done — the work finished, one way or another", () => { + expect(isDoneStatus("cancelled")).toBe(true); + expect(isDoneStatus("completed")).toBe(true); + expect(isDoneStatus("invoiced")).toBe(true); + expect(isDoneStatus("draft")).toBe(false); + expect(isDoneStatus("sent")).toBe(false); + expect(isDoneStatus("loaded")).toBe(false); + expect(isDoneStatus("po_received")).toBe(false); + }); +}); + +const row = (over: Record = {}) => + ({ + delivery_note_number: "DN26071401", + notes: "counter pickup", + customer_po: "PO-9931", + ...over, + }) as Parameters[0]; + +describe("matchesDeliveryNoteSearch", () => { + it("matches the DN number", () => { + expect(matchesDeliveryNoteSearch(row(), "26071401")).toBe(true); + expect(matchesDeliveryNoteSearch(row(), "26071401")).toBe(true); + }); + + it("matches the CUSTOMER name — the case the old server filter missed", () => { + // This is the regression that motivated moving search + // client-side: the old filter only read delivery_note_number and + // notes, so "what's outstanding for Acme" returned nothing on a + // list that is grouped by company. + const n = row({ + expand: { + order: { order_number: "ORD-2607-118" }, + customer: { customer: "Andrew Bester" }, + company: { company: "Acme Bolts" }, + }, + }); + expect(matchesDeliveryNoteSearch(n, "acme")).toBe(true); + expect(matchesDeliveryNoteSearch(n, "bester")).toBe(true); + }); + + it("matches the company and the order number", () => { + const n = row({ + expand: { + order: { order_number: "ORD-2607-118" }, + customer: { customer: "Andrew Bester" }, + company: { company: "Acme Bolts" }, + }, + }); + expect(matchesDeliveryNoteSearch(n, "ORD-2607-118")).toBe(true); + expect(matchesDeliveryNoteSearch(n, "acme bolts")).toBe(true); + }); + + it("matches the customer's own PO reference", () => { + expect(matchesDeliveryNoteSearch(row(), "PO-9931")).toBe(true); + }); + + it("is case-insensitive", () => { + expect(matchesDeliveryNoteSearch(row(), "COUNTER")).toBe(true); + }); + + it("requires EVERY word, so 'acme bolt' narrows rather than 404s", () => { + const n = row({ + expand: { company: { company: "Acme Bolts" } }, + }); + expect(matchesDeliveryNoteSearch(n, "acme bolt")).toBe(true); + expect(matchesDeliveryNoteSearch(n, "acme missing")).toBe(false); + }); + + it("returns everything for an empty or whitespace query", () => { + expect(matchesDeliveryNoteSearch(row(), "")).toBe(true); + expect(matchesDeliveryNoteSearch(row(), " ")).toBe(true); + }); + + it("does not throw on a note with no expansions at all", () => { + // Ad-hoc counter notes can carry a customer but no order, and a + // note mid-creation can have neither. A search box that throws on + // those rows takes the whole list down. + const bare = row(); + expect(() => matchesDeliveryNoteSearch(bare, "anything")).not.toThrow(); + expect(matchesDeliveryNoteSearch(bare, "anything")).toBe(false); + }); + + it("treats a quote in the query as ordinary text, not a filter break", () => { + // The old server-side filter interpolated the term into a PB + // filter string with no escaping, so a double quote produced a + // malformed filter and a list that failed to load. + const n = row({ + expand: { company: { company: 'Acme "Ltd" Bolts' } }, + }); + expect(matchesDeliveryNoteSearch(n, 'acme "ltd"')).toBe(true); }); }); diff --git a/src/lib/delivery-note-stage.ts b/src/lib/delivery-note-stage.ts index fe6b8f9..4790c21 100644 --- a/src/lib/delivery-note-stage.ts +++ b/src/lib/delivery-note-stage.ts @@ -59,14 +59,103 @@ export function hasSignedCopy(note: Pick): boolean } /** - * Which tab a status belongs to. Kept next to `deriveDnStage` so the - * tab buckets and the colour ladder can never disagree about what - * "active" means. + * Sub-filter for the Completed tab. + * + * The operator asked for cancelled and completed to live together + * with a way to narrow between them, rather than as two separate + * tabs. `both` is the default so the tab's count and its contents + * always agree — a tab that says "Completed 40" and then shows 29 + * rows with the other 11 hidden is the sort of thing that makes a + * list untrustworthy. */ -export function dnTabForStatus(status: DeliveryNoteStatus): DnStage | "active" { - if (status === "cancelled") return "cancelled"; - if (status === "invoiced" || status === "completed") return "invoiced"; - return "active"; +export type DoneFilter = "completed" | "cancelled" | "both"; + +/** + * Does a status fall inside the "done" bucket (the Completed tab)? + * Cancelled counts as done: the work is finished, one way or another. + */ +export function isDoneStatus(status: DeliveryNoteStatus): boolean { + return status === "invoiced" || status === "completed" || status === "cancelled"; +} + +/** + * Apply the sub-filter inside the done bucket. + * + * `"both"` means "both FINISHED kinds", not "any status at all" — it + * still refuses an in-flight note. The filter currently only runs + * inside the Completed tab so nothing can leak in practice, but a + * function that would happily return a `draft` for `both` relies on + * its call site staying correct. Guarding here means reusing it + * elsewhere can't quietly put work-in-progress in a done list. + */ +export function matchesDoneFilter( + status: DeliveryNoteStatus, + filter: DoneFilter, +): boolean { + if (!isDoneStatus(status)) return false; + if (filter === "both") return true; + if (filter === "cancelled") return status === "cancelled"; + return status === "invoiced" || status === "completed"; +} + +/** + * Client-side search over a fetched delivery note. + * + * Deliberately matches EVERY field the row displays. The previous + * server-side filter only covered `delivery_note_number` and `notes`, + * so typing a customer or company name — the first thing anyone + * types on a list grouped by company — returned nothing at all. + * + * Matching is case-insensitive substring across each field + * independently, and multi-word queries match if EVERY word appears + * somewhere on the row. That second rule matters more than it looks: + * operators type "acme bolt" meaning *Acme* AND *bolt*, and a plain + * whole-string substring test would find neither, because no single + * field contains "acme bolt". + */ +export function matchesDeliveryNoteSearch( + note: Pick< + DeliveryNote, + "delivery_note_number" | "notes" | "customer_po" + > & { + expand?: { + order?: { order_number?: string | null } | null; + customer?: { customer?: string | null } | null; + company?: { company?: string | null } | null; + }; + }, + query: string, +): boolean { + const q = query.trim().toLowerCase(); + if (!q) return true; + + const haystack = [ + note.delivery_note_number ?? "", + note.notes ?? "", + note.customer_po ?? "", + note.expand?.order?.order_number ?? "", + note.expand?.customer?.customer ?? "", + note.expand?.company?.company ?? "", + ] + .join(" ") + .toLowerCase(); + + // Every word must appear somewhere on the row. + return q.split(/\s+/).every((word) => haystack.includes(word)); +} + +/** + * Which top-level tab a status belongs to. Kept next to + * `deriveDnStage` so the tab buckets and the colour ladder can never + * disagree about what "active" means. + * + * `cancelled` is deliberately NOT its own tab any more — the operator + * asked for it to sit alongside completed under one "done" heading + * with a filter between them, rather than as a third bucket that + * needs a tab of its own for a small, cold set. + */ +export function dnTabForStatus(status: DeliveryNoteStatus): "active" | "done" { + return isDoneStatus(status) ? "done" : "active"; } /** diff --git a/src/lib/pb-helpers.ts b/src/lib/pb-helpers.ts index b5bbefe..17cc65a 100644 --- a/src/lib/pb-helpers.ts +++ b/src/lib/pb-helpers.ts @@ -1065,17 +1065,39 @@ export async function updateCollectionPoolStatus( return updated; } +/** + * Fetch delivery notes for the list page. + * + * ## Why there is no `search` parameter any more + * + * This used to take a search term and push it into a PocketBase + * filter over `delivery_note_number || notes`. Two problems, both + * visible to the operator: + * + * 1. **It missed most of what a row shows.** Customer name, company + * and order number were not searched — and "what's outstanding + * for Acme" is the first thing anyone types on a list that is + * grouped BY company. Those fields only arrive via `expand`, so + * they can't be filtered server-side by this shape of query + * without a join on every row. + * 2. **The term was never escaped.** The `repId` clause on the very + * next line escapes its quotes; the search clause didn't. A + * customer called `Acme "Ltd"` — or anyone typing a stray + * double quote — produced a broken filter literal and a list + * that failed to load. + * + * So the search moved client-side (`matchesDeliveryNoteSearch` in + * `src/lib/delivery-note-stage.ts`), over rows that are already + * fetched WITH their customer / company / order expanded. One code + * path, every visible field, and no way to build a malformed filter. + */ export async function getDeliveryNotes( page = 1, perPage = 50, - search = "", repId: string | null = null, ) { const pb = getPocketBase(); const clauses: string[] = []; - if (search) { - clauses.push(`(delivery_note_number ~ "${search}" || notes ~ "${search}")`); - } if (repId) { clauses.push(`customer.rep = "${repId.replace(/"/g, '\\"')}"`); }