pal-e-docs

Review: T5 basketball-api GET /api/jersey-public-orders (admin)

review-950-2026-04-10 Doc

review approved
Project

Verdict (first pass, 2026-04-10 22:10 UTC): NEEDS_REFINEMENT

Board item #950 — T5 basketball-api GET /api/jersey-public-orders (admin). Forgejo: forgejo_admin/basketball-api#432. Scope is largely solid but has two blockers that must be fixed before dispatch: the schemas file target does not match repo conventions, and there is a migration numbering collision upstream that cascades to this ticket.

Template Completeness

  • [x] Type (Feature)
  • [x] Lineage (depends on #429)
  • [x] Repo
  • [x] User Story
  • [x] Context (admin-gate pattern reference)
  • [x] File Targets
  • [x] Endpoint spec (query params, success body, errors)
  • [x] Acceptance Criteria (11 items)
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:WS-S31 label present — admin public jersey intake link
  • [ ] story note verification — caller should confirm WS-S31 entry exists on project-westside-basketball user-stories section
  • [x] arch:jersey-intake label present
  • [x] arch note verified — arch-jersey-intake exists with System B data flow and production chain sections
  • [x] Forgejo issue #432 — open, body well-formed
  • [x] wave:2 label — consistent with dependency on #429 (wave 1 migration)

File Targets

  • [x] src/basketball_api/routes/jersey_public.py — file does NOT yet exist in main. Correct IF T3 (#430 POST endpoint) lands first and creates it. T5 is positioned as "modification" which is valid given wave:2 ordering, but the ticket body should explicitly call out the implicit dependency on #430 (currently only #429 is listed under Lineage).
  • [ ] src/basketball_api/schemas/jersey_public.py — ISSUE: basketball-api has no schemas/ directory. Pydantic models in this repo live inline inside route files (verified: routes/jersey.py defines JerseyOptionResponse, JerseyCheckoutRequest, etc. inline via from pydantic import BaseModel). Creating a new schemas/ package is an architectural change this ticket should not silently introduce. Either define JerseyPublicOrderListItem inline in routes/jersey_public.py (matching convention) or scope a separate foundational ticket to introduce the schemas/ package.
  • [x] routes/admin.py reference pattern verified — require_admin = require_role("admin") at line 48, used as Depends(require_admin) throughout (20+ call sites). Pattern is real and correctly described.

Repo Placement

OK — single-repo change, correctly filed on forgejo_admin/basketball-api.

Dependencies

  • Declared: #429 (migration creating jersey_public_orders table).
  • Implicit, not declared: #430 (POST endpoint) must land before T5 because it creates the routes/jersey_public.py file T5 "modifies." Ticket body should list #430 under Lineage alongside #429.
  • Migration numbering collision [cascade blocker]: #429 is titled "Migration 014: jersey_public_orders table" but alembic/versions/014_add_password_reset_tokens.py is already merged. Current alembic head is 019 (019_player_teams_junction.py). #429's migration must be renumbered to 020+. Does not directly block T5's review, but if #429 lands with a colliding revision id, T5 cannot run. Flagging so Ava can push the correction upstream to #429 scoping.

Acceptance Criteria

11 acceptance criteria — all independently testable via pytest. Coverage is strong:
  • Auth: 401 unauth, 403 non-admin, 200 admin — complete
  • Filters: status, from_date, to_date — complete
  • Pagination: limit cap, offset+limit, total count semantics — complete
  • Ordering: created_at DESC, id DESC stable — complete
  • Validation: invalid status → 400 — complete
  • Response shape: submitter_keycloak_sub included, submission_ip excluded — complete
Minor omission: no explicit AC for limit max cap enforcement (spec says max 500 but AC only tests default and small values). Recommend adding: "limit=1000 → clamped to 500 or 422 validation error".

Blast Radius

  • Admin-gate pattern: reuses existing require_admin, zero auth duplication risk.
  • Downstream consumer: westside-landing admin UI (not in this ticket's scope, separate board item).
  • No mutation endpoints — read-only, cannot corrupt data.
  • Response schema PII: submitter_keycloak_sub is exposed. Acceptable for admin-only endpoint per ticket constraint; confirm no logging of full response body in production (standard FastAPI behavior is safe).

Decomposition Assessment

Three-thing limit and five-minute rule check:
  • Files touched: 1 (one route file, pydantic model inline) — within limit.
  • Discrete changes: (1) add route handler, (2) add response model, (3) add 7 pytest cases — at the three-thing ceiling but cohesive and non-parallelizable.
  • Estimated agent time: ~5 minutes for a focused implementation (pattern reuse from routes/admin.py + routes/jersey.py).
  • 11 AC but all simple pytest cases against a single endpoint.
  • No independent subtasks — all changes depend on the same migration and same endpoint handler.
No decomposition needed. Fits a single-agent pass once the file target issue is resolved.

Recommendation

  • [BODY] Remove src/basketball_api/schemas/jersey_public.py from File Targets. Replace with: "Define JerseyPublicOrderListItem response model inline in routes/jersey_public.py — matches repo convention (see routes/jersey.py inline pydantic models)."
  • [BODY] Add #430 (POST endpoint) to the Lineage section: "Depends on #429 (migration) and #430 (creates routes/jersey_public.py)."
  • [BODY] Add acceptance criterion for limit cap: "[ ] limit=1000 → clamped to 500 (or 422 validation error — implementer picks one)."
  • [SCOPE] Escalate to Ava: #429's migration number 014 collides with existing merged 014_add_password_reset_tokens.py. Current alembic head is 019. #429 body should be corrected to "Migration 020+" before that ticket advances. Not a T5 blocker directly, but a cascade risk on T5's dependency.
  • [SCOPE] Caller should verify story WS-S31 is listed in project-westside-basketball user-stories section.
Once recommendations 1–3 (BODY fixes) land, verdict flips to READY. SCOPE items (4–5) are parallel workstreams and do not block T5's review advancement, but Ava should be aware.

Verdict (re-review, 2026-04-10): APPROVED

Ticket body updated between first-pass review and now. All five BODY/SCOPE findings from the first pass have been verified as resolved against the current Forgejo issue #432 body. Flipping verdict to APPROVED. Ticket is ready to move from backlog to todo/next_up per the review gate.

Re-Review Checklist (what I verified against issue #432 body)

Finding Status Evidence in current issue body
1. Lineage lists both #429 AND #430 FIXED "Depends on <code>basketball-api#429</code> (migration 031) AND <code>basketball-api#430</code> (POST endpoint creates <code>routes/jersey_public.py</code>)." Also restated in Checklist ("Depends on #429 (migration 031) AND #430 (POST endpoint) merged first") and in Related section.
2. File Targets modifies routes/jersey_public.py only — no schemas/ file FIXED File Targets section lists only <code>src/basketball_api/routes/jersey_public.py</code> under "Files to modify." Explicit "Files the agent should NOT touch" list. No <code>schemas/</code> path anywhere in body.
3. JerseyPublicOrderListItem and JerseyPublicOrderListResponse declared inline FIXED File Targets says "Append the new response schema <strong>inline</strong> at the top of the file alongside the existing <code>JerseyPublicOrderIn</code> / <code>JerseyPublicOrderCreated</code> from #430." Dedicated "Pydantic schema (inline addition to routes/jersey_public.py)" section shows both models as inline BaseModel subclasses. Constraints section reinforces: "Inline Pydantic schemas — no <code>schemas/</code> directory."
4. AC for limit=1000 clamped to 500 FIXED Endpoint spec: "limit (optional, default 100, HARD CAP 500 — requests with limit &gt; 500 are clamped to 500)." Acceptance Criteria adds "[ ] ?limit=1000 is clamped to 500 (hard cap)." Test Expectations adds "[ ] Unit test: limit cap (1000 → 500)."
5. Admin gate uses require_admin = require_role("admin") from routes/admin.py:48 or routes/subscriptions.py:19 FIXED Context section: "Admin-gate primitive: <code>require_admin = require_role(\"admin\")</code> defined at <code>routes/admin.py</code> line 48 (also mirrored at <code>routes/subscriptions.py</code> line 19). Used as <code>user: User = Depends(require_admin)</code>." Keycloak integration section restates import and usage. Related section cites both references.
6. Context verifies admin-gate primitive location FIXED Context section opens with "Verified 2026-04-10 against live basketball-api source:" and lists the exact line numbers.
7. Dependency references migration 031 (not 014) FIXED Lineage: "migration 031." Checklist: "Depends on #429 (migration 031)." Related: "basketball-api#429 (migration 031)."

Correction: arch-jersey-intake exists

The first-pass review flagged via semantic search that arch-jersey-intake "might be missing." Re-verified via direct get_note(slug="arch-jersey-intake") — the note exists, is active, tagged architecture,active,arch:jersey-intake, under project westside-basketball, and contains the complete System A/B/C architecture including the System B data flow and production chain diagrams that this ticket depends on. The first-pass false negative was a semantic-search miss, not a missing note. Direct slug lookup should be the default for known slugs, per convention-block-first-access.

Outstanding non-blocking items (for Ava's awareness)

  • #429 migration number cascade: First-pass noted #429 body said "Migration 014" but live alembic head is 019. Current T5 body references "migration 031" which suggests #429 was already corrected upstream — worth confirming #429's body matches. Not a T5 blocker.
  • WS-S31 story note verification: Caller should confirm the WS-S31 entry is listed in project-westside-basketball user-stories section. Label is present on the ticket; the backing story note was not re-verified in this pass.

Final Verdict

APPROVED — all seven review findings resolved in the current ticket body. Ticket #432 / board item #950 is ready to advance from backlog to todo (then next_up) per the review gate. No further scope revisions required before dispatch.