feat: wire /admin/teams GET + POST /admin/teams/save for SPA team management #122

Open

forgejo_admin wants to merge 0 commits from 121-admin-teams-spa-endpoints into main

pull from: 121-admin-teams-spa-endpoints

merge into: forgejo_admin:main

forgejo_admin:main

forgejo_admin:232-schedule-data-model

forgejo_admin:205-clean-test-data-from-production-db-playe

forgejo_admin:114-email-verification

forgejo_admin:147-feat-add-get-jersey-player-info-endpoint

forgejo_admin:124-feat-many-to-many-player-team-assignment

forgejo_admin:125-feat-add-new-players-manually-seydou-gou

forgejo_admin:126-feat-team-placement-congratulations-emai

forgejo_admin:70-add-graduating-class-to-roster-api-respo

forgejo_admin:59-fix-auth-test

forgejo_admin:62-phase-4l-sveltekit-dashboard-wkq-tailsca

forgejo_admin:11-phase-1a-fix-ci-pipeline-venv-in-test-st

Conversation 2 Commits - Files changed -

forgejo_admin commented 2026-03-19 20:13:00 +00:00

Owner

Copy link

Summary

Adds two endpoints to the admin router for the SPA draft-board UI in westside-app. GET /admin/teams returns all teams and players in the shape the frontend expects, and POST /admin/teams/save accepts bulk player-to-team assignments including creating new teams from temporary IDs.

Changes

• src/basketball_api/routes/admin.py -- Added GET /admin/teams endpoint returning { teams, players } with auto-resolved tenant, and POST /admin/teams/save endpoint accepting { assignments: [{ player_id, team_id }] } with support for existing team assignment (int), new team creation (string starting with t), and unassignment (null)
• tests/test_admin_teams.py -- 17 new tests covering GET response shape, player/team data, empty state, coach presence/absence, POST assign/unassign/create/dedup/mixed operations, error handling, and auth enforcement
• src/basketball_api/routes/register.py -- Fixed pre-existing E501 lint violations (line length > 100)
• src/basketball_api/services/email.py -- Fixed pre-existing E501 lint violations in HTML template strings

Test Plan

• 17 new tests all pass: pytest tests/test_admin_teams.py -v
• Full suite: 435/435 pass (1 pre-existing failure in test_players.py::test_patch_ignores_read_only_fields unrelated to this PR)
• ruff check . and ruff format . clean

Review Checklist

• Endpoints match frontend contract in westside-app/src/routes/admin/teams/+page.svelte
• GET response shape: { teams: [{ id, name, coaches }], players: [{ id, name, division, position, team_id, team, height, photo_url, parent_name }] }
• POST handles int team_id, string temp ID (t-prefix), and null (unassign)
• Temp team deduplication (same temp ID creates only one team)
• Auth enforcement (401/403 without admin role)
• Pre-existing lint violations fixed (not introduced by this PR)

Related

• Closes #121

## Summary Adds two endpoints to the admin router for the SPA draft-board UI in westside-app. `GET /admin/teams` returns all teams and players in the shape the frontend expects, and `POST /admin/teams/save` accepts bulk player-to-team assignments including creating new teams from temporary IDs. ## Changes - `src/basketball_api/routes/admin.py` -- Added `GET /admin/teams` endpoint returning `{ teams, players }` with auto-resolved tenant, and `POST /admin/teams/save` endpoint accepting `{ assignments: [{ player_id, team_id }] }` with support for existing team assignment (int), new team creation (string starting with `t`), and unassignment (null) - `tests/test_admin_teams.py` -- 17 new tests covering GET response shape, player/team data, empty state, coach presence/absence, POST assign/unassign/create/dedup/mixed operations, error handling, and auth enforcement - `src/basketball_api/routes/register.py` -- Fixed pre-existing E501 lint violations (line length > 100) - `src/basketball_api/services/email.py` -- Fixed pre-existing E501 lint violations in HTML template strings ## Test Plan - 17 new tests all pass: `pytest tests/test_admin_teams.py -v` - Full suite: 435/435 pass (1 pre-existing failure in `test_players.py::test_patch_ignores_read_only_fields` unrelated to this PR) - `ruff check .` and `ruff format .` clean ## Review Checklist - [x] Endpoints match frontend contract in `westside-app/src/routes/admin/teams/+page.svelte` - [x] GET response shape: `{ teams: [{ id, name, coaches }], players: [{ id, name, division, position, team_id, team, height, photo_url, parent_name }] }` - [x] POST handles int team_id, string temp ID (t-prefix), and null (unassign) - [x] Temp team deduplication (same temp ID creates only one team) - [x] Auth enforcement (401/403 without admin role) - [x] Pre-existing lint violations fixed (not introduced by this PR) ## Related - Closes #121

