feat: add GET /admin/players/{player_id} detail endpoint

forgejo_admin commented

2026-03-29 20:08:23 +00:00

Contributor

Summary

Adds a comprehensive player detail endpoint GET /admin/players/{player_id} for the CRM detail view. Returns all player data needed by the frontend: personal info, parent info, team names, registration/contract/jersey status.

Changes

src/basketball_api/routes/admin.py -- Added AdminPlayerDetail Pydantic response model and admin_player_detail endpoint. Placed after all literal /players/* routes to avoid path parameter conflicts with /players/incomplete and /players/bulk-visibility.
tests/test_admin_spa.py -- Added TestAdminPlayerDetail class with 5 tests: happy path with team, player without team, 404 for nonexistent player, response field completeness, and 401 without auth.

Test Plan

pytest tests/test_admin_spa.py -v -- all 13 tests pass (8 existing + 5 new)
ruff format and ruff check pass clean

Review Checklist

Follows existing admin.py patterns (Pydantic response model, require_admin dependency, joinedload)
Route placed after literal /players/* paths to avoid FastAPI path parameter conflicts
404 returned for nonexistent player
All fields from issue spec included in response model
Tests cover happy path, edge cases, and auth guard
ruff format and ruff check pass

Closes #249

Forgejo issue: #249

## Summary Adds a comprehensive player detail endpoint `GET /admin/players/{player_id}` for the CRM detail view. Returns all player data needed by the frontend: personal info, parent info, team names, registration/contract/jersey status. ## Changes - `src/basketball_api/routes/admin.py` -- Added `AdminPlayerDetail` Pydantic response model and `admin_player_detail` endpoint. Placed after all literal `/players/*` routes to avoid path parameter conflicts with `/players/incomplete` and `/players/bulk-visibility`. - `tests/test_admin_spa.py` -- Added `TestAdminPlayerDetail` class with 5 tests: happy path with team, player without team, 404 for nonexistent player, response field completeness, and 401 without auth. ## Test Plan - `pytest tests/test_admin_spa.py -v` -- all 13 tests pass (8 existing + 5 new) - `ruff format` and `ruff check` pass clean ## Review Checklist - [x] Follows existing admin.py patterns (Pydantic response model, require_admin dependency, joinedload) - [x] Route placed after literal `/players/*` paths to avoid FastAPI path parameter conflicts - [x] 404 returned for nonexistent player - [x] All fields from issue spec included in response model - [x] Tests cover happy path, edge cases, and auth guard - [x] ruff format and ruff check pass ## Related Notes - Closes #249 ## Related - Forgejo issue: #249

forgejo_admin added 1 commit

2026-03-29 20:08:23 +00:00

feat: add GET /admin/players/{player_id} detail endpoint

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

6de34c2c61

Adds a comprehensive player detail endpoint for the CRM detail view,
returning personal info, parent info, team names, registration status,
contract status, and jersey details. Includes 5 tests covering happy
path, teamless player, 404, field completeness, and auth guard.

Closes #249

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

forgejo_admin commented

2026-03-29 20:11:01 +00:00

Author

Contributor

PR #251 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic

Endpoint correctness:

GET /admin/players/{player_id} is properly guarded with require_admin dependency.
404 raised correctly when player is None.
joinedload for Player.teams, Player.parent, and Player.registrations prevents N+1 queries -- consistent with the existing admin_list_players pattern.
Route placement is correct: all literal /players/* routes (/players, /players/incomplete, /players/bulk-visibility) are registered before the {player_id} parameterized route, avoiding FastAPI path-matching conflicts.
Enum .value access is correct for division, contract_status, jersey_option, jersey_size, jersey_order_status.
date_of_birth and contract_signed_at use .isoformat() with proper None guards.
registration_type is a plain str column (not an enum), so no .value call is needed -- correctly handled.

Response model completeness (vs. acceptance criteria):

Personal info: name, photo_url, date_of_birth, height, position, current_school, graduating_class, hometown, division, country -- all present.
Parent info: parent_name, parent_email -- present.
Team/registration: team_names, status (derived from registration payment_status), contract_status, contract_signed_at -- present.
Jersey: jersey_option, jersey_size, jersey_number, jersey_order_status -- present.
Extra field: registration_type -- good addition for CRM context.

Test quality (5 tests):

test_returns_player_detail -- happy path with team, asserts key fields including parent info, team_names, status derivation.
test_returns_player_without_team -- edge case: player with no team assignment, asserts team_names == [] and pending status.
test_404_for_nonexistent_player -- verifies 404 status and error detail message.
test_response_fields -- schema completeness check for all 22 fields.
test_401_without_auth -- auth guard verification using unauthenticated client fixture.

Tests reuse the existing populated_db fixture consistently.

BLOCKERS

None.

All blocker criteria pass:

New functionality has 5 tests covering happy path, edge cases, error handling, and auth.
No unvalidated user input (player_id is typed as int by FastAPI path parameter).
No secrets or credentials in code.
No DRY violation in auth/security paths (the require_admin dependency is reused, not duplicated).

NITS

DRY: Status derivation logic is duplicated. The "derive status from registrations" block (lines ~1101-1111 in the new endpoint) is an exact copy of lines 328-340 in admin_list_players. Same for the reg_type derivation. Consider extracting a helper like _derive_player_status(registrations) -> tuple[str, str | None] that both endpoints call. Not a blocker since this is business logic, not auth/security, but divergence risk increases with each new consumer.
Redundant latest_reg computation. The new endpoint computes latest_reg = max(player.registrations, key=lambda r: r.created_at) twice -- once for status and once for reg_type. These could share a single computation. This is also duplicated from the list endpoint's same pattern.
parent_name and parent_email are non-optional strings. If a Player could theoretically exist without a Parent loaded (e.g., orphaned data), accessing player.parent.name would raise AttributeError. The model has parent_id as non-nullable, so this should be safe in practice, but a defensive guard (or explicit documentation of the invariant) would be slightly more robust. Minor nit.

SOP COMPLIANCE

Branch named after issue: 249-admin-player-detail
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related references issue #249
Related section does not reference a plan slug (no plan slug mentioned -- acceptable if this is standalone kanban work)
No secrets committed
No unrelated file changes (only admin.py + test_admin_spa.py modified)
Commit messages are descriptive (PR title: feat: add GET /admin/players/{player_id} detail endpoint)
ruff format and ruff check reported as passing

PROCESS OBSERVATIONS

Deployment frequency: Clean, focused PR -- single endpoint addition with tests. Good for fast merge-to-deploy cycle.
Change failure risk: Low. No schema migrations, no existing behavior modified, purely additive. The duplicated status derivation logic is low-risk today but creates maintenance surface for future changes to registration status logic.
Pattern consistency: The endpoint follows established patterns in admin.py (Pydantic response model, require_admin, joinedload, manual field mapping). Good codebase coherence.

VERDICT: APPROVED

## PR #251 Review ### DOMAIN REVIEW **Stack:** Python / FastAPI / SQLAlchemy / Pydantic **Endpoint correctness:** - `GET /admin/players/{player_id}` is properly guarded with `require_admin` dependency. - 404 raised correctly when `player` is `None`. - `joinedload` for `Player.teams`, `Player.parent`, and `Player.registrations` prevents N+1 queries -- consistent with the existing `admin_list_players` pattern. - Route placement is correct: all literal `/players/*` routes (`/players`, `/players/incomplete`, `/players/bulk-visibility`) are registered before the `{player_id}` parameterized route, avoiding FastAPI path-matching conflicts. - Enum `.value` access is correct for `division`, `contract_status`, `jersey_option`, `jersey_size`, `jersey_order_status`. - `date_of_birth` and `contract_signed_at` use `.isoformat()` with proper `None` guards. - `registration_type` is a plain `str` column (not an enum), so no `.value` call is needed -- correctly handled. **Response model completeness (vs. acceptance criteria):** - Personal info: `name`, `photo_url`, `date_of_birth`, `height`, `position`, `current_school`, `graduating_class`, `hometown`, `division`, `country` -- all present. - Parent info: `parent_name`, `parent_email` -- present. - Team/registration: `team_names`, `status` (derived from registration payment_status), `contract_status`, `contract_signed_at` -- present. - Jersey: `jersey_option`, `jersey_size`, `jersey_number`, `jersey_order_status` -- present. - Extra field: `registration_type` -- good addition for CRM context. **Test quality (5 tests):** 1. `test_returns_player_detail` -- happy path with team, asserts key fields including parent info, team_names, status derivation. 2. `test_returns_player_without_team` -- edge case: player with no team assignment, asserts `team_names == []` and pending status. 3. `test_404_for_nonexistent_player` -- verifies 404 status and error detail message. 4. `test_response_fields` -- schema completeness check for all 22 fields. 5. `test_401_without_auth` -- auth guard verification using unauthenticated `client` fixture. Tests reuse the existing `populated_db` fixture consistently. ### BLOCKERS None. All blocker criteria pass: - New functionality has 5 tests covering happy path, edge cases, error handling, and auth. - No unvalidated user input (`player_id` is typed as `int` by FastAPI path parameter). - No secrets or credentials in code. - No DRY violation in auth/security paths (the `require_admin` dependency is reused, not duplicated). ### NITS 1. **DRY: Status derivation logic is duplicated.** The "derive status from registrations" block (lines ~1101-1111 in the new endpoint) is an exact copy of lines 328-340 in `admin_list_players`. Same for the `reg_type` derivation. Consider extracting a helper like `_derive_player_status(registrations) -> tuple[str, str | None]` that both endpoints call. Not a blocker since this is business logic, not auth/security, but divergence risk increases with each new consumer. 2. **Redundant `latest_reg` computation.** The new endpoint computes `latest_reg = max(player.registrations, key=lambda r: r.created_at)` twice -- once for status and once for `reg_type`. These could share a single computation. This is also duplicated from the list endpoint's same pattern. 3. **`parent_name` and `parent_email` are non-optional strings.** If a Player could theoretically exist without a Parent loaded (e.g., orphaned data), accessing `player.parent.name` would raise `AttributeError`. The model has `parent_id` as non-nullable, so this should be safe in practice, but a defensive guard (or explicit documentation of the invariant) would be slightly more robust. Minor nit. ### SOP COMPLIANCE - [x] Branch named after issue: `249-admin-player-detail` - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references issue #249 - [ ] Related section does not reference a plan slug (no plan slug mentioned -- acceptable if this is standalone kanban work) - [x] No secrets committed - [x] No unrelated file changes (only `admin.py` + `test_admin_spa.py` modified) - [x] Commit messages are descriptive (PR title: `feat: add GET /admin/players/{player_id} detail endpoint`) - [x] ruff format and ruff check reported as passing ### PROCESS OBSERVATIONS - **Deployment frequency:** Clean, focused PR -- single endpoint addition with tests. Good for fast merge-to-deploy cycle. - **Change failure risk:** Low. No schema migrations, no existing behavior modified, purely additive. The duplicated status derivation logic is low-risk today but creates maintenance surface for future changes to registration status logic. - **Pattern consistency:** The endpoint follows established patterns in `admin.py` (Pydantic response model, `require_admin`, `joinedload`, manual field mapping). Good codebase coherence. ### VERDICT: APPROVED