feat: add public teams endpoint (GET /public/teams) #178

Merged

forgejo_admin merged 2 commits from 176-public-teams-endpoint into main 2026-03-27 03:32:36 +00:00

Conversation 2 Commits 2 Files changed 5 +601

forgejo_admin commented 2026-03-27 03:25:59 +00:00

Owner

Copy link

Summary

• Adds unauthenticated GET /public/teams endpoint for the westside-app public website teams page
• Returns team rosters with only allowlisted fields, filtered to players where is_public = true
• Sorted by age group descending (U18 first) then tier (Elite > Select > Local)

Changes

• New: src/basketball_api/routes/public.py -- Public router with dedicated PublicPlayerResponse, PublicTeamResponse, PublicTeamsResponse schemas. No auth dependencies. Hardcoded tenant_id=1.
• Modified: src/basketball_api/main.py -- Register public router at /public prefix.
• Modified: src/basketball_api/models.py -- Add is_public boolean column to Player model (default false).
• New: alembic/versions/022_add_is_public_to_players.py -- Migration to add is_public column to players table.
• New: tests/test_public.py -- 13 tests covering all acceptance criteria.

Test Plan

• 13 new tests pass, 568 total suite passes (zero regressions)
• TestPublicTeamsUnauthenticated -- 200 without Bearer token, empty when no teams
• TestPublicFieldAllowlist -- only 5 player fields + 4 team fields present, 18 sensitive fields verified absent
• TestPublicPlayerFiltering -- is_public=false excluded, all-private/empty teams excluded
• TestPublicTeamsSortOrder -- age group descending, tier within group, full combined sort
• TestPublicCoachName -- coach name via join, null when no coach
• ruff format and ruff check pass clean

Review Checklist

• Dedicated public schemas -- never reuses admin response models
• No Depends(require_role) or Depends(require_admin) on public endpoint
• Security allowlist enforced: 18 sensitive fields verified absent in tests
• Alembic migration is additive (column add with default, no data loss)
• No secrets or credentials in changeset

Related Notes

• Closes #176
• Sibling ticket: #177 (public coaches -- shares public.py with no conflicts)
• User story: WS-S26

## Summary - Adds unauthenticated `GET /public/teams` endpoint for the westside-app public website teams page - Returns team rosters with only allowlisted fields, filtered to players where `is_public = true` - Sorted by age group descending (U18 first) then tier (Elite > Select > Local) ## Changes - **New: `src/basketball_api/routes/public.py`** -- Public router with dedicated `PublicPlayerResponse`, `PublicTeamResponse`, `PublicTeamsResponse` schemas. No auth dependencies. Hardcoded `tenant_id=1`. - **Modified: `src/basketball_api/main.py`** -- Register public router at `/public` prefix. - **Modified: `src/basketball_api/models.py`** -- Add `is_public` boolean column to `Player` model (default `false`). - **New: `alembic/versions/022_add_is_public_to_players.py`** -- Migration to add `is_public` column to players table. - **New: `tests/test_public.py`** -- 13 tests covering all acceptance criteria. ## Test Plan - [x] 13 new tests pass, 568 total suite passes (zero regressions) - [x] `TestPublicTeamsUnauthenticated` -- 200 without Bearer token, empty when no teams - [x] `TestPublicFieldAllowlist` -- only 5 player fields + 4 team fields present, 18 sensitive fields verified absent - [x] `TestPublicPlayerFiltering` -- `is_public=false` excluded, all-private/empty teams excluded - [x] `TestPublicTeamsSortOrder` -- age group descending, tier within group, full combined sort - [x] `TestPublicCoachName` -- coach name via join, null when no coach - [x] `ruff format` and `ruff check` pass clean ## Review Checklist - [x] Dedicated public schemas -- never reuses admin response models - [x] No `Depends(require_role)` or `Depends(require_admin)` on public endpoint - [x] Security allowlist enforced: 18 sensitive fields verified absent in tests - [x] Alembic migration is additive (column add with default, no data loss) - [x] No secrets or credentials in changeset ## Related Notes - Closes #176 - Sibling ticket: #177 (public coaches -- shares `public.py` with no conflicts) - User story: WS-S26

