feat: Team model + assignment endpoints (Phase 10a) #82

Merged

forgejo_admin merged 3 commits from 81-team-model-assignment-endpoints into main 2026-03-14 22:34:51 +00:00

Conversation 4 Commits 3 Files changed 5 +1458

forgejo_admin commented 2026-03-14 22:21:16 +00:00

Owner

Copy link

Summary

Adds the Team entity as the bridge between players and coaches. Teams have division, age_group, and an optional coach assignment. Players get a nullable team_id FK. All team management is admin-only; coaches can view their own teams via /api/teams/mine.

Changes

• src/basketball_api/models.py -- Added AgeGroup enum, Team model (id, tenant_id, name, division, age_group, coach_id, created_at), team_id FK on Player, and bidirectional relationships (Team<->Player, Team<->Coach, Team<->Tenant)
• alembic/versions/008_add_team_model.py -- Migration creating teams table with agegroup enum and adding Player.team_id FK column (chains from revision 007)
• src/basketball_api/routes/teams.py -- New route file with 10 endpoints: POST (create), GET (list), GET overview, GET mine, GET detail, PATCH (update), DELETE (unassigns players), POST assign players, DELETE unassign player
• src/basketball_api/main.py -- Registered teams router at /api/teams
• tests/test_teams.py -- 28 tests covering all CRUD, assignment, coach view, overview, and auth enforcement

Test Plan

• pytest tests/test_teams.py -v -- 28 tests pass
• pytest tests/ -v -- all 209 tests pass (zero regressions)
• ruff check and ruff format clean

Review Checklist

• Tests pass (28 new, 209 total)
• Linting clean (ruff check + ruff format)
• Migration chains correctly (008 -> 007 -> e09c9e678004)
• Multi-tenant: all queries scoped by tenant_id
• Auth: admin-only for CRUD/assignment, coach for /mine
• No unrelated changes

Related

• Plan: plan-2026-03-08-tryout-prep Phase 10a
• Forgejo issue: #81

Closes #81

## Summary Adds the Team entity as the bridge between players and coaches. Teams have division, age_group, and an optional coach assignment. Players get a nullable `team_id` FK. All team management is admin-only; coaches can view their own teams via `/api/teams/mine`. ## Changes - `src/basketball_api/models.py` -- Added `AgeGroup` enum, `Team` model (id, tenant_id, name, division, age_group, coach_id, created_at), `team_id` FK on Player, and bidirectional relationships (Team<->Player, Team<->Coach, Team<->Tenant) - `alembic/versions/008_add_team_model.py` -- Migration creating `teams` table with `agegroup` enum and adding `Player.team_id` FK column (chains from revision 007) - `src/basketball_api/routes/teams.py` -- New route file with 10 endpoints: POST (create), GET (list), GET overview, GET mine, GET detail, PATCH (update), DELETE (unassigns players), POST assign players, DELETE unassign player - `src/basketball_api/main.py` -- Registered teams router at `/api/teams` - `tests/test_teams.py` -- 28 tests covering all CRUD, assignment, coach view, overview, and auth enforcement ## Test Plan - `pytest tests/test_teams.py -v` -- 28 tests pass - `pytest tests/ -v` -- all 209 tests pass (zero regressions) - `ruff check` and `ruff format` clean ## Review Checklist - [x] Tests pass (28 new, 209 total) - [x] Linting clean (ruff check + ruff format) - [x] Migration chains correctly (008 -> 007 -> e09c9e678004) - [x] Multi-tenant: all queries scoped by tenant_id - [x] Auth: admin-only for CRUD/assignment, coach for /mine - [x] No unrelated changes ## Related - Plan: `plan-2026-03-08-tryout-prep` Phase 10a - Forgejo issue: #81 Closes #81

forgejo_admin added 1 commit 2026-03-14 22:21:16 +00:00

feat: add Team model, CRUD endpoints, and player assignment (Phase 10a) ... a9e63375cd

