feat: Player registration form, photo upload, data model expansion #10
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!10
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "7-phase-1-player-registration-form-data-mo"
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
Changes
src/basketball_api/models.py: AddTeamPreferenceenum 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 confirmationsrc/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 lifespansrc/basketball_api/config.py: Add upload_dir settingtests/conftest.py: Configure temp upload dir for teststests/test_models.py: Verify new Player fields are nullable and work correctlytests/test_register.py: 8 tests covering form rendering, create/update flow, duplicate detection, paid/unpaid redirect, validationtests/test_upload.py: 5 tests covering valid uploads, extension/size/content-type rejectionTest Plan
Review Checklist
Related
plan-2026-02-25-stripe-connect-payoutsphase 1PR #10 Review
BLOCKERS
1.
date_of_birthstring-to-Date type mismatch (register.py:289, 411, 431)The form receives
date_of_birthas a string (e.g.,"2010-05-15") viaform.get("date_of_birth", "").strip()on line 289. This string is passed directly to the SQLAlchemyPlayermodel on lines 411 and 431, but the column is typedMapped[date | None]withsa.Date. SQLite in tests silently accepts strings, masking the bug, but PostgreSQL will raise aDataErrorat runtime. The string must be parsed todatetime.datebefore assignment: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:# Stripe $30 tryout payment linkPay $30 Tryout FeeEither 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_idandtenant_idonly -- it does NOT filter by player name: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.nameto 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/photoendpoint inupload.pyvalidatesfile.content_typeto reject non-image content types (line 35-36). The inline photo upload inregister.pydoes 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
Duplicated upload logic --
register.pylines 334-363 reimplements the same file validation and save logic fromupload.py. Consider extracting a sharedsave_uploaded_photo()function to avoid the current inconsistency (content-type check missing) and future drift.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.jpgwith executable content would pass validation. For a youth sports program, consider addingPillow/PIL.Image.open().verify()or at minimum check the first few bytes.Test databases use file-based SQLite with relative paths --
test_register.dbandtest_models.dbare created in CWD. Consider using:memory:SQLite ortmp_pathfixtures for cleaner isolation, especially in CI where CWD may vary. The.gitignorecovers*.dbso this is non-blocking.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.
StaticFilesmount insidelifespancontext manager --app.mount()on line 51 ofmain.pyis 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.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
7-phase-1-player-registration-form-data-momatches issue #7## Summary,## Changes,## Test Plan,## Relatedplan-reference found)VERDICT: NOT APPROVED
Four blockers must be addressed before merge:
date_of_birthwill crash on PostgreSQL -- parse the string todatetime.date/upload/photoand/registerPR #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.pynow parses withdatetime.strptime(dob_str, "%Y-%m-%d").date(), returns 400 on invalid format. Model usesDatecolumn 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_LINKnow points toaFa8wRbky5KwgL0bI60VO01(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, ANDfunc.lower(Player.name) == player_name.lower(). Testtest_sibling_registration_creates_new_playerconfirms two children under the same parent email create separate records. Testtest_duplicate_parent_email_same_player_updates_existingconfirms same-name re-submission updates instead of duplicating.4. Content-type validation missing on inline photo upload — FIXED
Both
register.py(inline upload) andupload.py(standalone endpoint) now validatecontent_type.startswith("image/"). Tests:test_photo_with_invalid_content_type_returns_errorandtest_upload_rejects_non_image_content_type.BLOCKERS
None.
NITS
target_schoolsis plainText, issue says "JSON": The acceptance criteria saystarget_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.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.Update logic uses
if field:guards: When updating an existing player, fields likeheightare only updatedif 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.Hardcoded tryout date in HTML:
"March 13, 2026"appears in both the form and confirmation templates. Consider extracting to a constant for maintainability.Test DB files on disk:
test_register.pyandtest_models.pyuse 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.test_register.pymodule-level client:client = TestClient(app)is created at module scope beforesetup_moduleruns. This works becauseTestClientis lazy, but it couples test module initialization to app import order.SOP COMPLIANCE
7-phase-1-player-registration-form-data-moreferences issue #7plan-2026-02-25-stripe-connect-payoutsplan-2026-02-25-stripe-connect-payoutsBASKETBALL_prefix; conftest uses dummy test valuescoach.py(Phase 2) oremail.py(Phase 3) as required by issue constraintsVERDICT: APPROVED