forgejo_admin/basketball-api

Fork 1

feat: add contract signing backend with status tracking and endpoints #98

Merged

forgejo_admin merged 1 commit from 97-feat-add-contract-signing-backend-status into main

2026-03-18 15:33:52 +00:00

forgejo_admin commented

2026-03-18 15:01:09 +00:00

Owner

Summary

Adds digital contract signing backend for player program contracts, following existing waiver/contractor agreement patterns
Parents can sign via POST endpoint (auth by email match); admins can sign for any player
Contract status (none/offered/signed) exposed in account and admin player list responses

Changes

src/basketball_api/models.py: added ContractStatus enum (none/offered/signed) and four new fields on Player: contract_status, contract_signed_at, contract_signed_by, contract_signed_ip
alembic/versions/010_add_contract_fields.py: migration adding the contractstatus enum type and columns to the players table (down_revision: 009)
src/basketball_api/routes/players.py: added GET /players/{id}/contract and POST /players/{id}/contract with ContractSignRequest/ContractStatusResponse schemas
src/basketball_api/routes/account.py: added contract_status to AccountPlayerResponse
src/basketball_api/routes/admin.py: added contract_status to AdminPlayerItem
tests/test_contract.py: 14 new tests covering all acceptance criteria

Test Plan

Tests pass locally -- all 333 tests pass (pytest tests/ -v)
14 new contract tests: parent/admin signing, rejected without accepted, 403 non-parent, 409 re-sign, 404/401, DB persistence, account/admin response inclusion
No Stripe integration in this PR -- contract signing only

Review Checklist

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

Closes forgejo_admin/basketball-api#97
plan-wkq -- Phase 14 (Program Contract Flow)
Parent issue: forgejo_admin/westside-app#39

## Summary - Adds digital contract signing backend for player program contracts, following existing waiver/contractor agreement patterns - Parents can sign via POST endpoint (auth by email match); admins can sign for any player - Contract status (none/offered/signed) exposed in account and admin player list responses ## Changes - `src/basketball_api/models.py`: added `ContractStatus` enum (none/offered/signed) and four new fields on Player: `contract_status`, `contract_signed_at`, `contract_signed_by`, `contract_signed_ip` - `alembic/versions/010_add_contract_fields.py`: migration adding the contractstatus enum type and columns to the players table (down_revision: 009) - `src/basketball_api/routes/players.py`: added `GET /players/{id}/contract` and `POST /players/{id}/contract` with ContractSignRequest/ContractStatusResponse schemas - `src/basketball_api/routes/account.py`: added `contract_status` to AccountPlayerResponse - `src/basketball_api/routes/admin.py`: added `contract_status` to AdminPlayerItem - `tests/test_contract.py`: 14 new tests covering all acceptance criteria ## Test Plan - [x] Tests pass locally -- all 333 tests pass (`pytest tests/ -v`) - [x] 14 new contract tests: parent/admin signing, rejected without accepted, 403 non-parent, 409 re-sign, 404/401, DB persistence, account/admin response inclusion - [x] No Stripe integration in this PR -- contract signing only ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/basketball-api#97 - `plan-wkq` -- Phase 14 (Program Contract Flow) - Parent issue: forgejo_admin/westside-app#39

forgejo_admin added 1 commit

2026-03-18 15:01:09 +00:00

feat: add contract signing backend with status tracking and endpoints

ci/woodpecker/pr/woodpecker Pipeline failed

Details

2b4f3b78b8

Add ContractStatus enum (none/offered/signed) and contract signing fields
to the Player model, following the same digital signature pattern as
Parent.waiver_signed and Coach.contractor_agreement_signed. Includes
POST/GET /players/{id}/contract endpoints with parent/admin auth, and
updates /account/players and /admin/players responses to include
contract_status. 14 new tests, all 333 tests pass.

Closes #97

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

forgejo_admin commented

2026-03-18 15:01:44 +00:00

Author

Owner

Self-Review: Pass

Reviewed the full diff (6 files, +549/-3). No issues found.

Model & Migration

ContractStatus enum and Player fields follow the exact pattern established by SubscriptionStatus/Parent.waiver_signed_*/Coach.contractor_agreement_*
Migration 010 correctly chains from 009, creates enum with checkfirst=True, downgrade cleans up in reverse order

Endpoints

GET /players/{id}/contract -- any authenticated user, matches get_player_profile pattern
POST /players/{id}/contract -- admin or parent (email match), matches update_player_profile permission model
Auth check order correct: permission -> business validation (accepted, already-signed)
IP capture via Request.client.host, same pattern as coach.py accept_agreement

Route ordering

/{player_id}/contract defined after /{player_id} -- FastAPI correctly matches literal contract segment before wildcard, no conflict

Account/Admin responses

Single-field additions with .value accessor, consistent with existing subscription_status pattern

Tests

14 tests covering all acceptance criteria
Follows existing test patterns from test_players.py and test_account.py
All 333 tests pass

No secrets, no unrelated changes.

