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 29 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.
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
|
@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.
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
|
@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.
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
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. Completes CS-656 by reassigning org-scoped references, deleting the old member, and keeping the old user; user-level relations move and sessions clear only when the old user is single-org.New Features
org:{organizationId}tags and clearer logs; recomputes old-user org-membership inside the transaction to avoid races.migrate-org-email-domainlowercases emails/domains, no-ops if normalized domains match, pairs old→new by local-part among active members, triggers merges, and returns merged/failed counts with pair details.Bug Fixes
OffboardingAccessRevocation.revokedByIdwhen the old user has no other orgs; skips it for multi-org users.newEmaildiffers fromoldEmailcase-insensitively to prevent invalid merges.Written for commit 842eb0a. Summary will update on new commits.