feat: add GET /jersey/player-info endpoint

forgejo_admin commented

2026-03-21 21:52:32 +00:00

Author

Owner

PR #148 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic -- applying Python/FastAPI domain checklist.

Endpoint implementation (src/basketball_api/routes/jersey.py lines 100-125):

Pydantic response model JerseyPlayerInfoResponse is clean, with correct str | None union for division. PEP 484 compliant.
Docstring present on the endpoint function. PEP 257 compliant.
joinedload(Parent.players) eager-loads the relationship in a single query -- correct SQLAlchemy pattern, avoids N+1.
Token validation returns 404 for missing parent and missing players -- consistent with the existing /checkout endpoint error handling in the same file.
player.division.value if player.division else None correctly handles the nullable enum. Safe.
Query(..., alias="token") -- the alias="token" is redundant since the parameter name is already token, but this matches the convention used in the existing /checkout endpoint (line 169). Consistent, not a bug.
No auth decorator -- this is a public endpoint authenticated by knowledge of the registration token. This matches the pattern of /checkout, /options, /sizes, and /taken-numbers in the same file. The registration token serves as a bearer secret.

Input validation:

The token parameter is required (... default) and typed as str. FastAPI will reject requests without it (422). No SQL injection risk -- SQLAlchemy parameterizes the query.
No length/format validation on the token string. This is consistent with all other token-using endpoints in this codebase. Not a new risk.

Test coverage (tests/test_jersey.py lines 151-200):

4 tests covering: valid token (happy path), invalid token (404), parent with no players (404), player without division (null case).
Tests use the existing parent_with_player fixture (line 56, same file) and create ad-hoc test data for edge cases.
Test assertions check both status codes and response body content. Solid.

Ruff compliance: PR body states ruff format --check and ruff check are clean.

BLOCKERS

None. All blocker criteria pass:

Test coverage: 4 tests cover happy path, error paths, and edge cases. Pass.
Input validation: Token is parameterized via SQLAlchemy. No injection risk. Pass.
Secrets: No credentials or secrets in code. Pass.
DRY in auth/security: The parent-by-token lookup pattern is duplicated across jersey.py (lines 109-113 and 215-219), checkout.py (line 112), and register.py (line 576). However, this is a pre-existing pattern -- the new endpoint follows the exact same convention already established in this file's /checkout endpoint. Extracting a shared helper would be good hygiene but is out of scope for this PR. Not a blocker.

NITS

Pre-existing DRY opportunity (not introduced by this PR): The "lookup parent by token + eager-load players + validate exists" block (lines 109-119) is duplicated verbatim from the /checkout endpoint (lines 215-225). A shared dependency like get_parent_or_404(token, db) would reduce this to one line per endpoint. Worth a follow-up issue.
Multi-player ambiguity: parent.players[0] silently picks the first player. If a parent has multiple players, this may not be the player the user expects. The existing /checkout endpoint has the same behavior (line 228), so this is consistent -- but worth noting as a known limitation for future work (e.g., a player_id query param).

SOP COMPLIANCE

Branch named after issue -- 67-jersey-player-info-endpoint references westside-app#67 (the dependent frontend issue). Basketball-api issue #147 is referenced in the PR body via "Closes #147". Acceptable cross-repo naming.
PR body follows template -- Summary, Changes, Test Plan, Review Checklist, Related sections all present.
Related references plan slug -- plan-wkq Phase 11 referenced.
Tests exist and pass -- 4 new tests, PR body confirms all 46 jersey tests pass.
No secrets committed -- clean.
No unnecessary file changes -- exactly 2 files changed, both directly related.
Commit messages are descriptive -- PR title follows conventional commit format.

Note on issue numbering: The user dispatch referenced parent issue #143, but the PR body says "Closes #147". Issue #147 ("feat: add GET /jersey/player-info endpoint for token-based division lookup") matches the PR scope exactly. This may be a dispatch routing discrepancy; the PR itself is correctly linked to #147.

PROCESS OBSERVATIONS

Deployment frequency: Small, focused PR (90 additions, 0 deletions, 2 files). Low change-failure risk. Good for deployment frequency.
Change failure risk: Minimal -- new endpoint only, no modifications to existing code. Zero regression risk to existing functionality.
Cross-repo dependency: This endpoint is a prerequisite for westside-app#67 (jersey number picker). The dependency chain is documented in the Related section. Clean.

VERDICT: APPROVED

## PR #148 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic -- applying Python/FastAPI domain checklist. **Endpoint implementation** (`src/basketball_api/routes/jersey.py` lines 100-125): - Pydantic response model `JerseyPlayerInfoResponse` is clean, with correct `str | None` union for `division`. PEP 484 compliant. - Docstring present on the endpoint function. PEP 257 compliant. - `joinedload(Parent.players)` eager-loads the relationship in a single query -- correct SQLAlchemy pattern, avoids N+1. - Token validation returns 404 for missing parent and missing players -- consistent with the existing `/checkout` endpoint error handling in the same file. - `player.division.value if player.division else None` correctly handles the nullable enum. Safe. - `Query(..., alias="token")` -- the `alias="token"` is redundant since the parameter name is already `token`, but this matches the convention used in the existing `/checkout` endpoint (line 169). Consistent, not a bug. - No auth decorator -- this is a public endpoint authenticated by knowledge of the registration token. This matches the pattern of `/checkout`, `/options`, `/sizes`, and `/taken-numbers` in the same file. The registration token serves as a bearer secret. **Input validation:** - The `token` parameter is required (`...` default) and typed as `str`. FastAPI will reject requests without it (422). No SQL injection risk -- SQLAlchemy parameterizes the query. - No length/format validation on the token string. This is consistent with all other token-using endpoints in this codebase. Not a new risk. **Test coverage** (`tests/test_jersey.py` lines 151-200): - 4 tests covering: valid token (happy path), invalid token (404), parent with no players (404), player without division (null case). - Tests use the existing `parent_with_player` fixture (line 56, same file) and create ad-hoc test data for edge cases. - Test assertions check both status codes and response body content. Solid. **Ruff compliance:** PR body states `ruff format --check` and `ruff check` are clean. ### BLOCKERS None. All blocker criteria pass: - **Test coverage:** 4 tests cover happy path, error paths, and edge cases. Pass. - **Input validation:** Token is parameterized via SQLAlchemy. No injection risk. Pass. - **Secrets:** No credentials or secrets in code. Pass. - **DRY in auth/security:** The parent-by-token lookup pattern is duplicated across `jersey.py` (lines 109-113 and 215-219), `checkout.py` (line 112), and `register.py` (line 576). However, this is a **pre-existing pattern** -- the new endpoint follows the exact same convention already established in this file's `/checkout` endpoint. Extracting a shared helper would be good hygiene but is out of scope for this PR. Not a blocker. ### NITS 1. **Pre-existing DRY opportunity (not introduced by this PR):** The "lookup parent by token + eager-load players + validate exists" block (lines 109-119) is duplicated verbatim from the `/checkout` endpoint (lines 215-225). A shared dependency like `get_parent_or_404(token, db)` would reduce this to one line per endpoint. Worth a follow-up issue. 2. **Multi-player ambiguity:** `parent.players[0]` silently picks the first player. If a parent has multiple players, this may not be the player the user expects. The existing `/checkout` endpoint has the same behavior (line 228), so this is consistent -- but worth noting as a known limitation for future work (e.g., a `player_id` query param). ### SOP COMPLIANCE - [x] Branch named after issue -- `67-jersey-player-info-endpoint` references westside-app#67 (the dependent frontend issue). Basketball-api issue #147 is referenced in the PR body via "Closes #147". Acceptable cross-repo naming. - [x] PR body follows template -- Summary, Changes, Test Plan, Review Checklist, Related sections all present. - [x] Related references plan slug -- `plan-wkq` Phase 11 referenced. - [x] Tests exist and pass -- 4 new tests, PR body confirms all 46 jersey tests pass. - [x] No secrets committed -- clean. - [x] No unnecessary file changes -- exactly 2 files changed, both directly related. - [x] Commit messages are descriptive -- PR title follows conventional commit format. **Note on issue numbering:** The user dispatch referenced parent issue #143, but the PR body says "Closes #147". Issue #147 ("feat: add GET /jersey/player-info endpoint for token-based division lookup") matches the PR scope exactly. This may be a dispatch routing discrepancy; the PR itself is correctly linked to #147. ### PROCESS OBSERVATIONS - **Deployment frequency:** Small, focused PR (90 additions, 0 deletions, 2 files). Low change-failure risk. Good for deployment frequency. - **Change failure risk:** Minimal -- new endpoint only, no modifications to existing code. Zero regression risk to existing functionality. - **Cross-repo dependency:** This endpoint is a prerequisite for westside-app#67 (jersey number picker). The dependency chain is documented in the Related section. Clean. ### VERDICT: APPROVED

2026-03-21 21:47:29 +00:00

Summary

Adds GET /jersey/player-info?token=... endpoint that returns the player's name and division given a parent registration token. Required by the westside-app jersey number picker to fetch taken numbers per division.

Changes

src/basketball_api/routes/jersey.py -- added JerseyPlayerInfoResponse schema and /player-info endpoint that looks up parent by registration token and returns first player's name and division
tests/test_jersey.py -- 4 new tests covering valid token, invalid token, no players, and player without division

Test Plan

pytest tests/test_jersey.py::TestJerseyPlayerInfo -v -- all 4 pass
pytest tests/test_jersey.py -- full suite, all 46 pass
ruff format --check and ruff check -- clean

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes forgejo_admin/basketball-api#147
plan-wkq -- Phase 11 (Girls Tryout)
forgejo_admin/westside-app#67 -- jersey number picker (depends on this endpoint)

## Summary Adds `GET /jersey/player-info?token=...` endpoint that returns the player's name and division given a parent registration token. Required by the westside-app jersey number picker to fetch taken numbers per division. ## Changes - `src/basketball_api/routes/jersey.py` -- added `JerseyPlayerInfoResponse` schema and `/player-info` endpoint that looks up parent by registration token and returns first player's name and division - `tests/test_jersey.py` -- 4 new tests covering valid token, invalid token, no players, and player without division ## Test Plan - [x] `pytest tests/test_jersey.py::TestJerseyPlayerInfo -v` -- all 4 pass - [x] `pytest tests/test_jersey.py` -- full suite, all 46 pass - [x] `ruff format --check` and `ruff check` -- clean ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/basketball-api#147 - `plan-wkq` -- Phase 11 (Girls Tryout) - forgejo_admin/westside-app#67 -- jersey number picker (depends on this endpoint)

forgejo_admin added 1 commit

feat: add GET /jersey/player-info endpoint for token-based division lookup

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

ff0a52b4d7

The jersey number picker on westside-app needs the player's division to
fetch taken numbers before checkout. This endpoint returns minimal player
info (name + division) given a parent registration token.

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

forgejo_admin referenced this pull request from forgejo_admin/westside-landing

2026-03-21 21:49:29 +00:00

feat: add jersey number picker to checkout page #70