Add Team entity as bridge between players and coaches. Teams have division,
age_group, and optional coach assignment. Players get a nullable team_id FK.
Admin-only CRUD, bulk player assignment/unassignment, coach-filtered /mine
endpoint, and admin overview summary. 28 new tests, all 209 pass.

Closes #81

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

forgejo_admin added 1 commit 2026-03-14 22:22:32 +00:00

refactor: clean up teams.py imports and remove dead code ... dcfd1c8802

Move AgeGroup and Division imports to module level (were unnecessarily
deferred inside function bodies). Remove unused require_coach_or_admin
dependency. Found during self-review.

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

forgejo_admin commented 2026-03-14 22:22:41 +00:00

Author

Owner

Copy link

Self-Review Findings

Two issues found and fixed in commit dcfd1c8:

1. Unused import -- require_coach_or_admin was defined at module level but never used in any endpoint. Removed.
2. Unnecessary deferred imports -- AgeGroup and Division were imported inside create_team() and update_team() function bodies via from basketball_api.models import ..., but both are already available from the top-level import of the models module. Moved to module-level import.

No functional changes. All 28 team tests and 209 total tests pass.

## Self-Review Findings Two issues found and fixed in commit dcfd1c8: 1. **Unused import** -- `require_coach_or_admin` was defined at module level but never used in any endpoint. Removed. 2. **Unnecessary deferred imports** -- `AgeGroup` and `Division` were imported inside `create_team()` and `update_team()` function bodies via `from basketball_api.models import ...`, but both are already available from the top-level import of the `models` module. Moved to module-level import. No functional changes. All 28 team tests and 209 total tests pass.

forgejo_admin commented 2026-03-14 22:24:33 +00:00

Author

Owner

Copy link

PR #82 Review

BLOCKERS

1. Cannot clear coach_id or nullable fields via PATCH

In update_team, the TeamUpdate schema uses None as the default for all fields, and the endpoint checks if body.coach_id is not None before updating. This means there is no way to unassign a coach from a team via PATCH -- sending {"coach_id": null} in JSON deserializes to None, which is the same as "not provided." The same issue applies to division and age_group -- once set, they cannot be cleared back to null.

The standard pattern for this is to use a sentinel value or restructure the schema (e.g., use UNSET sentinel, or accept explicit null via Optional with exclude_unset). This is a functional gap: admins will need to unassign coaches and clear divisions.

File: /home/ldraney/basketball-api/src/basketball_api/routes/teams.py, lines 42-46 (TeamUpdate) and lines 340-385 (update_team).

2. get_team detail endpoint lacks tenant scoping

The GET /api/teams/{team_id} endpoint (get_team) looks up a team by team_id alone via _get_team_or_404, with no tenant filter. An admin authenticated against one tenant could view team details from a different tenant by guessing IDs. All other endpoints properly scope by tenant_id, but this one does not.

Similarly, PATCH /api/teams/{team_id} and DELETE /api/teams/{team_id} use _get_team_or_404 without tenant scoping. While this is a single-tenant deployment today, it breaks the multi-tenant contract the rest of the codebase enforces. The _get_team_or_404 helper should accept a tenant_id parameter for the detail/update/delete endpoints, or those endpoints should accept a tenant_id query param and validate it.

File: /home/ldraney/basketball-api/src/basketball_api/routes/teams.py, lines 115-122 (_get_team_or_404), lines 327-334 (get_team), lines 337-385 (update_team), lines 388-407 (delete_team), lines 416-449 (assign_players), lines 452-487 (unassign_player).

3. Player cross-tenant assignment is only partially guarded

In assign_players, the check Player.tenant_id == team.tenant_id correctly prevents assigning a player from tenant B to a team in tenant A. However, a player who is already assigned to a different team will be silently reassigned without warning. This may or may not be intentional -- the issue acceptance criteria says "assign player(s) to a team" but doesn't specify behavior for already-assigned players. If re-assignment is intentional, that is fine, but it should be documented in the docstring. If not, the endpoint should warn or reject.

This is a soft blocker -- clarify the intended behavior and document it.

