araemer/recipe-backend
1
0
Fork 0
Code Issues 4 Pull requests 3 Projects Releases 17 Packages Wiki Activity Actions

feat(shoppingList) Shopping list overview with item previews (spec 002) #25

Merged
araemer merged 17 commits from 002-shopping-list-overview-previews into stage 2026-10-04 06:14:40 +00:00
Conversation 2 Commits 17 Files changed 32 +3485 -151
araemer commented 2026-10-03 14:07:30 +00:00
Owner
Copy link

Implements spec 002 (shopping list overview with item previews) in recipe-backend.

What was implemented

Plan step Result
1 Integration test setup ADR; vitest.integration.config.ts; npm run test:integration; src/tests/integration/integrationDataSource.ts (schema per run in the database of .env.test, entities and migrations loaded from the .ts sources with import.meta.glob); npm test excludes *.integration.test.ts; CLAUDE.md and architecture §9
2 Migration 1780012800013-CreateShoppingListItemChangeTable.ts: table plus backfill with AT TIME ZONE current_setting('TimeZone')
3 Entity ShoppingListItemChangeEntity; one sentence on ShoppingListEntity
4 Repositories insertItemChange, recordItemChange; findCompactForUser joins the change table outside the union and orders by it; findOpenItemPreviews; ShoppingListItemPreviewProjection; deleteById and deleteTickedOnList return the row count
5 Handlers C1–C9 as in the plan's table. Tick, edit, delete and remove-ticked run in a transaction, in full, without a list lock. New itemContentEquals
6–7 Overview OPEN_ITEMS_PREVIEW_SIZE, ShoppingListOverviewRow, getOverview; mapper and service
8 Unit tests Recorded / not recorded and the call order for every method; itemContentEquals; overview, mapper and service cases
9 Integration tests ShoppingListItemChange.integration.test.ts, 45 tests: 9.1 recorded and list row untouched, 9.2 "written last" for every row of the plan's table plus the held-list-lock test, 9.3 tick × reorder (50 rounds) and all 16 pairs (20 rounds each), 9.4 the overview queries
10 Documentation docs/architecture.md §4.2, 4.4–4.7, 5, 6, 7, 9

Deviations from the plan

One, recorded in the plan's Deviations section:

  • ShoppingListItemHandler no longer takes a list repository (2026-10-04, decided by the user in the session). The plan kept the constructor and the factory unchanged; the unused listRepository parameter is now removed, and the factory, the unit test and the integration test pass three arguments.

