From 8e8cff8105dfacaeea477b698539202c0c54fced Mon Sep 17 00:00:00 2001 From: John Jahn Date: Tue, 10 Nov 2026 10:05:22 +0100 Subject: [PATCH] fix(reservations): keep day_id on partial updates of time-only bookings A partial update that left day_id out (a notes-only PUT, an MCP update_reservation or update_transport call) re-derived the day from the stored reservation_time whenever one was set. A time-only value such as "12:00" resolves to no day, so the booking's day_id was written as null and it vanished from its day in the planner. The end_day_id branch did the same when a time-only reservation_end_time was sent. The day is now derived only when the time carries a date that resolves to a trip day; otherwise an omitted day_id or end_day_id keeps the stored value. A dated time still re-pins the day, an explicit day_id null still clears it, and a cleared end time still clears end_day_id. REST and both MCP update tools share updateInTx, so the fix covers all three. Tests: service unit cases (RESV-SVC-044..048), an e2e case per behaviour on PUT /api/trips/:tripId/reservations/:id, and an update_reservation MCP case, all seeded with the ORM factories. Fixes #3025 --- server/scripts/size-baseline.json | 2 +- .../nest/reservations/reservations.service.ts | 39 +++++------- server/tests/e2e/reservations.e2e.test.ts | 59 ++++++++++++++++++- .../tests/unit/mcp/tools-reservations.test.ts | 26 ++++++++ .../unit/nest/reservations.service.test.ts | 56 +++++++++++++++++- 5 files changed, 155 insertions(+), 27 deletions(-) diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 6cb07eaa3..81e2eb780 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -15,6 +15,6 @@ "src/nest/places/places.service.ts": 1549, "src/nest/plugins/plugin-runtime.service.ts": 1460, "src/nest/plugins/runtime/plugin-sdk.ts": 1093, - "src/nest/reservations/reservations.service.ts": 1237, + "src/nest/reservations/reservations.service.ts": 1228, "src/nest/vacay/vacay.service.ts": 1409 } diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index c2f844c55..92715a0fd 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -132,7 +132,7 @@ export interface UpdateReservationData { confirmation_number?: string; notes?: string; url?: string; - day_id?: number; + day_id?: number | null; end_day_id?: number | null; place_id?: number; assignment_id?: number; @@ -944,32 +944,23 @@ export class ReservationsService { ? null : (reservation_end_time !== undefined ? (reservation_end_time || null) : current.reservation_end_time); - // day_id / end_day_id: honour an explicit value from the client, - // otherwise derive from the (possibly updated) reservation_time so the - // planner renders the booking on the correct day. - let nextDayId: number | null; - if (day_id != null) { - // Explicit day from the client (e.g. moved on the planner). - nextDayId = day_id; - } else if (resolvedType !== 'hotel' && nextReservationTime) { - // No day set but we have a date — pin it to the matching day so the booking - // still shows in the Plan (covers bookings saved without a selected day, and - // the case where an earlier edit cleared day_id). + // day_id / end_day_id: an explicit value from the client wins (e.g. moved on the + // planner), and an explicit null clears. Otherwise a dated time pins the booking to + // its day, which also covers one saved without a day or an earlier edit that cleared + // it. A time-only value ("12:00") names no day, so an update that leaves the field + // out keeps the stored day rather than reading the miss as "clear it". + let nextDayId: number | null = day_id ?? null; + if (day_id == null && resolvedType !== 'hotel') { nextDayId = await this.resolveDayIdFromTime(tripId, nextReservationTime); - } else if (day_id === undefined) { - // Field absent and nothing to derive from — keep whatever it had. - nextDayId = current.day_id ?? null; - } else { - nextDayId = null; } + if (nextDayId == null && day_id === undefined) nextDayId = current.day_id ?? null; - let nextEndDayId: number | null; - if (end_day_id !== undefined) { - nextEndDayId = end_day_id ?? null; - } else if (reservation_end_time !== undefined && resolvedType !== 'hotel') { - nextEndDayId = await this.resolveDayIdFromTime(tripId, nextReservationEndTime); - } else { - nextEndDayId = current.end_day_id ?? null; + let nextEndDayId: number | null = end_day_id === undefined ? (current.end_day_id ?? null) : end_day_id; + if (end_day_id === undefined && reservation_end_time !== undefined && resolvedType !== 'hotel') { + // A cleared end time clears the end day; a new one moves it unless it is time-only. + nextEndDayId = nextReservationEndTime + ? ((await this.resolveDayIdFromTime(tripId, nextReservationEndTime)) ?? nextEndDayId) + : null; } nextDayId = await this.resolvedOrNull('days', nextDayId); diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index b2dedf2ee..91a9f09f7 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -28,6 +28,7 @@ import { NotificationsService } from '../../src/nest/notifications/notifications import { PermissionsService } from '../../src/nest/permissions/permissions.service'; import { RealtimeModule } from '../../src/nest/realtime/realtime.module'; import { ReservationsModule } from '../../src/nest/reservations/reservations.module'; +import { makeReservation } from '../helpers/factories/reservations'; import { countRows, deleteRows, @@ -36,7 +37,7 @@ import { insertRow, insertRowIgnoringConflict, } from '../helpers/factories/rows'; -import { makeTrip, makeDay } from '../helpers/factories/trips'; +import { makeTrip, makeDay, readTripDays } from '../helpers/factories/trips'; import { makeUser } from '../helpers/factories/users'; import { createTestMikroOrmModule, createTestOrm, type TestOrm } from '../helpers/test-orm'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; @@ -502,4 +503,60 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real expect(res.status).toBe(200); }); }); + + // Adding notes to two time-only Shinkansen bookings from the browser took them + // off their days: the stored "12:00" resolved to no day, and the day_id the PUT + // left out was written as null. + describe('a partial update keeps the day it does not name', () => { + let days: number[]; + let datedTripId: number; + + beforeAll(async () => { + datedTripId = (await makeTrip(orm, 1, { title: 'Japan', start_date: '2026-11-10', end_date: '2026-11-12' })).id; + days = (await readTripDays(orm, datedTripId)).map((d) => d.id); + }); + + async function seedTrain(reservationTime: string, dayId: number | null) { + const train = await makeReservation(orm, datedTripId, { + title: 'Shinkansen', + type: 'train', + reservation_time: reservationTime, + day: dayId, + }); + return train.id; + } + + function put(reservationId: number, body: Record) { + return request(server) + .put(`/api/trips/${datedTripId}/reservations/${reservationId}`) + .set('Cookie', sessionCookie(1)) + .send(body); + } + + it('200 a notes-only update leaves a time-only booking on its day', async () => { + const rid = await seedTrain('12:00', days[1]); + const res = await put(rid, { notes: 'Car 7, seats 12A-B' }); + expect(res.status).toBe(200); + expect(res.body.reservation).toMatchObject({ + notes: 'Car 7, seats 12A-B', + reservation_time: '12:00', + day_id: days[1], + }); + expect(await findRow(orm, Reservations, { id: rid })).toMatchObject({ day_id: days[1] }); + }); + + it('200 a dated time still moves the booking to the day it names', async () => { + const rid = await seedTrain('12:00', days[0]); + const res = await put(rid, { reservation_time: '2026-11-12T09:00' }); + expect(res.status).toBe(200); + expect(await findRow(orm, Reservations, { id: rid })).toMatchObject({ day_id: days[2] }); + }); + + it('200 an explicit day_id null still takes the booking off its day', async () => { + const rid = await seedTrain('12:00', days[1]); + const res = await put(rid, { day_id: null, notes: 'Not planned yet' }); + expect(res.status).toBe(200); + expect(await findRow(orm, Reservations, { id: rid })).toMatchObject({ day_id: null }); + }); + }); }); diff --git a/server/tests/unit/mcp/tools-reservations.test.ts b/server/tests/unit/mcp/tools-reservations.test.ts index 586e29ef1..e4131991a 100644 --- a/server/tests/unit/mcp/tools-reservations.test.ts +++ b/server/tests/unit/mcp/tools-reservations.test.ts @@ -22,7 +22,10 @@ import { createCategory, addTripMember, } from '../../helpers/factories'; +import { makeReservation } from '../../helpers/factories/reservations'; import { countRows, findRow, findRows, insertRow, updateRows } from '../../helpers/factories/rows'; +import { makeTrip, readTripDays } from '../../helpers/factories/trips'; +import { makeUser } from '../../helpers/factories/users'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, parseResourceResult, type McpHarness } from '../../helpers/mcp-harness'; import { resetTestDb } from '../../helpers/test-db'; @@ -236,6 +239,29 @@ describe('Tool: create_reservation', () => { // --------------------------------------------------------------------------- describe('Tool: update_reservation', () => { + // The tool goes through the same update as REST, so an edit that names no day + // leaves a time-only booking on the one it has, as a notes-only PUT does. + it('keeps the day of a time-only booking on an edit that names no day', async () => { + const { user } = await makeUser(orm); + const trip = await makeTrip(orm, user.id, { start_date: '2030-05-01', end_date: '2030-05-03' }); + const days = await readTripDays(orm, trip.id); + const booking = await makeReservation(orm, trip.id, { + type: 'restaurant', + reservation_time: '19:00', + day: days[1].id, + }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'update_reservation', + arguments: { tripId: trip.id, reservationId: booking.id, notes: 'Counter seats' }, + }); + expect((parseToolResult(result) as { reservation: { day_id: number | null } }).reservation.day_id).toBe( + days[1].id, + ); + }); + expect((await reservationRow(booking.id)).day_id).toBe(days[1].id); + }); + it('updates reservation fields', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index 364f8fde4..6728566e9 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -33,7 +33,7 @@ import { HttpException } from '@nestjs/common'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip, createReservation, createBudgetItem, createPlace, createDay, createDayAccommodation, createDayAssignment, addTripMember } from '../../helpers/factories'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { ReservationsService, type UpdateReservationData } from '../../../src/nest/reservations/reservations.service'; import { ReservationsReadService } from '../../../src/nest/reservations/reservations-read.service'; import type { BudgetService } from '../../../src/nest/budget/budget.service'; // Was reservations.bridge, deleted along with the other three that had no @@ -63,6 +63,9 @@ import { createTestBudgetItemsRepo } from '../../helpers/files-repos'; import { sharedTestOrm } from '../../helpers/test-uow'; import type { TestOrm } from '../../helpers/test-orm'; import { countRows, deleteRows, findRow, findRows, insertRow, updateRows } from '../../helpers/factories/rows'; +import { makeReservation } from '../../helpers/factories/reservations'; +import { makeTrip, readTripDays } from '../../helpers/factories/trips'; +import { makeUser } from '../../helpers/factories/users'; import { BudgetItems } from '../../../src/db/entities/BudgetItems.entity'; import { DayAccommodations } from '../../../src/db/entities/DayAccommodations.entity'; import { DayAssignments } from '../../../src/db/entities/DayAssignments.entity'; @@ -1709,3 +1712,54 @@ describe('the day stop a hotel booking implies', () => { expect(await findRow(orm, DayAssignments, { id: theirStop })).toBeTruthy(); }); }); + +// A partial update (a notes-only PUT, an MCP edit) leaves a booking on its day unless it +// names one or carries a dated time. A bare "12:00" names no day, and reading that as +// "clear it" used to drop time-only trains off the planner on an edit of their notes. +describe('ReservationsService — the day a partial update leaves a booking on', () => { + async function train(fields: { reservation_time: string; reservation_end_time?: string }, start: number | null, end: number | null = null) { + const { user } = await makeUser(orm); + const trip = await makeTrip(orm, user.id, { start_date: '2030-05-01', end_date: '2030-05-03' }); + const days = (await readTripDays(orm, trip.id)).map(d => d.id); + const pick = (i: number | null) => (i === null ? null : days[i]); + const booking = await makeReservation(orm, trip.id, { title: 'Shinkansen', type: 'train', ...fields, day: pick(start), endDay: pick(end) }); + const update = async (data: UpdateReservationData) => { + const current = await svc.getReservation(String(booking.id), String(trip.id)); + if (!current) throw new Error('the booking under test is gone'); + return (await svc.update(String(booking.id), String(trip.id), data, current)).reservation; + }; + return { days, update }; + } + + it('RESV-SVC-044: a notes-only update keeps the day of a time-only booking', async () => { + const { days, update } = await train({ reservation_time: '12:00' }, 1); + expect(await update({ notes: 'Car 7, seats 12A-B' })) + .toMatchObject({ notes: 'Car 7, seats 12A-B', reservation_time: '12:00', day_id: days[1] }); + }); + + it('RESV-SVC-045: a new time-only reservation_time keeps the day as well', async () => { + const { days, update } = await train({ reservation_time: '12:00' }, 1); + expect(await update({ reservation_time: '14:30' })).toMatchObject({ reservation_time: '14:30', day_id: days[1] }); + }); + + it('RESV-SVC-046: a dated time still pins the day, sent with the update or stored', async () => { + const sent = await train({ reservation_time: '12:00' }, 0); + expect(await sent.update({ reservation_time: '2030-05-03T09:00' })).toMatchObject({ day_id: sent.days[2] }); + // Saved without a day: any edit puts it on the day its date names. + const stored = await train({ reservation_time: '2030-05-02T10:00' }, null); + expect(await stored.update({ notes: 'Window seat' })).toMatchObject({ day_id: stored.days[1] }); + }); + + it('RESV-SVC-047: an explicit day_id null still clears the day', async () => { + const { update } = await train({ reservation_time: '12:00' }, 1); + expect(await update({ day_id: null, notes: 'Not planned yet' })).toMatchObject({ day_id: null }); + }); + + it('RESV-SVC-048: a time-only end time keeps the end day; a dated one moves it and a cleared one clears it', async () => { + const { days, update } = await train({ reservation_time: '12:00', reservation_end_time: '18:00' }, 0, 1); + expect(await update({ reservation_end_time: '19:30' })) + .toMatchObject({ reservation_end_time: '19:30', day_id: days[0], end_day_id: days[1] }); + expect(await update({ reservation_end_time: '2030-05-03T08:00' })).toMatchObject({ end_day_id: days[2] }); + expect(await update({ reservation_end_time: '' })).toMatchObject({ reservation_end_time: null, end_day_id: null }); + }); +});