From 5cbd9e9d362abf6e89507799e2cdcf03c05dd856 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 15:31:18 +0200 Subject: [PATCH] fix(reservations): write a booking and its linked cost in one transaction A booking's create and update ran their own transaction, and the linked cost sync ran afterwards from the REST controller and the plugin RPC handler, so a failing cost write left the booking edited and its expense on the old category or price. MCP linked the cost in a second call of its own the same way. ReservationsService.createWithCost and updateWithCost now run the booking write and the cost sync in one transaction and hand the cost events back, so REST, MCP and the plugin RPC call the same method and broadcast only after the commit, in the order they did before. The cost sync keeps swallowing its own failures the way the REST route always has, so a cost that cannot be written still leaves the booking; MCP now behaves like REST there instead of failing the tool after the booking was already saved. The service sheds its inventory markers (RS1..RS54, plan history) to stay under its lint:size entry. --- .../reservations/reservations.controller.ts | 10 +- .../src/nest/reservations/reservations.mcp.ts | 34 ++--- .../src/nest/reservations/reservations.rpc.ts | 10 +- .../nest/reservations/reservations.service.ts | 122 +++++++++--------- .../unit/nest/reservations.controller.test.ts | 66 +++++----- .../unit/nest/reservations.service.test.ts | 57 ++++++++ .../tests/unit/plugins/rpc-fallbacks.test.ts | 4 +- .../unit/reservations/bookings.rpc.test.ts | 26 ++-- 8 files changed, 191 insertions(+), 138 deletions(-) diff --git a/server/src/nest/reservations/reservations.controller.ts b/server/src/nest/reservations/reservations.controller.ts index a55b0414b..bf4aefc82 100644 --- a/server/src/nest/reservations/reservations.controller.ts +++ b/server/src/nest/reservations/reservations.controller.ts @@ -72,11 +72,12 @@ export class ReservationsController { // Before the writes: the price keeps the currency it was quoted in, // at a rate frozen now (#2525). const budgetEntry = await this.reservations.withFrozenRate(tripId, body.create_budget_entry); - const { reservation, accommodationCreated } = await this.reservations.create(tripId, body as never); + // The booking and its linked cost are one write. + const { reservation, accommodationCreated, costEvents } = await this.reservations.createWithCost(tripId, body as never, budgetEntry); if (accommodationCreated) { this.reservations.broadcast(tripId, 'accommodation:created', {}, socketId); } - await this.reservations.syncBudgetOnCreate(tripId, reservation.id, body.title, body.type, budgetEntry, 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 }; @@ -112,12 +113,13 @@ export class ReservationsController { throw new HttpException({ error: 'Reservation not found' }, 404); } await this.rejectForeignReferences(tripId, body); - const { reservation, accommodationChanged } = await this.reservations.update(id, tripId, body as never, current as never); + // The booking and its linked cost are one write. + const { reservation, accommodationChanged, costEvents } = await this.reservations.updateWithCost(id, tripId, body as never, current, body.create_budget_entry); if (accommodationChanged) { this.reservations.broadcast(tripId, 'accommodation:updated', {}, socketId); } const cur = current as { title: string; type?: string }; - await this.reservations.syncBudgetOnUpdate(tripId, id, body.title ?? '', body.type, cur.title, cur.type, body.create_budget_entry, socketId); + 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 diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index 85d9f95a4..ed8968da6 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -103,6 +103,11 @@ function parseId(value: string | string[]): number | null { return Number.isInteger(n) && n > 0 ? n : null; } +/** The linked cost a tool's price becomes: the entry the booking form sends for one. */ +function linkedCost(price: number | null | undefined, category: string | undefined) { + return price != null && price > 0 ? { total_price: price, category } : undefined; +} + // --------------------------------------------------------------------------- // Multi-leg bookings (#1914) // @@ -384,25 +389,18 @@ export class ReservationsMcp { const metadata = price != null ? { price: String(price) } : undefined; - const { reservation, accommodationCreated } = await this.reservations.create(tripId, { + // 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), { 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', {}); } - - if (price != null && price > 0) { - const item = await this.budget.linkBudgetItemToReservation(tripId, reservation.id, { - name: title, - category: budget_category || type, - total_price: price, - }); - this.guards.safeBroadcast(tripId, 'budget:created', { item }); - } + for (const { event, payload } of costEvents) this.guards.safeBroadcast(tripId, event, payload); this.guards.safeBroadcast(tripId, 'reservation:created', { reservation }); return ok({ reservation }); @@ -741,7 +739,7 @@ export class ReservationsMcp { if (price != null) meta.price = String(price); - const { reservation } = await this.reservations.create(tripId, { + const { reservation, costEvents } = await this.reservations.createWithCost(String(tripId), { title, type, reservation_time: departureTime, @@ -756,16 +754,8 @@ export class ReservationsMcp { metadata: Object.keys(meta).length > 0 ? meta : undefined, endpoints: transportEndpoints, needs_review, - }); - - if (price != null && price > 0) { - const item = await this.budget.linkBudgetItemToReservation(tripId, reservation.id, { - name: title, - category: budget_category || type, - total_price: price, - }); - this.guards.safeBroadcast(tripId, 'budget:created', { item }); - } + }, linkedCost(price, budget_category || type)); + for (const { event, payload } of costEvents) this.guards.safeBroadcast(tripId, event, payload); this.guards.safeBroadcast(tripId, 'reservation:created', { reservation }); return ok({ reservation }); diff --git a/server/src/nest/reservations/reservations.rpc.ts b/server/src/nest/reservations/reservations.rpc.ts index 492dacd65..4e6029091 100644 --- a/server/src/nest/reservations/reservations.rpc.ts +++ b/server/src/nest/reservations/reservations.rpc.ts @@ -42,9 +42,9 @@ export class ReservationsRpc { const i = input as { title?: string; type?: string; create_budget_entry?: unknown }; // Same as the REST route: the price keeps its currency, at a rate frozen now (#2525). const budgetEntry = await this.reservations.withFrozenRate(tripId, i.create_budget_entry as never); - const { reservation, accommodationCreated } = await this.reservations.create(String(tripId), input as never); + const { reservation, accommodationCreated, costEvents } = await this.reservations.createWithCost(String(tripId), input as never, budgetEntry); if (accommodationCreated) this.realtime.broadcast(tripId, 'accommodation:created', {}, undefined); - await this.reservations.syncBudgetOnCreate(String(tripId), reservation.id, i.title ?? '', i.type, budgetEntry, undefined); + this.reservations.announceCost(tripId, costEvents, undefined); this.realtime.broadcast(tripId, 'reservation:created', { reservation }, undefined); await this.notifyBooking(actor, tripId, i.title ?? '', i.type ?? ''); return reservation; @@ -63,11 +63,11 @@ export class ReservationsRpc { const current = await this.reservations.getReservation(String(reservationId), String(tripId)); if (!current) throw new ForbiddenResource(`no reservation ${reservationId} on trip ${tripId}`); await this.requireOwnReferences(tripId, input); - const { reservation, accommodationChanged } = await this.reservations.update(String(reservationId), String(tripId), input as never, current as never); + 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 cur = current as { title: string; type?: string }; - const i = input as { title?: string; type?: string; create_budget_entry?: unknown }; - await this.reservations.syncBudgetOnUpdate(String(tripId), String(reservationId), i.title ?? '', i.type, cur.title, cur.type, i.create_budget_entry as never, undefined); + this.reservations.announceCost(tripId, costEvents, undefined); this.realtime.broadcast(tripId, 'reservation:updated', { reservation }, undefined); await this.notifyBooking(actor, tripId, i.title || cur.title, i.type || cur.type || ''); return reservation; diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 9fbd7cbd2..124722509 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -42,6 +42,12 @@ import { appClock } from '../common/timezoneService'; type Trip = TripAccess; type BudgetEntry = { total_price?: number; category?: string; currency?: string | null; exchange_rate?: number } | undefined; +/** What a booking write did to its linked costs, for the caller to send once the write has committed. */ +export type CostEvent = + | { event: 'budget:created'; payload: TrekWsPayload<'budget:created'> } + | { event: 'budget:updated'; payload: TrekWsPayload<'budget:updated'> } + | { event: 'budget:deleted'; payload: TrekWsPayload<'budget:deleted'> }; + export interface ReservationEndpoint { id?: number; reservation_id?: number; @@ -135,12 +141,6 @@ export interface UpdateReservationData { needs_review?: boolean; } -// The "does reservation_time actually carry a date, not just a bare HH:MM" -// question (#1934) that RS20's legacy `DATED` constant used to answer here -// now lives as `startsWithIsoDateKysely` inside -// `ReservationsRepository.listUpcomingForUser` (Plan 3d Task 4) — RS20 was -// this constant's only caller. - type AccommodationTimesMeta = { check_in_time?: string | null; check_in_end_time?: string | null; @@ -148,21 +148,10 @@ type AccommodationTimesMeta = { }; /** - * Reservations domain service — owns the reservation SQL (moved 1:1 from the - * legacy services/reservationService.ts: identical statements, the `||` - * falsy-coercion defaults, the COALESCE update semantics and the post-write - * re-selects; the multi-statement writes gained db.transaction() wrappers in - * the post-fold quirk-fix commit, and the accommodation metadata sync now - * keys off the resolved accommodation id so auto-created accommodations get - * their check-in/out times too). Trip access, - * the 'reservation_edit' permission and the WebSocket broadcast keep their - * legacy call paths. The legacy route's budget side effects (auto-create / - * update / delete a linked budget item) and the booking notification are - * encapsulated here so the controller stays thin — behaviour is 1:1. - * Every consumer injects this class now. reservations.bridge.ts existed for - * the legacy tripService, airtrail import/sync and the transit/transports MCP - * registrars, all of which have folded in; the plugin RPC host reaches it - * through ReservationsRpc. + * Reservations domain service: the reservation SQL with the legacy `||` + * defaults and COALESCE update semantics, every multi-statement write in one + * transaction, the linked-cost side effects and the booking notification, so + * REST, MCP and the plugin RPC (ReservationsRpc) stay thin. */ @Injectable() export class ReservationsService { @@ -185,14 +174,10 @@ export class ReservationsService { @InjectRepository(TripMembers) private readonly tripMembersRepo: TripMembersRepository, @InjectRepository(Users) private readonly usersRepo: UsersRepository, @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, - // Plan 3e Task 2 (budget) — additive, RS48/49/51-54 only. @InjectRepository(BudgetItems) private readonly budgetItemsRepo: BudgetItemsRepository, ) {} async verifyTripAccess(tripId: string | number, userId: number) { - // Plan 4 Task 2 — canAccessTrip's own DatabaseService delegation is - // gone: this reuses the TripsRepository already injected for other - // reads and calls findAccessible. return await this.tripsRepo.findAccessible(tripId, userId); } @@ -245,10 +230,8 @@ export class ReservationsService { // reaches nothing in this direction — and it hid the edge while handing the // send a second NotificationsService built outside the container. try { - // RS1 const actorEmail = await this.usersRepo.getEmail(actorId); if (!actorEmail) return; - // RS2 const tripTitle = await this.tripsRepo.getTitle(tripId); this.notifications.send({ event: 'booking_change', @@ -269,7 +252,6 @@ export class ReservationsService { } async loadEndpointsByTrip(tripId: string | number): Promise> { - // RS3 const rows: ReservationEndpointRow[] = await this.endpointsRepo.listForTrip(this.rowIdNum(tripId)); const map = new Map(); for (const r of rows) { @@ -295,7 +277,6 @@ export class ReservationsService { } async loadTravelersByTrip(tripId: string | number): Promise> { - // RS6 const rows = await this.travelersRepo.listForTrip(this.rowIdNum(tripId)); const map = new Map(); for (const row of rows) { @@ -321,9 +302,7 @@ export class ReservationsService { const ids = [...new Set(userIds)].filter(uid => allowed.has(uid)); const reservationIdNum = this.rowIdNum(reservationId); await this.uow.transactional(async () => { - // RS8 await this.travelersRepo.deleteForReservation(reservationIdNum); - // RS9 await this.travelersRepo.insertIgnore(reservationIdNum, ids); }); } @@ -348,7 +327,6 @@ export class ReservationsService { if (!time) return null; const datePart = time.slice(0, 10); if (!/^\d{4}-\d{2}-\d{2}$/.test(datePart)) return null; - // RS10 const exact = await this.daysRepo.findByTripAndDate(this.rowIdNum(tripId), datePart); if (exact) return exact.id; // Fallback: clamp to the nearest day in the trip so an imported booking whose @@ -356,7 +334,6 @@ export class ReservationsService { // Skipped by callers (e.g. resyncReservationDays) that must leave a booking whose // date now falls outside the range untouched instead of snapping it to an edge day. if (!clampToNearest) return null; - // RS11 const nearestId = await this.reservationsRepo.findNearestDayId(this.rowIdNum(tripId), datePart); return nearestId ?? null; } @@ -380,7 +357,6 @@ export class ReservationsService { ? ((await this.resolveDayIdFromTime(tripId, r.reservation_end_time, false)) ?? r.end_day_id) : r.end_day_id; if (newDayId !== r.day_id || newEndDayId !== r.end_day_id) { - // RS13 await this.reservationsRepo.setDays(r.id, newDayId, newEndDayId); } } @@ -396,7 +372,6 @@ export class ReservationsService { // bound its transaction lazily per call for the same reason ("The // database connection is not open"). await this.uow.transactional(async () => { - // RS16 await this.endpointsRepo.deleteForReservation(reservationId); // lat/lng are NOT NULL: an imported transport whose pick-up/return (or station/ // stop) couldn't be geocoded reaches here with null coords. Skip those rows rather @@ -404,7 +379,6 @@ export class ReservationsService { // on reservation_time/reservation_end_time, so the booking lands on its day either way. const filtered = endpoints.filter((e) => e.lat != null && e.lng != null); for (const [i, e] of filtered.entries()) { - // RS17 await this.endpointsRepo.insertEndpoint({ reservation_id: reservationId, role: e.role, @@ -434,7 +408,6 @@ export class ReservationsService { // hand-edited NULL) still reads back as `null` on the wire. const reservations: ReservationRow[] = (await this.reservationsRepo.listForTrip(tripId)).map((r) => ({ ...r })); - // RS19 const dayPositions = await this.dayPositionsRepo.listForTrip(this.rowIdNum(tripId)); const posMap = new Map>(); @@ -681,6 +654,45 @@ export class ReservationsService { return written; } + /** + * create and the linked cost the booking carries, as one write. The price's rate + * is frozen before (withFrozenRate, network I/O). REST, the plugin RPC and MCP + * all come through here and send the cost events after the commit. + */ + async createWithCost(tripId: string, data: CreateReservationData, entry: BudgetEntry) { + const costEvents: CostEvent[] = []; + const { stayMirror, ...written } = await this.uow.transactional(async () => { + const done = await this.createInTx(tripId, data); + await this.syncBudgetOnCreate(tripId, done.reservation.id, data.title, data.type, entry, undefined, costEvents); + return done; + }); + await this.announceStayMirror(tripId, stayMirror); + return { ...written, costEvents }; + } + + /** update and the linked cost it moves, as one write, the way createWithCost is. */ + async updateWithCost(id: string, tripId: string, data: UpdateReservationData, current: Reservation, entry: BudgetEntry) { + const costEvents: CostEvent[] = []; + const { stayMirror, ...written } = await this.uow.transactional(async () => { + const done = await this.updateInTx(id, tripId, data, current); + await this.syncBudgetOnUpdate(tripId, id, data.title ?? '', data.type, current.title, current.type ?? undefined, entry, undefined, costEvents); + return done; + }); + await this.announceStayMirror(tripId, stayMirror); + return { ...written, costEvents }; + } + + /** Sends a booking write's cost events to the trip. */ + announceCost(tripId: string | number, events: readonly CostEvent[], socketId: string | undefined): void { + for (const e of events) this.realtime.broadcast(tripId, e.event, e.payload, socketId); + } + + /** Broadcasts now, or collects for a caller whose transaction has not committed yet. */ + private sendCost(tripId: string, e: CostEvent, socketId: string | undefined, collect?: CostEvent[]): void { + if (collect) collect.push(e); + else this.announceCost(tripId, [e], socketId); + } + private async createInTx(tripId: string | number, data: CreateReservationData): Promise<{ reservation: ReservationRow; accommodationCreated: boolean; stayMirror: AccommodationMirror }> { const { title, reservation_time, reservation_end_time, location, @@ -703,7 +715,6 @@ export class ReservationsService { 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) { await this.requireResolvableStay(create_accommodation); - // RS27 resolvedAccommodationId = await this.dayAccommodationsRepo.insertBookingStay({ trip_id: tripId, place_id: accPlaceId || null, @@ -779,13 +790,11 @@ export class ReservationsService { if (resolvedAccommodationId && metadata) { const meta = (typeof metadata === 'string' ? JSON.parse(metadata) : metadata) as AccommodationTimesMeta; if (meta.check_in_time || meta.check_in_end_time || meta.check_out_time) { - // RS29 await this.dayAccommodationsRepo.patchTimes( resolvedAccommodationId, meta.check_in_time || null, meta.check_in_end_time || null, meta.check_out_time || null, ); } if (confirmation_number) { - // RS30 await this.dayAccommodationsRepo.patchConfirmation(resolvedAccommodationId, confirmation_number); } } @@ -907,7 +916,6 @@ export class ReservationsService { if (resolvedAccId) { // Scoped to the trip on purpose: an id belonging to someone else's trip // must read as absent here, not as an accommodation to write through to. - // RS37 const accExists = await this.dayAccommodationsRepo.existsInTrip(accIdForRead(resolvedAccId)!, this.rowIdNum(tripId)); if (!accExists) resolvedAccId = null; } @@ -917,9 +925,7 @@ export class ReservationsService { await this.requireResolvableStay(create_accommodation); if (resolvedAccId) { const resolvedAccIdNum = accIdForRead(resolvedAccId)!; - // RS38 const priorCheckIn = await this.dayAccommodationsRepo.getCheckIn(resolvedAccIdNum); - // RS39 await this.dayAccommodationsRepo.updateFromBooking(resolvedAccIdNum, { place_id: accPlaceId || null, start_day_id, @@ -935,7 +941,6 @@ export class ReservationsService { checkInChanged: (check_in || null) !== (priorCheckIn ?? null), }); } else if (accPlaceId) { - // RS40 resolvedAccId = await this.dayAccommodationsRepo.insertBookingStay({ trip_id: tripId, place_id: accPlaceId, @@ -1037,14 +1042,12 @@ export class ReservationsService { const meta = (typeof resolvedMeta === 'string' ? JSON.parse(resolvedMeta) : resolvedMeta) as AccommodationTimesMeta; const resolvedAccIdNum = accIdForRead(resolvedAccId)!; if (meta.check_in_time || meta.check_in_end_time || meta.check_out_time) { - // RS42 await this.dayAccommodationsRepo.patchTimes( resolvedAccIdNum, meta.check_in_time || null, meta.check_in_end_time || null, meta.check_out_time || null, ); } const resolvedConf = confirmation_number !== undefined ? confirmation_number : current.confirmation_number; if (resolvedConf) { - // RS43 await this.dayAccommodationsRepo.patchConfirmation(resolvedAccIdNum, resolvedConf); } } @@ -1069,7 +1072,6 @@ export class ReservationsService { return { deleted: undefined, accommodationDeleted: false, deletedBudgetItemId: null, deletedBudgetItemIds: [] }; } const removed = await this.uow.transactional(async () => { - // RS45 const reservation = await this.reservationsRepo.findHeaderInTrip(idNum, tripIdNum); if (!reservation) return { deleted: undefined, accommodationDeleted: false, deletedBudgetItemId: null, deletedBudgetItemIds: [], stayMirror: noStayMirror() }; @@ -1086,7 +1088,6 @@ export class ReservationsService { // WRITE-side raw string (`reservation.accommodation_id`) still ships // unconverted in the `accommodation:deleted` broadcast payload below. const accIdNum = Number(reservation.accommodation_id); - // RS46 const ownStay = await this.dayAccommodationsRepo.existsInTrip(accIdNum, tripIdNum); if (ownStay) { // Released before the row goes, not after: the release looks the stops up @@ -1094,7 +1095,6 @@ export class ReservationsService { // deleted. Reversed, the stop stands with nothing left to remove it, and // the day list hides it for carrying a booking id. stayMirror = await this.accommodations.dropStayStops(accIdNum); - // RS47 await this.dayAccommodationsRepo.deleteInTrip(accIdNum, tripIdNum); accommodationDeleted = true; } @@ -1105,7 +1105,6 @@ export class ReservationsService { const deletedBudgetItemIds = (await this.budgetItemsRepo.listIdAndCategoryByReservation(tripIdNum, idNum)).map(item => item.id); await this.budgetItemsRepo.deleteByIds(deletedBudgetItemIds); - // RS50 await this.reservationsRepo.deleteById(idNum); return { deleted: reservation, accommodationDeleted, deletedBudgetItemId: deletedBudgetItemIds[0] ?? null, deletedBudgetItemIds, stayMirror }; }); @@ -1134,8 +1133,8 @@ export class ReservationsService { return { ...rest, currency, ...(priced.exchange_rate != null ? { exchange_rate: priced.exchange_rate } : {}) }; } - /** POST side effect: auto-create a linked budget item when a price is provided. */ - async syncBudgetOnCreate(tripId: string, reservationId: number, title: string, type: string | undefined, entry: BudgetEntry, socketId: string | undefined): Promise { + /** POST side effect: auto-create a linked budget item when a price is provided. `collect` holds the broadcast for a caller inside its transaction. */ + async syncBudgetOnCreate(tripId: string, reservationId: number, title: string, type: string | undefined, entry: BudgetEntry, socketId: string | undefined, collect?: CostEvent[]): Promise { if (!entry || !(Number(entry.total_price) > 0)) return; try { const item = await this.budget.linkBudgetItemToReservation(tripId, reservationId, { @@ -1145,14 +1144,14 @@ export class ReservationsService { ...(entry.currency ? { currency: entry.currency } : {}), ...(entry.exchange_rate != null ? { exchange_rate: entry.exchange_rate } : {}), }); - this.realtime.broadcast(tripId, 'budget:created', { item }, socketId); + this.sendCost(tripId, { event: 'budget:created', payload: { item } }, socketId, collect); } catch (err) { console.error('[reservations] Failed to create budget entry:', err); } } - /** PUT side effect: drop the linked budget item when the price is cleared, else create/update it. */ - async syncBudgetOnUpdate(tripId: string, id: string, title: string, type: string | undefined, currentTitle: string, currentType: string | undefined, entry: BudgetEntry, socketId: string | undefined): Promise { + /** PUT side effect: drop the linked budget item when the price is cleared, else create/update it. `collect` as on create. */ + async syncBudgetOnUpdate(tripId: string, id: string, title: string, type: string | undefined, currentTitle: string, currentType: string | undefined, entry: BudgetEntry, socketId: string | undefined, collect?: CostEvent[]): Promise { // When the booking type changes, keep a linked expense's category in sync — // but only if it still carries the auto-derived category (so a manual pick in // the Costs editor is preserved). Runs regardless of create_budget_entry. @@ -1163,7 +1162,7 @@ export class ReservationsService { const linked = oldCat === newCat ? [] : await this.budgetItemsRepo.listIdAndCategoryByReservation(tripId, id); for (const item of linked.filter(i => i.category === oldCat)) { const updated = await this.budget.updateBudgetItem(item.id, tripId, { category: newCat }); - this.realtime.broadcast(tripId, 'budget:updated', { item: updated }, socketId); + this.sendCost(tripId, { event: 'budget:updated', payload: { item: updated } }, socketId, collect); } } @@ -1173,15 +1172,14 @@ export class ReservationsService { if (!entry) return; // The price field speaks for a single expense. With several linked (#2084) // it has none to mean, so they are managed from the Costs block only. - // RS51 if ((await this.budgetItemsRepo.listIdAndCategoryByReservation(tripId, id)).length > 1) return; if (!(Number(entry.total_price) > 0)) { - // Explicit clear (total_price 0/empty) — drop the linked item. RS52 — Plan 3e Task 2, converted. + // Explicit clear (total_price 0/empty) — drop the linked item. const linked = await this.budgetItemsRepo.findIdByReservationInTrip(tripId, id); if (linked) { await this.budget.deleteBudgetItem(linked.id, tripId); - this.realtime.broadcast(tripId, 'budget:deleted', { itemId: linked.id }, socketId); + this.sendCost(tripId, { event: 'budget:deleted', payload: { itemId: linked.id } }, socketId, collect); } return; } @@ -1189,17 +1187,15 @@ export class ReservationsService { try { const itemName = title || currentTitle; const category = entry.category || type || currentType || 'Other'; - // RS53 — Plan 3e Task 2, converted. const existing = await this.budgetItemsRepo.findIdByReservationInTrip(tripId, id); if (existing) { const updated = await this.budget.updateBudgetItem(existing.id, tripId, { name: itemName, category, total_price: entry.total_price }); - this.realtime.broadcast(tripId, 'budget:updated', { item: updated }, socketId); + this.sendCost(tripId, { event: 'budget:updated', payload: { item: updated } }, socketId, collect); } else { const item = await this.budget.createBudgetItem(tripId, { name: itemName, category, total_price: entry.total_price }); - // RS54 — Plan 3e Task 2, converted. await this.budgetItemsRepo.setReservationId(item.id, id); item.reservation_id = Number(id); - this.realtime.broadcast(tripId, 'budget:created', { item }, socketId); + this.sendCost(tripId, { event: 'budget:created', payload: { item } }, socketId, collect); } } catch (err) { console.error('[reservations] Failed to create/update budget entry:', err); diff --git a/server/tests/unit/nest/reservations.controller.test.ts b/server/tests/unit/nest/reservations.controller.test.ts index e0f0b0a5c..6d4bbbabf 100644 --- a/server/tests/unit/nest/reservations.controller.test.ts +++ b/server/tests/unit/nest/reservations.controller.test.ts @@ -18,8 +18,7 @@ function makeService(overrides: Partial = {}): Reservations referencesOutsideTrip: vi.fn().mockReturnValue([]), unresolvedReferences: vi.fn().mockReturnValue([]), broadcast: vi.fn(), - syncBudgetOnCreate: vi.fn(), - syncBudgetOnUpdate: vi.fn(), + announceCost: vi.fn(), notifyBookingChange: vi.fn(), // Hands the entry back as it came: what freezing does to it is the service's test. withFrozenRate: vi.fn(async (_tripId: string, entry: unknown) => entry), @@ -48,34 +47,40 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese // The bespoke 'Title is required' 400 moved to the global ZodValidationPipe // (ReservationCreateDto) — covered by the e2e suite. - it('creates, runs budget sync, broadcasts accommodation + reservation, notifies', async () => { - const create = vi.fn().mockReturnValue({ reservation: { id: 9 }, accommodationCreated: true }); - const broadcast = vi.fn(); const syncBudgetOnCreate = vi.fn(); const notifyBookingChange = vi.fn(); - const svc = makeService({ create, broadcast, syncBudgetOnCreate, notifyBookingChange } as Partial); + it('creates the booking and its cost in one write, broadcasts accommodation, cost and reservation in order, notifies', async () => { + const costEvents = [{ event: 'budget:created', payload: { item: { id: 3 } } }]; + 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 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({ 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(syncBudgetOnCreate).toHaveBeenCalledWith('5', 9, 'Hotel', 'lodging', { total_price: 200 }, 'sock'); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:created', { reservation: { id: 9 } }, 'sock'); + expect(order).toEqual(['accommodation:created', 'cost', 'reservation:created']); expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Hotel', 'lodging'); }); it('400s on a body id belonging to another trip, without writing', async () => { - const create = vi.fn(); + const createWithCost = vi.fn(); const svc = makeService({ - create, + createWithCost, 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({ status: 400, body: { error: 'Not part of this trip: accommodation_id' } }); - expect(create).not.toHaveBeenCalled(); + expect(createWithCost).not.toHaveBeenCalled(); }); it('400s on a body id that exists nowhere, in its own words, without writing', async () => { - const create = vi.fn(); + const createWithCost = vi.fn(); const svc = makeService({ - create, + createWithCost, unresolvedReferences: vi.fn().mockReturnValue(['place_id']), } as Partial); const body = { title: 'Hotel', place_id: 4711 }; @@ -83,12 +88,12 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese // the caller looking for it on another trip. expect(await thrown(() => new ReservationsController(svc, airtrailLink).create(user, '5', body))) .toEqual({ status: 400, body: { error: 'Unknown reference: place_id' } }); - expect(create).not.toHaveBeenCalled(); + expect(createWithCost).not.toHaveBeenCalled(); }); it('answers a foreign id with the older message when it is both', async () => { const svc = makeService({ - create: vi.fn(), + createWithCost: vi.fn(), referencesOutsideTrip: vi.fn().mockReturnValue(['place_id']), unresolvedReferences: vi.fn().mockReturnValue(['place_id']), } as Partial); @@ -98,18 +103,17 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese // #2525: an imported booking quoted in dollars. The price has to reach the linked // cost in dollars, at a rate frozen before anything is written. - it('hands the budget sync the entry with its currency and frozen rate', async () => { - const create = vi.fn().mockReturnValue({ reservation: { id: 9 }, accommodationCreated: false }); - const syncBudgetOnCreate = vi.fn(); + it('hands the write the entry with its currency and frozen rate', async () => { + const createWithCost = vi.fn().mockReturnValue({ reservation: { id: 9 }, accommodationCreated: false, costEvents: [] }); const withFrozenRate = vi.fn(async () => { - expect(create).not.toHaveBeenCalled(); + expect(createWithCost).not.toHaveBeenCalled(); return { total_price: 801.76, currency: 'USD', exchange_rate: 1.17 }; }); - const svc = makeService({ create, syncBudgetOnCreate, withFrozenRate } as Partial); + const svc = makeService({ createWithCost, withFrozenRate } as Partial); const body = { title: 'Aparthotel Silver', type: 'hotel', create_budget_entry: { total_price: 801.76, currency: 'usd' } }; await new ReservationsController(svc, airtrailLink).create(user, '5', body, 'sock'); expect(withFrozenRate).toHaveBeenCalledWith('5', { total_price: 801.76, currency: 'usd' }); - expect(syncBudgetOnCreate).toHaveBeenCalledWith('5', 9, 'Aparthotel Silver', 'hotel', { total_price: 801.76, currency: 'USD', exchange_rate: 1.17 }, 'sock'); + expect(createWithCost).toHaveBeenCalledWith('5', body, { total_price: 801.76, currency: 'USD', exchange_rate: 1.17 }); }); }); @@ -133,14 +137,18 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese expect(await thrown(() => new ReservationsController(svc, airtrailLink).update(user, '5', '9', { title: 'X' }))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); }); - it('updates, syncs budget with current fallbacks, broadcasts + notifies', async () => { - const getReservation = vi.fn().mockReturnValue({ title: 'Old', type: 'lodging' }); - const update = vi.fn().mockReturnValue({ reservation: { id: 9 }, accommodationChanged: true }); - const broadcast = vi.fn(); const syncBudgetOnUpdate = vi.fn(); const notifyBookingChange = vi.fn(); - const svc = makeService({ getReservation, update, broadcast, syncBudgetOnUpdate, notifyBookingChange } as Partial); - await new ReservationsController(svc, airtrailLink).update(user, '5', '9', { create_budget_entry: { total_price: 50 } }, 'sock'); + 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 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(updateWithCost).toHaveBeenCalledWith('9', '5', body, current, { total_price: 50 }); expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:updated', {}, 'sock'); - expect(syncBudgetOnUpdate).toHaveBeenCalledWith('5', '9', '', undefined, 'Old', 'lodging', { total_price: 50 }, 'sock'); + expect(announceCost).toHaveBeenCalledWith('5', costEvents, 'sock'); expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Old', 'lodging'); }); @@ -148,7 +156,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const update = vi.fn(); const svc = makeService({ getReservation: vi.fn().mockReturnValue({ title: 'Old', type: 'lodging' }), - update, + 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 }))) @@ -160,7 +168,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const update = vi.fn(); const svc = makeService({ getReservation: vi.fn().mockReturnValue({ title: 'Old', type: 'lodging' }), - update, + updateWithCost: update, unresolvedReferences: vi.fn().mockReturnValue(['day_id', 'place_id']), } as Partial); // The reporter's request: a foreign-key error used to reach the caller as diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index bf3b7a727..621f67e76 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -706,6 +706,63 @@ describe('ReservationsService (DI-native, real SQL)', () => { expect(testDb.prepare('SELECT reservation_id FROM budget_items WHERE id = ?').get(created.id)).toEqual({ reservation_id: res.id }); expect(broadcast).toHaveBeenCalledWith(String(trip.id), 'budget:created', { item: { id: created.id, reservation_id: res.id } }, 'sock'); }); + + it('RESV-TX-001: updateWithCost rolls the booking edit back when its cost write fails', async () => { + const { trip, res } = linkedItem({ category: 'other' }); + testDb.prepare("UPDATE reservations SET title = 'Old', type = 'other' WHERE id = ?").run(res.id); + const current = (await svc.getReservation(String(res.id), String(trip.id)))!; + budget.updateBudgetItem.mockRejectedValueOnce(new Error('boom')); + + // other -> flight re-files the linked expense, and that write throws. + await expect(svc.updateWithCost(String(res.id), String(trip.id), { title: 'New', type: 'flight' } as never, current, undefined)) + .rejects.toThrow('boom'); + + expect(testDb.prepare('SELECT title, type FROM reservations WHERE id = ?').get(res.id)).toEqual({ title: 'Old', type: 'other' }); + expect(broadcast).not.toHaveBeenCalled(); + }); + + it('RESV-TX-002: updateWithCost hands the cost events back instead of sending them inside the write', async () => { + const { trip, res, item } = linkedItem(); + const current = (await svc.getReservation(String(res.id), String(trip.id)))!; + budget.updateBudgetItem.mockReturnValue({ id: item.id }); + + const out = await svc.updateWithCost(String(res.id), String(trip.id), { title: 'New' } as never, current, { total_price: 80 }); + + expect(out.costEvents).toEqual([{ event: 'budget:updated', payload: { item: { id: item.id } } }]); + expect(broadcast).not.toHaveBeenCalledWith(String(trip.id), 'budget:updated', expect.anything(), expect.anything()); + svc.announceCost(String(trip.id), out.costEvents, 'sock'); + expect(broadcast).toHaveBeenCalledWith(String(trip.id), 'budget:updated', { item: { id: item.id } }, 'sock'); + }); + }); + + describe('createWithCost', () => { + it('RESV-TX-003: links the cost inside the booking write and hands its event back', async () => { + const { trip } = ownerTrip(); + budget.linkBudgetItemToReservation.mockReturnValue({ id: 7 }); + + const out = await svc.createWithCost(String(trip.id), { title: 'Hotel', type: 'other' } as never, { total_price: 200 }); + + expect(budget.linkBudgetItemToReservation).toHaveBeenCalledWith(String(trip.id), out.reservation.id, { name: 'Hotel', category: 'other', total_price: 200 }); + expect(out.costEvents).toEqual([{ event: 'budget:created', payload: { item: { id: 7 } } }]); + expect(broadcast).not.toHaveBeenCalledWith(String(trip.id), 'budget:created', expect.anything(), expect.anything()); + }); + + it('RESV-TX-004: a failing cost keeps the booking, as the REST route always has', async () => { + const { trip } = ownerTrip(); + budget.linkBudgetItemToReservation.mockRejectedValueOnce(new Error('boom')); + + const out = await svc.createWithCost(String(trip.id), { title: 'Hotel', type: 'other' } as never, { total_price: 200 }); + + expect(out.costEvents).toEqual([]); + expect(testDb.prepare('SELECT title FROM reservations WHERE id = ?').get(out.reservation.id)).toEqual({ title: 'Hotel' }); + }); + + it('RESV-TX-005: without a price there is no cost and no event', async () => { + const { trip } = ownerTrip(); + const out = await svc.createWithCost(String(trip.id), { title: 'Dinner' } as never, undefined); + expect(out.costEvents).toEqual([]); + expect(budget.linkBudgetItemToReservation).not.toHaveBeenCalled(); + }); }); describe('notifyBookingChange', () => { diff --git a/server/tests/unit/plugins/rpc-fallbacks.test.ts b/server/tests/unit/plugins/rpc-fallbacks.test.ts index 1655ded1f..2da99a27b 100644 --- a/server/tests/unit/plugins/rpc-fallbacks.test.ts +++ b/server/tests/unit/plugins/rpc-fallbacks.test.ts @@ -211,8 +211,8 @@ describe('service errors are translated into the RPC taxonomy', () => { const notifyBookingChange = vi.fn(); const reservations = { getReservation: vi.fn(() => ({ id: 5, title: 'Hotel', type: 'lodging' })), - update: vi.fn(() => ({ reservation: { id: 5 }, accommodationChanged: false })), - syncBudgetOnUpdate: vi.fn(), + updateWithCost: vi.fn(() => ({ reservation: { id: 5 }, accommodationChanged: false, costEvents: [] })), + announceCost: vi.fn(), referencesOutsideTrip: vi.fn(() => []), unresolvedReferences: vi.fn(() => []), notifyBookingChange, diff --git a/server/tests/unit/reservations/bookings.rpc.test.ts b/server/tests/unit/reservations/bookings.rpc.test.ts index e1c8d8620..839656f2e 100644 --- a/server/tests/unit/reservations/bookings.rpc.test.ts +++ b/server/tests/unit/reservations/bookings.rpc.test.ts @@ -36,16 +36,15 @@ function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; see // The expenses a delete takes with it: one with `cascade`, any number with `expenses` (#2084). const expenses = opts.expenses ?? (opts.cascade ? [7] : []); const reservations = { - create: vi.fn(() => ({ reservation: { id: 40 }, accommodationCreated: !!opts.cascade })), + createWithCost: vi.fn(() => ({ reservation: { id: 40 }, accommodationCreated: !!opts.cascade, costEvents: [] })), getReservation: vi.fn((id: string) => (id === '5' ? { id: 5, title: 'Hotel', type: 'lodging' } : undefined)), - update: vi.fn(() => ({ reservation: { id: 5 }, accommodationChanged: !!opts.cascade })), + updateWithCost: vi.fn(() => ({ reservation: { id: 5 }, accommodationChanged: !!opts.cascade, costEvents: [] })), remove: vi.fn((id: string) => id === '5' ? { deleted: { title: 'Hotel', type: 'lodging', accommodation_id: opts.cascade ? 11 : null }, accommodationDeleted: !!opts.cascade, deletedBudgetItemId: expenses[0] ?? null, deletedBudgetItemIds: expenses } : { deleted: null, accommodationDeleted: false, deletedBudgetItemId: null, deletedBudgetItemIds: [] }, ), - syncBudgetOnCreate: vi.fn(), - syncBudgetOnUpdate: vi.fn(), + announceCost: vi.fn(), // The price's currency and rate are the service's to resolve; here it passes through. withFrozenRate: vi.fn(async (_tripId: number, entry: unknown) => entry), notifyBookingChange: vi.fn(), @@ -122,7 +121,7 @@ describe('ReservationsRpc', () => { )) as RpcError; expect(res.error.code).toBe('BAD_PARAMS'); // Otherwise it fails deep in the service mid-transaction, or is silently dropped. - expect(f.reservations.create).not.toHaveBeenCalled(); + expect(f.reservations.createWithCost).not.toHaveBeenCalled(); }); it('BOOK-RPC-003 an absent endpoints field stays absent, which means "keep"', async () => { @@ -160,10 +159,11 @@ describe('ReservationsRpc', () => { ]); }); - it('BOOK-RPC-007 the budget sync and the booking notification both run', async () => { + it('BOOK-RPC-007 the booking and its cost are one service write, and the booking notification runs', async () => { const f = build(); await f.host().dispatch(req('reservations.create', { tripId: 1, input: { title: 'Hotel', type: 'lodging' } }), 42); - expect(f.reservations.syncBudgetOnCreate).toHaveBeenCalled(); + expect(f.reservations.createWithCost).toHaveBeenCalledWith('1', expect.objectContaining({ title: 'Hotel' }), undefined); + expect(f.reservations.announceCost).toHaveBeenCalledWith(1, [], undefined); expect(f.reservations.notifyBookingChange).toHaveBeenCalledWith(1, 42, 'Hotel', 'lodging'); }); @@ -180,7 +180,7 @@ describe('ReservationsRpc', () => { // defence in depth, but the schema is what a plugin actually hits first. const res = (await f.host().dispatch(req('reservations.create', { tripId: 1, input: {} }), 42)) as RpcError; expect(res.error.code).toBe('BAD_PARAMS'); - expect(f.reservations.create).not.toHaveBeenCalled(); + expect(f.reservations.createWithCost).not.toHaveBeenCalled(); }); it('BOOK-RPC-008c an update without a title keeps the current one', async () => { @@ -208,7 +208,7 @@ describe('ReservationsRpc', () => { req('reservations.update', { tripId: 1, reservationId: 5, input: { title: 'x', type: 'lodging', endpoints: [{ nonsense: true }] } }), 42, )) as RpcError; expect(res.error.code).toBe('BAD_PARAMS'); - expect(f.reservations.update).not.toHaveBeenCalled(); + expect(f.reservations.updateWithCost).not.toHaveBeenCalled(); }); it('BOOK-RPC-018 an id that resolves to nothing is BAD_PARAMS, not a constraint failure', async () => { @@ -218,7 +218,7 @@ describe('ReservationsRpc', () => { )) as RpcError; expect(res.error.code).toBe('BAD_PARAMS'); expect(res.error.message).toBe('unknown reference: place_id'); - expect(f.reservations.create).not.toHaveBeenCalled(); + expect(f.reservations.createWithCost).not.toHaveBeenCalled(); }); it('BOOK-RPC-019 the same on update', async () => { @@ -227,7 +227,7 @@ describe('ReservationsRpc', () => { req('reservations.update', { tripId: 1, reservationId: 5, input: { title: 'x', type: 'lodging', day_id: 999999 } }), 42, )) as RpcError; expect(res.error.code).toBe('BAD_PARAMS'); - expect(f.reservations.update).not.toHaveBeenCalled(); + expect(f.reservations.updateWithCost).not.toHaveBeenCalled(); }); it('BOOK-RPC-020 an id from another trip is RESOURCE_FORBIDDEN before the write, on create and update', async () => { @@ -240,13 +240,13 @@ describe('ReservationsRpc', () => { const created = (await f.host().dispatch(req('reservations.create', { tripId: 1, input: stay }), 42)) as RpcError; expect(created.error.code).toBe('RESOURCE_FORBIDDEN'); expect(created.error.message).toBe('not part of trip 1: create_accommodation.start_day_id'); - expect(f.reservations.create).not.toHaveBeenCalled(); + expect(f.reservations.createWithCost).not.toHaveBeenCalled(); const g = build({ foreign: ['day_id'] }); const updated = (await g.host().dispatch(req('reservations.update', { tripId: 1, reservationId: 5, input: { title: 'x', type: 'lodging', day_id: 4711 } }), 42)) as RpcError; expect(updated.error.code).toBe('RESOURCE_FORBIDDEN'); expect(updated.error.message).toBe('not part of trip 1: day_id'); - expect(g.reservations.update).not.toHaveBeenCalled(); + expect(g.reservations.updateWithCost).not.toHaveBeenCalled(); expect(g.realtime.broadcast).not.toHaveBeenCalled(); // The same body with this trip's ids goes through.