Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical test-isolation and moderate concurrency/backfill issues block approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a persistent per-program counter to assign unique sequential registrationProgramId values safely under concurrency.
Changes:
- Adds the counter entity, repository, relation, and module wiring.
- Uses atomic counter allocation for registrations.
- Adds migration backfill, seed updates, and integration coverage.
File summaries
| File | Summary | Review findings |
|---|---|---|
services/121-service/test/registrations/assign-registration-program-id.test.ts |
Adds counter-based assignment tests. | Critical (3 votes): Restore beforeEach database resets to isolate scenarios. |
services/121-service/src/scripts/services/seed-helper.service.ts |
Initializes counters for seeded programs. | — |
services/121-service/src/scripts/factories/registration-seed-factory.ts |
Synchronizes counters after bulk duplication. | Moderate (2 votes): Reserve ID ranges atomically to prevent collisions with API imports. |
services/121-service/src/registration/repositories/registration-scoped.repository.ts |
Allocates registration IDs atomically. | Nit (1 vote): Add an alias when creating the query builder. |
services/121-service/src/registration/registrations.module.ts |
Registers the counter entity. | — |
services/121-service/src/programs/repositories/program-registration-program-id-counter.repository.ts |
Provides counter creation. | — |
services/121-service/src/programs/programs.service.ts |
Creates counters with programs. | — |
services/121-service/src/programs/programs.module.ts |
Registers and exports counter dependencies. | — |
services/121-service/src/programs/entities/program.entity.ts |
Adds the counter relation. | — |
services/121-service/src/programs/entities/program-registration-program-id-counter.entity.ts |
Defines counter persistence. | Nit (1 vote): Remove the redundant unique index. |
services/121-service/src/migration/1788794699491-add-program-registration-program-id-counter.ts |
Creates and backfills the counter table. | Moderate (1 vote): Add coverage for existing programs and the empty-program backfill case. |
Review details
Suppressed comments (3)
services/121-service/src/migration/1788794699491-add-program-registration-program-id-counter.ts:25
- The migration backfill is not exercised by the added integration test: that test creates a program after migrations, while API resets only truncate tables and do not rerun migrations (
services/121-service/src/scripts/seed-init.ts:341-372). Add incremental-migration coverage with an existing program whose registrations have a known maximum, and assert the next imported ID isMAX + 1(plus the empty-program case), otherwise an incorrect backfill can pass CI and break existing programs after upgrade.
`INSERT INTO "121-service"."program_registration_program_id_counter" ("programId", "lastRegistrationProgramId")
SELECT p.id, COALESCE(MAX(r."registrationProgramId"), 0)
FROM "121-service"."program" p
LEFT JOIN "121-service"."registration" r ON r."programId" = p.id
GROUP BY p.id
ON CONFLICT ("programId") DO NOTHING`,
services/121-service/src/programs/entities/program-registration-program-id-counter.entity.ts:18
- The one-to-one
@JoinColumnalready makes the owningprogramIdunique; with this decorator the migration creates both theREL_... UNIQUEconstraint and a secondIDX_...unique index on the same column (lines 8 and 14 of the migration). Remove the redundant index definition and its generated migration statement to avoid duplicate index storage and write overhead.
@Index({ unique: true })
services/121-service/src/registration/repositories/registration-scoped.repository.ts:90
- This query builder is created without an alias, which violates the service repository guideline and differs from the aliased update builders elsewhere (for example,
services/121-service/src/payments/transactions/transaction.repository.ts). Create it with an alias so the update remains unambiguous if it is extended.
const updateResult = await this.counterRepository
.createQueryBuilder()
.update(ProgramRegistrationProgramIdCounterEntity)
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
elwinschmitz
left a comment
There was a problem hiding this comment.
"Naming things is hard". ;)
f3f04e7 to
020e5dc
Compare
AB#44393
Describe your changes
Okay in the end I went the more robust but more code route. This guarantees parallel processes can never be assigned the same registrationProgramId.
Checklist before requesting a code review
Portal preview-deployment
This PR does not have any preview deployments yet.