fix: contract email sends offer copy, not reminder #467

Merged

forgejo_admin merged 2 commits from 462-contract-offer-email into main 2026-04-13 14:36:03 +00:00

Conversation 3 Commits 2 Files changed 3 +252 -14

forgejo_admin commented 2026-04-13 00:08:44 +00:00

Contributor

Copy link

Summary

The POST /admin/email/contract/{player_id} endpoint was reusing send_contract_reminder_email, which sends reminder copy ("we noticed you haven't signed yet"). For a first-time contract send, parents should receive offer copy ("your child has been placed on the team"). This adds send_contract_offer_email and wires it into the single-player endpoint.

Changes

• src/basketball_api/services/email.py — Added send_contract_offer_email function that sends branded offer copy with Queens pink (#e91e8c) or Kings red (#d42026) accent based on team name. Uses _brand_wrapper, logs to email_log with EmailType.contract_offer, includes plain-text fallback.
• src/basketball_api/routes/admin.py — Updated POST /admin/email/contract/{player_id} to call send_contract_offer_email instead of send_contract_reminder_email. Simplified call signature (single player instead of list).

Test Plan

• Send test email via POST /admin/email/contract/{player_id}?test_email=draneylucas@gmail.com and verify offer copy appears (not reminder copy)
• Verify Queens team players get pink accent, Kings get red accent
• Verify email_log entry uses contract_offer type
• Existing /admin/email/contract-reminder endpoint unchanged (still uses reminder copy for bulk sends)

Review Checklist

• ruff format and ruff check pass
• Import verified (send_contract_offer_email imports cleanly)
• Existing contract-reminder endpoint unchanged
• EmailType.contract_offer already exists in enum (no migration needed)
• CI tests pass (requires Postgres)

Related Notes

• None

Related

• Closes #462
• Parent PR: #464 (added the endpoint, this fixes the email copy)

## Summary The `POST /admin/email/contract/{player_id}` endpoint was reusing `send_contract_reminder_email`, which sends reminder copy ("we noticed you haven't signed yet"). For a first-time contract send, parents should receive offer copy ("your child has been placed on the team"). This adds `send_contract_offer_email` and wires it into the single-player endpoint. ## Changes - `src/basketball_api/services/email.py` — Added `send_contract_offer_email` function that sends branded offer copy with Queens pink (#e91e8c) or Kings red (#d42026) accent based on team name. Uses `_brand_wrapper`, logs to `email_log` with `EmailType.contract_offer`, includes plain-text fallback. - `src/basketball_api/routes/admin.py` — Updated `POST /admin/email/contract/{player_id}` to call `send_contract_offer_email` instead of `send_contract_reminder_email`. Simplified call signature (single player instead of list). ## Test Plan - Send test email via `POST /admin/email/contract/{player_id}?test_email=draneylucas@gmail.com` and verify offer copy appears (not reminder copy) - Verify Queens team players get pink accent, Kings get red accent - Verify email_log entry uses `contract_offer` type - Existing `/admin/email/contract-reminder` endpoint unchanged (still uses reminder copy for bulk sends) ## Review Checklist - [x] ruff format and ruff check pass - [x] Import verified (`send_contract_offer_email` imports cleanly) - [x] Existing contract-reminder endpoint unchanged - [x] EmailType.contract_offer already exists in enum (no migration needed) - [ ] CI tests pass (requires Postgres) ## Related Notes - None ## Related - Closes #462 - Parent PR: #464 (added the endpoint, this fixes the email copy)

forgejo_admin added 1 commit 2026-04-13 00:08:44 +00:00

fix: contract email sends offer copy, not reminder ... 82123dbb95

The single-player contract email endpoint was reusing
send_contract_reminder_email which sends "we noticed you haven't
signed yet" copy. Added send_contract_offer_email for the initial
offer ("your child has been placed on the team") and wired it into
POST /admin/email/contract/{player_id}.

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

forgejo_admin commented 2026-04-13 00:09:31 +00:00

Author

Contributor

Copy link

QA Review — PR #467

What was reviewed

• Full diff: src/basketball_api/services/email.py (+113 lines), src/basketball_api/routes/admin.py (+3/-8 lines)
• Pattern consistency against existing email functions (send_contract_signed_email, send_contract_reminder_email)
• Import hygiene, enum usage, branding logic

Findings

No issues found. Clean implementation.

1. Pattern compliance — send_contract_offer_email follows the established email function pattern exactly: get_gmail_client -> build HTML with _brand_wrapper -> plain-text fallback -> client.send_message -> EmailLog commit -> return message_id. Matches send_contract_signed_email structure.

2. Queens/Kings branding — Uses _BRAND_QUEENS_PINK / _BRAND_RED constants already defined in the module. Team name check ("Queens" in team_name) is consistent with other Queens-specific logic in the codebase.

3. EmailType.contract_offer — Already exists in the enum (models.py line 72). No migration needed.

4. Import hygiene — send_contract_offer_email added alphabetically. send_contract_reminder_email retained (still used by bulk endpoint at line 1113).

5. Admin route — Clean substitution. The db.expunge(parent) pattern for test_email is preserved. Docstring updated to reflect offer vs reminder distinction.

6. Edge case — player.teams[0].name if player.teams else "Westside" fallback matches the same pattern in send_contract_reminder_email line 1691.

VERDICT: APPROVED

## QA Review — PR #467 ### What was reviewed - Full diff: `src/basketball_api/services/email.py` (+113 lines), `src/basketball_api/routes/admin.py` (+3/-8 lines) - Pattern consistency against existing email functions (`send_contract_signed_email`, `send_contract_reminder_email`) - Import hygiene, enum usage, branding logic ### Findings **No issues found.** Clean implementation. 1. **Pattern compliance** — `send_contract_offer_email` follows the established email function pattern exactly: `get_gmail_client` -> build HTML with `_brand_wrapper` -> plain-text fallback -> `client.send_message` -> `EmailLog` commit -> return message_id. Matches `send_contract_signed_email` structure. 2. **Queens/Kings branding** — Uses `_BRAND_QUEENS_PINK` / `_BRAND_RED` constants already defined in the module. Team name check (`"Queens" in team_name`) is consistent with other Queens-specific logic in the codebase. 3. **EmailType.contract_offer** — Already exists in the enum (models.py line 72). No migration needed. 4. **Import hygiene** — `send_contract_offer_email` added alphabetically. `send_contract_reminder_email` retained (still used by bulk endpoint at line 1113). 5. **Admin route** — Clean substitution. The `db.expunge(parent)` pattern for test_email is preserved. Docstring updated to reflect offer vs reminder distinction. 6. **Edge case** — `player.teams[0].name if player.teams else "Westside"` fallback matches the same pattern in `send_contract_reminder_email` line 1691. ### VERDICT: APPROVED

forgejo_admin commented 2026-04-13 00:11:46 +00:00

Author

Contributor

Copy link

PR #467 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Gmail SDK

What this PR does: Adds send_contract_offer_email() to email.py (initial offer copy -- "your child has been placed on the team") and wires it into POST /admin/email/contract/{player_id}, replacing the previous call to send_contract_reminder_email (reminder copy -- "we noticed you haven't signed yet"). Clean separation of offer vs. reminder email paths.

Code quality observations:

1. _brand_wrapper accent mismatch -- The function correctly computes accent (Queens pink vs. Kings red) and uses it in inline body styles (CTA button, player name highlight, team name highlight). However, _brand_wrapper(tenant.name, body_html) is called WITHOUT passing accent_color, so the wrapper's header border (border-bottom: 3px solid {accent_color}) will always be _BRAND_RED even for Queens players. The fix is _brand_wrapper(tenant.name, body_html, accent_color=accent). This is a visual inconsistency, not a functional bug, but it undermines the Queens/Kings branding logic the function explicitly implements.

2. Hardcoded season string -- "Spring/Summer 2026 season" is hardcoded in both HTML and plain-text bodies. The reminder function uses a _CONTRACT_DEADLINE constant. Consider extracting the season string to a module-level constant to avoid stale copy in future seasons.

3. escape() usage -- Properly applied to parent.name, player.name, team_name, and contract_url in HTML. Plain-text fallback correctly omits escaping. Good.

4. Error handling -- Catches all exceptions, logs with logger.exception, returns None. Consistent with the existing send_contract_reminder_email pattern. The caller in the route handler handles None gracefully (returns message_id: null in the response).

5. Session safety -- The db.expunge(parent) pattern in the route handler for test_email is preserved correctly, preventing the test email swap from being flushed to the database.

6. Type hints and docstring -- Return type str | None is correct. Docstring clearly distinguishes offer vs. reminder semantics. PEP 484/257 compliant.

BLOCKERS

Test coverage gap for the new email function -- The existing tests in test_single_contract_email.py mock load_email_template and get_gmail_client. The new send_contract_offer_email does NOT call load_email_template (it builds HTML inline), so the mock_load_template patch is now a dead mock that never fires. The tests still pass because they only assert on the response shape (player_name, parent_email, contract_link, message_id), but they do NOT verify:

• That the email body contains offer copy (not reminder copy) -- the core purpose of this PR
• That Queens teams get pink accent and Kings get red accent
• That EmailType.contract_offer (not contract_reminder) is logged

The tests validate the route plumbing but not the behavioral change this PR introduces. A unit test for send_contract_offer_email that asserts on the HTML content and email log type is needed to prevent regression back to reminder copy.

NITS

1. The _brand_wrapper accent mismatch (item 1 above) -- while visual-only, it should be fixed in this PR since the function explicitly branches on Queens vs. Kings.

2. Hardcoded "Spring/Summer 2026 season" -- extract to a constant like _CURRENT_SEASON = "Spring/Summer 2026" alongside _CONTRACT_DEADLINE.

3. The dead mock_load_template patches in test_single_contract_email.py should be removed since the new code path no longer calls load_email_template. Dead mocks are misleading.

SOP COMPLIANCE

• Branch named after issue (462-contract-offer-email matches issue #462)
• PR body follows template (Summary, Changes, Test Plan, Related)
• Related references plan slug -- no plan slug referenced (acceptable for a bug fix)
• No secrets committed
• No scope creep -- changes are tightly scoped to the email copy fix
• Commit message is descriptive

PROCESS OBSERVATIONS

• This is a fast follow-up to PR #464 which added the endpoint but used the wrong email function. Good catch and quick turnaround.
• The test_single_contract_email.py tests were written for the original implementation (which used load_email_template via send_contract_reminder_email). They need updating to match the new code path, otherwise they provide false confidence.
• Change failure risk is LOW for the route change itself (simple function swap), but the test gap means a regression to reminder copy would go undetected by CI.

VERDICT: NOT APPROVED

Reason: The test file test_single_contract_email.py has dead mocks (mock_load_template) and does not verify the behavioral change this PR introduces (offer copy vs. reminder copy, email type logging, team-based accent color). The tests need to be updated to mock get_gmail_client only (removing the dead load_email_template mock) and assert that the Gmail client receives HTML containing offer copy and that EmailType.contract_offer is written to email_log. Fix the _brand_wrapper accent passthrough as well.

## PR #467 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Gmail SDK **What this PR does**: Adds `send_contract_offer_email()` to `email.py` (initial offer copy -- "your child has been placed on the team") and wires it into `POST /admin/email/contract/{player_id}`, replacing the previous call to `send_contract_reminder_email` (reminder copy -- "we noticed you haven't signed yet"). Clean separation of offer vs. reminder email paths. **Code quality observations**: 1. **`_brand_wrapper` accent mismatch** -- The function correctly computes `accent` (Queens pink vs. Kings red) and uses it in inline body styles (CTA button, player name highlight, team name highlight). However, `_brand_wrapper(tenant.name, body_html)` is called WITHOUT passing `accent_color`, so the wrapper's header border (`border-bottom: 3px solid {accent_color}`) will always be `_BRAND_RED` even for Queens players. The fix is `_brand_wrapper(tenant.name, body_html, accent_color=accent)`. This is a visual inconsistency, not a functional bug, but it undermines the Queens/Kings branding logic the function explicitly implements. 2. **Hardcoded season string** -- `"Spring/Summer 2026 season"` is hardcoded in both HTML and plain-text bodies. The reminder function uses a `_CONTRACT_DEADLINE` constant. Consider extracting the season string to a module-level constant to avoid stale copy in future seasons. 3. **`escape()` usage** -- Properly applied to `parent.name`, `player.name`, `team_name`, and `contract_url` in HTML. Plain-text fallback correctly omits escaping. Good. 4. **Error handling** -- Catches all exceptions, logs with `logger.exception`, returns `None`. Consistent with the existing `send_contract_reminder_email` pattern. The caller in the route handler handles `None` gracefully (returns `message_id: null` in the response). 5. **Session safety** -- The `db.expunge(parent)` pattern in the route handler for `test_email` is preserved correctly, preventing the test email swap from being flushed to the database. 6. **Type hints and docstring** -- Return type `str | None` is correct. Docstring clearly distinguishes offer vs. reminder semantics. PEP 484/257 compliant. ### BLOCKERS **Test coverage gap for the new email function** -- The existing tests in `test_single_contract_email.py` mock `load_email_template` and `get_gmail_client`. The new `send_contract_offer_email` does NOT call `load_email_template` (it builds HTML inline), so the `mock_load_template` patch is now a dead mock that never fires. The tests still pass because they only assert on the response shape (player_name, parent_email, contract_link, message_id), but they do NOT verify: - That the email body contains offer copy (not reminder copy) -- the core purpose of this PR - That Queens teams get pink accent and Kings get red accent - That `EmailType.contract_offer` (not `contract_reminder`) is logged The tests validate the route plumbing but not the behavioral change this PR introduces. A unit test for `send_contract_offer_email` that asserts on the HTML content and email log type is needed to prevent regression back to reminder copy. ### NITS 1. The `_brand_wrapper` accent mismatch (item 1 above) -- while visual-only, it should be fixed in this PR since the function explicitly branches on Queens vs. Kings. 2. Hardcoded `"Spring/Summer 2026 season"` -- extract to a constant like `_CURRENT_SEASON = "Spring/Summer 2026"` alongside `_CONTRACT_DEADLINE`. 3. The dead `mock_load_template` patches in `test_single_contract_email.py` should be removed since the new code path no longer calls `load_email_template`. Dead mocks are misleading. ### SOP COMPLIANCE - [x] Branch named after issue (`462-contract-offer-email` matches issue #462) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [ ] Related references plan slug -- no plan slug referenced (acceptable for a bug fix) - [x] No secrets committed - [x] No scope creep -- changes are tightly scoped to the email copy fix - [x] Commit message is descriptive ### PROCESS OBSERVATIONS - This is a fast follow-up to PR #464 which added the endpoint but used the wrong email function. Good catch and quick turnaround. - The `test_single_contract_email.py` tests were written for the original implementation (which used `load_email_template` via `send_contract_reminder_email`). They need updating to match the new code path, otherwise they provide false confidence. - Change failure risk is LOW for the route change itself (simple function swap), but the test gap means a regression to reminder copy would go undetected by CI. ### VERDICT: NOT APPROVED **Reason**: The test file `test_single_contract_email.py` has dead mocks (`mock_load_template`) and does not verify the behavioral change this PR introduces (offer copy vs. reminder copy, email type logging, team-based accent color). The tests need to be updated to mock `get_gmail_client` only (removing the dead `load_email_template` mock) and assert that the Gmail client receives HTML containing offer copy and that `EmailType.contract_offer` is written to `email_log`. Fix the `_brand_wrapper` accent passthrough as well.

forgejo_admin added 1 commit 2026-04-13 00:17:05 +00:00

fix: address QA findings — tests, accent color, season constant ... 498645e65e

- Remove dead mock_load_template patches from tests (send_contract_offer_email
  builds HTML inline, never calls load_email_template)
- Add tests verifying offer copy ("has been placed on"), not reminder copy
- Add tests verifying EmailType.contract_offer is logged
- Add tests verifying Queens pink (#e91e8c) vs Kings red (#d42026) accent
- Fix _brand_wrapper call to pass accent_color=accent
- Extract "Spring/Summer 2026" to _CONTRACT_SEASON constant

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

forgejo_admin commented 2026-04-13 00:19:32 +00:00

Author

Contributor

Copy link

PR #467 Re-Review

Re-review after fix commit addressing 1 blocker and 2 nits from the initial review.

Previous Findings -- Verification

1. BLOCKER (dead test mocks) -- FIXED. @patch("basketball_api.services.email.load_email_template") and mock_load_template removed from test_send_returns_200 and test_test_email_param_redirects. The new send_contract_offer_email uses inline HTML + _brand_wrapper, not load_email_template, so these mocks were dead. Now correctly only mocking get_gmail_client.

2. NIT (accent not passed to _brand_wrapper) -- FIXED. send_contract_offer_email computes accent from team name (_BRAND_QUEENS_PINK for Queens, _BRAND_RED otherwise) and passes accent_color=accent to _brand_wrapper. Correctly uses the canonical _BRAND_QUEENS_PINK constant rather than the duplicate _CONTRACT_PINK.

3. NIT (hardcoded season string) -- FIXED. Extracted to _CONTRACT_SEASON = "Spring/Summer 2026" module-level constant at line 1615 (in the diff). Used in both HTML and plain-text body.

DOMAIN REVIEW

Python/FastAPI/SQLAlchemy:

• send_contract_offer_email follows the same pattern as other email functions in this module: get client, build HTML + plain text, send, log to email_log, return message ID or None.
• EmailType.contract_offer already exists in the enum (model line 72, migration 033). No migration needed.
• escape() correctly applied to all user-supplied data (parent.name, player.name, team_name, contract_url).
• Proper try/except with logger.exception on failure, returns None.
• EmailLog entry committed with correct fields.
• Type hints present (-> str | None).
• Docstring follows conventions.

Test coverage (5 new tests added):

• test_email_body_contains_offer_copy -- verifies offer copy present, reminder copy absent (both HTML and plain text)
• test_email_log_uses_contract_offer_type -- verifies EmailLog entry uses correct EmailType
• test_kings_player_gets_red_accent -- verifies #d42026 in HTML for non-Queens team
• test_queens_player_gets_pink_accent -- verifies #e91e8c in HTML for Queens team (with dedicated queens_parent_with_player fixture)
• Happy path + error path (404, 422) + test_email redirect already covered from initial commit

BLOCKERS

None.

NITS

1. _CONTRACT_SEASON will need updating each season. It is at least a named constant now (easily greppable), but consider whether this should come from tenant config or a more central "current season" constant shared across email functions. Low priority -- current approach is fine for now.

2. _CONTRACT_PINK is now redundant with _BRAND_QUEENS_PINK. Both are #e91e8c. The reminder function still uses _CONTRACT_PINK but that is out of scope for this PR. Worth a cleanup ticket.

SOP COMPLIANCE

• Branch named after issue: 462-contract-offer-email
• PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
• Related references closing issue: Closes #462
• No secrets committed
• No scope creep -- changes limited to email function, route wiring, and tests
• Commit messages descriptive

PROCESS OBSERVATIONS

Clean fix cycle. All three items from the initial review addressed in a single commit. Test coverage went from 3 tests to 8 tests, covering offer copy correctness, email type logging, and accent color branching for both team types. The blocker (dead mocks) was a real correctness issue -- those tests would have passed even if send_contract_offer_email was never called.

VERDICT: APPROVED

## PR #467 Re-Review Re-review after fix commit addressing 1 blocker and 2 nits from the initial review. ### Previous Findings -- Verification 1. **BLOCKER (dead test mocks)** -- FIXED. `@patch("basketball_api.services.email.load_email_template")` and `mock_load_template` removed from `test_send_returns_200` and `test_test_email_param_redirects`. The new `send_contract_offer_email` uses inline HTML + `_brand_wrapper`, not `load_email_template`, so these mocks were dead. Now correctly only mocking `get_gmail_client`. 2. **NIT (accent not passed to `_brand_wrapper`)** -- FIXED. `send_contract_offer_email` computes `accent` from team name (`_BRAND_QUEENS_PINK` for Queens, `_BRAND_RED` otherwise) and passes `accent_color=accent` to `_brand_wrapper`. Correctly uses the canonical `_BRAND_QUEENS_PINK` constant rather than the duplicate `_CONTRACT_PINK`. 3. **NIT (hardcoded season string)** -- FIXED. Extracted to `_CONTRACT_SEASON = "Spring/Summer 2026"` module-level constant at line 1615 (in the diff). Used in both HTML and plain-text body. ### DOMAIN REVIEW **Python/FastAPI/SQLAlchemy:** - `send_contract_offer_email` follows the same pattern as other email functions in this module: get client, build HTML + plain text, send, log to `email_log`, return message ID or None. - `EmailType.contract_offer` already exists in the enum (model line 72, migration 033). No migration needed. - `escape()` correctly applied to all user-supplied data (`parent.name`, `player.name`, `team_name`, `contract_url`). - Proper `try/except` with `logger.exception` on failure, returns `None`. - `EmailLog` entry committed with correct fields. - Type hints present (`-> str | None`). - Docstring follows conventions. **Test coverage (5 new tests added):** - `test_email_body_contains_offer_copy` -- verifies offer copy present, reminder copy absent (both HTML and plain text) - `test_email_log_uses_contract_offer_type` -- verifies `EmailLog` entry uses correct `EmailType` - `test_kings_player_gets_red_accent` -- verifies `#d42026` in HTML for non-Queens team - `test_queens_player_gets_pink_accent` -- verifies `#e91e8c` in HTML for Queens team (with dedicated `queens_parent_with_player` fixture) - Happy path + error path (404, 422) + test_email redirect already covered from initial commit ### BLOCKERS None. ### NITS 1. **`_CONTRACT_SEASON` will need updating each season.** It is at least a named constant now (easily greppable), but consider whether this should come from tenant config or a more central "current season" constant shared across email functions. Low priority -- current approach is fine for now. 2. **`_CONTRACT_PINK` is now redundant with `_BRAND_QUEENS_PINK`.** Both are `#e91e8c`. The reminder function still uses `_CONTRACT_PINK` but that is out of scope for this PR. Worth a cleanup ticket. ### SOP COMPLIANCE - [x] Branch named after issue: `462-contract-offer-email` - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references closing issue: `Closes #462` - [x] No secrets committed - [x] No scope creep -- changes limited to email function, route wiring, and tests - [x] Commit messages descriptive ### PROCESS OBSERVATIONS Clean fix cycle. All three items from the initial review addressed in a single commit. Test coverage went from 3 tests to 8 tests, covering offer copy correctness, email type logging, and accent color branching for both team types. The blocker (dead mocks) was a real correctness issue -- those tests would have passed even if `send_contract_offer_email` was never called. ### VERDICT: APPROVED

forgejo_admin merged commit de5f11d83a into main 2026-04-13 14:36:03 +00:00

forgejo_admin deleted branch 462-contract-offer-email 2026-04-13 14:36:03 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-13 14:36:04 +00:00

fix: contract email sends offer copy, not reminder (#467)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/basketball-api!467

No description provided.