# Storage Engine & Transaction Boundary Audit

**Date:** 2026-07-13
**Database audited:** `stepup_admin_db` (MySQL 5.7.44)

> **STATUS — Batch 1 (Finance) and Batch 2 (Attendance/Cancellations) are COMPLETE.**
>
> | Batch | Migration | Tables | Status |
> |---|---|---|---|
> | — | `2026_07_13_000001_convert_class_schedules_to_innodb` | 1 | Done |
> | 1 — Finance | `2026_07_13_000002_convert_finance_tables_to_innodb` | 8 | Done |
> | 2 — Attendance | `2026_07_13_000003_convert_attendance_tables_to_innodb` | 3 | Done |
> | 3 — Roles/Permissions | `2026_07_13_000004_convert_rbac_tables_to_innodb` | 2 | Done |
> | Payroll code defects | *see `PAYROLL_TRANSACTION_DEFECTS.md`* | — | Done |
> | 4 — Chat/Mastery/Programs/Prompts | `2026_07_14_000001_convert_module_tables_to_innodb` | 10 | Done |
>
> # ✅ AUDIT COMPLETE — all 18 non-atomic workflows are now genuinely atomic.
>
> MyISAM tables in the main DB: **42 → 19**. Every remaining MyISAM table is one that
> **no transaction writes** (see "Remaining MyISAM tables" at the end).
>
> Every `DB::transaction()` / `beginTransaction()` boundary in the codebase now has all of
> its participating tables on InnoDB, correctly paired commit/rollback, `\Throwable` catches,
> and no external side effects (mail) inside the boundary.
>
> Open follow-ups are tracked in `FOLLOWUP_TICKETS.md` — chiefly **T5: foreign keys are
> still missing** on the converted tables (MyISAM discarded the definitions at CREATE time,
> and an engine conversion does not restore them).

---

## Executive summary

**18 of 35 transactional workflows are NOT atomic.** Their code opens a transaction and rolls back on failure, but one or more participating tables is MyISAM, which silently ignores transactions. The rollback appears to succeed and returns no error — the writes simply stay.

**Every finance workflow is affected. All nine of them. Money is the worst-exposed subsystem in the application.**

| Area | Workflows | Not atomic | Verdict |
|---|---|---|---|
| Finance | 9 | **9** | Every one. `invoices`, `invoice_items`, `payments`, `transactions`, `accounts`, `expenses` are all MyISAM |
| Attendance / class ops | 8 | **3** | `attendance_records`, `class_cancellations`, `notifications` are MyISAM |
| Payroll | 4 | 0 | All InnoDB — **no action needed** |
| Parent / student / user | 4 | **2** | Only `role_permissions` is MyISAM |
| Other modules | 10 | **4** | Chat, Mastery, Programs, Prompt Levels |

Root cause: the MySQL server here has `default_storage_engine = MyISAM`, so any table created without an explicit engine landed as MyISAM. `config/database.php` now pins `'engine' => 'InnoDB'`, so **new** migrations are safe. The 42 MyISAM tables are legacy, created before that config existed.

---

## Method

1. Enumerated every `DB::transaction()` / `beginTransaction()` / `commit()` / `rollBack()` site in `app/` (28 files).
2. For each, traced every table **written** inside the transaction boundary, resolving Eloquent model classes to real table names — including writes made indirectly by services called inside the boundary (e.g. `LedgerService`).
3. Cross-referenced each write-table against `information_schema.tables.engine` programmatically.

A workflow is **atomic** only if *every* table it writes is InnoDB. One MyISAM participant breaks the whole guarantee.

---

## 1. Finance — CRITICAL

Files: `Helpers/LedgerService.php`, `Controllers/FinanceController.php`, `Controllers/InvoiceController.php`, `Controllers/PaymentController.php`, `Controllers/ParentPortalController.php`, `Console/Commands/GenerateMonthlyInvoices.php`, `Console/Commands/GeneratePerClassInvoices.php`

| # | Workflow | Method | Tables written | Atomic? | Risk |
|---|---|---|---|---|---|
| F1 | Ledger credit/debit primitive | `LedgerService::recordInternal()` L179 | `accounts` (MyISAM), `transactions` (MyISAM) | **NO** | Critical |
| F2 | Account-to-account transfer | `LedgerService::transfer()` L55 | `accounts` (MyISAM), `transactions` (MyISAM) | **NO** | Critical |
| F3 | Record an expense | `FinanceController::storeExpense()` L146 | `expenses`, `accounts`, `transactions` — all MyISAM | **NO** | Critical |
| F4 | Create/update invoice | `InvoiceController::store()` L132 | `invoices`, `invoice_items` — both MyISAM | **NO** | Critical |
| F5 | Gateway payment | `PaymentController::store()` L122 | `payments`, `invoices`, `accounts`, `transactions` — all MyISAM | **NO** | Critical |
| F6 | Manual payment (cash/cheque) | `PaymentController::recordManualPayment()` L403 | `payments`, `invoices`, `invoice_items`, `accounts`, `transactions` — all MyISAM | **NO** | Critical |
| F7 | Monthly invoice generation (cron) | `GenerateMonthlyInvoices::generateForStudent()` L103 | `invoices`, `invoice_items` — both MyISAM | **NO** | Critical |
| F8 | Per-class invoice generation (cron) | `GeneratePerClassInvoices::processStudentSessions()` L104 | `invoices` (MyISAM), `invoice_items` (MyISAM), `class_schedules` (InnoDB) | **NO — mixed** | Critical |
| F9 | Parent portal: pay invoices | `ParentPortalController::payInvoices()` L703 | `payments`, `invoices`, `accounts`, `transactions` — all MyISAM | **NO** | Critical |

### What this means concretely

**F6 (manual payment)** writes five MyISAM tables. If it fails after inserting the `payments` row but before syncing the invoice balance, the rollback does nothing: the payment exists, the invoice still shows a balance due, and the ledger has no matching `transactions` row. The books are silently out of balance and nothing surfaces an error.

