docs: add Tech Debt register and Inventory-on-Approval deep-dive
- New "Inventory on Approval" page: intended design (add ordered items to the delivery site stock at approval), the NEXT_PUBLIC_INVENTORY_ENABLED flag (UI-only gate), and how it actually behaves in prod -- dormant, because POs carry no siteId under the vessel-only cost-centre model and the write is not flag-gated. Includes remediation options. - New "Tech Debt" register (Engineering section): TD-1 inventory-on-approval dormant path; TD-2 migrations not coupled to the build (caused the paymentDate P2022 prod incident). - Sidebar links both pages; Inventory and Catalogue corrects the approval line and cross-links the new pages.
parent
67cd529f84
commit
c6bec19d85
4 changed files with 191 additions and 3 deletions
|
|
@ -72,9 +72,11 @@ surfaces are hidden; the vendor/product catalogue and cart remain available.
|
||||||
|
|
||||||
- **Sites** (`/admin/sites`) — ports/depots/offices that hold stock; geocoded
|
- **Sites** (`/admin/sites`) — ports/depots/offices that hold stock; geocoded
|
||||||
from pincode; vessels can be associated.
|
from pincode; vessels can be associated.
|
||||||
- **`ItemInventory`** — quantity per `(product, site)`. **Incremented at PO
|
- **`ItemInventory`** — quantity per `(product, site)`. **Designed to be
|
||||||
approval** (not on close) for the ordered quantities, when the PO has a
|
incremented at PO approval** (not on close) for the ordered quantities, when
|
||||||
`siteId`.
|
the PO has a `siteId`. ⚠️ In production this rarely fires — POs are raised
|
||||||
|
against a Vessel and carry no `siteId`, and the write isn't gated by the flag.
|
||||||
|
See [Inventory on Approval](Inventory-on-Approval) and [Tech Debt](Tech-Debt).
|
||||||
- **`ItemConsumption`** — daily draw-down per `(product, site, date)`, recorded
|
- **`ItemConsumption`** — daily draw-down per `(product, site, date)`, recorded
|
||||||
via the "Log Consumption" form on the site detail page (with `recordedBy`).
|
via the "Log Consumption" form on the site detail page (with `recordedBy`).
|
||||||
|
|
||||||
|
|
|
||||||
116
Inventory-on-Approval.md
Normal file
116
Inventory-on-Approval.md
Normal file
|
|
@ -0,0 +1,116 @@
|
||||||
|
# Inventory on Approval
|
||||||
|
|
||||||
|
How site inventory is (meant to be) updated when a PO is approved, the feature
|
||||||
|
flag that governs the inventory surfaces, and **how it actually behaves in
|
||||||
|
production today** — which diverges from the intended design.
|
||||||
|
|
||||||
|
> **TL;DR** — The design is "approving a PO adds its ordered items to the
|
||||||
|
> delivery site's stock immediately." In production this path is effectively
|
||||||
|
> dormant: POs no longer carry a `siteId` (cost centre is a **Vessel**), so the
|
||||||
|
> `if (po.siteId)` guard in the approve action is almost never true, and the
|
||||||
|
> write isn't gated by the inventory feature flag anyway. See
|
||||||
|
> [Tech Debt](Tech-Debt) for the open items.
|
||||||
|
|
||||||
|
## 1. What the design is meant to do
|
||||||
|
|
||||||
|
When a Manager approves a PO, the ordered line items should be added to the
|
||||||
|
**delivery site's** on-hand stock **right away** — so inventory reflects
|
||||||
|
orders-in-flight rather than waiting for delivery/closure.
|
||||||
|
|
||||||
|
- Inventory lives in `ItemInventory`, keyed by `(productId, siteId)`.
|
||||||
|
- On approval, for each line item the ordered quantity is **incremented** onto
|
||||||
|
that product's stock at the PO's site.
|
||||||
|
- This replaced an earlier design where inventory was only updated at receipt
|
||||||
|
confirmation (close). The intent of moving it to approval was to surface
|
||||||
|
committed demand sooner on the site dashboards.
|
||||||
|
|
||||||
|
Implementation lives in `approvePo()` —
|
||||||
|
`app/(portal)/approvals/[id]/actions.ts`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
// after the PO is set to MGR_APPROVED …
|
||||||
|
const siteId = po.siteId ?? null;
|
||||||
|
if (siteId) {
|
||||||
|
for (const li of po.lineItems) {
|
||||||
|
if (!li.productId) continue; // unlinked items are skipped
|
||||||
|
await db.itemInventory.upsert({
|
||||||
|
where: { productId_siteId: { productId: li.productId, siteId } },
|
||||||
|
update: { quantity: { increment: Number(li.quantity) } },
|
||||||
|
create: { productId: li.productId, siteId, quantity: Number(li.quantity) },
|
||||||
|
});
|
||||||
|
}
|
||||||
|
revalidatePath(`/admin/sites/${siteId}`);
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
## 2. The feature flag
|
||||||
|
|
||||||
|
`NEXT_PUBLIC_INVENTORY_ENABLED` — read once in `lib/feature-flags.ts`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
export const INVENTORY_ENABLED =
|
||||||
|
process.env.NEXT_PUBLIC_INVENTORY_ENABLED !== "false";
|
||||||
|
```
|
||||||
|
|
||||||
|
- **On unless explicitly `"false"`.** Unset ⇒ enabled.
|
||||||
|
- The flag **only controls UI visibility** of the inventory *tracking* surfaces:
|
||||||
|
the `Sites` admin area, per-site stock tables, the consumption charts/form, and
|
||||||
|
the Sites sidebar link. The vendor list, product catalogue, and cart used for
|
||||||
|
PO creation are always available, regardless of the flag.
|
||||||
|
- **The flag does _not_ gate the data write.** The `ItemInventory` upsert in
|
||||||
|
`approvePo()` (section 1) does **not** check `INVENTORY_ENABLED`. It runs
|
||||||
|
purely on the `po.siteId` condition. So "inventory disabled" hides the screens
|
||||||
|
but does **not** stop the approval-time mutation.
|
||||||
|
|
||||||
|
## 3. How it functions in production right now
|
||||||
|
|
||||||
|
Production does **not** set `NEXT_PUBLIC_INVENTORY_ENABLED`, so
|
||||||
|
`INVENTORY_ENABLED` is `true` and the inventory screens are visible. Despite
|
||||||
|
that, the auto-increment is effectively a no-op:
|
||||||
|
|
||||||
|
1. **POs don't carry a `siteId`.** The cost centre is a **Vessel** — PO
|
||||||
|
create/edit/import forms set `vesselId`, never `siteId` (`siteId` remains an
|
||||||
|
optional legacy column on `PurchaseOrder`). The `if (siteId)` guard is
|
||||||
|
therefore almost always false, so nothing is written.
|
||||||
|
2. **Even with a `siteId`, line items may be skipped.** Items are counted only
|
||||||
|
when they already have a `productId`. Products are auto-linked/created at
|
||||||
|
**payment** (`syncProductCatalog` in `payments/actions.ts`), so line items
|
||||||
|
typed by hand have no `productId` at approval and are skipped.
|
||||||
|
|
||||||
|
**Net effect in prod:** the Sites / inventory pages render (flag is on) but stay
|
||||||
|
empty, because the on-approve write rarely — in practice, never — fires for a
|
||||||
|
normally-created PO. Inventory is *visible but unpopulated*.
|
||||||
|
|
||||||
|
> The slightly different statement in
|
||||||
|
> [Inventory and Catalogue](Inventory-and-Catalogue) ("incremented at PO
|
||||||
|
> approval … when the PO has a `siteId`") is technically correct but reads as if
|
||||||
|
> the path is live; in the current vessel-only model the `siteId` precondition
|
||||||
|
> is the reason it isn't.
|
||||||
|
|
||||||
|
## 4. Why it diverges (root cause)
|
||||||
|
|
||||||
|
Inventory is modelled **per Site**, but POs are now raised against a **Vessel**
|
||||||
|
(the Vessel-or-Site cost-centre model was collapsed to Vessel-only). Nothing in
|
||||||
|
the current flow maps a PO to a Site, so the per-site write has no site to target.
|
||||||
|
|
||||||
|
## 5. Options to make it match the design (or retire it)
|
||||||
|
|
||||||
|
Pick one direction and apply consistently — tracked in [Tech Debt](Tech-Debt):
|
||||||
|
|
||||||
|
- **Give POs a site.** Derive a Site from the chosen Vessel (e.g. a vessel→home-site
|
||||||
|
link) or add an explicit "delivery site" field, so `po.siteId` is populated.
|
||||||
|
- **Link products at approval.** If inventory-on-approve is to mean anything,
|
||||||
|
ensure line items are product-linked (or create products) before the upsert,
|
||||||
|
rather than only at payment.
|
||||||
|
- **Gate the write on the flag.** Make the `ItemInventory` upsert honour
|
||||||
|
`INVENTORY_ENABLED` so "off" has no data side effects — or remove the
|
||||||
|
approval-time write entirely if the feature is being parked.
|
||||||
|
- **Retire it.** If inventory tracking isn't a near-term priority, set
|
||||||
|
`NEXT_PUBLIC_INVENTORY_ENABLED=false` in prod to hide the empty screens, and
|
||||||
|
delete/flag-gate the dormant write.
|
||||||
|
|
||||||
|
## Related
|
||||||
|
|
||||||
|
- [Inventory and Catalogue](Inventory-and-Catalogue) — products, pricing, cart, sites.
|
||||||
|
- [PO Lifecycle](PO-Lifecycle) — where approval sits in the flow.
|
||||||
|
- [Tech Debt](Tech-Debt) — the open items captured above.
|
||||||
66
Tech-Debt.md
Normal file
66
Tech-Debt.md
Normal file
|
|
@ -0,0 +1,66 @@
|
||||||
|
# Tech Debt
|
||||||
|
|
||||||
|
A running register of known shortcuts, dormant code paths, and
|
||||||
|
design/implementation mismatches that we've consciously accepted for now. The
|
||||||
|
goal is visibility: each item records **what**, **why it matters**, and a
|
||||||
|
**suggested direction** — not a commitment to fix on any timeline.
|
||||||
|
|
||||||
|
> Add new items at the top of the "Open" list with a short, honest description.
|
||||||
|
> Move resolved items to "Resolved" with the commit/PR that closed them.
|
||||||
|
|
||||||
|
## Open
|
||||||
|
|
||||||
|
### TD-1 · Inventory-on-approval is dormant in production
|
||||||
|
|
||||||
|
**What.** Approving a PO is meant to add its ordered items to the delivery
|
||||||
|
site's stock (`ItemInventory`, keyed by `(productId, siteId)`). In production the
|
||||||
|
write almost never fires, and it isn't governed by the inventory feature flag.
|
||||||
|
|
||||||
|
**Why it matters.**
|
||||||
|
|
||||||
|
- POs are raised against a **Vessel** (cost centre), and PO forms set `vesselId`,
|
||||||
|
never `siteId`. The `if (po.siteId)` guard in `approvePo()` is therefore
|
||||||
|
almost always false → no inventory is written. Inventory screens render
|
||||||
|
(the flag defaults on) but stay empty.
|
||||||
|
- The `ItemInventory` upsert is **not gated by `NEXT_PUBLIC_INVENTORY_ENABLED`** —
|
||||||
|
the flag only hides UI. So "inventory off" would still mutate data if a PO ever
|
||||||
|
did carry a `siteId`.
|
||||||
|
- Line items are counted only when they already have a `productId`, but products
|
||||||
|
are linked/created at **payment**, not approval — so hand-typed items would be
|
||||||
|
skipped even with a site.
|
||||||
|
- Root cause: inventory is modelled per **Site**, but POs are per **Vessel**, and
|
||||||
|
nothing maps a PO to a Site.
|
||||||
|
|
||||||
|
**Suggested direction.** Decide one: (a) give POs a Site (vessel→home-site link
|
||||||
|
or an explicit delivery-site field) and link products at approval; or (b) gate
|
||||||
|
the write on the flag / remove it and set `NEXT_PUBLIC_INVENTORY_ENABLED=false`
|
||||||
|
in prod to hide the empty screens. Full write-up:
|
||||||
|
[Inventory on Approval](Inventory-on-Approval).
|
||||||
|
|
||||||
|
**Touch points.** `app/(portal)/approvals/[id]/actions.ts` (the upsert),
|
||||||
|
`lib/feature-flags.ts`, `PurchaseOrder.siteId`, `payments/actions.ts`
|
||||||
|
(`syncProductCatalog`).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### TD-2 · Migrations are not coupled to the build
|
||||||
|
|
||||||
|
**What.** `pnpm build` runs `prisma generate` (TypeScript client) but **not**
|
||||||
|
`prisma migrate deploy`. Shipping code whose client expects a new column before
|
||||||
|
the DB has it throws `P2022 … column does not exist` at runtime.
|
||||||
|
|
||||||
|
**Why it matters.** This already caused a production incident (the `paymentDate`
|
||||||
|
column). The deploy workflow (`.forgejo/workflows/deploy.yml`) does run
|
||||||
|
`migrate deploy`, but **manual** deploys can skip it.
|
||||||
|
|
||||||
|
**Suggested direction.** Treat "apply migrations before the new build serves
|
||||||
|
traffic" as the invariant. Options: fold `prisma migrate deploy` into the
|
||||||
|
`start`/release script, or add a pre-deploy check that fails if migrations are
|
||||||
|
pending. Documented in the App `README.md`.
|
||||||
|
|
||||||
|
**Touch points.** `App/package.json` scripts, `.forgejo/workflows/deploy.yml`,
|
||||||
|
`App/README.md`.
|
||||||
|
|
||||||
|
## Resolved
|
||||||
|
|
||||||
|
_None yet._
|
||||||
|
|
@ -24,6 +24,7 @@
|
||||||
- [Purchase Orders](Purchase-Orders)
|
- [Purchase Orders](Purchase-Orders)
|
||||||
- [Vendors and GST Lookup](Vendors-and-GST-Lookup)
|
- [Vendors and GST Lookup](Vendors-and-GST-Lookup)
|
||||||
- [Inventory and Catalogue](Inventory-and-Catalogue)
|
- [Inventory and Catalogue](Inventory-and-Catalogue)
|
||||||
|
- [Inventory on Approval](Inventory-on-Approval)
|
||||||
- [Notifications](Notifications)
|
- [Notifications](Notifications)
|
||||||
- [File Storage](File-Storage)
|
- [File Storage](File-Storage)
|
||||||
- [Design System](Design-System)
|
- [Design System](Design-System)
|
||||||
|
|
@ -42,3 +43,6 @@
|
||||||
**Ops**
|
**Ops**
|
||||||
- [Deployment and Operations](Deployment-and-Operations)
|
- [Deployment and Operations](Deployment-and-Operations)
|
||||||
- [Issue-to-Deploy Pipeline](Issue-to-Deploy-Pipeline)
|
- [Issue-to-Deploy Pipeline](Issue-to-Deploy-Pipeline)
|
||||||
|
|
||||||
|
**Engineering**
|
||||||
|
- [Tech Debt](Tech-Debt)
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue