fix: resolve per-recipient placeholders in /email/blast data values

forgejo_admin commented

2026-04-03 20:21:24 +00:00

Owner

Summary

The /email/blast endpoint merged body.data over recipient data but never resolved {{key}} placeholders within data string values. Subject placeholders were resolved correctly, but data values like "Hey {{parent_name}}, ..." were passed through raw to the template engine.

Changes

src/basketball_api/routes/admin.py -- Added a resolution loop after internal key removal (lines 1131-1137) that substitutes recipient fields into data string values before subject resolution and before send_templated_email is called.
tests/test_email_blast.py -- Added test_data_placeholder_resolution verifying that body.data containing {{parent_name}} and {{player_name}} resolves to actual recipient values.

Test Plan

All 16 tests in test_email_blast.py pass, including the new test.
Full suite: 772 passed, 9 pre-existing failures in unrelated jersey/checkout tests.

Review Checklist

Code change is minimal and targeted to the bug
New test covers the exact failure scenario
Existing tests still pass
No unrelated changes

None.

Forgejo issue: #295
Closes #295

## Summary The `/email/blast` endpoint merged `body.data` over recipient data but never resolved `{{key}}` placeholders within data string values. Subject placeholders were resolved correctly, but data values like `"Hey {{parent_name}}, ..."` were passed through raw to the template engine. ## Changes - `src/basketball_api/routes/admin.py` -- Added a resolution loop after internal key removal (lines 1131-1137) that substitutes recipient fields into data string values before subject resolution and before `send_templated_email` is called. - `tests/test_email_blast.py` -- Added `test_data_placeholder_resolution` verifying that `body.data` containing `{{parent_name}}` and `{{player_name}}` resolves to actual recipient values. ## Test Plan - All 16 tests in `test_email_blast.py` pass, including the new test. - Full suite: 772 passed, 9 pre-existing failures in unrelated jersey/checkout tests. ## Review Checklist - [x] Code change is minimal and targeted to the bug - [x] New test covers the exact failure scenario - [x] Existing tests still pass - [x] No unrelated changes ## Related Notes None. ## Related - Forgejo issue: #295 - Closes #295

forgejo_admin added 1 commit

2026-04-03 20:21:24 +00:00

fix: resolve per-recipient placeholders inside body.data values

ci/woodpecker/pr/woodpecker Pipeline failed

Details

a4a3f05f95

The /email/blast endpoint merged body.data over recipient data but
never resolved {{key}} placeholders within data string values.
Subject placeholders were resolved, but data values like
"Hey {{parent_name}}, ..." were passed through raw. Add a resolution
loop after internal key removal that substitutes recipient fields
into data values before template rendering.

Closes #295

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

forgejo_admin commented

2026-04-03 20:30:33 +00:00

Author

Owner

PR #301 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy (basketball-api)

The fix adds a per-recipient placeholder resolution loop over template_data string values at lines 1131-1137 of admin.py. This uses str.replace(f"{{{{rkey}}}}", rval) -- identical to the existing subject resolution pattern and load_email_template's own placeholder engine. The fix is placed correctly: after internal keys (parent_id, players) are popped, before subject resolution, and before send_templated_email is called.

The resolution only applies to str values (isinstance guard on both data_val and rval), so nested dicts, ints, etc. pass through safely.

Security: The /email/blast endpoint is admin-only. Recipient field values come from the database, not from the request body. No injection vector introduced.

BLOCKERS

None.

NITS

Repeated placeholder resolution pattern -- The str.replace(f"{{{{key}}}}", value) loop now appears in three places: data value resolution (new code), subject resolution (existing), and load_email_template. Consider extracting a resolve_placeholders(template: str, context: dict) -> str helper in email.py. Not blocking -- the blast endpoint is admin-facing and the pattern is simple.
Test call_args extraction is fragile (lines 548-553) -- The fallback logic call_args[0][1] if len(call_args[0]) > 1 else call_args[1].get("data", call_args[0][1]) tries to handle both positional and keyword calling conventions. If the function signature changes, this will silently grab the wrong arg. Consider asserting the exact call form, e.g. mock_load_template.assert_called_once_with("contract-offer", ANY) then checking the second positional arg directly.
Non-string rval silently skipped -- If a recipient field is an int (e.g., a hypothetical numeric field), isinstance(rval, str) skips it. This is fine today since parent_name/player_name are strings, but worth documenting the assumption.

SOP COMPLIANCE

Branch named after issue (295-fix-data-placeholder-merge references #295)
PR body follows template (Summary, Changes, Test Plan, Related)
Related references parent issue #295
Related does not reference a plan slug (acceptable -- this is a standalone bug fix, not plan work)
No secrets committed
No unrelated changes (2 files, +51 lines, all on-target)
Test exists and covers the exact failure scenario

PROCESS OBSERVATIONS

Clean, minimal fix. 8 lines of production code, 43 lines of test. The 9 pre-existing failures in jersey/checkout tests (tracked in #300/#274) are unrelated and should not block this merge. Change failure risk is low -- the new code only adds resolution; it cannot break data values that contain no placeholders.

VERDICT: APPROVED

## PR #301 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy (basketball-api) The fix adds a per-recipient placeholder resolution loop over `template_data` string values at lines 1131-1137 of `admin.py`. This uses `str.replace(f"{{{{rkey}}}}", rval)` -- identical to the existing subject resolution pattern and `load_email_template`'s own placeholder engine. The fix is placed correctly: after internal keys (`parent_id`, `players`) are popped, before subject resolution, and before `send_templated_email` is called. The resolution only applies to `str` values (`isinstance` guard on both data_val and rval), so nested dicts, ints, etc. pass through safely. **Security**: The `/email/blast` endpoint is admin-only. Recipient field values come from the database, not from the request body. No injection vector introduced. ### BLOCKERS None. ### NITS 1. **Repeated placeholder resolution pattern** -- The `str.replace(f"{{{{key}}}}", value)` loop now appears in three places: data value resolution (new code), subject resolution (existing), and `load_email_template`. Consider extracting a `resolve_placeholders(template: str, context: dict) -> str` helper in `email.py`. Not blocking -- the blast endpoint is admin-facing and the pattern is simple. 2. **Test `call_args` extraction is fragile** (lines 548-553) -- The fallback logic `call_args[0][1] if len(call_args[0]) > 1 else call_args[1].get("data", call_args[0][1])` tries to handle both positional and keyword calling conventions. If the function signature changes, this will silently grab the wrong arg. Consider asserting the exact call form, e.g. `mock_load_template.assert_called_once_with("contract-offer", ANY)` then checking the second positional arg directly. 3. **Non-string rval silently skipped** -- If a recipient field is an `int` (e.g., a hypothetical numeric field), `isinstance(rval, str)` skips it. This is fine today since `parent_name`/`player_name` are strings, but worth documenting the assumption. ### SOP COMPLIANCE - [x] Branch named after issue (`295-fix-data-placeholder-merge` references #295) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references parent issue #295 - [ ] Related does not reference a plan slug (acceptable -- this is a standalone bug fix, not plan work) - [x] No secrets committed - [x] No unrelated changes (2 files, +51 lines, all on-target) - [x] Test exists and covers the exact failure scenario ### PROCESS OBSERVATIONS Clean, minimal fix. 8 lines of production code, 43 lines of test. The 9 pre-existing failures in jersey/checkout tests (tracked in #300/#274) are unrelated and should not block this merge. Change failure risk is low -- the new code only adds resolution; it cannot break data values that contain no placeholders. ### VERDICT: APPROVED