feat: Player registration form, photo upload, data model expansion #10

Merged
forgejo_admin merged 2 commits from 7-phase-1-player-registration-form-data-mo into main 2026-03-09 18:12:01 +00:00

Summary

  • Add mobile-friendly player registration form (GET/POST /register) with photo upload (POST /upload/photo)
  • Expand Player model with 8 new nullable fields and Alembic migration 002 (safe for existing 34 records)
  • Duplicate detection by parent email updates existing records; unpaid players see Stripe link, paid players see confirmation

Changes

  • src/basketball_api/models.py: Add TeamPreference enum and 8 new nullable Player columns (photo_url, date_of_birth, current_school, target_schools, hometown, team_preference, tryout_number, checked_in)
  • alembic/versions/002_add_player_profile_fields.py: Migration to add new columns safely (all nullable)
  • src/basketball_api/routes/register.py: GET /register serves branded HTML form; POST /register saves/updates player+parent, shows payment link or confirmation
  • src/basketball_api/routes/upload.py: POST /upload/photo handles image uploads (JPG/PNG/WebP, max 5 MB)
  • src/basketball_api/main.py: Wire up new routers, mount static file serving for uploads in lifespan
  • src/basketball_api/config.py: Add upload_dir setting
  • tests/conftest.py: Configure temp upload dir for tests
  • tests/test_models.py: Verify new Player fields are nullable and work correctly
  • tests/test_register.py: 8 tests covering form rendering, create/update flow, duplicate detection, paid/unpaid redirect, validation
  • tests/test_upload.py: 5 tests covering valid uploads, extension/size/content-type rejection

Test Plan

  • Tests pass locally (21/21 pass)
  • Manual verification: ruff check src/ tests/ passes with no errors
  • No regressions in existing health endpoint tests
  • Run alembic upgrade head against Postgres to verify migration 002
  • Verify GET /register renders mobile-friendly form on device
  • Verify POST /register with duplicate email updates existing player

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive
  • Closes #7
  • plan-2026-02-25-stripe-connect-payouts phase 1
## Summary - Add mobile-friendly player registration form (GET/POST /register) with photo upload (POST /upload/photo) - Expand Player model with 8 new nullable fields and Alembic migration 002 (safe for existing 34 records) - Duplicate detection by parent email updates existing records; unpaid players see Stripe link, paid players see confirmation ## Changes - `src/basketball_api/models.py`: Add `TeamPreference` enum and 8 new nullable Player columns (photo_url, date_of_birth, current_school, target_schools, hometown, team_preference, tryout_number, checked_in) - `alembic/versions/002_add_player_profile_fields.py`: Migration to add new columns safely (all nullable) - `src/basketball_api/routes/register.py`: GET /register serves branded HTML form; POST /register saves/updates player+parent, shows payment link or confirmation - `src/basketball_api/routes/upload.py`: POST /upload/photo handles image uploads (JPG/PNG/WebP, max 5 MB) - `src/basketball_api/main.py`: Wire up new routers, mount static file serving for uploads in lifespan - `src/basketball_api/config.py`: Add upload_dir setting - `tests/conftest.py`: Configure temp upload dir for tests - `tests/test_models.py`: Verify new Player fields are nullable and work correctly - `tests/test_register.py`: 8 tests covering form rendering, create/update flow, duplicate detection, paid/unpaid redirect, validation - `tests/test_upload.py`: 5 tests covering valid uploads, extension/size/content-type rejection ## Test Plan - [x] Tests pass locally (21/21 pass) - [x] Manual verification: ruff check src/ tests/ passes with no errors - [x] No regressions in existing health endpoint tests - [ ] Run alembic upgrade head against Postgres to verify migration 002 - [ ] Verify GET /register renders mobile-friendly form on device - [ ] Verify POST /register with duplicate email updates existing player ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #7 - `plan-2026-02-25-stripe-connect-payouts` phase 1
feat: Add player registration form, photo upload, and data model expansion
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
8f1bb868f1
Add mobile-friendly registration form (GET/POST /register), photo upload
endpoint (POST /upload/photo), and 8 new nullable Player fields (photo_url,
date_of_birth, current_school, target_schools, hometown, team_preference,
tryout_number, checked_in). Includes Alembic migration 002 that safely adds
columns without breaking existing 34 player records. Duplicate detection by
parent email updates existing records instead of creating duplicates. Unpaid
players see Stripe payment link; paid players see confirmation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
Owner

