forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 88 Pull requests 9 Projects Releases Packages Wiki Activity Actions

fix: auto-resolve tenant_id on all team endpoints #378

Merged
forgejo_admin merged 1 commit from 372-auto-resolve-tenant-id into main 2026-04-07 17:59:41 +00:00
Conversation 2 Commits 1 Files changed 2 +154 -18
forgejo_admin commented 2026-04-07 17:34:09 +00:00
Owner
Copy link

Summary

Frontend never sends tenant_id, causing 422 on GET /teams/{id} and all other team endpoints. This makes tenant_id optional on all 9 endpoints and auto-resolves the default tenant via DEFAULT_TENANT_SLUG when omitted -- the same pattern already used in schedule.py.

Changes

  • src/basketball_api/routes/teams.py -- imported DEFAULT_TENANT_SLUG, added _get_default_tenant and _resolve_tenant helpers, made tenant_id optional (default None) on all 8 query-param endpoints, made TeamCreate.tenant_id optional for the body-param endpoint
  • tests/test_teams.py -- added TestAutoResolveTenant class with 11 test cases covering every endpoint without tenant_id, plus backwards-compatibility test

Test Plan

  • All 51 tests pass (pytest tests/test_teams.py -v): 40 existing + 11 new
  • Backwards compatible: callers that pass tenant_id explicitly still work (existing tests unchanged and passing)
  • ruff format + ruff check clean

Review Checklist

  • Code follows existing patterns (_get_tenant helper from schedule.py)
  • All existing tests still pass (no regressions)
  • New tests cover every endpoint without tenant_id
  • Backwards compatible: explicit tenant_id still works
  • ruff format + ruff check clean

Related Notes

  • None

Related

## Summary Frontend never sends `tenant_id`, causing 422 on `GET /teams/{id}` and all other team endpoints. This makes `tenant_id` optional on all 9 endpoints and auto-resolves the default tenant via `DEFAULT_TENANT_SLUG` when omitted -- the same pattern already used in `schedule.py`. ## Changes - `src/basketball_api/routes/teams.py` -- imported `DEFAULT_TENANT_SLUG`, added `_get_default_tenant` and `_resolve_tenant` helpers, made `tenant_id` optional (default `None`) on all 8 query-param endpoints, made `TeamCreate.tenant_id` optional for the body-param endpoint - `tests/test_teams.py` -- added `TestAutoResolveTenant` class with 11 test cases covering every endpoint without `tenant_id`, plus backwards-compatibility test ## Test Plan - All 51 tests pass (`pytest tests/test_teams.py -v`): 40 existing + 11 new - Backwards compatible: callers that pass `tenant_id` explicitly still work (existing tests unchanged and passing) - ruff format + ruff check clean ## Review Checklist - [x] Code follows existing patterns (`_get_tenant` helper from `schedule.py`) - [x] All existing tests still pass (no regressions) - [x] New tests cover every endpoint without `tenant_id` - [x] Backwards compatible: explicit `tenant_id` still works - [x] ruff format + ruff check clean ## Related Notes - None ## Related - Closes forgejo_admin/basketball-api#372 - Board item: #877 on board-westside-basketball
feat: include is_public in admin players list response
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
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>
fix: auto-resolve tenant_id on all team endpoints when omitted
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
36e5a16f7c 
Frontend never sends tenant_id, causing 422 on GET /teams/{id} and all
other team endpoints. Make tenant_id optional (default None) on all 9
endpoints and fall back to DEFAULT_TENANT_SLUG when omitted, matching
the pattern already used in schedule.py.

Closes forgejo_admin/basketball-api#372

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-07 17:36:39 +00:00
Author
Owner
Copy link

PR #378 Review — QA Agent

Domain Review

Tech stack: Python 3.12 / FastAPI / SQLAlchemy 2.0 / Pydantic v2

Pattern match with schedule.py: Confirmed. schedule.py uses _get_tenant(db) which does the same DEFAULT_TENANT_SLUG lookup with a 500 on missing tenant. The teams.py implementation goes further by wrapping this in _resolve_tenant() which supports both auto-resolve fallback AND explicit id/slug — strictly better than schedule.py's approach.

Correctness: The _resolve_tenant helper correctly short-circuits to _get_default_tenant when tenant_id is None, and delegates to _get_tenant_or_404 for explicit values. All 9 endpoints updated consistently. TeamCreate.tenant_id change from int | str to int | str | None = None is backwards-compatible.

