forgejo_admin/basketball-api

Fork 1

fix: per-player contract rows and working action.mjml #306

Merged

forgejo_admin merged 1 commit from 303-fix-query-and-template into main

2026-04-03 21:21:41 +00:00

forgejo_admin commented

2026-04-03 21:18:30 +00:00

Owner

Summary

Fixes two bugs blocking the contract reminder email blast: the unsigned_contracts query silently dropped contracts for multi-child parents, and action.mjml used MJML v5-incompatible features that broke rendering.

Changes

src/basketball_api/services/email_queries.py — Removed parent_map grouping. Query now emits one row per player instead of one row per parent. Each contract is a separate legal document needing its own email with a direct sign link. Removed unused Parent import.
templates/email/action.mjml — Replaced broken template that used mj-group left-border trick and css-class/mj-include (all broken in MJML v5 beta) with working version using mj-table for the red left-border card and inline attributes.
tests/test_email_blast.py — Added test_multi_player_parent_returns_per_player_rows covering the exact bug scenario (Sandra Apaisa with 3 unsigned children).

Test Plan

28 tests pass (17 blast + 11 contract reminder)
New test verifies 3 children produce 3 result rows, each with correct player_name, contract_url, and parent info
ruff format and ruff check clean
Verify with test blast to draneylucas@gmail.com after deploy

Review Checklist

ruff format and ruff check pass
All existing tests pass (no regressions)
New test covers the multi-player-per-parent bug
No downstream consumers depend on removed player_count/players keys
action.mjml compiles without mj-group or css-class

None.

Closes #303

## Summary Fixes two bugs blocking the contract reminder email blast: the unsigned_contracts query silently dropped contracts for multi-child parents, and action.mjml used MJML v5-incompatible features that broke rendering. ## Changes - `src/basketball_api/services/email_queries.py` — Removed parent_map grouping. Query now emits one row per player instead of one row per parent. Each contract is a separate legal document needing its own email with a direct sign link. Removed unused `Parent` import. - `templates/email/action.mjml` — Replaced broken template that used `mj-group` left-border trick and `css-class`/`mj-include` (all broken in MJML v5 beta) with working version using `mj-table` for the red left-border card and inline attributes. - `tests/test_email_blast.py` — Added `test_multi_player_parent_returns_per_player_rows` covering the exact bug scenario (Sandra Apaisa with 3 unsigned children). ## Test Plan - 28 tests pass (17 blast + 11 contract reminder) - New test verifies 3 children produce 3 result rows, each with correct player_name, contract_url, and parent info - `ruff format` and `ruff check` clean - Verify with test blast to draneylucas@gmail.com after deploy ## Review Checklist - [x] ruff format and ruff check pass - [x] All existing tests pass (no regressions) - [x] New test covers the multi-player-per-parent bug - [x] No downstream consumers depend on removed `player_count`/`players` keys - [x] action.mjml compiles without mj-group or css-class ## Related Notes None. ## Related Closes #303

forgejo_admin added 1 commit

2026-04-03 21:18:30 +00:00

fix: emit per-player rows in unsigned_contracts query and replace broken action.mjml

ci/woodpecker/pr/woodpecker Pipeline failed

Details

b3ffa0abd2

The unsigned_contracts query grouped by parent, so Sandra Apaisa with 3
unsigned players got ONE email for her first child -- the other two contracts
were silently dropped. Each contract is a separate legal document and needs
its own email with a direct sign link. Removed parent_map grouping in favor
of one row per player.

Replaced action.mjml with a working v2 that uses mj-table for the red
left-border card instead of mj-group (broken in MJML v5 beta) and inline
attributes instead of css-class / mj-include (also broken in v5 beta).

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

forgejo_admin commented

2026-04-03 21:19:32 +00:00

Author

Owner

QA Review -- PR #306

Query change (email_queries.py)

Correctly removes parent_map grouping that caused multi-child parents to receive only one email for the first child
Per-player iteration is simpler and handles test_email filter inline
Unused Parent import properly removed
No downstream consumers depend on the removed player_count/players keys (verified via grep across src/)

Template change (action.mjml)