NITS

1. PlayerBrief has a position field but Player.position is not a common field for team rosters

The PlayerBrief response schema includes position but not height, graduating_class, or date_of_birth. This is a design choice but worth noting -- the roster endpoint includes more player detail. Not blocking.

2. created_at is typed as str in response schemas

TeamResponse.created_at and TeamDetail.created_at are typed as str, and the helper manually calls .isoformat(). Other endpoints in the codebase (e.g., RosterResponse) use datetime and let Pydantic handle serialization. Consider using datetime for consistency, but this works fine as-is.

3. Player.tenant_id in overview unassigned count

The overview endpoint counts unassigned players via Player.tenant_id == tenant_id and Player.team_id.is_(None). This counts all players in the tenant regardless of whether they have a Registration. The existing roster endpoints filter by join(Registration). This means the unassigned count might include players without registrations. Consider whether the count should only include registered players.

4. Route ordering: /mine and /overview before /{team_id}

This is correctly done -- /mine and /overview are defined before /{team_id} in the router, so FastAPI's route matching will not accidentally match "mine" or "overview" as a team_id. Good.

5. require_admin import in tests

The test file imports require_admin from basketball_api.routes.teams and overrides it in app.dependency_overrides. This follows the exact same pattern as admin.py where require_admin = require_role("admin") is a module-level dependency. Consistent and correct.

6. Missing __init__.py or explicit re-export of Team/AgeGroup

The new AgeGroup enum and Team model are added to models.py but not imported in __init__.py. This is consistent with the existing pattern (other models like Coach, EmailLog are not re-exported either). Not a problem, just noting.

7. tenant_id in the PR body says player.tenant_id -- the migration adds team_id not tenant_id to Player

This is just a PR body phrasing issue, not a code issue.

SOP COMPLIANCE

• Branch named after issue: 81-team-model-assignment-endpoints references issue #81
• PR body follows template: Has Summary, Changes, Test Plan, Related sections
• Related references plan slug: References plan-2026-03-08-tryout-prep Phase 10a
• Closes #81: Present in the PR body
• No secrets committed: No .env files, credentials, or API keys in the diff
• No scope creep: All 5 changed files are directly related to the Team feature
• Migration chains correctly: 008 -> 007 -> e09c9e678004 -> 005. Verified.
• Migration is reversible: downgrade() drops column, drops table, drops enum
• Commit messages: Single commit, descriptive title
• Auth guards: All CRUD and assignment endpoints use require_admin. /mine uses get_current_user. Correct.
• Test coverage: 28 tests cover create (5), list (2), detail (2), update (3), delete (2), assign (3), unassign (2), coach view (3), overview (2), auth enforcement (4). Edge cases covered: invalid division/age_group, nonexistent coach, nonexistent player, player not on team, delete unassigns players.

VERDICT: NOT APPROVED

Three blockers need resolution before merge:

1. PATCH cannot clear nullable fields (coach_id, division, age_group) -- functional gap that will bite admins immediately.
2. Detail/update/delete/assign/unassign endpoints lack tenant scoping -- breaks the multi-tenant contract enforced by every other endpoint in the codebase. Security concern.
3. Clarify and document re-assignment behavior when assigning an already-assigned player to a new team.

The overall quality is high -- clean code, follows existing patterns (module-level require_admin, Pydantic schemas, helper functions), solid test coverage, and correct migration. The blockers are fixable without major restructuring.

