feat: enrich TeamDetail with multi-coach list and player height/jersey

forgejo_admin commented

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

Author

Owner

PR #292 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic

Contract change: TeamDetail.coach: CoachBrief | None changed to coaches: list[CoachBrief]. This is a breaking API change for any consumer currently reading the coach key. The PR body acknowledges this is intentional to match what the frontend expects.

Model enrichment: CoachBrief gains role and phone; PlayerBrief gains height and jersey_number. All four fields exist in the DB models (Coach.role, Coach.phone, Player.height, Player.jersey_number) -- confirmed via model inspection. No new DB columns or migrations needed.

_coaches_list() helper: Clean bridge from singular FK to plural API contract. The # type: ignore[list-item] is justified -- _coach_brief() returns CoachBrief | None but the None guard runs first, so the value is always CoachBrief inside the list. Docstring is clear.

coach.role.value: Correctly serializes the enum to its string value. coach.phone is a plain string column, no enum conversion needed. Both match the model definitions.

TeamResponse still uses singular coach: This is not in scope for this PR (the issue targets TeamDetail only), but worth noting that list/PATCH endpoints still return coach: CoachBrief | None. The PATCH tests at lines 318, 333, 340, 382 still assert resp.json()["coach"] -- these remain valid since TeamResponse is unchanged. No inconsistency within this PR's scope.

Test coverage: 4 new tests covering:

No-coach returns empty list
Single coach includes role and phone
Player includes height and jersey_number
Player with null height/jersey returns null

Plus 1 existing test updated (test_coach_sees_own_teams) to assert the new coaches list shape and verify role is populated. Total: 45 team tests reported passing.

PEP compliance: Type hints present on all new fields and function signatures. Docstring on _coaches_list(). = None defaults on optional Pydantic fields follow PEP 484 conventions.

BLOCKERS

None.

New functionality has test coverage (4 new tests + 1 updated).
No unvalidated user input -- this PR only changes response serialization, not input handling.
No secrets or credentials in code.
No DRY violations in auth paths.

NITS

_coach_brief return type: Now that _coaches_list exists and handles the None case, _coach_brief is only ever called with a non-None coach. Consider narrowing its signature to _coach_brief(coach: Coach) -> CoachBrief and dropping the None guard, which would also eliminate the # type: ignore. Low priority since the current code is correct.
Consistency observation: TeamResponse (used by list/PATCH endpoints) still returns singular coach. If the frontend migration to coaches is complete, a follow-up ticket to align TeamResponse would prevent consumer confusion. Not in scope for this PR.

SOP COMPLIANCE

Branch named after issue: 284-team-detail-contract follows {issue-number}-{kebab-case-purpose}
PR body has Summary, Changes, Test Plan, Related sections
Related references parent issue #284 and spike #278
Related does not reference a plan slug (no plan associated -- acceptable for spike-derived work)
No secrets committed
No scope creep -- changes are strictly limited to TeamDetail response enrichment
Commit message is descriptive

PROCESS OBSERVATIONS

