CS-656 Create script to Migrate all users from old domain to new domain - remove duplicate people records#3304
Open
github-actions[bot] wants to merge 10 commits into
Open
CS-656 Create script to Migrate all users from old domain to new domain - remove duplicate people records#3304github-actions[bot] wants to merge 10 commits into
github-actions[bot] wants to merge 10 commits into
Conversation
…ail domain migration
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
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
Contributor
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
Contributor
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
Contributor
|
@cubic-dev-ai please review it |
Contributor
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
3 issues found across 2 files
Confidence score: 2/5
- In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, deletingoldUserbefore re-pointing user-level relations can cascade/null fields likeAuditLog.userId, which risks losing historical attribution after a duplicate merge — migrate those relations to the surviving user first, then delete. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, SOA/ISMSapproverIdvalues are not migrated before old-member deletion, so pending/approved document approver assignments can be cleared as a side effect — add an explicit approver reassignment step before the delete. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts,OffboardingAccessRevocation.revokedByIdis matched using member IDs instead of user IDs, so the actor link can be silently missed and revocation history becomes inaccurate — switch the comparison/update logic to user IDs before merging.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Contributor
|
@cubic-dev-ai please review this. |
Contributor
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 2/5
- In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts, the org-scoped duplicate merge can still perform a global delete ofoldUser, which is risky if that user still belongs to other orgs; merging as-is could remove valid cross-org access/data unexpectedly — gate deletion on zero remaining memberships (or merge all memberships first) before deleting. - In
apps/api/src/trigger/tasks/people/merge-duplicate-user.ts,memberIdrewrites may hit unique constraints when both duplicates already map to the same logical row, so the task can fail mid-merge and leave partial updates — add per-table dedupe/skip handling ahead of updates to avoid collisions.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Contributor
There was a problem hiding this comment.
3 issues found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Contributor
There was a problem hiding this comment.
2 issues found across 1 file (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.ts">
<violation number="1" location="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts:355">
P1: Merge completion log labels the deleted old user and member as the surviving IDs, contradicting the actual survivors and the return payload.</violation>
<violation number="2" location="apps/api/src/trigger/tasks/people/merge-duplicate-user.ts:355">
P1: Global User record is deleted in an org-scoped migration without verifying the user has no other memberships or un-migrated relations.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
|
||
| // ── Delete old user sessions and the user record ───────────────────── | ||
| await tx.session.deleteMany({ where: { userId: oldUser.id } }); | ||
| await tx.user.delete({ where: { id: oldUser.id } }); |
Contributor
There was a problem hiding this comment.
P1: Merge completion log labels the deleted old user and member as the surviving IDs, contradicting the actual survivors and the return payload.
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.ts, line 355:
<comment>Merge completion log labels the deleted old user and member as the surviving IDs, contradicting the actual survivors and the return payload.</comment>
<file context>
@@ -350,8 +350,9 @@ export const mergeDuplicateUser = schemaTask({
- // ── Delete old user sessions. ─────────────────────
+ // ── Delete old user sessions and the user record ─────────────────────
await tx.session.deleteMany({ where: { userId: oldUser.id } });
+ await tx.user.delete({ where: { id: oldUser.id } });
// ── Update pending invitations ───────────────────────────────────────
</file context>
|
|
||
| // ── Delete old user sessions and the user record ───────────────────── | ||
| await tx.session.deleteMany({ where: { userId: oldUser.id } }); | ||
| await tx.user.delete({ where: { id: oldUser.id } }); |
Contributor
There was a problem hiding this comment.
P1: Global User record is deleted in an org-scoped migration without verifying the user has no other memberships or un-migrated relations.
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.ts, line 355:
<comment>Global User record is deleted in an org-scoped migration without verifying the user has no other memberships or un-migrated relations.</comment>
<file context>
@@ -350,8 +350,9 @@ export const mergeDuplicateUser = schemaTask({
- // ── Delete old user sessions. ─────────────────────
+ // ── Delete old user sessions and the user record ─────────────────────
await tx.session.deleteMany({ where: { userId: oldUser.id } });
+ await tx.user.delete({ where: { id: oldUser.id } });
// ── Update pending invitations ───────────────────────────────────────
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. Completes CS-656 by consolidating all references and removing the old user to prevent conflicts.org:{organizationId}tagging and improved logging.signedBy), policy versions, tasks/task items, risks, vendors, findings, comments, audit logs, devices, trust access, offboarding, and SOA/ISMS approvals.McpOrgBinding, clears sessions, updates invitations, dedupes training/background checks/offboarding items, and deletes the old user record.migrate-org-email-domainscans active members, lowercases emails/domains, matches local-part fromoldDomain→newDomain, triggers merges per pair, and returns merged/failed counts; no-op if normalized domains match.Written for commit fc47152. Summary will update on new commits.