johnsturgeon/tgfp-web

Watch

Fork 0

This is the end of Phase II - the week table now has the attributes to handle state of the week #415

Merged

johnsturgeon merged 3 commits from phase-2-week-state-machine-408 into main

2026-08-22 16:26:14 +02:00

johnsturgeon commented

2026-08-22 15:42:58 +02:00

Owner

Review task #408 for detail of what was done

Resolves #408

Review task #408 for detail of what was done Resolves #408

johnsturgeon added 1 commit

2026-08-22 15:42:59 +02:00

This is the end of Phase II - the week table now has the attributes to handle state of the week

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

Details

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

Details

Tests / pytest (pull_request) Successful in 16s

Details

2c221b929c

forgejo-actions commented

2026-08-22 15:45:33 +02:00

This is the end of Phase II schema work; I've verified the migration chain (linear, 6f69813c34af is the current head), the status‑vocabulary cutover, and the new tests. Overall the change is clean and unusually well‑documented, but there's one silent‑correctness risk around the game_status value change worth confirming before merge.

Review

The PR renames the game_status vocabulary (STATUS_FINAL → FINAL, etc.) via the new GameStatus StrEnum, adds nullable odds columns to game, adds game_state/odds_state to week, and pivots Week.current() to return the row (callers now use .info). The test scaffolding (in‑memory SQLite fixture, make_game/make_week_of, op.env‑driven env seeding) is a genuinely good addition. Two things are worth a hard look and a few minor items follow.

Blocking

No data migration for the changed game_status vocabulary — existing rows silently mis‑classify. app/models/game.py:80-92 and app/tank01_api/tank01_api.py:31-37 now emit/compare bare values ("FINAL", "SCHEDULED"), but game.game_status is a plain string column and any row already stored with the old STATUS_* spelling is not rewritten by d183ea00f22f. New writes via update_game.py:44 / create_picks.py:52 are fine, but a game that already reached STATUS_FINAL and is no longer polled will read is_final == False forever — which flows into award recalculation and the _get_current_week_state fold (a stale STATUS_FINAL is neither final nor cancelled, so a truly‑done week never reaches ALL_FINAL). The c3f1a5d84b20 cutover truncated game, so this only bites if any games have been created since (it is preseason now, and create_picks runs weekly). Please confirm the game table is empty in prod, or add an UPDATE game SET game_status = ... step to the migration. It's a cheap fix for a failure that is invisible until standings look wrong.

Worth fixing

assert used for control‑flow validation in _get_current_week_state (app/jobs/sync_week_state_from_game_status.py:26-31). Running under python -O strips it, and it's logically always true given the preceding filter, so it buys nothing at runtime while reading as a real guard. Either drop it or turn it into an explicit raise if you actually want to catch an unexpected status leaking through.
sync_week_state_from_game_status() (the DB‑writing wrapper) is untested and unwired. Tests cover the pure _get_current_week_state thoroughly, but the function that reads Week.current, loads games, and commits game_state has no coverage, and nothing schedules it yet (grep shows it only in its own file + tests). If wiring is deferred to a later phase that's fine — but the commit/persist path is exactly the part the pure‑function tests can't protect, and it's easy to add now with the new session fixture.

Nits

Picks template now assumes a favorite that the model no longer guarantees. favorite_team_id is nullable and underdog_team correctly returns None, but app/templates/picks.j2:48 still does game.favorite_team.long_name / game.spread with no guard. A line‑less game reaching the picks page renders a blank favorite / None spread. The design notes say the LOCKED transition will guarantee non‑null before the page is released — that gate isn't in this PR, so until it lands the template is a step ahead of its invariant.
now() is shared as a single element instance across both created_at and updated_at (and as server_default + onupdate) in app/models/base.py:26-34. SQLAlchemy tolerates this for function elements, so no action needed — just noting it in case columns get copied around later.
odds_state / OddsState.LOCKED are modeled but never written anywhere yet — expected for Phase II, flagging only so it's a conscious deferral rather than a forgotten wire‑up.

Nice touch: test_a_week_of_nothing_but_postponements_reads_as_pending documenting a knowingly‑unhandled case so it stays a decision rather than a rediscovered bug.

