pal-e-docs

Review: Add POST /admin/contract/offer endpoint

review-931-2026-04-10 Doc

review needs-refinement
Project

Review: Add POST /admin/contract/offer endpoint (board #931, basketball-api #425)

Verdict: NEEDS_REFINEMENT (minor). All file targets verified, scope is well-specified, dependencies real. Two small gaps before promotion: field-name drift and missing arch backing note. No decomposition needed.

Template completeness (template-issue-feature, 11 sections)

  • Type — present (Feature)
  • Lineage — present (standalone, 2026-04-10 Westside Ops, links to #424)
  • Repo — present (forgejo_admin/basketball-api)
  • User Story — present (Ava/admin agent minting contract offers)
  • Context — present, unusually thorough; explains existing code state, the gap, and why now
  • File Targets — present, with explicit do-not-touch list
  • Acceptance Criteria — present, 14+ criteria covering request/response shape, all four state transitions, security, DB integrity, explicit non-goals
  • Test Expectations — present, 9 unit tests + 1 integration + run command
  • Constraints — present, covers patterns, token source, transaction boundaries, logging, audit-log scope
  • Checklist — present, 9 items including post-merge ArgoCD sync + smoke test
  • Related — present, links to #424, #422, #420, pal-e-deployments#104, sop-email-send, westside-email-agent
All 11 sections present. Template completeness: PASS.

Traceability triangle

  • story:WS-S7 — story exists on project-westside-basketball (Admin: "As an admin, I want to send branded email announcements so that parent comms are professional and consistent"). Fit is indirect: this endpoint mints the offered state that the branded-email blast flow consumes. Defensible as a prerequisite enabler, but a more literal fit would be WS-S23 ("configure custom contract terms per player"). Recommend: add WS-S23 as a secondary story label, keep WS-S7 as primary.
  • arch:basketball-api — label present on the board item, but no backing arch-basketball-api note exists in pal-e-docs (search_notes returns empty). [SCOPE] Create architecture note arch-basketball-api. This is a pre-existing gap across many basketball-api tickets, not unique to #931 — flag for later cleanup, do not block this ticket on it.
  • Forgejo issue — #425 open, body well-formed, matches board item title.

File targets — verified against fresh clone of main (commit 9598c4d)

  • src/basketball_api/routes/admin.py — EXISTS. Uses require_admin, Depends(get_db), imports player_teams junction, uses secrets.token_urlsafe(32) in /admin/generate-tokens (confirmed line 94). Convention target is sound.
  • src/basketball_api/services/contract_offers.py — NEW file, absent as expected.
  • tests/test_contract_offer.py — NEW file, absent as expected.
  • alembic/versions/NNN_add_contract_audit_log.py — latest version on main is 021. New migration number will be 022 (verify no in-flight PRs with conflicting 022 before merge).
  • src/basketball_api/services/email.py — EXISTS (do-not-touch).
  • src/basketball_api/services/email_queries.py — EXISTS, query_unsigned_contracts defined at line 31, registered in query dispatch at line 157 (do-not-touch).
  • src/basketball_api/models.py — EXISTS. Verified all referenced Player fields: contract_status (line 254), contract_signed_at/by/ip (257-259), contract_token (260), contract_signature_url (263), contract_version (266), monthly_fee (267).

Field-name drift (fixable nit)

Ticket refers to the archived field as signature_url in the signed→re-offer acceptance criterion. Actual model field is contract_signature_url. Also ticket says "old signed_ip, old signed_by" — actual fields are contract_signed_ip and contract_signed_by. Update ticket AC to use exact model field names so the implementing agent doesn't have to guess-and-grep.

Dependencies — all real, all open

  • #420 (Alice dedupe) — board #925, backlog, open. Correctly cited as blocking Alice's offer (otherwise token lands on ambiguous row).
  • #422 (Create 16U Local Queens team) — board #927, backlog, open. Correctly cited as blocking Jacelyn's tier-change target.
  • #424 (Marcus batch execution) — board #930, backlog, open. Downstream consumer; #425 unblocks it.
  • pal-e-deployments #104 — cited as the larger ops_audit_log precursor. Acknowledged in ticket scope ("narrower slice"). Good.
Note: #420 and #422 must land before #425's implementation can be fully smoke-tested on prod, but #425 itself has no code-level dependency on them — the endpoint can be built and unit-tested independently. Recommend: clarify in ticket that #420/#422 are prod-smoke-test blockers, not PR-merge blockers.

Decomposition assessment (5-minute rule)

  • 4 file targets (1 modify, 3 new) in 1 repo — borderline.
  • 14+ acceptance criteria — exceeds the >5 heuristic, but they are tightly cohesive (request/response shape + four state transitions + security + integrity all describe one endpoint's behavior).
  • 9 unit tests + 1 integration test — substantial.
  • Realistic agent time: 15-25 minutes.
Recommendation: do NOT decompose. The signed→re-offer case is the novel piece and must ship atomically with the rest to avoid half-implemented state machines. Splitting by state transition would create partial endpoints that each leave the system in an inconsistent state. Splitting migration from endpoint is possible but creates a dangling table. The ticket correctly acknowledges its size, and the cohesion justifies the exception.

Signed → re-offer handling review

Well-specified. Validates:
  • Requires target_team_id different from current (rejects otherwise with 422).
  • Archives old signed fields to contract_audit_log with full JSONB snapshot before clearing.
  • Clears player row's signed fields (prevents stale signature display).
  • Mints new token.
  • Updates player_teams junction (remove old, add new).
  • All in a single transaction.
One gap: unclear whether the OLD contract_token is preserved in the audit snapshot. Recommend adding "old contract_token" to the archived JSONB snapshot list — important for tracing back which signed URL was used if a parent disputes the tier change.

Transaction boundaries review

Ticket specifies "single DB transaction — either the whole offer lands or none of it does" and "Transaction boundaries must be explicit (with db.begin_nested() or equivalent)." Correct. One implementation nit: begin_nested() creates a SAVEPOINT, not a top-level transaction — the outer session handling (FastAPI's get_db pattern) should commit/rollback the outer transaction. Implementing agent should verify this matches the existing admin-endpoint pattern (check how /admin/generate-tokens handles commit).

Acceptance criteria ambiguity check

All criteria are verifiable. Two minor items:
  • force=true is documented as a query param for the offered→offered case but not listed in the request shape JSON. Clarify whether force is query-string or body field (query is fine, just say so once).
  • Response field previous_signed_state_archived: false — for none→offered case, this is false. For signed→offered, true. For offered→offered with force, unclear (probably false since nothing signed exists to archive). Clarify.

Blast radius

No other basketball-api endpoint mints contract tokens. The only code that sets contract_status='offered' today is the (uncommitted) ad-hoc SQL path — this ticket is the permanent replacement. No sibling services affected. Downstream: westside-contracts consumes the contract_token URL; ticket does not modify that contract. Good.

Required refinements before promoting to todo

  • Fix field names in the signed→re-offer AC: use contract_signature_url, contract_signed_ip, contract_signed_by (exact model field names).
  • Add contract_token to the archived JSONB snapshot so old signed URLs remain traceable.
  • Clarify force location (query param vs body) and behavior of previous_signed_state_archived response field across all four transitions.
  • Clarify dependency semantics: #420 and #422 are prod-smoke-test blockers, not PR-merge blockers. Endpoint code + unit tests can land independently.
  • Add secondary story label story:WS-S23 (custom contract terms per player) alongside the existing story:WS-S7.

Deferred / separate scope

  • [SCOPE] Create arch-basketball-api note — pre-existing gap, not unique to this ticket. File as a separate backlog item to clean up all basketball-api tickets' traceability triangles at once.

Verdict

NEEDS_REFINEMENT — 5 small fixable issues, no decomposition, no BLOCK-level problems. After the 5 refinements land in the issue body, this is ready to move backlog → todo.