## PR #82 Review ### BLOCKERS **1. Cannot clear coach_id or nullable fields via PATCH** In `update_team`, the `TeamUpdate` schema uses `None` as the default for all fields, and the endpoint checks `if body.coach_id is not None` before updating. This means there is no way to *unassign* a coach from a team via PATCH -- sending `{"coach_id": null}` in JSON deserializes to `None`, which is the same as "not provided." The same issue applies to `division` and `age_group` -- once set, they cannot be cleared back to null. The standard pattern for this is to use a sentinel value or restructure the schema (e.g., use `UNSET` sentinel, or accept explicit `null` via `Optional` with `exclude_unset`). This is a functional gap: admins will need to unassign coaches and clear divisions. File: `/home/ldraney/basketball-api/src/basketball_api/routes/teams.py`, lines 42-46 (TeamUpdate) and lines 340-385 (update_team). **2. `get_team` detail endpoint lacks tenant scoping** The `GET /api/teams/{team_id}` endpoint (`get_team`) looks up a team by `team_id` alone via `_get_team_or_404`, with no tenant filter. An admin authenticated against one tenant could view team details from a different tenant by guessing IDs. All other endpoints properly scope by `tenant_id`, but this one does not. Similarly, `PATCH /api/teams/{team_id}` and `DELETE /api/teams/{team_id}` use `_get_team_or_404` without tenant scoping. While this is a single-tenant deployment today, it breaks the multi-tenant contract the rest of the codebase enforces. The `_get_team_or_404` helper should accept a `tenant_id` parameter for the detail/update/delete endpoints, or those endpoints should accept a `tenant_id` query param and validate it. File: `/home/ldraney/basketball-api/src/basketball_api/routes/teams.py`, lines 115-122 (`_get_team_or_404`), lines 327-334 (`get_team`), lines 337-385 (`update_team`), lines 388-407 (`delete_team`), lines 416-449 (`assign_players`), lines 452-487 (`unassign_player`). **3. Player cross-tenant assignment is only partially guarded** In `assign_players`, the check `Player.tenant_id == team.tenant_id` correctly prevents assigning a player from tenant B to a team in tenant A. However, a player who is *already assigned to a different team* will be silently reassigned without warning. This may or may not be intentional -- the issue acceptance criteria says "assign player(s) to a team" but doesn't specify behavior for already-assigned players. If re-assignment is intentional, that is fine, but it should be documented in the docstring. If not, the endpoint should warn or reject. This is a soft blocker -- clarify the intended behavior and document it. ### NITS **1. `PlayerBrief` has a `position` field but `Player.position` is not a common field for team rosters** The `PlayerBrief` response schema includes `position` but not `height`, `graduating_class`, or `date_of_birth`. This is a design choice but worth noting -- the roster endpoint includes more player detail. Not blocking. **2. `created_at` is typed as `str` in response schemas** `TeamResponse.created_at` and `TeamDetail.created_at` are typed as `str`, and the helper manually calls `.isoformat()`. Other endpoints in the codebase (e.g., `RosterResponse`) use `datetime` and let Pydantic handle serialization. Consider using `datetime` for consistency, but this works fine as-is. **3. `Player.tenant_id` in overview unassigned count** The overview endpoint counts unassigned players via `Player.tenant_id == tenant_id` and `Player.team_id.is_(None)`. This counts *all* players in the tenant regardless of whether they have a Registration. The existing roster endpoints filter by `join(Registration)`. This means the unassigned count might include players without registrations. Consider whether the count should only include registered players. **4. Route ordering: `/mine` and `/overview` before `/{team_id}`** This is correctly done -- `/mine` and `/overview` are defined before `/{team_id}` in the router, so FastAPI's route matching will not accidentally match "mine" or "overview" as a `team_id`. Good. **5. `require_admin` import in tests** The test file imports `require_admin` from `basketball_api.routes.teams` and overrides it in `app.dependency_overrides`. This follows the exact same pattern as `admin.py` where `require_admin = require_role("admin")` is a module-level dependency. Consistent and correct. **6. Missing `__init__.py` or explicit re-export of Team/AgeGroup** The new `AgeGroup` enum and `Team` model are added to `models.py` but not imported in `__init__.py`. This is consistent with the existing pattern (other models like `Coach`, `EmailLog` are not re-exported either). Not a problem, just noting. **7. `tenant_id` in the PR body says `player.tenant_id` -- the migration adds `team_id` not `tenant_id` to Player** This is just a PR body phrasing issue, not a code issue. ### SOP COMPLIANCE - [x] **Branch named after issue**: `81-team-model-assignment-endpoints` references issue #81 - [x] **PR body follows template**: Has Summary, Changes, Test Plan, Related sections - [x] **Related references plan slug**: References `plan-2026-03-08-tryout-prep` Phase 10a - [x] **`Closes #81`**: Present in the PR body - [x] **No secrets committed**: No `.env` files, credentials, or API keys in the diff - [x] **No scope creep**: All 5 changed files are directly related to the Team feature - [x] **Migration chains correctly**: 008 -> 007 -> e09c9e678004 -> 005. Verified. - [x] **Migration is reversible**: `downgrade()` drops column, drops table, drops enum - [x] **Commit messages**: Single commit, descriptive title - [x] **Auth guards**: All CRUD and assignment endpoints use `require_admin`. `/mine` uses `get_current_user`. Correct. - [x] **Test coverage**: 28 tests cover create (5), list (2), detail (2), update (3), delete (2), assign (3), unassign (2), coach view (3), overview (2), auth enforcement (4). Edge cases covered: invalid division/age_group, nonexistent coach, nonexistent player, player not on team, delete unassigns players. ### VERDICT: NOT APPROVED Three blockers need resolution before merge: 1. **PATCH cannot clear nullable fields** (coach_id, division, age_group) -- functional gap that will bite admins immediately. 2. **Detail/update/delete/assign/unassign endpoints lack tenant scoping** -- breaks the multi-tenant contract enforced by every other endpoint in the codebase. Security concern. 3. **Clarify and document re-assignment behavior** when assigning an already-assigned player to a new team. The overall quality is high -- clean code, follows existing patterns (module-level `require_admin`, Pydantic schemas, helper functions), solid test coverage, and correct migration. The blockers are fixable without major restructuring.

