From 80ca4788a22f7aacd4e650bff8c86dd615c28617 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 8 Oct 2026 12:04:42 +0200 Subject: [PATCH] Give external invoices a supplier name; keep them off the sites page The name on an invoice means two different things. On an internal invoice it is a SITE, transcribed from the invoice's own description. On an external invoice it is the SUPPLIER - and the PDF does not print the supplier's company name at all, so there is nothing to transcribe and a human has to type it. `invoices.site` stays the single field; the column header follows the page (Site on Internal, Supplier on External) and only the external one is editable, via components/SupplierCell.tsx and PATCH /api/invoices/:id/site. It saves on blur or Enter, never per keystroke, and an empty value clears the name so a stray description fragment can be removed rather than displayed as a supplier. Sites stop taking their names from external invoices, in both places: - syncSitesFromInvoices only creates sites from internal invoices, so a job description can never be registered as a yard. - buildSiteSummaries only counts internal invoice lines towards a site, which is what /sites and the dashboard's per-site chart read. This is the "Move containers" case. PHI 192's description is "Crane Truck Hire 17 September 2026 Move containers", which had registered a site literally called "Move containers" - a job, not a yard - on /sites, on the dashboard's per-site chart and in the plant register's site filter. The record stays in the collection (sites are never deleted) but is no longer surfaced; the count went 13 -> 12. External income is not lost by this - it is booked to a machine and still appears on /income and /charts. It is simply not a site. The intake route stops resolving an external invoice's text against the site list, which would have invented a site out of a description, and reports needsSupplier so the import toast says the name is outstanding. npm run seed now captures existing invoice names before it wipes the collection and re-applies them where data/source does not specify one - the same data-loss trap the uploaded PDFs already had. A JSON name always wins, so internal sites are unchanged. Verified: typecheck clean, 45 tests pass (4 new, covering the "Move containers" case directly), production build green, the new PATCH route 401s when anonymous, and /sites plus both invoice pages pass the mobile checklist at 375x812 and 768x1024. The supplier edit was exercised end to end and the test value reverted. Still open: the three external invoices (PHI 178/186/192) have no supplier name yet and none can be derived from the data. They show "Set supplier" until the client supplies them. Note: lib/rows.ts, lib/server-data.ts and the running/insurance feature are someone else's in-flight work and were deliberately left uncommitted here. --- AGENTS.md | 30 ++++++ app/api/invoices/[id]/site/route.ts | 62 ++++++++++++ app/api/invoices/intake/route.ts | 31 ++++-- components/InvoiceIntake.tsx | 5 +- components/InvoiceList.tsx | 40 ++++++-- components/InvoiceRegisterPage.tsx | 7 +- components/SupplierCell.tsx | 146 ++++++++++++++++++++++++++++ lib/sites.test.ts | 67 +++++++++++++ lib/sites.ts | 27 ++++- scripts/seed.mjs | 20 +++- 10 files changed, 411 insertions(+), 24 deletions(-) create mode 100644 app/api/invoices/[id]/site/route.ts create mode 100644 components/SupplierCell.tsx create mode 100644 lib/sites.test.ts diff --git a/AGENTS.md b/AGENTS.md index b274713..d99fcca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,6 +79,36 @@ Three consequences that are easy to get wrong: it is the supplier's own wording transcribed from the PDF, **not** this internal/external classification. Never derive the split from it. +### The name on an invoice means two different things + +`invoices.site` is a **SITE** on an internal invoice and the **SUPPLIER** on an external +one. Same field, different meaning — so the column header follows the page (`Site` on +Internal, `Supplier` on External) and only the external one is editable. + +- **Internal** — transcribed from the invoice's description and folded against the existing + sites by `resolveSiteName`. Read-only in the UI; changing it would move real site data. +- **External** — **the PDF does not print the supplier's company name**, so there is nothing + to transcribe. A human types it on `/income/external-invoices` (`components/SupplierCell.tsx`, + `PATCH /api/invoices/:id/site`). Saves on blur or Enter, never per keystroke; an empty + value **clears** the name. + +The three external invoices are seeded that way and their names are **not yet known** — ask +the client rather than inventing them. Until then the page shows `Set supplier`. + +⚠️ **`syncSitesFromInvoices` and `buildSiteSummaries` only ever consider INTERNAL invoices.** +This is what keeps the sites page honest. PHI 192's description is "Crane Truck Hire 17 +September 2026 Move containers", which had registered a site literally called **"Move +containers"** — a job description, not a yard — on `/sites`, on the dashboard's per-site +chart and in the plant register's site filter. The site record is left in the collection +(never delete — see above); it is simply no longer surfaced, and the count went 13 → 12. + +External income is **not** lost by this: it is booked to a machine and still appears on +`/income` and `/charts`. It is simply not a site. + +The plant register's site dropdown still reads the `sites` collection directly, so "Move +containers" remains selectable there. That is deliberate — a container genuinely can be +parked for a move job — but change it if the client disagrees. + ## Invoice booking — run order matters `npm run seed` **wipes `monthly_income`** and re-imports only the workbook, so invoice-derived diff --git a/app/api/invoices/[id]/site/route.ts b/app/api/invoices/[id]/site/route.ts new file mode 100644 index 0000000..d3c47b0 --- /dev/null +++ b/app/api/invoices/[id]/site/route.ts @@ -0,0 +1,62 @@ +import { revalidatePath } from "next/cache"; +import { serverAuth } from "@/lib/pb"; +import { requireUser } from "@/lib/auth"; +import { revalidateInvoicePages } from "@/lib/invoice-revalidate"; +import { isInternalParty } from "@/lib/invoice-party"; +import type { Invoice } from "@/lib/data"; + +export const dynamic = "force-dynamic"; + +const MAX_NAME = 120; + +// PATCH /api/invoices/:id/site +// +// The "site" text on an invoice means a SITE on an internal invoice and the SUPPLIER on +// an external one — see lib/invoice-party-policy.json. Nothing in the PDF gives us the +// supplier's name, so it is typed in here and stored in the same field. +// +// Changing it can move an invoice between the two pages of the register, because the +// split is by the debtor and not by this name — so this route revalidates BOTH halves. +// The sites pages are refreshed too because an internal invoice's name is what the site +// is derived from. +export async function PATCH(req: Request, ctx: { params: Promise<{ id: string }> }) { + try { + await requireUser(); + const { id } = await ctx.params; + + let name: unknown; + try { + const body = await req.json(); + name = body?.site; + } catch { + return Response.json({ ok: false, error: "Expected a JSON body" }, { status: 400 }); + } + if (typeof name !== "string") { + return Response.json({ ok: false, error: "Send the name as a string" }, { status: 400 }); + } + + // Trimmed and capped: an empty string CLEARS the name, which is how a mis-parsed + // description fragment ("Move containers") is removed rather than displayed as a + // supplier. The description itself is untouched, so nothing is lost. + const clean = name.trim().slice(0, MAX_NAME); + + const pb = await serverAuth(); + const invoice = await pb.collection("invoices").getOne(id); + await pb.collection("invoices").update(id, { site: clean }); + + revalidateInvoicePages(); + // A site on an internal invoice is derived from this name, so /sites can move either + // way. These are cheap and the asymmetry is expensive to miss. + revalidatePath("/sites"); + revalidatePath("/"); + + return Response.json({ + ok: true, + site: clean, + // Which page this invoice belongs to, so the caller can say where it landed. + isInternal: isInternalParty(invoice), + }); + } catch (e: any) { + return Response.json({ ok: false, error: e?.message || "Could not save the name" }, { status: 500 }); + } +} \ No newline at end of file diff --git a/app/api/invoices/intake/route.ts b/app/api/invoices/intake/route.ts index b2b74be..0648122 100644 --- a/app/api/invoices/intake/route.ts +++ b/app/api/invoices/intake/route.ts @@ -2,6 +2,7 @@ import { serverAuth } from "@/lib/pb"; import { revalidateInvoicePages } from "@/lib/invoice-revalidate"; import { parseInvoicePdf, ParseError, registerCodeFrom } from "@/lib/invoice-parser"; import { syncSitesFromInvoices, findSiteContainer, resolveSiteName } from "@/lib/sites"; +import { partyOf } from "@/lib/invoice-party"; export const dynamic = "force-dynamic"; @@ -124,14 +125,24 @@ export async function POST(req: Request) { ); } - // The supplier abbreviates the site on the invoice ("…Sep 2026 Cosmo" for the site the - // register calls "Cosmo City"). Resolve it BEFORE writing, because sites are derived - // from the invoice wording — saving the abbreviation would create a second "Cosmo" and - // split this customer across two sites on /sites. - parsed.site = resolveSiteName( - parsed.site, - [...new Set(allInvoices.map((i: any) => i.site).filter((s: unknown): s is string => typeof s === "string" && !!s.trim()))], - ); + // An INTERNAL invoice's site is transcribed from its description, so the supplier + // abbreviates it ("…Sep 2026 Cosmo" for the register's "Cosmo City"). Resolve it BEFORE + // writing, because sites are derived from the invoice wording — saving the abbreviation + // would create a second "Cosmo" and split that customer across two sites on /sites. + // + // An EXTERNAL invoice's text is not a site at all — it is the supplier, and the + // description gives no company name. Folding it against the site list would invent a + // site out of a job description, so its name is left empty for a human to type on + // /income/external-invoices. See lib/invoice-party-policy.json. + const external = partyOf(parsed) === "external"; + if (external) { + parsed.site = ""; + } else { + parsed.site = resolveSiteName( + parsed.site, + [...new Set(allInvoices.map((i: any) => i.site).filter((s: unknown): s is string => typeof s === "string" && !!s.trim()))], + ); + } const periodEnd = new Date(parsed.period_to); const bookedMonth = periodEnd.getUTCMonth() + 1; @@ -259,6 +270,10 @@ export async function POST(req: Request) { invoice_no: parsed.invoice_no, invoiceId: invoiceRec.id, site: parsed.site, + // An external invoice landed on /income/external-invoices and has no name yet — the + // PDF does not print the supplier's company name. Said plainly rather than left to be + // discovered as a blank column. + needsSupplier: external, total_incl: parsed.total_incl, lines: lines.length, booked: lines.filter((l: any) => l.booked_asset).length, diff --git a/components/InvoiceIntake.tsx b/components/InvoiceIntake.tsx index 57c8b64..6e56198 100644 --- a/components/InvoiceIntake.tsx +++ b/components/InvoiceIntake.tsx @@ -93,7 +93,10 @@ export function AddInvoiceButton() { ? ` — needs a plant item: ${unassigned.join(", ")}` : data.fullyBooked ? " · all lines matched" - : ""), + : "") + + // An external invoice has no supplier name until someone types it. Naming + // that here beats leaving the reader to find an empty column. + (data.needsSupplier ? " · add the supplier on External Invoices" : ""), }); reset(); setOpen(false); diff --git a/components/InvoiceList.tsx b/components/InvoiceList.tsx index e53d636..7d0c37b 100644 --- a/components/InvoiceList.tsx +++ b/components/InvoiceList.tsx @@ -1,7 +1,8 @@ "use client"; import { useEffect, useRef, useState } from "react"; -import { ChevronRight, AlertTriangle, CheckCircle2, MapPin, FileText } from "lucide-react"; +import { ChevronRight, AlertTriangle, CheckCircle2, FileText } from "lucide-react"; +import { SupplierCell, ReadOnlyName } from "@/components/SupplierCell"; import type { Asset, BookedLine, Invoice } from "@/lib/data"; import { BOOK_INCLUSIVE_VAT, BOOKED_LONG_LABEL } from "@/lib/invoice-booking"; import { ZAR, ZAR2 } from "@/lib/utils"; @@ -18,12 +19,22 @@ export function InvoiceList({ invoices, linesByInvoice, assets, + partyScope = "internal", }: { invoices: Invoice[]; linesByInvoice: Record; assets: Asset[]; + /** Which half of the register this is. It decides what the second column means: a SITE + * on the internal page, the SUPPLIER on the external one. The same `site` field backs + * both, but the external name is typed in by a human — see components/SupplierCell. */ + partyScope?: "internal" | "external"; }) { const [open, setOpen] = useState(null); + // Names edited in place this session, keyed by invoice id. Held here so a supplier + // rename shows the moment it saves instead of waiting for the server round trip to + // replace this client tree. + const [names, setNames] = useState>({}); + const nameFor = (inv: Invoice) => names[inv.id] ?? inv.site ?? ""; // The expanded detail lives in a of THIS scrollable table, and it holds its own // wide line-item table. Without a width of its own, that inner table's min-width @@ -108,7 +119,9 @@ export function InvoiceList({ Invoice - Site + + {partyScope === "external" ? "Supplier" : "Site"} + Period Lines Revenue ex VAT @@ -135,6 +148,9 @@ export function InvoiceList({ setNames((m) => ({ ...m, [inv.id]: n }))} lineCount={lines.length} unbooked={unbooked.length} hasProblem={hasProblem} @@ -164,10 +180,13 @@ export function InvoiceList({ } function InvoiceRow({ - invoice, lineCount, unbooked, + invoice, name, partyScope, onNameSaved, lineCount, unbooked, hasProblem, isOpen, onToggle, expanded, detailW, }: { invoice: Invoice; + name: string; + partyScope: "internal" | "external"; + onNameSaved: (name: string) => void; lineCount: number; unbooked: number; hasProblem: boolean; @@ -207,14 +226,15 @@ function InvoiceRow({ - - {invoice.site ? ( - - - {invoice.site} - + + {partyScope === "external" ? ( + // External: the name is a SUPPLIER and is not in the PDF, so it is typed in. + ) : ( - — + // Internal: the name is a SITE transcribed from the invoice's own description. + + + )} diff --git a/components/InvoiceRegisterPage.tsx b/components/InvoiceRegisterPage.tsx index e84d477..dfea17b 100644 --- a/components/InvoiceRegisterPage.tsx +++ b/components/InvoiceRegisterPage.tsx @@ -109,7 +109,12 @@ export async function InvoiceRegisterPage({ scope }: { scope: PartyScope }) { {invoices.length === 0 ? ( ) : ( - + )} {unclassified > 0 && ( diff --git a/components/SupplierCell.tsx b/components/SupplierCell.tsx new file mode 100644 index 0000000..244f82a --- /dev/null +++ b/components/SupplierCell.tsx @@ -0,0 +1,146 @@ +"use client"; + +import { useEffect, useRef, useState } from "react"; +import { useRouter } from "next/navigation"; +import { Building2, Check, Loader2, Pencil } from "lucide-react"; +import { cn } from "@/lib/utils"; + +// The supplier name on an EXTERNAL invoice, edited in place. +// +// On an internal invoice the same field means a site, and that name is transcribed from +// the invoice's own description — so it is shown read-only. On an external invoice the +// description is not a company name at all ("Crane Truck Hire Sep 2026 Springs" yields +// "Springs"), so there is nothing to transcribe and a human has to type it. +// +// Saves on blur and on Enter, never on every keystroke: this is one field among many on a +// wide table, and a request per character would be both slow and impossible to undo. An +// empty value CLEARS the name — that is how a stray description fragment is removed. +export function SupplierCell({ + invoiceId, + value, + onSaved, +}: { + invoiceId: string; + value: string; + /** Called with the saved name so the table can show it immediately, before the refresh. */ + onSaved?: (name: string) => void; +}) { + const [draft, setDraft] = useState(value); + const [editing, setEditing] = useState(false); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + const inputRef = useRef(null); + const router = useRouter(); + + // The row is re-rendered from the server after a refresh, so a rename made elsewhere + // must not be overwritten by a stale local draft. + useEffect(() => { + if (!editing) setDraft(value); + }, [value, editing]); + + useEffect(() => { + if (editing) inputRef.current?.focus(); + }, [editing]); + + const save = async (next: string) => { + const clean = next.trim(); + if (clean === value.trim()) { + setEditing(false); + setDraft(value); + return; + } + setBusy(true); + setError(null); + try { + const res = await fetch(`/api/invoices/${invoiceId}/site`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ site: clean }), + }); + const data = await res.json().catch(() => ({ ok: false })); + if (!res.ok || !data.ok) { + setError(data.error || "Could not save"); + return; + } + setDraft(data.site); + setEditing(false); + onSaved?.(data.site); + router.refresh(); + } catch { + setError("Could not reach the server"); + } finally { + setBusy(false); + } + }; + + if (editing) { + return ( +
+ setDraft(e.target.value)} + onBlur={() => save(draft)} + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + save(draft); + } else if (e.key === "Escape") { + // Abandons the edit; a save already in flight is left to finish. + e.preventDefault(); + setDraft(value); + setEditing(false); + } + }} + disabled={busy} + aria-label="Supplier name" + placeholder="Supplier name" + maxLength={120} + className="w-[150px] min-h-[44px] px-2 text-sm rounded-lg border border-[#30638E] bg-white dark:bg-surface text-[#003D5B] dark:text-white focus:outline-none focus:ring-2 focus:ring-[#30638E]/40" + /> + {busy ? ( + + ) : ( +
+ ); + } + + return ( + + ); +} + +/** The read-only counterpart, used on the internal page where the name IS a site. */ +export function ReadOnlyName({ value }: { value: string }) { + if (!value) return —; + return ( + + + {value} + + ); +} \ No newline at end of file diff --git a/lib/sites.test.ts b/lib/sites.test.ts new file mode 100644 index 0000000..fd981b1 --- /dev/null +++ b/lib/sites.test.ts @@ -0,0 +1,67 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { buildSiteSummaries } from "./sites"; +import type { Asset, BookedLine, Invoice, Site } from "./data"; + +const site = (name: string) => ({ id: `s-${name}`, name }) as Site; + +// Internal = billed to Mega Infrastructure (09MIS001). External = a third-party debtor +// number, which is all three of PHI 178/186/192 actually carry. +const internal = (over: Partial = {}) => + ({ id: `inv-${Math.random()}`, invoice_no: "PHI 166", invoice_date: "2026-09-18", + debtor: "Mega Infrastructure", debtor_no: "09MIS001", site: "West Park", + total_excl: 100, total_incl: 115, vat: 15, period_to: "2026-09-18", + booked_month: 9, booked_year: 2026, ...over }) as Invoice; + +const external = (over: Partial = {}) => + ({ ...internal(over), debtor: "", debtor_no: "09DTE001" }) as Invoice; + +const line = (invoice: string, over: Partial = {}) => + ({ id: `l-${Math.random()}`, invoice, amount_excl: 100, vat: 15, total_incl: 115, + booked_asset: "", booked_site: "", machine_code: "CT01", isBooked: true, ...over }) as BookedLine; + +const summaries = (sites: Site[], invoices: Invoice[], lines: BookedLine[]) => + buildSiteSummaries(sites, invoices, lines, [] as Asset[]); + +test("an internal invoice's site is summarised with its money", () => { + const inv = internal(); + const [row] = summaries([site("West Park")], [inv], [line(inv.id)]); + assert.ok(row, "West Park should have a row"); + assert.equal(row.name, "West Park"); + assert.equal(row.revenueIncl, 115); + assert.equal(row.lineCount, 1); +}); + +test("an external invoice does NOT put its name on the sites page", () => { + // The real case: PHI 192's description "Crane Truck Hire 17 September 2026 Move + // containers" produced a site literally called "Move containers". An external invoice + // is not billed to a place, so its name is the supplier and must not become a site row. + const inv = external({ invoice_no: "PHI 192", site: "Move containers" }); + const rows = summaries([site("Move containers")], [inv], [line(inv.id)]); + assert.deepEqual(rows, [], "an external-only site must not appear on /sites"); +}); + +test("a site with both internal and external lines keeps only the internal money", () => { + const internalInv = internal({ site: "Castle Gate" }); + const externalInv = external({ invoice_no: "PHI 186", site: "Castle Gate" }); + const rows = summaries( + [site("Castle Gate")], + [internalInv, externalInv], + [line(internalInv.id, { total_incl: 115 }), line(externalInv.id, { total_incl: 500 })], + ); + assert.equal(rows.length, 1); + // 500 of external billing must not inflate the site's revenue. + assert.equal(rows[0].revenueIncl, 115); + assert.equal(rows[0].lineCount, 1); +}); + +test("the sites total cannot silently drop an internal site", () => { + // Guards the filter against over-reaching: internal sites must all survive. + const invoices = ["West Park", "Cosmo City", "The Hills"].map((s) => internal({ site: s })); + const rows = summaries( + ["West Park", "Cosmo City", "The Hills"].map(site), + invoices, + invoices.map((i) => line(i.id)), + ); + assert.deepEqual(rows.map((r) => r.name).sort(), ["Cosmo City", "The Hills", "West Park"]); +}); \ No newline at end of file diff --git a/lib/sites.ts b/lib/sites.ts index 8c07e98..7b12b7f 100644 --- a/lib/sites.ts +++ b/lib/sites.ts @@ -9,14 +9,28 @@ const EMPTY_INVOICE = { debtor: "", debtor_no: "" } as Pick { const existing = (await pb.collection("sites").getFullList()) as Site[]; const byName = new Map(existing.map((s: Site) => [(s.name || "").toLowerCase(), s])); - const wanted = [...new Set(invoices.map((i) => (i.site || "").trim()).filter(Boolean))]; + const wanted = [ + ...new Set( + invoices + .filter((i) => isInternalParty(i)) + .map((i) => (i.site || "").trim()) + .filter(Boolean), + ), + ]; for (const name of wanted) { if (byName.has(name.toLowerCase())) continue; try { @@ -113,6 +127,11 @@ function invoiceMonth(inv: Invoice): { year: number; month: number } { } // Per-site revenue and VAT, derived by walking invoice lines through their invoice. +// +// ONLY INTERNAL invoices count towards a site. An external invoice's "site" is its +// supplier, not a place, so letting it in here is what put "Move containers" on /sites as +// if it were a yard. Its income is not lost — it is booked to a machine and still shows on +// /income and /charts; it simply is not a site. export function buildSiteSummaries( sites: Site[], invoices: Invoice[], @@ -124,7 +143,9 @@ export function buildSiteSummaries( return sites .map((site) => { - const siteLines = lines.filter((l) => invById.get(l.invoice)?.site === site.name); + const siteLines = lines.filter( + (l) => invById.get(l.invoice)?.site === site.name && isInternalParty(invById.get(l.invoice)!), + ); const revExcl = siteLines.reduce((s, l) => s + (l.amount_excl || 0), 0); const vat = siteLines.reduce((s, l) => s + (l.vat || 0), 0); const incl = siteLines.reduce((s, l) => s + (l.total_incl || 0), 0); diff --git a/scripts/seed.mjs b/scripts/seed.mjs index 7c31137..07c3d1f 100644 --- a/scripts/seed.mjs +++ b/scripts/seed.mjs @@ -74,6 +74,22 @@ console.log(`→ Connecting to ${PB} as ${EMAIL}`); await pb.collection("_superusers").authWithPassword(EMAIL, PASS); // Part 5.3: _superusers, not admins console.log(" ✓ authenticated"); +// Names a human typed into the app are NOT in data/source, so a re-seed would otherwise +// silently erase them — the same trap as an uploaded PDF. Captured BEFORE the wipe below, +// keyed by invoice_no, and re-applied only where the JSON itself specifies no name (an +// external invoice's supplier is typed in; an internal site's name comes from the JSON and +// always wins). +const preservedNames = new Map(); +try { + for (const r of await pb.collection(INVOICES_COLL).getFullList()) { + const name = (r.site || "").trim(); + if (r.invoice_no && name) preservedNames.set(r.invoice_no, name); + } +} catch { + // No invoices collection yet — nothing to preserve. +} +console.log(` ✓ captured ${preservedNames.size} existing invoice name(s) to preserve`); + // Wipe existing rows so we can re-seed cleanly for (const c of [ASSETS_COLL, INCOME_COLL, RUN_COLL, OVERHEADS_COLL, COST_LINES_COLL, INVOICE_LINES_COLL, INVOICES_COLL]) { const items = await pb.collection(c).getFullList(); @@ -400,7 +416,9 @@ for (const inv of invoices) { period_from: inv.period_from, period_to: inv.period_to, description: inv.description, - site: inv.site || "", + // The JSON name is authoritative when it has one. Otherwise carry over whatever + // was typed into the app before this seed ran. + site: (inv.site || preservedNames.get(inv.invoice_no) || "").trim(), section: inv.section, category: inv.category, currency: inv.currency,