forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 88 Pull requests 9 Projects Releases Packages Wiki Activity Actions

fix: update confirmation email, Stripe labels, and API team_preference #385

Merged
forgejo_admin merged 2 commits from 383-fix-confirmation-email into main 2026-04-07 19:30:45 +00:00
Conversation 1 Commits 2 Files changed 2 +40 -208
forgejo_admin commented 2026-04-07 19:21:45 +00:00
Owner
Copy link

Summary

Replace hardcoded tryout-specific confirmation email with a generic registration confirmation that uses load_email_template() for HTML rendering. Update Stripe product names and add team_preference to the API registration endpoint.

Changes

  • src/basketball_api/services/email.py:

    • Changed subject line from "Complete {name}'s Tryout Registration" to "{name}'s Registration Confirmed"
    • Rewrote plaintext body: removed tryout date/time/location, added division, profile URL, and coach contact
    • Replaced _build_confirmation_html() call with load_email_template("registration-confirmation", ...) with fallback to plaintext
    • Deleted _build_confirmation_html() (~150 lines) and _build_credentials_html() (~25 lines) — no other callers
    • Added team_preference to admin registration notification (both plaintext and HTML)

  • src/basketball_api/routes/register.py:

    • Changed Stripe product label from "Tryout" / "Remote" to "Registration" / "Remote Evaluation"
    • Added team_preference: Optional[str] = None to APIRegistrationRequest model
    • Added team_preference handling in API handler for both existing and new players

Test Plan

  • pytest tests/ -k "test_register or test_email" — 66 tests pass
  • Verify confirmation email renders correctly when registration-confirmation.html template exists
  • Verify plaintext fallback works when template file is missing
  • Verify Stripe checkout shows updated product name

Review Checklist

  • ruff format and ruff check pass
  • 66 tests pass
  • No other callers of deleted functions (grep confirmed)
  • Graceful fallback when MJML template not yet deployed

Related Notes

None.

Related

Closes #383

## Summary Replace hardcoded tryout-specific confirmation email with a generic registration confirmation that uses `load_email_template()` for HTML rendering. Update Stripe product names and add `team_preference` to the API registration endpoint. ## Changes - **`src/basketball_api/services/email.py`**: - Changed subject line from "Complete {name}'s Tryout Registration" to "{name}'s Registration Confirmed" - Rewrote plaintext body: removed tryout date/time/location, added division, profile URL, and coach contact - Replaced `_build_confirmation_html()` call with `load_email_template("registration-confirmation", ...)` with fallback to plaintext - Deleted `_build_confirmation_html()` (~150 lines) and `_build_credentials_html()` (~25 lines) — no other callers - Added `team_preference` to admin registration notification (both plaintext and HTML) - **`src/basketball_api/routes/register.py`**: - Changed Stripe product label from "Tryout" / "Remote" to "Registration" / "Remote Evaluation" - Added `team_preference: Optional[str] = None` to `APIRegistrationRequest` model - Added `team_preference` handling in API handler for both existing and new players ## Test Plan - `pytest tests/ -k "test_register or test_email"` — 66 tests pass - Verify confirmation email renders correctly when `registration-confirmation.html` template exists - Verify plaintext fallback works when template file is missing - Verify Stripe checkout shows updated product name ## Review Checklist - [x] ruff format and ruff check pass - [x] 66 tests pass - [x] No other callers of deleted functions (grep confirmed) - [x] Graceful fallback when MJML template not yet deployed ## Related Notes None. ## Related Closes #383
feat: include is_public in admin players list response
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
3c167eae94 
The AdminPlayerItem model and GET /admin/players response now include
the is_public boolean field, enabling the westside-app admin UI to
display and toggle player visibility state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace hardcoded tryout-specific confirmation email with generic registration
confirmation that uses load_email_template() for HTML rendering. Remove stale
tryout date/time/location from email body. Update Stripe product names from
"Tryout" to "Registration" / "Remote Evaluation". Add team_preference field to
API registration endpoint and surface it in admin notification emails.

Closes #383

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-07 19:23:40 +00:00
Author
Owner
Copy link

PR #385 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