forgejo_admin added 1 commit 2026-03-19 20:13:00 +00:00

feat: add GET /admin/teams and POST /admin/teams/save for SPA draft board ... 26bc7e4794

Wire two endpoints consumed by the westside-app team management page:
- GET /admin/teams returns { teams, players } with auto-resolved tenant
- POST /admin/teams/save accepts bulk assignments, creates temp teams,
  and handles assign/unassign/create in a single transaction

Also fixes pre-existing E501 lint violations in register.py and email.py
that were blocking the commit hook.

Closes #121

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

forgejo_admin commented 2026-03-19 20:13:29 +00:00

Author

Owner

Copy link

Self-review complete. No issues found.

Verified:

• GET /admin/teams response shape matches westside-app frontend contract (teams with coaches array, players with team_id/team/division/position/height/photo_url/parent_name)
• POST /admin/teams/save handles all three cases: int team_id (assign), string t-prefix (create + assign), null (unassign)
• Temp team dedup works via dict lookup -- same temp ID creates only one Team row
• HTTPException on bad player/team ID triggers before db.commit(), so SQLAlchemy rolls back partial changes automatically
• 17 new tests cover happy paths, edge cases, error handling, and auth enforcement
• Full suite 435/435 passing (1 pre-existing failure unrelated to this PR)
• ruff check + format clean

Self-review complete. No issues found. **Verified:** - GET /admin/teams response shape matches westside-app frontend contract (teams with coaches array, players with team_id/team/division/position/height/photo_url/parent_name) - POST /admin/teams/save handles all three cases: int team_id (assign), string t-prefix (create + assign), null (unassign) - Temp team dedup works via dict lookup -- same temp ID creates only one Team row - HTTPException on bad player/team ID triggers before db.commit(), so SQLAlchemy rolls back partial changes automatically - 17 new tests cover happy paths, edge cases, error handling, and auth enforcement - Full suite 435/435 passing (1 pre-existing failure unrelated to this PR) - ruff check + format clean

forgejo_admin commented 2026-03-19 20:15:28 +00:00

Author

Owner

Copy link

PR #122 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic / pytest

Endpoints added:

• GET /admin/teams -- returns all teams and players for the SPA draft-board UI
• POST /admin/teams/save -- bulk save player-to-team assignments with temp team creation

Pydantic models: Well-structured. TeamCoachItem, TeamItem, TeamPlayerItem, TeamsResponse, PlayerAssignment, TeamsSaveRequest, TeamsSaveResponse -- all use PEP 484 type hints and follow existing codebase patterns.

Auth: Both endpoints use require_admin dependency (Keycloak JWT admin role check), consistent with all other admin endpoints. No DRY violation -- reuses the module-level require_admin = require_role("admin") pattern.

SQLAlchemy patterns:

• Eager loading via joinedload on Team.coach, Player.parent, Player.team -- correct, avoids N+1.
• db.flush() for temp team ID resolution before commit -- correct pattern.
• Single db.commit() at the end of POST -- good transactional behavior.

Tenant resolution: Uses db.query(Tenant).first() rather than the DEFAULT_TENANT_SLUG filter used by the email endpoints (lines 391, 431, 507). This is a deliberate "auto-resolve" for the single-tenant case, documented in the docstring. Acceptable given the single-tenant constraint, though it is a minor inconsistency.

Input validation:

• PlayerAssignment.team_id typed as int | str | None -- Pydantic validates the union at the schema level.
• String team_id branching: startswith("t") for temp IDs, else int() cast with proper ValueError/TypeError handling and 422 response.
• Player existence validated per assignment (404 on missing).
• Team existence validated for integer IDs (404 on missing).

Potential concern -- cross-tenant authorization in POST: The GET /admin/teams endpoint correctly filters players by Player.tenant_id == tenant.id. However, the POST /admin/teams/save endpoint queries Player by ID alone (Player.id == assignment.player_id) without filtering by tenant_id. In a single-tenant deployment this is fine. In a future multi-tenant scenario, an admin could theoretically reassign a player from another tenant. This is a nit given the current single-tenant reality, but worth noting.

Lint fixes: The changes to register.py and email.py are legitimate E501 lint fixes (line length). The email.py change also refactors an inline conditional into a variable (creds_html) for readability. All non-functional, formatting-only.

