feat: accept tenant slug in team endpoints

forgejo_admin commented

2026-03-15 02:29:36 +00:00

Author

Owner

PR #88 Backend Review

DOMAIN REVIEW

PEP compliance: Clean. Type hints are correct throughout: int | str union types (PEP 604), proper docstrings with PEP 257 formatting on both _resolve_tenant_id and _get_tenant_or_404. The """...""" docstrings use reStructuredText-style backticks consistently.

Security (OWASP): No injection risk. The _resolve_tenant_id helper uses Python's int() cast, not string interpolation into SQL. SQLAlchemy parameterized queries are used for both the ID and slug lookup paths. Tenant isolation is preserved -- all endpoints resolve the Tenant object first, then pass tenant.id (the integer PK) downstream to _get_team_or_404. No mass assignment risk since TeamCreate is the gatekeeper and response models always emit tenant_id: int.

Database patterns: Correct. Tenant.slug is already unique=True, index=True (confirmed in models.py line 78), so the slug lookup path in _get_tenant_or_404 hits an index. No N+1 risk introduced. The joinedload patterns on Team.coach and Team.players are preserved unchanged. No new queries added -- just the tenant resolution path changed from one query shape to a branching two-query shape (only one executes per call).

Migrations: Not needed. No schema changes -- this is purely application-layer logic. The Tenant.slug column already exists. Confirmed no Alembic version files in the diff.

Tests: Strong coverage. Six new tests in TestSlugBasedTenantLookup:

test_list_teams_by_slug -- list endpoint with slug
test_create_team_with_slug -- POST body with slug, verifies response returns integer tenant_id
test_overview_by_slug -- overview endpoint with slug
test_get_team_by_slug -- detail endpoint with slug
test_invalid_slug_returns_404 -- negative case
test_integer_tenant_id_still_works -- backward compatibility

Missing coverage (non-blocking): no slug-based tests for PATCH, DELETE, assign-players, or unassign-player endpoints. The code changes are mechanical (all follow the same pattern), so the risk is low, but completeness would be better.

API design: Good approach. Query params changed from int to str which is transparent to HTTP callers (query params are strings on the wire anyway). Response models correctly still emit tenant_id: int -- the canonical form. Backward compatibility is preserved since numeric strings parse back to integers.

BLOCKERS

None.

NITS

Numeric-slug ambiguity: If a tenant ever has an all-numeric slug (e.g., slug "123"), _resolve_tenant_id("123") would resolve it as integer ID 123, not as slug "123". This is an unlikely edge case given the current slugs (westside-kings-queens), but worth documenting in the docstring or adding a guard (e.g., try ID first, fall back to slug if no tenant found by ID). Non-blocking for current usage.
Duplicate _get_tenant_or_404 in subscriptions.py: The subscriptions routes (/home/ldraney/basketball-api/src/basketball_api/routes/subscriptions.py, line 95) have their own _get_tenant_or_404 that still only accepts int. If the frontend will also call subscription endpoints with slugs, a follow-up issue should be filed to DRY this into a shared helper and apply the same slug resolution. Non-blocking for this PR's scope (teams only per issue #87).
Test coverage gap for mutating slug endpoints: The PATCH (update_team), DELETE (delete_team), POST assign-players, and DELETE unassign-player endpoints all received the int -> str tenant_id change but lack dedicated slug-path tests. One test per write endpoint would close this gap. Non-blocking since the pattern is identical.

SOP COMPLIANCE

Branch named after issue: 87-feat-accept-tenant-slug-in-team-endpoint references issue #87
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Closes #87 present in PR body
Related section does not reference plan slug (plan-2026-03-08-tryout-prep) -- minor gap
No secrets committed -- only .py source files changed
No unnecessary file changes -- exactly 2 files, both in scope
Commit messages are descriptive (PR title: feat: accept tenant slug in team endpoints)

PROCESS OBSERVATIONS

Convention candidate: The _resolve_tenant_id + _get_tenant_or_404 pattern for slug-or-ID resolution should be extracted to a shared utility (e.g., basketball_api/utils/tenant.py) before other route modules (subscriptions, admin) need the same treatment. Currently duplicated between teams.py and subscriptions.py (subscriptions has the old int-only version).
Test gap: The 6 new tests cover read endpoints well but skip write-path slug tests. The mechanical uniformity of the changes makes this low-risk, but a future test pass should close it.
CI validation: PR body claims 248/248 tests pass locally. CI pipeline should confirm this.

VERDICT: APPROVED

Clean, well-scoped change. The slug resolution logic is correct, backward-compatible, and properly tested for the read paths. The Tenant.slug column is indexed, so no performance concern. Response models correctly return integer tenant_id. No blockers. The nits (numeric-slug edge case, DRY opportunity, write-path test coverage) are all non-blocking and appropriate for follow-up work.

