# Evaluation — Attempt 1

## Overall Verdict: MAJOR REVISION

## Overall Assessment
The interface extends the forest/lemon/glass admin language convincingly: the two entry actions are clear, the import modal explains the workflow well, and the camera reads as a compact identity booth. The production implementation cannot yet pass because the core importer does not resolve the workbook's actual first worksheet, does not bound or harden XLSX internals, and lacks the required parser/archive regression suite; these are data-integrity and denial-of-service risks in the feature's primary path.

## Scores
| Criterion | Score | Status | Weight | Notes |
|-----------|-------|--------|--------|-------|
| Design Quality | 2/3 | PASS | HIGH | The guided three-step intake, glass secondary action, lemon primary action, and photo booth form one coherent extension of the existing admin system. |
| Originality | 2/3 | PASS | HIGH | The numbered intake cards and compact camera booth are deliberate, custom decisions rather than stock Bootstrap presentation. |
| Craft | 2/3 | PASS | MEDIUM | Modal height, internal scrolling, 44px controls, mobile stacking, labels, live regions, focus trapping, and modal-return focus are handled cleanly. |
| Functionality | 0/3 | FAIL | MEDIUM | The standard happy path is present, but the promised safe first-sheet XLSX importer is not implemented as specified and untrusted XLSX XML is inflated and parsed without archive/XML bounds. Required regression tests are absent. This is a blocker for a production bulk-data tool. |

## What's Working Well
- `member_view.php` exposes both actions only when `members.create` is available, while both import endpoints independently require staff authentication/permission and the import POST verifies CSRF.
- The import copy clearly communicates all-or-nothing behavior, supported formats, size limits, photo matching, the 1,000-row cap, and silent import behavior.
- CSV parsing handles BOM, UTF-8 validation, and comma/semicolon/tab detection. Headers are normalized, duplicate headings are rejected, and all named template columns are required.
- Row validation is completed before transaction start. The transaction and written-file list provide a sound rollback basis, and errors are bounded to 50 before session rendering.
- Photo content is MIME-inspected and stored under random filenames. The archive helper rejects obvious traversal/absolute names, duplicate basenames, oversized photos, excessive entry counts, and high compression ratios.
- Camera access only begins after an explicit `Start camera` action. Capture uses canvas → JPEG Blob → File/DataTransfer so it continues through the existing image backend. Permission/API failure returns to upload mode with inline status and a toast.
- Active tracks are stopped on capture, mode change, modal close, form submit, and page unload. The same partial gives the standalone add page the same camera behavior.
- PHP lint passes for the changed files, and a read-only mapping smoke test succeeded without inserting a member.

## Issues Found

### Issue 1: The importer does not load the workbook's actual first sheet
- **What**: `memberImportXlsx()` hardcodes `xl/worksheets/sheet1.xml`. XLSX sheet order and part filenames are defined by `xl/workbook.xml` plus `xl/_rels/workbook.xml.rels`; the first visible sheet may point to `sheet2.xml` or a custom worksheet part.
- **Where**: `app/member_import.php`, `memberImportXlsx()`.
- **Why it matters**: A valid workbook can be rejected or, worse, the importer can read a different worksheet than the one the administrator sees first. That is a core data-integrity failure and directly misses the brief's workbook-relationship requirement.
- **Suggested fix**: Parse `workbook.xml` to obtain the first sheet relationship ID, resolve it through `workbook.xml.rels`, normalize and validate the internal target under `xl/worksheets/`, then read that exact part. Reject external relationships and any target that escapes the expected XLSX namespace.

### Issue 2: XLSX internals are not protected against decompression/XML abuse
- **What**: The uploaded XLSX is capped by compressed upload size only. `sharedStrings.xml` and worksheet XML are loaded completely using `getFromName()` and `simplexml_load_string()` without per-entry uncompressed limits, compression-ratio limits, overall uncompressed limits, row/cell limits during parsing, or explicit safe XML parsing controls.
- **Where**: `app/member_import.php`, `memberImportXlsx()`.
- **Why it matters**: XLSX is itself an untrusted ZIP. A small compressed workbook can expand into very large XML and exhaust PHP memory before the 1,000-row application cap is reached.
- **Suggested fix**: Inspect XLSX entry metadata before reading, cap entry count, individual and total uncompressed XML bytes, and compression ratios. Reject encrypted entries and external relationships. Use `XMLReader` with network/entity loading disabled and stop once structural/cell/row limits are exceeded.

