Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've built a solid foundation for the backend with a good project structure, which is a great start. However, several critical features are missing or broken, so changes are needed before this can be approved.
Here are the main issues to address:
- Missing Frontend Features: The UI for password reset, changing a password, and changing an email is missing from
src/templates/main.html. These are core requirements of the task. - Missing 404 Handler: The application must return a 404 status for any undefined routes. You need to add a catch-all middleware in
src/createServer.jsto handle this. - Incomplete Password Reset: The
resetPasswordfunction insrc/controllers/auth.controller.jsneeds to validate that thepasswordandconfirmationfields from the request body are identical. - Critical Backend Bug: In the
activatecontroller (src/controllers/auth.controller.js), theuser.save()operation must beawaited to prevent a race condition where the user isn't saved as active before the response is sent.
Please focus on fixing these blockers. Addressing them will bring your project much closer to meeting all the requirements.
✨ 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
| } | ||
|
|
||
| user.activationToken = null; | ||
| user.save(); |
There was a problem hiding this comment.
user.save() is an asynchronous operation, but it's not being awaited here. This means the response might be sent before the user's activation status is actually saved to the database. Always await promises to ensure operations complete in the correct order.
src/controllers/auth.controller.js
Outdated
| }; | ||
|
|
||
| const resetPassword = async (req, res) => { | ||
| const { token, password } = req.body; |
There was a problem hiding this comment.
The task description requires the password reset functionality to include a confirmation field that must be equal to the password. This function currently only accepts the new password. You need to destructure confirmation from req.body as well and add a check to ensure it matches password.
| const getAllActivated = async (req, res) => { | ||
| const users = await usersService.getAllActivated(); | ||
|
|
||
| res.send( | ||
| users.map(usersService.normalize) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This controller and its getAllActivated function implement a feature to fetch all activated users, which is not part of the requirements outlined in the description file. Please focus on implementing only the specified requirements to stay within the project's scope.
|
|
||
| const { client } = require('./src/utils/db.js'); | ||
|
|
||
| client.sync({ force: true }); No newline at end of file |
There was a problem hiding this comment.
Using { force: true } will drop all existing tables and recreate them every time this script is run, leading to complete data loss. While this might be useful for resetting the database during development, it's a very dangerous operation for a production environment. Be mindful of when and where this script is executed.
| res.send('Hello'); | ||
| }); | ||
|
|
||
| app.use(errorMiddleware); |
There was a problem hiding this comment.
According to the task description, the application should return a 404 for any routes that are not explicitly defined. You're missing a catch-all middleware here to handle such cases. It should be placed after all your route handlers but before this error middleware.
src/templates/main/index.html
Outdated
| const formData = new FormData(e.target); | ||
|
|
||
| try { | ||
| const response = await fetch('http://localhost:3005/register', { |
There was a problem hiding this comment.
The URL for the fetch request is hardcoded to port 3005. Based on your server configuration in src/index.js, the server is running on port 3004. This mismatch will cause the registration request to fail. Ensure your client-side API calls point to the correct server port.
src/templates/main/index.html
Outdated
| const data = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| showMessage(data.error || 'Something went wrong', 'error'); |
There was a problem hiding this comment.
The error handling logic looks for a data.error property in the JSON response. However, your backend API sends error details in data.message and data.errors. This will result in showing a generic "Something went wrong" message instead of the specific error from the server. You should adjust this to parse data.message.
src/templates/main.html
Outdated
|
|
||
| useEffect(() => { | ||
| if (token) { | ||
| axios.get(`${API}/profile`, { |
There was a problem hiding this comment.
This API call to /profile will fail because this endpoint is not defined in your backend routes. When a user with a valid token refreshes the page, this will cause them to be logged out. The user data should be retrieved and stored upon login, and this effect could be used to validate the token if a dedicated /me or /profile endpoint is created.
src/templates/main.html
Outdated
| setError(''); | ||
| setMessage(''); | ||
| try { | ||
| await axios.patch(`${API}/profile/name`, { name }, { |
There was a problem hiding this comment.
This API call is incorrect. Your backend route for updating the name is POST /update-name, but here you are calling PATCH /profile/name. You need to use the correct HTTP method and endpoint URL for this to work.
src/templates/main.html
Outdated
| <div style={styles.container}> | ||
| <h1>👤 Profile</h1> | ||
| <p><strong>Email:</strong> {user?.email}</p> | ||
| <p><strong>Name:</strong> {user?.name || 'Not set'}</p> | ||
|
|
||
| <hr /> | ||
|
|
||
| <h3>Update Name</h3> | ||
| <input | ||
| style={styles.input} | ||
| type="text" | ||
| placeholder="New name" | ||
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| /> | ||
| <button style={styles.button} onClick={handleUpdateName}>Update</button> | ||
|
|
||
| {error && <p style={styles.error}>❌ {error}</p>} | ||
| {message && <p style={styles.success}>{message}</p>} | ||
|
|
||
| <hr /> | ||
|
|
||
| <button style={styles.buttonDanger} onClick={handleLogout}>Logout</button> | ||
| </div> |
There was a problem hiding this comment.
The profile page is incomplete. According to the requirements, it should also allow the user to change their password and email address. The forms and handler functions for these features are missing.
…e in active auth_controller function
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the backend implementation! The API is well-structured, and you've addressed much of the previous feedback. However, some changes are needed because critical functionality is missing or broken.
The main issue is that all frontend files have been removed, so the application doesn't meet any of the UI requirements. Additionally, there are a couple of critical backend bugs:
- Incorrect Password Reset Link: In
src/services/email.service.js, thesendForgotEmailfunction sends an activation link instead of a password reset link, breaking the feature. - Sensitive Data Leak: The
activatefunction insrc/controllers/auth.controller.jsexposes the user's password hash. Please normalize the user object before sending it in the response, just as you do in other controllers.
Please re-implement the required UI and fix these backend issues. You're very close to a complete solution!
✨ 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
| user.activationToken = null; | ||
| await user.save(); | ||
|
|
||
| res.send(user); |
There was a problem hiding this comment.
The response sends the entire user object, which includes sensitive information like the password hash. You should normalize the user object to only include public-facing data before sending it. You already have a normalize function in usersService that you can use for this purpose.
src/middlewares/errorMiddleware.js
Outdated
| message: 'Server error' | ||
| }) | ||
|
|
||
| next(); |
There was a problem hiding this comment.
Calling next() here is not necessary and is generally not a best practice in an error-handling middleware that sends a response. Once res.send() is called, the request-response cycle is complete. While it doesn't cause an issue here because it's the last middleware, it's better to remove it to make the code's intent clearer.
src/services/email.service.js
Outdated
| }; | ||
|
|
||
| const sendActivationEmail = (email, token) => { | ||
| const href = `http://127.0.0.1:3005/activation/${token}`; |
There was a problem hiding this comment.
It's a good practice to avoid hardcoding the base URL. Consider moving http://127.0.0.1:3005 to an environment variable (e.g., process.env.API_URL). This makes your application more flexible for different deployment environments.
src/services/email.service.js
Outdated
| }; | ||
|
|
||
| const sendForgotEmail = (email, token) => { | ||
| const href = `http://localhost:5173/activation/${token}`; |
There was a problem hiding this comment.
The URL for the password reset link seems incorrect. It's pointing to /activation/${token}, which is the path for account activation. According to the requirements, this should lead to a password reset page. Please ensure the link directs the user to the correct frontend route for resetting their password.
… password reset link, breaking the feature. and add normalize the user object before sending it in the response
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your submission as an educational override so you can continue to the next stage. Your backend implementation is outstanding! You've built a secure, well-structured, and robust API, and you did a fantastic job fixing the critical issues from the last review. The code in your services, controllers, and models is clean and follows best practices.
However, please be aware that the entire frontend is missing, which is a major requirement. For your final submission, you will need to implement all the required UI pages. I also have a couple of minor suggestions for the backend:
- In
src/middlewares/errorMiddleware.js, you can remove the unusedjwtServiceimport and the unnecessarynext()call after sending a response. - In
src/services/email.service.js, it's a good practice to move hardcoded URLs to environment variables for better flexibility.
Excellent work on the backend, and I look forward to seeing the complete application!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
src/middlewares/errorMiddleware.js
Outdated
| @@ -0,0 +1,23 @@ | |||
| /* eslint-disable */ | |||
| const jwtService = require('../services/jwt.service.js') | |||
There was a problem hiding this comment.
This jwtService import isn't used within the file. It's a good practice to remove any unused imports to keep the code clean.
src/middlewares/errorMiddleware.js
Outdated
| message: 'Server error' | ||
| }) | ||
|
|
||
| next(); |
There was a problem hiding this comment.
Calling next() after sending a response with res.send() is unnecessary and can sometimes lead to issues. Once the response has been sent, the request-response cycle is considered finished, so there's no need to pass control to any subsequent middleware.
src/services/email.service.js
Outdated
| }; | ||
|
|
||
| const sendActivationEmail = (email, token) => { | ||
| const href = `http://127.0.0.1:3005/activation/${token}`; |
There was a problem hiding this comment.
It's a good practice to use environment variables for base URLs instead of hardcoding them. This makes the application more flexible across different environments (development, staging, production). Consider creating a variable like process.env.API_URL to store http://127.0.0.1:3005.
src/services/email.service.js
Outdated
| }; | ||
|
|
||
| const sendForgotEmail = (email, token) => { | ||
| const href = `http://localhost:5173/reset-password/${token}`; |
There was a problem hiding this comment.
Similarly, the client URL should be stored in an environment variable, for example, process.env.CLIENT_URL. This avoids having to change the code when deploying the application to a different domain.
…/middlewares/errorMiddleware.js and move hardcoded URLs to environment variables for better flexibility in src/services/email.service.js
No description provided.