forgejo_admin/basketball-api

Fork 1

fix: clear team assignments before appending in teams/save #174

Merged

forgejo_admin merged 1 commit from 173-fix-teams-save-replace-not-append into main

2026-03-26 23:44:35 +00:00

forgejo_admin commented

2026-03-26 23:42:01 +00:00

Owner

Summary

The POST /admin/teams/save endpoint used append-only logic for team assignments, causing players to accumulate multiple teams in the player_teams junction table instead of being moved to the new team. This adds player.teams.clear() before player.teams.append() in both the temp-team and existing-team branches, enforcing single-team draft board semantics.

Changes

src/basketball_api/routes/admin.py -- In admin_teams_save(), changed the idempotency check from "skip if player is already on this team" (real_team.id not in cids) to "skip if player is exclusively on this team" (current_ids != {target_id}). When a change is needed, existing team memberships are cleared before the new assignment is appended. Applied to both the temp-team branch (line ~749) and the existing-team branch (line ~769).

Test Plan

test_temp_team_dedup -- previously failing, now passes (player ends up with 1 team, not 2)
test_no_change_not_counted -- still passes (assigning to current team is a no-op)
All 17 test_admin_teams.py tests pass
Full suite: 555 passed, 0 failed
ruff format and ruff check clean

Review Checklist

player.teams.clear() called before appending in both temp-team and existing-team branches
Unassign branch (tid is None) unchanged -- already correct
Idempotency preserved: assigning to current exclusive team is a no-op
No test changes needed -- existing tests validate the fix
Full test suite passes (555/555)
ruff format and ruff check clean

Closes #173
Unblocks: #172 (jersey sync fix blocked by this CI failure)

## Summary The `POST /admin/teams/save` endpoint used append-only logic for team assignments, causing players to accumulate multiple teams in the `player_teams` junction table instead of being moved to the new team. This adds `player.teams.clear()` before `player.teams.append()` in both the temp-team and existing-team branches, enforcing single-team draft board semantics. ## Changes - `src/basketball_api/routes/admin.py` -- In `admin_teams_save()`, changed the idempotency check from "skip if player is already on this team" (`real_team.id not in cids`) to "skip if player is exclusively on this team" (`current_ids != {target_id}`). When a change is needed, existing team memberships are cleared before the new assignment is appended. Applied to both the temp-team branch (line ~749) and the existing-team branch (line ~769). ## Test Plan - `test_temp_team_dedup` -- previously failing, now passes (player ends up with 1 team, not 2) - `test_no_change_not_counted` -- still passes (assigning to current team is a no-op) - All 17 `test_admin_teams.py` tests pass - Full suite: 555 passed, 0 failed - `ruff format` and `ruff check` clean ## Review Checklist - [x] `player.teams.clear()` called before appending in both temp-team and existing-team branches - [x] Unassign branch (tid is None) unchanged -- already correct - [x] Idempotency preserved: assigning to current exclusive team is a no-op - [x] No test changes needed -- existing tests validate the fix - [x] Full test suite passes (555/555) - [x] ruff format and ruff check clean ## Related - Closes #173 - Unblocks: #172 (jersey sync fix blocked by this CI failure)

forgejo_admin added 1 commit

2026-03-26 23:42:01 +00:00

fix: clear existing team assignments before appending in teams/save endpoint

ci/woodpecker/pr/woodpecker Pipeline failed

Details

d976ebab90

The draft board save endpoint used append-only logic for team assignments,
causing players to accumulate multiple teams instead of being moved to the
new one. Now clears existing assignments before appending, enforcing
single-team semantics while preserving the no-op optimization when a
player is already exclusively on the target team.

Fixes #173

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

forgejo_admin commented

2026-03-26 23:42:27 +00:00

Author

Owner

Review: LGTM

Diff: +6/-4 in 1 file -- minimal, surgical fix.

Correctness

The old check (real_team.id not in cids) only tested whether the target team was already present, but never removed other teams. A player with team A assigned to team B would pass the check (B not in {A}), append B, and end up with {A, B}.
The new check (current_ids != {real_team.id}) correctly tests whether the player is exclusively on the target team. If not, it clears all memberships first, then appends. This enforces single-team draft board semantics.
The unassign branch (tid is None) was already correct and is unchanged.

Idempotency

If a player is already exclusively on the target team, current_ids == {target_id} evaluates true, the block is skipped, and updated is not incremented. This preserves the test_no_change_not_counted behavior.

