forgejo_admin/basketball-api

Fork 1

feat: MJML template loader and jersey reminder endpoint #135

Merged

forgejo_admin merged 2 commits from 134-mjml-email-service into main

2026-03-21 17:31:34 +00:00

forgejo_admin commented

2026-03-21 17:00:46 +00:00

Owner

Summary

Thin integration between basketball-api and westside-emails MJML compiled templates. Adds a template loader that reads compiled HTML and replaces {{key}} placeholders via simple string substitution, plus a new admin endpoint for sending jersey reminder emails. Existing email functions are untouched -- migration happens incrementally.

Changes

src/basketball_api/config.py -- add email_templates_dir setting (env var BASKETBALL_EMAIL_TEMPLATES_DIR, default /data/email-templates)
src/basketball_api/services/email.py -- add load_email_template() function that reads compiled HTML from configurable directory and replaces {{key}} markers; add send_jersey_reminder_email() that uses the template loader with {{name}} and {{jersey_url}} placeholders
src/basketball_api/routes/admin.py -- add POST /admin/email/jersey-reminder endpoint with optional test_email query param, matching the pattern of existing email endpoints (profile-reminder, tryout-announcement)
tests/test_jersey_reminder.py -- 8 tests: 4 unit tests for load_email_template() (placeholder replacement, missing template error, unreplaced keys preserved, HTML special chars) and 4 integration tests for the endpoint (send to test email, send to all parents, no match returns 0, failure handling with error capture)

Test Plan

All 464 tests pass (including 8 new ones)
ruff format and ruff check pass with no issues
Template loader correctly replaces {{name}} and {{jersey_url}} placeholders
Endpoint follows existing admin email patterns (auth, tenant lookup, test_email filter)
Deploy with westside-emails dist/ mounted at EMAIL_TEMPLATES_DIR and send test email

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #134
plan-wkq -- Phase 11 discovered scope
westside-emails PR: forgejo_admin/westside-emails#2

## Summary Thin integration between basketball-api and westside-emails MJML compiled templates. Adds a template loader that reads compiled HTML and replaces `{{key}}` placeholders via simple string substitution, plus a new admin endpoint for sending jersey reminder emails. Existing email functions are untouched -- migration happens incrementally. ## Changes - `src/basketball_api/config.py` -- add `email_templates_dir` setting (env var `BASKETBALL_EMAIL_TEMPLATES_DIR`, default `/data/email-templates`) - `src/basketball_api/services/email.py` -- add `load_email_template()` function that reads compiled HTML from configurable directory and replaces `{{key}}` markers; add `send_jersey_reminder_email()` that uses the template loader with `{{name}}` and `{{jersey_url}}` placeholders - `src/basketball_api/routes/admin.py` -- add `POST /admin/email/jersey-reminder` endpoint with optional `test_email` query param, matching the pattern of existing email endpoints (profile-reminder, tryout-announcement) - `tests/test_jersey_reminder.py` -- 8 tests: 4 unit tests for `load_email_template()` (placeholder replacement, missing template error, unreplaced keys preserved, HTML special chars) and 4 integration tests for the endpoint (send to test email, send to all parents, no match returns 0, failure handling with error capture) ## Test Plan - [x] All 464 tests pass (including 8 new ones) - [x] `ruff format` and `ruff check` pass with no issues - [x] Template loader correctly replaces `{{name}}` and `{{jersey_url}}` placeholders - [x] Endpoint follows existing admin email patterns (auth, tenant lookup, test_email filter) - [ ] Deploy with westside-emails dist/ mounted at EMAIL_TEMPLATES_DIR and send test email ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #134 - `plan-wkq` -- Phase 11 discovered scope - westside-emails PR: https://forgejo.tail5b443a.ts.net/forgejo_admin/westside-emails/pulls/2

forgejo_admin added 1 commit

2026-03-21 17:00:46 +00:00

feat: MJML template loader and jersey reminder email endpoint

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

4f0e96cb9d

Add thin integration between basketball-api and westside-emails compiled
HTML templates. Templates use literal {{key}} placeholders replaced via
simple string substitution at send time -- no Jinja2, no template engine.

- config.py: add email_templates_dir setting (BASKETBALL_EMAIL_TEMPLATES_DIR)
- email.py: add load_email_template() for loading compiled HTML and replacing
  {{key}} placeholders, add send_jersey_reminder_email() using the template
- admin.py: add POST /admin/email/jersey-reminder endpoint with optional
  test_email query param, matching existing email endpoint patterns
- test_jersey_reminder.py: 8 tests covering template loader (placeholder
  replacement, missing template, unreplaced keys, special chars) and
  endpoint (send to test email, send to all, no match, failure handling)

Existing email functions untouched -- migration happens incrementally.

Refs #134

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin added 1 commit

2026-03-21 17:02:42 +00:00

