# Questions for Validation — TinyPOS (`tnx-pos`)

> Produced by the Reversa **Reviewer** (phase: review) · doc_level: `complete`
> Generated 2026-09-22. Answer each question in its **Answer** field and then type `reversa` (or "answered the questions") so the Reviewer can update the specs and reclassify the affected items.

These are the 🔴 items that only a human stakeholder can resolve — behavior/intent decisions, "dead vs unfinished" calls, and confirmations of likely production defects. Pure implementation gaps that need no decision (e.g. "add observability") are tracked in `gaps.md`, not here.

---

## Question 1 — Dashboard month chart drops every 6th day

**Context:** `DashboardController::index` month bucketing (`app/Http/Controllers/DashboardController.php:85-94`). The loop builds a 5-day window `[start, start+5)` then advances the cursor by **5 days and then one further day** (`:87,91,93`). For a 30-day month the windows are `[1,6) [7,12) [13,18) [19,24) [25,30)`, so orders dated the **6th, 12th, 18th, 24th, 30th are counted in no bucket**. The label (e.g. `01/09-06/09`) even implies the 6th is included while the SQL excludes it.
**Affected spec:** `_reversa_sdd/dashboard/requirements.md` (BR-05, RF-08), `design.md`, `tasks.md` (T-06), `contracts.md`
**Question:** Is dropping every 6th day from the month sales chart intended, or is it a bug? Should the reimplementation reproduce the gap faithfully or fix it to contiguous windows?
**Impact:** If a bug, the month chart under-counts and the reimplementation must fix the window arithmetic; if intended, the "tile the month" wording stays corrected but the behavior is kept.

**Answer:** Confirmed a bug — fixed 2026-09-22. `DashboardController.php:91` changed `< $startMonth` to `<= $startMonth` (upper bound now inclusive), closing the 1-day gap; verified by trace that a 30-day month now yields 5 contiguous 6-day windows covering every day exactly once. A separate, pre-existing month-boundary spillover (non-6-multiple month lengths bleed a few days into the next month) was found during verification and deliberately deferred as its own gap, not fixed here. See `dashboard/requirements.md` BR-05/RF-08, `design.md`, `tasks.md` T-06.

---

## Question 2 — Dashboard sales chart metric

**Context:** Each chart bucket value is `COUNT(*)` of `orders` rows in the window, regardless of `status` (`DashboardController.php:101`).
**Affected spec:** `_reversa_sdd/dashboard/design.md`, `tasks.md`, `contracts.md`
**Question:** Should the sales chart show order **count** (current), **revenue**, or **units sold**? Should it be restricted to `status='done'` orders?
**Impact:** Changes the aggregation and the meaning of the chart before porting.

**Answer:** Deferred — no decision yet. Current legacy behaviour (order count, including drafts) stays documented as-is and unchanged for now; revisit before reimplementation.

---

## Question 3 — Product image cleanup writes to a non-existent path

**Context:** On product update, the old-image cleanup does `unlink(storage_path('app/public2') . …)` (`ProductController.php:201`). `public2` never matches the real `app/public` disk root, so the unlink always fails and the failure is swallowed (`logger('FAILED …')` only). Verified real.
**Affected spec:** `_reversa_sdd/products-catalog/requirements.md`, `design.md`, `contracts.md`, `tasks.md` (T-12)
**Question:** Was `app/public2` intentional, or should it be `app/public` so old images are actually deleted?
**Impact:** Orphaned image files accumulate indefinitely (storage housekeeping); low functional impact.

**Answer:** Deferred — no decision yet. Legacy behaviour (cleanup targets `app/public2`, always fails silently) stays documented unchanged pending a future decision.

---

## Question 4 — Pricing endpoint error contract

