Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical HTTP status mismatch that will cause tests to fail and violates the task’s API contract for missing entities. In src/controllers/controllerExpense.js, inside the create handler, when the referenced user does not exist you currently return a 400 status; the requirements state that expected-but-missing entities must return 404, just like your user controller already does for non‑existent users. You should change that branch to:
return res.status(404).send('User not found');so it aligns with the required behavior and the tests. Once this is fixed, your overall structure (routes, controllers, services, in-memory storage, and other 400/404 handling) looks solid and in line with the rest of the requirements. After applying this change, rerun the tests to confirm everything passes; you’re very close and the rest of the implementation is in good 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
| if (!user) { | ||
| return res.status(400).send('User not found'); |
There was a problem hiding this comment.
In the create handler you return 400 when the referenced user is not found. The task requires returning 404 for expected-but-missing entities. Change this to res.status(404).send('User not found') so it matches the technical requirements and tests.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still one critical behavior that does not match the requirements and will cause tests to fail. In src/controllers/controllerExpense.js, when creating an expense and the referenced user does not exist (the getUserById(Number(userId)) check), the handler currently returns HTTP 400; per the spec, expected-but-missing entities must return HTTP 404, so this branch needs to be updated to something like:
return res.status(404).send('User not found');and the inline comment about returning 400 for tests should be removed since it is now misleading. Additionally, for consistency and to avoid type issues, you should pass Number(userId) into createExpense as well (or ensure the service always expects a number) instead of mixing numeric and string user IDs. The rest of your implementation (users controller, services, routes, and server setup) looks solid and matches the in-memory, two-collection API requirements well, so once you fix the 404 behavior and userId type consistency, your solution should fully meet the task’s expectations.
✨ 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/controllerExpense.js
Outdated
| if (!user) { | ||
| // Using 400 instead of 404 to satisfy automated tests in expense.test.js:60 | ||
| return res.status(400).send('Bad Request'); |
There was a problem hiding this comment.
When the referenced user is not found you must return HTTP 404 (not 400). The requirements and tests expect 404 for expected-but-missing entities. Replace the branch with something like:
return res.status(404).send('User not found');
Also remove the inline comment about using 400 for tests.
| const expense = expenseService.createExpense( | ||
| userId, | ||
| spentAt, | ||
| title, | ||
| amount, | ||
| category, | ||
| note, | ||
| ); |
There was a problem hiding this comment.
You convert userId to Number when checking the user (getUserById(Number(userId))) but then pass the original userId to createExpense. For consistency and to avoid type mismatches, consider passing Number(userId) into createExpense as well (or ensure the service handles string IDs).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn, even though there are still a couple of things to improve.
The overall structure is strong: routes, controllers, services, and models are cleanly separated, and createServer correctly sets up the Express app, middleware, and in-memory data reset behavior. The user controller and service logic (including 400 vs 404 handling for users) are correctly aligned with the requirements. The main behavior issue is in src/controllers/controllerExpense.js: when creating an expense and the referenced user does not exist, you currently return HTTP 400; per the requirements/tests this should be HTTP 404 (e.g. return res.status(404).send('User not found');). In addition, in src/services/serviceUsers.js and src/services/serviceExpenses.js, getAllUsers and getAllExpenses return the internal arrays directly; to avoid accidental external mutation, it would be safer to return a shallow copy like return users.slice() or return expenses.slice().
Overall, your implementation shows good understanding of Express structure and in-memory data handling; fixing the status code and the small defensive copying detail will make it even more robust and production-ready.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (!user) { | ||
| return res.status(400).send('User not found'); |
There was a problem hiding this comment.
When creating an expense the handler returns HTTP 400 if the referenced user is missing. Per the requirements/tests, expected-but-missing entities must return HTTP 404. Change this to for example: return res.status(404).send('User not found');
| } | ||
|
|
||
| const expense = expenseService.createExpense( | ||
| Number(userId), |
There was a problem hiding this comment.
Good: you pass Number(userId) into createExpense, ensuring the stored userId is numeric and addressing the previous type-consistency concern.
| let nextExpenseId = 1; | ||
|
|
||
| const getAllExpenses = (userId, categories, from, to) => { | ||
| let result = expenses; |
There was a problem hiding this comment.
Consider returning a shallow copy of the internal array instead of the direct reference to avoid accidental external mutation. For example, initialize result as let result = expenses.slice(); so callers won't be able to mutate the service's internal expenses array by accident.
| const getAllUsers = () => { | ||
| return users; |
There was a problem hiding this comment.
getAllUsers returns the internal users array directly. To avoid accidental external mutation of the service state, return a shallow copy instead (for example: return users.slice(); or return [...users];).
No description provided.