# Dinex — Issues Tracker (Bad Logic & Heavy Load)

> Working backlog of known defects and performance risks, identified during the 2026-07-07 code review.
> **Rule:** when an item is fixed, change its status to `✅ FIXED (date, commit)` — do not delete rows.
> Companion documents: `BUSINESS_KNOWLEDGE_BASE.md` (full business context and the original risk register) and `SPRINT_1_UPDATES_09-08-2026.md` (change log of everything delivered since the 2026-07-11 baseline).

Status legend: `🔴 OPEN` · `🟡 IN PROGRESS` · `✅ FIXED` · `⚪ ACCEPTED` (business decided to keep behavior)

---

## A. Heavy load / blocking

### HL-1 — External HTTP calls block the cashier's request (30s timeouts) — ✅ FIXED 2026-07-10
- **Where:** `app/Services/Pos/OrderService.php` (`notifyPlatformOrderUpdated`, `approveRejectExternalOrder` → `sendOrderToNoon`)
- **What:** `DB::afterCommit` callbacks ran synchronously in the same request. Approving a delivery order waited for the platform callback (≤30s) *plus* the Noon create-task call (≤30s). Every status change on an external order had the same exposure.
- **Fix applied:** all outbound calls now dispatch to queued jobs instead of running inline:
  - New jobs `NotifyPlatformOrderUpdatedJob` (platform callback), `SendOrderToNoonJob` (Noon create-task, `onQueue('noon')`), `CancelNoonOrderJob` (Noon recall, `onQueue('noon')`) — each `ShouldQueue`, carries scalar ids only, re-resolves the order in the worker, `tries=3` with `[30,60,120]` backoff and a `failed()` logger (mirrors `ChangeOrderStatusNoonWebhookJob`).
  - `orderChangeStatus`, `orderKitchenChangeStatus`, `approveRejectExternalOrder` dispatch with `->afterCommit()` (only enqueue after the create/status transaction commits); `deliverExternalOrder` (no transaction) dispatches directly.
  - `notifyPlatformOrderUpdated` made `public` so the job can invoke it.
  - Net effect: the POS request returns immediately; the ≤30s+≤30s HTTP exposure moves to the queue worker. On the `sync` driver behaviour is unchanged from before; with a real worker (as used for Noon webhooks) it is fully async.
- **Out of scope:** `getOrderByProviderReferenceNumber` (via `trackOrderDelivery`) stays synchronous — it is a client-initiated tracking read, not a cashier action.

### HL-2 — Invoice PDF generated inside the order-create DB transaction — 🔴 OPEN
- **Where:** `OrderService::createOrder` / `orderUpdate` (SnappyPdf + Spatie PdfToImage)
- **What:** The transaction stays open while wkhtmltopdf renders and Imagick re-opens the PDF; a printing-infrastructure failure rolls back the sale itself.
- **Impact:** Long row locks per order; selling depends on PDF tooling health.
- **Suggested fix:** render the invoice after commit (or in a queued job) and update `invoice_url` separately.

### HL-3 — Order lists unpaginated with heavy relations, and polled — ✅ FIXED 2026-07-07 (query load; response unchanged)
- **Where:** `GET /v1/pos/orders/list`, `kitchen-list`, `listExternalOrders`, `view` (`app/Repositories/Pos/OrderRepository.php`, `OrderResource`, `ExternalOrderResource`)
- **What was fixed (constraint: zero change to the JSON response):**
  - Kitchen list had **no eager loading at all** — every order lazy-loaded items, images, ingredients, discount, branch, payments and payment types (~6+ queries per order). Now eager-loaded.
  - Removed wasted eager loads not present in the response (`discount.order`, `createdBy`).
  - Preloaded `paymentType.orderTypes` / `payments.paymentType.orderTypes` (PaymentTypeResource lazily queried `orderTypes` per payment row) and `orderTracker` on the external list.
  - `OrderType::find()` per serialized order replaced with a once-per-request cached name map (`OrderResource::orderTypeName`).
  - Net effect: query count per poll goes from O(orders × relations) to a fixed ~8–10 queries.
- **Still deferred (would change the response):** pagination / delta-sync (`updated_since`) — revisit with a coordinated frontend change if payload size becomes a problem.

