task#281
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has two critical bugs that will cause runtime failures. First, in src/services/user.service.js, the activation email is being called with an object sendActivationEmail({ email, activationToken }) but the email service expects two separate arguments: email as the first parameter and token as the second. This will cause the registration feature to fail. Second, there's a spelling error in multiple files (src/routes/auth.route.js, src/models/user.js, src/services/email.service.js, src/routes/user.route.js) where the route is written as /password-rest instead of /password-reset, which will cause 404 errors when the frontend tries to reach the password reset endpoint. Critical fixes required: 1. Change sendActivationEmail({ email, activationToken }) to sendActivationEmail(email, activationToken) in src/services/user.service.js 2. Fix the typo /password-rest → /password-reset in all four files listed above. Minor improvements: - Add newEmailConfirmation field validation in updateEmail function (src/controllers/user.controller.js) to match the requirement that users must confirm their new email - Fix the typo in the response message: "exosts" → "exists" in src/controllers/auth.controller.js
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| } | ||
|
|
||
| const user = await userService.findById(req.userId); | ||
| const isOldValid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (!isOldValid) { |
There was a problem hiding this comment.
The activation email is being sent with an object { email, activationToken } as the first argument, but emailService.sendActivationEmail expects email as the first argument and token as the second. This will cause a runtime error and users won't receive activation emails.
| res.send(userService.normalize(user)); | ||
| }; | ||
|
|
||
| const updatePassword = async (req, res) => { | ||
| const { oldPassword, newPassword, confirmation } = req.body; | ||
|
|
||
| if (newPassword !== confirmation) { | ||
| throw ApiError.badRequest('New passwords do not match'); | ||
| } |
There was a problem hiding this comment.
The password validation only checks for minimum length (6 characters), but the requirements state that users should be 'informed about the rules for a password'. The validation errors should include comprehensive password rules that are communicated to users during registration.
| res.send(userService.normalize(user)); | ||
| }; | ||
|
|
||
| const updatePassword = async (req, res) => { | ||
| const { oldPassword, newPassword, confirmation } = req.body; | ||
|
|
||
| if (newPassword !== confirmation) { | ||
| throw ApiError.badRequest('New passwords do not match'); | ||
| } |
There was a problem hiding this comment.
The validatePassword function doesn't return a value when validation passes (it returns undefined implicitly). While this works for the current logic, it could be clearer to explicitly return null or a truthy value for consistency.
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| secure: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The updateEmail function only takes newEmail without requiring confirmation of the new email. According to the requirements, changing email should require 'confirm the new email', meaning users should type the new email twice to prevent typos.
|
|
||
| authRouter.post( | ||
| '/password-rest', | ||
| catchError(authController.requestPasswordReset), |
There was a problem hiding this comment.
There's a typo in the route path: '/password-rest' should be '/password-reset' to be consistent with the naming convention used in the confirm endpoint ('/password-reset/confirm'). This mismatch will cause the password reset request endpoint to be unreachable.
| }, | ||
| activationToken: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
The route is misspelled as '/password-rest' but should be '/password-reset' (with an 'e' before 't'). This typo will cause 404 errors when the frontend tries to request a password reset, as it would typically call '/password-reset'.
| subject, | ||
| html, | ||
| }); |
There was a problem hiding this comment.
The route path is '/password-rest' but should be '/password-reset' to match the task description and be consistent with the confirmation endpoint '/password-reset/confirm' on line 23.
| '/profile/password', | ||
| authMiddleware, | ||
| catchError(userController.updatePassword), | ||
| ); |
There was a problem hiding this comment.
The route path is /password-rest (typo: missing 'e' in 'reset') but the requirements and the confirm endpoint use /password-reset. This inconsistency could cause frontend-backend integration issues if the client expects the correct path /password-reset.
| password: hashedPassword, | ||
| activationToken, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The sendActivationEmail is called with an object { email, activationToken } but the emailService.sendActivationEmail function expects two separate arguments: email as the first parameter and token as the second. This will cause a runtime error during user registration and prevent activation emails from being sent. Should be: emailService.sendActivationEmail(email, activationToken)
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. All critical bugs from previous reviews have been successfully resolved: the sendActivationEmail function is now called with two separate arguments (email, activationToken), the /password-reset route spelling is correct throughout, newEmailConfirmation validation is properly implemented in updateEmail, and the typo in auth.controller.js has been fixed. The codebase now correctly implements registration with activation email, login/logout, password reset, and profile management with all authenticated endpoints properly protected. All checklist requirements are met, including the 404 handler for unmapped routes. Great work on addressing all the issues thoroughly!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.