docs: non-invoiceable extras design + plan, quantity-handling audit
Three documents behind the two features on this branch: - specs/2026-08-30-non-invoiceable-order-extras-design.md — the design the implementation followed, including why the flag must be named for the exception (a positively-named field defaults to false and would mark every existing row non-billable the moment it is created). - plans/2026-08-30-non-invoiceable-order-extras.md — the task-by-task plan, now fully implemented. - specs/2026-08-30-quantity-handling-audit.md — the order_qty / quantity audit that produced the preceding fix, plus the partial collection findings that are still open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
9996923bf3
commit
4d95b7af8c
3 changed files with 665 additions and 0 deletions
|
|
@ -0,0 +1,260 @@
|
|||
# Non-Invoiceable Order Extras — Implementation Plan
|
||||
|
||||
> **For agentic workers:** Use `superpowers:subagent-driven-development` or
|
||||
> `superpowers:executing-plans` to implement this plan task-by-task. Steps use
|
||||
> checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** The operator can add extra line items to an order at any stage and
|
||||
mark them "don't invoice". Such lines get POs, drivers and collection tracking
|
||||
like any other line, and count toward order completion — but never appear on a
|
||||
customer invoice or delivery note.
|
||||
|
||||
**Spec:** `docs/superpowers/specs/2026-08-30-non-invoiceable-order-extras-design.md`
|
||||
|
||||
**Tech Stack:** Next.js 16 (App Router), TypeScript, PocketBase (via
|
||||
`getAdminPocketBase()`), Vitest, Tailwind + shadcn/ui, lucide-react.
|
||||
|
||||
---
|
||||
|
||||
## Pre-flight: verified before this plan was written
|
||||
|
||||
Do not re-investigate these.
|
||||
|
||||
- **The n8n QuickBooks workflow needs no changes.** Its "Build PocketBase
|
||||
Invoice Update" node maps over `order_item_ids` and patches each row with
|
||||
`item_status: 'invoiced'` + `order_item_details.invoice`. The QuickBooks
|
||||
lines come from the separate `items` array. Verified in
|
||||
`n8n_workfows/1 - WFDASH QUICKBOOKS INVOICE1 - WFDASH QUICKBOOKS INVOICE.json`.
|
||||
- **Nothing in the Next app flips `item_status` to `"invoiced"`.** That write
|
||||
is n8n's alone. Do not add an app-side flip.
|
||||
- **Both picking-slip routes need no changes.** They query PB by document
|
||||
identity (`print-bundle/route.ts:88`, `delivery-note-print/route.ts:92`), not
|
||||
by the submitted row list, so excluded rows are picked up automatically.
|
||||
Task 8 adds a regression test to lock this in.
|
||||
- **`selling_price` has `min: 0`** in the PB schema, so `0` is a legal value.
|
||||
- **Tests:** `npm run test:run`. Dev server is on **port 4001**, not 3000.
|
||||
- **`vitest.config.ts` has no `clearMocks`** — mock history leaks between
|
||||
tests. Reset explicitly or you get green-but-vacuous tests.
|
||||
|
||||
## File structure
|
||||
|
||||
| File | Responsibility |
|
||||
|---|---|
|
||||
| PocketBase admin *(manual)* | New `order_items.exclude_from_invoice` bool. **Task 0 — blocks everything.** |
|
||||
| `src/lib/types.ts` *(modify)* | `OrderItem.exclude_from_invoice?: boolean`. |
|
||||
| `src/lib/server/orders-service.ts` *(modify)* | Drop the pending gate; accept the flag in the create draft. |
|
||||
| `src/app/(app)/orders/[id]/process/page.tsx` *(modify)* | Add-item form; margin-gate bypass; excluded-row styling. |
|
||||
| `src/app/(app)/invoicing/view-model.ts` *(modify)* | Split `order_item_ids` from `items` in `buildQbInvoicePayload`. |
|
||||
| `src/app/(app)/invoicing/page.tsx` *(modify)* | Muted row + locked checkbox + all-excluded guard, in **both** modals. |
|
||||
| `src/app/api/invoicing/delivery-note/route.ts` *(modify)* | Excluded rows flip status but drop out of `lines:`. |
|
||||
| `src/app/api/mcp/route.ts` *(modify)* | `order_items.create` accepts the flag. |
|
||||
|
||||
## Phase boundaries
|
||||
|
||||
- **Phase 1 — Tasks 0–4.** Creating and pricing an excluded line. Ends with the
|
||||
operator able to add extras that survive the whole operational flow.
|
||||
- **Phase 2 — Tasks 5–9.** Keeping them off customer paperwork. Depends on
|
||||
Phase 1.
|
||||
|
||||
---
|
||||
|
||||
## Task 0 — PocketBase field (BLOCKS EVERYTHING)
|
||||
|
||||
**This is a manual step the user performs in the PB admin UI.** Per `AGENTS.md`,
|
||||
schema changes to PocketBase are not self-service; the API returns an opaque
|
||||
400/500 if code writes a field that doesn't exist yet.
|
||||
|
||||
- [ ] Ask the user to add to the `order_items` collection:
|
||||
- Name: `exclude_from_invoice`
|
||||
- Type: **Bool**
|
||||
- Required: **no**
|
||||
- [ ] Confirm it exists before starting Task 1. Do not proceed on assumption.
|
||||
|
||||
**Do not invert the name.** A positively-named flag (`bill_customer`,
|
||||
`invoiceable`) defaults to `false` and would mark every existing row in the
|
||||
database non-billable the moment the field is created.
|
||||
|
||||
Optionally refresh `pb-exports/pb_schema-2.json`, but remember it is a
|
||||
reference snapshot — the running instance is the source of truth.
|
||||
|
||||
## Task 1 — Type the field
|
||||
|
||||
- [ ] Add to the `OrderItem` interface in `src/lib/types.ts` (interface starts
|
||||
at line 782), near `surplus_qty` / `ready_for_collection`:
|
||||
|
||||
```ts
|
||||
/**
|
||||
* When true this line never reaches a customer-facing document. It
|
||||
* still gets a PO, a driver, a collection and a `delivered` status —
|
||||
* it is simply dropped from the QuickBooks `items` array and the
|
||||
* delivery-note PDF lines. Used for supplier-side component splits:
|
||||
* the customer approved "bolt, nut and washer" as one priced line,
|
||||
* but we must raise three PO lines to actually buy it.
|
||||
*
|
||||
* Named for the exception, not the norm: the PB default of `false`
|
||||
* must mean "bill this normally".
|
||||
*/
|
||||
exclude_from_invoice?: boolean;
|
||||
```
|
||||
|
||||
- [ ] `npx tsc --noEmit` passes.
|
||||
|
||||
## Task 2 — Allow adding lines mid-flight
|
||||
|
||||
`src/lib/server/orders-service.ts`.
|
||||
|
||||
- [ ] Add `exclude_from_invoice?: boolean` to `CreateOrderItemForOrderDraft`
|
||||
(line 3867).
|
||||
- [ ] **Delete the pending gate** in `createOrderItemForOrder` — the
|
||||
`nonPending` query and the `if (nonPending.length > 0)` throw at line
|
||||
3939. Replace the "Strict `pending` gate" doc comment above the function
|
||||
with an explanation of why adding a row alongside an in-flight PO is
|
||||
safe: the new row doesn't mutate the existing PO, it gets its own.
|
||||
- [ ] There is a second, unrelated `nonPending` gate at line 3744 in
|
||||
`setOrderQuote`. **Leave it alone.**
|
||||
- [ ] In the secondary-field patch block, forward the flag. When
|
||||
`exclude_from_invoice` is true, force `selling_price` to `0` regardless
|
||||
of what the caller passed — the flag is authoritative.
|
||||
- [ ] Keep the `orderLocked` field in the return type for API compatibility;
|
||||
it is now always `false`.
|
||||
|
||||
**Tests** in `orders-service.test.ts`:
|
||||
|
||||
- [ ] Adding a line to an order whose other items are `po_generated` succeeds
|
||||
(previously threw 409).
|
||||
- [ ] `exclude_from_invoice: true` persists and forces `selling_price` to 0
|
||||
even when the caller passes a non-zero price.
|
||||
- [ ] Omitting the flag leaves it falsy and preserves existing behaviour.
|
||||
|
||||
## Task 3 — Add-item UI on the process page
|
||||
|
||||
`src/app/(app)/orders/[id]/process/page.tsx`. The empty-state copy at line
|
||||
1684 ("No line items on this order. Add items before processing.") currently
|
||||
points at an action that doesn't exist — this task makes it true.
|
||||
|
||||
- [ ] Add an "Add item" button below the item card stack, opening a small form
|
||||
(reuse the existing `Sheet` pattern used by `PriceEditSheet`).
|
||||
- [ ] Fields: description (required), quantity, cost price, selling price,
|
||||
supplier, notes.
|
||||
- [ ] A `Checkbox` labelled **"Don't invoice this line"**, with helper text:
|
||||
*"Counts for the order and gets collected, but never appears on the
|
||||
customer's invoice or delivery note."*
|
||||
- [ ] When ticked: force the selling-price input to `0`, disable it, and show
|
||||
"Not invoiced" in place of the margin chip.
|
||||
- [ ] POST to the existing per-id create surface; refresh `items` on success.
|
||||
- [ ] Existing rows with `exclude_from_invoice` render with a muted "Not
|
||||
invoiced" badge in the card stack.
|
||||
|
||||
**Verification:** dev server on **port 4001**. Add an extra to an order that
|
||||
already has a `po_generated` line and confirm it saves without a 409.
|
||||
|
||||
## Task 4 — Bypass the margin gate (HIGHEST RISK)
|
||||
|
||||
`calculateOrderItemMarkup(cost, 0)` returns `-100`, well under
|
||||
`MARKUP_APPROVAL_THRESHOLD` (22, declared at `process/page.tsx:246`). Without
|
||||
this task every extra lands in `approval_needed` and wedges the order.
|
||||
|
||||
- [ ] At `process/page.tsx:643`, where the save path tests
|
||||
`computedMarkup < MARKUP_APPROVAL_THRESHOLD`, short-circuit when the item
|
||||
has `exclude_from_invoice` — skip the approval request entirely and save
|
||||
straight through.
|
||||
- [ ] Suppress the low-margin colouring for excluded rows (`LOW_MARGIN_THRESHOLD`
|
||||
at line 247) — a red "-100%" chip on every extra is noise.
|
||||
- [ ] Also guard the server side: `requestOrderItemMarginApproval`
|
||||
(`src/lib/server/order-item-margin-approval-service.ts:128`) should
|
||||
refuse with a clear error if asked to gate an excluded item, so the MCP
|
||||
path can't wedge an order either.
|
||||
|
||||
**Tests:**
|
||||
|
||||
- [ ] An excluded item with cost 50 / selling 0 saves without entering
|
||||
`approval_needed`.
|
||||
- [ ] A **non**-excluded item with the same numbers still triggers approval —
|
||||
proves the bypass is keyed on the flag, not on the price being zero.
|
||||
|
||||
## Task 5 — Split the QuickBooks payload
|
||||
|
||||
`src/app/(app)/invoicing/view-model.ts`.
|
||||
|
||||
- [ ] Add `excludeFromInvoice?: boolean` to `InvoicingRow` (line 4).
|
||||
- [ ] In `buildQbInvoicePayload` (line 393), keep `order_item_ids` built from
|
||||
**all** rows, and build `items` from **only** rows where
|
||||
`excludeFromInvoice` is not true.
|
||||
- [ ] Comment the split at the call site — the asymmetry looks like a bug
|
||||
otherwise. State that `order_item_ids` drives n8n's status write and
|
||||
`items` drives the QuickBooks lines.
|
||||
- [ ] Export a small helper (e.g. `billableRows(rows)`) so the modals and the
|
||||
guard in Task 6 share one definition of "billable".
|
||||
- [ ] In `src/app/(app)/invoicing/page.tsx`, map the new PB field into
|
||||
`InvoicingRow`: add `exclude_from_invoice` to both the `RawInvoicingRow`
|
||||
type (line 42) and the `INVOICING_FIELDS` projection (line 77). **The
|
||||
projection is load-bearing** — omit it and PB returns the field as
|
||||
`undefined`, silently invoicing every extra.
|
||||
|
||||
**Tests** in `view-model.test.ts`:
|
||||
|
||||
- [ ] Mixed set: excluded ids present in `order_item_ids`, absent from `items`.
|
||||
- [ ] All-billable set produces identical output to today (regression).
|
||||
- [ ] All-excluded set produces an empty `items` array, so Task 6's guard has
|
||||
something to detect.
|
||||
|
||||
## Task 6 — Invoicing UI
|
||||
|
||||
`src/app/(app)/invoicing/page.tsx`. **There are two modals** — single-order
|
||||
(~line 576) and bulk (~line 965) — each with its own `includedRowIds` state.
|
||||
Apply every change to both.
|
||||
|
||||
- [ ] Excluded rows render muted, with a "Not invoiced" chip.
|
||||
- [ ] Their include-checkbox is checked and **disabled** — the row must ride
|
||||
along for the status flip, and the operator must not be able to remove
|
||||
it or add it to the bill.
|
||||
- [ ] Exclude them from any invoice-total display in the modal.
|
||||
- [ ] **Guard:** if the billable subset is empty, disable submit and toast
|
||||
"This order has no billable lines." Apply to `sendToQuickBooks`
|
||||
(line ~1892) and `submitDeliveryNote` (line ~1980). The existing
|
||||
`noneIncluded` checks are about the operator's manual unticking and are
|
||||
not sufficient.
|
||||
|
||||
## Task 7 — Delivery note
|
||||
|
||||
`src/app/api/invoicing/delivery-note/route.ts`.
|
||||
|
||||
- [ ] Leave the status-flip loop at line 230 **untouched** — excluded rows must
|
||||
receive `item_status: "delivery_note"` and the DN number, or they strand.
|
||||
- [ ] At line 362, build `lines:` from billable items only.
|
||||
- [ ] Guard: if no billable items remain, 400 with "No billable lines for a
|
||||
delivery note" rather than rendering an empty PDF.
|
||||
- [ ] Fetch `exclude_from_invoice` in the `getFullList` at line 212.
|
||||
|
||||
**Test:** excluded rows get the status and DN number, and are absent from the
|
||||
`lines` passed to the PDF renderer.
|
||||
|
||||
## Task 8 — Picking-slip regression test
|
||||
|
||||
No production code changes. This task exists to lock in behaviour the design
|
||||
depends on, so a later refactor doesn't quietly break it.
|
||||
|
||||
- [ ] Test `print-bundle/route.ts`: an invoice whose rows include an excluded
|
||||
item produces a picking slip containing **all** rows, including the
|
||||
excluded one. The picker must pull the washers.
|
||||
- [ ] Same for `delivery-note-print/route.ts`.
|
||||
|
||||
## Task 9 — MCP surface
|
||||
|
||||
`src/app/api/mcp/route.ts`.
|
||||
|
||||
- [ ] `order_items.create` accepts `exclude_from_invoice`.
|
||||
- [ ] Update the tool description to explain what the flag does and that it
|
||||
forces `selling_price` to 0.
|
||||
- [ ] Update the description of the removed `pending` gate — the current text
|
||||
at line 794 documents a 409 that no longer fires.
|
||||
|
||||
## Definition of done
|
||||
|
||||
- [ ] `npm run test:run` passes.
|
||||
- [ ] `npx tsc --noEmit` passes.
|
||||
- [ ] Manually on port 4001: add an extra to an order with a `po_generated`
|
||||
sibling; process it onto a PO; confirm collection; send the order to
|
||||
QuickBooks; confirm the QuickBooks invoice shows only the billable line
|
||||
while all rows flip to `invoiced`; print the bundle and confirm the
|
||||
picking slip lists the extra.
|
||||
|
|
@ -0,0 +1,202 @@
|
|||
# Non-Invoiceable Order Extras — Design
|
||||
|
||||
**Date:** 2026-08-30
|
||||
**Status:** Approved, ready for implementation planning
|
||||
|
||||
## The problem
|
||||
|
||||
We quote a line as a single sellable unit — "M20 bolt, nut and washer" — and
|
||||
the customer approves that one line at that one price. But the supply side
|
||||
doesn't work that way: the bolt comes from one supplier, the nut and washer
|
||||
from another. To raise the purchase orders we need three line items, not one.
|
||||
|
||||
Today the operator has no way to add those two extra lines. `order_items` rows
|
||||
are the only unit the PO flow understands, so a component with no row of its
|
||||
own can't be ordered, can't be assigned to a driver, and never shows as
|
||||
collected. The operator works around it off-system.
|
||||
|
||||
Adding the rows naively creates the opposite problem: all three would flow to
|
||||
the invoicing screen and QuickBooks would bill the customer for three lines
|
||||
against a quote they approved as one.
|
||||
|
||||
## What we're building
|
||||
|
||||
A line item can be marked **excluded from invoice**. Such a line behaves like
|
||||
any other order item everywhere in the operational flow — it gets a supplier, a
|
||||
cost price, a purchase order, a driver assignment, and it shows as collected —
|
||||
but it never appears on a customer-facing invoice or delivery note.
|
||||
|
||||
The customer's paperwork comes out byte-identical to what it looks like today.
|
||||
|
||||
## Decisions
|
||||
|
||||
These were settled during brainstorming and are not open for reinterpretation
|
||||
during implementation.
|
||||
|
||||
| Question | Decision |
|
||||
|---|---|
|
||||
| Delivery note | **Excluded**, same as the invoice. Customer paperwork must agree. |
|
||||
| Terminal status | Auto-flips to `invoiced` alongside its siblings. The order closes normally. |
|
||||
| Where lines are added | Process page (`/orders/[id]/process`), for **any** line, with no `pending` gate. |
|
||||
| Cost and margin | Cost counts as normal. `selling_price` is forced to `0` and locked. |
|
||||
| Picking slip | **Included.** The picker must physically pull the washers. |
|
||||
|
||||
## Data model
|
||||
|
||||
One new field on `order_items`:
|
||||
|
||||
```
|
||||
exclude_from_invoice : bool
|
||||
```
|
||||
|
||||
**Polarity is load-bearing.** The flag must name the exception, not the norm.
|
||||
A positively-named field (`bill_customer`, `invoiceable`) defaults to `false`
|
||||
in PocketBase, which would silently mark every existing row in the database as
|
||||
non-billable the instant the field is created. `exclude_from_invoice` defaults
|
||||
to `false` = "bill this normally", so existing rows are unaffected.
|
||||
|
||||
Per `AGENTS.md`, the user adds this field via the PocketBase admin UI before
|
||||
any code references it. `pb-exports/pb_schema-2.json` is a snapshot for
|
||||
reference only.
|
||||
|
||||
No new `item_status` value is introduced. The existing select enum is
|
||||
untouched, which avoids a second manual PB admin change and avoids updating
|
||||
every status filter, pill, tab count and lifecycle comment in the app.
|
||||
|
||||
## Architecture
|
||||
|
||||
The design rests on a property of the existing invoice pipeline that makes the
|
||||
whole feature small.
|
||||
|
||||
### The invoice split
|
||||
|
||||
`buildQbInvoicePayload` (`src/app/(app)/invoicing/view-model.ts:393`) builds two arrays from
|
||||
one row list:
|
||||
|
||||
```ts
|
||||
order_item_ids: input.rows.map((row) => row.id), // drives the PB status write
|
||||
items: input.rows.map((row) => ({ ... })), // drives the QuickBooks lines
|
||||
```
|
||||
|
||||
The n8n workflow consumes them independently. Its **"Build PocketBase Invoice
|
||||
Update"** node maps over `order_item_ids` and patches each row with
|
||||
`item_status: 'invoiced'` plus `order_item_details.invoice`. The QuickBooks
|
||||
invoice itself is built from `items`.
|
||||
|
||||
So the change is: **excluded rows stay in `order_item_ids` and drop out of
|
||||
`items`.** n8n flips them to `invoiced` with the batch and stamps them with the
|
||||
same invoice number for audit; QuickBooks never learns they exist.
|
||||
|
||||
**No n8n changes are required.** This is the crux of the design — verified
|
||||
against `n8n_workfows/1 - WFDASH QUICKBOOKS INVOICE1 - WFDASH QUICKBOOKS INVOICE.json`.
|
||||
|
||||
### The delivery note split
|
||||
|
||||
`/api/invoicing/delivery-note/route.ts` has the same shape. Excluded rows stay
|
||||
in the status-flip loop (`src/app/api/invoicing/delivery-note/route.ts:230`), so they receive
|
||||
`item_status: "delivery_note"` and the DN number; they drop out of the `lines:`
|
||||
array that renders the PDF (`src/app/api/invoicing/delivery-note/route.ts:362`).
|
||||
|
||||
### Picking slips need no change at all
|
||||
|
||||
Both picking-slip routes query PocketBase by document identity, not by the row
|
||||
list the operator submitted:
|
||||
|
||||
- `print-bundle/route.ts:88` — `order_item_details.invoice.number = "…" && item_status = "invoiced"`
|
||||
- `delivery-note-print/route.ts:92` — `delivery_note_number = "…" && item_status = "delivery_note"`
|
||||
|
||||
Because excluded rows *do* receive the invoice number / DN number and *do*
|
||||
reach the matching status, they are picked up by these queries automatically.
|
||||
The picker sees the washers. This falls out of the design for free.
|
||||
|
||||
### The margin gate
|
||||
|
||||
`calculateOrderItemMarkup(cost, 0)` returns `-100`, far below the
|
||||
`ORDER_MARKUP_APPROVAL_THRESHOLD` of `22`
|
||||
(`src/lib/server/order-item-margin-approval-service.ts:4`). Without an
|
||||
explicit bypass, every extra the operator adds would immediately land in
|
||||
`approval_needed` and block the order pending manager review.
|
||||
|
||||
**This is the highest-risk part of the implementation.** The bypass must be
|
||||
keyed on `exclude_from_invoice`, applied in the process page's save path
|
||||
before the threshold comparison.
|
||||
|
||||
### Adding lines mid-flight
|
||||
|
||||
`createOrderItemForOrder` (`src/lib/server/orders-service.ts:3939`) currently throws a
|
||||
409 `order_locked` if any line on the order is past `pending`. By the time the
|
||||
operator discovers they need a washer line, the bolt line is already
|
||||
`po_generated`, so this gate fires every time. It is removed.
|
||||
|
||||
The gate's original purpose — preventing a PO's pricing/supplier reference from
|
||||
being orphaned — does not apply here. Adding a *new* row alongside an existing
|
||||
PO doesn't mutate that PO; the new row gets its own PO when the operator
|
||||
processes it.
|
||||
|
||||
## Flow
|
||||
|
||||
```
|
||||
Operator opens /orders/[id]/process
|
||||
│
|
||||
├── Adds "M20 nut" with [x] Don't invoice
|
||||
│ → selling_price forced 0, margin gate bypassed
|
||||
│ → row created with exclude_from_invoice = true, item_status = pending
|
||||
│
|
||||
├── Normal flow: PO generated → driver assigned → collected → delivered
|
||||
│ (identical to every other line; order "Complete" needs no change)
|
||||
│
|
||||
└── Invoicing screen
|
||||
├── row renders muted with a "Not invoiced" chip, checkbox locked
|
||||
├── Send to QuickBooks
|
||||
│ → order_item_ids: [bolt, nut, washer] ← all three
|
||||
│ → items: [bolt] ← one line billed
|
||||
│ → n8n flips all three to `invoiced`
|
||||
└── Print bundle
|
||||
→ picking slip query finds all three by invoice number
|
||||
→ picker pulls all three
|
||||
```
|
||||
|
||||
## Order completion
|
||||
|
||||
Unchanged. `isOrderComplete` (`src/lib/server/order-completion.ts:20`) requires every
|
||||
item to be `delivered`; excluded items reach `delivered` on the normal path.
|
||||
`markOrderInvoiced` (`src/lib/server/orders-service.ts:3397`) requires every item to be
|
||||
`invoiced`; excluded items get there via the n8n batch flip. Neither function
|
||||
needs to learn about the flag.
|
||||
|
||||
## Error handling
|
||||
|
||||
- **All rows in a group excluded.** Block the send with a toast rather than
|
||||
firing a zero-line invoice at QuickBooks. Applies to both the invoice and
|
||||
delivery-note submit paths.
|
||||
- **Operator un-ticks the only billable row** in the include-picker modal,
|
||||
leaving only excluded rows. Same guard, same message.
|
||||
- **Excluded row with a non-zero selling price** (legacy or hand-edited in PB
|
||||
admin). The payload builder forces the exclusion regardless of price — the
|
||||
flag is authoritative, price is not consulted.
|
||||
|
||||
## Testing
|
||||
|
||||
Unit tests, Vitest (`npm run test:run`):
|
||||
|
||||
- `view-model.test.ts` — `buildQbInvoicePayload` keeps excluded ids in
|
||||
`order_item_ids` and omits them from `items`; an all-excluded row set is
|
||||
detectable by the caller.
|
||||
- `orders-service.test.ts` — `createOrderItemForOrder` succeeds against an
|
||||
order with `po_generated` siblings; `exclude_from_invoice` and the zeroed
|
||||
selling price persist.
|
||||
- Delivery-note route test — excluded rows receive the status flip and DN
|
||||
number but are absent from the rendered `lines`.
|
||||
- Margin-gate test — an excluded line with cost > 0 and selling 0 does not
|
||||
enter `approval_needed`.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Reporting changes. Extras eat margin because their cost is real and their
|
||||
selling price is zero; that is the correct number and no report needs
|
||||
adjusting to produce it.
|
||||
- Retrofitting existing orders that were worked around off-system.
|
||||
- Any change to the n8n QuickBooks workflow.
|
||||
- Bulk-adding extras, or templating common component sets (bolt/nut/washer as
|
||||
a saved group). Worth considering later if the operator finds themselves
|
||||
typing the same three lines repeatedly.
|
||||
203
docs/superpowers/specs/2026-08-30-quantity-handling-audit.md
Normal file
203
docs/superpowers/specs/2026-08-30-quantity-handling-audit.md
Normal file
|
|
@ -0,0 +1,203 @@
|
|||
# Quantity Handling Audit — Surplus, Shortfall and Partial Collection
|
||||
|
||||
**Date:** 2026-08-30
|
||||
**Status:** Findings only. No design decisions taken yet.
|
||||
|
||||
Answers two operator questions:
|
||||
|
||||
1. We order 2 because of a supplier MOQ but the client only wanted 1. Does the
|
||||
extra get booked into stock, and does the picking slip show the client only
|
||||
the quantity that must actually go out?
|
||||
2. The driver collects 2 of 5. What happens to the other 3? Does it go back to
|
||||
the driver as outstanding, or is it lost?
|
||||
|
||||
---
|
||||
|
||||
## Question 1: MOQ surplus — mostly already works
|
||||
|
||||
**The good news: the picking slip already does exactly what you described.**
|
||||
|
||||
`computePickingSlipLine` (`src/app/(app)/invoicing/picking-slip.tsx:250`)
|
||||
computes five columns per line:
|
||||
|
||||
| Column | Formula | Meaning |
|
||||
|---|---|---|
|
||||
| `ordered` | `order_qty` | what the client asked for |
|
||||
| `collected` | `qty_collected` | what actually came in from the supplier |
|
||||
| `pickQty` | `min(collected, ordered)` | **what goes out to the client** |
|
||||
| `outstanding` | `max(ordered - pickQty, 0)` | still owed to the client |
|
||||
| `toStock` | `max(collected - pickQty, 0)` | the MOQ surplus |
|
||||
|
||||
`pickQty` is capped at the client's quantity, so the picker is never told to
|
||||
send out the surplus. Ordered 1, received 2 → pick 1, to stock 1. Exactly the
|
||||
behaviour you asked for, already shipped.
|
||||
|
||||
The invoice agrees. `resolveInvoiceLineDefaults`
|
||||
(`src/app/(app)/invoicing/view-model.ts:362`) bills `order_qty` and explicitly
|
||||
refuses to bill `qty_collected`, with a comment naming the MOQ case as the
|
||||
reason.
|
||||
|
||||
Surplus booking exists too: `src/lib/server/stock-surplus.ts` and
|
||||
`POST /api/v1/stock/surplus-book`, invoked from `/collection-confirm`. It
|
||||
increments `stock_items.current_qty`, writes a `purchase_surplus` stock
|
||||
movement, and stamps `order_items.surplus_qty`. The booking is capped at
|
||||
`qty_collected - customerQty` so it can't over-book.
|
||||
|
||||
### Gap 1.1 — surplus booking is manual and unprompted
|
||||
|
||||
Nothing makes the operator book the surplus. The picking slip prints
|
||||
`toStock: 1`, and if the operator never clicks the button on
|
||||
`/collection-confirm`, that unit is physically in the building and invisible to
|
||||
inventory forever. There is no "unbooked surplus" indicator, no count, no
|
||||
queue.
|
||||
|
||||
**Severity: medium.** Silent inventory loss, proportional to how often
|
||||
operators forget.
|
||||
|
||||
### Gap 1.2 — no check that the full quantity actually arrived
|
||||
|
||||
You said "if the full quantity came in you must check that." There is no such
|
||||
check. Nothing compares `qty_collected` against `order_qty` at any gate. See
|
||||
Question 2 — this is the same root cause.
|
||||
|
||||
---
|
||||
|
||||
## Question 2: Partial collection — the missing units disappear
|
||||
|
||||
**Direct answer: the 3 outstanding units do not go back to the driver. They
|
||||
are not tracked anywhere. The line is marked fully collected and moves on.**
|
||||
|
||||
Both paths that mark a line collected set `item_status = "collected"`
|
||||
unconditionally, without comparing quantities:
|
||||
|
||||
**Driver app** — `src/lib/server/driver-stops.ts:620`:
|
||||
|
||||
```ts
|
||||
const qtyCollected =
|
||||
input.qty_collected !== undefined
|
||||
? Math.floor(input.qty_collected)
|
||||
: Number(lineTyped.order_qty ?? lineTyped.quantity ?? 0);
|
||||
|
||||
const patch: Record<string, unknown> = { qty_collected: qtyCollected };
|
||||
if (input.action === "collected") {
|
||||
patch.item_status = "collected"; // ← no quantity comparison
|
||||
```
|
||||
|
||||
Driver reports 2 of 5 → `qty_collected: 2`, `item_status: "collected"`. The
|
||||
stop leaves their list. Nothing re-queues it.
|
||||
|
||||
**Agent / MCP path** — `confirmOrderItemsCollected`
|
||||
(`src/lib/server/order-items-service.ts:224`) is more careful, and then throws
|
||||
the care away. It computes `remaining`, validates the partial doesn't overrun,
|
||||
and writes a genuinely useful audit note — *"2 units added to qty_collected
|
||||
(now 2/5)"* — before patching `item_status: "collected"` at line 318 regardless.
|
||||
`remaining` is used only for validation and note text. It never holds the line
|
||||
open.
|
||||
|
||||
### What happens next
|
||||
|
||||
The line is `collected`, so it flows to `ready_invoice` and lands on the
|
||||
invoicing screen. There:
|
||||
|
||||
- The **picking slip is honest** — it shows `outstanding: 3`, because
|
||||
`computePickingSlipLine` does the subtraction. Good.
|
||||
- The **invoice is not** — `resolveInvoiceLineDefaults` bills `order_qty`,
|
||||
which is 5. **You invoice the client for 5 units when 2 arrived.**
|
||||
|
||||
The operator *can* correct this: the invoicing modal supports per-line `qty`
|
||||
overrides (`InvoiceLineOverride`). But the default is wrong and nothing flags
|
||||
the discrepancy, so it depends entirely on someone noticing.
|
||||
|
||||
### Gap 2.1 — `splitOrderItemRemainder` is locked out exactly when needed
|
||||
|
||||
There is already a purpose-built tool for a shorted line:
|
||||
`splitOrderItemRemainder` (`src/lib/server/order-items-service.ts:376`). It
|
||||
creates a remainder line for the shortfall, supplier-less so it sorts to the
|
||||
top of the process page as "needs supplier", and re-enters the normal PO flow.
|
||||
|
||||
It refuses unless the line is `pending` or `processed`:
|
||||
|
||||
```ts
|
||||
if (!["pending", "processed"].includes(status)) {
|
||||
throw new OrderItemSplitError(
|
||||
`Cannot split an item in status "${status}" — only pending/processed lines are still editable.`, 409);
|
||||
}
|
||||
```
|
||||
|
||||
A short-collected line is `assigned` or `collection_pool`, so the split is
|
||||
refused. **The mechanism that would solve this problem is unavailable in the
|
||||
only situation where the problem occurs.** It was built for a different case:
|
||||
the operator knowing up-front that supplier A can only supply 40 of 100.
|
||||
|
||||
### Gap 2.2 — `outstanding_since` is about age, not quantity
|
||||
|
||||
`driver-stops.ts:95` exposes `outstanding_since`, which looks like it tracks
|
||||
this. It doesn't — it's the parent order's `created` date, used for
|
||||
"outstanding N days" reporting. There is no quantity-outstanding concept
|
||||
anywhere in the model.
|
||||
|
||||
---
|
||||
|
||||
## Cross-cutting: `order_qty` means two different things
|
||||
|
||||
This is the root cause underneath both questions, and the most dangerous thing
|
||||
in this audit.
|
||||
|
||||
**`order_items.order_qty` has no doc comment** (`src/lib/types.ts:807`), while
|
||||
the neighbouring `quantity` has eleven lines of it. Two subsystems consequently
|
||||
disagree about what it means.
|
||||
|
||||
**Reading A — `order_qty` is what the CUSTOMER asked for:**
|
||||
|
||||
- `stock-surplus.ts:66` — *"`order_qty` is the canonical 'what the customer
|
||||
asked for' field"*
|
||||
- `view-model.ts:362` — invoice quantity
|
||||
- `driver-stops.ts:313` — `quantity_ordered: order_qty ?? quantity`
|
||||
- `picking-slip.tsx` — the `ordered` column
|
||||
|
||||
**Reading B — `order_qty` is what we're buying from THIS SUPPLIER:**
|
||||
|
||||
- `splitOrderItemRemainder` doc, `order-items-service.ts`: *"the process page
|
||||
lets the operator enter a supplier-facing `order_qty` that is LOWER than the
|
||||
customer-facing `quantity`"* — customer 100, `order_qty` 40.
|
||||
|
||||
The two readings converge only *after* a completed split, because the split
|
||||
reduces `quantity` down to match `order_qty`. In the window between the
|
||||
operator typing `order_qty = 40` and clicking Split, every consumer in Reading
|
||||
A believes the customer ordered 40.
|
||||
|
||||
**Failure scenario:** customer orders 100. Operator sets `order_qty = 40` for
|
||||
supplier A, intending to split the other 60 to supplier B. They get
|
||||
interrupted and never click Split. The invoice bills 40. The picking slip says
|
||||
ordered 40, outstanding 0. The other 60 units are gone with no trace and no
|
||||
warning.
|
||||
|
||||
There is also a smaller inconsistency: `confirmOrderItemsCollected` computes
|
||||
`remaining` from `row.quantity`, while `driver-stops.ts` defaults from
|
||||
`row.order_qty`. Two different denominators for the same concept.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| # | Finding | Severity |
|
||||
|---|---|---|
|
||||
| X-1 | `order_qty` means "customer qty" in four places and "supplier qty" in one, undocumented. Un-split lines silently under-bill. | **High** |
|
||||
| 2.1 | Partial collection marks the line fully `collected`; the shortfall is untracked and the invoice defaults to the full quantity. | **High** |
|
||||
| 2.2 | `splitOrderItemRemainder` is blocked by status exactly when a short collection needs it. | Medium |
|
||||
| 1.1 | Surplus booking is manual with no prompt or unbooked-surplus queue. | Medium |
|
||||
| 1.2 | No gate anywhere compares `qty_collected` to `order_qty`. | Medium |
|
||||
|
||||
The picking slip is the one component that already gets all of this right. The
|
||||
arithmetic you asked for exists — it just isn't enforced anywhere upstream of
|
||||
the print, and the invoice ignores it.
|
||||
|
||||
## Not yet decided
|
||||
|
||||
No design decisions have been taken. The open questions are:
|
||||
|
||||
- Should a short collection hold the line open (new status / partial state), or
|
||||
auto-split the remainder into a new line, or just warn loudly at invoicing?
|
||||
- Should `order_qty` be documented as customer-qty and the split flow moved to a
|
||||
separate `supplier_qty` field, or the reverse?
|
||||
- Should surplus booking be forced before a line can be invoiced?
|
||||
Loading…
Reference in a new issue