spec: drop claims UNIQUE constraint + 409 UX

2026-06-21 16:54:07 -06:00
1 changed files with 343 additions and 0 deletions
@@ -0,0 +1,343 @@
+# Drop `UNIQUE(batch_id, patient_control_number)` on Claims + Robust 409 UX
+
+**Date:** 2026-06-21
+**Branch:** `main`
+**Status:** Draft (brainstorming approved, awaiting writing-plans)
+**Scope:** Backend schema + minimal frontend error handling for collision 409.
+
+---
+
+## 1. Why this exists
+
+Today, every multi-claim 837P file in `docs/prodfiles/837p-from-axiscare/` and
+`docs/prodfiles/FromHPE/` returns **HTTP 409** on upload. The cause:
+
+* The `claims` table has `UNIQUE(batch_id, patient_control_number)` declared
+  inline in `CREATE TABLE` (auto-index `sqlite_autoindex_claims_2`).
+* `store._claim_837_row` populates `patient_control_number` from
+  `claim.subscriber.member_id` (the subscriber's insurance member id),
+  not from the CLM01 segment value.
+* A real 837P submission routinely contains many `CLM*` segments per
+  subscriber (a member seeing multiple providers on the same day). All
+  those rows share the same `member_id` → same `patient_control_number`.
+  Within one batch, the constraint fires on claim #2.
+
+Migration `0003_drop_claims_remits_unique_constraints.sql` already exists
+with the right intent but is **buggy**: it does
+`DROP INDEX IF EXISTS uq_claims_batch_pcn`, but the constraint is inline,
+not a named index — so the `DROP INDEX` is a no-op and the constraint
+survives.
+
+The 409 error message ("duplicate CLM01 control numbers") is also
+misleading because the column stores `member_id`, not CLM01.
+
+This SP fixes both: drops the constraint properly via SQLite table
+recreation, and gives the upload page a structured error panel for the
+collision case so the operator can find the existing batch in one click.
+
+---
+
+## 2. Operator surface
+
+| Surface | Change |
+|---|---|
+| DB | New migration `0013_drop_claims_unique_constraint.sql`; idempotent. |
+| Python | `store.find_existing_batch_for_claim(claim_id)` new helper. |
+| API | `POST /api/parse-837` and `/api/parse-835` 409 responses gain `existing_batch_id`. |
+| UI | `/upload` page renders an inline error panel for 409 with a link to the existing batch. |
+| Tests | New migration test, store helper test, API test, component test. |
+
+No CLI / settings changes.
+
+---
+
+## 3. Schema migration
+
+`backend/src/cyclone/migrations/0013_drop_claims_unique_constraint.sql`:
+
+```sql
+-- version: 13
+-- Drop the inline UNIQUE(batch_id, patient_control_number) on claims.
+-- Migration 0003 attempted DROP INDEX IF EXISTS uq_claims_batch_pcn but
+-- the constraint is inline in CREATE TABLE, so the drop was a no-op.
+-- The only way to remove an inline UNIQUE in SQLite is table recreation.
+--
+-- X12 837P allows any number of CLM segments per 2000B subscriber loop;
+-- claim identity is provided by the primary key (claims.id = CLM01).
+-- The remittances table had a parallel constraint already removed in 0003
+-- (because that one WAS a named index), so this migration only touches
+-- claims.
+
+PRAGMA foreign_keys=OFF;
+BEGIN;
+
+CREATE TABLE claims_new (
+    id TEXT PRIMARY KEY,
+    batch_id TEXT NOT NULL REFERENCES batches(id) ON DELETE CASCADE,
+    patient_control_number TEXT NOT NULL,
+    service_date_from DATE,
+    service_date_to DATE,
+    charge_amount NUMERIC(12, 2) NOT NULL DEFAULT 0,
+    provider_npi TEXT,
+    payer_id TEXT,
+    state TEXT NOT NULL DEFAULT 'submitted',
+    state_before_reversal TEXT,
+    matched_remittance_id TEXT REFERENCES remittances(id),
+    raw_json TEXT,
+    rejection_reason TEXT,
+    rejected_at TIMESTAMP,
+    resubmit_count INTEGER NOT NULL DEFAULT 0,
+    state_changed_at TIMESTAMP,
+    payer_rejected_at TEXT,
+    payer_rejected_reason TEXT,
+    payer_rejected_status_code TEXT,
+    payer_rejected_by_277ca_id TEXT,
+    payer_rejected_acknowledged_at TEXT,
+    payer_rejected_acknowledged_actor TEXT
+    -- NO UNIQUE (batch_id, patient_control_number) — removed.
+);
+
+INSERT INTO claims_new SELECT * FROM claims;
+DROP TABLE claims;
+ALTER TABLE claims_new RENAME TO claims;
+
+-- Recreate secondary indexes (same names, same columns as initial schema
+-- plus later migrations).
+CREATE INDEX ix_claims_state ON claims(state);
+CREATE INDEX ix_claims_patient_control_number ON claims(patient_control_number);
+CREATE INDEX ix_claims_service_date_from ON claims(service_date_from);
+CREATE INDEX ix_claims_state_changed_at ON claims(state, state_changed_at);
+CREATE INDEX idx_claims_payer_rejected_at ON claims(payer_rejected_at);
+CREATE INDEX idx_claims_payer_rejected_unack
+    ON claims(payer_rejected_at)
+    WHERE payer_rejected_acknowledged_at IS NULL;
+
+COMMIT;
+PRAGMA foreign_keys=ON;
+```
+
+**Note on the `db.py` ORM model:** `backend/src/cyclone/db.py` declares
+the `Claim` ORM model with `UNIQUE(batch_id, patient_control_number)` in
+`__table_args__`. After this migration the DB no longer enforces that
+constraint, so the ORM declaration becomes aspirational. We leave it in
+place as documentation (and as a guard if the DB ever gets rebuilt from
+ORM on a fresh schema). No code reads through it; the dedup happens via
+PK on `claims.id` in `store.add`.
+
+---
+
+## 4. Store helper
+
+New function in `backend/src/cyclone/store.py`:
+
+```python
+def find_existing_batch_for_claim(claim_id: str) -> str | None:
+    """Return the batch_id of the first batch containing this CLM01, or None."""
+    from cyclone import db
+    from cyclone.db import Claim
+    with db.SessionLocal()() as s:
+        row = s.execute(
+            select(Claim.batch_id).where(Claim.id == claim_id).limit(1)
+        ).first()
+        return row[0] if row else None
+
+
+def find_existing_batch_for_remit(payer_claim_control_number: str) -> str | None:
+    """Return the batch_id of the first batch containing this CLP01, or None."""
+    from cyclone import db
+    from cyclone.db import Remittance
+    with db.SessionLocal()() as s:
+        row = s.execute(
+            select(Remittance.batch_id)
+            .where(Remittance.id == payer_claim_control_number)
+            .limit(1)
+        ).first()
+        return row[0] if row else None
+```
+
+Both pure reads; no transaction management needed.
+
+---
+
+## 5. API change
+
+`backend/src/cyclone/api.py`:
+
+### 837 path (line 394-415)
+
+```python
+except IntegrityError as exc:
+    existing_batch_id = store.find_existing_batch_for_claim(...)
+    body = {
+        "error": "Duplicate claim",
+        "detail": "...",
+        "batch_id": rec.id,
+    }
+    if existing_batch_id:
+        body["existing_batch_id"] = existing_batch_id
+    log.warning("Duplicate claim while persisting batch %s: %s", rec.id, exc)
+    return JSONResponse(status_code=409, content=body)
+```
+
+The `...` argument is the first `claim.claim_id` from `result.claims`
+where the collision occurred. We pick the first one because the DB raises
+on the second insert; iterating through the parser's claim list to find
+which CLM01 already existed is cheap.
+
+### 835 path (line 588-602)
+
+Same pattern with `find_existing_batch_for_remit` and the existing remit's
+`payer_claim_control_number`.
+
+---
+
+## 6. Frontend change
+
+### `src/lib/api.ts`
+
+`ApiError` carries an optional `existingBatchId`:
+
+```typescript
+export class ApiError extends Error {
+  constructor(
+    public status: number,
+    message: string,
+    public existingBatchId: string | null = null,
+  ) {
+    super(message);
+  }
+}
+```
+
+`readErrorBody()` parses JSON and, when status ≥ 400, attempts to extract
+`existing_batch_id` and pass it through. `parse837`/`parse835` throw
+`new ApiError(res.status, detail, existingBatchId)`.
+
+### `src/pages/Upload.tsx`
+
+New local state:
+
+```typescript
+type UploadError = {
+  kind: "duplicate";
+  existingBatchId: string | null;
+  filename: string;
+};
+const [uploadError, setUploadError] = useState<UploadError | null>(null);
+```
+
+On mutation error:
+
+```typescript
+} catch (err) {
+  if (err instanceof ApiError && err.status === 409) {
+    setUploadError({
+      kind: "duplicate",
+      existingBatchId: err.existingBatchId,
+      filename: file.name,
+    });
+    toast.error("Duplicate claim — file not ingested");
+  } else {
+    toast.error(err instanceof Error ? err.message : "Failed to parse file");
+  }
+}
+```
+
+Inline error panel JSX (renders above streaming results when
+`uploadError` is set):
+
+```tsx
+{uploadError ? (
+  <div className="error-panel">
+    <Badge variant="destructive">409</Badge>
+    <div className="error-title">Duplicate claim — file not ingested</div>
+    <div className="error-detail">
+      {filename} collides with an existing record.
+      {existingBatchId
+        ? " Open the existing batch to compare, or pick a different file."
+        : " Pick a different file."}
+    </div>
+    <div className="error-actions">
+      {existingBatchId ? (
+        <Button onClick={() => navigate(`/batches/${existingBatchId}`)}>
+          Open existing batch →
+        </Button>
+      ) : null}
+      <Button variant="ghost" onClick={() => { pickFile(null); }}>
+        Pick a different file
+      </Button>
+    </div>
+  </div>
+) : null}
+```
+
+Styling matches the existing paper-toned surface (slate/parchment) per
+the hybrid dark/paper treatment used elsewhere in the app.
+
+---
+
+## 7. Files changed
+
+* `backend/src/cyclone/migrations/0013_drop_claims_unique_constraint.sql` — new
+* `backend/src/cyclone/store.py` — `find_existing_batch_for_claim`,
+  `find_existing_batch_for_remit`
+* `backend/src/cyclone/api.py` — both 409 handlers add
+  `existing_batch_id` lookup
+* `src/lib/api.ts` — `ApiError.existingBatchId`
+* `src/pages/Upload.tsx` — error state + inline panel
+* `src/pages/Upload.test.tsx` — new test file
+* `backend/tests/test_api_parse_persists.py` — new test cases
+* `backend/tests/test_db_migrate.py` — new migration test
+* `backend/tests/test_store.py` — new helper tests
+
+No new dependencies. No config changes.
+
+---
+
+## 8. Out of scope
+
+* A "Replace existing batch" destructive action — requires
+  `DELETE /api/batches/{id}` and reconciliation cascade handling; deferred.
+* General 4xx error UI for other statuses (empty file, parse error,
+  validation errors). Those already surface in toasts and the streaming
+  view; a future SP can generalize the inline panel pattern.
+* Renaming `patient_control_number` to `subscriber_member_id` to match
+  what it actually stores. Out of scope for this fix; tracked separately
+  if it becomes a source of confusion.
+
+---
+
+## 9. Risk
+
+* **Migration irreversibility**: SQLite has no `DROP CONSTRAINT`; the
+  recreation is destructive to schema (but not to data — `INSERT INTO
+  claims_new SELECT * FROM claims` preserves every row). If the
+  migration fails mid-way, the transaction rolls back. The
+  `PRAGMA foreign_keys=OFF` block is required because SQLite otherwise
+  can't drop and rename a table that other tables reference.
+* **Loss of uniqueness**: after the migration, two claims in one batch
+  *can* share a `patient_control_number`. This is the intended behavior.
+  Claim identity is still unique via `claims.id` (CLM01, PK) and the
+  existing dedup check in `store.add` (`s.get(Claim, claim.claim_id)`).
+* **`existing_batch_id` race**: the helper runs after the failed
+  `store.add` transaction has rolled back. Between rollback and helper
+  call, another writer could delete the colliding claim. Result: 409
+  fires with no `existing_batch_id`; UI shows panel without link.
+  Acceptable — the panel still explains the collision and offers
+  "Pick a different file."
+
+---
+
+## 10. Test plan
+
+| Test | Asserts |
+|---|---|
+| `test_db_migrate.py::test_drop_claims_unique_constraint` | After running migrations from v12, `user_version=13`, no `*claims*unique*` index exists, two rows with same `(batch_id, patient_control_number)` insert cleanly. |
+| `test_db_migrate.py::test_migration_idempotent` | Running migrations on a v13 DB is a no-op. |
+| `test_store.py::test_find_existing_batch_for_claim` | Helper returns None for unknown, returns batch_id for known, deterministic on duplicates. |
+| `test_api_parse_persists.py::test_409_response_includes_existing_batch_id` | Upload duplicate; assert body has `existing_batch_id` pointing to the right batch. |
+| `test_api_parse_persists.py::test_multi_claim_batch_with_duplicate_member_id_succeeds` | Upload a file with duplicate `member_id` claims; assert 200, all claims persisted. |
+| `Upload.test.tsx::test_error_panel_renders_on_409_with_link` | Mock `useParse` to throw `ApiError(409, ..., existingBatchId)`; assert panel visible, link present. |
+| `Upload.test.tsx::test_error_panel_renders_on_409_without_link` | Mock 409 with `existingBatchId: null`; assert panel visible, no link. |
+| `Upload.test.tsx::test_no_panel_on_non_409` | Mock 400; assert panel absent, only toast. |
+| `Upload.test.tsx::test_pick_different_clears_error` | Click button; assert `pickFile(null)` called and `errorState` cleared. |