forgejo_admin/basketball-api

Fork 1

Add POST /admin/contract/offer endpoint (mints contract offers) #425

New issue

Closed

opened 2026-04-10 20:10:56 +00:00 by forgejo_admin · 2 comments

forgejo_admin commented

2026-04-10 20:10:56 +00:00

Owner

Type

Feature

Lineage

Standalone — discovered during 2026-04-10 Westside Ops session. Marcus asked for 6 new contract offers (Alice, Brian, Vince, Kevin, Marie, Jacelyn). The existing /admin/email/blast endpoint uses query_unsigned_contracts which only finds players already in contract_status='offered' — it does not create the offered state. There is NO committed code path that mints contract offers. The existing 20 offered players got there via un-committed means (ad-hoc SQL or scripts not in the repo). This endpoint closes that gap permanently.

Revision history: review-931-2026-04-10 returned NEEDS_REFINEMENT with 5 precision fixes: (1) canonical field names, (2) contract_token in audit snapshot, (3) force param location, (4) dependency semantics, (5) secondary story label. All applied in this revision.

Repo

forgejo_admin/basketball-api

User Story

As Ava (or any admin agent) executing a batch of contract offers from Coach Marcus
I want a single endpoint that mints a contract offer for a player given a fee and optional team move
So that tonight's Marcus batch and every future batch runs through a committed, auditable, reusable code path instead of ad-hoc SQL

Context

What the code already has (verified on basketball-api main, commit 9598c4d):

POST /admin/email/blast — generic blast endpoint
email_queries.py::query_unsigned_contracts (line 31, registered at line 157) — finds players with contract_status='offered' AND contract_token IS NOT NULL AND contract_signed_at IS NULL, returns per-recipient data with a contract URL built from westside-contracts.tail5b443a.ts.net/contract/{token}
Templates live at /home/ldraney/westside-email-templates/compiled/ (hostPath, live-editable)
Keycloak JWT auth flow documented in ~/westside-email-agent/CLAUDE.md
Latest alembic migration on main: 021 (this ticket adds 022)
Token generation precedent at routes/admin.py:94 using secrets.token_urlsafe(32) in /admin/generate-tokens

What's missing:

No endpoint to transition a player from contract_status='none' → 'offered'
No endpoint to mint a contract_token (secure random URL-safe string)
No endpoint to set monthly_fee alongside the status transition
No endpoint to handle the signed → re-offer case (tier change without destroying audit state) — Jacelyn Bronson is the first real instance of this on the platform

The endpoint this ticket creates solves the common case (none → offered) AND the special case (signed → offered tier change), with explicit behavior for each.

Why now: tonight's Marcus batch (basketball-api #424) is blocked on this. Without it, we'd have to do the work via ad-hoc SQL, which violates feedback_never_alter_prod_directly and leaves no audit trail. An endpoint is the right enterprise answer.

File Targets

Files to modify:

src/basketball_api/routes/admin.py — add POST /admin/contract/offer handler + Pydantic request/response models
src/basketball_api/services/contract_offers.py — NEW file, holds the business logic (token mint, state transition, team move, audit log)
tests/test_contract_offer.py — NEW file, tests for all state transitions and edge cases
alembic/versions/022_add_contract_audit_log.py — NEW migration (022 is the next slot; 021 is the latest on main), creates contract_audit_log table to track offer state changes (especially the signed → re-offer case) — simpler predecessor to the full ops_audit_log from ticket pal-e-deployments#104

Files NOT to touch:

src/basketball_api/services/email.py — unchanged; the existing blast flow picks up newly-offered players via query_unsigned_contracts automatically
src/basketball_api/services/email_queries.py — unchanged; query_unsigned_contracts already returns exactly what we need
src/basketball_api/models.py — no schema changes on the players table; only the new contract_audit_log table is added via migration

Acceptance Criteria

Canonical player field names (verified against models.py):

contract_status (enum: none/offered/signed/declined)
contract_token (varchar)
contract_signed_at (timestamp)
contract_signed_by (varchar)
contract_signed_ip (varchar)
contract_signature_url (varchar)
contract_version (varchar)
contract_overrides (jsonb)
monthly_fee (integer)

All references below use these exact names.

Request shape:

POST /admin/contract/offer?force=true   // force is QUERY PARAM, not body
Body:
{
  "player_id": 191,
  "monthly_fee": 200,
  "contract_version": "v2026-spring-kings-select",  // optional
  "custom_notes": "...",                             // optional
  "target_team_id": 2                                // optional, only for tier-change moves
}

