Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've set up a good application structure, but there are critical changes needed to meet the task requirements. The submission implements a /users collection, but the requirement is for a /category collection. This needs to be corrected throughout the code in the router, controller, and service files. Furthermore, the API contract is not being followed: the expense endpoint should be /expense (singular), not /expenses, and the update method must be PUT, not PATCH. The data model for creating and updating expenses is also incorrect; it should accept name, amount, and categoryId. Lastly, please add validation to the update functions to ensure all required fields are present, returning a 400 error if they are not.
✨ 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 { Router } = require('express'); | ||
| const { usersController } = require('../controllers/users.controller'); | ||
|
|
||
| const usersRouter = Router(); |
There was a problem hiding this comment.
The task requires implementing a /category collection, not a /users collection. This router, and its corresponding controller and service, should be for category management as per the requirements.
| usersRouter.post('/', usersController.create); | ||
| usersRouter.get('/:id', usersController.getOne); | ||
| usersRouter.delete('/:id', usersController.deleteOne); | ||
| usersRouter.patch('/:id', usersController.update); |
There was a problem hiding this comment.
According to the task requirements and the provided Swagger documentation, the endpoint for updating a resource should use the PUT HTTP method, not PATCH.
|
|
||
| const update = async (req, res) => { | ||
| const { name } = req.body; | ||
| const user = await usersService.getById(+req.params.id); |
There was a problem hiding this comment.
The requirements for updating a resource state that the name is a required parameter. You need to add a check here to ensure name exists in the request body. If it's missing, you should return a 400 status code, similar to how it's handled in the create function.
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if (!userId || !spentAt || !title || !amount || !category || !note) { | ||
| return res.sendStatus(400); | ||
| } |
There was a problem hiding this comment.
The request body handling here doesn't match the project requirements. The POST /expense endpoint should expect name (string), amount (number), and categoryId (integer). The current implementation expects a different set of properties, which also makes the validation logic incorrect.
| res.sendStatus(204); | ||
| }; | ||
|
|
||
| const update = async (req, res) => { |
There was a problem hiding this comment.
This function is supposed to handle a PUT request, which typically replaces an entire resource. Therefore, it needs to validate that the request body contains all required fields (name, amount, categoryId) and return a 400 error if any are missing. The current logic performs a partial update (like PATCH) without this validation.
| expensesRouter.post('/', expensesController.create); | ||
| expensesRouter.get('/:id', expensesController.getOne); | ||
| expensesRouter.delete('/:id', expensesController.deleteOne); | ||
| expensesRouter.patch('/:id', expensesController.update); |
There was a problem hiding this comment.
The task requirements specify using the PUT method to update an expense, but PATCH is used here. Please change the method to put to match the requirements.
| function create(userId, spentAt, title, amount, category, note) { | ||
| const expense = { | ||
| id: currentId + 1, | ||
| userId, | ||
| spentAt, | ||
| title, | ||
| amount, | ||
| category, | ||
| note, | ||
| }; |
There was a problem hiding this comment.
The parameters for creating an expense do not match the API specification. According to the requirements, a new expense should be created using name, amount, and categoryId. This function, however, is designed to accept a different set of properties (userId, spentAt, title, etc.). The created expense object should also reflect the required structure.
| app.use(express.json()); | ||
| app.use(cors()); | ||
|
|
||
| app.use('/users', usersRouter); |
There was a problem hiding this comment.
The task requires a /category endpoint, but the server is configured to use a /users endpoint here. You need to create and use a router for categories instead.
| app.use(cors()); | ||
|
|
||
| app.use('/users', usersRouter); | ||
| app.use('/expenses', expensesRouter); |
There was a problem hiding this comment.
According to the Swagger documentation provided in the task description, this endpoint should be /expense (singular), not /expenses.
| @@ -0,0 +1,59 @@ | |||
| const users = []; | |||
There was a problem hiding this comment.
The task requires a category service, but this file implements a users service. The variable users and all related logic should be changed to manage categories as per the requirements.
3 participants
No description provided.