Design: Review V2 line-item override (quantity & unit price)
Status: APPROVED (/autoplan, 2026-06-14) — premise “build as specified”; 10 must-fixes folded in; 3 taste decisions resolved (all recommendations taken). See review report at end.
Owner: David
Last updated: 2026-06-14
Surface: apps/webapp order review (Review V2 cockpit)
Builds on: Review V2 (review-v2-multi-tab-excel.md), ENG-277 optimistic-lock + price provenance
Problem Statement
Section titled “Problem Statement”Legacy review let a CSR edit any line’s quantity and unit price as free-form inputs. Review V2 renders both as read-only display — there is no override path. That removed a legitimate, if narrow, capability:
- OCR/extraction misreads a quantity (e.g.
320vs300) and the corrected value isn’t a matcher decision. - A negotiated one-off price that is neither the ERP tier price nor a PO price.
- A unit-of-measure / pack mismatch the matcher and tier resolver cannot model.
Today the only way to “fix” these is a support-side DB edit or routing the order back as an error — both worse than a controlled, audited in-UI override.
The goal: restore override durably, without adding complexity to the three core flows (auto-validate, discounts, submit).
Why V2 dropped it (context, not a mistake)
Section titled “Why V2 dropped it (context, not a mistake)”V2 reframed review from editing a form to resolving matches. Free-form qty/price editing was dropped for three concrete reasons:
- Free price edits raced the async pricing writeback.
persistAutoPricedMatcheswrites resolved ERP-tier prices back tounit_priceunder arow_versionCAS guard specifically so it never clobbers an operator — see persist-auto-priced-matches.ts:27 and the CAS at :583. Legacy’s bundled whole-orderPUTdidn’t participate cleanly in that protocol → lost-update risk. ENG-277 addedrow_versionand the immutableextracted_unit_priceto close it. - Price became a structured decision. When PO price ≠ ERP tier, auto-validate emits a
validationErrorcarryingdetails.erpTierPrice; the operator accepts the tier price rather than typing one. That is already a constrained override — see accept-gross-price.ts:137. - Allowances moved to their own channel. Discounts/allowances are first-class custom-line items resolved via
DiscountMatchPanel, removing a major reason CSRs used to fudge price.
The casualty: the general override branch was dropped, leaving exactly one allowed move (accept ERP tier price).
Key insight: the substrate already exists
Section titled “Key insight: the substrate already exists”accept-gross-price is a price override — it sets:
.set({ unitPrice: grossPrice, validationError: null, isValidated: true, isAutoValidated: false, isEdited: true })…hardcoded to value = erpTierPrice and gated to items already in price-review. Generalizing that single endpoint gives durable override almost for free, riding rails that already exist:
| Rail | Already present | Source |
|---|---|---|
| Immutable original price | extracted_unit_price (scale 4) | schema:495 |
| Optimistic lock | row_version + CAS guard in writeback | schema:509 |
| Provenance enum | validation_source = worker | user | auto_validate | schema:487 |
| Submit reads resolved values | payload built from DB quantity/unitPrice | submit.ts (~252–266, 643) |
| Audit | SupportAccessAudit.logAccess | already called in accept-gross-price |
⚠️ Review correction (C1/H1/M1): three of these “rails” are unfinished substrate, not finished rails:
- Submit does NOT carry the override. submit.ts:641-644 builds
ErpOrderInput.itemsas{erpItemId, quantity, price, unitOfMeasure}only. wherefour-service.ts:525-532 drops the per-line price whenpreferTierOverOverride && tierAvailable && hasPrice && !explicitOverride(explicit =item.overrideUnitPrice === true). For CK (the first rollout target — see the flag’s own comment naming “CK and similar”), an overridden price is silently re-priced to tier. Must wireoverrideUnitPriceend-to-end (C1).validation_sourceis write-only. It is written in one place and read by no auto-validate path, so “re-validate skips user lines” does not exist yet (H1).price_decisionsis the real audit table. price-decisions.ts already hasdecision_stage='manual_override',source='manual_override',actor='operator',reason(“Required for manual_override”),chosenPrice,priorDecisionId— purpose-built for exactly this (M1, taste decision T1).
Proposed approach
Section titled “Proposed approach”Add one terminal line-resolution action — “Override” — alongside Confirm / Skip / Mark error / Remove, backed by a single new endpoint.
Data model deltas
Section titled “Data model deltas”extracted_quantity(new column). There is an immutableextracted_unit_pricebut no quantity equivalent, so a quantity override would lose the original. Addextracted_quantity integer(nullable), written once at extraction time and never overwritten — symmetric withextracted_unit_price.- Migration shape (M3 — match the
extracted_unit_priceprecedent, do NOT inline-backfill). ENG-277 addedextracted_unit_priceas an instant DDL-onlyADD COLUMN IF NOT EXISTS(migration 0062) with a separate idempotent backfill script (backfill-extracted-unit-price.ts+ test). Mirror that: migration =ALTER TABLE extracted_order_items ADD COLUMN IF NOT EXISTS extracted_quantity integer;(nullable, instant), numbered 0082 (rebased onto staging; latest base is 0081). A whole-tableUPDATE … SET extracted_quantity = quantityinside the DDL is not instant and risks wedging the migration ledger (active drift per PR #1668/0079). TreatNULL extracted_quantityas ”= current quantity” in the UI for legacy rows.
- Migration shape (M3 — match the
No other schema changes: override reuses unit_price, quantity, validation_source, row_version, is_edited, validation_error.
API: POST /api/extracted-orders/[id]/items/[itemId]/override
Section titled “API: POST /api/extracted-orders/[id]/items/[itemId]/override”Accepts { quantity?: number, unitPrice?: string, reason?: string, expectedRowVersion: number }.
Behavior (mirrors accept-gross-price, strictly safer):
- Per-customer authz (revised — see update note below). Override is opt-in per customer via the connection-scoped
review_v2_line_overrideflag (connection-flag.ts), default off. When enabled, a customer’s own CSRs (org users) may override only on their own org’s orders; Ordermatic support-admins keep cross-org break-glass access (audited). The flag is enforced server-side, not just in the UI. - Input bounds (H2/F4 — server zod + client mirror).
quantity: positive integer (column isinteger; reject0, negative, non-integer, blank).unitPrice: numeric,> 0, ≤ scale-4 (matchesextracted_unit_price); reject negative/blank. Deviation guard: when an ERP tier price is known and|unitPrice − erpTierPrice| / erpTierPriceexceeds a band (e.g. 25%, configurable), require an explicit confirm and a non-emptyreason. Soft-cap absurd values (qty > 100k, price > $100k) with confirm, not hard block. No-op overrides (qty & price unchanged) rejected. - CAS on
row_version(WHERE row_version = expectedRowVersion, thenrow_version + 1). - Sets the supplied
quantity/unitPrice,validation_source = 'user',is_edited = true,is_validated = true, clearsvalidation_error. - Writes
extracted_quantity/extracted_unit_priceonly if still null (preserve original). - Carry the override to the ERP (C1 — the must-fix). Persist an operator-override signal (reuse
validation_source = 'user'or addoverride_unit_price boolean) and, in submit.ts:641-644, setoverrideUnitPrice: trueon theErpOrderItemInputfor overridden lines (the bootstrapOrderItem[]map at:252-266must carry it too, or submit re-readsextracted_order_items). Without this, CK books at tier and the override is a silent no-op. - Audit (M1 / taste T1). Doc default is
SupportAccessAudit.logAccess(action: 'api_override_line_item'). Recommended: also write aprice_decisionsrow (decision_stage='manual_override',source='manual_override',actor='operator',reason,chosenPrice,priorDecisionId) — no migration needed, the enums exist, and it’s the table downstream reconciliation reads. - 409 on
row_versionmismatch → client refetches and re-presents (see UI F5). - Sibling CAS fix (M2 — now required, not optional).
accept-gross-price.tsANDvalidate-at-tier-price.tsupdate price/isValidatedwithout bumpingrow_version, violating the invariant the writeback CAS depends on (schema:506-507). Shipping a CAS-correct override next to two CAS-blind siblings undermines the safety argument. Patch both in the same PR.
UI: action-bar “Override” action
Section titled “UI: action-bar “Override” action”- Placement (taste T2). Doc default: a new ghost button + shortcut in the ItemDetailPane action bar at ReviewCenterPane.tsx (~977–1050). Design recommendation: Override is an edit that keeps you on the line, not a terminal action that advances focus like Skip/Error/Remove/Confirm — and a 5th button forces the bar to wrap on laptops. Prefer an inline “Edit” pencil affordance on the Quantity / Unit price rows in the “FROM DOCUMENT” block (where the edited values live); action bar stays 4 actions.
- Appearance conditions (F2/F3). Suppress on discount/custom lines (
isCustomLineItem(item)— they own their own panel). On a price-review line, “Accept tier price” stays primary; Override is the subordinate “neither value is right” escape hatch. - Opens a small inline editor (qty and/or price + reason), not a full form.
- States (F4 — CRITICAL, mirror
DiscountMatchPanel): saving (disable Save, block re-submit), non-409 failure (revert optimistic update, inline error, keep editor open with typed values), success. 409 (F5): surface what changed (“price auto-updated to $X while editing”), preserve typed input, let CSR re-apply deliberately — do not silently swap values under an open editor. - Keyboard (F10): trigger shortcut
OorE(S/X/Enter/Backspace/J/K/⌘K taken) added to the Kbd legend; the editor ownsEnter=Save /Escape=cancel viaonKeyDown+stopImmediatePropagation(RemoveLineConfirm precedent at ReviewCenterPane.tsx:271-277) so a background auto-validate that re-arms the parent listener can’t hijack keys. The global handler already early-returns onisInput, so typing a qty won’t fire Skip. Focus the qty field on open; return focus to the line on close. - Provenance + revert (F12 — required). Badge on the value rows it modifies: “Overridden: 320 → 300 (name · reason)” sourced from
extracted_*vs current (show the arrow only whenextracted_* !== current; handle nullextracted_quantity; attribute the real actor, not “you”). Every other resolution here is reversible (Remove restores; discount has “Clear pin”) — the badge needs “Revert to original” restoring fromextracted_quantity/extracted_unit_priceand resettingvalidation_source. Cheap (originals are preserved) and closes the fat-finger loop. - Numeric inputs use
inputMode="decimal"/"numeric"; editor fields stack under narrow@container(desktop-CSR-first, so “don’t break” on mobile). - Gated behind the connection-scoped
review_v2_line_overrideflag (per customer, default off). Enable per ERP connection (a customer may have several, e.g. prod vs sandbox). The UI reads the same connection flag the server enforces.
Why the core flows stay untouched
Section titled “Why the core flows stay untouched”Override is a terminal resolution state feeding the same columns the core flows already consume:
- Auto-validate — unchanged (H1 resolved by retraction). Override is an alternative resolution, not a matcher step. The original “re-validate skips
validation_source='user'lines” mechanism was never wired (the column was write-only) and is unnecessary: an override setsis_validated = true, and the auto-validate candidate selection already includes only un-validated lines, so an overridden line is never re-derived. Thevalidation_source='user'stamp is kept for provenance/audit, not for skip logic. - Discounts — unchanged. They remain their own line type via
DiscountMatchPanel; override targets product lines and quantity corrections only. - Submit — NOT unchanged (C1). It must set
overrideUnitPrice(see API), or CK books at tier. Also a documentation correction (M4): the submit gate blocks onvalidationError.type === 'needs_review'(isNeedsReviewValidationError), not onisValidated— an override unblocks the line by clearingvalidation_error. Note there are two gate implementations (bootstrapsubmit.ts:236-249and reload:343-364); an override must clearvalidation_errorfor both (it does). Add a test for the bootstrap path (v2 commonly hits it).
Rollout
Section titled “Rollout”- Migration 0082: instant DDL
ADD COLUMNforextracted_quantity+override_unit_price(no inline backfill) + separate idempotent backfill script (M3). - Wire
overrideUnitPricethrough submit→WhereFour + the submit-integration test against apreferTierOverOverrideconnection (C1). Nothing ships before this is green — it is the gate on delivering any value to CK. - Bump
row_versioninaccept-gross-priceANDvalidate-at-tier-price(M2 — required). - H1 resolved by retraction: overridden lines carry
is_validated = true, already excluded by the auto-validate candidate selection — no auto-validate predicate change needed. - Ship endpoint + UI behind
review_v2_line_override(off), support-admin-gated. - Enable for platform support-admins, then CK POC org, then GA.
Test plan
Section titled “Test plan”- API unit: override sets values +
validation_source='user'+ bumpsrow_version; 409 on staleexpectedRowVersion; originalextracted_*preserved on second override; auth/org-scope + support-admin audit row written. - Race: override interleaved with
persistAutoPricedMatches— CAS guarantees exactly one winner, no silent overwrite. - Submit integration (CRITICAL GAP today): assert the outbound payload’s
override_unit_pricesurvives for an operator-overridden line on apreferTierOverOverride=trueconnection — the chainextracted_order_items → submit.ts ErpOrderInput → erp-submission remap → WhereFour adapter. Notesubmit.test.tscurrently mocks Temporal entirely, so the payload is never asserted — that mock is exactly why C1 slipped; this test must assert the real body. Also: overridden line passes BOTH gates (bootstrap + reload); price-review blocker still fires for other unresolved lines. - Input bounds (H2): reject non-integer/zero/negative qty, negative/zero/blank price; deviation guard fires + requires reason past the band; no-op override rejected.
- Audit: override writes the chosen audit row(s) (SupportAccessAudit and/or
price_decisionsper T1); reviewer (non-admin) path audited if scope is widened. - Component: editor states (saving / failure-revert / 409 surface), keyboard (Enter=Save trapped, Backspace doesn’t Remove), provenance badge from
extracted_*incl. null, Revert to original. - Regression: auto-validate and discount-resolution suites unchanged and green.
Open questions
Section titled “Open questions”- Permission scope: RESOLVED — per-customer (connection flag). Customer CSRs override on their own org’s orders when enabled; support-admins keep cross-org break-glass. Support overrides log to
SupportAccessAudit; customer price overrides are audited viaprice_decisions(actor=operator). UI affordance follows the customer flag, so support break-glass when a customer has it OFF is API-level only. - Re-validate semantics: the skip must be implemented (H1), not assumed. Lean: skip
validation_source='user'lines, with an explicit “re-match anyway” affordance. - Reason required? See taste decision T3.
/autoplan Review Report
Section titled “/autoplan Review Report”Run: /autoplan, 2026-06-14 · branch claude/serene-bell-6640b4 · base main (PRs target staging)
Scope: CEO ✓ · Design ✓ (UI scope) · Eng ✓ · DX skipped (no developer-facing surface; internal Astro routes + CSR UI only)
Voices: [subagent-only] — Codex was entitlement-blocked on this host (ChatGPT-account login rejects gpt-5 / gpt-5.4 / gpt-5-codex). Second voice = direct code verification of every load-bearing claim. Fix codex model access to restore dual-voice.
Premise gate: confirmed “build as specified” (unified qty+price editor, generic audit acceptable), F1 non-negotiable.
Consensus (CEO / Design / Eng)
Section titled “Consensus (CEO / Design / Eng)”| Dimension | Verdict | Note |
|---|---|---|
| Right problem? | CONFIRM (owner) | Premise unevidenced by data, but owner has CK ground truth and confirmed build. |
| Does the override reach the ERP? | FAIL → must-fix | C1: dropped to tier for CK. Verified in submit.ts + wherefour-service.ts. |
| ”Rails already exist”? | PARTIAL | validation_source write-only (H1); submit override path absent (C1); price_decisions ignored (M1). |
| Pricing integrity / trust | FAIL → must-fix | H2: no qty/price bounds; fat-finger reaches customer ERP. |
| Concurrency safety | PARTIAL | M2: two sibling endpoints don’t bump row_version. |
| Migration safety | OK w/ fix | M3: DDL-only + separate backfill, number 0082. |
| UX completeness | PARTIAL | F4 (states), F7 (inline validation), F12 (revert) missing. |
Must-fixes (auto-decided — folded into the doc above)
Section titled “Must-fixes (auto-decided — folded into the doc above)”| # | Sev | Fix | Principle |
|---|---|---|---|
| C1 | CRITICAL | Wire overrideUnitPrice through submit→WhereFour + integration test on a preferTierOverOverride connection | P1 completeness |
| H1 | HIGH | Implement (or retract) the validation_source='user' re-validate skip | P5 explicit |
| H2/F4-eng | HIGH | Server+client input bounds (qty>0 int, price>0 scale-4) + deviation guard | P1 completeness |
| M2 | HIGH | Bump row_version in accept-gross-price + validate-at-tier-price (required) | P2 boil-the-lake |
| F4-ux | CRITICAL | Editor saving/failure/409 states (mirror DiscountMatchPanel) | P1 completeness |
| F7-ux | CRITICAL | Inline input validation before Save | P1 completeness |
| M3 | MED | Migration 0082: instant DDL + separate idempotent backfill | P5 explicit |
| M4 | MED | Correct submit-gate mechanism text; test bootstrap gate | P5 explicit |
| L3 | MED | Explicit hasPlatformSupportAdminRole gate on the new endpoint | P5 explicit |
| F12-ux | HIGH | ”Revert to original” from extracted_* | P1 completeness |
Taste decisions (RESOLVED — approved as-is, all recommendations taken)
Section titled “Taste decisions (RESOLVED — approved as-is, all recommendations taken)”- T1 — Audit substrate →
price_decisionsrow (decision_stage='manual_override',source='manual_override',actor='operator',reason,chosenPrice,priorDecisionId). KeepSupportAccessAuditonly as the cross-org access log. - T2 — Override placement → inline “Edit” pencil on the qty/price rows in the FROM DOCUMENT block; the action bar stays 4 actions (no 5th button).
- T3 — Reason field → mandatory on the deviation / price-to-$0 path; optional for pure quantity corrections.
- T4 — Revert affordance → included (must-fix F12).
Deferred / not in scope
Section titled “Deferred / not in scope”- Quantifying override demand (support-DB-edit + error-route counts) — owner declined the data-pull; revisit if adoption is low.
- Splitting price-override from quantity-correction into distinct trust tiers (CEO F5) — owner chose the unified surface.
- Generalizing
accept-gross-priceto “accept any resolved price” (CEO F8 alternative) — not pursued.