Added week_id to `game` model as a foreign key #418

Merged

johnsturgeon merged 2 commits from phase-3-game-week-id-foreign-key-409 into main

2026-08-23 09:43:32 +02:00

johnsturgeon commented

2026-08-23 09:11:45 +02:00

Owner

Add Game.week_id as a NOT NULL FK plus migration. One revision,
no backfill, while the tables are still empty.
create_the_picks sets week_id via get-or-create on Week, the
same way it already resolves teams.
Switch Game.games_for_week and Game.get_first_game_of_the_week
to filter on week_id.
Switch PlayerGamePick.find_picks_for_week and
upsert_award_with_args to the FK. Keep this separate from the task
above — different tables, different blast radius.

Resolves #409

* Add Game.week_id as a NOT NULL FK plus migration. One revision, no backfill, while the tables are still empty. * create_the_picks sets week_id via get-or-create on Week, the same way it already resolves teams. * Switch Game.games_for_week and Game.get_first_game_of_the_week to filter on week_id. * Switch PlayerGamePick.find_picks_for_week and upsert_award_with_args to the FK. Keep this separate from the task above — different tables, different blast radius. Resolves #409

johnsturgeon added 1 commit

2026-08-23 09:11:45 +02:00

Added week_id to game model as a foreign key

Claude Code Review / claude-review (pull_request) Successful in 2m10s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 31s

Details

Tests / pytest (pull_request) Successful in 14s

Details

d2a81c7240

* Add Game.week_id as a NOT NULL FK plus migration. One revision,
no backfill, while the tables are still empty.

* create_the_picks sets week_id via get-or-create on Week, the
same way it already resolves teams.

* Switch Game.games_for_week and Game.get_first_game_of_the_week
to filter on week_id.

* Switch PlayerGamePick.find_picks_for_week and
upsert_award_with_args to the FK. Keep this separate from the task
above — different tables, different blast radius.

forgejo-actions commented

2026-08-23 09:13:56 +02:00

I have enough to write the review.

Summary

This PR introduces a week_id FK on Game, migrates the week-scoped queries from the denormalized (season, season_type, week_no) triple to the FK, and replaces the Wednesday-cron create_picks job with an hourly sync_games_for_current_week upsert job. The model and query changes are coherent and the None-guards added around player.id/game.id are a real improvement, but there are a few gaps: an unmonitored set of jobs, a missing startup one-shot for the new sync job, and no test coverage for the new job or Week query methods.

Blocking

Confirm the game table is empty in every environment this migrates. alembic/versions/1cfb85d793c6_...py:26 adds week_id as NOT NULL with no server default and no backfill. Unlike Week.game_state (which got a server_default precisely because week already held rows — see app/models/week.py:34-41), this column will fail the migration with an integrity error if any game row exists. The PR description asserts the tables are still empty; that assumption is the whole safety of this migration, so it needs to be verified against prod before merge, not assumed.

Worth fixing

Awards, team-records, and schedule_jobs lost their monitoring with no replacement. The @monitor(...) decorators were removed from scheduled_update_all_awards (app/jobs/award_update_all.py), sync_the_team_records (app/jobs/sync_team_records.py), and schedule_jobs (app/jobs/scheduler.py), and the SENTRY_CRON_MONITOR_* config was deleted (app/config/config.py, config/op.env). Only the new sync_games_for_current_week got a Kuma monitor. So three previously-monitored recurring jobs now run with zero check-in coverage. If the intent is a Sentry→Kuma migration, the awards and team-records jobs need Kuma monitors too; otherwise a silent failure of the weekly award/records jobs is now invisible. Please confirm this is intentional.
No startup one-shot for sync_games_for_current_week. schedule_sync_games_for_current_week() (app/jobs/scheduler.py:167) registers only an IntervalTrigger(hours=1), whose first fire is ~1 hour out. sync_current_week deliberately pairs its interval with a run_date=now() startup job (app/main.py:136-143) precisely to populate an empty table immediately — the comment at app/main.py:124-128 spells this out. On a fresh-season database the picks page will show no games for up to an hour after boot. Consider adding the same one-shot date-trigger for the games sync.
No test coverage for the new job or the new Week methods. sync_games_for_current_week (including its create-vs-update branch at app/jobs/sync_games_for_current_week.py:81-95), Week.all_weeks_for_current_season, and Week.find_by_info are all untested. The upsert branch in particular carries the core behavior change of this PR and deserves a test (new game inserted with correct week_id; existing game updated in place).
PR description overstates the pick/award change. The description says it switches PlayerGamePick.find_picks_for_week and upsert_award_with_args to the FK, but app/models/player_game_pick.py:117-127 still filters on the denormalized week_no/season/season_type, and callers still pass week.info. That's fine as-is, but the description doesn't match the code — worth correcting so a future reader doesn't assume find_picks_for_week is FK-based.

Nits

