feat: accounting app solution#924
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a couple of functional issues that will cause your API to deviate from the required behavior and likely fail tests. In src/controllers/expenses.controller.js, in the PUT update handler for expenses (where you validate required fields near the top of the function), you currently return HTTP 422 when required fields are missing; the specification and tests expect HTTP 400 for missing required parameters, so you should change that status code to 400 (for example, using return res.sendStatus(400)). In src/services/expenses.service.js, within getAll, some of the filters (e.g., for userId and category) are applied to the original expenses array instead of the already filtered list, which means combined filters (user + date range + category) will not work correctly; update each step to filter filteredExpenses so the filters chain properly. Also in src/controllers/expenses.controller.js, the DELETE handlers respond with status 204 and a JSON body (e.g., res.status(204).json(...)), but a 204 No Content response must not include a body, so you should change this to res.sendStatus(204) or res.status(204).end(). On the non-blocking side, you are reusing express.json() middleware on individual routes even though it is already registered globally in createServer, which is redundant and can be safely removed for cleaner code. Once you address the PUT status code, fix the filter chaining logic, and adjust the 204 responses, your solution will more closely align with the requirements and your otherwise solid structure and tests integration will work as intended.
✨ 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.post('/', express.json(), expensesController.create); | ||
|
|
||
| router.delete('/:id', expensesController.deleteOne); | ||
|
|
||
| router.get('/:id', expensesController.getOne); | ||
|
|
||
| router.put('/:id', express.json(), expensesController.updatePut); | ||
|
|
||
| router.patch('/:id', express.json(), expensesController.updatePatch); |
There was a problem hiding this comment.
You register express.json() on the POST/PUT/PATCH routes, but the application already applies express.json() globally in createServer. This is redundant; you can remove the per-route body-parser middleware to avoid duplication.
|
|
||
| res.status(204).json(expense); |
There was a problem hiding this comment.
A 204 No Content response should not include a response body. Here the handler sets status 204 and then sends JSON; consider using res.sendStatus(204) or res.status(204).end() to avoid sending a body.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Two critical HIGH priority issues remain unfixed and will cause tests to fail. First, in src/services/expenses.service.js, the filter chaining is still broken - lines 9 and 13 filter the original expenses array instead of filteredExpanses, which means combined filters like userId + categories won't work correctly (the test at line 217 in expense.test.js specifically tests this). Change expenses.filter(...) to filteredExpanses.filter(...) for both the userId and categories conditions. Second, in src/controllers/users.controller.js, line 36 still uses res.status(204).json(user) which sends a JSON body with a 204 response, violating HTTP 204 No Content semantics - change this to res.sendStatus(204) to match what was correctly implemented in the expenses controller.
✨ 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 (categories) { | ||
| filteredExpanses = expenses.filter((ex) => ex.category === categories); |
There was a problem hiding this comment.
The filter for categories should use filteredExpanses.filter(...) instead of expenses.filter(...). Currently if both userId and categories filters are applied, the categories filter will override the userId filter instead of chaining correctly.
| if (userId) { | ||
| filteredExpanses = expenses.filter((ex) => ex.userId === userId); |
There was a problem hiding this comment.
The filter for userId should use filteredExpanses.filter(...) instead of expenses.filter(...). This ensures subsequent filters (categories, date range) work on the already-filtered results, not the full list.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is approved! All critical issues have been resolved — the filter chaining in expenses.service.js now correctly chains on filteredExpanses (lines 9 and 13), and the delete endpoint in users.controller.js properly uses res.sendStatus(204). All core requirements are met: 2 collections with 5 endpoints each, proper HTTP status codes (400/404), in-memory storage that clears on restart, and an Express app returned from createServer() without calling listen(). For continued improvement, consider renaming filteredExpanses to filteredExpenses (minor typo), and you may eventually want separate handlers for PUT (full replacement) vs PATCH (partial update) semantics, though your current implementation passes all tests. Great work on getting everything working!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.