**Context:** `GET /products/get-price` collapses **every** failure to `response()->json($e->getMessage(), 400)` (`ProductController.php:292-294`), and throws `ErrorException('Product not found')` for a **missing `product_id` parameter** (`:279-280`) — the same message used for an actually-missing product. A bad customer `id` (`Customer::findOrFail`, `:284`) also aborts the whole call with a 400 instead of degrading to retail.
**Affected spec:** `_reversa_sdd/products-pricing/design.md`, `contracts.md`, `tasks.md`, `requirements.md`
**Question:** What is the intended error contract (status codes + message shape)? Should a missing parameter be distinguished from a missing product, and should a bad customer id degrade to retail pricing rather than 400?
**Impact:** Defines a stable contract for reimplementers and stops leaking framework-internal exception text to the client.

**Answer:** Fixed 2026-09-22, all three sub-decisions resolved: (1) missing `product_id` → `422` "product_id is required", distinct from (2) unresolvable product → `404` "Product not found" (clean, no framework internals leaked); (3) a bad customer `id` now degrades to retail pricing via `Customer::find` instead of `findOrFail`. Verified zero client-visible impact — the POS terminal's AJAX call (`PosController.php:65-73`) has no `error:` handler, only `success`, so it never read status codes or error bodies either way. See `products-pricing/*.md`.

---

## Question 5 — `gift-received` page likely 500s (ambiguous `ORDER BY id`)

**Context:** `getListGiftReceived` orders by `gifts()->orderBy('id','desc')` (`CustomerController.php:349`). The `belongsToMany` query is `FROM gifts INNER JOIN customer_gift …` and **both** tables have an `id` column, so `ORDER BY id` is ambiguous → MySQL error 1052 on every row-fetch (the `paginate` count-query has no `ORDER BY`, which masks it). This is deterministic, not config-dependent.
**Affected spec:** `_reversa_sdd/customers-loyalty/design.md` (upgraded 🟡→🔴), `tasks.md`, `contracts.md`
**Question:** Is `GET /customers/{customer}/gift-received` actually exercised in production today, or does it currently error? Confirm the fix `orderBy('gifts.id','desc')`.
**Impact:** A user-facing page is very likely fully broken; the qualification must ship before this unit can be called complete.

**Answer:** Verified NOT a bug (2026-09-23). The project owner reported the page is not erroring in production; independently retested against the real database (MariaDB 10.11.13, via `php artisan tinker` in the app container) — `$customer->gifts()->orderBy('id','desc')->paginate(30)` runs the exact production SQL and returns correct rows with no ambiguous-column error. Retracting the 🔴 upgrade; no code change made. See `customers-loyalty/design.md` and `tasks.md`.

---

## Question 6 — Empty-query contract for the three scan/autocomplete endpoints

**Context:** `GET /pos/scan`, `GET /customers/scan`, and `GET settings/gifts/scan` all read `q` with no validation. An empty/blank `q` becomes `LIKE '%%'` and returns the first 10 live rows instead of an empty list (`PosController.php:759`, `CustomerController.php:285-287`, `GiftController.php:118`).
**Affected spec:** `_reversa_sdd/pos-scan`, `_reversa_sdd/customers-scan`, `_reversa_sdd/gifts-scan` (design/tasks/contracts)
**Question:** Should an empty/blank `q` return `[]`, or is returning an arbitrary first-10 acceptable?
**Impact:** The POS `select2` clients branch on array length; a default first-10 on an empty box could surface/attach an unintended customer, product, or gift.

**Answer:** Keep current legacy behaviour as-is (empty/blank `q` returns an arbitrary first-10) — no code change.

---

## Question 7 — `gifts-scan` default feed includes inactive / out-of-stock gifts

**Context:** `GiftController::scan` filters by `active` only when the `active` query param is present (`:119-121`) and never filters by `quantity_available`. Without `?active=1` the picker feed lists inactive and out-of-stock gifts; the authoritative redeemability gate is `checkGiftAvailable` at redemption time.
**Affected spec:** `_reversa_sdd/gifts-scan/design.md`, `requirements.md`, `contracts.md`
**Question:** Should the redemption picker feed be limited to redeemable (active, in-stock) gifts, or is surfacing all gifts intended (with the gate enforced only at redeem)?
**Impact:** Determines whether the picker can show rewards a customer can't actually redeem.

