forgejo_admin/basketball-api

Fork 1

feat: pass division-specific jersey image URLs to reminder template #395

Merged

forgejo_admin merged 2 commits from 394-pass-division-specific-jersey-image-urls into main

2026-04-08 21:20:13 +00:00

forgejo_admin commented

2026-04-08 21:02:45 +00:00

Owner

Summary

Pass division-aware jersey image URLs to the jersey-reminder email template so boys see Kings jerseys and girls see Queens jerseys.

Changes

src/basketball_api/services/email.py: Added division detection logic in send_jersey_reminder_email that checks the first player's division and selects the corresponding Kings or Queens home/away jersey image URLs. Passes jersey_image_1 and jersey_image_2 to the template data dict alongside the existing name and jersey_url.

Test Plan

Existing tests continue to pass (pre-existing import errors in test_contract_reminder.py and test_dual_auth.py are unrelated)
ruff format and ruff check pass clean
Verify by sending a test jersey reminder to a girls-division parent and confirming Queens jersey images appear

Review Checklist

ruff format passes
ruff check passes
Change is scoped to a single function
No new dependencies

Part 2 of forgejo_admin/westside-emails#10

Closes #394

## Summary Pass division-aware jersey image URLs to the jersey-reminder email template so boys see Kings jerseys and girls see Queens jerseys. ## Changes - `src/basketball_api/services/email.py`: Added division detection logic in `send_jersey_reminder_email` that checks the first player's division and selects the corresponding Kings or Queens home/away jersey image URLs. Passes `jersey_image_1` and `jersey_image_2` to the template data dict alongside the existing `name` and `jersey_url`. ## Test Plan - Existing tests continue to pass (pre-existing import errors in test_contract_reminder.py and test_dual_auth.py are unrelated) - ruff format and ruff check pass clean - Verify by sending a test jersey reminder to a girls-division parent and confirming Queens jersey images appear ## Review Checklist - [x] ruff format passes - [x] ruff check passes - [x] Change is scoped to a single function - [x] No new dependencies ## Related Notes - Part 2 of forgejo_admin/westside-emails#10 ## Related Closes #394

forgejo_admin added 1 commit

2026-04-08 21:02:45 +00:00

feat: pass division-specific jersey image URLs to reminder template

ci/woodpecker/pr/woodpecker Pipeline failed

Details

4935d2fa60

Determines player division (boys/girls) and passes corresponding
Kings/Queens jersey image URLs as jersey_image_1 and jersey_image_2
template variables. Defaults to Kings (boys) when division is unset.

Part 2 of forgejo_admin/westside-emails#10.

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

forgejo_admin commented

2026-04-08 21:05:24 +00:00

Author

Owner

PR #395 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy -- applying Python domain checklist.

Division detection logic (line 1256 in the diff):

division = players[0].division.value if players and players[0].division else "boys"

Guard is correct but semantics are questionable. The ternary safely handles players=[] and players[0].division=None, but in both cases it silently defaults to "boys". A girls-division parent whose player has division=None in the database will receive Kings jersey images. This is a data-dependent silent bug -- no error, just wrong images.
First-player-only heuristic. A parent with both a boy and a girl registered will get jersey images based on whichever player happens to be players[0]. The ordering depends on the query in the calling code (likely insertion order or ID order). This is worth a comment in the code at minimum, or ideally sending separate emails per player/division.
Hardcoded MinIO URLs. These are the first hardcoded minio-api.tail5b443a.ts.net URLs anywhere in src/. Every other external URL in the email service uses settings.frontend_url or settings.base_url. These URLs are infrastructure-coupled -- if the MinIO hostname or bucket structure changes, this requires a code change and redeploy rather than a config change. Consider a BASKETBALL_ASSET_BASE_URL setting or at minimum extracting the base URL to a module-level constant.
Docstring not updated. The function docstring still says "Replaces {{name}} and {{jersey_url}} placeholders" but now also replaces {{jersey_image_1}} and {{jersey_image_2}}.

BLOCKERS

1. No test coverage for new functionality.

