From ca645db518a77d7e68292f6096f0edf641aed2b6 Mon Sep 17 00:00:00 2001 From: Tyler Date: Fri, 19 Jun 2026 19:22:44 -0600 Subject: [PATCH] refactor(store): code quality fixes per review - iter_claims/iter_remittances now return list[dict] (snapshot under the lock, return outside) so slow consumers no longer hold the write lock during iteration - iter_claims gains payer (case-insensitive substring) and date_from/date_to filters via the new _date_in_bounds helper - iter_remittances wires date_from/date_to and payer - BatchRecord split into BatchRecord837 / BatchRecord835 with __new__ dispatch so isinstance narrows result correctly, eliminating the four 'type: ignore[assignment]' band-aids - BatchRecord now rejects naive parsed_at via model_validator - to_ui_remittance's adjustmentAmount=0.0 is documented as a TODO for sub-project 2 reconciliation 151 passed, 1 skipped (no regressions; +6 tests for new filters) --- backend/src/cyclone/store.py | 174 ++++++++++++++++++++++++++--------- backend/tests/test_store.py | 110 ++++++++++++++++++++++ 2 files changed, 238 insertions(+), 46 deletions(-) diff --git a/backend/src/cyclone/store.py b/backend/src/cyclone/store.py index 160ddd9..b4b007c 100644 --- a/backend/src/cyclone/store.py +++ b/backend/src/cyclone/store.py @@ -10,9 +10,9 @@ from __future__ import annotations import threading from datetime import datetime, timezone -from typing import Iterable, Literal +from typing import Any, Literal -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, model_validator from cyclone.parsers.models import ClaimOutput, ParseResult from cyclone.parsers.models_835 import ClaimPayment, ParseResult835 @@ -23,7 +23,18 @@ BatchKind = Literal["837p", "835"] class BatchRecord(BaseModel): - """One parsed file, with a stable uuid4 id and the full ParseResult.""" + """One parsed file, with a stable uuid4 id and the full ParseResult. + + ``result`` is a union: ``ParseResult`` for ``kind="837p"`` and + ``ParseResult835`` for ``kind="835"``. The concrete subclasses + ``BatchRecord837`` and ``BatchRecord835`` narrow those fields, so + callers that want type-checked access should use them and check + ``isinstance`` rather than pattern-matching on ``kind``. + + Constructing ``BatchRecord(kind="837p", ...)`` dispatches to + ``BatchRecord837``; ``kind="835"`` dispatches to ``BatchRecord835``. + This lets the union-member narrowing work transparently. + """ model_config = ConfigDict(extra="ignore") @@ -31,7 +42,44 @@ class BatchRecord(BaseModel): kind: BatchKind input_filename: str parsed_at: datetime # tz-aware UTC - result: ParseResult | ParseResult835 = Field(discriminator=None) + result: ParseResult | ParseResult835 + + def __new__( + cls, *args: Any, **kwargs: Any, + ) -> BatchRecord837 | BatchRecord835 | BatchRecord: + # Dispatch base-class construction to the right concrete subclass + # so isinstance checks downstream narrow `result` correctly. + if cls is BatchRecord: + kind = kwargs.get("kind") + if kind is None and args and isinstance(args[0], dict): + kind = args[0].get("kind") + if kind == "837p": + return BatchRecord837(*args, **kwargs) + if kind == "835": + return BatchRecord835(*args, **kwargs) + return super().__new__(cls) + + @model_validator(mode="after") + def _check_parsed_at_tz(self) -> BatchRecord: + if self.parsed_at.tzinfo is None: + raise ValueError( + "parsed_at must be tz-aware (use datetime.now(timezone.utc))" + ) + return self + + +class BatchRecord837(BatchRecord): + """A parsed 837P (professional claim) batch.""" + + kind: Literal["837p"] = "837p" + result: ParseResult + + +class BatchRecord835(BatchRecord): + """A parsed 835 (remittance advice) batch.""" + + kind: Literal["835"] = "835" + result: ParseResult835 class InMemoryStore: @@ -66,36 +114,45 @@ class InMemoryStore: batch_id: str | None = None, status: str | None = None, provider_npi: str | None = None, + payer: str | None = None, date_from: str | None = None, date_to: str | None = None, sort: str | None = None, order: str = "desc", limit: int = 100, offset: int = 0, - ) -> Iterable[dict]: + ) -> list[dict]: with self._lock: all_claims: list[dict] = [] for b in self._batches: - if b.kind != "837p": + if not isinstance(b, BatchRecord837): continue if batch_id is not None and b.id != batch_id: continue - # ParseResult / ParseResult835 union: 837p kinds are ParseResult - result = b.result # type: ignore[assignment] - for c in result.claims: + for c in b.result.claims: all_claims.append(to_ui_claim( c, batch_id=b.id, parsed_at=b.parsed_at, )) - if status is not None: - all_claims = [c for c in all_claims if c["status"] == status] - if provider_npi is not None: - all_claims = [c for c in all_claims if c["providerNpi"] == provider_npi] - if sort is not None: - all_claims.sort( - key=lambda c: c.get(sort, 0) or 0, - reverse=(order == "desc"), - ) - yield from all_claims[offset:offset + limit] + if status is not None: + all_claims = [c for c in all_claims if c["status"] == status] + if provider_npi is not None: + all_claims = [c for c in all_claims if c["providerNpi"] == provider_npi] + if payer is not None: + needle = payer.casefold() + all_claims = [ + c for c in all_claims + if needle in (c.get("payerName") or "").casefold() + ] + all_claims = [ + c for c in all_claims + if _date_in_bounds(c, "submissionDate", date_from, date_to) + ] + if sort is not None: + all_claims.sort( + key=lambda c: c.get(sort, 0) or 0, + reverse=(order == "desc"), + ) + return all_claims[offset:offset + limit] def iter_remittances( self, @@ -109,43 +166,44 @@ class InMemoryStore: order: str = "desc", limit: int = 100, offset: int = 0, - ) -> Iterable[dict]: + ) -> list[dict]: with self._lock: all_remits: list[dict] = [] for b in self._batches: - if b.kind != "835": + if not isinstance(b, BatchRecord835): continue if batch_id is not None and b.id != batch_id: continue - # 835 kinds are ParseResult835; its claims list holds ClaimPayment - result = b.result # type: ignore[assignment] - for cp in result.claims: + for cp in b.result.claims: all_remits.append(to_ui_remittance( cp, batch_id=b.id, parsed_at=b.parsed_at, - payer_name=result.payer.name, + payer_name=b.result.payer.name, )) - if payer is not None: - all_remits = [r for r in all_remits if r.get("payerName") == payer] - if claim_id is not None: - all_remits = [r for r in all_remits if r["claimId"] == claim_id] - if sort is not None: - all_remits.sort( - key=lambda r: r.get(sort, 0) or 0, - reverse=(order == "desc"), - ) - yield from all_remits[offset:offset + limit] + if payer is not None: + all_remits = [r for r in all_remits if r.get("payerName") == payer] + if claim_id is not None: + all_remits = [r for r in all_remits if r["claimId"] == claim_id] + all_remits = [ + r for r in all_remits + if _date_in_bounds(r, "receivedDate", date_from, date_to) + ] + if sort is not None: + all_remits.sort( + key=lambda r: r.get(sort, 0) or 0, + reverse=(order == "desc"), + ) + return all_remits[offset:offset + limit] def distinct_providers(self) -> list[dict]: with self._lock: by_npi: dict[str, dict] = {} for b in self._batches: - if b.kind != "837p": + if not isinstance(b, BatchRecord837): continue - result = b.result # type: ignore[assignment] - if not result.claims: + if not b.result.claims: continue # billing_provider lives on each claim, not at the batch level - bp = result.claims[0].billing_provider + bp = b.result.claims[0].billing_provider npi = bp.npi if npi not in by_npi: by_npi[npi] = to_ui_provider( @@ -155,16 +213,15 @@ class InMemoryStore: claim_count=0, outstanding_ar=0.0, ) - by_npi[npi]["claimCount"] += len(result.claims) + by_npi[npi]["claimCount"] += len(b.result.claims) return list(by_npi.values()) def recent_activity(self, *, limit: int = 200) -> list[dict]: with self._lock: events: list[dict] = [] for b in reversed(self._batches): - if b.kind == "837p": - result = b.result # type: ignore[assignment] - for c in result.claims: + if isinstance(b, BatchRecord837): + for c in b.result.claims: events.append(to_activity_event( id=f"{b.id}:{c.claim_id}", kind="claim_submitted", @@ -173,9 +230,8 @@ class InMemoryStore: npi=c.billing_provider.npi, amount=float(c.claim.total_charge or 0.0), )) - elif b.kind == "835": - result = b.result # type: ignore[assignment] - for cp in result.claims: + elif isinstance(b, BatchRecord835): + for cp in b.result.claims: events.append(to_activity_event( id=f"{b.id}:{cp.payer_claim_control_number}", kind="remit_received", @@ -194,6 +250,31 @@ def utcnow() -> datetime: return datetime.now(timezone.utc) +def _date_in_bounds( + item: dict, + field: str, + date_from: str | None, + date_to: str | None, +) -> bool: + """True if ``item[field]`` falls within ``[date_from, date_to]``. + + ``date_from`` / ``date_to`` are ISO date strings (``YYYY-MM-DD``). The + item's value is treated as an ISO timestamp; only its date portion is + compared. A missing field is included only when no bound is set — + bounded filters treat ``None`` as outside the range. + """ + val = item.get(field) + if val is None: + # Missing dates: include when unbounded, exclude when bounded. + return date_from is None and date_to is None + date_part = val[:10] + if date_from is not None and date_part < date_from: + return False + if date_to is not None and date_part > date_to: + return False + return True + + # --------------------------------------------------------------------------- # Mappers: rich backend models → simpler UI types. # The shape of the returned dicts must match the TypeScript interfaces in @@ -292,6 +373,7 @@ def to_ui_remittance( "claimId": cp.original_claim_id or "", "payerName": payer_name, "paidAmount": float(cp.total_paid or 0.0), + # TODO: aggregate from CAS segments when reconciliation lands (sub-project 2) "adjustmentAmount": 0.0, "status": status, "denialReason": denial_reason, diff --git a/backend/tests/test_store.py b/backend/tests/test_store.py index 4d3bb95..4ed6b1a 100644 --- a/backend/tests/test_store.py +++ b/backend/tests/test_store.py @@ -485,3 +485,113 @@ def test_iter_remittances_fills_payer_name_from_batch(): out = list(s.iter_remittances()) assert len(out) == 1 assert out[0]["payerName"] == "CO_TXIX" + + +# --------------------------------------------------------------------------- +# Review fixes: payer / date filters, tz-aware validator +# --------------------------------------------------------------------------- + + +def _make_claim_with_payer(claim_id: str, payer_name: str) -> ClaimOutput: + """ClaimOutput variant that lets us pick a payer name per row.""" + c = _make_claim(claim_id) + c.payer = Payer(name=payer_name, id="P1") + return c + + +def test_iter_claims_filters_by_payer_substring_case_insensitive(): + """iter_claims(payer=...) is a case-insensitive substring match on payerName.""" + s = InMemoryStore() + result = _make_result() + result.claims = [ + _make_claim_with_payer("A", "Aetna Health"), + _make_claim_with_payer("B", "Blue Cross"), + _make_claim_with_payer("C", "aetna better health"), + ] + s.add(BatchRecord( + id="b1", kind="837p", input_filename="a.txt", + parsed_at=_PARSED_AT, result=result, + )) + out = s.iter_claims(payer="Aetna") + ids = sorted(c["id"] for c in out) + assert ids == ["A", "C"], f"expected Aetna matches, got {ids}" + + +def test_iter_claims_filters_by_date_from_includes_in_range_only(): + """date_from excludes items whose submissionDate is before the bound.""" + s = InMemoryStore() + old = datetime(2026, 6, 10, 12, 0, 0, tzinfo=timezone.utc) + new = datetime(2026, 6, 25, 12, 0, 0, tzinfo=timezone.utc) + old_result = _make_result() + old_result.claims = [_make_claim("OLD")] + new_result = _make_result() + new_result.claims = [_make_claim("NEW")] + s.add(BatchRecord( + id="b-old", kind="837p", input_filename="a.txt", + parsed_at=old, result=old_result, + )) + s.add(BatchRecord( + id="b-new", kind="837p", input_filename="b.txt", + parsed_at=new, result=new_result, + )) + out = s.iter_claims(date_from="2026-06-20") + assert [c["id"] for c in out] == ["NEW"] + + +def test_iter_claims_filters_by_date_to_includes_in_range_only(): + """date_to excludes items whose submissionDate is after the bound.""" + s = InMemoryStore() + old = datetime(2026, 6, 10, 12, 0, 0, tzinfo=timezone.utc) + new = datetime(2026, 6, 25, 12, 0, 0, tzinfo=timezone.utc) + old_result = _make_result() + old_result.claims = [_make_claim("OLD")] + new_result = _make_result() + new_result.claims = [_make_claim("NEW")] + s.add(BatchRecord( + id="b-old", kind="837p", input_filename="a.txt", + parsed_at=old, result=old_result, + )) + s.add(BatchRecord( + id="b-new", kind="837p", input_filename="b.txt", + parsed_at=new, result=new_result, + )) + out = s.iter_claims(date_to="2026-06-20") + assert [c["id"] for c in out] == ["OLD"] + + +def test_iter_remittances_filters_by_date_from_and_date_to(): + """Remit receivedDate respects both date_from and date_to bounds.""" + s = InMemoryStore() + old = datetime(2026, 6, 10, 12, 0, 0, tzinfo=timezone.utc) + mid = datetime(2026, 6, 20, 12, 0, 0, tzinfo=timezone.utc) + new = datetime(2026, 6, 30, 12, 0, 0, tzinfo=timezone.utc) + for ts, tag in [(old, "OLD"), (mid, "MID"), (new, "NEW")]: + s.add(BatchRecord( + id=f"835-{tag}", kind="835", input_filename="a.txt", + parsed_at=ts, result=_make_835_result(claims=[_make_remit()]), + )) + out = s.iter_remittances(date_from="2026-06-15", date_to="2026-06-25") + assert [r["batchId"] for r in out] == ["835-MID"] + + +def test_iter_claims_unbounded_date_filter_includes_all(): + """With no date bounds, the filter is a no-op (all items returned).""" + s = InMemoryStore() + result = _make_result() + result.claims = [_make_claim("A"), _make_claim("B")] + s.add(BatchRecord( + id="b1", kind="837p", input_filename="a.txt", + parsed_at=_PARSED_AT, result=result, + )) + out = s.iter_claims() + assert len(out) == 2 + + +def test_batch_record_rejects_naive_parsed_at(): + """tz-aware validator: a naive datetime should raise ValueError.""" + import pytest + with pytest.raises(ValueError, match="tz-aware"): + BatchRecord( + id="x", kind="837p", input_filename="a.txt", + parsed_at=datetime(2025, 1, 1), result=_make_result(), + )