ldraney/basketball-api

Fork 0

feat: add incomplete_profiles query to email_queries.py #309

Merged

forgejo_admin merged 1 commit from 308-incomplete-profiles-query into main

2026-04-03 22:59:17 +00:00

forgejo_admin commented

2026-04-03 22:45:48 +00:00

Contributor

Summary

Adds query_incomplete_profiles to the email query registry, enabling blast emails to parents whose players have incomplete profiles (missing photo_url, height, position, or date_of_birth).

Changes

src/basketball_api/services/email_queries.py -- Added query_incomplete_profiles function following the query_unsigned_contracts pattern. Returns per-player rows with to, parent_id, parent_name, player_name, team_name, missing_fields, and profile_url. Registered as "incomplete_profiles" in QUERY_REGISTRY. Imported settings from basketball_api.config for frontend_url.
tests/test_email_blast.py -- Added 6 tests in TestQueryIncompleteProfiles: returns players with missing fields, includes profile_url with token, excludes complete profiles, handles partial missing fields, test_email filtering, and registry registration.

Test Plan

All 23 tests pass (17 existing + 6 new): pytest tests/test_email_blast.py -v
ruff format and ruff check clean

Review Checklist

ruff format and ruff check pass
All existing tests still pass
New tests cover happy path, exclusion, partial fields, filtering, and registry
Follows query_unsigned_contracts pattern exactly
Jersey number/jersey size excluded from checked fields (admin-assigned)

None.

Closes #308

## Summary Adds `query_incomplete_profiles` to the email query registry, enabling blast emails to parents whose players have incomplete profiles (missing photo_url, height, position, or date_of_birth). ## Changes - `src/basketball_api/services/email_queries.py` -- Added `query_incomplete_profiles` function following the `query_unsigned_contracts` pattern. Returns per-player rows with `to`, `parent_id`, `parent_name`, `player_name`, `team_name`, `missing_fields`, and `profile_url`. Registered as `"incomplete_profiles"` in `QUERY_REGISTRY`. Imported `settings` from `basketball_api.config` for `frontend_url`. - `tests/test_email_blast.py` -- Added 6 tests in `TestQueryIncompleteProfiles`: returns players with missing fields, includes profile_url with token, excludes complete profiles, handles partial missing fields, test_email filtering, and registry registration. ## Test Plan - All 23 tests pass (17 existing + 6 new): `pytest tests/test_email_blast.py -v` - ruff format and ruff check clean ## Review Checklist - [x] ruff format and ruff check pass - [x] All existing tests still pass - [x] New tests cover happy path, exclusion, partial fields, filtering, and registry - [x] Follows query_unsigned_contracts pattern exactly - [x] Jersey number/jersey size excluded from checked fields (admin-assigned) ## Related Notes None. ## Related Closes #308

forgejo_admin added 1 commit

2026-04-03 22:45:48 +00:00

feat: add incomplete_profiles query to email_queries.py

ci/woodpecker/pr/woodpecker Pipeline failed

Details

e8968619bd

Adds query_incomplete_profiles to find rostered players missing profile
fields (photo_url, height, position, date_of_birth) and returns
per-recipient data with missing_fields list and profile_url. Registered
as "incomplete_profiles" in QUERY_REGISTRY.

Closes #308

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

forgejo_admin commented

2026-04-03 22:46:55 +00:00

Author

Contributor

QA Review

Pattern Compliance

query_incomplete_profiles follows query_unsigned_contracts exactly: same function signature (db, tenant, test_email), same join/options/filter chain, same result dict structure, same test_email filtering logic.

Fields Checked

photo_url, height, position, date_of_birth -- matches issue spec. Jersey number/size correctly excluded (admin-assigned).

Profile URL

Uses settings.frontend_url from basketball_api.config with /register?token={parent.registration_token} -- matches issue spec.

Registry

Registered as "incomplete_profiles" in QUERY_REGISTRY -- correct.

Falsy Check

not getattr(player, f) treats None as missing. These are nullable columns that default to None when unset, so this is correct. Empty string would also be treated as missing, which is reasonable behavior.

Join Semantics

Inner join on player_teams means unrostered players are excluded -- correct, only rostered players should receive profile completion emails.

Tests

6 tests covering: happy path with all fields missing, profile_url with token, complete profile exclusion, partial missing fields, test_email filtering, and registry registration. Good coverage.

Nits

None.

VERDICT: APPROVED

## QA Review ### Pattern Compliance `query_incomplete_profiles` follows `query_unsigned_contracts` exactly: same function signature (`db`, `tenant`, `test_email`), same join/options/filter chain, same result dict structure, same `test_email` filtering logic. ### Fields Checked `photo_url`, `height`, `position`, `date_of_birth` -- matches issue spec. Jersey number/size correctly excluded (admin-assigned). ### Profile URL Uses `settings.frontend_url` from `basketball_api.config` with `/register?token={parent.registration_token}` -- matches issue spec. ### Registry Registered as `"incomplete_profiles"` in `QUERY_REGISTRY` -- correct. ### Falsy Check `not getattr(player, f)` treats `None` as missing. These are nullable columns that default to `None` when unset, so this is correct. Empty string would also be treated as missing, which is reasonable behavior. ### Join Semantics Inner join on `player_teams` means unrostered players are excluded -- correct, only rostered players should receive profile completion emails. ### Tests 6 tests covering: happy path with all fields missing, profile_url with token, complete profile exclusion, partial missing fields, test_email filtering, and registry registration. Good coverage. ### Nits None. **VERDICT: APPROVED**

forgejo_admin commented

2026-04-03 22:48:48 +00:00