**F8 is now MIXED**, and mixed is worse than uniformly broken. Since `class_schedules` was converted to InnoDB, a failure now rolls back the `class_schedules.invoiced` flags while **leaving the invoice and its line items committed** — sessions get re-invoiced on the next cron run because their `invoiced` flag was reverted but the invoice already exists. This is a direct, newly-introduced consequence of the `class_schedules` conversion and is an argument for doing Batch 1 promptly.

### Non-engine defects found in the same code (report only, not fixed)

These are independent of storage engine and should be tracked separately:

- `PaymentController::store()` L107 — `lockForUpdate()` runs **before** `beginTransaction()` (L122). Under autocommit the row lock releases immediately, so the overpayment check is not serialized.
- `PaymentController::store()` L141 — `PaymentGateway::charge()` (external I/O) is called **inside** the open transaction.
- `PaymentController::store()` L170-172 — ledger failure is caught and only logged; the payment stays `completed` with no `transactions` row.
- `PaymentController::store()` L198 — references an undefined `$totalPaid`.
- Invoice-number lookups (`InvoiceController` L180, `PaymentController` L477, `GenerateMonthlyInvoices` L152) are unlocked → duplicate `invoiceNumber` under concurrency.
- `FinanceController::storeExpense()` L144 — `getDefaultAccount()` can INSERT into `accounts` **before** `beginTransaction()`; that write is outside the rollback.
- `FinanceController::storeAccount()` L69-85, `InvoiceController::destroy()` L256, `InvoiceController::sendInvoice()` L301 — money-touching writes with **no transaction at all**.

---

## 2. Attendance & class operations — HIGH

Files: `AttendanceRecordController.php`, `ClassCancellationController.php`, `ClassScheduleController.php`, `classesAssignStudentController.php`, `Services/ClassOccurrenceService.php`, `Services/ClassSeriesEditService.php`

| # | Workflow | Method | Tables written | Atomic? | Risk |
|---|---|---|---|---|---|
| A1 | Save attendance + notify parents | `AttendanceRecordController::store()` L110 | `attendance_records` (MyISAM), `notifications` (MyISAM) | **NO** | High |
| A2 | Cancel class (+ makeup) | `ClassCancellationController::store()` L86 | `class_cancellations` (MyISAM), `notifications` (MyISAM), `class_schedules` (InnoDB) | **NO — mixed** | High |
| A3 | Release class (refund/makeup) | `ClassScheduleController::releaseClass()` L679 | `class_cancellations` (MyISAM), `notifications` (MyISAM), `class_schedules` (InnoDB) | **NO — mixed** | High |
| A4 | Switch staff for future classes | `ClassScheduleController::switchStaff()` L598 | `class_schedules` (InnoDB) | YES | Low |
| A5 | Generate occurrences | `ClassOccurrenceService::generate()` L220 | `class_schedules` (InnoDB) | YES | Low |
| A6 | Assign student + generate 6 months | `classesAssignStudentController::store()` L120 | `assign_student_classes`, `assign_classes_histories`, `class_schedules` — all InnoDB | YES | Low |
| A7 | Series edit: entire series | `ClassSeriesEditService::editEntireSeries()` L68 | `assign_classes`, `assign_classes_schedules`, `assign_student_classes`, `class_schedules` — all InnoDB | YES | Low |
| A8 | Series edit: this and future | `ClassSeriesEditService::editThisAndFuture()` L82 | same four, all InnoDB | YES | Low |

**A2 and A3 are mixed and dangerous.** Cancelling a class updates `class_schedules.status = 'cancelled'` (InnoDB, rolls back) and inserts a `class_cancellations` audit row (MyISAM, does not). On failure you get a **cancellation record for a class that is still marked scheduled** — the audit trail and the schedule disagree, and no error is raised.

A4–A8 are already safe (they were fixed by the `class_schedules` conversion).

### Non-engine defects (report only)

- `ClassScheduleController::switchStaff()` L629 — the single-class branch updates outside any transaction, yet the shared `catch` calls `DB::rollBack()` (L639) on a connection with no active transaction.
- `ClassSeriesEditService` L73 / L110 — regeneration via `generateForClass()` runs **after** the transaction commits. A failure there leaves the series deleted-but-not-regenerated.
- `classesAssignStudentController::destroy()` L174 — writes `assign_classes_Histories` + `assign_student_classes` with no transaction.
- `AttendanceRecordController::store()` — `Mail::raw` and Twilio SMS are sent **inside** the open transaction.

---

## 3. Payroll — NO ACTION NEEDED

Files: `EmployeePunchDetailsController.php`, `EmployeePunchRecordController.php`, `FaceEmpController.php`

| # | Workflow | Method | Tables written | Atomic? | Risk |
|---|---|---|---|---|---|
| P1 | Admin manual punch | `EmployeePunchDetailsController::store()` L2037 | `employee_punch_details`, `employee_delay_records`, `employee_punch_details_records`, `emp_admin_edit_punch_records`, `const_records` — all InnoDB | YES | Low |
| P2 | Device/fingerprint punch | `storeLocall()` L2342 | same five, all InnoDB | YES | Low |
| P3 | Admin punch delete | `destroy()` L2739 | four tables, all InnoDB | YES | Low |
| P4 | Payroll finalization / history | `EmployeePunchRecordController::moveAllPayrollData()` L1387 | `save_final_history_payroll_data`, `record_payroll_dates`, `save_emp_punch_details_records`, `employee_punch_details_records`, `save_history_payroll_records`, `history_payroll_records` — all InnoDB | YES | Low |

**Payroll is entirely InnoDB. No engine conversion required.** However it has the most severe *code-level* transaction defects in the codebase:

