Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've built a solid foundation for the application with a good project structure and error handling. However, several critical features required by the task are missing or incomplete, so I am requesting changes.
Here are the main areas to address:
- Missing Core Features: The password reset flow and the user profile management functionalities (changing name, password, and email) are completely unimplemented. Please add the necessary routes, controllers, and service logic for these features.
- Incomplete Registration: The user registration process is missing the required
namefield. This needs to be added to the database schema, theuserService, and theauthController. - Authentication Logic Flaws:
- The login controller must check if a user is active before issuing tokens.
- The logout function should remove the refresh token from the database to properly invalidate the session.
- The
authMiddlewareneeds to attach the decoded user data to thereqobject so protected routes can access it.
- Missing 404 Handler: A catch-all middleware for handling 404 errors for undefined routes needs to be added to
src/app.jsas specified in the requirements.
Addressing these points will bring the project in line with the task requirements. Keep up the great work!
✨ 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.use('/auth', authRouter); | ||
| app.use('/users', userRouter); | ||
| app.use(errorMiddleware); |
There was a problem hiding this comment.
The task requirements state that a 404 page should be displayed for all pages not explicitly defined. You're missing a middleware to handle these cases. It should be placed after all your route definitions and before your main errorMiddleware to catch any requests that don't match a defined route.
| @@ -0,0 +1,29 @@ | |||
| -- CreateTable | |||
| CREATE TABLE "users" ( | |||
There was a problem hiding this comment.
The users table is missing the name column. The requirements specify that a user should register with a name, email, and password, and also be able to change their name on their profile page. You'll need to add a name column to this table to meet these requirements.
src/controllers/auth.controller.js
Outdated
| } | ||
|
|
||
| async function register(req, res) { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
The task requires users to register with a name, email, and password. The name field is missing from the registration process.
|
|
||
| await userService.activate(user.id); | ||
|
|
||
| res.status(200).json({ message: 'Account successfully activated' }); |
There was a problem hiding this comment.
The requirement is to redirect the user to their profile after activation. For an API, this typically means you should automatically log the user in by generating and returning access and refresh tokens in the response. The client can then use these tokens to redirect the user to the profile page.
| if (!user) { | ||
| throw ApiError.BadRequest('User with this email does not exist'); | ||
| } | ||
|
|
There was a problem hiding this comment.
According to the requirements, if a user is not active when they try to log in, you should ask them to activate their email. You need to add a check here to see if the user has an activationToken. If they do, it means their account is not active, and you should return an error prompting them to check their email.
src/services/email.service.js
Outdated
| }); | ||
| } | ||
|
|
||
| export const emailService = { send, sendActivationLink }; |
There was a problem hiding this comment.
This service is missing functions for other required email interactions. The task description specifies a password reset flow and an email change notification, both of which require sending emails. You should add functions here to handle sending those emails, similar to how sendActivationLink is implemented.
|
|
||
| export const userRouter = Router(); | ||
|
|
||
| userRouter.get('/', authMiddleware, userController.getAll); |
There was a problem hiding this comment.
This router is missing the endpoints required for the profile page functionality. According to the task description, you need to add routes that allow an authenticated user to:
- Change their name
- Change their password
- Change their email
These routes should be protected by the authMiddleware.
src/services/user.service.js
Outdated
| }); | ||
| } | ||
|
|
||
| async function create(email, password) { |
There was a problem hiding this comment.
The registration requirement specifies that a user should sign up with a name, email, and password. This function is missing the name parameter and doesn't save it to the database.
| }); | ||
| } | ||
|
|
||
| function normalize(user) { |
There was a problem hiding this comment.
The normalized user object, which is sent to the client and used in JWTs, should also include the user's name as per the registration requirements.
| export const userService = { | ||
| getByEmail, | ||
| getByActivationToken, | ||
| create, | ||
| activate, | ||
| normalize, | ||
| getAllActive, | ||
| }; |
There was a problem hiding this comment.
This service is incomplete. You need to add several functions to meet the task requirements, including methods for:
- Handling the password reset flow (generating a token, finding a user by that token, and updating the password).
- Managing the user profile (updating name, password, and email).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on implementing the core features like registration, profile management, and the password reset flow. The overall structure of the application is very solid. However, a few changes are needed before this can be approved.
There are two critical logic issues in src/controllers/auth.controller.js:
- Logout: The
logoutfunction clears the client-side cookie but fails to remove therefreshTokenfrom the database. This means the session isn't fully invalidated on the server. Please use the token service to delete the token. - Password Reset: The
resetPasswordfunction is missing a required validation. It needs to check that the new password and a confirmation field from the request body are identical before updating the password.
Additionally, the build is failing due to an ESLint configuration issue. As the error log suggests, renaming your .eslintrc.js file to .eslintrc.cjs will resolve the problem.
Once these issues are addressed, your solution 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/controllers/auth.controller.js
Outdated
| return res.status(204).send(); | ||
| } | ||
|
|
||
| await tokenService.getByToken(refreshToken); |
There was a problem hiding this comment.
The logout function should invalidate the session on the server by removing the refresh token from the database. Currently, you are fetching the token with getByToken but not deleting it. Consider using a service method like removeByToken to ensure the token cannot be reused.
src/controllers/auth.controller.js
Outdated
| const user = await userService.getByEmail(email); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.BadRequest('User with this email does not exsist'); |
There was a problem hiding this comment.
There's a small typo here. It should be "exist".
src/controllers/auth.controller.js
Outdated
| } | ||
|
|
||
| async function resetPassword(req, res) { | ||
| const { resetToken, newPassword } = req.body; |
There was a problem hiding this comment.
The task requires a password confirmation check for the password reset flow. Similar to the changePassword functionality, this function should accept newPassword and a confirmation field from the request body and validate that they are equal before proceeding to reset the password.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Fantastic work on this project! You've successfully implemented all the required features, from authentication to profile management, and the code is clean and well-structured.
There is one small issue causing the build to fail: the ESLint configuration file needs to be updated for your project's module type. To fix this, simply rename .eslintrc.js to .eslintrc.cjs. This tells Node.js to treat it as a CommonJS file, resolving the error.
I am approving your solution as an educational override so you can proceed. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.