ldraney/basketball-api

Fork 0

infra: migration 048 — GRANT tournament tables to westside_streamlit_ro #515

Open

forgejo_admin wants to merge 1 commit from 510-grant-tournament-tables-to-ro-role into main

forgejo_admin commented

2026-04-25 13:55:13 +00:00

Contributor

Summary

Adds migration 048 extending the westside_streamlit_ro allowlist (created in 044) to include tournaments and tournament_products. Enables the upcoming Streamlit tournament section (forgejo_admin/westside-streamlit#15) to query tournament data joined through the already-granted orders table. ACL only — no schema changes.

Closes #510

Changes

alembic/versions/048_grant_tournament_tables_to_ro_role.py — new migration. revision="048", down_revision="047". upgrade() GRANTs SELECT on tournaments and tournament_products to westside_streamlit_ro; downgrade() REVOKEs.

Style mirrors migration 044: per-table loop with TABLE public.{table} qualifier, ROLE_NAME + GRANT_TABLES constants. registrations intentionally NOT included — tournament payments write to orders (already granted in 044), not registrations.

Test Plan

Round-trip verified locally against a Postgres DB stamped to 047 with the role + tournament tables seeded:

$ alembic upgrade head
INFO  [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro.

$ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';"
        grantee        |     table_name      | privilege_type
-----------------------+---------------------+----------------
 westside_streamlit_ro | tournament_products | SELECT
 westside_streamlit_ro | tournaments         | SELECT

$ alembic downgrade 047
INFO  [alembic.runtime.migration] Running downgrade 048 -> 047, GRANT SELECT on tournament tables to westside_streamlit_ro.

$ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';"
 grantee | table_name | privilege_type
---------+------------+----------------
(0 rows)

$ alembic upgrade head   # idempotent re-apply
INFO  [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro.
# GRANTs reapplied

ruff check alembic/versions/048_grant_tournament_tables_to_ro_role.py — clean
ruff format --check — clean
pytest tests/ — 474 pass; 1 pre-existing failure (test_jersey_reminder.py::test_plain_text_has_deadline_copy asserts "March 30" but copy now reads "April 10") confirmed to also fail on main and is unrelated to this migration

Review Checklist

Revision is exactly "048", down_revision is exactly "047" (no slot collision)
Role name westside_streamlit_ro matches migration 044
upgrade() GRANTs SELECT on tournaments and tournament_products only — registrations intentionally excluded
downgrade() REVOKEs SELECT on the same two tables
Migration style/header matches 044 (per-table loop with TABLE public.{table} qualifier)
No schema changes; ACL only
No changes to src/basketball_api/models.py
ruff check clean
ruff format --check clean
Round-trip (upgrade head / downgrade 047 / upgrade head) verified locally
Existing test suite still passes (modulo the unrelated pre-existing test_jersey_reminder failure on main)

Notes / Surprises

A clean alembic upgrade head from an empty DB hits a pre-existing issue at migration 029_add_schedule_tables (type "division" already exists on the second Enum(..., create_type=False) usage in the same migration with current SQLAlchemy 2.0.48). This is unrelated to migration 048 and is reproducible on main. Round-trip for 048 was therefore validated against a DB stamped at 047 with the role and tournament tables seeded — verifying the actual SQL emitted by 048 works correctly. Worth filing a separate ticket for the 029 enum issue.
Alembic head was 047 at branch time — slot 048 was not contended.

Forgejo issue: #510
Review verdict note: review-1072-2026-04-22 (APPROVED)
project-westside-streamlit — consumer project
story-westside-streamlit-tournament — user story this serves
feedback_migration_slot_coordination — slot-collision rule
feedback_basketball_hands_off — narrow-allowlist rationale
Migration 044_add_westside_streamlit_ro_role — defines the role being extended
Migration 045_add_tournament_tables — source of the tables being GRANTed
Consumer ticket: forgejo_admin/westside-streamlit#15 (C3 — Tournament section)

## Summary Adds migration `048` extending the `westside_streamlit_ro` allowlist (created in `044`) to include `tournaments` and `tournament_products`. Enables the upcoming Streamlit tournament section (`forgejo_admin/westside-streamlit#15`) to query tournament data joined through the already-granted `orders` table. ACL only — no schema changes. Closes #510 ## Changes - `alembic/versions/048_grant_tournament_tables_to_ro_role.py` — new migration. `revision="048"`, `down_revision="047"`. `upgrade()` GRANTs SELECT on `tournaments` and `tournament_products` to `westside_streamlit_ro`; `downgrade()` REVOKEs. Style mirrors migration `044`: per-table loop with `TABLE public.{table}` qualifier, `ROLE_NAME` + `GRANT_TABLES` constants. `registrations` intentionally NOT included — tournament payments write to `orders` (already granted in 044), not `registrations`. ## Test Plan Round-trip verified locally against a Postgres DB stamped to `047` with the role + tournament tables seeded: ``` $ alembic upgrade head INFO [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro. $ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';" grantee | table_name | privilege_type -----------------------+---------------------+---------------- westside_streamlit_ro | tournament_products | SELECT westside_streamlit_ro | tournaments | SELECT $ alembic downgrade 047 INFO [alembic.runtime.migration] Running downgrade 048 -> 047, GRANT SELECT on tournament tables to westside_streamlit_ro. $ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';" grantee | table_name | privilege_type ---------+------------+---------------- (0 rows) $ alembic upgrade head # idempotent re-apply INFO [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro. # GRANTs reapplied ``` - `ruff check alembic/versions/048_grant_tournament_tables_to_ro_role.py` — clean - `ruff format --check` — clean - `pytest tests/` — 474 pass; 1 pre-existing failure (`test_jersey_reminder.py::test_plain_text_has_deadline_copy` asserts "March 30" but copy now reads "April 10") confirmed to also fail on `main` and is unrelated to this migration ## Review Checklist - [x] Revision is exactly `"048"`, down_revision is exactly `"047"` (no slot collision) - [x] Role name `westside_streamlit_ro` matches migration 044 - [x] `upgrade()` GRANTs SELECT on `tournaments` and `tournament_products` only — `registrations` intentionally excluded - [x] `downgrade()` REVOKEs SELECT on the same two tables - [x] Migration style/header matches 044 (per-table loop with `TABLE public.{table}` qualifier) - [x] No schema changes; ACL only - [x] No changes to `src/basketball_api/models.py` - [x] `ruff check` clean - [x] `ruff format --check` clean - [x] Round-trip (`upgrade head` / `downgrade 047` / `upgrade head`) verified locally - [x] Existing test suite still passes (modulo the unrelated pre-existing `test_jersey_reminder` failure on `main`) ## Notes / Surprises - A clean `alembic upgrade head` from an empty DB hits a pre-existing issue at migration `029_add_schedule_tables` (`type "division" already exists` on the second `Enum(..., create_type=False)` usage in the same migration with current SQLAlchemy 2.0.48). This is unrelated to migration 048 and is reproducible on `main`. Round-trip for 048 was therefore validated against a DB stamped at `047` with the role and tournament tables seeded — verifying the actual SQL emitted by 048 works correctly. Worth filing a separate ticket for the 029 enum issue. - Alembic head was 047 at branch time — slot 048 was not contended. ## Related Notes - Forgejo issue: #510 - Review verdict note: `review-1072-2026-04-22` (APPROVED) - `project-westside-streamlit` — consumer project - `story-westside-streamlit-tournament` — user story this serves - `feedback_migration_slot_coordination` — slot-collision rule - `feedback_basketball_hands_off` — narrow-allowlist rationale - Migration `044_add_westside_streamlit_ro_role` — defines the role being extended - Migration `045_add_tournament_tables` — source of the tables being GRANTed - Consumer ticket: `forgejo_admin/westside-streamlit#15` (C3 — Tournament section)

forgejo_admin added 1 commit

2026-04-25 13:55:13 +00:00

infra: migration 048 - GRANT tournament tables to westside_streamlit_ro

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

65199be6a0

Extends migration 044's allowlist for the westside_streamlit_ro role to
include `tournaments` and `tournament_products` (added in migration 045).
Tournament payments flow through the existing `orders` table, which is
already granted in 044, so only these two tables need to be added.
`registrations` is intentionally NOT granted: it is for tryouts/practices
and tournament checkout writes to `orders`, not `registrations`.

ACL only - no schema changes. Defense-in-depth principle from 044 holds:
new tables stay denied by default; this extends the allowlist deliberately
and minimally.

Closes #510

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

forgejo_admin commented

2026-04-25 13:58:09 +00:00

Author

Contributor

PR #515 Review

DOMAIN REVIEW

Stack: Python / Alembic / Postgres ACL. ACL-only migration, no schema or model changes.

Migration slot coordination (feedback_migration_slot_coordination)

revision = "048", down_revision = "047" — slot correct.
PR body confirms head was 047 at branch time, no contention. Single migration, no parallel-agent collision risk.
File name 048_grant_tournament_tables_to_ro_role.py matches the {slot}_{snake_case}.py convention used by 044/045.

Style match with migration 044

Per-table loop with TABLE public.{table} qualifier — matches 044 exactly.
ROLE_NAME and GRANT_TABLES constants — same pattern as 044.
Module docstring documents revision/down_revision/create_date in the same shape as 044.
One stylistic divergence (in the right direction): 044's downgrade() uses REVOKE ALL PRIVILEGES (because it also drops the role); 048 uses targeted REVOKE SELECT because it must NOT touch the grants 044 owns. This is the correct narrowing — a REVOKE ALL here would clobber 044's allowlist on tournaments/tournament_products if those tables had been added to 044 retroactively. Targeted REVOKE is safer and idempotent.

Allowlist correctness (deliberate registrations exclusion)

GRANT_TABLES = ("tournaments", "tournament_products") — exactly what westside-streamlit#15 needs for the join path.
Docstring explicitly documents why registrations is NOT included: tournament checkout writes to orders (already granted in 044), registrations is for tryouts/practices. Correct per feedback_basketball_hands_off defense-in-depth: extend allowlist deliberately and minimally.
Verified against migration 045: tournament_products.product_id -> products.id (granted in 044), tournament_products.tournament_id -> tournaments.id (granted here). Join path closes without registrations.

Round-trip evidence

PR body shows three-step round-trip: upgrade head -> grants present, downgrade 047 -> grants removed (0 rows), upgrade head -> grants re-applied. This is the right test for an ACL migration.
information_schema.role_table_grants is the correct source-of-truth view to verify GRANTs. Output shows exactly the two expected rows and nothing else.
Re-upgrade is idempotent because GRANT SELECT on an already-granted privilege is a no-op in Postgres (no IF NOT EXISTS gymnastics needed).

Code quality (generic checklist)

f-string interpolation of {table} and {ROLE_NAME} into raw SQL — both are module-local constants under code review, not user input. No injection risk. Acceptable here for the same reason 044 does it.
No secrets in diff. No env var reads (correctly — role already exists from 044, password not needed).
No magic numbers, no hardcoded URLs, no swallowed errors.
ruff check / ruff format clean per PR body.

BLOCKERS

None.

NITS

None blocking. One observation for the record: the upgrade log message text ("GRANT SELECT on tournament tables to westside_streamlit_ro.") is supplied by Alembic from the docstring's first line, which matches. No action.

SOP COMPLIANCE

Branch named 510-grant-tournament-tables-to-ro-role — matches {issue}-{kebab-case} convention
PR body has Summary / Changes / Test Plan / Notes / Related — full template
Related references parent issue #510, consumer ticket westside-streamlit#15, conventions (feedback_migration_slot_coordination, feedback_basketball_hands_off), and predecessor migrations 044/045
Round-trip test evidence inline in Test Plan
No secrets, .env files, or credentials committed
Single-file change, no scope creep
Closes #510

PROCESS OBSERVATIONS

DORA notes:

Deployment frequency: low-risk ACL migration, single file, 48 LOC. Fast-merge candidate once approved.
Change failure risk: minimal. Round-trip verified, downgrade is targeted REVOKE (won't clobber sibling grants), no schema mutation, no model coupling.
Lead time: cleanly scoped — one ticket, one PR, one slot. Mirrors 044 architecture so reviewers can pattern-match instantly.

Discovered scope (do NOT block this PR — flag for separate tickets per feedback_discovered_scope_always_tracked):

Migration 029 enum bug — clean alembic upgrade head from empty DB fails at 029_add_schedule_tables with type "division" already exists. Reproducible on main. Confirmed in source: lines 34 and 77 both reference Enum("boys", "girls", name="division", create_type=False) without an explicit division.create() call, so SQLAlchemy 2.0.48 emits the type creation on the first hit and again on the second. Should be a separate Forgejo issue.
tests/test_jersey_reminder.py::test_plain_text_has_deadline_copy — asserts "March 30" deadline copy, but template now reads "April 10". Pre-existing failure on main, unrelated to this migration. Tracking ticket may already exist (#491 mentions "two pre-existing test failures keep CI red on main"); worth checking before filing a new one.

Neither blocks #515. Both warrant their own board items.

VERDICT: APPROVED

## PR #515 Review ### DOMAIN REVIEW Stack: Python / Alembic / Postgres ACL. ACL-only migration, no schema or model changes. **Migration slot coordination (`feedback_migration_slot_coordination`)** - `revision = "048"`, `down_revision = "047"` — slot correct. - PR body confirms head was `047` at branch time, no contention. Single migration, no parallel-agent collision risk. - File name `048_grant_tournament_tables_to_ro_role.py` matches the `{slot}_{snake_case}.py` convention used by 044/045. **Style match with migration 044** - Per-table loop with `TABLE public.{table}` qualifier — matches 044 exactly. - `ROLE_NAME` and `GRANT_TABLES` constants — same pattern as 044. - Module docstring documents revision/down_revision/create_date in the same shape as 044. - One stylistic divergence (in the right direction): 044's `downgrade()` uses `REVOKE ALL PRIVILEGES` (because it also drops the role); 048 uses targeted `REVOKE SELECT` because it must NOT touch the grants 044 owns. This is the correct narrowing — a `REVOKE ALL` here would clobber 044's allowlist on `tournaments`/`tournament_products` if those tables had been added to 044 retroactively. Targeted REVOKE is safer and idempotent. **Allowlist correctness (deliberate `registrations` exclusion)** - `GRANT_TABLES = ("tournaments", "tournament_products")` — exactly what `westside-streamlit#15` needs for the join path. - Docstring explicitly documents why `registrations` is NOT included: tournament checkout writes to `orders` (already granted in 044), `registrations` is for tryouts/practices. Correct per `feedback_basketball_hands_off` defense-in-depth: extend allowlist deliberately and minimally. - Verified against migration 045: `tournament_products.product_id` -> `products.id` (granted in 044), `tournament_products.tournament_id` -> `tournaments.id` (granted here). Join path closes without `registrations`. **Round-trip evidence** - PR body shows three-step round-trip: `upgrade head` -> grants present, `downgrade 047` -> grants removed (0 rows), `upgrade head` -> grants re-applied. This is the right test for an ACL migration. - `information_schema.role_table_grants` is the correct source-of-truth view to verify GRANTs. Output shows exactly the two expected rows and nothing else. - Re-upgrade is idempotent because `GRANT SELECT` on an already-granted privilege is a no-op in Postgres (no `IF NOT EXISTS` gymnastics needed). **Code quality (generic checklist)** - f-string interpolation of `{table}` and `{ROLE_NAME}` into raw SQL — both are module-local constants under code review, not user input. No injection risk. Acceptable here for the same reason 044 does it. - No secrets in diff. No env var reads (correctly — role already exists from 044, password not needed). - No magic numbers, no hardcoded URLs, no swallowed errors. - ruff check / ruff format clean per PR body. ### BLOCKERS None. ### NITS None blocking. One observation for the record: the upgrade log message text (`"GRANT SELECT on tournament tables to westside_streamlit_ro."`) is supplied by Alembic from the docstring's first line, which matches. No action. ### SOP COMPLIANCE - [x] Branch named `510-grant-tournament-tables-to-ro-role` — matches `{issue}-{kebab-case}` convention - [x] PR body has Summary / Changes / Test Plan / Notes / Related — full template - [x] Related references parent issue #510, consumer ticket `westside-streamlit#15`, conventions (`feedback_migration_slot_coordination`, `feedback_basketball_hands_off`), and predecessor migrations 044/045 - [x] Round-trip test evidence inline in Test Plan - [x] No secrets, .env files, or credentials committed - [x] Single-file change, no scope creep - [x] Closes #510 ### PROCESS OBSERVATIONS DORA notes: - Deployment frequency: low-risk ACL migration, single file, 48 LOC. Fast-merge candidate once approved. - Change failure risk: minimal. Round-trip verified, downgrade is targeted REVOKE (won't clobber sibling grants), no schema mutation, no model coupling. - Lead time: cleanly scoped — one ticket, one PR, one slot. Mirrors 044 architecture so reviewers can pattern-match instantly. Discovered scope (do NOT block this PR — flag for separate tickets per `feedback_discovered_scope_always_tracked`): 1. **Migration 029 enum bug** — clean `alembic upgrade head` from empty DB fails at `029_add_schedule_tables` with `type "division" already exists`. Reproducible on `main`. Confirmed in source: lines 34 and 77 both reference `Enum("boys", "girls", name="division", create_type=False)` without an explicit `division.create()` call, so SQLAlchemy 2.0.48 emits the type creation on the first hit and again on the second. Should be a separate Forgejo issue. 2. **`tests/test_jersey_reminder.py::test_plain_text_has_deadline_copy`** — asserts "March 30" deadline copy, but template now reads "April 10". Pre-existing failure on `main`, unrelated to this migration. Tracking ticket may already exist (#491 mentions "two pre-existing test failures keep CI red on main"); worth checking before filing a new one. Neither blocks #515. Both warrant their own board items. ### VERDICT: APPROVED

forgejo_admin referenced this pull request

2026-04-25 14:03:52 +00:00

Two pre-existing test failures keep CI red on main #491

forgejo_admin referenced this pull request

2026-05-08 05:32:18 +00:00

feat: add payment_links table and outstanding balance email #519

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 510-grant-tournament-tables-to-ro-role:510-grant-tournament-tables-to-ro-role

git switch 510-grant-tournament-tables-to-ro-role

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff 510-grant-tournament-tables-to-ro-role

git switch 510-grant-tournament-tables-to-ro-role

git rebase main

git switch main

git merge --ff-only 510-grant-tournament-tables-to-ro-role

git switch 510-grant-tournament-tables-to-ro-role

git rebase main

git switch main

git merge --no-ff 510-grant-tournament-tables-to-ro-role

git switch main

git merge --squash 510-grant-tournament-tables-to-ro-role