- `moveAllPayrollData()` L1585 — the `catch` block has **no `DB::rollBack()`**. An exception leaves the transaction open and unrolled. This one destroys payroll history (`history_payroll_records` is a **hard delete**, L1562).
- `moveAllPayrollData()` L1424 — the validator runs *after* `beginTransaction()`, so a validation-failure `return` leaks an open transaction.
- `storeLocall()` L2357 — early `return` after `beginTransaction()` with no rollback → leaked open transaction.
- **Five methods call `DB::rollBack()` with no `beginTransaction()` anywhere in scope** — the rollback is a no-op and the preceding writes are already committed under autocommit: `fingerprint()` L341, `testingPunch()` L965, `recordPunch()` L1451, `FaceEmpController::recordPunchFacialData()` L468, `FaceEmpController::storeLocall()` L945. `FaceEmpController` contains **zero** `beginTransaction`/`commit` calls yet writes four tables.

These are logic bugs, not engine bugs. Converting nothing will fix them. **Recommend a separate ticket.**

---

## 4. Parent / student / user — MEDIUM

| # | Workflow | Method | Tables written | Atomic? | Risk |
|---|---|---|---|---|---|
| U1 | Parent detach from children | `ParentController::destroy()` L245 | `relationship_tables` (InnoDB) | YES | Low |
| U2 | Parent soft-delete (cascade) | `ParentController::destroy()` L255 | `student_parents`, `relationship_tables`, `users` — all InnoDB | YES | Low |
| U3 | Role permission matrix save | `RolePermissionController::store()` L82 | `role_permissions` (MyISAM) | **NO** | Medium |
| U4 | Delete role + detach users | `UseRollController::destroy()` L272 | `users` (InnoDB), `user_roles` (InnoDB), `role_permissions` (MyISAM) | **NO — mixed** | **High** |

**U3 is a security-adjacent concern.** Saving the permission matrix bulk-soft-deletes every existing `role_permissions` row for the role, then re-inserts. If it fails between the two, the rollback does nothing and **the role is left with no permissions**.

**U4 is worse and mixed.** It hard-deletes `role_permissions` (MyISAM — permanent) while detaching users (InnoDB — rolls back). On failure the permissions are gone forever but the role and users are restored. Rated High despite being in the Medium group.

Parent/student management itself (U1, U2) is already safe.

---

## 5. Other modules — MEDIUM / LOW

| # | Workflow | Method | Tables written | Atomic? | Risk |
|---|---|---|---|---|---|
| M1 | Create chat conversation | `ChatController::createConversation()` L176 | `conversations`, `conversation_participants`, `chat_messages` — all MyISAM | **NO** | Medium |
| M2 | Mastery workflow save | `MasteryWorkflowController::store()` L103 | `mastery_workflows`, `mastery_workflow_phases`, `mastery_maintenance_schedules` — all MyISAM | **NO** | Medium |
| M3 | Clone program | `ProgramController::cloneProgram()` L317 | `programs`, `program_targets` — both MyISAM | **NO** | Medium |
| M4 | Prompt level template save | `PromptLevelTemplateController::store()` L86 | `prompt_level_templates`, `prompt_levels` — both MyISAM | **NO** | Medium |
| M5–M10 | Leads, Email Hub, Email templates | `LeadService`, `LeadContactController`, `EmailHubV3Service`, `EmailSyncService`, `EmailTemplateService` | `lead_*`, `customers`, `email_*`, `crm_email_templates` — all InnoDB | YES | Low |

The entire Leads/Email Hub subsystem is InnoDB (it is newer, built after the config fix). No action.

M1–M4 all follow the same "soft-delete children, re-insert children" pattern, which is exactly the pattern that corrupts without a working rollback: a mid-way failure leaves the parent with **zero** children (phases, targets, levels).

---

## Recommended conversion order

Batches are grouped **by workflow**, so each batch makes a complete set of workflows atomic. Never mix unrelated workflows in one migration.

### Batch 1 — Finance (CRITICAL, do first)
`accounts`, `transactions`, `invoices`, `invoice_items`, `payments`, `expenses`, `expense_categories`, `payment_methods`

Makes F1–F9 atomic. Also resolves the F8 mixed-state regression. Highest value: this is money.

### Batch 2 — Attendance & class cancellation (HIGH)
`attendance_records`, `class_cancellations`, `notifications`

Makes A1–A3 atomic, resolving the A2/A3 mixed states. `notifications` is shared with Batch 1's blast radius (it is written by several flows), so it must land no later than here.

### Batch 3 — Roles & permissions (HIGH, small)
`role_permissions`, `permissions`

Makes U3, U4 atomic. Tiny table, tiny risk, removes a permanent-data-loss path. Could reasonably be merged into Batch 2 by schedule, but keep it a **separate migration file**.

### Batch 4 — Remaining modules (MEDIUM)
Chat: `conversations`, `conversation_participants`, `chat_messages`, `messages`, `message_reads`
Mastery: `mastery_workflows`, `mastery_workflow_phases`, `mastery_maintenance_schedules`
Programs: `programs`, `program_folders`, `program_targets`
Prompt levels: `prompt_levels`, `prompt_level_templates`

### Not scheduled (no transactional workflow writes them)
`absence_reports`, `activity_logs`, `assignments`, `assignment_submissions`, `class_messages`, `class_requests`, `courses`, `course_materials`, `password_otps`, `session_data_records`, `staff_availability`, `student_assessments`, `student_challenges`, `student_notes`, `student_strengths`, `tutoring_planners`

Low priority. Convert opportunistically for FK-readiness and crash-safety, not for atomicity.

### Payroll
**No conversion needed.** Already InnoDB. Fix the code defects instead (see §3).

---

## Each batch must include

Following the pattern proven by `2026_07_13_000001_convert_class_schedules_to_innodb.php`:

1. **Preflight**: current engine, row count, index signature, table size, free disk.
2. **Duplicate / NULL checks** on any column set that a future unique key or FK would need — surface blockers *before* the ALTER, and before any FK work.
3. **Guards**: skip if already InnoDB; skip if the table does not exist; never a blind ALTER.
4. **Post-verification, inside the migration**: engine changed, row count unchanged, index set byte-for-byte unchanged — throw otherwise.
5. **`down()`** back to MyISAM with the same verification.
6. **One integration test per batch** that forces a failure mid-workflow and asserts every participating table rolled back. The test must assert the generator/service actually wrote rows *inside* the transaction first, so it cannot pass vacuously.

