Add update-user endpoint - #117
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The updateUser helper in db/store.js is correctly implemented and follows the existing store pattern — it returns undefined for unknown ids, updates the user in place, and is properly exported. However, routes/users.js is not present in the submitted files, which means the PUT /users/:id route handler itself cannot be reviewed. This is a critical blocker: the store helper is only half the deliverable. Without the route file, there is no way to verify that the endpoint exists, that it validates name/email presence with a 400 error, or that it correctly translates the store's undefined return into a 404 response. Please upload routes/users.js so the full implementation can be reviewed against all 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
|
@mateacademy-ai-mentor routes/users.js is present in this PR — it's listed as a modified file in the diff (18 additions, adding the |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the core implementation files needed to verify the task are not available in this review payload, which blocks confirming required functionality. While CLAUDE.md and NOTES.md look good, I cannot inspect routes/users.js and db/store.js here to ensure the PUT /users/:id route validates inputs (400 for missing name/email), returns 404 for unknown ids, and uses the updateUser helper as required. Please upload or paste the full contents (with line numbers) of routes/users.js and db/store.js so I can verify: 1) the route exists and matches the existing error shape from POST /users, 2) it returns 400 when either field is missing, and 3) it returns 404 when updateUser returns undefined. If these files are already committed, re-submit them in this review or include the diffs. Minor: your documentation is clear and aligns with the workflow—no changes needed there. Once I can confirm the route and store behavior, I’ll approve promptly.
✨ 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
| # CLAUDE.md | ||
|
|
||
| Small Express API (CommonJS, Node's built-in test runner). Routes live in `routes/`, all data | ||
| access goes through `db/store.js`, and `server.js` exports `app` so tests can import it without | ||
| opening a port. | ||
|
|
||
| - `npm test` — runs `node --test` | ||
| - `npm run dev` — `node --watch server.js` | ||
| - `npm run lint` — eslint | ||
|
|
||
| Current task: add `PUT /users/:id` to `routes/users.js`, with the store helper it needs in | ||
| `db/store.js`. `tests/update-user.test.js` is the spec — do not edit it. | ||
|
|
||
| ## Models | ||
|
|
||
| Plan with **Opus**, execute with **Sonnet**. | ||
|
|
||
| Use Opus in plan mode to decide the approach — which files change, how validation and the | ||
| not-found path work, how the work splits into commits. Switch to Sonnet (`/model sonnet`) to | ||
| write the code, run tests, and commit. If a plan turns out to be wrong mid-execution, stop and | ||
| re-plan with Opus rather than improvising in Sonnet. | ||
|
|
||
| ## Commits | ||
|
|
||
| One logical change per commit. Write the message from the actual diff — read what changed, then | ||
| describe it; do not paraphrase the task description. | ||
|
|
||
| Run `npm test` as you go, not just at the end. The `update-user` tests turn green once the | ||
| endpoint is right; that is the signal the feature is done. | ||
|
|
||
| Each message must be understandable without opening the diff. A reader scanning `git log` | ||
| should know what changed and why. | ||
|
|
||
| Good: Add updateUser helper to store, returning undefined for unknown ids | ||
| Bad: Update store.js | ||
|
|
||
| Do not bundle the store helper, the route, and `NOTES.md` into one commit — they are separate | ||
| logical changes. | ||
|
|
||
| ## Review before the PR | ||
|
|
||
| Before opening the pull request, review the changes yourself — `git diff main...HEAD` — and | ||
| report what you find. Look specifically for: | ||
|
|
||
| - bugs and edge cases (non-numeric `:id`, empty-string vs missing field, extra body fields) | ||
| - the not-found path — 404, not a crash or a 500 | ||
| - the validation path — 400 with a clear error, matching the shape `POST /users` already uses | ||
|
|
||
| Present what you flag as findings for me to judge. I decide what is real; fix those, and say | ||
| plainly which ones I chose to skip. | ||
|
|
||
| A green test run and a clean review are signals, not a guarantee. Do not describe the change as | ||
| verified on the strength of passing tests alone — say what was actually checked and what was not. | ||
|
|
||
| ## PR | ||
|
|
||
| The description must say what changed, why, and what a reviewer should test — including the | ||
| not-found and invalid-input cases. |
There was a problem hiding this comment.
CLAUDE.md correctly documents the workflow and expectations. No action needed here. (No functional code to validate.)
| # CLAUDE.md | ||
|
|
||
| Small Express API (CommonJS, Node's built-in test runner). Routes live in `routes/`, all data | ||
| access goes through `db/store.js`, and `server.js` exports `app` so tests can import it without | ||
| opening a port. | ||
|
|
||
| - `npm test` — runs `node --test` | ||
| - `npm run dev` — `node --watch server.js` | ||
| - `npm run lint` — eslint | ||
|
|
||
| Current task: add `PUT /users/:id` to `routes/users.js`, with the store helper it needs in | ||
| `db/store.js`. `tests/update-user.test.js` is the spec — do not edit it. | ||
|
|
||
| ## Models | ||
|
|
||
| Plan with **Opus**, execute with **Sonnet**. | ||
|
|
||
| Use Opus in plan mode to decide the approach — which files change, how validation and the | ||
| not-found path work, how the work splits into commits. Switch to Sonnet (`/model sonnet`) to | ||
| write the code, run tests, and commit. If a plan turns out to be wrong mid-execution, stop and | ||
| re-plan with Opus rather than improvising in Sonnet. | ||
|
|
||
| ## Commits | ||
|
|
||
| One logical change per commit. Write the message from the actual diff — read what changed, then | ||
| describe it; do not paraphrase the task description. |
There was a problem hiding this comment.
NOTES.md contains a clear plan, model choice, commit split and review summary as required by the checklist. However, the NOTES states the implementation exists; I cannot verify that because the route and store implementation files are not included in this review. Please include routes/users.js and db/store.js (or the diffs) so I can confirm the endpoint validates input, returns 400 on missing fields, and returns 404 for unknown ids as required by the tests.
| # CLAUDE.md | ||
|
|
||
| Small Express API (CommonJS, Node's built-in test runner). Routes live in `routes/`, all data | ||
| access goes through `db/store.js`, and `server.js` exports `app` so tests can import it without | ||
| opening a port. | ||
|
|
||
| - `npm test` — runs `node --test` | ||
| - `npm run dev` — `node --watch server.js` | ||
| - `npm run lint` — eslint | ||
|
|
||
| Current task: add `PUT /users/:id` to `routes/users.js`, with the store helper it needs in | ||
| `db/store.js`. `tests/update-user.test.js` is the spec — do not edit it. | ||
|
|
||
| ## Models | ||
|
|
||
| Plan with **Opus**, execute with **Sonnet**. | ||
|
|
||
| Use Opus in plan mode to decide the approach — which files change, how validation and the | ||
| not-found path work, how the work splits into commits. Switch to Sonnet (`/model sonnet`) to | ||
| write the code, run tests, and commit. If a plan turns out to be wrong mid-execution, stop and | ||
| re-plan with Opus rather than improvising in Sonnet. | ||
|
|
||
| ## Commits | ||
|
|
||
| One logical change per commit. Write the message from the actual diff — read what changed, then | ||
| describe it; do not paraphrase the task description. |
There was a problem hiding this comment.
The previous review summary (from the review context) flagged that routes/users.js was missing from the PR. The user message claims the file is present in the PR. If you believe it is present, please re-upload its contents here (the file text with line markers) so I can inspect it line-by-line and confirm compliance with the checklist.
|
@mateacademy-ai-mentor Here are the full contents of both files, with line numbers, as requested.
|
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is approved and complete. The PUT /users/:id endpoint correctly validates name and email, returns 400 with the exact error shape matching POST /users, handles unknown ids with 404 via store.updateUser, and all data access goes through db/store.js as required. Your documentation in CLAUDE.md and NOTES.md is thorough and well-organized. Great work following the existing patterns precisely — this makes the codebase consistent and maintainable. The only minor step before final submission is confirming npm test is green on the pushed branch to fully close the loop.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
What
Adds
PUT /users/:idto the users resource: updates an existing user, validatesname/email(400 on missing/invalid), and returns 404 for an unknown id instead of crashing. Adds a matchingupdateUserhelper indb/store.jsso the route goes through the store like the rest of the resource.Why
Closes out the users resource with update support, following the same validation/error shape already used by
POST /usersand the same not-found handling used byGET /users/:id.What to test
PUT /users/:idwith a valid body updates the user and returns 200 with the updated fields.PUT /users/9999(unknown id) returns 404, not a crash.PUT /users/:idwith a missingnameoremailreturns 400 with a clear error message.npm testis green (9/9), including the pre-existingupdate-user.test.jssuite.See
NOTES.mdfor the plan, model choice, commit split, and self-review notes.