forgejo_admin added 1 commit 2026-03-14 22:29:43 +00:00

fix: resolve 3 QA blockers — nullable PATCH, tenant scoping, reassignment warning ... 2f6b564473

1. PATCH cannot clear nullable fields: Use model_dump(exclude_unset=True)
   so sending {"coach_id": null} clears the coach while omitting coach_id
   leaves it unchanged. Same for division and age_group.

2. Tenant scoping on 6 endpoints: _get_team_or_404 now accepts optional
   tenant_id parameter. get_team, update_team, delete_team, assign_players,
   and unassign_player all require tenant_id query param, preventing
   cross-tenant access.

3. Silent player re-assignment: assign_players now tracks and returns a
   "reassigned" list in the response when players are moved from another
   team, so the caller knows which players were re-assigned.

Tests: 216 passing (7 new — clear coach, clear division, omitted fields
unchanged, reassign warning, same-team no warning, cross-tenant get 404,
cross-tenant delete 404).

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

forgejo_admin commented 2026-03-14 22:29:56 +00:00

Author

Owner

Copy link

QA Blocker Fixes (commit 2f6b564)

All three blockers from the review have been resolved:

1. PATCH cannot clear nullable fields

• update_team now uses body.model_dump(exclude_unset=True) to distinguish "not provided" from "explicitly set to null"
• Sending {"coach_id": null} clears the coach; omitting coach_id leaves it unchanged
• Same pattern applies to division and age_group
• Added name cannot be null validation guard

2. Tenant scoping on 6 endpoints

• _get_team_or_404 now accepts an optional tenant_id keyword argument and filters by Team.tenant_id when provided
• All 6 endpoints (get_team, update_team, delete_team, assign_players, unassign_player) now require a tenant_id query parameter
• Each endpoint validates the tenant exists via _get_tenant_or_404 before querying the team
• Cross-tenant access now returns 404

3. Silent player re-assignment documented with warning

• Chose the "allow re-assignment with warning" approach: if a player is already on a different team, the assignment proceeds but the response includes a reassigned list with player_id, player_name, from_team_id, and from_team_name
• If no players were re-assigned, the reassigned key is omitted from the response
• Behavior is documented in the endpoint docstring

New tests (7 added, 216 total passing)

