diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index d76da7c02..409d74708 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -23,7 +23,10 @@ export interface AccommodationMirror { stamped: PlaceWithTags | null; } -const noMirror = (): AccommodationMirror => ({ created: null, removed: [], stamped: null }); +/** A write that left the day plan alone. Exported for the surfaces that write a + * stay row themselves and have to answer with a mirror either way. */ +export const noStayMirror = (): AccommodationMirror => ({ created: null, removed: [], stamped: null }); +const noMirror = noStayMirror; export interface DayAccommodation { id: number; @@ -127,6 +130,37 @@ export class AccommodationsService { return this.deleteAccommodation(id, opts); } + // ------------------------------------------------------------------------- + // For a surface that writes the stay row itself + // + // The booking form does: it fills day_accommodations straight from a hotel + // reservation, with its own COALESCE semantics and its own field set, and + // folding that into createAccommodation would mean bending one of the two out + // of shape. What must not be duplicated is the stop, so these three open the + // mirror to it and keep the SQL here. + // + // A night entered on the booking form is a night entered in Days: it shows in + // the day header exactly like one added there. Road trip mode draws the same + // booking as a service stop instead, so the stop is not a second edit to the + // day plan, it is the same one in the other view. That is why writing it does + // not ask for more than the booking already did. + // ------------------------------------------------------------------------- + + /** Put a freshly written stay on the map. */ + attachStayStop(accommodationId: number, placeId: number | null, dayId: number): AccommodationMirror { + return this.mirrorStay(accommodationId, placeId, dayId); + } + + /** Carry a stay's own stop over to where the stay now is. */ + moveStayStop(accommodationId: number, placeId: number | null, dayId: number): AccommodationMirror { + return this.remirrorStay(accommodationId, placeId, dayId); + } + + /** Take back the stops of a stay that is being deleted elsewhere. */ + dropStayStops(accommodationId: number): AccommodationMirror { + return this.releaseStops(accommodationId, {}); + } + /** * Send what a stay write did to the day plan, and let the journey skeletons * catch up the way an assignment route does. @@ -236,6 +270,33 @@ export class AccommodationsService { ); } + /** + * Let go of the stops a booking owns, because the booking is going away. + * + * Only the ones it put there itself. A stop the traveller placed and then + * booked a night at keeps standing, which is how cancelling a night in the road + * trip has always behaved. + * + * keepStop hands it to the traveller instead of taking it away. That is the road + * trip popup turning a night back into a pause: they asked to drop the booking, + * not the place, and the stop is mid-drive where re-adding it would land it at + * the end of the day. + * + * Runs inside the caller's transaction. + */ + private releaseStops(accommodationId: number, opts: { keepStop?: boolean }): AccommodationMirror { + const mirror = noMirror(); + for (const stop of this.ownStops(accommodationId)) { + if (opts.keepStop) { + this.db.run('UPDATE day_assignments SET accommodation_id = NULL WHERE id = ?', stop.id); + continue; + } + this.db.run('DELETE FROM day_assignments WHERE id = ?', stop.id); + mirror.removed.push({ id: stop.id, dayId: stop.day_id }); + } + return mirror; + } + /** * Carry the mirrored stop over to wherever the booking now is. * @@ -372,23 +433,7 @@ export class AccommodationsService { this.db.run('DELETE FROM reservations WHERE id = ?', res.id); } - // Only the stops this booking put there itself. A stop the traveller placed - // and then booked a night at keeps standing, which is how cancelling a night - // in the road trip has always behaved. - // - // keepStop hands it to the traveller instead of taking it away. That is the - // road trip popup turning a night back into a pause: they asked to drop the - // booking, not the place, and the stop is mid-drive where re-adding it would - // land it at the end of the day. - const mirror = noMirror(); - for (const stop of this.ownStops(Number(id))) { - if (opts.keepStop) { - this.db.run('UPDATE day_assignments SET accommodation_id = NULL WHERE id = ?', stop.id); - continue; - } - this.db.run('DELETE FROM day_assignments WHERE id = ?', stop.id); - mirror.removed.push({ id: stop.id, dayId: stop.day_id }); - } + const mirror = this.releaseStops(Number(id), opts); this.db.run('DELETE FROM day_accommodations WHERE id = ?', id); const linkedReservationIds = linkedRes.map(r => r.id); diff --git a/server/src/nest/reservations/reservations.module.ts b/server/src/nest/reservations/reservations.module.ts index 3aa7d3df3..c4aa1dd3f 100644 --- a/server/src/nest/reservations/reservations.module.ts +++ b/server/src/nest/reservations/reservations.module.ts @@ -13,6 +13,7 @@ import { ReservationsMcp } from './reservations.mcp'; import { UpcomingReservationsController } from './upcoming-reservations.controller'; import { AuthModule } from '../auth/auth.module'; import { AssignmentsModule } from '../assignments/assignments.module'; +import { AccommodationsModule } from '../accommodations/accommodations.module'; import { NotificationsModule } from '../notifications/notifications.module'; import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; @@ -25,7 +26,10 @@ import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; @Module({ // DaysModule: ReservationsMcp injects DaysService for its nine getDay calls. // BudgetModule: ReservationsService + ReservationsMcp inject BudgetService (budget-sync seam). - imports: [McpSharedModule, NotificationsModule, DaysModule, AssignmentsModule, PermissionsModule, BudgetModule, AuthModule, RealtimeModule, PluginGuardsModule, ReservationsReadModule, AirtrailCoreModule], + // AccommodationsModule: a hotel booking writes its own day_accommodations row, and + // that stay owes the day plan the same stop one entered under Days does. No edge + // back — accommodations reaches neither days nor reservations (ACC-002). + imports: [McpSharedModule, NotificationsModule, DaysModule, AssignmentsModule, AccommodationsModule, PermissionsModule, BudgetModule, AuthModule, RealtimeModule, PluginGuardsModule, ReservationsReadModule, AirtrailCoreModule], controllers: [ReservationsController, UpcomingReservationsController], providers: [ReservationsService, ReservationsMcp, ReservationsRpc], // For in-container consumers (ReservationsRpc, TripsService, BookingImportService). diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 787681fe9..0c371e685 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -9,6 +9,7 @@ import type { Reservation, User } from '../../types'; import { BudgetService } from '../budget/budget.service'; import { typeToCostCategory } from '@trek/shared'; import { NotificationsService } from '../notifications/notifications.service'; +import { AccommodationsService, noStayMirror, type AccommodationMirror } from '../accommodations/accommodations.service'; type Trip = TripAccess; type BudgetEntry = { total_price?: number; category?: string } | undefined; @@ -149,6 +150,7 @@ export class ReservationsService { private readonly realtime: RealtimeService, private readonly notifications: NotificationsService, private readonly reads: ReservationsReadRepository, + private readonly accommodations: AccommodationsService, ) {} verifyTripAccess(tripId: string | number, userId: number) { @@ -163,6 +165,23 @@ export class ReservationsService { this.realtime.broadcast(tripId, event, payload, socketId); } + /** + * Announce the day stop a hotel booking wrote, the way the accommodations + * routes announce theirs. + * + * Here rather than at each surface: eleven call sites reach create/update/ + * remove, all but the hotel ones carry an empty mirror, and the fan-out is the + * one part that must not exist in eleven copies. Outside the transaction, so a + * write that rolls back announces nothing. + * + * No socket id to skip: the stop is news to the sender too. Every other + * booking event on this surface is echo-suppressed because the client already + * drew what it sent, and it never sent this. + */ + private announceStayMirror(tripId: string | number, mirror: AccommodationMirror): void { + this.accommodations.announceMirror(tripId, mirror, (event, payload) => this.realtime.broadcast(tripId, event, payload)); + } + /** Fire-and-forget booking-change notification, mirroring the legacy dynamic import. */ notifyBookingChange(tripId: string | number, actorId: number, booking: string, type: string): void { // Injected, not a lazy import of the old notifications bridge. The laziness bought @@ -549,10 +568,12 @@ export class ReservationsService { /** The accommodation insert, the reservation insert, the endpoint save and * the metadata sync are one logical write — all-or-nothing. */ create(tripId: string | number, data: CreateReservationData): { reservation: ReservationRow; accommodationCreated: boolean } { - return this.db.transaction(() => this.createInTx(tripId, data)); + const { stayMirror, ...written } = this.db.transaction(() => this.createInTx(tripId, data)); + this.announceStayMirror(tripId, stayMirror); + return written; } - private createInTx(tripId: string | number, data: CreateReservationData): { reservation: ReservationRow; accommodationCreated: boolean } { + private createInTx(tripId: string | number, data: CreateReservationData): { reservation: ReservationRow; accommodationCreated: boolean; stayMirror: AccommodationMirror } { const { title, reservation_time, reservation_end_time, location, confirmation_number, notes, url, day_id, end_day_id, place_id, assignment_id, @@ -561,15 +582,14 @@ export class ReservationsService { } = data; let accommodationCreated = false; + let stayMirror = noStayMirror(); // Auto-create accommodation for hotel reservations. // - // Deliberately NOT through AccommodationsService, which would also put the - // place on its check-in day: this surface is gated on 'reservation_edit' and - // that stop is 'day_edit' work, and the two are configured separately. A - // booking clerk must not rewrite the day plan as a side effect. A hotel - // booked here therefore still reaches the route once somebody opens the stay - // and saves it. + // The stay row is written here rather than through createAccommodation: this + // surface has its own field set and its own COALESCE semantics, and folding + // the two together would bend one of them out of shape. The day stop is the + // part that must not exist twice, so it comes from AccommodationsService. let resolvedAccommodationId: number | null = accommodation_id || null; if (type === 'hotel' && !resolvedAccommodationId && create_accommodation) { const { place_id: accPlaceId, start_day_id, end_day_id, check_in, check_out, confirmation: accConf } = create_accommodation; @@ -580,6 +600,11 @@ export class ReservationsService { ); resolvedAccommodationId = Number(accResult.lastInsertRowid); accommodationCreated = true; + // Same night, same day header, so the same stop the road trip draws for a + // night entered under Days. Without it the hotel booked on this form is the + // one place the drive does not know about, which is the duplicate entry this + // whole change exists to remove. + stayMirror = this.accommodations.attachStayStop(resolvedAccommodationId, accPlaceId || null, start_day_id); } } @@ -645,7 +670,7 @@ export class ReservationsService { // The row was just inserted, so the re-select can't miss (legacy typed this any). const reservation = this.getReservationWithJoins(Number(result.lastInsertRowid))!; - return { reservation, accommodationCreated }; + return { reservation, accommodationCreated, stayMirror }; } updatePositions(tripId: string | number, positions: { id: number; day_plan_position?: number }[], dayId?: number | string | null) { @@ -688,10 +713,12 @@ export class ReservationsService { /** The accommodation upsert, the reservation update, the endpoint replace * and the metadata sync are one logical write — all-or-nothing. */ update(id: string | number, tripId: string | number, data: UpdateReservationData, current: Reservation): { reservation: ReservationRow; accommodationChanged: boolean } { - return this.db.transaction(() => this.updateInTx(id, tripId, data, current)); + const { stayMirror, ...written } = this.db.transaction(() => this.updateInTx(id, tripId, data, current)); + this.announceStayMirror(tripId, stayMirror); + return written; } - private updateInTx(id: string | number, tripId: string | number, data: UpdateReservationData, current: Reservation): { reservation: ReservationRow; accommodationChanged: boolean } { + private updateInTx(id: string | number, tripId: string | number, data: UpdateReservationData, current: Reservation): { reservation: ReservationRow; accommodationChanged: boolean; stayMirror: AccommodationMirror } { const { title, reservation_time, reservation_end_time, location, confirmation_number, notes, url, day_id, end_day_id, place_id, assignment_id, @@ -700,6 +727,7 @@ export class ReservationsService { } = data; let accommodationChanged = false; + let stayMirror = noStayMirror(); // Update or create accommodation for hotel reservations let resolvedAccId: number | null = accommodation_id !== undefined ? (accommodation_id || null) : (current.accommodation_id ?? null); @@ -717,12 +745,17 @@ export class ReservationsService { 'UPDATE day_accommodations SET place_id = ?, start_day_id = ?, end_day_id = ?, check_in = ?, check_out = ?, confirmation = ? WHERE id = ?', accPlaceId || null, start_day_id, end_day_id, check_in || null, check_out || null, accConf || confirmation_number || null, resolvedAccId ); + // The stay just moved. Its stop moves with it, or it is left sitting on a + // day nobody sleeps there any more, hidden from the day list because it + // still carries this booking's id and stranded in the middle of the drive. + stayMirror = this.accommodations.moveStayStop(resolvedAccId, accPlaceId || null, start_day_id); } else if (accPlaceId) { const accResult = this.db.run( 'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_out, confirmation) VALUES (?, ?, ?, ?, ?, ?, ?)', tripId, accPlaceId, start_day_id, end_day_id, check_in || null, check_out || null, accConf || confirmation_number || null ); resolvedAccId = Number(accResult.lastInsertRowid); + stayMirror = this.accommodations.attachStayStop(resolvedAccId, accPlaceId, start_day_id); } accommodationChanged = true; } @@ -836,19 +869,20 @@ export class ReservationsService { // The caller passed the pre-checked `current` row, so the re-select can't // miss (legacy typed this any). const reservation = this.getReservationWithJoins(id)!; - return { reservation, accommodationChanged }; + return { reservation, accommodationChanged, stayMirror }; } /** The accommodation + budget-item + reservation deletes are one logical * cascade — all-or-nothing. */ remove(id: string | number, tripId: string | number): { deleted: { id: number; title: string; type: string; accommodation_id: number | null } | undefined; accommodationDeleted: boolean; deletedBudgetItemId: number | null } { - return this.db.transaction(() => { + const removed = this.db.transaction(() => { const reservation = this.db.get<{ id: number; title: string; type: string; accommodation_id: number | null }>( 'SELECT id, title, type, accommodation_id FROM reservations WHERE id = ? AND trip_id = ?', id, tripId ); - if (!reservation) return { deleted: undefined, accommodationDeleted: false, deletedBudgetItemId: null }; + if (!reservation) return { deleted: undefined, accommodationDeleted: false, deletedBudgetItemId: null, stayMirror: noStayMirror() }; let accommodationDeleted = false; + let stayMirror = noStayMirror(); if (reservation.accommodation_id) { // trip_id in the WHERE, not just the reservation's own scope: a row // written before referencesOutsideTrip existed can still carry a @@ -857,6 +891,11 @@ export class ReservationsService { 'DELETE FROM day_accommodations WHERE id = ? AND trip_id = ?', reservation.accommodation_id, tripId, ); accommodationDeleted = removed.changes > 0; + // Only once that DELETE actually took a row on THIS trip. The stops go by + // accommodation id alone, which is exactly the reach the trip_id guard above + // is there to deny a foreign id. A stop left carrying a dead booking's id is + // one the day list hides and nobody can reach to remove. + if (accommodationDeleted) stayMirror = this.accommodations.dropStayStops(reservation.accommodation_id); } const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE trip_id = ? AND reservation_id = ?', tripId, id); @@ -865,8 +904,11 @@ export class ReservationsService { } this.db.run('DELETE FROM reservations WHERE id = ?', id); - return { deleted: reservation, accommodationDeleted, deletedBudgetItemId: linkedBudget ? linkedBudget.id : null }; + return { deleted: reservation, accommodationDeleted, deletedBudgetItemId: linkedBudget ? linkedBudget.id : null, stayMirror }; }); + const { stayMirror, ...answer } = removed; + this.announceStayMirror(tripId, stayMirror); + return answer; } /** POST side effect: auto-create a linked budget item when a price is provided. */ diff --git a/server/tests/helpers/accommodations-service.ts b/server/tests/helpers/accommodations-service.ts index 9cbc8dea5..6864028a3 100644 --- a/server/tests/helpers/accommodations-service.ts +++ b/server/tests/helpers/accommodations-service.ts @@ -17,7 +17,17 @@ import { TrekPhotosRepository } from '../../src/nest/photos/trek-photos.reposito * why this is a helper and not five copies across the suites. */ export function makeAccommodationsService(conn: Database): AccommodationsService { - const dbs = new DatabaseService(conn); + return accommodationsOver(new DatabaseService(conn)); +} + +/** + * The same, for a suite that already holds the DatabaseService. + * + * ReservationsService takes one now, because a hotel booking owes the day plan + * the same stop a night entered under Days does, and every suite that builds + * that service by hand needs one to hand it. + */ +export function accommodationsOver(dbs: DatabaseService): AccommodationsService { const permissions = new PermissionsService(dbs); const realtime = new RealtimeService(); const assignments = new AssignmentsService( diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index ecaa1388f..2cad811ab 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -167,11 +167,12 @@ export function createMcpTestRegistry(): McpRegistry { journeyDomain, generalStorage, ); - const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService, notificationsStub(), new ReservationsReadRepository(dbService)); // One instance, four consumers: AssignmentsMcp, ReservationsMcp, PlacesMcp and // AccommodationsService, which writes the day stop a booked night implies. const assignmentsService = new AssignmentsService(dbService, permissionsService, realtimeService, queryHelpersService, journeyDomain); const accommodationsService = new AccommodationsService(dbService, permissionsService, realtimeService, assignmentsService); + // Built after it: a hotel booking writes the stay's day stop through this one. + const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService, notificationsStub(), new ReservationsReadRepository(dbService), accommodationsService); const membersService = new TripMembersService(dbService, budgetService, new UserCleanupService(dbService, budgetService), permissionsService, realtimeService, notificationsStub()); const tripsService = new TripsService( dbService, diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 3f36397de..afef3c31e 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -94,7 +94,6 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF const todos = new TodoService(dbs, permissions, realtime); const packing = new PackingService(dbs, permissions, realtime, notificationsStub()); const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService(), generalStorage); - const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs)); const collab = new CollabService(dbs, permissions, realtime, notificationsStub(), generalStorage, new RateLimitService()); const vacay = new VacayService(dbs, realtime, notificationsStub()); const days = new DaysService(dbs, permissions, realtime, queryHelpers); @@ -111,6 +110,8 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF const llmConfig = new LlmConfigResolver(new SettingsService(dbs), dbs, addons); const oauth = new PluginOAuthService(dbs); const accommodations = new AccommodationsService(dbs, permissions, realtime, assignments); + // After accommodations: a hotel booking writes the stay's day stop through it. + const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs), accommodations); const trips = new TripsService(dbs, reservations, days, permissions, budget, vacay, realtime, unsplash, generalStorage); const members = new TripMembersService(dbs, budget, new UserCleanupService(dbs, budget), permissions, realtime, notificationsStub()); const guards = new PluginGuards(dbs, permissions, addons); diff --git a/server/tests/unit/nest/calendar.service.test.ts b/server/tests/unit/nest/calendar.service.test.ts index 5c9f568a2..f27c780c2 100644 --- a/server/tests/unit/nest/calendar.service.test.ts +++ b/server/tests/unit/nest/calendar.service.test.ts @@ -56,6 +56,7 @@ import { CalendarService, foldICS } from '../../../src/nest/calendar/calendar.se import { CalendarModule } from '../../../src/nest/calendar/calendar.module'; import { expectRegisteredProvider } from '../../helpers/module-providers'; import { notificationsStub } from '../../helpers/notifications'; +import { accommodationsOver } from '../../helpers/accommodations-service'; const dbs = () => new DatabaseService(testDb); const budgetSvc = new BudgetService(dbs(), new PermissionsService(dbs()), new ExchangeRatesService(), new RealtimeService()); @@ -63,7 +64,7 @@ const budgetSvc = new BudgetService(dbs(), new PermissionsService(dbs()), new Ex // Named `svc` so the moved cases below read exactly as they did on TripsService. const svc = new CalendarService( dbs(), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), accommodationsOver(dbs())), ); beforeAll(() => { diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index ae30396df..58f39e6f8 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -74,8 +74,9 @@ const bridge = { }; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; +import { makeAccommodationsService } from '../../helpers/accommodations-service'; -const svc = new ReservationsService(new DatabaseService(testDb), permissionsStub, budget as unknown as BudgetService, new RealtimeService(), notificationsStub(notif.send), new ReservationsReadRepository(new DatabaseService(testDb))); +const svc = new ReservationsService(new DatabaseService(testDb), permissionsStub, budget as unknown as BudgetService, new RealtimeService(), notificationsStub(notif.send), new ReservationsReadRepository(new DatabaseService(testDb)), makeAccommodationsService(testDb)); beforeAll(() => { createTables(testDb); runMigrations(testDb); }); beforeEach(() => { @@ -1054,3 +1055,131 @@ describe('ReservationsService — referenced ids stay inside the trip', () => { expect(testDb.prepare('SELECT accommodation_id FROM reservations WHERE id = ?').get(res.id)).toEqual({ accommodation_id: null }); }); }); + +describe('the day stop a hotel booking implies', () => { + // Booking a night on this form is booking a night: it shows in the day header + // exactly like one entered under Days, and road trip mode draws the same booking + // as a service stop instead. Before this the booking form was the one door that + // wrote the stay without the stop, so a hotel entered here was invisible to the + // drive and had to be added a second time as an ordinary place. + const stopsOn = (dayId: number) => + testDb.prepare('SELECT id, place_id, accommodation_id FROM day_assignments WHERE day_id = ? ORDER BY order_index').all(dayId) as + { id: number; place_id: number; accommodation_id: number | null }[]; + const tripWithDays = () => { + const { trip } = ownerTrip({ start_date: '2030-05-01', end_date: '2030-05-03' }); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number }[]; + return { trip, days }; + }; + + it('RESV-STAY-001: a hotel booked on the form lands on its check-in day', () => { + const { trip, days } = tripWithDays(); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + + const { reservation } = svc.create(String(trip.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { place_id: place.id, start_day_id: days[0].id, end_day_id: days[2].id }, + }); + + expect(stopsOn(days[0].id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: Number(reservation.accommodation_id) })]); + // Typed as lodging, so the road trip draws it as the service stop it is. + expect(testDb.prepare('SELECT stop_type FROM places WHERE id = ?').get(place.id)).toMatchObject({ stop_type: 'hotel' }); + // Only the check-in day, the same as every other door. + expect(stopsOn(days[2].id)).toEqual([]); + }); + + it('RESV-STAY-002: a booking that is not a hotel puts nothing on the plan', () => { + const { trip, days } = tripWithDays(); + svc.create(String(trip.id), { title: 'Museum tour', type: 'tour', day_id: days[0].id }); + expect(stopsOn(days[0].id)).toEqual([]); + }); + + it('RESV-STAY-003: moving the booking to another day takes its stop along', () => { + const { trip, days } = tripWithDays(); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { reservation } = svc.create(String(trip.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { place_id: place.id, start_day_id: days[0].id, end_day_id: days[0].id }, + }); + const current = svc.getReservation(String(reservation.id), String(trip.id))!; + + svc.update(String(reservation.id), String(trip.id), { + type: 'hotel', + create_accommodation: { place_id: place.id, start_day_id: days[1].id, end_day_id: days[2].id }, + } as never, current); + + // Left behind it would sit on a day nobody sleeps there, hidden from the day + // list because it still carries this booking's id. + expect(stopsOn(days[0].id)).toEqual([]); + expect(stopsOn(days[1].id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: Number(reservation.accommodation_id) })]); + }); + + it('RESV-STAY-004: an edit that first creates the stay writes the stop too', () => { + // A booking saved without a date range, then given one. resolvedAccId is null on + // the way in, so this takes the insert branch rather than the update branch. + const { trip, days } = tripWithDays(); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { reservation } = svc.create(String(trip.id), { title: 'Hotel', type: 'hotel' }); + const current = svc.getReservation(String(reservation.id), String(trip.id))!; + + svc.update(String(reservation.id), String(trip.id), { + type: 'hotel', + create_accommodation: { place_id: place.id, start_day_id: days[1].id, end_day_id: days[1].id }, + } as never, current); + + const stay = testDb.prepare('SELECT id FROM day_accommodations WHERE trip_id = ?').get(trip.id) as { id: number }; + expect(stopsOn(days[1].id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: stay.id })]); + }); + + it('RESV-STAY-005: deleting the booking takes the stop with the stay', () => { + const { trip, days } = tripWithDays(); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { reservation } = svc.create(String(trip.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { place_id: place.id, start_day_id: days[0].id, end_day_id: days[0].id }, + }); + + const { accommodationDeleted } = svc.remove(String(reservation.id), String(trip.id)); + + expect(accommodationDeleted).toBe(true); + // A stop still carrying a dead booking's id is one the day list hides and + // nobody can reach to remove. + expect(stopsOn(days[0].id)).toEqual([]); + }); + + it('RESV-STAY-006: a place the traveller already planned that day keeps its own row', () => { + const { trip, days } = tripWithDays(); + const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const own = testDb.prepare('INSERT INTO day_assignments (day_id, place_id, order_index) VALUES (?, ?, 0)').run(days[0].id, place.id); + + const { reservation } = svc.create(String(trip.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { place_id: place.id, start_day_id: days[0].id, end_day_id: days[0].id }, + }); + + // No second row, and the booking claims neither: cancelling it must not delete + // a stop the traveller placed. + expect(stopsOn(days[0].id)).toEqual([{ id: Number(own.lastInsertRowid), place_id: place.id, accommodation_id: null }]); + svc.remove(String(reservation.id), String(trip.id)); + expect(stopsOn(days[0].id)).toHaveLength(1); + }); + + it('RESV-STAY-007: a foreign accommodation_id reaches no stop on the other trip', () => { + // The trip_id guard on the stay delete is what denies that reach; the stops go + // by accommodation id alone and would otherwise follow it straight over. + const { user: attacker } = createUser(testDb); + const { user: victim } = createUser(testDb, { email: 'victim@example.test' }); + const mine = createTrip(testDb, attacker.id, { start_date: '2030-05-01', end_date: '2030-05-03' }); + const theirs = createTrip(testDb, victim.id, { start_date: '2030-05-01', end_date: '2030-05-03' }); + const foreignPlace = createPlace(testDb, theirs.id, { name: 'Their hotel' }); + const foreignDay = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(theirs.id) as { id: number }; + const foreignAcc = createDayAccommodation(testDb, theirs.id, foreignPlace.id, foreignDay.id, foreignDay.id); + const theirStop = testDb.prepare('INSERT INTO day_assignments (day_id, place_id, order_index, accommodation_id) VALUES (?, ?, 0, ?)') + .run(foreignDay.id, foreignPlace.id, foreignAcc.id); + const res = createReservation(testDb, mine.id, { title: 'Hotel', type: 'hotel' }); + testDb.prepare('UPDATE reservations SET accommodation_id = ? WHERE id = ?').run(foreignAcc.id, res.id); + + svc.remove(String(res.id), String(mine.id)); + + expect(testDb.prepare('SELECT id FROM day_assignments WHERE id = ?').get(Number(theirStop.lastInsertRowid))).toBeTruthy(); + }); +}); diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index 0e9a81607..747b629b8 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -70,6 +70,7 @@ import { makeAccommodationsService } from '../../helpers/accommodations-service' import { MapsService } from '../../../src/nest/maps/maps.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { notificationsStub } from '../../helpers/notifications'; +import { accommodationsOver } from '../../helpers/accommodations-service'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { UnsplashService } from '../../../src/nest/unsplash/unsplash.service'; import { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; @@ -101,7 +102,7 @@ const buildReadModel = (database: DatabaseService, roster: TripMembersService = new TripReadModelService( database, roster, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), accommodationsOver(dbs())), new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub(), makeStorageFixture('').storage, new RateLimitService()), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 4e8a448bf..88b431ed0 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -118,7 +118,7 @@ const createAccommodation = accommodationsSvc.createAccommodation.bind(accommoda const svc = new TripsService( dbs(), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), accommodationsSvc), daysSvc, new PermissionsService(dbs()), budgetSvc, @@ -131,7 +131,7 @@ const membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupServi const readModelSvc = new TripReadModelService( dbs(), membersSvc, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), accommodationsSvc), new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub(), coversFx.storage, new RateLimitService()), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), @@ -1098,7 +1098,7 @@ describe('quirk fixes', () => { const fdbs = failingConnection(match); return new TripsService( fdbs, - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), accommodationsSvc), daysSvc, new PermissionsService(dbs()), budgetSvc, diff --git a/server/tests/unit/services/airtrailImport.test.ts b/server/tests/unit/services/airtrailImport.test.ts index 4c36d5b5a..6afd8b2d2 100644 --- a/server/tests/unit/services/airtrailImport.test.ts +++ b/server/tests/unit/services/airtrailImport.test.ts @@ -22,6 +22,7 @@ import { ReservationsReadRepository } from '../../../src/nest/reservations/reser import type { AirtrailClient } from '../../../src/nest/integrations/airtrail.client'; import type { AirtrailService } from '../../../src/nest/integrations/airtrail.service'; import { notificationsStub } from '../../helpers/notifications'; +import { accommodationsOver } from '../../helpers/accommodations-service'; // The client and the per-user credentials are the only stubs; the reservation // writes go through the real service against the real test DB, as before. They @@ -43,6 +44,7 @@ function makeImportService(): AirtrailImportService { realtime, notificationsStub(), new ReservationsReadRepository(dbs()), + accommodationsOver(dbs()), ), { listFlights } as unknown as AirtrailClient, { diff --git a/wiki/Accommodations.md b/wiki/Accommodations.md index 0b940d86c..6e01dd052 100644 --- a/wiki/Accommodations.md +++ b/wiki/Accommodations.md @@ -35,11 +35,18 @@ For each day between the **From** day and **To** day (inclusive), the accommodat ## On the route -Booking a night from the Day Detail panel also puts its place on the check-in day, as a -stop of its own. That stop is what the map draws a line to and what the Road Trip view -builds its route from, so the hotel now shows up there without having to be entered a -second time as an ordinary place. A night booked in the Road Trip view has always worked -this way; this makes the two directions match. +Booking a night also puts its place on the check-in day, as a stop of its own. That stop +is what the map draws a line to and what the Road Trip view builds its route from, so the +hotel shows up on the drive without having to be entered a second time as an ordinary +place. It works the same way whichever way you book the night: the Day Detail panel, the +booking form under Bookings, the phone, the Road Trip view, an MCP client or a plugin. + +The two views show the same night differently, and both are the whole picture: + +- **Days** keeps it in the day header, as the badge it has always been. The stop itself is + hidden there, because the row would be that same hotel a second time. +- **Road Trip** draws it as a service stop in the driving chain, which is the view that + needs to know where the day ends. A few details worth knowing: @@ -52,11 +59,11 @@ A few details worth knowing: placed, and the booking simply rides along with it. - Moving the booking to a different check-in day moves its stop with it. Deleting the booking removes the stop it created, and leaves a stop you placed yourself standing. -- Service stops, hotels among them, can be hidden from the day lists with **Show in Days - too** under **Service stops** in the Road Trip settings. +- Hotels are service stops, so **Show in Days too** under **Service stops** in the Road + Trip settings also decides whether they appear in the places list. -Stays booked before this existed keep their day plans untouched. Open such a booking and -save it once and it picks up its stop. +Nights booked before this existed are given their stop when the server upgrades, so trips +you already have show their hotels on the drive without anyone re-saving anything. ## In the day plan sidebar