BLOCKERS

None.

• Test coverage: 17 new tests covering GET response shape, player/team data, empty state, coach presence/absence, POST assign/unassign/create/dedup/mixed operations, error handling (404 for missing player/team), auth enforcement (401 without auth), edge cases (no-op assignment, empty list, numeric string team_id). This is thorough.
• No unvalidated user input -- all paths validated through Pydantic + explicit checks.
• No secrets or credentials in code.
• No DRY violations in auth paths.

NITS

1. Tenant lookup inconsistency: GET /admin/teams and POST /admin/teams/save use Tenant.first() while other admin endpoints use Tenant.slug == DEFAULT_TENANT_SLUG. Both work for single-tenant but the pattern divergence could confuse future contributors. Consider standardizing on one approach.

2. Cross-tenant guard on POST: POST /admin/teams/save does not filter Player.id by tenant_id. Not a risk today (single tenant) but adding .filter(Player.tenant_id == tenant.id) would be defensive.

3. Temp team naming: New teams created from temp IDs get the raw temp ID as their name (e.g., t1710878400). The comment says "name can be renamed later" which is fine, but this means the team name is a timestamp string until manually renamed. Acceptable for draft-board workflow.

4. PR body Related section: References Closes #121 but does not reference the plan slug plan-wkq. Per SOP, the Related section should include the plan slug.

SOP COMPLIANCE