PR #10 Review

BLOCKERS

1. date_of_birth string-to-Date type mismatch (register.py:289, 411, 431)

The form receives date_of_birth as a string (e.g., "2010-05-15") via form.get("date_of_birth", "").strip() on line 289. This string is passed directly to the SQLAlchemy Player model on lines 411 and 431, but the column is typed Mapped[date | None] with sa.Date. SQLite in tests silently accepts strings, masking the bug, but PostgreSQL will raise a DataError at runtime. The string must be parsed to datetime.date before assignment:

from datetime import date as date_type
if date_of_birth:
    date_of_birth = date_type.fromisoformat(date_of_birth)

2. Stripe link amount mismatch: code says "$30" but link charges $200 (register.py:23-24, 218)

Line 24 hardcodes STRIPE_TRYOUT_LINK = "https://buy.stripe.com/eVqaEZdsGgpa52i5jI0VO03". Per project memory, this URL was changed from $30 to $200 (full registration fee). However:

  • The code comment on line 23 says # Stripe $30 tryout payment link
  • The confirmation page HTML on line 218 says Pay $30 Tryout Fee

Either the link URL is wrong (should be the $30 link) or the displayed text is wrong (should say $200). Shipping this as-is will confuse parents who click "Pay $30" and see a $200 charge. This needs to be resolved with the product owner before merge.

3. Dedup logic only supports one child per parent email (register.py:392-397)

The duplicate detection query filters by parent_id and tenant_id only -- it does NOT filter by player name:

existing_player = (
    db.query(Player)
    .filter(Player.parent_id == parent.id, Player.tenant_id == tenant.id)
    .first()
)

If a parent registers a second child using the same email, this will silently overwrite the first child's record instead of creating a second player. The issue spec says "dedup by parent email (update vs create)" which this implements literally, but the real-world impact is data loss for multi-child families. At minimum, the dedup should also match on Player.name to distinguish siblings. This is a correctness issue for an AAU program where families commonly register multiple children.

4. Photo upload in register.py skips content-type validation (register.py:334-363)

The standalone /upload/photo endpoint in upload.py validates file.content_type to reject non-image content types (line 35-36). The inline photo upload in register.py does NOT perform this check. An attacker could submit a .jpg-named non-image file through the registration form. The content-type check should be applied consistently, or the register route should delegate to the upload module's logic rather than duplicating it.

NITS

  1. Duplicated upload logic -- register.py lines 334-363 reimplements the same file validation and save logic from upload.py. Consider extracting a shared save_uploaded_photo() function to avoid the current inconsistency (content-type check missing) and future drift.

  2. No magic-byte validation for uploads -- Both upload paths only check file extension and client-supplied content-type. Neither validates actual file content via magic bytes. A file named evil.jpg with executable content would pass validation. For a youth sports program, consider adding Pillow / PIL.Image.open().verify() or at minimum check the first few bytes.

  3. Test databases use file-based SQLite with relative paths -- test_register.db and test_models.db are created in CWD. Consider using :memory: SQLite or tmp_path fixtures for cleaner isolation, especially in CI where CWD may vary. The .gitignore covers *.db so this is non-blocking.

  4. Hardcoded tryout date in HTML -- "March 13, 2026" appears in both the form header (line 115) and confirmation page (line 259). If the tryout date changes, it must be updated in two places. Consider extracting to a constant.

  5. StaticFiles mount inside lifespan context manager -- app.mount() on line 51 of main.py is called inside the async lifespan. FastAPI route registration typically happens before startup. This works in practice (the mount is registered before the first request), but it is unconventional. A note or comment explaining the pattern would help future readers.

  6. Update logic is not idempotent for field clearing -- Lines 402-419 only update fields when the new value is truthy (if height: ...). If a user wants to clear a previously-set field (e.g., remove hometown), submitting an empty value will NOT clear it -- the old value persists. This may be intentional for "only update what's provided," but it prevents corrections.

