tyler/cyclone
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(store): code quality fixes per review

Browse Source 
- 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)
This commit is contained in:
Tyler
2026-06-19 19:22:44 -06:00
parent d0411b8186
commit ca645db518
2 changed files with 238 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/cyclone/store.py
+128 -46
View File
@@ -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,
backend/tests/test_store.py
+110
View File
@@ -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(),
)