This PR is one of several contract-alignment PRs (#287-#292) derived from spike #278. Good decomposition -- each PR targets a single response model.
Pre-existing test failure in test_checkout.py is tracked by #274. Not introduced by this PR.
Breaking API change (coach -> coaches) should be coordinated with frontend deployment. The PR body states the frontend already expects the plural form, which mitigates risk.

VERDICT: APPROVED

## PR #292 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Pydantic **Contract change**: `TeamDetail.coach: CoachBrief | None` changed to `coaches: list[CoachBrief]`. This is a **breaking API change** for any consumer currently reading the `coach` key. The PR body acknowledges this is intentional to match what the frontend expects. **Model enrichment**: `CoachBrief` gains `role` and `phone`; `PlayerBrief` gains `height` and `jersey_number`. All four fields exist in the DB models (`Coach.role`, `Coach.phone`, `Player.height`, `Player.jersey_number`) -- confirmed via model inspection. No new DB columns or migrations needed. **`_coaches_list()` helper**: Clean bridge from singular FK to plural API contract. The `# type: ignore[list-item]` is justified -- `_coach_brief()` returns `CoachBrief | None` but the `None` guard runs first, so the value is always `CoachBrief` inside the list. Docstring is clear. **`coach.role.value`**: Correctly serializes the enum to its string value. `coach.phone` is a plain string column, no enum conversion needed. Both match the model definitions. **`TeamResponse` still uses singular `coach`**: This is not in scope for this PR (the issue targets `TeamDetail` only), but worth noting that list/PATCH endpoints still return `coach: CoachBrief | None`. The PATCH tests at lines 318, 333, 340, 382 still assert `resp.json()["coach"]` -- these remain valid since `TeamResponse` is unchanged. No inconsistency within this PR's scope. **Test coverage**: 4 new tests covering: 1. No-coach returns empty list 2. Single coach includes role and phone 3. Player includes height and jersey_number 4. Player with null height/jersey returns null Plus 1 existing test updated (`test_coach_sees_own_teams`) to assert the new `coaches` list shape and verify `role` is populated. Total: 45 team tests reported passing. **PEP compliance**: Type hints present on all new fields and function signatures. Docstring on `_coaches_list()`. `= None` defaults on optional Pydantic fields follow PEP 484 conventions. ### BLOCKERS None. - New functionality has test coverage (4 new tests + 1 updated). - No unvalidated user input -- this PR only changes response serialization, not input handling. - No secrets or credentials in code. - No DRY violations in auth paths. ### NITS 1. **`_coach_brief` return type**: Now that `_coaches_list` exists and handles the `None` case, `_coach_brief` is only ever called with a non-None coach. Consider narrowing its signature to `_coach_brief(coach: Coach) -> CoachBrief` and dropping the `None` guard, which would also eliminate the `# type: ignore`. Low priority since the current code is correct. 2. **Consistency observation**: `TeamResponse` (used by list/PATCH endpoints) still returns singular `coach`. If the frontend migration to `coaches` is complete, a follow-up ticket to align `TeamResponse` would prevent consumer confusion. Not in scope for this PR. ### SOP COMPLIANCE - [x] Branch named after issue: `284-team-detail-contract` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references parent issue #284 and spike #278 - [ ] Related does not reference a plan slug (no plan associated -- acceptable for spike-derived work) - [x] No secrets committed - [x] No scope creep -- changes are strictly limited to `TeamDetail` response enrichment - [x] Commit message is descriptive ### PROCESS OBSERVATIONS - This PR is one of several contract-alignment PRs (#287-#292) derived from spike #278. Good decomposition -- each PR targets a single response model. - Pre-existing test failure in `test_checkout.py` is tracked by #274. Not introduced by this PR. - Breaking API change (`coach` -> `coaches`) should be coordinated with frontend deployment. The PR body states the frontend already expects the plural form, which mitigates risk. ### VERDICT: APPROVED

2026-04-03 17:58:50 +00:00

Summary

Changes the TeamDetail API response contract to return coaches (plural list) instead of coach (singular), and enriches CoachBrief with role/phone and PlayerBrief with height/jersey_number. All fields already exist in the DB -- this exposes them through the response models to match what the frontend expects.

Changes

src/basketball_api/routes/teams.py -- TeamDetail.coach: CoachBrief | None changed to coaches: list[CoachBrief]. Added role and phone to CoachBrief. Added height and jersey_number to PlayerBrief. Added _coaches_list() helper to bridge the singular DB FK to the plural API contract. Updated _coach_brief() and _player_brief() helpers to populate new fields.
tests/test_teams.py -- Updated existing coach assertion in test_coach_sees_own_teams to use coaches[0]. Added 4 new tests: no-coach returns empty list, single coach includes role/phone, player includes height/jersey_number, player with null height/jersey returns null.

Test Plan

pytest tests/test_teams.py -- 45 tests pass (4 new)
Full suite: 1 pre-existing failure in test_checkout.py (tracked by #274), unrelated to this change
ruff format and ruff check clean

Review Checklist

TeamDetail.coach changed to coaches: list[CoachBrief]
CoachBrief includes role and phone
PlayerBrief includes height and jersey_number
All fields populated from existing DB columns
45 team tests pass (4 new)
ruff clean

None

Forgejo issue: #284
Spike: #278 (Mismatch 9)
Pre-existing test failure: #274

Closes #284

## Summary Changes the `TeamDetail` API response contract to return `coaches` (plural list) instead of `coach` (singular), and enriches `CoachBrief` with `role`/`phone` and `PlayerBrief` with `height`/`jersey_number`. All fields already exist in the DB -- this exposes them through the response models to match what the frontend expects. ## Changes - `src/basketball_api/routes/teams.py` -- `TeamDetail.coach: CoachBrief | None` changed to `coaches: list[CoachBrief]`. Added `role` and `phone` to `CoachBrief`. Added `height` and `jersey_number` to `PlayerBrief`. Added `_coaches_list()` helper to bridge the singular DB FK to the plural API contract. Updated `_coach_brief()` and `_player_brief()` helpers to populate new fields. - `tests/test_teams.py` -- Updated existing coach assertion in `test_coach_sees_own_teams` to use `coaches[0]`. Added 4 new tests: no-coach returns empty list, single coach includes role/phone, player includes height/jersey_number, player with null height/jersey returns null. ## Test Plan - `pytest tests/test_teams.py` -- 45 tests pass (4 new) - Full suite: 1 pre-existing failure in `test_checkout.py` (tracked by #274), unrelated to this change - `ruff format` and `ruff check` clean ## Review Checklist - [x] `TeamDetail.coach` changed to `coaches: list[CoachBrief]` - [x] `CoachBrief` includes `role` and `phone` - [x] `PlayerBrief` includes `height` and `jersey_number` - [x] All fields populated from existing DB columns - [x] 45 team tests pass (4 new) - [x] ruff clean ## Related Notes - None ## Related - Forgejo issue: #284 - Spike: #278 (Mismatch 9) - Pre-existing test failure: #274 Closes #284

forgejo_admin added 1 commit

feat: enrich TeamDetail with multi-coach list and player height/jersey fields

ci/woodpecker/pr/woodpecker Pipeline failed

Details

85714f66be

Change TeamDetail.coach (singular CoachBrief|None) to TeamDetail.coaches
(list[CoachBrief]) to match the frontend contract. Add role and phone to
CoachBrief, and height and jersey_number to PlayerBrief. All fields are
populated from existing DB columns.

Closes #284

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