diff --git a/server/tests/unit/mcp/tools-days-accommodations.test.ts b/server/tests/unit/mcp/tools-days-accommodations.test.ts index 6139928c1..833559683 100644 --- a/server/tests/unit/mcp/tools-days-accommodations.test.ts +++ b/server/tests/unit/mcp/tools-days-accommodations.test.ts @@ -370,6 +370,28 @@ describe('Tool: create_place_accommodation', () => { expect(testDb.prepare('SELECT COUNT(*) as n FROM places WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 }); }); }); + + it('a write that fails inside the transaction rolls both halves back and says so', async () => { + // The place, the stay and the day stop the stay implies go in together or not at + // all. A category that is not there is the cheapest way to make the first insert + // fail on a foreign key; what matters is that the tool answers with an error + // instead of a half-written trip. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_place_accommodation', + arguments: { tripId: trip.id, name: 'Hotel Nirgendwo', category_id: 99999, start_day_id: day.id, end_day_id: day.id }, + }); + expect(result.isError).toBe(true); + const { content } = result as { content: { type: string; text?: string }[] }; + expect(content[0].text).toContain('Failed to create place and accommodation'); + expect(testDb.prepare('SELECT COUNT(*) as n FROM places WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 }); + expect(testDb.prepare('SELECT COUNT(*) as n FROM day_accommodations WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 }); + expect(testDb.prepare('SELECT COUNT(*) as n FROM day_assignments WHERE day_id = ?').get(day.id)).toMatchObject({ n: 0 }); + }); + }); }); // --------------------------------------------------------------------------- diff --git a/server/tests/unit/nest/accommodations.controller.test.ts b/server/tests/unit/nest/accommodations.controller.test.ts index d275e9e52..d426ad8f9 100644 --- a/server/tests/unit/nest/accommodations.controller.test.ts +++ b/server/tests/unit/nest/accommodations.controller.test.ts @@ -96,6 +96,35 @@ describe('AccommodationsController (parity with the legacy accommodations sub-ro .toEqual({ accommodation: { id: 9 }, assignment: mirror.created, removedAssignments: mirror.removed }); expect(announceMirror).toHaveBeenCalledWith('5', mirror, expect.any(Function), 'sock'); }); + + it('ACC-CTL-003 404s a place or day that is not on this trip, before the write', () => { + const get = vi.fn().mockReturnValue({ id: 9 }); + const update = vi.fn(); + const validateRefs = vi.fn().mockReturnValue([{ message: 'Place not found' }]); + const svc = makeService({ get, update, validateRefs } as Partial); + expect(thrown(() => new AccommodationsController(svc).update(user, '5', '9', refs))) + .toEqual({ status: 404, body: { error: 'Place not found' } }); + expect(update).not.toHaveBeenCalled(); + }); + + it('ACC-CTL-004 the mirror sender is the same broadcast, socket id and all', () => { + // announceMirror decides which events a stay write implies; the controller only + // hands it the door out. That door has to carry the sender's socket id, or the + // session that just moved the booking is told about its own stop twice. + const get = vi.fn().mockReturnValue({ id: 9 }); + const mirror = { created: { id: 78, day_id: 11 }, removed: [{ id: 77, dayId: 10 }], stamped: null }; + const update = vi.fn().mockReturnValue({ accommodation: { id: 9 }, mirror }); + const broadcast = vi.fn(); + type Send = (event: string, payload: unknown) => void; + const announceMirror = vi.fn((_tripId: string, m: typeof mirror, send: Send) => { + for (const stop of m.removed) send('assignment:deleted', { assignmentId: stop.id, dayId: stop.dayId }); + if (m.created) send('assignment:created', { assignment: m.created }); + }); + const svc = makeService({ get, update, broadcast, announceMirror } as Partial); + new AccommodationsController(svc).update(user, '5', '9', refs, 'sock'); + expect(broadcast).toHaveBeenCalledWith('5', 'assignment:deleted', { assignmentId: 77, dayId: 10 }, 'sock'); + expect(broadcast).toHaveBeenCalledWith('5', 'assignment:created', { assignment: mirror.created }, 'sock'); + }); }); describe('DELETE /:id', () => { @@ -130,5 +159,24 @@ describe('AccommodationsController (parity with the legacy accommodations sub-ro expect(broadcast).toHaveBeenCalledWith('5', 'reservation:deleted', { reservationId: 4 }, 'sock'); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:deleted', { reservationId: 5 }, 'sock'); }); + + it('ACC-CTL-005 ?keepStop=true is what turns a night back into an ordinary pause', () => { + // The road trip popup switching the night off: the booking goes, the stop it + // brought stays where it is in the drive. Nothing else may pass that flag, so + // only the literal string counts. + const get = vi.fn().mockReturnValue({ id: 9 }); + const remove = vi.fn().mockReturnValue({ + linkedReservationId: null, deletedBudgetItemId: null, + linkedReservationIds: [], deletedBudgetItemIds: [], mirror: noMirror, + }); + const svc = makeService({ get, remove } as Partial); + const controller = new AccommodationsController(svc); + + controller.remove(user, '5', '9', 'sock', 'true'); + expect(remove).toHaveBeenLastCalledWith('9', { keepStop: true }); + + controller.remove(user, '5', '9', 'sock'); + expect(remove).toHaveBeenLastCalledWith('9', { keepStop: false }); + }); }); }); diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index da4e0ee5f..c71818bf2 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -779,6 +779,38 @@ describe('the day stop a booking implies', () => { expect(mirror.removed).toEqual([]); }); + it('ACC-032b keepStop hands the mirrored stop to the traveller instead of taking it away', () => { + // The road trip popup switching a night back to an ordinary pause: the booking is + // what was cancelled, not the place, and the stop sits mid-drive where re-adding it + // would land it at the end of the day instead. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = book(trip.id, place.id, day.id, day.id); + const mirrored = stopsOn(day.id)[0]; + + const { mirror } = svc.deleteAccommodation(accommodation.id, { keepStop: true }); + + // Still there, and now unmarked: nothing may move or delete it on the next edit. + expect(stopsOn(day.id)).toEqual([expect.objectContaining({ id: mirrored.id, place_id: place.id, accommodation_id: null })]); + // Nothing to announce either — no session has to take a stop off its day plan. + expect(mirror.removed).toEqual([]); + expect(testDb.prepare('SELECT id FROM day_accommodations WHERE id = ?').get(accommodation.id)).toBeUndefined(); + }); + + it('ACC-032c remove() forwards keepStop, which is the only door the REST route uses', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = book(trip.id, place.id, day.id, day.id); + + svc.remove(accommodation.id, { keepStop: true }); + + expect(stopsOn(day.id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: null })]); + }); + it('ACC-033 announceMirror sends the removal before the arrival', () => { // Order matters on a move: the day plan would briefly hold the place twice if the // arrival went first. diff --git a/server/tests/unit/reservations/bookings.rpc.test.ts b/server/tests/unit/reservations/bookings.rpc.test.ts index 7f8741d5f..15603e29d 100644 --- a/server/tests/unit/reservations/bookings.rpc.test.ts +++ b/server/tests/unit/reservations/bookings.rpc.test.ts @@ -30,7 +30,7 @@ const req = (method: string, params: Record = {}): RpcRequest = const events = (r: { broadcast: ReturnType }) => r.broadcast.mock.calls.map((c) => c[1]); /** Trip 1 belongs to user 42; reservation 5 and accommodation 11 sit on it. */ -function build(opts: { canEdit?: boolean; cascade?: boolean; seenActions?: string[] } = {}) { +function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; seenActions?: string[] } = {}) { const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const reservations = { create: vi.fn(() => ({ reservation: { id: 40 }, accommodationCreated: !!opts.cascade })), @@ -45,25 +45,35 @@ function build(opts: { canEdit?: boolean; cascade?: boolean; seenActions?: strin syncBudgetOnUpdate: vi.fn(), notifyBookingChange: vi.fn(), } as unknown as ReservationsService & Record>; - /** A stay write that left the day plan alone. */ - const noMirror = () => ({ created: null, removed: [], stamped: null }); + /** What a stay write did to the day plan, on top of writing the stay itself. */ + type Mirror = { created: { id: number; day_id: number } | null; removed: { id: number; dayId: number }[]; stamped: null }; + /** A stay write that left the day plan alone, which is what most of these cases are. */ + const noMirror = (): Mirror => ({ created: null, removed: [], stamped: null }); + /** With `stop`, the write moved its day stop from day 3 to day 4. */ + const mirror = (): Mirror => (opts.stop ? { created: { id: 78, day_id: 4 }, removed: [{ id: 77, dayId: 3 }], stamped: null } : noMirror()); // The lodging blocks live in AccommodationsService; AccommodationsRpc still calls its // injected copy `days`, which is why the fixture keeps that name. const days = { validateAccommodationRefs: vi.fn((_t: number, placeId?: number) => (placeId === 404 ? [{ message: 'place 404 is not on this trip' }] : [])), - createAccommodation: vi.fn(() => ({ accommodation: { id: 60 }, mirror: noMirror() })), + createAccommodation: vi.fn(() => ({ accommodation: { id: 60 }, mirror: mirror() })), getAccommodation: vi.fn((id: number) => (id === 11 ? { id: 11 } : undefined)), - updateAccommodation: vi.fn(() => ({ accommodation: { id: 11 }, mirror: noMirror() })), + updateAccommodation: vi.fn(() => ({ accommodation: { id: 11 }, mirror: mirror() })), deleteAccommodation: vi.fn(() => ({ linkedReservationId: opts.cascade ? 40 : null, deletedBudgetItemId: opts.cascade ? 7 : null, linkedReservationIds: opts.cascade ? [40] : [], deletedBudgetItemIds: opts.cascade ? [7] : [], - mirror: noMirror(), + mirror: mirror(), })), - // The real one broadcasts the day stop a booking writes; these cases are about - // the booking events, so the fixture keeps it inert and BOOK-RPC-016 covers it. - announceMirror: vi.fn(), + // Stands in for the real fan-out rather than swallowing it, so the sender each + // handler builds is exercised: that sender is the plugin surface's only link + // between a stay write and the sockets, and a mock that never calls it would + // let a broken one through. Inert whenever the mirror is empty, which is every + // case not built with `stop`. + announceMirror: vi.fn((_tripId: number, m: Mirror, send: (event: string, payload: unknown) => void) => { + for (const stop of m.removed) send('assignment:deleted', { assignmentId: stop.id, dayId: stop.dayId }); + if (m.created) send('assignment:created', { assignment: m.created }); + }), } as unknown as AccommodationsService & Record>; const guards = new PluginGuards( { @@ -257,6 +267,34 @@ describe('AccommodationsRpc', () => { expect(f.days.announceMirror).toHaveBeenCalledWith(1, { created: null, removed: [], stamped: null }, expect.any(Function)); }); + it('BOOK-RPC-017b editing a block writes it, announces it and answers with the block', async () => { + const f = build(); + const res = await f.host().dispatch(req('accommodations.update', { tripId: 1, accommodationId: 11, input: { check_in: '15:00' } }), 42); + expect(f.days.updateAccommodation).toHaveBeenCalledWith(11, { id: 11 }, expect.objectContaining({ check_in: '15:00' })); + expect(events(f.realtime)).toEqual(['accommodation:updated']); + expect(f.days.announceMirror).toHaveBeenCalledWith(1, { created: null, removed: [], stamped: null }, expect.any(Function)); + // The block itself, not the { accommodation, mirror } pair the service returns: + // the mirror is a broadcast concern and plugins never asked for it. + expect(res).toMatchObject({ k: 'res', result: { id: 11 } }); + }); + + it('BOOK-RPC-017c what announceMirror sends goes out over the trip socket, on all three writes', async () => { + // The sender each handler hands announceMirror is the plugin surface's only link + // between a stay write and the day plan every other session is looking at. What + // belongs in the mirror is the service's call; that it reaches a socket at all is + // this one's, which is why the fixture's mirror is the same on all three. + for (const request of [ + req('accommodations.create', { tripId: 1, input: { place_id: 7, start_day_id: 3, end_day_id: 4 } }), + req('accommodations.update', { tripId: 1, accommodationId: 11, input: { start_day_id: 4 } }), + req('accommodations.delete', { tripId: 1, accommodationId: 11 }), + ]) { + const f = build({ stop: true }); + await f.host().dispatch(request, 42); + expect(f.realtime.broadcast).toHaveBeenCalledWith(1, 'assignment:deleted', { assignmentId: 77, dayId: 3 }); + expect(f.realtime.broadcast).toHaveBeenCalledWith(1, 'assignment:created', { assignment: { id: 78, day_id: 4 } }); + } + }); + it('BOOK-RPC-015d deleting a block without a partner cascades only its own event', async () => { const f = build(); await f.host().dispatch(req('accommodations.delete', { tripId: 1, accommodationId: 11 }), 42);