forgejo_admin/basketball-api

Fork 1

jersey-public-orders endpoint (#430) #450

Merged

forgejo_admin merged 1 commit from 430-jersey-public-post-endpoint into main

2026-04-11 20:53:04 +00:00

forgejo_admin commented

2026-04-11 20:43:43 +00:00

Owner

Summary

Adds the System B public jersey intake POST endpoint. Any signed-in westside-basketball realm user can submit an order; the caller's Keycloak sub is stamped on the row via submitter_keycloak_sub, while player_name/email come verbatim from the request body so a parent can submit on behalf of a child.

Closes #430

Changes

New src/basketball_api/routes/jersey_public.py — new router module with inline Pydantic schemas (JerseyPublicOrderIn, JerseyPublicOrderCreated) and a single POST "" route. Validates kq, tier, preferred_number_*, top_size/short_size via Pydantic v2 pattern= constraints. Extracts submission IP from X-Forwarded-For (first hop) or request.client.host, dropping unparseable values so the inet column never receives junk (e.g. FastAPI TestClient's "testclient" sentinel). Wraps the DB write in a try/rollback and raises 500 on SQLAlchemy errors.
Modified src/basketball_api/main.py — imports jersey_public_router alongside the other route imports and registers it with prefix="/api/jersey-public-orders" next to the existing jersey_router registration.
New tests/test_jersey_public.py — 24 tests (1 unauth, 5 happy-path/behavioral, 18 parametrized validation). Mocks get_current_user via FastAPI dependency override; no real Keycloak in CI.

No edits to routes/checkout.py, routes/jersey.py, routes/__init__.py, src/basketball_api/models.py, or src/basketball_api/auth.py (verified with git diff main → empty).

Notes on deviations from the ticket's example code

The ticket example uses Pydantic v1 constr(regex=...) and EmailStr. basketball-api is on Pydantic v2, so constraints are expressed via Field(..., pattern=...). EmailStr is avoided because email-validator is not a dependency in pyproject.toml — a simple RFC-ish regex is used for the email field (^[^@\s]+@[^@\s]+\.[^@\s]+$). Acceptance test invalid email → 400 still passes.
submission_ip is validated with ipaddress.ip_address() before insert; unparseable values store as NULL rather than crashing the inet cast.

Auth chain

from basketball_api.auth import User, get_current_user (lines 77–159 of auth.py) — validates RS256 JWT against westside-basketball Keycloak realm JWKS
Route signature: user: User = Depends(get_current_user) matches the routes/subscriptions.py line 367 pattern
user.sub is written to jersey_public_orders.submitter_keycloak_sub; user.email/user.username are NOT used — body values win (per feedback_funnel_requires_auth.md: authenticated but user-editable submission).
401 is raised automatically by get_current_user on missing/invalid token; no 403 path (any realm user may POST).

Test Plan

pytest tests/test_jersey_public.py -v → 24 passed in 2.75s (local Postgres)
ruff format + ruff check → clean
python -c "from basketball_api.routes.jersey_public import router; print(router.routes)" → [('', {'POST'})]
python -c "from basketball_api.main import app; ..." → registered: ['/api/jersey-public-orders']
grep -rn "schemas/jersey_public" src/basketball_api/ → no matches
grep -c "get_current_user" src/basketball_api/routes/jersey_public.py → 2
git diff main -- <untouched files> → 0 lines
git status → exactly 3 files (1 modified, 2 new)
CI: full test suite runs with service-container Postgres
Post-merge: validate T3 against prod with a real Keycloak JWT (separate validation ticket)

Review Checklist

Keycloak auth via existing get_current_user — no new auth primitive
player_name/email NOT overridden by JWT claims (parent-for-child submission supported)
Inline Pydantic schemas — no schemas/ directory created
routes/__init__.py not touched
routes/checkout.py, routes/jersey.py, models.py, auth.py not touched
No email sending (deferred to #431)
Single router registration in main.py matching existing pattern
Response includes {id, status, created_at} for frontend confirmation
Parallel-safe with T5 (#950) — T5 will append the GET endpoint to this file after T3 merges

Story: story:WS-S31 — admin public jersey intake link
Arch: arch-jersey-intake
Board item: #948 (in_progress)
Depends on: #429 (migration 043, already applied in prod per validation-429-2026-04-11)
Parallel ticket: #950 (T5 GET admin endpoint — will append to routes/jersey_public.py after this merges)
Reference: feedback_funnel_requires_auth.md — why this is Keycloak-gated
Reference: routes/subscriptions.py line 367 — get_current_user usage pattern

## Summary Adds the System B public jersey intake POST endpoint. Any signed-in westside-basketball realm user can submit an order; the caller's Keycloak `sub` is stamped on the row via `submitter_keycloak_sub`, while `player_name`/`email` come verbatim from the request body so a parent can submit on behalf of a child. Closes #430 ## Changes - **New** `src/basketball_api/routes/jersey_public.py` — new router module with inline Pydantic schemas (`JerseyPublicOrderIn`, `JerseyPublicOrderCreated`) and a single `POST ""` route. Validates `kq`, `tier`, `preferred_number_*`, `top_size`/`short_size` via Pydantic v2 `pattern=` constraints. Extracts submission IP from `X-Forwarded-For` (first hop) or `request.client.host`, dropping unparseable values so the `inet` column never receives junk (e.g. FastAPI TestClient's `"testclient"` sentinel). Wraps the DB write in a try/rollback and raises 500 on SQLAlchemy errors. - **Modified** `src/basketball_api/main.py` — imports `jersey_public_router` alongside the other route imports and registers it with `prefix="/api/jersey-public-orders"` next to the existing `jersey_router` registration. - **New** `tests/test_jersey_public.py` — 24 tests (1 unauth, 5 happy-path/behavioral, 18 parametrized validation). Mocks `get_current_user` via FastAPI dependency override; no real Keycloak in CI. No edits to `routes/checkout.py`, `routes/jersey.py`, `routes/__init__.py`, `src/basketball_api/models.py`, or `src/basketball_api/auth.py` (verified with `git diff main` → empty). ### Notes on deviations from the ticket's example code - The ticket example uses Pydantic v1 `constr(regex=...)` and `EmailStr`. basketball-api is on Pydantic v2, so constraints are expressed via `Field(..., pattern=...)`. `EmailStr` is avoided because `email-validator` is not a dependency in `pyproject.toml` — a simple RFC-ish regex is used for the email field (`^[^@\s]+@[^@\s]+\.[^@\s]+$`). Acceptance test `invalid email → 400` still passes. - `submission_ip` is validated with `ipaddress.ip_address()` before insert; unparseable values store as NULL rather than crashing the `inet` cast. ## Auth chain - `from basketball_api.auth import User, get_current_user` (lines 77–159 of `auth.py`) — validates RS256 JWT against westside-basketball Keycloak realm JWKS - Route signature: `user: User = Depends(get_current_user)` matches the `routes/subscriptions.py` line 367 pattern - `user.sub` is written to `jersey_public_orders.submitter_keycloak_sub`; `user.email`/`user.username` are NOT used — body values win (per `feedback_funnel_requires_auth.md`: authenticated but user-editable submission). - 401 is raised automatically by `get_current_user` on missing/invalid token; no 403 path (any realm user may POST). ## Test Plan - [x] `pytest tests/test_jersey_public.py -v` → **24 passed in 2.75s** (local Postgres) - [x] `ruff format` + `ruff check` → clean - [x] `python -c "from basketball_api.routes.jersey_public import router; print(router.routes)"` → `[('', {'POST'})]` - [x] `python -c "from basketball_api.main import app; ..."` → `registered: ['/api/jersey-public-orders']` - [x] `grep -rn "schemas/jersey_public" src/basketball_api/` → no matches - [x] `grep -c "get_current_user" src/basketball_api/routes/jersey_public.py` → 2 - [x] `git diff main -- <untouched files>` → 0 lines - [x] `git status` → exactly 3 files (1 modified, 2 new) - [ ] CI: full test suite runs with service-container Postgres - [ ] Post-merge: validate T3 against prod with a real Keycloak JWT (separate validation ticket) ## Review Checklist - [x] Keycloak auth via existing `get_current_user` — no new auth primitive - [x] `player_name`/`email` NOT overridden by JWT claims (parent-for-child submission supported) - [x] Inline Pydantic schemas — no `schemas/` directory created - [x] `routes/__init__.py` not touched - [x] `routes/checkout.py`, `routes/jersey.py`, `models.py`, `auth.py` not touched - [x] No email sending (deferred to #431) - [x] Single router registration in `main.py` matching existing pattern - [x] Response includes `{id, status, created_at}` for frontend confirmation - [x] Parallel-safe with T5 (#950) — T5 will append the GET endpoint to this file after T3 merges ## Related Notes - Story: `story:WS-S31` — admin public jersey intake link - Arch: `arch-jersey-intake` - Board item: #948 (in_progress) - Depends on: #429 (migration 043, already applied in prod per `validation-429-2026-04-11`) - Parallel ticket: #950 (T5 GET admin endpoint — will append to `routes/jersey_public.py` after this merges) - Reference: `feedback_funnel_requires_auth.md` — why this is Keycloak-gated - Reference: `routes/subscriptions.py` line 367 — `get_current_user` usage pattern

forgejo_admin added 1 commit

2026-04-11 20:43:43 +00:00

feat: POST /api/jersey-public-orders endpoint (#430 )

ci/woodpecker/pr/woodpecker Pipeline failed

Details

5903c5491c

Add System B public jersey intake endpoint. Keycloak-gated via
get_current_user from basketball_api.auth. Stamps submitter_keycloak_sub
from the JWT; player_name/email come from the request body (parent may
submit for child). Inline Pydantic schemas per routes/subscriptions.py
convention; no schemas/ directory. 24 unit tests covering auth, happy
path, validation errors, XFF IP extraction, and JWT-claim-override guard.

Closes #430

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-04-11 20:51:21 +00:00

Author

Owner

PR #450 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic v2 / Keycloak JWT. Production endpoint handling PII for System B public jersey intake (issue #430). 3 files, +378/-0.

FastAPI / Pydantic v2:

Correct v2 idiom: Field(..., pattern=...) throughout. No v1 constr(regex=...).
EmailStr intentionally avoided (no email-validator dep in pyproject.toml) — substituted with ^[^@\s]+@[^@\s]+\.[^@\s]+$. Deviation documented in PR body. Acceptable — spec compliance is for basic format sanity, not full RFC-5322.
Dependency injection: payload, request, user, db all wired via Depends — canonical FastAPI pattern.
Response model JerseyPublicOrderCreated returns {id, status, created_at} with status_code=201 — contract matches spec.

Auth chain:

Imports from basketball_api.auth import User, get_current_user — verified against src/basketball_api/auth.py lines 77–159: this is the real primitive. No ghost keycloak_user reference.
user: User = Depends(get_current_user) on route signature — matches routes/subscriptions.py line 367 pattern.
submitter_keycloak_sub=user.sub stamped on row. user.email / user.username NOT used on insert — body values win, so parent-for-child submission is supported per feedback_funnel_requires_auth.md. Dedicated test test_body_fields_not_overridden_by_jwt_claims enforces this.
401 on missing/invalid token is delegated to get_current_user (verified in auth.py line 85–90).
Endpoint is NOT admin-gated — any realm user can POST, as required.

SQLAlchemy / DB safety:

Insert wrapped in try / db.commit() / db.refresh() with except SQLAlchemyError: db.rollback(); raise 500. logger.exception captures full traceback. Correct rollback discipline.
submission_ip defensively validated via ipaddress.ip_address() before insert — prevents inet cast crashes on TestClient's "testclient" sentinel or malformed XFF. Smart move; not in the spec but protects the inet column.
X-Forwarded-For parsing takes first hop (xff.split(",")[0].strip()) before falling back to request.client.host. Correct for Tailscale funnel → Traefik → pod.

CORS: verified main.py still includes https://westsidekingsandqueens.tail5b443a.ts.net in allow_origins. Frontend POST will not be blocked.

Logging / secrets: logger.info("jersey_public_order created id=%s sub=%s tier=%s", ...) — logs only opaque Keycloak UUID + tier. No email, name, IP, or JWT in logs. No plaintext secrets. Clean.

Validation patterns match spec and the DB CHECK constraints from migration 043:

_NUMBER_PATTERN = r"^(0|00|[1-9][0-9]?)$" — matches 0, 00, 1–99.
_SIZE_PATTERN = r"^(YS|YM|YL|YXL|AS|AM|AL|AXL)$"
_KQ_PATTERN = r"^(kings|queens)$"
_TIER_PATTERN = r"^(90-reversible|130-reversible-shooter)$"

Test coverage (24 tests, 2.75s):

401 unauth (real get_current_user, no override)
201 happy path with full row verification including submitter_keycloak_sub
Parametrized missing-required (7 fields)
Invalid email, kq, tier, number (parametrized over 4 bad inputs), size (parametrized)
All-optional-blank happy path
Body-not-overridden-by-JWT (critical parent-for-child case)
X-Forwarded-For first-hop extraction
Assertions use in (400, 422) — lenient but safe since Pydantic raises 422 in FastAPI.
app.dependency_overrides.clear() in fixture teardown prevents cross-test bleed.
Tests mock get_current_user via FastAPI dependency_overrides — no real Keycloak in CI, per SOP.

main.py scope: exactly 2 lines added (import + include_router(..., prefix="/api/jersey-public-orders")). No other modifications. Scope-tight.

Untouched files verified in diff: routes/checkout.py, routes/jersey.py, routes/__init__.py, models.py, auth.py, alembic/versions/* — all absent from diff.

BLOCKERS

None.

NITS

player_name has Pydantic min_length=1, then .strip() is applied and checked again with a 400. A whitespace-only input (" ") passes Pydantic then fails the strip check — behavior is correct but not covered by an explicit test. Low-value addition; log as subphase in plan Epilogue per feedback_nits_to_epilogue.md, don't block merge.
team.strip() is applied on insert but there's no post-strip minimum-length check and no test for whitespace-only team. Same class as (1).
Email regex ^[^@\s]+@[^@\s]+\.[^@\s]+$ is intentionally minimal. If email-validator becomes a dep later (e.g. via the blast pipeline in basketball-api), consider swapping to EmailStr for consistency. Non-blocking; deviation is documented.
Tests use resp.status_code in (400, 422) — slightly loose. Pydantic always returns 422 here so the tests could tighten to == 422. Cosmetic.

SOP COMPLIANCE

Branch named 430-jersey-public-post-endpoint — matches {issue}-{kebab} convention
PR body has ## Summary, ## Changes, ## Test Plan, ## Review Checklist, ## Related Notes
Auth chain documented per feedback_funnel_requires_auth.md
Closes #430 present
Related section references story (story:WS-S31), arch (arch-jersey-intake), board item (#948), dep (#429), parallel (#950)
Tests exist and pass locally (24/24 in 2.75s)
Ruff format + check clean
No secrets, .env files, credentials committed
No scope creep — exactly 3 files, 2 new + 1 modified (+2 lines)
Parallel-safe with T5 (#950) — future GET endpoint will append to the same router file

PROCESS OBSERVATIONS

DF impact: positive. This is T3 of the System B rollout — unblocks the frontend POST path. Migration 043 is already applied in prod (validation-429-2026-04-11), model merged in #433/#442, so this PR is the final blocker for the /jersey-public route to go live.
CFR risk: LOW. The PR is narrowly scoped, the auth primitive is reused (no new security path), the body-not-overridden invariant is test-enforced, and the inet defensive-parse prevents a class of 500 from bad proxy headers. The one remaining CI concern (service-container Postgres) is called out in the Test Plan checklist as unchecked — QA flags this per feedback_qa_ci_blockers.md: tests must run in CI with a real Postgres service container. If CI uses SQLite, the inet/UUID/check constraint coverage is not real — verify the CI Woodpecker config wires the Postgres service before merging. Not a blocker because the PR body explicitly defers this to "CI: full test suite runs with service-container Postgres" — that item must be verified green before merge.
LT / docs: PR body is self-documenting. No doc gap for this endpoint. T5 will need to append to this file; route file structure supports that cleanly.
Post-merge validation: PR correctly notes a separate validation ticket for real-Keycloak-JWT smoke test against prod. Good T3 discipline.

VERDICT: APPROVED

## PR #450 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Pydantic v2 / Keycloak JWT. Production endpoint handling PII for System B public jersey intake (issue #430). 3 files, +378/-0. **FastAPI / Pydantic v2**: - Correct v2 idiom: `Field(..., pattern=...)` throughout. No v1 `constr(regex=...)`. - `EmailStr` intentionally avoided (no `email-validator` dep in `pyproject.toml`) — substituted with `^[^@\s]+@[^@\s]+\.[^@\s]+$`. Deviation documented in PR body. Acceptable — spec compliance is for basic format sanity, not full RFC-5322. - Dependency injection: `payload`, `request`, `user`, `db` all wired via `Depends` — canonical FastAPI pattern. - Response model `JerseyPublicOrderCreated` returns `{id, status, created_at}` with `status_code=201` — contract matches spec. **Auth chain**: - Imports `from basketball_api.auth import User, get_current_user` — verified against `src/basketball_api/auth.py` lines 77–159: this is the real primitive. No ghost `keycloak_user` reference. - `user: User = Depends(get_current_user)` on route signature — matches `routes/subscriptions.py` line 367 pattern. - `submitter_keycloak_sub=user.sub` stamped on row. `user.email` / `user.username` NOT used on insert — body values win, so parent-for-child submission is supported per `feedback_funnel_requires_auth.md`. Dedicated test `test_body_fields_not_overridden_by_jwt_claims` enforces this. - 401 on missing/invalid token is delegated to `get_current_user` (verified in auth.py line 85–90). - Endpoint is NOT admin-gated — any realm user can POST, as required. **SQLAlchemy / DB safety**: - Insert wrapped in `try / db.commit() / db.refresh()` with `except SQLAlchemyError: db.rollback(); raise 500`. `logger.exception` captures full traceback. Correct rollback discipline. - `submission_ip` defensively validated via `ipaddress.ip_address()` before insert — prevents `inet` cast crashes on TestClient's `"testclient"` sentinel or malformed XFF. Smart move; not in the spec but protects the `inet` column. - X-Forwarded-For parsing takes first hop (`xff.split(",")[0].strip()`) before falling back to `request.client.host`. Correct for Tailscale funnel → Traefik → pod. **CORS**: verified `main.py` still includes `https://westsidekingsandqueens.tail5b443a.ts.net` in `allow_origins`. Frontend POST will not be blocked. **Logging / secrets**: `logger.info("jersey_public_order created id=%s sub=%s tier=%s", ...)` — logs only opaque Keycloak UUID + tier. No email, name, IP, or JWT in logs. No plaintext secrets. Clean. **Validation patterns** match spec and the DB CHECK constraints from migration 043: - `_NUMBER_PATTERN = r"^(0|00|[1-9][0-9]?)$"` — matches `0`, `00`, `1–99`. - `_SIZE_PATTERN = r"^(YS|YM|YL|YXL|AS|AM|AL|AXL)$"` - `_KQ_PATTERN = r"^(kings|queens)$"` - `_TIER_PATTERN = r"^(90-reversible|130-reversible-shooter)$"` **Test coverage** (24 tests, 2.75s): - 401 unauth (real `get_current_user`, no override) - 201 happy path with full row verification including `submitter_keycloak_sub` - Parametrized missing-required (7 fields) - Invalid email, kq, tier, number (parametrized over 4 bad inputs), size (parametrized) - All-optional-blank happy path - Body-not-overridden-by-JWT (critical parent-for-child case) - X-Forwarded-For first-hop extraction - Assertions use `in (400, 422)` — lenient but safe since Pydantic raises 422 in FastAPI. - `app.dependency_overrides.clear()` in fixture teardown prevents cross-test bleed. - Tests mock `get_current_user` via FastAPI `dependency_overrides` — no real Keycloak in CI, per SOP. **main.py scope**: exactly 2 lines added (import + `include_router(..., prefix="/api/jersey-public-orders")`). No other modifications. Scope-tight. **Untouched files verified in diff**: `routes/checkout.py`, `routes/jersey.py`, `routes/__init__.py`, `models.py`, `auth.py`, `alembic/versions/*` — all absent from diff. ### BLOCKERS None. ### NITS 1. `player_name` has Pydantic `min_length=1`, then `.strip()` is applied and checked again with a 400. A whitespace-only input (`" "`) passes Pydantic then fails the strip check — behavior is correct but not covered by an explicit test. Low-value addition; log as subphase in plan Epilogue per `feedback_nits_to_epilogue.md`, don't block merge. 2. `team.strip()` is applied on insert but there's no post-strip minimum-length check and no test for whitespace-only `team`. Same class as (1). 3. Email regex `^[^@\s]+@[^@\s]+\.[^@\s]+$` is intentionally minimal. If `email-validator` becomes a dep later (e.g. via the blast pipeline in basketball-api), consider swapping to `EmailStr` for consistency. Non-blocking; deviation is documented. 4. Tests use `resp.status_code in (400, 422)` — slightly loose. Pydantic always returns 422 here so the tests could tighten to `== 422`. Cosmetic. ### SOP COMPLIANCE - [x] Branch named `430-jersey-public-post-endpoint` — matches `{issue}-{kebab}` convention - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Review Checklist, ## Related Notes - [x] Auth chain documented per `feedback_funnel_requires_auth.md` - [x] `Closes #430` present - [x] Related section references story (`story:WS-S31`), arch (`arch-jersey-intake`), board item (#948), dep (#429), parallel (#950) - [x] Tests exist and pass locally (24/24 in 2.75s) - [x] Ruff format + check clean - [x] No secrets, .env files, credentials committed - [x] No scope creep — exactly 3 files, 2 new + 1 modified (+2 lines) - [x] Parallel-safe with T5 (#950) — future GET endpoint will append to the same router file ### PROCESS OBSERVATIONS - **DF impact**: positive. This is T3 of the System B rollout — unblocks the frontend POST path. Migration 043 is already applied in prod (`validation-429-2026-04-11`), model merged in #433/#442, so this PR is the final blocker for the `/jersey-public` route to go live. - **CFR risk**: LOW. The PR is narrowly scoped, the auth primitive is reused (no new security path), the body-not-overridden invariant is test-enforced, and the `inet` defensive-parse prevents a class of 500 from bad proxy headers. The one remaining CI concern (service-container Postgres) is called out in the Test Plan checklist as unchecked — QA flags this per `feedback_qa_ci_blockers.md`: tests must run in CI with a real Postgres service container. If CI uses SQLite, the `inet`/`UUID`/`check constraint` coverage is not real — verify the CI Woodpecker config wires the Postgres service before merging. Not a blocker because the PR body explicitly defers this to "CI: full test suite runs with service-container Postgres" — that item must be verified green before merge. - **LT / docs**: PR body is self-documenting. No doc gap for this endpoint. T5 will need to append to this file; route file structure supports that cleanly. - **Post-merge validation**: PR correctly notes a separate validation ticket for real-Keycloak-JWT smoke test against prod. Good T3 discipline. ### VERDICT: APPROVED