Added after the review (on the user's request)

  • docs/howto-testing.md: running and writing tests, setting up the test database.
  • docs/howto-locking.md: the locks, the lock order, the rules for the change row (including "at most once per transaction"), the steps for adding a write, and the known gap of plan judgment call 10.
  • Both are linked from docs/architecture.md (header, §6, §9) and CLAUDE.md. Architecture §3 lists src/tests/integration/.
  • CLAUDE.md: the naming vocabulary gains …DataSource, and a short rule points shopping list writes to the locking how-to.
  • The deviation of 2026-10-04 is confirmed by the user.
  • Review findings fixed: explicit types in three test files; the matrix path names are one WritePath const; a 404 is now accepted only in deleteItem × untickItem and deleteItem × moveItem rounds on the same item and in removeTicked × untickItem (which has no "different items" variant, as the test now says). npm test 606 passed, npm run test:integration 45 passed three times in a row with the stricter rule.
  • coverage/ is in .gitignore. A coverage report was committed by mistake in afd6c04 and removed in 6630bba; both commits stay in the history.

Finding for the reviewer

recordItemChange must be called at most once per transaction. A second UPDATE of a change row that the same transaction already wrote makes PostgreSQL re-check the foreign key, and that check takes a key-share lock on the list row, the same lock the ADR rules out for an upsert. Found while checking that the integration tests can fail: with a second call added to removeTicked, the held-list-lock test ran into the lock timeout. The code calls it once per transaction everywhere (a move passes both lists in one call). The rule is now stated in the JSDoc of recordItemChange and in architecture §4.6 and §6. The ADR does not state it; it is immutable, so if the rule should be in an ADR, that needs a decision.

Judgment calls

  1. (Superseded by the deviation above: the unused listRepository constructor parameter was removed.)
  2. Tick × reorder (9.3) runs its 50 rounds on one list and unticks the ticked item after each round, so the list has 6 open items in every round. Each round ticks a different item.
  3. Matrix (9.3): a 404 is accepted only where the pair and round make it possible (see "Added after the review"). In every other pair any rejection fails the test. In "same item" rounds, tickItem × untickItem both act on an open item (the untick then either finds it open or undoes the tick), and deleteItem × untickItem both act on a ticked item.
  4. The "written last" probe (9.2) checks first that the handler call is still pending, then that the item rows are locked. A handler that never writes the change row then fails with the message that says so.
  5. integrationDataSource.ts loads .env.test with override: true, so DB_* variables from the shell cannot point the tests at another database.
  6. Reviewer grep 3 has one more hit than the plan expects: src/entities/ShoppingListEntity.ts names ShoppingListItemChangeEntity in the JSDoc sentence that plan step 3 asks for. It is a comment, not a write.
  7. Beyond the plan's list, the integration tests also check that items_changed_at is timestamp with time zone, that a rename with a version from before a partner's item changes succeeds, and that a previewed row carries amount and unit and no comment.

Verification

Check Result
npm run build in ../recipe-platform/api-contract ok
npm run build ok
npm test 56 files, 606 tests passed (re-run after the constructor change)
npm run test:integration 45 tests passed, three runs in a row, and once more after the constructor change (PostgreSQL 17.10, session time zone Europe/Berlin). CI does not run these.
Do the 9.2 tests fail when the rule is broken? Yes. Checked with temporary changes, not committed: record before the item write in removeTicked, and no record in tickItem. Both failed the tests
Migration on the development database (step 11.4) pg_typeof(items_changed_at) is timestamp with time zone; 1 list, 1 change row; the backfilled time equals the newest item update_date of that list as wall-clock time. The running start:dev server applied the migration as soon as it was compiled, so the "before" values were read from the unchanged source columns afterwards, not noted in advance. The database had one list, not two or three
Two users against the running server (step 11.5) All cases of the plan passed, run as a script against the HTTP API instead of by hand in Bruno: preview of 5 and count without ticked items; a partner's add brings the list to the front; DUPLICATE, tick on a ticked item and edit with the same content do not; tick, untick, edit, delete, remove-ticked, reorder and move do (move: both lists); rename with a version from an old overview returns 201; a new list is first with an empty preview and count 0. The test lists were deleted afterwards
git diff --stat stage (step 11.6) no change to AbstractEntity.ts, AbstractRepository.ts or any recipe or meal plan file
Reviewer greps (step 11.7) as expected: 8 calls of recordItemChange in ShoppingListItemHandler.ts, 1 call of insertItemChange in ShoppingListHandler.ts, 2 definitions; no upsert or save in recordItemChange; update(ShoppingListEntity) only in renameIfVersion; one this.itemRepository hit (first read of moveItem); no list lock in tick, edit, delete, remove-ticked. Grep 3: see judgment call 6

Not verified: nothing was skipped. The repo defines no linter.

Notes

  • #23 touches docs/architecture.md and the class comment of ShoppingListRepository. Expect text conflicts with whichever merges second; the plan says how to resolve them.
  • The contract pin (contract.ref) is on platform main.
  • Judgment call 10 of the plan (a deadlock between reorder and remove-ticked that spec 001 already has) is not fixed here and has no issue yet.

🤖 Generated with Claude Code

Implements spec 002 (shopping list overview with item previews) in recipe-backend. - Spec: https://git.raemer.net/araemer/recipe-platform/src/branch/main/docs/features/002-shopping-list-overview-previews.md - Plan: `docs/plans/002-shopping-list-overview-previews.md` - ADRs: `docs/adr/2026-10-03-shopping-list-item-change-time.md`, `docs/adr/2026-10-03-shopping-list-overview-query.md` (GATE B), and new in this PR: `docs/adr/2026-10-03-database-backed-integration-tests.md` (plan step 1.1) ## What was implemented | Plan step | Result | |---|---| | 1 Integration test setup | ADR; `vitest.integration.config.ts`; `npm run test:integration`; `src/tests/integration/integrationDataSource.ts` (schema per run in the database of `.env.test`, entities and migrations loaded from the `.ts` sources with `import.meta.glob`); `npm test` excludes `*.integration.test.ts`; `CLAUDE.md` and architecture §9 | | 2 Migration | `1780012800013-CreateShoppingListItemChangeTable.ts`: table plus backfill with `AT TIME ZONE current_setting('TimeZone')` | | 3 Entity | `ShoppingListItemChangeEntity`; one sentence on `ShoppingListEntity` | | 4 Repositories | `insertItemChange`, `recordItemChange`; `findCompactForUser` joins the change table outside the union and orders by it; `findOpenItemPreviews`; `ShoppingListItemPreviewProjection`; `deleteById` and `deleteTickedOnList` return the row count | | 5 Handlers | C1–C9 as in the plan's table. Tick, edit, delete and remove-ticked run in a transaction, in full, without a list lock. New `itemContentEquals` | | 6–7 Overview | `OPEN_ITEMS_PREVIEW_SIZE`, `ShoppingListOverviewRow`, `getOverview`; mapper and service | | 8 Unit tests | Recorded / not recorded and the call order for every method; `itemContentEquals`; overview, mapper and service cases | | 9 Integration tests | `ShoppingListItemChange.integration.test.ts`, 45 tests: 9.1 recorded and list row untouched, 9.2 "written last" for every row of the plan's table plus the held-list-lock test, 9.3 tick × reorder (50 rounds) and all 16 pairs (20 rounds each), 9.4 the overview queries | | 10 Documentation | `docs/architecture.md` §4.2, 4.4–4.7, 5, 6, 7, 9 | ## Deviations from the plan One, recorded in the plan's Deviations section: - **`ShoppingListItemHandler` no longer takes a list repository** (2026-10-04, decided by the user in the session). The plan kept the constructor and the factory unchanged; the unused `listRepository` parameter is now removed, and the factory, the unit test and the integration test pass three arguments. ## Added after the review (on the user's request) - `docs/howto-testing.md`: running and writing tests, setting up the test database. - `docs/howto-locking.md`: the locks, the lock order, the rules for the change row (including "at most once per transaction"), the steps for adding a write, and the known gap of plan judgment call 10. - Both are linked from `docs/architecture.md` (header, §6, §9) and `CLAUDE.md`. Architecture §3 lists `src/tests/integration/`. - `CLAUDE.md`: the naming vocabulary gains `…DataSource`, and a short rule points shopping list writes to the locking how-to. - The deviation of 2026-10-04 is confirmed by the user. - Review findings fixed: explicit types in three test files; the matrix path names are one `WritePath` const; a 404 is now accepted only in `deleteItem × untickItem` and `deleteItem × moveItem` rounds on the same item and in `removeTicked × untickItem` (which has no "different items" variant, as the test now says). `npm test` 606 passed, `npm run test:integration` 45 passed three times in a row with the stricter rule. - `coverage/` is in `.gitignore`. A coverage report was committed by mistake in `afd6c04` and removed in `6630bba`; both commits stay in the history. ## Finding for the reviewer **`recordItemChange` must be called at most once per transaction.** A second `UPDATE` of a change row that the same transaction already wrote makes PostgreSQL re-check the foreign key, and that check takes a key-share lock on the list row, the same lock the ADR rules out for an upsert. Found while checking that the integration tests can fail: with a second call added to `removeTicked`, the held-list-lock test ran into the lock timeout. The code calls it once per transaction everywhere (a move passes both lists in one call). The rule is now stated in the JSDoc of `recordItemChange` and in architecture §4.6 and §6. The ADR does not state it; it is immutable, so if the rule should be in an ADR, that needs a decision. ## Judgment calls 1. *(Superseded by the deviation above: the unused `listRepository` constructor parameter was removed.)* 2. **Tick × reorder (9.3) runs its 50 rounds on one list** and unticks the ticked item after each round, so the list has 6 open items in every round. Each round ticks a different item. 3. **Matrix (9.3): a 404 is accepted only where the pair and round make it possible** (see "Added after the review"). In every other pair any rejection fails the test. In "same item" rounds, `tickItem × untickItem` both act on an open item (the untick then either finds it open or undoes the tick), and `deleteItem × untickItem` both act on a ticked item. 4. **The "written last" probe (9.2) checks first that the handler call is still pending**, then that the item rows are locked. A handler that never writes the change row then fails with the message that says so. 5. **`integrationDataSource.ts` loads `.env.test` with `override: true`**, so `DB_*` variables from the shell cannot point the tests at another database. 6. **Reviewer grep 3 has one more hit than the plan expects:** `src/entities/ShoppingListEntity.ts` names `ShoppingListItemChangeEntity` in the JSDoc sentence that plan step 3 asks for. It is a comment, not a write. 7. Beyond the plan's list, the integration tests also check that `items_changed_at` is `timestamp with time zone`, that a rename with a version from before a partner's item changes succeeds, and that a previewed row carries amount and unit and no comment. ## Verification | Check | Result | |---|---| | `npm run build` in `../recipe-platform/api-contract` | ok | | `npm run build` | ok | | `npm test` | 56 files, 606 tests passed (re-run after the constructor change) | | `npm run test:integration` | 45 tests passed, three runs in a row, and once more after the constructor change (PostgreSQL 17.10, session time zone Europe/Berlin). **CI does not run these.** | | Do the 9.2 tests fail when the rule is broken? | Yes. Checked with temporary changes, not committed: record before the item write in `removeTicked`, and no record in `tickItem`. Both failed the tests | | Migration on the development database (step 11.4) | `pg_typeof(items_changed_at)` is `timestamp with time zone`; 1 list, 1 change row; the backfilled time equals the newest item `update_date` of that list as wall-clock time. The running `start:dev` server applied the migration as soon as it was compiled, so the "before" values were read from the unchanged source columns afterwards, not noted in advance. The database had one list, not two or three | | Two users against the running server (step 11.5) | All cases of the plan passed, run as a script against the HTTP API instead of by hand in Bruno: preview of 5 and count without ticked items; a partner's add brings the list to the front; `DUPLICATE`, tick on a ticked item and edit with the same content do not; tick, untick, edit, delete, remove-ticked, reorder and move do (move: both lists); rename with a version from an old overview returns 201; a new list is first with an empty preview and count 0. The test lists were deleted afterwards | | `git diff --stat stage` (step 11.6) | no change to `AbstractEntity.ts`, `AbstractRepository.ts` or any recipe or meal plan file | | Reviewer greps (step 11.7) | as expected: 8 calls of `recordItemChange` in `ShoppingListItemHandler.ts`, 1 call of `insertItemChange` in `ShoppingListHandler.ts`, 2 definitions; no upsert or save in `recordItemChange`; `update(ShoppingListEntity)` only in `renameIfVersion`; one `this.itemRepository` hit (first read of `moveItem`); no list lock in tick, edit, delete, remove-ticked. Grep 3: see judgment call 6 | Not verified: nothing was skipped. The repo defines no linter. ## Notes - #23 touches `docs/architecture.md` and the class comment of `ShoppingListRepository`. Expect text conflicts with whichever merges second; the plan says how to resolve them. - The contract pin (`contract.ref`) is on platform `main`. - Judgment call 10 of the plan (a deadlock between reorder and remove-ticked that spec 001 already has) is not fixed here and has no issue yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
docs(adr) Add shopping list overview ADRs for spec 002
Some checks failed
/ test (pull_request) Failing after 40s
Details
5f5f9e6d1a 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(plan) Add shopping list overview previews plan (spec 002)
Some checks failed
/ test (pull_request) Failing after 40s
Details
5f5c325589 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
chore(contract) Pin the contract for spec 002
Some checks failed
/ test (pull_request) Failing after 41s
Details
/ contract_pin (pull_request) Successful in 2s
Details
8f03157eaa 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test(integration) Add database-backed integration test setup
Some checks failed
/ contract_pin (pull_request) Has been cancelled
Details
/ test (pull_request) Has been cancelled
Details
c35966b494 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
feat(shoppingList) Add shopping_list_item_change table
Some checks failed
/ test (pull_request) Failing after 40s
Details
/ contract_pin (pull_request) Successful in 8s
Details
64b559de2f 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
feat(shoppingList) Record item changes as the last write of each transaction
Some checks failed
/ test (pull_request) Failing after 41s
Details
/ contract_pin (pull_request) Successful in 3s
Details
19998d4d2b 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
feat(shoppingList) Return item previews and order the overview by last item change
All checks were successful
/ test (pull_request) Successful in 1m9s
Details
/ contract_pin (pull_request) Successful in 2s
Details
efca1d8354 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test(shoppingList) Add concurrency and overview integration tests
Some checks failed
/ contract_pin (pull_request) Has been cancelled
Details
/ test (pull_request) Has been cancelled
Details
f4d33751f2 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(architecture) Document the item change time and the overview read
All checks were successful
/ test (pull_request) Successful in 1m8s
Details
/ contract_pin (pull_request) Successful in 2s
Details
783d735114 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
araemer changed title from WIP: feat(shoppingList) Shopping list overview with item previews (spec 002) to feat(shoppingList) Shopping list overview with item previews (spec 002) 2026-10-03 20:25:51 +00:00
refactor(shoppingList) Remove the unused list repository from ShoppingListItemHandler
All checks were successful
/ test (pull_request) Successful in 1m9s
Details
/ contract_pin (pull_request) Successful in 2s
Details
173e8401fa 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(howto) Add testing and locking how-tos and the DataSource naming suffix
Some checks failed
/ test (pull_request) Has been cancelled
Details
/ contract_pin (pull_request) Has been cancelled
Details
afd6c04f18 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
chore(coverage) Remove the coverage report committed by mistake
All checks were successful
/ test (pull_request) Successful in 1m11s
Details
/ contract_pin (pull_request) Successful in 3s
Details
6630bba099 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test(shoppingList) Fix review findings in the tests and ignore the coverage report
All checks were successful
/ test (pull_request) Successful in 1m9s
Details
/ contract_pin (pull_request) Successful in 2s
Details
78cad6bf85 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(architecture) Split the table of contents, shorten the testing section and link the tracking issues
All checks were successful
/ test (pull_request) Successful in 1m9s
Details
/ contract_pin (pull_request) Successful in 2s
Details
367f4f944d 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
araemer reviewed 2026-10-04 05:35:43 +00:00
src/entities/ShoppingListEntity.ts Outdated
@ -14,1 +14,4 @@
* that checks and bumps `version` itself (see ShoppingListRepository).
*
* The time of the last change to the list's items lives in ShoppingListItemChangeEntity,
* on purpose not on this row (docs/adr/2026-10-03-shopping-list-item-change-time.md).
araemer commented 2026-10-04 05:16:01 +00:00
Author
Owner
Copy link

a very brief single sentence why here would be more comfortable than opening the docs to find out.

a very brief single sentence why here would be more comfortable than opening the docs to find out.
araemer marked this conversation as resolved
src/handlers/ShoppingListHandler.ts Outdated
@ -76,0 +114,4 @@
// list, and pushing keeps that order.
const previewsByListId: Map<string, ShoppingListItemPreviewProjection[]> = new Map();
for (const preview of previews) {
const ofList: ShoppingListItemPreviewProjection[] = previewsByListId.get(preview.shopping_list_id) ?? [];
araemer commented 2026-10-04 05:19:40 +00:00
Author
Owner
Copy link

naming I'd prefer previewsOfList

naming I'd prefer previewsOfList
araemer marked this conversation as resolved
docs(shoppingList) Address PR feedback: state why the change time is not on the list row, rename previewsOfList
All checks were successful
/ test (pull_request) Successful in 1m15s
Details
/ contract_pin (pull_request) Successful in 2s
Details
720329e895 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
araemer commented 2026-10-04 05:38:23 +00:00
Author
Owner
Copy link

Review feedback addressed in 720329e:

  • src/entities/ShoppingListEntity.ts (why the change time is not on the list row): the JSDoc now states the reason in place: writing the list row for an item change would bump version and update_date, which cover the name only, and could deadlock with the list row lock. The ADR reference stays. The other places that cite this ADR already state their reason next to the reference.
  • src/handlers/ShoppingListHandler.ts (naming): ofList is renamed to previewsOfList.

npm run build ok, npm test 606 passed, npm run test:integration 45 passed.

🤖 Generated with Claude Code

Review feedback addressed in 720329e: - **`src/entities/ShoppingListEntity.ts`** (why the change time is not on the list row): the JSDoc now states the reason in place: writing the list row for an item change would bump `version` and `update_date`, which cover the name only, and could deadlock with the list row lock. The ADR reference stays. The other places that cite this ADR already state their reason next to the reference. - **`src/handlers/ShoppingListHandler.ts`** (naming): `ofList` is renamed to `previewsOfList`. `npm run build` ok, `npm test` 606 passed, `npm run test:integration` 45 passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Merge remote-tracking branch 'origin/stage' into 002-shopping-list-overview-previews
All checks were successful
/ test (pull_request) Successful in 1m10s
Details
/ contract_pin (pull_request) Successful in 2s
Details
5e6354d58a 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
araemer merged commit 5b8bd32ef1 into stage 2026-10-04 06:14:40 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
araemer/recipe-backend!25
No description provided.