feat: add POST /admin/contract/offer endpoint (#425) #426
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!426
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "425-admin-contract-offer-endpoint"
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
Adds a committed endpoint for minting contract offers on players, closing the gap where the existing
/admin/email/blast + query_unsigned_contractspath 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 andContractAuditLogmodel. Definesoffer_contract()which implements the state machine, token minting viasecrets.token_urlsafe(32), and JSONB snapshots for re-offer / tier-change events.src/basketball_api/routes/admin.py— newPOST /admin/contract/offerhandler with Pydantic request/response models.forceis a query param per the issue spec. Route-level try/except wraps the service call in an explicitdb.commit()/db.rollback()transaction boundary (notbegin_nested()).alembic/versions/040_add_contract_audit_log.py(new) — createscontract_audit_logtable (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 bogustarget_team_id, cross-tenant isolation, and the offer->blast integration round-trip.State transitions
noneofferedmoved_teams=false,archived=false.declinedofferedofferedoffered?force=true. With force: mint NEW token, logre_offeraudit row.signedofferedtarget_team_iddifferent from current. Archives signed fields (includingcontract_token) + old team_ids + fee tocontract_audit_log.old_state, clears signed fields on player, mints new token, moves teams, logstier_change.archived=true.Test Plan
pytest tests/test_contract_offer.py— 9/9 passtest_admin.py test_admin_email.py test_email_blast.py test_contract.py test_contract_reminder.py test_contract_offer.py) — 104 passed\d contract_audit_logruff format+ruff checkclean on entire repoReview Checklist
secrets.token_urlsafe(32)(matchesroutes/admin.py:94precedent)db.commit()/db.rollback()at the route level), NOTbegin_nested()/ SAVEPOINTold_stateincludescontract_token(critical for tracing disputed signed URLs)models.py)email.py,email_queries.py, or theplayerstable inmodels.pyContractAuditLogmodel placed in the new service file so the "don't touch models.py" constraint stays literal; registered onBase.metadatavia the admin-route import chainrequire_admindependencytarget_team_id, signed with sametarget_team_id, offered without?force=true(409)/admin/email/blastseparately)player_id,previous_status,new_status,actor,moved_teams,archivedruff formatandruff checkcleanDecisions made without explicit spec coverage
mainis039, not021. The issue body was written against an older tree. New migration uses slot040(depends on039,add_recovery_email_sent).ContractAuditLogSQLAlchemy model lives inservices/contract_offers.py, notmodels.py. The issue explicitly says "no code changes tomodels.py". Placing the model in the service keeps that constraint literal, and importing the service from the admin route registers the table onBase.metadatabeforecreate_allruns in the test fixture (tests don't invoke alembic).db.commit()/db.rollback()at the route level. SQLAlchemy's session auto-begins, sowith 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.actoris sourced fromuser.email(the admin JWT subject) and falls through to"admin"when absent.previous_signed_state_archivedisfalsefor theoffered -> offeredforce re-offer path even though an audit row IS written. Per the issue spec, that flag reports specifically whether signed state was archived; there_offerevent type is a distinct case. Testtest_offer_from_offered_with_forceasserts both the flag and the audit row explicitly._maybe_move_teamalso validates the target team lives in the same tenant (422 if not) — this drives the transaction-rollback test, since bogustarget_team_idsurfaces 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) — unchangedsend_emailflag on the endpoint; caller separately hits/admin/email/blastquery_unsigned_contractsRelated
story:WS-S23(primary),story:WS-S7(secondary)Related Notes
sop-email-sendin pal-e-docs (the blast endpoint usage pattern this plugs into)~/westside-email-agent/CLAUDE.md(Keycloak JWT auth flow used byrequire_admin)feedback_never_alter_prod_directly(this endpoint is the committed replacement for ad-hoc SQL on prod)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, writesre_offeraudit row.archived=Falseis semantically right per the pre-accepted judgment call.signed -> offered: validatestarget_team_idpresent AND different from current teams, snapshots full signed state (includingcontract_token), clears signed fields, mints new token, moves team, writestier_changeaudit row witharchived=True. Correct.Audit archive —
_snapshot_signed_stateincludescontract_token,contract_signed_at/by/ip,contract_signature_url,contract_version,team_ids,monthly_fee. The critical disputed-URL trace requirement is met. Testtest_signed_to_offered_archives_stateassertsold_state["contract_token"] == "signed-token-ccc"andtest_offer_from_offered_with_forceasserts the same on re_offer. Both paths verified.Transaction atomicity — Route-level try/except wraps
offer_contract()+db.commit()withdb.rollback()on both HTTPException and generic Exception. The service usesdb.add(ContractAuditLog(...))+db.flush()so audit INSERT and player UPDATE live in the same unit of work, committed atomically by the route._maybe_move_teamsurfaces bogus team IDs as 422 before any commit, which is then rolled back.test_transaction_rollback_on_failureverifies 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.ContractAuditLogcorrectly lives inservices/contract_offers.py.playerstable schema — only column writes, no DDL.Test coverage (9 cases):
test_offer_from_none— happy path + token length + URL format + no audittest_offer_from_declined— re-engage pathtest_offer_from_offered_rejects— 409 + unchanged statetest_offer_from_offered_with_force— new token + re_offer audit + old token archivedtest_signed_to_offered_requires_team_change— both missing AND same-team 422s, audit count == 0test_signed_to_offered_archives_state— full Jacelyn case, old_state contents, team movedtest_transaction_rollback_on_failure— rollback assertiontest_cross_tenant_rejected— 404 on wrong tenanttest_offer_then_blast_flow— end-to-end integration withquery_unsigned_contractsAll 4 transitions + tenant boundary + rollback covered.
Domain notes (Python/FastAPI/SQLAlchemy):
forcecorrectly as query param.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_default="admin") and migration (server_default=sa.text("'admin'")) match — good._maybe_move_teamtenant-scopes the target team lookup — prevents cross-tenant team injection viatarget_team_id. Nice catch.monthly_fee(positive int check) happens before the Player query — good ordering.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.datetimeis imported in the test file but only used once in_make_signed_player. Fine._snapshot_new_statedict includestarget_team_id(the requested value) rather than the actual resolved team id after move. Minor — if a future caller passestarget_team_idmatching 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_CHANGEconstants are good; consider an Enum in a follow-up if more event types arrive (ops_audit_log from pal-e-deployments#104).# Contract offer minting (issue #425)is nice documentation — keep the pattern.SOP COMPLIANCE
425-admin-contract-offer-endpointruff format+ruff checkclean per PR bodyrequire_adminPROCESS OBSERVATIONS
040. Flagged per instructions — merge-order will resolve, loser rebases. Not a blocker on this PR.ops_audit_logis clearly noted, so the broader audit system has a migration path.VERDICT: APPROVED
7d28518fe280d5b17cb8forgejo_admin referenced this pull request2026-04-11 20:21:09 +00:00