**Answer:** Verified not a practical risk (2026-09-23) — the project owner pointed out the UI already blocks selecting unavailable gifts; confirmed in code: the only consumer (`customer-statis.blade.php`) calls the endpoint with `?active=1` (excluding inactive gifts entirely) and its `templateResult` callback adds `.disable-div` (`pointer-events: none`) to any option with `quantity_available <= 0`, so out-of-stock gifts are shown greyed-out but not selectable. No endpoint change needed; `checkGiftAvailable` remains the authoritative server-side gate.

---

## Question 8 — "Statistics pending" vs genuinely-zero customer

**Context:** `customers-statistics` reads the nightly `customer_order_summary` via `->first()` and tolerates null with `summary->X ?? 0` (`CustomerController.php:224-234`). A brand-new customer, a customer skipped by the last run, and a failed nightly cron all render identical all-zero pages; the only cue is `statisticsUpdatedAt`.
**Affected spec:** `_reversa_sdd/customers-statistics/requirements.md`, `design.md`
**Question:** Should a `summary === null` row render a distinct "statistics pending / not yet computed" state, separate from a customer who genuinely has zero activity?
**Impact:** A silent cron failure or an unprocessed customer is currently indistinguishable from real zeros; operators may trust absent numbers as accurate.

**Answer:** Verified this already exists (2026-09-23) — the project owner pointed out the page shows "Chưa cập nhật" when the last-updated date is missing. Confirmed in code: `customer-statis.blade.php:107-111` renders `"chưa cập nhật"` when `statisticsUpdatedAt` is null (`summary === null`), distinct from a real date. This directly resolves the brand-new-customer / never-processed case. The claim was a self-contradiction within the unit's own docs (RF-07 already documented the placeholder). Residual, narrower gap left open: a customer with an *existing* summary whose next nightly refresh silently fails shows a stale-but-present date, not a prominent staleness warning — no decision requested on this narrower point.

---

## Question 9 — Categories `parent_id` is never settable through the UI

**Context:** `categories.parent_id` (nullable self-FK) exists in the schema, but neither the inline widget nor `form()` exposes it (`CategoryController.php`), so a UI-created/edited category keeps `parent_id = null`. The product dropdown lists only `Category::whereNotNull('parent_id')` (`ProductController.php:73,157`), so a UI-created category can never be assigned to a product.
**Affected spec:** `_reversa_sdd/categories/requirements.md`, `design.md`, `contracts.md`
**Question:** Is the taxonomy intended to be seed/DB-managed (the `parent_id` omission deliberate), or is the `parent_id` selector an unfinished feature that should be added?
**Impact:** Determines whether a `parent_id` control must be built, or the seed-managed model documented as the intended design.

**Answer:** Intentional (2026-09-23). The project owner's reasoning: categories are already undeletable (only `name`/`description` are editable), consistent with the taxonomy shape being deliberately fixed/DB-managed rather than reshapeable through the UI. No `parent_id` selector needed.

---

## Question 10 — Discount model / `discounts` table: dead or unfinished?

**Context:** The `Discount` model, the `discounts` table, a **live FK** `orders.discount_id` → `discounts` (`onDelete('cascade')`, `create_orders_table.php:26,28`), and `Order::discount()` `belongsTo` (`Order.php:34-37`) all exist and are internally consistent, but **no controller ever sets `discount_id`**. Checkout discounts instead flow through the free-form `orders.discount_amount` column (`OrderController.php:84,120,258,289`). The traceability matrices currently mark Discount as fully "unused."
**Affected spec:** `_reversa_sdd/traceability/code-spec-matrix.md`, `spec-impact-matrix.md`
**Question:** Was a normalized/named-discount feature started and then abandoned in favor of the free-form `discount_amount`, or is `discounts` intended to carry forward?
**Impact:** A migration/rewrite could silently drop the table + FK + relation and lose intended design, or carry dead schema forward. (The matrices will be requalified from "referenced by nothing" to "schema/model/FK wired but never populated; superseded by `discount_amount`" regardless.)

