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. |