From 3b0a744a66b2d1ff0e2a7224312f999ccd8b9bb2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 14:31:03 +1100 Subject: [PATCH 1/3] feat(review): page, decide and edit stored upload rows, insert-only (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 Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB --- alembic/versions/0008_review.py | 69 ++++++++++++ api/auth.py | 1 + api/db.py | 3 +- api/main.py | 3 +- api/routers/review_routes.py | 56 ++++++++++ api/services/audit.py | 2 +- api/services/dynamic_loader.py | 3 +- api/services/originals.py | 6 +- api/services/review.py | 188 ++++++++++++++++++++++++++++++++ docs/ROLES.md | 3 +- docs/VCRM.md | 14 +-- tests/test_api_hardening.py | 2 +- tests/test_migrations.py | 2 +- tests/test_review.py | 128 ++++++++++++++++++++++ tests/test_roles.py | 11 +- tests/test_tenant_isolation.py | 2 +- 16 files changed, 471 insertions(+), 22 deletions(-) create mode 100644 alembic/versions/0008_review.py create mode 100644 api/routers/review_routes.py create mode 100644 api/services/review.py create mode 100644 tests/test_review.py diff --git a/alembic/versions/0008_review.py b/alembic/versions/0008_review.py new file mode 100644 index 0000000..115f5dd --- /dev/null +++ b/alembic/versions/0008_review.py @@ -0,0 +1,69 @@ +"""Review of stored uploads: row decisions and versioned cell edits (M4a, #108; BR-25, BR-26, BR-27). + +Decided 28 Oct: review is recorded now and applied by M6's staged import; the +imported table doesn't change. A reason is required. + +- ``uploads`` gains ``columns`` and ``column_types``: the sanitised names and + types the upload used, so every stored row can be validated. +- ``row_decisions``: include or exclude a row, with actor, reason and time. +- ``cell_edits``: one row per edit, with old and new value, actor, reason and + time. The original stays in ``source_rows``. + +Both new tables are insert-only (the latest entry wins; earlier ones are the +history), with the same guard as 0006 and 0007. Runs per tenant. + +Revision ID: 0008 +Revises: 0007 +Create Date: 2026-10-28 +""" + +from alembic import op + +from api.config import settings + +revision = "0008" +down_revision = "0007" +branch_labels = None +depends_on = None + +_TABLES = ("row_decisions", "cell_edits") + + +def _schema() -> str: + return op.get_bind().dialect.identifier_preparer.quote(settings.UPLOADS_SCHEMA) + + +def upgrade() -> None: + s = _schema() + op.execute(f"ALTER TABLE {s}.uploads ADD COLUMN columns TEXT[], ADD COLUMN column_types TEXT[]") + op.execute(f"""CREATE TABLE {s}.row_decisions ( + id BIGSERIAL PRIMARY KEY, + upload_id BIGINT NOT NULL REFERENCES {s}.uploads (id), + row_number INTEGER NOT NULL CHECK (row_number > 0), + decision TEXT NOT NULL CHECK (decision IN ('include', 'exclude')), + reason TEXT NOT NULL CHECK (btrim(reason) <> ''), + actor TEXT, + decided_at TIMESTAMPTZ NOT NULL DEFAULT now())""") + op.execute(f"""CREATE TABLE {s}.cell_edits ( + id BIGSERIAL PRIMARY KEY, + upload_id BIGINT NOT NULL REFERENCES {s}.uploads (id), + row_number INTEGER NOT NULL CHECK (row_number > 0), + column_index INTEGER NOT NULL CHECK (column_index >= 0), + old_value TEXT NOT NULL, + new_value TEXT NOT NULL, + reason TEXT NOT NULL CHECK (btrim(reason) <> ''), + actor TEXT, + edited_at TIMESTAMPTZ NOT NULL DEFAULT now())""") + for t in _TABLES: + op.execute(f"CREATE INDEX {t}_by_row ON {s}.{t} (upload_id, row_number, id)") + op.execute(f"CREATE TRIGGER {t}_no_change BEFORE UPDATE OR DELETE ON {s}.{t} " + f"FOR EACH ROW EXECUTE FUNCTION {s}.audit_reject_change()") + op.execute(f"CREATE TRIGGER {t}_no_truncate BEFORE TRUNCATE ON {s}.{t} " + f"FOR EACH STATEMENT EXECUTE FUNCTION {s}.audit_reject_change()") + + +def downgrade() -> None: + s = _schema() + for t in reversed(_TABLES): + op.execute(f"DROP TABLE IF EXISTS {s}.{t}") + op.execute(f"ALTER TABLE {s}.uploads DROP COLUMN columns, DROP COLUMN column_types") diff --git a/api/auth.py b/api/auth.py index b424cee..93c920a 100644 --- a/api/auth.py +++ b/api/auth.py @@ -88,6 +88,7 @@ async def tenant_scope(authorization: str = Header(default=""), "delete_file": frozenset({"tenant_admin"}), "read_audit": frozenset({"tenant_admin", "auditor"}), "reset_password": frozenset({"tenant_admin"}), + "review_rows": frozenset({"reviewer", "tenant_admin"}), } diff --git a/api/db.py b/api/db.py index 27f267c..4f36d96 100644 --- a/api/db.py +++ b/api/db.py @@ -202,7 +202,8 @@ def owner_connect(): # Tables the runtime role may only read and insert (M3, #107). Triggers also stop the owner. -APPEND_ONLY = ("audit_log", "uploads", "upload_blobs", "source_rows", "alembic_version") +APPEND_ONLY = ("audit_log", "uploads", "upload_blobs", "source_rows", "row_decisions", "cell_edits", + "alembic_version") def grant_runtime(cur, schema: str, create: bool) -> None: diff --git a/api/main.py b/api/main.py index 788a9e7..bb816b1 100644 --- a/api/main.py +++ b/api/main.py @@ -18,7 +18,7 @@ from api import db, oidc from api.config import settings from api.ratelimit import rate_limit_middleware -from api.routers import audit_routes, auth_routes, csv_routes, te_routes +from api.routers import audit_routes, auth_routes, csv_routes, review_routes, te_routes logger = logging.getLogger(__name__) @@ -97,6 +97,7 @@ async def lifespan(app: FastAPI): app.include_router(auth_routes.router) app.include_router(csv_routes.router) +app.include_router(review_routes.router) app.include_router(te_routes.router) app.include_router(audit_routes.router) diff --git a/api/routers/review_routes.py b/api/routers/review_routes.py new file mode 100644 index 0000000..a31ef71 --- /dev/null +++ b/api/routers/review_routes.py @@ -0,0 +1,56 @@ +"""Review endpoints for stored uploads (M4a, #108; BR-25, BR-26, BR-27). See api/services/review.py.""" + +from __future__ import annotations + +from fastapi import APIRouter, Depends, HTTPException +from pydantic import BaseModel + +from api.auth import allow, tenant_scope +from api.db import Conn +from api.services import review + +router = APIRouter(prefix="/api/csv/uploads", tags=["review"], dependencies=[Depends(tenant_scope)]) + + +class DecisionRequest(BaseModel): + decision: str + reason: str + + +class EditRequest(BaseModel): + value: str + reason: str + + +def _run(fn, *args, commit: bool = False): + with Conn() as conn: + with conn.cursor() as cur: + try: + result = fn(cur, *args) + except review.ReviewError as exc: + conn.rollback() + raise HTTPException(exc.status, str(exc)) from None + if commit: + conn.commit() + return result + + +@router.get("/{upload_id}/rows", dependencies=[Depends(allow("read_rows"))]) +def rows(upload_id: int, offset: int = 0, limit: int = 100) -> dict: + """One page of every stored row, with current values, validation errors and decisions.""" + return _run(review.page, upload_id, offset, limit) + + +@router.put("/{upload_id}/rows/{row_number}/decision", dependencies=[Depends(allow("review_rows"))]) +def decide(upload_id: int, row_number: int, req: DecisionRequest) -> dict: + return _run(review.decide, upload_id, row_number, req.decision, req.reason, commit=True) + + +@router.put("/{upload_id}/rows/{row_number}/cells/{column}", dependencies=[Depends(allow("review_rows"))]) +def edit(upload_id: int, row_number: int, column: str, req: EditRequest) -> dict: + return _run(review.edit, upload_id, row_number, column, req.value, req.reason, commit=True) + + +@router.get("/{upload_id}/rows/{row_number}/history", dependencies=[Depends(allow("read_rows"))]) +def history(upload_id: int, row_number: int) -> list[dict]: + return _run(review.history, upload_id, row_number) diff --git a/api/services/audit.py b/api/services/audit.py index c67e9ce..ef9455c 100644 --- a/api/services/audit.py +++ b/api/services/audit.py @@ -38,7 +38,7 @@ # Actions the log is allowed to record. A typo becomes a loud failure here rather # than an unqueryable string in the table. -VALID_ACTIONS = frozenset({"upload", "delete", "password_reset"}) +VALID_ACTIONS = frozenset({"upload", "delete", "password_reset", "row_decision", "cell_edit"}) # Who is acting (M3a, #107). tenant_scope sets it per request: the signed-in # username, or "api_key" for a request made with the shared key. diff --git a/api/services/dynamic_loader.py b/api/services/dynamic_loader.py index c2a86e6..4cb49ec 100644 --- a/api/services/dynamic_loader.py +++ b/api/services/dynamic_loader.py @@ -330,7 +330,8 @@ def _do_upload( msg += f' First problem: row {e["rowNumber"]}, column "{e["column"]}": {e["reason"]}' _log(logs, "error", msg, "error") return {"status": "error", "message": msg, "rowErrors": row_errors, - "totalRows": len(data_rows), "failedRows": failed_row_count, "logs": logs} + "totalRows": len(data_rows), "failedRows": failed_row_count, + "columns": columns, "types": col_types, "logs": logs} # Insert inserted = 0 diff --git a/api/services/originals.py b/api/services/originals.py index 60f88fb..94ce358 100644 --- a/api/services/originals.py +++ b/api/services/originals.py @@ -46,13 +46,13 @@ def record_upload(file_name: str, content: str, mode: str, result: dict) -> int s = sql.Identifier(uploads_schema()) cur.execute(sql.SQL( "INSERT INTO {}.uploads (file_name, sha256, byte_size, uploader, mode, status, " - "total_rows, imported_rows, failed_rows, csv_file_id, row_errors, logs) " - "VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s) RETURNING id").format(s), + "total_rows, imported_rows, failed_rows, csv_file_id, row_errors, logs, columns, column_types) " + "VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s) RETURNING id").format(s), (file_name, hashlib.sha256(data).hexdigest(), len(data), current_actor(), mode, outcome(result), result.get("totalRows"), result.get("insertedRows"), result.get("failedRows"), coerce_file_id(result.get("fileId")) if ok else None, Json((result.get("rowErrors") or [])[:settings.MAX_ROW_ERRORS_REPORTED]), - Json(result.get("logs") or []))) + Json(result.get("logs") or []), result.get("columns"), result.get("types"))) upload_id = cur.fetchone()[0] cur.execute(sql.SQL("INSERT INTO {}.upload_blobs (upload_id, content) VALUES (%s, %s)").format(s), (upload_id, data)) diff --git a/api/services/review.py b/api/services/review.py new file mode 100644 index 0000000..cb2a5a7 --- /dev/null +++ b/api/services/review.py @@ -0,0 +1,188 @@ +"""Review of a stored upload: every row, its validation, row decisions and cell edits (M4a, #108). + +Works on M3's ``source_rows``; the original is never changed. Decisions and +edits are insert-only rows (migration 0008), and the latest one wins. They are +applied by M6's staged import, not to the table imported at upload time +(product owner, 28 Oct). +""" + +from __future__ import annotations + +from psycopg2 import sql + +from api.services.audit import current_actor, write_audit +from api.services.csv_parse import cast_value, extra_cells, ragged_row_reason, sanitize_columns +from api.tenancy import uploads_schema + +MAX_PAGE = 500 + + +class ReviewError(ValueError): + """A request the API answers with 4xx: ``status`` is the HTTP code.""" + + def __init__(self, status: int, message: str): + super().__init__(message) + self.status = status + + +def _s(): + return sql.Identifier(uploads_schema()) + + +def _upload(cur, upload_id: int) -> dict: + cur.execute(sql.SQL( + "SELECT u.file_name, u.columns, u.column_types, h.cells, " + " (SELECT count(*) FROM {s}.source_rows r WHERE r.upload_id = u.id AND r.row_number > 0) " + "FROM {s}.uploads u LEFT JOIN {s}.source_rows h ON h.upload_id = u.id AND h.row_number = 0 " + "WHERE u.id = %s").format(s=_s()), (upload_id,)) + row = cur.fetchone() + if row is None: + raise ReviewError(404, "No such upload") + name, columns, types, header, total = row + columns = columns or sanitize_columns(header or []) + # Uploads stored before 0008, or in T&E mode, have no types: every value counts as text. + types = types if types and len(types) == len(columns) else ["text"] * len(columns) + return {"fileName": name, "columns": columns, "types": types, "total": total} + + +def _validate(cells: list[str], raw: list[str], columns: list[str], types: list[str]) -> list[dict]: + errors = [] + if extra_cells(raw, len(columns)): + errors.append({"column": "(beyond header)", "value": extra_cells(raw, len(columns))[0], + "reason": ragged_row_reason(raw, len(columns))}) + for name, value, col_type in zip(columns, cells, types): + ok, _, reason = cast_value(value, col_type) + if not ok: + errors.append({"column": name, "value": value, "reason": reason}) + return errors + + +def _latest(cur, upload_id: int, numbers: list[int]) -> tuple[dict, dict]: + """The current decision per row and the current value per edited cell.""" + cur.execute(sql.SQL( + "SELECT DISTINCT ON (row_number) row_number, decision, reason, actor FROM {}.row_decisions " + "WHERE upload_id = %s AND row_number = ANY(%s) ORDER BY row_number, id DESC").format(_s()), + (upload_id, numbers)) + decisions = {n: {"decision": d, "reason": r, "actor": a} for n, d, r, a in cur.fetchall()} + cur.execute(sql.SQL( + "SELECT DISTINCT ON (row_number, column_index) row_number, column_index, new_value " + "FROM {}.cell_edits WHERE upload_id = %s AND row_number = ANY(%s) " + "ORDER BY row_number, column_index, id DESC").format(_s()), (upload_id, numbers)) + edits: dict = {} + for n, c, v in cur.fetchall(): + edits.setdefault(n, {})[c] = v + return decisions, edits + + +def _rows(cur, upload_id: int, meta: dict, numbers: list[int] | None = None, + offset: int = 0, limit: int = 100) -> list[dict]: + if numbers is None: + cur.execute(sql.SQL( + "SELECT row_number, cells FROM {}.source_rows WHERE upload_id = %s AND row_number > 0 " + "ORDER BY row_number OFFSET %s LIMIT %s").format(_s()), (upload_id, offset, limit)) + else: + cur.execute(sql.SQL( + "SELECT row_number, cells FROM {}.source_rows WHERE upload_id = %s AND row_number = ANY(%s) " + "ORDER BY row_number").format(_s()), (upload_id, numbers)) + source = cur.fetchall() + decisions, edits = _latest(cur, upload_id, [n for n, _ in source]) + width = len(meta["columns"]) + out = [] + for n, raw in source: + original = (list(raw) + [""] * width)[:width] + current = [edits.get(n, {}).get(c, v) for c, v in enumerate(original)] + decision = decisions.get(n) + out.append({ + "rowNumber": n, "original": original, "current": current, + "editedColumns": sorted(edits.get(n, {})), + "decision": decision["decision"] if decision else "include", + "decisionReason": decision["reason"] if decision else None, + "errors": _validate(current, raw, meta["columns"], meta["types"]), + }) + return out + + +def page(cur, upload_id: int, offset: int, limit: int) -> dict: + meta = _upload(cur, upload_id) + offset, limit = max(0, offset), max(1, min(limit, MAX_PAGE)) + cur.execute(sql.SQL( + "SELECT (SELECT count(*) FROM (SELECT DISTINCT ON (row_number) decision FROM {s}.row_decisions " + " WHERE upload_id = %s ORDER BY row_number, id DESC) d WHERE decision = 'exclude'), " + " (SELECT count(DISTINCT row_number) FROM {s}.cell_edits WHERE upload_id = %s)").format(s=_s()), + (upload_id, upload_id)) + excluded, edited = cur.fetchone() + return {**meta, "offset": offset, "limit": limit, "excludedRows": excluded, "editedRows": edited, + "rows": _rows(cur, upload_id, meta, offset=offset, limit=limit)} + + +def _check_row(meta: dict, row_number: int) -> None: + if not 1 <= row_number <= meta["total"]: + raise ReviewError(404, f"Row {row_number} doesn't exist; this upload has {meta['total']} rows") + + +def _reason(reason: str) -> str: + if not (reason or "").strip(): + raise ReviewError(422, "A reason is required") + return reason.strip() + + +def decide(cur, upload_id: int, row_number: int, decision: str, reason: str) -> dict: + """Include or exclude one row. The caller commits.""" + meta = _upload(cur, upload_id) + _check_row(meta, row_number) + if decision not in ("include", "exclude"): + raise ReviewError(422, "decision must be 'include' or 'exclude'") + reason = _reason(reason) + [before] = _rows(cur, upload_id, meta, numbers=[row_number]) + cur.execute(sql.SQL("INSERT INTO {}.row_decisions (upload_id, row_number, decision, reason, actor) " + "VALUES (%s, %s, %s, %s, %s)").format(_s()), + (upload_id, row_number, decision, reason, current_actor())) + write_audit(cur, "row_decision", None, meta["fileName"], None, None, + before={"upload": upload_id, "row": row_number, "decision": before["decision"]}, + after={"upload": upload_id, "row": row_number, "decision": decision, "reason": reason}) + [after] = _rows(cur, upload_id, meta, numbers=[row_number]) + return after + + +def edit(cur, upload_id: int, row_number: int, column: str, value: str, reason: str) -> dict: + """Set one cell's value as a new version. ``column`` is a name or a 0-based index. The caller commits.""" + meta = _upload(cur, upload_id) + _check_row(meta, row_number) + names = meta["columns"] + index = int(column) if column.isdigit() else (names.index(column) if column in names else -1) + if not 0 <= index < len(names): + raise ReviewError(404, f"No column {column!r}") + reason = _reason(reason) + [before] = _rows(cur, upload_id, meta, numbers=[row_number]) + old = before["current"][index] + if value == old: + raise ReviewError(422, "The new value is the same as the current one") + cur.execute(sql.SQL("INSERT INTO {}.cell_edits (upload_id, row_number, column_index, old_value, new_value, " + "reason, actor) VALUES (%s, %s, %s, %s, %s, %s, %s)").format(_s()), + (upload_id, row_number, index, old, value, reason, current_actor())) + cell = {"upload": upload_id, "row": row_number, "column": names[index]} + write_audit(cur, "cell_edit", None, meta["fileName"], None, None, + before={**cell, "value": old}, after={**cell, "value": value, "reason": reason}) + [after] = _rows(cur, upload_id, meta, numbers=[row_number]) + return after + + +def history(cur, upload_id: int, row_number: int) -> list[dict]: + """Every decision and edit on one row, oldest first.""" + meta = _upload(cur, upload_id) + _check_row(meta, row_number) + cur.execute(sql.SQL( + "SELECT 'decision', decided_at, actor, reason, decision, NULL::int, NULL, NULL FROM {s}.row_decisions " + " WHERE upload_id = %s AND row_number = %s " + "UNION ALL SELECT 'edit', edited_at, actor, reason, NULL, column_index, old_value, new_value " + " FROM {s}.cell_edits WHERE upload_id = %s AND row_number = %s ORDER BY 2").format(s=_s()), + (upload_id, row_number, upload_id, row_number)) + out = [] + for kind, at, actor, reason, decision, index, old, new in cur.fetchall(): + entry = {"kind": kind, "at": at.isoformat(), "actor": actor, "reason": reason} + if kind == "decision": + entry["decision"] = decision + else: + entry.update(column=meta["columns"][index], oldValue=old, newValue=new) + out.append(entry) + return out diff --git a/docs/ROLES.md b/docs/ROLES.md index 5ac0a76..69bded2 100644 --- a/docs/ROLES.md +++ b/docs/ROLES.md @@ -7,11 +7,12 @@ Every member of a tenant has one role (ADR-0003). This table is enforced by `PER | preview | `POST /api/csv/preview` | ✓ | | ✓ | | | upload | `POST /api/csv/upload` | ✓ | | ✓ | | | list_files | `GET /api/csv/files`, `GET /api/csv/uploads` | ✓ | ✓ | ✓ | ✓ | -| read_rows | `GET /api/csv/tables/{table}/rows`, `GET /api/csv/uploads/{id}/original` | ✓ | ✓ | ✓ | ✓ | +| read_rows | `GET /api/csv/tables/{table}/rows`, `GET /api/csv/uploads/{id}/original`, `GET /api/csv/uploads/{id}/rows`, `GET /api/csv/uploads/{id}/rows/{n}/history` | ✓ | ✓ | ✓ | ✓ | | list_te | `GET /api/te/tables` | ✓ | ✓ | ✓ | ✓ | | delete_file | `DELETE /api/csv/files/{id}` | | | ✓ | | | read_audit | `GET /api/audit/log`, `GET /api/audit/verify` | | | ✓ | ✓ | | reset_password | `POST /api/auth/users/{username}/password` | | | ✓ | | +| review_rows | `PUT /api/csv/uploads/{id}/rows/{n}/decision`, `PUT /api/csv/uploads/{id}/rows/{n}/cells/{column}` | | ✓ | ✓ | | A role that isn't allowed gets **403**. Two kinds of request are outside this table: diff --git a/docs/VCRM.md b/docs/VCRM.md index d3f8a84..f21a400 100644 --- a/docs/VCRM.md +++ b/docs/VCRM.md @@ -144,9 +144,9 @@ Status as assessed on 2026-10-26 at `main` 032b522. None of these is "Verified" |----|--------|--------|----------------------|-----------| | BR-23 | ✅ | Every upload attempt gets an `uploads` row (M3b, #107): id, original filename, SHA-256, uploader, timestamp and outcome (imported, partly_imported, failed, duplicate, rejected). The tenant is the schema the row lives in (decision 6), not a column. Registry UNIQUE hash and name from M1 (#87). `test_preserved_uploads.py` | Integration + DB constraint tests | M1, M3 | | BR-24 | ⚠️ Partial | Original bytes (`upload_blobs`) and parsed rows (`source_rows`) kept for every upload, including failed ones and files later overwritten or deleted (M3b, #107). Triggers reject UPDATE, DELETE and TRUNCATE (`test_preserved_uploads.py`, 7 tests), and the API's runtime role has no UPDATE, DELETE or TRUNCATE grant on them (M3d, `test_runtime_role.py`). Still to do: show edits kept separate once they exist (M4) | DB negative tests | M3 | -| BR-25 | ⚠️ Partial | 10-row preview sample; 200-row cap on table view | API paging + e2e | M4 | -| BR-26 | ❌ Not started | No code | API + e2e | M4 | -| BR-27 | ❌ Not started | No code | API + DB tests | M4 | +| BR-25 | ⚠️ Partial | API: `GET /api/csv/uploads/{id}/rows` pages through every stored row (up to 500 a page), with column types and each row's validation errors, including rows beyond the 200 errors reported at upload (M4a, #108, `test_review.py`). Still to do: the review screen and its e2e test (M4b) | API paging + e2e | M4 | +| BR-26 | ⚠️ Partial | API: include or exclude a row with a required reason; actor and time recorded; insert-only history, latest wins; audited with before and after (M4a, #108, `test_review.py`). Applied to imports in M6 (decision 1 on #108). Still to do: the review screen and its e2e test (M4b) | API + e2e | M4 | +| BR-27 | ✅ | Cell edits are insert-only versions in `cell_edits` with old and new value, actor, time and a required reason; the original in `source_rows` is unchanged; edited values are revalidated; each edit is audited (M4a, #108, `test_review.py`: API and DB tests). The screen to make edits arrives in M4b | API + DB tests | M4 | | BR-28 | ⚠️ Partial | Race guard enforced by UNIQUE constraints (M1: #87, `tests/test_migrations.py`). For signed-in tenants the registry is per tenant, so the same file in two tenants is not a duplicate (M2: #100, `test_duplicate_detection_is_per_tenant`). Requests without a session still share the default registry; M5 finishes the per-tenant model | Concurrency + cross-tenant tests | M1, M5 | | BR-29 | ⚠️ Partial | In-file exact match only | Unit + integration | M5 | | BR-30 | ⚠️ Partial | Content-derived names; repeats refused or destructive | Unit + integration | M5 | @@ -178,12 +178,12 @@ Status as assessed on 2026-10-26 at `main` 032b522. None of these is "Verified" | Status | Count | Requirements | |--------|-------|-------------| -| ✅ Verified | 20 | BR-01, BR-03, BR-04, BR-05, BR-06, BR-07, BR-08, BR-09, BR-10, BR-11, BR-12, BR-13, BR-14, BR-15, BR-16, BR-17, BR-18, BR-19, BR-20, BR-23 (M3b) | -| ⚠️ Partial | 11 | BR-24, BR-25, BR-28, BR-29, BR-30, BR-31, BR-33, BR-34, BR-35, BR-36, BR-37 (ART, added 2026-10-26) | -| ❌ Not started | 4 | BR-26, BR-27, BR-32 (ART, added 2026-10-26); BR-38 (added 2026-10-27) | +| ✅ Verified | 21 | BR-01, BR-03, BR-04, BR-05, BR-06, BR-07, BR-08, BR-09, BR-10, BR-11, BR-12, BR-13, BR-14, BR-15, BR-16, BR-17, BR-18, BR-19, BR-20, BR-23 (M3b), BR-27 (M4a) | +| ⚠️ Partial | 12 | BR-24, BR-25, BR-26, BR-28, BR-29, BR-30, BR-31, BR-33, BR-34, BR-35, BR-36, BR-37 (ART, added 2026-10-26) | +| ❌ Not started | 2 | BR-32 (ART, added 2026-10-26); BR-38 (added 2026-10-27) | | ⏸ Deferred by decision | 3 | BR-02, BR-21, BR-22 | -**Headline (2026-10-28):** with BR-23 verified by M3b (#107), **20 of 38 (53 %)** are verified, 3 are deferred by decision, and the other 15 are partial or not started. +**Headline (2026-10-28):** with BR-23 verified by M3b (#107) and BR-27 by M4a (#108), **21 of 38 (55 %)** are verified, 3 are deferred by decision, and the other 14 are partial or not started. **Headline (2026-10-26):** with the 15 ART requirements added, **19 of 37 (51 %)** are verified, 3 are deferred by decision, and the other 15 are partial or not started. diff --git a/tests/test_api_hardening.py b/tests/test_api_hardening.py index 2f4db56..45acdb3 100644 --- a/tests/test_api_hardening.py +++ b/tests/test_api_hardening.py @@ -191,7 +191,7 @@ class AuditWriterContract(unittest.TestCase): """O-04: the shared audit writer's guardrails, without touching a database.""" def test_only_known_actions_are_accepted(self): - self.assertEqual(VALID_ACTIONS, {"upload", "delete", "password_reset"}) + self.assertEqual(VALID_ACTIONS, {"upload", "delete", "password_reset", "row_decision", "cell_edit"}) def test_unknown_action_raises_before_any_sql_runs(self): class ExplodingCursor: diff --git a/tests/test_migrations.py b/tests/test_migrations.py index d6f9079..ad818b4 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -148,7 +148,7 @@ def setUp(self): def test_bootstrap_brings_the_schema_to_head(self): with Conn() as conn, conn.cursor() as cur: cur.execute(f'SELECT version_num FROM "{settings.UPLOADS_SCHEMA}".alembic_version') - self.assertEqual(cur.fetchone()[0], "0007") + self.assertEqual(cur.fetchone()[0], "0008") def test_duplicate_hash_is_rejected_by_the_database(self): insert = (f'INSERT INTO "{settings.UPLOADS_SCHEMA}".csv_files (file_name, file_hash, table_name, mode) ' diff --git a/tests/test_review.py b/tests/test_review.py new file mode 100644 index 0000000..92a159a --- /dev/null +++ b/tests/test_review.py @@ -0,0 +1,128 @@ +"""Review of stored uploads: paging, row decisions and versioned cell edits (M4a, #108; BR-25..27). + +Integration: needs a live PostgreSQL. Fails (never skips) without one. +""" + +from __future__ import annotations + +import os +import unittest +import uuid + +import psycopg2.errors +import pytest +from fastapi.testclient import TestClient +from psycopg2 import sql + +from api.config import settings +from api.db import Conn +from api.main import app + + +@pytest.mark.integration +class Review(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.ctx = TestClient(app) + cls.client = cls.ctx.__enter__() + + @classmethod + def tearDownClass(cls): + cls.ctx.__exit__(None, None, None) + + def setUp(self): + self.tag = f"{os.getpid()}_{uuid.uuid4().hex[:8]}" + + def _upload(self, content: str, types: list[str]) -> dict: + return self.client.post("/api/csv/upload", json={ + "fileName": f"rev_{self.tag}.csv", "content": content, "types": types}).json() + + def _three_rows(self) -> str: + res = self._upload(f"n,t\n1,{self.tag}\nx,b\n3,c\n", ["int8", "text"]) + self.assertEqual(res["status"], "ok", res) + return res["uploadId"] + + def _rows(self, uid: str, **params) -> dict: + res = self.client.get(f"/api/csv/uploads/{uid}/rows", params=params) + self.assertEqual(res.status_code, 200, res.text) + return res.json() + + def _decide(self, uid: str, row: int, decision: str, reason: str = "checked with the owner"): + return self.client.put(f"/api/csv/uploads/{uid}/rows/{row}/decision", + json={"decision": decision, "reason": reason}) + + def _edit(self, uid: str, row: int, column: str, value: str, reason: str = "typo in source"): + return self.client.put(f"/api/csv/uploads/{uid}/rows/{row}/cells/{column}", + json={"value": value, "reason": reason}) + + def test_pages_through_every_row_with_validation(self): + uid = self._three_rows() + page = self._rows(uid) + self.assertEqual((page["total"], page["columns"], page["types"]), (3, ["n", "t"], ["int8", "text"])) + self.assertEqual([r["rowNumber"] for r in page["rows"]], [1, 2, 3]) + self.assertEqual([len(r["errors"]) for r in page["rows"]], [0, 1, 0]) + self.assertEqual(page["rows"][1]["errors"][0]["reason"], '"x" is not a whole number') + self.assertEqual([r["rowNumber"] for r in self._rows(uid, offset=2, limit=2)["rows"]], [3]) + + def test_validates_rows_beyond_the_reported_error_cap(self): + n = settings.MAX_ROW_ERRORS_REPORTED + 50 + res = self._upload("n\n" + "".join(f"bad{i}_{self.tag}\n" for i in range(n)), ["int8"]) + self.assertEqual(res["status"], "error", res) + self.assertEqual(len(res["rowErrors"]), settings.MAX_ROW_ERRORS_REPORTED) + last = self._rows(res["uploadId"], offset=n - 1, limit=1)["rows"][0] + self.assertEqual((last["rowNumber"], len(last["errors"])), (n, 1)) + + def test_row_decisions_record_actor_and_reason_and_the_latest_wins(self): + uid = self._three_rows() + self.assertEqual(self._decide(uid, 2, "exclude").status_code, 200) + page = self._rows(uid) + self.assertEqual((page["rows"][1]["decision"], page["excludedRows"]), ("exclude", 1)) + self.assertEqual(self._decide(uid, 2, "include", "fixed upstream").json()["decision"], "include") + self.assertEqual(self._rows(uid)["excludedRows"], 0) + history = self.client.get(f"/api/csv/uploads/{uid}/rows/2/history").json() + self.assertEqual([(h["decision"], h["actor"], h["reason"]) for h in history], + [("exclude", "api_key", "checked with the owner"), ("include", "api_key", "fixed upstream")]) + + def test_cell_edits_are_versions_over_the_unchanged_original(self): + uid = self._three_rows() + row = self._edit(uid, 2, "n", "2").json() + self.assertEqual((row["original"], row["current"], row["editedColumns"], row["errors"]), + (["x", "b"], ["2", "b"], [0], [])) + self.assertEqual(self._edit(uid, 2, "0", "20", "second look").json()["current"], ["20", "b"]) + edits = [h for h in self.client.get(f"/api/csv/uploads/{uid}/rows/2/history").json() if h["kind"] == "edit"] + self.assertEqual([(e["column"], e["oldValue"], e["newValue"], e["reason"]) for e in edits], + [("n", "x", "2", "typo in source"), ("n", "2", "20", "second look")]) + with Conn() as conn, conn.cursor() as cur: # M3's original is untouched + cur.execute(sql.SQL("SELECT cells FROM {}.source_rows WHERE upload_id = %s AND row_number = 2") + .format(sql.Identifier(settings.UPLOADS_SCHEMA)), (int(uid),)) + self.assertEqual(cur.fetchone()[0], ["x", "b"]) + listed = [f for f in self.client.get("/api/csv/files").json() if f["upload_id"] == uid] + self.assertEqual(listed[0]["row_count"], 2, "decision 1: the imported table doesn't change until M6") + + def test_refusals(self): + uid = self._three_rows() + for res, code in ((self._decide(uid, 2, "exclude", " "), 422), (self._edit(uid, 2, "n", "5", ""), 422), + (self._decide(uid, 2, "maybe"), 422), (self._edit(uid, 2, "n", "x"), 422), + (self._decide(uid, 4, "exclude"), 404), (self._edit(uid, 1, "nope", "1"), 404), + (self.client.get("/api/csv/uploads/999999999/rows"), 404)): + with self.subTest(body=res.text): + self.assertEqual(res.status_code, code) + self.assertEqual(self.client.get(f"/api/csv/uploads/{uid}/rows/2/history").json(), []) + + def test_changes_are_audited_and_insert_only(self): + uid = self._three_rows() + self._decide(uid, 3, "exclude") + self._edit(uid, 2, "n", "2") + with Conn() as conn, conn.cursor() as cur: + s = sql.Identifier(settings.UPLOADS_SCHEMA) + cur.execute(sql.SQL("SELECT action, before_value, after_value FROM {}.audit_log " + "WHERE after_value->>'upload' = %s ORDER BY id").format(s), (uid,)) + [(a1, b1, f1), (a2, b2, f2)] = cur.fetchall() + self.assertEqual((a1, b1["decision"], f1["decision"]), ("row_decision", "include", "exclude")) + self.assertEqual((a2, b2["value"], f2["value"], f2["column"]), ("cell_edit", "x", "2", "n")) + for table in ("row_decisions", "cell_edits"): + with self.subTest(table=table), self.assertRaises(psycopg2.errors.InsufficientPrivilege): + cur.execute(sql.SQL("DELETE FROM {}.{} WHERE upload_id = %s").format(s, sql.Identifier(table)), + (int(uid),)) + conn.rollback() + self.assertTrue(self.client.get("/api/audit/verify").json()["intact"]) diff --git a/tests/test_roles.py b/tests/test_roles.py index 553d32d..55d2775 100644 --- a/tests/test_roles.py +++ b/tests/test_roles.py @@ -35,6 +35,7 @@ "delete_file": ("DELETE", "/api/csv/files/{id}"), "read_audit": ("GET", "/api/audit/log"), "reset_password": ("POST", "/api/auth/users/{user}/password"), + "review_rows": ("PUT", "/api/csv/uploads/{upload}/rows/1/decision"), } @@ -96,16 +97,16 @@ def tearDownClass(cls): conn.commit() cls.ctx.__exit__(None, None, None) - def _file(self) -> tuple[str, str]: + def _file(self) -> tuple[str, str, str]: tag = uuid.uuid4().hex[:10] res = self.client.post("/api/csv/upload", headers=self.headers["tenant_admin"], json={"fileName": f"r_{tag}.csv", "content": f"k\n{tag}\n"}).json() assert res["status"] == "ok", res - return res["tableName"], res["fileId"] + return res["tableName"], res["fileId"], res["uploadId"] def _call(self, role: str, action: str) -> int: method, path = _ROUTES[action] - table, file_id = self._file() + table, file_id, upload_id = self._file() body, user = None, None if action in ("preview", "upload"): tag = uuid.uuid4().hex[:10] @@ -121,7 +122,9 @@ def _call(self, role: str, action: str) -> int: accounts.add_membership(cur, accounts.create_user(cur, user, _PW), tid, "uploader") conn.commit() body = {"newPassword": "a brand new passphrase"} - res = self.client.request(method, path.format(table=table, id=file_id, user=user), + if action == "review_rows": + body = {"decision": "exclude", "reason": "role matrix test"} + res = self.client.request(method, path.format(table=table, id=file_id, user=user, upload=upload_id), headers=self.headers[role], json=body) return res.status_code diff --git a/tests/test_tenant_isolation.py b/tests/test_tenant_isolation.py index bbd4a01..5fc4599 100644 --- a/tests/test_tenant_isolation.py +++ b/tests/test_tenant_isolation.py @@ -199,7 +199,7 @@ def test_tenant_schema_is_at_head_with_its_own_registry(self): with Conn() as conn, conn.cursor() as cur: cur.execute(sql.SQL("SELECT version_num FROM {}.alembic_version").format( sql.Identifier(schema_for(self.slugs["b"])))) - self.assertEqual(cur.fetchone()[0], "0007") + self.assertEqual(cur.fetchone()[0], "0008") cur.execute("SELECT table_name FROM information_schema.tables WHERE table_schema = %s", (schema_for(self.slugs["b"]),)) self.assertTrue({"csv_files", "audit_log"} <= {r[0] for r in cur.fetchall()}) From 747033880ab1509e0137e23a016dc6daf4fc126f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 14:37:18 +1100 Subject: [PATCH 2/3] fix(api): allow PUT in CORS so the browser can send review changes (#108) 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 Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB --- api/main.py | 2 +- tests/test_api_coverage.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/api/main.py b/api/main.py index bb816b1..5389d03 100644 --- a/api/main.py +++ b/api/main.py @@ -86,7 +86,7 @@ async def lifespan(app: FastAPI): app.add_middleware( CORSMiddleware, allow_origins=settings.CORS_ORIGINS, - allow_methods=["GET", "POST", "DELETE"], + allow_methods=["GET", "POST", "PUT", "DELETE"], # PUT: review changes (M4, #108) allow_headers=["Content-Type", "X-API-Key", "Authorization"], ) diff --git a/tests/test_api_coverage.py b/tests/test_api_coverage.py index 5fe2f76..a04b006 100644 --- a/tests/test_api_coverage.py +++ b/tests/test_api_coverage.py @@ -117,6 +117,16 @@ def test_preflight_returns_cors_headers(self): }) self.assertIn("access-control-allow-origin", r.headers) + def test_preflight_allows_put_for_review_changes(self): + """#108: the browser's review screen sends PUT; without it the preflight fails.""" + r = client.options("/api/csv/uploads/1/rows/1/decision", headers={ + "Origin": "http://localhost:8080", + "Access-Control-Request-Method": "PUT", + "Access-Control-Request-Headers": "authorization,content-type", + }) + self.assertEqual(r.status_code, 200, r.text) + self.assertIn("PUT", r.headers.get("access-control-allow-methods", "")) + def test_cors_allows_configured_origin(self): r = client.get("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/api/health", headers={"Origin": "http://localhost:5173"}) self.assertEqual( From ef8bd892f3b0b1f88a6630a6622de08011d6020c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 15:59:00 +1100 Subject: [PATCH 3/3] test(review): positive and negative test cases for the review API (#108) 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 Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB --- docs/test-cases/M4-review.md | 97 +++++++++++++++ tests/test_review_cases.py | 233 +++++++++++++++++++++++++++++++++++ tests/test_runtime_role.py | 12 +- 3 files changed, 340 insertions(+), 2 deletions(-) create mode 100644 docs/test-cases/M4-review.md create mode 100644 tests/test_review_cases.py diff --git a/docs/test-cases/M4-review.md b/docs/test-cases/M4-review.md new file mode 100644 index 0000000..bb5a060 --- /dev/null +++ b/docs/test-cases/M4-review.md @@ -0,0 +1,97 @@ +# Test cases: M4 review API (#108, PR #142) + +**Scope:** the review API that M4a adds (`GET /api/csv/uploads/{id}/rows`, `PUT …/rows/{n}/decision`, `PUT …/rows/{n}/cells/{column}`, `GET …/rows/{n}/history`), its database tables (`row_decisions`, `cell_edits`) and the CORS change for `PUT`. +**Requirements:** BR-25, BR-26, BR-27 (review), BR-35 (append-only audit), BR-36 (roles and tenant isolation). +**Decisions they test** (product owner, 28 Oct, on #108): review is recorded now and applied in M6; only `reviewer` and `tenant_admin` may change; a reason is required. + +There are 17 positive and 16 negative cases. Every case has an automated test, and CI runs them all. Where you can also check a case by hand on your PC, the **Manual** column gives the command. + +## How to run + +### Automated (all cases) + +CI runs them on every push. To run them on your PC, PostgreSQL must be running with the `PG*` values from QUICKSTART step 3 set. The tests make their own throwaway tenants and users, and remove them afterwards. + +```powershell +cd "C:\Users\User\OneDrive\Desktop\Migration using ai\PostgreDataMigrationAppWithCSVLoader" +python -m pytest -q tests/test_review.py tests/test_review_cases.py tests/test_runtime_role.py tests/test_roles.py tests/test_api_coverage.py +``` + +**Pass:** pytest ends with `passed` and no `failed`. + +### Manual (after #142 is merged, with the API running) + +Run this once in a PowerShell window. It signs in as your tenant admin and uploads a three-row file whose row 2 is invalid. Use your own tenant and username if they differ from `dev` and `admin`. + +```powershell +cd "C:\Users\User\OneDrive\Desktop\Migration using ai\PostgreDataMigrationAppWithCSVLoader" +$api = "http://localhost:8000" +$sec = Read-Host -AsSecureString "Password for admin" +$pw = [Runtime.InteropServices.Marshal]::PtrToStringAuto([Runtime.InteropServices.Marshal]::SecureStringToBSTR($sec)) +$login = Invoke-RestMethod -Method Post "$api/api/auth/login" -ContentType "application/json" -Body (@{tenant="dev"; username="admin"; password=$pw} | ConvertTo-Json) +$h = @{ Authorization = "Bearer $($login.token)" } +function Call($method, $path, $body) { + $req = @{ Method = $method; Uri = "$api$path"; Headers = $h } + if ($body) { $req.ContentType = "application/json"; $req.Body = ($body | ConvertTo-Json) } + try { Invoke-RestMethod @req } catch { "HTTP " + [int]$_.Exception.Response.StatusCode } +} +$tag = Get-Random +$up = Call Post "/api/csv/upload" @{ fileName = "tc-m4-$tag.csv"; content = "n,t`n1,first-$tag`nx,second`n3,third`n"; types = @("int8","text") } +$u = $up.uploadId +"upload: $($up.status), id $u" +``` + +**Expected:** `upload: ok, id `. Every command in the table below runs in **this same window**, so `$u` and `Call` are still defined. A refused request prints `HTTP ` instead of throwing an error. + +## Positive cases + +| ID | Requirement | What it checks | Manual command | Expected | Automated test | +|---|---|---|---|---|---| +| P01 | BR-25 | Every row is listed, with column types and validation | `$p = Call Get "/api/csv/uploads/$u/rows"; $p.total; $p.types -join ","; $p.rows[1].errors.reason` | `3`, `int8,text`, `"x" is not a whole number` | `test_review.py::test_pages_through_every_row_with_validation` | +| P02 | BR-25 | Paging limits: the page size is capped at 500, an offset past the end returns no rows, and a negative offset starts at 0 | `(Call Get "/api/csv/uploads/$u/rows?limit=10000").limit; (Call Get "/api/csv/uploads/$u/rows?offset=3").rows.Count` | `500`, `0` | `test_review_cases.py::test_P02_paging_bounds` | +| P03 | BR-25 | Rows beyond the 200 errors reported at upload are still validated | Automated only (needs a 250-row file) | Row 250 shows its error | `test_review.py::test_validates_rows_beyond_the_reported_error_cap` | +| P04 | BR-25 | A row wider than the header is flagged | `$r = Call Post "/api/csv/upload" @{fileName="tc-rag-$tag.csv"; content=(("a,b","1,ok-$tag","2,y,extra","") -join [char]10); types=@("text","text")}; (Call Get "/api/csv/uploads/$($r.uploadId)/rows").rows[1].errors.column` | `(beyond header)` | `test_review_cases.py::test_P04_ragged_row_is_flagged` | +| P05 | BR-25, BR-26 | An upload where every row failed can still be reviewed | `$f = Call Post "/api/csv/upload" @{fileName="tc-fail-$tag.csv"; content=(("n","bad-$tag","") -join [char]10); types=@("int8")}; (Call Get "/api/csv/uploads/$($f.uploadId)/rows").total` | `1` (the upload's status was `error`) | `test_review_cases.py::test_P05_a_failed_upload_can_be_reviewed` | +| P06 | BR-26 | Exclude, then include: the latest decision wins, and both are kept in the history | `Call Put "/api/csv/uploads/$u/rows/3/decision" @{decision="exclude"; reason="duplicate"}; Call Put "/api/csv/uploads/$u/rows/3/decision" @{decision="include"; reason="not a duplicate"}; (Call Get "/api/csv/uploads/$u/rows/3/history").decision -join ","` | `exclude,include`; the row's `decision` is `include` | `test_review.py::test_row_decisions_record_actor_and_reason_and_the_latest_wins` | +| P07 | BR-27 | An edit becomes a new version, the original is unchanged, and the row is revalidated | `$e = Call Put "/api/csv/uploads/$u/rows/2/cells/n" @{value="2"; reason="typo"}; $e.original[0]; $e.current[0]; $e.errors.Count` | `x`, `2`, `0` | `test_review.py::test_cell_edits_are_versions_over_the_unchanged_original` | +| P08 | BR-27 | An edit to a value of the wrong type is flagged | `(Call Put "/api/csv/uploads/$u/rows/1/cells/n" @{value="one"; reason="test"}).errors.reason` | `"one" is not a whole number` | `test_review_cases.py::test_P08_an_edit_to_a_bad_value_is_flagged` | +| P09 | BR-27 | A column can be given by index, and a text cell can be set to empty | `(Call Put "/api/csv/uploads/$u/rows/3/cells/1" @{value=""; reason="blank it"}).current -join ","` | `3,` | `test_review_cases.py::test_P09_edit_by_column_index_and_to_an_empty_value` | +| P10 | BR-26 | The reason is stored trimmed | `Call Put "/api/csv/uploads/$u/rows/1/decision" @{decision="exclude"; reason=" spaces "}; (Call Get "/api/csv/uploads/$u/rows/1/history")[-1].reason` | `spaces` | `test_review_cases.py::test_P10_reason_is_stored_trimmed` | +| P11 | BR-26, BR-27 | The actor is the signed-in user | `(Call Get "/api/csv/uploads/$u/rows/2/history").actor` | `admin` | `test_review_cases.py::test_P11_actor_is_the_signed_in_user` | +| P12 | BR-36 | Every role (uploader, reviewer, tenant admin, auditor) can view rows and history | Automated only (no tool yet creates members with other roles) | 200 for each role | `test_review_cases.py::test_P12_every_role_can_view_rows_and_history` | +| P13 | BR-35 | Each change writes an audit row with before and after values, and the hash chain stays intact | `(Call Get "/api/audit/verify").intact` | `True` | `test_review.py::test_changes_are_audited_and_insert_only` | +| P14 | Decision 1 | The imported table doesn't change | `(Call Get "/api/csv/files").Where({ $_.upload_id -eq $u }).row_count` | `2` (rows 1 and 3, as imported at upload) | `test_review.py::test_cell_edits_are_versions_over_the_unchanged_original` | +| P15 | BR-35 | Review changes work through the runtime role `te_app` | On your PC, the API runs as `te_app` since M3d. P06 and P07 passing there is the check | P06 and P07 pass | `test_runtime_role.py::test_the_api_works_as_the_runtime_role` | +| P16 | — | The browser may send `PUT` (CORS preflight) | Use the Review screen from #143: Exclude a row | The change saves | `test_api_coverage.py::test_preflight_allows_put_for_review_changes` | +| P17 | BR-26 | An untouched row has an empty history | `(Call Get "/api/csv/uploads/$($f.uploadId)/rows/1/history").Count` | `0` | `test_review.py::test_refusals` | + +## Negative cases + +| ID | Requirement | What it checks | Manual command | Expected | Automated test | +|---|---|---|---|---|---| +| N01 | BR-26, decision 3 | A decision with a blank reason is refused | `Call Put "/api/csv/uploads/$u/rows/2/decision" @{decision="exclude"; reason=" "}` | `HTTP 422` | `test_review.py::test_refusals` | +| N02 | BR-27, decision 3 | An edit with an empty reason is refused | `Call Put "/api/csv/uploads/$u/rows/2/cells/n" @{value="5"; reason=""}` | `HTTP 422` | `test_review.py::test_refusals` | +| N03 | BR-26 | An unknown decision is refused | `Call Put "/api/csv/uploads/$u/rows/2/decision" @{decision="maybe"; reason="r"}` | `HTTP 422` | `test_review.py::test_refusals` | +| N04 | BR-27 | An edit that doesn't change the value is refused | `Call Put "/api/csv/uploads/$u/rows/2/cells/t" @{value="second"; reason="r"}` | `HTTP 422` | `test_review.py::test_refusals` | +| N05 | BR-26, BR-27 | Row 0 (the header) and rows past the end are refused | `Call Put "/api/csv/uploads/$u/rows/0/decision" @{decision="exclude"; reason="r"}; Call Put "/api/csv/uploads/$u/rows/4/cells/n" @{value="1"; reason="r"}` | `HTTP 404`, `HTTP 404` | `test_review_cases.py::test_N05_row_numbers_outside_the_upload` | +| N06 | BR-27 | An unknown column name or index is refused | `Call Put "/api/csv/uploads/$u/rows/1/cells/nope" @{value="1"; reason="r"}; Call Put "/api/csv/uploads/$u/rows/1/cells/2" @{value="1"; reason="r"}` | `HTTP 404`, `HTTP 404` | `test_review.py::test_refusals`, `test_review_cases.py::test_N06_column_index_out_of_range` | +| N07 | BR-25..27 | An unknown upload is refused on every route | `Call Get "/api/csv/uploads/999999999/rows"; Call Put "/api/csv/uploads/999999999/rows/1/decision" @{decision="exclude"; reason="r"}` | `HTTP 404`, `HTTP 404` | `test_review_cases.py::test_N07_unknown_upload_on_every_route` | +| N08 | BR-36, decision 2 | An uploader can't decide or edit, and nothing is recorded | Automated only (needs an uploader account) | 403 | `test_review_cases.py::test_N08_N09_uploader_and_auditor_cannot_change`, `test_roles.py` | +| N09 | BR-36, decision 2 | An auditor can't decide or edit | Automated only (needs an auditor account) | 403 | Same as N08 | +| N10 | BR-36 | An invalid session is refused | `$good = $h; $h = @{ Authorization = "Bearer not-a-real-token" }; Call Get "/api/csv/uploads/$u/rows"; $h = $good` | `HTTP 401` | `test_review_cases.py::test_N10_an_invalid_session_is_refused` | +| N11 | BR-36 | Another tenant can't see or change the upload, and the upload is unchanged | Optional: create a second tenant with `create_tenant_admin.py`, sign in as its admin, then `Call Get "/api/csv/uploads/$u/rows"` | `HTTP 404` | `test_review_cases.py::test_N11_another_tenant_cannot_see_or_change_the_upload` | +| N12 | — | A missing field, or a field of the wrong type, is refused | `Call Put "/api/csv/uploads/$u/rows/1/decision" @{decision="exclude"}` | `HTTP 422` | `test_review_cases.py::test_N12_missing_or_wrong_body_fields` | +| N13 | BR-35 | The review tables refuse UPDATE, DELETE and TRUNCATE, even for the owner | Automated only (needs direct SQL) | Refused with `insufficient_privilege` | `test_review.py::test_changes_are_audited_and_insert_only`, `test_review_cases.py::test_N13_review_tables_reject_update_delete_and_truncate` | +| N14 | Decision 3 | The database itself refuses a blank reason | Automated only (needs direct SQL) | Refused by a CHECK constraint | `test_review_cases.py::test_N14_database_rejects_a_blank_reason` | +| N15 | BR-35 | The runtime role can't change the review tables | Automated only (needs direct SQL as `te_app`) | Refused by privilege, not by the trigger | `test_runtime_role.py::test_runtime_role_cannot_change_or_unprotect_append_only_tables` | +| N16 | — | A non-numeric upload id is refused | `Call Get "/api/csv/uploads/abc/rows"` | `HTTP 422` | `test_review_cases.py::test_N16_a_non_numeric_upload_id` | + +## Not covered here +- **The review screen** (M4b, PR #143) has its own component tests (`review-table.test.tsx`) and the browser check `scripts/e2e/m4-review.js`. +- **T&E-mode uploads** store no column types, so their values are reviewed as text with no type checks. That's a known limit, noted on #142. +- **Applying review to the import** belongs to M6 (#110). It isn't tested here, because it isn't built yet. + +## Record of runs +| Date | Commit | Who | Automated | Manual | Notes | +|---|---|---|---|---|---| +| 28 Oct 2026 | PR #142 branch | Claude, in the cloud workspace (PostgreSQL 16, PowerShell 7) | Local CI replica, quoted on PR #142 | 23 of 23 commands gave the expected result | Run as test account `e2eadmin4`, so P11 showed that name. Not yet run on Windows PowerShell 5.1 | diff --git a/tests/test_review_cases.py b/tests/test_review_cases.py new file mode 100644 index 0000000..c173c96 --- /dev/null +++ b/tests/test_review_cases.py @@ -0,0 +1,233 @@ +"""Positive and negative test cases for the review API (M4a, #108). + +Each test names its case ID from docs/test-cases/M4-review.md. Cases already +covered elsewhere are mapped in that document; this file holds the rest. + +Integration: needs a live PostgreSQL. Fails (never skips) without one. Two +throwaway tenants with one member per role; dropped afterwards. +""" + +from __future__ import annotations + +import os +import unittest +import uuid + +import psycopg2.errors +import pytest +from fastapi.testclient import TestClient +from psycopg2 import sql + +from api.config import settings +from api.db import Conn +from api.main import app +from api.services import accounts +from api.services.accounts import ROLES +from api.tenancy import migrate_tenant, schema_for + +_PW = "review case passphrase" + + +@pytest.mark.integration +class ReviewCases(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.ctx = TestClient(app) + cls.client = cls.ctx.__enter__() + tag = f"{os.getpid()}_{uuid.uuid4().hex[:6]}" + cls.tenants = {"a": f"rca_{tag}", "b": f"rcb_{tag}"} + cls.users: dict[tuple[str, str], str] = {} + with Conn() as conn: + with conn.cursor() as cur: + for key, slug in cls.tenants.items(): + tid = accounts.create_tenant(cur, slug, f"Review cases {key}") + for role in ROLES: + name = f"{slug}_{role}" + accounts.add_membership(cur, accounts.create_user(cur, name, _PW), tid, role) + cls.users[(key, role)] = name + conn.commit() + cls.headers = {} + for key, slug in cls.tenants.items(): + migrate_tenant(slug) + for role in ROLES: + res = cls.client.post("/api/auth/login", json={ + "tenant": slug, "username": cls.users[(key, role)], "password": _PW}) + assert res.status_code == 200, res.text + cls.headers[(key, role)] = {"Authorization": f"Bearer {res.json()['token']}"} + + @classmethod + def tearDownClass(cls): + with Conn() as conn: + with conn.cursor() as cur: + for slug in cls.tenants.values(): + cur.execute(sql.SQL("DROP SCHEMA IF EXISTS {} CASCADE").format(sql.Identifier(schema_for(slug)))) + cur.execute(sql.SQL("DELETE FROM {}.tenants WHERE slug = %s").format( + sql.Identifier(settings.CONTROL_SCHEMA)), (slug,)) + cur.execute(sql.SQL("DELETE FROM {}.users WHERE username LIKE %s").format( + sql.Identifier(settings.CONTROL_SCHEMA)), (f"{slug}_%",)) + conn.commit() + cls.ctx.__exit__(None, None, None) + + # helpers ------------------------------------------------------------- + def _h(self, role: str = "tenant_admin", tenant: str = "a") -> dict: + return self.headers[(tenant, role)] + + def _upload(self, content: str, types: list[str], tenant: str = "a") -> dict: + tag = uuid.uuid4().hex[:10] + return self.client.post("/api/csv/upload", headers=self._h(tenant=tenant), json={ + "fileName": f"case_{tag}.csv", "content": content.replace("{tag}", tag), "types": types}).json() + + def _three(self, tenant: str = "a") -> str: + res = self._upload("n,t\n1,{tag}\nx,b\n3,c\n", ["int8", "text"], tenant) + assert res["status"] == "ok", res + return res["uploadId"] + + def _get(self, path: str, role: str = "tenant_admin", tenant: str = "a", **params): + return self.client.get(path, headers=self._h(role, tenant), params=params) + + def _put(self, path: str, body: dict, role: str = "tenant_admin", tenant: str = "a"): + return self.client.put(path, headers=self._h(role, tenant), json=body) + + def _decide(self, uid, row, decision="exclude", reason="case reason", role="tenant_admin", tenant="a"): + return self._put(f"/api/csv/uploads/{uid}/rows/{row}/decision", + {"decision": decision, "reason": reason}, role, tenant) + + def _edit(self, uid, row, column, value, reason="case reason", role="tenant_admin", tenant="a"): + return self._put(f"/api/csv/uploads/{uid}/rows/{row}/cells/{column}", + {"value": value, "reason": reason}, role, tenant) + + # positive ------------------------------------------------------------ + def test_P02_paging_bounds(self): + uid = self._three() + big = self._get(f"/api/csv/uploads/{uid}/rows", limit=10_000).json() + self.assertEqual((big["limit"], len(big["rows"])), (500, 3)) + self.assertEqual(self._get(f"/api/csv/uploads/{uid}/rows", offset=3).json()["rows"], []) + neg = self._get(f"/api/csv/uploads/{uid}/rows", offset=-5).json() + self.assertEqual((neg["offset"], neg["rows"][0]["rowNumber"]), (0, 1)) + + def test_P04_ragged_row_is_flagged(self): + res = self._upload("a,b\n1,{tag}\n2,y,extra\n", ["text", "text"]) + row = self._get(f"/api/csv/uploads/{res['uploadId']}/rows").json()["rows"][1] + self.assertEqual(row["errors"][0]["column"], "(beyond header)") + self.assertIn("extra value(s) would be lost", row["errors"][0]["reason"]) + + def test_P05_a_failed_upload_can_be_reviewed(self): + res = self._upload("n\nbad_{tag}\nworse\n", ["int8"]) + self.assertEqual(res["status"], "error", res) + uid = res["uploadId"] + page = self._get(f"/api/csv/uploads/{uid}/rows").json() + self.assertEqual((page["total"], [len(r["errors"]) for r in page["rows"]]), (2, [1, 1])) + self.assertEqual(self._edit(uid, 1, "n", "1").json()["errors"], []) + self.assertEqual(self._decide(uid, 2).json()["decision"], "exclude") + + def test_P08_an_edit_to_a_bad_value_is_flagged(self): + uid = self._three() + row = self._edit(uid, 1, "n", "one").json() + self.assertEqual(row["errors"][0]["reason"], '"one" is not a whole number') + + def test_P09_edit_by_column_index_and_to_an_empty_value(self): + uid = self._three() + self.assertEqual(self._edit(uid, 1, "1", "renamed").json()["current"][1], "renamed") + self.assertEqual(self._edit(uid, 3, "t", "").json()["current"], ["3", ""]) + + def test_P10_reason_is_stored_trimmed(self): + uid = self._three() + self._decide(uid, 1, reason=" needs a second look ") + [entry] = self._get(f"/api/csv/uploads/{uid}/rows/1/history").json() + self.assertEqual(entry["reason"], "needs a second look") + + def test_P11_actor_is_the_signed_in_user(self): + uid = self._three() + self._decide(uid, 1, role="reviewer") + self._edit(uid, 2, "n", "2", role="tenant_admin") + actors = {h["kind"]: h["actor"] for r in (1, 2) + for h in self._get(f"/api/csv/uploads/{uid}/rows/{r}/history").json()} + self.assertEqual(actors, {"decision": self.users[("a", "reviewer")], + "edit": self.users[("a", "tenant_admin")]}) + + def test_P12_every_role_can_view_rows_and_history(self): + uid = self._three() + for role in ROLES: + with self.subTest(role=role): + self.assertEqual(self._get(f"/api/csv/uploads/{uid}/rows", role).status_code, 200) + self.assertEqual(self._get(f"/api/csv/uploads/{uid}/rows/1/history", role).status_code, 200) + + # negative ------------------------------------------------------------ + def test_N05_row_numbers_outside_the_upload(self): + uid = self._three() + for row in (0, 4): + with self.subTest(row=row): + self.assertEqual(self._decide(uid, row).status_code, 404) + self.assertEqual(self._edit(uid, row, "n", "9").status_code, 404) + self.assertEqual(self._get(f"/api/csv/uploads/{uid}/rows/{row}/history").status_code, 404) + + def test_N06_column_index_out_of_range(self): + uid = self._three() + self.assertEqual(self._edit(uid, 1, "2", "x").status_code, 404) + + def test_N07_unknown_upload_on_every_route(self): + for res in (self._decide(999_999_999, 1), self._edit(999_999_999, 1, "n", "1"), + self._get("/api/csv/uploads/999999999/rows/1/history")): + with self.subTest(url=str(res.url)): + self.assertEqual(res.status_code, 404) + + def test_N08_N09_uploader_and_auditor_cannot_change(self): + uid = self._three() + for role in ("uploader", "auditor"): + with self.subTest(role=role): + self.assertEqual(self._decide(uid, 1, role=role).status_code, 403) + self.assertEqual(self._edit(uid, 1, "n", "5", role=role).status_code, 403) + self.assertEqual(self._get(f"/api/csv/uploads/{uid}/rows/1/history").json(), [], "nothing recorded") + + def test_N10_an_invalid_session_is_refused(self): + uid = self._three() + bad = {"Authorization": "Bearer not-a-real-token"} + self.assertEqual(self.client.get(f"/api/csv/uploads/{uid}/rows", headers=bad).status_code, 401) + self.assertEqual(self.client.put(f"/api/csv/uploads/{uid}/rows/1/decision", headers=bad, + json={"decision": "exclude", "reason": "x"}).status_code, 401) + + def test_N11_another_tenant_cannot_see_or_change_the_upload(self): + uid = self._three(tenant="a") + self.assertEqual(self._get(f"/api/csv/uploads/{uid}/rows", tenant="b").status_code, 404) + self.assertEqual(self._decide(uid, 1, tenant="b").status_code, 404) + self.assertEqual(self._edit(uid, 1, "n", "7", tenant="b").status_code, 404) + page = self._get(f"/api/csv/uploads/{uid}/rows").json() + self.assertEqual((page["excludedRows"], page["editedRows"]), (0, 0), "tenant a's upload is unchanged") + + def test_N12_missing_or_wrong_body_fields(self): + uid = self._three() + for path, body in ((f"/api/csv/uploads/{uid}/rows/1/decision", {"decision": "exclude"}), + (f"/api/csv/uploads/{uid}/rows/1/decision", {"reason": "r"}), + (f"/api/csv/uploads/{uid}/rows/1/cells/n", {"reason": "r"}), + (f"/api/csv/uploads/{uid}/rows/1/cells/n", {"value": 5, "reason": "r"})): + with self.subTest(body=body): + self.assertEqual(self._put(path, body).status_code, 422) + + def test_N13_review_tables_reject_update_delete_and_truncate(self): + uid = self._three() + self._decide(uid, 1) + self._edit(uid, 2, "n", "2") + s = sql.Identifier(schema_for(self.tenants["a"])) + for statement in ("UPDATE {}.row_decisions SET reason = 'x'", "DELETE FROM {}.row_decisions", + "TRUNCATE {}.row_decisions", "UPDATE {}.cell_edits SET new_value = 'x'", + "DELETE FROM {}.cell_edits", "TRUNCATE {}.cell_edits"): + with self.subTest(statement=statement), Conn() as conn, conn.cursor() as cur: + with self.assertRaises(psycopg2.errors.InsufficientPrivilege): + cur.execute(sql.SQL(statement).format(s)) + conn.rollback() + + def test_N14_database_rejects_a_blank_reason(self): + uid = self._three() + s = sql.Identifier(schema_for(self.tenants["a"])) + for statement, args in ( + ("INSERT INTO {}.row_decisions (upload_id, row_number, decision, reason) VALUES (%s, 1, 'exclude', ' ')", + (int(uid),)), + ("INSERT INTO {}.cell_edits (upload_id, row_number, column_index, old_value, new_value, reason) " + "VALUES (%s, 1, 0, '1', '2', '')", (int(uid),))): + with self.subTest(statement=statement[:40]), Conn() as conn, conn.cursor() as cur: + with self.assertRaises(psycopg2.errors.CheckViolation): + cur.execute(sql.SQL(statement).format(s), args) + conn.rollback() + + def test_N16_a_non_numeric_upload_id(self): + self.assertEqual(self._get("/api/csv/uploads/abc/rows").status_code, 422) diff --git a/tests/test_runtime_role.py b/tests/test_runtime_role.py index 5699e26..0ac0858 100644 --- a/tests/test_runtime_role.py +++ b/tests/test_runtime_role.py @@ -78,6 +78,13 @@ def test_the_api_works_as_the_runtime_role(self): over = self.client.post("/api/csv/upload", json={ "fileName": name, "content": f"v\nnew_{name}\n", "overwrite": True}).json() self.assertEqual(over["status"], "ok", over) + # P15 (docs/test-cases/M4-review.md): review changes work through the runtime role. + decided = self.client.put(f"/api/csv/uploads/{up['uploadId']}/rows/1/decision", + json={"decision": "exclude", "reason": "runtime role case"}) + self.assertEqual(decided.status_code, 200, decided.text) + edited = self.client.put(f"/api/csv/uploads/{up['uploadId']}/rows/1/cells/v", + json={"value": "changed", "reason": "runtime role case"}) + self.assertEqual(edited.status_code, 200, edited.text) self.assertEqual(self.client.delete(f"/api/csv/files/{over['fileId']}").status_code, 200) self.assertTrue(self.client.get("/api/audit/verify").json()["intact"]) with Conn() as conn, conn.cursor() as cur: # the API's own request pool @@ -86,9 +93,10 @@ def test_the_api_works_as_the_runtime_role(self): def test_runtime_role_cannot_change_or_unprotect_append_only_tables(self): schema = settings.UPLOADS_SCHEMA - statements = [f"{verb} {{}}.{table}" for table in ("audit_log", "uploads", "upload_blobs", "source_rows") - for verb in ("DELETE FROM", "TRUNCATE")] + tables = ("audit_log", "uploads", "upload_blobs", "source_rows", "row_decisions", "cell_edits") + statements = [f"{verb} {{}}.{table}" for table in tables for verb in ("DELETE FROM", "TRUNCATE")] statements += ["UPDATE {}.audit_log SET actor = 'x'", "UPDATE {}.uploads SET status = 'failed'", + "UPDATE {}.row_decisions SET reason = 'x'", "UPDATE {}.cell_edits SET new_value = 'x'", # N15 "ALTER TABLE {}.audit_log DISABLE TRIGGER audit_no_change"] conn = self._as_runtime() try: