|
| 1 | +# Notes: update a user (`PUT /users/:id`) |
| 2 | + |
| 3 | +## Plan |
| 4 | + |
| 5 | +Implemented the "update a user" feature per `tests/update-user.test.js`, which fully |
| 6 | +specified the contract: 200 + updated user on success, 404 for an unknown id, 400 when |
| 7 | +`name` or `email` is missing. Explored the existing codebase first (`routes/users.js`, |
| 8 | +`db/store.js`) and reused its conventions rather than inventing new ones: hand-rolled |
| 9 | +truthiness validation (matching `POST /users`), `Number(req.params.id)` + lookup for |
| 10 | +existence (matching `GET /users/:id`), and the `{ error: "<message>" }` response shape |
| 11 | +for both 400 and 404. Validation runs before the not-found lookup so a request missing a |
| 12 | +field always returns 400, even for an existing user. |
| 13 | + |
| 14 | +## Model choice |
| 15 | + |
| 16 | +Added `updateUser(id, { name, email })` to `db/store.js`, mirroring `createUser`'s style: |
| 17 | +look up the user via the existing `getUserById`, return `undefined` if not found (letting |
| 18 | +the route decide the 404 response), otherwise mutate `name`/`email` in place and return |
| 19 | +the same object reference. No new dependencies or validation library were introduced — |
| 20 | +the app has none, and adding one for two fields would be inconsistent with the rest of |
| 21 | +the codebase. |
| 22 | + |
| 23 | +## Commit split |
| 24 | + |
| 25 | +Two logical pieces: (1) the store helper `updateUser` plus its export, and (2) the route |
| 26 | +handler `router.put("/:id", ...)` that wires validation, the store call, and the 404/200 |
| 27 | +responses together. This `NOTES.md` rides along as a third, since `tests/notes.test.js` |
| 28 | +requires it for the suite to pass. |
| 29 | + |
| 30 | +## Review |
| 31 | + |
| 32 | +Checked the three grading tests in `tests/update-user.test.js` line by line against the |
| 33 | +handler: update-and-200, unknown-id-404, and missing-field-400. Confirmed the existing |
| 34 | +`tests/users.test.js` and `tests/notes.test.js` still pass, and that `npm run lint` stays |
| 35 | +clean (no new eslint-disable comments needed, no unused vars). |
| 36 | + |
| 37 | +Ran a broader self-review (correctness, reuse, and cross-file angles) before opening the |
| 38 | +PR. It confirmed the not-found and malformed-JSON paths already behave the same as the |
| 39 | +existing `GET /users/:id` and `POST /users` routes (nothing new introduced there), and |
| 40 | +flagged two things worth naming rather than fixing: (1) validation is truthy-only, so a |
| 41 | +value like `name: 0` would pass and get persisted — an existing gap in `POST /users` that |
| 42 | +this change now also applies to overwriting existing records; and (2) the id-parsing, |
| 43 | +validation, and 404-response blocks are each a few lines duplicated from `GET /:id` / |
| 44 | +`POST /`. Left both as-is: they match this repo's existing (helper-free) conventions, and |
| 45 | +extracting shared validation for two fields across a 53-line file would be inconsistent |
| 46 | +with the rest of the codebase. |
0 commit comments