• test_update_team_clear_coach -- verifies {"coach_id": null} clears the coach
• test_update_team_clear_division -- verifies {"division": null} clears division
• test_update_team_omitted_fields_unchanged -- verifies omitted fields stay unchanged
• test_reassign_player_returns_warning -- verifies reassigned list in response
• test_assign_already_on_same_team_no_warning -- verifies no warning for same-team re-assign
• test_get_team_wrong_tenant_returns_404 -- verifies cross-tenant isolation
• test_delete_team_wrong_tenant_returns_404 -- verifies cross-tenant delete blocked

## QA Blocker Fixes (commit 2f6b564) All three blockers from the review have been resolved: ### 1. PATCH cannot clear nullable fields - `update_team` now uses `body.model_dump(exclude_unset=True)` to distinguish "not provided" from "explicitly set to null" - Sending `{"coach_id": null}` clears the coach; omitting `coach_id` leaves it unchanged - Same pattern applies to `division` and `age_group` - Added `name cannot be null` validation guard ### 2. Tenant scoping on 6 endpoints - `_get_team_or_404` now accepts an optional `tenant_id` keyword argument and filters by `Team.tenant_id` when provided - All 6 endpoints (`get_team`, `update_team`, `delete_team`, `assign_players`, `unassign_player`) now require a `tenant_id` query parameter - Each endpoint validates the tenant exists via `_get_tenant_or_404` before querying the team - Cross-tenant access now returns 404 ### 3. Silent player re-assignment documented with warning - Chose the "allow re-assignment with warning" approach: if a player is already on a different team, the assignment proceeds but the response includes a `reassigned` list with `player_id`, `player_name`, `from_team_id`, and `from_team_name` - If no players were re-assigned, the `reassigned` key is omitted from the response - Behavior is documented in the endpoint docstring ### New tests (7 added, 216 total passing) - `test_update_team_clear_coach` -- verifies `{"coach_id": null}` clears the coach - `test_update_team_clear_division` -- verifies `{"division": null}` clears division - `test_update_team_omitted_fields_unchanged` -- verifies omitted fields stay unchanged - `test_reassign_player_returns_warning` -- verifies `reassigned` list in response - `test_assign_already_on_same_team_no_warning` -- verifies no warning for same-team re-assign - `test_get_team_wrong_tenant_returns_404` -- verifies cross-tenant isolation - `test_delete_team_wrong_tenant_returns_404` -- verifies cross-tenant delete blocked

forgejo_admin commented 2026-03-14 22:34:00 +00:00

Author

Owner

Copy link

PR #82 Review (Re-review)

BLOCKERS

None. All three blockers from the previous review have been resolved.

Previous Blockers -- Verified Fixed

1. PATCH nullable fields -- update_team now uses body.model_dump(exclude_unset=True) (line 361). Sending {"coach_id": null} clears the coach; omitting coach_id leaves it unchanged. Same pattern for division and age_group. Explicit null-guard on name prevents setting it to null (line 364-365). Verified by three new tests: test_update_team_clear_coach, test_update_team_clear_division, test_update_team_omitted_fields_unchanged.

2. Tenant scoping -- _get_team_or_404 now accepts a tenant_id keyword argument (line 116). All 6 mutating/detail endpoints pass tenant_id to it. Coach lookup in PATCH also scopes by team.tenant_id (line 400). Player lookups in assign/unassign scope by team.tenant_id (lines 465, 528). Verified by TestTenantIsolation class with cross-tenant GET and DELETE tests.

3. Re-assignment warning -- assign_players now tracks players moving from a different team and includes a reassigned list with player_id, player_name, from_team_id, from_team_name (lines 478-508). The key is omitted when no re-assignments occur (line 506 conditional). Verified by test_reassign_player_returns_warning and test_assign_already_on_same_team_no_warning.

NITS

1. The reload calls after commit (lines 229, 411, 495, 544) call _get_team_or_404(db, team.id) without tenant_id. This is safe since the team was already tenant-validated earlier in each endpoint, but passing tenant_id consistently would be more defensive. Non-blocking.

