Shipped change - #143
Conversation
Adds a data-access function to update an existing user by id, returning undefined when the id doesn't exist, mirroring the not-found handling already used by getUserById. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E6kkuE2k1kVobAGLNPt4uT
Updates an existing user, validating that name and email are present (400 if missing) and returning 404 when the user doesn't exist, following the same pattern as the existing GET/POST routes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E6kkuE2k1kVobAGLNPt4uT
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E6kkuE2k1kVobAGLNPt4uT
Previously, missing name/email was validated before checking whether the user exists, so PUT to a nonexistent id with an incomplete body returned 400 instead of 404, masking the real problem. Now existence is checked first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E6kkuE2k1kVobAGLNPt4uT
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E6kkuE2k1kVobAGLNPt4uT
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission because the core functionality is correctly implemented and matches the requirements. The PUT /users/:id route validates inputs (returns 400 for missing name or email), returns 404 for unknown IDs, and updates users via db/store.updateUser, which returns undefined when not found—this aligns with the task’s expectations. Tests for these behaviors should pass given the current logic.
Minor improvements are suggested for clarity in NOTES.md: make commit counts consistent, list which files were changed and why, rewrite a few ambiguous sentences (e.g., “I made Claude to create the plan,” “3rd step had no impact on the code”) in plain language, and add a brief confirmation that local tests (npm test) passed and that NOTES.md was committed. These are documentation refinements and not blockers.
Nice work delivering a clean, predictable update flow and adhering to the data-access pattern. Keep the documentation tweaks in mind to make verification even smoother.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -0,0 +1,2 @@ | |||
| I made Claude to create the plan. I approved the 1st and 2nd steps: add/implement the updateUser() function and add PUT handler to routes.js. 3rd step had no impact on the code. In the 4th step would have created and filled the NOTES.md file. I asked to skip it as I had to do that. I chose Sonnet for model because this task seemed not be too complex, and so the whole is cheaper. Two commits were asked: 1: store.js as it contains the updateUser() function; 2: users.js as it uses updateUser(). 3rd will contain NOTES.md. After saving this with content everything was fine, all tests were green. The review caught 1 bug, which was fixed in the 4th commit. | |||
There was a problem hiding this comment.
The file should clearly state what was actually changed (which files were modified and why). Right now it says steps were approved but doesn't explicitly confirm which files were committed or the final status besides "tests were green". Consider listing the actual commits or the exact files changed to make verification straightforward.
| @@ -0,0 +1,2 @@ | |||
| I made Claude to create the plan. I approved the 1st and 2nd steps: add/implement the updateUser() function and add PUT handler to routes.js. 3rd step had no impact on the code. In the 4th step would have created and filled the NOTES.md file. I asked to skip it as I had to do that. I chose Sonnet for model because this task seemed not be too complex, and so the whole is cheaper. Two commits were asked: 1: store.js as it contains the updateUser() function; 2: users.js as it uses updateUser(). 3rd will contain NOTES.md. After saving this with content everything was fine, all tests were green. The review caught 1 bug, which was fixed in the 4th commit. | |||
There was a problem hiding this comment.
There are grammar and clarity issues that make the meaning ambiguous (e.g., "I made Claude to create the plan", "3rd step had no impact on the code"). Please rewrite to plain language describing the plan you approved and any edits you made before approving.
| @@ -0,0 +1,2 @@ | |||
| I made Claude to create the plan. I approved the 1st and 2nd steps: add/implement the updateUser() function and add PUT handler to routes.js. 3rd step had no impact on the code. In the 4th step would have created and filled the NOTES.md file. I asked to skip it as I had to do that. I chose Sonnet for model because this task seemed not be too complex, and so the whole is cheaper. Two commits were asked: 1: store.js as it contains the updateUser() function; 2: users.js as it uses updateUser(). 3rd will contain NOTES.md. After saving this with content everything was fine, all tests were green. The review caught 1 bug, which was fixed in the 4th commit. | |||
There was a problem hiding this comment.
The notes mention choosing the "Sonnet" model and that it was chosen to reduce cost — the course asked to state which model was chosen and why, which you've done, but consider also adding a sentence confirming that the tests passed locally (e.g., npm test passed) and that NOTES.md was committed as required by the grading script.
| @@ -0,0 +1,2 @@ | |||
| I made Claude to create the plan. I approved the 1st and 2nd steps: add/implement the updateUser() function and add PUT handler to routes.js. 3rd step had no impact on the code. In the 4th step would have created and filled the NOTES.md file. I asked to skip it as I had to do that. I chose Sonnet for model because this task seemed not be too complex, and so the whole is cheaper. Two commits were asked: 1: store.js as it contains the updateUser() function; 2: users.js as it uses updateUser(). 3rd will contain NOTES.md. After saving this with content everything was fine, all tests were green. The review caught 1 bug, which was fixed in the 4th commit. | |||
There was a problem hiding this comment.
The closing sentence references a bug fixed in a "4th commit", but earlier you said there were only two (or three) commits. Make the commit count consistent: either list each commit and its purpose or correct the numbers so reviewers can follow the history.
2 participants
No description provided.