SOP COMPLIANCE

  • Branch named after issue: 7-phase-1-player-registration-form-data-mo matches issue #7
  • PR body has ## Summary, ## Changes, ## Test Plan, ## Related
  • Related section references plan slug (plan- reference found)
  • No secrets, .env files, or credentials committed (verified via grep)
  • No scope creep: coach.py and email.py untouched, no unrelated files changed
  • Migration is safe: all 8 new columns are nullable, downgrade drops columns and enum type
  • Tests exist: 3 new test files with 16 tests covering models, registration, and upload

VERDICT: NOT APPROVED

Four blockers must be addressed before merge:

  1. date_of_birth will crash on PostgreSQL -- parse the string to datetime.date
  2. Stripe amount mismatch ($30 label vs $200 link) needs product decision
  3. Dedup logic will silently overwrite siblings -- at minimum add player name to the match
  4. Photo upload content-type validation gap between /upload/photo and /register
## PR #10 Review ### BLOCKERS **1. `date_of_birth` string-to-Date type mismatch (register.py:289, 411, 431)** The form receives `date_of_birth` as a string (e.g., `"2010-05-15"`) via `form.get("date_of_birth", "").strip()` on line 289. This string is passed directly to the SQLAlchemy `Player` model on lines 411 and 431, but the column is typed `Mapped[date | None]` with `sa.Date`. SQLite in tests silently accepts strings, masking the bug, but PostgreSQL will raise a `DataError` at runtime. The string must be parsed to `datetime.date` before assignment: ```python from datetime import date as date_type if date_of_birth: date_of_birth = date_type.fromisoformat(date_of_birth) ``` **2. Stripe link amount mismatch: code says "$30" but link charges $200 (register.py:23-24, 218)** Line 24 hardcodes `STRIPE_TRYOUT_LINK = "https://buy.stripe.com/eVqaEZdsGgpa52i5jI0VO03"`. Per project memory, this URL was changed from $30 to $200 (full registration fee). However: - The code comment on line 23 says `# Stripe $30 tryout payment link` - The confirmation page HTML on line 218 says `Pay $30 Tryout Fee` Either the link URL is wrong (should be the $30 link) or the displayed text is wrong (should say $200). Shipping this as-is will confuse parents who click "Pay $30" and see a $200 charge. This needs to be resolved with the product owner before merge. **3. Dedup logic only supports one child per parent email (register.py:392-397)** The duplicate detection query filters by `parent_id` and `tenant_id` only -- it does NOT filter by player name: ```python existing_player = ( db.query(Player) .filter(Player.parent_id == parent.id, Player.tenant_id == tenant.id) .first() ) ``` If a parent registers a second child using the same email, this will silently overwrite the first child's record instead of creating a second player. The issue spec says "dedup by parent email (update vs create)" which this implements literally, but the real-world impact is data loss for multi-child families. At minimum, the dedup should also match on `Player.name` to distinguish siblings. This is a correctness issue for an AAU program where families commonly register multiple children. **4. Photo upload in register.py skips content-type validation (register.py:334-363)** The standalone `/upload/photo` endpoint in `upload.py` validates `file.content_type` to reject non-image content types (line 35-36). The inline photo upload in `register.py` does NOT perform this check. An attacker could submit a `.jpg`-named non-image file through the registration form. The content-type check should be applied consistently, or the register route should delegate to the upload module's logic rather than duplicating it. ### NITS 1. **Duplicated upload logic** -- `register.py` lines 334-363 reimplements the same file validation and save logic from `upload.py`. Consider extracting a shared `save_uploaded_photo()` function to avoid the current inconsistency (content-type check missing) and future drift. 2. **No magic-byte validation for uploads** -- Both upload paths only check file extension and client-supplied content-type. Neither validates actual file content via magic bytes. A file named `evil.jpg` with executable content would pass validation. For a youth sports program, consider adding `Pillow` / `PIL.Image.open().verify()` or at minimum check the first few bytes. 3. **Test databases use file-based SQLite with relative paths** -- `test_register.db` and `test_models.db` are created in CWD. Consider using `:memory:` SQLite or `tmp_path` fixtures for cleaner isolation, especially in CI where CWD may vary. The `.gitignore` covers `*.db` so this is non-blocking. 4. **Hardcoded tryout date in HTML** -- "March 13, 2026" appears in both the form header (line 115) and confirmation page (line 259). If the tryout date changes, it must be updated in two places. Consider extracting to a constant. 5. **`StaticFiles` mount inside `lifespan` context manager** -- `app.mount()` on line 51 of `main.py` is called inside the async lifespan. FastAPI route registration typically happens before startup. This works in practice (the mount is registered before the first request), but it is unconventional. A note or comment explaining the pattern would help future readers. 6. **Update logic is not idempotent for field clearing** -- Lines 402-419 only update fields when the new value is truthy (`if height: ...`). If a user wants to clear a previously-set field (e.g., remove hometown), submitting an empty value will NOT clear it -- the old value persists. This may be intentional for "only update what's provided," but it prevents corrections. ### SOP COMPLIANCE - [x] Branch named after issue: `7-phase-1-player-registration-form-data-mo` matches issue #7 - [x] PR body has `## Summary`, `## Changes`, `## Test Plan`, `## Related` - [x] Related section references plan slug (`plan-` reference found) - [x] No secrets, .env files, or credentials committed (verified via grep) - [x] No scope creep: coach.py and email.py untouched, no unrelated files changed - [x] Migration is safe: all 8 new columns are nullable, downgrade drops columns and enum type - [x] Tests exist: 3 new test files with 16 tests covering models, registration, and upload ### VERDICT: NOT APPROVED Four blockers must be addressed before merge: 1. `date_of_birth` will crash on PostgreSQL -- parse the string to `datetime.date` 2. Stripe amount mismatch ($30 label vs $200 link) needs product decision 3. Dedup logic will silently overwrite siblings -- at minimum add player name to the match 4. Photo upload content-type validation gap between `/upload/photo` and `/register`
fix: Resolve 4 QA blockers on registration form
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
734eb8c478
- Parse date_of_birth string to datetime.date before DB assignment
  (PostgreSQL requires actual date type, not string)