**Answer:** Keep it (2026-09-23) — the `Discount` model, `discounts` table, and the `orders.discount_id` FK are intended for future use, not dead legacy. Requalified across `code-spec-matrix.md`, `spec-impact-matrix.md`, and `domain.md` GAP-O2: "schema/model/FK wired but never populated; superseded today by `discount_amount`" — must be carried forward on any migration/rewrite, not dropped.

---

## Question 11 — Order create/finalise atomicity

**Context:** `OrderController::store`/`update` are **not** wrapped in `DB::transaction` (contrast `destroy`, which is). Points credit, `order->save()`, pivot attach/detach, and the `pos_debt` ledger post are separate statements (`:64-165`, `:206-337`). Additionally, in `update` the pivots are `detach()`ed and re-attached at recomputed current prices **before** the debt guard runs (`:291-306`), so a debt-guard rejection on a normal validation path leaves the `order_product` rows mutated while `order->save()` (`:321`) is never reached — the line items change but the header counters don't.
**Affected spec:** `_reversa_sdd/orders-crud/design.md` (Risks), `tasks.md` (T-13), `contracts.md`
**Question:** Should the create/finalise write paths be made atomic (wrap in a transaction), and should the `update` debt guard run **before** any pivot mutation? Is atomicity required before reimplementation?
**Impact:** Money/points/pivot corruption on partial writes, including on a routine validation rejection.

**Answer:** Fixed 2026-09-25 — both sub-decisions resolved yes. `store`/`update` now wrap points-award/order-save/pivot-mutation/`pos_debt`-record in a `DB::transaction`; `store` also no longer attaches pivots after a failed save. In `update`, the debt guard now runs immediately after totals are computed (in-memory) and before any pivot detach/attach, so a guard rejection leaves the order completely untouched. Verified: the existing 5-test `OrderControllerTest` suite still passes (`php vendor/bin/phpunit --filter=OrderControllerTest`), and a direct `php artisan tinker` reproduction of a debt-guard-rejected `update()` call (debt_amount far exceeding total) shows the order's pivots and `updated_at` byte-identical before/after. See `orders-crud/*.md`.

---

## Question 12 — Finalising a draft re-prices at current catalog prices

**Context:** `create_now_mode` rebuilds a draft's items from stored pivots and re-prices them at **current** `getPriceByCustomerType` prices rather than the drafted prices captured in the pivot (`OrderController.php:225-242,278`).
**Affected spec:** `_reversa_sdd/orders-crud/design.md`, `tasks.md`
**Question:** Is re-pricing a parked draft at current prices on finalisation intended, or should the draft honor the prices captured when it was parked?
**Impact:** A draft's total can silently change between parking and finalisation.

**Answer:** Intentional (2026-09-25) — no code change.

---

## Question 13 — Receipt reprints: draft printing and live points

**Context:** `GET /orders/{order}/print` has no status guard, so a `draft` order prints as a finalised receipt; and the customer block renders the customer's **current** loyalty points, not the balance at sale time (`pos-print.blade.php:48`), so a reprinted historical receipt can misstate the sale-time balance.
**Affected spec:** `_reversa_sdd/orders-print/requirements.md`, `design.md`, `contracts.md`
**Question:** Should draft orders be blocked/watermarked when printed, and should the receipt snapshot points as of sale time instead of showing the live balance?
**Impact:** Unfinalised orders can be printed as valid invoices; reprinted receipts can show wrong historical loyalty balances.

**Answer:** Two-part, both resolved 2026-09-25:
1. Draft printing — **kept intentional**, no code change (a draft prints exactly like a `done` order).
2. Points snapshot — **deferred**. Analysis: `points_awarded_at`'s existing once-only guard means a snapshot only ever needs to be written at that single moment (the `done ↔ draft` toggle doesn't add complexity, since points are awarded exactly once regardless of how many times an order flips status). However, adopting it needs a new `orders` column + migration, a decision on what to show for pre-existing orders with no snapshot, and — critically — relabeling "Điểm thưởng hiện tại" ("current" points), since showing a historical snapshot under a "current" label would be equally misleading in the opposite direction. Bigger than a simple fix; left open for a future decision. Current behaviour (live points, accurate label) stays unchanged.