sync_games_for_current_week matches existing games by external_game_id alone (app/jobs/sync_games_for_current_week.py:78), while the documented/enforced key is (data_source, external_game_id) (see uq_game_data_source_external_game_id in app/models/game.py:18-24). Harmless with a single provider today, but inconsistent with the model's key; adding .where(Game.data_source == DataSource.TANK01) would keep it honest. The trailing .limit(1) is also redundant given that unique constraint.
The update branch never refreshes week_id (app/jobs/sync_games_for_current_week.py:82-87), so a game whose week association is ever wrong won't self-correct on the next sync. Low-risk since a game's week doesn't change, but worth a comment if intentional.
Misleading log line: sync_games_for_current_week logs "Creating weekly picks page for ..." (app/jobs/sync_games_for_current_week.py:65-67) — copied from the deleted create_the_picks; it's now a game sync, not a picks-page creation.
Stale comment: app/jobs/scheduler.py:28 still references "the gap before create_the_picks fires Wednesday 6am PT," but that job was deleted in this PR.

I have enough to write the review. --- ## Summary This PR introduces a `week_id` FK on `Game`, migrates the week-scoped queries from the denormalized `(season, season_type, week_no)` triple to the FK, and replaces the Wednesday-cron `create_picks` job with an hourly `sync_games_for_current_week` upsert job. The model and query changes are coherent and the None-guards added around `player.id`/`game.id` are a real improvement, but there are a few gaps: an unmonitored set of jobs, a missing startup one-shot for the new sync job, and no test coverage for the new job or `Week` query methods. ## Blocking - **Confirm the `game` table is empty in every environment this migrates.** `alembic/versions/1cfb85d793c6_...py:26` adds `week_id` as `NOT NULL` with no server default and no backfill. Unlike `Week.game_state` (which got a `server_default` precisely because `week` already held rows — see `app/models/week.py:34-41`), this column will fail the migration with an integrity error if any `game` row exists. The PR description asserts the tables are still empty; that assumption is the whole safety of this migration, so it needs to be verified against prod before merge, not assumed. ## Worth fixing - **Awards, team-records, and `schedule_jobs` lost their monitoring with no replacement.** The `@monitor(...)` decorators were removed from `scheduled_update_all_awards` (`app/jobs/award_update_all.py`), `sync_the_team_records` (`app/jobs/sync_team_records.py`), and `schedule_jobs` (`app/jobs/scheduler.py`), and the `SENTRY_CRON_MONITOR_*` config was deleted (`app/config/config.py`, `config/op.env`). Only the *new* `sync_games_for_current_week` got a Kuma monitor. So three previously-monitored recurring jobs now run with zero check-in coverage. If the intent is a Sentry→Kuma migration, the awards and team-records jobs need Kuma monitors too; otherwise a silent failure of the weekly award/records jobs is now invisible. Please confirm this is intentional. - **No startup one-shot for `sync_games_for_current_week`.** `schedule_sync_games_for_current_week()` (`app/jobs/scheduler.py:167`) registers only an `IntervalTrigger(hours=1)`, whose first fire is ~1 hour out. `sync_current_week` deliberately pairs its interval with a `run_date=now()` startup job (`app/main.py:136-143`) precisely to populate an empty table immediately — the comment at `app/main.py:124-128` spells this out. On a fresh-season database the picks page will show no games for up to an hour after boot. Consider adding the same one-shot date-trigger for the games sync. - **No test coverage for the new job or the new `Week` methods.** `sync_games_for_current_week` (including its create-vs-update branch at `app/jobs/sync_games_for_current_week.py:81-95`), `Week.all_weeks_for_current_season`, and `Week.find_by_info` are all untested. The upsert branch in particular carries the core behavior change of this PR and deserves a test (new game inserted with correct `week_id`; existing game updated in place). - **PR description overstates the pick/award change.** The description says it switches `PlayerGamePick.find_picks_for_week` and `upsert_award_with_args` to the FK, but `app/models/player_game_pick.py:117-127` still filters on the denormalized `week_no`/`season`/`season_type`, and callers still pass `week.info`. That's fine as-is, but the description doesn't match the code — worth correcting so a future reader doesn't assume `find_picks_for_week` is FK-based. ## Nits - **`sync_games_for_current_week` matches existing games by `external_game_id` alone** (`app/jobs/sync_games_for_current_week.py:78`), while the documented/enforced key is `(data_source, external_game_id)` (see `uq_game_data_source_external_game_id` in `app/models/game.py:18-24`). Harmless with a single provider today, but inconsistent with the model's key; adding `.where(Game.data_source == DataSource.TANK01)` would keep it honest. The trailing `.limit(1)` is also redundant given that unique constraint. - **The update branch never refreshes `week_id`** (`app/jobs/sync_games_for_current_week.py:82-87`), so a game whose week association is ever wrong won't self-correct on the next sync. Low-risk since a game's week doesn't change, but worth a comment if intentional. - **Misleading log line:** `sync_games_for_current_week` logs `"Creating weekly picks page for ..."` (`app/jobs/sync_games_for_current_week.py:65-67`) — copied from the deleted `create_the_picks`; it's now a game sync, not a picks-page creation. - **Stale comment:** `app/jobs/scheduler.py:28` still references "the gap before `create_the_picks` fires Wednesday 6am PT," but that job was deleted in this PR.

johnsturgeon added 1 commit

2026-08-23 09:22:15 +02:00

Fixed Claude review's concerns

Claude Code Review / claude-review (pull_request) Successful in 2m22s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 23s

Details

Tests / pytest (pull_request) Successful in 10s

Details

560fdf1189

johnsturgeon merged commit abbeada064 into main

2026-08-23 09:43:32 +02:00