email.py changes (confirmation email):

  • Subject correctly changed from "Complete {name}'s Tryout Registration" to "{name}'s Registration Confirmed"
  • Plaintext body has NO hardcoded event dates -- "March 24", "Kongo Gym", "4:00 - 5:30 PM" are all removed. Replaced with division, profile URL, and coach contact.
  • _build_confirmation_html() (~150 lines) and _build_credentials_html() (~25 lines) are both deleted entirely. Net -208 lines -- good cleanup.
  • load_email_template("registration-confirmation", data) is called with proper try/except FileNotFoundError fallback to html_body = None, which will send plaintext-only. This is the correct graceful degradation pattern matching the existing load_email_template() function which raises FileNotFoundError when the template file is missing.
  • Credentials block is conditionally built: empty string "" when no credentials, inline HTML <p> block when present. Correctly uses escape() on credential email.
  • team_preference added to admin notification email (both plaintext and HTML table row). Uses player.team_preference.value if player.team_preference else "Not specified" -- correct enum access.

register.py changes (Stripe + team_preference):

  • Stripe product name changed from "Tryout Registration" / "Remote Registration" to "Registration" / "Remote Evaluation". Matches issue requirement.
  • team_preference: Optional[str] = None added to APIRegistrationRequest Pydantic model. Correct -- accepts string from API, parsed to TeamPreference enum internally.
  • TeamPreference(body.team_preference.strip()) with try/except ValueError: pass for both existing and new player paths. Invalid values silently ignored (existing player keeps current value, new player gets None).

admin.py changes (is_public field):

  • is_public: bool added to AdminPlayerItem response model and populated in the player list endpoint.
  • This is technically outside the scope of issue #383 (confirmation email / Stripe label / team_preference). See NITS.

test_admin_spa.py changes:

  • "is_public" added to expected fields assertion for admin player list response. Test updated to match the model change.

BLOCKERS

None.

  • load_email_template() has proper FileNotFoundError handling -- verified at /home/ldraney/basketball-api/src/basketball_api/services/email.py:1122-1125.
  • _build_confirmation_html has no remaining callers after deletion (grep confirmed against repo).
  • TeamPreference enum is an existing model field (/home/ldraney/basketball-api/src/basketball_api/models.py:213), not a schema change.
  • No secrets, credentials, or hardcoded sensitive values in the diff.
  • Input is validated (enum parsing with try/except, HTML escaping on all user data).

NITS

  1. Scope creep: is_public field in admin.py -- The AdminPlayerItem.is_public addition and test update are unrelated to issue #383 (confirmation email, Stripe label, team_preference). This is a minor addition (2 lines of production code, 1 line of test), but it should ideally be tracked separately. Not blocking since it is a trivial read-only field exposure that was already on the model.

  2. Silent team_preference rejection -- Invalid team_preference values are silently ignored (except ValueError: pass). The API caller gets no feedback that their value was rejected. Consider returning a 422 or including a warning in the response. Non-blocking since the field is optional and the behavior is safe.

  3. DRY: team_preference parsing duplicated -- The TeamPreference(body.team_preference.strip()) try/except block appears twice (once for existing player, once for new player). Could be extracted to a single parse before the if/else branch. Non-blocking.

SOP COMPLIANCE

  • Branch named after issue: 383-fix-confirmation-email
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related section references plan slug -- says "None" (no plan slug referenced; acceptable if this is standalone board work)
  • No secrets committed
  • No unnecessary file changes (minor scope creep with is_public noted above but non-blocking)
  • Commit messages are descriptive (PR title follows conventional commits: fix:)

PROCESS OBSERVATIONS

  • Net -141 lines: replacing inline HTML builder with MJML template system is a clear architectural improvement. Reduces maintenance burden and separates concerns.
  • The load_email_template pattern is already proven (used by jersey-reminder email per /home/ldraney/basketball-api/tests/test_jersey_reminder.py). Good consistency.
  • 66 tests reported passing in Test Plan. No new test coverage for team_preference parsing or the is_public field addition, but existing registration tests cover the happy path and the changes are additive/optional fields.

VERDICT: APPROVED