• Branch named after issue: 121-admin-teams-spa-endpoints (references #121)
• PR body has Summary, Changes, Test Plan, Related sections
• Related section references plan slug -- missing plan-wkq
• Tests exist: 17 new tests in tests/test_admin_teams.py
• No secrets committed
• No unnecessary file changes -- lint fixes in register.py and email.py are pre-existing violations, documented as such
• Commit messages are descriptive

PROCESS OBSERVATIONS

• Deployment frequency: Clean feature addition, no schema migration needed (endpoints query existing tables). Low deployment risk.
• Change failure risk: Low. New endpoints only, no modifications to existing endpoint behavior. Lint-only changes to existing files.
• Test quality: 17 tests is strong coverage for 2 endpoints. Happy path, edge cases, error cases, and auth enforcement all covered. The test fixture pattern (admin_client, sample_data) follows existing codebase conventions from conftest.py.
• Documentation gap: The plan slug reference in the PR body is missing. Minor SOP deviation.

VERDICT: APPROVED

## PR #122 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic / pytest **Endpoints added**: - `GET /admin/teams` -- returns all teams and players for the SPA draft-board UI - `POST /admin/teams/save` -- bulk save player-to-team assignments with temp team creation **Pydantic models**: Well-structured. `TeamCoachItem`, `TeamItem`, `TeamPlayerItem`, `TeamsResponse`, `PlayerAssignment`, `TeamsSaveRequest`, `TeamsSaveResponse` -- all use PEP 484 type hints and follow existing codebase patterns. **Auth**: Both endpoints use `require_admin` dependency (Keycloak JWT admin role check), consistent with all other admin endpoints. No DRY violation -- reuses the module-level `require_admin = require_role("admin")` pattern. **SQLAlchemy patterns**: - Eager loading via `joinedload` on `Team.coach`, `Player.parent`, `Player.team` -- correct, avoids N+1. - `db.flush()` for temp team ID resolution before commit -- correct pattern. - Single `db.commit()` at the end of POST -- good transactional behavior. **Tenant resolution**: Uses `db.query(Tenant).first()` rather than the `DEFAULT_TENANT_SLUG` filter used by the email endpoints (lines 391, 431, 507). This is a deliberate "auto-resolve" for the single-tenant case, documented in the docstring. Acceptable given the single-tenant constraint, though it is a minor inconsistency. **Input validation**: - `PlayerAssignment.team_id` typed as `int | str | None` -- Pydantic validates the union at the schema level. - String team_id branching: `startswith("t")` for temp IDs, else `int()` cast with proper `ValueError`/`TypeError` handling and 422 response. - Player existence validated per assignment (404 on missing). - Team existence validated for integer IDs (404 on missing). **Potential concern -- cross-tenant authorization in POST**: The `GET /admin/teams` endpoint correctly filters players by `Player.tenant_id == tenant.id`. However, the `POST /admin/teams/save` endpoint queries `Player` by ID alone (`Player.id == assignment.player_id`) without filtering by `tenant_id`. In a single-tenant deployment this is fine. In a future multi-tenant scenario, an admin could theoretically reassign a player from another tenant. This is a nit given the current single-tenant reality, but worth noting. **Lint fixes**: The changes to `register.py` and `email.py` are legitimate E501 lint fixes (line length). The `email.py` change also refactors an inline conditional into a variable (`creds_html`) for readability. All non-functional, formatting-only. ### BLOCKERS None. - Test coverage: 17 new tests covering GET response shape, player/team data, empty state, coach presence/absence, POST assign/unassign/create/dedup/mixed operations, error handling (404 for missing player/team), auth enforcement (401 without auth), edge cases (no-op assignment, empty list, numeric string team_id). This is thorough. - No unvalidated user input -- all paths validated through Pydantic + explicit checks. - No secrets or credentials in code. - No DRY violations in auth paths. ### NITS 1. **Tenant lookup inconsistency**: `GET /admin/teams` and `POST /admin/teams/save` use `Tenant.first()` while other admin endpoints use `Tenant.slug == DEFAULT_TENANT_SLUG`. Both work for single-tenant but the pattern divergence could confuse future contributors. Consider standardizing on one approach. 2. **Cross-tenant guard on POST**: `POST /admin/teams/save` does not filter `Player.id` by `tenant_id`. Not a risk today (single tenant) but adding `.filter(Player.tenant_id == tenant.id)` would be defensive. 3. **Temp team naming**: New teams created from temp IDs get the raw temp ID as their name (e.g., `t1710878400`). The comment says "name can be renamed later" which is fine, but this means the team name is a timestamp string until manually renamed. Acceptable for draft-board workflow. 4. **PR body Related section**: References `Closes #121` but does not reference the plan slug `plan-wkq`. Per SOP, the Related section should include the plan slug. ### SOP COMPLIANCE - [x] Branch named after issue: `121-admin-teams-spa-endpoints` (references #121) - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references plan slug -- missing `plan-wkq` - [x] Tests exist: 17 new tests in `tests/test_admin_teams.py` - [x] No secrets committed - [x] No unnecessary file changes -- lint fixes in `register.py` and `email.py` are pre-existing violations, documented as such - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean feature addition, no schema migration needed (endpoints query existing tables). Low deployment risk. - **Change failure risk**: Low. New endpoints only, no modifications to existing endpoint behavior. Lint-only changes to existing files. - **Test quality**: 17 tests is strong coverage for 2 endpoints. Happy path, edge cases, error cases, and auth enforcement all covered. The test fixture pattern (`admin_client`, `sample_data`) follows existing codebase conventions from `conftest.py`. - **Documentation gap**: The plan slug reference in the PR body is missing. Minor SOP deviation. ### VERDICT: APPROVED

forgejo_admin referenced this pull request 2026-03-21 14:01:21 +00:00

Enterprise login: Keycloak SMTP + self-service password reset #129

forgejo_admin referenced this pull request 2026-03-21 14:50:48 +00:00

Spike: Gmail OAuth token persistence in Postgres #130

forgejo_admin referenced this pull request 2026-03-24 21:14:34 +00:00

feat: Gmail OAuth token persistence in Postgres #163

forgejo_admin referenced this pull request 2026-03-25 00:19:44 +00:00

feat: Gmail OAuth token persistence in Postgres #163

forgejo_admin referenced this pull request 2026-03-25 00:37:36 +00:00

feat: Gmail OAuth token persistence in Postgres #163

forgejo_admin referenced this pull request 2026-03-25 00:45:36 +00:00

feat: enterprise login -- Keycloak self-service password reset #167

forgejo_admin referenced this pull request 2026-03-25 00:49:27 +00:00

feat: enterprise login -- Keycloak self-service password reset #167

Some checks failed

Hide all checks

ci/woodpecker/pr/woodpecker Pipeline failed

Details

This branch is already included in the target branch. There is nothing to merge.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 121-admin-teams-spa-endpoints:121-admin-teams-spa-endpoints

git switch 121-admin-teams-spa-endpoints

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff 121-admin-teams-spa-endpoints

git switch 121-admin-teams-spa-endpoints

git rebase main

git switch main

git merge --ff-only 121-admin-teams-spa-endpoints

git switch 121-admin-teams-spa-endpoints

git rebase main

git switch main

git merge --no-ff 121-admin-teams-spa-endpoints

git switch main

git merge --squash 121-admin-teams-spa-endpoints

git switch main

git merge --ff-only 121-admin-teams-spa-endpoints

git switch main

git merge 121-admin-teams-spa-endpoints

git push origin main

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!122

No description provided.