Update email in member DB when identity is also updated (PS-433) #18
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| console.log('[Dry Run] Would truncate target identity.user_social_login'); | ||
| } else { | ||
| console.log('Truncating target identity.user_social_login before import...'); | ||
| await targetDb.$executeRaw`TRUNCATE TABLE identity.user_social_login RESTART IDENTITY CASCADE`; |
There was a problem hiding this comment.
[❗❗ security]
Using $executeRaw for SQL execution can be risky if any part of the query is constructed dynamically, as it may lead to SQL injection vulnerabilities. Ensure that the query is static or properly sanitized.
|
|
||
| console.log('user_social_login migration completed'); | ||
| } finally { | ||
| await Promise.allSettled([sourceDb.$disconnect(), targetDb.$disconnect()]); |
There was a problem hiding this comment.
[correctness]
Consider using Promise.all instead of Promise.allSettled if you want to ensure that both database connections are properly closed. Promise.allSettled will not throw an error if one of the promises is rejected, which might mask issues with disconnecting.
|
|
||
| const mockMemberPrisma: Partial<MemberPrismaService> = { | ||
| const memberUpdateMock = jest.fn(); | ||
| const mockMemberPrisma: any = { |
There was a problem hiding this comment.
[maintainability]
Using any for mockMemberPrisma can lead to type safety issues. Consider using a more specific type or interface to ensure the mock object aligns with expected structure.
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
|
|
||
| memberUpdateMock.mockResolvedValue(undefined); |
There was a problem hiding this comment.
[correctness]
The test case should log an error if members.member update fails but continue does not verify the continuation of the process after logging the error. Consider adding assertions to ensure the process continues as expected.
| const userIdString = '1'; | ||
| const oldStatus = 'U'; | ||
| const newStatus = 'A'; // Unverified to Active | ||
| const statusComment = 'Unit test comment'; |
There was a problem hiding this comment.
[💡 design]
The addition of statusComment in the updateStatus method calls should be verified for its intended use. Ensure that the comment is being utilized correctly within the method logic.
| if (isNaN(userId)) { | ||
| throw new BadRequestException('Invalid user ID format.'); | ||
| } | ||
| const normalizedEmail = newEmail.toLowerCase(); |
There was a problem hiding this comment.
[correctness]
Consider validating newEmail before converting it to lowercase to ensure it is a valid email format. This can prevent unnecessary operations if the email is invalid.
|
|
||
| if (emailChanged) { | ||
| try { | ||
| await this.memberPrisma.member.update({ |
There was a problem hiding this comment.
[❗❗ maintainability]
The update operation on members.member should be part of the transaction to ensure consistency. If the transaction fails after updating the email in the email table but before updating members.member, the data could become inconsistent.
| if (emailAddress && user?.handle) { | ||
| const domain = | ||
| this.configService.get<string>('APP_DOMAIN') || 'topcoder-dev.com'; | ||
| const domain = CommonUtils.getAppDomain(this.configService); |
There was a problem hiding this comment.
[correctness]
Ensure that CommonUtils.getAppDomain(this.configService) returns a valid domain. If the configuration is incorrect, it could lead to malformed email addresses.
https://topcoder.atlassian.net/browse/PS-433