Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a strong submission with a well-structured API. However, I am requesting changes for two important issues before it can be approved.
- Critical Security Risk: Your
.gitignorefile is missing an entry for.env. This is a major security concern as it could lead to sensitive credentials being committed to your repository. Please add.envto your.gitignorefile. - Incorrect Password Reset Flow: In
src/api/auth.controller.js, after a successful password reset, you redirect the user directly to the login page. The requirement is to first show a success page that then links to the login page. Please adjust the redirect in theconfirmPasswordResetfunction to point to a success route.
Your implementation of the middleware, routing, and repository pattern is excellent. Once these two items are addressed, your project will be in great shape!
✨ 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
src/api/auth.controller.js
Outdated
|
|
||
| return res.status(200).json({ | ||
| message: 'Password reset successful', | ||
| redirectTo: '/auth/login', |
There was a problem hiding this comment.
According to the task description, after a successful password reset, the application should 'Show Success page with a link to login'. This implementation redirects the user directly to the login page. It would be better to redirect to a success page route, which can then provide a link or redirect to the login page.
| # env files | ||
| *.env | ||
| .env* | ||
| /src/generated/prisma |
There was a problem hiding this comment.
While ignoring the generated Prisma client is correct, this file is missing a crucial entry. Your project uses a .env file for secrets (as seen in src/index.js and prisma.config.ts). You must add .env to this .gitignore file to prevent accidentally committing sensitive credentials like database passwords and API keys.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this submission! I am happy to approve your solution as you've successfully addressed all the critical issues from the previous review and met all the project requirements. The code is clean, well-structured, and the application is fully functional.
As a minor suggestion for future improvement, consider the response from the getResetSuccessPage function in src/api/auth.controller.js. While providing the loginLink is correct, including redirectTo could be ambiguous for a client. Removing the redirectTo key would make the API's intent clearer, ensuring the user sees the success page as intended. This is just a best-practice tip and not a required change.
Great job overall!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return res.status(200).json({ | ||
| message: 'Password reset success page', | ||
| loginLink: '/auth/login', | ||
| redirectTo: '/auth/login', |
There was a problem hiding this comment.
The requirement is to show a success page that contains a link to the login page. You've correctly included the loginLink property. However, adding redirectTo might cause a front-end client to immediately redirect the user, preventing them from seeing the success message and the link. It's better to remove the redirectTo key to ensure the user stays on the success page.
No description provided.