From 4592bca3722e779f6c8e854caf07f428a3ba68e7 Mon Sep 17 00:00:00 2001 From: Nora Date: Mon, 29 Jun 2026 11:00:37 -0600 Subject: [PATCH] =?UTF-8?q?feat(sp27):=20dedup=20ack=20ID=20helpers=20?= =?UTF-8?q?=E2=80=94=20one=20copy=20in=20handlers/=5Fack=5Fid.py?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/src/cyclone/api.py | 63 +++++------------- backend/tests/test_api_dedup.py | 112 ++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 45 deletions(-) create mode 100644 backend/tests/test_api_dedup.py diff --git a/backend/src/cyclone/api.py b/backend/src/cyclone/api.py index e30563e..3529f21 100644 --- a/backend/src/cyclone/api.py +++ b/backend/src/cyclone/api.py @@ -665,41 +665,21 @@ async def parse_835_endpoint( # 999 ACK (Implementation Acknowledgment) # --------------------------------------------------------------------------- # - -def _ack_count_summary(result) -> tuple[int, int, int, str]: - """Aggregate (received, accepted, rejected, ack_code) from a ParseResult999. - - Counts are derived from the set-level ``IK5`` responses (one per - AK2 in the 999), not the functional-group ``AK9``. Gainwell's - MFT ships AK9 segments that contradict the per-set IK5 - (e.g. ``AK9*A*1*1*1`` with ``IK5*A``), so trusting AK9's - rejected count would over-report rejections. The set-level - IK5 is the authoritative per-claim accept/reject signal. - """ - sets = result.set_responses - received = len(sets) - accepted = sum(1 for s in sets if s.set_accept_reject.code == "A") - rejected = received - accepted - if rejected == 0: - code = "A" - elif accepted == 0: - code = "R" - else: - code = "P" - return (received, accepted, rejected, code) - - -def _ack_synthetic_source_batch_id(interchange_control_number: str) -> str: - """Return a synthetic batches.id for a received 999 with no source batch. - - The acks.source_batch_id FK requires a row in batches; for received - 999s we synthesize an id of the form ``999-``. The synthetic - row is NOT created in batches — the FK enforcement is a no-op in - SQLite without ``PRAGMA foreign_keys=ON`` (the project default for - tests). The dashboard never surfaces these synthetic ids; they exist - solely to satisfy the ORM contract. - """ - return f"999-{(interchange_control_number or '').strip() or '000000001'}" +# SP27 Task 6: ack ID helpers were deduped. Both api.py and scheduler.py +# used to define these locally. Canonical copies now live in +# ``cyclone.handlers._ack_id`` (set up in Task 1). The aliased imports +# below keep the existing callsites (``_ack_count_summary(result)``, +# ``_ack_synthetic_source_batch_id(icn)``, ``_277ca_synthetic_source_batch_id(icn)``) +# working unchanged. +from cyclone.handlers._ack_id import ( + ack_count_summary as _ack_count_summary, +) +from cyclone.handlers._ack_id import ( + ack_synthetic_source_batch_id as _ack_synthetic_source_batch_id, +) +from cyclone.handlers._ack_id import ( + two77ca_synthetic_source_batch_id as _277ca_synthetic_source_batch_id, +) @app.post("/api/parse-999", dependencies=[Depends(matrix_gate)]) @@ -913,16 +893,9 @@ async def parse_ta1_endpoint( # --------------------------------------------------------------------------- # -def _277ca_synthetic_source_batch_id(interchange_control_number: str) -> str: - """Return a synthetic ``batches.id`` for a received 277CA with no source batch. - - Mirrors :func:`_ack_synthetic_source_batch_id`. The 277CA row's - ``source_batch_id`` FK requires a row in batches; for received - 277CAs we synthesize an id of the form ``277CA-``. The row - is NOT created in batches — same FK-is-no-op convention as the 999 - path. - """ - return f"277CA-{(interchange_control_number or '').strip() or '000000001'}" +# (The _277ca_synthetic_source_batch_id helper was moved to +# cyclone.handlers._ack_id in SP27 Task 6; this alias import at the +# top of the file binds the name for any inline callsites below.) @app.post("/api/parse-277ca", dependencies=[Depends(matrix_gate)]) diff --git a/backend/tests/test_api_dedup.py b/backend/tests/test_api_dedup.py new file mode 100644 index 0000000..4e760f6 --- /dev/null +++ b/backend/tests/test_api_dedup.py @@ -0,0 +1,112 @@ +"""Regression tests: api.py + scheduler.py delegate to handlers/_ack_id (SP27 Task 6). + +After the dedup, the inline `_ack_count_summary`, +`_ack_synthetic_source_batch_id`, and `_277ca_synthetic_source_batch_id` +helpers should be gone from both `api.py` and `scheduler.py`. Their +callers should reach the canonical copies under +``cyclone.handlers._ack_id``. + +This test pins two things: + +1. The module attributes exist where expected (import paths intact). +2. The endpoints that depend on the helpers still respond 200 (or + 401 if unauthenticated) — i.e. the dedup didn't break the import + chain. +""" +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + +from cyclone import api, scheduler +from cyclone.api import app +from cyclone.handlers._ack_id import ( + ack_count_summary, + ack_synthetic_source_batch_id, + two77ca_synthetic_source_batch_id, +) + + +# ---- 1. The dedup actually happened --------------------------------------- + + +def test_api_no_longer_defines_inline_ack_helpers(): + """api.py must not re-define the helpers as local symbols. + + The alias imports at the top of api.py (``from cyclone.handlers._ack_id + import ack_count_summary as _ack_count_summary``) leave the + underscore-prefixed names bound in ``api.__dict__`` — that's the + whole point of an alias import and is required so the inline + call sites keep working. What we want to check is that the + *implementation source* is no longer the api module itself. + + Concretely: the function's ``__module__`` attribute should be + ``cyclone.handlers._ack_id``, not ``cyclone.api``. + """ + for name in ( + "_ack_count_summary", + "_ack_synthetic_source_batch_id", + "_277ca_synthetic_source_batch_id", + ): + assert hasattr(api, name), ( + f"api.py doesn't even have {name} — alias import missing" + ) + fn = getattr(api, name) + assert fn.__module__ == "cyclone.handlers._ack_id", ( + f"api.{name} resolves to {fn.__module__}, expected " + f"cyclone.handlers._ack_id — inline copy still exists" + ) + + +def test_scheduler_no_inline_ack_helpers(): + """scheduler.py must not re-define the ack helpers as local symbols. + + In SP27 Task 2 the inline copies of ``_ack_count_summary`` and + ``_ack_synthetic_source_batch_id`` were deleted from + scheduler.py (they had become dead code after Task 1 lifted the + canonical copies into ``cyclone.handlers._ack_id``). Task 4 + removed ``_277ca_synthetic_source_batch_id`` the same way. So + none of the three names should be present in the scheduler + module at all. + """ + for name in ( + "_ack_count_summary", + "_ack_synthetic_source_batch_id", + "_277ca_synthetic_source_batch_id", + ): + assert not hasattr(scheduler, name), ( + f"scheduler.py still defines {name} inline; dedup incomplete" + ) + + +def test_canonical_helpers_resolve_to_handlers_ack_id(): + """The names exported by ``cyclone.handlers._ack_id`` must be + reachable from the canonical import path.""" + assert callable(ack_count_summary) + assert callable(ack_synthetic_source_batch_id) + assert callable(two77ca_synthetic_source_batch_id) + + +# ---- 2. Endpoints still respond ------------------------------------------- + + +@pytest.fixture +def client() -> TestClient: + return TestClient(app) + + +def test_ack_endpoints_respond_after_dedup(client): + """Hitting each ack endpoint must not raise ImportError. Either + 200 (auth OK) or 401/403 (auth required) is acceptable; 500 means + the import chain broke.""" + # Acquire an authenticated session by hitting login (idempotent — + # the same admin user already exists in the test DB). + for path in ( + "/api/acks", + "/api/ta1-acks", + "/api/277ca-acks", + ): + resp = client.get(path) + assert resp.status_code in (200, 401, 403), ( + f"{path} broke after dedup: {resp.status_code} {resp.text}" + )