### Foreign keys — later, separately
Do **not** add FKs in an engine-conversion migration. InnoDB makes FKs *possible*; it does not require them. FKs need their own audit because MyISAM ignored FK definitions for years, so orphan rows are likely to exist and would block the constraint. Batch 1's duplicate/NULL preflight will begin surfacing that.

---

## Test environment drift

### Why `stepup_test_db` is 40 migrations behind

It is not seeded or rebuilt by any automated process. `phpunit.xml` sets `DB_DATABASE=stepup_test_db`, but nothing ever runs migrations against it — there is no `RefreshDatabase` trait in `tests/TestCase.php` (it is an empty stub) and no CI step. Every test class manages its own fixtures and cleans up by marker. So the test DB only ever advanced when a developer manually ran `migrate` against it, and it drifted from that point on.

Consequences observed:
- 40 pending migrations.
- **96 of 98 tables are MyISAM** (vs 42 of 127 in the main DB) — so the test DB could not validate transactional behaviour *at all* until now.
- `users.last_login_at` is missing → 8 real test failures (see below).

### Proposed safe rebuild process

**Do not hand-patch individual tables again.** (I did exactly that to unblock the `class_schedules` rollback test — converting the four tables in that transaction. That was expedient, not correct, and it is precisely the drift this section exists to end.)

The rebuild must be impossible to point at production. Three independent guards:

1. **Add a dedicated `testing` environment file** — `.env.testing`, with `DB_DATABASE=stepup_test_db`. Laravel loads it automatically for `--env=testing`.
2. **Add a name guard to the command.** Before any destructive step, assert the connected database name contains `test`. `ParentManagementTest::setUp()` already does exactly this and is the right precedent:
   ```php
   $dbName = DB::connection()->getDatabaseName();
   if (! str_contains($dbName, 'test')) {
       $this->fail("Refusing to run against non-test database '{$dbName}'.");
   }
   ```
   Promote this into a small `artisan test:refresh` command (or a `DatabaseSafetyGuard` invoked from `AppServiceProvider::boot()` when `app()->environment('testing')`) so **no** destructive test command can execute against a database whose name lacks `test`.
3. **Never pass `--force`.** `migrate:fresh` refuses to run in production without it; keep that safety net armed.

Then:
```bash
php artisan migrate:fresh --seed --env=testing
```
This drops and recreates every table. Because `config/database.php` pins `'engine' => 'InnoDB'`, **every table comes back as InnoDB**, which fixes the test-DB engine drift wholesale and lets the suite validate real rollback behaviour.

Verify afterwards:
```sql
SELECT COUNT(*) FROM information_schema.tables
WHERE table_schema = 'stepup_test_db' AND engine <> 'InnoDB';   -- expect 0
```

One caveat to expect: a full `migrate:fresh` on InnoDB may surface index-length errors that MyISAM tolerated (InnoDB's 3072-byte limit vs MyISAM's 1000-byte key limit, on utf8mb4). If a migration fails, that is a **real latent bug** in that migration, not a reason to abandon the rebuild.

---

## ParentManagementTest — 8 failures (documented, NOT fixed)

Per instruction, these were not touched.

**Failing tests** (all in `tests/Feature/ParentManagementTest.php`):
1. `create parent creates record and portal account`
2. `list shows parent with multiple children`
3. `disable one child hides only that child from portal`
4. `disable parent blocks login`
5. `cannot delete parent with children`
6. `stp username increments and never reuses deleted numbers`
7. `can delete parent whose only links are to deleted students`
8. `primary billing flag is preserved`

(`generated password is six alnum with all categories` passes — it is the only test that never hits the API.)

**Current error:** `POST /api/parent` returns **HTTP 500** instead of 200. Underlying exception:
```
SQLSTATE[42S22]: Column not found: 1054
Unknown column 'last_login_at' in 'field list'
```

**Cause: schema drift, not stale fixtures and not a code bug.**
`ParentController.php:725` selects `last_login_at`:
```php
->get(['id', 'parent_id', 'username', 'email', 'status', 'last_login_at'])
```
That column is added by `2026_07_01_000000_add_last_login_at_to_users_table.php`, which is **Pending** on `stepup_test_db`. The column exists in `stepup_admin_db`. A schema diff of `users` between the two databases shows exactly one difference: `last_login_at`, missing in test.

**Expected resolution:** the `migrate:fresh --env=testing` rebuild above should fix all 8 with no code change. That should be confirmed, not assumed.

**Likely module owner:** Parent Management (the module that introduced `ParentController` and the `last_login_at` column).

**Confirmed NOT caused by the engine work:** `students` was reverted to MyISAM and the suite re-run — all 8 still failed identically. Suite totals are unchanged from before the `class_schedules` migration: **45 passed, 8 failed, 3 skipped**.

---

## Environment note

The MySQL server's `default_storage_engine` is **MyISAM** (MySQL 5.7.44, WAMP default). Where the environment is under our control it should be set to `InnoDB`:

```ini
[mysqld]
default_storage_engine = InnoDB
```

**Keep `'engine' => 'InnoDB'` in `config/database.php` regardless.** It is the only thing that made new tables safe while the server default was wrong, it documents the intent in code, and it protects against deployment to any server whose default is not InnoDB. Belt and braces.

---

# Batch 1 — completion record (Finance)

**Migration:** `database/migrations/2026_07_13_000002_convert_finance_tables_to_innodb.php`
**Duration:** ~1s (8 tables, 47.6 KB total)
**Tables skipped:** none — all 8 existed and were MyISAM.

## Before / after

