forgejo_admin/basketball-api

Fork 1

feat: add parent_phone, school, height to CoachPlayerBrief #290

Merged

forgejo_admin merged 1 commit from 283-coach-dashboard-parent-contact into main

2026-04-03 22:45:14 +00:00

forgejo_admin commented

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

Owner

Summary

Adds parent_phone, school, and height fields to CoachPlayerBrief so coaches can see parent contact info and player details in their dashboard roster. Eagerly loads the Player.parent relationship to avoid N+1 queries.

Changes

src/basketball_api/routes/coaches_api.py — Added three optional fields to CoachPlayerBrief model, populated them in _build_team_response, and chained joinedload(Player.parent) on both endpoint queries
tests/test_coaches_api.py — Updated team_with_players fixture with phone/school/height data, added two new tests verifying field presence and null handling

Test Plan

All 15 coach API tests pass (13 existing + 2 new)
test_includes_parent_phone_school_height verifies populated fields
test_null_fields_when_missing verifies null when player lacks school/height
ruff format and ruff check clean

Review Checklist

Implementation matches acceptance criteria
Field name mapping: frontend expects school, DB stores current_school
Eager loading prevents N+1 queries on parent relationship
All existing tests still pass
New tests cover both populated and null field cases
Linting clean

None

Closes #283
Spike: #278 (Mismatch 4)

## Summary Adds `parent_phone`, `school`, and `height` fields to `CoachPlayerBrief` so coaches can see parent contact info and player details in their dashboard roster. Eagerly loads the `Player.parent` relationship to avoid N+1 queries. ## Changes - `src/basketball_api/routes/coaches_api.py` — Added three optional fields to `CoachPlayerBrief` model, populated them in `_build_team_response`, and chained `joinedload(Player.parent)` on both endpoint queries - `tests/test_coaches_api.py` — Updated `team_with_players` fixture with phone/school/height data, added two new tests verifying field presence and null handling ## Test Plan - All 15 coach API tests pass (13 existing + 2 new) - `test_includes_parent_phone_school_height` verifies populated fields - `test_null_fields_when_missing` verifies null when player lacks school/height - `ruff format` and `ruff check` clean ## Review Checklist - [x] Implementation matches acceptance criteria - [x] Field name mapping: frontend expects `school`, DB stores `current_school` - [x] Eager loading prevents N+1 queries on parent relationship - [x] All existing tests still pass - [x] New tests cover both populated and null field cases - [x] Linting clean ## Related Notes - None ## Related - Closes #283 - Spike: #278 (Mismatch 4)

forgejo_admin added 1 commit

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

feat: add parent_phone, school, height to CoachPlayerBrief

ci/woodpecker/pr/woodpecker Pipeline failed

Details

c38d38ef35

Coaches need parent contact info and player details visible in their
dashboard roster. Adds three fields to CoachPlayerBrief and eagerly loads
the parent relationship to avoid N+1 queries.

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

forgejo_admin commented

2026-04-03 17:57:22 +00:00

Author

Owner

QA Review -- PR #290

Diff Analysis

Files changed: 2 (1 source, 1 test)
Scope: +46 / -3 lines -- minimal, well-contained

Findings

No issues found.

