feat(shoppingList) Shopping lists (spec 001) #21
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "001-shopping-lists"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements spec 001 (shopping lists) in recipe-backend.
docs/plans/001-shopping-lists.md@recipe/contract—shopping-lists.ts,errors.ts,users.ts,shared.tsApproved at GATE B
docs/adr/2026-10-02-shopping-list-item-writes.mddocs/adr/2026-10-02-shopping-list-item-order.md(revised: the lock also covers share writes; untick and move re-read the item after locking)docs/adr/2026-10-02-shopping-list-default-storage.mddocs/adr/2026-10-02-single-error-serialization-point.mdApproved at GATE C
docs/plans/001-shopping-lists.mdWhat's in it (by plan step)
HttpErrorcarries a requiredErrorCode(generic default per status, per-status code unions).errorLoggerMiddlewareis the only writer of error bodies ({ error, code }). Direct writes in auth/authorization middleware,AuthRestResource,ImageRestResourceand the unknown-route fallback now throw.errorHandler.tsis deleted.Uuid.parsethrowsValidationError.UserHandlerusesUSER_NOT_FOUND.GET /user/list-shareable: returnsCompactUserDto(no email or role), registered before/:userId.3–4. Five migrations and entities:
shopping_list,_share,_item,_item_reason,_default.ShoppingListTransactionHelper, with list row locks in id order. Item rows are written only viaupdateKeepingVersionorupdateContentIfVersion. The plan's W1–W11 review grep finds only those two update paths.shoppingItemNameUtils: German plural duplicate match.ShoppingListPermissionHelper,ShoppingListHandler,ShoppingListItemHandler.CompactUserMapper.ShoppingListApplicationService: checks the JSON shape of request bodies and returns 400 before any handler call./shopping-list(withGET /allfirst),/shopping-list-item,/shopping-list-share.seqvalues are renumbered, which also fixes the duplicateseq: 8.docs/architecture.md: updated.Deviations from the plan
npm run migrationfixed along the way. It usedtypeorm-ts-node-commonjsonsrc/data-source.ts, which couldn't load the ESM sources, and the data source only finds compiled migrations underbuild/. The script now builds first and runs againstbuild/data-source.js. The migrations for this feature had already been applied by the running dev server on startup.Judgment calls
name(lists and items) with 400. Otherwise the handler's.trim()would crash and answer 500.versionto be a positive integer, the same check as edit and change-permission. A non-number would otherwise reach the SQL and answer 500.nulloptional fields.amount,unitandcommentsent asnullcount as absent instead of being rejected.loadList,loadListWithPermissionandlockListOrNotFoundlive insrc/handlers/shoppingListHandlerUtils.tsand are used by both handlers. Permission resolution itself stays inShoppingListPermissionHelper, which stays pure as the plan asked.CLAUDE.md:ShoppingListAccessPolicy→ShoppingListPermissionHelper,ShoppingListTransactionRunner→ShoppingListTransactionHelper,ShoppingItemNameMatcher→shoppingItemNameUtils, and the shared loading helpers →shoppingListHandlerUtils. The plan keeps its original names and has a rename note.allowDuplicate.addItemreads the open and ticked items only when the duplicate check runs.Verification
npm run build(contract and backend) andnpm test: 557 of 557 passed (366 before).DUPLICATEon "möhren" against "Möhre".tickedBystays the first user andversiondoesn't change.CONFLICT, and an edit withoutversionanswers 400.TARGET_SHOPPING_LIST_ACCESS_DENIED, and to an unknown list 404TARGET_SHOPPING_LIST_NOT_FOUND.defaultListIdisnulland the list answers 403.{ error, code }.git diff --stat: no changes toAbstractRepository, the recipe or meal plan repositories, handlers or services.Also adds a naming rule to
CLAUDE.md: class and module names end in a purpose suffix.Related: #20 (the
AbstractRepository.update()race, out of scope here)🤖 Generated with Claude Code
WIP: feat(shoppingList) Shopping lists (spec 001)to feat(shoppingList) Shopping lists (spec 001)@ -0,0 +18,4 @@// -------------------------------------------------------------------------describe("defaults", () => {const cases: Array<{ name: string; error: HttpError; status: number; code: ErrorCode }> = [{ name: "ValidationError", error: new ValidationError("bad"), status: 400, code: ErrorCode.VALIDATION_FAILED },should we be using the httpStatusCode enum here instead of plain numbers?
@ -0,0 +50,4 @@*/export class ShoppingListHandler {constructor(private readonly lists: ShoppingListRepository,this naming isn't good. instead of lists and items use listRepository and itemRepository to make the role clear. And no two letter names like tx either. They only cause confusion
@ -0,0 +94,4 @@if (!list) throw new NotFoundError(LIST_NOT_FOUND);// Any access level may readconst permission: ShoppingListEffectivePermission = this.permissionHelper.resolve(list, userId);but what about no access level at all? shouldn't it return null? I don't see any treatment for that case.
@ -0,0 +73,4 @@const openIds: Set<string> = new Set(open.map((item: ShoppingListItemEntity): string => item.id!));// Named open items first, in request order; a Set keeps the first occurrence onlyconst named: Set<string> = new Set(requested.filter((id: string): boolean => openIds.has(id)));I had to read this several time to understand openIds and named. Open seems to be unticked - worth a comment I think. and named seems to be open items mentioned in the request. We need to improve the names here.
@ -0,0 +96,4 @@*/export class ShoppingListItemHandler {constructor(private readonly lists: ShoppingListRepository,meaningful names please... listRepository, itemRepository, transaction or transactionHelper...
@ -0,0 +277,4 @@const locked: Set<Uuid> = await lists.lockLists([sourceListId, targetListId]);// Re-read under the lock; if another move changed its list, lock that one nextconst item: ShoppingListItemEntity = await this.findItemOrNotFound(items, itemId);Hm, is this correct? If another move already changed the item's list, should we silently move it again or create a user-facing message "The item you are requesting has already moved to list A/another list (if the user doesn't have access to the traget list)
@ -0,0 +476,4 @@}/** Trims an optional text; empty or absent becomes null. */private trimToNull(value: string | null | undefined): string | null {Wouldn't trimOrNull be better?
@ -0,0 +6,4 @@* Raw user columns as a SQL projection returns them, e.g. the owner columns of the* shopping list overview query.*/export interface CompactUserColumns {why columns and not projection here?
@ -0,0 +9,4 @@* One-way: items are changed through dedicated request types (add, edit, tick, …), never* by sending an item DTO back, so the reverse direction does not exist.** Optional values are absent keys, not null, as the contract says: `amount`, `unit` andI don't understand that comment
@ -25,3 +36,3 @@dto.lastName = entity.lastName;dto.role = entity.role;dto.avatarUrl = entity.avatarKey ? `${IMAGE_BASE_ROUTE}/${entity.avatarKey}` : null;dto.avatarUrl = avatarUrlFor(entity.avatarKey);The method name is strange. I'd prefer avatarUrlByKey
@ -0,0 +147,4 @@// ---------------------------------------------------------------------------/*** W1 — inserts a new, open item. The database sets `version` to 1.why w1, w2 and so on. Is it a reference to the plan? I'd remove the cryptic w somethings.
@ -0,0 +72,4 @@.createQueryBuilder(ShoppingListEntity, LIST_ALIAS).select(`${LIST_ALIAS}.id`).where(`${LIST_ALIAS}.id = :id`, { id }).setLock("pessimistic_write")I didn't find an ulock. Does typeORM handle that on it's own after write, did i miss it or did we forget it?
@ -0,0 +6,4 @@/** Repositories bound to one transaction's EntityManager. */export interface ShoppingListTransactionRepositories {/** Lists, shares and defaults, on the transaction's manager. */lists: ShoppingListRepository;Names... listRepository, itemRepository
Review feedback addressed in
7c298c7. One entry per comment:ShoppingListRepository.ts:75): nothing was missing. PostgreSQL releasesSELECT … FOR UPDATElocks when the transaction commits or rolls back, and every lock runs insideShoppingListTransactionHelper.run(). ThelockListsJSDoc now says so.lists/items/tx(three comments): renamed tolistRepository,itemRepositoryandtransactionHelper, including theShoppingListTransactionRepositoriesfields. Transaction callbacks take one namedtransactionRepositoriesparameter, so calls bound to the transaction read differently fromthis.listRepository. Also fixed:em→entityManager, and the lambda parameterss→shareandr→reason.HttpStatusCodein tests: all 7 new test files now use the enum instead of literal status numbers.CompactUserFields/fromFields(). The JSDoc explains why it is not a projection: no query returns this shape, and callers fill it from their own projection's columns.getList: already handled.resolve()throws 403FORBIDDEN, which the contract requires and the app relies on. The misleading line comment is fixed.computeReordernames: nowopenItems,requestedItemIds,openItemIds,requestedOpenItemIdsandremainingOpenItemIds, with "open = not ticked" spelled out.trimToNull→trimOrNull: done.CompactUserMapperwere reworded too.avatarUrlFor→avatarUrlByKey: done, includingdocs/architecture.md.Verification:
npm run buildandnpm test: 554 of 554 pass.🤖 Generated with Claude Code