feat: add POST /admin/contract/offer endpoint (#425) #426

Merged
forgejo_admin merged 2 commits from 425-admin-contract-offer-endpoint into main 2026-04-10 22:53:14 +00:00

Summary

Adds a committed endpoint for minting contract offers on players, closing the gap where the existing /admin/email/blast + query_unsigned_contracts path could only find offers but not create them. Handles all four state transitions with explicit archive-and-audit semantics for the signed -> offered tier-change case.

Changes

  • src/basketball_api/services/contract_offers.py (new) — business logic and ContractAuditLog model. Defines offer_contract() which implements the state machine, token minting via secrets.token_urlsafe(32), and JSONB snapshots for re-offer / tier-change events.
  • src/basketball_api/routes/admin.py — new POST /admin/contract/offer handler with Pydantic request/response models. force is a query param per the issue spec. Route-level try/except wraps the service call in an explicit db.commit() / db.rollback() transaction boundary (not begin_nested()).
  • alembic/versions/040_add_contract_audit_log.py (new) — creates contract_audit_log table (id, ts, player_id FK w/ CASCADE, event_type, old_state JSONB, new_state JSONB, actor, source).
  • tests/test_contract_offer.py (new) — 9 test cases covering every transition, force semantics, archive contents (contract_token included), transaction rollback on bogus target_team_id, cross-tenant isolation, and the offer->blast integration round-trip.

State transitions

From To Behavior
none offered Mint token, set fee. moved_teams=false, archived=false.
declined offered Same as above.
offered offered 409 without ?force=true. With force: mint NEW token, log re_offer audit row.
signed offered Requires target_team_id different from current. Archives signed fields (including contract_token) + old team_ids + fee to contract_audit_log.old_state, clears signed fields on player, mints new token, moves teams, logs tier_change. archived=true.

Test Plan

  • pytest tests/test_contract_offer.py — 9/9 pass
  • Full regression across related suites (test_admin.py test_admin_email.py test_email_blast.py test_contract.py test_contract_reminder.py test_contract_offer.py) — 104 passed
  • Migration 040 applies cleanly on a fresh Postgres stamped at 039; table schema verified via \d contract_audit_log
  • ruff format + ruff check clean on entire repo
  • CI green on PR (pending)
  • Post-merge: smoke test on prod via single test player, then run Marcus batch (#424)

Review Checklist

  • Token generation uses secrets.token_urlsafe(32) (matches routes/admin.py:94 precedent)
  • Transaction boundary is a real top-level transaction (explicit db.commit() / db.rollback() at the route level), NOT begin_nested() / SAVEPOINT
  • Archived JSONB old_state includes contract_token (critical for tracing disputed signed URLs)
  • Canonical Player field names used throughout (verified against models.py)
  • No changes to email.py, email_queries.py, or the players table in models.py
  • ContractAuditLog model placed in the new service file so the "don't touch models.py" constraint stays literal; registered on Base.metadata via the admin-route import chain
  • Endpoint requires admin JWT via the existing require_admin dependency
  • Cross-tenant requests rejected with 404 (tenant_id scoping on Player query)
  • 422 on invalid transitions: signed without target_team_id, signed with same target_team_id, offered without ?force=true (409)
  • No jersey fields touched, no email sent (caller hits /admin/email/blast separately)
  • Logger includes player_id, previous_status, new_status, actor, moved_teams, archived
  • ruff format and ruff check clean

Decisions made without explicit spec coverage

  1. Latest migration on main is 039, not 021. The issue body was written against an older tree. New migration uses slot 040 (depends on 039, add_recovery_email_sent).
  2. ContractAuditLog SQLAlchemy model lives in services/contract_offers.py, not models.py. The issue explicitly says "no code changes to models.py". Placing the model in the service keeps that constraint literal, and importing the service from the admin route registers the table on Base.metadata before create_all runs in the test fixture (tests don't invoke alembic).
  3. Transaction boundary is explicit db.commit() / db.rollback() at the route level. SQLAlchemy's session auto-begins, so with db.begin() conflicts with the test fixture's shared session. The semantics match the issue's intent ("top-level transaction, not SAVEPOINT") and mirror every other admin route in this file.
  4. actor is sourced from user.email (the admin JWT subject) and falls through to "admin" when absent.
  5. previous_signed_state_archived is false for the offered -> offered force re-offer path even though an audit row IS written. Per the issue spec, that flag reports specifically whether signed state was archived; the re_offer event type is a distinct case. Test test_offer_from_offered_with_force asserts both the flag and the audit row explicitly.
  6. _maybe_move_team also validates the target team lives in the same tenant (422 if not) — this drives the transaction-rollback test, since bogus target_team_id surfaces as a clean 422 rather than an FK violation mid-transaction.

Explicitly NOT touched

  • services/email.py, services/email_queries.py, models.py (player table) — unchanged
  • No send_email flag on the endpoint; caller separately hits /admin/email/blast
  • No jersey fields touched
  • No changes to query_unsigned_contracts
  • Closes #425
  • Forgejo issue: forgejo_admin/basketball-api#425
  • Unblocks: basketball-api#424 (Marcus 2026-04-10 batch)
  • Soft-depends (smoke-test only, not PR): basketball-api#420 (Alice dedupe), basketball-api#422 (Local Queens team)
  • Related: pal-e-deployments#104 (full ops audit log — this is a narrower slice)
  • Story labels: story:WS-S23 (primary), story:WS-S7 (secondary)
  • sop-email-send in pal-e-docs (the blast endpoint usage pattern this plugs into)
  • ~/westside-email-agent/CLAUDE.md (Keycloak JWT auth flow used by require_admin)
  • feedback_never_alter_prod_directly (this endpoint is the committed replacement for ad-hoc SQL on prod)
## Summary Adds a committed endpoint for minting contract offers on players, closing the gap where the existing `/admin/email/blast + query_unsigned_contracts` path could only *find* offers but not *create* them. Handles all four state transitions with explicit archive-and-audit semantics for the signed -> offered tier-change case. ## Changes - `src/basketball_api/services/contract_offers.py` (new) — business logic and `ContractAuditLog` model. Defines `offer_contract()` which implements the state machine, token minting via `secrets.token_urlsafe(32)`, and JSONB snapshots for re-offer / tier-change events. - `src/basketball_api/routes/admin.py` — new `POST /admin/contract/offer` handler with Pydantic request/response models. `force` is a query param per the issue spec. Route-level try/except wraps the service call in an explicit `db.commit()` / `db.rollback()` transaction boundary (not `begin_nested()`). - `alembic/versions/040_add_contract_audit_log.py` (new) — creates `contract_audit_log` table (id, ts, player_id FK w/ CASCADE, event_type, old_state JSONB, new_state JSONB, actor, source). - `tests/test_contract_offer.py` (new) — 9 test cases covering every transition, force semantics, archive contents (contract_token included), transaction rollback on bogus `target_team_id`, cross-tenant isolation, and the offer->blast integration round-trip. ## State transitions | From | To | Behavior | |---|---|---| | `none` | `offered` | Mint token, set fee. `moved_teams=false`, `archived=false`. | | `declined` | `offered` | Same as above. | | `offered` | `offered` | 409 without `?force=true`. With force: mint NEW token, log `re_offer` audit row. | | `signed` | `offered` | Requires `target_team_id` different from current. Archives signed fields (including `contract_token`) + old team_ids + fee to `contract_audit_log.old_state`, clears signed fields on player, mints new token, moves teams, logs `tier_change`. `archived=true`. | ## Test Plan - [x] `pytest tests/test_contract_offer.py` — 9/9 pass - [x] Full regression across related suites (`test_admin.py test_admin_email.py test_email_blast.py test_contract.py test_contract_reminder.py test_contract_offer.py`) — 104 passed - [x] Migration 040 applies cleanly on a fresh Postgres stamped at 039; table schema verified via `\d contract_audit_log` - [x] `ruff format` + `ruff check` clean on entire repo - [ ] CI green on PR (pending) - [ ] Post-merge: smoke test on prod via single test player, then run Marcus batch (#424) ## Review Checklist - [x] Token generation uses `secrets.token_urlsafe(32)` (matches `routes/admin.py:94` precedent) - [x] Transaction boundary is a real top-level transaction (explicit `db.commit()` / `db.rollback()` at the route level), NOT `begin_nested()` / SAVEPOINT - [x] Archived JSONB `old_state` includes `contract_token` (critical for tracing disputed signed URLs) - [x] Canonical Player field names used throughout (verified against `models.py`) - [x] No changes to `email.py`, `email_queries.py`, or the `players` table in `models.py` - [x] `ContractAuditLog` model placed in the new service file so the "don't touch models.py" constraint stays literal; registered on `Base.metadata` via the admin-route import chain - [x] Endpoint requires admin JWT via the existing `require_admin` dependency - [x] Cross-tenant requests rejected with 404 (tenant_id scoping on Player query) - [x] 422 on invalid transitions: signed without `target_team_id`, signed with same `target_team_id`, offered without `?force=true` (409) - [x] No jersey fields touched, no email sent (caller hits `/admin/email/blast` separately) - [x] Logger includes `player_id`, `previous_status`, `new_status`, `actor`, `moved_teams`, `archived` - [x] `ruff format` and `ruff check` clean ## Decisions made without explicit spec coverage 1. **Latest migration on `main` is `039`, not `021`.** The issue body was written against an older tree. New migration uses slot `040` (depends on `039`, `add_recovery_email_sent`). 2. **`ContractAuditLog` SQLAlchemy model lives in `services/contract_offers.py`, not `models.py`.** The issue explicitly says "no code changes to `models.py`". Placing the model in the service keeps that constraint literal, and importing the service from the admin route registers the table on `Base.metadata` before `create_all` runs in the test fixture (tests don't invoke alembic). 3. **Transaction boundary is explicit `db.commit()` / `db.rollback()` at the route level.** SQLAlchemy's session auto-begins, so `with db.begin()` conflicts with the test fixture's shared session. The semantics match the issue's intent ("top-level transaction, not SAVEPOINT") and mirror every other admin route in this file. 4. **`actor` is sourced from `user.email`** (the admin JWT subject) and falls through to `"admin"` when absent. 5. **`previous_signed_state_archived` is `false` for the `offered -> offered` force re-offer path** even though an audit row IS written. Per the issue spec, that flag reports specifically whether *signed* state was archived; the `re_offer` event type is a distinct case. Test `test_offer_from_offered_with_force` asserts both the flag and the audit row explicitly. 6. **`_maybe_move_team` also validates the target team lives in the same tenant** (422 if not) — this drives the transaction-rollback test, since bogus `target_team_id` surfaces as a clean 422 rather than an FK violation mid-transaction. ## Explicitly NOT touched - `services/email.py`, `services/email_queries.py`, `models.py` (player table) — unchanged - No `send_email` flag on the endpoint; caller separately hits `/admin/email/blast` - No jersey fields touched - No changes to `query_unsigned_contracts` ## Related - Closes #425 - Forgejo issue: forgejo_admin/basketball-api#425 - Unblocks: basketball-api#424 (Marcus 2026-04-10 batch) - Soft-depends (smoke-test only, not PR): basketball-api#420 (Alice dedupe), basketball-api#422 (Local Queens team) - Related: pal-e-deployments#104 (full ops audit log — this is a narrower slice) - Story labels: `story:WS-S23` (primary), `story:WS-S7` (secondary) ## Related Notes - `sop-email-send` in pal-e-docs (the blast endpoint usage pattern this plugs into) - `~/westside-email-agent/CLAUDE.md` (Keycloak JWT auth flow used by `require_admin`) - `feedback_never_alter_prod_directly` (this endpoint is the committed replacement for ad-hoc SQL on prod)
feat: add POST /admin/contract/offer endpoint (#425)
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
7d28518fe2
Mints contract offers for players with explicit state-machine handling of
all four transitions: none/declined -> offered (happy path), offered ->
offered (refused without ?force=true), and signed -> offered (tier change
that archives signed state to contract_audit_log and moves teams).

Token generation uses secrets.token_urlsafe(32).  State mutations run in
a top-level transaction (route-level commit/rollback, not SAVEPOINT).  The
signed-state archive includes contract_token for tracing disputed signed
URLs.

Adds migration 040 creating contract_audit_log (id, ts, player_id FK,
event_type, old_state JSONB, new_state JSONB, actor, source) as a narrow
predecessor to the ops_audit_log from pal-e-deployments#104.

Does not send email: caller hits /admin/email/blast separately with
query=unsigned_contracts, which picks up newly-offered players via the
existing query_unsigned_contracts path.

Covers all 9 test cases from the ticket: none/declined happy paths,
offered rejection + force re-offer, signed tier-change archive +
team-change requirement, transaction rollback on bogus target_team_id,
cross-tenant 404, and the offer->blast integration round-trip.

Refs: basketball-api#425
Unblocks: basketball-api#424 (Marcus 2026-04-10 batch)

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

PR #426 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy 2.0 (Mapped typing) / Alembic / Postgres JSONB. Tests use pytest + TestClient with dependency overrides.

State machine (all 4 transitions) — correctly implemented:

  • none -> offered: mints token, sets fee, no audit row. Correct.
  • declined -> offered: identical happy path. Correct.
  • offered -> offered: 409 without ?force=true; with force mints new token, writes re_offer audit row. archived=False is semantically right per the pre-accepted judgment call.
  • signed -> offered: validates target_team_id present AND different from current teams, snapshots full signed state (including contract_token), clears signed fields, mints new token, moves team, writes tier_change audit row with archived=True. Correct.

Audit archive_snapshot_signed_state includes contract_token, contract_signed_at/by/ip, contract_signature_url, contract_version, team_ids, monthly_fee. The critical disputed-URL trace requirement is met. Test test_signed_to_offered_archives_state asserts old_state["contract_token"] == "signed-token-ccc" and test_offer_from_offered_with_force asserts the same on re_offer. Both paths verified.

Transaction atomicity — Route-level try/except wraps offer_contract() + db.commit() with db.rollback() on both HTTPException and generic Exception. The service uses db.add(ContractAuditLog(...)) + db.flush() so audit INSERT and player UPDATE live in the same unit of work, committed atomically by the route. _maybe_move_team surfaces bogus team IDs as 422 before any commit, which is then rolled back. test_transaction_rollback_on_failure verifies player row + audit count are both unchanged after a 422. Atomicity confirmed.

Files untouched (verified in diff):

  • services/email.py — not in diff.
  • services/email_queries.py — not in diff (only imported by test).
  • models.py — not in diff. ContractAuditLog correctly lives in services/contract_offers.py.
  • Jersey fields — not referenced anywhere.
  • players table schema — only column writes, no DDL.

Test coverage (9 cases):

  1. test_offer_from_none — happy path + token length + URL format + no audit
  2. test_offer_from_declined — re-engage path
  3. test_offer_from_offered_rejects — 409 + unchanged state
  4. test_offer_from_offered_with_force — new token + re_offer audit + old token archived
  5. test_signed_to_offered_requires_team_change — both missing AND same-team 422s, audit count == 0
  6. test_signed_to_offered_archives_state — full Jacelyn case, old_state contents, team moved
  7. test_transaction_rollback_on_failure — rollback assertion
  8. test_cross_tenant_rejected — 404 on wrong tenant
  9. test_offer_then_blast_flow — end-to-end integration with query_unsigned_contracts

All 4 transitions + tenant boundary + rollback covered.

Domain notes (Python/FastAPI/SQLAlchemy):

  • Pydantic models are clean, types explicit, force correctly as query param.
  • Service uses Mapped[...] typing consistently with SQLAlchemy 2.0 style.
  • db.expire(player, ["teams"]) after team swap is the correct pattern to force reload of the relationship.
  • Server defaults on the ORM model (server_default="admin") and migration (server_default=sa.text("'admin'")) match — good.
  • _maybe_move_team tenant-scopes the target team lookup — prevents cross-tenant team injection via target_team_id. Nice catch.
  • Input validation on monthly_fee (positive int check) happens before the Player query — good ordering.
  • Logger includes all required context fields.
  • JSONB column uses postgresql.JSONB(astext_type=sa.Text()) in the migration — standard pattern.

BLOCKERS

None.

NITS

  • _CONTRACT_BASE_URL = "https://westside-contracts.tail5b443a.ts.net/contract" is hardcoded. Matches existing precedent in the codebase (per PR body), but should be env-configured eventually. Non-blocking — file an epilogue subphase during /update-docs.
  • datetime is imported in the test file but only used once in _make_signed_player. Fine.
  • The _snapshot_new_state dict includes target_team_id (the requested value) rather than the actual resolved team id after move. Minor — if a future caller passes target_team_id matching current team on a none→offered transition, the snapshot records the request, not the outcome. Not a correctness issue given current tests.
  • EVENT_RE_OFFER / EVENT_TIER_CHANGE constants are good; consider an Enum in a follow-up if more event types arrive (ops_audit_log from pal-e-deployments#104).
  • Route comment block header # Contract offer minting (issue #425) is nice documentation — keep the pattern.

SOP COMPLIANCE

  • Branch named after issue: 425-admin-contract-offer-endpoint
  • PR body follows template: Summary, Changes, Test Plan, Related all present
  • Related references issue #425, unblocks #424, mentions pal-e-deployments#104
  • No secrets committed
  • Tests exist and pass (9/9 + 104 regression per PR body)
  • ruff format + ruff check clean per PR body
  • Explicit "Decisions made without explicit spec coverage" section — excellent SOP hygiene
  • Admin JWT enforced via require_admin
  • Tenant scoping on Player query (404 cross-tenant)

PROCESS OBSERVATIONS

  • Migration collision with PR #427: Both claim slot 040. Flagged per instructions — merge-order will resolve, loser rebases. Not a blocker on this PR.
  • Deployment frequency: Net-new endpoint with high test coverage, ready to ship. Unblocks Marcus batch (#424).
  • Change failure risk: Low. State machine is small, dispatch is on a single enum, rollback test verifies atomicity, cross-tenant test verifies isolation, archive test verifies the disputed-URL trace requirement.
  • Documentation: PR body documents all 6 judgment calls explicitly — review chain can audit them. Narrow precursor to ops_audit_log is clearly noted, so the broader audit system has a migration path.
  • Post-merge: PR body already lists smoke-test plan (single player → Marcus batch). Good.

VERDICT: APPROVED

## PR #426 Review ### DOMAIN REVIEW Stack: Python / FastAPI / SQLAlchemy 2.0 (Mapped typing) / Alembic / Postgres JSONB. Tests use pytest + TestClient with dependency overrides. **State machine (all 4 transitions)** — correctly implemented: - `none -> offered`: mints token, sets fee, no audit row. Correct. - `declined -> offered`: identical happy path. Correct. - `offered -> offered`: 409 without `?force=true`; with force mints new token, writes `re_offer` audit row. `archived=False` is semantically right per the pre-accepted judgment call. - `signed -> offered`: validates `target_team_id` present AND different from current teams, snapshots full signed state (including `contract_token`), clears signed fields, mints new token, moves team, writes `tier_change` audit row with `archived=True`. Correct. **Audit archive** — `_snapshot_signed_state` includes `contract_token`, `contract_signed_at/by/ip`, `contract_signature_url`, `contract_version`, `team_ids`, `monthly_fee`. The critical disputed-URL trace requirement is met. Test `test_signed_to_offered_archives_state` asserts `old_state["contract_token"] == "signed-token-ccc"` and `test_offer_from_offered_with_force` asserts the same on re_offer. Both paths verified. **Transaction atomicity** — Route-level try/except wraps `offer_contract()` + `db.commit()` with `db.rollback()` on both HTTPException and generic Exception. The service uses `db.add(ContractAuditLog(...))` + `db.flush()` so audit INSERT and player UPDATE live in the same unit of work, committed atomically by the route. `_maybe_move_team` surfaces bogus team IDs as 422 *before* any commit, which is then rolled back. `test_transaction_rollback_on_failure` verifies player row + audit count are both unchanged after a 422. Atomicity confirmed. **Files untouched (verified in diff)**: - `services/email.py` — not in diff. - `services/email_queries.py` — not in diff (only imported by test). - `models.py` — not in diff. `ContractAuditLog` correctly lives in `services/contract_offers.py`. - Jersey fields — not referenced anywhere. - `players` table schema — only column writes, no DDL. **Test coverage (9 cases)**: 1. `test_offer_from_none` — happy path + token length + URL format + no audit 2. `test_offer_from_declined` — re-engage path 3. `test_offer_from_offered_rejects` — 409 + unchanged state 4. `test_offer_from_offered_with_force` — new token + re_offer audit + old token archived 5. `test_signed_to_offered_requires_team_change` — both missing AND same-team 422s, audit count == 0 6. `test_signed_to_offered_archives_state` — full Jacelyn case, old_state contents, team moved 7. `test_transaction_rollback_on_failure` — rollback assertion 8. `test_cross_tenant_rejected` — 404 on wrong tenant 9. `test_offer_then_blast_flow` — end-to-end integration with `query_unsigned_contracts` All 4 transitions + tenant boundary + rollback covered. **Domain notes (Python/FastAPI/SQLAlchemy)**: - Pydantic models are clean, types explicit, `force` correctly as query param. - Service uses `Mapped[...]` typing consistently with SQLAlchemy 2.0 style. - `db.expire(player, ["teams"])` after team swap is the correct pattern to force reload of the relationship. - Server defaults on the ORM model (`server_default="admin"`) and migration (`server_default=sa.text("'admin'")`) match — good. - `_maybe_move_team` tenant-scopes the target team lookup — prevents cross-tenant team injection via `target_team_id`. Nice catch. - Input validation on `monthly_fee` (positive int check) happens before the Player query — good ordering. - Logger includes all required context fields. - JSONB column uses `postgresql.JSONB(astext_type=sa.Text())` in the migration — standard pattern. ### BLOCKERS None. ### NITS - `_CONTRACT_BASE_URL = "https://westside-contracts.tail5b443a.ts.net/contract"` is hardcoded. Matches existing precedent in the codebase (per PR body), but should be env-configured eventually. Non-blocking — file an epilogue subphase during /update-docs. - `datetime` is imported in the test file but only used once in `_make_signed_player`. Fine. - The `_snapshot_new_state` dict includes `target_team_id` (the requested value) rather than the actual resolved team id after move. Minor — if a future caller passes `target_team_id` matching current team on a none→offered transition, the snapshot records the request, not the outcome. Not a correctness issue given current tests. - `EVENT_RE_OFFER` / `EVENT_TIER_CHANGE` constants are good; consider an Enum in a follow-up if more event types arrive (ops_audit_log from pal-e-deployments#104). - Route comment block header `# Contract offer minting (issue #425)` is nice documentation — keep the pattern. ### SOP COMPLIANCE - [x] Branch named after issue: `425-admin-contract-offer-endpoint` - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [x] Related references issue #425, unblocks #424, mentions pal-e-deployments#104 - [x] No secrets committed - [x] Tests exist and pass (9/9 + 104 regression per PR body) - [x] `ruff format` + `ruff check` clean per PR body - [x] Explicit "Decisions made without explicit spec coverage" section — excellent SOP hygiene - [x] Admin JWT enforced via `require_admin` - [x] Tenant scoping on Player query (404 cross-tenant) ### PROCESS OBSERVATIONS - **Migration collision with PR #427**: Both claim slot `040`. Flagged per instructions — merge-order will resolve, loser rebases. Not a blocker on this PR. - **Deployment frequency**: Net-new endpoint with high test coverage, ready to ship. Unblocks Marcus batch (#424). - **Change failure risk**: Low. State machine is small, dispatch is on a single enum, rollback test verifies atomicity, cross-tenant test verifies isolation, archive test verifies the disputed-URL trace requirement. - **Documentation**: PR body documents all 6 judgment calls explicitly — review chain can audit them. Narrow precursor to `ops_audit_log` is clearly noted, so the broader audit system has a migration path. - **Post-merge**: PR body already lists smoke-test plan (single player → Marcus batch). Good. ### VERDICT: APPROVED
forgejo_admin force-pushed 425-admin-contract-offer-endpoint from 7d28518fe2
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
to 80d5b17cb8
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
2026-04-10 22:32:35 +00:00
Compare
forgejo_admin deleted branch 425-admin-contract-offer-endpoint 2026-04-10 22:53:14 +00:00
Sign in to join this conversation.
No description provided.