pal-e-docs

Review: C2 — basketball-api migration 048 GRANT tournament tables

review-1072-2026-04-22 Doc

review approved
Project

Verdict: APPROVED

Note: Both READY and APPROVED are accepted as passing verdicts by the check-board-advance hook. Use either keyword interchangeably.
Board item #1072 — Forgejo issue forgejo_admin/basketball-api#510. Third review round; scope corrected on 2026-04-24 to drop registrations after C3 cascading review confirmed tournament payments flow through orders (already in 044's GRANT allowlist). Two minor follow-ups flagged but neither blocks moving to todo.

Template Completeness

Issue type: Feature — checked against template-issue-feature:
  • [x] Type
  • [x] Lineage (calls out 044 extension + 2026-04-24 scope correction)
  • [x] Repo
  • [x] User Story
  • [x] Context (explains the orders-vs-registrations join chain)
  • [x] File Targets (single new migration file + explicit do-not-touch list)
  • [x] Acceptance Criteria (7 items, all binary-verifiable)
  • [x] Test Expectations (with run commands)
  • [x] Constraints (slot rule, op.execute style, no model edits, NO registrations)
  • [x] Checklist
  • [x] Related (links migrations 044 + 045, story note, feedback memories, consumer ticket)

Traceability

  • [x] story:tournament label — Story: Tournament Signup + Payment Visibility
  • [x] story note verified — story-westside-streamlit-tournament (id 1594) exists in pal-e-docs and explicitly documents the orders-not-registrations join chain that this migration enables
  • [ ] story not yet listed in project-westside-streamlit user-stories table — current rows: triage, blast, clusters, jerseys, audit, reachable. The tournament key needs a row added.
  • [x] arch:postgres-role label — postgres-role component (the read-only role created in 044 and being extended here)
  • [ ] no dedicated arch-postgres-role note found via search_notes. Pattern is documented in project-westside-streamlit Architecture section + review-936-2026-04-10, which is sufficient context, but a standalone arch note would harden traceability.
  • [x] Forgejo issue — forgejo_admin/basketball-api#510, state: open

File Targets

  • [x] alembic/versions/048_grant_tournament_tables_to_ro_role.py — slot is free; current head is 047_add_stripe_checkout_url_to_orders.py, slots 030–047 occupied + one legacy hash-named file (e09c9e678004_add_division_column_to_players.py). down_revision 047 is correct.
  • [x] Reference migration 044_add_westside_streamlit_ro_role.py exists and defines role westside_streamlit_ro with the canonical ACL-only style.
  • [x] Reference migration 045_add_tournament_tables.py exists and creates exactly the two tables being granted (tournaments, tournament_products).
  • [x] src/basketball_api/models.py defines Tournament (line 596, table tournaments) and TournamentProduct (line 614, table tournament_products) — names match exactly. No edits required (correctly listed under "Files NOT to touch").
  • [x] registrations table exists in models.py (line 303, Registration bound to tryouts/practices) — correctly excluded from this migration. Confirms the C3-driven scope correction is right.

Repo Placement

OK. Migration belongs in basketball-api (where alembic + the role + the tables live). Consumer dashboard work is correctly split into westside-streamlit#15 (board item #1073, C3).

Dependencies

Documented and consistent with the board:
  • Upstream: migrations 044 (role) and 045 (tournament tables) — both already merged, both at HEAD.
  • Downstream: board item #1073 (C3 — westside-streamlit Tournament section) is explicitly blocked by C2. C3's title carries "(blocked by C2)" — consistent.
  • Slot risk: slot 048 is currently free in ~/basketball-api main. Per feedback_migration_slot_coordination, if any other PR claims 048 between approval and PR open, dev must rebase and renumber. The issue body already calls this out under Constraints.

Acceptance Criteria

Seven AC, all binary-verifiable:
  • Revision strings are exact strings — verifiable by inspection.
  • Role name is verifiable by reading 044.
  • upgrade()/downgrade() statements are exact text — verifiable by inspection.
  • Round-trip is verifiable via alembic upgrade head + alembic downgrade 047.
  • Style match to migration 044 — see Blast Radius below.
Test commands (pytest, ruff, alembic) are real and runnable. No missing criteria.

Blast Radius

Minimal — ACL-only, two tables, single role. No schema mutations. No model edits. No app code changes.
Style note (informational, not blocking): The AC #3 prescribes op.execute("GRANT SELECT ON tournaments, tournament_products TO westside_streamlit_ro") — comma-separated form. Migration 044 itself uses a per-table loop with GRANT SELECT ON TABLE public.{table} TO {ROLE_NAME}; (lines 82–83) — explicit TABLE keyword and explicit public. schema. Both forms are valid PostgreSQL; the comma-separated form will execute correctly. This is purely a stylistic mismatch with the "match migration 044's SQL style" constraint. Dev agent should be aware they have two valid paths: (a) honor AC #3 verbatim, or (b) match 044's per-table loop style. Either is fine — flagging so the QA reviewer doesn't bounce the PR for style during code review.
No similar pattern needs changing in sibling services. westside-ops uses the same read-only-role pattern but with its own role and its own allowlist; out of scope here.

Decomposition Assessment

1 file target, 7 AC, 1 repo. Estimated agent work: 5–10 minutes (write migration, run upgrade/downgrade locally, ruff, pytest, open PR). Well within the 5-minute rule for the actual code change; the longest portion is the local round-trip test which is mechanical. No decomposition needed.

Recommendation

Move from backlog → todo. Two non-blocking follow-ups for Ava to decide whether to track separately:
  • [SCOPE] Add a row for the tournament key to the User Stories table on project-westside-streamlit (anchor user-stories). Story note story-westside-streamlit-tournament already exists (id 1594) but isn't linked from the project page table.
  • [SCOPE] Consider creating a standalone arch-postgres-role note documenting the read-only-role pattern (defense-in-depth, allowlist-only, password from env). The pattern is currently documented across review-936-2026-04-10, migration 044's docstring, and project-westside-streamlit Architecture — consolidation would harden the arch:postgres-role label's traceability target.
Neither follow-up blocks ticket #1072 from advancing. Migration 048 is well-scoped, correctly bounded, and ready for an agent.