feat: add team_ids to AccountPlayerResponse (#333) #334

Merged

forgejo_admin merged 1 commit from 333-add-team-ids-account-response into main 2026-04-04 21:44:47 +00:00

Conversation 3 Commits 1 Files changed 2 +18

forgejo_admin commented 2026-04-04 21:35:23 +00:00

Contributor

Copy link

Summary

• Adds team_ids: list[int] to AccountPlayerResponse so the parent dashboard can filter /public/schedule by team
• Populated from player.teams M2M relationship (already eagerly loaded)
• Backwards compatible — team_name field unchanged

Changes

• src/basketball_api/routes/account.py — 2 lines added (field + population)

Test Plan

• 7 existing account tests pass
• CI pipeline passes
• QA verifies GET /account/players response includes team_ids

Review Checklist

• No new endpoints — enriches existing response only
• team_name unchanged (backwards compatible)
• Zero-team players return empty list []
• Pattern matches existing team_ids usage in admin route (line 713)

Related Notes

• westside-landing#214 (parent dashboard schedule — consumes this field)
• story:WS-S28 — parent schedule user story
• arch-auth-westside-basketball — updated to include schedule viewing for parent role

Closes #333

## Summary - Adds `team_ids: list[int]` to `AccountPlayerResponse` so the parent dashboard can filter `/public/schedule` by team - Populated from `player.teams` M2M relationship (already eagerly loaded) - Backwards compatible — `team_name` field unchanged ## Changes - `src/basketball_api/routes/account.py` — 2 lines added (field + population) ## Test Plan - [x] 7 existing account tests pass - [ ] CI pipeline passes - [ ] QA verifies `GET /account/players` response includes `team_ids` ## Review Checklist - [x] No new endpoints — enriches existing response only - [x] `team_name` unchanged (backwards compatible) - [x] Zero-team players return empty list `[]` - [x] Pattern matches existing `team_ids` usage in admin route (line 713) ## Related Notes - westside-landing#214 (parent dashboard schedule — consumes this field) - `story:WS-S28` — parent schedule user story - `arch-auth-westside-basketball` — updated to include schedule viewing for parent role Closes #333

forgejo_admin added 2 commits 2026-04-04 21:35:23 +00:00

feat: include is_public in admin players list response ... 3c167eae94

The AdminPlayerItem model and GET /admin/players response now include
the is_public boolean field, enabling the westside-app admin UI to
display and toggle player visibility state.

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

feat: add team_ids to AccountPlayerResponse for schedule filtering ... 7e704c1a09

Parent dashboard needs team IDs to filter /public/schedule client-side
and show each player's team practice schedule. Adds team_ids: list[int]
populated from the player_teams M2M relationship.

Closes #333

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

forgejo_admin commented 2026-04-04 21:37:27 +00:00

Author

Contributor

Copy link

PR #334 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

This PR makes two separate changes:

1. team_ids: list[int] added to AccountPlayerResponse (src/basketball_api/routes/account.py)

• Field added to the Pydantic model (line 31 in diff context)
• Populated via [t.id for t in player.teams] -- matches the existing pattern used in TeamPlayerItem at admin.py:715
• The Player.teams M2M relationship uses lazy="selectin" on the model, and the query already has joinedload(Player.teams) -- no N+1 risk
• team_name field unchanged -- backwards compatible
• Zero-team players return [] (empty list comprehension) -- correct

2. is_public: bool added to AdminPlayerItem (src/basketball_api/routes/admin.py)

• Field added to the Pydantic model (line 302)
• Populated from player.is_public which exists on the Player model with server_default=text("false")
• Test updated in test_admin_spa.py to assert is_public in expected fields

Both changes are straightforward field enrichments to existing response models. The code quality is clean.

BLOCKERS

1. Missing test coverage for team_ids in account route

The test_account.py file has 7 tests including test_response_fields (line 227-245) and test_includes_team_name (line 169-181). Neither test is updated to verify the new team_ids field. Specifically:

• test_response_fields checks for id, name, status, team_name, subscription_status, division, position, graduating_class, photo_url -- but NOT team_ids
• test_includes_team_name asserts team_name values for assigned/unassigned players but does not assert team_ids
• No test covers the happy path (team_ids == [team.id] for assigned player) or the empty case (team_ids == [] for unassigned player)

New functionality with zero test coverage is a blocker per review criteria. The admin is_public change has its test updated, but the headline feature (team_ids) does not.

File: /home/ldraney/basketball-api/tests/test_account.py -- test_response_fields at line 227 and test_includes_team_name at line 169 need updates.

NITS

1. Undisclosed scope: The PR title and summary describe only the team_ids addition to AccountPlayerResponse. The is_public addition to AdminPlayerItem is not mentioned anywhere in the PR body. The Changes section says "2 lines added" but the diff shows 5 additions across 3 files. This should be documented in the PR body for traceability.

2. Related section references: The Related section references westside-landing#214 and story:WS-S28. The is_public admin change presumably has its own motivation -- it should reference the related issue or story.

SOP COMPLIANCE

• Branch named after issue: 333-add-team-ids-account-response follows {issue-number}-{kebab-case-purpose} convention
• PR body has Summary, Changes, Test Plan, Related sections
• Changes section is inaccurate: says "2 lines added" but diff has 5 additions across 3 files including undisclosed is_public change
• Related references story slug (story:WS-S28)
• No secrets committed
• Scope creep: is_public addition to AdminPlayerItem is unrelated to issue #333 (adding team_ids to account response)

PROCESS OBSERVATIONS

Small enrichment PRs like this have low change failure risk. The pattern is well-established in the codebase (identical list comprehension at admin.py:715). The missing test is a mechanical fix -- add team_ids assertions to the existing test_response_fields and test_includes_team_name tests, which already set up the right fixtures (one player assigned to a team, one not).

The undisclosed is_public scope should either be documented in this PR or split to its own issue/PR for traceability. It is a one-liner with its test already updated, so documenting it here is the pragmatic path.

VERDICT: NOT APPROVED

Fix required: Add test coverage for team_ids in tests/test_account.py (assert field presence in test_response_fields, assert correct values in test_includes_team_name or a new dedicated test). Document the is_public admin change in the PR body.

## PR #334 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic This PR makes two separate changes: **1. `team_ids: list[int]` added to `AccountPlayerResponse`** (`src/basketball_api/routes/account.py`) - Field added to the Pydantic model (line 31 in diff context) - Populated via `[t.id for t in player.teams]` -- matches the existing pattern used in `TeamPlayerItem` at `admin.py:715` - The `Player.teams` M2M relationship uses `lazy="selectin"` on the model, and the query already has `joinedload(Player.teams)` -- no N+1 risk - `team_name` field unchanged -- backwards compatible - Zero-team players return `[]` (empty list comprehension) -- correct **2. `is_public: bool` added to `AdminPlayerItem`** (`src/basketball_api/routes/admin.py`) - Field added to the Pydantic model (line 302) - Populated from `player.is_public` which exists on the Player model with `server_default=text("false")` - Test updated in `test_admin_spa.py` to assert `is_public` in expected fields Both changes are straightforward field enrichments to existing response models. The code quality is clean. ### BLOCKERS **1. Missing test coverage for `team_ids` in account route** The `test_account.py` file has 7 tests including `test_response_fields` (line 227-245) and `test_includes_team_name` (line 169-181). Neither test is updated to verify the new `team_ids` field. Specifically: - `test_response_fields` checks for `id`, `name`, `status`, `team_name`, `subscription_status`, `division`, `position`, `graduating_class`, `photo_url` -- but NOT `team_ids` - `test_includes_team_name` asserts `team_name` values for assigned/unassigned players but does not assert `team_ids` - No test covers the happy path (`team_ids == [team.id]` for assigned player) or the empty case (`team_ids == []` for unassigned player) New functionality with zero test coverage is a blocker per review criteria. The admin `is_public` change has its test updated, but the headline feature (`team_ids`) does not. File: `/home/ldraney/basketball-api/tests/test_account.py` -- `test_response_fields` at line 227 and `test_includes_team_name` at line 169 need updates. ### NITS 1. **Undisclosed scope**: The PR title and summary describe only the `team_ids` addition to `AccountPlayerResponse`. The `is_public` addition to `AdminPlayerItem` is not mentioned anywhere in the PR body. The Changes section says "2 lines added" but the diff shows 5 additions across 3 files. This should be documented in the PR body for traceability. 2. **Related section references**: The Related section references `westside-landing#214` and `story:WS-S28`. The `is_public` admin change presumably has its own motivation -- it should reference the related issue or story. ### SOP COMPLIANCE - [x] Branch named after issue: `333-add-team-ids-account-response` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Changes section is inaccurate: says "2 lines added" but diff has 5 additions across 3 files including undisclosed `is_public` change - [x] Related references story slug (`story:WS-S28`) - [x] No secrets committed - [ ] Scope creep: `is_public` addition to `AdminPlayerItem` is unrelated to issue #333 (adding `team_ids` to account response) ### PROCESS OBSERVATIONS Small enrichment PRs like this have low change failure risk. The pattern is well-established in the codebase (identical list comprehension at `admin.py:715`). The missing test is a mechanical fix -- add `team_ids` assertions to the existing `test_response_fields` and `test_includes_team_name` tests, which already set up the right fixtures (one player assigned to a team, one not). The undisclosed `is_public` scope should either be documented in this PR or split to its own issue/PR for traceability. It is a one-liner with its test already updated, so documenting it here is the pragmatic path. ### VERDICT: NOT APPROVED **Fix required**: Add test coverage for `team_ids` in `tests/test_account.py` (assert field presence in `test_response_fields`, assert correct values in `test_includes_team_name` or a new dedicated test). Document the `is_public` admin change in the PR body.

forgejo_admin force-pushed 333-add-team-ids-account-response from 7e704c1a09 to 5405b3af8e 2026-04-04 21:39:51 +00:00 Compare

forgejo_admin commented 2026-04-04 21:40:01 +00:00

Author

Contributor

Copy link

QA fixes applied

BLOCKER fixed: Added test_includes_team_ids — asserts Jordan has [sample_team.id] and Alex has []. Also added team_ids to test_response_fields assertions.

Scope creep fixed: Rebased onto Forgejo's current main (5ad9fab). The is_public change is no longer in this PR's diff. Clean 2-file diff: account.py (2 additions) + test_account.py (16 additions).

10 tests passing (7 existing + 3 from upstream + 1 new test_includes_team_ids).

### QA fixes applied **BLOCKER fixed:** Added `test_includes_team_ids` — asserts Jordan has `[sample_team.id]` and Alex has `[]`. Also added `team_ids` to `test_response_fields` assertions. **Scope creep fixed:** Rebased onto Forgejo's current `main` (`5ad9fab`). The `is_public` change is no longer in this PR's diff. Clean 2-file diff: `account.py` (2 additions) + `test_account.py` (16 additions). **10 tests passing** (7 existing + 3 from upstream + 1 new `test_includes_team_ids`).

forgejo_admin commented 2026-04-04 21:44:19 +00:00

Author

Contributor

Copy link

PR #334 Review (Re-review)

Previous review identified two issues: (1) missing test for team_ids field, (2) scope creep with is_public changes. This re-review verifies both fixes.

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic

Schema change -- AccountPlayerResponse gains team_ids: list[int] at line 32. Type is correct (Pydantic list[int]). No default needed since it is always populated explicitly in the constructor.

Population -- team_ids=[t.id for t in player.teams] at line 102. This mirrors the identical pattern in the admin route (admin.py:742). The player.teams M2M relationship is already eager-loaded via subqueryload(Player.teams) on line 70, so no N+1 query risk.

Backwards compatibility -- The existing team_name and team_id fields are untouched. New team_ids field is additive only. Zero-team players correctly produce [].

Model verification -- Player.teams is a proper M2M via player_teams secondary table with lazy="selectin" default loading (models.py:258-259). The explicit subqueryload in the query overrides this, which is fine.

Scope creep check -- No is_public references exist in account.py. The previous scope creep has been fully removed. Diff touches exactly 2 files with 18 additions and 0 deletions, all directly related to the team_ids feature.

BLOCKERS

None.

Previous blockers resolved:

1. Missing test -- FIXED. test_includes_team_ids added (lines 201-214). Tests both the assigned case (jordan["team_ids"] == [sample_team.id]) and the unassigned case (alex["team_ids"] == []). Uses the sample_team fixture for exact ID comparison rather than hardcoded values.
2. Scope creep -- FIXED. The is_public changes are gone. Only team_ids-related changes remain.

Additionally, test_response_fields (line 307) now asserts "team_ids" in player, ensuring the field presence is validated in the exhaustive field check.

NITS

None.

SOP COMPLIANCE

• Branch named after issue (333-add-team-ids-account-response)
• PR body follows template (Summary, Changes, Test Plan, Related)
• Related section references user story (story:WS-S28) and architecture (arch-auth-westside-basketball)
• Tests exist: test_includes_team_ids + test_response_fields updated
• No secrets committed
• No unnecessary file changes -- exactly 2 files, both on-topic
• Commit message is descriptive

PROCESS OBSERVATIONS

Clean fix cycle. Both previous review findings addressed correctly. The pattern follows the established team_ids convention from the admin route, maintaining consistency across parent and admin response models. Low change-failure risk -- additive field on an existing endpoint with test coverage for both happy path (player with team) and edge case (player with no team).

VERDICT: APPROVED

## PR #334 Review (Re-review) Previous review identified two issues: (1) missing test for `team_ids` field, (2) scope creep with `is_public` changes. This re-review verifies both fixes. ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Pydantic **Schema change** -- `AccountPlayerResponse` gains `team_ids: list[int]` at line 32. Type is correct (Pydantic `list[int]`). No default needed since it is always populated explicitly in the constructor. **Population** -- `team_ids=[t.id for t in player.teams]` at line 102. This mirrors the identical pattern in the admin route (`admin.py:742`). The `player.teams` M2M relationship is already eager-loaded via `subqueryload(Player.teams)` on line 70, so no N+1 query risk. **Backwards compatibility** -- The existing `team_name` and `team_id` fields are untouched. New `team_ids` field is additive only. Zero-team players correctly produce `[]`. **Model verification** -- `Player.teams` is a proper M2M via `player_teams` secondary table with `lazy="selectin"` default loading (`models.py:258-259`). The explicit `subqueryload` in the query overrides this, which is fine. **Scope creep check** -- No `is_public` references exist in `account.py`. The previous scope creep has been fully removed. Diff touches exactly 2 files with 18 additions and 0 deletions, all directly related to the `team_ids` feature. ### BLOCKERS None. Previous blockers resolved: 1. **Missing test** -- FIXED. `test_includes_team_ids` added (lines 201-214). Tests both the assigned case (`jordan["team_ids"] == [sample_team.id]`) and the unassigned case (`alex["team_ids"] == []`). Uses the `sample_team` fixture for exact ID comparison rather than hardcoded values. 2. **Scope creep** -- FIXED. The `is_public` changes are gone. Only `team_ids`-related changes remain. Additionally, `test_response_fields` (line 307) now asserts `"team_ids" in player`, ensuring the field presence is validated in the exhaustive field check. ### NITS None. ### SOP COMPLIANCE - [x] Branch named after issue (`333-add-team-ids-account-response`) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related section references user story (`story:WS-S28`) and architecture (`arch-auth-westside-basketball`) - [x] Tests exist: `test_includes_team_ids` + `test_response_fields` updated - [x] No secrets committed - [x] No unnecessary file changes -- exactly 2 files, both on-topic - [x] Commit message is descriptive ### PROCESS OBSERVATIONS Clean fix cycle. Both previous review findings addressed correctly. The pattern follows the established `team_ids` convention from the admin route, maintaining consistency across parent and admin response models. Low change-failure risk -- additive field on an existing endpoint with test coverage for both happy path (player with team) and edge case (player with no team). ### VERDICT: APPROVED

forgejo_admin merged commit 2853efd8fc into main 2026-04-04 21:44:47 +00:00

forgejo_admin deleted branch 333-add-team-ids-account-response 2026-04-04 21:44:47 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-04 21:44:49 +00:00

feat: add team_ids to AccountPlayerResponse (#333) (#334)

forgejo_admin referenced this pull request 2026-04-04 22:43:50 +00:00

Fix Alembic multiple heads — migration branch conflict #337

forgejo_admin referenced this pull request 2026-04-05 05:27:00 +00:00

Fix Alembic multiple heads — migration branch conflict #337

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/basketball-api!334

No description provided.