forgejo_admin/basketball-api

Fork 1

jersey-public-orders admin endpoint (#432) #451

Merged

forgejo_admin merged 1 commit from 432-jersey-public-get-admin into main

2026-04-11 21:01:54 +00:00

forgejo_admin commented

2026-04-11 20:58:07 +00:00

Owner

Summary

Adds the admin-gated GET /api/jersey-public-orders list endpoint to routes/jersey_public.py (T5 of the System B production rollout). Appends to the file created by T3 (#430/#450) without modifying the existing POST handler.

The endpoint returns public jersey intake submissions with filters (status, from_date, to_date), offset pagination, and a 500-row hard cap — ready for the westside-landing admin review-and-reconcile table.

Closes #432

Changes

src/basketball_api/routes/jersey_public.py
- Added imports: date, Query, require_role
- Added module-level require_admin = require_role("admin") (mirrors the pattern from routes/subscriptions.py:19)
- Added _ALLOWED_STATUSES set constant for validation
- Added inline Pydantic v2 schemas: JerseyPublicOrderListItem, JerseyPublicOrderListResponse
- Added GET "" route list_jersey_public_orders:
  - status (query alias — internal param status_filter to avoid shadowing fastapi.status)
  - from_date, to_date, limit (default 100, cap 500), offset (default 0)
  - Computes total before applying limit/offset
  - Orders by created_at DESC, id DESC
  - 400 on invalid status value
  - Excludes submission_ip from response (internal triage data)
- T3 POST handler is byte-identical — only imports + new schemas + new route were added
tests/test_jersey_public.py
- Added 13 new tests under a dedicated GET section covering auth, ordering, filters, pagination, limit clamping, and response shape
- Added admin_client fixture that overrides require_admin (matches tests/test_admin.py pattern)
- Added _seed_order helper with direct created_at override for stable ordering tests
- Existing T3 tests untouched

Auth chain

Per feedback_funnel_requires_auth.md:

Uses require_admin = require_role("admin") from basketball_api.auth, validates the Keycloak JWT against the westside-basketball realm and enforces the admin role claim. Unauthenticated requests get 401 from get_current_user; authenticated non-admins get 403 from require_role. Matches the primitive at routes/admin.py:48 and routes/subscriptions.py:19.

Test Plan

pytest tests/test_jersey_public.py -v — all 37 tests pass (24 T3 + 13 new T5)
python -c "from basketball_api.routes.jersey_public import router; print([(r.path, sorted(list(r.methods))) for r in router.routes])" shows BOTH ('', ['POST']) and ('', ['GET'])
git diff main -- routes/checkout.py routes/jersey.py models.py auth.py main.py — empty (hands-off files untouched)
git diff main -- routes/jersey_public.py | grep "^-" — only the 3 import lines that were expanded (T3 POST handler byte-identical)
ruff format --check and ruff check — clean

Unit test coverage:

Unauth GET → 401
Non-admin GET → 403
Admin + empty DB → {items: [], total: 0}
Admin + seeded rows → stable created_at DESC, id DESC order
?status=pending filter
?status=bogus → 400
?from_date=...&to_date=... range filter
?limit=10 caps rows
?limit=1000 accepted (internally clamped to 500)
?offset=20&limit=10 returns correct slice
Response includes submitter_keycloak_sub
Response excludes submission_ip
total ignores limit/offset, respects filters

Review Checklist

T3 POST handler untouched (verified via git diff)
Inline Pydantic schemas (no schemas/ directory created)
require_admin reused from require_role("admin") — no new auth logic
Pydantic v2 syntax throughout (Field(..., pattern=...), no constr(regex=...))
ruff format + ruff check clean
All existing jersey_public tests still green

Story: story:WS-S31 — admin public jersey intake link
Arch: arch-jersey-intake
Board item: #950 (in_progress)
Depends on: #429 (migration 031 / JerseyPublicOrder model), #430 (POST endpoint, merged as #450)
Reference: routes/admin.py:48, routes/subscriptions.py:19 for require_admin primitive pattern

## Summary Adds the admin-gated `GET /api/jersey-public-orders` list endpoint to `routes/jersey_public.py` (T5 of the System B production rollout). Appends to the file created by T3 (#430/#450) without modifying the existing POST handler. The endpoint returns public jersey intake submissions with filters (status, from_date, to_date), offset pagination, and a 500-row hard cap — ready for the westside-landing admin review-and-reconcile table. Closes #432 ## Changes - `src/basketball_api/routes/jersey_public.py` - Added imports: `date`, `Query`, `require_role` - Added module-level `require_admin = require_role("admin")` (mirrors the pattern from `routes/subscriptions.py:19`) - Added `_ALLOWED_STATUSES` set constant for validation - Added inline Pydantic v2 schemas: `JerseyPublicOrderListItem`, `JerseyPublicOrderListResponse` - Added `GET ""` route `list_jersey_public_orders`: - `status` (query alias — internal param `status_filter` to avoid shadowing `fastapi.status`) - `from_date`, `to_date`, `limit` (default 100, cap 500), `offset` (default 0) - Computes `total` before applying limit/offset - Orders by `created_at DESC, id DESC` - 400 on invalid `status` value - Excludes `submission_ip` from response (internal triage data) - T3 POST handler is byte-identical — only imports + new schemas + new route were added - `tests/test_jersey_public.py` - Added 13 new tests under a dedicated GET section covering auth, ordering, filters, pagination, limit clamping, and response shape - Added `admin_client` fixture that overrides `require_admin` (matches `tests/test_admin.py` pattern) - Added `_seed_order` helper with direct `created_at` override for stable ordering tests - Existing T3 tests untouched ## Auth chain Per `feedback_funnel_requires_auth.md`: Uses `require_admin = require_role("admin")` from `basketball_api.auth`, validates the Keycloak JWT against the westside-basketball realm and enforces the `admin` role claim. Unauthenticated requests get 401 from `get_current_user`; authenticated non-admins get 403 from `require_role`. Matches the primitive at `routes/admin.py:48` and `routes/subscriptions.py:19`. ## Test Plan - [x] `pytest tests/test_jersey_public.py -v` — all 37 tests pass (24 T3 + 13 new T5) - [x] `python -c "from basketball_api.routes.jersey_public import router; print([(r.path, sorted(list(r.methods))) for r in router.routes])"` shows BOTH `('', ['POST'])` and `('', ['GET'])` - [x] `git diff main -- routes/checkout.py routes/jersey.py models.py auth.py main.py` — empty (hands-off files untouched) - [x] `git diff main -- routes/jersey_public.py | grep "^-"` — only the 3 import lines that were expanded (T3 POST handler byte-identical) - [x] `ruff format --check` and `ruff check` — clean Unit test coverage: - Unauth GET → 401 - Non-admin GET → 403 - Admin + empty DB → `{items: [], total: 0}` - Admin + seeded rows → stable `created_at DESC, id DESC` order - `?status=pending` filter - `?status=bogus` → 400 - `?from_date=...&to_date=...` range filter - `?limit=10` caps rows - `?limit=1000` accepted (internally clamped to 500) - `?offset=20&limit=10` returns correct slice - Response includes `submitter_keycloak_sub` - Response excludes `submission_ip` - `total` ignores limit/offset, respects filters ## Review Checklist - [x] T3 POST handler untouched (verified via `git diff`) - [x] Inline Pydantic schemas (no `schemas/` directory created) - [x] `require_admin` reused from `require_role("admin")` — no new auth logic - [x] Pydantic v2 syntax throughout (`Field(..., pattern=...)`, no `constr(regex=...)`) - [x] `ruff format` + `ruff check` clean - [x] All existing jersey_public tests still green ## Related Notes - Story: `story:WS-S31` — admin public jersey intake link - Arch: `arch-jersey-intake` - Board item: #950 (in_progress) - Depends on: #429 (migration 031 / JerseyPublicOrder model), #430 (POST endpoint, merged as #450) - Reference: `routes/admin.py:48`, `routes/subscriptions.py:19` for `require_admin` primitive pattern

forgejo_admin added 1 commit

2026-04-11 20:58:07 +00:00

feat: GET /api/jersey-public-orders admin endpoint (#432 )

ci/woodpecker/pr/woodpecker Pipeline failed

Details

4e51c51b73

Add admin-gated list endpoint to routes/jersey_public.py with filters
(status, from_date, to_date), offset pagination, and 500-row hard cap.
Uses require_admin dependency from auth.require_role("admin"). Response
excludes submission_ip (internal triage data).

13 new tests covering auth (401/403), empty DB, ordering, status filter,
date filter, limit cap, offset pagination, submitter_keycloak_sub
inclusion, and submission_ip exclusion. All 37 jersey_public tests pass.

Closes #432

forgejo_admin commented

2026-04-11 21:00:58 +00:00

Author

Owner

PR #451 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic v2. T5 of System B production rollout — adds admin-gated GET /api/jersey-public-orders to the existing routes/jersey_public.py.

Auth gate (CRITICAL — verified clean):

require_admin = require_role("admin") declared at module scope, imported from basketball_api.auth. Endpoint signature: user: User = Depends(require_admin). Correct — this is the admin-gated primitive, not get_current_user.
require_role internally depends on get_current_user, so unauth → 401 (from get_current_user), authed non-admin → 403 (from require_role's role check). Auth chain matches feedback_funnel_requires_auth.md.
Matches the pattern established in routes/admin.py:48 and routes/subscriptions.py:19. No new auth logic — zero DRY violation risk in the security path.
Module-level dependency (not inline Depends(require_role("admin"))) is the correct pattern for dependency_overrides to work in tests.

T3 POST handler byte-identical (CRITICAL — verified clean):
The only - lines in the diff are the three import-line expansions:

from datetime import datetime → from datetime import date, datetime
from fastapi import APIRouter, Depends, HTTPException, Request, status → …, Query, Request, status
from basketball_api.auth import User, get_current_user → …, require_role

POST handler body (create_jersey_public_order) is unchanged. Append-only patch, as promised.

Query correctness:

Hard cap: limit = min(limit, 500) after negative clamp. Good defensive input handling (spec only required 500, dev went further).
Ordering: created_at DESC, id DESC — id tiebreaker gives stable pagination on same-millisecond inserts.
total = query.count() computed AFTER status/date filters, BEFORE offset/limit. Client-side pagination math works.
Status validation: _ALLOWED_STATUSES set + 400 on invalid. SQL-injection safe because the query uses the validated literal.
Date filter: FastAPI date type parses ISO YYYY-MM-DD — good. (See nit below on datetime/date comparison semantics.)

Response shape:

JerseyPublicOrderListItem includes submitter_keycloak_sub (admin cross-reference).
submission_ip is NOT in the schema — correctly excluded from admin UI output per spec. Triage-only field stays server-side.
Pydantic v2 syntax throughout. No constr/regex=. Uses Optional[...] for nullable fields.

The status_filter shadow workaround:
status_filter: Optional[str] = Query(default=None, alias="status") — client uses ?status=pending (verified by the filter test passing with params={"status": "pending"}), internal name dodges the fastapi.status module import. Clean, idiomatic handling of the naming collision.

Test coverage:
All 13 required cases present and mapped:

unauth → 401, non-admin → 403 (overrides get_current_user to exercise the real require_role 403 path — correct)
admin empty → {items: [], total: 0}
admin seeded + stable DESC ordering
?status=pending filter, ?status=bogus → 400
?from_date=...&to_date=... range
?limit=10 caps rows, ?limit=1000 accepted without 422
?offset=20&limit=10 pagination slice
submitter_keycloak_sub present, submission_ip absent
total ignores limit/offset

Tests mock require_admin via app.dependency_overrides[require_admin] = ... — no real Keycloak in CI. Matches tests/test_admin.py pattern.

Hands-off files verified untouched:
Diff only modifies src/basketball_api/routes/jersey_public.py and tests/test_jersey_public.py. checkout.py, jersey.py, routes/__init__.py, models.py, main.py, auth.py, alembic/versions/* — all untouched.

BLOCKERS

None.

NITS

Limit-clamp test only seeds 5 rows. test_get_admin_limit_over_500_is_clamped proves the endpoint accepts ?limit=1000 without a 422 but cannot actually prove the 500-row cap because the dataset is tiny. Dev agent acknowledges this in the test comment. Stronger assertion would be a unit-level check of the internal limit variable (e.g., via a parametrized test that monkeypatches min or asserts row count when seeding 501). Non-blocking for T5 — functional behavior is still visible via code review.
to_date edge-case semantics. created_at <= to_date with to_date: date will coerce the date to midnight of that day in Postgres, meaning ?to_date=2026-04-05 excludes any row created after 00:00:00 on April 5. Admin users will reasonably expect "to April 5" to include all of April 5. Two options for follow-up: (a) document in docstring that the range is half-open [from_date 00:00, to_date 00:00), or (b) coerce to_date + timedelta(days=1) internally so the full day is included. Current tests happen to pass because they use dates outside the seeded range. File as follow-up ticket, not a blocker.
Mid-file imports in the test file (from datetime import datetime, timedelta, timezone # noqa: E402 after line 214). Functional and noqa'd, but moving to the top of the file is cleaner style. Minor ruff-hygiene nit.
Loop-seeded tests commit per row. _seed_order calls db.commit() inside the helper; tests that seed 25–30 rows do 25–30 round-trips. Fine for test speed at this scale but worth noting if the suite grows.

SOP COMPLIANCE

Branch 432-jersey-public-get-admin follows {issue}-{kebab} convention
PR body has Summary / Changes / Auth chain / Test Plan / Review Checklist / Related Notes
Closes #432 present
Related section references story WS-S31, arch arch-jersey-intake, board item #950, dependencies #429 and #430/#450
Auth chain section explicitly documented per feedback_funnel_requires_auth.md
No secrets or credentials in diff
No scope creep — only the T5 scope is modified
Ruff format + check reported clean
37 tests pass (24 T3 + 13 new T5)

PROCESS OBSERVATIONS

Clean append-only patch. The discipline of byte-identical preservation of T3's POST handler is exactly what sequenced rollout tasks require — T5 can ship independently without re-validating T3.
Module-level require_admin declaration is the right call: keeps the dependency overridable in tests and matches the established subscriptions.py:19 pattern. Consistency across admin endpoints reduces cognitive load for reviewers.
The status_filter alias workaround is the right solution to an annoying FastAPI-specific collision. Noting it in the PR body saves the next reviewer a trip through the code.
Follow-up ticket recommended for to_date half-open-range semantics. Not urgent — the admin UI isn't wired up yet — but worth capturing before someone hits it in production.
DORA impact: small, well-scoped, high-test-coverage change = low change-failure risk, supports deployment frequency.

VERDICT: APPROVED

## PR #451 Review ### DOMAIN REVIEW Stack: Python / FastAPI / SQLAlchemy / Pydantic v2. T5 of System B production rollout — adds admin-gated `GET /api/jersey-public-orders` to the existing `routes/jersey_public.py`. **Auth gate (CRITICAL — verified clean):** - `require_admin = require_role("admin")` declared at module scope, imported from `basketball_api.auth`. Endpoint signature: `user: User = Depends(require_admin)`. Correct — this is the admin-gated primitive, not `get_current_user`. - `require_role` internally depends on `get_current_user`, so unauth → 401 (from `get_current_user`), authed non-admin → 403 (from `require_role`'s role check). Auth chain matches `feedback_funnel_requires_auth.md`. - Matches the pattern established in `routes/admin.py:48` and `routes/subscriptions.py:19`. No new auth logic — zero DRY violation risk in the security path. - Module-level dependency (not inline `Depends(require_role("admin"))`) is the correct pattern for `dependency_overrides` to work in tests. **T3 POST handler byte-identical (CRITICAL — verified clean):** The only `-` lines in the diff are the three import-line expansions: - `from datetime import datetime` → `from datetime import date, datetime` - `from fastapi import APIRouter, Depends, HTTPException, Request, status` → `…, Query, Request, status` - `from basketball_api.auth import User, get_current_user` → `…, require_role` POST handler body (`create_jersey_public_order`) is unchanged. Append-only patch, as promised. **Query correctness:** - Hard cap: `limit = min(limit, 500)` after negative clamp. Good defensive input handling (spec only required 500, dev went further). - Ordering: `created_at DESC, id DESC` — id tiebreaker gives stable pagination on same-millisecond inserts. - `total = query.count()` computed AFTER status/date filters, BEFORE `offset/limit`. Client-side pagination math works. - Status validation: `_ALLOWED_STATUSES` set + 400 on invalid. SQL-injection safe because the query uses the validated literal. - Date filter: FastAPI `date` type parses ISO `YYYY-MM-DD` — good. (See nit below on datetime/date comparison semantics.) **Response shape:** - `JerseyPublicOrderListItem` includes `submitter_keycloak_sub` (admin cross-reference). - `submission_ip` is NOT in the schema — correctly excluded from admin UI output per spec. Triage-only field stays server-side. - Pydantic v2 syntax throughout. No `constr`/`regex=`. Uses `Optional[...]` for nullable fields. **The `status_filter` shadow workaround:** `status_filter: Optional[str] = Query(default=None, alias="status")` — client uses `?status=pending` (verified by the filter test passing with `params={"status": "pending"}`), internal name dodges the `fastapi.status` module import. Clean, idiomatic handling of the naming collision. **Test coverage:** All 13 required cases present and mapped: - unauth → 401, non-admin → 403 (overrides `get_current_user` to exercise the real `require_role` 403 path — correct) - admin empty → `{items: [], total: 0}` - admin seeded + stable DESC ordering - `?status=pending` filter, `?status=bogus` → 400 - `?from_date=...&to_date=...` range - `?limit=10` caps rows, `?limit=1000` accepted without 422 - `?offset=20&limit=10` pagination slice - `submitter_keycloak_sub` present, `submission_ip` absent - `total` ignores limit/offset Tests mock `require_admin` via `app.dependency_overrides[require_admin] = ...` — no real Keycloak in CI. Matches `tests/test_admin.py` pattern. **Hands-off files verified untouched:** Diff only modifies `src/basketball_api/routes/jersey_public.py` and `tests/test_jersey_public.py`. `checkout.py`, `jersey.py`, `routes/__init__.py`, `models.py`, `main.py`, `auth.py`, `alembic/versions/*` — all untouched. ### BLOCKERS None. ### NITS 1. **Limit-clamp test only seeds 5 rows.** `test_get_admin_limit_over_500_is_clamped` proves the endpoint accepts `?limit=1000` without a 422 but cannot actually prove the 500-row cap because the dataset is tiny. Dev agent acknowledges this in the test comment. Stronger assertion would be a unit-level check of the internal `limit` variable (e.g., via a parametrized test that monkeypatches `min` or asserts row count when seeding 501). Non-blocking for T5 — functional behavior is still visible via code review. 2. **`to_date` edge-case semantics.** `created_at <= to_date` with `to_date: date` will coerce the date to midnight of that day in Postgres, meaning `?to_date=2026-04-05` excludes any row created after 00:00:00 on April 5. Admin users will reasonably expect "to April 5" to include all of April 5. Two options for follow-up: (a) document in docstring that the range is half-open `[from_date 00:00, to_date 00:00)`, or (b) coerce `to_date + timedelta(days=1)` internally so the full day is included. Current tests happen to pass because they use dates outside the seeded range. File as follow-up ticket, not a blocker. 3. **Mid-file imports in the test file** (`from datetime import datetime, timedelta, timezone # noqa: E402` after line 214). Functional and `noqa`'d, but moving to the top of the file is cleaner style. Minor ruff-hygiene nit. 4. **Loop-seeded tests commit per row.** `_seed_order` calls `db.commit()` inside the helper; tests that seed 25–30 rows do 25–30 round-trips. Fine for test speed at this scale but worth noting if the suite grows. ### SOP COMPLIANCE - [x] Branch `432-jersey-public-get-admin` follows `{issue}-{kebab}` convention - [x] PR body has Summary / Changes / Auth chain / Test Plan / Review Checklist / Related Notes - [x] `Closes #432` present - [x] Related section references story `WS-S31`, arch `arch-jersey-intake`, board item #950, dependencies #429 and #430/#450 - [x] Auth chain section explicitly documented per `feedback_funnel_requires_auth.md` - [x] No secrets or credentials in diff - [x] No scope creep — only the T5 scope is modified - [x] Ruff format + check reported clean - [x] 37 tests pass (24 T3 + 13 new T5) ### PROCESS OBSERVATIONS - Clean append-only patch. The discipline of byte-identical preservation of T3's POST handler is exactly what sequenced rollout tasks require — T5 can ship independently without re-validating T3. - Module-level `require_admin` declaration is the right call: keeps the dependency overridable in tests and matches the established `subscriptions.py:19` pattern. Consistency across admin endpoints reduces cognitive load for reviewers. - The `status_filter` alias workaround is the right solution to an annoying FastAPI-specific collision. Noting it in the PR body saves the next reviewer a trip through the code. - Follow-up ticket recommended for `to_date` half-open-range semantics. Not urgent — the admin UI isn't wired up yet — but worth capturing before someone hits it in production. - DORA impact: small, well-scoped, high-test-coverage change = low change-failure risk, supports deployment frequency. ### VERDICT: APPROVED