Author

Contributor

PR #309 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy

The new query_incomplete_profiles function follows the established query_unsigned_contracts pattern closely. The query structure is sound: joins Player to player_teams, eager-loads parent and teams via joinedload, filters by tenant, and post-processes in Python to identify missing fields. The _INCOMPLETE_PROFILE_FIELDS tuple is a clean constant. The test_email keyword-only parameter follows the same filtering contract as the existing query.

SQLAlchemy patterns: joinedload for Player.parent and Player.teams avoids N+1 queries. The .distinct() is appropriate given the join to the association table. Session management follows the existing pattern (caller-owned session).

Type hints / PEP 484: Function signature is properly typed with list[dict] return and str | None for test_email. Docstring is thorough (PEP 257 compliant).

Test coverage: 6 new tests covering: happy path (missing all fields), profile URL correctness, exclusion of complete profiles, partial missing fields, test_email filtering, and registry registration. This is solid coverage.

BLOCKERS

None.

NITS

registration_token could be None (/home/ldraney/basketball-api/src/basketball_api/models.py:183): Parent.registration_token is Mapped[str | None]. If a parent has no token, the profile URL becomes .../register?token=None -- a broken link. Other query endpoints in admin.py (lines 578, 867) explicitly filter .filter(Parent.registration_token.isnot(None)). Consider adding a guard:
```
if not parent.registration_token:
    continue
```
This is a pre-existing pattern gap (the same unguarded usage exists in email.py:92), so not a blocker for this PR, but worth hardening.
Missing fields display names: The missing_fields value uses raw column names (photo_url, date_of_birth). If these appear in user-facing emails, a display-name mapping (e.g., "photo_url" -> "Photo", "date_of_birth" -> "Date of Birth") would be friendlier. This is a template/UX concern rather than a query concern -- the template can handle the mapping.
No test for parentless edge case: While parent_id is non-nullable (Mapped[int]), a test confirming behavior when a player has no team roster entry (no player_teams row) would strengthen coverage. Currently, such players are implicitly excluded by the join, which is correct but undocumented.

SOP COMPLIANCE

Branch named after issue: 308-incomplete-profiles-query
PR body follows template (Summary, Changes, Test Plan, Related)
Related references plan slug -- "None" listed, acceptable for standalone ticket
No secrets committed
No scope creep -- exactly 2 files changed, both directly related
Commit messages are descriptive
ruff format and ruff check reported clean
All tests reported passing (23 total)

PROCESS OBSERVATIONS

Clean, focused PR. 255 additions, 0 deletions, 2 files. The query-per-email-type pattern scales well for the blast system. Deployment risk is low -- this adds a new registry entry without modifying existing code paths.

VERDICT: APPROVED

## PR #309 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy The new `query_incomplete_profiles` function follows the established `query_unsigned_contracts` pattern closely. The query structure is sound: joins `Player` to `player_teams`, eager-loads `parent` and `teams` via `joinedload`, filters by tenant, and post-processes in Python to identify missing fields. The `_INCOMPLETE_PROFILE_FIELDS` tuple is a clean constant. The `test_email` keyword-only parameter follows the same filtering contract as the existing query. **SQLAlchemy patterns**: `joinedload` for `Player.parent` and `Player.teams` avoids N+1 queries. The `.distinct()` is appropriate given the join to the association table. Session management follows the existing pattern (caller-owned session). **Type hints / PEP 484**: Function signature is properly typed with `list[dict]` return and `str | None` for `test_email`. Docstring is thorough (PEP 257 compliant). **Test coverage**: 6 new tests covering: happy path (missing all fields), profile URL correctness, exclusion of complete profiles, partial missing fields, `test_email` filtering, and registry registration. This is solid coverage. ### BLOCKERS None. ### NITS 1. **`registration_token` could be None** (`/home/ldraney/basketball-api/src/basketball_api/models.py:183`): `Parent.registration_token` is `Mapped[str | None]`. If a parent has no token, the profile URL becomes `.../register?token=None` -- a broken link. Other query endpoints in `admin.py` (lines 578, 867) explicitly filter `.filter(Parent.registration_token.isnot(None))`. Consider adding a guard: ```python if not parent.registration_token: continue ``` This is a pre-existing pattern gap (the same unguarded usage exists in `email.py:92`), so not a blocker for this PR, but worth hardening. 2. **Missing fields display names**: The `missing_fields` value uses raw column names (`photo_url`, `date_of_birth`). If these appear in user-facing emails, a display-name mapping (e.g., `"photo_url" -> "Photo"`, `"date_of_birth" -> "Date of Birth"`) would be friendlier. This is a template/UX concern rather than a query concern -- the template can handle the mapping. 3. **No test for parentless edge case**: While `parent_id` is non-nullable (`Mapped[int]`), a test confirming behavior when a player has no team roster entry (no `player_teams` row) would strengthen coverage. Currently, such players are implicitly excluded by the join, which is correct but undocumented. ### SOP COMPLIANCE - [x] Branch named after issue: `308-incomplete-profiles-query` - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [ ] Related references plan slug -- "None" listed, acceptable for standalone ticket - [x] No secrets committed - [x] No scope creep -- exactly 2 files changed, both directly related - [x] Commit messages are descriptive - [x] ruff format and ruff check reported clean - [x] All tests reported passing (23 total) ### PROCESS OBSERVATIONS Clean, focused PR. 255 additions, 0 deletions, 2 files. The query-per-email-type pattern scales well for the blast system. Deployment risk is low -- this adds a new registry entry without modifying existing code paths. ### VERDICT: APPROVED