Ready to request#286
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation covers the core authentication flow (registration, login, activation, password reset) with solid JWT middleware and password validation. However, several critical features are explicitly required by the task but missing from the implementation: 1. No 404 handler - The requirements state '404 for all the other pages' but no catch-all route exists in src/index.js. Add app.use((req, res) => { res.status(404).json({ message: 'Page not found' }); }); after all other routes. 2. Email change endpoint is completely missing - The requirements explicitly state: 'To change an email you should type the password, confirm the new email and notify the old email address about the change.' There is no PATCH /email route in src/routes/auth.js that validates password, requires email confirmation, and sends notification to the old email. 3. Password change missing confirmation field - The endpoint only destructures { oldPassword, newPassword } but requirements specify 'old password, new password, and confirmation' should be accepted. 4. No logout endpoint - The task requires a logout feature to invalidate JWT tokens, but no such endpoint exists. These are not minor improvements or style preferences—they are explicit checklist requirements that must be implemented for the task to be considered complete. Please address these issues and resubmit.
✨ 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
| app.listen(3000, () => { | ||
| // eslint-disable-next-line no-console | ||
| console.log('The server is listening on port 3000'); | ||
| }); |
There was a problem hiding this comment.
Missing 404 handler. The requirements state '404 for all the other pages' but no catch-all route exists. Add app.use((req, res) => { res.status(404).json({ message: 'Page not found' }); }); after all other routes.
| name: name, | ||
| email: email, | ||
| password: hash, | ||
| activationToken: activationToken, |
There was a problem hiding this comment.
The requirements specify password change needs 'old password, new password, and confirmation' but the endpoint only destructures { oldPassword, newPassword }. Consider adding a confirmation field validation to match the requirements.
| }; | ||
|
|
||
| const token = jwt.sign(payload, process.env.JWT_SECRET, { |
There was a problem hiding this comment.
Email change endpoint is completely missing. The requirements state: 'To change an email you should type the password, confirm the new email and notify the old email address about the change.' Add a PATCH /email route that validates password, requires email confirmation, and sends notification to the old email.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Two critical issues prevent approval. First, the 404 catch-all handler in src/index.js (lines 16-18) is placed BEFORE the /api/status route, which means all requests will match the 404 handler first and return 404 instead of reaching any defined routes. This handler must be moved to AFTER all other routes. Second, the password reset endpoint in src/routes/auth.js only accepts newPassword but the requirements explicitly state the reset-password confirmation page should have password and confirmation fields that must be equal. You need to add a confirmPassword field and validate that newPassword === confirmPassword before processing the reset. These are explicit requirements that must be implemented correctly before approval.
✨ 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
| async function testConnection() { | ||
| try { | ||
| await sequelize.authenticate(); |
There was a problem hiding this comment.
The 404 catch-all handler is placed before the /api/status route, causing all requests to that endpoint to return 404 instead of the expected status response. Move this handler after all valid routes.
|
|
||
| router.patch('/reset-password/:token', async (req, res) => { | ||
| try { | ||
| const token = req.params.token; |
There was a problem hiding this comment.
The requirements explicitly state the reset-password confirmation page should have password and confirmation fields that must be equal. Currently, the endpoint only accepts newPassword without a confirmation check. You should accept both password and confirmation fields and validate that they are equal before allowing the password reset.
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 core requirements are implemented correctly: both critical issues from the previous review have been resolved — the 404 handler is now properly positioned after all routes in src/index.js, and the password reset endpoint correctly validates both newPassword and confirmPassword fields with matching validation. Your authentication flow, password validation rules, email notifications, and route protections are all functional and meet the task specifications. Great work completing all the requirements!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
Solution