interest endpoint for lead capture (#186) #187

Merged

forgejo_admin merged 1 commit from 186-interest-leads-endpoint into main

2026-03-27 10:47:30 +00:00

forgejo_admin commented

2026-03-27 08:34:31 +00:00

Owner

Summary

Add public endpoint for prospective player/parent interest form submissions
Captures lead data (player name, age/grade, parent email, optional phone and program) into Postgres
Includes email regex validation, program validation (kings/queens), and best-effort notification stub

Changes

src/basketball_api/models.py: Added InterestLead model with tenant_id, player_name, age_grade, parent_email, parent_phone, program, created_at
alembic/versions/025_add_interest_leads_table.py: Migration to create interest_leads table
src/basketball_api/routes/public.py: Added POST /public/interest endpoint with InterestLeadRequest schema, email/program validators, _notify_new_interest() stub
tests/test_interest.py: 14 tests covering valid submissions, DB persistence, field normalization, and validation errors

Test Plan

Tests pass locally (582 passed, 0 failed)
14 new tests: valid 201, DB persistence, optional fields, email lowercasing, program lowercasing, created_at populated, missing required fields 422, invalid email 422, invalid program 422, empty body 422, no DB record on validation failure
No regressions in existing public endpoint tests (13 passed)
ruff format and ruff check clean

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #186
basketball-api -- the basketball program management API

## Summary - Add public endpoint for prospective player/parent interest form submissions - Captures lead data (player name, age/grade, parent email, optional phone and program) into Postgres - Includes email regex validation, program validation (kings/queens), and best-effort notification stub ## Changes - `src/basketball_api/models.py`: Added `InterestLead` model with tenant_id, player_name, age_grade, parent_email, parent_phone, program, created_at - `alembic/versions/025_add_interest_leads_table.py`: Migration to create `interest_leads` table - `src/basketball_api/routes/public.py`: Added `POST /public/interest` endpoint with `InterestLeadRequest` schema, email/program validators, `_notify_new_interest()` stub - `tests/test_interest.py`: 14 tests covering valid submissions, DB persistence, field normalization, and validation errors ## Test Plan - [x] Tests pass locally (582 passed, 0 failed) - [x] 14 new tests: valid 201, DB persistence, optional fields, email lowercasing, program lowercasing, created_at populated, missing required fields 422, invalid email 422, invalid program 422, empty body 422, no DB record on validation failure - [x] No regressions in existing public endpoint tests (13 passed) - [x] ruff format and ruff check clean ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #186 - `basketball-api` -- the basketball program management API

forgejo_admin added 1 commit

2026-03-27 08:34:31 +00:00

feat: POST /public/interest endpoint for lead capture (#186 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

1949fb07d7

Add public endpoint for prospective player interest form submissions.
Captures player name, age/grade, parent email, optional phone and program
(kings/queens) into the interest_leads table. Includes email validation
via regex, program validation, and best-effort notification stub.

- InterestLead model in models.py
- Alembic migration 025 for interest_leads table
- POST /public/interest endpoint in routes/public.py
- 14 tests covering valid submissions, DB persistence, field normalization,
  and validation error cases (missing fields, invalid email, invalid program)

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

forgejo_admin commented

2026-03-27 08:35:34 +00:00

Author

Owner

QA Review -- PR #187

Scope Verification

PR implements exactly what issue #186 requests: POST /public/interest endpoint for lead capture. No scope creep, no missing pieces.

Code Quality

Model: InterestLead follows existing mapped_column style precisely. Correctly placed before OAuthToken. Good docstring.
Migration: 025 correctly chains from 024. Table definition matches model. Clean downgrade().
Endpoint: Follows the same pattern as GET /public/teams. Email regex validation avoids unnecessary email-validator dependency. Program validation restricts to kings/queens. Best-effort notification with try/except never blocks the 201 response.
Imports: Clean -- no unused imports in committed code.

Test Coverage

14 tests across two well-organized classes:

Happy path: valid submission 201, DB persistence, optional field omission, email lowercasing, program lowercasing, created_at auto-populated
Validation: missing player_name/age_grade/parent_email (422), invalid email (422), email without domain (422), invalid program (422), empty body (422), no DB record on validation failure

Findings

No issues found. Code is clean, follows existing patterns, tests are comprehensive.

VERDICT: APPROVED

## QA Review -- PR #187 ### Scope Verification PR implements exactly what issue #186 requests: `POST /public/interest` endpoint for lead capture. No scope creep, no missing pieces. ### Code Quality - **Model**: `InterestLead` follows existing `mapped_column` style precisely. Correctly placed before `OAuthToken`. Good docstring. - **Migration**: `025` correctly chains from `024`. Table definition matches model. Clean `downgrade()`. - **Endpoint**: Follows the same pattern as `GET /public/teams`. Email regex validation avoids unnecessary `email-validator` dependency. Program validation restricts to `kings`/`queens`. Best-effort notification with try/except never blocks the 201 response. - **Imports**: Clean -- no unused imports in committed code. ### Test Coverage 14 tests across two well-organized classes: - **Happy path**: valid submission 201, DB persistence, optional field omission, email lowercasing, program lowercasing, created_at auto-populated - **Validation**: missing player_name/age_grade/parent_email (422), invalid email (422), email without domain (422), invalid program (422), empty body (422), no DB record on validation failure ### Findings No issues found. Code is clean, follows existing patterns, tests are comprehensive. **VERDICT: APPROVED**

forgejo_admin commented

2026-03-27 10:46:33 +00:00

Author

Owner

PR #187 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Pydantic v2

Pattern consistency: The new endpoint follows the established public.py conventions well -- dedicated Pydantic request/response schemas, hardcoded tenant_id=1 (matching public_teams), Session = Depends(get_db) injection, and structured logging. The model placement in models.py follows existing ordering (alphabetical within the module). The InterestLeadRequest schema correctly uses field_validator (Pydantic v2 style) rather than the deprecated validator. The migration chain is correct: 024 -> 025.

Migration quality: The 025_add_interest_leads_table.py migration is clean and reversible (create_table / drop_table). Column types, lengths, and FK match the model. The server_default=sa.func.now() on created_at is correct.

Email validation: The regex _EMAIL_RE is a reasonable RFC 5322 approximation. The .lower().strip() normalization is applied correctly. The order of operations in the validator (check regex first, then normalize) is correct.

Program validation: Lowercases before comparison, returns lowercased value. Handles None correctly.

Error handling: The _notify_new_interest stub is wrapped in a try/except that logs with exc_info=True -- failures never block the 201 response. This is the correct pattern for best-effort side effects.

Test quality: 14 tests across two well-organized classes. Coverage includes: happy path (201), DB persistence verification, optional field omission, email lowercasing, program lowercasing, created_at population, missing required fields (3 tests), invalid email (2 tests), invalid program, empty body, and no-DB-record-on-validation-failure. The public_client fixture correctly overrides get_db without auth -- matching the existing client fixture pattern from conftest.py but deliberately unauthenticated. The _seed_tenant autouse fixture ensures tenant_id=1 exists.

BLOCKERS

None.

All four blocker criteria pass:

Test coverage: 14 new tests covering happy path, edge cases, and error handling.
Input validation: Email validated via regex, program validated against allowlist, required fields enforced by Pydantic, SQL injection not possible (SQLAlchemy parameterized queries).
No secrets: No credentials, API keys, or tokens committed.
No DRY violations in auth paths: This endpoint is intentionally unauthenticated (public lead capture), consistent with the existing /public/teams pattern.

NITS

Missing index=True on tenant_id: Every other model in models.py uses mapped_column(ForeignKey("tenants.id"), index=True) for tenant_id. The InterestLead model uses mapped_column(Integer, ForeignKey("tenants.id"), nullable=False) -- no index. The migration also omits the index. This is a minor consistency gap. In a single-tenant deployment it has zero performance impact, but if the table grows or multi-tenancy is ever added, queries by tenant will need a sequential scan. Consider adding index=True for pattern consistency.
No max_length / min_length on Pydantic string fields: player_name and age_grade accept any non-empty string (Pydantic requires the field but allows "" -- an empty string passes validation). A blank player_name would create a useless lead record. Consider adding min_length=1 on player_name, age_grade, and parent_email (the email regex would catch empty email, but the other two have no guard). Alternatively, a field_validator that strips and rejects whitespace-only strings.
No max_length on Pydantic string fields: The DB columns have length constraints (String(200), String(100), etc.) but the Pydantic schema does not enforce them. A 10,000-character player_name would pass Pydantic validation and then fail at the DB level with a less user-friendly error. Consider Field(max_length=200) on player_name, etc., to match the DB constraints.
Logging PII: _notify_new_interest logs parent_email at INFO level. This is a public endpoint so the volume of leads could be high. Consider whether email addresses in application logs are acceptable per your data handling posture, or whether the log should omit the email or use DEBUG level.
player_name and age_grade not stripped: The email validator calls .lower().strip(), but player_name and age_grade are stored as-is. Leading/trailing whitespace in a player name would create messy data. Consider stripping these fields in the endpoint or via validators.

SOP COMPLIANCE

Branch named after issue: 186-interest-leads-endpoint references issue #186
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present
Related references issue: Closes #186
No secrets committed
No unnecessary file changes: 4 files changed, all directly related to the feature
Commit messages are descriptive
Related section references plan slug: No plan slug referenced -- PR body says basketball-api -- the basketball program management API instead of a plan slug. Acceptable if this is standalone kanban work (no active plan).

PROCESS OBSERVATIONS

Deployment risk: Low. New table, new endpoint. No schema changes to existing tables. Migration is additive only.
Change failure risk: Low. The endpoint is isolated from existing functionality. The 14 tests provide good regression coverage.
Future work: The _notify_new_interest TODO for gmail-sdk integration should be tracked as a separate issue to avoid it becoming forgotten tech debt.

VERDICT: APPROVED

## PR #187 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Alembic / Pydantic v2 **Pattern consistency:** The new endpoint follows the established `public.py` conventions well -- dedicated Pydantic request/response schemas, hardcoded `tenant_id=1` (matching `public_teams`), `Session = Depends(get_db)` injection, and structured logging. The model placement in `models.py` follows existing ordering (alphabetical within the module). The `InterestLeadRequest` schema correctly uses `field_validator` (Pydantic v2 style) rather than the deprecated `validator`. The migration chain is correct: `024 -> 025`. **Migration quality:** The `025_add_interest_leads_table.py` migration is clean and reversible (`create_table` / `drop_table`). Column types, lengths, and FK match the model. The `server_default=sa.func.now()` on `created_at` is correct. **Email validation:** The regex `_EMAIL_RE` is a reasonable RFC 5322 approximation. The `.lower().strip()` normalization is applied correctly. The order of operations in the validator (check regex first, then normalize) is correct. **Program validation:** Lowercases before comparison, returns lowercased value. Handles `None` correctly. **Error handling:** The `_notify_new_interest` stub is wrapped in a try/except that logs with `exc_info=True` -- failures never block the 201 response. This is the correct pattern for best-effort side effects. **Test quality:** 14 tests across two well-organized classes. Coverage includes: happy path (201), DB persistence verification, optional field omission, email lowercasing, program lowercasing, `created_at` population, missing required fields (3 tests), invalid email (2 tests), invalid program, empty body, and no-DB-record-on-validation-failure. The `public_client` fixture correctly overrides `get_db` without auth -- matching the existing `client` fixture pattern from `conftest.py` but deliberately unauthenticated. The `_seed_tenant` autouse fixture ensures `tenant_id=1` exists. ### BLOCKERS None. All four blocker criteria pass: - **Test coverage:** 14 new tests covering happy path, edge cases, and error handling. - **Input validation:** Email validated via regex, program validated against allowlist, required fields enforced by Pydantic, SQL injection not possible (SQLAlchemy parameterized queries). - **No secrets:** No credentials, API keys, or tokens committed. - **No DRY violations in auth paths:** This endpoint is intentionally unauthenticated (public lead capture), consistent with the existing `/public/teams` pattern. ### NITS 1. **Missing `index=True` on `tenant_id`:** Every other model in `models.py` uses `mapped_column(ForeignKey("tenants.id"), index=True)` for `tenant_id`. The `InterestLead` model uses `mapped_column(Integer, ForeignKey("tenants.id"), nullable=False)` -- no index. The migration also omits the index. This is a minor consistency gap. In a single-tenant deployment it has zero performance impact, but if the table grows or multi-tenancy is ever added, queries by tenant will need a sequential scan. Consider adding `index=True` for pattern consistency. 2. **No `max_length` / `min_length` on Pydantic string fields:** `player_name` and `age_grade` accept any non-empty string (Pydantic requires the field but allows `""` -- an empty string passes validation). A blank `player_name` would create a useless lead record. Consider adding `min_length=1` on `player_name`, `age_grade`, and `parent_email` (the email regex would catch empty email, but the other two have no guard). Alternatively, a `field_validator` that strips and rejects whitespace-only strings. 3. **No `max_length` on Pydantic string fields:** The DB columns have length constraints (`String(200)`, `String(100)`, etc.) but the Pydantic schema does not enforce them. A 10,000-character `player_name` would pass Pydantic validation and then fail at the DB level with a less user-friendly error. Consider `Field(max_length=200)` on `player_name`, etc., to match the DB constraints. 4. **Logging PII:** `_notify_new_interest` logs `parent_email` at INFO level. This is a public endpoint so the volume of leads could be high. Consider whether email addresses in application logs are acceptable per your data handling posture, or whether the log should omit the email or use DEBUG level. 5. **`player_name` and `age_grade` not stripped:** The email validator calls `.lower().strip()`, but `player_name` and `age_grade` are stored as-is. Leading/trailing whitespace in a player name would create messy data. Consider stripping these fields in the endpoint or via validators. ### SOP COMPLIANCE - [x] Branch named after issue: `186-interest-leads-endpoint` references issue #186 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections present - [x] Related references issue: `Closes #186` - [x] No secrets committed - [x] No unnecessary file changes: 4 files changed, all directly related to the feature - [x] Commit messages are descriptive - [ ] Related section references plan slug: No plan slug referenced -- PR body says `basketball-api -- the basketball program management API` instead of a plan slug. Acceptable if this is standalone kanban work (no active plan). ### PROCESS OBSERVATIONS - **Deployment risk:** Low. New table, new endpoint. No schema changes to existing tables. Migration is additive only. - **Change failure risk:** Low. The endpoint is isolated from existing functionality. The 14 tests provide good regression coverage. - **Future work:** The `_notify_new_interest` TODO for gmail-sdk integration should be tracked as a separate issue to avoid it becoming forgotten tech debt. ### VERDICT: APPROVED