fix: Allow multiple accounts per (party, org, instrument) - unblock nightly Demo Reset#2174
fix: Allow multiple accounts per (party, org, instrument) - unblock nightly Demo Reset#2174
Conversation
…ervice billing Drops idx_account_syndicate_scope_integrity, the unique partial index on (party_id, org_party_id, instrument_code) introduced in PRD-022. The original constraint modelled a syndicate where each participant holds one position per currency in a venture, but it does not match utility billing patterns where a single customer legitimately holds separate billing accounts of the same instrument with the same supplier (e.g. PPM-ELEC-CUST001 and PPM-GAS-CUST001 under the same supplier party for Margaret Thornton in the PAYG demo). This has caused the nightly Demo Reset workflow to fail since the PAYG Energy tenant was added on 2026-03-27 (#1963). With the index removed, account uniqueness is still enforced by idx_account_account_identification, so the constraint relaxation does not allow accidental duplication; it only allows the legitimate multi-service-per-supplier pattern. Updates the org-scoped migration test to assert the new behaviour (multiple GBP accounts allowed per party+org, account_identification uniqueness still enforced).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a migration that drops the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummaryWell-executed fix for a 25-day nightly Demo Reset failure. The migration correctly drops The safety argument is sound: Second commit ( What I verified:
Risk Assessment
FindingsNo actionable findings. The change is correct, well-tested, and appropriately scoped. Bot Review NotesCodeRabbit requested tightening the duplicate-key assertion (commit 1). The author addressed this in commit 2 by adding Notes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/current-account/adapters/persistence/org_scoped_migration_test.go`:
- Around line 229-247: The test currently asserts any insert failure for dup via
assert.Error(t, err) — tighten this to assert that the error is specifically a
unique-constraint violation on account_identification: after
gormDB.Create(dup).Error, use errors.As/Type assertion to detect the Postgres
error type (e.g., *pgconn.PgError or *pq.Error) and assert the SQLSTATE 23505 or
that the ConstraintName equals "idx_account_account_identification" (or assert
the error message contains "duplicate key" and the constraint name) so the test
proves the failure is due to the account_identification uniqueness constraint
rather than some other DB error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60e5eb6b-7372-4ca0-89a1-910152b822fb
⛔ Files ignored due to path filters (1)
services/current-account/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
services/current-account/adapters/persistence/org_scoped_migration_test.goservices/current-account/migrations/20260416000001_drop_syndicate_scope_integrity_index.sql
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Per CodeRabbit review: assert.Error alone passes on any insert failure. Tighten to require the failure references account_identification so the test specifically proves uniqueness on that column survives dropping the syndicate scope integrity index.
1 participant
Summary
Drops the unique partial index
idx_account_syndicate_scope_integrityon(party_id, org_party_id, instrument_code)introduced by PRD-022. The constraint was modelled on a betting-syndicate use case (one Alice position per currency in Venture Alpha) but does not generalise to utility billing, where a single customer legitimately holds multiple billing accounts of the same instrument with the same supplier.Why now
The nightly Demo Reset workflow has failed 25 nights running since the PAYG Energy demo tenant was added on 2026-03-27 (#1963). The fixture seeder creates two GBP accounts per customer for Utilita PAYG billing - one for electricity (
PPM-ELEC-CUST00n), one for gas (PPM-GAS-CUST00n) - both with the supplier asorg_party_id. The second insert hits the unique index, the server returnsAlreadyExists, the seeder cannot find the conflicting account by external_id, and the seed bails out.The same shape applies beyond gas/electric: multi-site customers (one billing account per premise), multi-product offerings (separate accounts per service line), and aggregated views all require multiple instrument-equivalent accounts under one supplier.
Changes
20260416000001_drop_syndicate_scope_integrity_index.sqldrops the index. Includes rationale in the file header.atlas.sumupdated viaatlas migrate hash.org_scoped_migration_test.goupdated:IndexesCreatedno longer asserts the dropped index exists.ScopeIntegrityIndexDroppedsubtest documents the migration outcome.UniqueConstraintOnPartyOrgInstrumentCoderenamed toMultipleAccountsPerPartyOrgInstrumentAllowedand inverted: two GBP accounts for the same (party, org) now succeed; a duplicateaccount_identificationis still rejected.Safety
idx_account_account_identification(UNIQUE onaccount_identification) still prevents accidental duplicate accounts - the only thing that changes is the (party, org, instrument) triple is no longer constrained to a single row.idx_account_participant_syndicate,idx_account_syndicate_participants) supporting NFR-1 / NFR-2 syndicate position queries are untouched.createAccountIdempotentflow already handles the legitimate case correctly.Test plan
TestOrgScopedMigrations_CockroachDBpasses (covers index-dropped assertion + multi-service billing case).workflow_dispatch.