| Table | Engine before | Engine after | Rows before | Rows after | Indexes before | Indexes after |
|---|---|---|---|---|---|---|
| accounts | MyISAM | **InnoDB** | 0 | 0 | 3 | 3 |
| transactions | MyISAM | **InnoDB** | 0 | 0 | 5 | 5 |
| invoices | MyISAM | **InnoDB** | 2 | 2 | 6 | 6 |
| invoice_items | MyISAM | **InnoDB** | 2 | 2 | 3 | 3 |
| payments | MyISAM | **InnoDB** | 2 | 2 | 3 | 3 |
| expenses | MyISAM | **InnoDB** | 0 | 0 | 3 | 3 |
| expense_categories | MyISAM | **InnoDB** | 0 | 0 | 2 | 2 |
| payment_methods | MyISAM | **InnoDB** | 0 | 0 | 2 | 2 |

Every index preserved by name, uniqueness and column order — verified inside the migration,
which throws on any drift. Preflight found **0 duplicate groups** against every existing
UNIQUE index (including `invoices.invoiceNumber` and `transactions.txn_source_unique`) and
no NULLs in any indexed column except the nullable `invoice_items.relatedClassId`.

## Tests

`tests/Feature/FinanceTransactionTest.php` — **5 passed, 35 assertions**

1. Finance tables use a transactional engine (precondition; fails loudly otherwise).
2. **Manual payment failure** rolls back `payments`, `invoices` (paid_amount + balance_due),
   `accounts` (balance), `transactions` — all together.
3. **Per-class invoice failure** leaves no invoice, no invoice items, and
   `class_schedules.invoiced` unchanged (the re-invoicing bug).
4. **Expense failure** rolls back `expenses`, `transactions`, and the account balance.
5. Happy path still commits every finance write.

Each test asserts **inside the open transaction** that rows really landed (payment exists,
account balance moved 1000 → 1200, session flagged invoiced) before forcing the exception,
so none can pass vacuously.

Full suite: **60 passed, 1 failed** (was 45 passed, 8 failed).

---

# New findings from Batch 1

## N1 — `migrate:fresh` was broken (FIXED)

Rebuilding the test database exposed that **`php artisan migrate:fresh` could not complete at all**:

```
SQLSTATE[23000]: 1452 Cannot add or update a child row: a foreign key constraint fails
(`privileges`, CONSTRAINT `privileges_created_by_foreign` FOREIGN KEY (`created_by`) REFERENCES `users`(`id`))
```

`2025_04_13_000019_seed_new_privileges.php` inserts rows with `created_by = 1`, but on a fresh
database no user with id 1 exists yet. This never surfaced because it only bites when the table
is actually InnoDB — MyISAM parsed the FK definition and silently discarded it. The project's
`migrate:fresh` had therefore been quietly broken for its entire life.

**Fixed** by `2025_04_13_000018_ensure_system_user_exists.php`, which idempotently creates a
system user (id 1) before the privilege seeds. It is a no-op on every existing database.

## N2 — The main database has NO foreign keys on the finance tables

| Database | FKs on the 8 finance tables |
|---|---|
| `stepup_admin_db` (converted from MyISAM) | **0** |
| `stepup_test_db` (rebuilt fresh, InnoDB) | **4** |

The migrations *define* these foreign keys. MyISAM discarded them at CREATE time, so on the
main database they simply do not exist — and **converting the engine does not bring them back**.
Production and a fresh install now have materially different constraint sets.

This is hard evidence for the deferred FK audit: the gap is real, it is not theoretical, and it
is invisible unless you diff `information_schema.table_constraints`. Expect orphan rows
(MyISAM never enforced anything) to block some constraints when they are added.

## N3 — Test suite: 8 Parent failures RESOLVED, 1 new failure surfaced

**ParentManagementTest — all 8 failures are gone.** As predicted in the original audit, they were
schema drift: `users.last_login_at` was missing from the stale test DB while
`ParentController.php:725` selects it. The `migrate:fresh` rebuild fixed all 8 with no code change.

**EmailProviderTest — 1 failure, newly surfaced, NOT a regression.**
`duplicate messages are prevented across a re sync`:

```
SQLSTATE[HY000]: 1364 Field 'subject' doesn't have a default value
insert into `email_threads` (`thread_id`, `account_id`, ...)
```

`tests/Feature/EmailProviderTest.php:82` creates an `EmailThread` without a `subject`, but
`email_threads.subject` is NOT NULL in the current schema (in the main DB too). The stale test DB
had an older, laxer version of the table, which is why it passed before. **This is a test-fixture
bug, not application code** — no production impact. Left unfixed per the "do not fix other
modules' tests" instruction; it belongs to the Email Hub module and needs one field added.

## N4 — Test environment (RESOLVED)

`stepup_test_db` was rebuilt with `php artisan test:refresh --env=testing`, a new guarded command
(`app/Console/Commands/RefreshTestDatabase.php`). It refuses to run unless **both** the environment
is `testing` **and** the database name contains `test`, and additionally refuses the primary
database by name. Both guards were verified to actually block:

```
$ php artisan test:refresh
REFUSING: environment is 'local', expected 'testing'.

$ DB_DATABASE=stepup_admin_db php artisan test:refresh --env=testing
REFUSING: database 'stepup_admin_db' does not contain 'test'.
```

Result: **127 tables, 0 non-InnoDB.** The test DB now matches the current schema and can validate
real transactional behaviour. `.env.testing` was added (`APP_ENV=testing`, `DB_DATABASE=stepup_test_db`).

Note: `--seed` still fails on a fresh DB for an unrelated reason (seeder ordering vs. the now-enforced
FKs). Migrations alone are sufficient for the suite, which builds its own fixtures. Worth a follow-up.

---

# Batch 2 — completion record (Attendance & Class Cancellations)

**Migration:** `database/migrations/2026_07_13_000003_convert_attendance_tables_to_innodb.php`
**Duration:** 497 ms (3 tables, 6.0 KB)
**Tables skipped:** none — all 3 existed and were MyISAM.

## Before / after

| Table | Engine before | Engine after | Rows before | Rows after | Indexes before | Indexes after |
|---|---|---|---|---|---|---|
| attendance_records | MyISAM | **InnoDB** | 0 | 0 | 3 | 3 |
| class_cancellations | MyISAM | **InnoDB** | 0 | 0 | 2 | 2 |
| notifications | MyISAM | **InnoDB** | 0 | 0 | 2 | 2 |