Model -- Three new optional fields (parent_phone, school, height) with = None defaults. Backward compatible for existing API consumers.
Field mapping -- school correctly maps from current_school DB field, matching frontend contract expectation (Spike #278 Mismatch 4).
Null safety -- p.parent.phone if p.parent else None guards against orphaned players.
Query performance -- Both endpoints chain joinedload(Player.parent) to prevent N+1 queries. Correct approach.
Tests -- Two new tests cover populated fields and null fields. Fixture updated with phone, school, height data. All 15 coach tests pass.
Linting -- ruff format and ruff check clean per PR body.

Acceptance Criteria Check

parent_phone: Optional[str] on CoachPlayerBrief
school: Optional[str] populated from current_school
height: Optional[str] on CoachPlayerBrief
Fields populated from player's parent and profile relationships
All existing tests pass
Unit test verifying new fields in coach roster response
Test with player missing parent/school/height (fields null)

VERDICT: APPROVED

## QA Review -- PR #290 ### Diff Analysis - **Files changed**: 2 (1 source, 1 test) - **Scope**: +46 / -3 lines -- minimal, well-contained ### Findings **No issues found.** 1. **Model** -- Three new optional fields (`parent_phone`, `school`, `height`) with `= None` defaults. Backward compatible for existing API consumers. 2. **Field mapping** -- `school` correctly maps from `current_school` DB field, matching frontend contract expectation (Spike #278 Mismatch 4). 3. **Null safety** -- `p.parent.phone if p.parent else None` guards against orphaned players. 4. **Query performance** -- Both endpoints chain `joinedload(Player.parent)` to prevent N+1 queries. Correct approach. 5. **Tests** -- Two new tests cover populated fields and null fields. Fixture updated with phone, school, height data. All 15 coach tests pass. 6. **Linting** -- `ruff format` and `ruff check` clean per PR body. ### Acceptance Criteria Check - [x] `parent_phone: Optional[str]` on CoachPlayerBrief - [x] `school: Optional[str]` populated from `current_school` - [x] `height: Optional[str]` on CoachPlayerBrief - [x] Fields populated from player's parent and profile relationships - [x] All existing tests pass - [x] Unit test verifying new fields in coach roster response - [x] Test with player missing parent/school/height (fields null) VERDICT: **APPROVED**

forgejo_admin commented

2026-04-03 22:42:30 +00:00

Author

Owner

PR #290 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

Model correctness -- All three new fields map correctly to existing DB columns:

parent_phone -- sourced from Player.parent.phone (Parent model, String(50), nullable)
school -- sourced from Player.current_school (Player model, String(200), nullable)
height -- sourced from Player.height (Player model, String(20), nullable)

The field name mapping school (API) to current_school (DB) is an intentional frontend-contract decision, documented in the PR checklist. This is fine.

SQLAlchemy eager loading -- The chained .joinedload(Team.players).joinedload(Player.parent) is applied to both the /me and /{coach_id} queries. Note that Team.players already has lazy="selectin" on the relationship definition, so the explicit joinedload(Team.players) overrides that to a JOIN for those queries. The chained .joinedload(Player.parent) correctly prevents N+1 on the parent lookup. Both endpoints are updated consistently -- no DRY divergence.

Null safety -- p.parent.phone if p.parent else None correctly handles the case where a player has no parent record (though parent_id is non-nullable in the model, defensive coding is appropriate). p.current_school and p.height are nullable columns and map directly to optional Pydantic fields.

Pydantic model -- CoachPlayerBrief adds three str | None = None fields with from_attributes = True. Default None ensures backward compatibility if the model is ever constructed without these fields.

Import -- Player import added to support the type reference in joinedload(Player.parent). Correct and necessary.

Test coverage -- Two new tests:

test_includes_parent_phone_school_height -- verifies populated values (happy path)
test_null_fields_when_missing -- verifies null for school/height when not set on the player

The fixture is updated with phone="555-1234" on the parent, height and current_school on only the first player (Jordan), leaving the second player (Alex) without those values. This properly exercises both code paths.

One observation: both new tests only exercise the /coaches/me endpoint. The /{coach_id} endpoint uses the same _build_team_response helper and the same eager-loading pattern, so the coverage is effectively shared. This is acceptable given the shared helper.

BLOCKERS

None.

New fields have test coverage (2 new tests, happy path + null handling)
No unvalidated user input (these are read-only response fields)
No secrets or credentials in code
No DRY violation in auth/security paths -- both endpoints share _build_team_response

NITS

Test: no coverage of /{coach_id} with new fields -- While the shared helper means the logic is tested, a single assertion on the profile endpoint response would provide full confidence that the eager loading works on both query paths. Low risk, non-blocking.
Player import on main branch -- The diff adds Player to the import line. On main, coaches_api.py imports only Coach, Team. The Player import is necessary for the joinedload chain. Clean.

SOP COMPLIANCE

Branch named after issue: 283-coach-dashboard-parent-contact follows {issue-number}-{kebab-case-purpose}
PR body follows template: Summary, Changes, Test Plan, Related sections all present
Related references parent issue: Closes #283, spike #278
Related references plan slug -- No plan slug referenced (none appears to exist for this work; it traces to spike #278)
No secrets committed
No scope creep -- exactly 2 files changed, both directly related to the issue
Commit messages descriptive (PR title matches conventional commit format)

PROCESS OBSERVATIONS

Clean, minimal PR -- 46 additions, 3 deletions across 2 files. Tight scope.
The eager-loading fix is a good pattern to establish. Other endpoints that traverse Player.parent should follow this same pattern.
This is part of a spike (#278) that identified API/frontend contract mismatches. Good traceability from spike to fix.
Deployment risk is low: additive-only change to a response model, no breaking changes for existing consumers.

VERDICT: APPROVED

## PR #290 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic **Model correctness** -- All three new fields map correctly to existing DB columns: - `parent_phone` -- sourced from `Player.parent.phone` (Parent model, `String(50)`, nullable) - `school` -- sourced from `Player.current_school` (Player model, `String(200)`, nullable) - `height` -- sourced from `Player.height` (Player model, `String(20)`, nullable) The field name mapping `school` (API) to `current_school` (DB) is an intentional frontend-contract decision, documented in the PR checklist. This is fine. **SQLAlchemy eager loading** -- The chained `.joinedload(Team.players).joinedload(Player.parent)` is applied to both the `/me` and `/{coach_id}` queries. Note that `Team.players` already has `lazy="selectin"` on the relationship definition, so the explicit `joinedload(Team.players)` overrides that to a JOIN for those queries. The chained `.joinedload(Player.parent)` correctly prevents N+1 on the parent lookup. Both endpoints are updated consistently -- no DRY divergence. **Null safety** -- `p.parent.phone if p.parent else None` correctly handles the case where a player has no parent record (though `parent_id` is non-nullable in the model, defensive coding is appropriate). `p.current_school` and `p.height` are nullable columns and map directly to optional Pydantic fields. **Pydantic model** -- `CoachPlayerBrief` adds three `str | None = None` fields with `from_attributes = True`. Default `None` ensures backward compatibility if the model is ever constructed without these fields. **Import** -- `Player` import added to support the type reference in `joinedload(Player.parent)`. Correct and necessary. **Test coverage** -- Two new tests: 1. `test_includes_parent_phone_school_height` -- verifies populated values (happy path) 2. `test_null_fields_when_missing` -- verifies null for school/height when not set on the player The fixture is updated with `phone="555-1234"` on the parent, `height` and `current_school` on only the first player (Jordan), leaving the second player (Alex) without those values. This properly exercises both code paths. One observation: both new tests only exercise the `/coaches/me` endpoint. The `/{coach_id}` endpoint uses the same `_build_team_response` helper and the same eager-loading pattern, so the coverage is effectively shared. This is acceptable given the shared helper. ### BLOCKERS None. - New fields have test coverage (2 new tests, happy path + null handling) - No unvalidated user input (these are read-only response fields) - No secrets or credentials in code - No DRY violation in auth/security paths -- both endpoints share `_build_team_response` ### NITS 1. **Test: no coverage of `/{coach_id}` with new fields** -- While the shared helper means the logic is tested, a single assertion on the profile endpoint response would provide full confidence that the eager loading works on both query paths. Low risk, non-blocking. 2. **`Player` import on main branch** -- The diff adds `Player` to the import line. On `main`, `coaches_api.py` imports only `Coach, Team`. The `Player` import is necessary for the `joinedload` chain. Clean. ### SOP COMPLIANCE - [x] Branch named after issue: `283-coach-dashboard-parent-contact` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related sections all present - [x] Related references parent issue: `Closes #283`, spike #278 - [ ] Related references plan slug -- No plan slug referenced (none appears to exist for this work; it traces to spike #278) - [x] No secrets committed - [x] No scope creep -- exactly 2 files changed, both directly related to the issue - [x] Commit messages descriptive (PR title matches conventional commit format) ### PROCESS OBSERVATIONS - Clean, minimal PR -- 46 additions, 3 deletions across 2 files. Tight scope. - The eager-loading fix is a good pattern to establish. Other endpoints that traverse `Player.parent` should follow this same pattern. - This is part of a spike (#278) that identified API/frontend contract mismatches. Good traceability from spike to fix. - Deployment risk is low: additive-only change to a response model, no breaking changes for existing consumers. ### VERDICT: APPROVED