2. Test count in the PR body says "28 tests" and "209 total" but the actual counts are 35 tests in test_teams.py and 216 total. The body should be updated to reflect the fix cycle additions. Non-blocking.

SOP COMPLIANCE

• Branch named after issue (81-team-model-assignment-endpoints -> Issue #81)
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug (plan-2026-03-08-tryout-prep Phase 10a) and issue #81
• Closes #81 present in PR body
• Tests exist (35 tests covering CRUD, assignment, coach view, overview, auth, tenant isolation)
• 216 total tests across the repo
• No secrets, .env files, or credentials committed
• No scope creep -- changes limited to Team model, migration, routes, and tests
• Migration chains correctly (008 -> 007 -> e09c9e678004 -> 005 -> ...)

VERDICT: APPROVED

## PR #82 Review (Re-review) ### BLOCKERS None. All three blockers from the previous review have been resolved. ### Previous Blockers -- Verified Fixed 1. **PATCH nullable fields** -- `update_team` now uses `body.model_dump(exclude_unset=True)` (line 361). Sending `{"coach_id": null}` clears the coach; omitting `coach_id` leaves it unchanged. Same pattern for `division` and `age_group`. Explicit null-guard on `name` prevents setting it to null (line 364-365). Verified by three new tests: `test_update_team_clear_coach`, `test_update_team_clear_division`, `test_update_team_omitted_fields_unchanged`. 2. **Tenant scoping** -- `_get_team_or_404` now accepts a `tenant_id` keyword argument (line 116). All 6 mutating/detail endpoints pass `tenant_id` to it. Coach lookup in PATCH also scopes by `team.tenant_id` (line 400). Player lookups in assign/unassign scope by `team.tenant_id` (lines 465, 528). Verified by `TestTenantIsolation` class with cross-tenant GET and DELETE tests. 3. **Re-assignment warning** -- `assign_players` now tracks players moving from a different team and includes a `reassigned` list with `player_id`, `player_name`, `from_team_id`, `from_team_name` (lines 478-508). The key is omitted when no re-assignments occur (line 506 conditional). Verified by `test_reassign_player_returns_warning` and `test_assign_already_on_same_team_no_warning`. ### NITS 1. The reload calls after commit (lines 229, 411, 495, 544) call `_get_team_or_404(db, team.id)` without `tenant_id`. This is safe since the team was already tenant-validated earlier in each endpoint, but passing `tenant_id` consistently would be more defensive. Non-blocking. 2. Test count in the PR body says "28 tests" and "209 total" but the actual counts are 35 tests in test_teams.py and 216 total. The body should be updated to reflect the fix cycle additions. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue (`81-team-model-assignment-endpoints` -> Issue #81) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references plan slug (`plan-2026-03-08-tryout-prep Phase 10a`) and issue #81 - [x] `Closes #81` present in PR body - [x] Tests exist (35 tests covering CRUD, assignment, coach view, overview, auth, tenant isolation) - [x] 216 total tests across the repo - [x] No secrets, .env files, or credentials committed - [x] No scope creep -- changes limited to Team model, migration, routes, and tests - [x] Migration chains correctly (008 -> 007 -> e09c9e678004 -> 005 -> ...) ### VERDICT: APPROVED

forgejo_admin merged commit 63dce23e0b into main 2026-03-14 22:34:51 +00:00

forgejo_admin deleted branch 81-team-model-assignment-endpoints 2026-03-14 22:34:51 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-14 22:34:52 +00:00

feat: Team model + assignment endpoints (Phase 10a) (#82)

forgejo_admin referenced this pull request 2026-03-14 23:27:48 +00:00

feat: Stripe subscription endpoints + webhooks for monthly payments #84

forgejo_admin referenced this pull request 2026-03-15 01:05:06 +00:00

bug: non-idempotent division enum in migration 008 causes CrashLoopBackOff on deploy #85

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

forgejo_admin/basketball-api!82

No description provided.