Feat/User email verification #1874
Conversation
test need this options
WalkthroughThe pull request introduces several changes focused on implementing email verification functionality within the application. Key modifications include the addition of new columns in the user table for email verification, updates to notification adapters to handle email confirmation flows, and enhancements to user and project resolvers to enforce email verification checks. Additionally, new error messages and localization entries are added to improve user feedback during the email verification process. Test cases are also introduced to validate the new functionalities related to email verification. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (12)
src/adapters/notifications/NotificationAdapterInterface.ts (1)
75-78: Consider improving method signature and adding documentation.A few suggestions to enhance this interface method:
- The method name could be more concise, e.g.,
sendEmailVerificationCode- Consider extracting the params type for reusability:
export interface EmailVerificationParams { email: string; user: User; }
- Add JSDoc comments to document the method's purpose and params
Apply this diff:
+/** Parameters for email verification code flow */ +export interface EmailVerificationParams { + email: string; + user: User; +} + +/** + * Sends an email verification code to the user + * @param params The email and user details + */ - sendUserEmailConfirmationCodeFlow(params: { - email: string; - user: User; - }): Promise<void>; + sendEmailVerificationCode(params: EmailVerificationParams): Promise<void>;src/entities/user.ts (1)
199-204: Consider adding an index for email verification lookups.The implementation of email verification columns looks good with appropriate types, defaults, and security considerations (verification code not exposed via GraphQL). However, since you'll likely query users by
emailVerificationCode, consider adding an index for better performance.Add an index decorator to optimize verification code lookups:
@Column('bool', { default: false }) isEmailVerified: boolean; + @Index() @Column('varchar', { nullable: true, default: null }) emailVerificationCode?: string | null;src/utils/locales/en.json (2)
120-120: Improve user-friendliness of QR code error messageThe message "QR_CODE_DATA_URL_REQUIRED" is not user-friendly as it's identical to the key. Consider using a more descriptive message that better explains the error to end users.
- "QR_CODE_DATA_URL_REQUIRED": "QR_CODE_DATA_URL_REQUIRED", + "QR_CODE_DATA_URL_REQUIRED": "QR code image data is required",
121-124: Consider adding error messages for additional verification scenariosThe current set of email verification error messages covers the basic scenarios well. However, consider adding messages for these common scenarios to improve the user experience:
- Verification code expiration
- Too many verification attempts
- Rate limiting for code requests
"USER_EMAIL_ALREADY_VERIFIED": "User email already verified", "USER_EMAIL_CODE_NOT_FOUND": "User email verification code not found", "USER_EMAIL_CODE_NOT_MATCH": "User email verification code not match", - "EMAIL_NOT_VERIFIED": "Email not verified" + "EMAIL_NOT_VERIFIED": "Email not verified", + "USER_EMAIL_CODE_EXPIRED": "Verification code has expired. Please request a new one", + "USER_EMAIL_TOO_MANY_ATTEMPTS": "Too many incorrect attempts. Please request a new code", + "USER_EMAIL_CODE_REQUEST_LIMIT": "Please wait before requesting another verification code"src/utils/errorMessages.ts (1)
208-211: Consider making error messages more user-friendly and descriptive.The current messages are technically accurate but could be more helpful to end users. Consider these improvements:
- USER_EMAIL_ALREADY_VERIFIED: 'User email already verified', + USER_EMAIL_ALREADY_VERIFIED: 'This email address has already been verified. You can proceed to use your account.', - USER_EMAIL_CODE_NOT_FOUND: 'User email verification code not found', + USER_EMAIL_CODE_NOT_FOUND: 'The verification code has expired or is invalid. Please request a new code.', - USER_EMAIL_CODE_NOT_MATCH: 'User email verification code not match', + USER_EMAIL_CODE_NOT_MATCH: 'The verification code you entered is incorrect. Please try again or request a new code.', - EMAIL_NOT_VERIFIED: 'Email not verified', + EMAIL_NOT_VERIFIED: 'Please verify your email address to access this feature.',src/adapters/notifications/NotificationCenterAdapter.ts (1)
114-116: Consider enhancing error handling granularity.The current error handling could be improved by:
- Distinguishing between different error types (network, validation, etc.)
- Adding error context to logs
- } catch (e) { - logger.error('sendUserEmailConfirmationCodeFlow >> error', e); + } catch (e) { + const context = { + userId: user.id, + errorType: e instanceof Error ? e.name : 'Unknown', + message: e instanceof Error ? e.message : String(e) + }; + logger.error('sendUserEmailConfirmationCodeFlow >> error', context);test/testUtils.ts (1)
Line range hint
395-436: Diversify seed data for better test coverageAll users in SEED_DATA are marked as verified (
isEmailVerified: true). This limits the ability to test scenarios involving unverified users.Consider diversifying the seed data to include both verified and unverified users:
FIRST_USER: { // ... other properties ... isEmailVerified: true, }, SECOND_USER: { // ... other properties ... - isEmailVerified: true, + isEmailVerified: false, // At least one unverified user for testing },This provides better test coverage for email verification-related functionality.
src/resolvers/projectResolver.ts (2)
1082-1089: Email verification check looks good, consider some improvements.The implementation correctly validates user's email verification status before allowing project updates. However, consider:
- Moving the user query to a common service/util to avoid duplication with createProject mutation
- Creating a custom error type for email verification failures for better error handling
+// In a common service file (e.g., src/services/userService.ts) +export async function validateUserEmailVerification(userId: number): Promise<void> { + const dbUser = await findUserById(userId); + if (!dbUser || !dbUser.isEmailVerified) { + throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); + } +} // In projectResolver.ts -const dbUser = await findUserById(user.userId); -if (!dbUser || !dbUser.isEmailVerified) { - throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); -} +await validateUserEmailVerification(user.userId);
1373-1379: Identical email verification check, consolidate with updateProject.The implementation correctly validates user's email verification status before allowing project creation. However, this is identical to the check in updateProject mutation, which reinforces the need to move this logic to a shared utility function.
Apply the same refactoring suggestion as provided for the updateProject mutation to reduce code duplication.
src/resolvers/userResolver.ts (2)
355-357: Avoid redundant email validationEmail validation is performed in both
sendUserEmailConfirmationCodeFlowandsendUserConfirmationCodeFlow. Consider extracting this validation logic into a helper function to reduce code duplication.
248-271: Simplify overly verbose documentation commentsThe comments from lines 248 to 271 provide a detailed explanation of the
sendUserEmailConfirmationCodeFlowmutation. While documentation is valuable, the comments may be too verbose and could be simplified for better readability.Consider summarizing the key points and using JSDoc tags for clarity.
src/resolvers/userResolver.test.ts (1)
996-996: Use dynamic verification codes in testsHardcoding verification codes like
'12345'reduces the realism of the tests. Consider generating random verification codes to better simulate real-world scenarios and enhance test robustness.Also applies to: 1001-1002, 1041-1042, 1109-1109, 1144-1145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
config/test.env(1 hunks)migration/1731071653657-addUserEmailVerificationColumns.ts(1 hunks)src/adapters/notifications/MockNotificationAdapter.ts(1 hunks)src/adapters/notifications/NotificationAdapterInterface.ts(1 hunks)src/adapters/notifications/NotificationCenterAdapter.ts(1 hunks)src/analytics/analytics.ts(1 hunks)src/entities/user.ts(2 hunks)src/resolvers/projectResolver.ts(2 hunks)src/resolvers/userResolver.test.ts(9 hunks)src/resolvers/userResolver.ts(4 hunks)src/utils/errorMessages.ts(2 hunks)src/utils/locales/en.json(1 hunks)src/utils/locales/es.json(1 hunks)src/utils/utils.ts(1 hunks)test/graphqlQueries.ts(1 hunks)test/testUtils.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/test.env
🔇 Additional comments (11)
migration/1731071653657-addUserEmailVerificationColumns.ts (1)
1-5: LGTM! Class declaration follows TypeORM conventions.
The migration class is properly structured and follows TypeORM's naming and implementation conventions.
src/analytics/analytics.ts (1)
52-52: LGTM! Good spacing.
The added empty line improves readability by separating the new email verification related event from other notification types.
src/entities/user.ts (1)
37-37: Verify the intention to expose email verification status publicly.
The addition of isEmailVerified to publicSelectionFields means this information will be publicly accessible. While this is typically acceptable for showing verification status in UI, please confirm this aligns with your privacy requirements.
Let's check if similar verification fields are publicly exposed in other parts of the system:
✅ Verification successful
Based on the search results, I can now generate the final response:
Exposing email verification status publicly is consistent with the codebase's patterns
The addition of isEmailVerified to publicSelectionFields aligns with the codebase's approach to verification statuses. The codebase already exposes other verification-related fields publicly, as evidenced by:
- Project verification status is publicly exposed and used across multiple queries
- Donation verification status is publicly accessible and used in donation-related queries
- Social profile verification status is exposed in public APIs
This pattern indicates that verification statuses are intentionally public to support UI features and business logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other verification-related public fields
rg -l "publicSelectionFields.*verified" --type ts
Length of output: 51
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# First, let's see all publicSelectionFields usage
rg "publicSelectionFields" --type ts -A 5 -B 5
# Also search for any verification-related fields in entities
rg "isVerified|verified" --type ts -A 2 -B 2
Length of output: 103718
src/utils/locales/es.json (1)
109-113: LGTM! Consider verifying translations.
The new email verification error messages are well-structured and maintain consistency with the existing translations. The messages cover the essential error cases for email verification flows.
Let's verify consistency with English translations:
✅ Verification successful
Spanish translations are consistent with English counterparts
The verification confirms that all email verification-related keys in the Spanish locale file have matching entries in the English locale file. The translations maintain semantic equivalence while being properly adapted to Spanish language conventions:
- "USER_EMAIL_ALREADY_VERIFIED"
- "USER_EMAIL_CODE_NOT_FOUND"
- "USER_EMAIL_CODE_NOT_MATCH"
- "EMAIL_NOT_VERIFIED"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare email verification messages between English and Spanish locales
# Expected: Both files should have the same keys for email verification
# Check if the English locale file exists and contains corresponding keys
rg -A 1 "USER_EMAIL_" "src/utils/locales/en.json"
Length of output: 298
src/utils/errorMessages.ts (1)
208-211: LGTM! Error messages follow consistent patterns.
The new email verification error messages are consistently implemented in both errorMessages and translationErrorMessagesKeys objects, maintaining the existing pattern and enabling i18n support.
Also applies to: 386-389
src/adapters/notifications/NotificationCenterAdapter.ts (2)
97-117: LGTM! Clean implementation of email confirmation flow.
The implementation follows the established patterns:
- Proper TypeScript typing
- Consistent error handling
- Reuse of existing notification infrastructure
103-113: Verify verification code format and expiry.
While the implementation is functional, consider adding validation for:
- Verification code format
- Code expiration time in the payload
✅ Verification successful
Based on the search results, I can now generate the final response:
Verification code format and expiry are properly implemented.
The codebase shows:
- Verification codes are 5-digit numeric codes generated via
generateRandomNumericCode(5) - Code expiry is handled through:
- Validation checks in
userResolver.tsbefore code verification - Error handling with specific messages for expired/invalid codes
- Automatic code clearing after successful verification
- Validation checks in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for verification code format validation in the codebase
ast-grep --pattern 'emailVerificationCode = $CODE'
Length of output: 52
Script:
#!/bin/bash
# Search for verification code related patterns in the codebase
rg -A 5 "emailVerificationCode"
# Search for any validation functions related to verification codes
rg -A 5 "verif.*code" -i
# Search for any code expiry or validation patterns
ast-grep --pattern 'function $NAME($ARGS) {
$$$
verif
$$$
}'
Length of output: 25567
Script:
#!/bin/bash
# Search for code generation function
rg -A 5 "generateRandomNumericCode"
# Search for any expiry related code
rg -A 5 "expir|ttl|timeout" -i
Length of output: 135041
test/testUtils.ts (1)
94-96: LGTM: JWT payload updated correctly
The addition of isEmailVerified to the JWT payload is implemented correctly with proper null safety using optional chaining.
src/resolvers/userResolver.ts (2)
33-34: Imports are appropriate
The required modules getLoggedInUser and generateRandomNumericCode are correctly imported.
176-183: Verify if restricting profile updates for unverified emails is intended
The current logic prevents users with unverified emails from updating any profile information, including their name, location, and avatar. This might affect user experience if users cannot update their profiles before email verification.
Is this the intended behavior? If not, consider modifying the condition to restrict only email updates until verification is complete, allowing other profile fields to be updated.
src/resolvers/userResolver.test.ts (1)
36-36: Test suite initialization looks good
The new test suite for userEmailVerification is properly set up.
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| queryRunner.query(` | ||
| ALTER TABLE "user" | ||
| DROP COLUMN IF EXISTS "emailVerificationCode", | ||
| DROP COLUMN IF EXISTS "isEmailVerified"; | ||
| `); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add index cleanup to down migration.
If implementing the suggested index in the up migration, ensure it's properly cleaned up in the down migration.
Apply this diff to include index cleanup:
public async down(queryRunner: QueryRunner): Promise<void> {
queryRunner.query(`
+ DROP INDEX IF EXISTS "IDX_USER_EMAIL_VERIFICATION_CODE";
ALTER TABLE "user"
DROP COLUMN IF EXISTS "emailVerificationCode",
DROP COLUMN IF EXISTS "isEmailVerified";
`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async down(queryRunner: QueryRunner): Promise<void> { | |
| queryRunner.query(` | |
| ALTER TABLE "user" | |
| DROP COLUMN IF EXISTS "emailVerificationCode", | |
| DROP COLUMN IF EXISTS "isEmailVerified"; | |
| `); | |
| } | |
| public async down(queryRunner: QueryRunner): Promise<void> { | |
| queryRunner.query(` | |
| DROP INDEX IF EXISTS "IDX_USER_EMAIL_VERIFICATION_CODE"; | |
| ALTER TABLE "user" | |
| DROP COLUMN IF EXISTS "emailVerificationCode", | |
| DROP COLUMN IF EXISTS "isEmailVerified"; | |
| `); | |
| } |
| @@ -49,4 +49,6 @@ export enum NOTIFICATIONS_EVENT_NAMES { | |||
| SUBSCRIBE_ONBOARDING = 'Subscribe onboarding', | |||
| CREATE_ORTTO_PROFILE = 'Create Ortto profile', | |||
| SEND_EMAIL_CONFIRMATION = 'Send email confirmation', | |||
|
|
|||
| SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW = 'Send email confirmation code flow', | |||
There was a problem hiding this comment.
💡 Codebase verification
MockNotificationAdapter needs to be updated to match the interface
The MockNotificationAdapter implementation of sendUserEmailConfirmationCodeFlow doesn't match the interface definition. The interface requires both email and user parameters, but the mock implementation only accepts email.
src/adapters/notifications/MockNotificationAdapter.ts: Update the method signature tosendUserEmailConfirmationCodeFlow(params: { email: string; user: User }): Promise<void>
🔗 Analysis chain
LGTM! Well-structured enum addition.
The new notification event name follows the established naming convention and clearly describes its purpose within the email verification flow.
Let's verify the usage of this new enum value in the notification adapters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of email confirmation code flow in notification adapters
# Expected: Find implementations in MockNotificationAdapter and NotificationCenterAdapter
# Search for the new enum usage
rg -A 5 "SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW"
# Search for related method implementations
ast-grep --pattern 'sendUserEmailConfirmationCodeFlow($_) {
$$$
}'
Length of output: 816
Script:
#!/bin/bash
# Let's verify if there are any other notification adapters in the codebase
fd ".*Adapter.ts" src/adapters/notifications
# Check for any test files related to notification adapters
fd ".*Adapter.test.ts" src/adapters/notifications
# Search for any mock adapter implementations
rg -A 5 "class Mock.*Adapter"
Length of output: 1397
Script:
#!/bin/bash
# Let's check the implementation in MockNotificationAdapter for the new enum
rg -A 10 "SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW" src/adapters/notifications/MockNotificationAdapter.ts
# Also check the interface definition to ensure the method is properly defined
ast-grep --pattern 'interface NotificationAdapterInterface {
$$$
}'
Length of output: 8501
Script:
#!/bin/bash
# Let's check the implementation of sendUserEmailConfirmationCodeFlow in both adapters
rg -B 2 -A 10 "sendUserEmailConfirmationCodeFlow" src/adapters/notifications/MockNotificationAdapter.ts src/adapters/notifications/NotificationCenterAdapter.ts
Length of output: 3415
| // Check if user email is verified and it's not the first update | ||
| if (!dbUser.isEmailVerified) { | ||
| throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); | ||
| } | ||
| // Check if old email is verified and user entered new one and it's not the first update | ||
| if (dbUser.isEmailVerified && email !== dbUser.email) { | ||
| throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify email verification logic and error messages
In the updateUser mutation, the logic for email verification could be improved:
- On line 177, if the user's email is not verified, an error
'EMAIL_NOT_VERIFIED'is thrown, preventing any updates. - On line 181, if the user's email is verified but the new email is different from the current one, the same error
'EMAIL_NOT_VERIFIED'is thrown.
Using the same error message in both cases might be confusing. Consider using a different error message for the second condition, such as 'EMAIL_ALREADY_VERIFIED_CANNOT_CHANGE', to clearly indicate that an already verified email cannot be changed without re-verification.
Apply this diff to provide a more specific error message:
if (!dbUser.isEmailVerified) {
throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED));
}
if (dbUser.isEmailVerified && email !== dbUser.email) {
- throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED));
+ throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_ALREADY_VERIFIED_CANNOT_CHANGE));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if user email is verified and it's not the first update | |
| if (!dbUser.isEmailVerified) { | |
| throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); | |
| } | |
| // Check if old email is verified and user entered new one and it's not the first update | |
| if (dbUser.isEmailVerified && email !== dbUser.email) { | |
| throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); | |
| } | |
| // Check if user email is verified and it's not the first update | |
| if (!dbUser.isEmailVerified) { | |
| throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_NOT_VERIFIED)); | |
| } | |
| // Check if old email is verified and user entered new one and it's not the first update | |
| if (dbUser.isEmailVerified && email !== dbUser.email) { | |
| throw new Error(i18n.__(translationErrorMessagesKeys.EMAIL_ALREADY_VERIFIED_CANNOT_CHANGE)); | |
| } |
| location: 'location', | ||
| email: user.email, // email should not be updated because verification is required |
There was a problem hiding this comment.
Potential inconsistency in email update behavior across multiple test cases
In the updateUserTestCases, the email field is included in updateUserData with a comment indicating that the email should not be updated because verification is required. However, the tests assert that updatedUser?.email equals updateUserData.email, which suggests the email is being updated without verification. Please ensure the tests correctly verify that the email is not updated unless properly verified.
Apply this diff to correct the assertions:
- assert.equal(updatedUser?.email, updateUserData.email);
+ assert.equal(updatedUser?.email, user.email); // Email should remain unchangedAlso applies to: 655-655, 692-692, 721-721, 749-749, 780-780, 819-819
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests