ldraney/basketball-api

Fork 0

feat: add jersey fields to PlayerProfileResponse (#257) #259

Merged

forgejo_admin merged 1 commit from 257-jersey-profile-fields into main

2026-03-30 21:00:56 +00:00

forgejo_admin commented

2026-03-30 16:26:54 +00:00

Contributor

Summary

Expose jersey_option, jersey_size, jersey_number, and jersey_order_status in the player profile endpoint so the parent-facing UI can render jersey ordering status. These fields already exist on the Player model and were already exposed via AdminPlayerItem — this mirrors them for the authenticated player profile route.

Changes

src/basketball_api/routes/players.py — Added 4 jersey fields (jersey_option, jersey_size, jersey_number, jersey_order_status) to PlayerProfileResponse schema and mapped them in _player_profile_response() helper with proper enum-to-string conversion
tests/test_players.py — Added 2 new tests: test_player_profile_includes_jersey_fields (verifies all 4 fields return correct values) and test_player_profile_jersey_fields_null_when_empty (verifies null/default behavior)

Test Plan

pytest tests/ -k test_player_profile — both new tests pass
Full suite: 691 passed, 0 failed
ruff format and ruff check clean

Review Checklist

Only touches routes/players.py and tests/test_players.py — no unrelated changes
Follows existing pattern from _player_profile_response() helper
Field names match AdminPlayerItem for consistency
Enum values serialized to strings via .value
Null-safe: fields return None when unset (except jersey_order_status which defaults to "none")
No new dependencies

None

Closes #257
forgejo_admin/westside-landing#197 — frontend consumer of these fields
forgejo_admin/basketball-api#248 / #249 — admin version of same fields

## Summary Expose `jersey_option`, `jersey_size`, `jersey_number`, and `jersey_order_status` in the player profile endpoint so the parent-facing UI can render jersey ordering status. These fields already exist on the Player model and were already exposed via AdminPlayerItem — this mirrors them for the authenticated player profile route. ## Changes - `src/basketball_api/routes/players.py` — Added 4 jersey fields (`jersey_option`, `jersey_size`, `jersey_number`, `jersey_order_status`) to `PlayerProfileResponse` schema and mapped them in `_player_profile_response()` helper with proper enum-to-string conversion - `tests/test_players.py` — Added 2 new tests: `test_player_profile_includes_jersey_fields` (verifies all 4 fields return correct values) and `test_player_profile_jersey_fields_null_when_empty` (verifies null/default behavior) ## Test Plan - `pytest tests/ -k test_player_profile` — both new tests pass - Full suite: 691 passed, 0 failed - `ruff format` and `ruff check` clean ## Review Checklist - [x] Only touches `routes/players.py` and `tests/test_players.py` — no unrelated changes - [x] Follows existing pattern from `_player_profile_response()` helper - [x] Field names match AdminPlayerItem for consistency - [x] Enum values serialized to strings via `.value` - [x] Null-safe: fields return `None` when unset (except `jersey_order_status` which defaults to `"none"`) - [x] No new dependencies ## Related Notes - None ## Related - Closes #257 - `forgejo_admin/westside-landing#197` — frontend consumer of these fields - `forgejo_admin/basketball-api#248` / `#249` — admin version of same fields

forgejo_admin added 1 commit

2026-03-30 16:26:54 +00:00

feat: add jersey fields to PlayerProfileResponse (#257 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

ea7624b32d

Expose jersey_option, jersey_size, jersey_number, and jersey_order_status
in the player profile endpoint so the parent-facing UI can render jersey
ordering status. Mirrors the fields already present on AdminPlayerItem.

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

forgejo_admin commented

2026-03-30 16:27:28 +00:00

Author

Contributor

QA Review -- PR #259

Diff Summary

2 files changed, 49 additions, 0 deletions
src/basketball_api/routes/players.py -- 4 fields added to schema + helper mapping
tests/test_players.py -- 2 new tests covering populated and empty jersey data

Findings

Schema (PlayerProfileResponse): All 4 jersey fields added as str | None, consistent with how division and other enum-backed fields are typed. Field placement between subscription_status and team/parent is logical.

Helper mapping (_player_profile_response): Enum-to-string conversion via .value if X else None follows the existing pattern for division. jersey_number is a plain string passthrough -- correct per the model (String(2)).

Note: jersey_order_status has a defensive if player.jersey_order_status else None guard. The model column is non-nullable with a server_default of "none", so it should always have a value after DB fetch. The guard is harmless but technically unnecessary. Not a blocker.

Tests: Both tests are well-structured, use existing fixtures, and cover the acceptance criteria:

Populated jersey data returns correct enum string values
Empty jersey data returns None for nullable fields and "none" for jersey_order_status

Acceptance Criteria

GET /api/players/{id} includes jersey_option, jersey_size, jersey_number, jersey_order_status
Null fields when player has no jersey data
Correct values when player has jersey data
No unrelated changes
Tests pass (691 passed, 0 failed)
ruff clean

VERDICT: APPROVED

## QA Review -- PR #259 ### Diff Summary - 2 files changed, 49 additions, 0 deletions - `src/basketball_api/routes/players.py` -- 4 fields added to schema + helper mapping - `tests/test_players.py` -- 2 new tests covering populated and empty jersey data ### Findings **Schema (PlayerProfileResponse):** All 4 jersey fields added as `str | None`, consistent with how `division` and other enum-backed fields are typed. Field placement between `subscription_status` and `team`/`parent` is logical. **Helper mapping (_player_profile_response):** Enum-to-string conversion via `.value if X else None` follows the existing pattern for `division`. `jersey_number` is a plain string passthrough -- correct per the model (`String(2)`). **Note:** `jersey_order_status` has a defensive `if player.jersey_order_status else None` guard. The model column is non-nullable with a server_default of `"none"`, so it should always have a value after DB fetch. The guard is harmless but technically unnecessary. Not a blocker. **Tests:** Both tests are well-structured, use existing fixtures, and cover the acceptance criteria: - Populated jersey data returns correct enum string values - Empty jersey data returns `None` for nullable fields and `"none"` for `jersey_order_status` ### Acceptance Criteria - [x] GET /api/players/{id} includes `jersey_option`, `jersey_size`, `jersey_number`, `jersey_order_status` - [x] Null fields when player has no jersey data - [x] Correct values when player has jersey data - [x] No unrelated changes - [x] Tests pass (691 passed, 0 failed) - [x] ruff clean ### VERDICT: APPROVED

forgejo_admin commented

2026-03-30 16:29:26 +00:00

Author

Contributor

PR #259 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

Schema correctness: All 4 jersey fields added to PlayerProfileResponse with str | None type annotations. This matches the existing pattern used by division (enum serialized to string, nullable). Consistent with how the codebase handles enum-backed optional fields.

Helper mapping correctness: _player_profile_response() correctly maps all 4 fields:

jersey_option and jersey_size use .value with null guard -- matches the division pattern on line 116.
jersey_number is passed through directly (plain String(2) column, no enum) -- correct.
jersey_order_status uses .value with null guard. The model declares this as Mapped[JerseyOrderStatus] (non-optional) with server_default="none", so the null branch should never execute for database-persisted rows. The defensive guard is fine.

Test coverage: Two tests added:

test_player_profile_includes_jersey_fields -- sets all 4 fields on sample_player, verifies correct enum-to-string serialization in the response. Good.
test_player_profile_jersey_fields_null_when_empty -- uses the default fixture (no jersey data), verifies None for nullable fields and "none" for the server-defaulted jersey_order_status. Good.

Both happy path and null/default edge cases are covered.

PEP compliance: Type hints present. The multi-line conditional for jersey_order_status follows ruff formatting conventions (confirmed by PR body: ruff format + ruff check clean).

BLOCKERS

None.

NITS

NIT: jersey_order_status: str | None in the schema is more permissive than the model guarantees (Mapped[JerseyOrderStatus] with server_default, never null in practice). Consider jersey_order_status: str (non-optional) for tighter API contract accuracy. Not blocking -- the current approach is defensively safe and matches the helper's null guard.

SOP COMPLIANCE

Branch named after issue (257-jersey-profile-fields)
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related section references closing issue (#257) and cross-repo context (#248, #249, westside-landing#197)
Tests exist and pass (691 passed per PR body)
No secrets committed
No unnecessary file changes -- exactly 2 files, 49 additions, 0 deletions
Commit message is descriptive

PROCESS OBSERVATIONS

Clean, minimal PR. 49 lines added across 2 files. Follows established patterns exactly (mirrors division field handling). No scope creep. Good cross-repo traceability to the frontend consumer (westside-landing#197) and the admin-side precedent (#248/#249).

VERDICT: APPROVED

## PR #259 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic **Schema correctness:** All 4 jersey fields added to `PlayerProfileResponse` with `str | None` type annotations. This matches the existing pattern used by `division` (enum serialized to string, nullable). Consistent with how the codebase handles enum-backed optional fields. **Helper mapping correctness:** `_player_profile_response()` correctly maps all 4 fields: - `jersey_option` and `jersey_size` use `.value` with null guard -- matches the `division` pattern on line 116. - `jersey_number` is passed through directly (plain `String(2)` column, no enum) -- correct. - `jersey_order_status` uses `.value` with null guard. The model declares this as `Mapped[JerseyOrderStatus]` (non-optional) with `server_default="none"`, so the null branch should never execute for database-persisted rows. The defensive guard is fine. **Test coverage:** Two tests added: - `test_player_profile_includes_jersey_fields` -- sets all 4 fields on `sample_player`, verifies correct enum-to-string serialization in the response. Good. - `test_player_profile_jersey_fields_null_when_empty` -- uses the default fixture (no jersey data), verifies `None` for nullable fields and `"none"` for the server-defaulted `jersey_order_status`. Good. Both happy path and null/default edge cases are covered. **PEP compliance:** Type hints present. The multi-line conditional for `jersey_order_status` follows ruff formatting conventions (confirmed by PR body: ruff format + ruff check clean). ### BLOCKERS None. ### NITS 1. NIT: `jersey_order_status: str | None` in the schema is more permissive than the model guarantees (`Mapped[JerseyOrderStatus]` with server_default, never null in practice). Consider `jersey_order_status: str` (non-optional) for tighter API contract accuracy. Not blocking -- the current approach is defensively safe and matches the helper's null guard. ### SOP COMPLIANCE - [x] Branch named after issue (`257-jersey-profile-fields`) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related section references closing issue (#257) and cross-repo context (#248, #249, westside-landing#197) - [x] Tests exist and pass (691 passed per PR body) - [x] No secrets committed - [x] No unnecessary file changes -- exactly 2 files, 49 additions, 0 deletions - [x] Commit message is descriptive ### PROCESS OBSERVATIONS Clean, minimal PR. 49 lines added across 2 files. Follows established patterns exactly (mirrors `division` field handling). No scope creep. Good cross-repo traceability to the frontend consumer (westside-landing#197) and the admin-side precedent (#248/#249). ### VERDICT: APPROVED