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.