Every index preserved by name, uniqueness and column order — verified inside the migration,
which throws on drift. Preflight: **0 duplicate groups** against every UNIQUE index (notably
`attendance_records (classScheduleId, studentId)`), no NULLs in any indexed column.

## Workflows now atomic

| # | Workflow | Was | Now |
|---|---|---|---|
| A1 | Save attendance + notify parents | NOT atomic | **Atomic** |
| A2 | Cancel a class (+ makeup) | NOT atomic — **mixed** | **Atomic** |
| A3 | Release a class (refund/makeup) | NOT atomic — **mixed** | **Atomic** |

The mixed-engine failure mode is closed: a cancellation can no longer leave an audit row
behind for a class that is still marked `scheduled`, an abandoned release can no longer strand
an orphaned zero-fee makeup occurrence, and a parent can no longer be notified of an absence
that was never recorded.

**No mixed-engine transaction remains in Finance or Class Operations.**

## Tests

`tests/Feature/AttendanceCancellationTransactionTest.php` — **5 passed, 32 assertions**

1. Class-ops tables use a transactional engine (precondition; fails loudly otherwise).
2. **Cancel-class failure** rolls back the occurrence status, the `class_cancellations` audit
   row and the notification, together.
3. **Release-class failure** rolls back the status change, the makeup occurrence, the release
   history and the notification — and leaves `assign_student_classes` enrolment untouched.
4. **Attendance-save failure** rolls back `attendance_records` (including the `parentNotified`
   stamp) and the notification.
5. Happy path still commits every write.

Each asserts **inside the open transaction** that the rows really landed before forcing the
exception, so none can pass vacuously.

## Suite

| Suite | Result |
|---|---|
| ClassScheduleTransactionTest | 3 passed |
| FinanceTransactionTest | 5 passed |
| AttendanceCancellationTransactionTest | 5 passed |
| ParentManagementTest | **9 passed** |
| ClassOccurrenceTest | 11 passed |
| **Full suite** | **65 passed, 1 failed** |

The single failure is the pre-existing `EmailProviderTest` fixture bug (N3 above) — a test that
creates an `EmailThread` without the NOT NULL `subject`. **Unchanged by Batch 2 and not caused by
it**; it belongs to the Email Hub module and is a one-field fixture fix.

## Hardening applied to `ensure_system_user_exists` (N1 follow-up)

The system row (`users.id = 1`), added to unblock `migrate:fresh`, was reviewed for credential risk
and hardened. It is a referential placeholder, not an account:

- Password is a bcrypt hash of **64 cryptographically-random characters** generated in the
  migration and never stored, logged or returned. There is no default and nothing to guess.
- `user_role = 0` → resolves to **0 permissions** (verified).
- No email → cannot be targeted by password reset.
- Real admin provisioning remains `UserSeeder` (`vmadmin`), which keys on email and does not claim id 1.

A non-bcrypt "unusable password" sentinel (the Django convention) was tried and **rejected**:
Laravel's `BcryptHasher::check()` *throws* `This password does not use the Bcrypt algorithm` on a
non-bcrypt hash, so a login attempt against `system` would return 500 instead of a clean rejection.
A random bcrypt hash is equally unusable and fails closed, quietly. Verified: `system`, `password`,
`admin`, `""`, `AdminAdmin@123` and `vmadmin` are all rejected, with no exception thrown.

---

# Batch 3 — next recommended scope

**Tables:** `role_permissions`, `permissions` (both still MyISAM — confirmed).

**Workflows it makes atomic:**

| # | Workflow | Method | Risk |
|---|---|---|---|
| U3 | Role permission matrix save | `RolePermissionController::store()` L82 | Medium — bulk soft-deletes every `role_permissions` row for the role then re-inserts; a failure between the two leaves the role with **no permissions** |
| U4 | Delete role + detach users | `UseRollController::destroy()` L272 | **High** — **hard-deletes** `role_permissions` (MyISAM, permanent) while detaching users (InnoDB, rolls back). On failure the permissions are gone forever but the role and users are restored |

Small tables, small conversion, and it removes a permanent-data-loss path. Same migration pattern.

**Remaining MyISAM tables after Batch 3 (~29):** the Batch 4 module set (chat, mastery, programs,
prompt levels) plus the 16 tables no transactional workflow writes (see "Not scheduled" above).

**Order from here:** Batch 3 → payroll code defects (`PAYROLL_TRANSACTION_DEFECTS.md`) → Batch 4 →
foreign-key audit (see N2: the main DB has **0 FKs** on the finance tables while a fresh install has 4).

---

# Batch 3 — completion record (Roles & Permissions)

**Migration:** `database/migrations/2026_07_13_000004_convert_rbac_tables_to_innodb.php`
**Duration:** 311 ms (2 tables, 76.8 KB)
**Tables skipped:** none — both existed and were MyISAM.
**Scope check:** the U4 transaction also writes `users` and `user_roles`, but both were already
InnoDB, so neither was touched. `privileges` is InnoDB; `role_privileges` does not exist.

## Before / after

| Table | Engine before | Engine after | Rows before | Rows after | Indexes before | Indexes after |
|---|---|---|---|---|---|---|
| permissions | MyISAM | **InnoDB** | 199 | **199** | 2 | 2 |
| role_permissions | MyISAM | **InnoDB** | 290 | **290** | 3 | 3 |

Unlike Batches 1 and 2 (which were empty), these tables hold **real data** — so the row-count
check was a genuine safety net, not a formality. All 489 rows preserved.

**Index signatures preserved exactly:**
- `permissions`: `PRIMARY (id)` UNIQUE, `permissions_permissionname_unique (permissionName)` UNIQUE
- `role_permissions`: `PRIMARY (id)` UNIQUE, `role_permissions_roleid_permissionid_unique (roleId, permissionId)` UNIQUE, `role_permissions_permissionid_foreign (permissionId)` INDEX

