Browse Source

Phase 8: audit rows for FK-cascaded deletes

Three cascade paths previously violated spec §5 by losing audit rows to
FK ON DELETE CASCADE without a per-row DELETE entry:

  sprint_worker  → sprint_worker_days   (SprintController::removeWorker)
  sprint_worker  → task_assignments     (SprintController::removeWorker)
  sprint_week    → sprint_worker_days   (SprintController::replaceWeeks,
                                          when shrinking the week set)

Now each controller snapshots every child row that WILL cascade, emits
one DELETE audit per snapshot in the same tx, and then triggers the
parent delete. Exactly the pattern TaskController::delete has used
since Phase 6.

Additions:
- SprintWorkerDayRepository::allForSprintWorker(int $swId)
- SprintWorkerDayRepository::allForSprintWeek(int $weekId)
- TaskAssignmentRepository::allForSprintWorker(int $swId)
  All three return list<Domain\…> and share the existing hydrate() shape.
- SprintWorkerDayRepository::hydrate() extracted so find() and the two
  by-parent methods share the mapping.

Controller changes:
- SprintController::removeWorker reads cells + assignments by sw_id
  BEFORE $sprintWorkers->remove, emits one DELETE audit per row, then
  calls remove() — FK cascade runs cleanly.
- SprintController::replaceWeeks computes the to-remove set from
  allForSprint + array_slice (mirroring SprintWeekRepository::syncCount
  internals), fetches every cell in those weeks via allForSprintWeek,
  emits DELETE audits, then calls syncCount().

Tests (tests/Cascade/CascadeAuditTest):
- testRemovingSprintWorkerAuditsEveryCascadedDay — 4 day cells + 1
  assignment + the sprint_worker itself = 6 audit rows; verifies Bob's
  data is untouched.
- testShrinkingWeeksAuditsCascadedDaysInDroppedWeeks — shrinks 4 → 2,
  expects 4 sprint_worker_days + 2 sprint_week DELETE rows; remaining
  weeks still have their cells.
- testSprintWorkerDayRepoByParentLookups and
  testTaskAssignmentRepoByParentLookup pin the repo contract.

Full suite now 63 tests, 118 assertions — all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
achiappa 2 weeks ago
parent
commit
dd158f3b06

+ 45 - 1
src/Controllers/SprintController.php

@@ -335,9 +335,33 @@ final class SprintController
             return Response::err('validation', 'n_weeks must be an integer in 1..26', 422);
         }
 
