johnsturgeon/tgfp-web

Watch

Fork 0

Email the pool when the picks page opens #457

Merged

johnsturgeon merged 2 commits from re-automate-the-picks-page-is-ready-email-209 into main

2026-09-02 21:19:22 +02:00

johnsturgeon commented

2026-09-02 21:00:57 +02:00

Owner

The Discord announcement went out on the odds lock but the email never did, so
anyone not in the Discord found out by checking the site.

Same transition, two channels

Rule 4 already dispatched the Discord announce; launch_announce_picks_open
now schedules both. Separate job ids -- announce_picks_page_is_open_discord
and ..._email -- so one failing leaves its own jobstate row and does not
hide the other.

Still exactly once per week, because the reconciler that flips odds_state
falsifies its own condition.

Layering

send_picks_page_ready_email(first_name, recipients) takes the same shape as
send_welcome_email. The job owns the session and the loop over active
players:

    with Session(engine) as session:
        for player in Player.active_players(session=session):
            asyncio.run(
                send_picks_page_ready_email(player.first_name, [player.email])
            )

That keeps app/mailer.py free of the database -- no Session, no engine,
no Player -- and puts the session on the entry point, where the rest of the
codebase keeps it. Same split as #453: the mailer knows how to send, the caller
knows who to send to.

asyncio.run at the job boundary because fastapi_mail is async and
APScheduler runs jobs in a thread pool.

Template

email_picks_page_ready.j2 was one line; it now matches the welcome mail's
structure and links to the picks page. Its URLs are hardcoded for now, the same
way the nag bot and the Discord announce already do -- #456 collects all of
them behind SITE_BASE_URL.

Tests

The two lock tests now assert both dispatches, in order, which pins that the
lock announces on both channels exactly once.

284 passing.

resolves #209

🤖 Generated with Claude Code

The Discord announcement went out on the odds lock but the email never did, so anyone not in the Discord found out by checking the site. ## Same transition, two channels Rule 4 already dispatched the Discord announce; `launch_announce_picks_open` now schedules both. Separate job ids -- `announce_picks_page_is_open_discord` and `..._email` -- so one failing leaves its own `jobstate` row and does not hide the other. Still exactly once per week, because the reconciler that flips `odds_state` falsifies its own condition. ## Layering `send_picks_page_ready_email(first_name, recipients)` takes the same shape as `send_welcome_email`. The job owns the session and the loop over active players: ```python with Session(engine) as session: for player in Player.active_players(session=session): asyncio.run( send_picks_page_ready_email(player.first_name, [player.email]) ) ``` That keeps `app/mailer.py` free of the database -- no `Session`, no `engine`, no `Player` -- and puts the session on the entry point, where the rest of the codebase keeps it. Same split as #453: the mailer knows how to send, the caller knows who to send to. `asyncio.run` at the job boundary because `fastapi_mail` is async and APScheduler runs jobs in a thread pool. ## Template `email_picks_page_ready.j2` was one line; it now matches the welcome mail's structure and links to the picks page. Its URLs are hardcoded for now, the same way the nag bot and the Discord announce already do -- #456 collects all of them behind `SITE_BASE_URL`. ## Tests The two lock tests now assert both dispatches, in order, which pins that the lock announces on both channels exactly once. 284 passing. resolves #209 🤖 Generated with [Claude Code](https://claude.com/claude-code)

johnsturgeon added 1 commit

2026-09-02 21:00:57 +02:00

Email the pool when the picks page opens

Claude Code Review / claude-review (pull_request) Successful in 1m15s

Details

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

Details

Tests / pytest (pull_request) Successful in 30s

Details

8b36a9b9c1

The Discord announcement went out on the odds lock but the email never did, so
anyone not in the Discord found out by checking.

Same transition, so the same launcher: locking the odds now schedules both, on
separate job ids so one failing does not hide the other.

`send_picks_page_ready_email` takes a name and recipients like its sibling. The
job owns the session and the loop over active players, which keeps the mailer
free of the database and the session on the entry point.

The template grows into the same shape as the welcome mail, and hardcodes its
links -- #456 collects those.

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

resolves #209

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

forgejo-actions commented

2026-09-02 21:02:13 +02:00

This is a clean, well-scoped change. I read the actual files (app/jobs/announce_picks_page.py, app/mailer.py, app/models/player.py) plus the dispatcher test wiring to confirm the behavior.

This PR adds an email announcement alongside the existing Discord one when the picks page opens, splitting them into two independent scheduler jobs so a failure in one doesn't hide the other. The layering matches the #453 convention (mailer stays DB-free, the job owns the session), the per-player error handling keeps one bad address from stranding the rest of the roster, and the test coverage is genuinely good.

The change looks good to merge. A couple of small observations, none blocking.

Nits

