Node Accounting App with db#503
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the CRUD functionality for categories and setting up the PostgreSQL database. However, some changes are needed before this can be approved.
The most critical issue is that the new category feature isn't fully integrated with expenses. Currently, an expense can be created or updated with a category that doesn't exist in your database, which can lead to data integrity issues. To fix this, please use the categoryService within your create and update functions in src/controllers/expenseController.js to verify that the provided category exists before proceeding. If it doesn't, you should return a 400 error.
Additionally, please correct the error messages in src/controllers/categoryController.js. In several places (like getOne, update, and remove), the API incorrectly returns 'User not found' instead of 'Category not found'.
Once these validations and corrections are in place, 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
| /* eslint-disable no-console */ | ||
|
|
||
| const expenseService = require('../services/expenseService.js'); | ||
| const userService = require('../services/userService.js'); |
There was a problem hiding this comment.
To validate the category's existence when creating or updating an expense, you'll need to import the categoryService here.
| .json({ message: `User with id=${userId} not found` }); | ||
| } | ||
|
|
||
| const newExpense = await expenseService.create({ |
There was a problem hiding this comment.
Just as you verify the user's existence, you should also ensure the provided category exists in the database before creating an expense. You can use the categoryService for this. If the category doesn't exist, you should return a 400 error.
| return res.status(400).send({ error: 'Invalid amount format' }); | ||
| } | ||
|
|
||
| const updatedExpense = await expenseService.update({ |
There was a problem hiding this comment.
If a new category is provided in the update request, you should validate its existence in the database here, similar to what's needed in the create function. This prevents updating an expense with a non-existent category.
| const category = await categoryService.getById(id); | ||
|
|
||
| if (!category) { | ||
| return res.status(404).json({ message: 'User not found' }); |
There was a problem hiding this comment.
The error message here is for a 'User', but this controller is for categories. It should probably say 'Category not found' to be accurate.
| const category = await categoryService.getById(id); | ||
|
|
||
| if (!category) { | ||
| return res.status(404).json({ message: 'User not found' }); |
There was a problem hiding this comment.
This error message should refer to a 'Category' not being found, not a 'User'.
| const category = await categoryService.update({ id, name }); | ||
|
|
||
| if (!category) { | ||
| return res.status(404).json({ message: 'User not found' }); |
There was a problem hiding this comment.
The error message here should also be updated to refer to a 'Category' instead of a 'User'.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the CRUD functionality for categories and adding the necessary validation. However, this requires changes because the new category validation logic is not covered by tests.
Please add tests to tests/expense.test.js to verify that creating or updating an expense with a non-existent category correctly returns a 400 error. This is crucial for ensuring your implementation is robust and preventing future regressions.
Additionally, I noticed a couple of areas for improvement:
- The
removefunctions incategoryService.jsanduserService.jsare inefficient. Consider refactoring them to use a singledestroyquery, similar to how it's done inexpenseService.js. - For consistency, the
getAllfunction inuserService.jsshould return plain objects instead of Sequelize instances.
Once the tests are added, this will be ready to go. 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 remove = async (req, res) => { | ||
| const { id } = req.params; |
There was a problem hiding this comment.
For consistency with getOne and update in this same file, you should convert id to a number using Number(req.params.id). While Sequelize can often handle string representations of numeric IDs, being explicit prevents potential issues and makes the code more predictable.
| const remove = async (req, res) => { | ||
| const { id } = req.params; | ||
| const category = await categoryService.getById(id); | ||
|
|
||
| if (!category) { | ||
| return res.status(404).json({ message: 'Category not found' }); | ||
| } | ||
|
|
||
| await categoryService.remove(id); | ||
|
|
||
| res.sendStatus(204); |
There was a problem hiding this comment.
This implementation for removing a category is a bit inefficient as it results in two separate database queries: one to find the category (categoryService.getById) and another within categoryService.remove to delete it.
A more efficient approach would be to have categoryService.remove perform a single delete operation and return whether the operation was successful (e.g., by checking the number of deleted rows). This is how the remove function for expenses is implemented. Consider refactoring to follow that pattern for consistency and better performance.
| const remove = async (id) => { | ||
| const category = await Category.findByPk(id); | ||
|
|
||
| if (!category) { | ||
| return false; | ||
| } | ||
|
|
||
| await category.destroy(); | ||
| }; |
There was a problem hiding this comment.
This implementation for removing a category is inefficient as it results in two separate database queries: one to find the category (findByPk) and another to delete it (destroy).
A more efficient approach, which you've already used in expenseService.remove, is to perform a single delete operation using Category.destroy({ where: { id } }). This method returns the number of affected rows, which you can then use to return a boolean indicating success (result > 0). This change would also simplify the logic in the categoryController.
| models: { User }, | ||
| } = require('../models/models'); | ||
|
|
||
| const getAll = async () => User.findAll(); |
There was a problem hiding this comment.
For consistency with the other services, this function should return an array of plain JavaScript objects instead of Sequelize model instances. You can achieve this by mapping over the result of User.findAll() and calling .toJSON() on each user instance.
| const remove = async (id) => { | ||
| const user = await User.findByPk(id); | ||
|
|
||
| if (!user) { | ||
| return false; | ||
| } | ||
|
|
||
| await user.destroy(); | ||
| }; |
There was a problem hiding this comment.
This remove function is a bit inefficient because it requires two database queries: one in the controller (getById) to check if the user exists, and another here (findByPk) to fetch the user again before deleting. A more efficient approach, like the one used in expenseService.remove, is to use a single User.destroy({ where: { id } }) query. This returns the number of affected rows, which the controller can then use to verify if the deletion was successful.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you move forward. You've done a great job refactoring the remove functions in the service layer to be more efficient!
For your next steps, please keep these points in mind as they were part of the original request:
- Most importantly, the tests for validating a non-existent category are still missing in
tests/expense.test.js. Adding these tests is crucial for ensuring your validation logic is robust. - The
removefunctions incategoryController.jsanduserController.jscan be made more efficient by using the boolean returned from the service, avoiding the extra database call. - For consistency, please remember to have
userService.getAllreturn plain objects by using.toJSON().
Excellent progress so far, keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const { id } = req.params; | ||
| const category = await categoryService.getById(id); | ||
|
|
||
| if (!category) { | ||
| return res.status(404).json({ message: 'Category not found' }); | ||
| } | ||
|
|
||
| await categoryService.remove(id); |
There was a problem hiding this comment.
This implementation makes two database calls: one to getById and another to remove. The categoryService.remove function now returns a boolean indicating whether a category was deleted. You can use this return value to check if the category existed and send a 404 response if not, thus avoiding the extra getById call. Take a look at expenseController.remove for an example of a more efficient implementation.
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if ( | ||
| userId === undefined || | ||
| !spentAt || | ||
| !title || | ||
| amount === undefined || |
There was a problem hiding this comment.
This implementation is inefficient as it makes two separate database calls (getById and then remove). You can optimize this by using the boolean value returned from categoryService.remove(id) to check if the deletion was successful, similar to how it's handled in expenseController.
|
|
||
| const create = async (req, res) => { | ||
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if ( | ||
| userId === undefined || | ||
| !spentAt || |
There was a problem hiding this comment.
Similar to the categoryController, this remove function can be made more efficient by using a single database query. Please update it to use the boolean returned by userService.remove(id) to check if the user was found and deleted.
| /* eslint-disable no-console */ | ||
|
|
||
| const expenseService = require('../services/expenseService.js'); | ||
| const userService = require('../services/userService.js'); |
There was a problem hiding this comment.
This function still returns Sequelize model instances. For consistency with the other services, please convert the results to plain JavaScript objects before returning them. You can do this by mapping over the results and calling .toJSON() on each instance.
2 participants
No description provided.