## PR #385 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Pydantic **email.py changes (confirmation email):** - Subject correctly changed from "Complete {name}'s Tryout Registration" to "{name}'s Registration Confirmed" - Plaintext body has NO hardcoded event dates -- "March 24", "Kongo Gym", "4:00 - 5:30 PM" are all removed. Replaced with division, profile URL, and coach contact. - `_build_confirmation_html()` (~150 lines) and `_build_credentials_html()` (~25 lines) are both deleted entirely. Net -208 lines -- good cleanup. - `load_email_template("registration-confirmation", data)` is called with proper `try/except FileNotFoundError` fallback to `html_body = None`, which will send plaintext-only. This is the correct graceful degradation pattern matching the existing `load_email_template()` function which raises `FileNotFoundError` when the template file is missing. - Credentials block is conditionally built: empty string `""` when no credentials, inline HTML `<p>` block when present. Correctly uses `escape()` on credential email. - `team_preference` added to admin notification email (both plaintext and HTML table row). Uses `player.team_preference.value if player.team_preference else "Not specified"` -- correct enum access. **register.py changes (Stripe + team_preference):** - Stripe product name changed from `"Tryout Registration"` / `"Remote Registration"` to `"Registration"` / `"Remote Evaluation"`. Matches issue requirement. - `team_preference: Optional[str] = None` added to `APIRegistrationRequest` Pydantic model. Correct -- accepts string from API, parsed to `TeamPreference` enum internally. - `TeamPreference(body.team_preference.strip())` with `try/except ValueError: pass` for both existing and new player paths. Invalid values silently ignored (existing player keeps current value, new player gets `None`). **admin.py changes (is_public field):** - `is_public: bool` added to `AdminPlayerItem` response model and populated in the player list endpoint. - This is technically outside the scope of issue #383 (confirmation email / Stripe label / team_preference). See NITS. **test_admin_spa.py changes:** - `"is_public"` added to expected fields assertion for admin player list response. Test updated to match the model change. ### BLOCKERS None. - `load_email_template()` has proper FileNotFoundError handling -- verified at `/home/ldraney/basketball-api/src/basketball_api/services/email.py:1122-1125`. - `_build_confirmation_html` has no remaining callers after deletion (grep confirmed against repo). - `TeamPreference` enum is an existing model field (`/home/ldraney/basketball-api/src/basketball_api/models.py:213`), not a schema change. - No secrets, credentials, or hardcoded sensitive values in the diff. - Input is validated (enum parsing with try/except, HTML escaping on all user data). ### NITS 1. **Scope creep: `is_public` field in admin.py** -- The `AdminPlayerItem.is_public` addition and test update are unrelated to issue #383 (confirmation email, Stripe label, team_preference). This is a minor addition (2 lines of production code, 1 line of test), but it should ideally be tracked separately. Not blocking since it is a trivial read-only field exposure that was already on the model. 2. **Silent team_preference rejection** -- Invalid `team_preference` values are silently ignored (`except ValueError: pass`). The API caller gets no feedback that their value was rejected. Consider returning a 422 or including a warning in the response. Non-blocking since the field is optional and the behavior is safe. 3. **DRY: team_preference parsing duplicated** -- The `TeamPreference(body.team_preference.strip())` try/except block appears twice (once for existing player, once for new player). Could be extracted to a single parse before the if/else branch. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `383-fix-confirmation-email` - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references plan slug -- says "None" (no plan slug referenced; acceptable if this is standalone board work) - [x] No secrets committed - [x] No unnecessary file changes (minor scope creep with `is_public` noted above but non-blocking) - [x] Commit messages are descriptive (PR title follows conventional commits: `fix:`) ### PROCESS OBSERVATIONS - Net -141 lines: replacing inline HTML builder with MJML template system is a clear architectural improvement. Reduces maintenance burden and separates concerns. - The `load_email_template` pattern is already proven (used by jersey-reminder email per `/home/ldraney/basketball-api/tests/test_jersey_reminder.py`). Good consistency. - 66 tests reported passing in Test Plan. No new test coverage for `team_preference` parsing or the `is_public` field addition, but existing registration tests cover the happy path and the changes are additive/optional fields. ### VERDICT: APPROVED
remove team_preference from API — not needed per Marcus
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
fc67ba4ae7 
Strips team_preference from APIRegistrationRequest, Player constructor,
admin notification email. Keeps email template swap, Stripe label fix.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin force-pushed 383-fix-confirmation-email from fc67ba4ae7
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
to 1a86670454
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
2026-04-07 19:30:38 +00:00 Compare
forgejo_admin deleted branch 383-fix-confirmation-email 2026-04-07 19:30:45 +00:00
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
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
forgejo_admin/basketball-api!385
No description provided.