Skip to content

feat(review): page, decide and edit stored upload rows, insert-only (M4a, #108) - #142

Merged
amar-python merged 3 commits into
mainfrom
feat/108-m4a-review-api
Oct 29, 2026
Merged

amar-python merged 3 commits into
mainfrom
feat/108-m4a-review-api

Conversation

@amar-python

@amar-python amar-python commented Oct 28, 2026 •

Copy link
Copy Markdown
Owner

Refs #108. This is part M4a of ART Milestone 4, the API and database for review. The product owner's 28 Oct decisions are on #108:

  1. Review is recorded now and applied in M6.
  2. reviewer and tenant_admin review.
  3. A reason is required.

What it does

  • Migration 0008 (per tenant):
    • uploads gains columns and column_types, the names and types each upload used. The all-rows-failed result now returns them too, so failed uploads can be reviewed.
    • New insert-only row_decisions and cell_edits. Triggers reject UPDATE, DELETE and TRUNCATE, and a CHECK rejects a blank reason.
    • Both tables are added to the runtime role's read-and-insert list (M3d).
  • GET /api/csv/uploads/{id}/rows?offset=&limit= (read_rows) returns:
    • the upload's columns and types, and its total row count;
    • the counts of excluded and edited rows;
    • and for each row on the page (up to 500): its original and current values, which columns were edited, the decision with its reason, and its validation errors, recomputed from the stored types.
  • PUT …/rows/{n}/decision with {decision, reason} records an include or exclude. PUT …/rows/{n}/cells/{column} with {value, reason} records an edit. {column} takes a name or a 0-based index. Both are review_rows (reviewer, tenant_admin), and each also writes an audit row with before and after values. This is the first use of the M3a columns.
  • GET …/rows/{n}/history (read_rows) lists a row's decisions and edits, oldest first.
  • Refusals:
    • a blank reason, an unknown decision, or an edit that doesn't change the value gives 422;
    • an unknown upload, row or column gives 404.

Second commit: CORS

The API now allows PUT in CORS. Without it, the browser's preflight for the review endpoints is refused, as M4b's end-to-end check showed. A new unit test covers it and fails without the change.

Third commit: test cases (requested by the product owner)

docs/test-cases/M4-review.md sets out 17 positive and 16 negative cases (P01–P17, N01–N16). Each gives its requirement, a manual PowerShell command where one is practical, the expected result, and the automated test that covers it. The new tests/test_review_cases.py holds the 18 cases that weren't covered yet; test_runtime_role.py adds P15 and N15.

  • Local CI replica: 354 collected, 354 passed, 0 failed, 0 skipped.
  • Manual commands: all 23 run in PowerShell 7 against a local API, each giving the expected result. They haven't been run on Windows PowerShell 5.1 yet.
  • Mutation checks: each of these made its case fail:
    • dropping the review tables' guard triggers fails N13, with 4 sub-cases;
    • dropping the database's blank-reason CHECK fails N14, with 2 sub-cases;
    • storing reasons untrimmed fails P10.

Not in this PR

  • The review screen and its e2e test (M4b).
  • Filtering pages to invalid, excluded or edited rows. It would mean validating the whole upload on every request. I'll add it if M4b needs it.
  • T&E-mode uploads have no stored types. Their values are reviewed as text, with no type validation.

Verification

Check Result
Local CI integration replica (PostgreSQL 16) 335 collected, 335 passed, 0 failed, 0 skipped
Evals 26 of 26
make lint Clean
make health 88 passed
New tests/test_review.py (6 tests) Paging and per-row validation. Rows beyond the 200-error cap are still validated, here row 250 of 250. Decisions record actor and reason, and the latest wins. Edits are versions over the unchanged original, are revalidated, and list their history. The imported table is unchanged (decision 1). All the refusals. Both kinds of change are audited with before and after values, the tables are insert-only, and the chain stays intact
tests/test_roles.py Every role against review_rows: reviewer and tenant_admin allowed, uploader and auditor refused (403)

I broke the code three times to check that the tests catch it. Each time the expected test failed:

What I removed or weakened Test that failed
The oldest decision wins instead of the latest The decisions test
The original value validated instead of the edited one The edits test
uploader allowed to review The role-matrix docs test

VCRM:

  • BR-27 is now ✅. API and DB tests are what its row asks for.
  • BR-25 and BR-26 stay ⚠️ until the M4b screen and its e2e test.
  • The headline is now 21 of 38.

Generated by Claude Code

…M4a, #108)

Migration 0008 stores each upload's columns and types, and adds
insert-only row_decisions and cell_edits per tenant. The latest entry
wins; earlier ones are the history.

- GET /api/csv/uploads/{id}/rows pages through every stored row, with
  original and current values, validation errors and the decision.
- PUT .../rows/{n}/decision and PUT .../rows/{n}/cells/{column} record
  a change with a required reason (reviewer, tenant_admin). Each change
  is also audited with before and after values.
- GET .../rows/{n}/history lists a row's decisions and edits.

Review is recorded only; M6 applies it (product owner, 28 Oct).
VCRM: BR-27 verified; BR-25 and BR-26 partial until the screen (M4b).
CI replica 335/0/0, evals 26/26, lint and health clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB
)

Found by the M4b end-to-end check: the browser's preflight for
PUT /api/csv/uploads/{id}/rows/{n}/decision was refused (400), because
CORS allowed only GET, POST and DELETE. New unit test covers the
preflight; it fails without PUT.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB
docs/test-cases/M4-review.md lists 17 positive and 16 negative cases
(P01-P17, N01-N16), each with its requirement, a manual PowerShell
command where one is practical, the expected result and its automated
test. The product owner asked for these to verify the outcomes.

New tests/test_review_cases.py adds the 18 cases not already covered:
paging bounds, ragged rows, failed uploads, bad edits, column index and
empty values, trimmed reasons, signed-in actors, viewing by every role,
out-of-range rows, unknown uploads on every route, uploader and auditor
refused, invalid session, cross-tenant isolation, bad bodies, the
review tables refusing UPDATE/DELETE/TRUNCATE and blank reasons at
database level, and non-numeric ids. test_runtime_role.py now also
covers review changes through, and refused for, the runtime role.

CI replica 354/0/0, evals 26/26, lint clean. The 23 manual commands
were run in PowerShell 7 against a local API: 23 of 23 as expected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB
@amar-python
amar-python merged commit 5a3df99 into main Oct 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants