forgejo_admin/basketball-api

Fork 1

fix: dedupe Alice Uwamahoro player rows (#420) #428

Merged

forgejo_admin merged 2 commits from 420-dedupe-alice-uwamahoro into main

2026-04-10 23:03:27 +00:00

forgejo_admin commented

2026-04-10 20:47:12 +00:00

Owner

Summary

Delete the stale Alice Uwamahoro submission (player id 201, parent id 175) caused by a typo'd email (aliceuwamahoro13a.@gmail.com), keeping the canonical rows (player 202, parent 176). Implements basketball-api#420.

Changes

alembic/versions/039_dedupe_alice_uwamahoro.py (new) — data-only migration that deletes player 201 + parent 175 with safety guards.

Migration Notes

Revision number: Used 039 (down_revision 038), not 040 as the ticket anticipated. Current head on main at the time of branching is 038 (038_add_declined_contract_status), so 039 is the next slot. basketball-api#422 can chain onto 039 as 040 when it lands.
Idempotent: short-circuits if player 201 / parent 175 are already absent, so it is safe to re-run.
Dependent-row guard: refuses to delete player 201 and raises RuntimeError with counts if any rows exist in player_teams, orders, or registrations for player_id 201. No silent orphaning.
Parent guard: refuses to delete parent 175 if any player row still references parent_id = 175.
downgrade() is a no-op: deletion is irreversible; we do not retain original row data and will not guess.
Uses op.execute() via sa.text() with bound params, matching the data-only style of migrations like 035_set_player_contract_overrides.py.
Player / Parent models are untouched.

Test Plan

Validated locally against a sqlite sandbox pre-seeded with rows matching the ticket's state:

Happy path: player 201 + parent 175 + player 202 + parent 176, no dependents. Ran upgrade().
- SELECT COUNT(*) FROM players WHERE name = 'Alice Uwamahoro' -> 1
- Player 201 deleted, parent 175 deleted.
- Player 202 still has parent_id = 176, parent 176 intact.
Idempotency: ran upgrade() a second time on the post-migration state. No errors, no-op.
Guard: seeded an orders row referencing player 201. upgrade() raised RuntimeError: Refusing to delete player 201: dependent rows exist (player_teams=0, orders=1, registrations=0). Manual cleanup required before this migration can run. as expected.
Alembic chain: ScriptDirectory walk confirms single head 039 chaining cleanly back through 038 -> 037 -> ... -> base.
Lint: ruff format + ruff check clean.
Full test suite: pytest tests/ — 891 passed. 4 pre-existing collection/import failures unrelated to this migration (test_contract_reminder.py + 3 test_reconciliation.py cases missing groupme_sdk) were observed; they fail identically on main without this change.

Post-merge (not part of this PR): ArgoCD sync + alembic upgrade head against prod will execute the delete.

Review Checklist

Migration is idempotent (safe to re-run)
Dependent-row guard in place for player_teams / orders / registrations
downgrade() documented as no-op (deletion irreversible)
Uses op.execute() + sa.text() with bound params, matching existing data-only migration style
Player / Parent models untouched
ruff format + ruff check clean
Alembic single-head chain verified (039 -> 038 -> ...)
Tested against sqlite sandbox (happy path + idempotency + guard)
Scope limited to player 201 / parent 175; no other Alice rows touched

Forgejo issue: Closes #420
Board item: #925 on board-westside-basketball (APPROVED via review-925-2026-04-10)
Related but out of scope: #418 (Marcus dupe root cause), #419 (Test Queens Player symptom), #422 (separate dedupe that will chain as 040 on top of this 039)

## Summary Delete the stale Alice Uwamahoro submission (player id 201, parent id 175) caused by a typo'd email (`aliceuwamahoro13a.@gmail.com`), keeping the canonical rows (player 202, parent 176). Implements basketball-api#420. ## Changes - `alembic/versions/039_dedupe_alice_uwamahoro.py` (new) — data-only migration that deletes player 201 + parent 175 with safety guards. ## Migration Notes - **Revision number**: Used `039` (down_revision `038`), not `040` as the ticket anticipated. Current head on `main` at the time of branching is `038` (`038_add_declined_contract_status`), so `039` is the next slot. basketball-api#422 can chain onto `039` as `040` when it lands. - **Idempotent**: short-circuits if player 201 / parent 175 are already absent, so it is safe to re-run. - **Dependent-row guard**: refuses to delete player 201 and raises `RuntimeError` with counts if any rows exist in `player_teams`, `orders`, or `registrations` for player_id 201. No silent orphaning. - **Parent guard**: refuses to delete parent 175 if any player row still references `parent_id = 175`. - **downgrade() is a no-op**: deletion is irreversible; we do not retain original row data and will not guess. - Uses `op.execute()` via `sa.text()` with bound params, matching the data-only style of migrations like `035_set_player_contract_overrides.py`. - `Player` / `Parent` models are untouched. ## Test Plan Validated locally against a sqlite sandbox pre-seeded with rows matching the ticket's state: 1. **Happy path**: player 201 + parent 175 + player 202 + parent 176, no dependents. Ran `upgrade()`. - `SELECT COUNT(*) FROM players WHERE name = 'Alice Uwamahoro'` -> `1` - Player 201 deleted, parent 175 deleted. - Player 202 still has `parent_id = 176`, parent 176 intact. 2. **Idempotency**: ran `upgrade()` a second time on the post-migration state. No errors, no-op. 3. **Guard**: seeded an `orders` row referencing player 201. `upgrade()` raised `RuntimeError: Refusing to delete player 201: dependent rows exist (player_teams=0, orders=1, registrations=0). Manual cleanup required before this migration can run.` as expected. 4. **Alembic chain**: `ScriptDirectory` walk confirms single head `039` chaining cleanly back through `038 -> 037 -> ... -> base`. 5. **Lint**: `ruff format` + `ruff check` clean. 6. **Full test suite**: `pytest tests/` — 891 passed. 4 pre-existing collection/import failures unrelated to this migration (`test_contract_reminder.py` + 3 `test_reconciliation.py` cases missing `groupme_sdk`) were observed; they fail identically on `main` without this change. Post-merge (not part of this PR): ArgoCD sync + `alembic upgrade head` against prod will execute the delete. ## Review Checklist - [x] Migration is idempotent (safe to re-run) - [x] Dependent-row guard in place for `player_teams` / `orders` / `registrations` - [x] `downgrade()` documented as no-op (deletion irreversible) - [x] Uses `op.execute()` + `sa.text()` with bound params, matching existing data-only migration style - [x] `Player` / `Parent` models untouched - [x] `ruff format` + `ruff check` clean - [x] Alembic single-head chain verified (`039 -> 038 -> ...`) - [x] Tested against sqlite sandbox (happy path + idempotency + guard) - [x] Scope limited to player 201 / parent 175; no other Alice rows touched ## Related Notes - Forgejo issue: Closes #420 - Board item: #925 on `board-westside-basketball` (APPROVED via `review-925-2026-04-10`) - Related but out of scope: #418 (Marcus dupe root cause), #419 (Test Queens Player symptom), #422 (separate dedupe that will chain as 040 on top of this 039)

forgejo_admin added 1 commit

2026-04-10 20:47:12 +00:00

fix: dedupe Alice Uwamahoro player rows (#420 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

fb6917d73e

Delete the stale Alice Uwamahoro submission (player id 201, parent id 175)
caused by a typo'd email, leaving the canonical rows (player 202, parent 176).

The migration is idempotent and refuses to run if player 201 has any
dependent rows in player_teams, orders, or registrations. downgrade() is a
no-op because the original row data is not retained.

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

forgejo_admin added 1 commit

2026-04-10 22:07:18 +00:00

fix: rebase migration to 042 over main chain (was 039, collided with existing)

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

216496241b

forgejo_admin commented

2026-04-10 22:08:44 +00:00

Author

Owner

PR #428 Review

DOMAIN REVIEW

Stack: Alembic data-only migration against Postgres (SQLAlchemy Core via op.get_bind() + sa.text()). Verified against the post-force-push diff.

Migration header (correct after rebase)

Filename: alembic/versions/042_dedupe_alice_uwamahoro.py
revision = "042", down_revision = "041"
Targets the expected merge order slot: #427 -> 040, #426 -> 041, #428 -> 042. Chain will still resolve if 427/426 swap because #428 only anchors to 041.

Dedupe logic

Deletes players.id = 201 (stale Alice submission from typo'd email).
Deletes parents.id = 175 (orphaned after player delete).
Idempotent: both deletes are guarded by SELECT 1 ... WHERE id = :pid pre-checks. Safe to re-run.
Dependent-row guard on player 201 against player_teams, orders, registrations — raises RuntimeError with per-table counts rather than silently orphaning. Correct behavior for contract_status='none' rows which should have zero dependents.
Extra parent guard: refuses to delete parent 175 if any player row still references it. Defensive and correct.
downgrade() is an explicit documented no-op (deletion is irreversible, no row data retained to reconstruct). Acceptable pattern for data-cleanup migrations.
All SQL uses bound params via sa.text() — no injection surface, matches style of existing data migrations like 035_set_player_contract_overrides.py.

Alembic / Postgres specifics

No schema DDL, so no transactional DDL concerns.
No cascade — explicit per-table dependency check is safer than relying on FK ON DELETE behavior and gives actionable error messages.
sa.text() + dict params portable across sqlite (used in test sandbox) and Postgres (prod). Verified.
Chain resolves to single head after merge per PR body's ScriptDirectory walk.

BLOCKERS

None.

NITS

PR body is stale: still describes the migration as 039_dedupe_alice_uwamahoro.py with down_revision 038. The code is correct (042/041) but the body should be updated for historical clarity. Non-blocking; route to plan Epilogue if desired.
Consider adding a one-line log / print before each delete so the alembic run output shows what was removed (helps post-deploy verification). Optional, not required.

SOP COMPLIANCE

Branch named after issue (420-dedupe-alice-uwamahoro)
PR body has Summary / Changes / Test Plan / Related
Related references board item #925 and parent issue #420
No secrets committed
Ruff format + check clean (per PR body)
Full test suite passed (891 passed; 4 pre-existing unrelated failures)
Scope limited to player 201 / parent 175 — no unrelated churn
Ticket passed scope review (review-925-2026-04-10 APPROVED)

PROCESS OBSERVATIONS

Merge-order dependency (#427 -> #426 -> #428) is explicit and documented. Since #428 anchors on down_revision = "041" (not a specific predecessor filename), it's robust to swap between #427 and #426 — but whichever PR ends up at 041 must merge first. Flag for merge coordinator, do not block.
Data-only migration with irreversible downgrade is worth flagging in the deploy runbook; post-merge validation should confirm SELECT COUNT(*) FROM players WHERE name = 'Alice Uwamahoro' returns 1 before closing #420.
PR-body staleness after force-push is a minor process gap — agent rebased code but not the descriptive text. Worth mentioning to the fix-up agent pattern so future rebases update both.

VERDICT: APPROVED

## PR #428 Review ### DOMAIN REVIEW Stack: Alembic data-only migration against Postgres (SQLAlchemy Core via `op.get_bind()` + `sa.text()`). Verified against the post-force-push diff. **Migration header (correct after rebase)** - Filename: `alembic/versions/042_dedupe_alice_uwamahoro.py` - `revision = "042"`, `down_revision = "041"` - Targets the expected merge order slot: #427 -> 040, #426 -> 041, #428 -> 042. Chain will still resolve if 427/426 swap because #428 only anchors to `041`. **Dedupe logic** - Deletes `players.id = 201` (stale Alice submission from typo'd email). - Deletes `parents.id = 175` (orphaned after player delete). - Idempotent: both deletes are guarded by `SELECT 1 ... WHERE id = :pid` pre-checks. Safe to re-run. - Dependent-row guard on player 201 against `player_teams`, `orders`, `registrations` — raises `RuntimeError` with per-table counts rather than silently orphaning. Correct behavior for `contract_status='none'` rows which should have zero dependents. - Extra parent guard: refuses to delete parent 175 if any player row still references it. Defensive and correct. - `downgrade()` is an explicit documented no-op (deletion is irreversible, no row data retained to reconstruct). Acceptable pattern for data-cleanup migrations. - All SQL uses bound params via `sa.text()` — no injection surface, matches style of existing data migrations like `035_set_player_contract_overrides.py`. **Alembic / Postgres specifics** - No schema DDL, so no transactional DDL concerns. - No cascade — explicit per-table dependency check is safer than relying on FK ON DELETE behavior and gives actionable error messages. - `sa.text()` + dict params portable across sqlite (used in test sandbox) and Postgres (prod). Verified. - Chain resolves to single head after merge per PR body's `ScriptDirectory` walk. ### BLOCKERS None. ### NITS - PR body is stale: still describes the migration as `039_dedupe_alice_uwamahoro.py` with `down_revision 038`. The code is correct (042/041) but the body should be updated for historical clarity. Non-blocking; route to plan Epilogue if desired. - Consider adding a one-line log / `print` before each delete so the alembic run output shows what was removed (helps post-deploy verification). Optional, not required. ### SOP COMPLIANCE - [x] Branch named after issue (`420-dedupe-alice-uwamahoro`) - [x] PR body has Summary / Changes / Test Plan / Related - [x] Related references board item #925 and parent issue #420 - [x] No secrets committed - [x] Ruff format + check clean (per PR body) - [x] Full test suite passed (891 passed; 4 pre-existing unrelated failures) - [x] Scope limited to player 201 / parent 175 — no unrelated churn - [x] Ticket passed scope review (review-925-2026-04-10 APPROVED) ### PROCESS OBSERVATIONS - Merge-order dependency (#427 -> #426 -> #428) is explicit and documented. Since #428 anchors on `down_revision = "041"` (not a specific predecessor filename), it's robust to swap between #427 and #426 — but whichever PR ends up at 041 must merge first. Flag for merge coordinator, do not block. - Data-only migration with irreversible downgrade is worth flagging in the deploy runbook; post-merge validation should confirm `SELECT COUNT(*) FROM players WHERE name = 'Alice Uwamahoro'` returns 1 before closing #420. - PR-body staleness after force-push is a minor process gap — agent rebased code but not the descriptive text. Worth mentioning to the fix-up agent pattern so future rebases update both. ### VERDICT: APPROVED