**Preflight findings:** 0 duplicate `(roleId, permissionId)` grants, 0 duplicate `permissionName`
values, 0 NULLs in any indexed column. 110.7 GB free vs ~153 KB transient need.

## The permanent-loss bug, reproduced and then closed

Before converting, the failure was reproduced end-to-end against a MyISAM `role_permissions`:

```
BEFORE the failed role deletion:
  role isDelete      : 0
  user.user_role     : 170 (attached)
  role_permissions   : 3 grants

Transaction FAILED and rolled back: simulated failure after role deletion

AFTER the rollback:
  role isDelete      : 0   -> RESTORED (InnoDB)
  user.user_role     : 170 -> RESTORED (InnoDB)
  role_permissions   : 0 grants -> *** PERMANENTLY LOST ***
```

The role and the user link came back. The permissions did not. The role was left existing with
**zero permissions**, unrecoverably. After the conversion the same flow restores all three grants.

## Workflows now atomic

| # | Workflow | Was | Now |
|---|---|---|---|
| U3 | Role permission matrix save | NOT atomic | **Atomic** |
| U4 | Delete role + detach users | NOT atomic — **permanent data loss** | **Atomic** |

## Tests

`tests/Feature/RolePermissionTransactionTest.php` — **5 passed, 23 assertions**

1. RBAC tables use a transactional engine (precondition; fails loudly otherwise).
2. **Role-deletion failure** restores the role, keeps the user attached, and restores **every**
   permission row — asserted by *value*, not merely by count.
3. **Permission-matrix save failure** leaves the original grants intact — the role is never left
   holding nothing.
4. Successful delete path still commits: role deleted, user link removed, grants removed.
5. Role-deletion protections unchanged (see below).

**The tests fail before the conversion and pass after**, as required: with `role_permissions`
forced back to MyISAM, 4 of the 5 failed; after the migration all 5 pass.

## Protected roles — confirmed unweakened

The engine conversion touches no code or routing, and these guards were pinned by test 5 and
re-verified by the existing suites:

| Protection | Where | Status |
|---|---|---|
| Only a system admin may delete a role | `UseRollController::destroy()` — `PermissionHelper::isSystemAdmin($currentUser)` | **Intact** (pinned by test) |
| Role deletion requires the privilege | `routes/api.php` — `DELETE /user/role/{id}` → `middleware('permission:User_Role_Delete')` | **Intact** (pinned by test) |
| Super-admin user cannot be deleted | `SuperAdminProtectionTest` | **6 passed** |
| Protected `vmadmin@gmail.com` cannot be deleted | `SuperAdminProtectionTest` | **6 passed** |
| Protected account's role cannot be changed | `SuperAdminProtectionTest` | **6 passed** |
| Super-admin bypasses permission middleware | `SuperAdminProtectionTest` | **6 passed** |
| Data scoping / route access | `DataScopeAuthorizationTest` | **14 passed** |

No privilege redesign, no seeded-default changes, no column renames, no foreign keys added.

## Suite

| Suite | Result |
|---|---|
| RolePermissionTransactionTest | **5 passed** |
| SuperAdminProtectionTest | 6 passed |
| DataScopeAuthorizationTest | 14 passed |
| ParentManagementTest | 9 passed |
| FinanceTransactionTest | 5 passed |
| AttendanceCancellationTransactionTest | 5 passed |
| ClassScheduleTransactionTest | 3 passed |
| ClassOccurrenceTest | 11 passed |
| UserAccountFieldsTest | 4 passed |
| **Full suite** | **70 passed, 1 failed** |

The single failure remains the pre-existing `EmailProviderTest` fixture bug (N3) — unchanged by
Batch 3 and not caused by it.

> **Note:** `PrivilegeTest` and `PrivilegeRouteAccessTest` were requested but **do not exist** in
> this repository. RBAC is covered instead by `SuperAdminProtectionTest` (6) and
> `DataScopeAuthorizationTest` (14), both of which pass. Flagging rather than silently substituting.

---

# Remaining non-atomic workflows (all Batch 4)

| # | Workflow | Tables (all MyISAM) | Risk |
|---|---|---|---|
| M1 | Create chat conversation | `conversations`, `conversation_participants`, `chat_messages` | Medium |
| M2 | Mastery workflow save | `mastery_workflows`, `mastery_workflow_phases`, `mastery_maintenance_schedules` | Medium |
| M3 | Clone program | `programs`, `program_targets` | Medium |
| M4 | Prompt level template save | `prompt_level_templates`, `prompt_levels` | Medium |

All four share the "soft-delete the children, re-insert them" pattern — the shape that corrupts
without a working rollback, leaving a parent with **zero** children (phases, targets, levels).
None touch money, attendance or access control, hence Medium rather than High.

**Next, per plan: the payroll code defects** (`PAYROLL_TRANSACTION_DEFECTS.md`). Those tables are
already InnoDB, so they need logic fixes — a missing `rollBack()` in `moveAllPayrollData()`'s catch
(which hard-deletes payroll history), five `rollBack()` calls with no `beginTransaction()`, and
`FaceEmpController` writing four tables with no transaction at all. Nothing there is fixed by an
engine change.

---

# Batch 4 — completion record (Chat / Mastery / Programs / Prompt Levels)

**Migration:** `database/migrations/2026_07_14_000001_convert_module_tables_to_innodb.php`
**Duration:** ~1s (10 tables, 16.0 KB)
**Tables skipped:** none — all 10 existed and were MyISAM.

## Tables converted (all 0 rows, all indexes intact)

| Workflow | Tables | Engine | Indexes |
|---|---|---|---|
| M1 Chat — `ChatController::createConversation()` | `conversations`, `conversation_participants`, `chat_messages` | MyISAM → **InnoDB** | 4, 3, 3 |
| M2 Mastery — `MasteryWorkflowController::store()` | `mastery_workflows`, `mastery_workflow_phases`, `mastery_maintenance_schedules` | MyISAM → **InnoDB** | 1, 2, 2 |
| M3 Programs — `ProgramController::cloneProgram()` | `programs`, `program_targets` | MyISAM → **InnoDB** | 5, 4 |
| M4 Prompt levels — `PromptLevelTemplateController::store()` | `prompt_level_templates`, `prompt_levels` | MyISAM → **InnoDB** | 1, 2 |

