Conversation
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 API! The code is well-structured and most of the endpoints are working as expected. However, changes are needed for the DELETE endpoints.
Currently, both DELETE /users/:id and DELETE /expenses/:id are missing validation for the id parameter. If a non-numeric ID is provided, the server incorrectly returns a 404 Not Found status instead of the required 400 Bad Request. Please add a validation check to these endpoints to ensure they handle invalid IDs consistently with the GET and PATCH endpoints.
✨ 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
| app.delete('/users/:id', (req, res) => { | ||
| const id = Number(req.params.id); |
There was a problem hiding this comment.
This endpoint is missing validation for the id parameter. If a non-numeric ID is provided, it will incorrectly result in a 404 Not Found instead of a 400 Bad Request. Consider adding a check using isValidId for consistency with the GET and PATCH endpoints for this resource.
| app.delete('/expenses/:id', (req, res) => { | ||
| const id = Number(req.params.id); |
There was a problem hiding this comment.
Similar to the DELETE /users/:id endpoint, the id parameter here is not validated. An invalid ID format should result in a 400 Bad Request, but it currently leads to a 404 Not Found. Please add a validation check consistent with the other endpoints that use an ID.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the validation for non-numeric IDs in the DELETE endpoints! However, this submission requires changes because the implementation for creating and updating users and expenses does not align with the project requirements.
Here are the specific issues:
- Users: When creating a user (
POST), the requiredemailfield is missing. When updating a user (PATCH),emailis not handled, andnameis incorrectly treated as a required field instead of optional. - Expenses: The
POSTandPATCHendpoints are using incorrect fields (e.g.,title,category). The requirements specify that these endpoints should handleuserId,description, andamount.
Please carefully review the requirements to ensure the user and expense objects match the specified structure. Aligning the create and update endpoints is the next critical step.
✨ 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
| app.post('/users', (req, res) => { | ||
| const { name } = req.body; | ||
|
|
||
| if (!name) { | ||
| return res.status(400).send('Bad request'); | ||
| } | ||
|
|
||
| const user = { | ||
| id: userIdCounter++, | ||
| name, | ||
| }; | ||
|
|
||
| users.push(user); | ||
|
|
||
| res.status(201).json(user); |
There was a problem hiding this comment.
According to the requirements, a new user should be created with both a name and an email. This implementation is missing the email field when processing the request body and creating the new user object.
| app.patch('/users/:id', (req, res) => { | ||
| const id = Number(req.params.id); | ||
| const { name } = req.body; | ||
|
|
||
| if (!isValidId(id) || !name) { | ||
| return res.status(400).send('Bad request'); | ||
| } | ||
|
|
||
| const user = users.find((u) => u.id === id); | ||
|
|
||
| if (!user) { | ||
| return res.status(404).send('Not found'); | ||
| } | ||
|
|
||
| user.name = name; | ||
|
|
||
| res.json(user); |
There was a problem hiding this comment.
The PATCH endpoint for users should allow updating name and/or email optionally. Currently, email is not handled, and name is incorrectly treated as a required field. The update logic should check for the presence of each field individually and update the user object accordingly.
| app.post('/expenses', (req, res) => { | ||
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if ( | ||
| userId === undefined || | ||
| !spentAt || | ||
| !title || | ||
| amount === undefined || | ||
| !category | ||
| ) { | ||
| return res.status(400).send('Bad request'); | ||
| } | ||
|
|
||
| const userExists = users.some((u) => u.id === userId); | ||
|
|
||
| if (!userExists) { | ||
| return res.status(400).send('Bad request'); | ||
| } | ||
|
|
||
| const expense = { | ||
| id: expenseIdCounter++, | ||
| userId, | ||
| spentAt, | ||
| title, | ||
| amount, | ||
| category, | ||
| }; | ||
|
|
||
| if (note !== undefined) { | ||
| expense.note = note; | ||
| } | ||
|
|
||
| expenses.push(expense); | ||
|
|
||
| res.status(201).json(expense); |
There was a problem hiding this comment.
The fields for creating an expense do not match the task requirements. The required fields are userId, description, and amount. This implementation uses spentAt, title, and category instead, and is missing the required description field.
| app.patch('/expenses/:id', (req, res) => { | ||
| const id = Number(req.params.id); | ||
|
|
||
| if (!isValidId(id)) { | ||
| return res.status(400).send('Bad request'); | ||
| } | ||
|
|
||
| const expense = expenses.find((e) => e.id === id); | ||
|
|
||
| if (!expense) { | ||
| return res.status(404).send('Not found'); | ||
| } | ||
|
|
||
| if (Object.keys(req.body).length === 0) { | ||
| return res.status(400).send('Bad request'); | ||
| } | ||
|
|
||
| const { spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if (spentAt !== undefined) { | ||
| expense.spentAt = spentAt; | ||
| } | ||
|
|
||
| if (title !== undefined) { | ||
| expense.title = title; | ||
| } | ||
|
|
||
| if (amount !== undefined) { | ||
| expense.amount = amount; | ||
| } | ||
|
|
||
| if (category !== undefined) { | ||
| expense.category = category; | ||
| } | ||
|
|
||
| if (note !== undefined) { | ||
| expense.note = note; | ||
| } | ||
|
|
||
| res.json(expense); |
There was a problem hiding this comment.
Similar to the POST endpoint, the fields for updating an expense are incorrect. The requirements state that userId, description, and/or amount should be updatable. This implementation allows updating a different set of fields and does not handle userId or description.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
brespect
left a comment
brespect
left a comment
There was a problem hiding this comment.
Good job, but let`s separate your up accourding to MVC model, to routres, services and controllers
3 participants
No description provided.