feat: flatten PlayerProfileResponse with top-level parent/team/coach fields

forgejo_admin commented

2026-04-03 17:56:36 +00:00

Owner

Summary

Adds parent_name, parent_phone, parent_email, team_name, team_id, and coach_name as top-level convenience fields on PlayerProfileResponse. This is additive -- nested parent and team objects remain unchanged.

Changes

src/basketball_api/routes/players.py -- Added 6 Optional fields to PlayerProfileResponse model and populated them from existing relationships in _player_profile_response()
tests/test_players.py -- Added TestFlattenedProfileFields class with 2 tests: one verifying populated fields (player with team+coach), one verifying null fields (player without team)

Test Plan

All 22 tests pass (18 existing + 2 new + 2 jersey tests that were already in the file)
ruff format and ruff check clean
New test covers: player with team/coach (all flattened fields populated), player without team (team/coach fields null, parent fields still populated)

Review Checklist

ruff format and ruff check clean
All existing tests pass
New tests written and passing
Additive only -- no breaking changes to existing response shape

None.

Closes #280
Spike: #278 (Mismatch 1)

## Summary Adds `parent_name`, `parent_phone`, `parent_email`, `team_name`, `team_id`, and `coach_name` as top-level convenience fields on `PlayerProfileResponse`. This is additive -- nested `parent` and `team` objects remain unchanged. ## Changes - `src/basketball_api/routes/players.py` -- Added 6 `Optional` fields to `PlayerProfileResponse` model and populated them from existing relationships in `_player_profile_response()` - `tests/test_players.py` -- Added `TestFlattenedProfileFields` class with 2 tests: one verifying populated fields (player with team+coach), one verifying null fields (player without team) ## Test Plan - All 22 tests pass (18 existing + 2 new + 2 jersey tests that were already in the file) - `ruff format` and `ruff check` clean - New test covers: player with team/coach (all flattened fields populated), player without team (team/coach fields null, parent fields still populated) ## Review Checklist - [x] `ruff format` and `ruff check` clean - [x] All existing tests pass - [x] New tests written and passing - [x] Additive only -- no breaking changes to existing response shape ## Related Notes None. ## Related - Closes #280 - Spike: #278 (Mismatch 1)

forgejo_admin added 1 commit

2026-04-03 17:56:36 +00:00

feat: flatten player profile response with top-level parent/team/coach fields

ci/woodpecker/pr/woodpecker Pipeline failed

Details

6cf90867dd

Add parent_name, parent_phone, parent_email, team_name, team_id, and
coach_name as top-level Optional fields on PlayerProfileResponse. These
are populated from existing nested relationships for frontend convenience.
Nested objects remain unchanged (additive only).

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

forgejo_admin commented

2026-04-03 22:43:04 +00:00

Author

Owner

PR #291 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic v2

Schema design: The additive approach is correct -- 6 new Optional fields on PlayerProfileResponse with defaults of None, so existing consumers are unaffected. No migration needed since these are computed fields on the Pydantic model, not DB columns.

Refactor of _player_profile_response(): The extraction of parent_obj = player.parent and team_obj = player.team is a clean local-variable refactoring. The ParentInfo construction correctly moved from player.parent.X to parent_obj.X.

Relationship loading: player.parent is backed by a non-nullable FK (parent_id: Mapped[int]), so it is always populated. player.team is a @property returning self.teams[0] if self.teams else None. Both are already eagerly loaded via joinedload in _get_player_or_404(). No N+1 risk.

Pydantic compliance: model_config = {"from_attributes": True} is present. Union syntax str | None is PEP 604 compliant. Field defaults are properly set with = None.

Ruff: PR body claims ruff format and ruff check clean.

BLOCKERS

None.

NITS

