feat: add welcome practice email with parent-player meeting info #318
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo_admin/basketball-api!318
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "312-welcome-practice-email"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Add a welcome-to-the-season email that includes the April 7 parent-player meeting details and each team's weekly practice schedule queried dynamically from the
practice_schedulestable.Changes
src/basketball_api/services/email.py— addedsend_welcome_practice_email(),_format_day(),_format_time_12h()helpers. Uses branded inline HTML with meeting card and schedule table. Logs toemail_logwithEmailType.announcement.src/basketball_api/routes/admin.py— addedPOST /admin/email/welcome-practiceendpoint withtest_email,division(boys/girls/all), andteam_idquery params. Excludes TEST players, deduplicates by parent, queriespractice_schedulesper team.Test Plan
pytest tests/passes (467 passed; pre-existing failures in test_checkout and test_schedule unrelated to this change)test_email=draneylucas@gmail.comto verify rendering before bulk sendReview Checklist
EmailType.announcement)send_jersey_reminder_email)Related Notes
None.
Related
Closes #312
c5ebf31edf078bf6c34cQA Review
Scope Check
send_welcome_practice_email()added to email.py with meeting info + dynamic schedulePOST /admin/email/welcome-practiceadded to admin.py with division/team_id/test_email filters"TEST" in player.name.upper()emailed_parentssetis_active=True,ORDER BY day_of_weekEmailType.announcement(reuses existing enum -- no migration needed)_format_day()and_format_time_12h()helpers handle edge casesPattern Compliance
send_jersey_reminder_emailpattern exactly (client, subject, plain+html, send, log, commit)_brand_wrapper+ inline styles consistent with existing emails/email/*patterns (tenant lookup, query builder, test_email filter, error collection)Teamin email.py,PracticeSchedule+send_welcome_practice_emailin admin.py)Nits
HTTPException(400)is raised inside the player loop -- if an invalid division string is passed, it raises on the first player iteration rather than upfront. Functionally correct but could be moved before the loop for clarity. Low priority.Tests
VERDICT: APPROVED
PR #318 Review
DOMAIN REVIEW
Tech stack: Python / FastAPI / SQLAlchemy / Gmail SDK (inline HTML email)
Python/FastAPI quality:
send_welcome_practice_emailisstr | None, matching the pattern._format_day()and_format_time_12h()are clean, well-scoped helpers with proper fallback behavior.escape()fromhtmlmodule is used on all user-supplied data interpolated into HTML (parent name, player name, team name, location, day, times). XSS risk is mitigated.WelcomePracticeResponsemodel is correctly defined for the response.SQLAlchemy patterns:
joinedload(Parent.players).joinedload(Player.teams)which is redundant sincePlayer.teamsalready haslazy="selectin"on the model relationship (line 257 ofmodels.py). Not harmful, but unnecessary -- the selectin strategy will fire regardless. This is a nit, not a blocker.PracticeSchedulequery per-player inside the loop is an N+1 pattern. For the expected scale (dozens of parents, not thousands), this is acceptable. If the roster grows significantly, this could be batched by pre-fetching all active schedules keyed byteam_id. Acceptable for now.db.add()+db.commit()per email sent, consistent withsend_contract_reminder_emailand other senders.Email pattern consistency:
_brand_wrapper()+ inline HTML +EmailLogpattern used bysend_jersey_reminder_emailandsend_contract_reminder_email. Good consistency.email_logwithEmailType.announcement-- reuses existing enum value, no migration needed. Correct.Logic review:
emailed_parentsset +breakafter first eligible player) means each parent gets exactly one email for their first eligible player/team. This is documented in the docstring. The intent is clear.Parent.registration_token.isnot(None)filter ensures only registered parents receive emails. Reasonable gate."TEST" in player.name.upper()which is consistent with existing patterns in this codebase.HTTPException(400)for invalid values. Good.Hardcoded values concern:
BLOCKERS
1. No test coverage for new endpoint or service function.
This PR adds 306 lines of new functionality across two files:
POST /admin/email/welcome-practicewith three query params and complex filtering logic (division, team_id, TEST player exclusion, deduplication)send_welcome_practice_email()with HTML generation and email logging_format_day()and_format_time_12h()Zero tests were added. The test file
tests/test_admin_email.pyalready has extensive test patterns for similar email endpoints (profile reminders, roster export) that could be followed. At minimum:_format_day()edge cases (0-6 valid, out of range returns string)_format_time_12h()conversions (midnight, noon, PM, invalid input fallback)Per BLOCKER criteria: "New functionality with zero test coverage" is a blocker.
NITS
Redundant joinedload chain (line ~1196 of admin.py):
.joinedload(Player.teams)is redundant givenlazy="selectin"on thePlayer.teamsrelationship. Removing it would reduce query complexity without changing behavior.Hardcoded meeting details: The April 7 meeting info is baked into the email template. For future seasons, consider making meeting details configurable (query params or a config table). Low priority since this is a one-shot seasonal email.
practicesparameter typed aslist(line ~1741 of email.py): Thepracticesparameter is typed as barelistrather thanlist[PracticeSchedule]. Adding the type parameter would improve IDE support and documentation.SOP COMPLIANCE
312-welcome-practice-emailfollows{issue-number}-{kebab-case-purpose}conventionPROCESS OBSERVATIONS
test_emailparam mitigates this for manual testing, but automated tests would catch regression in filtering logic (division, deduplication, TEST exclusion) that manual testing might miss.VERDICT: NOT APPROVED
One blocker: zero test coverage for 306 lines of new functionality. The existing
test_admin_email.pyprovides clear patterns to follow. Add tests covering at minimum: happy path, division/team filtering, TEST player exclusion, deduplication, helper function edge cases, and error handling.PR #318 Re-Review
DOMAIN REVIEW
Stack: Python / FastAPI / SQLAlchemy
Previous findings -- verification:
No tests (BLOCKER) -- FIXED. 23 tests added in
tests/test_welcome_practice_email.pycovering: helper functions (_format_day,_format_time_12h) with happy path + edge cases, endpoint logic (division filtering, team_id filtering, TEST player exclusion, parent deduplication, registration token gating, auth enforcement, invalid division 400, send failure error capture). Solid coverage.Redundant joinedload -- On re-examination, this was a false finding in the original review.
Parent.playersuses default lazy loading (lazy="select"), so the explicitjoinedload(Parent.players)is correct and prevents N+1 queries. Correctly retained.Bare
listtype hint -- FIXED. Response model useslist[str](parametrized). Route-level variables uselist[str]andset[int].New observations:
player.teamaccessor: valid --Playermodel defines a@property team(line 260-263 in models.py) that returnsself.teams[0]from the many-to-many. Used consistently across the codebase.html.escape()applied to all user-supplied strings (parent name, player name, team name, location, day, time) in the HTML template. XSS-safe.errorslist. Does not abort the loop on single failure.EmailLogentry created withEmailType.announcement. Matches existing patterns (send_jersey_reminder_email,send_contract_reminder_email).require_admindependency. Test verifies 401/403 without auth.BLOCKERS
None.
NITS
Hardcoded event date: "Tuesday, April 7" and "West High Field House (241 N 300 W, SLC)" are hardcoded in the email template (both plain text and HTML). Acceptable for a one-time operational email, but if this pattern recurs, consider parameterizing meeting details as endpoint arguments or a config table.
One email per parent uses first eligible player only: The
breakafter sending means if a parent has two players on different teams, only the first player's team schedule is included. The deduplication test verifies this behavior, so it is intentional. Worth documenting in the endpoint docstring that only the first eligible player/team is used (not all of them).Division validation location: The
Division(division)enum validation happens inside the per-player loop, meaning it re-validates on every iteration. Moving it before the loop (once) would be marginally cleaner, though functionally equivalent since it raises HTTPException on first hit.SOP COMPLIANCE
312-welcome-practice-email)Closes #312)feat:)PROCESS OBSERVATIONS
VERDICT: APPROVED