## Self-Review: Pass Reviewed the full diff (6 files, +549/-3). No issues found. **Model & Migration** - `ContractStatus` enum and Player fields follow the exact pattern established by `SubscriptionStatus`/`Parent.waiver_signed_*`/`Coach.contractor_agreement_*` - Migration 010 correctly chains from 009, creates enum with `checkfirst=True`, downgrade cleans up in reverse order **Endpoints** - GET `/players/{id}/contract` -- any authenticated user, matches `get_player_profile` pattern - POST `/players/{id}/contract` -- admin or parent (email match), matches `update_player_profile` permission model - Auth check order correct: permission -> business validation (accepted, already-signed) - IP capture via `Request.client.host`, same pattern as `coach.py` `accept_agreement` **Route ordering** - `/{player_id}/contract` defined after `/{player_id}` -- FastAPI correctly matches literal `contract` segment before wildcard, no conflict **Account/Admin responses** - Single-field additions with `.value` accessor, consistent with existing `subscription_status` pattern **Tests** - 14 tests covering all acceptance criteria - Follows existing test patterns from `test_players.py` and `test_account.py` - All 333 tests pass **No secrets, no unrelated changes.**

forgejo_admin commented

2026-03-18 15:07:32 +00:00

Author

Owner

PR #98 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Pydantic -- Python/FastAPI domain checklist applies.

Migration (010_add_contract_fields.py)

Follows the exact pattern established by migration 009 (subscription fields): enum type creation with checkfirst=True, column additions with server_default, and clean downgrade that drops columns then the enum type.
Revision chain is correct: down_revision = "009", revision = "010".
Column types match the model: String(200) for contract_signed_by, String(45) for IP (IPv6-safe), DateTime for timestamp.

Model (ContractStatus enum + Player fields)

Enum follows existing SubscriptionStatus pattern. Placement in the file is consistent (between SubscriptionStatus and OnboardingStatus).
Player fields follow the established signing pattern: the Parent model has waiver_signed_at / waiver_signed_ip, the Coach model has contractor_agreement_signed_at / contractor_agreement_ip. This PR applies the same pattern to Player contracts. Consistent.

Endpoints (GET/POST /players/{id}/contract)

Permission model reuses the existing _is_player_owner helper from the same file -- no duplicated auth logic.
Permission check pattern (if "admin" not in user_roles: if not _is_player_owner(...)) matches the existing update_player_profile endpoint exactly.
IP capture pattern (request.client.host if request.client else "unknown") matches the waiver signing implementation in register.py:865-868.
Timezone-aware timestamps via datetime.now(timezone.utc) -- correct.
Proper idempotency guard (409 for already-signed contracts).
Audit logging with structured fields (player name, id, signer, user, IP).

Account + Admin response changes

Single field addition (contract_status: str) to both AccountPlayerResponse and AdminPlayerItem. Minimal surface area change, consistent with how subscription_status was added.

SQLAlchemy patterns

Uses joinedload for relationship loading (via existing _get_player_or_404 helper).
Proper db.commit() + db.refresh() after mutation.
No raw SQL, no string interpolation in queries -- parameterized by SQLAlchemy ORM throughout.

BLOCKERS

None.

Test coverage: 14 new tests across GET/POST contract endpoints + account/admin response inclusion. Covers happy path, 401, 403, 400, 404, 409, DB persistence. No gaps in the acceptance criteria.
Input validation: signature_name is a Pydantic str field (type-validated). SQL injection is not possible -- SQLAlchemy ORM uses parameterized queries throughout. No XSS risk since this is a JSON API (no HTML rendering of user input).
No secrets or credentials in code.
No DRY violation in auth paths -- reuses existing _is_player_owner helper.

NITS

signature_name has no length constraint -- The ContractSignRequest.signature_name field is a bare str with no min_length or max_length. The DB column is String(200). A name longer than 200 characters would cause a database-level truncation or error depending on the Postgres String behavior. Consider adding Field(min_length=1, max_length=200) to match the column constraint and provide a clean Pydantic validation error instead of a DB error. This also prevents empty-string signatures.
Duplicated ContractStatusResponse construction -- The response object is built identically in both get_contract_status and sign_contract (lines 238-246 and 312-320). A small _contract_status_response(player) helper would eliminate the duplication, matching the existing _player_profile_response pattern already in this file. Non-blocking since both occurrences are in the same file.
GET /players/{id}/contract allows any authenticated user -- The GET endpoint has no ownership check: any authenticated user can query any player's contract status. This matches the existing get_player_profile endpoint's behavior (also no ownership restriction on GET), so it is consistent. But worth flagging as a design choice to revisit if contract data becomes more sensitive.

SOP COMPLIANCE