### Issue 3: Bundle validation is incomplete and temporary-file cleanup is not exception-safe
- **What**: `memberImportBundle()` accepts `members.csv/xlsx` in any nested directory despite the UI promising the bundle layout, does not check the embedded sheet's uncompressed size/ratio before loading it, does not check `getFromName()`, `tempnam()`, or `file_put_contents()` failures, and only unlinks the temporary file on the success path.
- **Where**: `app/member_import.php`, `memberImportBundle()`.
- **Why it matters**: A malformed or compressed bundle can bypass the intended 10 MB spreadsheet bound, leak temporary files after parse exceptions, or produce ambiguous errors.
- **Suggested fix**: Allow exactly root `members.*` (or document and intentionally support one layout), inspect the selected entry before reading, enforce the 10 MB spreadsheet and ratio limits, reject encryption, verify every I/O return value, and close/unlink in `finally`.

### Issue 4: Archive policy silently ignores unsupported/encrypted entries
- **What**: Entries whose bytes cannot be read are silently skipped, and non-image files are silently ignored. Encryption flags are never inspected. Directory scope is not limited to archive root or `photos/`.
- **Where**: `memberImportPhotosFromZip()`.
- **Why it matters**: The brief requires rejecting encrypted/unsupported files and constraining the documented archive shape. Silent skipping makes operator mistakes hard to diagnose and weakens the safety contract.
- **Suggested fix**: Inspect ZipArchive encryption/method metadata where supported, fail when any candidate entry cannot be read, accept only image files in root or `photos/` plus the one bundle spreadsheet, and provide a precise safe error for unsupported content.

### Issue 5: Required regression verification is missing
- **What**: No unit-like fixtures/tests exist for relationship-based first-sheet selection, shared strings, inline strings, missing cells, Excel dates, invalid/duplicate headers, traversal, duplicate photo basenames, missing photos, or camera fallback/state cleanup.
- **Where**: Repository test coverage for `app/member_import.php` and `assets/portal/js/admin.js`.
- **Why it matters**: This parser processes large batches all-or-nothing. Without repeatable safety tests, subtle changes can cause silent column shifts, wrong dates, or archive regressions.
- **Suggested fix**: Add a standalone read-only PHP test script that generates tiny CSV/XLSX/ZIP fixtures in a unique temp directory, asserts each required case, cleans in `finally`, and never loads the database bootstrap. Add a static/browser camera test using a stubbed rejected `getUserMedia`, plus track-stop assertions; do not activate hardware.

### Issue 6: Camera preview object URLs and captured state are not fully reset
- **What**: Each capture creates an object URL without revoking it. Closing the modal or switching modes stops tracks but retains the Blob, preview URL, disabled `Use this photo` state, and previous UI state.
- **Where**: `assets/portal/js/admin.js`, photo-booth controller.
- **Why it matters**: Repeated captures/modal sessions leak memory and can reopen in a confusing stale state.
- **Suggested fix**: Track the active object URL, call `URL.revokeObjectURL()` before replacing/resetting it, and centralize a `resetCamera()` routine invoked on modal close and form reset. Preserve an already selected input only when that behavior is explicit.

## Priority Fixes for Next Attempt
1. Replace the hardcoded XLSX worksheet path with safe workbook relationship resolution and bounded streaming XML parsing.
2. Harden both ZIP paths—including embedded sheet limits, encryption/unsupported-entry rejection, strict layout, and exception-safe temp cleanup.
3. Add the required generated-fixture parser/ZIP tests and camera fallback/cleanup test, then fix camera URL/state reset.

## Should the next attempt REFINE or PIVOT?
REFINE. The visual direction and overall interaction model are sound; the required work is concentrated in parser/archive hardening, deterministic tests, and camera lifecycle cleanup rather than a redesign.
