Bug: teams/save endpoint appends teams instead of replacing assignment #173

New issue

Closed

opened 2026-03-26 23:15:07 +00:00 by forgejo_admin · 1 comment

forgejo_admin commented

2026-03-26 23:15:07 +00:00

Owner

Type

Bug

Lineage

standalone — discovered during CI investigation (pre-existing failure blocking deploy of #172)

Repo

forgejo_admin/basketball-api

What Broke

The POST /admin/teams/save endpoint uses player.teams.append(team) to assign players to teams, but never clears existing team assignments first. When a player already has a team and gets reassigned (e.g., via the draft board), they end up with multiple teams instead of being moved to the new one.

The player_teams many-to-many junction table accumulates rows instead of replacing. Test test_temp_team_dedup fails because player1 starts with 1 team (from fixture), gets assigned to temp team t999, and ends up with 2 teams.

Error: assert len(player1.teams) == 1 → AssertionError: assert 2 == 1

Repro Steps

Player1 is assigned to "U14 Boys Elite" (via fixture)
POST /admin/teams/save with {"assignments": [{"player_id": player1.id, "team_id": "t999"}, {"player_id": player2.id, "team_id": "t999"}]}
Response returns 200, created_teams: 1, updated: 2 (looks correct)
player1.teams has 2 entries: original team + new temp team
Expected: player1 should only be on the new temp team

Expected Behavior

When a player is assigned to a team via the draft board save endpoint, the assignment should replace any existing team membership, not append to it. The draft board is an exclusive assignment tool — a player belongs to one team.

Environment

Cluster/namespace: CI (also affects prod behavior)
Service version/commit: current main (pre-existing since many-to-many migration)
Related alerts: CI pipeline #143 failure (blocks deploy of PR #172)

File Targets

src/basketball_api/routes/admin.py — admin_teams_save() (line 697). Three branches need fix:
- Temp team branch (line 749-753): add player.teams.clear() before player.teams.append(real_team)
- Existing team branch (line 770-773): add player.teams.clear() before player.teams.append(team)
- Unassign branch (line 732-735): already correct — calls player.teams.clear()
tests/test_admin_teams.py — test_temp_team_dedup (line 290). Currently failing. Should pass after fix with no test changes needed.

Test Expectations

Run: cd ~/basketball-api && python -m pytest tests/test_admin_teams.py -v

test_temp_team_dedup should pass (currently failing)
All other test_admin_teams.py tests should continue to pass
Full suite: python -m pytest — 555 passed, 0 failed

Acceptance Criteria

player.teams.clear() called before appending in both temp-team and existing-team branches of admin_teams_save()
test_temp_team_dedup passes
No regressions in test_admin_teams.py (all 18 tests pass)
Full test suite passes (555 total, 0 failures)
CI pipeline succeeds on merge

Constraints

This is a 2-line fix — player.teams.clear() in two places
Draft board semantics = exclusive assignment (1 player : 1 team at a time)
The many-to-many player_teams table stays (future multi-team support possible), but the save endpoint enforces single-team for now

forgejo_admin/basketball-api#170 — jersey sync fix blocked by this CI failure
alembic/versions/019_player_teams_junction.py — migration that introduced the many-to-many
project-westside-basketball — project this affects

### Type Bug ### Lineage standalone — discovered during CI investigation (pre-existing failure blocking deploy of #172) ### Repo `forgejo_admin/basketball-api` ### What Broke The `POST /admin/teams/save` endpoint uses `player.teams.append(team)` to assign players to teams, but never clears existing team assignments first. When a player already has a team and gets reassigned (e.g., via the draft board), they end up with multiple teams instead of being moved to the new one. The `player_teams` many-to-many junction table accumulates rows instead of replacing. Test `test_temp_team_dedup` fails because `player1` starts with 1 team (from fixture), gets assigned to temp team `t999`, and ends up with 2 teams. Error: `assert len(player1.teams) == 1` → `AssertionError: assert 2 == 1` ### Repro Steps 1. Player1 is assigned to "U14 Boys Elite" (via fixture) 2. `POST /admin/teams/save` with `{"assignments": [{"player_id": player1.id, "team_id": "t999"}, {"player_id": player2.id, "team_id": "t999"}]}` 3. Response returns `200`, `created_teams: 1`, `updated: 2` (looks correct) 4. `player1.teams` has 2 entries: original team + new temp team 5. Expected: player1 should only be on the new temp team ### Expected Behavior When a player is assigned to a team via the draft board save endpoint, the assignment should **replace** any existing team membership, not append to it. The draft board is an exclusive assignment tool — a player belongs to one team. ### Environment - Cluster/namespace: CI (also affects prod behavior) - Service version/commit: current main (pre-existing since many-to-many migration) - Related alerts: CI pipeline #143 failure (blocks deploy of PR #172) ### File Targets - `src/basketball_api/routes/admin.py` — `admin_teams_save()` (line 697). Three branches need fix: - Temp team branch (line 749-753): add `player.teams.clear()` before `player.teams.append(real_team)` - Existing team branch (line 770-773): add `player.teams.clear()` before `player.teams.append(team)` - Unassign branch (line 732-735): already correct — calls `player.teams.clear()` - `tests/test_admin_teams.py` — `test_temp_team_dedup` (line 290). Currently failing. Should pass after fix with no test changes needed. ### Test Expectations Run: `cd ~/basketball-api && python -m pytest tests/test_admin_teams.py -v` - `test_temp_team_dedup` should pass (currently failing) - All other `test_admin_teams.py` tests should continue to pass - Full suite: `python -m pytest` — 555 passed, 0 failed ### Acceptance Criteria - [ ] `player.teams.clear()` called before appending in both temp-team and existing-team branches of `admin_teams_save()` - [ ] `test_temp_team_dedup` passes - [ ] No regressions in `test_admin_teams.py` (all 18 tests pass) - [ ] Full test suite passes (555 total, 0 failures) - [ ] CI pipeline succeeds on merge ### Constraints - This is a 2-line fix — `player.teams.clear()` in two places - Draft board semantics = exclusive assignment (1 player : 1 team at a time) - The many-to-many `player_teams` table stays (future multi-team support possible), but the save endpoint enforces single-team for now ### Related - `forgejo_admin/basketball-api#170` — jersey sync fix blocked by this CI failure - `alembic/versions/019_player_teams_junction.py` — migration that introduced the many-to-many - `project-westside-basketball` — project this affects

forgejo_admin commented

2026-03-26 23:17:39 +00:00

Author

Owner

Scope Review: NEEDS_REFINEMENT

Review note: review-404-2026-03-26

Scope document is thorough and well-written. All file targets verified (line numbers, code patterns, three branches). Acceptance criteria are all agent-verifiable. Single issue found:

Label mismatch: Board item #404 has arch:stripe-webhook but this bug is in /admin/teams/save (no Stripe/webhook involvement). Should be arch:admin-api or arch:draft-board. Same mislabel on board item #393.

Once the label is corrected, this ticket is READY for execution.

## Scope Review: NEEDS_REFINEMENT Review note: `review-404-2026-03-26` Scope document is thorough and well-written. All file targets verified (line numbers, code patterns, three branches). Acceptance criteria are all agent-verifiable. Single issue found: - **Label mismatch**: Board item #404 has `arch:stripe-webhook` but this bug is in `/admin/teams/save` (no Stripe/webhook involvement). Should be `arch:admin-api` or `arch:draft-board`. Same mislabel on board item #393. Once the label is corrected, this ticket is READY for execution.