diff --git a/client/src/api/client.ts b/client/src/api/client.ts index 595451215..b477380ba 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -1428,7 +1428,7 @@ export const accommodationsApi = { list: (tripId: number | string) => apiClient.get(`/trips/${tripId}/accommodations`).then(r => r.data), create: (tripId: number | string, data: AccommodationCreateRequest) => apiClient.post(`/trips/${tripId}/accommodations`, data).then(r => r.data), update: (tripId: number | string, id: number, data: AccommodationUpdateRequest) => apiClient.put(`/trips/${tripId}/accommodations/${id}`, data).then(r => r.data), - delete: (tripId: number | string, id: number) => apiClient.delete(`/trips/${tripId}/accommodations/${id}`).then(r => r.data), + delete: (tripId: number | string, id: number, opts?: { keepStop?: boolean }) => apiClient.delete(`/trips/${tripId}/accommodations/${id}`, opts?.keepStop ? { params: { keepStop: 'true' } } : undefined).then(r => r.data), } export const dayNotesApi = { diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 8a2cb08f0..55394d69c 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -55,9 +55,10 @@ import { usePlannerHistory } from '../../hooks/usePlannerHistory' import { useAirtrailConnection } from '../../hooks/useAirtrailConnection' import { useIsTouch } from '../../hooks/useIsTouch' import { usePluginStore } from '../../store/pluginStore' -import type { Accommodation, TripMember, Day, Place, Reservation } from '../../types' +import type { Accommodation, Assignment, TripMember, Day, Place, Reservation } from '../../types' import { OFM_POSITRON, DEFAULT_MAP_LAT, DEFAULT_MAP_LNG, DEFAULT_MAP_ZOOM } from '../../constants/mapDefaults' import { useTileUrl } from '../../hooks/useTileUrl' +import { applyStayStops } from '../../store/stayStops' import { resolvePoolAssignmentId } from './tripPlannerModel' import { isDeepLinkableTripTab, TRIP_TAB_LABEL_KEYS } from '../../constants/tripTabs' import { isRoutableReservation } from '../../utils/reservationRoutes' @@ -128,9 +129,27 @@ export function useTripPlanner() { // someone planning a road trip stays in it across reloads without it leaking into // their next, non-driving trip. const [roadtripMode, setRoadtripMode] = useState(() => sessionStorage.getItem(`trip-roadtrip-${tripId}`) === '1') - const assignments = useMemo(() => roadtripMode || roadtripSettings.roadtrip_service_stops_in_days !== false ? storedAssignments : Object.fromEntries( - Object.entries(storedAssignments).map(([dayId, visits]) => [dayId, visits.filter(visit => !isServiceStopType(visit.place?.stop_type))]), - ), [roadtripMode, roadtripSettings.roadtrip_service_stops_in_days, storedAssignments]) + // Two reasons a stop can be road-trip-only, and they are not the same reason. + // + // The switch is the traveller's: it hides the petrol stations and rest areas + // they added along the drive from a day list they want to read as a plan. + // + // A stop a lodging booking put there is hidden whatever the switch says, + // because the day already shows that booking as its own overnight block and + // the row would be the same hotel a second time. Road trip mode wants it: the + // drive has to end somewhere, and that somewhere is where you sleep. + const assignments = useMemo(() => { + if (roadtripMode) return storedAssignments + const hideServiceStops = roadtripSettings.roadtrip_service_stops_in_days === false + const hidden = (visit: Assignment) => visit.accommodation_id != null + || (hideServiceStops && isServiceStopType(visit.place?.stop_type)) + // Same object back when nothing is hidden, so a trip without bookings does not + // rebuild every day list on each render of this hook. + if (!Object.values(storedAssignments).some(visits => visits.some(hidden))) return storedAssignments + return Object.fromEntries( + Object.entries(storedAssignments).map(([dayId, visits]) => [dayId, visits.filter(v => !hidden(v))]), + ) + }, [roadtripMode, roadtripSettings.roadtrip_service_stops_in_days, storedAssignments]) const places = useMemo(() => roadtripMode || roadtripSettings.roadtrip_service_stops_in_days !== false ? allPlaces : allPlaces.filter(place => !isServiceStopType(place.stop_type)), [roadtripMode, roadtripSettings.roadtrip_service_stops_in_days, allPlaces]) const toggleRoadtripMode = useCallback(() => { setRoadtripMode(prev => { @@ -937,7 +956,9 @@ export function useTripPlanner() { if (stopDraft.editing) { await tripActions.updatePlace(tripId, stopDraft.editing.placeId, { stop_type: stopType, duration_minutes: dwellMinutes }) if (stopDraft.editing.accommodationId) { - await accommodationsApi.delete(tripId, stopDraft.editing.accommodationId) + // The night is what was switched off, not the stop: it stays where it is + // in the drive and becomes an ordinary pause. + applyStayStops(await accommodationsApi.delete(tripId, stopDraft.editing.accommodationId, { keepStop: true })) await loadAccommodations() } updateRouteForDay(dayId) diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 74af2d634..84b00ab7d 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -4735,13 +4735,53 @@ function runMigrations(db: Database.Database): void { // Provenance for a day stop that a lodging booking put there rather than the // traveller: it carries the stay's id, so moving or deleting the booking can // move or delete exactly that stop and never one somebody placed by hand. - // Deliberately left NULL for every row that already exists: everything planned - // before this shipped was planned by hand, and a booking must not start - // claiming ownership of it. + // Every row that already exists stays NULL: those were planned by hand, and a + // booking must not start claiming ownership of them. The step below adds the + // missing stops instead, which is a different thing from claiming old ones. () => { const hasColumn = db.prepare("SELECT 1 FROM pragma_table_info('day_assignments') WHERE name = 'accommodation_id'").get(); if (!hasColumn) db.exec('ALTER TABLE day_assignments ADD COLUMN accommodation_id INTEGER'); }, + + /* + * Give every stay booked before this release the day stop it would get today. + * + * Road trip mode builds its drive out of day_assignments alone, so a hotel + * booked in Days mode was invisible there and the traveller had to add the + * same place a second time by hand. New bookings get the stop as they are + * written; without this step the fix would only ever apply to trips planned + * after the upgrade, and the trips people already have would stay broken. + * + * Skipped on purpose: a stay whose place is gone (place_id is ON DELETE SET + * NULL, and the booking form writes stays that never had one), and a place the + * traveller already planned for that day, whose row stays theirs and unmarked. + * Re-runnable: the same NOT EXISTS decides both times. + */ + () => { + const stays = db.prepare(` + SELECT a.id, a.place_id, a.start_day_id + FROM day_accommodations a + WHERE a.place_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM day_assignments da + WHERE da.day_id = a.start_day_id AND da.place_id = a.place_id + ) + ORDER BY a.id + `).all() as Array<{ id: number; place_id: number; start_day_id: number }>; + + const insert = db.prepare( + `INSERT INTO day_assignments (day_id, place_id, order_index, accommodation_id) + VALUES (?, ?, COALESCE((SELECT MAX(order_index) + 1 FROM day_assignments WHERE day_id = ?), 0), ?)` + ); + // Arriving somewhere is what the day was for, so the stop goes last, the same + // position a stay booked today lands in. + const stamp = db.prepare("UPDATE places SET stop_type = 'hotel' WHERE id = ? AND (stop_type IS NULL OR stop_type = '')"); + for (const stay of stays) { + insert.run(stay.start_day_id, stay.place_id, stay.start_day_id, stay.id); + stamp.run(stay.place_id); + } + if (stays.length > 0) console.log(`[DB] Put ${stays.length} booked night(s) on their check-in day`); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/nest/accommodations/accommodations.controller.ts b/server/src/nest/accommodations/accommodations.controller.ts index 88505078a..436e56624 100644 --- a/server/src/nest/accommodations/accommodations.controller.ts +++ b/server/src/nest/accommodations/accommodations.controller.ts @@ -2,6 +2,7 @@ import { Body, Controller, Delete, + Query, Get, Headers, HttpException, @@ -117,11 +118,15 @@ export class AccommodationsController { @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string, + @Query('keepStop') keepStop?: string, ) { if (!this.accommodations.get(id, tripId)) { throw new HttpException({ error: 'Accommodation not found' }, 404); } - const { linkedReservationIds, deletedBudgetItemIds, mirror } = this.accommodations.remove(id); + // Turning a night back into a pause in road trip mode: the booking goes, the + // stop stays and becomes the traveller's. Everywhere else a cancelled booking + // takes the stop it brought with it. + const { linkedReservationIds, deletedBudgetItemIds, mirror } = this.accommodations.remove(id, { keepStop: keepStop === 'true' }); this.accommodations.announceMirror(tripId, mirror, (event, payload) => this.accommodations.broadcast(tripId, event, payload, socketId), socketId); for (const reservationId of linkedReservationIds) { this.accommodations.broadcast(tripId, 'reservation:deleted', { reservationId }, socketId); diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index dd086bffc..d76da7c02 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -123,8 +123,8 @@ export class AccommodationsService { return this.updateAccommodation(id, existing, fields); } - remove(id: string | number) { - return this.deleteAccommodation(id); + remove(id: string | number, opts: { keepStop?: boolean } = {}) { + return this.deleteAccommodation(id, opts); } /** @@ -239,15 +239,18 @@ export class AccommodationsService { /** * Carry the mirrored stop over to wherever the booking now is. * - * A booking that owns no stop gets one, which is how a stay made before any of - * this existed picks one up: re-save it once and it appears. The cost is that a - * mirrored stop somebody deleted by hand comes back the next time they touch the - * booking, which is the lesser of the two surprises. + * Only its own stop moves. A booking that owns none is one whose stop belongs to + * the traveller: booked in road trip mode, where the place was put on the day + * first, or booked for a place they had already planned there. Moving that is not + * ours to do, and putting a second one on the new day next to it is exactly the + * duplicate this whole change is meant to remove. Stays booked before any of this + * existed get their stop from the migration, not from the next edit. * * Runs inside the caller's transaction. */ private remirrorStay(accommodationId: number, placeId: number | null, dayId: number): AccommodationMirror { const own = this.ownStops(accommodationId); + if (own.length === 0) return noMirror(); if (own.length === 1 && own[0].day_id === dayId && own[0].place_id === placeId) return noMirror(); const mirror = noMirror(); @@ -350,7 +353,7 @@ export class AccommodationsService { * that no longer exists. `linkedReservationId` / `deletedBudgetItemId` stay on the * result as the first of each, because the RPC, MCP and REST callers read them. */ - deleteAccommodation(id: string | number): { + deleteAccommodation(id: string | number, opts: { keepStop?: boolean } = {}): { linkedReservationId: number | null; deletedBudgetItemId: number | null; linkedReservationIds: number[]; @@ -372,8 +375,17 @@ export class AccommodationsService { // 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 }); } diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index 7447e10dd..da4e0ee5f 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -729,19 +729,22 @@ describe('the day stop a booking implies', () => { expect(mirror).toEqual({ created: null, removed: [], stamped: null }); }); - it('ACC-030 a stay booked before any of this existed picks up its stop on the next save', () => { - // The deliberate alternative to rewriting everybody finished trips in a migration: - // the booking reaches the route the moment somebody opens it and saves. + it('ACC-030 a stay whose stop belongs to the traveller does not get a second one', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); + const second = createDay(testDb, trip.id); const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); - const old = createDayAccommodation(testDb, trip.id, place.id, day.id, day.id) as any; + // The road trip books a night on a place it put on the day first, so the stop is + // the traveller's and the booking owns nothing. + createDayAssignment(testDb, day.id, place.id); + const stay = createDayAccommodation(testDb, trip.id, place.id, day.id, day.id) as any; - const existing = svc.getAccommodation(old.id, trip.id)!; - svc.updateAccommodation(old.id, existing, { check_in: '15:00' }); + const existing = svc.getAccommodation(stay.id, trip.id)!; + svc.updateAccommodation(stay.id, existing, { start_day_id: second.id, end_day_id: second.id }); - expect(stopsOn(day.id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: old.id })]); + expect(stopsOn(second.id)).toEqual([]); + expect(stopsOn(day.id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: null })]); }); it('ACC-031 cancelling a booking takes back its own stop and leaves the other one', () => { diff --git a/shared/src/assignment/assignment.schema.ts b/shared/src/assignment/assignment.schema.ts index 64c5a7988..e342c8b98 100644 --- a/shared/src/assignment/assignment.schema.ts +++ b/shared/src/assignment/assignment.schema.ts @@ -48,6 +48,11 @@ export const assignmentSchema = z.object({ // not a place (booking arrival / morning hotel). null = inherit the day default. // Inert when the previous timeline element is a place. incoming_leg_transport_mode: z.string().nullable().optional(), + // The lodging booking that put this stop on the day, when one did. The day + // planner already shows that booking as its own overnight block, so it hides + // this row and leaves it to road trip mode, which is the view that needs the + // hotel in the driving chain. Null for every stop a traveller placed. + accommodation_id: z.number().nullable().optional(), participants: z.array(assignmentParticipantSchema).optional(), created_at: z.string().optional(), place: assignmentPlaceSchema,