From dbb70fc525086848dad398b066d0f563652dc30e Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 22 Oct 2026 12:14:25 +0200 Subject: [PATCH] fix(realtime): booking writes share one use case on REST and MCP --- server/scripts/test-new-service-baseline.json | 1 - .../reservation-writes.service.ts | 141 +++++++ .../reservations/reservations.controller.ts | 64 +--- .../src/nest/reservations/reservations.mcp.ts | 74 ++-- .../nest/reservations/reservations.module.ts | 4 +- .../src/nest/reservations/reservations.rpc.ts | 19 +- .../nest/reservations/reservations.service.ts | 7 +- .../broadcast-parity/bookings.cases.ts | 357 ++++++++++++++++-- ...ols-assignments-reservations-extra.test.ts | 17 + .../tests/unit/mcp/tools-reservations.test.ts | 51 +++ .../tests/unit/mcp/tools-transports.test.ts | 116 +++++- .../nest/accommodations.controller.test.ts | 99 +++++ .../nest/accommodations.event-parity.test.ts | 193 ---------- .../unit/nest/reservations.controller.test.ts | 183 +++++---- .../unit/reservations/bookings.rpc.test.ts | 20 +- 15 files changed, 948 insertions(+), 398 deletions(-) create mode 100644 server/src/nest/reservations/reservation-writes.service.ts delete mode 100644 server/tests/unit/nest/accommodations.event-parity.test.ts diff --git a/server/scripts/test-new-service-baseline.json b/server/scripts/test-new-service-baseline.json index 2e655a029..b818f531b 100644 --- a/server/scripts/test-new-service-baseline.json +++ b/server/scripts/test-new-service-baseline.json @@ -34,7 +34,6 @@ "tests/unit/mcp/tools-transit.test.ts": 1, "tests/unit/nest-rpc/plugin-guards.test.ts": 1, "tests/unit/nest-rpc/rpc-kit/registry.service.test.ts": 3, - "tests/unit/nest/accommodations.event-parity.test.ts": 1, "tests/unit/nest/accommodations.service.test.ts": 1, "tests/unit/nest/addons.service.test.ts": 2, "tests/unit/nest/admin.service.test.ts": 11, diff --git a/server/src/nest/reservations/reservation-writes.service.ts b/server/src/nest/reservations/reservation-writes.service.ts new file mode 100644 index 000000000..769dc6d60 --- /dev/null +++ b/server/src/nest/reservations/reservation-writes.service.ts @@ -0,0 +1,141 @@ +import type { Reservation, User } from '../../types'; +import { DomainError } from '../common/domain-error'; +import { restTripWriter, type TripWriter } from '../common/trip-writer'; +import { AirtrailLinkService } from '../integrations/airtrail-link.service'; +import { RealtimeService } from '../realtime/realtime.service'; +import { + ReservationsService, + type CostEvent, + type CreateReservationData, + type UpdateReservationData, +} from './reservations.service'; +import { Injectable } from '@nestjs/common'; + +/** The linked cost a booking write takes, as ReservationsService.createWithCost reads it. */ +type BudgetEntry = Parameters[2]; +/** The day plan order a reorder writes, as ReservationsService.updatePositions reads it. */ +type Positions = Parameters[1]; + +const bookingNotFound = (mcpMessage: string) => new DomainError(404, 'Reservation not found', { mcpMessage }); + +/** + * The booking writes REST and MCP share, one method per use case: the write, + * then every event it sends and the notice to fellow travellers, so both + * surfaces send the same things in the same order. + * + * Each runs after its caller checked the trip and the references (REST's + * TripAccessGuard with reservation_edit and the controller's foreign id check, + * the tool's own checks on MCP), writes in ReservationsService's transaction and + * sends once that committed. The day stop a hotel booking brings is announced by + * ReservationsService itself, before the events here, to every socket. + * + * MCP used to send these events from the tool and had drifted: no booking + * notice, no push of an edited AirTrail flight (the next pull reverted the + * edit), no `accommodation:deleted` when a transport still carried a stay, its + * own day key on `reservation:positions`, and the hotel link announced a stay it + * had not changed. + */ +@Injectable() +export class ReservationWritesService { + constructor( + private readonly reservations: ReservationsService, + private readonly airtrailLink: AirtrailLinkService, + private readonly realtime: RealtimeService, + ) {} + + /** The REST caller of a booking write: skips the sender's socket, as the routes always did. */ + restWriter(tripId: string, user: User, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** A new booking with its linked cost, and the stay a hotel booking brings. */ + async createBooking(tripId: string, data: CreateReservationData, entry: BudgetEntry, writer: TripWriter) { + const { reservation, accommodationCreated, costEvents } = await this.reservations.createWithCost( + tripId, + data, + entry, + ); + if (accommodationCreated) writer.events.emit('accommodation:created', {}); + this.sendCost(costEvents, writer); + writer.events.emit('reservation:created', { reservation }); + await this.reservations.notifyBookingChange(tripId, writer.userId, data.title, data.type ?? ''); + return { reservation }; + } + + /** + * An edit of a booking the caller already read (`current`), with the costs it + * re-files. A stay the edit wrote for the first time is `accommodation:created`, + * one it changed `accommodation:updated`. A linked AirTrail flight is pushed + * back to AirTrail, or the next pull would overwrite the edit. + * + * @txIndependent the booking write commits in one transaction; the AirTrail + * push after it is network I/O against another system and may detach the link + * on its own, so it never shares that transaction. + */ + async updateBooking( + id: string, + tripId: string, + data: UpdateReservationData, + current: Reservation, + entry: BudgetEntry, + writer: TripWriter, + ) { + const { reservation, accommodationChanged, accommodationCreated, costEvents } = + await this.reservations.updateWithCost(id, tripId, data, current, entry); + if (accommodationCreated) writer.events.emit('accommodation:created', {}); + else if (accommodationChanged) writer.events.emit('accommodation:updated', {}); + this.sendCost(costEvents, writer); + writer.events.emit('reservation:updated', { reservation }); + // Fire and forget, under the importer's credentials (#214). + if (reservation.external_source === 'airtrail' && reservation.sync_enabled) { + void this.airtrailLink.pushReservationToAirtrail(Number(reservation.id), Number(tripId)).catch(() => {}); + } + await this.reservations.notifyBookingChange( + tripId, + writer.userId, + data.title || current.title, + data.type || current.type || '', + ); + return { reservation }; + } + + /** + * A booking deleted with its stay and its linked costs. `mcpNotFound` is the + * wording the tool has always answered a missing booking with. + */ + async deleteBooking( + id: string | number, + tripId: string | number, + writer: TripWriter, + mcpNotFound = 'Reservation not found.', + ) { + const { deleted, accommodationDeleted, deletedBudgetItemIds } = await this.reservations.remove(id, tripId); + if (!deleted) throw bookingNotFound(mcpNotFound); + if (accommodationDeleted) + writer.events.emit('accommodation:deleted', { accommodationId: deleted.accommodation_id }); + for (const itemId of deletedBudgetItemIds) writer.events.emit('budget:deleted', { itemId }); + writer.events.emit('reservation:deleted', { reservationId: Number(id) }); + await this.reservations.notifyBookingChange(tripId, writer.userId, deleted.title, deleted.type || ''); + return { success: true }; + } + + /** The order of the bookings in the day plan, of one day when `dayId` names it. */ + async reorderBookings( + tripId: string | number, + positions: Positions, + dayId: number | string | null | undefined, + writer: TripWriter, + ) { + await this.reservations.updatePositions(tripId, positions, dayId); + // REST's `day_id` is the canonical key. The MCP tool always named it `dayId`, + // so its event keeps that key next to it for a client that reads it. + writer.events.emit( + 'reservation:positions', + writer.surface === 'mcp' ? { positions, day_id: dayId, dayId } : { positions, day_id: dayId }, + ); + } + + private sendCost(events: readonly CostEvent[], writer: TripWriter): void { + for (const e of events) writer.events.emit(e.event, e.payload); + } +} diff --git a/server/src/nest/reservations/reservations.controller.ts b/server/src/nest/reservations/reservations.controller.ts index 3a608935c..a4435aca4 100644 --- a/server/src/nest/reservations/reservations.controller.ts +++ b/server/src/nest/reservations/reservations.controller.ts @@ -1,8 +1,8 @@ import type { User } from '../../types'; import { CurrentUser } from '../auth-core/current-user.decorator'; import { JwtAuthGuard } from '../auth-core/jwt-auth.guard'; -import { AirtrailLinkService } from '../integrations/airtrail-link.service'; import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; +import { ReservationWritesService } from './reservation-writes.service'; import { ReservationCreateDto, ReservationUpdateDto, @@ -25,6 +25,8 @@ type ReservationBody = Record & { * trip access (404), 'reservation_edit' permission (403), create 201 / rest 200, * the bespoke 404 bodies, the accommodation + budget side effects, the booking * notifications, and all WebSocket broadcasts with the forwarded X-Socket-Id. + * Create, edit, delete and reorder run the use cases of ReservationWritesService, + * which the MCP tools share, so both surfaces send the same events. * Invalid bodies (missing title, non-array positions/user_ids) now 400 through * the global ZodValidationPipe envelope instead of the bespoke strings. * /positions is declared before /:id so it wins over the param. @@ -37,8 +39,7 @@ type ReservationBody = Record & { export class ReservationsController { constructor( private readonly reservations: ReservationsService, - // Injected from AirtrailCoreModule — the split that retired airtrail.bridge. - private readonly airtrailLink: AirtrailLinkService, + private readonly writes: ReservationWritesService, ) {} @Get() @@ -60,18 +61,12 @@ export class ReservationsController { // at a rate frozen now (#2525). const budgetEntry = await this.reservations.withFrozenRate(tripId, body.create_budget_entry); // The booking and its linked cost are one write. - const { reservation, accommodationCreated, costEvents } = await this.reservations.createWithCost( + return this.writes.createBooking( tripId, body as never, budgetEntry, + this.writes.restWriter(tripId, user, socketId), ); - if (accommodationCreated) { - this.reservations.broadcast(tripId, 'accommodation:created', {}, socketId); - } - this.reservations.announceCost(tripId, costEvents, socketId); - this.reservations.broadcast(tripId, 'reservation:created', { reservation }, socketId); - await this.reservations.notifyBookingChange(tripId, user.id, body.title, body.type ?? ''); - return { reservation }; } @RequirePermission('reservation_edit') @@ -84,16 +79,11 @@ export class ReservationsController { ) { // The legacy signature declares day_plan_position required, but the wire // contract tolerates absent values (bind NULL) — see the shared schema. - await this.reservations.updatePositions( + await this.writes.reorderBookings( tripId, body.positions as { id: number; day_plan_position: number }[], body.day_id, - ); - this.reservations.broadcast( - tripId, - 'reservation:positions', - { positions: body.positions, day_id: body.day_id }, - socketId, + this.writes.restWriter(tripId, user, socketId), ); return { success: true }; } @@ -113,27 +103,15 @@ export class ReservationsController { throw new HttpException({ error: 'Reservation not found' }, 404); } await this.rejectForeignReferences(tripId, body); - // The booking and its linked cost are one write. - const { reservation, accommodationChanged, costEvents } = await this.reservations.updateWithCost( + // The booking and its linked cost are one write; a linked AirTrail flight is pushed back after it. + return this.writes.updateBooking( id, tripId, body as never, current, body.create_budget_entry, + this.writes.restWriter(tripId, user, socketId), ); - if (accommodationChanged) { - this.reservations.broadcast(tripId, 'accommodation:updated', {}, socketId); - } - const cur = current as { title: string; type?: string }; - this.reservations.announceCost(tripId, costEvents, socketId); - this.reservations.broadcast(tripId, 'reservation:updated', { reservation }, socketId); - // Push a locally-edited AirTrail flight back to AirTrail (fire-and-forget, - // under the importer's credentials — see airtrailSync). #214 - if ((reservation as any)?.external_source === 'airtrail' && (reservation as any)?.sync_enabled) { - void this.airtrailLink.pushReservationToAirtrail(Number((reservation as any).id), Number(tripId)).catch(() => {}); - } - await this.reservations.notifyBookingChange(tripId, user.id, body.title || cur.title, body.type || cur.type || ''); - return { reservation }; } @RequirePermission('reservation_edit') @@ -166,24 +144,8 @@ export class ReservationsController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const { deleted, accommodationDeleted, deletedBudgetItemIds } = await this.reservations.remove(id, tripId); - if (!deleted) { - throw new HttpException({ error: 'Reservation not found' }, 404); - } - if (accommodationDeleted) { - this.reservations.broadcast( - tripId, - 'accommodation:deleted', - { accommodationId: deleted.accommodation_id }, - socketId, - ); - } - for (const itemId of deletedBudgetItemIds) { - this.reservations.broadcast(tripId, 'budget:deleted', { itemId }, socketId); - } - this.reservations.broadcast(tripId, 'reservation:deleted', { reservationId: Number(id) }, socketId); - await this.reservations.notifyBookingChange(tripId, user.id, deleted.title, deleted.type || ''); - return { success: true }; + // A missing booking answers { error: 'Reservation not found' } with 404. + return this.writes.deleteBooking(id, tripId, this.writes.restWriter(tripId, user, socketId)); } /** diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index 577186ee6..a58a0949d 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -9,6 +9,7 @@ import { z } from 'zod'; import { BudgetService } from '../budget/budget.service'; import { noAccess, permissionDenied } from '../../mcp/tools/_shared'; import { ReservationsService } from './reservations.service'; +import { ReservationWritesService } from './reservation-writes.service'; import { DaysService } from '../days/days.service'; import { findByIata } from '../airports/airports.data'; import type { EndpointInput } from './reservations.service'; @@ -330,6 +331,9 @@ export class ReservationsMcp { // positionally. private readonly assignments: AssignmentsService, private readonly guards: McpToolGuardsService, + // The booking use cases the REST routes run too: the events, the booking + // notice and the AirTrail push leave from there, not from the tools. + private readonly writes: ReservationWritesService, ) {} @Tool({ @@ -388,20 +392,13 @@ export class ReservationsMcp { const metadata = price != null ? { price: String(price) } : undefined; - // The booking and its linked cost are one write, the same service path REST takes. - const { reservation, accommodationCreated, costEvents } = await this.reservations.createWithCost(String(tripId), { + // The booking and its linked cost are one write, the same use case REST runs. + const { reservation } = await this.writes.createBooking(String(tripId), { title, type, reservation_time, reservation_end_time, url, location, confirmation_number, notes, day_id, place_id, assignment_id, create_accommodation: createAccommodation, metadata, - }, linkedCost(price, budget_category || type)); - - if (accommodationCreated) { - this.guards.safeBroadcast(tripId, 'accommodation:created', {}); - } - for (const { event, payload } of costEvents) this.guards.safeBroadcast(tripId, event, payload); - - this.guards.safeBroadcast(tripId, 'reservation:created', { reservation }); + }, linkedCost(price, budget_category || type), await this.guards.tripWriter(tripId, ctx.userId)); return ok({ reservation }); } @@ -446,15 +443,12 @@ export class ReservationsMcp { return errorResult('assignment_id does not belong to this trip.'); // The booking and the linked expenses a type change re-files are one write, - // the same service path REST and the plugin RPC take. - const { reservation, accommodationChanged, costEvents } = await this.reservations.updateWithCost(String(reservationId), String(tripId), { + // the same use case REST runs. + const { reservation } = await this.writes.updateBooking(String(reservationId), String(tripId), { title, type, reservation_time, reservation_end_time, url, location, confirmation_number, notes, status, place_id: place_id !== undefined ? place_id ?? undefined : undefined, assignment_id: assignment_id !== undefined ? assignment_id ?? undefined : undefined, - }, existing, undefined); - if (accommodationChanged) this.guards.safeBroadcast(tripId, 'accommodation:updated', {}); - for (const { event, payload } of costEvents) this.guards.safeBroadcast(tripId, event, payload); - this.guards.safeBroadcast(tripId, 'reservation:updated', { reservation }); + }, existing, undefined, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ reservation }); } @@ -471,14 +465,7 @@ export class ReservationsMcp { async deleteReservation({ tripId, reservationId }: { tripId: number; reservationId: number }, ctx: McpContext) { if (!(await this.reservations.verifyTripAccess(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('reservation_edit', tripId, ctx.userId))) return permissionDenied(); - const { deleted, accommodationDeleted, deletedBudgetItemIds } = await this.reservations.remove(reservationId, tripId); - if (!deleted) return errorResult('Reservation not found.'); - if (accommodationDeleted) { - this.guards.safeBroadcast(tripId, 'accommodation:deleted', { accommodationId: deleted.accommodation_id }); - } - for (const itemId of deletedBudgetItemIds) this.guards.safeBroadcast(tripId, 'budget:deleted', { itemId }); - this.guards.safeBroadcast(tripId, 'reservation:deleted', { reservationId }); - return ok({ success: true }); + return ok(await this.writes.deleteBooking(reservationId, tripId, await this.guards.tripWriter(tripId, ctx.userId))); } @Tool({ @@ -542,8 +529,7 @@ export class ReservationsMcp { if (dayId && !(await this.days.getDay(dayId, tripId))) return errorResult('dayId does not belong to this trip.'); - await this.reservations.updatePositions(tripId, positions, dayId); - this.guards.safeBroadcast(tripId, 'reservation:positions', { positions, dayId }); + await this.writes.reorderBookings(tripId, positions, dayId, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } @@ -582,16 +568,14 @@ export class ReservationsMcp { if (!(await this.days.getDay(end_day_id, tripId))) return errorResult('end_day_id does not belong to this trip.'); - const isNewAccommodation = !current.accommodation_id; - const { reservation } = await this.reservations.update(reservationId, tripId, { + // The edit the booking form's PUT makes, through the same use case: a stay + // written for the first time is announced as created, a changed one as updated. + const { reservation } = await this.writes.updateBooking(String(reservationId), String(tripId), { place_id, type: current.type, status: current.status as string, create_accommodation: { place_id, start_day_id, end_day_id, check_in: check_in || undefined, check_out: check_out || undefined }, - }, current); - - this.guards.safeBroadcast(tripId, isNewAccommodation ? 'accommodation:created' : 'accommodation:updated', {}); - this.guards.safeBroadcast(tripId, 'reservation:updated', { reservation }); + }, current, undefined, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ reservation, accommodation_id: reservation?.accommodation_id ?? null }); } @@ -736,7 +720,7 @@ export class ReservationsMcp { if (price != null) meta.price = String(price); - const { reservation, costEvents } = await this.reservations.createWithCost(String(tripId), { + const { reservation } = await this.writes.createBooking(String(tripId), { title, type, reservation_time: departureTime, @@ -751,10 +735,7 @@ export class ReservationsMcp { metadata: Object.keys(meta).length > 0 ? meta : undefined, endpoints: transportEndpoints, needs_review, - }, linkedCost(price, budget_category || type)); - for (const { event, payload } of costEvents) this.guards.safeBroadcast(tripId, event, payload); - - this.guards.safeBroadcast(tripId, 'reservation:created', { reservation }); + }, linkedCost(price, budget_category || type), await this.guards.tripWriter(tripId, ctx.userId)); return ok({ reservation }); } @@ -856,8 +837,9 @@ export class ReservationsMcp { if (applied.endpointsChanged) resolvedEndpoints = applied.endpoints; } - // One write with the linked expenses a type change re-files, as on update_reservation. - const { reservation, accommodationChanged, costEvents } = await this.reservations.updateWithCost(String(reservationId), String(tripId), { + // One write with the linked expenses a type change re-files, as on update_reservation; + // a linked AirTrail flight is pushed back, as on REST. + const { reservation } = await this.writes.updateBooking(String(reservationId), String(tripId), { title, type, reservation_time: departureTime, @@ -871,10 +853,7 @@ export class ReservationsMcp { metadata: nextMetadata, endpoints: resolvedEndpoints, needs_review, - }, existing, undefined); - if (accommodationChanged) this.guards.safeBroadcast(tripId, 'accommodation:updated', {}); - for (const { event, payload } of costEvents) this.guards.safeBroadcast(tripId, event, payload); - this.guards.safeBroadcast(tripId, 'reservation:updated', { reservation }); + }, existing, undefined, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ reservation }); } @@ -891,10 +870,9 @@ export class ReservationsMcp { async deleteTransport({ tripId, reservationId }: { tripId: number; reservationId: number }, ctx: McpContext) { if (!(await this.reservations.verifyTripAccess(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('reservation_edit', tripId, ctx.userId))) return permissionDenied(); - const { deleted, deletedBudgetItemIds } = await this.reservations.remove(reservationId, tripId); - if (!deleted) return errorResult('Transport not found.'); - for (const itemId of deletedBudgetItemIds) this.guards.safeBroadcast(tripId, 'budget:deleted', { itemId }); - this.guards.safeBroadcast(tripId, 'reservation:deleted', { reservationId }); - return ok({ success: true }); + // The same cascade as delete_reservation: a row a REST edit retyped from a + // hotel still takes its stay along, and says so with accommodation:deleted. + const writer = await this.guards.tripWriter(tripId, ctx.userId); + return ok(await this.writes.deleteBooking(reservationId, tripId, writer, 'Transport not found.')); } } diff --git a/server/src/nest/reservations/reservations.module.ts b/server/src/nest/reservations/reservations.module.ts index e8f5c151c..0bdb6de14 100644 --- a/server/src/nest/reservations/reservations.module.ts +++ b/server/src/nest/reservations/reservations.module.ts @@ -20,6 +20,7 @@ import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; import { NotificationsModule } from '../notifications/notifications.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { RealtimeModule } from '../realtime/realtime.module'; +import { ReservationWritesService } from './reservation-writes.service'; import { ReservationsReadModule } from './reservations-read.module'; import { ReservationsController } from './reservations.controller'; import { ReservationsMcp } from './reservations.mcp'; @@ -79,7 +80,8 @@ import { Module } from '@nestjs/common'; ]), ], controllers: [ReservationsController, UpcomingReservationsController], - providers: [ReservationsService, ReservationsMcp, ReservationsRpc], + // ReservationWritesService: the booking use cases the controller and the MCP tools share. + providers: [ReservationsService, ReservationWritesService, ReservationsMcp, ReservationsRpc], // For in-container consumers (ReservationsRpc, TripsService, BookingImportService). exports: [ReservationsService], }) diff --git a/server/src/nest/reservations/reservations.rpc.ts b/server/src/nest/reservations/reservations.rpc.ts index 04196c0a9..9983b1ce7 100644 --- a/server/src/nest/reservations/reservations.rpc.ts +++ b/server/src/nest/reservations/reservations.rpc.ts @@ -68,14 +68,17 @@ export class ReservationsRpc { if (!current) throw new ForbiddenResource(`no reservation ${reservationId} on trip ${tripId}`); await this.requireOwnReferences(tripId, input); const i = input as { title?: string; type?: string; create_budget_entry?: unknown }; - const { reservation, accommodationChanged, costEvents } = await this.reservations.updateWithCost( - String(reservationId), - String(tripId), - input as never, - current, - i.create_budget_entry as never, - ); - if (accommodationChanged) this.realtime.broadcast(tripId, 'accommodation:updated', {}, undefined); + const { reservation, accommodationChanged, accommodationCreated, costEvents } = + await this.reservations.updateWithCost( + String(reservationId), + String(tripId), + input as never, + current, + i.create_budget_entry as never, + ); + // A stay this edit wrote for the first time is new, as the REST route announces it. + if (accommodationCreated) this.realtime.broadcast(tripId, 'accommodation:created', {}, undefined); + else if (accommodationChanged) this.realtime.broadcast(tripId, 'accommodation:updated', {}, undefined); const cur = current as { title: string; type?: string }; this.reservations.announceCost(tripId, costEvents, undefined); this.realtime.broadcast(tripId, 'reservation:updated', { reservation }, undefined); diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 30f59ad56..e94390194 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -43,7 +43,7 @@ import { decodeJson } from '../../utils/json-column'; type Trip = TripAccess; type BudgetEntry = { total_price?: number; category?: string; currency?: string | null; exchange_rate?: number } | undefined; -/** A booking's cost as the service writes it: the entry, dated on the booking's day (bookingDate), never by the caller. */ +/** A booking's cost as written: the entry, dated on the booking's day (bookingDate), never by the caller. */ type DatedBudgetEntry = (NonNullable & { expense_date?: string | null }) | undefined; /** What a booking write did to its linked costs, for the caller to send once the write has committed. */ @@ -842,7 +842,7 @@ export class ReservationsService { return written; } - private async updateInTx(id: string | number, tripId: string | number, data: UpdateReservationData, current: Reservation): Promise<{ reservation: ReservationRow; accommodationChanged: boolean; stayMirror: AccommodationMirror }> { + private async updateInTx(id: string | number, tripId: string | number, data: UpdateReservationData, current: Reservation) { const { title, reservation_time, reservation_end_time, location, confirmation_number, notes, url, day_id, end_day_id, place_id, assignment_id, @@ -891,6 +891,7 @@ export class ReservationsService { const accExists = await this.dayAccommodationsRepo.existsInTrip(accIdForRead(resolvedAccId)!, this.rowIdNum(tripId)); if (!accExists) resolvedAccId = null; } + const hadStay = Boolean(resolvedAccId); if (type === 'hotel' && create_accommodation) { const { place_id: accPlaceId, start_day_id, end_day_id, check_in, check_out, confirmation: accConf } = create_accommodation; if (start_day_id && end_day_id) { @@ -1027,7 +1028,7 @@ export class ReservationsService { // The caller passed the pre-checked `current` row, so the re-select can't // miss (legacy typed this any). const reservation = (await this.getReservationWithJoins(id))!; - return { reservation, accommodationChanged, stayMirror }; + return { reservation, accommodationChanged, accommodationCreated: !hadStay && Boolean(resolvedAccId), stayMirror }; } /** The accommodation + budget-item + reservation deletes are one logical diff --git a/server/tests/integration/broadcast-parity/bookings.cases.ts b/server/tests/integration/broadcast-parity/bookings.cases.ts index 8f9736a0e..3bb8fa366 100644 --- a/server/tests/integration/broadcast-parity/bookings.cases.ts +++ b/server/tests/integration/broadcast-parity/bookings.cases.ts @@ -1,4 +1,133 @@ +import { Reservations } from '../../../src/db/entities/Reservations.entity'; +import { + AirtrailClient, + type AirtrailAirport, + type AirtrailFlightRaw, +} from '../../../src/nest/integrations/airtrail.client'; +import { AirtrailService } from '../../../src/nest/integrations/airtrail.service'; +import { makeBudgetItem } from '../../helpers/factories/budget'; +import { updateRows } from '../../helpers/factories/rows'; import type { CaseGroup } from './types'; +import { MikroORM } from '@mikro-orm/core'; +import type { INestApplication } from '@nestjs/common'; + +import { vi } from 'vitest'; + +/** + * The notice a booking write sends the other travellers of the trip + * (`notifyBookingChange`), which reaches their bell a beat after the write + * answered. MCP sends it since REST and the tools share the booking use cases. + */ +const BOOKING_NOTICE = { event: 'notification:new', user: 'others' } as const; + +/** A hotel booking's stay, written by the booking and announced to every socket before the booking's own events. */ +const STAY_STOP_ADDED = [ + { event: 'assignment:created', all: true }, + { event: 'place:updated', all: true }, + { event: 'assignment:reordered', all: true }, +] as const; + +/** The stop of a stay that goes, and the order of its day. */ +const STAY_STOP_REMOVED = [ + { event: 'assignment:deleted', all: true }, + { event: 'assignment:reordered', all: true }, +] as const; + +/** A walk and one subway ride in Osaka, the morning of the scene's first day (09:00 to 09:30 local). */ +const TRANSIT_FROM = { name: 'Namba', lat: 34.667, lng: 135.501 }; +const TRANSIT_TO = { name: 'Umeda', lat: 34.702, lng: 135.496 }; +const TRANSIT_ITINERARY = { + startTime: '2027-05-03T00:00:00Z', + endTime: '2027-05-03T00:30:00Z', + duration: 1800, + transfers: 0, + walkSeconds: 300, + legs: [ + { + mode: 'WALK', + from: { ...TRANSIT_FROM, name: 'START', time: '2027-05-03T00:00:00Z', scheduledTime: null, track: null }, + to: { + name: 'Namba Station', + lat: 34.666, + lng: 135.5, + time: '2027-05-03T00:05:00Z', + scheduledTime: null, + track: null, + }, + duration: 300, + distance: 300, + headsign: null, + line: null, + lineColor: null, + lineTextColor: null, + agency: null, + intermediateStops: 0, + geometry: null, + geometryPrecision: 6, + }, + { + mode: 'SUBWAY', + from: { + name: 'Namba Station', + lat: 34.666, + lng: 135.5, + time: '2027-05-03T00:05:00Z', + scheduledTime: null, + track: '1', + }, + to: { ...TRANSIT_TO, name: 'END', time: '2027-05-03T00:30:00Z', scheduledTime: null, track: '2' }, + duration: 1500, + distance: 5000, + headsign: 'Umeda', + line: 'M', + lineColor: '#E5171F', + lineTextColor: '#FFFFFF', + agency: 'Osaka Metro', + intermediateStops: 3, + geometry: 'encoded', + geometryPrecision: 6, + }, + ], +}; + +function airport(id: number, iata: string, name: string, lat: number, lon: number, tz: string): AirtrailAirport { + return { id, icao: null, iata, name, lat, lon, tz, country: null }; +} + +/** The flight the stubbed AirTrail account holds, on the scene's first day. */ +const AIRTRAIL_FLIGHT: AirtrailFlightRaw = { + id: 501, + from: airport(1, 'BRU', 'Brussels', 50.9014, 4.4844, 'Europe/Brussels'), + to: airport(2, 'HEL', 'Helsinki-Vantaa', 60.3172, 24.9633, 'Europe/Helsinki'), + date: '2027-05-03', + datePrecision: 'day', + departure: '2027-05-03T06:00:00.000+00:00', + arrival: '2027-05-03T09:30:00.000+00:00', + departureScheduled: null, + arrivalScheduled: null, + airline: { id: 1, icao: 'FIN', iata: 'AY', name: 'Finnair' }, + flightNumber: 'AY1502', + aircraft: null, + aircraftReg: null, + note: null, + seats: [], +}; + +/** A connected AirTrail account holding AIRTRAIL_FLIGHT, without the network. */ +function stubAirtrail(app: INestApplication): () => void { + const spies = [ + vi + .spyOn(app.get(AirtrailService), 'getAirtrailCredentials') + .mockResolvedValue({ baseUrl: 'https://airtrail.example.test', apiKey: 'key', allowInsecureTls: false }), + vi.spyOn(app.get(AirtrailClient), 'listFlights').mockResolvedValue([AIRTRAIL_FLIGHT]), + ]; + return () => spies.forEach((spy) => spy.mockRestore()); +} + +/** Sets a booking's type, as an edit of the booking form does. */ +function retype(app: INestApplication, id: number, type: string): Promise { + return updateRows(app.get(MikroORM), Reservations, { id }, { type }); +} /** Bookings: reservations, transports, transit, stays and the AirTrail import. */ export const bookings: CaseGroup = { @@ -22,29 +151,213 @@ export const bookings: CaseGroup = { // The stay, the ping for its partner hotel booking, then the check-in stop // the day plan gains (with the stop's time stamped on the hotel place), // which the booking tab has not drawn yet either. - expect: [ - 'accommodation:created', - 'reservation:created', - { event: 'assignment:created', all: true }, - { event: 'place:updated', all: true }, - { event: 'assignment:reordered', all: true }, - ], + expect: ['accommodation:created', 'reservation:created', ...STAY_STOP_ADDED], + }, + { + tool: 'update_accommodation', + needs: ['stay'], + args: (s) => ({ tripId: s.trip.id, accommodationId: s.stay.id, notes: 'Late arrival' }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/accommodations/${s.stay.id}`, + body: () => ({ notes: 'Late arrival' }), + }, + expect: ['accommodation:updated'], + }, + { + tool: 'delete_accommodation', + needs: ['stay'], + args: (s) => ({ tripId: s.trip.id, accommodationId: s.stay.id }), + rest: { method: 'delete', path: (s) => `/api/trips/${s.trip.id}/accommodations/${s.stay.id}` }, + // The stop goes first, then the partner booking, then the stay (MCP names + // it `id` too and lists the bookings it took along). + expect: [...STAY_STOP_REMOVED, 'reservation:deleted', 'accommodation:deleted'], + }, + { + tool: 'import_airtrail_flights', + args: (s) => ({ tripId: s.trip.id, flightIds: [String(AIRTRAIL_FLIGHT.id)] }), + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/reservations/import/airtrail`, + body: () => ({ flightIds: [String(AIRTRAIL_FLIGHT.id)] }), + }, + stub: stubAirtrail, + expect: ['reservation:created'], + }, + { + // A hotel with its stay and a price: the stop, then the stay ping, the cost and the booking. + tool: 'create_reservation', + needs: ['place', 'member'], + args: (s) => ({ + tripId: s.trip.id, + title: `Hotel ${s.n}`, + type: 'hotel', + place_id: s.place.id, + start_day_id: s.days[0].id, + end_day_id: s.days[1].id, + price: 240, + }), + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/reservations`, + body: (s) => ({ + title: `Hotel ${s.n}`, + type: 'hotel', + place_id: s.place.id, + create_accommodation: { place_id: s.place.id, start_day_id: s.days[0].id, end_day_id: s.days[1].id }, + create_budget_entry: { total_price: 240, category: 'hotel' }, + }), + }, + settle: true, + expect: [...STAY_STOP_ADDED, 'accommodation:created', 'budget:created', 'reservation:created', BOOKING_NOTICE], + }, + { + tool: 'update_reservation', + needs: ['stay', 'member'], + args: (s) => ({ tripId: s.trip.id, reservationId: s.stay.reservationId, status: 'confirmed' }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/reservations/${s.stay.reservationId}`, + body: () => ({ status: 'confirmed' }), + }, + settle: true, + expect: ['reservation:updated', BOOKING_NOTICE], + }, + { + // A hotel booking takes its stay (and the stay's stop) and its linked cost along. + tool: 'delete_reservation', + needs: ['stay', 'member'], + setup: async (s, app) => { + await makeBudgetItem(app.get(MikroORM), s.trip.id, { name: `Hotel ${s.n}`, reservation: s.stay.reservationId }); + }, + args: (s) => ({ tripId: s.trip.id, reservationId: s.stay.reservationId }), + rest: { method: 'delete', path: (s) => `/api/trips/${s.trip.id}/reservations/${s.stay.reservationId}` }, + settle: true, + expect: [...STAY_STOP_REMOVED, 'accommodation:deleted', 'budget:deleted', 'reservation:deleted', BOOKING_NOTICE], + }, + { + tool: 'set_reservation_travelers', + needs: ['transport', 'member'], + args: (s) => ({ tripId: s.trip.id, reservationId: s.transport.id, user_ids: [s.member.id] }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/reservations/${s.transport.id}/travelers`, + body: (s) => ({ user_ids: [s.member.id] }), + }, + expect: ['reservation:travelers-updated'], + }, + { + // MCP sends REST's `day_id` and keeps the `dayId` its tool always sent. + tool: 'reorder_reservations', + needs: ['transport'], + args: (s) => ({ + tripId: s.trip.id, + positions: [{ id: s.transport.id, day_plan_position: 2 }], + dayId: s.days[0].id, + }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/reservations/positions`, + body: (s) => ({ positions: [{ id: s.transport.id, day_plan_position: 2 }], day_id: s.days[0].id }), + }, + expect: ['reservation:positions'], + }, + { + // A hotel booking without a stay yet gets one, as the booking form's PUT + // gives it: the stop, then the new stay, then the booking. + tool: 'link_hotel_accommodation', + needs: ['transport', 'place', 'member'], + setup: async (s, app) => { + await retype(app, s.transport.id, 'hotel'); + }, + args: (s) => ({ + tripId: s.trip.id, + reservationId: s.transport.id, + place_id: s.place.id, + start_day_id: s.days[0].id, + end_day_id: s.days[1].id, + }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/reservations/${s.transport.id}`, + body: (s) => ({ + type: 'hotel', + place_id: s.place.id, + create_accommodation: { place_id: s.place.id, start_day_id: s.days[0].id, end_day_id: s.days[1].id }, + }), + }, + settle: true, + expect: [...STAY_STOP_ADDED, 'accommodation:created', 'reservation:updated', BOOKING_NOTICE], + }, + { + tool: 'create_transport', + needs: ['member'], + args: (s) => ({ tripId: s.trip.id, type: 'train', title: `ICE ${s.n}`, start_day_id: s.days[0].id, price: 89 }), + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/reservations`, + body: (s) => ({ + title: `ICE ${s.n}`, + type: 'train', + day_id: s.days[0].id, + create_budget_entry: { total_price: 89, category: 'train' }, + }), + }, + settle: true, + expect: ['budget:created', 'reservation:created', BOOKING_NOTICE], + }, + { + tool: 'update_transport', + needs: ['transport', 'member'], + args: (s) => ({ tripId: s.trip.id, reservationId: s.transport.id, status: 'confirmed' }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/reservations/${s.transport.id}`, + body: () => ({ status: 'confirmed' }), + }, + settle: true, + expect: ['reservation:updated', BOOKING_NOTICE], + }, + { + // A hotel booking a REST edit retyped as a train still carries its stay: + // deleting it takes the stay along, and the sidebar hears of it on MCP too. + tool: 'delete_transport', + needs: ['stay', 'member'], + setup: async (s, app) => { + await retype(app, s.stay.reservationId, 'train'); + }, + args: (s) => ({ tripId: s.trip.id, reservationId: s.stay.reservationId }), + rest: { method: 'delete', path: (s) => `/api/trips/${s.trip.id}/reservations/${s.stay.reservationId}` }, + settle: true, + expect: [...STAY_STOP_REMOVED, 'accommodation:deleted', 'reservation:deleted', BOOKING_NOTICE], + }, + { + tool: 'create_transit_journey', + needs: ['member'], + args: (s) => ({ + tripId: s.trip.id, + dayId: s.days[0].id, + from: TRANSIT_FROM, + to: TRANSIT_TO, + itinerary: TRANSIT_ITINERARY, + }), + // The transit panel saves the itinerary it picked through the booking route. + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/reservations`, + body: (s) => ({ + title: 'Namba → Umeda', + type: 'transit', + status: 'confirmed', + day_id: s.days[0].id, + end_day_id: s.days[0].id, + reservation_time: '2027-05-03T09:00', + reservation_end_time: '2027-05-03T09:30', + }), + }, + settle: true, + expect: ['reservation:created', BOOKING_NOTICE], }, ], exempt: [], - pending: [ - 'update_accommodation', - 'delete_accommodation', - 'import_airtrail_flights', - 'create_reservation', - 'update_reservation', - 'delete_reservation', - 'set_reservation_travelers', - 'reorder_reservations', - 'link_hotel_accommodation', - 'create_transport', - 'update_transport', - 'delete_transport', - 'create_transit_journey', - ], }; diff --git a/server/tests/unit/mcp/tools-assignments-reservations-extra.test.ts b/server/tests/unit/mcp/tools-assignments-reservations-extra.test.ts index b38d946ee..1b016bba9 100644 --- a/server/tests/unit/mcp/tools-assignments-reservations-extra.test.ts +++ b/server/tests/unit/mcp/tools-assignments-reservations-extra.test.ts @@ -238,6 +238,23 @@ describe('Tool: reorder_reservations', () => { }); }); + it("names the day under REST's day_id and keeps the dayId the tool always sent", async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const res = createReservation(testDb, trip.id, { title: 'Train', type: 'train', day_id: day.id }); + const positions = [{ id: res.id, day_plan_position: 3 }]; + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'reorder_reservations', + arguments: { tripId: trip.id, positions, dayId: day.id }, + }); + }); + expect(broadcastMock.mock.calls.filter((c) => c[1] === 'reservation:positions')).toEqual([ + [trip.id, 'reservation:positions', { positions, day_id: day.id, dayId: day.id, _source: 'mcp' }], + ]); + }); + 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-reservations.test.ts b/server/tests/unit/mcp/tools-reservations.test.ts index db09d3120..2907f48f5 100644 --- a/server/tests/unit/mcp/tools-reservations.test.ts +++ b/server/tests/unit/mcp/tools-reservations.test.ts @@ -9,6 +9,7 @@ import { ReservationDayPositions } from '../../../src/db/entities/ReservationDay import { ReservationTravelers } from '../../../src/db/entities/ReservationTravelers.entity'; import { Reservations } from '../../../src/db/entities/Reservations.entity'; import { Trips } from '../../../src/db/entities/Trips.entity'; +import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; import { createUser, createTrip, @@ -478,6 +479,51 @@ describe('Tool: delete_reservation', () => { }); }); +// --------------------------------------------------------------------------- +// The notice to fellow travellers +// --------------------------------------------------------------------------- + +describe('Reservation tools: the booking notice', () => { + // The booking routes always told the other travellers; the tools did not. + it('create, update, the hotel link and delete each tell the fellow travellers, as REST does', async () => { + const notify = vi.spyOn(ReservationsService.prototype, 'notifyBookingChange').mockResolvedValue(undefined); + try { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id, { day_number: 1 }); + const day2 = createDay(testDb, trip.id, { day_number: 2 }); + const hotel = createPlace(testDb, trip.id, { name: 'Ritz' }); + await withHarness(user.id, async (h) => { + const created = parseToolResult( + await h.client.callTool({ + name: 'create_reservation', + arguments: { tripId: trip.id, title: 'Ritz', type: 'hotel' }, + }), + ) as { reservation: { id: number } }; + const reservationId = created.reservation.id; + await h.client.callTool({ + name: 'update_reservation', + arguments: { tripId: trip.id, reservationId, title: 'Ritz Paris' }, + }); + await h.client.callTool({ + name: 'link_hotel_accommodation', + arguments: { tripId: trip.id, reservationId, place_id: hotel.id, start_day_id: day1.id, end_day_id: day2.id }, + }); + await h.client.callTool({ name: 'delete_reservation', arguments: { tripId: trip.id, reservationId } }); + }); + // The trip id travels as text from some callers and as a number from others; the notice reads both alike. + expect(notify.mock.calls.map(([tripId, ...rest]) => [Number(tripId), ...rest])).toEqual([ + [trip.id, user.id, 'Ritz', 'hotel'], + [trip.id, user.id, 'Ritz Paris', 'hotel'], + [trip.id, user.id, 'Ritz Paris', 'hotel'], + [trip.id, user.id, 'Ritz Paris', 'hotel'], + ]); + } finally { + notify.mockRestore(); + } + }); +}); + // --------------------------------------------------------------------------- // link_hotel_accommodation // --------------------------------------------------------------------------- @@ -512,6 +558,8 @@ describe('Tool: link_hotel_accommodation', () => { expect(Number.isInteger(data.reservation.accommodation_id)).toBe(true); expect(Number.isInteger(data.accommodation_id)).toBe(true); expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'accommodation:created', expect.any(Object)); + // A new stay is created, not also updated: the booking form's PUT sends the same. + expect(broadcastMock.mock.calls.some((c) => c[1] === 'accommodation:updated')).toBe(false); }); }); @@ -540,6 +588,7 @@ describe('Tool: link_hotel_accommodation', () => { }); // Update link + broadcastMock.mockClear(); await withHarness(user.id, async (h) => { await h.client.callTool({ name: 'link_hotel_accommodation', @@ -552,6 +601,8 @@ describe('Tool: link_hotel_accommodation', () => { }, }); expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'accommodation:updated', expect.any(Object)); + // The stay already existed, so it is not announced as a new one. + expect(broadcastMock.mock.calls.some((c) => c[1] === 'accommodation:created')).toBe(false); }); }); diff --git a/server/tests/unit/mcp/tools-transports.test.ts b/server/tests/unit/mcp/tools-transports.test.ts index 6cd941475..03dceaa52 100644 --- a/server/tests/unit/mcp/tools-transports.test.ts +++ b/server/tests/unit/mcp/tools-transports.test.ts @@ -6,9 +6,12 @@ */ import { db as testDb } from '../../../src/db/database'; import { BudgetItems } from '../../../src/db/entities/BudgetItems.entity'; +import { DayAccommodations } from '../../../src/db/entities/DayAccommodations.entity'; import { ReservationEndpoints } from '../../../src/db/entities/ReservationEndpoints.entity'; import { Reservations } from '../../../src/db/entities/Reservations.entity'; -import { createUser, createTrip, createDay } from '../../helpers/factories'; +import { AirtrailLinkService } from '../../../src/nest/integrations/airtrail-link.service'; +import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { createUser, createTrip, createDay, createPlace } from '../../helpers/factories'; import { makeBudgetItem } from '../../helpers/factories/budget'; import { countRows, findRow, findRows, insertRow, updateRows } from '../../helpers/factories/rows'; import { makeDay } from '../../helpers/factories/trips'; @@ -524,6 +527,48 @@ describe('Tool: delete_transport', () => { }); }); + // A hotel booking the booking form retyped as a train keeps its stay link. The + // tool used to take the stay along without saying so, so the planner's hotel + // block outlived it until a reload; REST always sent accommodation:deleted. + it('takes along the stay a booking retyped from a hotel still carries, and says so as REST does', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id, { day_number: 1 }); + const day2 = createDay(testDb, trip.id, { day_number: 2 }); + const hotel = createPlace(testDb, trip.id, { name: 'Grand Hotel' }); + await withHarness(user.id, async (h) => { + const created = await h.client.callTool({ + name: 'create_reservation', + arguments: { + tripId: trip.id, + title: 'Grand Hotel', + type: 'hotel', + place_id: hotel.id, + start_day_id: day1.id, + end_day_id: day2.id, + }, + }); + const { reservation } = parseToolResult(created) as { reservation: { id: number } }; + await updateRows(orm, Reservations, { id: reservation.id }, { type: 'train' }); + broadcastMock.mockClear(); + + const result = await h.client.callTool({ + name: 'delete_transport', + arguments: { tripId: trip.id, reservationId: reservation.id }, + }); + expect(parseToolResult(result)).toEqual({ success: true }); + expect(await countRows(orm, DayAccommodations, { trip: trip.id })).toBe(0); + expect(broadcastMock.mock.calls.map((c) => c[1]).slice(-2)).toEqual([ + 'accommodation:deleted', + 'reservation:deleted', + ]); + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'accommodation:deleted', { + accommodationId: expect.anything(), + _source: 'mcp', + }); + }); + }); + it('404s an unknown reservation id', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -552,6 +597,75 @@ describe('Tool: delete_transport', () => { }); }); +// --------------------------------------------------------------------------- +// What a transport write does past the trip's room, as on REST +// --------------------------------------------------------------------------- + +describe('Transport tools: the booking notice and the AirTrail push', () => { + it('create, update and delete each tell the fellow travellers, as the booking routes do', async () => { + const notify = vi.spyOn(ReservationsService.prototype, 'notifyBookingChange').mockResolvedValue(undefined); + try { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + const created = parseToolResult( + await h.client.callTool({ + name: 'create_transport', + arguments: { tripId: trip.id, type: 'train', title: 'ICE 1001' }, + }), + ) as { reservation: { id: number } }; + const reservationId = created.reservation.id; + await h.client.callTool({ + name: 'update_transport', + arguments: { tripId: trip.id, reservationId, title: 'ICE 1003' }, + }); + await h.client.callTool({ name: 'delete_transport', arguments: { tripId: trip.id, reservationId } }); + }); + // The trip id travels as text from some callers and as a number from others; the notice reads both alike. + expect(notify.mock.calls.map(([tripId, ...rest]) => [Number(tripId), ...rest])).toEqual([ + [trip.id, user.id, 'ICE 1001', 'train'], + [trip.id, user.id, 'ICE 1003', 'train'], + [trip.id, user.id, 'ICE 1003', 'train'], + ]); + } finally { + notify.mockRestore(); + } + }); + + // Before, an edit over MCP stayed local and the next AirTrail pull overwrote it. + it('pushes an edit of a linked AirTrail flight back to AirTrail, and only of a linked one', async () => { + const push = vi.spyOn(AirtrailLinkService.prototype, 'pushReservationToAirtrail').mockResolvedValue(undefined); + try { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + let linked = 0; + await withHarness(user.id, async (h) => { + const create = async (title: string) => + ( + parseToolResult( + await h.client.callTool({ + name: 'create_transport', + arguments: { tripId: trip.id, type: 'flight', title, endpoints: flightEndpoints }, + }), + ) as { reservation: { id: number } } + ).reservation.id; + linked = await create('AY1502'); + const local = await create('LX638'); + await updateRows(orm, Reservations, { id: linked }, { external_source: 'airtrail', external_id: '501' }); + for (const reservationId of [linked, local]) { + await h.client.callTool({ + name: 'update_transport', + arguments: { tripId: trip.id, reservationId, status: 'confirmed' }, + }); + } + }); + expect(push.mock.calls).toEqual([[linked, trip.id]]); + } finally { + push.mockRestore(); + } + }); +}); + // --------------------------------------------------------------------------- // Multi-leg bookings (#1914) // diff --git a/server/tests/unit/nest/accommodations.controller.test.ts b/server/tests/unit/nest/accommodations.controller.test.ts index 2cb738b72..b618f896d 100644 --- a/server/tests/unit/nest/accommodations.controller.test.ts +++ b/server/tests/unit/nest/accommodations.controller.test.ts @@ -1,5 +1,6 @@ import { AccommodationsController } from '../../../src/nest/accommodations/accommodations.controller'; import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; +import { DomainError } from '../../../src/nest/common/domain-error'; import type { User } from '../../../src/types'; import { HttpException } from '@nestjs/common'; @@ -268,3 +269,101 @@ describe('AccommodationsController (parity with the legacy accommodations sub-ro }); }); }); + +/** + * The gate and the checks every surface shares, called on the use cases + * directly with an MCP writer. That REST and MCP send the same events is held + * by the broadcast parity group (tests/integration/broadcast-parity/bookings.cases.ts). + */ +describe('stay use cases: the shared gate and checks', () => { + const writer = (role = 'user') => ({ + userId: 1, + role, + surface: 'mcp' as const, + events: { emit: vi.fn(), emitAll: vi.fn() }, + }); + const stay = { place_id: 2, start_day_id: 10, end_day_id: 11 }; + const cancelled = { + linkedReservationId: 4, + deletedBudgetItemId: null, + linkedReservationIds: [4, 5], + deletedBudgetItemIds: [], + mirror: noMirror, + }; + + it("ACC-UC-001: a trip the writer cannot see refuses with REST's 404 and MCP's own wording", async () => { + const svc = makeService({ trips: { findAccessible: vi.fn().mockResolvedValue(undefined) } }); + const err = await svc.createStay(5, stay, writer()).catch((e: unknown) => e); + expect(err).toBeInstanceOf(DomainError); + expect((err as DomainError).getStatus()).toBe(404); + expect((err as DomainError).toBody()).toEqual({ error: 'Trip not found' }); + expect((err as DomainError).mcpMessage).toBe('Trip not found or access denied.'); + }); + + it('ACC-UC-002: a writer without day_edit is refused before anything is read or written', async () => { + const create = vi.fn(); + const svc = makeService({ permissions: { checkPermission: vi.fn().mockResolvedValue(false) }, create }); + const err = (await svc.createStay(5, stay, writer()).catch((e: unknown) => e)) as DomainError; + expect(err.getStatus()).toBe(403); + expect(err.mcpMessage).toBe('You do not have permission to perform this action on this trip.'); + expect(create).not.toHaveBeenCalled(); + }); + + it("ACC-UC-003: the permission check sees the writer's role and whether it is a member", async () => { + const checkPermission = vi.fn().mockResolvedValue(true); + const svc = makeService({ + permissions: { checkPermission }, + trips: { findAccessible: vi.fn().mockResolvedValue({ user_id: 2 }) }, + get: vi.fn().mockResolvedValue({ id: 9 }), + remove: vi.fn().mockResolvedValue(cancelled), + }); + await svc.deleteStay(5, 9, writer('admin')); + expect(checkPermission).toHaveBeenCalledWith('day_edit', 'admin', 2, 1, true); + }); + + it('ACC-UC-004: every reference miss is listed for MCP, the first one answers REST', async () => { + const validateRefs = vi.fn().mockResolvedValue([{ message: 'Place not found' }, { message: 'End day not found' }]); + const svc = makeService({ validateRefs }); + const err = (await svc.createStay(5, stay, writer()).catch((e: unknown) => e)) as DomainError; + expect(err.getStatus()).toBe(404); + expect(err.toBody()).toEqual({ error: 'Place not found' }); + expect(err.mcpMessage).toBe('Place not found, End day not found'); + }); + + it('ACC-UC-005: an edit checks the references it names, on MCP as on REST', async () => { + // The MCP tool used to write a place or day it never looked up. + const update = vi.fn(); + const validateRefs = vi.fn().mockResolvedValue([{ message: 'Place not found' }]); + const svc = makeService({ validateRefs, update, get: vi.fn().mockResolvedValue({ id: 9 }) }); + const err = (await svc.updateStay(5, 9, { place_id: 99 }, writer()).catch((e: unknown) => e)) as DomainError; + expect(err.getStatus()).toBe(404); + expect(validateRefs).toHaveBeenCalledWith(5, 99, undefined, undefined); + expect(update).not.toHaveBeenCalled(); + }); + + it('ACC-UC-006: an edit or a cancellation of a stay that is not on the trip is a 404', async () => { + const svc = makeService({ get: vi.fn().mockResolvedValue(undefined) }); + const errors = (await Promise.all( + [svc.updateStay(5, 9, {}, writer()), svc.deleteStay(5, 9, writer())].map((p) => p.catch((e: unknown) => e)), + )) as DomainError[]; + for (const err of errors) { + expect(err.toBody()).toEqual({ error: 'Accommodation not found' }); + expect(err.mcpMessage).toBe('Accommodation not found.'); + } + }); + + it("ACC-UC-007: a cancellation over MCP keeps the keys its tool always sent next to REST's", async () => { + const svc = makeService({ + get: vi.fn().mockResolvedValue({ id: 9 }), + remove: vi.fn().mockResolvedValue(cancelled), + }); + const mcp = writer(); + await svc.deleteStay(5, 9, mcp); + expect(mcp.events.emit).toHaveBeenLastCalledWith('accommodation:deleted', { + accommodationId: 9, + id: 9, + linkedReservationId: 4, + linkedReservationIds: [4, 5], + }); + }); +}); diff --git a/server/tests/unit/nest/accommodations.event-parity.test.ts b/server/tests/unit/nest/accommodations.event-parity.test.ts deleted file mode 100644 index c43b5bae3..000000000 --- a/server/tests/unit/nest/accommodations.event-parity.test.ts +++ /dev/null @@ -1,193 +0,0 @@ -import { AccommodationsController } from '../../../src/nest/accommodations/accommodations.controller'; -import { AccommodationsMcp } from '../../../src/nest/accommodations/accommodations.mcp'; -import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; -import { DomainError } from '../../../src/nest/common/domain-error'; -import { McpToolGuardsService } from '../../../src/nest/mcp-shared/mcp-tool-guards.service'; -import type { User } from '../../../src/types'; - -import { describe, it, expect, vi } from 'vitest'; - -/** - * REST and MCP are adapters over the same stay use cases, so a client sees the - * same events whichever of the two made the change. Before, the MCP tools sent - * no reservation:created on a booking and no reservation:deleted/budget:deleted - * on its cascade. Each case runs one write through both surfaces against the - * same stubbed persistence and compares the event names in order. - */ - -const user = { id: 1, role: 'user', email: 'u@example.test' } as User; -const stop = { id: 77, day_id: 10 }; -const mirror = { created: stop, moved: null, updated: [], removed: [{ id: 70, dayId: 10 }], stamped: null }; - -function makeService(realtime: { broadcast: ReturnType }, overrides: Record = {}) { - const stubs: Record = { - trips: { findAccessible: vi.fn().mockResolvedValue({ user_id: 1 }) }, - permissions: { checkPermission: vi.fn().mockResolvedValue(true) }, - realtime, - assignments: { reconcile: vi.fn() }, - dayAssignmentsRepo: { listIdsForDay: vi.fn().mockResolvedValue([70, 77]) }, - validateAccommodationRefs: vi.fn().mockResolvedValue([]), - getAccommodation: vi.fn().mockResolvedValue({ id: 9 }), - createAccommodation: vi.fn().mockResolvedValue({ accommodation: { id: 9 }, mirror }), - updateAccommodation: vi.fn().mockResolvedValue({ accommodation: { id: 9 }, mirror }), - deleteAccommodation: vi.fn().mockResolvedValue({ - linkedReservationId: 4, - deletedBudgetItemId: 7, - linkedReservationIds: [4, 5], - deletedBudgetItemIds: [7], - mirror, - }), - ...overrides, - }; - return Object.assign( - Object.create(AccommodationsService.prototype) as object, - stubs, - ) as unknown as AccommodationsService; -} - -function surfaces(overrides: Record = {}) { - const restRealtime = { broadcast: vi.fn() }; - const mcpRealtime = { broadcast: vi.fn() }; - const rest = new AccommodationsController(makeService(restRealtime, overrides)); - const mcpService = makeService(mcpRealtime, overrides); - const guards = new McpToolGuardsService( - { getOwnerId: vi.fn().mockResolvedValue(1) } as never, - { getRole: vi.fn().mockResolvedValue('user') } as never, - { checkPermission: vi.fn().mockResolvedValue(true) } as never, - mcpRealtime as never, - ); - const mcp = new AccommodationsMcp(mcpService, {} as never, guards, {} as never); - const names = (rt: { broadcast: ReturnType }) => rt.broadcast.mock.calls.map((c) => c[1] as string); - const payloadOf = (rt: { broadcast: ReturnType }, event: string) => - rt.broadcast.mock.calls.find((c) => c[1] === event)?.[2] as Record | undefined; - return { rest, mcp, restRealtime, mcpRealtime, names, payloadOf, ctx: { userId: 1, scopes: null } as never }; -} - -describe('accommodation events: REST == MCP', () => { - it('ACC-PARITY-001: a booking sends the same events on both surfaces, reservation:created included', async () => { - const s = surfaces(); - await s.rest.create(user, '5', { place_id: 2, start_day_id: 10, end_day_id: 11 }, 'sock'); - await s.mcp.createAccommodation({ tripId: 5, place_id: 2, start_day_id: 10, end_day_id: 11 }, s.ctx); - expect(s.names(s.mcpRealtime)).toEqual(s.names(s.restRealtime)); - expect(s.names(s.restRealtime)).toEqual([ - 'accommodation:created', - 'reservation:created', - 'assignment:deleted', - 'assignment:created', - 'assignment:reordered', - ]); - expect(s.payloadOf(s.mcpRealtime, 'reservation:created')).toEqual({ _source: 'mcp' }); - }); - - it('ACC-PARITY-002: an edit sends the same events on both surfaces', async () => { - const s = surfaces(); - await s.rest.update(user, '5', '9', { notes: 'x' }, 'sock'); - await s.mcp.updateAccommodation({ tripId: 5, accommodationId: 9, notes: 'x' }, s.ctx); - expect(s.names(s.mcpRealtime)).toEqual(s.names(s.restRealtime)); - expect(s.names(s.restRealtime)[0]).toBe('accommodation:updated'); - }); - - it('ACC-PARITY-003: a cancellation sends the reservation and budget cascade on both surfaces', async () => { - const s = surfaces(); - await s.rest.remove(user, '5', '9', 'sock'); - await s.mcp.deleteAccommodation({ tripId: 5, accommodationId: 9 }, s.ctx); - expect(s.names(s.mcpRealtime)).toEqual(s.names(s.restRealtime)); - expect(s.names(s.restRealtime).slice(-4)).toEqual([ - 'reservation:deleted', - 'reservation:deleted', - 'budget:deleted', - 'accommodation:deleted', - ]); - // REST keeps its canonical payload; MCP adds it next to the keys its tool always sent. - expect(s.payloadOf(s.restRealtime, 'accommodation:deleted')).toEqual({ accommodationId: 9 }); - expect(s.payloadOf(s.mcpRealtime, 'accommodation:deleted')).toEqual({ - accommodationId: 9, - id: 9, - linkedReservationId: 4, - linkedReservationIds: [4, 5], - _source: 'mcp', - }); - }); -}); - -describe('stay use cases: the shared gate and checks', () => { - const writer = (role = 'user') => ({ - userId: 1, - role, - surface: 'mcp' as const, - events: { emit: vi.fn(), emitAll: vi.fn() }, - }); - - it("ACC-UC-001: a trip the writer cannot see refuses with REST's 404 and MCP's own wording", async () => { - const svc = makeService( - { broadcast: vi.fn() }, - { trips: { findAccessible: vi.fn().mockResolvedValue(undefined) } }, - ); - const err = await svc - .createStay(5, { place_id: 2, start_day_id: 10, end_day_id: 11 }, writer()) - .catch((e: unknown) => e); - expect(err).toBeInstanceOf(DomainError); - expect((err as DomainError).getStatus()).toBe(404); - expect((err as DomainError).toBody()).toEqual({ error: 'Trip not found' }); - expect((err as DomainError).mcpMessage).toBe('Trip not found or access denied.'); - }); - - it('ACC-UC-002: a writer without day_edit is refused before anything is read or written', async () => { - const createAccommodation = vi.fn(); - const svc = makeService( - { broadcast: vi.fn() }, - { permissions: { checkPermission: vi.fn().mockResolvedValue(false) }, createAccommodation }, - ); - const err = (await svc - .createStay(5, { place_id: 2, start_day_id: 10, end_day_id: 11 }, writer()) - .catch((e: unknown) => e)) as DomainError; - expect(err.getStatus()).toBe(403); - expect(err.mcpMessage).toBe('You do not have permission to perform this action on this trip.'); - expect(createAccommodation).not.toHaveBeenCalled(); - }); - - it("ACC-UC-003: the permission check sees the writer's role and whether it is a member", async () => { - const checkPermission = vi.fn().mockResolvedValue(true); - const svc = makeService( - { broadcast: vi.fn() }, - { permissions: { checkPermission }, trips: { findAccessible: vi.fn().mockResolvedValue({ user_id: 2 }) } }, - ); - await svc.deleteStay(5, 9, writer('admin')); - expect(checkPermission).toHaveBeenCalledWith('day_edit', 'admin', 2, 1, true); - }); - - it('ACC-UC-004: every reference miss is listed for MCP, the first one answers REST', async () => { - const validateAccommodationRefs = vi - .fn() - .mockResolvedValue([{ message: 'Place not found' }, { message: 'End day not found' }]); - const svc = makeService({ broadcast: vi.fn() }, { validateAccommodationRefs }); - const err = (await svc - .createStay(5, { place_id: 2, start_day_id: 10, end_day_id: 11 }, writer()) - .catch((e: unknown) => e)) as DomainError; - expect(err.getStatus()).toBe(404); - expect(err.toBody()).toEqual({ error: 'Place not found' }); - expect(err.mcpMessage).toBe('Place not found, End day not found'); - }); - - it('ACC-UC-005: an edit checks the references it names, on MCP as on REST', async () => { - // The MCP tool used to write a place or day it never looked up. - const updateAccommodation = vi.fn(); - const validateAccommodationRefs = vi.fn().mockResolvedValue([{ message: 'Place not found' }]); - const svc = makeService({ broadcast: vi.fn() }, { validateAccommodationRefs, updateAccommodation }); - const err = (await svc.updateStay(5, 9, { place_id: 99 }, writer()).catch((e: unknown) => e)) as DomainError; - expect(err.getStatus()).toBe(404); - expect(validateAccommodationRefs).toHaveBeenCalledWith(5, 99, undefined, undefined); - expect(updateAccommodation).not.toHaveBeenCalled(); - }); - - it('ACC-UC-006: an edit or a cancellation of a stay that is not on the trip is a 404', async () => { - const svc = makeService({ broadcast: vi.fn() }, { getAccommodation: vi.fn().mockResolvedValue(undefined) }); - const errors = (await Promise.all( - [svc.updateStay(5, 9, {}, writer()), svc.deleteStay(5, 9, writer())].map((p) => p.catch((e: unknown) => e)), - )) as DomainError[]; - for (const err of errors) { - expect(err.toBody()).toEqual({ error: 'Accommodation not found' }); - expect(err.mcpMessage).toBe('Accommodation not found.'); - } - }); -}); diff --git a/server/tests/unit/nest/reservations.controller.test.ts b/server/tests/unit/nest/reservations.controller.test.ts index dd0292886..3950907fb 100644 --- a/server/tests/unit/nest/reservations.controller.test.ts +++ b/server/tests/unit/nest/reservations.controller.test.ts @@ -1,4 +1,5 @@ import type { AirtrailLinkService } from '../../../src/nest/integrations/airtrail-link.service'; +import { ReservationWritesService } from '../../../src/nest/reservations/reservation-writes.service'; import { ReservationsController } from '../../../src/nest/reservations/reservations.controller'; import type { ReservationsService } from '../../../src/nest/reservations/reservations.service'; import type { User } from '../../../src/types'; @@ -10,9 +11,8 @@ const user = { id: 1, role: 'user', email: 'u@example.test' } as User; const trip = { id: 5, user_id: 1 }; // Fire-and-forget write-back trigger, injected since airtrail.bridge died. -const airtrailLink = { - pushReservationToAirtrail: vi.fn().mockResolvedValue(undefined), -} as unknown as AirtrailLinkService; +const pushReservationToAirtrail = vi.fn().mockResolvedValue(undefined); +const airtrailLink = { pushReservationToAirtrail } as unknown as AirtrailLinkService; function makeService(overrides: Partial = {}): ReservationsService { return { @@ -29,6 +29,20 @@ function makeService(overrides: Partial = {}): Reservations } as unknown as ReservationsService; } +/** + * The route over the real booking use cases (ReservationWritesService) with the + * service stubbed, so these cases pin what the route writes and sends, not how + * it calls into the use case. The realtime transport is the stub's `broadcast`. + */ +function controllerOver(svc: ReservationsService): ReservationsController { + const writes = Object.assign(Object.create(ReservationWritesService.prototype) as ReservationWritesService, { + reservations: svc, + airtrailLink, + realtime: { broadcast: (svc as unknown as { broadcast: unknown }).broadcast }, + }); + return new ReservationsController(svc, writes); +} + async function thrown(fn: () => unknown): Promise<{ status: number; body: unknown }> { try { await fn(); @@ -43,7 +57,7 @@ async function thrown(fn: () => unknown): Promise<{ status: number; body: unknow describe('ReservationsController (parity with the legacy /api/trips/:tripId/reservations route)', () => { it('GET / returns reservations', async () => { const svc = makeService({ list: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); - expect(await new ReservationsController(svc, airtrailLink).list(user, '5')).toEqual({ reservations: [{ id: 1 }] }); + expect(await controllerOver(svc).list(user, '5')).toEqual({ reservations: [{ id: 1 }] }); }); describe('POST /', () => { @@ -55,29 +69,24 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const createWithCost = vi .fn() .mockReturnValue({ reservation: { id: 9 }, accommodationCreated: true, costEvents }); - const order: string[] = []; - const broadcast = vi.fn((_tripId: string, event: string) => { - order.push(event); - }); - const announceCost = vi.fn(() => { - order.push('cost'); - }); + const broadcast = vi.fn(); const notifyBookingChange = vi.fn(); const svc = makeService({ createWithCost, broadcast, - announceCost, notifyBookingChange, } as Partial); const body = { title: 'Hotel', type: 'lodging', create_budget_entry: { total_price: 200 } }; - expect(await new ReservationsController(svc, airtrailLink).create(user, '5', body, 'sock')).toEqual({ + expect(await controllerOver(svc).create(user, '5', body, 'sock')).toEqual({ reservation: { id: 9 }, }); expect(createWithCost).toHaveBeenCalledWith('5', body, { total_price: 200 }); - expect(announceCost).toHaveBeenCalledWith('5', costEvents, 'sock'); - expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:created', {}, 'sock'); - expect(broadcast).toHaveBeenCalledWith('5', 'reservation:created', { reservation: { id: 9 } }, 'sock'); - expect(order).toEqual(['accommodation:created', 'cost', 'reservation:created']); + // Every event skips the socket that sent the request, the cost between the stay and the booking. + expect(broadcast.mock.calls).toEqual([ + ['5', 'accommodation:created', {}, 'sock'], + ['5', 'budget:created', { item: { id: 3 } }, 'sock'], + ['5', 'reservation:created', { reservation: { id: 9 } }, 'sock'], + ]); expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Hotel', 'lodging'); }); @@ -88,7 +97,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese referencesOutsideTrip: vi.fn().mockReturnValue(['accommodation_id']), } as Partial); const body = { title: 'Hotel', accommodation_id: 4711 }; - expect(await thrown(() => new ReservationsController(svc, airtrailLink).create(user, '5', body))).toEqual({ + expect(await thrown(() => controllerOver(svc).create(user, '5', body))).toEqual({ status: 400, body: { error: 'Not part of this trip: accommodation_id' }, }); @@ -104,7 +113,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const body = { title: 'Hotel', place_id: 4711 }; // Not 'Not part of this trip': an id that is part of nothing would send // the caller looking for it on another trip. - expect(await thrown(() => new ReservationsController(svc, airtrailLink).create(user, '5', body))).toEqual({ + expect(await thrown(() => controllerOver(svc).create(user, '5', body))).toEqual({ status: 400, body: { error: 'Unknown reference: place_id' }, }); @@ -117,11 +126,10 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese referencesOutsideTrip: vi.fn().mockReturnValue(['place_id']), unresolvedReferences: vi.fn().mockReturnValue(['place_id']), } as Partial); - expect( - await thrown(() => - new ReservationsController(svc, airtrailLink).create(user, '5', { title: 'Hotel', place_id: 4711 }), - ), - ).toEqual({ status: 400, body: { error: 'Not part of this trip: place_id' } }); + expect(await thrown(() => controllerOver(svc).create(user, '5', { title: 'Hotel', place_id: 4711 }))).toEqual({ + status: 400, + body: { error: 'Not part of this trip: place_id' }, + }); }); // #2525: an imported booking quoted in dollars. The price has to reach the linked @@ -140,7 +148,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese type: 'hotel', create_budget_entry: { total_price: 801.76, currency: 'usd' }, }; - await new ReservationsController(svc, airtrailLink).create(user, '5', body, 'sock'); + await controllerOver(svc).create(user, '5', body, 'sock'); expect(withFrozenRate).toHaveBeenCalledWith('5', { total_price: 801.76, currency: 'usd' }); expect(createWithCost).toHaveBeenCalledWith('5', body, { total_price: 801.76, @@ -159,14 +167,9 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const broadcast = vi.fn(); const svc = makeService({ updatePositions, broadcast } as Partial); const positions = [{ id: 1, day_plan_position: 0 }]; - expect( - await new ReservationsController(svc, airtrailLink).updatePositions( - user, - '5', - { positions, day_id: 3 }, - 'sock', - ), - ).toEqual({ success: true }); + expect(await controllerOver(svc).updatePositions(user, '5', { positions, day_id: 3 }, 'sock')).toEqual({ + success: true, + }); expect(updatePositions).toHaveBeenCalledWith('5', positions, 3); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:positions', { positions, day_id: 3 }, 'sock'); }); @@ -175,36 +178,78 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese describe('PUT /:id', () => { it('404 when the reservation is missing', async () => { const svc = makeService({ getReservation: vi.fn().mockReturnValue(undefined) } as Partial); - expect( - await thrown(() => new ReservationsController(svc, airtrailLink).update(user, '5', '9', { title: 'X' })), - ).toEqual({ status: 404, body: { error: 'Reservation not found' } }); + expect(await thrown(() => controllerOver(svc).update(user, '5', '9', { title: 'X' }))).toEqual({ + status: 404, + body: { error: 'Reservation not found' }, + }); }); it('updates the booking and its cost in one write, broadcasts + notifies with current fallbacks', async () => { const current = { title: 'Old', type: 'lodging' }; const getReservation = vi.fn().mockReturnValue(current); const costEvents = [{ event: 'budget:updated', payload: { item: { id: 3 } } }]; - const updateWithCost = vi - .fn() - .mockReturnValue({ reservation: { id: 9 }, accommodationChanged: true, costEvents }); + const updateWithCost = vi.fn().mockReturnValue({ + reservation: { id: 9 }, + accommodationChanged: true, + accommodationCreated: false, + costEvents, + }); const broadcast = vi.fn(); - const announceCost = vi.fn(); const notifyBookingChange = vi.fn(); const svc = makeService({ getReservation, updateWithCost, broadcast, - announceCost, notifyBookingChange, } as Partial); const body = { create_budget_entry: { total_price: 50 } }; - await new ReservationsController(svc, airtrailLink).update(user, '5', '9', body, 'sock'); + expect(await controllerOver(svc).update(user, '5', '9', body, 'sock')).toEqual({ reservation: { id: 9 } }); expect(updateWithCost).toHaveBeenCalledWith('9', '5', body, current, { total_price: 50 }); - expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:updated', {}, 'sock'); - expect(announceCost).toHaveBeenCalledWith('5', costEvents, 'sock'); + expect(broadcast.mock.calls).toEqual([ + ['5', 'accommodation:updated', {}, 'sock'], + ['5', 'budget:updated', { item: { id: 3 } }, 'sock'], + ['5', 'reservation:updated', { reservation: { id: 9 } }, 'sock'], + ]); expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Old', 'lodging'); }); + // G6: the hotel link tool makes this same edit, and both name a stay written for the first time created. + it('announces a stay the edit wrote for the first time as created, not as updated', async () => { + const broadcast = vi.fn(); + const svc = makeService({ + getReservation: vi.fn().mockReturnValue({ title: 'Hotel', type: 'hotel' }), + updateWithCost: vi.fn().mockReturnValue({ + reservation: { id: 9 }, + accommodationChanged: true, + accommodationCreated: true, + costEvents: [], + }), + broadcast, + } as Partial); + await controllerOver(svc).update(user, '5', '9', { type: 'hotel' }, 'sock'); + expect(broadcast.mock.calls.map((c) => c[1])).toEqual(['accommodation:created', 'reservation:updated']); + }); + + it('pushes a linked AirTrail flight back after the write, and leaves an unlinked one alone', async () => { + pushReservationToAirtrail.mockClear(); + const linked = { id: 9, external_source: 'airtrail', sync_enabled: 1 }; + const updateWithCost = vi + .fn() + .mockReturnValueOnce({ reservation: linked, accommodationChanged: false, costEvents: [] }) + .mockReturnValueOnce({ + reservation: { ...linked, sync_enabled: 0 }, + accommodationChanged: false, + costEvents: [], + }); + const svc = makeService({ + getReservation: vi.fn().mockReturnValue({ title: 'AY1502', type: 'flight' }), + updateWithCost, + } as Partial); + await controllerOver(svc).update(user, '5', '9', { notes: 'Gate B' }, 'sock'); + await controllerOver(svc).update(user, '5', '9', { notes: 'Gate C' }, 'sock'); + expect(pushReservationToAirtrail.mock.calls).toEqual([[9, 5]]); + }); + it('400s on a body id belonging to another trip, without writing', async () => { const update = vi.fn(); const svc = makeService({ @@ -212,11 +257,10 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese updateWithCost: update, referencesOutsideTrip: vi.fn().mockReturnValue(['day_id', 'place_id']), } as Partial); - expect( - await thrown(() => - new ReservationsController(svc, airtrailLink).update(user, '5', '9', { day_id: 1, place_id: 2 }), - ), - ).toEqual({ status: 400, body: { error: 'Not part of this trip: day_id, place_id' } }); + expect(await thrown(() => controllerOver(svc).update(user, '5', '9', { day_id: 1, place_id: 2 }))).toEqual({ + status: 400, + body: { error: 'Not part of this trip: day_id, place_id' }, + }); expect(update).not.toHaveBeenCalled(); }); @@ -229,11 +273,10 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese } as Partial); // The reporter's request: a foreign-key error used to reach the caller as // a bare 500 here. - expect( - await thrown(() => - new ReservationsController(svc, airtrailLink).update(user, '5', '9', { day_id: 1, place_id: 2 }), - ), - ).toEqual({ status: 400, body: { error: 'Unknown reference: day_id, place_id' } }); + expect(await thrown(() => controllerOver(svc).update(user, '5', '9', { day_id: 1, place_id: 2 }))).toEqual({ + status: 400, + body: { error: 'Unknown reference: day_id, place_id' }, + }); expect(update).not.toHaveBeenCalled(); }); }); @@ -244,11 +287,10 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese it('404 when the reservation is off-trip / missing', async () => { const svc = makeService({ setTravelers: vi.fn().mockReturnValue(null) } as Partial); - expect( - await thrown(() => - new ReservationsController(svc, airtrailLink).updateTravelers(user, '5', '9', { user_ids: [1] }), - ), - ).toEqual({ status: 404, body: { error: 'Reservation not found' } }); + expect(await thrown(() => controllerOver(svc).updateTravelers(user, '5', '9', { user_ids: [1] }))).toEqual({ + status: 404, + body: { error: 'Reservation not found' }, + }); }); it('assigns travelers, broadcasts, and returns { travelers, reservation }', async () => { @@ -257,9 +299,10 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const setTravelers = vi.fn().mockReturnValue({ travelers, reservation }); const broadcast = vi.fn(); const svc = makeService({ setTravelers, broadcast } as Partial); - expect( - await new ReservationsController(svc, airtrailLink).updateTravelers(user, '5', '9', { user_ids: [2] }, 'sock'), - ).toEqual({ travelers, reservation }); + expect(await controllerOver(svc).updateTravelers(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ + travelers, + reservation, + }); expect(setTravelers).toHaveBeenCalledWith('9', '5', [2]); expect(broadcast).toHaveBeenCalledWith( '5', @@ -282,7 +325,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese }), broadcast, } as Partial); - expect(await thrown(() => new ReservationsController(svc, airtrailLink).remove(user, '5', '9'))).toEqual({ + expect(await thrown(() => controllerOver(svc).remove(user, '5', '9'))).toEqual({ status: 404, body: { error: 'Reservation not found' }, }); @@ -299,12 +342,14 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const broadcast = vi.fn(); const notifyBookingChange = vi.fn(); const svc = makeService({ remove, broadcast, notifyBookingChange } as Partial); - expect(await new ReservationsController(svc, airtrailLink).remove(user, '5', '9', 'sock')).toEqual({ + expect(await controllerOver(svc).remove(user, '5', '9', 'sock')).toEqual({ success: true, }); - expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:deleted', { accommodationId: 3 }, 'sock'); - expect(broadcast).toHaveBeenCalledWith('5', 'budget:deleted', { itemId: 7 }, 'sock'); - expect(broadcast).toHaveBeenCalledWith('5', 'reservation:deleted', { reservationId: 9 }, 'sock'); + expect(broadcast.mock.calls).toEqual([ + ['5', 'accommodation:deleted', { accommodationId: 3 }, 'sock'], + ['5', 'budget:deleted', { itemId: 7 }, 'sock'], + ['5', 'reservation:deleted', { reservationId: 9 }, 'sock'], + ]); expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Hotel', 'lodging'); }); @@ -317,7 +362,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese }); const broadcast = vi.fn(); const svc = makeService({ remove, broadcast } as Partial); - await new ReservationsController(svc, airtrailLink).remove(user, '5', '9', 'sock'); + await controllerOver(svc).remove(user, '5', '9', 'sock'); expect(broadcast.mock.calls).toEqual([ ['5', 'budget:deleted', { itemId: 7 }, 'sock'], ['5', 'budget:deleted', { itemId: 8 }, 'sock'], @@ -334,7 +379,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese }); const broadcast = vi.fn(); const svc = makeService({ remove, broadcast } as Partial); - await new ReservationsController(svc, airtrailLink).remove(user, '5', '9', 'sock'); + await controllerOver(svc).remove(user, '5', '9', 'sock'); expect(broadcast.mock.calls).toEqual([['5', 'reservation:deleted', { reservationId: 9 }, 'sock']]); }); }); diff --git a/server/tests/unit/reservations/bookings.rpc.test.ts b/server/tests/unit/reservations/bookings.rpc.test.ts index 8de1632b5..0d8ce434d 100644 --- a/server/tests/unit/reservations/bookings.rpc.test.ts +++ b/server/tests/unit/reservations/bookings.rpc.test.ts @@ -41,6 +41,8 @@ function build( opts: { canEdit?: boolean; cascade?: boolean; + /** The update wrote the booking's stay for the first time. */ + created?: boolean; stop?: boolean; seenActions?: string[]; unresolved?: string[]; @@ -54,7 +56,12 @@ function build( const reservations = { createWithCost: vi.fn(() => ({ reservation: { id: 40 }, accommodationCreated: !!opts.cascade, costEvents: [] })), getReservation: vi.fn((id: string) => (id === '5' ? { id: 5, title: 'Hotel', type: 'lodging' } : undefined)), - updateWithCost: vi.fn(() => ({ reservation: { id: 5 }, accommodationChanged: !!opts.cascade, costEvents: [] })), + updateWithCost: vi.fn(() => ({ + reservation: { id: 5 }, + accommodationChanged: !!opts.cascade || !!opts.created, + accommodationCreated: !!opts.created, + costEvents: [], + })), remove: vi.fn((id: string) => id === '5' ? { @@ -279,6 +286,17 @@ describe('ReservationsRpc', () => { expect(events(f.realtime)).toEqual(['reservation:deleted']); }); + it('BOOK-RPC-022 an update that writes the stay for the first time announces it as created, as REST does', async () => { + const f = build({ created: true }); + await f + .host() + .dispatch( + req('reservations.update', { tripId: 1, reservationId: 5, input: { title: 'x', type: 'hotel' } }), + 42, + ); + expect(events(f.realtime)).toEqual(['accommodation:created', 'reservation:updated']); + }); + it('BOOK-RPC-008e an update with a malformed endpoints array is BAD_PARAMS too', async () => { const f = build(); const res = (await f.host().dispatch(