Row counts unchanged (all 0). Every index preserved by name, uniqueness and column order —
verified inside the migration, which throws on drift. Preflight: 0 duplicates, 0 NULLs in
indexed columns.

**Deliberately excluded** (MyISAM, but **not written inside any of these four transactions** —
verified against the code, not the audit summary):
`program_folders` (only *read* by cloneProgram), `activity_logs` (written by
`ProgramController::updateStatus()`, which has no transaction), `messages` and `message_reads`
(a different, older messaging module — not touched by `ChatController`).

## `cloneProgram()` — root cause and fix

**Three separate defects, two of them fatal:**

1. **Leaked transaction.** The `if (!$original)` "Program not found" branch `return`ed from
   *inside* an open transaction (`beginTransaction()` was called before the lookup), leaving a
   dangling transaction on the pooled connection.
2. **`catch (Exception)`, not `\Throwable`.** A `TypeError` in the clone loop would skip the
   rollback entirely.
3. **The method had NEVER worked.** It copied `$original->description` — and `programs` has
   **no `description` column** (nor does `program_targets`, which has `notes`). Under PHP 8 that
   is an `Error: Undefined property`, not a warning. It also wrote `promptLevelTemplateId`; the
   real column is `promptTemplateId`. Every clone attempt fatalled before reaching the database.
   Because the failure was an `Error` and the catch only caught `Exception`, it did not even
   produce the intended 500 — it produced an unhandled fatal, *with the transaction still open*.

**Fix:**
- Source lookup and the "not found" return moved **before** the transaction opens.
- Body wrapped in `DB::transaction(function () { ... })` — Laravel rolls back on any `Throwable`
  and rethrows, so no exit path can leave a half-cloned program.
- Outer `catch (\Throwable $e)` returns a 500 with the message; errors are never swallowed.
- Insert column lists corrected against the real schema (`measurementType`, `objective`,
  `baselineNotes`, `tags`, `promptTemplateId`, `notes`).

`cloneProgram()` now functions for the first time — proven by the happy-path test, which
clones a program with 3 targets and asserts all 3 land on the copy.

## Tests

`tests/Feature/ModuleTransactionTest.php` — **7 passed, 34 assertions**

| # | Test | Proves |
|---|---|---|
| 1 | Module tables use a transactional engine | Precondition; fails loudly otherwise |
| 2 | Chat failure rolls back conversation + participants + first message | No conversation with zero participants |
| 3 | Mastery failure leaves original phases intact | The wipe-and-reinsert cannot strand a workflow with zero phases; the maintenance schedule survives too |
| 4 | Program clone failure leaves no partial clone | New program + 1 of 2 targets written, then fail → no orphan program, no orphan targets, original keeps both targets |
| 5 | Prompt level failure restores original levels **and order** | Asserts by value and order, not just count |
| 6 | `cloneProgram()` "not found" leaves no open transaction | `DB::transactionLevel() === 0` after the early return, **and** an independent later write still commits (it would be swallowed by a leaked transaction) |
| 7 | Successful clone commits program and all targets | Happy path — and the first time this feature has ever worked |

Each failure test asserts **inside the open transaction** that the writes really landed before
throwing, so none can pass vacuously.

## Suite

**83 passed, 1 failed** (up from 76/1). The single failure remains the unrelated, pre-existing
`EmailProviderTest` fixture bug (creates an `EmailThread` without the NOT NULL `subject`).

| Suite | Result |
|---|---|
| FinanceTransactionTest | 5 passed |
| AttendanceCancellationTransactionTest | 5 passed |
| RolePermissionTransactionTest | 5 passed |
| PayrollPunchTransactionTest | 6 passed |
| ClassScheduleTransactionTest | 3 passed |
| **ModuleTransactionTest** | **7 passed** |
| ParentManagementTest | 9 passed |
| SuperAdminProtectionTest | 6 passed |
| DataScopeAuthorizationTest | 14 passed |

---

# Remaining MyISAM tables (19) — none is written by any transaction

`absence_reports`, `activity_logs`, `assignments`, `assignment_submissions`, `class_messages`,
`class_requests`, `courses`, `course_materials`, `messages`, `message_reads`, `password_otps`,
`program_folders`, `session_data_records`, `staff_availability`, `student_assessments`,
`student_challenges`, `student_notes`, `student_strengths`, `tutoring_planners`

These were re-checked against every transaction boundary in `app/`: **none of them is written
inside one.** They are therefore not an atomicity risk, and no further batch is required.

Convert them opportunistically for crash-safety and FK-readiness — not for correctness.

---

# Remaining known transaction risks

1. **Foreign keys are missing** on every table converted from MyISAM (`FOLLOWUP_TICKETS.md` T5).
   The main DB has **0 FKs** on the finance tables; a fresh install has 4. This is the largest
   remaining structural gap.
2. **Mail inside transactions** in `EmployeePunchDetailsController::store()`, `storeLocall()`,
   `destroy()` (T2). Atomic, but holds the transaction across SMTP and can email about a punch
   that later rolls back.
3. **Dead punch methods** `fingerprint()` / `testingPunch()` (T1) — unrouted, still carrying the
   old no-transaction shape. Recommend deletion after reference verification.
4. **No DB-level payroll unique key** (T3) — app-level guard added; constraint needs a duplicate
   audit first.
5. **Test DB vs production schema divergence** — `stepup_test_db` was rebuilt fresh (all InnoDB,
   real FKs); `stepup_admin_db` was converted in place (InnoDB, no FKs). Tests therefore run
   against a *stricter* schema than production. That is the safe direction, but it means a test
   pass does not prove production FK behaviour — resolved by T5.