# Follow-up tickets (deliberately NOT in Batch 4) Kept separate so Batch 4 stays reviewable. None of these are blocking; all are known and scoped. --- ## T1 — Remove dead punch methods **Files:** `app/Http/Controllers/EmployeePunchDetailsController.php` | Method | Status | |---|---| | `fingerprint()` (~L31) | **No route anywhere** in `routes/api.php` | | `testingPunch()` (~L668) | Route exists but is **commented out** (`routes/api.php:176`) | Both are byte-identical clones of `recordPunch()` and still carry its original defects: no transaction around a multi-table write, and (until the payroll remediation) an unmatched `DB::rollBack()`. Their misleading rollback calls have been removed, but the methods remain. **Recommended:** delete both. **Before deleting, verify:** 1. `grep -rn "fingerprint\|testingPunch" routes/ app/ resources/` — no live references. 2. Confirm no external device firmware posts to a legacy path that still resolves to them. 3. Confirm the commented route at `routes/api.php:176` is not re-enabled by any deployment config. Deletion is destructive and needs sign-off, hence a ticket rather than an edit. --- ## T2 — Move email sending outside transactions **Files:** `app/Http/Controllers/EmployeePunchDetailsController.php` | Method | Issue | |---|---| | `store()` | `Mail::to()->send()` inside the open transaction | | `storeLocall()` | same | | `destroy()` | same | These three already have correct `beginTransaction`/`commit`/`rollBack` pairing, so they **are** atomic — this is not a correctness hole in the database. But: - The transaction is held open across an SMTP round-trip, lengthening lock windows. - Mail is not rollbackable: a later failure rolls the punch back **after** the admin has already been emailed about it. **Fix:** the same `$pendingMails` pattern already applied to `recordPunch()`, `FaceEmpController::recordPunchFacialData()` and `FaceEmpController::storeLocall()`: ```php $pendingMails[] = ['to' => $adminEmail, 'mail' => new EmployeePunchNotification(...)]; // ... after DB::commit(): foreach ($pendingMails as $p) { Mail::to($p['to'])->send($p['mail']); } ``` Ideally combine with a queued mailer so the request does not block on SMTP at all. --- ## T3 — Payroll unique constraint (audit first, do NOT add yet) **Proposed:** ```sql ALTER TABLE save_final_history_payroll_data ADD UNIQUE KEY uq_payroll_emp (empid, payrollId); ``` **Why:** nothing in `moveAllPayrollData()` filters on `isGeneratePayroll` / `isPayrollGenerate`, so re-running it for the same period would insert a second set of archive rows — double payroll. An application-level idempotency guard has been added (using the existing `record_payroll_dates.isPayrollGenerate` column), which closes the practical hole. A DB-level unique key would make it **structurally** impossible. **Do not add it yet.** First audit: 1. Duplicates: ```sql SELECT empid, payrollId, COUNT(*) FROM save_final_history_payroll_data GROUP BY empid, payrollId HAVING COUNT(*) > 1; ``` 2. NULLs in either column (a NULL `payrollId` would not be constrained as intended): ```sql SELECT SUM(empid IS NULL), SUM(payrollId IS NULL) FROM save_final_history_payroll_data; ``` 3. Same two checks on `save_emp_punch_details_records (empid, payrollId)`. Historic data may legitimately contain duplicates from before the guard existed; those must be resolved (or the constraint scoped) before it can be applied. --- ## T4 — `program_folders` is still MyISAM Not converted in Batch 4 because **no transaction writes it** — `cloneProgram()` only reads `folderId`. It is safe today, but `programs.folderId` points at it, so it should be converted whenever foreign keys are introduced. Same applies to `activity_logs` (written by `ProgramController::updateStatus()`, which has no transaction). --- ## T5 — Foreign keys are missing on the converted tables > **AUDIT COMPLETE — see `FOREIGN_KEY_RESTORATION_AUDIT.md`.** > **51 FKs missing** in production (69 present vs 120 canonical), **0 extra**, **0 type mismatches**, > **62 orphan rows across 3 constraints**. Five proposed FK-batches, plus three business decisions > that block the finance batch. Re-runnable inventory: `php artisan tinker --execute="require base_path('scripts/fk_inventory.php');"` > **Nothing was changed — audit only.** The single largest remaining structural gap, carried forward from the audit (finding N2). MyISAM **parsed and silently discarded** foreign key definitions at CREATE time. Converting the engine does **not** bring them back. So on the main database: | Database | FKs on the 8 finance tables | |---|---| | `stepup_admin_db` (converted from MyISAM) | **0** | | `stepup_test_db` (rebuilt fresh, InnoDB) | **4** | Production and a fresh install now have materially different constraint sets — and the tests run against the stricter one. Expect orphan rows (nothing was ever enforced) to block some constraints. **This needs its own audit and its own batches.** Do not bolt FKs onto an engine-conversion migration. --- ## T6 — ✅ COMPLETE `EmailProviderTest > duplicate messages are prevented across a re sync`. Originally logged as a one-line test-fixture bug (`EmailThread` created without the NOT NULL `subject`), surfaced when the test database was rebuilt to match the real schema. Fixing it exposed a genuine production defect underneath — see **T6 — CLOSED** below for the full record. --- ## T6 — CLOSED (2026-07-14) **Result: the suite is green. 94 passed, 0 failed, 0 skipped (465 assertions).** The fixture fix was the easy half. Repairing it exposed a **genuine production defect** that the failing test had been masking — exactly the case the T6 scope allowed for. ### What was actually wrong **1. The fixture (two NOT NULL fields, not one).** `EmailProviderTest.php:82` created an `EmailThread` with no `subject`, and the `Email` fixture also omitted `to_emails` (NOT NULL, JSON, cast to `array`). Both supplied; the duplicate-prevention assertion itself was left **unchanged**. **2. THE REAL DEFECT — the unique index the test asserts has never existed.** Once the fixture inserted correctly, the second insert did **not** throw. There is no unique index on `emails (account_id, internet_message_id)` — in production *or* on a fresh install. `internet_message_id` was not indexed at all. Root cause — `2026_04_03_100001_upgrade_email_hub_v2.php` swallows the failure: ```php try { Schema::table('emails', fn ($t) => $t->unique(['account_id', 'internet_message_id'], 'emails_acct_inet_msg_unique')); } catch (\Exception $e) {} // <-- silently hid a permanent failure ``` It could never have succeeded. `internet_message_id` is `varchar(500)`; under utf8mb4 that is **2000 bytes**, well past the **767-byte** index limit of InnoDB's COMPACT row format: ``` SQLSTATE[HY000]: 1709 Index column size too large. The maximum column size is 767 bytes. ``` The empty catch hid it for the life of the feature, and the test that would have caught it was red for an unrelated fixture reason. **Was duplicate prevention actually broken in production?** No — `EmailSyncService::store()` does an application-level `exists()` check on `(account_id, internet_message_id)` before inserting. But that check is **racy under concurrent syncs** and had no database backstop. ### Fix `2026_07_14_000009_add_missing_emails_unique_index.php`: - Preflight: refuses (throws with samples) if real duplicate `(account_id, internet_message_id)` groups exist. **It will not delete or merge email records to make itself succeed.** Verified: **0 duplicates** in both databases. - Switches `emails` to `ROW_FORMAT=DYNAMIC` — this server has `innodb_large_prefix=ON` and file format Barracuda, which raises the index limit to **3072 bytes**. The **full** column is indexed: no prefix index, no truncation, no silent uniqueness gap for Message-IDs sharing a long prefix. - Adds `emails_acct_inet_msg_unique`, verifies it exists and that the row count is unchanged. - Reversible (`down()` drops the index; ROW_FORMAT is left DYNAMIC, which is strictly more capable than COMPACT). Applied to both databases; rows unchanged (0 emails present). ### Outcome The assertion the test always intended now genuinely passes: the **database** rejects the twin message, not just the application. `EmailProviderTest`: **6 passed**. ### Spun out — new ticket **T7 — Migrations that swallow exceptions.** `upgrade_email_hub_v2.php` wraps numerous schema changes in `try { ... } catch (\Exception $e) {}`. That pattern is exactly what let a permanently broken index ship unnoticed. Audit every `catch {}` in `database/migrations/` and either handle the error or let it fail loudly. **No other silently-skipped schema change has been ruled out yet.** --- ## T7 — ✅ COMPLETE (2026-07-14) — Audit and Repair Schema Changes Silently Skipped by Exception-Swallowing Migrations **Result: `emails_acct_inet_msg_unique` (already repaired in T6) was the ONLY schema object a swallowed catch ever permanently hid. No further repair migrations are needed.** **Suite: 113 passed, 0 failed, 0 skipped (520 assertions).** ### 1. Inventory Nine migration files contain `catch` blocks (one is T6's own repair, which throws rather than swallows). Every guarded statement was classified: | Guarded operation | Count | |---|---| | `index(...)` — create | 16 | | `unique(...)` — create | 2 | | `dropIndex(...)` / `dropUnique(...)` | 10 | | **column / FK / raw `ALTER` / `Schema::create` / `dropColumn`** | **0** | **No catch anywhere guards a column, a foreign key, a raw `ALTER`, or a table create/drop.** They only wrap index/unique creation and defensive drops. That bounds the blast radius: the worst a swallowed catch in this codebase could ever have hidden is a missing index. The 10 drops are legitimately defensive (a `dropIndex` in `down()`, or a "may not exist" cleanup) and carry no risk. ### 2. Verification — declared intent vs. reality, in BOTH databases `scripts/swallowed_schema_audit.php` checks all **18** catch-guarded creates against production **and** a fresh install, comparing name, columns and uniqueness. > This is the key method. A swallowed failure is invisible to a prod-vs-fresh diff (T5's > technique) because it fails in *both*. It can only be found by comparing what the migration > **declares** against what actually **exists**. ``` OK: 18 Missing in BOTH: 0 Drift: 0 No swallowed schema object is missing. Every declared index/unique exists in both databases. ``` The method is self-validating: run before T6, this same script flags `emails_acct_inet_msg_unique` as `*** MISSING IN BOTH ***`. ### 3. Repairs **None required.** The only ever-hidden object — `emails_acct_inet_msg_unique` — was repaired in T6 by `2026_07_14_000009_add_missing_emails_unique_index.php`, which is additive, has a fail-loud duplicate preflight, and is independently reversible. Both production and fresh installs now have it, so no historical migration needed correcting for fresh-install parity. **No production data was read into, deleted, merged, truncated or normalised.** This ticket was schema-only; row counts were untouched throughout. ### 4. Tests `tests/Feature/SchemaIntegrityTest.php` — **19 passed** - **18 data-provider cases**, one per catch-guarded object, asserting it exists with the exact columns and uniqueness its migration declares. A future swallowed failure now fails CI instead of shipping silently. - **1 enforcement test**: `crm_email_templates.slug` unique actually *rejects* a duplicate — an index that exists but doesn't enforce is worthless. (The `emails (account_id, internet_message_id)` counterpart is already exercised end-to-end by `EmailProviderTest::duplicate_messages_are_prevented_across_a_re_sync`.) ### 5. The catches themselves — left in place, deliberately They were **not** removed. Their intent (idempotency: "skip if the index already exists") is legitimate, and rewriting eight historical migrations to fail loudly risks breaking fresh installs for no benefit now that every object is verified present and pinned by CI. The real fix is the **test**, not the catch: the pattern's flaw is that it cannot distinguish "already exists" from "can never be created", and no amount of care in the migration fixes that. A test that asserts the end state can. **Recommended for new migrations:** check existence explicitly rather than catching — ```php if (!$this->indexExists('emails', 'emails_foo_idx')) { Schema::table('emails', fn ($t) => $t->index('foo', 'emails_foo_idx')); } ``` ### Verification command ```bash php artisan tinker --execute="require base_path('scripts/swallowed_schema_audit.php');" # → OK: 18 Missing in BOTH: 0 Drift: 0 ``` --- ## T1 — ✅ COMPLETE (2026-07-14) — Removed the unrouted dead punch clones **1,262 lines deleted. Suite unchanged: 113 passed, 0 failed, 0 skipped.** ### Pre-deletion verification (the ticket's checklist) | Check | Result | |---|---| | Any registered route resolving to `EmployeePunchDetailsController@fingerprint`? | **None** (`php artisan route:list`) | | Any registered route resolving to `@testingPunch`? | **None** | | Any caller anywhere in `routes/`, `app/`, `config/`, `resources/`? | **None** | | Could a device be hitting a legacy path? | **No.** The only `fingerprint` URI is `POST api/update/fingerprint`, which resolves to **`EmployeeBiometricsController@store`** — a different controller. It was never these methods. | | Is the commented route at `routes/api.php:176` pending re-enablement? | **No.** It duplicated the path on the line *above* it, which is already bound to `store()`. It was superseded, not parked. | Every other `fingerprint` hit in the codebase is a **column name** (`leftthumb_fingerprint` / `rightthumb_fingerprint` on `employee_biometrics`) and is unrelated. ### What was removed `app/Http/Controllers/EmployeePunchDetailsController.php`: - `fingerprint()` — lines 31–671 - `testingPunch()` — lines 672–1292 Both were byte-identical clones of `recordPunch()` and still carried its original defects: a multi-table write with **no transaction**, and a `DB::rollBack()` that rolled nothing back. They had also been silently left behind by every earlier fix — a standing trap for anyone who copied from them or re-routed them. The commented-out route at `routes/api.php:176` was removed with them, so no dangling reference to a method that no longer exists remains. A short marker comment was left at the deletion site so the removal is discoverable from the file itself, not just from history. File: **2,931 → 1,674 lines.** ### Post-deletion verification | Check | Result | |---|---| | PHP syntax | clean | | All 8 remaining methods still routed | `recordPunch`, `index`, `getEmp`, `getImg`, `dashboard`, `store`, `storeLocall`, `destroy` — **all ✓** | | Dangling references to the deleted methods | **none** | | Orphaned `use` imports left behind | **none** | | Transaction pairing intact | `recordPunch` / `store` / `storeLocall` / `destroy` — all `begin=1 commit=1`, **zero orphan rollbacks** | | Live device endpoint still works | `PayrollPunchTransactionTest` drives the real `POST /api/punch/details` end-to-end — **passes** | | Full suite | **113 passed, 0 failed, 0 skipped (520 assertions)** — identical to before | The `PayrollPunchTransactionTest::test_no_controller_calls_rollback_without_beginning_a_transaction` invariant still passes, and now has two fewer offenders to forgive. --- ## T2 — ✅ COMPLETE (2026-07-14) — Mail moved outside the punch transactions **5 mail sites deferred, 3 catches widened. Suite: 114 passed, 0 failed, 0 skipped.** ### What was wrong `store()`, `storeLocall()` and `destroy()` had correct `beginTransaction`/`commit`/`rollBack` pairing, so they were already atomic — but they sent notification mail **inside** the open transaction. Two consequences: - Mail is not rollbackable: a later failure rolled the punch back **after** the admin had already been emailed about it. - The transaction was held open across an SMTP round-trip, lengthening lock windows. ### Fix — the established `$pendingMails` pattern Every punch workflow now follows **BEGIN → collect → COMMIT → SEND**: ```php DB::beginTransaction(); $pendingMails = []; try { ... writes ... $pendingMails[] = ['to' => $adminEmail, 'mail' => new AdminEditPunchNotification(...)]; DB::commit(); // Only now that the punch is durably committed do we notify anyone. foreach ($pendingMails as $pending) { Mail::to($pending['to'])->send($pending['mail']); } } catch (\Throwable $e) { DB::rollBack(); ... } ``` | Method | Mail sites deferred | |---|---| | `store()` | 2 (Punch-Out, Punch-In admin edit) | | `storeLocall()` | 2 (Punch-Out ×2) | | `destroy()` | 1 (punch deletion) | Verified ordering across the whole controller — every method is now `BEGIN → collect → COMMIT → SEND`, with zero raw `Mail::to()` inside any transaction window. `recordPunch()` and both `FaceEmpController` punch methods already used this pattern. ### Also fixed (same defect class, in scope because it strengthens the DB rollback) The three transaction-bearing catches used `catch (Exception $e)`, which does **not** catch a `TypeError` — so an `Error` inside the transaction would skip the rollback entirely. Widened to `catch (\Throwable $e)`, matching `recordPunch()`, `moveAllPayrollData()` and `FaceEmpController`. **Transaction pairing is unchanged**: `store`, `storeLocall`, `destroy`, `recordPunch` all remain `begin=1 commit=1`, zero orphan rollbacks. Rollback behaviour for database writes is untouched — only the *mail* moved. ### Test — a real regression guard, not a vacuous one `PayrollPunchTransactionTest::test_no_controller_sends_mail_inside_an_open_transaction` scans `EmployeePunchDetailsController`, `EmployeePunchRecordController` and `FaceEmpController` and fails if any raw `Mail::to()` appears between a `beginTransaction()` and its `commit()`. **Proven non-vacuous:** run against the pre-T2 file, the detector reports exactly the 5 sites that were fixed — ``` EPDC.t2.bak: 5 mail-inside-transaction offender(s) - store() line 908 - store() line 1069 - storeLocall() line 1237 - storeLocall() line 1369 - destroy() line 1613 ``` — and 0 against the current code, and 0 against `FaceEmpController`. One subtlety worth recording: the detector must **not** close its window on `DB::rollBack()`. An intermediate rollBack inside an if/else branch does not end the transaction for the rest of the method, and treating it as a close made the first version under-report (it found 1 of the 5). The window runs from `beginTransaction()` until `commit()` or the end of the method. ### Remaining Mail is still sent **synchronously** after commit, so the request still blocks on SMTP. Moving to a queued mailer would remove that too — worth doing, but it is a performance change rather than a correctness one, and is not required for T2. --- ## T3 — ✅ COMPLETE (2026-07-14) — Payroll uniqueness enforced **Suite: 130 passed, 0 failed, 0 skipped (574 assertions).** ### Audit first (scripts/payroll_uniqueness_audit.php) | Table | Engine | Rows | Duplicate (empid, payrollId) | NULLs | |---|---|---|---|---| | `save_final_history_payroll_data` | InnoDB | 0 | **0** | **0** | | `save_emp_punch_details_records` | InnoDB | 0 | **0** | **0** | **VERDICT: CLEAN.** No historical double-payroll rows to reconcile. ### But a unique index ALONE would have been cosmetic The audit's NULL analysis — the part the ticket insisted on — is what made this ticket worth doing properly. **Both key columns were NULLABLE**, and MySQL permits unlimited NULLs in a unique index, so any NULL row is exempt from it. NULLs really could get there: ```php 'payrollId' => 'nullable|exists:record_payroll_dates,id', 'payrollList.*.employee_id' => 'nullable|integer', ``` Worse, the idempotency guard added earlier reads `if ($payrollId && ...)` — **a NULL payrollId skips the guard entirely.** So a request omitting `payrollId` would have bypassed *both* protections at once, and double payroll would still have been possible with the index in place. Enforcement therefore needs three parts, not one: 1. `empid` + `payrollId` → **NOT NULL** (safe: 0 rows, 0 NULLs) 2. **UNIQUE (empid, payrollId)** on both archive tables 3. Validation tightened to **`required`** — without it, a request omitting `payrollId` would turn from a silently-wrong insert into a raw 500. With it, the existing clean validation response is returned. ### Migration — `2026_07_14_000010_add_payroll_uniqueness_constraints.php` Preflight re-verifies duplicates and NULLs against live data and **throws rather than deleting, merging or normalising a single payroll row**. Fully reversible: `down()` drops the indexes and restores nullability, a true inverse. Applied to both databases; row counts unchanged (0). | Table | Constraint | |---|---| | `save_final_history_payroll_data` | `uq_final_payroll_emp (empid, payrollId)` | | `save_emp_punch_details_records` | `uq_emp_punch_payroll (empid, payrollId)` | Only `moveAllPayrollData`'s validation was tightened. `editPayrollData` also has a nullable `payrollId` but does not write the archive tables, so it was left alone. ### Tests - `payroll archive rejects a duplicate empid and payroll` — the **database** rejects the twin row (`Duplicate entry ... for key 'uq_final_payroll_emp'`), not just the application guard. - `payroll key columns are not nullable` — pins the NOT NULL, because a unique index over nullable columns does not actually constrain. Double payroll is now structurally impossible, not merely unlikely. --- ## Cross-test defect found and fixed while running T3 The full suite surfaced a failure in `ClassScheduleVisibilityTest` — a test class added outside this workstream. **The cause was mine.** `ModuleTransactionTest::tearDown()` deleted permissions **unscoped**: ```php DB::table('permissions')->where('permissionGroup', 'TEST')->delete(); // <-- not scoped to MARK ``` It removed permission rows other test classes had created under the same group — and since T5 added `role_permissions.permissionId → permissions.id ON DELETE CASCADE`, deleting those permissions now **cascades away their grants too**. The FK restoration turned a latent, tolerated mess into a visible cross-test failure. Fixed by removing the permission deletion entirely: the permissions this class touches (`System_Admin`, `Program_View`) are real seeded rows, and only the *grants* are ours — those are already removed with the role. `ClassScheduleVisibilityTest`: **14/14 passing.** Worth recording as a lesson: a test tearDown must be scoped as tightly as its setUp, and the FK work made that a hard requirement rather than a nicety. --- # Class import / Today-page workstream (2026-07-29 onward) Items surfaced by the Today-page divergence investigation and the class-backup re-import cleanup. Grouped by hazard, not chronology. --- ## T12 — [DEPLOY CONSTRAINT] A1 (`legacy_slot` format) requires the wipe **Files:** `app/Services/BackupImport/ClassImportValidator.php`, `database/migrations/2026_07_27_140000_add_legacy_import_keys_staff_classes.php` A1 changed `legacy_slot` from a two-format scheme (3-part for REGULARLY, 4-part for PERIOD/SINGLE) to a single 4-part format (`|||`). This is what makes `updateOrCreate` genuinely idempotent across re-imports. **Constraint:** A1 must NOT reach any environment without the wipe following in the same operational unit. Running A1 against the current data collides with all 75 previously-imported staff and triples the ~205 duplicate slot count. Treat A1 → wipe → clean re-import as one atomic step. Commit body and code comment both call this out; reviewer should reject any deploy PR that includes A1 without a queued wipe step. --- ## T13 — [BLOCKS A2] A11: guard the generic generator against imported rows **Files:** `app/Services/ClassOccurrenceService.php`, `app/Services/ClassSeriesEditService.php`, `app/Http/Controllers/classesAssignController.php` (assignStudent, duplicateClass), `app/Console/Commands/GenerateFutureClasses.php`, `routes/console.php` After A2 lands the per-date roster gate for imports, a single `generateForClass()` call on an imported class regenerates it from `assign_classes_schedules` with the blanket roster and silently reintroduces the phantom students A2 removed. Trigger surfaces already in-tree: `duplicateClass(series)`, `seriesEdit(scope=series)`, `assignStudent(scope=future)`, and the `classes:generate-future` cron (currently commented out at `routes/console.php:31-42` for this exact reason). **Rule:** `ClassOccurrenceService::generate()` (or its callers) must refuse to run the generic blanket-roster path when `$class->import_source === 'legacy_backup'`. Options: refuse outright and require callers to pass an explicit per-date roster; log-and-skip; fall back to a legacy-schedules-derived pair set. Pick one deliberately. **Must ship in the SAME COMMIT as A2.** Late-shipping A11 means one cron firing or one manual `duplicateClass` between A2's landing and A11's landing degrades every imported class it touches. Re-enable the cron block in `routes/console.php` as part of this ticket's commit, not before. --- ## T14 — FE rename: `ensureOccurrence` response key **Files:** frontend consumer of `POST /api/class-schedule/ensure-occurrence` (planner/attendance flows; grep for `data.occurrences` in the FE). B1 renamed the response's legacy-schedule collection from `data.occurrences` (misleading — those were `class_schedules` rows, and callers were passing them into occurrence-based endpoints where an id collision hit the wrong row) to `data.legacy_schedules`. Real `class_occurrences` continue under `data.class_occurrences`. The rename is a breaking API change. Passes CI, fails in the browser. Needs a matching FE commit before B1 reaches production — do not deploy B1 solo. --- ## T15 — `editThisOccurrence` multi-student mirror staleness **File:** `app/Services/ClassSeriesEditService.php` (~L68), consumed by `ClassScheduleController::seriesEdit(scope=this)`. `class_schedules` is a per-STUDENT mirror table — a 6-student session has 6 rows on the tuple `(classId, scheduledDate, startTime, endTime)`. `legacyScheduleFor()` in the controller returns `first()`. The service updates that single passed model and mirrors to the ClassOccurrence, but leaves the other five student rows stale. Not a B1 regression; pre-existing. A2 partially self-resolves this for imported classes (rewrites the mirror from the pair set); manual edits on non-imported classes keep the hazard. **Fix:** either (a) `legacyScheduleFor()` returns a collection and the service iterates all matches, or (b) `editThisOccurrence` re-keys on the tuple itself and updates every matching row. --- ## T16 — Group B read-path items (batched) Independent of the import wipe. In rough priority order: - **`eligibleFutureSeries()` missing weekday** — matches `class_id + start + end` with no weekday. `remove_future` on a Mon+Wed class silently hits both. File: `ClassScheduleController.php`. - **Hybrid counter buckets** — `Hybrid` counted into both `Online` and `In-Person` counters at `TimetableController.php:329-331`, so the sum OVERSHOOTS total. Make three mutually exclusive buckets (Online / In-Person / Hybrid summing to total). Filter can stay `whereIn(['Online','Hybrid'])`. - **Location filter fallback** — `applyPhase2Filters()` filters `class_occurrences.location_id` while the row output falls back to `assign_classes.location_id`. Diagnostic §8b confirms 40 rows on 07-31 have `location_id = 0/null` (real, not hypothetical) — they vanish under any location filter. - **`destroy()` / `store()` dual-write gap** — §5 / §6b currently clean, so latent. A manually-deleted `class_schedules` row would leave the occurrence; a manually-created one would never reach the Today page. Route creates through `ClassOccurrenceService`; soft-delete the occurrence alongside `class_schedules` on destroy. - **`store()` id handling** — `$request->has('id')` → `filled('id')` (null currently 500s). Extend occurrence-level conflict checks to the UPDATE path — only checks on create today; the fallback `findConflicts()` reads `class_schedules` and can't see zero-student occurrences. - **`total_classes` scope drift** — reads `$selOccs` (includes cancelled) while mode counters read `$selAlive`. §8 shows 0 cancelled today so not manifesting, but wrong. Also surface `cancelled_classes` next to the mode buckets. - **Mode vocabulary** — `Online / Offline / Hybrid / InPerson / online / in_person` all appear. Normalize behind a helper. - **`notifications::create(['userId' => $student_id])`** at `notifyOccurrenceStudents()` — writes `students.id` into a `users` FK. Currently caught by try/catch that logs. - **`className` concatenation in `index()`** — ships the concatenated className while `details()` deliberately refuses to (see its `safeDisplayName` comment). Give the list the safe label. --- ## T17 — N+1 hotspots (report only, no refactor yet) - `makeupOptions()`, `replacementStaff()`, `computeStaffAvailability()` — ~500-800 queries per request at current volumes. - `filterOptions()` — unbounded `whereIn` over every occurrence id ever generated. Scale poorly post-wipe. Address after A-series lands; don't gold-plate in flight. --- ## T18 — [PRE-WIPE INVESTIGATION] Live's sick/absence source is unidentified Live renders orange "- Sick" cards on all six days sampled. No `status` field on `legacy.day_details`; no attendance/absence table has been enumerated. Either there is a legacy table nobody has mapped, or something in local already writes this state. **Why it matters for the wipe:** if a table was missed, the migration is incomplete in ways we don't yet know. Answering this is a completeness check on the entire migration, not just makeup/attendance. Pre-wipe list must include a "count all legacy tables read by importers vs list of legacy tables that exist" reconciliation. **Also open here:** - `day_details.details[].present` (0/1) is unvalidated as attendance. 67% "absent" cluster is implausible — likely reflects unmarked future rows. Split past/future before anyone builds on it. - Should historical makeups migrate at all? Legacy's last makeup target is 2026-07-20 and all resolvable rows are `released`. Product decision — may be nothing operationally live to carry over. --- ## T19 — Makeup importer UX + reader hazard **File:** `LegacyMakeupClassImportService` / makeup importer read path. Two separate issues: 1. **UX** — date fields on the makeup import page filter on makeup date, not original missed-class date. Confirm label matches semantics; empty state should hint at cache-the-count rather than rescan. 2. **Reader hazard (unverified — needs diagnosis before code)** — plausible bug: the reader may filter original `day_details` rows to the selected date range before resolving `makeup_id`. If so, a makeup reference on an older missed-class row (whose `details[].makeup = "1"` and `makeup_id` points to a session inside the range) is missed. Correct order: locate original references → extract `makeup_id` → load referenced rows → apply date filter to REFERENCED row's `cdate`. Verify with read-only diagnostic queries first; do not change UI until API returns real records. Handle both `"makeup":"1"` (string) and `"makeup":1` (integer) in the JSON. Deferred until after the main class re-import lands. --- ## T20 — [READ HAS WRITE SIDE EFFECT] `previewClass` auto-creates pivot rows **File:** `app/Services/BackupImport/ClassImportValidator.php` (previewClass → LegacyGradeSubjectMapper::ensureMappingExists at `app/Services/BackupImport/LegacyGradeSubjectMapper.php:153`). `preview()` runs with `autoCreateMappings = true` by default (line 45), so the mapper's `resolveTokens()` can (and does) INSERT rows into `subjects_grades_tables` from what is nominally a read/preview operation. A5 closed the most visible symptom — an invalid-time slot that was about to be blocked no longer runs through the mapper first, so it doesn't leave orphan pivot rows behind. But the underlying design smell persists: any preview of any staff can silently backfill the pivot table without the caller intending a write, which: - Makes `preview()` non-idempotent across environments (calling it against prod data mutates prod). - Means "validate-only" runs (LegacyClassImportService::validateStaff) do in fact write, contrary to the method's contract. - Complicates rollback — an aborted or blocked run may still have added rows the wipe query doesn't target. **Options:** 1. Split `resolveTokens()` into a read-only path (returns null for missing mappings) and an explicit write path called only from the import writer. 2. Gate `autoCreate` off during preview/validate; keep it on for actual import. 3. Move pivot backfill into a separate explicit step (`preview` returns the list of would-create rows; `import` materializes them). Preferred: (2), because it's the smallest change and matches the current call site's actual intent — the write path already goes through `importStaff` which can flip autoCreate back on. Not blocking any of the A-series; captured because side-effecting reads are the class of bug that resurfaces indefinitely otherwise. --- ## T22 — Eligibility gate should require future work UNDER SCOPE **File:** `app/Services/BackupImport/LegacyStaffClassReader.php` (`importableStaffQuery`, added in A0). Current logic (OR-eligibility): ``` active AND ( reg_classes non-empty OR any future day_details row ) ``` This admits staff whose future work is empty **under the current `IMPORTED_C_TYPES` scope**. Under A4 (`REGULARLY` only), the post-wipe 2026-07-29 run showed three staff who were "expected" but produced zero classes because they had no REGULARLY future rows at all: - 114 Chantelle Philips — 0 future day_details total - 226 Aliya Wallace — 0 future day_details total - 241 Samantha Davies — 0 future day_details total All three are correctly blocked with "No future class sessions from 2026-07-29 onwards", but they should not have entered the expected set in the first place. The reader's `reg_classes` clause is descriptive provenance, not a work indicator. **Fix:** tighten to `active AND has-future-day_details-under-IMPORTED_C_TYPES`. `reg_classes` stays available as a display column, but stops gating. **Scope-dependent side effect (important):** the expected count changes whenever `IMPORTED_C_TYPES` changes. - Post-wipe run under A4 (REGULARLY only): **expected = 62**. - After T22: **expected = 59** (drops the three above). - After PERIOD comes back into scope: expected rises again, and the three currently-`fully_deferred` staff (Ayesha 268, Anu 269, Kristika 270) move into the expected set automatically — correct behaviour, but the 62/59 figures MUST NOT be hardcoded as a stable baseline anywhere. Every reconciliation report is scope-scoped. Record both numbers in the runbook so a future reader doesn't read the 59 as a regression from this run's 62. --- ## T23 — A10 verifies the wrong predicate **File:** `app/Services/BackupImport/ClassImportLogger.php` (`reconcile`). Current check: **"was every expected staff attempted?"** — implemented as `expected_set − attempted_set == ∅`. `attempted` includes staff whose terminal status is `blocked` or `failed`. The check you actually want: **"did every staff with importable work produce output?"** — those coincide only when nothing blocks. When a staff is blocked (e.g. by an unmapped catalogue token, malformed time, missing future work), the current predicate files them as OK. A11's purpose was to guarantee no imported occurrence gets silently mis-generated; A10's purpose is to guarantee no expected staff gets silently produced-nothing. The current implementation only guards the first half of that. The post-wipe run illustrated it: 62 expected, 62 attempted, 4 blocked producing zero classes each, reconciliation reported `has_delta = false`. That's the exact failure shape (a therapist absent from the Today page with the reconciliation reporting OK) the ticket was built to prevent. **Fix** — rewrite `reconcile` to compare two different sets: ``` expected_with_work = { active staff with future day_details under IMPORTED_C_TYPES } (per-scope; T22 makes this the same set that seeds importableStaffQuery) produced_output = { staff whose staff_log has classes_created + classes_updated > 0 } missing_output = expected_with_work − produced_output has_delta = missing_output not empty ``` `missing_output` should be surfaced with the same prominence as `missing_staff` is today. Existing `missing_staff` (never attempted) stays as a distinct block — it's a different failure shape (importer skipped them entirely) from `missing_output` (importer tried and got nothing back). Both are red flags. Ties into T22: once eligibility becomes work-based, `missing_output` becomes the primary check and `missing_staff` becomes vestigial (no path can skip a staff who's genuinely eligible unless the runner itself malfunctions). --- ## T26 — Legacy student double-booking (data-quality; 27 pairs Zaeen Nawaf) **Data:** post-wipe run #6, `class_backup_import_staff_logs.warnings_json` for legacy_staff_id=271 (Roma Sallen), plus corresponding pivot rows on Shiyannie Waran (legacy_staff_id=248). The run's `cross_class_time_overlap` detector fired 118 times across all 62 staff; 27 of those are Zaeen Nawaf (live student 13212) simultaneously enrolled with Roma Sallen (09:00–12:00) AND Shiyannie Waran (10:30–11:30) on 27 dates through 2027-01. Half-open interval overlap: `[09:00, 12:00) ∩ [10:30, 11:30) = [10:30, 11:30)` — genuine time collision, not back-to-back. **Import behaviour (correct per A2 contract):** both pivots imported; each attach warned; no dedup. Legacy is the source of truth and legacy records both as active enrolment. **Consequences that need decisions:** 1. **`studentOccurrenceConflict` guard will reject edits.** The new system's own conflict guard would refuse to CREATE this pair through the UI. Anyone editing either session may hit the guard and be unable to save data that imported fine. Two options: - Grandfather imported conflicts (add an `is_grandfathered` flag or check `import_source` before firing the guard). - Fix the underlying data — decide which enrolment is correct, remove the other. 2. **Billing derives from pivots.** Both pivots have `fee_per_class` set from legacy's `fees`. Every one of those 27 dates, Zaeen accrues fees against BOTH sessions (once with Roma, once with Shiyannie). Confirm this is intended — could be dual-service billing (SLP + ABA overlapping on paper), or could be duplicate billing that legacy tolerated silently. Live's rendering of Zaeen "twice on Jul 31" strongly suggests billing was single-counted somehow, but the mechanism isn't in the code we've reviewed. 3. **A user visiting either session sees Zaeen in both.** The Today page will render Zaeen at 09:00-12:00 with Roma AND at 10:30-11:30 with Shiyannie. From a "who's in this room" perspective, one of those has to be wrong. **Full list of overlap cases (not just Zaeen):** run's staff_logs contain 118 `cross_class_time_overlap` warnings across the whole run — the same detector fires for other students too. Query: ```sql SELECT legacy_staff_id, warnings_json FROM class_backup_import_staff_logs WHERE run_id = AND warnings_json LIKE '%cross_class_time_overlap%'; ``` **Cross-check against live.** Live shows Zaeen in Roma's blocks on Jul 30/31 per screenshots, but I don't have a screenshot showing Zaeen concurrently under Shiyannie. Someone should verify whether live actually renders both teachers or just one — the answer indicates whether legacy's overlap is a data error tolerated by live's rendering, or a real dual-enrolment. Product decision required before UI guard is reconciled or billing is generated for the affected dates. --- ## T25 — [BLOCKS DAILY USE] Today-page "deferred by scope" banner **Files:** frontend Today-page header (backend already provides the data). Under A4's `IMPORTED_C_TYPES=['REGULARLY']` scope, **398 legacy sessions across 16 named staff are absent from the Today page by design**. Anyone using the page tomorrow will notice caseload gaps and report them as bugs unless the page tells them scope is deliberately narrow. The data is already in the backend response: - Console command prints a "Deferred by scope" list at run end. - `class_backup_import_runs.summary_json.reconciliation.out_of_scope_staff` is the persisted named list, split into `partial_scope` (imported for REGULARLY, PERIOD portion deferred) and `fully_deferred` (Ayesha, Anu, Kristika — no REGULARLY at all). **Fix:** Today-page renders a persistent banner: "N sessions across M staff are excluded from the current import scope (REGULARLY only). Expand" → drawer with the named list from the latest run's reconciliation payload. Notable named staff who will be conspicuously missing: - Ayesha Motiar — entire Thursday caseload - Anu Sebastian — entire Thursday caseload - Kristika Raguthas — entire Thursday caseload - Katrina Ramjattan — Thursday/Sunday PERIOD portion (~57 sessions) - Kevin Sookraj, Nzambi Teixeira, Deeksha Gogia — PERIOD portions **Do not roll this into T14** (`ensureOccurrence` rename) — different consumer, different urgency. But treat it as equally blocking for production use. --- ## T24 — `schedule:diagnose` §4a stale under A2's per-pair semantics **File:** `app/Console/Commands/DiagnoseSchedule.php` (§4 coverage gap). Pre-A2, the generator created a `class_occurrence` for every date whose weekday matched `assign_classes_schedules.day`, regardless of enrolment. §4a flagged the shape "template says today, no occurrence generated" as a defect — under the old blanket-roster path it usually meant the generator was skipping. Post-A2, imported classes only create occurrences for dates in their `active_pairs` set. A template whose weekday matches today but has no pair for that date correctly produces no occurrence — that used to be a phantom zero-student block (C2 in the original diagnostic), which A2 deliberately eliminates. §4a still flags these as gaps. Post-wipe 07-29 count: 39 rows (mostly this pattern). Not a defect; diagnostic is stale. **Fix:** §4a should exclude templates where `import_source='legacy_backup'` AND the class has no legacy pair for the checked date. For non-imported classes the check still means what it used to. Cheapest expression: rename §4a to "template scheduled, occurrence absent — for imported classes this is normal on dates the legacy enumeration excludes; for non-imported it is a generator defect worth investigating." Or split into two subsections. --- ## T21 — Imported-class horizon monitor **File:** new artisan check + optional cron. A11 makes the generic `classes:generate-future` cron skip `import_source='legacy_backup'` rows (they'd throw otherwise, and the top-up is wrong for them anyway — legacy pre-materialises the horizon). Consequence: imported classes now **never** get topped up. When legacy's horizon (currently 2027-01-31 for REGULARLY) runs out, imported classes just stop, with no cron and no warning. **Symptom:** the Today page slowly empties out months in the future, with no error at any point. First user-visible sign is probably a therapist noticing "next week is empty" on the calendar. **Fix (monitor):** artisan check that reports imported classes whose latest occurrence is within 30 days of today, e.g. ```php SELECT ac.id, ac.staff_id, MAX(co.scheduled_date) AS last_occ FROM assign_classes ac LEFT JOIN class_occurrences co ON co.class_id = ac.id AND co.isDelete = 0 WHERE ac.isDelete = 0 AND ac.import_source = 'legacy_backup' GROUP BY ac.id, ac.staff_id HAVING last_occ IS NULL OR last_occ < DATE_ADD(CURDATE(), INTERVAL 30 DAY); ``` Run weekly; alert if any rows come back. **Fix (structural):** either extend the legacy re-import to run periodically against a fresh legacy dump, or migrate imported classes to `import_source = 'manual'` at some point in their lifecycle so the cron picks them up. Product decision — needs an answer before Jan 2027. --- ## T27 — Retire `programs.tags` JSON column (one release after Batch 14) **Files (to remove or amend when retiring):** - `app/Http/Controllers/ProgramController.php` - `store()` — stop writing `$payload['tags'] = json_encode(...)` (the `syncProgramTagsToPivot` call becomes the sole write path) - `index()` and `libraryMetrics()` — stop returning the `tags` field in the response (leave `tagsWithColour` as the only tag shape) - `database/migrations/_drop_tags_column_from_programs.php` — new migration dropping the `programs.tags` column - `frontend/src/Pages/ProgramLibrary/List/index.tsx` — remove the `tags` (string[]) fallback branch inside the tag chip render (currently dead code once the endpoint always populates `tagsWithColour`) - `frontend/src/Pages/ProgramLibrary/Templates/index.tsx` — same - `frontend/src/Pages/Program/View/index.tsx` — same - `frontend/src/Pages/ProgramLibrary/Create/index.tsx` — remove the JSON-parse hydration fallback (Batch 56 §1a code) **Context.** Batch 14 (2026-08-04) rewired program tags from a JSON string array to the `program_tag_map` pivot + `tags` table. The pivot now owns display, colour, and lifecycle; a rename or colour change in Settings propagates uniformly. **Current state (as of Batch 14 close):** - Pivot rows: 30 (populated by Step 1 backfill migration). - Write path: **dual** — `store()` writes both `program_tag_map` and the legacy JSON column (Step 2). - Read path: pivot everywhere. Four surfaces confirmed on the pivot (Library list card, Templates workspace, Program Detail header, Editor form hydration). Legacy `tags` field is still returned by the endpoints but only consumed as a fallback for rollback-window safety. **When to retire.** After one release cycle with the pivot as authority and zero regression reports. The dual write is cheap insurance during that window; the JSON column is a rollback path if a defect in the pivot code surfaces post-deploy. **Checklist for the retirement PR:** 1. Confirm no consumer still reads `program.tags` for display. Grep `\.tags\b` across `frontend/src/Pages/**` and confirm every hit is either a fallback branch (safe to delete) or a write path (safe to delete once `store` is single-write). 2. Grep `programs\.tags` and `->tags` across `backend/app/**` and confirm no controller/service still reads or writes the column. 3. Migration: `ALTER TABLE programs DROP COLUMN tags`. Down migration re-creates the column as `longtext NULL` (no data restoration). 4. `AbaDemoSeeder` — remove the `tags => json_encode([...])` write from every program insert. Extension programs already write via the pivot (or would, once the seeder is refactored to call `syncProgramTagsToPivot`). **Blocks:** none. **Depends on:** one release cycle of production burn-in with dual write + pivot read. Do not retire before that. --- ## T28 — Add RateRecorder and re-add `rate` to measurement_type enum **Files (to add / amend):** - `frontend/src/Pages/SessionData/shared/Recorders.tsx` — new `RateRecorder` component (events per unit time; entries capture timestamp + event marker; summary shows count / elapsed). - `backend/database/migrations/_add_rate_to_measurement_type_enum.php` — `ALTER TABLE programs MODIFY measurement_type ENUM(..., 'rate')` and same on `program_targets`. - `frontend/src/Pages/ProgramLibrary/shared/measurementTypes.ts` (from Batch 15 Step 3) — add `rate` to the exported enum + its display label. - `frontend/src/Pages/SessionData/Create/index.tsx` — add `case 'rate': return ` to the dispatch switch. **Context.** Batch 15 dropped `Rate` from the measurement_type enum because no Recorder implementation exists — anyone who selected it silently landed on TrialBasedRecorder. Zero rows in the DB chose it (both live and soft-deleted), so removal cost nothing. But `rate` is a legitimate ABA measurement — events per unit time, distinct from frequency-count and from trial-based accuracy — and its absence is a gap, not a design decision. Logging so the gap is visible rather than disappeared. **Blocks:** none. **Depends on:** Batch 15 close, plus a design pass for the RateRecorder UI (what's the input affordance? big button + optional duration override? tap-counter with a start/stop clock?). Consult the client's tool before implementing. --- ## T29 — Retire `programs.measurementType` / `program_targets.measurementType` varchar columns **Files (to remove or amend):** - `backend/database/migrations/_drop_measurement_type_varchar.php` — `ALTER TABLE … DROP COLUMN measurementType` on both tables. - `backend/app/Http/Controllers/ProgramController.php` — remove legacy varchar from validators, store(), cloneProgramRow, index select list. - `backend/app/Http/Controllers/ProgramTargetController.php` — same for the target-level write paths. - `backend/app/Http/Controllers/SessionDataController.php` and `SessionAutosaveService.php` — stop writing the varchar into `session_data_records.measurementType`. - `backend/app/Models/programs.php`, `program_targets.php` — remove from `$fillable`. - `frontend/src/Pages/SessionData/Create/index.tsx`, `Pages/Program/View/index.tsx`, `Pages/ProgramLibrary/Create/index.tsx`, `Pages/ProgramLibrary/shared/TargetsWorkspace.tsx` — stop reading or writing the varchar. **Context.** Batch 15 (2026-08-04) added a `measurement_type` enum column alongside the legacy `measurementType` varchar and moved the dispatch onto the enum. The varchar stays as the rollback path for one release cycle, parallel to how Batch 14 handled `programs.tags`. **Current state (as of Batch 15 close):** - Enum column authoritative for dispatch (Step 2). - Both writers dual-write enum + varchar (Step 3-ish, if the write paths carry both). - Every row's enum value matches the varchar-derived expectation. - Nothing reads the varchar for behaviour — only for rollback safety. **Retirement gate:** one release cycle of enum-authoritative dispatch in production with zero regression reports. **Blocks:** none. **Depends on:** enum burn-in. --- ## T30 — Client questions awaiting Kerthiga's answer Consolidated here so there is one list rather than several. Each is a decision the batch that raised it deferred pending the client, not a technical unknown on our side. - **RED state on OutcomeStateToggle** — clinical meaning of the "unsuccessful" outcome: does it count in the denominator only, or is it excluded from both numerator and denominator? Enum shipped as `unsuccessful/not_counted/successful` on the denominator-only assumption; scoring code has been kept naming-only until the meaning is confirmed. See Batch 13 §1.1. - **"The allows" typo** in the Prompt Level Templates description paragraph — reproduced verbatim from the client's page. Change to "This allows" if intended. One-line change. - **Rating Scale card anatomy** — the third outcome measurement mode. `PromptLevelCard` currently renders NO outcome column for rating-scale mode per user instruction ("no placeholder, no TODO, no dev copy"). Awaiting the client's spec for the card shape. - **Second Advanced settings submenu ending "...ng Hints"** — the spec text was truncated in the source screenshot; the item was not built. - **Number of states the Successful outcome toggle has** in the client's tool — ours has three (unsuccessful/not-counted/ successful); the client's might have a different count. - **"Show target filter"** — Batch 4A specced the item but the filter UI itself was never described. - **The full merge-token shortcut list** — Batch 3A referenced merge tokens like `{Learner}` and `{He/She}` but the full list was not provided. - **Learnings Moderation page contents** — Batch 5C specced it as the third child under Sessions but never described what it does. - **Program Data Fields** (Batch 12, report-only): full description, empty-state copy, table columns, the modal kebab items, the "obscured field" screenshot, the Data Type options. All left open when the batch was frozen at report-only. - **RateRecorder UI** (T28 companion) — what's the input affordance? Tap-counter + start/stop clock? Big button with optional duration override? Consult the client's tool before building. - **SUGGEST button on the empty Targets toolbar** — Batch 4A omitted this from the ship because its behaviour is unspecified and rendering it wired to nothing would apologise to the clinician for a missing feature. Restore when the client's spec defines what "suggest targets" does (heuristic recommendation from a library, AI-driven, both?). - **Program instructions during session recording (surfaced by Batch 3A audit)** — should program instructions be visible to the therapist during session recording? The shortcut feature implies yes — the whole point of substituting the learner's name is reading it in context. Today no session surface renders instruction text at all; it lives only on the two authoring surfaces (program viewer and editor). That's a product question much larger than the token engine's pronoun-mapping detail: building the substitution engine without a session-time render surface would substitute names on screens the clinician reads while planning and never on the screen they use while recording. Frame this for Kerthiga before building anything else on the 3A feature. --- ## T31 — Batches specced but NOT BUILT (Kerthiga-parity items) Client-parity batches that were written up and approved-in-principle but never landed as code. Each is one line. Ordered by demo-visibility. - **Batch 3A** — Instructions merge-token TIP banner ({Learner}, {He/She}). **BUILT** as banner-only (no replacement logic per user turn — full shortcut list still open, replacement engine, gender-to-pronoun mapping, and new render surfaces on the SessionData recorder all remain unbuilt work if the shortcuts feature is later approved to ship). - **Batch 4A** — Targets EMPTY-state chrome. **BUILT.** SUGGEST renders per spec but is wired to a "coming soon" toast (client-side flow unspecified). Kebab's "Show target filter" toggle is wired to state; the filter UI itself is deferred pending T30 client answer. - **Batch 5** — Settings sidebar tree + `SettingsPageLayout` wrapper. NOT BUILT. (Menu.tsx has the Settings sub-group per Batch 61 §1, but the wrapper component doesn't exist.) - **Batch 5B** — `SettingsDataTable` shared component. NOT BUILT. (Comment in `Settings/PromptTemplates/List/index.tsx` L256 says "when SettingsDataTable (Batch 5B) eventually ships, this markup lifts out" — that lift has not happened.) - **Batch 5C** — Learnings Moderation page as third child under Sessions. NOT BUILT (no page file, no menu entry). - **Batch 7C** — Workflow modal: maintenance intervals on the terminal phase, inline ADD NEW PHASE between cards, phase transition rows, Waiting phase card, workflow name at bottom. **BUILT** across Batches 7C Steps 1-6, plus supporting Batches 16 (advancement fix), 17 (auto-open engine), 18 (interval preservation). 7C-Part-2 (real editable transitions replacing the linear transitionsTo string) remains post-demo. - **Batch 10** — Data Recording tab EXISTS as a tab in the Create wizard (`TAB_ORDER` in Create/index.tsx L544-550 includes "data"). "Customize prompt levels" program-scoped override modal: NOT BUILT (no matches on that string anywhere). - **Batch 11** — Settings list pages Tags / Workflows list / Program Templates with Client/Staff split menu. Workflows list BUILT (Batch 40, restyled Batch 13 Part C). Program Templates BUILT as `Pages/ProgramLibrary/Templates` (Batch 61 §5). Tags settings page NOT BUILT. Client/Staff split menu on Templates NOT BUILT. - **Batch 12** — Program Data Fields (custom field engine). NOT BUILT (only a `// Tags and Program Data Fields are deferred` comment in Menu.tsx L210). Report-only per user, so this one is by design. Several of these overlap with post-demo work vs demo-critical. The demo-critical shortlist as of this batch: - ~~Batch 3A merge-token banner~~ — DONE (banner only; engine still unbuilt but that's a T30 client-answer wait, not a build task). - ~~Batch 4A empty state~~ — DONE (Suggest inert, filter UI deferred to T30 client answer, both flagged in the batch commit). - ~~Batch 7C Workflow modal~~ — DONE. - **Batch 10 "Customize prompt levels"** (visible on Data Recording tab, program-scoped override). Still not built. Only remaining demo-critical from this list. Post-demo (deferred by design or by client-question dependency): - Batch 5 (Settings sidebar tree + SettingsPageLayout wrapper) - Batch 5B (SettingsDataTable shared component) - Batch 5C (Learnings Moderation page — T30 open) - Batch 11 remainder (Tags settings page, Client/Staff split menu on Templates) - Batch 12 (Program Data Fields — report-only per user) - 7C-Part-2 (real editable transitions on mastery_workflow_phases) --- ## T32 — Neutral ink/canvas legacy tokens deliberately not swept Part C left **63 references** to `AbaTokens.ink3` / `.ink2` / `.canvas2` unchanged across the ABA module. Both these and their B13 equivalents (`AbaB13Ink.ink3` / `AbaB13Ink.ink2` / `AbaB13.canvas`) are muted greys with no state semantics, so a full sweep would be churn for zero visible benefit. **This was a decision, not an oversight.** If someone later "finishes the sweep", they should know: the state-carrying `brand*` sweep is complete (2 remaining, both intentional palette entries on ColorPicker and StudentAvatar — documented in-file). The ink/canvas neutrals are visually equivalent; touching them changes nothing. If a future release wants a single-token-source-of-truth cleanup, the change is safe but has no user-visible effect. --- ## T36 — Backend tightening on the invoice validator **Filed:** Batch 19 Part B. Approved as a ticket, not built. Three server-side holes surfaced while aligning FE validation to the backend in Part B. All three are validation-only; none has a live symptom in the data today (`0/4599` invoices have any non-zero taxRate or discount). The FE now guards all three, so **the exposure is API-direct callers only** — cURL, a rogue integration, or any future non-Create-page surface that bypasses the FE. | Column / field | Current backend guard | What can be sent | |---|---|---| | `taxRate` | `nullable\|numeric` | Negative percent (nonsense). | | `discountAmount` | `nullable\|numeric` | Negative discount — INFLATES the total (`subtotal + tax − (−discount) = subtotal + tax + discount`). | | `dueDate` vs `issueDate` | both `required\|date`, no cross-check | An invoice with `dueDate` before its own `issueDate`. | | `items.*.description` | `required\|string` (no `max:`) | Unbounded free text in the TEXT column. | **Recommended shape** (single-batch fix, low blast radius): ``` 'taxRate' => 'nullable|numeric|min:0|max:100', 'discountAmount' => 'nullable|numeric|min:0', 'dueDate' => 'required|date|after_or_equal:issueDate', 'items.*.description' => 'required|string|max:500', ``` Applied in [`InvoiceController::store`](../app/Http/Controllers/InvoiceController.php#L301-L317) — one file, four lines. Test cover: reject a negative discount, reject a `dueDate < issueDate`, reject a description over 500 chars. **Blast radius on existing data**: none. Confirmed with a full-table scan — no row currently violates any of these clauses. --- ## T37 — Finance-settings admin surface **Filed:** Batch 19 Part B. Approved as a ticket, not built. Batch 19 added two nullable columns to the existing `settings` table: - `due_days_manual` (falls back to 30 in `PaymentTermsResolver::manual()`) - `due_days_recurring` (falls back to 7 in `PaymentTermsResolver::recurring()`) Both read by the invoice-creation paths already. No admin UI exists to set them yet — only the shipped SQL. An admin who wants to change the values today has to update `settings` via a DB tool. **Recommended shape**: a Settings > Finance page under the existing Settings sub-group. Two small integer inputs, helper text explaining the split ("Monthly recurring invoices use N days; manual and duplicated invoices use M days"). Also the natural home for future finance-scoped settings (tax defaults, invoice-numbering prefix, currency, notice terms). The existing `settings` table's company-info columns (companyName, companyEmail, etc.) also lack an admin UI today, so this is one of several missing settings surfaces. When one lands they can share layout — a shared `SettingsFormLayout` component is Batch 5 territory (also unbuilt, T31). --- ## T38 — Legacy import may have zeroed real discount data **Filed:** Batch 19 Part B. Report-only. [`ImportLegacyFinance.php:213`](../app/Console/Commands/ImportLegacyFinance.php#L213) hardcodes `'discountAmount' => 0` when creating each `L-*` invoice. If the legacy source system had per-invoice discount fields, they were flattened to zero on import — no evidence in the current data of what those values might have been. **Impact assessment**: - 4,599 imported invoices, 4,549 paid, $3.4M invoiced. - If a legacy discount had been recorded per invoice for accounting purposes and this import dropped it, historical totals in the app now show the pre-discount amount rather than the actually-billed amount. - Paid invoices still balance (paid_amount matches expected total from items × price), so the aggregate arithmetic reconciles regardless. But per-invoice historical accuracy (what was actually charged) may be off for any legacy invoice that carried a discount. **Recommended action**: consult the legacy source schema. If a discount column existed and was populated, either: (a) re-run the import with the discount mapped, or (b) accept the loss as historical noise and document it in whatever accounting narrative goes to finance. Not fixable at the app layer without touching the legacy import. **Answerability caveat**: this question can only be answered against the legacy source system. If that system is no longer accessible (retired, decommissioned, backups purged), this ticket should be closed as **unanswerable** rather than left open forever. An unanswered ticket that could never be answered is worse than an acknowledged historical loss. Confirm legacy accessibility before scoping any resolution work. --- ## T46 — Canvas body text (`#7e7e7e`) has never cleared WCAG AA **Filed:** post-Batch-43 companion audit. Report-only; not fixed in this batch. The theme's primary body-text colour is `#7e7e7e` (mid-grey), sourced from `light_theme_colors.textPrimary`. Its contrast against surfaces: | Surface | Contrast | Status | |---|---|---| | White card (`cardBackground = #ffffff`) | **4.51** | Clears WCAG AA (4.5) by 0.01 — marginal | | Page canvas after Batch 43 (`background = #F7F9FB`) | **4.35** | Below AA by 0.15 | | Page canvas before Batch 43 (`background = #e3efeb`) | **4.31** | Below AA by 0.19 | **Batch 43's flip made canvas contrast worse by 0.04, but it has been below AA for the life of the theme.** This is a pre-existing accessibility gap, not something Batch 43 caused. Canvas body text has never passed AA against any canvas this theme has shipped. **Where it shows up**: page titles, section headers rendered directly on the page canvas outside cards, and any list-item text on pages that don't use card wrappers. Anywhere a page renders body text without putting it inside a white-surface card. **Candidate fixes** (all DB-side — see the crucial note below): - `#666666` clears AA on both card (5.74) and canvas (5.51). Safe minimum. - `#5a5a5a` clears with more headroom — card (6.85), canvas (6.58). More conservative choice. - Anything darker (e.g. `#333`) reads as heavy in this palette; not recommended. **Crucial note for the next dev**: fixing this is a **DB row UPDATE, not a code change**. The code fallback at [`appcolors.ts:206`](../../frontend/src/theme/appcolors.ts#L206) is `#7e7e7e` but the live value comes from `light_theme_colors.textPrimary` per T45's precedence chain — the code path never runs in a real session. Someone hunting for the fix in `appcolors.ts` will find the same value and conclude "it's already correct." ```sql UPDATE light_theme_colors SET textPrimary = '#666666' WHERE id = 1; ``` Users must clear localStorage or re-log to pick up the change. **Why deferred**: a text-colour change touches every page in the app. Batch 43 has already moved the canvas underneath them; stacking a text-colour change on top would compound the visual delta the user has to spot-check. Land in a separate turn after the canvas has been observed clean. **Related**: T47 (primary/secondary inversion). --- ## T47 — `textPrimary`/`textSecondary` semantics inverted in code, collapsed in DB **Filed:** post-Batch-43 companion audit. Report-only. Two problems stacked, and the second hides the first: **Problem 1 — code fallbacks are inverted vs MUI convention.** MUI convention: `text.primary` is the DARKER value (main body copy), `text.secondary` is the LIGHTER value (helper/hint copy). This theme's code fallbacks at [`appcolors.ts:206-207`](../../frontend/src/theme/appcolors.ts#L206-L207): ``` "--bs-text-color": lightThemeColor?.textPrimary || "#7e7e7e", ← mid grey "--bs-secondary-text": lightThemeColor?.textSecondary || "#252d36", ← near-black ``` Fallback `textSecondary` (near-black) is DARKER than fallback `textPrimary` (mid-grey). Inverted from convention. Any developer using `theme.palette.text.primary` expecting the darker value would get the lighter one. **Problem 2 — DB row collapses both to `#7e7e7e`.** `light_theme_colors.textPrimary` and `.textSecondary` are both currently `#7e7e7e` — the mid-grey code fallback for `textPrimary`. In the running app there is no visible distinction between `text.primary` and `text.secondary`. The DB collapse **hides** the code inversion — running the app, nobody notices Problem 1 because the two values are identical. Only surfaces when someone reads the fallbacks and expects them to encode a semantic distinction. **Attributing intent — a guess, recorded as one**: someone MAY have flattened the DB values as a workaround for the code inversion (avoiding "the wrong value in the wrong slot"). No evidence either way — could equally be a copy-paste in an admin form that touched one field and inadvertently duplicated it. **Do not assert intent.** Fix requires deciding what the correct semantics ARE (align code to MUI convention, or align convention to what the code already says) and then setting DB values to match. **Recommended fix shape** (post-demo, after T46 is decided): 1. Pick canonical values, MUI-standard: `text.primary` = the darker text (main body), `text.secondary` = the lighter text (helper/hint). 2. Fix code fallbacks first: swap the fallback values so `textPrimary → #252d36`, `textSecondary → #7e7e7e` (or whatever T46 lands on). 3. Then DB UPDATE to set both fields distinctly per the new convention. 4. Grep every `theme.palette.text.secondary` consumer to check whether it currently RELIES on the values being identical (unlikely but worth checking — if any UI reads secondary and expects mid-grey, it'll suddenly get the light-grey after the fix). **Do not fix independently of T46.** The two touch the same DB row and the same code chain; splitting them means two sessions of "clear localStorage, spot-check text everywhere." **Related**: T46 (canvas body-text WCAG gap). --- ## T45 — Two silent sources of truth for the app palette (code fallback vs `light_theme_colors` DB row) **Filed:** Batch 43 post-flip diagnostic. Cost a full audit turn to discover. The app's canvas colour has TWO sources: 1. **Code fallback** at [`appcolors.ts:198`](../../frontend/src/theme/appcolors.ts#L198): ``` "--bs-body-bg": lightThemeColor?.background || "#F7F9FB", ``` 2. **DB row** at `light_theme_colors.background`, served by [`UserController::getThemeColor`](../app/Http/Controllers/UserController.php#L212) into the login response, cached in `localStorage.lightThemeColor` at [`Login.tsx:122`](../../frontend/src/Pages/Login/Login.tsx#L122), and consumed by the code fallback via `lightThemeColor?.background`. **Precedence**: DB wins silently. The code fallback only fires when localStorage is empty AND the DB row has no `background`. In any deployed instance with users who have logged in, the code path never runs. **Consequence**: Batch 43 changed `appcolors.ts:198` to `#F7F9FB` and this had ZERO visible effect on the app, because the DB row (`#e3efeb`) took precedence. Discovered only when the user spot-checked and found the mint canvas still rendering. Seven layers of indirection between the change and the effective source. **Related drift**: the code fallback was `#eaf3f2` (pale mint) but the DB row is `#e3efeb` (slightly greener mint) — two "mints" that disagree. Nobody noticed because the DB value wins and nobody reads the fallback. **Recommended fixes** (all deferred): - **Documentation**: add a comment block at the top of `appcolors.ts` explaining that its fallbacks are inert in a session with a populated `lightThemeColor` — and that live theme values come from the DB via login. One paragraph in the file the next dev will read. - **Consolidate precedence**: strip theme-source keys from `lightThemeColor` at the palette layer so the code fallback becomes the SINGLE source of truth for shipping defaults. Customer-specific customisation moves to a smaller opt-in set of keys (brand colours, logo, etc.) rather than "every palette token." - **Or the opposite**: delete the code fallbacks entirely and treat `light_theme_colors` as the sole source of truth, plus a seed migration so a fresh install has the same shipping default as a deployed one. Requires a migration + seeder pass. **Why record this**: any future theme work will hit the same wall unless the precedence is documented somewhere findable. Batch 43 spent a full turn discovering it; the next batch should not. --- ## T43 — Delete the `Pages/ProgramLibrary/shared/abaTokens.ts` shim **Filed:** Batch 43 (T41 landing). Delete-when-safe ticket, paired with the shim it enables. Batch 43 hoisted the token file to [`frontend/src/theme/b13Tokens.ts`](../../frontend/src/theme/b13Tokens.ts) and renamed every export (`AbaTokens` → `LegacyTokens`, `AbaB13` → `B13`, etc.). All known consumers were codemod-updated in the same batch to import from the new location under the new names. The old path [`frontend/src/Pages/ProgramLibrary/shared/abaTokens.ts`](../../frontend/src/Pages/ProgramLibrary/shared/abaTokens.ts) is now a backward-compat re-export shim that re-exports the new tokens under their OLD `Aba*`-prefixed names. It exists as a safety net for anything the codemod might have missed OR for consumers added on stale branches merged after the codemod. **Deletion procedure**: 1. Verify zero live consumers of the shim: ``` grep -RnE "from ['\"][^'\"]*abaTokens['\"]" frontend/src ``` Expected: no matches. 2. If the grep returns anything, codemod those files first (rename identifiers + update import path to `frontend/src/theme/b13Tokens`). 3. `git rm frontend/src/Pages/ProgramLibrary/shared/abaTokens.ts` and its enclosing folder if empty. 4. Typecheck. If clean, commit. **When to do this**: after one release cycle of the shim being in place, or immediately if a branch-merge conflict makes the shim actively harmful. Not urgent — the shim is inert as of Batch 43. --- ## T44 — ProgramLibrary/List page-level canvas overrides its own table stripes **Filed:** Batch 43 (T42 canvas-flip audit, incidental finding). Not caused by this batch — pre-existing cosmetic defect surfaced by the audit. [`Pages/ProgramLibrary/List/index.tsx:1354`](../../frontend/src/Pages/ProgramLibrary/List/index.tsx#L1354) sets the outermost `` background to `AbaB13.canvas` (`#F7F9FB`, now `B13.canvas`). Multiple tables on that page use the same colour for their head-row stripes ([lines 1520, 1674](../../frontend/src/Pages/ProgramLibrary/List/index.tsx#L1520) and others). Stripe colour and page-bg colour are identical → **stripes are invisible today.** Not caused by the T42 canvas flip. The page has been carrying this cosmetic defect since the wrapper was added; the audit surfaced it because we were looking at every canvas-setter and noticed the collision. **Options**: - Remove the page-wrapper canvas override — after Batch 43's theme flip, the page inherits `#F7F9FB` from `theme.background.default` and the wrapper is redundant. Removing it makes the page identical AND lets the stripe read against the surrounding light card surfaces where present. - Change the stripe colour to `AbaTokens.canvas2` (`#EDF2EF`) — a slightly darker inset surface that reads against `B13.canvas`. **Do first**: the visual spot-check the user is running for T42. If stripes vanish anywhere else after the theme flip, batch those fixes together. --- ## T41 — Hoist B13 tokens out of `Pages/ProgramLibrary/shared/abaTokens.ts` **Filed:** post-Batch-19 §3 canvas audit. The B13 palette (colours, ink levels, radii, `tint12()` helper) lives in `frontend/src/Pages/ProgramLibrary/shared/abaTokens.ts` because ABA led the migration. The palette itself is not ABA-specific — `AbaB13.primary`, `AbaTokens.line`, `AbaB13Ink.ink3` are the app's forward-direction visual language, not module-scoped decisions. Batch 19 §3 imports them into Finance to apply hairlines / neutral surfaces / tokenised radii on the Create page. **Semantic problem**: Finance now depends on ABA's token file. Any Finance dev reading `import { AbaTokens, AbaB13 } from "../../ProgramLibrary/shared/abaTokens"` at the top of a finance page is going to assume ABA is authoritative and think twice about editing tokens. That's a false constraint. **Recommended**: move `abaTokens.ts` to `frontend/src/theme/b13Tokens.ts` (or similar shared location), re-export from the current path for backward compatibility, update new consumers to import from the shared location. Rename `AbaB13*` → `B13*` in the same pass; the `Aba` prefix is a history artefact of the migration and not a semantic marker. **Blast radius**: ~30 files consume the tokens today (all ABA). Renaming is codemod-able; the file-move is a single git-mv plus updated imports. Non-behavioural. **Do**: when the app-wide canvas decision (T42 candidate — see below) is made, do this at the same time. Consolidates the palette question into one change. --- ## T42 — App-wide canvas: pale-green `#eaf3f2` default vs cool `#F7F9FB` B13 **Filed:** post-Batch-19 §3 canvas audit. The MUI theme's `background.default` is `#eaf3f2` (a pale mint green), set at [theme/appcolors.ts:198](../../frontend/src/theme/appcolors.ts#L198). Every non-ABA page inherits it — Finance, Parents, Classes, Students, Reports, Payment, ParentPortal, etc. The ABA module opted out during Batch 13 (`AbaB13.canvas = #F7F9FB`, cooler) via per-page wrapper components (`AbaPageSurface` and inline `backgroundColor: AbaB13.canvas` overrides). **Not a Finance-specific decision.** The Finance canvas is green because the app-wide default is green. Batch 19 §3 kept it — a Finance-only canvas change would mean Finance uses a different canvas from every other non-ABA module, which fragments the visual language further. **Two paths for the global decision**: - **(a)** Change `theme.palette.background.default` to `#F7F9FB` at the app level (one line in `appcolors.ts`). Every non-ABA page automatically follows; ABA's per-page wrappers become redundant no-ops (safe to delete over time). - **(b)** Keep the per-module wrapper pattern ABA established. Finance opts in via a `FinancePageSurface` component. Every module ends up with its own canvas wrapper, which is more code but preserves the current shape. **Recommend (a).** The wrapper pattern is a Batch-13 migration artefact — it was per-module because the migration went module-by-module. Once every module wants B13, the wrapper is a workaround for a decision that could be made at the theme layer once. **Not a demo blocker.** Both canvases look fine; the green isn't broken, just older. Decide alongside T41 (tokens hoist) — they naturally land together. --- ## T40 — Two parallel invoice UIs; retire the legacy `/invoice/*` tree **Filed:** post-Batch-19 routing audit. Approved as a ticket. The frontend has two coexisting invoice UIs, and the routing is inconsistent between them: | Legacy `/invoice/*` | New `/finance/*` | |---|---| | `/invoice/list` (Invoice/List) | `/finance/invoices` (Finance/Invoices) | | `/invoice/create` (Invoice/Create — the one Batch 19 Part B rewrote) | (no create page under /finance) | | `/invoice/view/:id` — **route never registered** | `/finance/invoices/:id` — registered | **Sidebar** points at `/finance/*`. **Create page** used to navigate to bare `/invoice` on save/cancel (404 — fixed in this turn by pointing at `/finance/invoices`). **Legacy List page** row-click navigated to unregistered `/invoice/view/:id` (also 404 — fixed by pointing at `/finance/invoices/:id`). **Dashboard** has three navigations to `/invoice/list` ([Dashboard.tsx:504, 637, 767](../../frontend/src/Pages/Dashboard/index.tsx#L504)) — those resolve but land the user in the legacy list rather than the new one the sidebar points at. **Recommended shape**: 1. Redirect `/invoice/list` → `/finance/invoices` (one line in Routes.tsx via ``). 2. Redirect `/invoice/create` → same, OR move the Batch 19 Part B Create page under `/finance/invoices/new` and redirect the legacy path there. 3. Update Dashboard's three navigations to point at `/finance/invoices`. 4. Delete `Pages/Invoice/List` once no consumer remains. **Blast radius**: bounded — every reference to `/invoice/*` in the codebase is enumerated in the routing audit above. Not a demo blocker (Batch 19's spot fix at both broken navigations closed the visible 404). Worth doing when the Finance module gets a coherent post-observation pass (aligns naturally with T39's operational shakedown). **Do first**: T39's shakedown. That reveals which of the two UIs is actually being used and by whom. Retiring the wrong one is worse than delaying the cleanup. **Do the two UIs write through the same controller?** Yes. Verified: `useCreateInvoiceMutation` is the ONLY create hook exported by `attoDeskApi.ts`, it POSTs `/invoice`, and it's consumed by exactly one component — `Pages/Invoice/Create/index.tsx` (the page Batch 19 Part B rewrote). There is no separate `/finance/*/create` FE component; the "new" UI has no create surface at all. Every write goes through the ONE Create page and hits the ONE endpoint (`InvoiceController::store` → `StudentMonthlyInvoiceService::getOrCreateMonthlyInvoice`). Consequence: the Part B fixes (discount key rename, validation alignment, single money site, race-safe number generator) apply universally. Nobody hits an unpatched Create flow regardless of which sidebar route they came in through — the split is List/View only. This tightens the ticket scope: legacy retirement can drop `Pages/Invoice/Create` too, since Finance provides no replacement yet. --- ## T39 — Finance write paths have never executed against live data **Filed:** Batch 19 Part B closeout. Recorded per user turn: > "Finance write paths have never executed in production. All 4,599 invoices are legacy imports. The monthly recurring job, class-assignment invoicing, and manual creation are all unexercised against live data. First real use should be observed, not assumed." **What the diagnostic showed**: every row in the `invoices` table is `L-*` prefixed. Zero `INV-*`. Zero rows produced by `StudentMonthlyInvoiceService::create`, `InvoiceController::store`, `InvoiceController::duplicate`, `GenerateMonthlyInvoices`, `GeneratePerClassInvoices`, or `ConsolidateDuplicateInvoices`. **What this means for the Finance module**: - The code is **tested** (52 tests pass across the invoice suites). It is **untried** — no live user has run it end-to-end. - Three bugs surfaced in Batch 19 by *reading* the code, not by running it: - The number generator had a race condition (Batch 19 Part A fix). - The FE Create form dropped user-entered discounts silently (Batch 19 Part B fix — `discount` vs `discountAmount` key mismatch). - Two default payment terms disagreed with no documented reason (Batch 19 Part B fix — now settings-driven). - First real run should be expected to surface more. Reading catches obvious defects; running catches integration defects (URL prefixes, permission binding, real student/parent data shapes, invoice-item join edge cases, PDF template rendering with real customer names). **Recommended pre-production shakedown** (deferred per user — not for now): 1. In a non-production database seeded with a handful of real students, create one invoice through each of the three write paths: - Manual create via the Create page. - Monthly recurring job via `php artisan invoices:generate-monthly`. - Class-assignment auto-invoice via the classes-assign flow. 2. For each: verify persistence, verify the invoice PDF renders with the real data, verify the email template resolves the invoice number and student name, verify the payment-matching flow finds the invoice. 3. Report each path's actual observed behaviour, including anything the tests don't cover (permission gates, cross-controller integration, real data shape edge cases). Not a code batch. An operational activity — the shape a QA pass takes when the module has been built but never observed. --- ## T35 — FE test harness for the four phase-array pointer-remap helpers **Filed:** Batch 7C Step 4. Scope: **the four helpers only**, not a general FE test strategy. The `move`, `duplicate`, `remove`, and `insertAt` helpers in [`WorkflowEditorBody.tsx`](../frontend/src/Pages/Settings/Workflows/shared/WorkflowEditorBody.tsx) each rewrite the `phases[]` array AND remap `autoOpenInitialPhaseIndex`. All four use pointer arithmetic: - `move(from, to)` — pointer at `from` moves to `to`; pointer in the swept range shifts by 1 in the opposite direction. - `duplicate(i)` — pointer strictly above `i` bumps by 1 (insertion at `i+1`). - `remove(i)` — pointer at `i` becomes null; pointer above `i` decrements. - `insertAt(at, ...)` — pointer at or above `at` bumps by 1. Currently verified only by code review — the remap shape mirrors between helpers, and the codebase has no FE test harness (no Jest, Vitest, or RTL configured). Pointer arithmetic is where off-by-ones live; three of these four helpers were shipped in Batch 18 (with the wrong-phase silent-bind fix), and the fourth in Step 4. A test could have caught any drift between the four styles or an incorrect boundary condition. **Recommended shape**: Vitest + React Testing Library, ONE test file — `WorkflowEditorBody.remap.test.tsx` — with parameterised cases: ``` For each helper × each pointer-position class (below, at, above, null): render body with pointer set → invoke helper → assert new pointer value ``` Roughly 16 cases, ~200 lines including harness setup. Tractable first target; a good landing pad for a wider FE test suite if the team later commits to one. **Not**: a general FE testing strategy, story-testing all editor states, snapshot testing. Those are open-ended. Start with the four helpers where a real defect class (silent wrong-phase rebind) has already been caught by user question, not by test — because there was no test. **Estimate**: half a day for setup + tests. Not appropriate pre-demo; a week-1 post-demo target. --- ## T34 — Standing rule: audit FK stability before adding a new child **Filed:** Batch 18. Standing rule, applies to every subsequent batch and to existing FKs retroactively. **Rule:** > Before adding an FK to any table, establish whether the parent's rows are stable across a save. If the parent is soft-deleted and re-inserted, the child needs an explicit preservation strategy, stated in the migration approval. **Origin.** Batch 7C Step 2 added `mastery_workflow_phase_intervals.phaseId` FK (mine, `ON DELETE CASCADE`), and Batch 17 added `mastery_workflows.autoOpenInitialPhaseId` FK (mine, `ON DELETE SET NULL`). `MasteryWorkflowController::save` soft-deletes every phase row on every save and re-inserts with new PKs — a shape that pre-dates both batches. Neither FK addition triggered a check on parent-row stability. Intervals were silently orphaned to tombstoned phase rows on every workflow save until Batch 18 caught it. Batch 17 got away with it because the index-remapping path knew IDs weren't stable — that knowledge existed one file away and was not carried across to Step 1's intervals design. **Retroactive audit** (done in Batch 18): | Child FK | Parent | Preservation strategy | |---|---|---| | `mastery_workflow_phase_intervals.phaseId` | `mastery_workflow_phases` (re-insert on save) | **fixed in Batch 18** — snapshot before soft-delete, re-attach after re-insert using payload's `phase.id` as the correlation key. | | `mastery_maintenance_schedules.workflowPhaseId` | `mastery_workflow_phases` (re-insert on save) | **already handled** — save method soft-deletes and re-inserts the schedules from the payload's `phase.maintenanceSchedules[]` on each save. Manual, not a diff, but functional. | | `mastery_workflows.autoOpenInitialPhaseId` | `mastery_workflow_phases` (re-insert on save) | **fixed in Batch 17 + Batch 18** — save re-resolves via `autoOpenInitialPhaseIndex` (payload index); duplicate() remaps via old→new phase id map. | No other child tables reference `mastery_workflow_phases`. Grep confirmed: ``` mastery_maintenance_schedules_workflowphaseid_foreign mastery_workflow_phase_intervals_phaseid_foreign fk_mw_autoopen_phase ``` All three now have an explicit preservation strategy. **Also implicated: any parent table with the "soft-delete + re-insert" save shape.** Programs / program_targets do NOT use this shape (they update in place via `PATCH` on the target row). Only the mastery-workflow tree currently has this pattern, so the retroactive scope is bounded — but the standing rule applies to every future migration that adds an FK, since the child migration is the last chance to catch the mismatch before it lands. **Follow-up (post-demo, separate ticket):** rewrite `MasteryWorkflowController::save` as a diff-update rather than soft-delete-and-reinsert. Preferred end state. Not done here because rewriting the save method days before a demo — when it's the code path every workflow edit goes through — is not a trade to take. Option (b) (mirror the maintenance-schedule handling) is the demo-safe shape; option (a) (diff-update) is the correct one. --- ## T33 — Not-found convention on `whereNumber` routes is inconsistent (200-with-status:false vs 404) **Origin:** Batch 16 audit turn. Surfaced by the `invalid_status_transition_is_rejected` test that "passed" pre-APP_URL-fix because `id=0` matched `whereNumber('id')` and the controller returned HTTP 200 with `{status: false, message: 'Session not found'}`. That behaviour is not itself wrong — but the codebase does not do it consistently, and inconsistency is what makes it a footgun. **Scope:** 58 routes in `routes/api.php` use `whereNumber(...)`. `0` passes the constraint (it is numeric). Controllers behind those routes then either: - return **HTTP 404** with `{code: 404, status: false, message: '... not found.'}` — the newer convention (Assessment, AttendanceRecord, ClassDetail, Batch 15+ controllers including my own `MasteryWorkflowPhaseIntervalController`), or - return **HTTP 200** with `{status: false, message: '... not found'}` — the older convention (SessionDataController's transition endpoints, `classesAssignController`, `classesAssignStudentController`). **Why it matters:** - Frontends that treat any 2xx as "the call worked" will happily accept a not-found as a soft failure. Any UI that keys off `!response.ok` (fetch's default) misses the entire class of misses on the older-convention endpoints. - Any caller passing a defaulted/zero id (`(int) $maybeMissing` collapses to 0) gets a 200 and a falsy body — no throw, no error banner, no logging. Silent soft-fails. - Tests asserting only on the falsy body pass without reaching their subject. That is what the Batch 16 audit caught in one case; there are probably more. **Not blocking, not urgent.** No user-visible defect has been traced to it. Filing so a future decision can either standardise on 404 (my recommendation — matches HTTP semantics, matches what the newer controllers already do, matches what `ModelNotFoundException` would produce by default) or intentionally endorse the 200-with-status:false shape and document it. **Estimate:** the newer convention is already established; the older sites are a bounded list (approximately 5-8 controllers, majority of hits in the two `classesAssign*` controllers). A single sweep-and-standardise turn is a couple of hours at most, but front-end consumers need to be checked for any that depend on the current 200-body-check shape. --- ## T48 — Audit the 18 `onInputChange`-driven Autocompletes for open-time preload **Filed:** ABA shakedown, Assign ABA Program dialog. One instance found, seventeen candidates unexamined. **Scope:** every Autocomplete in `frontend/src` whose search-fetch is wired through `onInputChange` (the fetch fires only when the user types). Grep returns **18 files** ([full list in-file comment on the audit ticket branch]). For each one, determine whether it preloads its options on `open` / mount, and — where it does not — decide per-file whether preload is warranted. **Judgement call, not a sweep.** A picker over thousands of rows SHOULD require typing (the initial fetch would either time out or paginate awkwardly). A picker over three rows should not — clicking the arrow and seeing "No options" on a control with data behind it reads as broken. The audit is deciding which each of the 18 is. **Worked example (the one this ticket was born from):** - **File:** `Pages/SessionData/shared/AssignAbaProgramDrawer.tsx` — the "Assigned ABA Staff" picker. - **Endpoint under the picker:** `GET /aba/staff/search?student_id=X&q=…` — returned **3 rows** for the tested student (id=12635, location=4). The endpoint had data. - **FE behaviour before the fix:** the picker's fetch fired only on `onInputChange`. Opening the dropdown by clicking the arrow → the fetch never ran → the user saw an empty picker on a control with three usable options behind it. Same defect shape as the pre-Batch-114 lock indicator: backend correct, FE never asks. - **Fix:** one `useEffect` on `[open, student?.id]` that calls `searchAbaStaff({ q: "", student_id })` when the dialog opens. Mirrors the existing programs-load pattern in the same file. Landed in the shakedown turn. **The audit steps for each of the other 17:** 1. Read the picker's `onInputChange` handler and the state that feeds `options`. 2. Determine how many rows the endpoint returns in the typical case (10s, 100s, 1000s+?). 3. If small: add the same open-time useEffect pattern. 4. If large: leave as-is, but confirm `noOptionsText` explains "Type to search" — an empty picker over 10k rows is fine IF the empty state says so. 5. Report the count per bucket (preloaded / gated-by-typing / needs-adding) at the end. **Files to audit** (from grep of `onInputChange={`): - `Pages/SessionData/shared/AsyncPicker.tsx` — shared component; if this preloads correctly, some consumers get it for free - `Pages/ProgramLibrary/shared/StudentsStaffWorkspace.tsx` — the workspace that this dialog embeds; likely same bug shape - `Pages/Classes/ClassForm/index.tsx` - `Pages/Classes/Create/studentcreate.tsx`, `empCreate.tsx`, `classStudentPopUp.tsx` - `Pages/Invoice/Create/index.tsx` - `Pages/Student/Create/index.tsx`, `StudentPerent/assignPerents.tsx` - `Pages/ProgramLibrary/shared/ImportTargetsWizard.tsx` - `Pages/ClassSchedule/Create/index.tsx` - `Pages/Tasks/MyTasks.tsx` - `Pages/LeadSystem/MergeLeadDialog.tsx`, `FlagSameEventDialog.tsx`, `CreateLeadDialog.tsx` - `Components/AddressAutocomplete/index.tsx` — likely legitimate (address search over huge pool) - `Pages/Message/Create/index.tsx` **Estimate:** half a day. Most of the time is deciding-per-file; the actual fix per file is one useEffect if warranted. **The pattern this ticket belongs to.** This is the fifth "one instance found, N candidates unexamined" ticket filed in the same session: - **T33** — one 200-vs-404 convention drift found, `~58` whereNumber routes unexamined. - The Program-library `libraryMetrics` projection drift — one column found (`isLocked`), full projection list unaudited until a schema-drift test was written to force the classification. - The `TARGET_STATUS_COLORS` per-page palette — one duplicate found, `resolveStatusTriad` audit deferred. - The `window.confirm` sweep spun off from ONE named case (PromptLevelModal) that turned into 19 sites plus a lint rule. - **T48 (this one)** — one Autocomplete found, 17 candidates unexamined. **Filing note.** The pattern of finding one and shelving the rest is **deliberate scoping**, not backlog drift. Each was scoped that way to avoid sweeping a codebase on the strength of a single symptom — the sweep either finds nothing (waste), finds too much for one turn (unbounded), or lands untested changes across surfaces nobody asked us to touch (blast radius). Each ticket carries the worked example so the audit has an anchor to test its fix against. **Not blocking, not urgent.** The shipped fix covers the one user-visible instance. The other 17 may all be legitimate (large-result pickers where typing-first is correct); may all be defective; more likely a mix. The audit finds out. --- ## T49 — Extract `ProgramAssignmentController::searchStaff` enrichment (~150 lines) when a THIRD caller arrives **Filed:** ABA shakedown, staff-picker hardening. The recorder's autosave guard, the recorder's picker, and the assign dialog's picker now all read the same four primitives (`parseLocationIds`, `locationMatch`, `programAssignmentMatch`, `validateChain`) via `App\Support\StaffEligibility`. The extracted primitives are the shared load-bearing predicates. The ~150-line **enrichment** — ABA-eligibility OR-scoring across role keywords + PSA + Special-Program class + permissions, caseload/capacity, per-row priority ranking, availability tags, `assignment_relationship` derivation — stays inline in `ProgramAssignmentController::searchStaff`. **Trigger for revisiting this:** a THIRD caller that wants the same enriched row shape (not just a subset of the primitives). Not general tidiness. Two callers with policy-driven differences (assign = soft-annotate, recorder = hard-filter) is exactly the case for keeping the enrichment inline — the recorder's picker consumes the enriched rows from the assign endpoint and post-filters on flags rather than needing its own enrichment. If a third caller shows up (e.g. a supervisor caseload dashboard that wants "which of these staff match X student's location and Y program's PSAs, with caseload"), that's the point at which the enrichment migrates to a helper (`StaffEligibility::enrichedPool()` or similar). Two callers is duplication risk; three is duplication proven. **Explicitly NOT the trigger:** - Refactor cleanliness impulse (churn without a beneficiary). - Wanting to test the enrichment logic in isolation (a unit test on the current method works fine). - One field added to the row (this batch added `psa_match`; it landed as an inline row-map addition without needing extraction). **Estimate when it fires:** half a day — the enrichment is stateful (uses several lookup maps computed once per request) but self-contained. Extract to `StaffEligibility::enrichedPool($q, $studentId, $programId, $options)` returning a Collection; callers apply their own filter/annotate policy. **Not blocking, not urgent.** Nothing user-facing depends on this. The primitives are shared today; the enrichment stays with its one caller. --- ## T50 — Recorder prompt-level arming policy **Filed:** ABA shakedown, recorder audit. Discovered while auditing item 3 of the walkthrough (the "blank PROMPT column" observation). **The gap:** A therapist can tap "Correct" / "Incorrect" / "Correct with Prompt" / etc. in the recorder without first arming (selecting) a prompt level, even on a target whose program has a prompt-level template assigned. The trial persists with `promptLevelId: null` and `promptLevelName: null`. Clinical record says the trial happened; says nothing about what level of support was provided. That is silent data loss. The trial happened at some level of prompting (or independently); the record commits to nothing, and no error surfaces. On a program that DOES have a template, this reads as "the therapist forgot to record support level" — but the UI didn't require them to. **Options** (product / clinical decision, not engineering): 1. **Require arm before results are live.** Trial-result buttons disabled until a prompt-level chip is selected. Zero silent nulls. Cost: two taps per trial, therapist workflow slows. Also means the "Correct with Prompt" button becomes semantically odd — the level chip PLUS "Correct with Prompt" is redundant expression. 2. **Auto-arm a default level.** Program's default prompt level (currently `default_prompt_level_id` column, unused as of this audit) pre-selects on target open. Therapist can override per trial. Zero silent nulls once the default is set; falls back to (1) or (3) if the program has no default. 3. **Accept nulls explicitly.** Add an "Unspecified" chip that a therapist can explicitly tap to record "no prompt level was chosen for this trial." Records `promptLevelId: null` with intent, not by omission. Requires the entry log's PROMPT column to distinguish "—" (accidental null) from "Unspecified" (chosen null). 4. **Status quo.** Nulls remain implicit. Add a subtle indicator (amber counter or banner) when trials-with-null > 0 on a program that has a template. Nudge without blocking. **Interaction with the outcomeState reader audit** (Follow-up 1 above): if `outcomeState` gains a consumer — e.g. phase advancement starts honouring "only trials at levels marked 'successful' count as correct" — a null prompt level becomes an ARITHMETIC HOLE, not just a blank column. A trial with `promptLevelId: null` cannot be classified as successful/unsuccessful because there's no level to check the flag on. Current accuracy calc doesn't care (counts `result === 'correct'` verbatim); a future outcomeState-aware calc would need a decision for null-level trials. **So this ticket must NOT be scoped independently of Follow-up 1.** If Kerthiga's tri-state toggle is going to grow a real consumer (option A on that decision), the arming policy has to accept nulls in a way the new consumer can handle. If the tri-state is being reduced to a binary (or removed), then option 3 or 4 here is cheaper. **Estimate:** engineering is small — UI change on the recorder, one write-time validator adjustment if arming is required. The decision is what takes time; the code is a couple of hours per option. **Not blocking, not urgent.** Not blocking today because the accuracy calc doesn't consume prompt-level state. Would become blocking the day the calc does. **Related:** T51 (outcomeState reader gap) — do not scope this ticket independently of T51's answers. The arming policy only matters if the picked level ever affects a calculation. --- ## T51 — Prompt-level `outcomeState` is a tri-state with one partial consumer **Filed:** ABA shakedown reader audit. Do NOT act on this ticket until the clinical owner (Kerthiga) answers the two questions at the bottom; each option has different downstream work. ### The audit `prompt_levels.outcomeState` (`unsuccessful` / `not_counted` / `successful`) and its legacy boolean companion `countsAsIndependent` (`true` iff `outcomeState === 'successful'`). **Writer paths** — where these fields are set: - [`PromptLevelTemplateController::store`](../app/Http/Controllers/PromptLevelTemplateController.php#L279-L366) — editor endpoint; validates, persists, keeps the boolean in sync with the tri-state. - [`PromptLevelTemplateController::index`](../app/Http/Controllers/PromptLevelTemplateController.php#L181-L182), `::duplicate` — passthroughs (emit / copy). - `AbaDemoSeeder` — seed data. - Frontend editor surfaces: `PromptLevelEditorBody.tsx`, `PromptLevelCard.tsx`, `OutcomeStateToggle.tsx`, `Editor/index.tsx`, `HierarchyQuickModal.tsx`, `QuickHierarchyDrawer.tsx`. **Explicit non-readers** (confirmed via grep): - `SessionDataController::checkMasteryCriteria` reads `session_data_records.percentCorrect` only. No prompt-level join. - `SessionDataAggregationService` — no reference to prompt-level state. - `MasteryAutoOpenService`, `SessionAutosaveService`, `SessionReviewService`, `SessionStatusTransitionService` — none join `prompt_levels`. - Adjacent field `weightedScore` — writer paths only, zero readers. **The one reader** — [`SessionData/View/index.tsx:94-101`](../../frontend/src/Pages/SessionData/View/index.tsx#L94-L101): ``` const independentPct = useMemo(() => { if (!trials.length) return null; const indep = trials.filter((e) => { const lvl = promptLevels.find((p) => String(p.id) === String(e.promptLevelId)); return lvl?.countsAsIndependent; }).length; return Math.round((indep / trials.length) * 100); }, [trials, promptLevels]); ``` Displays as the "% Independent" figure card on the session view. Consumes **only `countsAsIndependent`** (the boolean). Does not distinguish `unsuccessful` from `not_counted` — both map to `countsAsIndependent === false` and don't count toward the numerator. ### What the audit means, concretely - The **"successful vs everything else"** distinction has ONE downstream reader: the session-view "% Independent" figure. - The **"unsuccessful vs not_counted"** distinction has **zero** downstream readers. The tri-state is a duo-state as far as the app is concerned. Whichever Kerthiga picks between those two produces identical behaviour app-wide (except in the editor's own display). - Mastery/phase advancement reads `percentCorrect`, which is derived from `session_data_entries.result === 'correct'` alone. Prompt level is captured on every entry but ignored in the calc. ### Questions for the clinical owner Both phrased so a session-runner can answer them without needing to read code. Do NOT ask "should we change the calc" — ask what the intended behaviour was, and let the answer decide whether the code changes. **Q1 — Does prompt-level marking affect mastery?** > "A therapist records a trial as Correct with Prompt at the Gestural level. Gestural is marked as a Successful outcome in the prompt level template. > > Does that trial count toward the percentage that decides whether the target has been mastered? > > Today it does not — only unprompted Correct counts. The Successful marking affects a separate '% Independent' figure on the session view, and nothing else." The current behaviour is defensible (mastery == independence is a real clinical position) but it should be an explicit decision rather than an accident. **Q2 — What is the difference between Unsuccessful and Not counted?** > "You can mark a prompt level as Unsuccessful, Not counted, or Successful. What did you intend the difference between Unsuccessful and Not counted to mean? Today they behave identically." If the answer is a real distinction (e.g. Not counted for levels used during baseline probes that shouldn't affect any figure), then it needs a consumer built. If the answer is "I thought one of them did something," the toggle reduces to binary. ### Do not act until answered - If Q1 answer is "yes, prompted trials at successful levels should count as correct for mastery" → real work: `SessionDataAggregationService` or `checkMasteryCriteria` needs to consult `prompt_levels.countsAsIndependent` when computing `correctTrials`. Interacts with **T50 (arming policy)** — a null prompt level becomes an arithmetic hole in that new calc. - If Q2 answer is "meaningful distinction" → real work: build the consumer that reads `outcomeState === 'unsuccessful'` (probably a "% failed" or "% needed prompt" figure) OR distinguish per-configuration behaviour. Do not touch the field until the reader is spec'd. - If Q1 is "no, current behaviour is right" AND Q2 is "no real distinction" → deferred code hygiene: reduce the tri-state UI to a binary toggle, keep the DB column as tri-state for schema-safety but drop the "Unsuccessful" option in the editor. **Do NOT drop the DB column** — Kerthiga has already entered values against it. ### Related - **T50 (recorder prompt-level arming policy)** — do not scope T50 independently of these answers. If prompt level becomes load-bearing in the calc, arming policy must accept nulls in a way the new calc can handle. If not, T50's option 3 or 4 is cheaper. ### Filing note — the third configuration-surface-with-no-reader This is the third feature in this codebase fully built at the configuration layer with no or partial runtime consumers: - **Workflow graph pre-Batch-16** — full phase/transition editor, phase advancement didn't consume the graph. - **Finance write paths (T39)** — full write UI, endpoints untried in production. - **This** — full tri-state prompt-level toggle, one partial reader for one third of the state space. The pattern is now general enough to be its own rule — see the new memory entry `feedback_configuration_surface_needs_consumer_first`. --- ## T52 — Server rejection messages are inline literals; FE routing depends on their exact wording **Filed:** ABA shakedown, session-recorder inline-error work. The recorder's inline-error routing (which wizard step surfaces which server rejection) works today, backed by a test. The underlying fragility remains — this ticket documents it and proposes the actual fix. ### The current shape Eleven server rejection message strings are scattered as inline literals across six PHP files. A frontend classifier ([`frontend/src/Pages/SessionData/shared/staffEligibility.ts`](../../frontend/src/Pages/SessionData/shared/staffEligibility.ts) `classifyBlockingMessage`) reads the message text to decide which wizard step owns the error and where to render the inline Alert. Routing depends on the exact wording. **Source files with inline messages:** - `App\Support\StaffEligibility::validateChain` — six literal returns for staff variants (not available / not ABA-authorized / not active / suspended / wrong location / wrong program). - `App\Support\ProgramLockGuard::MESSAGE` — the "Program is locked. Unlock it before editing its targets." variant, plus similar strings inline in `ProgramController::update / destroy / updateStatus` and `ProgramMediaController`, `ProgramTagController`, `ProgramSubtargetController`. - `SessionDataController::autosave` — inline "Selected Program is not assigned to this Student." and "Selected Target is not part of this Program.". **FE classifier** at [`staffEligibility.ts`](../../frontend/src/Pages/SessionData/shared/staffEligibility.ts): substring match on lowercased message → wizard step (2 / 3 / 5 / null). ### The mitigation currently in place - **Jest test** at [`staffEligibility.test.tsx`](../../frontend/src/Pages/SessionData/shared/staffEligibility.test.tsx) — hardcodes every currently-known message string as an assertion. Reword any of the eleven server-side and the test fails, naming the pattern that stopped matching. 19 assertions. - **Page-level fallback Alert** — when a message doesn't classify, a red Alert renders above the wizard with the raw message so the user still sees the error. Never silent. **The test is the mitigation, not the fix.** It covers only currently-known message shapes. A NEW rejection message added on the server without updating the classifier → routes to null → falls through to the page-level Alert. The user sees the message; they don't get the inline-at-the-offending-field routing. ### The proposed fix Introduce error codes; classifier keys off codes, not prose. **Server side:** - Extract each blocking-message string to a named constant, grouped by domain. `StaffEligibility::ERR_STAFF_LOCATION_MISMATCH`, `ProgramLockGuard::ERR_PROGRAM_LOCKED`, `SessionDataController::ERR_PROGRAM_NOT_ASSIGNED`, etc. - Response payload gains a `code` field alongside `message`: `{ status: false, code: 'STAFF_LOCATION_MISMATCH', message: 'The selected Staff member is not authorized...' }`. - FE consumers key routing off `code`; `message` is only for display. **FE side:** - Rename `classifyBlockingMessage(text)` to `classifyBlockingCode(code)` or similar. Table lookup: code → step. - The test's assertions shift from message-text-in → step-out to code-in → step-out. Rewording a message is now purely a display change; routing is code-driven and immune. - The page-level fallback Alert stays as a safety net for unknown codes (a new domain adds a new error but forgets to update the classifier). **Estimate:** a day. Eleven strings, six files, plus payload-shape change on ~6 controller response returns, plus FE classifier + test rewrite. Additive on the response shape (existing consumers ignore the new `code` field) — the FE classifier can migrate incrementally, code-first with text fallback, until all callers are code-aware. ### Canary logging — separate concern, same scope The page-level fallback Alert is a canary — it fires only when the classifier misses. **The app has NO FE structured logging** (grep for Sentry / LogRocket / Datadog / newrelic returned zero). `console.error` is used ad-hoc in 15 files but there's no aggregation. The canary works only when someone is watching the screen at the moment it fires. **Two options:** 1. **Add structured FE logging (Sentry or similar).** Every canary fire logs the message text + user context. Immediate feedback loop when a server-side message rewrite drifts from the classifier. Also solves the wider "we have no crash reporting" problem. 2. **Log to the app's own telemetry table via a `/log-client-event` endpoint.** Cheaper, no third-party dependency, but requires building the endpoint + a viewer. **Recommendation:** file (1) as its own broader ticket ("no FE telemetry / crash reporting exists"). This ticket, T52, ships the code refactor and gets the CANARY correctness for free from any future telemetry work. Both are strict improvements; the code refactor is more urgent than the logging because it eliminates the class of drift that would need logging to detect. ### Related - **Session-recorder inline-error work** (this session) — added the classifier + fallback Alert + test. Documented the fragility in the classifier's docblock, filed here as follow-up. - **T33** (whereNumber 200-vs-404 convention drift) — related in that both are cases of the FE reading server response shape without a stable contract to lean on. **Not blocking, not urgent.** Current test covers currently-known shapes; page-level fallback prevents silence for unknown ones. The window for a new-message-added-without-classifier-update is real but narrow — messages get added when new server validations do, which is a well-observed batch event. --- ## T53 — No structured frontend logging exists **Filed:** ABA shakedown, session-recorder canary work. Recording an existing gap that has cost the project multiple defect-discovery cycles. **The state today:** grep for `Sentry | @sentry | LogRocket | Datadog | newrelic` in `frontend/package.json` + `frontend/src/` returns **zero**. No structured client-side logging, no crash reporting, no telemetry aggregation. The 15 files that use `console.error` are ad-hoc — each site invented its own logging because the need was there and the platform wasn't. **The need has been met piecemeal, which is evidence the platform-level solution is overdue, not that the piecemeal is enough.** **Consequence — what "no logging" actually costs:** - Every **unclassified server rejection** (see T52) goes unseen unless someone happens to be looking at the screen when the fallback Alert fires. - Every **silent failure a user works around** goes unreported. If a user retries, refreshes, or navigates away rather than reporting, the defect never enters the queue. - **"Nobody reported it" and "nobody noticed it" are indistinguishable.** With no telemetry, absence of complaints is not evidence of absence of defects. **The defects in this thread that were live for weeks-to-months and surfaced only when someone screenshotted the exact screen:** - Invoice discount never persisted (Batch 38 fallout — the field was on the form, saved through the API, but the write path lost it). - Staff picker never fetched on open (this session — 18 Autocompletes with the same pattern, only found because one was clicked). - Lock indicator never rendered (three turns of this session — column added, projection missing, serialiser dropping, and finally the discovery that the renderer never existed). None errored. None threw a stack trace. All were only visible to someone looking directly at them at the moment. Structured logging turns "user has to notice AND report" into "the app tells us it happened." **The privacy constraint that scoping this must start from:** this is a clinical application handling student data (names, session notes, per-trial clinical results, IEP-adjacent content). Any error report or telemetry payload can capture DOM state, form values, URL params, and stack-trace context. Off-the-shelf tools ship data to third-party servers by default. The scoping question is not "which SaaS logger" — it's: - **Data residency**: where is the log stored and under whose jurisdiction? Cross-border transfer of student data is a compliance boundary, not a preference. - **PII scrubbing**: what's the mechanism for stripping names, notes, dates-of-birth from payloads BEFORE they leave the browser? Whose responsibility is the redaction list? - **Retention**: how long are logs kept and who can access them? - **Consent**: what does the terms-of-service / privacy notice say about crash reporting today, and does the vendor choice change what it must say? These are the questions that decide whether the answer is a hosted SaaS, a self-hosted stack (Sentry / GlitchTip / self-hosted equivalent), or an in-app `/log-client-event` endpoint into the org's own database. Different answers to those questions produce different-shape solutions, and the wrong shape produces a compliance defect worse than the observability gap. **Not proposing a vendor.** Whoever scopes this starts from the four questions above, not from "which one is popular." The ad-hoc `console.error` piecemeal doesn't answer any of them — it just leaks silently into the browser console and disappears. **Related:** - T52 — the canary that fires when a rejection message is unclassified is invisible without this ticket done. - Defect-discovery pattern generally — the primitives / consumer-first / drift-guard memories all boil down to "detect the defect before the user does." This ticket is the same rule applied to runtime rather than build-time. **Not blocking, not urgent.** No feature is blocked by its absence. Everything shipped in this session works. What we can't tell is what's silently not working elsewhere. --- ## T54 — Staff location is unpopulated across all 189 rows **Filed:** ABA shakedown, session-recorder location-gate audit. **The state today:** 100% of non-deleted `teams` rows (189/189) have `location_id` empty (null). Meanwhile 42% of `students` (1,308/3,109) DO carry a populated `location_id`. The `validateChain` location gate was rejecting when the staff side was null-vs-populated, which meant recording was silently unrecordable for every student-with-location — a rule the picker had been sidestepping via its soft filter. **What shipped alongside this ticket:** the server gate narrowed from `$match !== true` to `$match === false` — reject only on populated-vs-populated real disagreement. Null on either side passes. Server now matches the picker's `location_mismatch !== true` semantics. Recording works today because both sides now agree that a null staff-location doesn't block; nothing enforces that a staff has a location. **The location authorisation gate currently cannot fire on a true mismatch because no staff has a location to mismatch with.** A gate on an empty column is not enforcement. ### Questions for the clinical owner (Kerthiga) 1. **Should staff carry a location, and is it one location or several?** The schema (`teams.location_id`) is a comma-separated string, so it accommodates multi-location staff. But nothing in the app today requires or prompts for a value. If the answer is "staff work at one primary site" → the field becomes a required single-value on the staff editor. If "staff work across sites" → the field stays multi-value and the picker's "authorised elsewhere" behaviour becomes meaningful. 2. **If populated, should the gate then reject cross-location staff, or warn?** Reject is the current server behaviour on populated-vs-populated. Warn would let admins assign cross-location staff deliberately (matching the assign-dialog policy) and only block the record-time flow if the therapist is unauthorised for THIS student's location. Different clinical policies; both defensible. ### Interaction with the recorder — narrow window before the gate becomes live The current shipped state (`$match === false`) is safe **precisely because the column is empty**. The moment someone populates the location_id on any staff record, the gate starts firing on real mismatches against students at other locations — and question 2 above becomes live. That is the trigger for revisiting, not a tidiness impulse. **Data-entry blocker for enabling the gate:** 189 staff rows need `location_id` populated before question 2's answer changes anything on-screen. Each row requires a clinical decision about which site(s) the staff member belongs to — including judgement calls on staff who work across sites. Not a scriptable backfill. **Test coverage of the narrowed gate:** `StaffEligibilityTest::validateChain_passes_staff_with_null_location_against_student_with_location` locks the null-passes behaviour; `validateChain_still_rejects_true_populated_mismatch` locks that a real populated mismatch still rejects. The two tests together prove the gate narrowed, didn't disappear. **Related:** - Session-recorder location-gate audit (this session) — the audit that surfaced the 100% figure. - The picker's `location_mismatch` flag derivation in `ProgramAssignmentController::searchStaff` — matches the current server rule; if question 2's answer is "warn, not reject," both sides adjust together. **Not blocking, not urgent.** Nothing user-facing is broken today. What can't happen today is the location-based authorisation the code implies it enforces — but nothing on the current data would benefit from that enforcement anyway. --- ## Coordination one-pager (current) | Ticket | Blocks | Depends on | Status | |---|---|---|---| | T12 (A1 deploy) | wipe | — | code shipped | | T13 (A11) | A2 deploy | A2 code | ✅ code shipped | | T14 (FE ensureOccurrence) | B1 deploy | — | FE not started | | T15 (editThisOccurrence) | — | — | not started | | T16 (Group B batch) | — | — | not started | | T17 (N+1) | — | A-series done | not started | | T18 (sick source) | wipe completeness | — | not started | | T19 (makeup importer) | — | main re-import | not started | | T20 (preview side effect) | — | — | not started | | T21 (imported horizon monitor) | — | wipe + import complete | not started | | T22 (eligibility gate — future-work-under-scope) | — | — | not started | | T23 (A10 verifies wrong predicate) | — | T22 (shared set) | not started | | T24 (§4a diagnostic stale under A2) | — | — | not started | | **T25 (FE "deferred by scope" banner)** | **daily use of Today page** | **—** | **FE not started** | | T26 (student double-booking; 118 cases across run) | UI conflict guard + billing | product decision | not started | | T27 (retire programs.tags JSON column) | — | one release of pivot-authoritative reads | not started | | T28 (add RateRecorder + re-add `rate` to measurement_type enum) | — | Batch 15 close + client design pass | not started | | T29 (retire programs/program_targets.measurementType varchar) | — | one release of enum-authoritative dispatch | not started | | T30 (client questions awaiting Kerthiga) | — | client answers | 10 open | | T31 (batches specced but not built) | demo readiness | client priority call | 4 demo-critical, 5 post-demo | | T32 (neutral ink/canvas tokens deliberately not swept) | — | — | decision recorded | | T33 (whereNumber not-found convention 200-vs-404 inconsistency) | silent soft-fails on defaulted ids | FE consumer audit | not started | | T34 (FK-vs-soft-delete-and-reinsert standing rule) | demo-fatal data loss (Batch 18 fixed intervals + auto-open) | — | standing rule + retroactive audit complete; diff-update rewrite of save() deferred post-demo | | T35 (FE test harness for the four phase-array pointer-remap helpers) | pointer arithmetic verified only by code review; wrong-phase silent-bind caught by user question, not by test | — | not started; scoped to the four helpers, ~16 cases, ~half a day | | T36 (backend tightening on the invoice validator) | negative discount inflates total; negative taxRate; dueDate < issueDate; unbounded description | — | one file, four lines, no data violations | | T37 (finance-settings admin surface) | payment-terms + future finance settings have no UI | — | Batch 5 territory (SettingsFormLayout) blocks a shared layout, but a standalone page can land ahead | | T38 (legacy import may have zeroed real discount data) | historical per-invoice accuracy if legacy carried discounts | legacy schema audit (may be unanswerable if source retired) | report-only; not fixable in-app without re-import | | T39 (Finance write paths never executed in production) | module is tested but untried; first real run expected to surface integration defects | pre-production shakedown of all three paths against real data | operational, not code; deferred | | T40 (two parallel invoice UIs) | routing inconsistency between legacy /invoice/* and new /finance/*; Dashboard + Create used to 404-land users | do T39 first — reveals which UI is actually in use | spot-fixes landed this turn; broader cleanup deferred | | T41 (hoist B13 tokens out of ProgramLibrary/shared/abaTokens) | Finance now imports from an ABA path — semantic mismatch | do alongside T42 | non-behavioural; ~30 consumer files, codemod-able | | T42 (app-wide canvas: pale-green default vs cool #F7F9FB B13) | Finance is on the app default #eaf3f2 while ABA moved to #F7F9FB; not a Finance-scoped decision | global visual-language call | not a demo blocker; land with T41 | | T43 (delete Pages/ProgramLibrary/shared/abaTokens shim) | shim exists as safety net after Batch 43's codemod | verify grep returns no consumers, then delete | inert; delete after one release cycle | | T44 (ProgramLibrary/List invisible table stripes) | canvas-on-canvas from page wrapper matching stripe colour | Batch 43 canvas flip may make it worse; do visual spot-check first | pre-existing, incidental find; not caused by T42 | | T45 (two silent sources of truth for palette: code fallback vs `light_theme_colors` DB row) | Batch 43's theme flip was inert until the DB row was updated; cost a full audit turn to discover | precedence documentation + consolidation | code-hygiene; not a demo blocker | | T46 (canvas body text `#7e7e7e` never cleared WCAG AA) | 4.35 on canvas, 4.51 on white cards (marginal); pre-existing gap Batch 43 made 0.04 worse | DB row update, not code — see T45 precedence | accessibility; not a demo blocker | | T47 (textPrimary/textSecondary inverted in code, collapsed in DB) | primary/secondary distinction invisible; MUI convention violated | fix with T46, they touch the same row and chain | code-hygiene; do not fix independently | | T48 (18 onInputChange Autocompletes; only 1 fixed) | pickers with data behind them can show "No options" until user types — user reads as broken | per-file judgement (small pool → preload; large pool → keep as-is + explain in noOptionsText) | one fixed (AssignAbaProgramDrawer staff picker); 17 unaudited; ~half a day | | T49 (extract ProgramAssignmentController::searchStaff enrichment) | none today | a THIRD caller wanting the enriched row shape (not the primitives, which are already shared) | primitives extracted this batch; enrichment stays inline; extract when 3rd caller arrives, not on tidiness impulse | | T50 (recorder prompt-level arming policy) | silent clinical data loss — trial happens at some level of support, record says nothing | product decision (require arm / auto-arm default / accept nulls) + T51's answers | not started; clinical policy question, not engineering; **do not scope independently of T51** | | T51 (outcomeState tri-state with one partial consumer) | configuration surface with no consumer for 2/3 of the state space | Kerthiga's answers to two clinical-owner questions in the ticket body | not started; questions posed; scope depends on answers | | T52 (server rejection messages are inline literals; FE routing depends on wording) | classifier drift when a message is reworded — new messages fall through to a page-level fallback Alert; canary is invisible without FE logging | none blocking; ideally paired with adding FE telemetry so the canary is observable | mitigation shipped (test + fallback); real fix is server error codes; ~day of work | | T53 (no structured frontend logging exists) | silent failures indistinguishable from absence of failures; several thread-defects surfaced only via screenshot | data-residency / PII-scrubbing / retention / consent decisions (student data) — not a vendor pick | not started; scope starts from privacy constraints, not vendor comparison | | T54 (100% of staff have empty location_id; gate cannot fire on true mismatch) | location authorisation is decorative until the column is populated; the moment it is, warn-vs-reject becomes live | Kerthiga's answers to two clinical questions + per-row backfill (189 rows) | server gate narrowed (`$match === false`) this session; safe today; revisit when column populated | | T55 (three unread clinical flags on mastery_workflows named after safety rules) | **Naming problem, not consumer problem.** `automaticProgression`, `manualOverrideAllowed`, `supervisorApprovalRequired` exist in schema with correct-sounding defaults, are never read by any code path, and were until this session being silently reset on every workflow save. A supervisor reading "Supervisor approval required" in a schema would reasonably assume supervision is enforced — it is not. Advancement (`SessionDataController::checkMasteryCriteria` at :544) uses phase-level rules alone. Decide: wire real readers (advancement engine consults `supervisorApprovalRequired` before auto-advance; manual phase change gated on `manualOverrideAllowed`) OR drop the columns and the names with them. **Cannot be answered from data — see T58.** | clinical policy decision on whether supervision-gated advancement is a real requirement | fifth instance of "configuration surface with no consumer"; distinct because these have no writer either — the columns exist solely because the schema has them | | T56 (archive on mastery_workflows is a label with no downstream effect) | Programs currently using an archived workflow keep using it, and nothing tells anyone. This session added a narrow list-picker exclusion so new selections cannot pick archived workflows; existing assignments are untouched. Bigger feature: bulk unarchive control, warnings on program pages listing an archived workflow, soft-exclusion with override, definition of what "archive" means operationally beyond the picker gate. | product/clinical decision on what archive means (soft-hide, prevent-new-use, prevent-any-use, or something else) | narrow picker fix landed this session; bigger feature deferred | | T57 (no default mastery workflow set in production) | Two workflows in prod, both `isDefault=0`. Whether that state is original or the result of the silent-reset bug fixed this session cannot be determined from data (see T58). Nothing today depends on a default being set, but the FE editor exposes a "Set default" affordance whose semantic value depends on there being exactly one. | product decision on whether a default is required; if yes, someone picks explicitly | undeterminable from data; do not set on a guess | | T58 (no audit history on mastery_workflows) | Six workflow-level fields (status, isDefault, scope, automaticProgression, manualOverrideAllowed, supervisorApprovalRequired) were silently resetting on every save until this session. Because the table has only `updated_by`/`updated_at` snapshots and no history table, we cannot tell how long that had been happening, which specific values were lost, or whether the current row state is the deliberate configuration or the last accidental clobber. This is the direct blocker on T55 (answering "were these flags ever set to non-default values?"). Same shape as `session_data_audit` and `payment_audit_logs` which exist for other clinical/financial tables. | design decision on audit granularity (column-level diff vs full row snapshot); precedent set by `session_data_audit` | cross-linked with T55; without this ticket, T55 stays a data-blind decision |