Added an admin page for sending emails #461

Merged

johnsturgeon merged 2 commits from create-an-admin-page-where-i-can-email-everybody-my-own-text-445 into main

2026-09-04 08:34:46 +02:00

johnsturgeon commented

2026-09-04 07:40:55 +02:00

Owner

Resolves #237
Resolves #445

Resolves #237 Resolves #445

johnsturgeon added 1 commit

2026-09-04 07:40:55 +02:00

Added an admin page for sending emails

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

Details

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

Details

Tests / pytest (pull_request) Successful in 24s

Details

4dff05d178

Resolves #237
Resolves #445
Resolves #178

forgejo-actions commented

2026-09-04 07:42:16 +02:00

I have enough to write the review. Let me verify one detail about the markdown preset behavior from documentation knowledge and finalize.

Based on my review of the code and surrounding context, here is the review:

Review: Admin page for sending emails

This is a well-structured, well-tested feature: the mailer refactor into a shared email_base.j2 + _footer_body() removes real duplication, and the route's per-recipient send-and-report loop is clean. Most of my notes are about server-side validation and one non-obvious markdown default; nothing here is a hard blocker.

Worth fixing

MarkdownIt("commonmark") passes raw HTML through — app/mailer.py:70. The commonmark preset sets html: True (unlike the default preset), so raw HTML embedded in the composed markdown is emitted verbatim and then rendered with {{ body_html | safe }} in email_custom.j2. The UI (cheat sheet, "Some markdown." placeholder) implies markdown-only input, so this is almost certainly unintended. Severity is low — the composer is an authenticated admin and the on-page preview is a sandboxed iframe — but if the intent is "markdown only," construct with MarkdownIt() (the default preset, html=False) or pass MarkdownIt("commonmark", {"html": False}). Worth confirming the actual html option value against the pinned markdown-it-py>=4.2.0 before merging, since I couldn't execute it in this sandbox.
Subject/body are only validated client-side — app/routers/v2.py:557-568. The empty-recipients case is guarded on the server, but empty subject/body are only stopped by the JS submit handler in admin_email.j2. A JS-disabled browser or a crafted POST will send blank-subject/blank-body emails to real players. Consider mirroring the recipients check server-side (redirect back with a flash) rather than relying on the alert.
A full-pool send is N sequential SMTP round-trips inside one request — app/routers/v2.py:571-581. Each selected player triggers a separate send_custom_email → FastMail(conf).send_message (new connection per email), awaited one at a time. For a large active roster this can make the request run for many seconds and risk a gateway/proxy timeout — and there's no idempotency, so a client retry re-sends. The per-recipient split is the right call for privacy and personalization, but this reads like a good candidate for the existing scheduler/background-job pattern (see resend_welcome and the jobs infra) rather than doing it inline in the handler.

Nits

The "What went out" preview greets the admin, not the recipients — app/routers/v2.py:588 renders email_custom.j2 with custom_email_body(body, admin.first_name), so the bounce page shows "Hi {admin}!" even though each actual email used the recipient's first name. Fine as a representative preview, but slightly misleading given the "exactly as it went out" framing in the docstring.
No test covers the empty subject/body path — the suite thoroughly covers recipient selection, partial failure, and markdown rendering, but since subject/body validation is client-only there's nothing pinning server behavior for a blank composition. If you add the server-side check above, add a test with it.
Version bump 2.23.2 → 2.25.0 skips a minor; harmless, just noting it's a two-step jump for one PR per the CLAUDE.md "bump once per PR" convention.

I have enough to write the review. Let me verify one detail about the markdown preset behavior from documentation knowledge and finalize. Based on my review of the code and surrounding context, here is the review: --- ## Review: Admin page for sending emails This is a well-structured, well-tested feature: the mailer refactor into a shared `email_base.j2` + `_footer_body()` removes real duplication, and the route's per-recipient send-and-report loop is clean. Most of my notes are about server-side validation and one non-obvious markdown default; nothing here is a hard blocker. ### Worth fixing - **`MarkdownIt("commonmark")` passes raw HTML through** — `app/mailer.py:70`. The `commonmark` preset sets `html: True` (unlike the `default` preset), so raw HTML embedded in the composed markdown is emitted verbatim and then rendered with `{{ body_html | safe }}` in `email_custom.j2`. The UI (cheat sheet, "Some **markdown**." placeholder) implies markdown-only input, so this is almost certainly unintended. Severity is low — the composer is an authenticated admin and the on-page preview is a sandboxed iframe — but if the intent is "markdown only," construct with `MarkdownIt()` (the `default` preset, `html=False`) or pass `MarkdownIt("commonmark", {"html": False})`. Worth confirming the actual `html` option value against the pinned `markdown-it-py>=4.2.0` before merging, since I couldn't execute it in this sandbox. - **Subject/body are only validated client-side** — `app/routers/v2.py:557-568`. The empty-recipients case is guarded on the server, but empty `subject`/`body` are only stopped by the JS `submit` handler in `admin_email.j2`. A JS-disabled browser or a crafted POST will send blank-subject/blank-body emails to real players. Consider mirroring the recipients check server-side (redirect back with a flash) rather than relying on the alert. - **A full-pool send is N sequential SMTP round-trips inside one request** — `app/routers/v2.py:571-581`. Each selected player triggers a separate `send_custom_email` → `FastMail(conf).send_message` (new connection per email), awaited one at a time. For a large active roster this can make the request run for many seconds and risk a gateway/proxy timeout — and there's no idempotency, so a client retry re-sends. The per-recipient split is the right call for privacy and personalization, but this reads like a good candidate for the existing scheduler/background-job pattern (see `resend_welcome` and the jobs infra) rather than doing it inline in the handler. ### Nits - **The "What went out" preview greets the admin, not the recipients** — `app/routers/v2.py:588` renders `email_custom.j2` with `custom_email_body(body, admin.first_name)`, so the bounce page shows "Hi {admin}!" even though each actual email used the recipient's first name. Fine as a representative preview, but slightly misleading given the "exactly as it went out" framing in the docstring. - **No test covers the empty subject/body path** — the suite thoroughly covers recipient selection, partial failure, and markdown rendering, but since subject/body validation is client-only there's nothing pinning server behavior for a blank composition. If you add the server-side check above, add a test with it. - **Version bump 2.23.2 → 2.25.0** skips a minor; harmless, just noting it's a two-step jump for one PR per the CLAUDE.md "bump once per PR" convention.

johnsturgeon added 1 commit

2026-09-04 08:09:29 +02:00

Add recipient selection, bounce page, and tests to email admin

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

Details

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

Details

Tests / pytest (pull_request) Successful in 14s

Details

18b874d320

Compose page now picks recipients (checkbox list + select all) instead
of mailing every active player, and guards an empty subject or body in
the browser. After sending, a bounce page renders the email exactly as
it went out and a per-recipient delivery light (green sent, red failed).

Share one render between send and the bounce page via custom_email_body,
add type hints and a docstring to send_custom_email, and cover the routes
with tests that stub the mailer.

Resolves #445

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

johnsturgeon referenced this pull request

2026-09-04 08:33:37 +02:00

Send admin emails via a background job with live status reporting #462

johnsturgeon merged commit 2c82a83f20 into main

2026-09-04 08:34:46 +02:00

johnsturgeon deleted branch create-an-admin-page-where-i-can-email-everybody-my-own-text-445

2026-09-04 08:34:46 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-04 08:34:47 +02:00

Merge pull request 'Added an admin page for sending emails' (#461) from create-an-admin-page-where-i-can-email-everybody-my-own-text-445 into main