CS-656 Create script to Migrate all users from old domain to new domain - remove duplicate people records#3304
CS-656 Create script to Migrate all users from old domain to new domain - remove duplicate people records#3304github-actions[bot] wants to merge 31 commits into
Conversation
…ail domain migration
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 1 file
Confidence score: 2/5
- In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, there is no guard foroldEmail === newEmail, so a self-merge can delete the very user/member being kept; merging as-is risks destructive data loss on valid-looking input — add an explicit equality short-circuit before any delete/update steps. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, the training completion migration dropsidand then later updates byid, which can make that migration fail or silently affect zero rows; this creates a concrete partial-migration/regression risk — keepidin the projection (or change downstream updates to available keys) and verify with a targeted migration test before merging. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, resolving members outside the transaction introduces a TOCTOU window where merge targets can change mid-operation, leading to inconsistent merges under concurrency — move member resolution inside the same transaction (or lock rows) before merge. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, the final merge log reports deleted IDs as survivors, which can mislead incident response and audits after a bad merge — correct survivor/deleted ID mapping in the log payload before shipping.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
chasprowebdev
changed the title
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai please review it |
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai please review this. |
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 3/5
- In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, ISMS records can retainownerMemberIdreferences to the merged-away member, which risks orphaned ownership links or broken ownership lookups after deduplication; migrateownerMemberIdfrom old member ID to new member ID in the same transaction before deleting the old member.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts">
<violation number="1" location="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts:273">
P1: This migration can rewrite revocation actor attribution outside the target org, because `revokedById` is updated globally by userId even when the old user still has memberships elsewhere. Scoping this update by `organizationId` (or moving it into the user-level branch) avoids cross-org data changes.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
…ail-migration-script
|
@cubic-dev-ai please review it. |
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 2/5
- In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, the merge path can resolveoldEmailandnewEmailto the same membership id and still executetx.member.delete({ id: o }), which risks deleting the surviving target membership and breaking account/org access after merge — add an explicit same-member guard (or schema-level inequality constraint) before allowing delete and verify with a same-account test before merging. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts,oldUserHasOtherOrgsis computed before the transaction, so concurrent membership changes can make that safety check stale and lead to incorrect merge decisions under load — recompute membership state inside the same transaction (or lock/read consistently) to de-risk race conditions before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts">
<violation number="1" location="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts:7">
P1: This task can delete the target membership when `oldEmail` and `newEmail` are the same account, because the merge still reaches `tx.member.delete({ id: o })` after resolving both members to one id. A schema-level inequality guard for the two emails would prevent this destructive path.</violation>
<violation number="2" location="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts:62">
P2: User-level merge safety is decided from a pre-transaction membership count, so concurrent org-membership writes can make `oldUserHasOtherOrgs` stale during execution. Recomputing that condition inside the same transaction (or locking rows) would keep relation migration aligned with final membership state.</violation>
<violation number="3" location="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts:273">
P1: This migration can rewrite revocation actor attribution outside the target org, because `revokedById` is updated globally by userId even when the old user still has memberships elsewhere. Scoping this update by `organizationId` (or moving it into the user-level branch) avoids cross-org data changes.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai please review it. |
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 7 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
…ail-migration-script
…of a hand-maintained list
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts">
<violation number="1" location="apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts:84">
P2: Historical checklist completions whose template was deleted can be lost during a merge: all nullable `templateItemId` values collapse to the same `"null"` dedupe key. Treat NULL template IDs as non-conflicting and migrate them rather than dropping them.</violation>
<violation number="2" location="apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts:150">
P1: Merges with the same training video completed by both members always roll back: `toDrop` remains attached to the old member and the final FK safety check rejects it before the cascade delete. Delete those duplicate rows before the assertion, as done for the other unique exceptions.</violation>
<violation number="3" location="apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts:226">
P1: ISMS role assignments and audit-finding owners still point to the deleted member because these two plain member-ID fields are not catalog-discoverable and are omitted from both repointing and the safety check. Repoint and assert `IsmsRoleAssignment.memberId` and `IsmsAuditFinding.ownerMemberId` alongside `IsmsObjective.ownerMemberId`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| ); | ||
|
|
||
| await assertNoDanglingMemberReferences(tx, foreignKeys, oldMemberId, { | ||
| 'IsmsObjective.ownerMemberId': () => |
There was a problem hiding this comment.
P1: ISMS role assignments and audit-finding owners still point to the deleted member because these two plain member-ID fields are not catalog-discoverable and are omitted from both repointing and the safety check. Repoint and assert IsmsRoleAssignment.memberId and IsmsAuditFinding.ownerMemberId alongside IsmsObjective.ownerMemberId.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts, line 226:
<comment>ISMS role assignments and audit-finding owners still point to the deleted member because these two plain member-ID fields are not catalog-discoverable and are omitted from both repointing and the safety check. Repoint and assert `IsmsRoleAssignment.memberId` and `IsmsAuditFinding.ownerMemberId` alongside `IsmsObjective.ownerMemberId`.</comment>
<file context>
@@ -0,0 +1,239 @@
+ );
+
+ await assertNoDanglingMemberReferences(tx, foreignKeys, oldMemberId, {
+ 'IsmsObjective.ownerMemberId': () =>
+ tx.ismsObjective.count({ where: { ownerMemberId: oldMemberId } }),
+ 'Policy.signedBy': () =>
</file context>
| newCompletionKeys, | ||
| (c) => c.videoId, | ||
| ); | ||
| if (completionSplit.toMigrate.length > 0) { |
There was a problem hiding this comment.
P1: Merges with the same training video completed by both members always roll back: toDrop remains attached to the old member and the final FK safety check rejects it before the cascade delete. Delete those duplicate rows before the assertion, as done for the other unique exceptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts, line 150:
<comment>Merges with the same training video completed by both members always roll back: `toDrop` remains attached to the old member and the final FK safety check rejects it before the cascade delete. Delete those duplicate rows before the assertion, as done for the other unique exceptions.</comment>
<file context>
@@ -0,0 +1,239 @@
+ newCompletionKeys,
+ (c) => c.videoId,
+ );
+ if (completionSplit.toMigrate.length > 0) {
+ await tx.employeeTrainingVideoCompletion.updateMany({
+ where: { id: { in: completionSplit.toMigrate.map((c) => c.id) } },
</file context>
| const checklistSplit = splitDuplicates( | ||
| existingChecklist, | ||
| newChecklistKeys, | ||
| (c) => String(c.templateItemId), |
There was a problem hiding this comment.
P2: Historical checklist completions whose template was deleted can be lost during a merge: all nullable templateItemId values collapse to the same "null" dedupe key. Treat NULL template IDs as non-conflicting and migrate them rather than dropping them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/trigger/tasks/people/merge-duplicate-user-member-relations.ts, line 84:
<comment>Historical checklist completions whose template was deleted can be lost during a merge: all nullable `templateItemId` values collapse to the same `"null"` dedupe key. Treat NULL template IDs as non-conflicting and migrate them rather than dropping them.</comment>
<file context>
@@ -0,0 +1,239 @@
+ const checklistSplit = splitDuplicates(
+ existingChecklist,
+ newChecklistKeys,
+ (c) => String(c.templateItemId),
+ );
+ if (checklistSplit.toMigrate.length > 0) {
</file context>
3 participants
This is an automated pull request to merge chas/email-migration-script into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Adds trigger tasks
merge-duplicate-userandmigrate-org-email-domainto merge duplicate users during org email-domain migrations. Member relations are now auto-discovered from Postgres and re-pointed safely with a final dangling-reference check; completes CS-656.New Features
org:{organizationId}tags; recomputes the old user’s other-org membership inside the transaction.Member.idforeign keys from Postgres, generic repointing, and a safety check that fails if any relation still points at the old member; explicit handling forPolicy.signedByandIsmsObjective.ownerMemberId.Bug Fixes
OffboardingAccessRevocation.revokedByIdwhen the old user has no other orgs.newEmaildiffers fromoldEmailcase-insensitively.Written for commit 4465ddb. Summary will update on new commits.