fix: insert outbox event after contract signing #8
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "6-fix-contract-sign-outbox-event"
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
The
POST /contract/{token}/signendpoint updated the player record but never created an outbox event, so basketball-api's outbox worker never sent the confirmation email with GroupMe share link. This adds the missing outbox INSERT after the player UPDATE.Changes
src/routes/contract/[token]/sign/+server.ts: Expanded initial SELECT to includenameandparent_idcolumns. AddedINSERT INTO outboxwithcontract_signedevent type and JSON payload containingplayer_id,player_name,team_id,parent_id. Added fire-and-forget HTTP call to basketball-api/admin/process-outboxfor immediate processing (gracefully fails if admin auth blocks it -- the cron worker picks it up regardless).Test Plan
npm run buildpassesnpm testpasses (12 tests)SELECT * FROM outbox WHERE event_type = 'contract_signed' ORDER BY created_at DESC LIMIT 1;Review Checklist
Related
Review -- LGTM
Reviewed the diff against the outbox worker in
basketball-api/src/basketball_api/services/outbox.pyand the migration in018_add_groupme_and_outbox.py.Verified:
nameandparent_id-- matches what_handle_contract_signedreads from the payloadplayer_id,player_name,team_id,parent_id) matches the outbox worker's expected keys.catch()to prevent unhandled rejection; comment correctly notes admin auth will block it and cron is the reliable pathnpm run buildandnpm test(12 tests) both passNo fixes needed.
PR #8 Review
DOMAIN REVIEW
Stack: SvelteKit (adapter-node) + TypeScript + pg (raw SQL) + MinIO S3 SDK. Server-side API route, no frontend changes.
Outbox pattern correctness: The INSERT payload fields (
player_id,player_name,team_id,parent_id) align with whatbasketball-api/src/basketball_api/services/outbox.pyexpects in_handle_contract_signed(). The outbox table schema (tenant_id, event_type, payload, status, created_at) matches the migration in018_add_groupme_and_outbox.py. Event typecontract_signedis a recognized enum value. The pattern is sound.Fire-and-forget design: The
.catch()pattern on the fetch is appropriate -- the comment correctly documents that the cron worker is the reliability mechanism and this is just an optimization. Good.BLOCKERS
1. No transaction wrapping -- UPDATE and INSERT can diverge
src/routes/contract/[token]/sign/+server.tslines 74-98The
UPDATE players(line 74) andINSERT INTO outbox(line 94) are separatepool.query()calls without an explicit transaction.pg.Poolauto-commits each query individually. If the INSERT fails after the UPDATE succeeds, the contract is markedsignedbut no outbox event exists -- the confirmation email never sends and there is no recovery path. These two operations must be atomic.Fix: wrap lines 74-98 in a client-level transaction:
2. Null
parent_idwill crash the outbox workersrc/routes/contract/[token]/sign/+server.tsline 91The
Playertype definesparent_id: number | null. If a player has no parent assigned,parent_id: nullgets serialized into the outbox payload. The basketball-api outbox worker then callsdb.query(Parent).filter(Parent.id == payload["parent_id"])withNone, which will either return no result or behave unexpectedly, causing the event to be markedfailed. The sign endpoint should validate thatparent_idis not null before inserting the outbox event, or the outbox worker needs a null guard. Since this PR owns the INSERT, it should guard here.3. Hardcoded internal service URL without env var fallback
src/routes/contract/[token]/sign/+server.tsline 103The
db.tsandminio.tsmodules follow the pattern ofprocess.env.X || 'default'. This new URL is fully hardcoded with no env var override. This makes local development and testing impossible without the k8s DNS. Should follow the existing pattern:4. No test coverage for new functionality
The PR adds two new behaviors (outbox INSERT + fire-and-forget fetch) with zero test coverage. The existing 12 tests in
tests/validation.test.tsonly cover input validation. There are no integration or unit tests for the outbox event creation, payload structure, or error handling paths.At minimum, tests should cover:
NITS
1. Hardcoded
tenant_id = 1Lines 96 and 103 both hardcode
tenant_id = 1. This is likely fine for now (single-tenant system), but worth extracting to a constant at the top of the file for when multi-tenancy arrives. At minimum, a// TODO: multi-tenantcomment would signal intent.2.
player_namein payload is redundantThe outbox worker re-fetches the player from the database by
player_id(line 62 ofoutbox.py) and usesplayer.namefrom that query, not from the payload. Includingplayer_namein the payload is harmless but misleading -- it suggests the worker uses it. Consider removing it to avoid confusion, or have the worker use it directly instead of re-querying.SOP COMPLIANCE
6-fix-contract-sign-outbox-eventreferences issue #6Closes #6but no plan slug. Acceptable if this is standalone board work without a plan.PROCESS OBSERVATIONS
+server.tsduplicatingvalidation.ts) is tracked in issue #5 and is NOT introduced by this PR. Not blocking here, but should be addressed soon.VERDICT: NOT APPROVED
Four blockers found. The transaction atomicity gap (blocker 1) is the highest risk -- it creates a silent failure mode where contracts appear signed but no confirmation email sends. The null
parent_idguard (blocker 2) will crash the downstream worker. The hardcoded URL (blocker 3) breaks the env var pattern used by every other service connector in this codebase. The missing tests (blocker 4) are required per review criteria.PR #8 Re-Review (Post Fix Round)
Previous review identified 3 blockers. Commit
92207bbaddressed all 3. This is a re-review of the current state.DOMAIN REVIEW
Tech stack: SvelteKit (adapter-node) + raw
pgPool + MinIO + PostgreSQL outbox pattern.Transaction correctness: The
pool.connect()/BEGIN/COMMIT/ROLLBACK/finally { client.release() }pattern at lines 76-118 is textbook correct. The comment explaining why MinIO upload is intentionally outside the transaction is helpful and accurate -- an orphaned signature PNG is harmless, but a signed player with no outbox event is the actual bug this PR fixes.Null guard on parent_id: The
if (player.parent_id)guard at line 95 correctly prevents inserting an outbox event when parent_id is null. The else branch logs a console.warn, which is appropriate -- the basketball-api outbox worker would fail on null parent_id anyway (it doesdb.query(Parent).filter(Parent.id == payload["parent_id"])). Defensive and correct.Outbox INSERT column alignment: Verified against migration
018_add_groupme_and_outbox.py. Columns(tenant_id, event_type, payload, status, created_at)match exactly. Theidcolumn is auto-increment,processed_atis nullable with no default -- both correctly omitted from the INSERT.Outbox payload alignment: The worker (
basketball_api.services.outbox._handle_contract_signed) expectsplayer_id,parent_id, andteam_idfrom the JSON payload. The PR sends all three plusplayer_name(extra, harmless -- the worker re-queries the player record).Environment variable handling:
BASKETBALL_API_URLreads fromprocess.envwith fallback tohttp://basketball-api.basketball-api.svc.cluster.local:8000. This matches the established pattern insrc/lib/db.tswhere all config uses env vars with in-cluster defaults. The env var is not inkustomize/overlays/prod/secrets.yamlbut the fallback is correct for the cluster topology. Acceptable.Fire-and-forget fetch: The
.catch()on the fetch at line 130 prevents unhandled promise rejections. The admin endpoint requires auth (confirmed:require_admindependency in basketball-api), so this call will likely 401 -- but the comment correctly notes the cron worker handles it regardless. This is a delivery optimization, not a correctness requirement.BLOCKERS
None. All 3 previous blockers are resolved:
if (player.parent_id)before outbox INSERT. Warning logged on skip.process.env.BASKETBALL_API_URLwith cluster-local fallback.NITS
Hardcoded tenant_id=1 (line 104): The outbox INSERT uses
VALUES (1, ...)fortenant_id. This is currently correct (single-tenant deployment), but if multi-tenancy ever happens, this becomes a bug. Low risk given the current architecture -- just noting it.Extra payload field:
player_nameis included in the outbox JSON payload but the basketball-api worker does not use it (it re-queries the player). Not harmful, but slightly inconsistent. Could be useful for debugging outbox rows directly in Postgres, so arguably a feature.CI pipeline missing
npm test: The.woodpecker.ymlonly runsnpm run build, notnpm test. The 12 validation tests are not executed in CI. This is a pre-existing issue (tracked in issue #3/#5), not introduced by this PR, so not blocking here.SOP COMPLIANCE
6-fix-contract-sign-outbox-eventreferences issue #6Closes #6.env*files, kustomize secrets use REPLACE_ME placeholders)PROCESS OBSERVATIONS
BASKETBALL_API_URLenv var should be added tokustomize/overlays/prod/secrets.yamlfor explicitness, even though the fallback works. This is a nit, not a blocker.VERDICT: APPROVED