From 3382641a9e7e59da2608030defb89e6596798016 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 23:23:18 +0000 Subject: [PATCH 1/2] fix(review): T&E review shows why the database rejected a row (#190) T&E uploads stored an empty column-type list, so review fell back to text and returned errors: [] for rows a CHECK/FK/UNIQUE rejected. - te_loader returns the target's mapped column types, so review revalidates T&E values against them (unmapped types stay text). - review attaches the stored upload-time database reason as a "(database)" error on unedited T&E rows. - review table shows any error not tied to a column beside the row. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY --- api/services/review.py | 22 ++++++++++++----- api/services/te_loader.py | 4 +++- .../src/__tests__/review-table.test.tsx | 8 ++++++- .../src/components/review-table.tsx | 3 ++- tests/test_review.py | 24 +++++++++++++++++++ 5 files changed, 52 insertions(+), 9 deletions(-) diff --git a/api/services/review.py b/api/services/review.py index cb2a5a7..f8f8e25 100644 --- a/api/services/review.py +++ b/api/services/review.py @@ -31,18 +31,24 @@ def _s(): def _upload(cur, upload_id: int) -> dict: cur.execute(sql.SQL( - "SELECT u.file_name, u.columns, u.column_types, h.cells, " + "SELECT u.file_name, u.columns, u.column_types, h.cells, u.mode, u.row_errors, " " (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 + name, columns, types, header, mode, stored_errors, 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. + # Uploads stored before 0008, or T&E uploads before #190, 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} + # #190: T&E rows the database rejected (CHECK, FK, UNIQUE, ...). Errors with a column are + # type failures, which _validate finds again; these carry only the database's reason. + # ponytail: stored list is capped at MAX_ROW_ERRORS_REPORTED, rows past the cap show no + # database reason; edited rows aren't re-checked against constraints until M6's import. + db_errors = {e["rowNumber"]: e["reason"] for e in (stored_errors or []) + if mode == "te" and "column" not in e and "rowNumber" in e} + return {"fileName": name, "columns": columns, "types": types, "total": total, "_dbErrors": db_errors} def _validate(cells: list[str], raw: list[str], columns: list[str], types: list[str]) -> list[dict]: @@ -92,12 +98,15 @@ def _rows(cur, upload_id: int, meta: dict, numbers: list[int] | None = None, original = (list(raw) + [""] * width)[:width] current = [edits.get(n, {}).get(c, v) for c, v in enumerate(original)] decision = decisions.get(n) + errors = _validate(current, raw, meta["columns"], meta["types"]) + if n in meta["_dbErrors"] and n not in edits: + errors.append({"column": "(database)", "value": "", "reason": meta["_dbErrors"][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"]), + "errors": errors, }) return out @@ -111,7 +120,8 @@ def page(cur, upload_id: int, offset: int, limit: int) -> dict: " (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, + public = {k: v for k, v in meta.items() if k != "_dbErrors"} + return {**public, "offset": offset, "limit": limit, "excludedRows": excluded, "editedRows": edited, "rows": _rows(cur, upload_id, meta, offset=offset, limit=limit)} diff --git a/api/services/te_loader.py b/api/services/te_loader.py index be213ce..594e01f 100644 --- a/api/services/te_loader.py +++ b/api/services/te_loader.py @@ -332,7 +332,9 @@ def _err(row_number: int, column: str, value: str, reason: str) -> None: "duplicateRowsSkipped": 0, "failedRows": failed_row_count, "columns": columns, - "types": [], + # #190: stored with the upload so review revalidates against the target's types. + # Columns Postgres casts itself (uuid, enums, ...) review as text. + "types": [t or "text" for t in col_types], "rowErrors": row_errors, "logs": logs, } diff --git a/csv-table-hub-main/src/__tests__/review-table.test.tsx b/csv-table-hub-main/src/__tests__/review-table.test.tsx index c208e0e..19a03f5 100644 --- a/csv-table-hub-main/src/__tests__/review-table.test.tsx +++ b/csv-table-hub-main/src/__tests__/review-table.test.tsx @@ -32,7 +32,11 @@ const page = { row(2, ["x", "b"], { errors: [{ column: "n", value: "x", reason: '"x" is not a whole number' }], }), - row(3, ["3", "c"], { decision: "exclude", decisionReason: "duplicate of row 1" }), + row(3, ["3", "c"], { + decision: "exclude", + decisionReason: "duplicate of row 1", + errors: [{ column: "(database)", value: "", reason: "violates check constraint" }], + }), ], }; @@ -82,6 +86,8 @@ it("shows every row with its validation, edits and decision", async () => { ); expect(screen.getByText("Invalid")).toBeInTheDocument(); expect(screen.getByText("Excluded")).toHaveAttribute("title", "duplicate of row 1"); + // #190: a reason not tied to a column (the database rejected the row) is shown beside it. + expect(screen.getByText("violates check constraint")).toBeInTheDocument(); }); it("a reviewer excludes a row with a required reason", async () => { diff --git a/csv-table-hub-main/src/components/review-table.tsx b/csv-table-hub-main/src/components/review-table.tsx index 2e21b15..8924aa4 100644 --- a/csv-table-hub-main/src/components/review-table.tsx +++ b/csv-table-hub-main/src/components/review-table.tsx @@ -161,7 +161,8 @@ function ReviewLine({ }) { const excluded = row.decision === "exclude"; const errorFor = (name: string) => row.errors.find((e) => e.column === name); - const extra = errorFor("(beyond header)"); + // Reasons not tied to a column: "(beyond header)" cells, or "(database)" for a row the target rejected (#190). + const extra = row.errors.find((e) => !columns.includes(e.column)); return ( diff --git a/tests/test_review.py b/tests/test_review.py index 92a159a..1b40730 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -109,6 +109,30 @@ def test_refusals(self): self.assertEqual(res.status_code, code) self.assertEqual(self.client.get(f"/api/csv/uploads/{uid}/rows/2/history").json(), []) + def test_te_review_shows_the_database_reason_a_row_was_rejected(self): + # #190: T&E uploads stored no types, so review said errors: [] for a row the CHECK rejected. + good, bad = f"TE_REV_{self.tag}_ok", f"TE_REV_{self.tag}_bad" + res = self.client.post("/api/csv/upload", json={ + "fileName": f"te_rev_{self.tag}.csv", "mode": "te", "targetTable": "organisations", + "content": f"name,org_type,country\n{good},government,AU\n{bad},not_a_type,AU\n"}).json() + try: + self.assertEqual(res["failedRows"], 1, res) + page = self._rows(res["uploadId"]) + self.assertEqual(len(page["types"]), 3) + ok_row, bad_row = page["rows"] + self.assertEqual(ok_row["errors"], []) + [err] = bad_row["errors"] + self.assertEqual(err["column"], "(database)") + self.assertIn("org_type", err["reason"]) + # Once a reviewer edits the row, the upload-time reason no longer describes it. + self.assertEqual(self._edit(res["uploadId"], 2, "org_type", "prime").status_code, 200) + self.assertEqual(self._rows(res["uploadId"])["rows"][1]["errors"], []) + finally: + with Conn() as conn, conn.cursor() as cur: + cur.execute(sql.SQL("DELETE FROM {}.organisations WHERE name = ANY(%s)").format( + sql.Identifier(settings.TE_SCHEMA)), ([good, bad],)) + conn.commit() + def test_changes_are_audited_and_insert_only(self): uid = self._three_rows() self._decide(uid, 3, "exclude") From 99a63c591201670bdeb8187338f82d45c734d407 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 04:03:45 +0000 Subject: [PATCH 2/2] test(#190): T&E response now carries one type per column The contract test still expected types: [] for T&E uploads. Update it, and the M4 test-case note that described the old "reviewed as text" limit. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_011T6Wqi8yaiaYWUVH8C4QAY --- docs/test-cases/M4-review.md | 2 +- tests/test_api_coverage.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/test-cases/M4-review.md b/docs/test-cases/M4-review.md index bb5a060..ee4a634 100644 --- a/docs/test-cases/M4-review.md +++ b/docs/test-cases/M4-review.md @@ -88,7 +88,7 @@ $u = $up.uploadId ## 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. +- **T&E-mode uploads** store the target table's column types (#190), and review shows the database's reason for rows it rejected (CHECK, FK, UNIQUE) until the row is edited. Edited T&E rows are re-checked against types only; constraints are checked at M6's import. Uploads made before #190 have no stored types and review as text. - **Applying review to the import** belongs to M6 (#110). It isn't tested here, because it isn't built yet. ## Record of runs diff --git a/tests/test_api_coverage.py b/tests/test_api_coverage.py index a04b006..140d37c 100644 --- a/tests/test_api_coverage.py +++ b/tests/test_api_coverage.py @@ -389,7 +389,8 @@ def test_response_shape_matches_contract(self): "columns", "rowErrors"): self.assertIn(key, body, f"missing key '{key}'") self.assertEqual(body["duplicateRowsSkipped"], 0) - self.assertEqual(body["types"], []) + # #190: one reviewable type per column (organisations' columns are all text). + self.assertEqual(body["types"], ["text"] * len(body["columns"])) def test_column_mismatch_returns_error(self): csv = "bogus_col,fake_col\nval1,val2\n"