---

## Question 14 — Order notes editable with no window/status guard

**Context:** `PUT /orders/{order}/note` has no `is_editable`/24h/status check (unlike `orders-crud` update), so a note is editable on any order forever, including `done` orders past the 24h edit window (`OrderController.php:342-356`).
**Affected spec:** `_reversa_sdd/orders-note/requirements.md`, `design.md`, `contracts.md`
**Question:** Is editing notes on aged/finalised orders intended (notes as free metadata), or should the note channel respect the same edit window as the rest of the order?
**Impact:** The note channel bypasses the order edit window the rest of the system enforces.

**Answer:** Intentional (2026-09-25) — notes are free metadata, not financial state. No code change.

---

## Question 15 — Login rate-limiting and failed-login auditing

**Context:** `postLogin` has no throttle/lockout, and failed logins are unlogged (`operation_log.enable=false`) (`AuthController.php:39-60`).
**Affected spec:** `_reversa_sdd/auth/design.md` (Risks), `tasks.md` (Pending Gaps), `contracts.md`
**Question:** Should the reimplementation add login rate-limiting/lockout and failed-login auditing?
**Impact:** Security posture; determines whether new infra (throttle store, auth-log table) is in scope. (Modernization decision, not a legacy-behavior gap.)

**Answer:** Not needed (2026-09-25) — no code change; kept as legacy behaviour for the reimplementation too.

---

## Question 16 — `gifts-crud`: is `used` preserved when editing a gift?

**Context:** The `saving` hook is `used = id ? used : 0` (`GiftController.php:103`). `used` is not a posted field (only a disabled display, `:93-95`), so on edit `$form->used` reads an un-posted value whose preservation depends on Encore\Admin `Form` magic-getter internals — not provable from the audited code (reclassified 🟢→🟡).
**Affected spec:** `_reversa_sdd/gifts-crud/design.md`, `requirements.md`
**Question:** Confirm (ideally with a targeted test) whether editing a gift preserves the stored `used` value or resets it.
**Impact:** If `used` resets on edit, stock accounting and the `quantity ≥ used` guard are silently corrupted.

**Answer:** Verified 2026-09-25 — preserved, not a bug. Tested with a real HTTP request against the running app: logged in with a real admin session, submitted an edit to gift id 1 (`used=1` beforehand) with the exact fields a browser would send (name/points/limit/quantity/active — `used` omitted, since it's a disabled, non-submitted field), and confirmed `used` remained `1` after the edit (only `name` and `updated_at` changed). Encore\Admin's `Form` internals correctly preserve the unsubmitted `used` value rather than nulling it.

---

## Question 17 — Debt-rule source of truth (POS client vs server)

**Context:** The POS terminal client mirrors the debt validation rules that `OrderController` also enforces server-side (`PosController.php:311-337` vs `OrderController` store/update). The two can diverge on modernization.
**Affected spec:** `_reversa_sdd/pos-terminal/design.md`, `tasks.md`, `_reversa_sdd/orders-crud`
**Question:** Which is the authoritative source of truth for debt rules, and how should the client mirror be kept from diverging (or reduced to UX-only)?
**Impact:** Determines where the guardrail logic lives and whether the client check is decorative. (Architecture decision.)

**Answer:** Decided 2026-09-25 — the server (`OrderController::store`/`update`) is the single source of truth; the client (`PosController.php`) is UX-only and has no protective value (the server always re-validates independently regardless of what the client allowed through). Three options were weighed: (A) remove the client check entirely — rejected, loses instant inline feedback for the cashier (would become a full-page-reload round trip); (C) a shared-validation endpoint the client calls before submit — rejected as too large an architecture change for this pass (new endpoint, extracted shared method, added round-trip). Went with (B): kept the duplication as-is, but added explicit cross-referencing comments in both `PosController.php` (`:314-317`) and `OrderController.php` (`:125,303`, both `store` and `update`) so a future edit to one side is more likely to prompt updating the other. This does not eliminate drift risk, only reduces the chance of it going unnoticed.