johnsturgeon/tgfp-web

Watch

Fork 0

Gate the picks page and form on the odds being locked #440

Merged

johnsturgeon merged 1 commit from fix-picks-page-to-only-show-if-the-odds-are-locked-for-that-week-424 into main

2026-09-01 18:47:14 +02:00

johnsturgeon commented

2026-09-01 18:31:22 +02:00

Owner

Closes the last gap between what the picks page shows and what the picks form
accepts.

The page

Refuses when the odds are still PRELIMINARY, when the week is ALL_FINAL,
when the player has already picked, and -- new -- when every game has kicked
off, which would otherwise submit a full slate of home teams nobody chose.

The form

Every rule the page applies at render time is re-checked at submit time. The
threat is not an adversary, it is a stale tab: open the page, make a sandwich,
submit into a world that has moved.

Started games no longer depend on JavaScript. Their radios render disabled and
send no key, so the absence of a key is the evidence:

key in form	game started	outcome
yes	no	the player's pick
yes	yes	kicked off while they were deciding -- refused
no	yes	server fills in the home team
no	no	they missed a pick

Generated picks can be neither a lock nor an upset.

Upsets are now checked against the spread instead of trusting the dropdown, and
a value that is not a team id is treated as a missed pick rather than becoming
a picked_team_id of 0.

Player lookup

All eight routes resolve their player through get_player. Six were still on
verify_player plus a manual lookup that 404s -- the dead end get_player was
written in #403 to avoid, applied then to only two routes.

Tests

First route tests in the repo: TestClient plus a get_session dependency
override onto the existing in-memory session fixture. Constructed rather than
entered as a context manager, since __enter__ runs the lifespan and starts
APScheduler against the real engine.

23 tests over the page, the form, stale submissions, malformed values, the lock
rules and postponed games. Several were written failing first and caught real
defects on the way: three guards that built a response and discarded it, an
upset check comparing a game id to a team id, and two paths where a submission
was rolled back by a swallowed IntegrityError while the success page
rendered.

resolves #424

Closes the last gap between what the picks page shows and what the picks form accepts. ## The page Refuses when the odds are still `PRELIMINARY`, when the week is `ALL_FINAL`, when the player has already picked, and -- new -- when every game has kicked off, which would otherwise submit a full slate of home teams nobody chose. ## The form Every rule the page applies at render time is re-checked at submit time. The threat is not an adversary, it is a stale tab: open the page, make a sandwich, submit into a world that has moved. Started games no longer depend on JavaScript. Their radios render disabled and send no key, so the absence of a key *is* the evidence: | key in form | game started | outcome | |---|---|---| | yes | no | the player's pick | | yes | yes | kicked off while they were deciding -- refused | | no | yes | server fills in the home team | | no | no | they missed a pick | Generated picks can be neither a lock nor an upset. Upsets are now checked against the spread instead of trusting the dropdown, and a value that is not a team id is treated as a missed pick rather than becoming a `picked_team_id` of 0. ## Player lookup All eight routes resolve their player through `get_player`. Six were still on `verify_player` plus a manual lookup that 404s -- the dead end `get_player` was written in #403 to avoid, applied then to only two routes. ## Tests First route tests in the repo: `TestClient` plus a `get_session` dependency override onto the existing in-memory session fixture. Constructed rather than entered as a context manager, since `__enter__` runs the lifespan and starts APScheduler against the real engine. 23 tests over the page, the form, stale submissions, malformed values, the lock rules and postponed games. Several were written failing first and caught real defects on the way: three guards that built a response and discarded it, an upset check comparing a game id to a team id, and two paths where a submission was rolled back by a swallowed `IntegrityError` while the success page rendered. resolves #424

johnsturgeon added 1 commit

2026-09-01 18:31:22 +02:00

Gate the picks page and form on the odds being locked

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

Details

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

Details

Tests / pytest (pull_request) Successful in 24s

Details

53a125e7ca

The page rendered against preliminary spreads, and the form behind it enforced
nothing at all -- a stale tab was enough to book a pick against a number that
had since moved. Both now check the same things, and the form checks them
against the state at submit time rather than at render time.

Started games no longer rely on JavaScript. Their radios render disabled and
send no key, so the absence of a key is itself evidence the game had already
kicked off, and the server fills in the home team. A key that *does* arrive for
a started game means the page was built before kickoff, and the submission is
refused.

Upsets are checked against the spread rather than trusting the dropdown, and
every route now resolves its player through `get_player`, so a cookie pointing
at a deleted account redirects to /login instead of dead-ending on a 404.

Also hoists the score-poll cadence to SCORE_POLL_MINUTES.

Code by John. Tests by Claude Opus 5, directed and reviewed by John.

resolves #424

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

forgejo-actions commented

2026-09-01 18:34:20 +02:00