- Fix Stripe payment link URL to use the $30 tryout link
- Fix dedup logic to match on parent_id + tenant_id + player name
  (case-insensitive) so siblings are not overwritten
- Add content-type validation on inline photo upload in registration
  form POST handler (matching the standalone /upload/photo endpoint)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
Owner

PR #10 Review (Round 2)

PREVIOUS BLOCKERS — RE-VERIFICATION

All four blockers from Round 1 have been verified as correctly fixed:

1. date_of_birth string not parsed to datetime.date — FIXED
register.py now parses with datetime.strptime(dob_str, "%Y-%m-%d").date(), returns 400 on invalid format. Model uses Date column type. Two new tests cover the happy path (test_date_of_birth_parsed_to_date) and error path (test_invalid_date_of_birth_returns_error).

2. Stripe link was $200 but UI said "$30" — FIXED
STRIPE_TRYOUT_LINK now points to aFa8wRbky5KwgL0bI60VO01 (the $30 tryout link). UI copy matches: "Pay $30 Tryout Fee" and "Complete your $30 tryout fee to finalize registration."

3. Dedup overwrote siblings (matched parent only) — FIXED
Dedup query now matches on Player.parent_id, Player.tenant_id, AND func.lower(Player.name) == player_name.lower(). Test test_sibling_registration_creates_new_player confirms two children under the same parent email create separate records. Test test_duplicate_parent_email_same_player_updates_existing confirms same-name re-submission updates instead of duplicating.