This is the end of Phase II schema work; I've verified the migration chain (linear, `6f69813c34af` is the current head), the status‑vocabulary cutover, and the new tests. Overall the change is clean and unusually well‑documented, but there's one silent‑correctness risk around the `game_status` value change worth confirming before merge. --- ## Review The PR renames the `game_status` vocabulary (`STATUS_FINAL` → `FINAL`, etc.) via the new `GameStatus` StrEnum, adds nullable odds columns to `game`, adds `game_state`/`odds_state` to `week`, and pivots `Week.current()` to return the row (callers now use `.info`). The test scaffolding (in‑memory SQLite fixture, `make_game`/`make_week_of`, op.env‑driven env seeding) is a genuinely good addition. Two things are worth a hard look and a few minor items follow. ### Blocking - **No data migration for the changed `game_status` vocabulary — existing rows silently mis‑classify.** `app/models/game.py:80-92` and `app/tank01_api/tank01_api.py:31-37` now emit/compare bare values (`"FINAL"`, `"SCHEDULED"`), but `game.game_status` is a plain string column and any row already stored with the old `STATUS_*` spelling is not rewritten by `d183ea00f22f`. New writes via `update_game.py:44` / `create_picks.py:52` are fine, but a game that already reached `STATUS_FINAL` and is no longer polled will read `is_final == False` forever — which flows into award recalculation and the `_get_current_week_state` fold (a stale `STATUS_FINAL` is neither final nor cancelled, so a truly‑done week never reaches `ALL_FINAL`). The `c3f1a5d84b20` cutover truncated `game`, so this only bites if any games have been created since (it is preseason now, and `create_picks` runs weekly). Please confirm the `game` table is empty in prod, or add an `UPDATE game SET game_status = ...` step to the migration. It's a cheap fix for a failure that is invisible until standings look wrong. ### Worth fixing - **`assert` used for control‑flow validation in `_get_current_week_state`** (`app/jobs/sync_week_state_from_game_status.py:26-31`). Running under `python -O` strips it, and it's logically always true given the preceding filter, so it buys nothing at runtime while reading as a real guard. Either drop it or turn it into an explicit raise if you actually want to catch an unexpected status leaking through. - **`sync_week_state_from_game_status()` (the DB‑writing wrapper) is untested and unwired.** Tests cover the pure `_get_current_week_state` thoroughly, but the function that reads `Week.current`, loads games, and commits `game_state` has no coverage, and nothing schedules it yet (`grep` shows it only in its own file + tests). If wiring is deferred to a later phase that's fine — but the commit/persist path is exactly the part the pure‑function tests can't protect, and it's easy to add now with the new `session` fixture. ### Nits - **Picks template now assumes a favorite that the model no longer guarantees.** `favorite_team_id` is nullable and `underdog_team` correctly returns `None`, but `app/templates/picks.j2:48` still does `game.favorite_team.long_name` / `game.spread` with no guard. A line‑less game reaching the picks page renders a blank favorite / `None` spread. The design notes say the `LOCKED` transition will guarantee non‑null before the page is released — that gate isn't in this PR, so until it lands the template is a step ahead of its invariant. - `now()` is shared as a single element instance across both `created_at` and `updated_at` (and as `server_default` + `onupdate`) in `app/models/base.py:26-34`. SQLAlchemy tolerates this for function elements, so no action needed — just noting it in case columns get copied around later. - `odds_state` / `OddsState.LOCKED` are modeled but never written anywhere yet — expected for Phase II, flagging only so it's a conscious deferral rather than a forgotten wire‑up. Nice touch: `test_a_week_of_nothing_but_postponements_reads_as_pending` documenting a knowingly‑unhandled case so it stays a decision rather than a rediscovered bug.

johnsturgeon added 1 commit

2026-08-22 15:59:36 +02:00

Added in tests, and bumped the version

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

Details

Python FastAPI Jinja Linting / build (pull_request) Failing after 22s

Details

Tests / pytest (pull_request) Successful in 12s

Details

fe92a182a6

johnsturgeon added 1 commit

2026-08-22 16:20:50 +02:00

Share one Game factory between the two test modules

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

Details

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

Details

Tests / pytest (pull_request) Successful in 11s

Details

5b23db85bc

pylint R0801 flagged the eleven-field Game construction duplicated across
tests/test_week_state.py and tests/test_game_model.py. Fair catch: none of
those fields below game_status is ever the subject of a test, so it was
pure scaffolding copied twice.

Moved to conftest as make_game()/make_week_of(), which is the arrangement
pyproject.toml already sets up ("Repo root on sys.path so tests can import
tests.conftest helpers") and how load_fixture is already shared. Call sites
now write only the fields they assert on, and a new required column on Game
breaks one place instead of two.

Worth knowing: scripts/test_and_lint.sh lints $(git ls-files '*.py'), so it
never saw either file while they were untracked and reported 10.00/10
throughout. R0801 only surfaced when pylint was pointed at them directly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

johnsturgeon merged commit 30db86617c into main

2026-08-22 16:26:14 +02:00

johnsturgeon deleted branch phase-2-week-state-machine-408

2026-08-22 16:26:14 +02:00

johnsturgeon referenced this pull request from a commit

2026-08-22 16:26:15 +02:00

This is the end of Phase II - the week table now has the attributes to handle state of the week (#415)