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
This commit is contained in:
John Jahn
2026-11-10 10:05:22 +01:00
committed by Maurice
parent 6b086eaed5
commit 8e8cff8105
5 changed files with 155 additions and 27 deletions
+1 -1
View File
@@ -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
}
@@ -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);
+58 -1
View File
@@ -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<string, unknown>) {
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 });
});
});
});
@@ -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);
@@ -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 });
});
});