diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index bb959e4cf..7d40965dd 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -20,11 +20,12 @@ function parseId(value: string | string[]): number | null { } /** - * Packing MCP surface — ported 1:1 from the legacy registrars: the seventeen + * Packing MCP surface — ported from the legacy registrars: the seventeen * tools from src/mcp/tools/packing.ts and the trek://trips/{tripId}/packing + * trek://trips/{tripId}/packing/bags resources from src/mcp/resources.ts - * (identical names, descriptions, schemas, annotations, error/payload shapes - * and broadcasts). The registration-time gates map to `when` (the + * (identical names, descriptions, schemas, annotations and error/payload + * shapes; the one deviation is the packing:bag-deleted payload, aligned with + * the REST route). The registration-time gates map to `when` (the * whole-registrar packing-addon early return) plus the declarative packing * read/write access markers (the legacy `if (R)` / `if (W)` checks, resolved * by trekMcpAccessPolicy). The two template-management tools keep their inline @@ -232,7 +233,9 @@ export class PackingMcp { if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); this.packing.deleteBag(tripId, bagId); - safeBroadcast(tripId, 'packing:bag-deleted', { id: bagId }); + // { bagId } matches the REST route and the plugin host (the legacy + // registrar's { id } was the odd one out). + safeBroadcast(tripId, 'packing:bag-deleted', { bagId }); return ok({ success: true }); } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 1817b1b57..7de9c2c88 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -27,13 +27,15 @@ interface ImportItem { const BAG_COLORS = ['#6366f1', '#ec4899', '#f97316', '#10b981', '#06b6d4', '#8b5cf6', '#ef4444', '#f59e0b', '#3b82f6', '#84cc16', '#d946ef', '#14b8a6', '#f43f5e', '#a855f7', '#eab308', '#64748b']; /** - * Packing domain service — owns the packing SQL (moved 1:1 from the legacy - * services/packingService.ts: identical statements, the `||` falsy-coercion - * defaults, the bodyKeys sentinel protocol on the updates, the #858 three-tier - * sharing model and the post-write re-selects). Trip access, the - * 'packing_edit' permission and the WebSocket broadcast keep their legacy call - * paths. The remaining non-Nest consumer (the legacy MCP prompts registrar) - * goes through packing.bridge.ts instead of importing this class directly. + * Packing domain service — owns the packing SQL (moved from the legacy + * services/packingService.ts: the bodyKeys sentinel protocol on the updates, + * the #858 three-tier sharing model and the post-write re-selects). Trip + * access, the 'packing_edit' permission and the WebSocket broadcast keep their + * legacy call paths. Post-migration fixes over the legacy code: a single + * 'Other' category default, bodyKeys-gated weight_limit_grams (explicit null + * clears it), and transactions around every multi-statement write. The + * remaining non-Nest consumer (the legacy MCP prompts registrar) goes through + * packing.bridge.ts instead of importing this class directly. */ @Injectable() export class PackingService { @@ -126,8 +128,10 @@ export class PackingService { listItems(tripId: string | number, userId?: number) { // Three-tier visibility (#858): Common (is_private=0) is visible to everyone; // Personal/Shared (is_private=1) only to the owner (bringer) and the recipients - // it was explicitly shared with. Without a userId (internal callers such as - // trip export) the unfiltered list is returned for back-compat. + // it was explicitly shared with. Without a userId the unfiltered list is + // returned — every current caller (trip summary, offline bundle, prompts, + // resources, plugin host) passes the viewer; omit it only for genuinely + // viewer-less internal reads. let rows: any[]; if (userId == null) { rows = this.db.all( @@ -172,7 +176,7 @@ export class PackingService { const itemId = this.db.transaction(() => { const result = this.db.run( 'INSERT INTO packing_items (trip_id, name, checked, category, sort_order, quantity, is_private, owner_id, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)', - tripId, data.name, data.checked ? 1 : 0, data.category || 'Allgemein', sortOrder, qty, isPrivate, ownerId ?? null + tripId, data.name, data.checked ? 1 : 0, data.category || 'Other', sortOrder, qty, isPrivate, ownerId ?? null ); const id = Number(result.lastInsertRowid); // "Shared with specific people" — record the recipients it covers. @@ -229,7 +233,7 @@ export class PackingService { bodyKeys.includes('bag_id') ? 1 : 0, data.bag_id ?? null, bodyKeys.includes('quantity') ? 1 : 0, - data.quantity ? Math.max(1, Math.min(999, Number(data.quantity))) : 1, + Math.max(1, Math.min(999, Number(data.quantity) || 1)), bodyKeys.includes('is_private') ? 1 : 0, data.is_private ? 1 : 0, claimOwner ? 1 : 0, @@ -382,11 +386,13 @@ export class PackingService { setBagMembers(tripId: string | number, bagId: string | number, userIds: number[]) { const bag = this.db.get('SELECT * FROM packing_bags WHERE id = ? AND trip_id = ?', bagId, tripId); if (!bag) return null; - this.db.run('DELETE FROM packing_bag_members WHERE bag_id = ?', bagId); - const ins = this.db.prepare('INSERT OR IGNORE INTO packing_bag_members (bag_id, user_id) VALUES (?, ?)'); - // Only real trip members may be bag members — never write an arbitrary account id. - const roster = this.tripRosterIds(tripId); - for (const uid of userIds) if (roster.has(uid)) ins.run(bagId, uid); + this.db.transaction(() => { + this.db.run('DELETE FROM packing_bag_members WHERE bag_id = ?', bagId); + const ins = this.db.prepare('INSERT OR IGNORE INTO packing_bag_members (bag_id, user_id) VALUES (?, ?)'); + // Only real trip members may be bag members — never write an arbitrary account id. + const roster = this.tripRosterIds(tripId); + for (const uid of userIds) if (roster.has(uid)) ins.run(bagId, uid); + }); const rows = this.db.all<{ user_id: number; username: string; avatar: string | null }>(` SELECT bm.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar FROM packing_bag_members bm JOIN users u ON bm.user_id = u.id @@ -414,15 +420,18 @@ export class PackingService { // A bag may only be assigned to a real trip member; an off-roster id becomes unassigned. const assignUser = data.user_id != null && this.tripRosterIds(tripId).has(data.user_id) ? data.user_id : null; + // weight_limit_grams follows the bodyKeys presence protocol like user_id: + // an omitted key leaves the limit unchanged, an explicit null clears it. this.db.run(`UPDATE packing_bags SET name = COALESCE(?, name), color = COALESCE(?, color), - weight_limit_grams = ?, + weight_limit_grams = CASE WHEN ? THEN ? ELSE weight_limit_grams END, user_id = CASE WHEN ? THEN ? ELSE user_id END WHERE id = ?`, data.name?.trim() || null, data.color || null, - data.weight_limit_grams ?? (bag as any).weight_limit_grams ?? null, + bodyKeys?.includes('weight_limit_grams') ? 1 : 0, + data.weight_limit_grams ?? null, bodyKeys?.includes('user_id') ? 1 : 0, assignUser, bagId @@ -479,11 +488,13 @@ export class PackingService { const insert = this.db.prepare('INSERT INTO packing_items (trip_id, name, checked, category, sort_order, is_private, owner_id, updated_at) VALUES (?, ?, 0, ?, ?, ?, ?, CURRENT_TIMESTAMP)'); const added: any[] = []; - for (const ti of templateItems) { - const result = insert.run(tripId, ti.name, ti.category, sortOrder++, isPrivate, owner); - const item = this.db.get('SELECT * FROM packing_items WHERE id = ?', result.lastInsertRowid); - added.push(item); - } + this.db.transaction(() => { + for (const ti of templateItems) { + const result = insert.run(tripId, ti.name, ti.category, sortOrder++, isPrivate, owner); + const item = this.db.get('SELECT * FROM packing_items WHERE id = ?', result.lastInsertRowid); + added.push(item); + } + }); return added; } @@ -497,24 +508,27 @@ export class PackingService { if (items.length === 0) return null; - const result = this.db.run('INSERT INTO packing_templates (name, created_by) VALUES (?, ?)', templateName, userId); - const templateId = result.lastInsertRowid; - const categories = [...new Set(items.map(i => i.category || 'Other'))]; - const catIdMap = new Map(); - for (let i = 0; i < categories.length; i++) { - const catResult = this.db.run('INSERT INTO packing_template_categories (template_id, name, sort_order) VALUES (?, ?, ?)', templateId, categories[i], i); - catIdMap.set(categories[i], catResult.lastInsertRowid); - } + const templateId = this.db.transaction(() => { + const result = this.db.run('INSERT INTO packing_templates (name, created_by) VALUES (?, ?)', templateName, userId); + const id = result.lastInsertRowid; - const itemsByCategory = new Map(); - for (const item of items) { - const catId = catIdMap.get(item.category || 'Other')!; - const order = itemsByCategory.get(item.category || 'Other') || 0; - this.db.run('INSERT INTO packing_template_items (category_id, name, sort_order) VALUES (?, ?, ?)', catId, item.name, order); - itemsByCategory.set(item.category || 'Other', order + 1); - } + const catIdMap = new Map(); + for (let i = 0; i < categories.length; i++) { + const catResult = this.db.run('INSERT INTO packing_template_categories (template_id, name, sort_order) VALUES (?, ?, ?)', id, categories[i], i); + catIdMap.set(categories[i], catResult.lastInsertRowid); + } + + const itemsByCategory = new Map(); + for (const item of items) { + const catId = catIdMap.get(item.category || 'Other')!; + const order = itemsByCategory.get(item.category || 'Other') || 0; + this.db.run('INSERT INTO packing_template_items (category_id, name, sort_order) VALUES (?, ?, ?)', catId, item.name, order); + itemsByCategory.set(item.category || 'Other', order + 1); + } + return id; + }); return { id: Number(templateId), name: templateName, categoryCount: categories.length, itemCount: items.length }; } @@ -540,12 +554,14 @@ export class PackingService { } updateCategoryAssignees(tripId: string | number, categoryName: string, userIds: number[] | undefined) { - this.db.run('DELETE FROM packing_category_assignees WHERE trip_id = ? AND category_name = ?', tripId, categoryName); + this.db.transaction(() => { + this.db.run('DELETE FROM packing_category_assignees WHERE trip_id = ? AND category_name = ?', tripId, categoryName); - if (Array.isArray(userIds) && userIds.length > 0) { - const insert = this.db.prepare('INSERT OR IGNORE INTO packing_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)'); - for (const uid of userIds) insert.run(tripId, categoryName, uid); - } + if (Array.isArray(userIds) && userIds.length > 0) { + const insert = this.db.prepare('INSERT OR IGNORE INTO packing_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)'); + for (const uid of userIds) insert.run(tripId, categoryName, uid); + } + }); const updated = this.db.all<{ user_id: number; username: string; avatar: string | null }>(` SELECT pca.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar diff --git a/server/tests/e2e/packing.e2e.test.ts b/server/tests/e2e/packing.e2e.test.ts index 686cb2488..ec6346301 100644 --- a/server/tests/e2e/packing.e2e.test.ts +++ b/server/tests/e2e/packing.e2e.test.ts @@ -195,8 +195,8 @@ describe('Packing e2e (real auth guard + real SQL over temp SQLite)', () => { insertItem(tripId, 'Existing', { sort_order: 4 }); const res = await request(server).post(`/api/trips/${tripId}/packing`).set('Cookie', sessionCookie(1)).send({ name: 'Socks' }); expect(res.status).toBe(201); - // 'Allgemein' is the legacy createItem category default; quantity clamps to 1. - expect(res.body.item).toMatchObject({ name: 'Socks', checked: 0, category: 'Allgemein', quantity: 1, sort_order: 5, owner_id: 1 }); + // 'Other' is the unified category default (shared with bulkImport/saveAsTemplate). + expect(res.body.item).toMatchObject({ name: 'Socks', checked: 0, category: 'Other', quantity: 1, sort_order: 5, owner_id: 1 }); expect(db.prepare('SELECT name FROM packing_items WHERE id = ?').get(res.body.item.id)).toEqual({ name: 'Socks' }); }); @@ -321,6 +321,14 @@ describe('Packing e2e (real auth guard + real SQL over temp SQLite)', () => { expect(updated.status).toBe(200); expect(updated.body.bag).toMatchObject({ name: 'Duffel', color: '#ff0000' }); + // weight_limit_grams follows the bodyKeys protocol: set, keep when omitted, clear with null. + const limited = await request(server).put(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)).send({ weight_limit_grams: 8000 }); + expect(limited.body.bag.weight_limit_grams).toBe(8000); + const kept = await request(server).put(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)).send({ name: 'Duffel XL' }); + expect(kept.body.bag.weight_limit_grams).toBe(8000); + const cleared = await request(server).put(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)).send({ weight_limit_grams: null }); + expect(cleared.body.bag.weight_limit_grams).toBeNull(); + const deleted = await request(server).delete(`/api/trips/${tripId}/packing/bags/${bagId}`).set('Cookie', sessionCookie(1)); expect(deleted.status).toBe(200); expect(deleted.body).toEqual({ success: true }); diff --git a/server/tests/unit/mcp/tools-packing-advanced.test.ts b/server/tests/unit/mcp/tools-packing-advanced.test.ts index 7a2a8132a..bbe52e086 100644 --- a/server/tests/unit/mcp/tools-packing-advanced.test.ts +++ b/server/tests/unit/mcp/tools-packing-advanced.test.ts @@ -235,7 +235,8 @@ describe('Tool: delete_packing_bag', () => { }); const data = parseToolResult(result) as any; expect(data.success).toBe(true); - expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:bag-deleted', expect.any(Object)); + // { bagId } — aligned with the REST route and the plugin host. + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:bag-deleted', expect.objectContaining({ bagId })); expect(testDb.prepare('SELECT id FROM packing_bags WHERE id = ?').get(bagId)).toBeUndefined(); }); }); diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index 35e4a4a59..91c528dc6 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -1,10 +1,12 @@ /** * Unit tests for the DI-native PackingService — PACK-SVC-001 through - * PACK-SVC-049 moved 1:1 from the legacy tests/unit/services/packingService.test.ts + * PACK-SVC-049 moved from the legacy tests/unit/services/packingService.test.ts * (real in-memory SQLite so the SQL logic is exercised faithfully), plus the * wrapper-helper cases (canEdit, the #858 broadcast scoping, getItemPrivacy, * notifyTagged) carried over from the old delegation suite — now running - * against real SQL — and PACK-SVC-050 pinning the packing.bridge delegation. + * against real SQL — PACK-SVC-050 pinning the packing.bridge delegation, and + * PACK-SVC-051..053 pinning the post-migration fixes over the legacy quirks + * ('Other' category default, bodyKeys-gated weight_limit_grams, quantity clamp). */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -576,6 +578,45 @@ describe('three-tier packing sharing (#858)', () => { }); }); +// ── Post-migration fixes over the legacy quirks ─────────────────────────────── + +describe('legacy-quirk fixes', () => { + it('PACK-SVC-051: createItem defaults the category to "Other" (unified with bulkImport)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const item = svc.createItem(trip.id, { name: 'Socks' }, user.id) as any; + expect(item.category).toBe('Other'); + }); + + it('PACK-SVC-052: updateBag gates weight_limit_grams on bodyKeys — omitted keeps, explicit null clears', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const bag = svc.createBag(trip.id, { name: 'Duffel' }) as any; + + const limited = svc.updateBag(trip.id, bag.id, { weight_limit_grams: 8000 }, ['weight_limit_grams']) as any; + expect(limited.weight_limit_grams).toBe(8000); + + // A rename without the key must not touch the limit. + const kept = svc.updateBag(trip.id, bag.id, { name: 'Duffel XL' }, ['name']) as any; + expect(kept.weight_limit_grams).toBe(8000); + + // An explicit null clears it. + const cleared = svc.updateBag(trip.id, bag.id, { weight_limit_grams: null }, ['weight_limit_grams']) as any; + expect(cleared.weight_limit_grams).toBeNull(); + }); + + it('PACK-SVC-053: updateItem clamps a provided quantity into 1..999', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const item = svc.createItem(trip.id, { name: 'Socks', quantity: 5 }, user.id) as any; + + expect((svc.updateItem(trip.id, item.id, { quantity: 0 }, ['quantity']) as any).quantity).toBe(1); + expect((svc.updateItem(trip.id, item.id, { quantity: 9999 }, ['quantity']) as any).quantity).toBe(999); + // Omitted key leaves the quantity unchanged. + expect((svc.updateItem(trip.id, item.id, { name: 'Wool socks' }, ['name']) as any).quantity).toBe(999); + }); +}); + // ── Wrapper helpers (carried over from the old delegation suite) ────────────── describe('canEdit', () => {