Multi-tenancy security: Tenant isolation preserved. Every endpoint resolves to a specific Tenant object and filters by tenant.id. Fallback only activates when tenant_id is omitted, not when invalid — invalid values still 404 via _get_tenant_or_404.

Blockers

None.

Nits (non-blocking)

  1. DRY: _get_default_tenant in teams.py duplicates _get_tenant in schedule.py nearly verbatim. Consider extracting a shared resolve_default_tenant(db) helper in a future cleanup.
  2. Diff scope: PR diff includes unrelated admin.py changes (is_public in AdminPlayerItem) from prior commit 3c167ea. Minor scope creep — not a code quality issue.
  3. Test gap (minor): No negative test for the 500 path when DEFAULT_TENANT_SLUG isn't seeded. Low priority (server misconfiguration scenario).

SOP Compliance

  • ✅ Branch named after issue: 372-auto-resolve-tenant-id
  • ✅ No secrets committed
  • ✅ PR body has Summary, Changes, Test Plan, Related sections
  • ✅ Tests exist: 11 new tests covering all 9 endpoints + backwards-compat
  • ✅ Closes reference: Closes forgejo_admin/basketball-api#372

Process

  • Low change failure risk: additive change (optional param with fallback), fully backwards-compatible
  • Test coverage thorough: every endpoint has a dedicated auto-resolve test
  • Pattern is proven — schedule.py already runs this approach

VERDICT: APPROVE

## PR #378 Review — QA Agent ### Domain Review **Tech stack:** Python 3.12 / FastAPI / SQLAlchemy 2.0 / Pydantic v2 **Pattern match with schedule.py:** Confirmed. `schedule.py` uses `_get_tenant(db)` which does the same `DEFAULT_TENANT_SLUG` lookup with a 500 on missing tenant. The `teams.py` implementation goes further by wrapping this in `_resolve_tenant()` which supports both auto-resolve fallback AND explicit id/slug — strictly better than `schedule.py`'s approach. **Correctness:** The `_resolve_tenant` helper correctly short-circuits to `_get_default_tenant` when `tenant_id is None`, and delegates to `_get_tenant_or_404` for explicit values. All 9 endpoints updated consistently. `TeamCreate.tenant_id` change from `int | str` to `int | str | None = None` is backwards-compatible. **Multi-tenancy security:** Tenant isolation preserved. Every endpoint resolves to a specific `Tenant` object and filters by `tenant.id`. Fallback only activates when `tenant_id` is omitted, not when invalid — invalid values still 404 via `_get_tenant_or_404`. ### Blockers None. ### Nits (non-blocking) 1. **DRY:** `_get_default_tenant` in `teams.py` duplicates `_get_tenant` in `schedule.py` nearly verbatim. Consider extracting a shared `resolve_default_tenant(db)` helper in a future cleanup. 2. **Diff scope:** PR diff includes unrelated `admin.py` changes (`is_public` in `AdminPlayerItem`) from prior commit `3c167ea`. Minor scope creep — not a code quality issue. 3. **Test gap (minor):** No negative test for the 500 path when `DEFAULT_TENANT_SLUG` isn't seeded. Low priority (server misconfiguration scenario). ### SOP Compliance - ✅ Branch named after issue: `372-auto-resolve-tenant-id` - ✅ No secrets committed - ✅ PR body has Summary, Changes, Test Plan, Related sections - ✅ Tests exist: 11 new tests covering all 9 endpoints + backwards-compat - ✅ Closes reference: `Closes forgejo_admin/basketball-api#372` ### Process - Low change failure risk: additive change (optional param with fallback), fully backwards-compatible - Test coverage thorough: every endpoint has a dedicated auto-resolve test - Pattern is proven — `schedule.py` already runs this approach **VERDICT: APPROVE**
forgejo_admin commented 2026-04-07 17:45:48 +00:00
Author
Owner
Copy link

PR #378 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

Pattern consistency with schedule.py:

The _get_default_tenant helper in teams.py correctly mirrors the _get_tenant helper in schedule.py (line 128). Both query Tenant by DEFAULT_TENANT_SLUG and raise HTTP 500 if missing. The error messages differ slightly ("Default tenant not configured" vs "Tenant not configured") -- trivial, but noted.