force is a query parameter (?force=true), not a body field. This matches FastAPI convention for "modifier" flags that change endpoint behavior without being part of the resource payload.

Response shape:

{
  "player_id": 191,
  "contract_token": "a1b2c3...",
  "contract_url": "https://westside-contracts.tail5b443a.ts.net/contract/a1b2c3...",
  "previous_status": "none",
  "new_status": "offered",
  "moved_teams": false,
  "previous_signed_state_archived": false
}

previous_signed_state_archived is present on ALL responses:

false for none→offered, declined→offered, offered→offered (force) — no archive happened
true ONLY for signed→offered — confirms the archive row was written to contract_audit_log

State transitions the endpoint must handle:

none → offered: mint contract_token via secrets.token_urlsafe(32), set contract_status='offered', set monthly_fee. Response: previous_signed_state_archived=false, moved_teams=false. (Alice, Brian, Vince, Kevin, Marie cases.)
declined → offered: same as none → offered. Declined is not a terminal state. Response: previous_signed_state_archived=false.
offered → offered (re-offer): refuse with HTTP 409 unless ?force=true query param is passed. With force, mints a NEW token (old token is replaced), updates fee, logs event. Response: previous_signed_state_archived=false.
signed → offered (tier change, Jacelyn case): REQUIRES target_team_id to be different from the player's current team (reject with 422 otherwise). Archives the following fields to contract_audit_log (JSONB snapshot): contract_signed_at, contract_signed_by, contract_signed_ip, contract_signature_url, contract_version, contract_token, old team_id(s), old monthly_fee. THEN clears contract_signed_at, contract_signed_by, contract_signed_ip, contract_signature_url on the player row, mints new contract_token, flips contract_status='offered', updates player_teams (remove old team, add new team), sets new monthly_fee. Response: previous_signed_state_archived=true, moved_teams=true.
New migration 022 creates contract_audit_log table with columns: id SERIAL PK, ts TIMESTAMPTZ DEFAULT now(), player_id INT NOT NULL FK players(id), event_type TEXT NOT NULL (values: 're_offer', 'tier_change'), old_state JSONB NOT NULL (snapshot of archived fields), new_state JSONB NOT NULL (snapshot of incoming offer data), actor TEXT NOT NULL DEFAULT 'admin', source TEXT NOT NULL DEFAULT 'api'

The archived JSONB snapshot in old_state MUST include:

contract_token (critical for tracing disputed signed URLs later)
contract_signed_at
contract_signed_by
contract_signed_ip
contract_signature_url
contract_version
team_ids (list of int, from player_teams)
monthly_fee

Security / auth:

