add solution#508
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This submission requires changes because the main requirement of the task, which is to implement CRUD functionality for categories, is missing. While the existing code for users and expenses provides a good foundation, the new feature has not been added.
To complete the task, you need to create the necessary components for managing categories:
- A
Categorymodel (src/models/Category.model.js) - A service for business logic (
src/services/categories.service.js) - A router and controller for handling API requests (
src/routers/categories.router.js,src/controllers/categories.controller.js) - Finally, you must register the new route in
src/createServer.js.
Once these parts are implemented, the project will meet the requirements.
✨ 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 express = require('express'); | ||
|
|
||
| const { | ||
| getAllExpenses, | ||
| getExpenseById, | ||
| deleteExpense, | ||
| createExpense, | ||
| updateExpense, | ||
| } = require('../routers/expenses.router.js'); | ||
|
|
||
| const expensesRoute = express.Router(); | ||
|
|
||
| expensesRoute.get('/', getAllExpenses); | ||
|
|
||
| expensesRoute.get('/:id', getExpenseById); | ||
|
|
||
| expensesRoute.post('/', createExpense); | ||
|
|
||
| expensesRoute.delete('/:id', deleteExpense); | ||
|
|
||
| expensesRoute.patch('/:id', updateExpense); | ||
|
|
||
| module.exports = { expensesRoute }; |
There was a problem hiding this comment.
The core task is to implement CRUD functionality for categories. This file handles expenses, but the new functionality for categories seems to be missing entirely from the project. You'll need to add a new set of files (Category.model.js, categories.service.js, categories.router.js, categories.controller.js) and register the new route in createServer.js.
| const cors = require('cors'); | ||
|
|
||
| const { expensesRoute } = require('./controllers/expenses.controller.js'); | ||
| const { usersRoute } = require('./controllers/users.controller.js'); |
There was a problem hiding this comment.
It seems you've missed importing the router for categories. To manage categories, you'll need a dedicated controller and router, similar to what you have for expenses and users.
| app.use(express.json()); | ||
|
|
||
| app.use('/expenses', expensesRoute); | ||
| app.use('/users', usersRoute); |
There was a problem hiding this comment.
The server is missing the endpoint for categories. You should mount the categories route here using app.use('/categories', categoriesRoute); after you've created and imported it.
| const { | ||
| getAll, | ||
| getById, | ||
| create, | ||
| update, | ||
| remove, | ||
| } = require('../services/expenses.service.js'); | ||
|
|
||
| const { getById: getByUserId } = require('../services/users.service.js'); | ||
|
|
||
| const getAllExpenses = async (req, res) => { | ||
| const { userId, from, to, categories } = req.query; | ||
|
|
||
| const expenses = await getAll({ | ||
| userId: userId ? Number(userId) : undefined, | ||
| from, | ||
| to, | ||
| categories, | ||
| }); | ||
|
|
||
| res.send(expenses); | ||
| }; | ||
|
|
||
| const getExpenseById = async (req, res) => { | ||
| const { id } = req.params; | ||
|
|
||
| const expense = await getById(id); | ||
|
|
||
| if (!expense) { | ||
| res.status(404).send({ message: 'Not found' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| res.send(expense); | ||
| }; | ||
|
|
||
| const createExpense = async (req, res) => { | ||
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if (userId == null || spentAt == null || title == null || amount == null) { | ||
| res.status(400).send({ message: 'Missing required field' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const user = await getByUserId(userId); | ||
|
|
||
| if (!user) { | ||
| return res.status(400).send({ message: 'User not found' }); | ||
| } | ||
|
|
||
| if (typeof amount !== 'number' || Number.isNaN(new Date(spentAt).getTime())) { | ||
| return res.status(400).send({ message: 'Invalid field' }); | ||
| } | ||
|
|
||
| const expense = await create({ | ||
| userId, | ||
| spentAt, | ||
| title, | ||
| amount, | ||
| category, | ||
| note, | ||
| }); | ||
|
|
||
| res.status(201).send(expense); | ||
| }; | ||
|
|
||
| const deleteExpense = async (req, res) => { | ||
| const { id } = req.params; | ||
|
|
||
| const expense = await remove(id); | ||
|
|
||
| if (!expense) { | ||
| res.status(404).send({ message: 'Not found' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| res.status(204).send(); | ||
| }; | ||
|
|
||
| const updateExpense = async (req, res) => { | ||
| const { id } = req.params; | ||
| const { amount, spentAt } = req.body; | ||
|
|
||
| const existingExpense = await getById(id); | ||
|
|
||
| if (!existingExpense) { | ||
| res.status(404).send({ message: 'Not found' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (amount !== undefined && typeof amount !== 'number') { | ||
| res.status(400).send({ message: 'Invalid field' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (spentAt !== undefined) { | ||
| const date = new Date(spentAt); | ||
|
|
||
| if (Number.isNaN(date.getTime())) { | ||
| res.status(400).send({ message: 'Invalid field' }); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
|
|
||
| const updatedData = { | ||
| ...existingExpense, | ||
| ...req.body, | ||
| id, | ||
| }; | ||
|
|
||
| const expense = await update(updatedData); | ||
|
|
||
| if (!expense) { | ||
| res.status(404).send({ message: 'Not found' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| res.status(200).send(expense); | ||
| }; | ||
|
|
||
| module.exports = { | ||
| getAllExpenses, | ||
| getExpenseById, | ||
| deleteExpense, | ||
| createExpense, | ||
| updateExpense, | ||
| }; |
There was a problem hiding this comment.
The core requirement for this task is to implement CRUD functionality for categories. While this file correctly handles expenses, the required functionality for categories is completely missing from the project. You need to create a new set of files for categories (model, service, router, controller) and wire them up in createServer.js.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on implementing the full CRUD functionality for categories! Your solution meets all the requirements, and the new feature is well-structured with a model, service, and router. I'm approving your submission.
I have a couple of suggestions to improve your code even further:
-
Validation Order: In
src/routers/categories.router.jsandsrc/routers/users.router.js, consider checking for missing fields (e.g.,name === undefined) before checking their type. This will provide more accurate error messages to the user, such as 'Missing required field' instead of 'Invalid field'. -
File Naming: As a convention in Express.js, files that define routes are often named
*.router.js, while the files containing the request handler logic are named*.controller.js. Swapping the content of yourcategories.controller.jsandcategories.router.jswould align your project with this common pattern.
These are just best practices to keep in mind for future projects. Great job completing the task!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const { | ||
| getAllCategories, | ||
| getCategoryById, | ||
| deleteCategory, | ||
| createCategory, | ||
| updateCategory, | ||
| } = require('../routers/categories.router.js'); |
There was a problem hiding this comment.
Great job separating concerns! Just a note on naming conventions: typically, this file, which defines the routes, would be named categories.router.js. The file containing the request handler logic (like getAllCategories) would be the controller. You currently have this logic in ../routers/categories.router.js. Swapping the file names or their contents would align your project better with common Express.js patterns and make it more intuitive for other developers.
| } | ||
|
|
||
| const updatedData = { | ||
| ...existingExpense, |
There was a problem hiding this comment.
existingExpense here is a Sequelize model instance, not a plain JavaScript object. Spreading it includes many internal properties and methods from Sequelize, which is not ideal. It's better to convert it to a plain object first using existingExpense.toJSON() if you need to merge it with other data. A cleaner approach would be to only pass the data that needs to be updated to the update service.
| if (typeof name !== 'string') { | ||
| res.status(400).send({ message: 'Invalid field' }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (name === undefined) { | ||
| res.status(400).send({ message: 'Missing required field' }); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
The order of your validation checks could be improved. When the name field is missing from the request (undefined), the first check typeof name !== 'string' evaluates to true, and the response is 'Invalid field'. A more accurate response would be 'Missing required field'. Consider swapping this block with the one below to check for presence (name === undefined) before checking the type. This same logic applies to the updateUser function as well.
| if (typeof name !== 'string') { | ||
| res.status(400).send({ message: 'Invalid field' }); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
The order of validation checks here can lead to a less specific error message. If name is missing (undefined), the typeof name !== 'string' check will trigger first, sending 'Invalid field'. It's better to check for missing fields before checking their types. Consider swapping this if block with the one below.
| if (typeof name !== 'string') { | ||
| res.status(400).send({ message: 'Invalid field' }); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Similar to the createCategory function, the validation order here should be reversed. Checking for name === undefined before checking its type will ensure a more accurate error message is returned when the field is missing.
2 participants
No description provided.