Skip to content

added solution#919

Open
Demtar23 wants to merge 3 commits into
mate-academy:masterfrom
Demtar23:develop
Open

added solution#919
Demtar23 wants to merge 3 commits into
mate-academy:masterfrom
Demtar23:develop

Conversation

@Demtar23
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Changes are needed before this solution can be considered complete, mainly because a few behaviors do not match the documented API and will cause tests to fail.

  • In src/createServer.js, the GET /expenses route currently reads req.query.categories, but the API spec expects a single category query parameter; adjust the handler to use req.query.category so filtering works correctly.
  • In the POST /expenses handler, you compare user.id === userId, which can fail when userId comes in as a string; coerce the request value to a number (e.g., via Number(userId)) or normalize IDs earlier before comparison.
  • The same POST /expenses handler returns HTTP 400 when the referenced user does not exist, but the requirements specify a 404 for missing entities, so update this branch to send a 404 status instead.
  • You currently treat note as a required field for new expenses, but according to the Swagger docs it may be optional; review the spec and remove note from the required-fields check if it’s not mandatory so valid requests without note don’t get rejected.
    Your overall structure (collections, endpoints, in-memory persistence) looks solid, and once these contract-alignment issues are fixed your implementation should be 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/createServer.js
Comment on lines +108 to +111
if (req.query.categories) {
result = result.filter(
(expense) => expense.category === req.query.categories,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET /expenses filters by the query parameter categories here, but the API spec uses category (singular). Use req.query.category so the filtering matches the documentation and tests.

Comment thread src/createServer.js Outdated
return;
}

const userExist = users.find((user) => user.id === userId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When checking whether a user exists you compare user.id === userId (possible type mismatch). Coerce userId to a number (e.g., user.id === Number(userId)) or normalize earlier to avoid false negatives.

Comment thread src/createServer.js
Comment on lines +148 to +151
if (!userExist) {
res.sendStatus(400);

return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the referenced user does not exist you return HTTP 400 here. Per the task requirements tests expect HTTP 404 for missing entities — change this to res.sendStatus(404) so missing related resources return 404.

Comment thread src/createServer.js Outdated
Comment on lines +134 to +141
if (
userId == null ||
title == null ||
amount == null ||
category == null ||
note == null
) {
res.sendStatus(400);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You validate note as required in the POST /expenses request body. Verify the Swagger docs: if note is optional in the spec, remove it from the required-fields check so the endpoint behavior matches the API documentation and tests.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are still two behavior mismatches with the API/tests that will cause functionality to break. In src/createServer.js, in the GET /expenses handler (the block where you read req.query to filter by category), you currently use req.query.categories; this needs to be changed to req.query.category so that filtering works correctly and matches the documented single category query parameter ([CHECKLIST ITEM #21]). Also in src/createServer.js, in the POST /expenses handler where you validate that the referenced user exists, the branch for a missing user currently does res.sendStatus(400); this should be res.sendStatus(404) to comply with the requirement that missing entities return 404 ([CHECKLIST ITEM #23]). Both of these issues are critical because they will cause contract tests to fail and will return incorrect responses to clients even though the rest of your implementation is solid. To fix them, update the query parameter name used in the filter condition and adjust the status code for the missing-user branch, then re-run the tests to ensure everything passes. Aside from these two points, your structure with in-memory users and expenses, the five endpoints per collection, and the note and userId handling looks very good and close to complete. With these small but important corrections, your solution should fully meet the task 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/createServer.js
Comment on lines +108 to +111
if (req.query.categories) {
result = result.filter(
(expense) => expense.category === req.query.categories,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API spec and tests expect a single query parameter named category. Here the handler checks req.query.categories and filters by req.query.categories. Change this to use req.query.category so filtering by category works as expected and matches the documentation/tests.

Comment thread src/createServer.js
Comment on lines +142 to +145
if (!userExist) {
res.sendStatus(400);

return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the referenced user doesn't exist the handler responds with status 400. Per the task requirements (and tests) missing entities should produce a 404. Replace res.sendStatus(400) with res.sendStatus(404) for this branch.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can continue progressing while you address a couple of small but important contract issues. Overall, your createServer implementation is solid: you correctly define both users and expenses collections, use in-memory arrays that reset on server start, return the Express app from createServer, and handle most 400/404 cases as required.

The main issues to fix are:

  • In src/createServer.js, in the GET /expenses route (the block starting with app.get('/expenses'...)), you currently check req.query.categories when filtering; the API contract and tests expect a single category query parameter, so update the filter to use req.query.category instead [CHECKLIST ITEM #20].
  • In the POST /expenses route (the block starting with app.post('/expenses'...)), when you look up the user by userId and it is not found, you send a 400 status; per the requirements, missing entities must return 404, so change only that branch to send 404 while keeping 400 for truly invalid/missing request data [CHECKLIST ITEM #21].

As minor polish, you could prefer res.status(…) over setting res.statusCode directly and make your validation checks a bit more explicit for readability, but these are not blocking. Your solution is approved, and with those small adjustments you’ll have a clean, spec-compliant implementation that should satisfy the tests fully.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/createServer.js
Comment on lines +108 to +111
if (req.query.categories) {
result = result.filter(
(expense) => expense.category === req.query.categories,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GET /expenses you check req.query.categories and compare expense.category to req.query.categories. The docs/tests expect a single category query parameter. Use req.query.category so filtering matches the API contract.

Comment thread src/createServer.js
Comment on lines +142 to +143
if (!userExist) {
res.sendStatus(400);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In POST /expenses, when the referenced user is not found you return res.sendStatus(400). Per the requirements, missing entities must return 404. Change this branch to return 404 to match the spec/tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants