|
| 1 | +# Notes: PUT /users/:id ("update a user") |
| 2 | + |
| 3 | +Adds an update endpoint to the users resource: `PUT /users/:id`, with input |
| 4 | +validation, a not-found response, and data access through `db/store.js`, |
| 5 | +following the same pattern as the existing `GET`/`POST` routes. |
| 6 | + |
| 7 | +## The plan I approved |
| 8 | + |
| 9 | +The approved plan was: add an `updateUser(id, { name, email })` helper to |
| 10 | +`db/store.js` that returns the updated user or `undefined` if no user has |
| 11 | +that id (mirroring the `undefined` contract `getUserById` already uses), |
| 12 | +then add a `PUT /:id` route in `routes/users.js` that validates the body |
| 13 | +first and only then looks the user up, returning 400 for invalid input, 404 |
| 14 | +for a missing user, and 200 with the updated user otherwise. Work was split |
| 15 | +into a store commit, a route commit, and a docs commit. |
| 16 | + |
| 17 | +Before approving, I asked Claude to choose between three validation |
| 18 | +strictness options and picked the stricter one: a `isNonEmptyString` helper |
| 19 | +that rejects missing, empty, whitespace-only, and non-string values, applied |
| 20 | +only to the new `PUT` route. The existing `POST` route's looser |
| 21 | +`!name || !email` check was left untouched, so no working behaviour changed |
| 22 | +as a side effect of this feature. I also asked for the pre-push smoke checks |
| 23 | +to be run against a live server automatically rather than by hand, and for |
| 24 | +any review fixes to land as a separate follow-up commit rather than an |
| 25 | +amend, so the history stays honest about what the review actually did. |
| 26 | + |
| 27 | +## Model choice |
| 28 | + |
| 29 | +Two models, split by stage. **Opus 5** for planning: the design decisions |
| 30 | +(the store's `undefined`-for-missing contract, how strict validation should |
| 31 | +be, validating before the lookup, how to split the commits) are where a |
| 32 | +wrong call would have been expensive to unwind, and as a Data Analyst rather |
| 33 | +than a JavaScript developer I was relying on the model's judgement there, |
| 34 | +not just its typing speed. **Sonnet 5** for implementation: once the plan |
| 35 | +was approved, the remaining work was a well-specified ~25-line diff with |
| 36 | +`npm test` and `npm run lint` as objective correctness checks, so the |
| 37 | +faster, cheaper model was the right fit for executing an already-agreed |
| 38 | +design. |
| 39 | + |
| 40 | +## Commit split |
| 41 | + |
| 42 | +Three commits: `db/store.js` (the `updateUser` helper), `routes/users.js` |
| 43 | +(the `PUT /:id` route), then this file. The store change is a standalone |
| 44 | +data-access contract, kept separate from the HTTP layer that uses it. The |
| 45 | +route commit is the one that turns `tests/update-user.test.js` green, so |
| 46 | +`git log` shows exactly where the feature landed. Each commit leaves the |
| 47 | +tree in a working, test-passing state on its own (the store commit alone |
| 48 | +changes no observable behaviour; the route commit alone makes all three |
| 49 | +update-user tests pass). Docs went last so this file could describe the |
| 50 | +review that only happens after the code exists. |
| 51 | + |
| 52 | +## What the review caught |
| 53 | + |
| 54 | +I ran `/code-review` (medium effort) against the branch diff before pushing. |
| 55 | +It reported **no findings** — an empty result, not "review not run". No |
| 56 | +follow-up commit was needed. |
| 57 | + |
| 58 | +Beyond the automated review, I ran live smoke checks against a real running |
| 59 | +server (not just the three graded tests), covering cases the grading tests |
| 60 | +don't touch: |
| 61 | + |
| 62 | +- Happy path (`PUT /users/1` with a valid body) → `200` with the updated |
| 63 | + user, and a follow-up `GET /users/1` confirmed the change persisted |
| 64 | + through the store, not just in the response. |
| 65 | +- Whitespace-only name (`" "`) → `400`. |
| 66 | +- Non-string field (`name: 123`) → `400`. |
| 67 | +- Extra unknown key in the body (`admin: true`) → `200`, and the key was |
| 68 | + silently dropped rather than written to the record, confirming |
| 69 | + `updateUser` only ever assigns `name` and `email`. |
| 70 | +- Non-numeric id (`/users/abc`) and unknown numeric id (`/users/9999`) → |
| 71 | + both `404` with a clean JSON body, no crash. |
| 72 | +- `GET /health` after all of the above → still `200`, confirming the |
| 73 | + process stayed up through every case, including the invalid ones. |
| 74 | + |
| 75 | +Everything the review and the smoke checks confirmed was already fine, so |
| 76 | +the endpoint shipped as designed in the approved plan with no changes. |
0 commit comments