forgejo_admin added 1 commit 2026-03-27 03:25:59 +00:00

feat: add public teams endpoint (GET /public/teams) (#176) ... 9fe882a8ee

Unauthenticated endpoint for the public website teams page. Returns
teams with nested player rosters using dedicated allowlisted schemas
(PublicPlayerResponse, PublicTeamResponse) to prevent sensitive data
leakage. Adds is_public boolean column to players table so coaches
control public visibility. Teams sorted by age group descending then
tier (Elite > Select > Local), excluding teams with zero public players.

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

forgejo_admin commented 2026-03-27 03:26:36 +00:00

Author

Owner

Copy link

Self-Review

Result: PASS -- no issues found.

Security

• Dedicated PublicPlayerResponse and PublicTeamResponse schemas with exactly the allowlisted fields (5 player, 4 team). No admin schema reuse.
• 18 sensitive fields explicitly verified absent in test_sensitive_fields_not_exposed.
• No auth dependencies on the endpoint -- only Depends(get_db).

Data Integrity

• is_public column defaults to false via server_default -- existing players remain private until explicitly toggled.
• Migration 022 is additive (column add with default). No data loss risk. Clean downgrade.
• Model and migration are consistent (Boolean, server_default=text("false")).

Query / Performance

• joinedload(Team.coach) and joinedload(Team.players) avoids N+1 queries.
• is_public filtering happens in Python after eager load. Acceptable at current scale (single tenant). If player count grows significantly, a subquery filter could be added later.

Sort Logic

• Age group sort uses the AgeGroup enum .value mapped to priority integers (U18=0 through U8=5).
• Tier sort extracts "elite"/"select"/"local" from team name (case-insensitive). Falls back to rank 99 for unrecognized tiers.
• Tertiary sort by team name for deterministic ordering.
• All three sort dimensions verified by tests.

Tests

• 13 tests covering: unauth access, empty state, field allowlist, sensitive field exclusion, is_public filtering, empty team exclusion, age group sort, tier sort, combined sort, coach name join, null coach.
• Full suite: 568 tests pass with zero regressions.

## Self-Review **Result: PASS -- no issues found.** ### Security - Dedicated `PublicPlayerResponse` and `PublicTeamResponse` schemas with exactly the allowlisted fields (5 player, 4 team). No admin schema reuse. - 18 sensitive fields explicitly verified absent in `test_sensitive_fields_not_exposed`. - No auth dependencies on the endpoint -- only `Depends(get_db)`. ### Data Integrity - `is_public` column defaults to `false` via `server_default` -- existing players remain private until explicitly toggled. - Migration 022 is additive (column add with default). No data loss risk. Clean downgrade. - Model and migration are consistent (`Boolean, server_default=text("false")`). ### Query / Performance - `joinedload(Team.coach)` and `joinedload(Team.players)` avoids N+1 queries. - `is_public` filtering happens in Python after eager load. Acceptable at current scale (single tenant). If player count grows significantly, a subquery filter could be added later. ### Sort Logic - Age group sort uses the `AgeGroup` enum `.value` mapped to priority integers (U18=0 through U8=5). - Tier sort extracts "elite"/"select"/"local" from team name (case-insensitive). Falls back to rank 99 for unrecognized tiers. - Tertiary sort by team name for deterministic ordering. - All three sort dimensions verified by tests. ### Tests - 13 tests covering: unauth access, empty state, field allowlist, sensitive field exclusion, is_public filtering, empty team exclusion, age group sort, tier sort, combined sort, coach name join, null coach. - Full suite: 568 tests pass with zero regressions.

forgejo_admin commented 2026-03-27 03:29:29 +00:00

Author

Owner

Copy link

PR #178 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Pydantic v2

Security (CRITICAL -- primary concern for this PR):

The core security design is sound. The implementation uses a dedicated allowlist architecture:

1. Dedicated Pydantic schemas (PublicPlayerResponse, PublicTeamResponse, PublicTeamsResponse) -- never reuses admin response models. This is the correct pattern.
2. Explicit field extraction in _public_player() -- manually maps 5 specific fields (name, jersey_number, position, height, current_school) rather than using from_attributes=True ORM-to-schema mapping. This means even if new sensitive columns are added to the Player model later, they will NOT leak unless someone explicitly adds them to this function.
3. Coach data isolation -- the ORM joinedload(Team.coach) loads the full Coach object, but only team.coach.name is extracted into the coach_name: str | None field. Coach email, phone, Stripe Connect account ID, tax ID, contractor agreement details, and invite tokens are never serialized.
4. Team data isolation -- Team fields tenant_id, division, age_group, coach_id, groupme_group_id, groupme_share_url, created_at are all excluded by the schema.
5. is_public filtering -- players default to is_public=false, so no data leaks until explicitly opted in. Teams with zero public players are excluded entirely.
6. No auth dependency -- the endpoint correctly has no Depends(require_role) or Depends(require_admin), making it truly public.

Test coverage is strong: 13 tests across 5 test classes covering unauthenticated access, field allowlisting (with set-equality assertion), sensitive field absence (18 fields explicitly checked), is_public filtering, sort order (3 scenarios), and coach name join behavior.

SQLAlchemy patterns:

• joinedload(Team.coach) and joinedload(Team.players) avoid N+1 queries -- correct.
• Note: the Team.players relationship already uses lazy="selectin" (line 304 of models.py), so the explicit joinedload(Team.players) in the query is redundant but not harmful. The joinedload(Team.coach) is the one that actually matters here.
• Session management via FastAPI Depends(get_db) is standard pattern.

FastAPI specifics:

• response_model=PublicTeamsResponse on the endpoint ensures FastAPI's response serialization is also schema-bound.
• No model_config needed since schemas are constructed manually (not from ORM objects).

Pydantic/PEP compliance:

• Type hints present on all function signatures.
• Module docstring present.
• Code follows existing project patterns.

BLOCKERS

1. Duplicate Alembic revision ID -- migration will fail

The new migration 022_add_is_public_to_players.py has revision = "022" and down_revision = "021". However, 022_merge_heads.py already exists on main with revision = "022" and down_revision = ("019", "021"). Additionally, 023_backfill_player_jersey_from_orders.py already exists with down_revision = "022".

This creates two problems:

• Duplicate revision ID: Alembic will error with "Duplicate revision identifiers" at startup.
• Broken chain: The new migration descends from "021" instead of the current head "023".

Fix required: Rename to 024_add_is_public_to_players.py with revision = "024" and down_revision = "023".

Files:

• /home/ldraney/basketball-api/alembic/versions/022_add_is_public_to_players.py (new, conflicts)
• /home/ldraney/basketball-api/alembic/versions/022_merge_heads.py (existing on main)
• /home/ldraney/basketball-api/alembic/versions/023_backfill_player_jersey_from_orders.py (existing on main)

NITS

1. Redundant joinedload (non-blocking): joinedload(Team.players) in public.py:109 is redundant because Team.players already has lazy="selectin" on the relationship definition. Not harmful, but could be removed for clarity. Alternatively, a comment explaining it's intentional override would help.

2. Hardcoded tenant_id=1: Documented in the docstring as "single-tenant" and matches the current architecture. When multi-tenancy is needed, this will need a path or header parameter. Fine for now, but worth tracking.

3. Tier extraction from team name (fragile): _team_sort_key parses tier (Elite/Select/Local) from the team name string using substring matching. If a team is named "17U Selection Committee" it would match "select". This works for the current naming convention but is brittle. Consider a tier column on Team if this grows. Non-blocking for now.

4. Sensitive fields test list could be more complete: The test_sensitive_fields_not_exposed forbidden list has 18 fields but misses tenant_id, graduating_class, team_preference, division, checked_in, subscription_status, contract_signed_at, contract_signed_by, contract_signed_ip, contract_signature_url, contract_version, jersey_order_status, custom_notes, is_public, created_at. This is NOT a blocker because test_player_fields_are_allowlisted uses assert set(p.keys()) == allowed_keys which catches any leak. The forbidden list is secondary documentation. Still, expanding it would make intent even clearer.

SOP COMPLIANCE

• Branch named after issue (176-public-teams-endpoint matches issue #176)
• PR body follows template (Summary, Changes, Test Plan, Related sections all present)
• Related references user story (WS-S26) and sibling ticket (#177)
• No secrets committed
• No unnecessary file changes (5 files, all on-topic)
• Commit messages are descriptive
• Review checklist in PR body is thorough and honest

PROCESS OBSERVATIONS

• Change failure risk: MEDIUM due to the migration collision. The code and tests are solid, but the deployment will fail at the Alembic migration step. Easy fix once the revision ID is corrected.
• Deployment frequency impact: POSITIVE -- this unblocks the westside-app public teams page, a user-facing feature.
• Security posture: STRONG -- the allowlist-by-construction pattern (explicit field extraction + dedicated schemas + set-equality tests) is defense-in-depth. This is a model for how public endpoints should be built in this codebase.
• Sibling ticket awareness: PR #177 (public coaches) is noted as sharing public.py. The file structure supports this cleanly -- a new endpoint can be added to the same router without conflict.

VERDICT: NOT APPROVED

One blocker: duplicate Alembic revision ID "022" conflicts with the existing 022_merge_heads.py on main. The migration must be renumbered to 024 (with down_revision = "023") before merge. All other aspects of this PR -- security design, code quality, test coverage, SOP compliance -- are excellent.

## PR #178 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Alembic / Pydantic v2 **Security (CRITICAL -- primary concern for this PR):** The core security design is sound. The implementation uses a dedicated allowlist architecture: 1. **Dedicated Pydantic schemas** (`PublicPlayerResponse`, `PublicTeamResponse`, `PublicTeamsResponse`) -- never reuses admin response models. This is the correct pattern. 2. **Explicit field extraction** in `_public_player()` -- manually maps 5 specific fields (`name`, `jersey_number`, `position`, `height`, `current_school`) rather than using `from_attributes=True` ORM-to-schema mapping. This means even if new sensitive columns are added to the Player model later, they will NOT leak unless someone explicitly adds them to this function. 3. **Coach data isolation** -- the ORM `joinedload(Team.coach)` loads the full Coach object, but only `team.coach.name` is extracted into the `coach_name: str | None` field. Coach email, phone, Stripe Connect account ID, tax ID, contractor agreement details, and invite tokens are never serialized. 4. **Team data isolation** -- Team fields `tenant_id`, `division`, `age_group`, `coach_id`, `groupme_group_id`, `groupme_share_url`, `created_at` are all excluded by the schema. 5. **`is_public` filtering** -- players default to `is_public=false`, so no data leaks until explicitly opted in. Teams with zero public players are excluded entirely. 6. **No auth dependency** -- the endpoint correctly has no `Depends(require_role)` or `Depends(require_admin)`, making it truly public. **Test coverage** is strong: 13 tests across 5 test classes covering unauthenticated access, field allowlisting (with set-equality assertion), sensitive field absence (18 fields explicitly checked), `is_public` filtering, sort order (3 scenarios), and coach name join behavior. **SQLAlchemy patterns:** - `joinedload(Team.coach)` and `joinedload(Team.players)` avoid N+1 queries -- correct. - Note: the `Team.players` relationship already uses `lazy="selectin"` (line 304 of models.py), so the explicit `joinedload(Team.players)` in the query is redundant but not harmful. The `joinedload(Team.coach)` is the one that actually matters here. - Session management via FastAPI `Depends(get_db)` is standard pattern. **FastAPI specifics:** - `response_model=PublicTeamsResponse` on the endpoint ensures FastAPI's response serialization is also schema-bound. - No `model_config` needed since schemas are constructed manually (not from ORM objects). **Pydantic/PEP compliance:** - Type hints present on all function signatures. - Module docstring present. - Code follows existing project patterns. ### BLOCKERS **1. Duplicate Alembic revision ID -- migration will fail** The new migration `022_add_is_public_to_players.py` has `revision = "022"` and `down_revision = "021"`. However, `022_merge_heads.py` already exists on main with `revision = "022"` and `down_revision = ("019", "021")`. Additionally, `023_backfill_player_jersey_from_orders.py` already exists with `down_revision = "022"`. This creates two problems: - **Duplicate revision ID**: Alembic will error with "Duplicate revision identifiers" at startup. - **Broken chain**: The new migration descends from `"021"` instead of the current head `"023"`. **Fix required:** Rename to `024_add_is_public_to_players.py` with `revision = "024"` and `down_revision = "023"`. Files: - `/home/ldraney/basketball-api/alembic/versions/022_add_is_public_to_players.py` (new, conflicts) - `/home/ldraney/basketball-api/alembic/versions/022_merge_heads.py` (existing on main) - `/home/ldraney/basketball-api/alembic/versions/023_backfill_player_jersey_from_orders.py` (existing on main) ### NITS 1. **Redundant `joinedload`** (non-blocking): `joinedload(Team.players)` in `public.py:109` is redundant because `Team.players` already has `lazy="selectin"` on the relationship definition. Not harmful, but could be removed for clarity. Alternatively, a comment explaining it's intentional override would help. 2. **Hardcoded `tenant_id=1`**: Documented in the docstring as "single-tenant" and matches the current architecture. When multi-tenancy is needed, this will need a path or header parameter. Fine for now, but worth tracking. 3. **Tier extraction from team name** (fragile): `_team_sort_key` parses tier (Elite/Select/Local) from the team name string using substring matching. If a team is named "17U Selection Committee" it would match "select". This works for the current naming convention but is brittle. Consider a `tier` column on Team if this grows. Non-blocking for now. 4. **Sensitive fields test list could be more complete**: The `test_sensitive_fields_not_exposed` forbidden list has 18 fields but misses `tenant_id`, `graduating_class`, `team_preference`, `division`, `checked_in`, `subscription_status`, `contract_signed_at`, `contract_signed_by`, `contract_signed_ip`, `contract_signature_url`, `contract_version`, `jersey_order_status`, `custom_notes`, `is_public`, `created_at`. This is NOT a blocker because `test_player_fields_are_allowlisted` uses `assert set(p.keys()) == allowed_keys` which catches any leak. The forbidden list is secondary documentation. Still, expanding it would make intent even clearer. ### SOP COMPLIANCE - [x] Branch named after issue (`176-public-teams-endpoint` matches issue #176) - [x] PR body follows template (Summary, Changes, Test Plan, Related sections all present) - [x] Related references user story (WS-S26) and sibling ticket (#177) - [x] No secrets committed - [x] No unnecessary file changes (5 files, all on-topic) - [x] Commit messages are descriptive - [ ] Review checklist in PR body is thorough and honest ### PROCESS OBSERVATIONS - **Change failure risk: MEDIUM** due to the migration collision. The code and tests are solid, but the deployment will fail at the Alembic migration step. Easy fix once the revision ID is corrected. - **Deployment frequency impact: POSITIVE** -- this unblocks the westside-app public teams page, a user-facing feature. - **Security posture: STRONG** -- the allowlist-by-construction pattern (explicit field extraction + dedicated schemas + set-equality tests) is defense-in-depth. This is a model for how public endpoints should be built in this codebase. - **Sibling ticket awareness**: PR #177 (public coaches) is noted as sharing `public.py`. The file structure supports this cleanly -- a new endpoint can be added to the same router without conflict. ### VERDICT: NOT APPROVED One blocker: duplicate Alembic revision ID `"022"` conflicts with the existing `022_merge_heads.py` on main. The migration must be renumbered to `024` (with `down_revision = "023"`) before merge. All other aspects of this PR -- security design, code quality, test coverage, SOP compliance -- are excellent.

forgejo_admin added 1 commit 2026-03-27 03:30:40 +00:00

fix: renumber migration 022→024 to resolve duplicate revision ... d18279d686

Alembic revision 022 already exists (022_merge_heads.py). Renumbered
to 024 with down_revision=023 to chain after existing head.

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

forgejo_admin merged commit feb24d8b91 into main 2026-03-27 03:32:36 +00:00

forgejo_admin deleted branch 176-public-teams-endpoint 2026-03-27 03:32:36 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-27 03:32:36 +00:00

feat: add public teams endpoint (GET /public/teams) (#178)

forgejo_admin referenced this pull request 2026-03-27 03:43:33 +00:00

feat: add GET /public/coaches endpoint with allowlisted fields #180

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

No description provided.