We now log what happened, and raise #449

Merged

johnsturgeon merged 1 commit from picks-form-swallows-every-integrityerror-as-a-duplicate-submission-438 into main

2026-09-02 15:02:43 +02:00

johnsturgeon commented

2026-09-02 14:54:19 +02:00

Owner

Resolves #438

johnsturgeon added 1 commit

2026-09-02 14:54:19 +02:00

We now log what happened, and raise

Claude Code Review / claude-review (pull_request) Successful in 3m39s

Details

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

Details

Tests / pytest (pull_request) Successful in 12s

Details

816e5ec7c2

Resolves #438

forgejo-actions commented

2026-09-02 14:57:58 +02:00

I now have a complete picture. Let me write the review.

Review

This is a small, well-targeted change that replaces a silently-swallowed IntegrityError with a "log-and-raise for genuine failures, render success only for a real concurrent-submission race" split, resolving #438. The logic is correct and matches the intent; my only substantive concern is that the new behavior is entirely untested, plus a couple of nits.

Worth fixing

No test covers either new branch (app/main.py:441-449). The route's except IntegrityError branch is effectively unreachable through validated form input — a submitted pick for a started game is rejected earlier by _picks_not_valid (app/main.py:207), and malformed values are zeroed by _form_int's .isdigit() check (app/main.py:354) and caught as missed picks, so TestAMalformedSubmission and TestAStaleForm never actually reach the commit failure their docstrings describe. That means the new raise path and the benign-race fall-through are both new, load-bearing behavior with zero coverage. Consider a test that forces the failure (e.g. monkeypatch session.commit to raise IntegrityError, or seed a conflicting row) and asserts: (a) with no existing picks → the request raises / returns 500, and (b) with picks already present for the week → the success page renders and nothing is re-raised. While there, the stale docstrings on TestAMalformedSubmission / TestAStaleForm claiming they exercise the IntegrityError catch are now doubly misleading and could be corrected.

Nits

Log uses the raw integer season type (app/main.py:447). week.season_type renders as e.g. 2; Week.season_type_name (app/models/week.py:50) exists and yields a readable label (pre/reg/post). Using it would make the Sentry message self-explanatory.
Possible double signal in Sentry (app/main.py:445-449). With enable_logs=True the sentry_logger.error(...) sends a structured log, and the subsequent raise will also surface the IntegrityError as an event via the ASGI integration. That's harmless and arguably intentional (the log adds the player/week context the bare exception lacks), just worth being aware of.

The version bump to 2.21.1 and the psycopg 3.3.4 → 3.3.5 patch (with a consistent uv.lock update) follow the project conventions correctly.

I now have a complete picture. Let me write the review. ## Review This is a small, well-targeted change that replaces a silently-swallowed `IntegrityError` with a "log-and-raise for genuine failures, render success only for a real concurrent-submission race" split, resolving #438. The logic is correct and matches the intent; my only substantive concern is that the new behavior is entirely untested, plus a couple of nits. ### Worth fixing - **No test covers either new branch** (`app/main.py:441-449`). The route's `except IntegrityError` branch is effectively unreachable through validated form input — a submitted pick for a started game is rejected earlier by `_picks_not_valid` (`app/main.py:207`), and malformed values are zeroed by `_form_int`'s `.isdigit()` check (`app/main.py:354`) and caught as missed picks, so `TestAMalformedSubmission` and `TestAStaleForm` never actually reach the commit failure their docstrings describe. That means the new `raise` path and the benign-race fall-through are both new, load-bearing behavior with zero coverage. Consider a test that forces the failure (e.g. monkeypatch `session.commit` to raise `IntegrityError`, or seed a conflicting row) and asserts: (a) with no existing picks → the request raises / returns 500, and (b) with picks already present for the week → the success page renders and nothing is re-raised. While there, the stale docstrings on `TestAMalformedSubmission` / `TestAStaleForm` claiming they exercise the `IntegrityError` catch are now doubly misleading and could be corrected. ### Nits - **Log uses the raw integer season type** (`app/main.py:447`). `week.season_type` renders as e.g. `2`; `Week.season_type_name` (`app/models/week.py:50`) exists and yields a readable label (`pre`/`reg`/`post`). Using it would make the Sentry message self-explanatory. - **Possible double signal in Sentry** (`app/main.py:445-449`). With `enable_logs=True` the `sentry_logger.error(...)` sends a structured log, and the subsequent `raise` will also surface the `IntegrityError` as an event via the ASGI integration. That's harmless and arguably intentional (the log adds the player/week context the bare exception lacks), just worth being aware of. The version bump to `2.21.1` and the `psycopg` `3.3.4 → 3.3.5` patch (with a consistent `uv.lock` update) follow the project conventions correctly.

johnsturgeon merged commit 1b316f5272 into main

2026-09-02 15:02:43 +02:00

johnsturgeon deleted branch picks-form-swallows-every-integrityerror-as-a-duplicate-submission-438

2026-09-02 15:02:44 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-02 15:02:45 +02:00

Merge pull request 'We now log what happened, and raise' (#449) from picks-form-swallows-every-integrityerror-as-a-duplicate-submission-438 into main