# KB update report — 2026-09-24, code baseline `bf2fc8e`

> Status: **Section 1 done, Section 2 done. Stopped here per instructions** — no module-file content (Section 3) has been touched yet. This report covers structure/tooling setup and the V-01…V-07 code verifications only.

## Verifications (V-01…V-07)

| ID | Finding in code (file::method) | Doc change made | Needs PO decision? |
|---|---|---|---|
| **V-01** | `DailySalesRepository::dsr()` — the `$hasNotDeliveredExternalOrders` check (~lines 91–98) runs on `(clone $this->baseOrders)`, scoped only to `day_closed=0` / `closed_at IS NULL` / `related_date IS NULL` / `branch_id`, joined to `external_orders_data.is_delivered=0`. **No `status` filter.** Confirmed: a `cancelled` external order (rejected by the cashier, cancelled via `mobile/cancel`, or cancelled at Noon) that never gets `is_delivered=1` blocks day close **with no override** — the guard has no `close_orders`-style bypass. | None yet. Logged **`BL-12` (🔴 OPEN)** in `ISSUES_TRACKER.md`. Module 06 doc change (describing this precisely, replacing the vaguer existing note) is queued for Section 3. | **Yes.** Should `cancelled`/`returned` orders be excluded from this guard, or should there be an authorized override? Today an affected branch cannot close its day at all. |
| **V-02** | `OrderService::cancelMobileOrder()` (called from `OrderController::cancelMobileOrder`) vs. `OrderService::orderChangeStatus()` (the cashier's cancel path), both read directly. **(a) Noon recall:** No — `cancelMobileOrder` does a plain `$order->update([...])` with no call to `CancelNoonOrderJob`/`cancelNoonOrder`; only `orderChangeStatus` does that (guarded on `status === CANCELLED` + an active, non-terminal Noon tracker). **(b) Closed-day freeze:** No — `cancelMobileOrder` never calls `assertOrderDayNotClosed()` (every other status-mutating method does) and isn't wrapped in `DB::transaction`. **(c) Status activity log:** **Yes** — `OrderObserver::updated()` is a model-level observer (`Order::observe(OrderObserver::class)` in `AppServiceProvider`), so it fires on *any* Eloquent `update()` regardless of call site, including this one; `updated_by` resolves to `Auth::id()`, which on a platform-authenticated request is the platform's related user (not null). **(d) Platform callback:** No — `NotifyPlatformOrderUpdatedJob` is never dispatched from this method (arguably correct, since the platform itself initiated the cancel — but worth confirming as intentional, not an oversight). | None yet. Findings queued for Module 05 Business Rules + Edge Cases in Section 3. | **Yes, recommended.** (a) and (b) look like real gaps, not intentional design: an already-dispatched Noon courier is never recalled when the platform cancels, and this is the only order-mutation path in the codebase that bypasses the day-closed freeze. Worth a PO call on whether that's acceptable or should be fixed (not changed in this pass — code only). |
| **V-03** | `OrderService::deliverExternalOrder()` / `OrderController::deliverExternalOrder()` — no order-type or provider check anywhere in either method. It validates `id` exists and that `externalOrderData` is present, then unconditionally sets `is_delivered=true` and fires the platform callback. No special-casing for Noon/Delivery-type orders. | None yet. Module 05 doc will state the actual (uniform, always-enabled) behavior in Section 3. | **Yes.** The brief states a PO decision to disable manual handover for Noon delivery orders — **this is not implemented in code.** Confirm whether that decision still stands (and should be built) or is superseded. |
| **V-04** | No `seen_at` column exists anywhere in `database/migrations/` or the codebase (`grep -rn "seen_at"` → zero matches). Only the boolean `external_orders_data.is_seen` (Sprint 1) exists, written by `OrderService::changeIsSeenStatusExternalOrder()` via `POST /pos/orders/change-is-seen-external-order`. Same check for `orders.external_dedup_key` (the As-Built Spec's claimed DB-level dedupe column, referenced by C05-13) — also zero matches; dedup is validation-only, matching the KB's existing description. | None yet. Module 05 will state plainly: only `is_seen` exists; no `seen_at`, no DB-level dedup key. | **Yes, informational.** If a "when was it seen" timestamp is wanted, `seen_at` needs a new migration — it doesn't exist today under any name. |
| **V-05** | `OrderService::createOrder()`: `DB::beginTransaction()` at line 306; `$this->getInvoiceContent(...)` (renders the PDF via SnappyPdf/wkhtmltopdf) at line 478; `new Pdf($fullFilePath)` (Spatie PdfToImage) at line 499; `DB::commit()` at line 508. `orderUpdate()` has the identical shape (begin ~528, render ~mid-function, commit 680). **Both PDF-related calls run before commit — i.e. inside the transaction**, exactly matching `HL-2`'s existing description in `ISSUES_TRACKER.md`. | None yet — `HL-2` stays `🔴 OPEN`, no status change (it was already accurate). Module 04 workflow steps 12–13 will restate this precisely in Section 3. | No — this confirms the existing doc/tracker, no new decision needed. |
| **V-06** | `NoonIntegrationService::changeOrderStatusNoonWebHook()` (~line 350): `$newRequest = Request(); $newRequest->id = $order->id; app(OrderService::class)->deliverExternalOrder($newRequest);`. `Request()` resolves via PHP's case-insensitive function lookup to Laravel's global `request()` helper — it returns the **current, container-bound HTTP request** (the inbound Noon webhook request itself), which then gets a dynamic `->id` property bolted on. Confirmed exactly as described. | None yet. Logged **`DC-2` (🔴 OPEN, tech debt/fragility)** in `ISSUES_TRACKER.md`. One neutral sentence queued for Module 08 Hidden Rules in Section 3. | No — logged as tech debt, not a decision point. Works correctly today; flagged so it isn't a surprise later. |
| **V-07** | `IngredientController::IngredientCreationCC()` / `getUnit()`: no `$request->validate(...)` call anywhere (confirmed — no field validation at all, matching the brief). `getUnit()` does `return $units[$CCUnit];` with no `isset`/`array_key_exists` guard — an unrecognized code is a PHP "Undefined array key" **Warning**, not a thrown exception, so execution continues with `unit => null`. However: the `unit` column (`database/migrations/2024_05_25_201202_add_unit_to_ingredients_table.php`) is a plain `enum(...)` with **no `->nullable()`** — so the subsequent `Ingredient::updateOrCreate([...'unit'=>null])` throws a `QueryException` (NOT NULL constraint), which **is** caught by the surrounding `catch (\Exception $e)` (Laravel's `QueryException` extends `\Exception`). Net effect: a bad unit code produces a clean `{status:false, message:<DB error>}` response, not a silent null write and not an uncaught 500. Route: `POST /v1/pos/ingredient/cc/ingredient-creation`, middleware `['cors', 'auth.platform']` — platform-authenticated only, no role/permission gate beyond that. | None yet. Module 03 + Module 10 risk-register wording queued for Section 3, refined to the precise mechanism above (not "may silently store null" — it's a caught DB error). | No — risk is real (no input validation on name/sku either) but behavior is now precisely known; documenting it is enough, no code change proposed here. |

## Items done / blocked

| ID | Status (Done / Blocked / Changed approach) | Note |
|---|---|---|
| 1.1 (build script + check + CLAUDE.md) | Done | `scripts/build-kb.sh`, `scripts/check-kb.sh` created; not yet run (module content isn't final) — will run in step 6. `CLAUDE.md` rewritten for the generated-file workflow. |
| 1.2 (R-01…R-04) | Done | Sprint 2 draft file renamed + all references updated; README rewritten with Status/Code-baseline columns and links to `KB_CHANGELOG.md`/`ISSUES_TRACKER.md`/sprint registers. |
| 1.3 (standard headers) | Done | Applied to all 11 module files. `Epic: EP-N`, `Related flags: AB-xx`, `Open decisions: OD-xx` left as explicit **TBD placeholders** — see "Decisions the PO must take" below, this is a real gap, not an oversight. |
| 1.4 (`KB_CHANGELOG.md`) | Done | Seeded with Sprint 1, the Draft period, and the 2026-07-07→07-15 and 07-10 review-cycle fixes (from `ISSUES_TRACKER.md`). Will gain more entries during Section 3 as history is stripped out of module prose. |
| Section 2 (V-01…V-07) | Done | See table above. |
| Section 3 (module reconciliation) | **Blocked — awaiting your confirmation per your instructions.** | Not started. Order once confirmed: 05, 04, 08, 06, 09, 07, 02, 03, 01, 00, 10. |
| Section 4, build, Section 7 checks | Blocked | Depends on Section 3. |

## Code says something different from this brief

- **V-01:** confirmed as described — no brief/code disagreement, just confirmation.
- **V-03:** brief says a PO decision disabled manual Noon-delivery handover; **code has no such restriction** — `deliverExternalOrder` is uniform across all external order types/providers.
- **V-04:** brief frames `seen_at` as "decided by PO, check if built"; **it isn't built under that name, and there's no partial/stub version either** — clean "decided, not built."
- **V-05:** brief frames this as KB-vs-As-Built-Spec disagreement; **code agrees with the KB/`HL-2`**, so the As-Built Spec's "post-commit printing" claim is the one that's wrong, not the KB.
- **C05-13-adjacent (surfaced while checking V-04):** the As-Built Spec's claimed `orders.external_dedup_key` DB-level unique constraint **does not exist** — dedup is validation-only, matching the KB's original (pre-brief) description, not the As-Built Spec's.

## New issues added to ISSUES_TRACKER.md

- **`BL-12`** — Day-close guard doesn't exclude cancelled/rejected external orders (🔴 OPEN). See V-01.
- **`DC-2`** — Auto-deliver-on-webhook reuses the inbound webhook's Request object (🔴 OPEN, tech debt/fragility). See V-06.
- `HL-2` reviewed, left unchanged (still accurate, still 🔴 OPEN). See V-05.

## Decisions the PO must take (new or confirmed ODs)

1. **V-01 / `BL-12`:** should `cancelled` (and/or `returned`) external orders be excluded from the non-overridable "not delivered" day-close guard, or is an authorized override wanted instead? Today there is no escape hatch.
2. **V-02:** should the platform's `mobile/cancel` path (a) recall an already-dispatched Noon courier and (b) respect the closed-day freeze, matching every other order-mutation path? Currently it does neither.
3. **V-03:** is "manual handover disabled for Noon delivery orders" still a live decision that needs building, or superseded by the automatic-handover-on-webhook behavior added in the Draft period?
4. **Blocking gap, not from V-01…V-07:** the standard module header template (brief section 1.3) calls for `Epic: EP-N`, `Related flags: AB-xx`, and `Open decisions: OD-xx`, and section 3's C10-08 asks me to align new Open Questions with "the As-Built Spec OD register (OD-01…OD-16)". **I don't have access to `DINEX_AS_BUILT_SPEC.docx`** (explicitly out of scope per brief section 4, and not supplied separately), so I can't populate these IDs — they're left as explicit `TBD` placeholders in every module header rather than invented. If you want these filled in, either share the AB/OD registry (or the docx) or confirm `TBD` is acceptable for this pass, to be filled in later.

## As-Built Spec changes needed (summary; full list in `AS_BUILT_SPEC_CHANGES.md`)

Not started — brief section 4 explicitly places `DINEX_AS_BUILT_SPEC.docx` / `AS_BUILT_SPEC_CHANGES.md` out of scope for this pass ("After the KB is done..."). Flagging now, confirmed from this session's verifications, so it isn't lost: the As-Built Spec disagrees with the actual code on at least the PDF-timing claim (V-05) and the dedup-key claim (surfaced under V-04) — both go in that later pass, not this one.