diff --git a/client/src/components/Planner/usePlacesSidebar.test.tsx b/client/src/components/Planner/usePlacesSidebar.test.tsx index 03918897b..3bf66d52f 100644 --- a/client/src/components/Planner/usePlacesSidebar.test.tsx +++ b/client/src/components/Planner/usePlacesSidebar.test.tsx @@ -1,4 +1,4 @@ -// FE-PLANNER-PSHOOK-001 to FE-PLANNER-PSHOOK-048 +// FE-PLANNER-PSHOOK-001 to FE-PLANNER-PSHOOK-051 import { http, HttpResponse } from 'msw'; import userEvent from '@testing-library/user-event'; import { render, screen, fireEvent, act, waitFor } from '../../../tests/helpers/render'; @@ -137,6 +137,39 @@ describe('usePlacesSidebar filtering', () => { expect([...S.plannedIds]).toEqual([1]); }); + // #2072 — a hotel is linked through its stay, never dragged onto a day, so the + // pool called it unplanned while the day header printed its name. + it('FE-PLANNER-PSHOOK-049: a place linked to a stay counts as planned', () => { + const hotel = buildPlace({ id: 1, name: 'Hotel' }); + const loose = buildPlace({ id: 2, name: 'Loose' }); + const accommodations = [{ place_id: 1, start_day_id: 3, end_day_id: 5 }]; + render(); + + act(() => { S.setFilter('unplanned'); }); + expect(names()).toEqual(['Loose']); + act(() => { S.setFilter('planned'); }); + expect(names()).toEqual(['Hotel']); + }); + + it('FE-PLANNER-PSHOOK-050: a place on a day-anchored booking counts as planned', () => { + const venue = buildPlace({ id: 1, name: 'Venue' }); + const loose = buildPlace({ id: 2, name: 'Loose' }); + seedStore(useTripStore, { reservations: [{ id: 9, place_id: 1, day_id: 3 }] }); + render(); + + act(() => { S.setFilter('unplanned'); }); + expect(names()).toEqual(['Loose']); + }); + + it('FE-PLANNER-PSHOOK-051: a booking with no day leaves its place unplanned', () => { + const venue = buildPlace({ id: 1, name: 'Venue' }); + seedStore(useTripStore, { reservations: [{ id: 9, place_id: 1, day_id: null }] }); + render(); + + act(() => { S.setFilter('unplanned'); }); + expect(names()).toEqual(['Venue']); + }); + it('FE-PLANNER-PSHOOK-006: the planned filter keeps only assigned places', () => { const planned = buildPlace({ id: 1, name: 'Planned' }); const loose = buildPlace({ id: 2, name: 'Loose' }); diff --git a/client/src/components/Planner/usePlacesSidebar.ts b/client/src/components/Planner/usePlacesSidebar.ts index aab094a62..2b6bd7734 100644 --- a/client/src/components/Planner/usePlacesSidebar.ts +++ b/client/src/components/Planner/usePlacesSidebar.ts @@ -15,10 +15,16 @@ import { placeToSaveTarget } from '../Collections/saveTarget' import type { Place, Category, Day, AssignmentsMap } from '../../types' import { getGoogleMapsUrlForPlace } from './placeGoogleMaps' import { safeHttpUrl } from '../../utils/safeUrl' +import { plannedPlaceIds, type PlannedAccommodation } from '../../utils/plannedPlaces' + +/** Stable identity — a fresh [] default would invalidate the planned memo on every render. */ +const NO_ACCOMMODATIONS: PlannedAccommodation[] = [] export interface PlacesSidebarProps { tripId: number places: Place[] + /** The trip's stays — hook-local state in useTripPlanner, so it arrives as a prop. */ + accommodations?: PlannedAccommodation[] categories: Category[] assignments: AssignmentsMap selectedDayId: number | null @@ -45,13 +51,15 @@ export interface PlacesSidebarProps { */ export function usePlacesSidebar(props: PlacesSidebarProps) { const { - tripId, places, assignments, selectedDayId, + tripId, places, assignments, selectedDayId, accommodations = NO_ACCOMMODATIONS, pushUndo, initialScrollTop, onScrollTopChange, } = props const { t } = useTranslation() const toast = useToast() const ctxMenu = useContextMenu() const trip = useTripStore((s) => s.trip) + // A booking plans the place it points at, so the pool has to see them (#2072). + const reservations = useTripStore((s) => s.reservations) const loadTrip = useTripStore((s) => s.loadTrip) const can = useCanDo() const canEditPlaces = can('place_edit', trip) @@ -216,9 +224,10 @@ export function usePlacesSidebar(props: PlacesSidebarProps) { const hasTracks = useMemo(() => places.some(p => p.route_geometry), [places]) useEffect(() => { if (filter === 'tracks' && !hasTracks) setFilter('all') }, [hasTracks, filter]) - const plannedIds = useMemo(() => new Set( - Object.values(assignments).flatMap(da => da.map(a => a.place?.id).filter(Boolean)) - ), [assignments]) + const plannedIds = useMemo( + () => plannedPlaceIds({ assignments, accommodations, reservations }), + [assignments, accommodations, reservations], + ) const filtered = useMemo(() => { const list = places.filter(p => { diff --git a/client/src/hooks/useRouteCalculation.ts b/client/src/hooks/useRouteCalculation.ts index f66df2349..67de7a8eb 100644 --- a/client/src/hooks/useRouteCalculation.ts +++ b/client/src/hooks/useRouteCalculation.ts @@ -2,7 +2,7 @@ import { useState, useCallback, useRef, useEffect, useMemo } from 'react' import { useTripStore } from '../store/tripStore' import { useSettingsStore } from '../store/settingsStore' import { calculateRouteWithLegs, withHotelBookends, type RouteProfileKey } from '../components/Map/RouteCalculator' -import { getTransportRouteEndpoints } from '../utils/dayMerge' +import { getTransportRouteEndpoints, getTransportForDay, getMergedItems } from '../utils/dayMerge' import { getDayBookendHotels, shouldDrawMorningLeg, shouldDrawEveningLeg } from '../utils/dayOrder' import { resolveLegMode } from '../components/Planner/legMode' import type { TripStoreState } from '../store/tripStore' @@ -54,46 +54,45 @@ export function useRouteCalculation(tripStore: TripStoreState, selectedDayId: nu } const thisOrder = dayOrder(dayId) - // Transport reservations for this day with a known position — mirrors getTransportForDay semantics - const dayTransports = thisOrder == null ? [] : allReservations.filter(r => { - if (!TRANSPORT_TYPES.includes(r.type)) return false - const startId = r.day_id - if (startId == null) return false - const endId = r.end_day_id ?? startId - if (startId === endId) { - if (startId !== dayId) return false - } else { - const startOrder = dayOrder(startId) - const endOrder = dayOrder(endId) - if (startOrder == null || endOrder == null) return false - if (thisOrder < startOrder || thisOrder > endOrder) return false - } - const pos = r.day_positions?.[dayId] ?? r.day_positions?.[String(dayId)] ?? r.day_plan_position - return pos != null + // The order the day plan shows is the order the map has to draw, so take it from + // the same place the plan does rather than rebuilding it here. The old builder + // read the BOOKING's position and never expanded metadata.legs, while a + // multi-leg booking stores its position per leg — so a layover flight was + // dropped from the waypoint list entirely and the airport before it was joined + // to the airport after it in one road run across an ocean (#2071). + // + // getTransportForDay brings the span filter, the hotel/assignment exclusions and + // the leg expansion with it; getMergedItems places each leg by its own saved + // position, falling back to its time. Notes carry no coordinates, so none are + // passed. + const dayTransports = thisOrder == null ? [] : getTransportForDay({ + reservations: allReservations.filter(r => TRANSPORT_TYPES.includes(r.type)), + dayId, + dayAssignmentIds: da.map(a => a.id), + days: allDays, }) + const merged = getMergedItems({ dayAssignments: da, dayNotes: [], dayTransports, dayId }) // Build a unified list of places + transports sorted by effective position. type Entry = | { kind: 'place'; lat: number; lng: number; pos: number; time: string | null; mode: string | null; incoming: string | null } | { kind: 'transport'; from: { lat: number; lng: number } | null; to: { lat: number; lng: number } | null; pos: number } - const entries: Entry[] = [ - ...da.filter(a => a.place?.lat && a.place?.lng).map(a => ({ - kind: 'place' as const, lat: a.place.lat!, lng: a.place.lng!, pos: a.order_index, time: a.place?.place_time ?? null, - // Per-segment travel mode (#1281): mode of the leg leaving this place. - mode: (a as { leg_transport_mode?: string | null }).leg_transport_mode ?? null, - // Boundary-leg mode (#1281 follow-up): mode of the leg arriving at this place. - incoming: (a as { incoming_leg_transport_mode?: string | null }).incoming_leg_transport_mode ?? null, - })), - ...dayTransports.map(r => { - const { from, to } = getTransportRouteEndpoints(r, dayId) - return { - kind: 'transport' as const, - from, - to, - pos: (r.day_positions?.[dayId] ?? r.day_positions?.[String(dayId)] ?? r.day_plan_position) as number, - } - }), - ].sort((a, b) => a.pos - b.pos) + const entries: Entry[] = merged.flatMap((item): Entry[] => { + if (item.type === 'place') { + const a = item.data + if (!a.place?.lat || !a.place?.lng) return [] + return [{ + kind: 'place', lat: a.place.lat, lng: a.place.lng, pos: item.sortKey, time: a.place?.place_time ?? null, + // Per-segment travel mode (#1281): mode of the leg leaving this place. + mode: (a as { leg_transport_mode?: string | null }).leg_transport_mode ?? null, + // Boundary-leg mode (#1281 follow-up): mode of the leg arriving at this place. + incoming: (a as { incoming_leg_transport_mode?: string | null }).incoming_leg_transport_mode ?? null, + }] + } + if (item.type !== 'transport') return [] + const { from, to } = getTransportRouteEndpoints(item.data, dayId) + return [{ kind: 'transport', from, to, pos: item.sortKey }] + }) // Group located places into driving runs. // - A transport WITH a location anchors the route to its departure point (you diff --git a/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx b/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx index f757716df..46b9557df 100644 --- a/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx +++ b/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx @@ -59,7 +59,12 @@ export default function MPlacesBrowser({ planner, shell }: MPlacesBrowserProps) if (filter === 'tracks' && !hasTracks) setFilter('all') }, [filter, hasTracks, setFilter]) - const plannedIds = useMemo(() => plannedPlaceIds(assignments), [assignments]) + // A hotel is linked through its stay and a venue through its booking; neither is + // ever dragged onto a day, and the pool used to call both unplanned (#2072). + const plannedIds = useMemo( + () => plannedPlaceIds(assignments, planner.tripAccommodations, planner.reservations), + [assignments, planner.tripAccommodations, planner.reservations], + ) const dayNumberByPlace = useMemo(() => firstPlannedDayNumbers(assignments, days), [assignments, days]) const filtered = useMemo( () => filterPool(places, { filter, categoryFilters, search, plannedIds }), diff --git a/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts b/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts index f1f18b527..d26d9a9b6 100644 --- a/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts +++ b/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts @@ -151,4 +151,16 @@ describe('placesBrowserModel — tracks pool', () => { expect(c.planned).toBe(1) expect(c.unplanned).toBe(1) }) + + // #2072 — the phone pool has to agree with the desktop one; both ask the same + // shared helper now. + it('FE-MOB-PMODEL-014: a stay and a day-anchored booking both count as planned', () => { + const ids = plannedPlaceIds( + {}, + [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + [{ place_id: 8, day_id: 10 }, { place_id: 9, day_id: null }], + ) + expect([...ids].sort((a, b) => a - b)).toEqual([7, 8]) + }) + }) diff --git a/client/src/mobile/screens/trip/places/placesBrowserModel.ts b/client/src/mobile/screens/trip/places/placesBrowserModel.ts index 1e62c9a76..fae376d1e 100644 --- a/client/src/mobile/screens/trip/places/placesBrowserModel.ts +++ b/client/src/mobile/screens/trip/places/placesBrowserModel.ts @@ -1,4 +1,8 @@ import type { AssignmentsMap, Day, Place } from '../../../../types' +import { + plannedPlaceIds as sharedPlannedPlaceIds, + type PlannedAccommodation, type PlannedReservation, +} from '../../../../utils/plannedPlaces' /** * Pure filter model of the mobile places browser. Mirrors the desktop @@ -7,14 +11,12 @@ import type { AssignmentsMap, Day, Place } from '../../../../types' * and the map markers always agree (#1541). */ -export function plannedPlaceIds(assignments: AssignmentsMap): Set { - const ids = new Set() - for (const dayAssignments of Object.values(assignments)) { - for (const a of dayAssignments) { - if (a.place?.id != null) ids.add(a.place.id) - } - } - return ids +export function plannedPlaceIds( + assignments: AssignmentsMap, + accommodations: PlannedAccommodation[] = [], + reservations: PlannedReservation[] = [], +): Set { + return sharedPlannedPlaceIds({ assignments, accommodations, reservations }) } /** place id → number of the first (lowest-numbered) day it is planned on. */ diff --git a/client/src/pages/TripPlannerPage.tsx b/client/src/pages/TripPlannerPage.tsx index 6f7c99375..ff0a0ea4d 100644 --- a/client/src/pages/TripPlannerPage.tsx +++ b/client/src/pages/TripPlannerPage.tsx @@ -553,6 +553,7 @@ function TripPlannerPageDesktop(): React.ReactElement | null { places={places} categories={categories} assignments={assignments} + accommodations={tripAccommodations} selectedDayId={selectedDayId} selectedPlaceId={selectedPlaceId} onPlaceClick={handlePlaceClick} @@ -711,7 +712,7 @@ function TripPlannerPageDesktop(): React.ReactElement | null {
{mobileSidebarOpen === 'left' ? { handleSelectDay(id); setMobileSidebarOpen(null) }} onPlaceClick={(placeId, assignmentId) => { handlePlaceClick(placeId, assignmentId) }} onReorder={handleReorder} onReorderDays={handleReorderDays} onAddDay={handleAddDay} onUpdateDayTitle={handleUpdateDayTitle} onAssignToDay={handleAssignToDay} onRouteCalculated={(r) => { if (r) { setRoute([r.coordinates]); setRouteInfo(r) } else { setRoute(null); setRouteInfo(null) } }} reservations={reservations} visibleConnectionIds={visibleConnections} onToggleConnection={toggleConnection} allConnectionsShown={allConnectionsShown} onToggleAllConnections={toggleAllConnections} onAddReservation={(dayId) => { setEditingReservation(null); tripActions.setSelectedDay(dayId); setShowReservationModal(true); setMobileSidebarOpen(null) }} onAddTransport={can('day_edit', trip) ? (dayId) => { setTransportModalDayId(dayId); setEditingTransport(null); setTransitPrefill(null); setTransportModalAutomated(false); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onOpenTransit={(r) => { setTransitJourney(r); setMobileSidebarOpen(null) }} onPlanTransit={can('day_edit', trip) && tripHasDates ? (dayId) => { setTransportModalDayId(dayId); setEditingTransport(null); setTransitPrefill(null); setTransportModalAutomated(true); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onPlanTransitLeg={can('day_edit', trip) && tripHasDates ? ({ dayId, from, to, time }) => { setTransportModalDayId(dayId); setEditingTransport(null); setTransitPrefill({ from, to, time }); setTransportModalAutomated(true); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onAddPlace={() => { setEditingPlace(null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onDayDetail={(day) => { setShowDayDetail(day); setSelectedPlaceId(null); selectAssignment(null) }} onRemoveAssignment={handleRemoveAssignment} onEditPlace={(place, assignmentId) => { setEditingPlace(place); setEditingAssignmentId(assignmentId || null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onDeletePlace={(placeId) => handleDeletePlace(placeId)} accommodations={tripAccommodations} routeShown={routeShown} routeProfile={routeProfile} onToggleRoute={() => setRouteShown(v => !v)} onSetRouteProfile={setRouteProfile} onNavigateToFiles={() => { setMobileSidebarOpen(null); handleTabChange('dateien') }} onExpandedDaysChange={setExpandedDayIds} pushUndo={pushUndo} canUndo={canUndo} lastActionLabel={lastActionLabel} onUndo={handleUndo} onEditTransport={can('day_edit', trip) ? (reservation) => { setEditingTransport(reservation); setTransportModalDayId(reservation.day_id ?? null); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onEditReservation={can('reservation_edit', trip) ? (r) => { setEditingReservation(r); setShowReservationModal(true); setMobileSidebarOpen(null) } : undefined} initialScrollTop={mobilePlanScrollTopRef.current} onScrollTopChange={(top) => { mobilePlanScrollTopRef.current = top }} showRouteToolsWhenExpanded isMobile /> - : { handlePlaceClick(placeId); setMobileSidebarOpen(null) }} onAddPlace={() => { setEditingPlace(null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onAssignToDay={handleAssignToDay} onEditPlace={(place) => { openPlaceEditor(place); setMobileSidebarOpen(null) }} onDeletePlace={(placeId) => handleDeletePlace(placeId)} onBulkDeletePlaces={(ids) => setDeletePlaceIds(ids)} onBulkDeleteConfirm={(ids) => confirmDeletePlaces(ids)} onBulkChangeCategory={(ids, catId) => confirmChangeCategory(ids, catId)} days={days} isMobile pushUndo={pushUndo} initialScrollTop={mobilePlacesScrollTopRef.current} onScrollTopChange={(top) => { mobilePlacesScrollTopRef.current = top }} /> + : { handlePlaceClick(placeId); setMobileSidebarOpen(null) }} onAddPlace={() => { setEditingPlace(null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onAssignToDay={handleAssignToDay} onEditPlace={(place) => { openPlaceEditor(place); setMobileSidebarOpen(null) }} onDeletePlace={(placeId) => handleDeletePlace(placeId)} onBulkDeletePlaces={(ids) => setDeletePlaceIds(ids)} onBulkDeleteConfirm={(ids) => confirmDeletePlaces(ids)} onBulkChangeCategory={(ids, catId) => confirmChangeCategory(ids, catId)} days={days} isMobile pushUndo={pushUndo} initialScrollTop={mobilePlacesScrollTopRef.current} onScrollTopChange={(top) => { mobilePlacesScrollTopRef.current = top }} /> }
diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 93172a5a1..5997543d5 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -35,6 +35,7 @@ import { toggleConnectionId, toggleAllConnections as flipAllConnectionsMode, type StoredConnections, } from '../../utils/connectionsVisibility' +import { plannedPlaceIds, plannedPlaceIdsForDay } from '../../utils/plannedPlaces' /** * Trip planner page logic — the big one. Owns the trip store wiring, addon @@ -445,10 +446,9 @@ export function useTripPlanner() { // the whole plan (#2024). 'unplanned' always uses the whole-trip set — a place // assigned to any day is not unplanned. const plannedIds = placesFilter === 'unplanned' || placesFilter === 'planned' - ? new Set((placesFilter === 'planned' && selectedDayId - ? (assignments[String(selectedDayId)] || []) - : Object.values(assignments).flat() - ).map(a => a.place?.id).filter(Boolean)) + ? (placesFilter === 'planned' && selectedDayId + ? plannedPlaceIdsForDay(selectedDayId, days, { assignments, accommodations: tripAccommodations, reservations }) + : plannedPlaceIds({ assignments, accommodations: tripAccommodations, reservations })) : null return places.filter(p => { @@ -467,7 +467,7 @@ export function useTripPlanner() { if (placesFilter === 'planned' && plannedIds && !plannedIds.has(p.id)) return false return true }) - }, [places, placesCategoryFilter, placesFilter, assignments, expandedDayIds, selectedDayId]) + }, [places, placesCategoryFilter, placesFilter, assignments, expandedDayIds, selectedDayId, days, tripAccommodations, reservations]) const { route, routeSegments, routeVias, routeInfo, setRoute, setRouteInfo, updateRouteForDay } = useRouteCalculation({ assignments } as any, selectedDayId, routeShown, routeProfile, tripAccommodations) diff --git a/client/src/utils/dayMerge.ts b/client/src/utils/dayMerge.ts index 302925694..d8b5dbb5a 100644 --- a/client/src/utils/dayMerge.ts +++ b/client/src/utils/dayMerge.ts @@ -1,3 +1,8 @@ +// `orderedEndpoints` is the geometry order's single source of truth, in the module +// that documents the multi-leg model. Sorting endpoints a second time here is how +// the two drift. +import { orderedEndpoints } from './flightLegs' + export const TRANSPORT_TYPES = new Set(['flight', 'train', 'bus', 'car', 'taxi', 'bicycle', 'cruise', 'ferry', 'transit', 'transport_other']) export interface MergedItem { @@ -60,9 +65,23 @@ export function getTransportRouteEndpoints( r: any, dayId: number ): { from: { lat: number; lng: number } | null; to: { lat: number; lng: number } | null } { + // A leg of a multi-leg booking spans its OWN pair of airports, endpoints[i] and + // endpoints[i+1] — not the booking's outermost two. Handing back the booking's + // from/to for every leg put a stop between two legs on a road route from the + // arrival airport back to the departure one (#2071). + // + // Only when the endpoints line up one-per-stop with the legs. An endpoint that + // failed to geocode never reaches the row, and a short list would silently map + // every earlier leg to the wrong pair — so an ambiguous booking keeps the role + // lookup it has always used. + const ordered = orderedEndpoints(r) + const legIdx: number | null = + r.__leg && ordered.length === r.__leg.total + 1 ? (r.__leg.index as number) : null + const point = (e: { lat?: number | null; lng?: number | null } | undefined): { lat: number; lng: number } | null => + e && e.lat != null && e.lng != null ? { lat: e.lat, lng: e.lng } : null const ep = (role: 'from' | 'to'): { lat: number; lng: number } | null => { - const e = (r.endpoints || []).find((x: any) => x.role === role) - return e && e.lat != null && e.lng != null ? { lat: e.lat, lng: e.lng } : null + if (legIdx != null) return point(ordered[role === 'from' ? legIdx : legIdx + 1]) + return point((r.endpoints || []).find((x: { role?: string }) => x.role === role)) } switch (getSpanPhase(r, dayId)) { case 'start': @@ -240,8 +259,13 @@ export function getMergedItems(opts: { const timed = timedTransports[ti] const minutes = timed.minutes - // Per-day position takes precedence (set by user reorder) - const perDayPos = timed.data.day_positions?.[dayId] ?? timed.data.day_positions?.[String(dayId)] + // Per-day position takes precedence (set by user reorder), then the booking's + // own slot. The map's waypoint builder has always read day_plan_position and + // this never did, so a booking carrying only that value sat between the places + // on the map and at the bottom of the day everywhere else (#2071). + const perDayPos = timed.data.day_positions?.[dayId] + ?? timed.data.day_positions?.[String(dayId)] + ?? timed.data.day_plan_position if (perDayPos != null) { result.push({ type: timed.type, sortKey: perDayPos, data: timed.data }) continue diff --git a/client/src/utils/plannedPlaces.test.ts b/client/src/utils/plannedPlaces.test.ts new file mode 100644 index 000000000..a0c786868 --- /dev/null +++ b/client/src/utils/plannedPlaces.test.ts @@ -0,0 +1,103 @@ +import { describe, it, expect } from 'vitest' +import { plannedPlaceIds, plannedPlaceIdsForDay } from './plannedPlaces' +import type { AssignmentsMap, Day } from '../types' + +// FE-UTIL-PLANNED-001 to FE-UTIL-PLANNED-010 +// +// #2072 — a hotel linked to a booking sat under "Unplanned" while its name was +// printed in the day header, because being planned meant one thing only: a +// day_assignments row. + +const assignments = (map: Record): AssignmentsMap => + Object.fromEntries( + Object.entries(map).map(([dayId, ids]) => [dayId, ids.map((id, i) => ({ + id: 900 + id, day_id: Number(dayId), order_index: i, place: { id }, + }))]), + ) as unknown as AssignmentsMap + +const DAYS = [ + { id: 10, day_number: 1 }, { id: 11, day_number: 2 }, { id: 12, day_number: 3 }, { id: 13, day_number: 4 }, +] as unknown as Day[] + +describe('plannedPlaceIds', () => { + it('FE-UTIL-PLANNED-001: an assigned place is planned, as it always was', () => { + expect([...plannedPlaceIds({ assignments: assignments({ 10: [1, 2] }) })]).toEqual([1, 2]) + }) + + it('FE-UTIL-PLANNED-002: a stay plans its place without any assignment', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + }) + expect(ids.has(7)).toBe(true) + }) + + it('FE-UTIL-PLANNED-003: a stay whose place was deleted contributes nothing', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + accommodations: [{ place_id: null, start_day_id: 10, end_day_id: 12 }], + }) + expect(ids.size).toBe(0) + }) + + it('FE-UTIL-PLANNED-004: a booking on a day plans the place it points at', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + reservations: [{ place_id: 5, day_id: 10 }], + }) + expect(ids.has(5)).toBe(true) + }) + + // The boundary the issue itself calls out: a booking that names no day is not + // on the plan, and the day view renders nothing for it either. + it('FE-UTIL-PLANNED-005: a booking with no day plans nothing', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + reservations: [{ place_id: 5, day_id: null }], + }) + expect(ids.size).toBe(0) + }) + + it('FE-UTIL-PLANNED-006: a place reachable two ways is counted once', () => { + const ids = plannedPlaceIds({ + assignments: assignments({ 10: [7] }), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + reservations: [{ place_id: 7, day_id: 10 }], + }) + expect([...ids]).toEqual([7]) + }) + + it('FE-UTIL-PLANNED-007: no sources at all is an empty set, not a throw', () => { + expect(plannedPlaceIds({ assignments: {} as AssignmentsMap }).size).toBe(0) + }) +}) + +describe('plannedPlaceIdsForDay', () => { + it('FE-UTIL-PLANNED-008: a stay covers the nights in between, not just its two ends', () => { + const sources = { + assignments: assignments({}), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + } + expect(plannedPlaceIdsForDay(10, DAYS, sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(11, DAYS, sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(12, DAYS, sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(13, DAYS, sources).has(7)).toBe(false) + }) + + // The day rows are the only thing that can order a stay. Without them the + // planner falls back to the id range rather than dropping the middle nights. + it('FE-UTIL-PLANNED-009: an unloaded day list still keeps the middle of a stay', () => { + const sources = { + assignments: assignments({}), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + } + expect(plannedPlaceIdsForDay(11, [], sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(13, [], sources).has(7)).toBe(false) + }) + + it('FE-UTIL-PLANNED-010: a booking counts only on the day it names', () => { + const sources = { assignments: assignments({}), reservations: [{ place_id: 5, day_id: 11 }] } + expect(plannedPlaceIdsForDay(11, DAYS, sources).has(5)).toBe(true) + expect(plannedPlaceIdsForDay(10, DAYS, sources).has(5)).toBe(false) + }) +}) diff --git a/client/src/utils/plannedPlaces.ts b/client/src/utils/plannedPlaces.ts new file mode 100644 index 000000000..30dfe9344 --- /dev/null +++ b/client/src/utils/plannedPlaces.ts @@ -0,0 +1,88 @@ +import { isDayInAccommodationRange } from './dayOrder' +import type { AssignmentsMap, Day } from '../types' + +/** + * Which of a trip's places are "planned", i.e. have a place in the itinerary. + * + * Dragging a place onto a day is the obvious way, and for a long time it was the + * only one this answered — so a hotel linked to a booking sat under "Unplanned" + * while its name was printed in the day header two panels away (#2072). A stay is + * on days by construction (day_accommodations.start_day_id / end_day_id are both + * NOT NULL), and a booking that names a day is on the plan as much as any dragged + * pin, so both count. + * + * A booking with no day does not: it exists, but nothing says when, which is + * exactly what "unplanned" means. + * + * The three surfaces that ask this — the desktop sidebar, the map's marker set and + * the phone browser — have to agree, or the list and the pins disagree about the + * same place (#1541). Hence one function rather than three predicates. + */ + +/** A stay, as the planner and the reservation rows carry it. */ +export interface PlannedAccommodation { + place_id?: number | null + start_day_id?: number | null + end_day_id?: number | null +} + +/** A booking, reduced to what decides whether it plans its place. */ +export interface PlannedReservation { + place_id?: number | null + day_id?: number | null +} + +export interface PlannedSources { + assignments: AssignmentsMap + accommodations?: PlannedAccommodation[] + reservations?: PlannedReservation[] +} + +function addAssignmentPlaces(ids: Set, dayAssignments: { place?: { id?: number | null } | null }[]): void { + for (const a of dayAssignments) { + if (a.place?.id != null) ids.add(a.place.id) + } +} + +/** Every place the trip has a plan for, across all days. */ +export function plannedPlaceIds({ assignments, accommodations = [], reservations = [] }: PlannedSources): Set { + const ids = new Set() + for (const dayAssignments of Object.values(assignments || {})) addAssignmentPlaces(ids, dayAssignments) + for (const acc of accommodations) { + // A deleted place leaves place_id NULL behind (ON DELETE SET NULL). + if (acc.place_id != null) ids.add(acc.place_id) + } + for (const r of reservations) { + // day_id only, matching what the day plan itself renders: a booking carrying + // just an end day appears on no day at all. + if (r.place_id != null && r.day_id != null) ids.add(r.place_id) + } + return ids +} + +/** The same question for one day — what the map answers while a day is selected. */ +export function plannedPlaceIdsForDay( + dayId: number, + days: Day[], + { assignments, accommodations = [], reservations = [] }: PlannedSources, +): Set { + const ids = new Set() + addAssignmentPlaces(ids, assignments?.[String(dayId)] || []) + const day = (days || []).find(d => d.id === dayId) + for (const acc of accommodations) { + if (acc.place_id == null || acc.start_day_id == null) continue + const end = acc.end_day_id ?? acc.start_day_id + // A stay covers the nights in between, so its place is planned on each of + // them, not only on the two days it is pinned to. The day rows are the only + // thing that can order them; without them, fall back to the id range the + // rest of the planner uses. + const inRange = day + ? isDayInAccommodationRange(day, acc.start_day_id, end, days) + : dayId >= Math.min(acc.start_day_id, end) && dayId <= Math.max(acc.start_day_id, end) + if (inRange) ids.add(acc.place_id) + } + for (const r of reservations) { + if (r.place_id != null && r.day_id === dayId) ids.add(r.place_id) + } + return ids +} diff --git a/client/tests/integration/hooks/useRouteCalculation.test.ts b/client/tests/integration/hooks/useRouteCalculation.test.ts index cd2c6f8b3..d8d7c7b0c 100644 --- a/client/tests/integration/hooks/useRouteCalculation.test.ts +++ b/client/tests/integration/hooks/useRouteCalculation.test.ts @@ -339,6 +339,94 @@ describe('useRouteCalculation', () => { expect(legs).toContainEqual([`${actB.lat},${actB.lng}`, `${hotel.lat},${hotel.lng}`]); }); + // #2071 — Frankfurt → Vancouver via Reykjavik, an airport place either side. The + // booking's position lives per LEG in metadata.legs, which this hook never read, so + // the flight vanished from the waypoint list and the two airports were joined into + // one driving run across the Atlantic. + it('FE-HOOK-ROUTE-024: a layover flight breaks the run instead of joining the airports', async () => { + const fra = { lat: 50.0379, lng: 8.5622 }; + const kef = { lat: 63.985, lng: -22.6056 }; + const yvr = { lat: 49.1947, lng: -123.1792 }; + const fraPlace = buildPlace({ lat: fra.lat, lng: fra.lng }); + const yvrPlace = buildPlace({ lat: yvr.lat, lng: yvr.lng }); + + const flight = { + id: 300, type: 'flight', day_id: 1, end_day_id: 1, + endpoints: [ + { role: 'from', sequence: 0, lat: fra.lat, lng: fra.lng }, + { role: 'stop', sequence: 1, lat: kef.lat, lng: kef.lng }, + { role: 'to', sequence: 2, lat: yvr.lat, lng: yvr.lng }, + ], + metadata: JSON.stringify({ + legs: [ + { from: 'FRA', to: 'KEF', dep_time: '10:00', arr_time: '12:00', day_positions: { 1: 0.5 } }, + { from: 'KEF', to: 'YVR', dep_time: '14:00', arr_time: '16:00', day_positions: { 1: 0.6 } }, + ], + }), + }; + const before = buildAssignment({ day_id: 1, order_index: 0, place: fraPlace }); + const after = buildAssignment({ day_id: 1, order_index: 1, place: yvrPlace }); + const store = { assignments: { '1': [before, after] } } as unknown as TripStoreState; + useTripStore.setState({ + assignments: store.assignments, + reservations: [flight], + days: [{ id: 1, day_number: 1 }], + } as any); + + const { result } = renderHook(() => useRouteCalculation(store, 1, true, 'driving')); + await act(async () => {}); + + const legs = (result.current.route ?? []).map(run => run.map(p => `${p[0]},${p[1]}`)); + // The transatlantic pair is the bug: it must not appear in any run. + for (const run of legs) { + const i = run.indexOf(`${fra.lat},${fra.lng}`); + if (i >= 0) expect(run[i + 1]).not.toBe(`${yvr.lat},${yvr.lng}`); + } + expect(legs).not.toContainEqual([`${fra.lat},${fra.lng}`, `${yvr.lat},${yvr.lng}`]); + }); + + // Each leg spans its OWN pair of airports. Reading the booking's outermost two for + // every leg put a Reykjavik stop on a Vancouver → Frankfurt drive. + it('FE-HOOK-ROUTE-025: a stop between two legs routes against that leg\'s airports', async () => { + const fra = { lat: 50.0379, lng: 8.5622 }; + const kef = { lat: 63.985, lng: -22.6056 }; + const yvr = { lat: 49.1947, lng: -123.1792 }; + const cafe = buildPlace({ lat: 64.1466, lng: -21.9426 }); + + const flight = { + id: 301, type: 'flight', day_id: 1, end_day_id: 1, + endpoints: [ + { role: 'from', sequence: 0, lat: fra.lat, lng: fra.lng }, + { role: 'stop', sequence: 1, lat: kef.lat, lng: kef.lng }, + { role: 'to', sequence: 2, lat: yvr.lat, lng: yvr.lng }, + ], + metadata: JSON.stringify({ + legs: [ + { from: 'FRA', to: 'KEF', dep_time: '08:00', arr_time: '10:00', day_positions: { 1: 0.5 } }, + { from: 'KEF', to: 'YVR', dep_time: '16:00', arr_time: '18:00', day_positions: { 1: 1.5 } }, + ], + }), + }; + const stop = buildAssignment({ day_id: 1, order_index: 1, place: cafe }); + const store = { assignments: { '1': [stop] } } as unknown as TripStoreState; + useTripStore.setState({ + assignments: store.assignments, + reservations: [flight], + days: [{ id: 1, day_number: 1 }], + } as any); + + const { result } = renderHook(() => useRouteCalculation(store, 1, true, 'driving')); + await act(async () => {}); + + const legs = (result.current.route ?? []).map(run => run.map(p => `${p[0]},${p[1]}`)); + // The café is reached from Reykjavik and left towards Reykjavik. + expect(legs).toContainEqual([`${kef.lat},${kef.lng}`, `${cafe.lat},${cafe.lng}`, `${kef.lat},${kef.lng}`]); + // Neither of the far airports may appear next to it. + const flat = legs.flat(); + expect(flat).not.toContain(`${fra.lat},${fra.lng}`); + expect(flat).not.toContain(`${yvr.lat},${yvr.lng}`); + }); + it('FE-HOOK-ROUTE-015: day-1 with a first activity timed after check-in keeps the hotel → first-activity leg', async () => { // The check-in day is still a home-base loop when the first activity provably happens // at/after check-in (you dropped your bags first) — the hotel → first-stop leg remains.