## PR #88 Backend Review ### DOMAIN REVIEW **PEP compliance:** Clean. Type hints are correct throughout: `int | str` union types (PEP 604), proper docstrings with PEP 257 formatting on both `_resolve_tenant_id` and `_get_tenant_or_404`. The `"""..."""` docstrings use reStructuredText-style backticks consistently. **Security (OWASP):** No injection risk. The `_resolve_tenant_id` helper uses Python's `int()` cast, not string interpolation into SQL. SQLAlchemy parameterized queries are used for both the ID and slug lookup paths. Tenant isolation is preserved -- all endpoints resolve the `Tenant` object first, then pass `tenant.id` (the integer PK) downstream to `_get_team_or_404`. No mass assignment risk since `TeamCreate` is the gatekeeper and response models always emit `tenant_id: int`. **Database patterns:** Correct. `Tenant.slug` is already `unique=True, index=True` (confirmed in `models.py` line 78), so the slug lookup path in `_get_tenant_or_404` hits an index. No N+1 risk introduced. The `joinedload` patterns on `Team.coach` and `Team.players` are preserved unchanged. No new queries added -- just the tenant resolution path changed from one query shape to a branching two-query shape (only one executes per call). **Migrations:** Not needed. No schema changes -- this is purely application-layer logic. The `Tenant.slug` column already exists. Confirmed no Alembic version files in the diff. **Tests:** Strong coverage. Six new tests in `TestSlugBasedTenantLookup`: - `test_list_teams_by_slug` -- list endpoint with slug - `test_create_team_with_slug` -- POST body with slug, verifies response returns integer `tenant_id` - `test_overview_by_slug` -- overview endpoint with slug - `test_get_team_by_slug` -- detail endpoint with slug - `test_invalid_slug_returns_404` -- negative case - `test_integer_tenant_id_still_works` -- backward compatibility Missing coverage (non-blocking): no slug-based tests for PATCH, DELETE, assign-players, or unassign-player endpoints. The code changes are mechanical (all follow the same pattern), so the risk is low, but completeness would be better. **API design:** Good approach. Query params changed from `int` to `str` which is transparent to HTTP callers (query params are strings on the wire anyway). Response models correctly still emit `tenant_id: int` -- the canonical form. Backward compatibility is preserved since numeric strings parse back to integers. ### BLOCKERS None. ### NITS 1. **Numeric-slug ambiguity:** If a tenant ever has an all-numeric slug (e.g., slug `"123"`), `_resolve_tenant_id("123")` would resolve it as integer ID 123, not as slug `"123"`. This is an unlikely edge case given the current slugs (`westside-kings-queens`), but worth documenting in the docstring or adding a guard (e.g., try ID first, fall back to slug if no tenant found by ID). Non-blocking for current usage. 2. **Duplicate `_get_tenant_or_404` in subscriptions.py:** The subscriptions routes (`/home/ldraney/basketball-api/src/basketball_api/routes/subscriptions.py`, line 95) have their own `_get_tenant_or_404` that still only accepts `int`. If the frontend will also call subscription endpoints with slugs, a follow-up issue should be filed to DRY this into a shared helper and apply the same slug resolution. Non-blocking for this PR's scope (teams only per issue #87). 3. **Test coverage gap for mutating slug endpoints:** The PATCH (`update_team`), DELETE (`delete_team`), POST assign-players, and DELETE unassign-player endpoints all received the `int -> str` tenant_id change but lack dedicated slug-path tests. One test per write endpoint would close this gap. Non-blocking since the pattern is identical. ### SOP COMPLIANCE - [x] Branch named after issue: `87-feat-accept-tenant-slug-in-team-endpoint` references issue #87 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] `Closes #87` present in PR body - [ ] Related section does not reference plan slug (`plan-2026-03-08-tryout-prep`) -- minor gap - [x] No secrets committed -- only `.py` source files changed - [x] No unnecessary file changes -- exactly 2 files, both in scope - [x] Commit messages are descriptive (PR title: `feat: accept tenant slug in team endpoints`) ### PROCESS OBSERVATIONS - **Convention candidate:** The `_resolve_tenant_id` + `_get_tenant_or_404` pattern for slug-or-ID resolution should be extracted to a shared utility (e.g., `basketball_api/utils/tenant.py`) before other route modules (subscriptions, admin) need the same treatment. Currently duplicated between teams.py and subscriptions.py (subscriptions has the old int-only version). - **Test gap:** The 6 new tests cover read endpoints well but skip write-path slug tests. The mechanical uniformity of the changes makes this low-risk, but a future test pass should close it. - **CI validation:** PR body claims 248/248 tests pass locally. CI pipeline should confirm this. ### VERDICT: APPROVED Clean, well-scoped change. The slug resolution logic is correct, backward-compatible, and properly tested for the read paths. The `Tenant.slug` column is indexed, so no performance concern. Response models correctly return integer `tenant_id`. No blockers. The nits (numeric-slug edge case, DRY opportunity, write-path test coverage) are all non-blocking and appropriate for follow-up work.

