Review (pass 3): Create 16U Local Queens team
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:386—Team.contract_config: Mapped[dict | None] = mapped_column(JSONB, nullable=True)— exists.src/basketball_api/models.py:279—Player.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. IncludesLOCAL_CONFIG_16Udict 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 noarch-basketball-apinote 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_urlfield; 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 is040(highest current is039_add_recovery_email_sent.py). Recommend the ticket pin the filename to040_create_16u_local_queens_team.pyand setdown_revision = "039". Minor refinement.migrations/data/16u_local_queens_config.json(optional) — path does not exist. There is no top-levelmigrations/directory; all migrations live underalembic/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. Noslug/internal_keycolumn 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. divisionis an Enum with valuesboysandgirls— ticket saysdivision = 'girls'. PASS.age_groupis an Enum with valuesU8, U10, U12, U14, U16, U18. "16U" maps cleanly tou16. PASS.tenant_idis 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_idis 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 NOTHINGon(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:bugin the board store; prompt says labels were updated totype: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.jsonfile 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_idset to Westside tenant (lookup by name or matching ids 1-7 precedent). - Clarify whether
coach_idshould 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:bug→type:featureto match the Forgejo issue's### Type: Featureheader.
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.