The added _resolve_tenant wrapper is a clean addition that schedule.py does not need (schedule.py never accepts explicit tenant_id). This two-layer approach (_resolve_tenant delegates to either _get_default_tenant or _get_tenant_or_404) is well-structured and correctly preserves backwards compatibility.

Backwards compatibility: Confirmed. All 9 endpoints change tenant_id from required to Optional[...] = None. When callers pass tenant_id explicitly, _resolve_tenant delegates to _get_tenant_or_404 which handles both int and slug -- no behavior change.

Type correctness: _resolve_tenant signature is (db: Session, tenant_id: str | int | None) -> Tenant. The TeamCreate.tenant_id field is int | str | None = None. Query param endpoints declare tenant_id: str | None = None. All consistent with how FastAPI coerces query strings.

DRY observation (not a blocker): The default-tenant-by-slug pattern is now duplicated in 5 files: schedule.py, teams.py, admin.py (4 inline occurrences), register.py (2 inline), and password_reset.py (1 inline with its own _DEFAULT_TENANT_SLUG redeclaration). The teams.py PR correctly extracts it into a helper. The codebase would benefit from a shared utility, but that is out of scope for this bug fix.

BLOCKERS

None.

Test coverage: 10 new tests (not 11 as the PR body states) in TestAutoResolveTenant cover all 9 endpoints without tenant_id plus 1 backwards-compatibility test. Each endpoint has at least one test exercising the auto-resolve path. This is adequate coverage.

No unvalidated user input: tenant_id flows through _get_tenant_or_404 (which does a DB lookup by id or slug) or _get_default_tenant (hardcoded slug). No injection risk.

No secrets in code. No credentials committed.

NITS

  1. Scope creep -- is_public field in admin.py and test_admin_spa.py. Two of the four changed files (admin.py, test_admin_spa.py) add is_public to AdminPlayerItem and its test assertion. This is unrelated to issue #372 (tenant auto-resolve). These changes should be in a separate PR tied to their own ticket. Not a blocker because the change is correct (the model field exists on Player), but it violates single-purpose PR discipline.

  2. PR body claims 11 tests but there are 10. The TestAutoResolveTenant class contains exactly 10 test methods. Minor documentation inaccuracy.

  3. Error message inconsistency. _get_default_tenant says "Default tenant not configured" while schedule.py's _get_tenant says "Tenant not configured". Consider matching the wording for grep-ability, or better yet, extracting a shared helper (future ticket).

  4. _resolve_tenant_id is now partially redundant. The old _resolve_tenant_id helper (which parses strings to int) still exists alongside the new _resolve_tenant. They serve different purposes but the naming is confusing: _resolve_tenant_id vs _resolve_tenant. Consider renaming _resolve_tenant_id to _parse_tenant_id for clarity.