2026-03-15 02:27:02 +00:00

Summary

Team endpoints now accept tenant slugs (e.g. westside-kings-queens) in addition to integer IDs for the tenant_id parameter. This resolves the contract mismatch where the westside-app frontend sends slugs but the API expected integers, causing 422 errors.

Changes

src/basketball_api/routes/teams.py: Added _resolve_tenant_id helper and expanded _get_tenant_or_404 to resolve by numeric ID or slug. Changed all tenant_id query params from int to str. Changed TeamCreate.tenant_id from int to int | str. Response models still return tenant_id: int.
tests/test_teams.py: Added TestSlugBasedTenantLookup class with 6 new tests covering list by slug, create with slug, overview by slug, get team by slug, invalid slug 404, and integer backward compat.

Test Plan

Tests pass locally (248/248, including 6 new slug-based tests)
Manual verification: existing integer-based callers unaffected (backward compatible)
No regressions in team CRUD, player assignment, coach view, tenant isolation

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #87
Forgejo issue: #87

## Summary Team endpoints now accept tenant slugs (e.g. `westside-kings-queens`) in addition to integer IDs for the `tenant_id` parameter. This resolves the contract mismatch where the westside-app frontend sends slugs but the API expected integers, causing 422 errors. ## Changes - `src/basketball_api/routes/teams.py`: Added `_resolve_tenant_id` helper and expanded `_get_tenant_or_404` to resolve by numeric ID or slug. Changed all `tenant_id` query params from `int` to `str`. Changed `TeamCreate.tenant_id` from `int` to `int | str`. Response models still return `tenant_id: int`. - `tests/test_teams.py`: Added `TestSlugBasedTenantLookup` class with 6 new tests covering list by slug, create with slug, overview by slug, get team by slug, invalid slug 404, and integer backward compat. ## Test Plan - [x] Tests pass locally (248/248, including 6 new slug-based tests) - [x] Manual verification: existing integer-based callers unaffected (backward compatible) - [x] No regressions in team CRUD, player assignment, coach view, tenant isolation ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #87 - Forgejo issue: #87

forgejo_admin added 1 commit

feat: accept tenant slug in team endpoints for frontend compatibility

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

aa88723535

Change tenant_id query params from int to str across all team routes,
resolving to Tenant by numeric ID or slug string via an enhanced
_get_tenant_or_404 helper. Backwards compatible with integer callers.
TeamCreate body also accepts slug. Response models still return int.
Adds 6 slug-based tests covering list, create, overview, detail,
invalid slug 404, and integer backward compat.

Closes #87

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

2026-03-15 02:27:29 +00:00

Self-Review: LGTM

Files reviewed: src/basketball_api/routes/teams.py, tests/test_teams.py

Correctness

All 8 endpoint functions resolve tenant_id through the enhanced _get_tenant_or_404 helper and use tenant.id (int) for all downstream DB queries. No stale references to the raw string tenant_id remain in query filters.
_resolve_tenant_id correctly tries int() first so numeric query strings still resolve by ID (backward compat).
TeamCreate.tenant_id: int | str accepts both types in the request body. Response models remain int.

Test Coverage

6 new tests in TestSlugBasedTenantLookup: list, create, overview, detail, invalid slug 404, integer backward compat.
All 248 tests pass (242 existing + 6 new). Zero regressions.

No Issues Found

No secrets, no unnecessary changes, no missing edge cases.
Ruff format and lint both clean.

## Self-Review: LGTM **Files reviewed:** `src/basketball_api/routes/teams.py`, `tests/test_teams.py` ### Correctness - All 8 endpoint functions resolve `tenant_id` through the enhanced `_get_tenant_or_404` helper and use `tenant.id` (int) for all downstream DB queries. No stale references to the raw string `tenant_id` remain in query filters. - `_resolve_tenant_id` correctly tries `int()` first so numeric query strings still resolve by ID (backward compat). - `TeamCreate.tenant_id: int | str` accepts both types in the request body. Response models remain `int`. ### Test Coverage - 6 new tests in `TestSlugBasedTenantLookup`: list, create, overview, detail, invalid slug 404, integer backward compat. - All 248 tests pass (242 existing + 6 new). Zero regressions. ### No Issues Found - No secrets, no unnecessary changes, no missing edge cases. - Ruff format and lint both clean.

forgejo_admin merged commit 012c3fb134 into main

2026-03-15 02:41:16 +00:00

forgejo_admin deleted branch 87-feat-accept-tenant-slug-in-team-endpoint

forgejo_admin referenced this pull request from a commit

2026-03-15 02:41:17 +00:00

feat: accept tenant slug in team endpoints (#88)

Rows
Columns

feat: accept tenant slug in team endpoints #88