Review (pass 3): Create 16U Local Queens team

review-927-2026-04-10-pass3 Doc

review needs-refinement

Scope Review (pass 3): Create 16U Local Queens team

Verdict: NEEDS_REFINEMENT (minor)
Board item: #927 on board-westside-basketball
Forgejo issue: forgejo_admin/basketball-api#422
Type (per issue body): Feature
Labels (per prompt): type:feature, arch:basketball-api, story:WS-S23, scope:discovered

Correction of prior review

Review review-927-2026-04-10 (BLOCK) was based on a stale local checkout (/home/ldraney/basketball-api on branch 111-player-visibility-api). That branch predates the addition of Team.contract_config. This pass re-verified against a fresh clone of origin/main at commit 9598c4d.

Ground truth verified on origin/main @ 9598c4d

  • src/basketball_api/models.py:386Team.contract_config: Mapped[dict | None] = mapped_column(JSONB, nullable=True)exists.
  • src/basketball_api/models.py:279Player.contract_overrides: Mapped[dict | None] = mapped_column(JSONB, nullable=True)exists.
  • alembic/versions/031_add_contract_config_to_teams.py — column migration.
  • alembic/versions/034_seed_team_contract_configs.py — seeds all 7 existing teams. Includes LOCAL_CONFIG_16U dict literal (lines 334-367): variant=local, monthly_fee_default=200, empty tournaments, Monday+Friday BWill practices, sections for jersey/commitment/communication only. This is the exact template the ticket asks the dev agent to clone.
  • alembic/versions/035_set_player_contract_overrides.py — precedent for per-player overrides (Jacelyn custom $160 follow-up).

Template completeness (template-issue-feature)

All required sections present and high quality: Type, Lineage, Repo, User Story, Context, File Targets, Acceptance Criteria, Test Expectations, Constraints, Checklist, Related. The Context section is unusually strong — it already names the exact template dict (team id 7 = 16U Local Kings = LOCAL_CONFIG_16U) and the variant/fee/tournament deltas.

Traceability triangle

  • story:WS-S23 — verified on project-westside-basketball#stories-admin: "As an admin, I want to configure custom contract terms per player (different fee, tournaments, practice schedule)…" Story slot is a clean fit for a new team row + custom-override follow-up. PASS.
  • arch:basketball-api — backing arch note was not located via search_notes("arch basketball-api"). If no arch-basketball-api note exists, that is a standing [SCOPE] gap shared across every ticket on this repo, not specific to #927. Flagging as a cross-cutting item, not a per-ticket blocker.
  • Forgejo issue — #422 is open. However, the board item #927 has an empty forgejo_url field; the link from board to issue is not wired. Fixing the link is a minor refinement.

File target verification

  • alembic/versions/NNN_create_16u_local_queens_team.py — directory exists. Next free number is 040 (highest current is 039_add_recovery_email_sent.py). Recommend the ticket pin the filename to 040_create_16u_local_queens_team.py and set down_revision = "039". Minor refinement.
  • migrations/data/16u_local_queens_config.json (optional) — path does not exist. There is no top-level migrations/ directory; all migrations live under alembic/versions/, and the existing pattern (see 034) is to define config as a Python dict literal inline in the migration module, not as an external JSON file. Recommend dropping the optional JSON file from the ticket and instructing the dev agent to follow the 034 pattern (inline dict). Minor refinement.

Schema / acceptance-criteria sanity checks

  • Team columns on main: id, tenant_id, name, division, age_group, coach_id, groupme_group_id, groupme_share_url, contract_config, created_at. No slug / internal_key column exists. The ticket's Constraints line "Team slug / internal key naming should follow existing convention" is based on a column that does not exist on main. Recommend removing or clarifying that constraint. Minor refinement.
  • division is an Enum with values boys and girls — ticket says division = 'girls'. PASS.
  • age_group is an Enum with values U8, U10, U12, U14, U16, U18. "16U" maps cleanly to u16. PASS.
  • tenant_id is NOT NULL — the migration must select the Westside tenant id (lookup by name or hardcode id=1, matching migration 034 precedent of hardcoded team ids). Add to acceptance criteria: "tenant_id correctly set to the Westside tenant." Minor refinement.
  • coach_id is nullable — fine to leave NULL until Marcus assigns a coach, or set to match 16U Elite Queens' coach. Ticket is silent; either is acceptable, but worth an explicit note. Nit.
  • Idempotency criterion: achievable via INSERT … ON CONFLICT DO NOTHING on (tenant_id, name), or by checking existence before insert. Viable.

Dependencies / blast radius

  • Unblocks: basketball-api#424 (Marcus 2026-04-10 batch — Jacelyn Bronson move to 16U Local Queens at $160/mo). The actual move + $160 override is explicitly out of scope for this ticket (Constraints section states it correctly).
  • Blast radius: Creating a new team row cannot break existing code paths — all queries are tenant-scoped and team-list APIs just return the extra row. Contract rendering reads team.contract_config — an additional row with a well-formed local variant config cannot regress existing teams.
  • Label drift: Board item #927 shows type:bug in the board store; prompt says labels were updated to type:feature. Board state is stale. Minor refinement.

5-minute / decomposition check

  • File targets: 1 migration file (and the optional JSON should be dropped). Single repo.
  • Acceptance criteria: 6 — at the edge but all tightly scoped to a single migration.
  • Estimated agent work: single alembic migration that copies LOCAL_CONFIG_16U, swaps variant/tenant/name/division/age_group lookups, inserts one row. Well under 5 minutes.
  • No decomposition needed.

Refinement list (all minor, non-blocking)

  • Pin migration filename to 040_create_16u_local_queens_team.py, down_revision = "039".
  • Drop the optional migrations/data/16u_local_queens_config.json file target — follow migration 034's inline-dict pattern.
  • Remove the "Team slug / internal key naming" constraint — no such column exists on main.
  • Add acceptance criterion: tenant_id set to Westside tenant (lookup by name or matching ids 1-7 precedent).
  • Clarify whether coach_id should be NULL or copied from 16U Elite Queens (nit).
  • Fix board item #927 → issue #422 Forgejo URL wiring (empty today).
  • Sync board label from type:bugtype:feature to match the Forgejo issue's ### Type: Feature header.

Verdict

NEEDS_REFINEMENT — no architectural blockers, ticket premise is sound and fully supported by origin/main. All flagged items are small edits to the ticket body plus two pieces of board metadata hygiene (forgejo_url, label). Once addressed, this is ready for todo.
If Ava prefers to waive the minor refinements and dispatch as-is, the dev agent can be instructed inline to (a) follow migration 034's inline-dict pattern, (b) set tenant_id via lookup, (c) ignore the non-existent slug column, and (d) use filename 040.