Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on structuring the application and implementing the core service logic. However, there are a few critical issues with input validation that need to be addressed before this can be approved.
Specifically, there are two main blockers:
- In
src/controllers/expenses.controller.js, the validation for creating an expense using!amountwill incorrectly fail for a valid amount of0. This check should be more specific, for instance, by verifying that the value is notundefined. - In
src/controllers/users.controller.js, theupdateUserByIdfunction does not validate thatreq.body.nameis present, which is a required field according to the API specification. Please add this check and return a 400 error if it's missing.
Additionally, please remove the extra PATCH /users/:id route from src/routers/users.route.js, as the specification only requires a PUT method for updates.
Once these validation issues are resolved, 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
| const createNewExpense = (req, res) => { | ||
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if (!userId || !spentAt || !title || !amount || !category || !note) { |
There was a problem hiding this comment.
This check for required fields can be problematic. A value of 0 for amount or an empty string '' for note would be considered falsy and cause this validation to fail, even though they can be valid inputs. Consider checking specifically for undefined or null to verify presence.
| return res.status(400).json({ error: 'All fields are required' }); | ||
| } | ||
|
|
||
| const user = getUser(userId); |
There was a problem hiding this comment.
Before using the destructured properties, you should also validate their types according to the API documentation. For instance, userId and amount should be numbers, and spentAt should be a valid date format string.
| const user = getUser(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).json({ error: 'User not found' }); |
There was a problem hiding this comment.
The technical requirements state that a 404 status should be returned if an expected entity doesn't exist. In this case, the user is an expected entity, so this should return a 404 status code.
| return res.status(400).json({ error: 'ID should be a number' }); | ||
| } | ||
|
|
||
| const expense = updateExpenseService(idNumber, req.body); |
There was a problem hiding this comment.
The properties in req.body should be validated before being passed to the service. For example, if the body contains { "amount": "abc" }, it will be saved, which could cause issues later. Ensure that any provided fields have the correct types. Also, if userId is being updated, you should check that the new userId corresponds to an existing user.
| const updateUserById = (req, res) => { | ||
| const idNumber = Number(req.params.id); | ||
|
|
||
| if (isNaN(idNumber)) { | ||
| return res.status(400).json({ | ||
| error: 'ID should be a number', | ||
| }); | ||
| } | ||
|
|
||
| const user = updateUserService(idNumber, req.body); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).json({ | ||
| error: `user with ID ${idNumber} not found`, | ||
| }); | ||
| } | ||
|
|
||
| res.send(user); | ||
| }; |
There was a problem hiding this comment.
This function is missing validation for the request body. According to the Swagger documentation, the name property is required for an update. You should add a check to ensure req.body.name exists and return a 400 status if it's missing, similar to what you've done in the createUser function.
| router.get('/:id', getUserById); | ||
| router.post('/', createUser); | ||
| router.put('/:id', updateUserById); | ||
| router.patch('/:id', updateUserById); |
There was a problem hiding this comment.
According to the Swagger documentation, the update endpoint for the /users collection should only respond to the PUT HTTP method. This PATCH route is not part of the specification and should be removed to ensure the implementation matches the requirements exactly.
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 validation feedback from the last review! The controller and service logic looks solid. However, changes are needed in your router files to align with the API specification. In src/routers/expenses.route.js, the endpoint for updating an expense must use the PUT method instead of PATCH. Additionally, in src/routers/users.route.js, the extra PATCH route for updating a user must be removed, as the specification only requires a PUT endpoint for this action. You're very close to finishing this; please make these adjustments.
✨ 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
| router.get('/', getExpenses); | ||
| router.get('/:id', getExpenseById); | ||
| router.post('/', createNewExpense); | ||
| router.patch('/:id', updateExpenseById); |
There was a problem hiding this comment.
According to the Swagger documentation, the endpoint for updating an expense should use the PUT HTTP method, not PATCH. Please change the method to put to match the API specification.
| router.get('/:id', getUserById); | ||
| router.post('/', createUser); | ||
| router.put('/:id', updateUserById); | ||
| router.patch('/:id', updateUserById); |
There was a problem hiding this comment.
As mentioned in the previous review, the API specification only requires a PUT method for updating users. This PATCH route is a duplicate and should be removed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on the controller and service logic! You've correctly implemented the core functionality for the application. I am approving this submission as an educational override to help you progress, but there is one important correction needed to fully meet the project requirements.
The API specification in the Swagger documentation requires that update operations use only the PUT method. While you've added the PUT routes correctly, the redundant PATCH routes should be removed.
To fix this, please remove the router.patch('/:id', ...) line from both src/routers/expenses.route.js and src/routers/users.route.js. You are very close to a perfect solution—keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| router.get('/:id', getExpenseById); | ||
| router.post('/', createNewExpense); | ||
| router.put('/:id', updateExpenseById); | ||
| router.patch('/:id', updateExpenseById); |
There was a problem hiding this comment.
According to the API specification in the Swagger documentation, the endpoint for updating an expense should use the PUT method. This extra PATCH route is not required and should be removed.
| router.get('/:id', getUserById); | ||
| router.post('/', createUser); | ||
| router.put('/:id', updateUserById); | ||
| router.patch('/:id', updateUserById); |
There was a problem hiding this comment.
As per the API specification in the Swagger documentation, the update endpoint for a user should only use the PUT method. This redundant PATCH route should be removed to match the requirements.
2 participants
No description provided.