### HL-4 — Dashboard items list N+1 — ✅ FIXED 2026-07-07 (response unchanged)
- **Where:** `app/Http/Controllers/Dashboard/ItemController.php::index`, `DBItemResource`, `Item::valueOfAttribute`
- **What was found:** worse than first assessed — besides the per-item Attachment + ingredients queries in the controller, `DBItemResource` ran per item: one `Attribute` definitions query, one `Item::find()` **per attribute**, and `valueOfAttribute()` ran two more queries per attribute (definition lookup + pivot values). ≈ N×(4+3A) queries (~1,000 for 100 items with 2 attributes).
- **What was fixed:**
  - Controller batch-loads attachments (`whereIn` + groupBy) and Eloquent items with `ingredients`, `attributes`, `image` eager-loaded; the model is passed to the resource (`eloquentModel`).
  - `DBItemResource` caches the attribute-definition list once per request and reuses the preloaded model instead of `Item::find()` per attribute.
  - `Item::valueOfAttribute()` memoizes definition lookups and reads the eager-loaded `attributes` relation (via `getRelation()`, with `->values()` to preserve JSON array shape) instead of fresh pivot queries.
  - Net effect: ~6 fixed queries total regardless of item count. Also removed the leftover `DB::enableQueryLog()` (memory growth on large responses).

### HL-5 — Items fetched twice per order line (validation + creation) — ✅ FIXED 2026-07-10
- **Where:** `OrderStoreRequest::rules()` / `OrderUpdateRequest::rules()` + `OrderService::createOrder`/`orderUpdate`
- **What:** `Item::find()` per line in validation to compute the total, then again per line in the service (2N queries per order).
- **Fix applied:** each of the four loops now preloads all lines once with `Item::whereIn('id', <ids>)->get()->keyBy('id')` and looks up from that map. Query count for the item lines drops from 2N to 2 (one in validation, one in the service — they run in separate request phases; the mobile path skips validation and does its own single fetch). Behaviour/response unchanged.
- **Note:** the validation fetch and the service fetch stay separate because the service is also the non-validated mobile-order path, so it must remain self-sufficient.

### HL-6 — Dashboard charts run correlated subqueries per branch on every load — ✅ FIXED 2026-07-10
- **Where:** `app/Http/Repositories/Dashboard/DashboardChartsRepository.php::closed_opened_branch`
- **What:** The `CASE-WHEN` in the latest-order-per-branch select ran a correlated `COUNT(*)` **and** a correlated `MIN(created_at)` subquery against the full orders table for every branch row.
- **Fix applied:** dropped the correlated subqueries. `closed_opened_branch` now runs three flat queries: (1) latest order per branch via the existing `MAX(created_at)` summary join (select only), (2) one grouped `COUNT(*)` by `(branch_id, related_date)` for the closed branches, (3) one grouped `MIN(created_at)` for the open branches. Results are matched to each branch in PHP. Query count is now fixed (3) instead of O(branches × 2 correlated scans); the response shape is unchanged. Added an early return for the no-branches case.
- **Not done (intentional):** short-lived caching was left out to avoid staleness on the live dashboard — the query rewrite removes the scaling problem on its own. The other chart methods (`TotalAndNetSalesLineChart`, etc.) do single grouped scans already and were out of scope for the correlated-subquery fix.

---

## B. Bad logic / correctness

### BL-1 — `orderUpdate` stacks discount rows and shows the oldest — ✅ FIXED 2026-07-07
- **Where:** `OrderService::orderUpdate`
- **What:** A new `discounts` row was created on every edit, never deleting the previous; the `hasOne` relation returned the *oldest* row, so the displayed discount could be stale.
- **Fix applied:** `$order->discount()->delete()` before recreating — the discount is now fully replaced on each update (consistent with how items and payment rows are wiped and re-added). Updating an order without a discount now correctly clears any prior discount.

### BL-2 — `orderUpdate` silently drops `mobile_delivery_fee` — ✅ FIXED 2026-07-07
- **Where:** `OrderService::orderUpdate`
- **What:** Recomputed totals/VAT excluded the delivery fee and the column wasn't rewritten — editing a platform order removed its delivery fee from the money figures.
- **Fix applied:** `$mobileDeliveryFee = (float) ($request->mobile_delivery_fee ?? $order->mobile_delivery_fee ?? 0)` carried through VAT (`(after_discount + fee) * 5%`) and `final_price` exactly as in `createOrder`; the fee is now written to the order row and shown on the reprinted invoice.

### BL-3 — Split-payment sum not validated on create — ✅ FIXED 2026-07-07
- **Where:** `OrderService::createOrder`

- **What:** A split whose amounts didn't add up to the final price was accepted at creation.
- **Fix applied:** ports the update flow's ±0.01 reconciliation check, guarded to split payments only (`count(payments) > 1`) — single payments are exempt because their amount is overridden to the final price in `recordOrderPayments()`.