Endpoint requires admin JWT (same Keycloak auth as other /admin/* endpoints — require_admin dep)
Rejects requests for players in other tenants (tenant_id scoping)
Returns 404 if player_id doesn't exist
Returns 422 on invalid fee (non-positive integer)
Returns 422 on invalid state transition: (a) signed without target_team_id, (b) signed with same target_team_id as current, (c) offered without force=true

Database integrity:

Token generation uses secrets.token_urlsafe(32) — same pattern as routes/admin.py:94
All state changes happen in a single DB transaction — either the whole offer lands or none of it does
Audit log INSERT is part of the same transaction as the player UPDATE (no orphan audit rows, no orphan state changes)
If the transaction rolls back (e.g., FK violation on player_teams move), the player row is unchanged
Note: use a real DB transaction, not begin_nested() (which is a SAVEPOINT, not a top-level transaction)

Behavior the endpoint explicitly DOES NOT do:

Does not send any email. The caller must separately hit /admin/email/blast with query=unsigned_contracts to send the actual email.
Does not touch jersey_option, jersey_order_status, or any jersey field.
Does not create new parents, players, or teams — all three must already exist.
Does not validate the monthly_fee against the team's contract_config.monthly_fee_default — custom fees are intentional (Marcus sets per-player).

Test Expectations

Unit test: test_offer_from_none — happy path, none → offered, verifies token format, status, fee, no audit row, previous_signed_state_archived=false
Unit test: test_offer_from_declined — declined → offered works
Unit test: test_offer_from_offered_rejects — HTTP 409 without force=true
Unit test: test_offer_from_offered_with_force — allowed with ?force=true, mints NEW token (old one is replaced)
Unit test: test_signed_to_offered_requires_team_change — HTTP 422 when target_team_id missing or same as current
Unit test: test_signed_to_offered_archives_state — Jacelyn case: old signed fields archived to contract_audit_log.old_state JSONB (including contract_token), player row's contract_signed_at/contract_signed_by/contract_signed_ip/contract_signature_url cleared, team moved, new token, new fee
Unit test: test_transaction_rollback_on_failure — simulate FK violation on player_teams, verify player row is unchanged AND no audit row was written
Unit test: test_cross_tenant_rejected — HTTP 404 for player in different tenant
Integration test: test_offer_then_blast_flow — mint via new endpoint, then call /admin/email/blast with query=unsigned_contracts, verify the player shows up in the recipient list
Run command: cd ~/basketball-api && pytest tests/test_contract_offer.py -v

Constraints

Match existing admin endpoint conventions in routes/admin.py — Pydantic models, require_admin dep, db: Session = Depends(get_db), logger usage
Token generation MUST use secrets.token_urlsafe(32) — no other random source
The contract_audit_log table is a simpler precursor to the ops_audit_log from ticket pal-e-deployments#104 — keep the schema narrow, it's not trying to be a general ops audit system
Do NOT modify the unsigned_contracts query — it already returns exactly the right data
Do NOT add a "send email" flag to this endpoint — that's the caller's job via /admin/email/blast
Transaction boundaries must be explicit (top-level db.begin() / with block, NOT begin_nested() which is a SAVEPOINT)
Logger output must include player_id, previous_status, new_status, actor for every call
The endpoint returns the contract_url for operator convenience (so the caller can share it directly if email fails)
Use canonical field names from models.py throughout — no shorthand

Checklist

PR opened against basketball-api main
Unit + integration tests pass
Migration 022 applied cleanly on local dev
Manual test against local dev: mint one offer, verify DB state, then call blast and verify recipient
Reviewed by Ava before merge
CI green on PR
After merge: ArgoCD sync, pod restart, smoke test on prod with a single test player
No unrelated changes
No code changes to email.py, email_queries.py, or models.py (player table)

Dependency Semantics (revised)

PR-merge blockers (none):

This endpoint can land independently. No other ticket blocks the PR from merging.

Prod-smoke-test blockers (two, but only for specific batch runs):

basketball-api #420 (Alice Uwamahoro dedupe) — must land before minting Alice's offer so the token goes to the canonical row
basketball-api #422 (Create 16U Local Queens team) — must land before minting Jacelyn's re-offer so her target_team_id resolves

Neither blocks this endpoint's PR itself. They block specific invocations of the endpoint during Marcus's batch (#424).

westside-basketball — project this affects
Unblocks: basketball-api #424 (Marcus 2026-04-10 batch execution)
Soft-depends on (smoke-test, not PR): basketball-api #420, basketball-api #422
Related: pal-e-deployments #104 (full write-capability + audit-log system — this ticket is a narrower slice focused only on contract offers)
Related: sop-email-send in pal-e-docs — documents the blast endpoint usage pattern
Related: ~/westside-email-agent/CLAUDE.md — documents the Keycloak JWT auth flow
Traceability: story:WS-S23 (primary — custom contract terms per player), story:WS-S7 (secondary — enables branded email blasts to new contracts)

### Type Feature ### Lineage Standalone — discovered during 2026-04-10 Westside Ops session. Marcus asked for 6 new contract offers (Alice, Brian, Vince, Kevin, Marie, Jacelyn). The existing `/admin/email/blast` endpoint uses `query_unsigned_contracts` which only **finds** players already in `contract_status='offered'` — it does not **create** the offered state. There is NO committed code path that mints contract offers. The existing 20 offered players got there via un-committed means (ad-hoc SQL or scripts not in the repo). This endpoint closes that gap permanently. **Revision history**: review-931-2026-04-10 returned NEEDS_REFINEMENT with 5 precision fixes: (1) canonical field names, (2) contract_token in audit snapshot, (3) force param location, (4) dependency semantics, (5) secondary story label. All applied in this revision. ### Repo `forgejo_admin/basketball-api` ### User Story As Ava (or any admin agent) executing a batch of contract offers from Coach Marcus I want a single endpoint that mints a contract offer for a player given a fee and optional team move So that tonight's Marcus batch and every future batch runs through a committed, auditable, reusable code path instead of ad-hoc SQL ### Context **What the code already has** (verified on basketball-api main, commit 9598c4d): - `POST /admin/email/blast` — generic blast endpoint - `email_queries.py::query_unsigned_contracts` (line 31, registered at line 157) — finds players with `contract_status='offered' AND contract_token IS NOT NULL AND contract_signed_at IS NULL`, returns per-recipient data with a contract URL built from `westside-contracts.tail5b443a.ts.net/contract/{token}` - Templates live at `/home/ldraney/westside-email-templates/compiled/` (hostPath, live-editable) - Keycloak JWT auth flow documented in `~/westside-email-agent/CLAUDE.md` - Latest alembic migration on main: 021 (this ticket adds 022) - Token generation precedent at `routes/admin.py:94` using `secrets.token_urlsafe(32)` in `/admin/generate-tokens` **What's missing**: - No endpoint to transition a player from `contract_status='none' → 'offered'` - No endpoint to mint a `contract_token` (secure random URL-safe string) - No endpoint to set `monthly_fee` alongside the status transition - No endpoint to handle the **signed → re-offer** case (tier change without destroying audit state) — Jacelyn Bronson is the first real instance of this on the platform **The endpoint this ticket creates** solves the common case (`none → offered`) AND the special case (`signed → offered` tier change), with explicit behavior for each. **Why now**: tonight's Marcus batch (basketball-api #424) is blocked on this. Without it, we'd have to do the work via ad-hoc SQL, which violates `feedback_never_alter_prod_directly` and leaves no audit trail. An endpoint is the right enterprise answer. ### File Targets Files to modify: - `src/basketball_api/routes/admin.py` — add `POST /admin/contract/offer` handler + Pydantic request/response models - `src/basketball_api/services/contract_offers.py` — NEW file, holds the business logic (token mint, state transition, team move, audit log) - `tests/test_contract_offer.py` — NEW file, tests for all state transitions and edge cases - `alembic/versions/022_add_contract_audit_log.py` — NEW migration (022 is the next slot; 021 is the latest on main), creates `contract_audit_log` table to track offer state changes (especially the signed → re-offer case) — simpler predecessor to the full `ops_audit_log` from ticket pal-e-deployments#104 Files NOT to touch: - `src/basketball_api/services/email.py` — unchanged; the existing blast flow picks up newly-offered players via `query_unsigned_contracts` automatically - `src/basketball_api/services/email_queries.py` — unchanged; `query_unsigned_contracts` already returns exactly what we need - `src/basketball_api/models.py` — no schema changes on the `players` table; only the new `contract_audit_log` table is added via migration ### Acceptance Criteria **Canonical player field names** (verified against `models.py`): - `contract_status` (enum: none/offered/signed/declined) - `contract_token` (varchar) - `contract_signed_at` (timestamp) - `contract_signed_by` (varchar) - `contract_signed_ip` (varchar) - `contract_signature_url` (varchar) - `contract_version` (varchar) - `contract_overrides` (jsonb) - `monthly_fee` (integer) All references below use these exact names. **Request shape**: ``` POST /admin/contract/offer?force=true // force is QUERY PARAM, not body Body: { "player_id": 191, "monthly_fee": 200, "contract_version": "v2026-spring-kings-select", // optional "custom_notes": "...", // optional "target_team_id": 2 // optional, only for tier-change moves } ``` `force` is a query parameter (`?force=true`), not a body field. This matches FastAPI convention for "modifier" flags that change endpoint behavior without being part of the resource payload. **Response shape**: ``` { "player_id": 191, "contract_token": "a1b2c3...", "contract_url": "https://westside-contracts.tail5b443a.ts.net/contract/a1b2c3...", "previous_status": "none", "new_status": "offered", "moved_teams": false, "previous_signed_state_archived": false } ``` `previous_signed_state_archived` is present on ALL responses: - `false` for none→offered, declined→offered, offered→offered (force) — no archive happened - `true` ONLY for signed→offered — confirms the archive row was written to `contract_audit_log` **State transitions the endpoint must handle**: - [ ] **`none → offered`**: mint `contract_token` via `secrets.token_urlsafe(32)`, set `contract_status='offered'`, set `monthly_fee`. Response: `previous_signed_state_archived=false`, `moved_teams=false`. (Alice, Brian, Vince, Kevin, Marie cases.) - [ ] **`declined → offered`**: same as `none → offered`. Declined is not a terminal state. Response: `previous_signed_state_archived=false`. - [ ] **`offered → offered` (re-offer)**: refuse with HTTP 409 unless `?force=true` query param is passed. With force, mints a NEW token (old token is replaced), updates fee, logs event. Response: `previous_signed_state_archived=false`. - [ ] **`signed → offered` (tier change, Jacelyn case)**: REQUIRES `target_team_id` to be different from the player's current team (reject with 422 otherwise). Archives the following fields to `contract_audit_log` (JSONB snapshot): `contract_signed_at`, `contract_signed_by`, `contract_signed_ip`, `contract_signature_url`, `contract_version`, `contract_token`, old `team_id(s)`, old `monthly_fee`. THEN clears `contract_signed_at`, `contract_signed_by`, `contract_signed_ip`, `contract_signature_url` on the player row, mints new `contract_token`, flips `contract_status='offered'`, updates `player_teams` (remove old team, add new team), sets new `monthly_fee`. Response: `previous_signed_state_archived=true`, `moved_teams=true`. - [ ] **New migration 022 creates `contract_audit_log` table** with columns: `id SERIAL PK`, `ts TIMESTAMPTZ DEFAULT now()`, `player_id INT NOT NULL FK players(id)`, `event_type TEXT NOT NULL` (values: `'re_offer'`, `'tier_change'`), `old_state JSONB NOT NULL` (snapshot of archived fields), `new_state JSONB NOT NULL` (snapshot of incoming offer data), `actor TEXT NOT NULL DEFAULT 'admin'`, `source TEXT NOT NULL DEFAULT 'api'` **The archived JSONB snapshot in `old_state` MUST include**: - `contract_token` (critical for tracing disputed signed URLs later) - `contract_signed_at` - `contract_signed_by` - `contract_signed_ip` - `contract_signature_url` - `contract_version` - `team_ids` (list of int, from `player_teams`) - `monthly_fee` **Security / auth**: - [ ] Endpoint requires admin JWT (same Keycloak auth as other `/admin/*` endpoints — `require_admin` dep) - [ ] Rejects requests for players in other tenants (tenant_id scoping) - [ ] Returns 404 if player_id doesn't exist - [ ] Returns 422 on invalid fee (non-positive integer) - [ ] Returns 422 on invalid state transition: (a) signed without `target_team_id`, (b) signed with same `target_team_id` as current, (c) offered without `force=true` **Database integrity**: - [ ] Token generation uses `secrets.token_urlsafe(32)` — same pattern as `routes/admin.py:94` - [ ] All state changes happen in a single DB transaction — either the whole offer lands or none of it does - [ ] Audit log INSERT is part of the same transaction as the player UPDATE (no orphan audit rows, no orphan state changes) - [ ] If the transaction rolls back (e.g., FK violation on player_teams move), the player row is unchanged - [ ] Note: use a real DB transaction, not `begin_nested()` (which is a SAVEPOINT, not a top-level transaction) **Behavior the endpoint explicitly DOES NOT do**: - Does not send any email. The caller must separately hit `/admin/email/blast` with `query=unsigned_contracts` to send the actual email. - Does not touch `jersey_option`, `jersey_order_status`, or any jersey field. - Does not create new parents, players, or teams — all three must already exist. - Does not validate the `monthly_fee` against the team's `contract_config.monthly_fee_default` — custom fees are intentional (Marcus sets per-player). ### Test Expectations - [ ] Unit test: `test_offer_from_none` — happy path, `none → offered`, verifies token format, status, fee, no audit row, `previous_signed_state_archived=false` - [ ] Unit test: `test_offer_from_declined` — `declined → offered` works - [ ] Unit test: `test_offer_from_offered_rejects` — HTTP 409 without `force=true` - [ ] Unit test: `test_offer_from_offered_with_force` — allowed with `?force=true`, mints NEW token (old one is replaced) - [ ] Unit test: `test_signed_to_offered_requires_team_change` — HTTP 422 when `target_team_id` missing or same as current - [ ] Unit test: `test_signed_to_offered_archives_state` — Jacelyn case: old signed fields archived to `contract_audit_log.old_state` JSONB (including `contract_token`), player row's `contract_signed_at/contract_signed_by/contract_signed_ip/contract_signature_url` cleared, team moved, new token, new fee - [ ] Unit test: `test_transaction_rollback_on_failure` — simulate FK violation on `player_teams`, verify player row is unchanged AND no audit row was written - [ ] Unit test: `test_cross_tenant_rejected` — HTTP 404 for player in different tenant - [ ] Integration test: `test_offer_then_blast_flow` — mint via new endpoint, then call `/admin/email/blast` with `query=unsigned_contracts`, verify the player shows up in the recipient list - Run command: `cd ~/basketball-api && pytest tests/test_contract_offer.py -v` ### Constraints - Match existing admin endpoint conventions in `routes/admin.py` — Pydantic models, `require_admin` dep, `db: Session = Depends(get_db)`, logger usage - Token generation MUST use `secrets.token_urlsafe(32)` — no other random source - The `contract_audit_log` table is a simpler precursor to the `ops_audit_log` from ticket pal-e-deployments#104 — keep the schema narrow, it's not trying to be a general ops audit system - Do NOT modify the `unsigned_contracts` query — it already returns exactly the right data - Do NOT add a "send email" flag to this endpoint — that's the caller's job via `/admin/email/blast` - Transaction boundaries must be explicit (top-level `db.begin()` / `with` block, NOT `begin_nested()` which is a SAVEPOINT) - Logger output must include `player_id`, `previous_status`, `new_status`, `actor` for every call - The endpoint returns the `contract_url` for operator convenience (so the caller can share it directly if email fails) - Use canonical field names from `models.py` throughout — no shorthand ### Checklist - [ ] PR opened against basketball-api main - [ ] Unit + integration tests pass - [ ] Migration 022 applied cleanly on local dev - [ ] Manual test against local dev: mint one offer, verify DB state, then call blast and verify recipient - [ ] Reviewed by Ava before merge - [ ] CI green on PR - [ ] After merge: ArgoCD sync, pod restart, smoke test on prod with a single test player - [ ] No unrelated changes - [ ] No code changes to `email.py`, `email_queries.py`, or `models.py` (player table) ### Dependency Semantics (revised) **PR-merge blockers** (none): - This endpoint can land independently. No other ticket blocks the PR from merging. **Prod-smoke-test blockers** (two, but only for specific batch runs): - basketball-api #420 (Alice Uwamahoro dedupe) — must land before minting Alice's offer so the token goes to the canonical row - basketball-api #422 (Create 16U Local Queens team) — must land before minting Jacelyn's re-offer so her `target_team_id` resolves Neither blocks this endpoint's PR itself. They block specific invocations of the endpoint during Marcus's batch (#424). ### Related - `westside-basketball` — project this affects - Unblocks: basketball-api #424 (Marcus 2026-04-10 batch execution) - Soft-depends on (smoke-test, not PR): basketball-api #420, basketball-api #422 - Related: pal-e-deployments #104 (full write-capability + audit-log system — this ticket is a narrower slice focused only on contract offers) - Related: `sop-email-send` in pal-e-docs — documents the blast endpoint usage pattern - Related: `~/westside-email-agent/CLAUDE.md` — documents the Keycloak JWT auth flow - Traceability: `story:WS-S23` (primary — custom contract terms per player), `story:WS-S7` (secondary — enables branded email blasts to new contracts)

forgejo_admin commented

2026-04-10 20:14:31 +00:00

Author

Owner

Scope Review: NEEDS_REFINEMENT (minor)

Review note: review-931-2026-04-10

All 11 template sections present. All file targets verified against main @ 9598c4d (routes/admin.py, services/email.py, services/email_queries.py, models.py all exist; model fields confirmed on lines 254-267). Dependencies #420/#422/#424 all open. No decomposition needed — the signed->re-offer atomicity justifies keeping the ticket cohesive despite being larger than typical.

Five small refinements before promoting backlog -> todo:

Fix field-name drift in the signed->re-offer AC. Use the exact model field names: contract_signature_url (not signature_url), contract_signed_ip (not signed_ip), contract_signed_by (not signed_by).
Add contract_token to the archived JSONB snapshot in the signed->re-offer case — important for tracing back which signed URL was used if a parent disputes the tier change.
Clarify force location: the request shape JSON doesn't show force, but the offered->offered AC calls it a query param. State once (query-string is fine) and drop into the example. Also clarify previous_signed_state_archived response value across all four transitions (true only for signed->offered; false otherwise).
Clarify dependency semantics: #420 and #422 are prod-smoke-test blockers, not PR-merge blockers. The endpoint code + unit tests can land independently of those two tickets.
Add secondary story label story:WS-S23 (custom contract terms per player) alongside story:WS-S7. WS-S7 is the branded-email umbrella and defensible, but WS-S23 is the more literal fit for a custom-fee contract-offer flow.

Deferred (not blocking this ticket)

[SCOPE] No arch-basketball-api backing note exists in pal-e-docs. Pre-existing gap across many basketball-api tickets — file as a separate backlog item to clean up traceability triangles in one pass.

Implementation nit (for the dev agent, not a refinement)

with db.begin_nested() creates a SAVEPOINT, not a top-level transaction. The outer FastAPI get_db session still owns commit/rollback. Verify the pattern matches how /admin/generate-tokens handles commit in the existing admin.py.

After the 5 refinements land in the issue body, this ticket is ready to move backlog -> todo.

## Scope Review: NEEDS_REFINEMENT (minor) Review note: `review-931-2026-04-10` All 11 template sections present. All file targets verified against main @ 9598c4d (routes/admin.py, services/email.py, services/email_queries.py, models.py all exist; model fields confirmed on lines 254-267). Dependencies #420/#422/#424 all open. No decomposition needed — the signed->re-offer atomicity justifies keeping the ticket cohesive despite being larger than typical. Five small refinements before promoting backlog -> todo: 1. **Fix field-name drift** in the signed->re-offer AC. Use the exact model field names: `contract_signature_url` (not `signature_url`), `contract_signed_ip` (not `signed_ip`), `contract_signed_by` (not `signed_by`). 2. **Add `contract_token` to the archived JSONB snapshot** in the signed->re-offer case — important for tracing back which signed URL was used if a parent disputes the tier change. 3. **Clarify `force` location**: the request shape JSON doesn't show `force`, but the offered->offered AC calls it a query param. State once (query-string is fine) and drop into the example. Also clarify `previous_signed_state_archived` response value across all four transitions (true only for signed->offered; false otherwise). 4. **Clarify dependency semantics**: #420 and #422 are prod-smoke-test blockers, not PR-merge blockers. The endpoint code + unit tests can land independently of those two tickets. 5. **Add secondary story label `story:WS-S23`** (custom contract terms per player) alongside `story:WS-S7`. WS-S7 is the branded-email umbrella and defensible, but WS-S23 is the more literal fit for a custom-fee contract-offer flow. ### Deferred (not blocking this ticket) - [SCOPE] No `arch-basketball-api` backing note exists in pal-e-docs. Pre-existing gap across many basketball-api tickets — file as a separate backlog item to clean up traceability triangles in one pass. ### Implementation nit (for the dev agent, not a refinement) `with db.begin_nested()` creates a SAVEPOINT, not a top-level transaction. The outer FastAPI `get_db` session still owns commit/rollback. Verify the pattern matches how `/admin/generate-tokens` handles commit in the existing admin.py. After the 5 refinements land in the issue body, this ticket is ready to move backlog -> todo.

forgejo_admin commented

2026-04-10 20:18:17 +00:00

Author

Owner

Scope Review: APPROVED (second pass)

Review note: review-931-2026-04-10-v2

All 5 refinements from review-931-2026-04-10 verified applied:

Canonical field names used throughout (no shorthand)
contract_token present in archived JSONB old_state snapshot
force explicit as query parameter (?force=true)
Dependency Semantics section correctly distinguishes PR-merge vs prod-smoke-test blockers (#420/#422 are smoke-test-only)
story:WS-S23 added as primary story label; story:WS-S7 retained as secondary

Bonus: transaction boundary nit addressed — explicit db.begin() with note that begin_nested() is a SAVEPOINT, not a top-level transaction.

Ticket is ready to move backlog → todo.

## Scope Review: APPROVED (second pass) Review note: `review-931-2026-04-10-v2` All 5 refinements from `review-931-2026-04-10` verified applied: - Canonical field names used throughout (no shorthand) - `contract_token` present in archived JSONB `old_state` snapshot - `force` explicit as query parameter (`?force=true`) - Dependency Semantics section correctly distinguishes PR-merge vs prod-smoke-test blockers (#420/#422 are smoke-test-only) - `story:WS-S23` added as primary story label; `story:WS-S7` retained as secondary Bonus: transaction boundary nit addressed — explicit `db.begin()` with note that `begin_nested()` is a SAVEPOINT, not a top-level transaction. Ticket is ready to move backlog → todo.</body> </invoke>