This is a careful, well-tested PR. I've verified the dependency chain (get_player/verify_player), the state enums, is_pregame, the template's disabled-radio mechanism, and the form/page validation paths. My assessment:

This PR closes the render-time/submit-time gap on the picks flow cleanly: every gate the page enforces (odds locked, week not over, nothing-left-to-pick, already-picked) is re-checked in picks_form, and the disabled-radio → absent-key → server-fills-home-team mechanism is sound and well pinned by tests. The refactor to route all eight handlers through get_player, the id-vs-team-id hardening (_form_int), and the first route-level test suite are all solid — I found no correctness or security bugs.

The tests are unusually thorough: the stale-form, malformed-value, upset-namespace-collision, and postponed-game cases each target a distinct real defect and the reasoning in the docstrings holds up against the implementation.

Nits

app/main.py:425 — the error-message path returns the error_picks.j2 response without an explicit session.rollback(), even though rows were already session.add()ed in the loop at app/main.py:422. It's harmless because get_session's with Session(...) discards uncommitted state on teardown, but it's inconsistent with the already-picked path at app/main.py:407, which rolls back explicitly. An explicit session.rollback() before returning would make the "nothing is written" guarantee local to the handler rather than dependent on the dependency's cleanup.
app/main.py:382 — _append_auto_picks doesn't guard game.id is None, whereas both _pg_pick_from_form (app/main.py:368) and the main form loop (app/main.py:417) do. Persisted games always have an id so this is theoretical, but the inconsistency is a small readability wart.
app/main.py:200-209 — _missed_a_pick (strict subset <) and _picks_not_valid (!=) each rebuild both id sets, and the latter subsumes the former. Fine as-is since they emit different messages and _missed_a_pick runs first, but a single set computation shared between them would be marginally clearer.
picks_form is async def but does synchronous SQLAlchemy work (games_for_week, commit) that blocks the event loop, and /picks lazy-loads home_team/away_team/underdog_team/favorite_team per game (a modest N+1 in the template). Both are pre-existing and not introduced here — worth a follow-up if this pool ever grows, but not blocking.

This is a careful, well-tested PR. I've verified the dependency chain (`get_player`/`verify_player`), the state enums, `is_pregame`, the template's disabled-radio mechanism, and the form/page validation paths. My assessment: --- This PR closes the render-time/submit-time gap on the picks flow cleanly: every gate the page enforces (odds locked, week not over, nothing-left-to-pick, already-picked) is re-checked in `picks_form`, and the disabled-radio → absent-key → server-fills-home-team mechanism is sound and well pinned by tests. The refactor to route all eight handlers through `get_player`, the id-vs-team-id hardening (`_form_int`), and the first route-level test suite are all solid — I found no correctness or security bugs. The tests are unusually thorough: the stale-form, malformed-value, upset-namespace-collision, and postponed-game cases each target a distinct real defect and the reasoning in the docstrings holds up against the implementation. ### Nits - `app/main.py:425` — the error-message path returns the `error_picks.j2` response without an explicit `session.rollback()`, even though rows were already `session.add()`ed in the loop at `app/main.py:422`. It's harmless because `get_session`'s `with Session(...)` discards uncommitted state on teardown, but it's inconsistent with the already-picked path at `app/main.py:407`, which rolls back explicitly. An explicit `session.rollback()` before returning would make the "nothing is written" guarantee local to the handler rather than dependent on the dependency's cleanup. - `app/main.py:382` — `_append_auto_picks` doesn't guard `game.id is None`, whereas both `_pg_pick_from_form` (`app/main.py:368`) and the main form loop (`app/main.py:417`) do. Persisted games always have an id so this is theoretical, but the inconsistency is a small readability wart. - `app/main.py:200-209` — `_missed_a_pick` (strict subset `<`) and `_picks_not_valid` (`!=`) each rebuild both id sets, and the latter subsumes the former. Fine as-is since they emit different messages and `_missed_a_pick` runs first, but a single set computation shared between them would be marginally clearer. - `picks_form` is `async def` but does synchronous SQLAlchemy work (`games_for_week`, `commit`) that blocks the event loop, and `/picks` lazy-loads `home_team`/`away_team`/`underdog_team`/`favorite_team` per game (a modest N+1 in the template). Both are pre-existing and not introduced here — worth a follow-up if this pool ever grows, but not blocking.

johnsturgeon merged commit 3cbc55603e into main

2026-09-01 18:47:14 +02:00

johnsturgeon deleted branch fix-picks-page-to-only-show-if-the-odds-are-locked-for-that-week-424

2026-09-01 18:47:14 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-01 18:47:14 +02:00

Gate the picks page and form on the odds being locked (#440)

johnsturgeon referenced this pull request from a commit

2026-09-01 18:51:11 +02:00

Sweep TECH_DEBT entries the control loop and #440 resolved

johnsturgeon referenced this pull request

2026-09-01 18:51:28 +02:00