refactor: move os and pathlib imports to module level in email.py

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

6415f38c0b

Review fix: imports for os and pathlib.Path were inside load_email_template()
function body. Moved to module-level imports where they belong, following
the existing import conventions in the file.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-21 17:02:53 +00:00

Author

Owner

Review-fix loop

Finding 1 (fixed): os and pathlib.Path were imported inside the load_email_template() function body instead of at module level. Moved to top-level imports in commit 6415f38.

Finding 2 (noted, acceptable): send_jersey_reminder_email logs with EmailType.announcement since there is no jersey_reminder enum value. Adding a new enum would require an Alembic migration (DB schema change), which is out of scope for this PR. Can be added in a follow-up if needed.

All 464 tests pass. ruff format and ruff check clean.

## Review-fix loop **Finding 1 (fixed):** `os` and `pathlib.Path` were imported inside the `load_email_template()` function body instead of at module level. Moved to top-level imports in commit 6415f38. **Finding 2 (noted, acceptable):** `send_jersey_reminder_email` logs with `EmailType.announcement` since there is no `jersey_reminder` enum value. Adding a new enum would require an Alembic migration (DB schema change), which is out of scope for this PR. Can be added in a follow-up if needed. **All 464 tests pass.** `ruff format` and `ruff check` clean.

forgejo_admin commented

2026-03-21 17:08:49 +00:00

Author

Owner

PR #135 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic -- applying Python/FastAPI domain checklist.

Pattern conformance: The new endpoint and email service function follow the established patterns closely. The POST /admin/email/jersey-reminder endpoint mirrors the structure of /email/profile-reminder (lines 386-419 of admin.py) and /email/tryout-announcement with the same {sent_count, errors} response model, require_admin dependency, tenant lookup, parent query loop, and error capture. Well done.

Template loader design: load_email_template() is a clean, minimal function. Hardcoded template name at the only call site ("jersey-reminder") means no user-controlled path input today. The {{key}} string replacement is appropriate for the use case -- no need for Jinja2 overhead.

Graceful fallback: When the MJML template is missing, the function falls back to plain_body (sets html_body = None). The plain text body is well-written with all the relevant info. This is good resilience for first deployment before the template volume is mounted.

Type hints and docstrings: All new functions have proper type hints and PEP 257 docstrings. Parameters and return types are documented.

Test quality: 8 tests covering:

Template loader: placeholder replacement, missing template error, unreplaced keys preserved, HTML special chars
Endpoint: test_email filter, send to all, no match returns 0, send failure capture

Tests use tmp_path for template fixtures and mock Gmail client properly. Fixtures reuse the existing conftest.py patterns (db, tenant).

BLOCKERS

None. No blocker criteria are triggered:

Test coverage: 8 new tests cover both the template loader and the endpoint (happy path, edge cases, error handling).
Unvalidated user input: The test_email query param is used only as a SQLAlchemy filter value (Parent.email == test_email), which is parameterized -- no SQL injection risk. The template_name in load_email_template is hardcoded at the call site, not user-supplied.
Secrets: No secrets or credentials in code.
DRY in auth paths: Auth uses the existing require_admin dependency -- no duplication.

NITS

EmailType reuse (email.py:1020): Jersey reminder emails are logged as EmailType.announcement, making them indistinguishable from tryout announcement emails in the email_log table. Consider adding a jersey_reminder value to the EmailType enum (with an Alembic migration) so email logs can be filtered by type. Not blocking since the existing announcement email also uses this type, but it will matter when you need to audit "which parents got the jersey email."
Path traversal hardening (email.py:935-936): While template_name is currently hardcoded, if a future caller ever passes user input, the Path(templates_dir) / f"{template_name}.html" construction would allow path traversal (e.g., ../../etc/passwd). A one-line guard like if "/" in template_name or ".." in template_name: raise ValueError(...) would make this defense-in-depth. Low priority since the current call site is safe.
data dict type hint (email.py:923): data: dict could be data: dict[str, str] for stricter typing, since all values go through str(value) anyway. Minor.
html import unused in new code: from html import escape is imported at the top of email.py but the new template loader does NOT escape parent.name before inserting into HTML. This is consistent with the existing pattern (e.g., send_tryout_announcement_email also inserts parent.name unescaped into plain text bodies), but worth noting as pre-existing tech debt. If a parent registers with a name containing HTML entities, the MJML template would render it raw. Low risk since names come from your own registration form, but a future hardening task.

SOP COMPLIANCE

