done#283
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has a critical functional issue that blocks the email confirmation feature. The route /profile/confirm-email/:token is defined under userRouter with authMiddleware applied globally, but users access this endpoint from email links where they won't have an active session cookie. This means email confirmations will always fail with a 401 Unauthorized error. Move this specific route to auth.route.js and remove authMiddleware from it, or apply guestMiddleware instead. All other authentication features (registration, activation, login, password reset, profile management) are correctly implemented.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The function confirmEmailChange must be moved from user.controller.js and user.service.js to auth.controller.js and auth.service.js respectively, and the duplicate functions should be removed. This is required to fix a runtime error where an undefined function is being called during email confirmation.
✨ 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
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The confirmEmailChange function is imported here but doesn't exist in auth.controller.js. This function is currently only in user.controller.js (lines 76-89). It needs to be moved to auth.controller.js.
| return res.status(err.status || 500).json({ message: err.message }); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
The route references authController.confirmEmailChange, but this function is defined in user.controller.js, not auth.controller.js. This import mismatch will cause a runtime error when users access email confirmation links.
|
|
||
| authRouter.get( | ||
| '/confirm-email/:token', | ||
| guestMiddleware, | ||
| authController.confirmEmailChange, |
There was a problem hiding this comment.
The route calls authController.confirmEmailChange, but this function is not exported from auth.controller.js. Checking auth.controller.js (lines 1-117), the exported functions are: register, activate, login, logout, requestPasswordReset, confirmPasswordReset. The confirmEmailChange function is only in user.controller.js. Move confirmEmailChange from user.controller.js to auth.controller.js, and move confirmEmailChange from user.service.js to auth.service.js.
|
|
||
| const user = await User.create({ | ||
| name, | ||
| email, | ||
| password: hashedPassword, |
There was a problem hiding this comment.
The route references authController.confirmEmailChange, but this function is not exported from auth.controller.js. The exported functions are: register, activate, login, logout, requestPasswordReset, confirmPasswordReset. The confirmEmailChange function needs to be moved from user.controller.js to auth.controller.js to fix this broken reference.
| <p>Please activate your account by clicking the link below:</p> | ||
| <a href="${link}">${link}</a> | ||
| <p>This link will expire in 24 hours.</p> | ||
| `, |
There was a problem hiding this comment.
The route calls authController.confirmEmailChange, but this function is not exported from auth.controller.js. The exported functions in auth.controller.js are: register, activate, login, logout, requestPasswordReset, confirmPasswordReset. The confirmEmailChange function exists only in user.controller.js (lines 76-89). Move confirmEmailChange from user.controller.js to auth.controller.js and the service call from user.service.js to auth.service.js.
|
|
||
| export const confirmEmailChange = async (token) => { | ||
| const user = await User.findOne({ where: { emailChangeToken: token } }); | ||
|
|
||
| if (!user || !user.pendingEmail) { | ||
| throw Object.assign(new Error('Invalid email change token'), { | ||
| status: 400, | ||
| }); | ||
| } | ||
|
|
||
| const oldEmail = user.email; | ||
|
|
||
| user.email = user.pendingEmail; | ||
| user.pendingEmail = null; | ||
| user.emailChangeToken = null; | ||
| await user.save(); | ||
|
|
||
| try { | ||
| await emailService.sendEmailChangeNotice(oldEmail); | ||
| } catch (emailErr) { | ||
| throw new Error(emailErr.message); | ||
| } | ||
|
|
||
| return user; |
There was a problem hiding this comment.
The confirmEmailChange function is defined here in user.service.js, but it's called by a route in auth.route.js via authController.confirmEmailChange. This function doesn't exist in auth.controller.js. Move confirmEmailChange from user.service.js to auth.service.js and from user.controller.js to auth.controller.js to fix this broken reference.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is excellent and addresses all core requirements. The confirmEmailChange function has been correctly relocated from user.controller.js to auth.controller.js (lines 117-130) and from user.service.js to auth.service.js (lines 132-155), eliminating the runtime error. All authentication features work properly: registration with password validation, account activation, login/logout with inactive account checks, password reset with token expiry, and complete profile management including email change with old email notification.
There's one configuration fix needed: rename .eslintrc.js to .eslintrc.cjs since your project uses ES modules ("type": "module" in package.json) but the ESLint config uses CommonJS syntax (module.exports). This is why ESLint is failing, not your application code. Prettier ran successfully on all files, confirming your code formatting is correct.
Great work on the clean separation of concerns between controllers and services, and thorough implementation of all the required functionality!
✨ 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
3 participants
No description provided.