### BL-4 — Quantity unvalidated on POS create/update — ✅ FIXED 2026-07-07
- **Where:** `OrderStoreRequest` / `OrderUpdateRequest`
- **What:** Zero/negative qty passed; since the client total used the same qty, a matching negative total got through.
- **Fix applied:** added `items => required|array|min:1` and `items.*.qty => required|integer|min:1`; the in-rules total loop now uses `$item['qty'] ?? 0` so a missing qty is caught by validation rather than fataling.

### BL-5 — `total_price` validated by fragile `in:` string comparison — ✅ FIXED 2026-07-07
- **Where:** `OrderStoreRequest` / `OrderUpdateRequest`
- **What:** Compared against `round($total, 2)` cast to string — PHP renders `100.50` as `"100.5"`, so `"100.50"` from the client failed. Intermittent false validation failures.
- **Fix applied:** replaced with `['required','numeric', closure]` doing `abs((float)$value - $computedTotal) > 0.01` — robust numeric tolerance instead of string equality.

### BL-6 — Surcharges stack at full rate on split payments — 🔴 OPEN
- **Where:** `OrderService::calculateAdditionalFees`
- **What:** Each fee-bearing payment type adds its full percentage of the *entire* order total, not of its own share.
- **Note:** confirm intended business rule before changing (may be accepted policy).

### BL-7 — `storeMobileOrder` returns the raw exception object — ✅ FIXED 2026-07-10
- **Where:** `app/Http/Controllers/Pos/OrderController.php::storeMobileOrder` (`return $e;`)
- **What:** Leaked stack traces/internal details to the platform; the proper ApiResponse line below was unreachable.
- **Fix applied:** removed the `return $e;` debug line so the `catch` returns the standard `ApiResponse` error envelope.

### BL-8 — Order-number assignment race — ✅ FIXED 2026-07-10
- **Where:** `Helpers::get_next_order_number` + duplicate check in `OrderService::createOrder`
- **What:** Check-then-insert without locking; two simultaneous creates could take the same number.
- **Fix applied:** `get_next_order_number()` now reads the highest open-day order with `lockForUpdate()`, and `createOrder`'s duplicate-number check also locks (`lockForUpdate()`), both inside the existing create transaction. Concurrent creators serialise on the same open-day rows: the loser blocks until the winner commits, then re-reads the taken number and falls through to `get_next_order_number()`.

### BL-9 — Noon integration leftovers — ✅ FIXED 2026-07-10
- **Where:** `app/Services/Pos/MarketPlaceServices/NoonIntegrationService.php`, `OrderService::orderChangeStatus`
- **What / fix applied:**
  - (a) `getOrderByProviderReferenceNumber` — removed the early `return`; the polled `status_code` now actually syncs the tracker (was dead code).
  - (b) dispatch `catch` — no longer returns the undefined `$response`; records the tracker `error` state and returns `null`.
  - (c) `cancelNoonOrder` — now wired: `orderChangeStatus` recalls the courier after commit when a delivery order already dispatched to Noon is set to `cancelled` (skipped when the tracker is already `cancelled`/`delivered`).
  - (d) webhook — `changeOrderStatusNoonWebHook` now compares numeric lifecycle ranks and skips non-advancing statuses (duplicates *and* regressions); `cancelled` is terminal.

### BL-10 — `approveReject`/`deliverExternalOrder` lack input validation — ✅ FIXED 2026-07-10
- **Where:** `OrderController::approveRejectExternalOrder`, `deliverExternalOrder` (+ `OrderService::deliverExternalOrder`)
- **What:** No validation of `id`/`approved`; bad id → null dereference (500); `deliverExternalOrder` on an order without external data crashed.
- **Fix applied:** `approveRejectExternalOrder` validates `id => required|exists:orders,id`, `approved => required|boolean`; `deliverExternalOrder` validates `id => required|exists:orders,id`. The service's `deliverExternalOrder` now asserts the day isn't closed and throws a clean error when `externalOrderData` is missing instead of dereferencing null.

### BL-11 — Delivery fee VAT treatment & reporting inconsistency — ✅ FIXED 2026-07-07
- **Where:** `OrderService::createOrder` / `orderUpdate`
- **What was wrong:** `mobile_delivery_fee` was added onto `final_price` (and its VAT) *after* the totals, but was **not** included in `price_before_discount`/`price_after_discount`. The report formulas (`gross = before_discount + vat`, `net = final − vat`) then produced **gross < net** for delivery orders, and the fee escaped the discount base.
- **Fix applied (per business decision):** the delivery fee is treated as a **net / VAT-exclusive** amount and added into the pre-discount net base *after* the 100/105 VAT extraction (not run through it), so:
  - discount applies to it, the 5% VAT is charged on top of it, and it lands inside `price_before_discount`/`price_after_discount` — restoring gross ≥ net.
  - VAT is no longer double-added (the old code added the fee to both `final` and the VAT base without extracting).