4. Content-type validation missing on inline photo upload — FIXED
Both register.py (inline upload) and upload.py (standalone endpoint) now validate content_type.startswith("image/"). Tests: test_photo_with_invalid_content_type_returns_error and test_upload_rejects_non_image_content_type.


BLOCKERS

None.


NITS

  1. target_schools is plain Text, issue says "JSON": The acceptance criteria says target_schools (JSON) but the implementation uses comma-separated text. This is arguably simpler and works fine for the current use case (textarea input). The test uses "Duke, UNC, Kentucky" as a string. Not blocking, but worth noting the deviation from spec — if downstream consumers expect JSON, this will need a migration later.

  2. func.lower() for case-insensitive name match: This works on both SQLite (tests) and PostgreSQL (prod), but on large datasets a functional index would be needed for performance. Fine at 34 records.

  3. Update logic uses if field: guards: When updating an existing player, fields like height are only updated if height:. This means a parent cannot clear a previously-set optional field (e.g., set height back to empty). This is a minor UX limitation, not a blocker for tryout day.

  4. Hardcoded tryout date in HTML: "March 13, 2026" appears in both the form and confirmation templates. Consider extracting to a constant for maintainability.

  5. Test DB files on disk: test_register.py and test_models.py use file-backed SQLite (sqlite:///test_register.db, sqlite:///test_models.db) instead of :memory:. The teardown cleans them up, but :memory: would be cleaner and avoid potential CI race conditions.

  6. test_register.py module-level client: client = TestClient(app) is created at module scope before setup_module runs. This works because TestClient is lazy, but it couples test module initialization to app import order.


SOP COMPLIANCE

  • Branch named after issue: 7-phase-1-player-registration-form-data-mo references issue #7
  • PR body has Summary: Yes
  • PR body has Changes: Yes
  • PR body has Test Plan: Yes
  • PR body has Related: Yes, references "Closes #7" and plan-2026-02-25-stripe-connect-payouts
  • Related references plan slug: plan-2026-02-25-stripe-connect-payouts
  • Tests exist: 21 tests across 3 test files (test_register.py, test_upload.py, test_models.py)
  • No secrets or credentials committed: Config uses env vars via BASKETBALL_ prefix; conftest uses dummy test values
  • No unnecessary file changes (scope creep): 10 files changed, all within scope. No changes to coach.py (Phase 2) or email.py (Phase 3) as required by issue constraints
  • Commit messages are descriptive: 2 commits in PR
  • Acceptance criteria coverage:
    • 8 new nullable Player fields (photo_url, date_of_birth, current_school, target_schools, hometown, team_preference, tryout_number, checked_in)
    • TeamPreference enum (local, travel, both)
    • Safe Alembic migration 002 (all nullable, enum created with checkfirst)
    • GET /register serves mobile-friendly branded HTML form
    • POST /register saves/updates Player+Parent
    • Dedup by parent email + player name
    • POST /upload/photo with JPG/PNG/WebP, 5MB max, content-type validation
    • Unpaid → Stripe $30 link, Paid → confirmation
    • Backfill verification for existing 34 Stripe sessions (not in PR scope — manual task per test plan)

VERDICT: APPROVED

## PR #10 Review (Round 2) ### PREVIOUS BLOCKERS — RE-VERIFICATION All four blockers from Round 1 have been verified as correctly fixed: **1. date_of_birth string not parsed to datetime.date** — FIXED `register.py` now parses with `datetime.strptime(dob_str, "%Y-%m-%d").date()`, returns 400 on invalid format. Model uses `Date` column type. Two new tests cover the happy path (`test_date_of_birth_parsed_to_date`) and error path (`test_invalid_date_of_birth_returns_error`). **2. Stripe link was $200 but UI said "$30"** — FIXED `STRIPE_TRYOUT_LINK` now points to `aFa8wRbky5KwgL0bI60VO01` (the $30 tryout link). UI copy matches: "Pay $30 Tryout Fee" and "Complete your $30 tryout fee to finalize registration." **3. Dedup overwrote siblings (matched parent only)** — FIXED Dedup query now matches on `Player.parent_id`, `Player.tenant_id`, AND `func.lower(Player.name) == player_name.lower()`. Test `test_sibling_registration_creates_new_player` confirms two children under the same parent email create separate records. Test `test_duplicate_parent_email_same_player_updates_existing` confirms same-name re-submission updates instead of duplicating. **4. Content-type validation missing on inline photo upload** — FIXED Both `register.py` (inline upload) and `upload.py` (standalone endpoint) now validate `content_type.startswith("image/")`. Tests: `test_photo_with_invalid_content_type_returns_error` and `test_upload_rejects_non_image_content_type`. --- ### BLOCKERS None. --- ### NITS 1. **`target_schools` is plain `Text`, issue says "JSON"**: The acceptance criteria says `target_schools (JSON)` but the implementation uses comma-separated text. This is arguably simpler and works fine for the current use case (textarea input). The test uses `"Duke, UNC, Kentucky"` as a string. Not blocking, but worth noting the deviation from spec — if downstream consumers expect JSON, this will need a migration later. 2. **`func.lower()` for case-insensitive name match**: This works on both SQLite (tests) and PostgreSQL (prod), but on large datasets a functional index would be needed for performance. Fine at 34 records. 3. **Update logic uses `if field:` guards**: When updating an existing player, fields like `height` are only updated `if height:`. This means a parent cannot clear a previously-set optional field (e.g., set height back to empty). This is a minor UX limitation, not a blocker for tryout day. 4. **Hardcoded tryout date in HTML**: `"March 13, 2026"` appears in both the form and confirmation templates. Consider extracting to a constant for maintainability. 5. **Test DB files on disk**: `test_register.py` and `test_models.py` use file-backed SQLite (`sqlite:///test_register.db`, `sqlite:///test_models.db`) instead of `:memory:`. The teardown cleans them up, but `:memory:` would be cleaner and avoid potential CI race conditions. 6. **`test_register.py` module-level client**: `client = TestClient(app)` is created at module scope before `setup_module` runs. This works because `TestClient` is lazy, but it couples test module initialization to app import order. --- ### SOP COMPLIANCE - [x] **Branch named after issue**: `7-phase-1-player-registration-form-data-mo` references issue #7 - [x] **PR body has Summary**: Yes - [x] **PR body has Changes**: Yes - [x] **PR body has Test Plan**: Yes - [x] **PR body has Related**: Yes, references "Closes #7" and `plan-2026-02-25-stripe-connect-payouts` - [x] **Related references plan slug**: `plan-2026-02-25-stripe-connect-payouts` - [x] **Tests exist**: 21 tests across 3 test files (test_register.py, test_upload.py, test_models.py) - [x] **No secrets or credentials committed**: Config uses env vars via `BASKETBALL_` prefix; conftest uses dummy test values - [x] **No unnecessary file changes (scope creep)**: 10 files changed, all within scope. No changes to `coach.py` (Phase 2) or `email.py` (Phase 3) as required by issue constraints - [x] **Commit messages are descriptive**: 2 commits in PR - [x] **Acceptance criteria coverage**: - [x] 8 new nullable Player fields (photo_url, date_of_birth, current_school, target_schools, hometown, team_preference, tryout_number, checked_in) - [x] TeamPreference enum (local, travel, both) - [x] Safe Alembic migration 002 (all nullable, enum created with checkfirst) - [x] GET /register serves mobile-friendly branded HTML form - [x] POST /register saves/updates Player+Parent - [x] Dedup by parent email + player name - [x] POST /upload/photo with JPG/PNG/WebP, 5MB max, content-type validation - [x] Unpaid → Stripe $30 link, Paid → confirmation - [ ] Backfill verification for existing 34 Stripe sessions (not in PR scope — manual task per test plan) ### VERDICT: APPROVED
forgejo_admin deleted branch 7-phase-1-player-registration-form-data-mo 2026-03-09 18:12:01 +00:00
Sign in to join this conversation.
No description provided.