Replaces mj-group (broken in MJML v5 beta) with mj-table for the red left-border card
Removes css-class and mj-include references (both broken in v5 beta)
Inlines font-family and brand attributes via mj-attributes block
All placeholders preserved: {{headline}}, {{body}}, {{cta_url}}, {{cta_text}}, {{footer_note}}
Explicit background-color on headline/body/CTA sections (#141414) where old template relied on inherited/undefined background

Test addition (test_email_blast.py)

New test creates 1 parent (Sandra Apaisa) with 3 unsigned players, asserts 3 result rows
Verifies per-player data: unique player_name, unique contract_url with correct token, same parent email on all rows
Directly covers the exact bug scenario described in the issue

Nits

None.

VERDICT: APPROVED

## QA Review -- PR #306 ### Query change (email_queries.py) - Correctly removes parent_map grouping that caused multi-child parents to receive only one email for the first child - Per-player iteration is simpler and handles test_email filter inline - Unused `Parent` import properly removed - No downstream consumers depend on the removed `player_count`/`players` keys (verified via grep across src/) ### Template change (action.mjml) - Replaces `mj-group` (broken in MJML v5 beta) with `mj-table` for the red left-border card - Removes `css-class` and `mj-include` references (both broken in v5 beta) - Inlines font-family and brand attributes via `mj-attributes` block - All placeholders preserved: `{{headline}}`, `{{body}}`, `{{cta_url}}`, `{{cta_text}}`, `{{footer_note}}` - Explicit `background-color` on headline/body/CTA sections (#141414) where old template relied on inherited/undefined background ### Test addition (test_email_blast.py) - New test creates 1 parent (Sandra Apaisa) with 3 unsigned players, asserts 3 result rows - Verifies per-player data: unique player_name, unique contract_url with correct token, same parent email on all rows - Directly covers the exact bug scenario described in the issue ### Nits None. VERDICT: APPROVED

forgejo_admin commented

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

Author

Owner

PR #306 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy (query layer), MJML (email templates), pytest (tests).

Query logic (email_queries.py):

The refactor from parent-grouped to per-player rows is correct. The old parent_map approach collapsed N children into one entry, meaning only the first child's contract_url was used as the primary link. Parents with multiple unsigned children would receive one email with one link -- the other contracts were buried in a players list that the action template had no mechanism to render per-row.
The new flat loop emits one dict per player, each with the correct contract_url. This matches the stated requirement: each contract is a separate legal document needing its own sign link.
test_email filtering moved from post-grouping dict comprehension to inline continue -- functionally equivalent, slightly simpler.
Removed Parent import is correct -- player.parent is accessed via the ORM relationship, no direct Parent query needed.
Removed keys (player_count, players list) were only consumed internally by this query function's output shape. The PR checklist confirms no downstream consumers depend on these keys. The blast endpoint consumes the flat dict keys (to, parent_name, player_name, team_name, contract_url) which are all preserved.

Template (action.mjml):

Removed mj-group (deprecated/broken in MJML v5 beta), css-class attribute (not supported without custom CSS head), and mj-include (requires build tooling).
Replacement uses mj-table with inline style for the red left-border card -- this is the standard workaround for left-border cards in MJML and renders reliably across email clients.
background-color attributes moved from mj-section default to explicit per-section -- #141414 for content sections, #0a0a0a for header/footer. Consistent with brand.
Template variables preserved: {{headline}}, {{body}}, {{cta_url}}, {{cta_text}}, {{footer_note}} -- no contract breakage with consumers.

Test (test_email_blast.py):

New test test_multi_player_parent_returns_per_player_rows directly exercises the bug scenario: Sandra Apaisa with 3 unsigned children.
Asserts: 3 result rows, correct player names (set comparison), correct contract URLs (token presence check), all rows addressed to same parent.
Test fixture properly creates parent, team, 3 players with distinct contract_token values, and junction table entries.
Uses db.flush() between creates for FK integrity -- correct pattern for SQLAlchemy test fixtures.

BLOCKERS

None.

New functionality has test coverage (the multi-player regression test).
No unvalidated user input (query is parameterized via SQLAlchemy ORM).
No secrets or credentials in code.
No DRY violations in auth/security paths.

NITS

team_name fallback: player.teams[0].name if player.teams else "Westside" -- the hardcoded "Westside" fallback was inherited from the old code, not introduced here. Consider extracting this to a constant (DEFAULT_TEAM_NAME) in a future cleanup ticket. Not blocking.
Test assertion style: The contract URL check assert any(tok in url for url in result_urls) works but is slightly indirect. A more direct assertion would be assert f"{_CONTRACT_BASE_URL}/{tok}" in result_urls -- though this would require importing the constant. Minor preference, not blocking.

SOP COMPLIANCE

Branch named after issue: 303-fix-query-and-template references issue #303
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Related references parent issue: Closes #303
No secrets committed
No scope creep: all 3 files are directly related to the fix
Commit messages are descriptive (PR title: fix: per-player contract rows and working action.mjml)

One minor gap: the Related section says "None" for plan slug references, which is acceptable since this is a bug fix, not plan-driven work.

PROCESS OBSERVATIONS

Change failure risk: Low. The query shape change removes keys (player_count, players) that were unused by the template consumer. The blast endpoint passes individual dict values into template placeholders -- the flat per-player shape aligns better with this pattern.
Deployment frequency: This unblocks the contract reminder email blast, which is an active operational need (issue #275).
Testing: 28 tests reported passing including the new regression test. The test directly models the real-world scenario (Sandra Apaisa, 3 children) which is good signal.

VERDICT: APPROVED

## PR #306 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy (query layer), MJML (email templates), pytest (tests). **Query logic (`email_queries.py`)**: - The refactor from parent-grouped to per-player rows is correct. The old `parent_map` approach collapsed N children into one entry, meaning only the first child's `contract_url` was used as the primary link. Parents with multiple unsigned children would receive one email with one link -- the other contracts were buried in a `players` list that the action template had no mechanism to render per-row. - The new flat loop emits one dict per player, each with the correct `contract_url`. This matches the stated requirement: each contract is a separate legal document needing its own sign link. - `test_email` filtering moved from post-grouping dict comprehension to inline `continue` -- functionally equivalent, slightly simpler. - Removed `Parent` import is correct -- `player.parent` is accessed via the ORM relationship, no direct `Parent` query needed. - Removed keys (`player_count`, `players` list) were only consumed internally by this query function's output shape. The PR checklist confirms no downstream consumers depend on these keys. The blast endpoint consumes the flat dict keys (`to`, `parent_name`, `player_name`, `team_name`, `contract_url`) which are all preserved. **Template (`action.mjml`)**: - Removed `mj-group` (deprecated/broken in MJML v5 beta), `css-class` attribute (not supported without custom CSS head), and `mj-include` (requires build tooling). - Replacement uses `mj-table` with inline `style` for the red left-border card -- this is the standard workaround for left-border cards in MJML and renders reliably across email clients. - `background-color` attributes moved from `mj-section` default to explicit per-section -- `#141414` for content sections, `#0a0a0a` for header/footer. Consistent with brand. - Template variables preserved: `{{headline}}`, `{{body}}`, `{{cta_url}}`, `{{cta_text}}`, `{{footer_note}}` -- no contract breakage with consumers. **Test (`test_email_blast.py`)**: - New test `test_multi_player_parent_returns_per_player_rows` directly exercises the bug scenario: Sandra Apaisa with 3 unsigned children. - Asserts: 3 result rows, correct player names (set comparison), correct contract URLs (token presence check), all rows addressed to same parent. - Test fixture properly creates parent, team, 3 players with distinct `contract_token` values, and junction table entries. - Uses `db.flush()` between creates for FK integrity -- correct pattern for SQLAlchemy test fixtures. ### BLOCKERS None. - New functionality has test coverage (the multi-player regression test). - No unvalidated user input (query is parameterized via SQLAlchemy ORM). - No secrets or credentials in code. - No DRY violations in auth/security paths. ### NITS 1. **`team_name` fallback**: `player.teams[0].name if player.teams else "Westside"` -- the hardcoded `"Westside"` fallback was inherited from the old code, not introduced here. Consider extracting this to a constant (`DEFAULT_TEAM_NAME`) in a future cleanup ticket. Not blocking. 2. **Test assertion style**: The contract URL check `assert any(tok in url for url in result_urls)` works but is slightly indirect. A more direct assertion would be `assert f"{_CONTRACT_BASE_URL}/{tok}" in result_urls` -- though this would require importing the constant. Minor preference, not blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `303-fix-query-and-template` references issue #303 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references parent issue: `Closes #303` - [x] No secrets committed - [x] No scope creep: all 3 files are directly related to the fix - [x] Commit messages are descriptive (PR title: `fix: per-player contract rows and working action.mjml`) One minor gap: the Related section says "None" for plan slug references, which is acceptable since this is a bug fix, not plan-driven work. ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The query shape change removes keys (`player_count`, `players`) that were unused by the template consumer. The blast endpoint passes individual dict values into template placeholders -- the flat per-player shape aligns better with this pattern. - **Deployment frequency**: This unblocks the contract reminder email blast, which is an active operational need (issue #275). - **Testing**: 28 tests reported passing including the new regression test. The test directly models the real-world scenario (Sandra Apaisa, 3 children) which is good signal. ### VERDICT: APPROVED