Hotfix - Prod - Fix email setting (PS-433) #19
Conversation
Update email in member DB when identity is also updated (PS-433)
![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 not handled properly, as it may lead to SQL injection vulnerabilities. Ensure that the input is sanitized and that this operation is safe in the context it is used.
|
|
||
| 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 and handle any errors that might occur during disconnection. Promise.allSettled will suppress errors, which might not be desirable in this context.
|
|
||
| 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 defining a more specific type to ensure that only the necessary properties are mocked and to catch potential errors at compile time.
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
|
|
||
| memberUpdateMock.mockResolvedValue(undefined); |
There was a problem hiding this comment.
[correctness]
Resetting the mock implementation of memberUpdateMock to resolve undefined in every beforeEach might hide potential issues if the mock is expected to return a specific value. Ensure that the mock's behavior aligns with the test's expectations.
| { userId: 1, handle: 'testuser' }, | ||
| ); | ||
| expect(result).toEqual(mockUser); | ||
| expect(memberUpdateMock).toHaveBeenCalledWith({ |
There was a problem hiding this comment.
[❗❗ correctness]
The test checks if memberUpdateMock is called with a specific object. Ensure that the userId variable is defined and correctly scoped within the test to avoid potential reference errors.
| ); | ||
| }); | ||
|
|
||
| it('should log an error if members.member update fails but continue', async () => { |
There was a problem hiding this comment.
[design]
The test logs an error if members.member update fails but continues execution. Consider whether this behavior is appropriate, as it might lead to inconsistent states if the update is critical. Evaluate if the test should assert on the final state or outcome.
| const userIdString = '1'; | ||
| const oldStatus = 'U'; | ||
| const newStatus = 'A'; // Unverified to Active | ||
| const statusComment = 'Unit test comment'; |
There was a problem hiding this comment.
[❗❗ correctness]
The updateStatus method now includes a statusComment parameter. Ensure that all usages of this method, including in production code, handle this new parameter correctly to avoid unexpected behavior.
| 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 could prevent potential issues with invalid email addresses being processed.
| ); | ||
| } | ||
|
|
||
| if (emailChanged) { |
There was a problem hiding this comment.
[correctness]
The emailChanged flag is used to determine if the email update should be propagated to the members.member table. Ensure that this flag is correctly set only when the email update is successful to avoid inconsistent states.
| 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.
[💡 maintainability]
The change to use CommonUtils.getAppDomain(this.configService) improves maintainability by centralizing domain retrieval logic. Ensure that CommonUtils.getAppDomain handles all edge cases, such as missing or malformed configuration values.
https://topcoder.atlassian.net/browse/PS-433