SOP COMPLIANCE

  • Branch named after issue: 372-auto-resolve-tenant-id
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related references plan slug -- no plan slug referenced (References board item #877 only)
  • No secrets committed
  • No unnecessary file changes -- admin.py and test_admin_spa.py contain is_public changes unrelated to #372

PROCESS OBSERVATIONS

  • Change failure risk: LOW. The change is additive (required param becomes optional with fallback). Existing callers are unaffected. New behavior (omitted tenant_id) has test coverage.
  • Scope creep: The is_public additions in admin.py should have their own issue and PR. Mixing concerns in a single PR makes rollback harder if either change causes problems.
  • DRY debt: The default-tenant lookup is now in 5 files. A future ticket to extract get_default_tenant() into a shared module would reduce divergence risk.

VERDICT: APPROVED

The core fix is correct, well-tested, and follows the established pattern from schedule.py. The is_public scope creep is a process nit, not a correctness issue. The 10 tests adequately cover the auto-resolve behavior on all 9 endpoints plus backwards compatibility.

## PR #378 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic **Pattern consistency with schedule.py:** The `_get_default_tenant` helper in teams.py correctly mirrors the `_get_tenant` helper in `schedule.py` (line 128). Both query `Tenant` by `DEFAULT_TENANT_SLUG` and raise HTTP 500 if missing. The error messages differ slightly ("Default tenant not configured" vs "Tenant not configured") -- trivial, but noted. The added `_resolve_tenant` wrapper is a clean addition that schedule.py does not need (schedule.py never accepts explicit tenant_id). This two-layer approach (`_resolve_tenant` delegates to either `_get_default_tenant` or `_get_tenant_or_404`) is well-structured and correctly preserves backwards compatibility. **Backwards compatibility:** Confirmed. All 9 endpoints change `tenant_id` from required to `Optional[...] = None`. When callers pass `tenant_id` explicitly, `_resolve_tenant` delegates to `_get_tenant_or_404` which handles both int and slug -- no behavior change. **Type correctness:** `_resolve_tenant` signature is `(db: Session, tenant_id: str | int | None) -> Tenant`. The `TeamCreate.tenant_id` field is `int | str | None = None`. Query param endpoints declare `tenant_id: str | None = None`. All consistent with how FastAPI coerces query strings. **DRY observation (not a blocker):** The default-tenant-by-slug pattern is now duplicated in 5 files: `schedule.py`, `teams.py`, `admin.py` (4 inline occurrences), `register.py` (2 inline), and `password_reset.py` (1 inline with its own `_DEFAULT_TENANT_SLUG` redeclaration). The teams.py PR correctly extracts it into a helper. The codebase would benefit from a shared utility, but that is out of scope for this bug fix. ### BLOCKERS None. **Test coverage:** 10 new tests (not 11 as the PR body states) in `TestAutoResolveTenant` cover all 9 endpoints without `tenant_id` plus 1 backwards-compatibility test. Each endpoint has at least one test exercising the auto-resolve path. This is adequate coverage. **No unvalidated user input:** `tenant_id` flows through `_get_tenant_or_404` (which does a DB lookup by id or slug) or `_get_default_tenant` (hardcoded slug). No injection risk. **No secrets in code.** No credentials committed. ### NITS 1. **Scope creep -- `is_public` field in admin.py and test_admin_spa.py.** Two of the four changed files (`admin.py`, `test_admin_spa.py`) add `is_public` to `AdminPlayerItem` and its test assertion. This is unrelated to issue #372 (tenant auto-resolve). These changes should be in a separate PR tied to their own ticket. Not a blocker because the change is correct (the model field exists on `Player`), but it violates single-purpose PR discipline. 2. **PR body claims 11 tests but there are 10.** The `TestAutoResolveTenant` class contains exactly 10 test methods. Minor documentation inaccuracy. 3. **Error message inconsistency.** `_get_default_tenant` says "Default tenant not configured" while `schedule.py`'s `_get_tenant` says "Tenant not configured". Consider matching the wording for grep-ability, or better yet, extracting a shared helper (future ticket). 4. **`_resolve_tenant_id` is now partially redundant.** The old `_resolve_tenant_id` helper (which parses strings to int) still exists alongside the new `_resolve_tenant`. They serve different purposes but the naming is confusing: `_resolve_tenant_id` vs `_resolve_tenant`. Consider renaming `_resolve_tenant_id` to `_parse_tenant_id` for clarity. ### SOP COMPLIANCE - [x] Branch named after issue: `372-auto-resolve-tenant-id` - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related references plan slug -- no plan slug referenced (References board item #877 only) - [x] No secrets committed - [ ] No unnecessary file changes -- `admin.py` and `test_admin_spa.py` contain `is_public` changes unrelated to #372 ### PROCESS OBSERVATIONS - **Change failure risk: LOW.** The change is additive (required param becomes optional with fallback). Existing callers are unaffected. New behavior (omitted tenant_id) has test coverage. - **Scope creep:** The `is_public` additions in admin.py should have their own issue and PR. Mixing concerns in a single PR makes rollback harder if either change causes problems. - **DRY debt:** The default-tenant lookup is now in 5 files. A future ticket to extract `get_default_tenant()` into a shared module would reduce divergence risk. ### VERDICT: APPROVED The core fix is correct, well-tested, and follows the established pattern from schedule.py. The `is_public` scope creep is a process nit, not a correctness issue. The 10 tests adequately cover the auto-resolve behavior on all 9 endpoints plus backwards compatibility.
forgejo_admin force-pushed 372-auto-resolve-tenant-id from 36e5a16f7c
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
to d2107e240c
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
2026-04-07 17:57:41 +00:00 Compare
forgejo_admin deleted branch 372-auto-resolve-tenant-id 2026-04-07 17:59:41 +00:00
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
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!378
No description provided.