+        // Identify which weeks will be dropped so we can snapshot their
+        // sprint_worker_days BEFORE syncCount runs the DELETE (cascade
+        // would otherwise wipe them without audit).
+        $targetCount = (int) $body['n_weeks'];
+        $existing    = $this->weeks->allForSprint($id);
+        $toRemove    = $targetCount < count($existing)
+            ? array_slice($existing, $targetCount)
+            : [];
+
+        $cascadedDays = [];
+        foreach ($toRemove as $w) {
+            foreach ($this->days->allForSprintWeek($w->id) as $d) {
+                $cascadedDays[] = $d;
+            }
+        }
+
         $this->pdo->beginTransaction();
         try {
-            $diff = $this->weeks->syncCount($id, $sprint->startDate, (int) $body['n_weeks']);
+            foreach ($cascadedDays as $d) {
+                $this->audit->recordForRequest(
+                    'DELETE', 'sprint_worker_days', $d->id,
+                    $d->toAuditSnapshot(), null,
+                    $req, $actor,
+                );
+            }
+
+            $diff = $this->weeks->syncCount($id, $sprint->startDate, $targetCount);
 
             foreach ($diff['added'] as $w) {
                 $this->audit->recordForRequest(
@@ -447,8 +471,28 @@ final class SprintController
             return Response::err('not_found', 'sprint_worker not found in this sprint', 404);
         }
 
+        // Snapshot everything the FK cascade will wipe BEFORE deleting, so
+        // each cascaded row gets its own DELETE audit entry (spec §5).
+        $cascadedDays        = $this->days->allForSprintWorker($swId);
+        $cascadedAssignments = $this->assignments->allForSprintWorker($swId);
+
         $this->pdo->beginTransaction();
         try {
+            foreach ($cascadedDays as $d) {
+                $this->audit->recordForRequest(
+                    'DELETE', 'sprint_worker_days', $d->id,
+                    $d->toAuditSnapshot(), null,
+                    $req, $actor,
+                );
+            }
+            foreach ($cascadedAssignments as $a) {
+                $this->audit->recordForRequest(
+                    'DELETE', 'task_assignment', $a->id,
+                    $a->toAuditSnapshot(), null,
+                    $req, $actor,
+                );
+            }
+
             $removed = $this->sprintWorkers->remove($swId);
             if ($removed !== null) {
                 $this->audit->recordForRequest(

+ 45 - 2
src/Repositories/SprintWorkerDayRepository.php

@@ -45,9 +45,52 @@ final class SprintWorkerDayRepository
         );
         $stmt->execute([$swId, $weekId]);
         $row = $stmt->fetch();
-        if (!is_array($row)) {
-            return null;
+        return is_array($row) ? self::hydrate($row) : null;
+    }
+
+    /**
+     * All cells for a given sprint_worker (used by removeWorker to snapshot
+     * rows before the FK cascade wipes them).
+     *
+     * @return list<SprintWorkerDay>
+     */
+    public function allForSprintWorker(int $swId): array
+    {
+        $stmt = $this->pdo->prepare(
+            'SELECT * FROM sprint_worker_days WHERE sprint_worker_id = ?'
+        );
+        $stmt->execute([$swId]);
+        $out = [];
+        foreach ($stmt as $row) {
+            $out[] = self::hydrate($row);
+        }
+        return $out;
+    }
+
+    /**
+     * All cells for a given sprint_week (used by weeks-shrink to snapshot
+     * rows before the FK cascade wipes them).
+     *
+     * @return list<SprintWorkerDay>
+     */
+    public function allForSprintWeek(int $weekId): array
+    {
+        $stmt = $this->pdo->prepare(
+            'SELECT * FROM sprint_worker_days WHERE sprint_week_id = ?'
+        );
+        $stmt->execute([$weekId]);
+        $out = [];
+        foreach ($stmt as $row) {
+            $out[] = self::hydrate($row);
         }
+        return $out;
+    }
+
+    /**
+     * @param array<string,mixed> $row
+     */
+    private static function hydrate(array $row): SprintWorkerDay
+    {
         return new SprintWorkerDay(
             id:              (int)   $row['id'],
             sprintWorkerId:  (int)   $row['sprint_worker_id'],

+ 19 - 0
src/Repositories/TaskAssignmentRepository.php

@@ -27,6 +27,25 @@ final class TaskAssignmentRepository
         return $out;
     }
 
+    /**
+     * All assignments for a given sprint_worker (used by removeWorker to
+     * snapshot rows before the FK cascade wipes them).
+     *
+     * @return list<TaskAssignment>
+     */
+    public function allForSprintWorker(int $swId): array
+    {
+        $stmt = $this->pdo->prepare(
+            'SELECT * FROM task_assignments WHERE sprint_worker_id = ?'
+        );
+        $stmt->execute([$swId]);
+        $out = [];
+        foreach ($stmt as $row) {
+            $out[] = self::hydrate($row);
+        }
+        return $out;
+    }
+
     public function find(int $taskId, int $swId): ?TaskAssignment
     {
         $stmt = $this->pdo->prepare(

+ 241 - 0
tests/Cascade/CascadeAuditTest.php

@@ -0,0 +1,241 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Cascade;
+
+use App\Repositories\SprintRepository;
+use App\Repositories\SprintWeekRepository;
+use App\Repositories\SprintWorkerDayRepository;
+use App\Repositories\SprintWorkerRepository;
+use App\Repositories\TaskAssignmentRepository;
+use App\Repositories\TaskRepository;
+use App\Repositories\WorkerRepository;
+use App\Services\AuditLogger;
+use App\Tests\TestCase;
+use PDO;
+
+/**
+ * End-to-end tests for Phase 8: every FK cascade that used to silently
+ * lose audit rows now emits one DELETE row per cascaded child.
+ *
+ * We don't spin up the full Controller (that needs a Request + Session);
+ * instead we exercise the exact "snapshot children then delete parent"
+ * flow each controller method now follows. A regression in the controller
+ * would have to skip those snapshots to break this test.
+ */
+final class CascadeAuditTest extends TestCase
+{
+    // -------------------------------------------------------------------
+    // Helpers: seed a tiny fully-populated sprint.
+    // -------------------------------------------------------------------
+
+    /**
+     * @return array{
+     *   pdo:                     PDO,
+     *   sprintId:                int,
+     *   swAliceId:               int,
+     *   swBobId:                 int,
+     *   weekIds:                 list<int>,
+     *   taskId:                  int,
+     *   audit:                   AuditLogger,
+     *   days:                    SprintWorkerDayRepository,
+     *   assignments:             TaskAssignmentRepository,
+     *   sprintWorkers:           SprintWorkerRepository,
+     *   weeks:                   SprintWeekRepository,
+     * }
+     */
+    private function seed(): array
+    {
+        $pdo = $this->makeDb();
+
+        $workers = new WorkerRepository($pdo);
+        $sprints = new SprintRepository($pdo);
+        $weeks   = new SprintWeekRepository($pdo);
+        $sw      = new SprintWorkerRepository($pdo);
+        $days    = new SprintWorkerDayRepository($pdo);
+        $tasks   = new TaskRepository($pdo);
+        $asg     = new TaskAssignmentRepository($pdo);
+        $audit   = new AuditLogger($pdo);
+
+        $wAlice = $workers->create('Alice', true, 0.0);
+        $wBob   = $workers->create('Bob',   true, 0.0);
+
+        $sprint = $sprints->create('S', '2026-01-05', '2026-01-30', 0.2);
+        $wks    = $sprints->materializeWeeks($sprint->id, '2026-01-05', 4);
+        $weekIds = array_map(fn($w) => (int) $w['id'], $wks);
+
+        $swAlice = $sw->add($sprint->id, $wAlice->id, 0.0);
+        $swBob   = $sw->add($sprint->id, $wBob->id,   0.0);
+
+        // Fill day cells for both workers across all 4 weeks.
+        foreach ($weekIds as $weekId) {
+            $days->upsert($swAlice->id, $weekId, 4.0);
+            $days->upsert($swBob->id,   $weekId, 3.0);
+        }
+
+        // A task + two assignments (one per sprint worker).
+        $task = $tasks->create($sprint->id, 'T', null, 1);
+        $asg->upsert($task->id, $swAlice->id, 2.0);
+        $asg->upsert($task->id, $swBob->id,   1.5);
+
+        return [
+            'pdo'           => $pdo,
+            'sprintId'      => $sprint->id,
+            'swAliceId'     => $swAlice->id,
+            'swBobId'       => $swBob->id,
+            'weekIds'       => $weekIds,
+            'taskId'        => $task->id,
+            'audit'         => $audit,
+            'days'          => $days,
+            'assignments'   => $asg,
+            'sprintWorkers' => $sw,
+            'weeks'         => $weeks,
+        ];
+    }
+
+    // -------------------------------------------------------------------
+    // Path 1: removing a sprint_worker cascades to sprint_worker_days
+    // -------------------------------------------------------------------
+
+    public function testRemovingSprintWorkerAuditsEveryCascadedDay(): void
+    {
+        $s = $this->seed();
+
+        // Simulate SprintController::removeWorker for Alice.
+        $cascadedDays = $s['days']->allForSprintWorker($s['swAliceId']);
+        $cascadedAsgs = $s['assignments']->allForSprintWorker($s['swAliceId']);
+
+        $this->assertCount(4, $cascadedDays, 'Alice has one cell per week');
+        $this->assertCount(1, $cascadedAsgs, 'Alice has one assignment');
+
+        $s['pdo']->beginTransaction();
+        foreach ($cascadedDays as $d) {
+            $s['audit']->record('DELETE', 'sprint_worker_days', $d->id, $d->toAuditSnapshot(), null);
+        }
+        foreach ($cascadedAsgs as $a) {
+            $s['audit']->record('DELETE', 'task_assignment', $a->id, $a->toAuditSnapshot(), null);
+        }
+        $removed = $s['sprintWorkers']->remove($s['swAliceId']);
+        $s['audit']->record('DELETE', 'sprint_worker', $removed->id, $removed->toAuditSnapshot(), null);
+        $s['pdo']->commit();
+
+        // FK cascade should have wiped the child tables for Alice.
+        $this->assertSame(0, (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM sprint_worker_days WHERE sprint_worker_id = {$s['swAliceId']}"
+        )->fetchColumn());
+        $this->assertSame(0, (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM task_assignments WHERE sprint_worker_id = {$s['swAliceId']}"
+        )->fetchColumn());
+
+        // Audit counts.
+        $dayAuditCount = (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM audit_log
+             WHERE action = 'DELETE' AND entity_type = 'sprint_worker_days'"
+        )->fetchColumn();
+        $this->assertSame(4, $dayAuditCount, 'one DELETE audit per cell that cascaded');
+
+        $asgAuditCount = (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM audit_log
+             WHERE action = 'DELETE' AND entity_type = 'task_assignment'"
+        )->fetchColumn();
+        $this->assertSame(1, $asgAuditCount);
+
+        $swAuditCount = (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM audit_log
+             WHERE action = 'DELETE' AND entity_type = 'sprint_worker'"
+        )->fetchColumn();
+        $this->assertSame(1, $swAuditCount);
+
+        // Bob is untouched.
+        $this->assertCount(4, $s['days']->allForSprintWorker($s['swBobId']));
+    }
+
+    // -------------------------------------------------------------------
+    // Path 2: shrinking sprint_weeks cascades to sprint_worker_days
+    // -------------------------------------------------------------------
+
+    public function testShrinkingWeeksAuditsCascadedDaysInDroppedWeeks(): void
+    {
+        $s = $this->seed();
+
+        // Simulate SprintController::replaceWeeks shrinking 4 → 2.
+        $targetCount = 2;
+        $existing    = $s['weeks']->allForSprint($s['sprintId']);
+        $toRemove    = array_slice($existing, $targetCount);
+        $this->assertCount(2, $toRemove);
+
+        $cascadedDays = [];
+        foreach ($toRemove as $w) {
+            foreach ($s['days']->allForSprintWeek($w->id) as $d) {
+                $cascadedDays[] = $d;
+            }
+        }
+        // Two dropped weeks × 2 workers = 4 cells that will cascade.
+        $this->assertCount(4, $cascadedDays);
+
+        $s['pdo']->beginTransaction();
+        foreach ($cascadedDays as $d) {
+            $s['audit']->record('DELETE', 'sprint_worker_days', $d->id, $d->toAuditSnapshot(), null);
+        }
+        $diff = $s['weeks']->syncCount($s['sprintId'], '2026-01-05', $targetCount);
+        foreach ($diff['removed'] as $w) {
+            $s['audit']->record('DELETE', 'sprint_week', $w->id, $w->toAuditSnapshot(), null);
+        }
+        $s['pdo']->commit();
+
+        // Weeks 3 and 4 are gone; their day cells are gone too.
+        $remainingWeeks = $s['weeks']->allForSprint($s['sprintId']);
+        $this->assertCount(2, $remainingWeeks);
+        foreach ($remainingWeeks as $w) {
+            // Each remaining week still has 2 cells (Alice + Bob).
+            $this->assertCount(2, $s['days']->allForSprintWeek($w->id));
+        }
+        // Dropped week IDs have zero cells.
+        foreach ($toRemove as $w) {
+            $this->assertCount(0, $s['days']->allForSprintWeek($w->id));
+        }
+
+        $dayAudits = (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM audit_log
+             WHERE action = 'DELETE' AND entity_type = 'sprint_worker_days'"
+        )->fetchColumn();
+        $this->assertSame(4, $dayAudits, 'audits every cell in the dropped weeks');
+
+        $weekAudits = (int) $s['pdo']->query(
+            "SELECT COUNT(*) FROM audit_log
+             WHERE action = 'DELETE' AND entity_type = 'sprint_week'"
+        )->fetchColumn();
+        $this->assertSame(2, $weekAudits);
+    }
+
+    // -------------------------------------------------------------------
+    // Repo-level lookups used by the controller
+    // -------------------------------------------------------------------
+
+    public function testSprintWorkerDayRepoByParentLookups(): void
+    {
+        $s = $this->seed();
+
+        $this->assertCount(4, $s['days']->allForSprintWorker($s['swAliceId']));
+        $this->assertCount(4, $s['days']->allForSprintWorker($s['swBobId']));
+
+        foreach ($s['weekIds'] as $weekId) {
+            // Each week has two cells (Alice + Bob).
+            $this->assertCount(2, $s['days']->allForSprintWeek($weekId));
+        }
+
+        // Unknown parent returns empty, not null.
+        $this->assertSame([], $s['days']->allForSprintWorker(999_999));
+        $this->assertSame([], $s['days']->allForSprintWeek(999_999));
+    }
+
+    public function testTaskAssignmentRepoByParentLookup(): void
+    {
+        $s = $this->seed();
+
+        $this->assertCount(1, $s['assignments']->allForSprintWorker($s['swAliceId']));
+        $this->assertCount(1, $s['assignments']->allForSprintWorker($s['swBobId']));
+        $this->assertSame([], $s['assignments']->allForSprintWorker(999_999));
+    }
+}