From d81b6ed4fc6456e03f8aaccbc97caab2384375aa Mon Sep 17 00:00:00 2001 From: Nora Date: Mon, 29 Jun 2026 13:22:42 -0600 Subject: [PATCH] fix(sp27): /api/claims + /api/remittances total counts the full population MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this fix, the list endpoints computed `total` via `len(list(store.iter_*(**common)))`. Both `iter_claims` and `iter_remittances` default to `limit=100`, so the reported total silently capped at 100 even when the DB held 60k claims or 835 remits. The frontend rendered a `data.total` of 100 in the KPI tile and a 100-row table, the page looked complete, and the bug stayed hidden — exactly the Dashboard silent-failure pattern that 59c3275 (server-aggregated KPIs) was meant to retire. The Remittances page symptom reported on Jun 29 ('REMITS 100', empty CLAIM column on 100 rows) was this bug. The empty CLAIM is a separate matching concern (those remits have `claim_id = NULL` in the DB); the count fix addresses the population-size lie. Fix: * `CycloneStore.count_claims` + `count_remittances` reuse the iter's filter pipeline (DB filters + in-memory payer/date filters) with an effectively-unbounded limit so the count reflects the true DB population. * `list_claims` + `list_remittances` call the new count helpers instead of slicing `iter_*` twice. * 13 new tests in `test_list_endpoint_counts.py` cover the empty/filter/per-dimension/cardinality cases plus the HTTP regression (seed 150/120, assert total matches). Bonus fix: * `conftest._reset_rate_limit_buckets` walks the middleware stack and clears `RateLimitMiddleware._buckets` between tests. Without this, the full suite tripped the 300 req/60s rate limiter mid-run and 9 later tests (4 of mine, 5 pre-existing in test_inbox_endpoints / test_payer_summary) got 429s. All 1167 tests now pass. Follow-ups noted but out of scope: * `/api/admin/audit-log`, `/api/admin/backup/list`, `/api/admin/scheduler/processed-files` use the same `len(rows)`-after-`.limit(limit)` pattern. Admin-only, no `has_more` consumption, so impact is bounded. * `count_*` could short-circuit to `func.count()` to skip the UI-dict build, but iter already calls `q.all()` so the win is modest. --- backend/src/cyclone/api.py | 9 +- backend/src/cyclone/store.py | 65 ++++ backend/tests/conftest.py | 37 ++- backend/tests/test_list_endpoint_counts.py | 345 +++++++++++++++++++++ 4 files changed, 453 insertions(+), 3 deletions(-) create mode 100644 backend/tests/test_list_endpoint_counts.py diff --git a/backend/src/cyclone/api.py b/backend/src/cyclone/api.py index 974ef53..1c4b674 100644 --- a/backend/src/cyclone/api.py +++ b/backend/src/cyclone/api.py @@ -1714,7 +1714,10 @@ def list_claims( items = list(store.iter_claims( sort=sort, order=order, limit=limit, offset=offset, **common, )) - total = len(list(store.iter_claims(**common))) + # SP27 Task 13b: count the full population, not a 100-row sample. + # `iter_claims` defaults to limit=100; counting its output silently + # capped the reported total at 100 even when the DB held 60k rows. + total = store.count_claims(**common) returned = len(items) has_more = total > offset + returned if _wants_ndjson(request): @@ -2220,7 +2223,9 @@ def list_remittances( items = list(store.iter_remittances( sort=sort, order=order, limit=limit, offset=offset, **common, )) - total = len(list(store.iter_remittances(**common))) + # SP27 Task 13b: count the full population, not a 100-row sample. + # See the matching note in list_claims — same silent-failure pattern. + total = store.count_remittances(**common) returned = len(items) has_more = total > offset + returned if _wants_ndjson(request): diff --git a/backend/src/cyclone/store.py b/backend/src/cyclone/store.py index a8a76b4..4079c1b 100644 --- a/backend/src/cyclone/store.py +++ b/backend/src/cyclone/store.py @@ -851,6 +851,15 @@ def _date_in_bounds( return True +# Effectively-unbounded iter_* limit, used by count_claims / +# count_remittances so they can reuse the iter's filter pipeline +# (incl. the in-memory ``payer`` substring + ``date_from/to`` checks) +# without being silently capped at the iter's default ``limit=100``. +# 2**31 - 1 is the largest signed 32-bit int — far above any realistic +# X12 batch population. +_ITER_UNBOUNDED = 2**31 - 1 + + # --------------------------------------------------------------------------- # Backward-compat shim: tests called ``_batches.clear()`` on the in-memory # store. The DB-backed store doesn't have an in-memory list, so we expose @@ -1754,6 +1763,62 @@ class CycloneStore: r.pop("_sort_receivedDate", None) return out[offset:offset + limit] + # -- count helpers (SP27 Task 13b) --------------------------------- + # + # The list endpoints (``/api/claims`` and ``/api/remittances``) + # previously computed ``total`` by calling ``iter_*`` with default + # ``limit=100`` and taking ``len(...)``. With 60k+ claims and 800+ + # remits in production, the reported total silently capped at 100 + # even when the UI rendered a "100" KPI tile and a 100-row table — + # the page looked complete but the population was 600× larger. These + # helpers reuse the iter's filter pipeline with an effectively + # unbounded ``limit`` so the count reflects the true DB population, + # not a 100-row sample. Mirrors Dashboard's "server-aggregated + # counts" fix from commit 59c3275. + def count_claims( + self, + *, + 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, + ) -> int: + """Count claims that would be returned by ``iter_claims``. + + Same filter parameters as :meth:`iter_claims` (excluding + ``sort``/``order``/``limit``/``offset``, which don't affect + cardinality). + """ + rows = self.iter_claims( + batch_id=batch_id, status=status, provider_npi=provider_npi, + payer=payer, date_from=date_from, date_to=date_to, + sort=None, order="desc", limit=_ITER_UNBOUNDED, offset=0, + ) + return len(rows) + + def count_remittances( + self, + *, + batch_id: str | None = None, + payer: str | None = None, + claim_id: str | None = None, + date_from: str | None = None, + date_to: str | None = None, + ) -> int: + """Count remittances that would be returned by ``iter_remittances``. + + Same filter parameters as :meth:`iter_remittances` (excluding + ``sort``/``order``/``limit``/``offset``). + """ + rows = self.iter_remittances( + batch_id=batch_id, payer=payer, claim_id=claim_id, + date_from=date_from, date_to=date_to, + sort=None, order="desc", limit=_ITER_UNBOUNDED, offset=0, + ) + return len(rows) + def distinct_providers(self) -> list[dict]: """Group claims by NPI and return one row per provider.""" with db.SessionLocal()() as s: diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index be2a14a..f9fd92f 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -49,9 +49,44 @@ def _auto_init_db(tmp_path, monkeypatch): from cyclone import api as _api_mod _api_mod.app.state.event_bus = EventBus() deps.AUTH_DISABLED = True + # The rate-limit middleware keeps a per-IP sliding window in + # ``_buckets``. Without a reset between tests, later tests in a + # full-suite run get ``429 Too Many Requests`` once the testclient + # IP exhausts its 300 req/60s budget. Walk the middleware stack + # and clear the buckets so every test starts with a fresh window. + # Trigger the stack build with a cheap health probe (the only + # request exempt from the limiter — see RateLimitMiddleware.EXEMPT_PATHS). + _reset_rate_limit_buckets(_api_mod.app) try: yield finally: deps.AUTH_DISABLED = False _api_mod.app.state.event_bus = None - db._reset_for_tests() \ No newline at end of file + db._reset_for_tests() + + +def _reset_rate_limit_buckets(app) -> None: + """Clear the rate-limit middleware's per-IP sliding-window buckets. + + SP27 Task 13b follow-up: ``RateLimitMiddleware._buckets`` is shared + across all tests in a process (TestClient reuses the same ``app`` + instance), so without a reset between tests the full suite trips + the limiter after ~300 requests and later tests get 429s. + + The middleware stack is only built on the first request, so we + prime it with an exempt health probe before walking to the + RateLimit layer. If the stack ever stops being a single chain + of ``.app`` links, this helper raises AttributeError — better + to fail loudly than silently leak state. + """ + from fastapi.testclient import TestClient + TestClient(app).get("/api/health") + cur = app.middleware_stack + while cur is not None: + if hasattr(cur, "_buckets"): + cur._buckets.clear() + return + cur = getattr(cur, "app", None) + # No RateLimitMiddleware in the stack — nothing to reset. Should + # not happen in this codebase (security.py registers it at boot) + # but we don't want a missing reset to crash unrelated tests. \ No newline at end of file diff --git a/backend/tests/test_list_endpoint_counts.py b/backend/tests/test_list_endpoint_counts.py new file mode 100644 index 0000000..6d16a6c --- /dev/null +++ b/backend/tests/test_list_endpoint_counts.py @@ -0,0 +1,345 @@ +"""Regression tests for SP27 Task 13b: ``total`` on the list endpoints +must reflect the full DB population, not a 100-row sample. + +Before the fix, both ``/api/claims`` and ``/api/remittances`` computed +``total`` via:: + + total = len(list(store.iter_(**filters))) + +``iter_`` defaults to ``limit=100``, so the returned list was capped +at 100 regardless of how many rows the DB actually held. The frontend +then rendered ``data.total`` as the "Remits" / "Claims" KPI tile — +silently reporting 100 when the real count was 835 (or 60,000+). The +page also showed 100 rows, looked complete, and the bug stayed hidden. + +The fix added ``count_claims`` and ``count_remittances`` to the store, +both of which reuse the iter's filter pipeline with an +effectively-unbounded limit so the count reflects the true population. + +These tests cover: + +* Module-level: ``count_*`` returns the right cardinality with each + filter dimension (none, status, payer substring, date range, + batch_id). +* Module-level: ``count_* > 100`` when the DB has more than 100 rows + (the original symptom). +* HTTP: ``/api/claims?limit=25`` reports ``total`` matching the full + DB count, not the page size. +* HTTP: ``/api/remittances`` ditto. +""" + +from __future__ import annotations + +from datetime import date, datetime, timedelta, timezone +from decimal import Decimal + +import pytest +from fastapi.testclient import TestClient + +from cyclone import db +from cyclone.api import app +from cyclone.db import Batch, Claim, ClaimState, Remittance +from cyclone.store import store as global_store + + +@pytest.fixture +def client() -> TestClient: + return TestClient(app) + + +def _seed_batch(s, batch_id: str, parsed_at: datetime, kind: str = "837p") -> None: + s.add(Batch( + id=batch_id, + kind=kind, + input_filename=f"{batch_id}.edi", + parsed_at=parsed_at, + totals_json={"total_claims": 0}, + validation_json={"passed": True, "warnings": [], "errors": []}, + raw_result_json={"_": "stub"}, + )) + + +def _seed_claim( + s, + claim_id: str, + batch_id: str, + *, + payer_name: str = "Colorado Medicaid", + state: ClaimState = ClaimState.SUBMITTED, + service_date: date = date(2026, 6, 1), +) -> None: + s.add(Claim( + id=claim_id, + batch_id=batch_id, + patient_control_number=claim_id, + service_date_from=service_date, + service_date_to=service_date, + charge_amount=Decimal("100.00"), + provider_npi="1881068062", + payer_id="SKCO0", + state=state, + # NB: raw_json carries the payer name because iter_claims' + # `payer` filter is an in-memory substring on `payerName` + # recovered from the claim's raw_json payload. + raw_json={"payer": {"name": payer_name}}, + )) + + +def _seed_remit( + s, + remit_id: str, + batch_id: str, + *, + received_at: datetime, + payer_name: str = "Colorado Medicaid", + claim_id: str | None = None, + total_paid: Decimal = Decimal("100.00"), +) -> None: + # The remittance's batch row carries raw_result_json (with payer + # name) because iter_remittances reads payer_name from there. + s.add(Batch( + id=batch_id, + kind="835", + input_filename=f"{remit_id}.edi", + parsed_at=received_at, + totals_json={"total_claims": 1}, + validation_json={"passed": True, "warnings": [], "errors": []}, + raw_result_json={"payer": {"name": payer_name}}, + )) + s.add(Remittance( + id=remit_id, + batch_id=batch_id, + payer_claim_control_number=remit_id, + claim_id=claim_id, + status_code="1", + total_charge=Decimal("100.00"), + total_paid=total_paid, + adjustment_amount=Decimal("0"), + received_at=received_at, + )) + + +# --------------------------------------------------------------------------- # +# Module-level: count_claims +# --------------------------------------------------------------------------- # + + +def test_count_claims_zero_when_empty(): + assert global_store.count_claims() == 0 + + +def test_count_claims_unfiltered_returns_full_population(): + """Regression: with 150 claims seeded and default iter limit=100, + the old ``len(list(iter_claims()))`` returned 100. count_claims + must return 150.""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-claims-pop", base) + for i in range(150): + _seed_claim(s, f"CLM-{i:04d}", "b-claims-pop") + s.commit() + + assert global_store.count_claims() == 150 + + +def test_count_claims_filters_by_status(): + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-status", base) + for i in range(5): + _seed_claim(s, f"CLM-SUB-{i}", "b-status", state=ClaimState.SUBMITTED) + for i in range(3): + _seed_claim(s, f"CLM-PAID-{i}", "b-status", state=ClaimState.PAID) + s.commit() + + assert global_store.count_claims(status="submitted") == 5 + assert global_store.count_claims(status="paid") == 3 + + +def test_count_claims_filters_by_payer_substring(): + """The `payer` filter is a case-insensitive substring on payerName + recovered from raw_json — make sure count_claims applies it the + same way iter_claims does.""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-payer", base) + for i in range(4): + _seed_claim(s, f"CLM-CO-{i}", "b-payer", payer_name="Colorado Medicaid") + for i in range(2): + _seed_claim(s, f"CLM-AZ-{i}", "b-payer", payer_name="Arizona Medicaid") + s.commit() + + assert global_store.count_claims() == 6 + assert global_store.count_claims(payer="Colorado") == 4 + assert global_store.count_claims(payer="Arizona") == 2 + assert global_store.count_claims(payer="colorado") == 4 # case-insensitive + + +def test_count_claims_filters_by_provider_npi(): + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-npi", base) + for i in range(3): + _seed_claim(s, f"CLM-A-{i}", "b-npi") + # Switch the rest to a different NPI via direct insert. + for i in range(7): + s.add(Claim( + id=f"CLM-B-{i}", batch_id="b-npi", + patient_control_number=f"CLM-B-{i}", + service_date_from=date(2026, 6, 1), + service_date_to=date(2026, 6, 1), + charge_amount=Decimal("100.00"), + provider_npi="9999999999", + payer_id="SKCO0", + state=ClaimState.SUBMITTED, + )) + s.commit() + + assert global_store.count_claims(provider_npi="1881068062") == 3 + assert global_store.count_claims(provider_npi="9999999999") == 7 + + +# --------------------------------------------------------------------------- # +# Module-level: count_remittances +# --------------------------------------------------------------------------- # + + +def test_count_remittances_zero_when_empty(): + assert global_store.count_remittances() == 0 + + +def test_count_remittances_unfiltered_returns_full_population(): + """Regression: same shape as test_count_claims_unfiltered — the + old ``len(list(iter_remittances()))`` was capped at 100.""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-remit-pop", base) + for i in range(120): + _seed_remit( + s, f"RMT-{i:04d}", f"b-{i:04d}", + received_at=base + timedelta(minutes=i), + ) + s.commit() + + assert global_store.count_remittances() == 120 + + +def test_count_remittances_filters_by_claim_id(): + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-cid", base) + _seed_remit(s, "RMT-A", "b-cid-A", received_at=base, claim_id="CLM-1") + _seed_remit(s, "RMT-B", "b-cid-B", received_at=base, claim_id="CLM-2") + _seed_remit(s, "RMT-C", "b-cid-C", received_at=base, claim_id=None) + s.commit() + + assert global_store.count_remittances() == 3 + assert global_store.count_remittances(claim_id="CLM-1") == 1 + assert global_store.count_remittances(claim_id="CLM-2") == 1 + assert global_store.count_remittances(claim_id="CLM-missing") == 0 + + +def test_count_remittances_filters_by_payer_exact_match(): + """iter_remittances' `payer` filter is an exact match (not a + substring). Verify count_remittances mirrors that.""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-rpayer", base) + for i in range(4): + _seed_remit( + s, f"RMT-CO-{i}", f"b-co-{i}", + received_at=base, payer_name="Colorado Medicaid", + ) + for i in range(2): + _seed_remit( + s, f"RMT-AZ-{i}", f"b-az-{i}", + received_at=base, payer_name="Arizona Medicaid", + ) + s.commit() + + assert global_store.count_remittances(payer="Colorado Medicaid") == 4 + assert global_store.count_remittances(payer="Arizona Medicaid") == 2 + assert global_store.count_remittances(payer="colorado medicaid") == 0 # case-sensitive exact + + +# --------------------------------------------------------------------------- # +# HTTP regression: list endpoint `total` reflects full population +# --------------------------------------------------------------------------- # + + +def test_api_claims_total_reflects_full_population(client: TestClient): + """Bug repro: seed 150 claims, hit /api/claims?limit=25, assert + ``total == 150`` (not 25, not 100).""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-http-claims", base) + for i in range(150): + _seed_claim(s, f"CLM-HTTP-{i:04d}", "b-http-claims") + s.commit() + + resp = client.get("/api/claims", params={"limit": 25}) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["total"] == 150, body + assert body["returned"] == 25 + assert body["has_more"] is True + + +def test_api_remittances_total_reflects_full_population(client: TestClient): + """Bug repro: seed 120 remits, hit /api/remittances?limit=25, + assert ``total == 120`` (the user's exact symptom — KPI tile + showed "100" because the old code capped at iter's limit=100).""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-http-remit", base) + for i in range(120): + _seed_remit( + s, f"RMT-HTTP-{i:04d}", f"b-h-{i:04d}", + received_at=base + timedelta(minutes=i), + ) + s.commit() + + resp = client.get("/api/remittances", params={"limit": 25}) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["total"] == 120, body + assert body["returned"] == 25 + assert body["has_more"] is True + + +def test_api_remittances_total_zero_when_empty(client: TestClient): + """Empty DB → total=0. (Sanity check the count path doesn't break + for the no-data case.)""" + resp = client.get("/api/remittances") + assert resp.status_code == 200 + body = resp.json() + assert body == {"items": [], "total": 0, "returned": 0, "has_more": False} + + +def test_api_remittances_total_respects_filters(client: TestClient): + """Filter by payer → total narrows correctly (covers the in-memory + payer filter on the count path).""" + base = datetime(2026, 6, 1, 12, 0, tzinfo=timezone.utc) + with db.SessionLocal()() as s: + _seed_batch(s, "b-http-filter", base) + for i in range(7): + _seed_remit( + s, f"RMT-F-CO-{i}", f"b-fco-{i}", + received_at=base, payer_name="Colorado Medicaid", + ) + for i in range(3): + _seed_remit( + s, f"RMT-F-AZ-{i}", f"b-faz-{i}", + received_at=base, payer_name="Arizona Medicaid", + ) + s.commit() + + resp = client.get( + "/api/remittances", + params={"payer": "Colorado Medicaid", "limit": 100}, + ) + assert resp.status_code == 200 + body = resp.json() + assert body["total"] == 7 + assert body["returned"] == 7 + assert body["has_more"] is False \ No newline at end of file