Unnecessary defensive guard on parent_obj: The flattened parent fields use parent_obj.name if parent_obj else None, but parent_id is a non-nullable FK, meaning player.parent is never None. The ParentInfo construction on the line above accesses parent_obj.id unconditionally (no guard), which would crash first anyway if parent were somehow null. The guards are harmless but inconsistent. Consider removing them for consistency, or adding a guard to ParentInfo as well if the intent is belt-and-suspenders.

File: src/basketball_api/routes/players.py, flattened parent fields block.
PR body missing Related plan slug: The Related section references issue #280 and spike #278 but does not reference a plan slug. If there is no governing plan for this spike work, this is fine, but the SOP template calls for it.

SOP COMPLIANCE

Branch named after issue (280-flatten-player-profile matches #280)
PR body follows template (Summary, Changes, Test Plan, Related)
Related references plan slug -- no plan slug present (acceptable if no governing plan exists)
No secrets committed
Tests exist -- 2 new tests covering populated fields and null fields
No scope creep -- changes are limited to the 2 files named in the issue

PROCESS OBSERVATIONS

Small, focused PR: 74 additions, 4 deletions, 2 files. Good decomposition from the spike (#278).
Test coverage is solid: happy path (player with team+coach) and edge case (player without team) both covered. The assertions verify both the new flattened fields AND that the nested objects remain intact.
No migration needed since all changes are in the Pydantic response model layer. Low change-failure risk.

VERDICT: APPROVED

## PR #291 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Pydantic v2 **Schema design**: The additive approach is correct -- 6 new `Optional` fields on `PlayerProfileResponse` with defaults of `None`, so existing consumers are unaffected. No migration needed since these are computed fields on the Pydantic model, not DB columns. **Refactor of `_player_profile_response()`**: The extraction of `parent_obj = player.parent` and `team_obj = player.team` is a clean local-variable refactoring. The `ParentInfo` construction correctly moved from `player.parent.X` to `parent_obj.X`. **Relationship loading**: `player.parent` is backed by a non-nullable FK (`parent_id: Mapped[int]`), so it is always populated. `player.team` is a `@property` returning `self.teams[0] if self.teams else None`. Both are already eagerly loaded via `joinedload` in `_get_player_or_404()`. No N+1 risk. **Pydantic compliance**: `model_config = {"from_attributes": True}` is present. Union syntax `str | None` is PEP 604 compliant. Field defaults are properly set with `= None`. **Ruff**: PR body claims `ruff format` and `ruff check` clean. ### BLOCKERS None. ### NITS 1. **Unnecessary defensive guard on `parent_obj`**: The flattened parent fields use `parent_obj.name if parent_obj else None`, but `parent_id` is a non-nullable FK, meaning `player.parent` is never `None`. The `ParentInfo` construction on the line above accesses `parent_obj.id` unconditionally (no guard), which would crash first anyway if parent were somehow null. The guards are harmless but inconsistent. Consider removing them for consistency, or adding a guard to `ParentInfo` as well if the intent is belt-and-suspenders. File: `src/basketball_api/routes/players.py`, flattened parent fields block. 2. **PR body missing Related plan slug**: The Related section references issue #280 and spike #278 but does not reference a plan slug. If there is no governing plan for this spike work, this is fine, but the SOP template calls for it. ### SOP COMPLIANCE - [x] Branch named after issue (`280-flatten-player-profile` matches #280) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [ ] Related references plan slug -- no plan slug present (acceptable if no governing plan exists) - [x] No secrets committed - [x] Tests exist -- 2 new tests covering populated fields and null fields - [x] No scope creep -- changes are limited to the 2 files named in the issue ### PROCESS OBSERVATIONS - Small, focused PR: 74 additions, 4 deletions, 2 files. Good decomposition from the spike (#278). - Test coverage is solid: happy path (player with team+coach) and edge case (player without team) both covered. The assertions verify both the new flattened fields AND that the nested objects remain intact. - No migration needed since all changes are in the Pydantic response model layer. Low change-failure risk. ### VERDICT: APPROVED