- **Business rules confirmed:** delivery fee is VAT-exclusive; it IS discountable (folded in before discount).

### BL-12 — Day-close guard doesn't exclude cancelled/rejected external orders — 🔴 OPEN
- **Where:** `app/Http/Repositories/Pos/DailySalesRepository.php::dsr()` — the `$hasNotDeliveredExternalOrders` check (~lines 91–98).
- **What:** The check 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`. **There is no `status` filter.** An external order that is `cancelled` (cashier reject, `POST /pos/orders/mobile/cancel`, or a Noon-side cancellation) but never gets `is_delivered = 1` permanently blocks day close — and unlike the active-orders guard, this one has **no `close_orders` override**.
- **Impact:** A single rejected/cancelled mobile delivery order can deadlock a branch's day close indefinitely, with no operator escape hatch.
- **Suggested fix:** exclude `cancelled` (and arguably `returned`) orders from the is-delivered check, or add an authorized override. **Needs a PO decision** — not changed here.

---

## C. Dead code / duplication

### DC-1 — Duplicate & unused repository files — ✅ FIXED 2026-07-10
- **Where:** `app/Http/Repositories/Pos/OrderRepository.php`, `app/Http/Repositories/Dashboard/ReportsRepository2.php`
- **What:** two stale duplicates of active repositories, resolvable by nobody:
  - `app/Http/Repositories/Pos/OrderRepository.php` (583 lines) — older sibling of the live `app/Repositories/Pos/OrderRepository.php` (111 lines). It `implements OrderInterface`, but `AppServiceProvider` binds `OrderInterface` → `OrderService`, and nothing imports the `Http\Repositories` namespace. The repo actually injected everywhere (`OrderService`, `NoonIntegrationService`) is `App\Repositories\Pos\OrderRepository`.
  - `app/Http/Repositories/Dashboard/ReportsRepository2.php` (348 lines) — duplicate of `ReportsRepository`; `ReportsInterface` is bound to `ReportsRepository` (no `2`) via the Dashboard module loop. Zero references; last touched 2025-07-03.
- **Fix applied:** both files deleted (recoverable via git history).
- **Verified safe:** the other repositories that showed "0 literal references" (`Branch`, `DailySales`, `Reports`, `DashboardCharts`, `OrderType`) are **not** dead — `AppServiceProvider` binds them through string concatenation (`'…\\'.$module.'Repository'`), so their names never appear literally. They were kept.

### DC-2 — Auto-deliver-on-webhook reuses the inbound webhook's Request object — 🔴 OPEN (tech debt / fragility, not dead code)
- **Where:** `app/Services/Pos/MarketPlaceServices/NoonIntegrationService.php::changeOrderStatusNoonWebHook` (~line 350).
- **What:** On the first `delivered` webhook for a tracker, the code does `$newRequest = Request(); $newRequest->id = $order->id; app(OrderService::class)->deliverExternalOrder($newRequest);`. `Request()` is a case-insensitive call into Laravel's global `request()` helper — it returns the **current, container-bound HTTP request** (the inbound Noon webhook request itself), not a fresh object, and a dynamic `->id` property is bolted onto it.
- **Impact:** Works today because `deliverExternalOrder` only reads `->id` off the object it's given. Fragile: if that method (or anything it calls) is later changed to read other request fields (auth user, headers, other input), it will silently read the *webhook's* request data instead of order-specific data — because it's mutating and reusing the live current-request singleton, not an isolated value object.
- **Suggested fix:** pass the order id (or a small DTO) as a plain argument instead of a mutated `Request`.

---

## Previously fixed in this review cycle (see BUSINESS_KNOWLEDGE_BASE.md register)

| Item | Status |
|---|---|
| Order update responded as 500 after committing (missing 2nd URL) | ✅ FIXED 2026-07-07 |
| Platform callback used test URL regardless of environment (+ correct Asparagus host mapping) | ✅ FIXED 2026-07-07 |
| Per-line fee calculation used `$itemPrice` before assignment in `createOrder` | ✅ FIXED 2026-07-07 |
| Day-close window-start query relied on implicit BranchScope | ✅ FIXED 2026-07-07 |
| Dashboard `branches/list` unauthenticated | ✅ FIXED 2026-07-07 |
| Noon dispatch depended on legacy `OrderTypesEnum` | ✅ FIXED 2026-07-07 (now `OrderType::DELIVERY` model constant; IDs 1/2/3 are company standard) |
| Status changes on day-closed orders | ✅ FIXED 2026-07-07 (orders frozen after day close; free live-day transitions ⚪ ACCEPTED) |