app/jobs/announce_picks_page.py:45-47 — the session is closed before the players are iterated, so the emails are sent against detached ORM instances. This is safe here (the accessed fields — first_name, email, nick_name — are plain columns already loaded by active_players, and there's no commit() to expire them), so no DetachedInstanceError. Worth being aware of if a future edit ever touches a relationship or lazy-loaded attribute inside _email_every_active_player; at that point the with block would need to wrap the loop.
The PR description's code snippet shows asyncio.run(...) inside the per-player loop, but the merged code correctly wraps the whole roster in a single asyncio.run(_email_every_active_player(...)) (and test_one_event_loop_for_the_whole_roster pins exactly that). Just a stale description — the code is the better version.
announce_picks_page_to_discord still has no error handling, so a webhook failure fails that job (leaving its own jobstate row). That's consistent with the PR's stated "separate job ids, one failing doesn't hide the other" design and is pre-existing, so it's fine — noting only that the two channels are now asymmetric: the email job can never fail the whole job (all per-player exceptions swallowed), while the Discord job can. That asymmetry is intentional and reasonable.

Test coverage

Coverage is solid: active-only filtering, empty roster, single-event-loop, and the partial-failure resilience case are all exercised in tests/test_announce_picks_page.py, and tests/test_dispatcher.py confirms both job ids are dispatched in order on the lock transition (the dispatched fixture patches add_job, so launch_announce_picks_open adding both jobs is genuinely exercised). No gaps worth blocking on — the untested pieces (announce_picks_page_to_discord, the .j2 render) were untested before this PR too.

This is a clean, well-scoped change. I read the actual files (`app/jobs/announce_picks_page.py`, `app/mailer.py`, `app/models/player.py`) plus the dispatcher test wiring to confirm the behavior. --- This PR adds an email announcement alongside the existing Discord one when the picks page opens, splitting them into two independent scheduler jobs so a failure in one doesn't hide the other. The layering matches the `#453` convention (mailer stays DB-free, the job owns the session), the per-player error handling keeps one bad address from stranding the rest of the roster, and the test coverage is genuinely good. The change looks good to merge. A couple of small observations, none blocking. ## Nits - `app/jobs/announce_picks_page.py:45-47` — the session is closed before the players are iterated, so the emails are sent against detached ORM instances. This is safe here (the accessed fields — `first_name`, `email`, `nick_name` — are plain columns already loaded by `active_players`, and there's no `commit()` to expire them), so no `DetachedInstanceError`. Worth being aware of if a future edit ever touches a relationship or lazy-loaded attribute inside `_email_every_active_player`; at that point the `with` block would need to wrap the loop. - The PR description's code snippet shows `asyncio.run(...)` *inside* the per-player loop, but the merged code correctly wraps the whole roster in a single `asyncio.run(_email_every_active_player(...))` (and `test_one_event_loop_for_the_whole_roster` pins exactly that). Just a stale description — the code is the better version. - `announce_picks_page_to_discord` still has no error handling, so a webhook failure fails that job (leaving its own `jobstate` row). That's consistent with the PR's stated "separate job ids, one failing doesn't hide the other" design and is pre-existing, so it's fine — noting only that the two channels are now asymmetric: the email job can never fail the whole job (all per-player exceptions swallowed), while the Discord job can. That asymmetry is intentional and reasonable. ## Test coverage Coverage is solid: active-only filtering, empty roster, single-event-loop, and the partial-failure resilience case are all exercised in `tests/test_announce_picks_page.py`, and `tests/test_dispatcher.py` confirms both job ids are dispatched in order on the lock transition (the `dispatched` fixture patches `add_job`, so `launch_announce_picks_open` adding both jobs is genuinely exercised). No gaps worth blocking on — the untested pieces (`announce_picks_page_to_discord`, the `.j2` render) were untested before this PR too.

johnsturgeon referenced this pull request from a commit

2026-09-02 21:12:12 +02:00

Keep emailing the roster when one address fails

johnsturgeon added 1 commit

2026-09-02 21:12:12 +02:00

Keep emailing the roster when one address fails

Claude Code Review / claude-review (pull_request) Successful in 1m18s

Details

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

Details

Tests / pytest (pull_request) Successful in 14s

Details

104e5ee389

Review feedback on #457.

The send loop had no isolation, and this job runs once a week: rule 4 has
already falsified its own condition by the time it starts, so a raise partway
through left everyone after the bad address with nothing and nothing to retry
it. Each send is now guarded and logged.

Reading the roster and sending it are separated, so one event loop covers the
whole pool rather than one per player. The SMTP connection count is unchanged
-- `_send` still builds a `FastMail` per message.

Tests cover the loop itself, which nothing did: who gets mail, who does not,
and that a refused recipient is one player's problem.

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

johnsturgeon merged commit 239813c111 into main

2026-09-02 21:19:22 +02:00

johnsturgeon deleted branch re-automate-the-picks-page-is-ready-email-209

2026-09-02 21:19:22 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-02 21:19:23 +02:00

Email the pool when the picks page opens (#457)