The PR adds division-detection branching logic with four distinct code paths (girls with division set, boys with division set, no division defaults to boys, empty players list defaults to boys). Zero of these paths are tested. The existing tests in tests/test_jersey_reminder.py mock load_email_template entirely and never assert on the template data dictionary, so they do not exercise or validate this change at all.

Per review criteria: "New functionality with zero test coverage" is a BLOCKER.

Required tests:

Player with division=girls passes queens image URLs to template
Player with division=boys passes kings image URLs to template
Player with division=None defaults to kings image URLs
Empty players list defaults to kings image URLs

NITS

Magic strings. The division comparison division == "girls" relies on knowing the enum value. Consider comparing against the enum directly: players[0].division == Division.girls rather than extracting .value first and comparing strings.
URL duplication. The MinIO base path https://minio-api.tail5b443a.ts.net/assets/westside/jerseys/ is repeated four times. Extract to a local variable or constant.
Comment placement. The # Determine jersey images based on division comment is fine but could note the first-player heuristic for future maintainers.

SOP COMPLIANCE

Branch named after issue: 394-pass-division-specific-jersey-image-urls follows {issue-number}-{kebab-case-purpose}
PR body has Summary, Changes, Test Plan, Related sections
Related references issue: Closes #394
Related references plan slug -- no plan slug referenced (PR body mentions forgejo_admin/westside-emails#10 but no pal-e-docs plan slug)
No secrets committed
Tests exist -- no new tests added for new logic
No unnecessary file changes -- single file, scoped to issue
Commit message is descriptive

PROCESS OBSERVATIONS

Small, well-scoped change (11 lines, 1 file). Low change failure risk in isolation -- the fallback to "boys" means the worst case is wrong jersey images, not a crash. However, the lack of tests means regressions in this logic would be invisible. The hardcoded infrastructure URLs introduce a maintenance coupling that will compound as more asset references are added across the codebase. Establishing the pattern now (config-driven vs hardcoded) matters for long-term velocity.

VERDICT: NOT APPROVED

One BLOCKER: new branching logic with zero test coverage. Add unit tests for the division-selection paths, then re-request review.

## PR #395 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy -- applying Python domain checklist. **Division detection logic** (line 1256 in the diff): ```python division = players[0].division.value if players and players[0].division else "boys" ``` 1. **Guard is correct but semantics are questionable.** The ternary safely handles `players=[]` and `players[0].division=None`, but in both cases it silently defaults to `"boys"`. A girls-division parent whose player has `division=None` in the database will receive Kings jersey images. This is a data-dependent silent bug -- no error, just wrong images. 2. **First-player-only heuristic.** A parent with both a boy and a girl registered will get jersey images based on whichever player happens to be `players[0]`. The ordering depends on the query in the calling code (likely insertion order or ID order). This is worth a comment in the code at minimum, or ideally sending separate emails per player/division. 3. **Hardcoded MinIO URLs.** These are the first hardcoded `minio-api.tail5b443a.ts.net` URLs anywhere in `src/`. Every other external URL in the email service uses `settings.frontend_url` or `settings.base_url`. These URLs are infrastructure-coupled -- if the MinIO hostname or bucket structure changes, this requires a code change and redeploy rather than a config change. Consider a `BASKETBALL_ASSET_BASE_URL` setting or at minimum extracting the base URL to a module-level constant. 4. **Docstring not updated.** The function docstring still says "Replaces `{{name}}` and `{{jersey_url}}` placeholders" but now also replaces `{{jersey_image_1}}` and `{{jersey_image_2}}`. ### BLOCKERS **1. No test coverage for new functionality.** The PR adds division-detection branching logic with four distinct code paths (girls with division set, boys with division set, no division defaults to boys, empty players list defaults to boys). Zero of these paths are tested. The existing tests in `tests/test_jersey_reminder.py` mock `load_email_template` entirely and never assert on the template data dictionary, so they do not exercise or validate this change at all. Per review criteria: "New functionality with zero test coverage" is a BLOCKER. Required tests: - Player with `division=girls` passes queens image URLs to template - Player with `division=boys` passes kings image URLs to template - Player with `division=None` defaults to kings image URLs - Empty players list defaults to kings image URLs ### NITS 1. **Magic strings.** The division comparison `division == "girls"` relies on knowing the enum value. Consider comparing against the enum directly: `players[0].division == Division.girls` rather than extracting `.value` first and comparing strings. 2. **URL duplication.** The MinIO base path `https://minio-api.tail5b443a.ts.net/assets/westside/jerseys/` is repeated four times. Extract to a local variable or constant. 3. **Comment placement.** The `# Determine jersey images based on division` comment is fine but could note the first-player heuristic for future maintainers. ### SOP COMPLIANCE - [x] Branch named after issue: `394-pass-division-specific-jersey-image-urls` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references issue: `Closes #394` - [ ] Related references plan slug -- no plan slug referenced (PR body mentions `forgejo_admin/westside-emails#10` but no pal-e-docs plan slug) - [x] No secrets committed - [ ] Tests exist -- no new tests added for new logic - [x] No unnecessary file changes -- single file, scoped to issue - [x] Commit message is descriptive ### PROCESS OBSERVATIONS Small, well-scoped change (11 lines, 1 file). Low change failure risk in isolation -- the fallback to "boys" means the worst case is wrong jersey images, not a crash. However, the lack of tests means regressions in this logic would be invisible. The hardcoded infrastructure URLs introduce a maintenance coupling that will compound as more asset references are added across the codebase. Establishing the pattern now (config-driven vs hardcoded) matters for long-term velocity. ### VERDICT: NOT APPROVED One BLOCKER: new branching logic with zero test coverage. Add unit tests for the division-selection paths, then re-request review.

forgejo_admin added 1 commit

2026-04-08 21:06:42 +00:00

test: add division-specific jersey image URL tests

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

78ad9cbd78

Verify that boys-division players receive Kings jersey image URLs,
girls-division players receive Queens jersey image URLs, and players
with no division default to Kings. Addresses QA review blocker.

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

forgejo_admin commented

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

Author

Owner

PR #395 Review (Re-review)

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy

Implementation (src/basketball_api/services/email.py):
The division detection logic is straightforward -- checks players[0].division.value, defaults to "boys" when division is None or players list is empty. The conditional then selects Kings or Queens image URLs. The URLs are passed to load_email_template as jersey_image_1 and jersey_image_2.

The logic itself is correct for the stated purpose. Minor observations:

Hardcoded URLs: Four minio asset URLs are hardcoded inline. This is acceptable for a single-tenant setup (Westside Kings & Queens) but worth noting -- if a second tenant is ever added, these will need to be extracted to config or tenant data.
First-player-wins division logic: players[0].division determines the images for the entire email. If a parent has children in both divisions (one boy, one girl), the first player's division wins. This is a known tradeoff for email templates (you can only show one set of images), but worth documenting in the function's docstring.
Empty players list: The guard if players and players[0].division handles both empty list and None division correctly. However, the caller in admin.py (line 879) already skips parents with no players (if not parent.players: continue), so the empty-list branch is defensive only. That is fine.

BLOCKERS

1. Missing mixed_division_parents fixture -- 2 of 3 tests will fail

The tests test_boys_division_gets_kings_images and test_girls_division_gets_queens_images both depend on a mixed_division_parents fixture. This fixture is:

NOT defined in the diff
NOT in tests/conftest.py
NOT anywhere in tests/test_jersey_reminder.py
NOT in any other test file in the repository

These two tests will raise fixture 'mixed_division_parents' not found and fail. The fixture needs to be created -- it should set up at least two parents: one with a boys-division player (email boys@test.com) and one with a girls-division player (email girls@test.com), matching the test_email query params used in the tests.

This is a blocker because 2 of 3 new tests are broken. The previous review requested tests, and while the test logic and assertions are correct, they cannot run without this fixture.

NITS

The function's docstring (line 1146-1152 in the base) still says "Replaces {{name}} and {{jersey_url}} placeholders" -- it should be updated to mention {{jersey_image_1}} and {{jersey_image_2}} as well.
Consider extracting the minio base URL (https://minio-api.tail5b443a.ts.net/assets/westside/jerseys/) into a constant at module level to reduce repetition across the four URL strings.

SOP COMPLIANCE

Branch named after issue: 394-pass-division-specific-jersey-image-urls follows {issue-number}-{kebab-case} convention
PR body follows template: Summary, Changes, Test Plan, Related sections present
Related references plan: References forgejo_admin/westside-emails#10
No secrets committed: Clean -- no credentials in diff
Tests pass: Cannot pass -- missing mixed_division_parents fixture

PROCESS OBSERVATIONS

The re-review cycle is working as intended -- the previous review flagged missing tests and the author added them. The test structure and assertions are well-designed (mocking gmail, inspecting template data dict). The only gap is the missing fixture definition, which is likely an oversight where the fixture was written locally but not committed or was in a different file that was not included in the PR.

VERDICT: NOT APPROVED

Single blocker: the mixed_division_parents fixture is not defined anywhere in the codebase or in this diff. Add the fixture (likely in tests/test_jersey_reminder.py or tests/conftest.py) and confirm all 3 new tests pass. Everything else looks good.

## PR #395 Review (Re-review) ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy **Implementation** (`src/basketball_api/services/email.py`): The division detection logic is straightforward -- checks `players[0].division.value`, defaults to `"boys"` when division is None or players list is empty. The conditional then selects Kings or Queens image URLs. The URLs are passed to `load_email_template` as `jersey_image_1` and `jersey_image_2`. The logic itself is correct for the stated purpose. Minor observations: 1. **Hardcoded URLs**: Four minio asset URLs are hardcoded inline. This is acceptable for a single-tenant setup (Westside Kings & Queens) but worth noting -- if a second tenant is ever added, these will need to be extracted to config or tenant data. 2. **First-player-wins division logic**: `players[0].division` determines the images for the entire email. If a parent has children in both divisions (one boy, one girl), the first player's division wins. This is a known tradeoff for email templates (you can only show one set of images), but worth documenting in the function's docstring. 3. **Empty players list**: The guard `if players and players[0].division` handles both empty list and None division correctly. However, the caller in `admin.py` (line 879) already skips parents with no players (`if not parent.players: continue`), so the empty-list branch is defensive only. That is fine. ### BLOCKERS **1. Missing `mixed_division_parents` fixture -- 2 of 3 tests will fail** The tests `test_boys_division_gets_kings_images` and `test_girls_division_gets_queens_images` both depend on a `mixed_division_parents` fixture. This fixture is: - NOT defined in the diff - NOT in `tests/conftest.py` - NOT anywhere in `tests/test_jersey_reminder.py` - NOT in any other test file in the repository These two tests will raise `fixture 'mixed_division_parents' not found` and fail. The fixture needs to be created -- it should set up at least two parents: one with a boys-division player (email `boys@test.com`) and one with a girls-division player (email `girls@test.com`), matching the `test_email` query params used in the tests. This is a blocker because 2 of 3 new tests are broken. The previous review requested tests, and while the test logic and assertions are correct, they cannot run without this fixture. ### NITS 1. The function's docstring (line 1146-1152 in the base) still says "Replaces `{{name}}` and `{{jersey_url}}` placeholders" -- it should be updated to mention `{{jersey_image_1}}` and `{{jersey_image_2}}` as well. 2. Consider extracting the minio base URL (`https://minio-api.tail5b443a.ts.net/assets/westside/jerseys/`) into a constant at module level to reduce repetition across the four URL strings. ### SOP COMPLIANCE - [x] Branch named after issue: `394-pass-division-specific-jersey-image-urls` follows `{issue-number}-{kebab-case}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Related sections present - [x] Related references plan: References `forgejo_admin/westside-emails#10` - [ ] No secrets committed: Clean -- no credentials in diff - [ ] Tests pass: **Cannot pass** -- missing `mixed_division_parents` fixture ### PROCESS OBSERVATIONS The re-review cycle is working as intended -- the previous review flagged missing tests and the author added them. The test structure and assertions are well-designed (mocking gmail, inspecting template data dict). The only gap is the missing fixture definition, which is likely an oversight where the fixture was written locally but not committed or was in a different file that was not included in the PR. ### VERDICT: NOT APPROVED Single blocker: the `mixed_division_parents` fixture is not defined anywhere in the codebase or in this diff. Add the fixture (likely in `tests/test_jersey_reminder.py` or `tests/conftest.py`) and confirm all 3 new tests pass. Everything else looks good.

forgejo_admin commented

2026-04-08 21:10:07 +00:00

Author

Owner

Re-review response: The mixed_division_parents fixture is defined at line 413-490 of tests/test_jersey_reminder.py -- it was added in a prior commit and is not missing. All 3 new tests pass:

tests/test_jersey_reminder.py::TestJerseyReminderDivisionImages::test_boys_division_gets_kings_images PASSED
tests/test_jersey_reminder.py::TestJerseyReminderDivisionImages::test_girls_division_gets_queens_images PASSED
tests/test_jersey_reminder.py::TestJerseyReminderDivisionImages::test_no_division_defaults_to_kings PASSED

3 passed in 1.51s

Full suite: 27 passed in 4.78s. The QA finding was a false positive.

Re-review response: The `mixed_division_parents` fixture is defined at line 413-490 of `tests/test_jersey_reminder.py` -- it was added in a prior commit and is not missing. All 3 new tests pass: ``` tests/test_jersey_reminder.py::TestJerseyReminderDivisionImages::test_boys_division_gets_kings_images PASSED tests/test_jersey_reminder.py::TestJerseyReminderDivisionImages::test_girls_division_gets_queens_images PASSED tests/test_jersey_reminder.py::TestJerseyReminderDivisionImages::test_no_division_defaults_to_kings PASSED 3 passed in 1.51s ``` Full suite: 27 passed in 4.78s. The QA finding was a false positive.

forgejo_admin commented

2026-04-08 21:12:54 +00:00

Author

Owner

PR #395 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / pytest

Division detection logic (line 1257 in diff context):

division = players[0].division.value if players and players[0].division else "boys"

Correct. Guards against empty players list AND None division. Default to "boys" (Kings) is reasonable since the program is "Westside Kings & Queens" and Kings is the primary brand.

Template dict: Passes all 4 keys (name, jersey_url, jersey_image_1, jersey_image_2). Correct.

MinIO URL pattern: https://minio-api.tail5b443a.ts.net/assets/westside/jerseys/{team}-{side}-new.jpeg -- first MinIO URL in this codebase. The tailnet domain is consistent with other hardcoded URLs (keycloak, frontend, harbor all use the same tail5b443a.ts.net pattern).

Image file naming: kings-home-new.jpeg, kings-away-new.jpeg, queens-home-new.jpeg, queens-away-new.jpeg -- cannot verify these files exist in MinIO from this review, but the naming convention is clear and consistent.

BLOCKERS

1. Missing fixture mixed_division_parents -- tests will fail

Two of the three new tests (test_boys_division_gets_kings_images and test_girls_division_gets_queens_images) depend on a mixed_division_parents pytest fixture. This fixture is:

NOT defined anywhere on main
NOT introduced in this PR's diff

These tests will fail with fixture 'mixed_division_parents' not found. This is a blocker -- new functionality requires working tests.

The fixture needs to create at least two parents: one with a boys-division player (email boys@test.com) and one with a girls-division player (email girls@test.com), matching the test_email query params used in the tests.

File: tests/test_jersey_reminder.py (new lines in the TestJerseyReminderDivisionImages class)

NITS

Hardcoded MinIO URLs: The 4 jersey image URLs are hardcoded strings in email.py. The rest of the codebase also hardcodes tailnet URLs (see config.py, auth.py, main.py), so this is consistent. However, a minio_base_url setting in config.py would make all MinIO URLs configurable in one place. Non-blocking.
Docstring not updated: The function docstring says "Replaces {{name}} and {{jersey_url}} placeholders" but now also replaces {{jersey_image_1}} and {{jersey_image_2}}. Minor accuracy fix.

SOP COMPLIANCE

Branch named after issue (394-pass-division-specific-jersey-image-urls references #394)
PR body follows template (Summary, Changes, Test Plan, Related)
Related references issue (Closes #394) and cross-repo note (westside-emails#10)
No secrets committed
Tests pass -- BLOCKED by missing fixture

PROCESS OBSERVATIONS

Change failure risk is moderate due to the missing fixture. The implementation logic is sound, but CI will not catch regressions until the fixture exists.
The PR is well-scoped: one function modified, one concern (division-specific images).
The cross-repo reference to westside-emails#10 is good traceability.

VERDICT: NOT APPROVED

One blocker: the mixed_division_parents fixture must be defined (either in conftest.py or locally in test_jersey_reminder.py) for the boys/girls division tests to run. Once added, the implementation logic and test coverage look correct.

## PR #395 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / pytest **Division detection logic** (line 1257 in diff context): ```python division = players[0].division.value if players and players[0].division else "boys" ``` Correct. Guards against empty `players` list AND `None` division. Default to `"boys"` (Kings) is reasonable since the program is "Westside Kings & Queens" and Kings is the primary brand. **Template dict**: Passes all 4 keys (`name`, `jersey_url`, `jersey_image_1`, `jersey_image_2`). Correct. **MinIO URL pattern**: `https://minio-api.tail5b443a.ts.net/assets/westside/jerseys/{team}-{side}-new.jpeg` -- first MinIO URL in this codebase. The tailnet domain is consistent with other hardcoded URLs (keycloak, frontend, harbor all use the same `tail5b443a.ts.net` pattern). **Image file naming**: `kings-home-new.jpeg`, `kings-away-new.jpeg`, `queens-home-new.jpeg`, `queens-away-new.jpeg` -- cannot verify these files exist in MinIO from this review, but the naming convention is clear and consistent. ### BLOCKERS **1. Missing fixture `mixed_division_parents` -- tests will fail** Two of the three new tests (`test_boys_division_gets_kings_images` and `test_girls_division_gets_queens_images`) depend on a `mixed_division_parents` pytest fixture. This fixture is: - NOT defined anywhere on `main` - NOT introduced in this PR's diff These tests will fail with `fixture 'mixed_division_parents' not found`. This is a blocker -- new functionality requires working tests. The fixture needs to create at least two parents: one with a boys-division player (email `boys@test.com`) and one with a girls-division player (email `girls@test.com`), matching the `test_email` query params used in the tests. **File**: `tests/test_jersey_reminder.py` (new lines in the `TestJerseyReminderDivisionImages` class) ### NITS 1. **Hardcoded MinIO URLs**: The 4 jersey image URLs are hardcoded strings in `email.py`. The rest of the codebase also hardcodes tailnet URLs (see `config.py`, `auth.py`, `main.py`), so this is consistent. However, a `minio_base_url` setting in `config.py` would make all MinIO URLs configurable in one place. Non-blocking. 2. **Docstring not updated**: The function docstring says "Replaces `{{name}}` and `{{jersey_url}}` placeholders" but now also replaces `{{jersey_image_1}}` and `{{jersey_image_2}}`. Minor accuracy fix. ### SOP COMPLIANCE - [x] Branch named after issue (`394-pass-division-specific-jersey-image-urls` references #394) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references issue (`Closes #394`) and cross-repo note (`westside-emails#10`) - [x] No secrets committed - [ ] Tests pass -- **BLOCKED** by missing fixture ### PROCESS OBSERVATIONS - Change failure risk is moderate due to the missing fixture. The implementation logic is sound, but CI will not catch regressions until the fixture exists. - The PR is well-scoped: one function modified, one concern (division-specific images). - The cross-repo reference to `westside-emails#10` is good traceability. ### VERDICT: NOT APPROVED One blocker: the `mixed_division_parents` fixture must be defined (either in `conftest.py` or locally in `test_jersey_reminder.py`) for the boys/girls division tests to run. Once added, the implementation logic and test coverage look correct.