diff --git a/client/src/components/PDF/TripPDF.test.ts b/client/src/components/PDF/TripPDF.test.ts index 1b7ac9523..6d9b990ad 100644 --- a/client/src/components/PDF/TripPDF.test.ts +++ b/client/src/components/PDF/TripPDF.test.ts @@ -4,6 +4,7 @@ import { http, HttpResponse } from 'msw' import { downloadTripPDF } from './TripPDF' import { server } from '../../../tests/helpers/msw/server' import { clearExchangeRateCache } from '../../hooks/useExchangeRates' +import { getMergedItems, getTransportForDay } from '../../utils/dayMerge' // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -1096,3 +1097,114 @@ describe('page breaks between days (#1292)', () => { expect(html.slice(at - 10, at)).toContain('.pdf-flow ') }) }) + +/** + * The order a day prints in (#1978). + * + * The export used to order a day by itself, on `day_plan_position` — one global + * number per booking, auto-seeded from whichever day happened to render first. + * On any other day of a span it therefore pointed at the wrong slot, and where + * it was still null the export fell back to the order the API returned rows in. + * That is why the same trip printed correctly some of the time and swapped two + * bookings the rest of it, and why adding a place and removing it again "fixed" + * it: the reseed happened to land on the day being looked at. + * + * It now goes through utils/dayMerge, the same functions the day plan uses, so + * the two cannot disagree. These cases are written from the plan's rules rather + * than from the old implementation's. + */ +describe('a printed day follows the plan (#1978)', () => { + const d1 = { id: 21, day_number: 1, title: 'Day One', date: '2025-06-01' } as any + const d2 = { id: 22, day_number: 2, title: 'Day Two', date: '2025-06-02' } as any + const srcdoc = () => getIframe()!.srcdoc + + /** Where each needle first appears, so order can be asserted without parsing. */ + const positions = (html: string, needles: string[]) => needles.map(n => html.indexOf(n)) + + it('orders the day by the clock, not by the order the API sent rows in', async () => { + await downloadTripPDF({ + ...minimalArgs, + trip: { id: 21, title: 'Order Trip', description: null, cover_image: null } as any, + days: [d1], + // As the API returns them: reservation_time ASC with nulls first. + reservations: [ + { id: 1, title: 'Untimed Transfer', type: 'transfer', day_id: 21, reservation_time: null }, + { id: 2, title: 'Evening Train', type: 'train', day_id: 21, reservation_time: '2025-06-01T19:30' }, + { id: 3, title: 'Morning Flight', type: 'flight', day_id: 21, reservation_time: '2025-06-01T07:15' }, + ], + }) + const [morning, evening] = positions(srcdoc(), ['Morning Flight', 'Evening Train']) + expect(morning).toBeGreaterThan(-1) + expect(evening).toBeGreaterThan(-1) + expect(morning).toBeLessThan(evening) + }) + + /* + * The reported shape: a booking on the last day of a span whose global + * position was seeded from a busier earlier day, which sank it below + * everything on the day it actually belongs to. + */ + it('ignores a global position seeded from another day', async () => { + await downloadTripPDF({ + ...minimalArgs, + trip: { id: 22, title: 'Span Trip', description: null, cover_image: null } as any, + days: [d1, d2], + reservations: [ + { + id: 4, title: 'Car Return', type: 'car', day_id: 21, end_day_id: 22, + reservation_time: '2025-06-01T10:00', reservation_end_time: '2025-06-02T09:00', + // Seeded from day one, where five places had already been placed. + day_plan_position: 5, + }, + { id: 5, title: 'Late Flight', type: 'flight', day_id: 22, reservation_time: '2025-06-02T18:00' }, + ], + }) + const html = srcdoc() + // On day two the 09:00 return comes before the 18:00 flight, whatever the + // stale global position says. + const [ret, flight] = positions(html, ['Car Return', 'Late Flight']) + expect(ret).toBeGreaterThan(-1) + expect(flight).toBeGreaterThan(-1) + expect(ret).toBeLessThan(flight) + }) + + /* + * The strongest form of the same claim: whatever the plan would show, the + * print shows, in that order. Asserted against getMergedItems itself rather + * than against a hand-written expectation, so the two cannot drift apart + * again without this failing. + */ + it('prints a mixed day in exactly the order the plan merges it', async () => { + const reservations = [ + { id: 8, title: 'Untimed Transfer', type: 'transfer', day_id: 21, reservation_time: null }, + { id: 9, title: 'Evening Train', type: 'train', day_id: 21, reservation_time: '2025-06-01T19:30' }, + { id: 10, title: 'Morning Flight', type: 'flight', day_id: 21, reservation_time: '2025-06-01T07:15' }, + ] + const dayAssignments = [ + { id: 100, day_id: 21, order_index: 0, place: { id: 1, name: 'Museum', place_time: '11:00' } }, + { id: 101, day_id: 21, order_index: 1, place: { id: 2, name: 'Market', place_time: '16:00' } }, + ] + + await downloadTripPDF({ + ...minimalArgs, + trip: { id: 24, title: 'Mixed Trip', description: null, cover_image: null } as any, + days: [d1], + assignments: { '21': dayAssignments } as any, + reservations, + }) + const html = srcdoc() + + const expected = getMergedItems({ + dayAssignments, + dayNotes: [], + dayTransports: getTransportForDay({ + reservations, dayId: 21, dayAssignmentIds: dayAssignments.map(a => a.id), days: [d1], + }), + dayId: 21, + }).map(item => (item.type === 'place' ? item.data.place.name : item.data.title)) + + const seen = expected.map(name => html.indexOf(name)) + for (const at of seen) expect(at).toBeGreaterThan(-1) + expect(seen).toEqual([...seen].sort((a, b) => a - b)) + }) +}) diff --git a/client/src/components/PDF/TripPDF.tsx b/client/src/components/PDF/TripPDF.tsx index f5d4745b3..179c96461 100644 --- a/client/src/components/PDF/TripPDF.tsx +++ b/client/src/components/PDF/TripPDF.tsx @@ -5,7 +5,7 @@ import { FileText, Info, Clock, MapPin, Navigation, Train, Plane, Bus, Car, Ship import { accommodationsApi, mapsApi, pluginsApi } from '../../api/client' import type { Trip, Day, Place, Category, AssignmentsMap, DayNote } from '../../types' import { isDayInAccommodationRange, getDayOrder } from '../../utils/dayOrder' -import { hidesOnMiddleDay } from '../../utils/dayMerge' +import { hidesOnMiddleDay, getTransportForDay, getMergedItems, getSpanPhase, getDisplayTimeForDay } from '../../utils/dayMerge' import { safeHexColor } from '../../utils/safeColor' import { renderIconMarkup } from '../../utils/iconMarkup' import { formatMoney, formatMoneySum, splitReservationDateTime, type MoneyEntry } from '../../utils/formatters' @@ -209,22 +209,19 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca const fxRates = needsFx ? await fetchExchangeRates(tripCur) : null const totalCostLabel = formatMoneySum(allCostEntries, tripCur, loc || 'en', fxRates) - // Span helpers for multi-day transport (mirrors DayPlanSidebar logic) - const pdfGetDayOrder = (d: Day) => d.day_number - const pdfGetSpanPhase = (r: any, dayId: number): 'single' | 'start' | 'middle' | 'end' => { - const startId = r.day_id - const endId = r.end_day_id ?? startId - if (!startId || startId === endId) return 'single' - if (dayId === startId) return 'start' - if (dayId === endId) return 'end' - return 'middle' - } - const pdfGetDisplayTime = (r: any, dayId: number): string | null => { - const phase = pdfGetSpanPhase(r, dayId) - if (phase === 'end') return r.reservation_end_time || null - if (phase === 'middle') return null - return r.reservation_time || null - } + /* + * The span label is the only PDF-specific piece left here. Everything else + * about how a day is ordered — which transports belong to it, what time each + * one shows, where it sits among the places — comes from utils/dayMerge, the + * same functions the day plan itself uses (#1978). + * + * There used to be a second implementation in this file. It ordered by + * `day_plan_position`, which is one global number per booking, seeded from + * whichever day happened to render first; on any other day of a span it + * pointed at the wrong slot, and when it was still null the export fell back + * to the order the API returned. That is why the PDF was right some of the + * time and swapped two bookings the rest of it. + */ const pdfGetSpanLabel = (r: any, phase: string): string | null => { if (phase === 'single') return null if (r.type === 'flight') return tr(`reservations.span.${phase === 'start' ? 'departure' : phase === 'end' ? 'arrival' : 'inTransit'}`) @@ -234,47 +231,43 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca if (r.type === 'parking') return tr(`reservations.span.${phase === 'start' ? 'dropOff' : phase === 'end' ? 'pickup' : 'ongoing'}`) return tr(`reservations.span.${phase === 'start' ? 'start' : phase === 'end' ? 'end' : 'ongoing'}`) } - const pdfGetTransportForDay = (dayId: number) => (reservations || []).filter(r => { - if (r.type === 'hotel') return false - const startId = r.day_id - const endId = r.end_day_id ?? startId - if (startId == null) return false - if (endId !== startId) { - const startDay = sorted.find(d => d.id === startId) - const endDay = sorted.find(d => d.id === endId) - const thisDay = sorted.find(d => d.id === dayId) - if (!startDay || !endDay || !thisDay) return false - return pdfGetDayOrder(thisDay) >= pdfGetDayOrder(startDay) && pdfGetDayOrder(thisDay) <= pdfGetDayOrder(endDay) - } - return startId === dayId - }) - // Build day HTML const daysHtml = sorted.map((day, di) => { - const assigned = assignments[String(day.id)] || [] - const notes = (dayNotes || []).filter(n => n.day_id === day.id) + const assigned = (assignments[String(day.id)] || []).slice() + .sort((a: any, b: any) => (a.order_index ?? 0) - (b.order_index ?? 0)) + const notes = (dayNotes || []).filter(n => n.day_id === day.id).slice() + .sort((a: any, b: any) => (a.sort_order ?? 0) - (b.sort_order ?? 0)) const cost = dayCost(assignments, day.id, loc, tripCur, fxRates) - // Reservations for this day (hotel rendered via accommodations block; car middle-phase rendered in sidebar header only, - // a multi-day parking's middle days nowhere at all) - const dayReservations = pdfGetTransportForDay(day.id) - .filter(r => !(r.type === 'car' && pdfGetSpanPhase(r, day.id) === 'middle')) - .filter(r => !hidesOnMiddleDay(r, day.id)) - - const merged = [] - assigned.forEach(a => merged.push({ type: 'place', k: a.order_index ?? 0, data: a })) - notes.forEach(n => merged.push({ type: 'note', k: n.sort_order ?? 0, data: n })) - dayReservations.forEach(r => { - const pos = r.day_positions?.[day.id] ?? r.day_positions?.[String(day.id)] ?? r.day_plan_position ?? (merged.length > 0 ? Math.max(...merged.map(m => m.k)) + 0.5 : 0.5) - merged.push({ type: 'reservation', k: pos, data: r }) + // Assembled exactly the way DayPlanSidebar assembles it, so the page and the + // print cannot disagree about what order a day is in. + const dayTransports = getTransportForDay({ + reservations: reservations || [], + dayId: day.id, + dayAssignmentIds: assigned.map((a: any) => a.id), + days: sorted, + }) + + // Hotels come out in the accommodations block, a rental car's middle days + // appear only in the sidebar header, and a multi-day parking's middle days + // nowhere at all. + const merged = getMergedItems({ + dayAssignments: assigned, + dayNotes: notes, + dayTransports, + dayId: day.id, + }).filter(item => { + if (item.type !== 'transport') return true + const r: any = item.data + if (r.type === 'car' && getSpanPhase(r, day.id) === 'middle') return false + return !hidesOnMiddleDay(r, day.id) }) - merged.sort((a, b) => a.k - b.k) let pi = 0 const itemsHtml = merged.length === 0 ? `
${escHtml(tr('dayplan.emptyDay'))}
` : merged.map(item => { - if (item.type === 'reservation') { + if (item.type === 'transport') { const r = item.data const meta = typeof r.metadata === 'string' ? JSON.parse(r.metadata || '{}') : (r.metadata || {}) const icon = reservationIconSvg(r.type) @@ -282,7 +275,30 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca let subtitle = '' // Flights render one subtitle line per leg (see below); everything else is a single line. let subtitleLines: string[] = [] - if (r.type === 'flight') { + /* + * A multi-leg booking arrives as one item per leg now, the way the + * day plan shows it, because the shared merge expands legs so each + * one can sit at its own time among the day's places. Previously + * the export printed a single row with every leg as a subtitle + * line, which put the second leg's departure next to the first + * leg's clock. + */ + if (r.__leg) { + const l = r.__leg + // The segment's own booking code, which the leg object does not + // carry — read back from the booking by index. At the gate that is + // the code the airline asks for (#1943), so losing it here would + // have been a real regression for a stopover flight. + const source = (r.type === 'train' ? getTrainLegs(r) : getFlightLegs(r))[l.index] + subtitleLines = [[ + l.airline, l.flight_number, l.train_number, + l.platform ? `Gl. ${l.platform}` : '', + l.seat ? `Seat ${l.seat}` : '', + (l.from || l.to) ? [l.from, l.to].filter(Boolean).join(' → ') : '', + source?.confirmation_number, + ].filter(Boolean).join(' · ')].filter(Boolean) + } + else if (r.type === 'flight') { const legs = getFlightLegs(r) if (legs.length > 1) { // Multi-leg: one line per leg so every flight number + segment route is @@ -324,9 +340,9 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca else if (r.type === 'tour') subtitle = [meta.operator].filter(Boolean).join(' · ') if (subtitleLines.length === 0 && subtitle) subtitleLines = [subtitle] const locationLine = r.location || meta.location || '' - const phase = pdfGetSpanPhase(r, day.id) + const phase = getSpanPhase(r, day.id) const spanLabel = pdfGetSpanLabel(r, phase) - const displayTime = pdfGetDisplayTime(r, day.id) + const displayTime = getDisplayTimeForDay(r, day.id) // Start and end, the way the day plan draws it (#1310). Without the // landing time a flight reads as an open-ended block and the reader // cannot tell how much of the day is left for anything else.