Add Users and Expenses API#884
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall structure of the application! The separation into services, controllers, and routes is well-executed. However, a couple of adjustments are needed to fully meet the project's requirements.
Here are the key points to address:
- In
src/controllers/users.controller.js, thePATCHendpoint for updating a user needs validation. According to the Swagger documentation, thenameproperty is required, but the current implementation doesn't check for its presence in the request body. Please add a validation to ensurereq.body.nameis provided. - In
src/controllers/expenses.controller.js, when creating an expense for auserIdthat does not exist, the API should return a404status code, not400. This is because the user is a required entity that was not found.
Once these validations and status codes are corrected, your solution will be fully aligned with the specifications. You're very close!
✨ 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
| if (!data || !Object.keys(data).length) { | ||
| return res.status(400).json({ error: 'Bad request' }); | ||
| } |
There was a problem hiding this comment.
This validation correctly handles an empty request body. However, according to the Swagger documentation, the name property is required for this PATCH request. The current logic doesn't check for the presence of name within the request body, which could lead to updates with missing required data. Please add a validation to ensure req.body.name is provided.
| const user = userService.getUserById(Number(userId)); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ error: 'User not found' }); |
There was a problem hiding this comment.
The requirement specifies returning a 404 status if an expected entity doesn't exist. Since the user associated with userId is an expected entity for creating an expense, returning a 404 status code here would align more closely with the requirements than a 400.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thanks for your submission! The overall structure of your application is solid. However, a couple of key requirements from the previous review still need to be addressed before this can be approved.
Here are the two issues that need fixing:
-
User Update Validation (
src/controllers/users.controller.js): The validation for thePATCH /users/{id}endpoint needs adjustment. The requirement is to check for the presence of thenameproperty specifically (e.g.,if (!req.body.name)), not just for a non-empty body. Also, this validation must be performed before you attempt to update the user. -
Incorrect Status Code (
src/controllers/expenses.controller.js): When creating an expense (POST /expenses) for auserIdthat does not exist, the API must return a404 Not Foundstatus code. Your implementation is currently returning400 Bad Request.
You're on the right track, and these changes will bring your solution in line with the 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
| const user = userService.getUserById(Number(userId)); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ error: 'User not found' }); |
There was a problem hiding this comment.
The task requires returning a 404 status code when a referenced entity (like a user) doesn't exist. This check correctly identifies a missing user, but it should return 404 Not Found instead of 400 Bad Request.
| const updated = userService.updateUserById(id, data); | ||
|
|
||
| if (!data || !Object.keys(data).length) { | ||
| return res.status(400).json({ error: 'Bad request' }); | ||
| } | ||
|
|
||
| if (!updated) { | ||
| return res.status(404).json({ error: 'User not found' }); | ||
| } |
There was a problem hiding this comment.
This implementation still needs a couple of adjustments to meet the requirements.
-
Incorrect Validation: The check on line 47 only verifies that the request body isn't empty. Per the Swagger documentation and previous feedback, it needs to specifically validate that the
nameproperty is present in the body (req.body.name). -
Incorrect Order of Operations: The validations should happen before the update operation on line 45. The correct flow is to first validate the request body, then check if the user exists, and only then perform the update.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the feedback on the user update validation! You're very close to a perfect solution.
I am approving your submission as an educational override so you can keep moving forward, but there's one small correction left to make. When creating an expense for a userId that doesn't exist (POST /expenses), the API should return a 404 Not Found status code, but it's currently returning 400.
You can fix this in src/controllers/expenses.controller.js by finding the check for the non-existent user and changing the status code from res.status(400) to res.status(404). Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const user = userService.getUserById(Number(userId)); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ error: 'User not found' }); |
There was a problem hiding this comment.
According to the task requirements, you should return a 404 status code when an expected entity (in this case, the user) does not exist. This endpoint is currently returning 400.
2 participants
Implemented REST API with CRUD operations, filtering, validation, and MVC structure.
All tests are passing.