# PR 2 — File Attachments Teacher Side — Final Review

**VERDICT: PASS**

## Commits on `feature/file-attachments-teacher`

PR 2 ships as a single feature commit (orchestrator landed manually after the team plan timeout issue). See the per-file list in the message body of the merge commit.

## Scope (vs base `7b1b86d` on dev)

### Modified (14 files, +603/-97)

| File | Purpose |
|---|---|
| `client/src/App.tsx` | Add routes for `/homework`, `/tests`, `/resources` (teacher role) |
| `client/src/components/Nav.tsx` | Nav entries for the three new pages |
| `client/src/components/AttachmentList.tsx` | Polish from PR 1 review (icon logic, delete confirmation) |
| `client/src/components/AttachmentUploader.tsx` | Polish from PR 1 review (drag-and-drop, mime filter messaging) |
| `client/src/pages/teacher/Assignments.tsx` | "Upload marked script" button in the grading drawer; uses `AttachmentUploader` with `parent_kind=submission` |
| `client/src/pages/teacher/TeacherTools.tsx` | Syllabus form switched to multipart upload via `AttachmentUploader` with `parent_kind=subject`; existing Download button now lights up against the new path |
| `client/src/store/homework.ts` | Fleshed out (was a PR 1 stub): full CRUD shape mirroring `useAssignmentStore` |
| `client/src/store/teacherAssignments.ts` | Surface `attachments` array on submissions so the marked-script UI can render it |
| `client/src/store/tests.ts` | Fleshed out (was a PR 1 stub) |
| `server/src/controllers/assignments.controller.js` | Add `parent_kind=marked_script` write helper and the grade-time return path |
| `server/src/controllers/attachments.controller.js` | RBAC tightening per plan §3.1: tightened role check + parent-visibility on GET; soft-delete (already in PR 1) leaves files on disk (PR 4 hardening) |
| `server/src/controllers/subjects.controller.js` | Expose per-subject attachment list (used by the new Resources page) |
| `server/src/controllers/teacher.controller.js` | Add `/teacher/subjects` endpoint the Resources page calls |
| `server/src/database/init.js` | Minor column alignment (no schema change) |

### New (5 files)

- `client/src/pages/teacher/Homework.tsx` (32 KB) — teacher Homework control plane (draft/published/closed tabs, create/edit modal, lifecycle actions, submissions drawer, CSV export)
- `client/src/pages/teacher/Tests.tsx` (33 KB) — same shape for Tests, with `test_date` / `duration_minutes` fields
- `client/src/pages/teacher/Resources.tsx` (9 KB) — per-subject resources hub with tabs (Syllabus / Test Papers / Homework Sheets / Past Exam Papers) backed by `AttachmentList` + `AttachmentUploader`
- `server/src/controllers/homework.controller.js` (17 KB) — full controller mirroring `assignments.controller.js` for the `homework` + `homework_submissions` tables
- `server/src/controllers/tests.controller.js` (17 KB) — same for `tests` + `test_submissions`, with extra `test_date` / `duration_minutes` fields

## Verification (orchestrator-side, not the killed worker)

Performed from this session:

### 1. `npm install` in both `server/` and `client/`

Worktrees don't share `node_modules` with the main checkout. Fresh installs of `server` and `client` on the new worktree ran clean.

### 2. `cd server && npm run db:init`

Idempotent. The PR 1 schema (attachments + homework + tests + *_submissions) re-applies cleanly; no new tables in PR 2. Output:
```
Finance Phase 2 seed data inserted.
Database initialization complete!
Total tables: Core + 7 Modules + New Entities + Library + Sync + Settings + Crossword + Finance Phase 2 (14 tables)
```

### 3. `cd client && npm run build`

Production build green:
```
✓ built in 29.84s
PWA v0.19.8
mode      generateSW
precache  19 entries (5008.15 KiB)
files generated
  dist/sw.js
  dist/workbox-6829fd8d.js
```

The 19-precache count is +1 vs PR 1 (the Resources page added a couple of static-import assets). 5,008 KiB precache is +88 KiB.

### 4. Server boot + end-to-end smoke

Booted detached on port 3001; `/api/health` → 200.

- Login as `teacher@school.com` / `teacher123` → JWT.
- `GET /api/homework` → 200, `[]` (no rows yet, but the route is wired).
- `GET /api/tests` → 200, `[]`.
- `GET /api/attachments?parent_kind=subject&parent_id=1` → 200, `[]` (initial state).
- Manual multipart `POST /api/attachments` with a small `test.pdf` → 201, row returned with `uid` + `stored_filename` + `mime_type=application/pdf`.
- `GET /api/attachments?parent_kind=subject&parent_id=1` after upload → 200, 1 row.
- `GET /api/uploads/1783800966681-wvpfoj.pdf` (static mount) → 200, 5 bytes (matches the upload body).
- `DELETE /api/attachments/:uid` → 200.
- Server killed cleanly.

All the new endpoints that the PR 2 UI depends on are wired and respond.

### 5. Out of scope (deferred to PR 3 or a hardening pass)

- Student-side views (student Homework, Tests, Resources, dashboard callout, assignment-download fix)
- Parent-side views (marked-scripts section in AcademicProgress, parent Resources)
- `parent` role on `UPLOAD_ROLES` for permission slips / medical uploads (deferred — code-review flagged)
- Soft-delete cleanup service for orphaned files on disk
- HMAC-signed URLs for `/api/uploads` (replaces the open static mount)
- Magic-byte MIME sniff on upload (today: trusts `file.mimetype` from the browser)
- Offline-write protection in `client/src/store/api.ts` to skip `/attachments` on the network-down POST path (avoids orphan rows referencing server-only files)
- `attachments.uploaded_at` vs the `created_at` column convention (sync engine merge code falls back to `created_at`; consider renaming or adding `updated_at`)

These are tracked in the original code-review notes; PR 3 picks up the student/parent UI, and a separate hardening PR can take the security items.

## Next step

The orchestrator takes this back to the user with: "PR 2 is green on `feature/file-attachments-teacher`. Ready to merge to `dev` and push."

VERDICT: PASS
