diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index 9e3d9bdd3..be2dc90df 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -13,7 +13,7 @@ import { } from '@nestjs/common'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import type { User } from '../../types'; -import { PackingService } from './packing.service'; +import { PackingService, isInvalidBagRef } from './packing.service'; import { isUpdateConflict } from '../common/conflictResult'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; @@ -97,7 +97,12 @@ export class PackingController { @Headers('x-socket-id') socketId?: string, ) { // checked arrives as boolean or legacy 0/1 — the service coerces by truthiness. - const item = this.packing.createItem(tripId, { name: body.name, category: body.category, checked: body.checked === undefined ? undefined : !!body.checked, is_private: body.is_private, visibility: body.visibility, recipient_ids: body.recipient_ids }, user.id); + const item = this.packing.createItem(tripId, { name: body.name, category: body.category, checked: body.checked === undefined ? undefined : !!body.checked, weight_grams: body.weight_grams, bag_id: body.bag_id, quantity: body.quantity, is_private: body.is_private, visibility: body.visibility, recipient_ids: body.recipient_ids }, user.id); + // A bag referenced in the body must exist on this trip (#2154). The payload + // is at fault, so 400 — the 404 'Bag not found' stays with the path routes. + if (isInvalidBagRef(item)) { + throw new HttpException({ error: 'Bag not found' }, 400); + } this.packing.emitToViewers(tripId, 'packing:created', { item }, item, socketId); return { item }; } @@ -139,6 +144,10 @@ export class PackingController { if (isUpdateConflict(updated)) { throw new HttpException({ error: 'conflict', server: updated.server }, 409); } + // A bag referenced in the body must exist on this trip (#2154) — see create. + if (isInvalidBagRef(updated)) { + throw new HttpException({ error: 'Bag not found' }, 400); + } this.packing.broadcastUpdate(tripId, id, updated as PackingItemRow, !!before?.is_private, socketId); return { item: updated }; } @@ -254,7 +263,7 @@ export class PackingController { if (!body.name.trim()) { throw new HttpException({ error: 'Name is required' }, 400); } - const bag = this.packing.createBag(tripId, { name: body.name, color: body.color }); + const bag = this.packing.createBag(tripId, { name: body.name, color: body.color, weight_limit_grams: body.weight_limit_grams }); this.packing.broadcast(tripId, 'packing:bag-created', { bag }, socketId); return { bag }; } diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index 3c2f0cc3b..742f61207 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -9,8 +9,9 @@ import { z } from 'zod'; import { AuthService } from '../auth/auth.service'; import { ADDON_IDS } from '../../addons'; import { noAccess, permissionDenied, adminRequired } from '../../mcp/tools/_shared'; -import { PackingService } from './packing.service'; +import { PackingService, isInvalidBagRef } from './packing.service'; import { + packingCreateBagRequestSchema, packingCreateItemRequestSchema, packingSetSharingRequestSchema, packingUpdateBagRequestSchema, @@ -62,6 +63,9 @@ export class PackingMcp { tripId: z.number().int().positive(), name: z.string().min(1).max(200), category: z.string().max(100).optional().describe('Packing category (e.g. Clothes, Electronics)'), + bag_id: packingCreateItemRequestSchema.shape.bag_id.describe('Bag to pack the item into (ids come from list_packing_bags)'), + quantity: packingCreateItemRequestSchema.shape.quantity.describe('How many to pack, clamped to 1-999'), + weight_grams: packingCreateItemRequestSchema.shape.weight_grams.describe('Weight in grams, which feeds the bag fill bar'), checked: packingCreateItemRequestSchema.shape.checked.describe('Create the item already ticked off'), is_private: packingCreateItemRequestSchema.shape.is_private.describe('Keep the item to yourself; visibility says the same thing with more nuance'), visibility: packingCreateItemRequestSchema.shape.visibility.describe("Which list the item belongs to: 'common' (the group pool, the default), 'personal' (yours alone), or 'shared' (yours plus recipient_ids)"), @@ -72,7 +76,7 @@ export class PackingMcp { access: { group: 'packing', mode: 'write' }, }) async createPackingItem( - { tripId, name, category, checked, is_private, visibility, recipient_ids }: { tripId: number; name: string; category?: string; checked?: boolean | number; is_private?: boolean; visibility?: PackingVisibility; recipient_ids?: number[] }, + { tripId, name, category, bag_id, quantity, weight_grams, checked, is_private, visibility, recipient_ids }: { tripId: number; name: string; category?: string; bag_id?: number | null; quantity?: number; weight_grams?: number | null; checked?: boolean | number; is_private?: boolean; visibility?: PackingVisibility; recipient_ids?: number[] }, ctx: McpContext, ) { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); @@ -81,12 +85,17 @@ export class PackingMcp { const item = this.packing.createItem(tripId, { name, category: category || 'General', + bag_id, + quantity, + weight_grams, // checked takes a boolean or the legacy 0/1, exactly as the REST body does. checked: checked === undefined ? undefined : !!checked, is_private, visibility, recipient_ids, }, ctx.userId); + // A referenced bag must exist on this trip (#2154), as on the REST route. + if (isInvalidBagRef(item)) return errorResult('Bag not found.'); // A restricted item (#858) reaches its owner and recipients only; a Common // one answers null here and goes to the whole room. this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); @@ -176,6 +185,8 @@ export class PackingMcp { const wasPrivate = !!this.packing.getItemPrivacy(tripId, itemId)?.is_private; const item = this.packing.updateItem(tripId, itemId, fields, bodyKeys, undefined, ctx.userId); if (!item) return errorResult('Packing item not found.'); + // A referenced bag must exist on this trip (#2154), as on the REST route. + if (isInvalidBagRef(item)) return errorResult('Bag not found.'); this.broadcastItemUpdate(tripId, itemId, item, wasPrivate); return ok({ item }); } @@ -274,18 +285,19 @@ export class PackingMcp { tripId: z.number().int().positive(), name: z.string().min(1).max(100), color: z.string().optional(), + weight_limit_grams: packingCreateBagRequestSchema.shape.weight_limit_grams.describe('Allowance in grams the bag is measured against (the fill bar)'), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, when: packingAddonOn, access: { group: 'packing', mode: 'write' }, }) - async createPackingBag({ tripId, name, color }: { tripId: number; name: string; color?: string }, ctx: McpContext) { + async createPackingBag({ tripId, name, color, weight_limit_grams }: { tripId: number; name: string; color?: string; weight_limit_grams?: number | null }, ctx: McpContext) { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); // createBag returns a bare row; hydrate with the empty members array that // listBags and the schema always carry, so the client/AI consumer matches. - const bag = { ...(this.packing.createBag(tripId, { name, color }) as object), members: [] }; + const bag = { ...(this.packing.createBag(tripId, { name, color, weight_limit_grams }) as object), members: [] }; this.guards.safeBroadcast(tripId, 'packing:bag-created', { bag }); return ok({ bag }); } diff --git a/server/src/nest/packing/packing.rpc.ts b/server/src/nest/packing/packing.rpc.ts index aef080f71..c59a08068 100644 --- a/server/src/nest/packing/packing.rpc.ts +++ b/server/src/nest/packing/packing.rpc.ts @@ -5,7 +5,7 @@ import { BadParams, ForbiddenResource } from '../plugins/host/rpc-errors'; import { asPayload, num, schemaMessage } from '../plugins/host/rpc-params'; import type { PluginRpcContext } from '../plugins/host/rpc-kit/types'; import { RealtimeService } from '../realtime/realtime.service'; -import { PackingService } from './packing.service'; +import { PackingService, isInvalidBagRef } from './packing.service'; import { isUpdateConflict } from '../common/conflictResult'; /** Packing rides on the app's own 'packing_edit' permission, exactly like the REST path. */ @@ -48,6 +48,8 @@ export class PackingRpc { if (!parsed.success) throw new BadParams(`invalid packing item: ${schemaMessage(parsed.error)}`); this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); const item = this.packing.createItem(String(tripId), parsed.data as never, actor) as PrivacyItem; + // A referenced bag must exist on this trip (#2154), as on the REST route. + if (isInvalidBagRef(item)) throw new BadParams(`no packing bag ${parsed.data.bag_id} on trip ${tripId}`); this.packing.emitToViewers(String(tripId), 'packing:created', { item }, item, undefined); return item; } @@ -66,6 +68,8 @@ export class PackingRpc { const updated = this.packing.updateItem(String(tripId), String(itemId), input as never, Object.keys(input), undefined, actor); if (!updated) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); if (isUpdateConflict(updated)) throw new BadParams('packing item was modified concurrently'); + // A referenced bag must exist on this trip (#2154), as on the REST route. + if (isInvalidBagRef(updated)) throw new BadParams(`no packing bag ${parsed.data.bag_id} on trip ${tripId}`); this.packing.broadcastUpdate(String(tripId), itemId, updated as PrivacyItem, !!before?.is_private, undefined); return updated; } @@ -96,7 +100,11 @@ export class PackingRpc { const input = asPayload(params.input); if (typeof input.name !== 'string' || input.name.trim() === '') throw new BadParams('bag name is required'); this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); - const bag = this.packing.createBag(String(tripId), { name: input.name, color: typeof input.color === 'string' ? input.color : undefined }); + const bag = this.packing.createBag(String(tripId), { + name: input.name, + color: typeof input.color === 'string' ? input.color : undefined, + weight_limit_grams: typeof input.weight_limit_grams === 'number' ? input.weight_limit_grams : undefined, + }); this.realtime.broadcast(tripId, 'packing:bag-created', { bag }, undefined); return bag; } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 502d6bb7c..006c16073 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -11,6 +11,21 @@ import { NotificationsService } from '../notifications/notifications.service'; /** Privacy fields stamped on a packing item (#858). */ type PrivacyFields = { is_private?: number; owner_id?: number | null }; +/** + * Rejection sentinel for a body-referenced bag that does not exist on the trip + * (#2154). The packing_bags FK only guarantees the id exists somewhere: a + * cross-trip bag_id used to be accepted silently and a dead one surfaced as an + * SQLite FK error. Returned by createItem/updateItem so REST, MCP and the + * plugin RPC all map it to their surface's 400/BadParams. + */ +export interface InvalidBagRef { + invalidBag: true; +} + +export function isInvalidBagRef(result: unknown): result is InvalidBagRef { + return !!result && typeof result === 'object' && (result as { invalidBag?: unknown }).invalidBag === true; +} + type Trip = TripAccess; export type PackingVisibility = 'common' | 'personal' | 'shared'; @@ -217,6 +232,7 @@ export class PackingService { data: { name: string; category?: string; checked?: boolean; quantity?: number; weight_grams?: number | null; bag_id?: number | null; is_private?: boolean; visibility?: PackingVisibility; recipient_ids?: number[] }, ownerId?: number, ) { + if (data.bag_id != null && !this.bagInTrip(tripId, data.bag_id)) return { invalidBag: true } as const; const maxOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM packing_items WHERE trip_id = ?', tripId)!; const sortOrder = (maxOrder.max !== null ? maxOrder.max : -1) + 1; const qty = Math.max(1, Math.min(999, Number(data.quantity) || 1)); @@ -259,6 +275,12 @@ export class PackingService { return { conflict: true, server: this.db.get('SELECT * FROM packing_items WHERE id = ?', id) }; } + // A non-null bag about to be bound must belong to this trip (#2154) — the + // FK alone let any member point an item at another trip's bag. + if (bodyKeys.includes('bag_id') && data.bag_id != null && !this.bagInTrip(tripId, data.bag_id)) { + return { invalidBag: true } as const; + } + // Privatizing an unowned (legacy) item stamps the acting user as its owner so // the visibility filter still has someone to match (#858). const claimOwner = bodyKeys.includes('is_private') && !!data.is_private && item.owner_id == null && actingUserId != null; @@ -376,6 +398,12 @@ export class PackingService { return this.enrichItems([this.db.get('SELECT * FROM packing_items WHERE id = ?', id)])[0]; } + /** True when the bag exists AND belongs to the trip — the referenced-id rule + * the FK cannot enforce (it only checks existence). */ + private bagInTrip(tripId: string | number, bagId: number): boolean { + return !!this.db.get('SELECT id FROM packing_bags WHERE id = ? AND trip_id = ?', bagId, tripId); + } + /** * A copy keeps the original's bag only when that bag is the caller's to pack: one nobody * owns, or one they belong to. Inheriting someone else's bag would drop the copy into @@ -509,10 +537,10 @@ export class PackingService { return rows.map(m => ({ ...m, avatar: avatarUrl(m) })); } - createBag(tripId: string | number, data: { name: string; color?: string }) { + createBag(tripId: string | number, data: { name: string; color?: string; weight_limit_grams?: number | null }) { const maxOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM packing_bags WHERE trip_id = ?', tripId)!; - const result = this.db.run('INSERT INTO packing_bags (trip_id, name, color, sort_order) VALUES (?, ?, ?, ?)', - tripId, data.name.trim(), data.color || '#6366f1', (maxOrder.max ?? -1) + 1 + const result = this.db.run('INSERT INTO packing_bags (trip_id, name, color, sort_order, weight_limit_grams) VALUES (?, ?, ?, ?, ?)', + tripId, data.name.trim(), data.color || '#6366f1', (maxOrder.max ?? -1) + 1, data.weight_limit_grams ?? null ); return this.db.get('SELECT * FROM packing_bags WHERE id = ?', result.lastInsertRowid); } diff --git a/server/tests/e2e/packing.e2e.test.ts b/server/tests/e2e/packing.e2e.test.ts index 1f9539438..a754a7cc7 100644 --- a/server/tests/e2e/packing.e2e.test.ts +++ b/server/tests/e2e/packing.e2e.test.ts @@ -207,6 +207,47 @@ describe('Packing e2e (real auth guard + real SQL over temp SQLite)', () => { expect(db.prepare('SELECT name FROM packing_items WHERE id = ?').get(res.body.item.id)).toEqual({ name: 'Socks' }); }); + it('201 on create persists weight_grams, bag_id and quantity (#2154)', async () => { + const bag = await request(server).post(`/api/trips/${tripId}/packing/bags`).set('Cookie', sessionCookie(1)).send({ name: 'Carry-On' }); + const res = await request(server) + .post(`/api/trips/${tripId}/packing`) + .set('Cookie', sessionCookie(1)) + .send({ name: 'Tent', weight_grams: 250, bag_id: bag.body.bag.id, quantity: 3 }); + expect(res.status).toBe(201); + expect(res.body.item).toMatchObject({ name: 'Tent', weight_grams: 250, bag_id: bag.body.bag.id, quantity: 3 }); + expect(db.prepare('SELECT weight_grams, bag_id, quantity FROM packing_items WHERE id = ?').get(res.body.item.id)) + .toEqual({ weight_grams: 250, bag_id: bag.body.bag.id, quantity: 3 }); + }); + + it('400 "Bag not found" on create for a bag off the trip; camelCase keys still strip (#2154)', async () => { + // A REAL bag on a different trip — existence alone must not be enough. + const otherTripId = Number(db.prepare('INSERT INTO trips (user_id, title) VALUES (1, ?)').run('Other').lastInsertRowid); + const foreignBag = Number(db.prepare('INSERT INTO packing_bags (trip_id, name) VALUES (?, ?)').run(otherTripId, 'Foreign').lastInsertRowid); + + const cross = await request(server).post(`/api/trips/${tripId}/packing`).set('Cookie', sessionCookie(1)).send({ name: 'Tent', bag_id: foreignBag }); + expect(cross.status).toBe(400); + expect(cross.body).toEqual({ error: 'Bag not found' }); + const dead = await request(server).post(`/api/trips/${tripId}/packing`).set('Cookie', sessionCookie(1)).send({ name: 'Tent', bag_id: 99999 }); + expect(dead.status).toBe(400); + expect(dead.body).toEqual({ error: 'Bag not found' }); + expect((db.prepare('SELECT COUNT(*) AS n FROM packing_items WHERE trip_id = ?').get(tripId) as { n: number }).n).toBe(0); + + // camelCase was never part of the contract: the keys strip as they always did. + const camel = await request(server).post(`/api/trips/${tripId}/packing`).set('Cookie', sessionCookie(1)).send({ name: 'Probe', weightGrams: 250, bagId: foreignBag }); + expect(camel.status).toBe(201); + expect(camel.body.item).toMatchObject({ weight_grams: null, bag_id: null, quantity: 1 }); + }); + + it('400 "Bag not found" on update for a bag off the trip, leaving the row alone (#2154)', async () => { + const otherTripId = Number(db.prepare('INSERT INTO trips (user_id, title) VALUES (1, ?)').run('Other').lastInsertRowid); + const foreignBag = Number(db.prepare('INSERT INTO packing_bags (trip_id, name) VALUES (?, ?)').run(otherTripId, 'Foreign').lastInsertRowid); + const id = insertItem(tripId, 'Tent'); + const res = await request(server).put(`/api/trips/${tripId}/packing/${id}`).set('Cookie', sessionCookie(1)).send({ bag_id: foreignBag }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Bag not found' }); + expect((db.prepare('SELECT bag_id FROM packing_items WHERE id = ?').get(id) as { bag_id: number | null }).bag_id).toBeNull(); + }); + it('403 on create without permission, writing nothing', async () => { checkPermission.mockReturnValue(false); const res = await request(server).post(`/api/trips/${tripId}/packing`).set('Cookie', sessionCookie(1)).send({ name: 'X' }); @@ -344,6 +385,13 @@ describe('Packing e2e (real auth guard + real SQL over temp SQLite)', () => { expect(missing.body).toEqual({ error: 'Bag not found' }); }); + it('201 on bag create persists weight_limit_grams (#2154)', async () => { + const res = await request(server).post(`/api/trips/${tripId}/packing/bags`).set('Cookie', sessionCookie(1)).send({ name: 'Backpack', weight_limit_grams: 8000 }); + expect(res.status).toBe(201); + expect(res.body.bag).toMatchObject({ name: 'Backpack', weight_limit_grams: 8000 }); + expect((db.prepare('SELECT weight_limit_grams FROM packing_bags WHERE id = ?').get(res.body.bag.id) as { weight_limit_grams: number | null }).weight_limit_grams).toBe(8000); + }); + it('bag members: sets roster members only, dropping off-trip user ids', async () => { db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(tripId, 2); const created = await request(server).post(`/api/trips/${tripId}/packing/bags`).set('Cookie', sessionCookie(1)).send({ name: 'Main' }); diff --git a/server/tests/unit/mcp/tools-packing-advanced.test.ts b/server/tests/unit/mcp/tools-packing-advanced.test.ts index bbe52e086..0fa111948 100644 --- a/server/tests/unit/mcp/tools-packing-advanced.test.ts +++ b/server/tests/unit/mcp/tools-packing-advanced.test.ts @@ -155,6 +155,21 @@ describe('Tool: create_packing_bag', () => { }); }); + it('creates a bag with a weight limit (#2154)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_packing_bag', + arguments: { tripId: trip.id, name: 'Checked bag', weight_limit_grams: 23000 }, + }); + const data = parseToolResult(result) as any; + expect(data.bag.weight_limit_grams).toBe(23000); + const row = testDb.prepare('SELECT weight_limit_grams FROM packing_bags WHERE id = ?').get(data.bag.id) as { weight_limit_grams: number | null }; + expect(row.weight_limit_grams).toBe(23000); + }); + }); + it('returns access denied for non-member', async () => { const { user } = createUser(testDb); const { user: other } = createUser(testDb); diff --git a/server/tests/unit/mcp/tools-packing.test.ts b/server/tests/unit/mcp/tools-packing.test.ts index 09161c144..af0d215b6 100644 --- a/server/tests/unit/mcp/tools-packing.test.ts +++ b/server/tests/unit/mcp/tools-packing.test.ts @@ -240,6 +240,47 @@ describe('Tool: create_packing_item sharing', () => { }); }); +// --------------------------------------------------------------------------- +// create_packing_item: bag, quantity, weight in one write (#2154) +// +// The tool used to inherit the create schema's gap: an assistant could not +// give a new item a weight or put it in a bag without a second write. +// --------------------------------------------------------------------------- + +describe('Tool: create_packing_item bag, quantity, weight (#2154)', () => { + it('creates the item with its bag, quantity and weight in one write', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const bagId = Number(testDb.prepare('INSERT INTO packing_bags (trip_id, name) VALUES (?, ?)').run(trip.id, 'Carry-On').lastInsertRowid); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_packing_item', + arguments: { tripId: trip.id, name: 'Tent', bag_id: bagId, quantity: 3, weight_grams: 250 }, + }); + const row = itemRow(createdItemId(result)); + expect(row.bag_id).toBe(bagId); + expect(row.quantity).toBe(3); + expect(row.weight_grams).toBe(250); + }); + }); + + it('refuses a bag from another trip, inserting nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const foreignBag = Number(testDb.prepare('INSERT INTO packing_bags (trip_id, name) VALUES (?, ?)').run(otherTrip.id, 'Foreign').lastInsertRowid); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_packing_item', + arguments: { tripId: trip.id, name: 'Tent', bag_id: foreignBag }, + }); + expect(result.isError).toBe(true); + expect(errorText(result)).toContain('Bag not found'); + }); + expect((testDb.prepare('SELECT COUNT(*) AS n FROM packing_items WHERE trip_id = ?').get(trip.id) as { n: number }).n).toBe(0); + }); +}); + // --------------------------------------------------------------------------- // update_packing_item // --------------------------------------------------------------------------- @@ -430,6 +471,23 @@ describe('Tool: update_packing_item bag, quantity, weight, privacy', () => { expect(itemRow(item.id).bag_id).toBeNull(); }); }); + + it('refuses a bag from another trip (#2154)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const item = createPackingItem(testDb, trip.id); + const foreignBag = makeBag(otherTrip.id, 'Foreign'); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'update_packing_item', + arguments: { tripId: trip.id, itemId: item.id, bag_id: foreignBag }, + }); + expect(result.isError).toBe(true); + expect(errorText(result)).toContain('Bag not found'); + expect(itemRow(item.id).bag_id).toBeNull(); + }); + }); }); // --------------------------------------------------------------------------- diff --git a/server/tests/unit/nest/packing.controller.test.ts b/server/tests/unit/nest/packing.controller.test.ts index 4d7a97a7a..9db2edafb 100644 --- a/server/tests/unit/nest/packing.controller.test.ts +++ b/server/tests/unit/nest/packing.controller.test.ts @@ -95,6 +95,26 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r new PackingController(svc).create(user, '5', { name: 'Power bank', visibility: 'shared', recipient_ids: [2] }, 'sock'); expect(broadcastToViewers).toHaveBeenCalledWith('5', 'packing:created', { item }, [1, 2], 'sock'); }); + + it('forwards weight_grams, bag_id and quantity to the service (#2154)', () => { + // The create route used to hand only the six legacy fields through, so + // the values a caller sent vanished into a 201. + const createItem = vi.fn().mockReturnValue({ id: 9, name: 'Tent', is_private: 0 }); + const svc = makeService({ createItem, broadcast: vi.fn() } as Partial); + new PackingController(svc).create(user, '5', { name: 'Tent', weight_grams: 250, bag_id: 3, quantity: 3 }, 'sock'); + expect(createItem).toHaveBeenCalledWith('5', expect.objectContaining({ weight_grams: 250, bag_id: 3, quantity: 3 }), user.id); + }); + + it('400 "Bag not found" for a bag off the trip (#2154), broadcasting nothing', () => { + // Body validation error, so 400 — the 404 'Bag not found' stays with the + // /bags/:bagId path routes. + const broadcast = vi.fn(); + const svc = makeService({ createItem: vi.fn().mockReturnValue({ invalidBag: true }), broadcast } as Partial); + expect(thrown(() => new PackingController(svc).create(user, '5', { name: 'Tent', bag_id: 999 }))).toEqual({ + status: 400, body: { error: 'Bag not found' }, + }); + expect(broadcast).not.toHaveBeenCalled(); + }); }); it('GET / lists items for the trip, scoped to the viewer (#858)', () => { @@ -184,6 +204,15 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r expect(broadcast).toHaveBeenCalledWith('5', 'packing:updated', { item: updated }, 'sock'); }); + it('400 "Bag not found" for a bag off the trip (#2154), broadcasting nothing', () => { + const broadcast = vi.fn(); + const svc = makeService({ updateItem: vi.fn().mockReturnValue({ invalidBag: true }), broadcast } as Partial); + expect(thrown(() => new PackingController(svc).update(user, '5', '9', { bag_id: 999 }))).toEqual({ + status: 400, body: { error: 'Bag not found' }, + }); + expect(broadcast).not.toHaveBeenCalled(); + }); + it('forwards the X-Base-Updated-At token and 409s on a conflict (#1135)', () => { const updateItem = vi.fn().mockReturnValue({ conflict: true, server: { id: 9, name: 'Theirs' } }); const broadcast = vi.fn(); @@ -314,6 +343,13 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r expect(broadcast).toHaveBeenCalledWith('5', 'packing:bag-created', { bag: { id: 3, name: 'Carry-on' } }, 'sock'); }); + it('forwards weight_limit_grams on bag create (#2154)', () => { + const createBag = vi.fn().mockReturnValue({ id: 3, name: 'Backpack', weight_limit_grams: 8000 }); + const svc = makeService({ createBag, broadcast: vi.fn() } as Partial); + new PackingController(svc).createBag(user, '5', { name: 'Backpack', weight_limit_grams: 8000 }); + expect(createBag).toHaveBeenCalledWith('5', { name: 'Backpack', color: undefined, weight_limit_grams: 8000 }); + }); + it('404 on bag update when missing', () => { const svc = makeService({ updateBag: vi.fn().mockReturnValue(null) } as Partial); expect(thrown(() => new PackingController(svc).updateBag(user, '5', '3', { name: 'X' }))).toEqual({ diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index 649db5838..87887f199 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -62,7 +62,7 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin, createTrip, addTripMember } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { PackingService } from '../../../src/nest/packing/packing.service'; +import { PackingService, isInvalidBagRef } from '../../../src/nest/packing/packing.service'; // Was packing.bridge, deleted with the other three that had no consumer outside the // container. The assertions stayed; they point at the service now. const bridgeListItems = (tripId: string | number, viewerId?: number) => svc.listItems(tripId, viewerId); @@ -685,6 +685,60 @@ describe('legacy-quirk fixes', () => { }); }); +// ── Create-path fields + bag trip scope (#2154) ─────────────────────────────── + +describe('create-path fields + bag trip scope (#2154)', () => { + it('PACK-SVC-054: createItem persists weight_grams, bag_id and quantity in one write', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const bag = svc.createBag(trip.id, { name: 'Carry-On' }) as any; + + const item = svc.createItem(trip.id, { name: 'Tent', weight_grams: 250, bag_id: bag.id, quantity: 3 }, user.id) as any; + expect(item).toMatchObject({ weight_grams: 250, bag_id: bag.id, quantity: 3 }); + }); + + it('PACK-SVC-055: createItem refuses a bag off the trip with the invalidBag sentinel, inserting nothing', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const foreignBag = svc.createBag(otherTrip.id, { name: 'Not yours' }) as any; + + // Existence alone is not enough — the bag must belong to THIS trip. + expect(svc.createItem(trip.id, { name: 'Tent', bag_id: foreignBag.id }, user.id)).toEqual({ invalidBag: true }); + // A dead id refuses the same way (it used to be an SQLite FK error). + expect(isInvalidBagRef(svc.createItem(trip.id, { name: 'Tent', bag_id: 99999 }, user.id))).toBe(true); + expect((testDb.prepare('SELECT COUNT(*) AS n FROM packing_items WHERE trip_id = ?').get(trip.id) as any).n).toBe(0); + }); + + it('PACK-SVC-056: updateItem refuses a cross-trip bag_id and leaves the row alone; null still clears', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const own = svc.createBag(trip.id, { name: 'Mine' }) as any; + const foreign = svc.createBag(otherTrip.id, { name: 'Not mine' }) as any; + const item = svc.createItem(trip.id, { name: 'Tent', bag_id: own.id }, user.id) as any; + + const refused = svc.updateItem(trip.id, item.id, { bag_id: foreign.id }, ['bag_id'], undefined, user.id); + expect(isInvalidBagRef(refused)).toBe(true); + expect((testDb.prepare('SELECT bag_id FROM packing_items WHERE id = ?').get(item.id) as any).bag_id).toBe(own.id); + + // Clearing the bag with an explicit null is untouched by the check. + const cleared = svc.updateItem(trip.id, item.id, { bag_id: null }, ['bag_id'], undefined, user.id) as any; + expect(cleared.bag_id).toBeNull(); + }); + + it('PACK-SVC-057: createBag persists weight_limit_grams; omitted stays null', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const limited = svc.createBag(trip.id, { name: 'Checked', weight_limit_grams: 23000 }) as any; + expect(limited.weight_limit_grams).toBe(23000); + + const bare = svc.createBag(trip.id, { name: 'Day pack' }) as any; + expect(bare.weight_limit_grams).toBeNull(); + }); +}); + // ── Wrapper helpers (carried over from the old delegation suite) ────────────── describe('canEdit', () => { diff --git a/server/tests/unit/packing/packing.rpc.test.ts b/server/tests/unit/packing/packing.rpc.test.ts index 73891664c..451a65f03 100644 --- a/server/tests/unit/packing/packing.rpc.test.ts +++ b/server/tests/unit/packing/packing.rpc.test.ts @@ -157,6 +157,39 @@ describe('PackingRpc through the router', () => { expect(res.error.message).toBe('no packing item 404 on trip 1'); }); + it('PACKING-RPC-018 create hands the widened fields through the schema (#2154)', async () => { + const f = build(); + await f.host('db:write:packing').dispatch(req('packing.create', { tripId: 1, input: { name: 'Tent', weight_grams: 250, bag_id: 19, quantity: 3 } }), 42); + expect(f.data.createItem).toHaveBeenCalledWith('1', { name: 'Tent', weight_grams: 250, bag_id: 19, quantity: 3 }, 42); + }); + + it('PACKING-RPC-019 an off-trip bag is BAD_PARAMS on create and update alike, broadcasting nothing (#2154)', async () => { + const f = build(); + const host = f.host('db:write:packing'); + + f.data.createItem.mockReturnValueOnce({ invalidBag: true } as never); + const created = (await host.dispatch(req('packing.create', { tripId: 1, input: { name: 'Tent', bag_id: 404 } }), 42)) as RpcError; + expect(created.error.code).toBe('BAD_PARAMS'); + expect(created.error.message).toBe('no packing bag 404 on trip 1'); + + f.data.updateItem.mockReturnValueOnce({ invalidBag: true } as never); + const updated = (await host.dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { bag_id: 404 } }), 42)) as RpcError; + expect(updated.error.code).toBe('BAD_PARAMS'); + expect(updated.error.message).toBe('no packing bag 404 on trip 1'); + + // The sentinel must never leave the process as an item. + expect(f.realtime.broadcast).not.toHaveBeenCalled(); + }); + + it('PACKING-RPC-020 createBag forwards a numeric weight limit and drops a non-number (#2154)', async () => { + const f = build(); + const host = f.host('db:write:packing'); + await host.dispatch(req('packing.createBag', { tripId: 1, input: { name: 'Bag', weight_limit_grams: 23000 } }), 42); + expect(f.data.createBag).toHaveBeenCalledWith('1', { name: 'Bag', color: undefined, weight_limit_grams: 23000 }); + await host.dispatch(req('packing.createBag', { tripId: 1, input: { name: 'Bag', weight_limit_grams: 'heavy' } }), 42); + expect(f.data.createBag).toHaveBeenLastCalledWith('1', { name: 'Bag', color: undefined, weight_limit_grams: undefined }); + }); + it('PACKING-RPC-008 the class is listed in its module providers', () => { expectRegisteredProvider(PackingModule, PackingRpc); }); diff --git a/shared/src/packing/packing.schema.spec.ts b/shared/src/packing/packing.schema.spec.ts index 7c39fb573..fd639d7f7 100644 --- a/shared/src/packing/packing.schema.spec.ts +++ b/shared/src/packing/packing.schema.spec.ts @@ -19,6 +19,28 @@ describe('packingCreateItemRequestSchema', () => { ).toBe(true); expect(packingCreateItemRequestSchema.safeParse({ name: '' }).success).toBe(false); }); + + it('carries weight_grams, bag_id and quantity like the update schema (#2154)', () => { + const parsed = packingCreateItemRequestSchema.safeParse({ + name: 'Tent', + weight_grams: 300, + bag_id: 19, + quantity: 3, + }); + expect(parsed.success).toBe(true); + expect(parsed.success && parsed.data).toEqual({ name: 'Tent', weight_grams: 300, bag_id: 19, quantity: 3 }); + // Nullable, mirroring the update contract. + expect(packingCreateItemRequestSchema.safeParse({ name: 'Tent', weight_grams: null, bag_id: null }).success).toBe( + true, + ); + expect(packingCreateItemRequestSchema.safeParse({ name: 'Tent', bag_id: 'carry-on' }).success).toBe(false); + }); + + it('still strips unknown keys — camelCase was never part of the contract', () => { + const parsed = packingCreateItemRequestSchema.safeParse({ name: 'Tent', weightGrams: 300, bagId: 19 }); + expect(parsed.success).toBe(true); + expect(parsed.success && parsed.data).toEqual({ name: 'Tent' }); + }); }); describe('packingImportRequestSchema', () => { @@ -36,6 +58,16 @@ describe('packingCreateBagRequestSchema', () => { expect(packingCreateBagRequestSchema.safeParse({ name: 'Carry-on' }).success).toBe(true); expect(packingCreateBagRequestSchema.safeParse({}).success).toBe(false); }); + + it('carries weight_limit_grams like the update schema (#2154)', () => { + const parsed = packingCreateBagRequestSchema.safeParse({ name: 'Backpack', weight_limit_grams: 8000 }); + expect(parsed.success).toBe(true); + expect(parsed.success && parsed.data).toEqual({ name: 'Backpack', weight_limit_grams: 8000 }); + expect(packingCreateBagRequestSchema.safeParse({ name: 'Backpack', weight_limit_grams: null }).success).toBe(true); + expect(packingCreateBagRequestSchema.safeParse({ name: 'Backpack', weight_limit_grams: 'heavy' }).success).toBe( + false, + ); + }); }); describe('packingSaveTemplateRequestSchema', () => { diff --git a/shared/src/packing/packing.schema.ts b/shared/src/packing/packing.schema.ts index 919fe97da..733a5d623 100644 --- a/shared/src/packing/packing.schema.ts +++ b/shared/src/packing/packing.schema.ts @@ -86,6 +86,11 @@ export const packingCreateItemRequestSchema = z.object({ category: z.string().optional(), // The legacy route accepted both boolean and 0/1 for checked — both stay valid. checked: z.union([z.boolean(), z.number().int().min(0).max(1)]).optional(), + // Same shapes as the update schema below (#2154) — the create route used to + // strip these three silently, forcing a second write to set them. + weight_grams: z.number().nullable().optional(), + bag_id: z.number().nullable().optional(), + quantity: z.number().optional(), // Mark the new item private to its creator (#858, legacy flag). is_private: z.boolean().optional(), // Three-tier sharing (#858): which list the item belongs to, and — for 'shared' — @@ -128,6 +133,8 @@ export type PackingReorderRequest = z.infer; export const packingCreateBagRequestSchema = z.object({ name: z.string().min(1), color: z.string().optional(), + // Same shape as the update schema below (#2154) — POST used to strip it. + weight_limit_grams: z.number().nullable().optional(), }); export type PackingCreateBagRequest = z.infer;