Edge cases considered

Player with no teams assigned to a new team: current_ids = set(), set() != {team_id} is true, clear() on empty list is a no-op, append works. Correct.
Player with multiple teams (shouldn't happen but defensive): clear() removes all, append adds the one. Correct.

Test coverage

555/555 pass. The previously-failing test_temp_team_dedup now passes. No regressions.

No fixes needed. Ready for merge.

## Review: LGTM **Diff: +6/-4 in 1 file** -- minimal, surgical fix. ### Correctness - The old check (`real_team.id not in cids`) only tested whether the target team was already present, but never removed other teams. A player with team A assigned to team B would pass the check (B not in {A}), append B, and end up with {A, B}. - The new check (`current_ids != {real_team.id}`) correctly tests whether the player is *exclusively* on the target team. If not, it clears all memberships first, then appends. This enforces single-team draft board semantics. - The unassign branch (tid is None) was already correct and is unchanged. ### Idempotency - If a player is already exclusively on the target team, `current_ids == {target_id}` evaluates true, the block is skipped, and `updated` is not incremented. This preserves the `test_no_change_not_counted` behavior. ### Edge cases considered - Player with no teams assigned to a new team: `current_ids = set()`, `set() != {team_id}` is true, `clear()` on empty list is a no-op, append works. Correct. - Player with multiple teams (shouldn't happen but defensive): `clear()` removes all, append adds the one. Correct. ### Test coverage - 555/555 pass. The previously-failing `test_temp_team_dedup` now passes. No regressions. **No fixes needed. Ready for merge.**

forgejo_admin commented

2026-03-26 23:43:49 +00:00

Author

Owner

PR #174 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy (many-to-many relationship via player_teams junction table).

What the fix does: The admin_teams_save() endpoint previously used an append-only check (real_team.id not in cids) which silently accumulated multiple team assignments in the player_teams junction table. The fix changes both the temp-team and existing-team branches to:

Check whether the player's current team set already equals the target (current_ids != {target_id}) -- preserving idempotency.
Call player.teams.clear() before player.teams.append(...) -- enforcing single-team semantics.

Correctness analysis:

The player.team_id property (line 229 in models.py) returns self.teams[0].id if self.teams else None, confirming the codebase assumes at most one team per player. The clear-then-append pattern is the correct fix for this invariant.
The idempotency check current_ids != {real_team.id} is semantically correct: if the player is exclusively on the target team already, no work is done and updated is not incremented. This is a tighter check than the old not in -- it also triggers reassignment when the player is on multiple teams (cleanup of prior corruption), which is desirable.
The unassign branch (tid is None) was already using player.teams.clear() and is unchanged -- correct.
Both the temp-team branch (line ~752) and existing-team branch (line ~772) receive the same fix, so there is no DRY divergence between the two code paths. The duplication between them is structural (different lookup logic for temp vs. existing teams) and acceptable.
lazy="selectin" on the relationship means player.teams is loaded eagerly, so the {t.id for t in player.teams} comprehension will not cause N+1 query issues within the loop.

SQLAlchemy session safety: player.teams.clear() followed by player.teams.append(real_team) within the same transaction (committed at line 777) is safe. SQLAlchemy will issue DELETE then INSERT on the junction table within the flush.

No issues found in domain review.

BLOCKERS

None.

No new functionality without tests -- this is a bug fix, and existing tests (test_temp_team_dedup, test_no_change_not_counted) validate the corrected behavior. 17 tests in test_admin_teams.py cover happy path, edge cases (empty assignments, nonexistent player/team, mixed operations, string integer team IDs), and auth (401 without auth).
No unvalidated user input -- team_id and player_id are validated (404 for missing player/team, 422 for invalid team_id format).
No secrets or credentials in code.
No DRY violation in auth paths -- require_admin dependency is used consistently.

NITS

The variable cids was renamed to current_ids in both branches -- good improvement for readability. No further naming concerns.
The two branches (temp-team at line ~750 and existing-team at line ~771) share identical clear-then-append logic. A future refactor could extract a _reassign_player(player, team) helper, but for a 2-line pattern this is acceptable as-is and would be scope creep for a bug fix PR.

SOP COMPLIANCE

Branch named after issue (173-fix-teams-save-replace-not-append references issue #173)
PR body has: Summary, Changes, Test Plan, Related
Related section references parent issue (Closes #173) and downstream dependency (#172)
No plan slug required (board-driven bug fix, not plan work)
Tests exist and pass (555/555 per PR body, 17 in test_admin_teams.py)
No secrets, .env files, or credentials committed
No unnecessary file changes (1 file changed, surgically scoped)
Commit message is descriptive (fix: clear team assignments before appending in teams/save)
ruff format and ruff check clean per PR body

PROCESS OBSERVATIONS

Change failure risk: Low. The fix is minimal (6 additions, 4 deletions), surgically scoped to one function, and covered by existing tests that previously exposed the bug.
Deployment frequency: Unblocks #172 (jersey sync fix), so merging promptly maintains flow.
Documentation: No docs impact -- this is an internal API behavior fix, not a contract change.

VERDICT: APPROVED

## PR #174 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy (many-to-many relationship via `player_teams` junction table). **What the fix does**: The `admin_teams_save()` endpoint previously used an append-only check (`real_team.id not in cids`) which silently accumulated multiple team assignments in the `player_teams` junction table. The fix changes both the temp-team and existing-team branches to: 1. Check whether the player's current team set already equals the target (`current_ids != {target_id}`) -- preserving idempotency. 2. Call `player.teams.clear()` before `player.teams.append(...)` -- enforcing single-team semantics. **Correctness analysis**: - The `player.team_id` property (line 229 in `models.py`) returns `self.teams[0].id if self.teams else None`, confirming the codebase assumes at most one team per player. The clear-then-append pattern is the correct fix for this invariant. - The idempotency check `current_ids != {real_team.id}` is semantically correct: if the player is exclusively on the target team already, no work is done and `updated` is not incremented. This is a tighter check than the old `not in` -- it also triggers reassignment when the player is on *multiple* teams (cleanup of prior corruption), which is desirable. - The unassign branch (`tid is None`) was already using `player.teams.clear()` and is unchanged -- correct. - Both the temp-team branch (line ~752) and existing-team branch (line ~772) receive the same fix, so there is no DRY divergence between the two code paths. The duplication between them is structural (different lookup logic for temp vs. existing teams) and acceptable. - `lazy="selectin"` on the relationship means `player.teams` is loaded eagerly, so the `{t.id for t in player.teams}` comprehension will not cause N+1 query issues within the loop. **SQLAlchemy session safety**: `player.teams.clear()` followed by `player.teams.append(real_team)` within the same transaction (committed at line 777) is safe. SQLAlchemy will issue DELETE then INSERT on the junction table within the flush. No issues found in domain review. ### BLOCKERS None. - No new functionality without tests -- this is a bug fix, and existing tests (`test_temp_team_dedup`, `test_no_change_not_counted`) validate the corrected behavior. 17 tests in `test_admin_teams.py` cover happy path, edge cases (empty assignments, nonexistent player/team, mixed operations, string integer team IDs), and auth (401 without auth). - No unvalidated user input -- `team_id` and `player_id` are validated (404 for missing player/team, 422 for invalid team_id format). - No secrets or credentials in code. - No DRY violation in auth paths -- `require_admin` dependency is used consistently. ### NITS 1. The variable `cids` was renamed to `current_ids` in both branches -- good improvement for readability. No further naming concerns. 2. The two branches (temp-team at line ~750 and existing-team at line ~771) share identical clear-then-append logic. A future refactor could extract a `_reassign_player(player, team)` helper, but for a 2-line pattern this is acceptable as-is and would be scope creep for a bug fix PR. ### SOP COMPLIANCE - [x] Branch named after issue (`173-fix-teams-save-replace-not-append` references issue #173) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references parent issue (`Closes #173`) and downstream dependency (`#172`) - [x] No plan slug required (board-driven bug fix, not plan work) - [x] Tests exist and pass (555/555 per PR body, 17 in `test_admin_teams.py`) - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes (1 file changed, surgically scoped) - [x] Commit message is descriptive (`fix: clear team assignments before appending in teams/save`) - [x] `ruff format` and `ruff check` clean per PR body ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The fix is minimal (6 additions, 4 deletions), surgically scoped to one function, and covered by existing tests that previously exposed the bug. - **Deployment frequency**: Unblocks #172 (jersey sync fix), so merging promptly maintains flow. - **Documentation**: No docs impact -- this is an internal API behavior fix, not a contract change. ### VERDICT: APPROVED