Branch named after issue (134-mjml-email-service for issue #134)
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related references plan slug (plan-wkq -- Phase 11 discovered scope)
No secrets committed
No unnecessary file changes (4 files, all on-topic)
Commit messages are descriptive
Tests exist and pass (464 total, 8 new)

PROCESS OBSERVATIONS

Deployment frequency: Clean additive change (398 additions, 0 deletions). No breaking changes to existing endpoints. Low merge risk.
Change failure risk: Low. The endpoint follows a battle-tested pattern used by 3 other admin email endpoints. Template fallback to plain text adds resilience. The only deployment dependency is mounting the westside-emails dist/ directory at EMAIL_TEMPLATES_DIR.
Cross-repo coordination: This PR depends on westside-emails PR #2 for the compiled template. The test plan correctly marks the integration test as pending ([ ] Deploy with westside-emails dist/ mounted). Good awareness.

VERDICT: APPROVED

## PR #135 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic -- applying Python/FastAPI domain checklist. **Pattern conformance**: The new endpoint and email service function follow the established patterns closely. The `POST /admin/email/jersey-reminder` endpoint mirrors the structure of `/email/profile-reminder` (lines 386-419 of `admin.py`) and `/email/tryout-announcement` with the same `{sent_count, errors}` response model, `require_admin` dependency, tenant lookup, parent query loop, and error capture. Well done. **Template loader design**: `load_email_template()` is a clean, minimal function. Hardcoded template name at the only call site (`"jersey-reminder"`) means no user-controlled path input today. The `{{key}}` string replacement is appropriate for the use case -- no need for Jinja2 overhead. **Graceful fallback**: When the MJML template is missing, the function falls back to `plain_body` (sets `html_body = None`). The plain text body is well-written with all the relevant info. This is good resilience for first deployment before the template volume is mounted. **Type hints and docstrings**: All new functions have proper type hints and PEP 257 docstrings. Parameters and return types are documented. **Test quality**: 8 tests covering: - Template loader: placeholder replacement, missing template error, unreplaced keys preserved, HTML special chars - Endpoint: test_email filter, send to all, no match returns 0, send failure capture Tests use `tmp_path` for template fixtures and mock Gmail client properly. Fixtures reuse the existing `conftest.py` patterns (`db`, `tenant`). ### BLOCKERS None. No blocker criteria are triggered: - **Test coverage**: 8 new tests cover both the template loader and the endpoint (happy path, edge cases, error handling). - **Unvalidated user input**: The `test_email` query param is used only as a SQLAlchemy filter value (`Parent.email == test_email`), which is parameterized -- no SQL injection risk. The `template_name` in `load_email_template` is hardcoded at the call site, not user-supplied. - **Secrets**: No secrets or credentials in code. - **DRY in auth paths**: Auth uses the existing `require_admin` dependency -- no duplication. ### NITS 1. **EmailType reuse** (`email.py:1020`): Jersey reminder emails are logged as `EmailType.announcement`, making them indistinguishable from tryout announcement emails in the `email_log` table. Consider adding a `jersey_reminder` value to the `EmailType` enum (with an Alembic migration) so email logs can be filtered by type. Not blocking since the existing announcement email also uses this type, but it will matter when you need to audit "which parents got the jersey email." 2. **Path traversal hardening** (`email.py:935-936`): While `template_name` is currently hardcoded, if a future caller ever passes user input, the `Path(templates_dir) / f"{template_name}.html"` construction would allow path traversal (e.g., `../../etc/passwd`). A one-line guard like `if "/" in template_name or ".." in template_name: raise ValueError(...)` would make this defense-in-depth. Low priority since the current call site is safe. 3. **`data` dict type hint** (`email.py:923`): `data: dict` could be `data: dict[str, str]` for stricter typing, since all values go through `str(value)` anyway. Minor. 4. **`html` import unused in new code**: `from html import escape` is imported at the top of `email.py` but the new template loader does NOT escape `parent.name` before inserting into HTML. This is consistent with the existing pattern (e.g., `send_tryout_announcement_email` also inserts `parent.name` unescaped into plain text bodies), but worth noting as pre-existing tech debt. If a parent registers with a name containing HTML entities, the MJML template would render it raw. Low risk since names come from your own registration form, but a future hardening task. ### SOP COMPLIANCE - [x] Branch named after issue (`134-mjml-email-service` for issue #134) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references plan slug (`plan-wkq` -- Phase 11 discovered scope) - [x] No secrets committed - [x] No unnecessary file changes (4 files, all on-topic) - [x] Commit messages are descriptive - [x] Tests exist and pass (464 total, 8 new) ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean additive change (398 additions, 0 deletions). No breaking changes to existing endpoints. Low merge risk. - **Change failure risk**: Low. The endpoint follows a battle-tested pattern used by 3 other admin email endpoints. Template fallback to plain text adds resilience. The only deployment dependency is mounting the `westside-emails dist/` directory at `EMAIL_TEMPLATES_DIR`. - **Cross-repo coordination**: This PR depends on `westside-emails` PR #2 for the compiled template. The test plan correctly marks the integration test as pending (`[ ] Deploy with westside-emails dist/ mounted`). Good awareness. ### VERDICT: APPROVED