Branch named after issue (97-feat-add-contract-signing-backend-status references issue #97)
PR body has: Summary, Changes, Test Plan, Related sections
Related references plan slug (plan-wkq Phase 14) and parent issue (westside-app#39)
Tests exist (14 new tests in tests/test_contract.py)
No secrets, .env files, or credentials committed
No unnecessary file changes (6 files, all directly related to contract signing)
Commit messages are descriptive

PROCESS OBSERVATIONS

Deployment frequency: Clean, scoped PR. Single feature, well-isolated. Merges without risk to existing functionality.
Change failure risk: Low. Migration is additive-only (new columns with defaults). No existing columns modified. Rollback via alembic downgrade 009 is clean.
Pattern consistency: The PR establishes a third signing pattern in the codebase (Parent waiver, Coach contractor agreement, Player contract). All three follow the same field convention (*_signed_at, *_signed_ip, *_signed_by). Good consistency.

VERDICT: APPROVED

## PR #98 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic / Pydantic -- Python/FastAPI domain checklist applies. **Migration (010_add_contract_fields.py)** - Follows the exact pattern established by migration 009 (subscription fields): enum type creation with `checkfirst=True`, column additions with `server_default`, and clean downgrade that drops columns then the enum type. - Revision chain is correct: `down_revision = "009"`, `revision = "010"`. - Column types match the model: `String(200)` for `contract_signed_by`, `String(45)` for IP (IPv6-safe), `DateTime` for timestamp. **Model (ContractStatus enum + Player fields)** - Enum follows existing `SubscriptionStatus` pattern. Placement in the file is consistent (between `SubscriptionStatus` and `OnboardingStatus`). - Player fields follow the established signing pattern: the Parent model has `waiver_signed_at` / `waiver_signed_ip`, the Coach model has `contractor_agreement_signed_at` / `contractor_agreement_ip`. This PR applies the same pattern to Player contracts. Consistent. **Endpoints (GET/POST /players/{id}/contract)** - Permission model reuses the existing `_is_player_owner` helper from the same file -- no duplicated auth logic. - Permission check pattern (`if "admin" not in user_roles: if not _is_player_owner(...)`) matches the existing `update_player_profile` endpoint exactly. - IP capture pattern (`request.client.host if request.client else "unknown"`) matches the waiver signing implementation in `register.py:865-868`. - Timezone-aware timestamps via `datetime.now(timezone.utc)` -- correct. - Proper idempotency guard (409 for already-signed contracts). - Audit logging with structured fields (player name, id, signer, user, IP). **Account + Admin response changes** - Single field addition (`contract_status: str`) to both `AccountPlayerResponse` and `AdminPlayerItem`. Minimal surface area change, consistent with how `subscription_status` was added. **SQLAlchemy patterns** - Uses `joinedload` for relationship loading (via existing `_get_player_or_404` helper). - Proper `db.commit()` + `db.refresh()` after mutation. - No raw SQL, no string interpolation in queries -- parameterized by SQLAlchemy ORM throughout. ### BLOCKERS None. - Test coverage: 14 new tests across GET/POST contract endpoints + account/admin response inclusion. Covers happy path, 401, 403, 400, 404, 409, DB persistence. No gaps in the acceptance criteria. - Input validation: `signature_name` is a Pydantic `str` field (type-validated). SQL injection is not possible -- SQLAlchemy ORM uses parameterized queries throughout. No XSS risk since this is a JSON API (no HTML rendering of user input). - No secrets or credentials in code. - No DRY violation in auth paths -- reuses existing `_is_player_owner` helper. ### NITS 1. **`signature_name` has no length constraint** -- The `ContractSignRequest.signature_name` field is a bare `str` with no `min_length` or `max_length`. The DB column is `String(200)`. A name longer than 200 characters would cause a database-level truncation or error depending on the Postgres `String` behavior. Consider adding `Field(min_length=1, max_length=200)` to match the column constraint and provide a clean Pydantic validation error instead of a DB error. This also prevents empty-string signatures. 2. **Duplicated `ContractStatusResponse` construction** -- The response object is built identically in both `get_contract_status` and `sign_contract` (lines 238-246 and 312-320). A small `_contract_status_response(player)` helper would eliminate the duplication, matching the existing `_player_profile_response` pattern already in this file. Non-blocking since both occurrences are in the same file. 3. **GET /players/{id}/contract allows any authenticated user** -- The GET endpoint has no ownership check: any authenticated user can query any player's contract status. This matches the existing `get_player_profile` endpoint's behavior (also no ownership restriction on GET), so it is consistent. But worth flagging as a design choice to revisit if contract data becomes more sensitive. ### SOP COMPLIANCE - [x] Branch named after issue (`97-feat-add-contract-signing-backend-status` references issue #97) - [x] PR body has: Summary, Changes, Test Plan, Related sections - [x] Related references plan slug (`plan-wkq` Phase 14) and parent issue (westside-app#39) - [x] Tests exist (14 new tests in `tests/test_contract.py`) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (6 files, all directly related to contract signing) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean, scoped PR. Single feature, well-isolated. Merges without risk to existing functionality. - **Change failure risk**: Low. Migration is additive-only (new columns with defaults). No existing columns modified. Rollback via `alembic downgrade 009` is clean. - **Pattern consistency**: The PR establishes a third signing pattern in the codebase (Parent waiver, Coach contractor agreement, Player contract). All three follow the same field convention (`*_signed_at`, `*_signed_ip`, `*_signed_by`). Good consistency. ### VERDICT: APPROVED