diff --git a/client/src/components/Journey/JourneyDetailPageEntryEditor.test.tsx b/client/src/components/Journey/JourneyDetailPageEntryEditor.test.tsx index ad10e65fc..b560134e1 100644 --- a/client/src/components/Journey/JourneyDetailPageEntryEditor.test.tsx +++ b/client/src/components/Journey/JourneyDetailPageEntryEditor.test.tsx @@ -5,6 +5,9 @@ import { http, HttpResponse, delay } from 'msw' import { localIsoDate } from '../../utils/localDate' import userEvent from '@testing-library/user-event' import { render, screen, waitFor, fireEvent } from '../../../tests/helpers/render' +import { seedStore } from '../../../tests/helpers/store' +import { buildSettings } from '../../../tests/helpers/factories' +import { useSettingsStore } from '../../store/settingsStore' import { server } from '../../../tests/helpers/msw/server' import type { GalleryPhoto, JourneyEntry, JourneyPhoto, JourneyTrip } from '../../store/journeyStore' import type { ResilientResult, UploadProgress } from '../../utils/uploadQueue' @@ -92,6 +95,9 @@ const originalCreateObjectURL = URL.createObjectURL beforeEach(() => { toastSpy.mockClear() + // The time field reads time_format, so pin it: without this the cases that + // assert a 24h string depend on whichever test ran before them (#2067). + seedStore(useSettingsStore, { settings: buildSettings({ time_format: '24h' }) }) window.__addToast = toastSpy Object.defineProperty(URL, 'createObjectURL', { configurable: true, writable: true, value: vi.fn(() => 'blob:preview'), @@ -865,9 +871,8 @@ describe('EntryEditor', () => { const user = userEvent.setup() const { onSave } = mountEditor(buildEntry({ id: 10, title: 'Old', entry_time: '14:30:00' })) - // The column carries HH:MM:SS; a time input only accepts HH:MM. + // The column carries HH:MM:SS; the picker is seeded from a HH:MM slice. const timeInput = screen.getByDisplayValue('14:30') - expect(timeInput).toHaveAttribute('type', 'time') fireEvent.change(timeInput, { target: { value: '09:05' } }) await user.click(screen.getByRole('button', { name: 'Save' })) @@ -876,6 +881,40 @@ describe('EntryEditor', () => { expect(onSave.mock.calls[0][0]).toEqual(expect.objectContaining({ entry_time: '09:05' })) }) + // #2067 — the field used to be a native , which paints 12h or + // 24h from the browser locale and cannot be told otherwise. These pin that the + // user's setting decides what is shown, and that storage stays 24h either way. + it('FE-JRN-EDITOR-048: a 24h user sees a 24h clock with no meridiem', () => { + seedStore(useSettingsStore, { settings: buildSettings({ time_format: '24h' }) }) + const { container } = mountEditor(buildEntry({ id: 10, title: 'Old', entry_time: '14:30:00' })) + + expect(screen.getByDisplayValue('14:30')).toBeInTheDocument() + expect(container.textContent).not.toMatch(/\bPM\b/) + }) + + it('FE-JRN-EDITOR-049: a 12h user sees the same stored time as a meridiem clock', () => { + seedStore(useSettingsStore, { settings: buildSettings({ time_format: '12h' }) }) + mountEditor(buildEntry({ id: 10, title: 'Old', entry_time: '14:30:00' })) + + expect(screen.getByDisplayValue('2:30 PM')).toBeInTheDocument() + expect(screen.queryByDisplayValue('14:30')).not.toBeInTheDocument() + }) + + it('FE-JRN-EDITOR-050: a meridiem typed by a 12h user is still stored as 24h', async () => { + const user = userEvent.setup() + seedStore(useSettingsStore, { settings: buildSettings({ time_format: '12h' }) }) + const { onSave } = mountEditor(buildEntry({ id: 10, title: 'Old', entry_time: '14:30:00' })) + + const field = screen.getByDisplayValue('2:30 PM') + fireEvent.focus(field) + fireEvent.change(field, { target: { value: '5:30 pm' } }) + fireEvent.blur(field) + await user.click(screen.getByRole('button', { name: 'Save' })) + + await waitFor(() => expect(onSave).toHaveBeenCalled()) + expect(onSave.mock.calls[0][0]).toEqual(expect.objectContaining({ entry_time: '17:30' })) + }) + it('FE-JRN-EDITOR-042: clearing the time sends null rather than an empty string', async () => { const user = userEvent.setup() const { onSave } = mountEditor(buildEntry({ id: 10, entry_time: '14:30:00' })) diff --git a/client/src/components/Journey/JourneyDetailPageEntryEditor.tsx b/client/src/components/Journey/JourneyDetailPageEntryEditor.tsx index 2f6c33237..d504a30bc 100644 --- a/client/src/components/Journey/JourneyDetailPageEntryEditor.tsx +++ b/client/src/components/Journey/JourneyDetailPageEntryEditor.tsx @@ -13,6 +13,7 @@ import { MOOD_CONFIG, WEATHER_CONFIG } from '../../pages/journeyDetail/JourneyDe import { photoUrl, isValidGeoPoint, geoOnceErrorKey } from '../../pages/journeyDetail/JourneyDetailPage.helpers' import MarkdownToolbar from './MarkdownToolbar' import { DatePicker } from './JourneyDetailPageDatePicker' +import CustomTimePicker from '../shared/CustomTimePicker' import { ProviderPicker, type ProviderPhotoGroup } from './JourneyDetailPageProviderPicker' type PendingProviderGroup = ProviderPhotoGroup & { provider: string } @@ -601,19 +602,20 @@ export function EntryEditor({ entry, journeyId, tripDates, galleryPhotos, trips, {/* Time sat in state and went to the server, it just had no input here — so a draft's auto-stamped clock time showed up in the timeline, the map, the PDF and the public share, and the desktop had no way to correct it (#1614). */} -
+ {/* 136px, not 104: the custom picker adds a clock button and, in 12h, + shows "2:30 PM" where the native input showed a fixed-width HH:MM. */} +
- setEntryTime(e.target.value)} - className="w-full h-[42px] px-3 rounded-xl border border-zinc-200 dark:border-zinc-700 bg-transparent text-[13px] text-zinc-900 dark:text-white outline-none focus:border-zinc-400 dark:focus:border-zinc-500 [font-variant-numeric:tabular-nums]" - /> + {/* A native paints 12h or 24h from the browser + locale, and no attribute overrides it — so it ignored the user's + setting outright (#2067). The rest of TREK has used this picker + for exactly that reason; the journey editor was never migrated. */} +
diff --git a/client/src/components/PDF/TripPDF.test.ts b/client/src/components/PDF/TripPDF.test.ts index 58e5133a1..35e9acba0 100644 --- a/client/src/components/PDF/TripPDF.test.ts +++ b/client/src/components/PDF/TripPDF.test.ts @@ -578,6 +578,73 @@ describe('downloadTripPDF remaining branches', () => { const srcdoc = () => getIframe()!.srcdoc + // #2066 — the document is assembled outside React, so it read no setting at all + // and printed the stored column. Four surfaces carried a clock; all four were 24h + // whatever the reader had chosen. + describe('time format (#2066)', () => { + // The place chip reads the assignment's embedded place, not the places array, + // so both have to carry the clock under test. + const at = (placeTime: string, over: Record = {}) => { + const place = { ...placeWithDetails, place_time: placeTime } + return { + ...richArgs, + places: [place], + assignments: { '10': [{ ...assignmentForDay, place }] }, + reservations: [{ + id: 700, title: 'Ferry', type: 'ferry', day_id: 10, + reservation_time: '2025-06-01T09:05', reservation_end_time: '2025-06-01T16:45', + }], + ...over, + } + } + + it('FE-W5PDF-031: a 12h reader gets meridiem clocks on places and transports', async () => { + await downloadTripPDF(at('14:30', { timeFormat: '12h' }) as never) + const html = srcdoc() + + expect(html).toContain('2:30 PM') + expect(html).toContain('9:05 AM') + expect(html).toContain('4:45 PM') + expect(html).not.toContain('14:30') + expect(html).not.toContain('16:45') + }) + + it('FE-W5PDF-032: a 24h reader gets the same times without a meridiem', async () => { + await downloadTripPDF(at('14:30', { timeFormat: '24h' }) as never) + const html = srcdoc() + + expect(html).toContain('14:30') + expect(html).toContain('09:05') + expect(html).toContain('16:45') + expect(html).not.toContain('2:30 PM') + }) + + // check_in / check_out come off the accommodation row and were the one pair + // that printed the raw column in BOTH directions. + it('FE-W5PDF-033: accommodation check-in and check-out follow the setting too', async () => { + server.use(http.get('/api/trips/:id/accommodations', () => HttpResponse.json({ + accommodations: [{ + id: 1, place_id: 1, place_name: 'Hotel Roma', place_address: 'Via Roma 1', + start_day_id: 10, end_day_id: 10, check_in: '15:00', check_out: '11:00', notes: null, + }], + }))) + + await downloadTripPDF(at('14:30', { timeFormat: '12h' }) as never) + + expect(srcdoc()).toContain('3:00 PM') + }) + + // A clock stored with a meridiem — the booking importer and a 12h user typing + // into the place form both produce one — must not print as 3 AM for a 24h reader. + it('FE-W5PDF-034: a stored meridiem is converted, not printed raw', async () => { + await downloadTripPDF(at('3:00 PM', { timeFormat: '24h' }) as never) + + const html = srcdoc() + expect(html).toContain('15:00') + expect(html).not.toContain('3:00 PM') + }) + }) + it('FE-W5PDF-001: a multi-day cruise is labelled start / ongoing / end with the right times', async () => { await downloadTripPDF(spanArgs([{ id: 500, title: 'Nordic Cruise', type: 'cruise', day_id: 10, end_day_id: 12, diff --git a/client/src/components/PDF/TripPDF.tsx b/client/src/components/PDF/TripPDF.tsx index 8a7c3c789..d15de335f 100644 --- a/client/src/components/PDF/TripPDF.tsx +++ b/client/src/components/PDF/TripPDF.tsx @@ -8,7 +8,8 @@ import { isDayInAccommodationRange, getDayOrder } from '../../utils/dayOrder' 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' +import { formatMoney, formatMoneySum, formatClockTime, splitReservationDateTime, type MoneyEntry } from '../../utils/formatters' +import { useSettingsStore } from '../../store/settingsStore' import { fetchExchangeRates } from '../../hooks/useExchangeRates' import { getFlightLegs, getTrainLegs } from '../../utils/flightLegs' @@ -178,14 +179,24 @@ interface downloadTripPDFProps { reservations?: any[] t: (key: string, params?: Record) => string locale: string + /** + * '12h' | '24h'. The document is a plain HTML string assembled outside React, + * so the setting cannot be read with a hook in here — it comes over the same + * way `locale` does (#2066). + */ + timeFormat?: string } // `assignments` is normalised here once — every read below (and fetchPlacePhotos) // relies on it being an object. -export async function downloadTripPDF({ trip, days, places, assignments = {}, categories, dayNotes, reservations = [], t: _t, locale: _locale }: downloadTripPDFProps) { +export async function downloadTripPDF({ trip, days, places, assignments = {}, categories, dayNotes, reservations = [], t: _t, locale: _locale, timeFormat: _timeFormat }: downloadTripPDFProps) { const breaksPerDay = pageBreakPerDay() const loc = _locale || undefined const tr = _t || (k => k) + // The store read is the fallback, not the source: a caller that forgets the + // prop still prints the reader's own format instead of silently reverting. + const is12h = (_timeFormat || useSettingsStore.getState().settings.time_format || '24h') === '12h' + const fmtTime = (v?: string | null) => formatClockTime(v, is12h) const sorted = [...(days || [])].sort((a, b) => a.day_number - b.day_number) const range = longDateRange(sorted, loc) const coverImg = safeImg(trip?.cover_image) @@ -358,8 +369,8 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca // span the arrival belongs to the arrival day, which already shows // it as its own time, and repeating it here would put tomorrow's // clock next to today's departure. - const startTime = splitReservationDateTime(displayTime).time ?? '' - const endTime = phase === 'single' ? (splitReservationDateTime(r.reservation_end_time).time ?? '') : '' + const startTime = fmtTime(splitReservationDateTime(displayTime).time) + const endTime = phase === 'single' ? fmtTime(splitReservationDateTime(r.reservation_end_time).time) : '' const time = [startTime, endTime].filter(Boolean).join(' – ') const titleHtml = `${spanLabel ? escHtml(spanLabel) + ': ' : ''}${escHtml(r.title)}` return ` @@ -412,7 +423,7 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca
` const chips = [ - place.place_time ? `${svgClock}${escHtml(place.place_time)}` : '', + place.place_time ? `${svgClock}${escHtml(fmtTime(place.place_time))}` : '', place.price && Number.parseFloat(place.price) > 0 ? `${svgMoney}${formatMoney(Number(place.price), place.currency || trip.currency, loc)}` : '', ].filter(Boolean).join('') @@ -452,8 +463,8 @@ export async function downloadTripPDF({ trip, days, places, assignments = {}, ca const actionIcon = isCheckIn ? accommodationIconSvg('checkin') : isCheckOut ? accommodationIconSvg('checkout') : accommodationIconSvg('accommodation') - const timeStr = isCheckIn ? (item.check_in || '') - : isCheckOut ? (item.check_out || '') + const timeStr = isCheckIn ? fmtTime(item.check_in) + : isCheckOut ? fmtTime(item.check_out) : '' return ` diff --git a/client/src/components/Planner/TripExportModal.tsx b/client/src/components/Planner/TripExportModal.tsx index 365032472..894aeaf59 100644 --- a/client/src/components/Planner/TripExportModal.tsx +++ b/client/src/components/Planner/TripExportModal.tsx @@ -5,6 +5,7 @@ import Modal from '../shared/Modal' import { IcsSubscribeModal } from './IcsSubscribeModal' import { useToast } from '../shared/Toast' import type { Trip, Day, Place, Category, AssignmentsMap, Reservation, DayNote } from '../../types' +import { useSettingsStore } from '../../store/settingsStore' /** * What a GPX download can carry. Worded by what someone wants on their device @@ -57,6 +58,8 @@ export function TripExportModal({ // Which row is working, so the dialog can say so instead of looking inert // while a 226 kB PDF builder is fetched and a document is rendered. const [busy, setBusy] = useState(null) + // The PDF is built outside React, so it cannot read this itself (#2066). + const timeFormat = useSettingsStore(s => s.settings.time_format) || '24h' const fileBase = trip?.title || 'trip' // Shared tail of every download: Firefox and Safari cancel the download when @@ -83,7 +86,7 @@ export function TripExportModal({ // exported. A missing chunk lands in the catch and shows the same error // the export already had. const { downloadTripPDF } = await import('../PDF/TripPDF') - await downloadTripPDF({ trip, days, places, assignments, categories, dayNotes: flatNotes, reservations, t, locale }) + await downloadTripPDF({ trip, days, places, assignments, categories, dayNotes: flatNotes, reservations, t, locale, timeFormat }) onClose() } catch (e) { console.error('PDF error:', e) diff --git a/client/src/components/shared/CustomTimePicker.tsx b/client/src/components/shared/CustomTimePicker.tsx index cebfbd58d..a84508158 100644 --- a/client/src/components/shared/CustomTimePicker.tsx +++ b/client/src/components/shared/CustomTimePicker.tsx @@ -10,9 +10,12 @@ interface CustomTimePickerProps { onChange: (value: string) => void placeholder?: string style?: React.CSSProperties + /** Read-only surfaces keep the field, they just cannot type in it — same + * affordance the native input they replaced offered (#2067). */ + disabled?: boolean } -export default function CustomTimePicker({ value, onChange, placeholder = '00:00', style = {} }: CustomTimePickerProps) { +export default function CustomTimePicker({ value, onChange, placeholder = '00:00', style = {}, disabled = false }: CustomTimePickerProps) { const is12h = useSettingsStore(s => s.settings.time_format) === '12h' const [open, setOpen] = useState(false) const [inputFocused, setInputFocused] = useState(false) @@ -132,6 +135,7 @@ export default function CustomTimePicker({ value, onChange, placeholder = '00:00 onFocus={() => setInputFocused(true)} onBlur={() => { setInputFocused(false); handleBlur() }} placeholder={is12h ? '2:30 PM' : placeholder} + disabled={disabled} style={{ flex: 1, border: 'none', outline: 'none', background: 'transparent', padding: '8px 10px 8px 14px', fontSize: 'calc(13px * var(--fs-scale-body, 1))', fontFamily: 'inherit', @@ -142,8 +146,9 @@ export default function CustomTimePicker({ value, onChange, placeholder = '00:00
{t('mobileJourney.time')}
-
- setEntryTime(e.target.value)} - className="block min-w-0 w-full box-border border-0 bg-transparent px-3 py-[10px] text-center text-[0.78125rem] font-semibold text-m-ink outline-none [font-variant-numeric:tabular-nums]" - /> -
+ {/* A native paints 12h or 24h from the browser + locale, whatever the user picked in settings (#2067). The picker + brings its own shell, so fieldShell goes with the input. */} +
diff --git a/client/src/mobile/screens/trip/sheets/MExportSheet.tsx b/client/src/mobile/screens/trip/sheets/MExportSheet.tsx index 6b7fe7563..dbc5444ce 100644 --- a/client/src/mobile/screens/trip/sheets/MExportSheet.tsx +++ b/client/src/mobile/screens/trip/sheets/MExportSheet.tsx @@ -3,6 +3,7 @@ import { CalendarPlus, ChevronRight, FileDown, Share2 } from 'lucide-react' import MSheet from '../../../components/MSheet' import { IcsSubscribeModal } from '../../../../components/Planner/IcsSubscribeModal' import { useTripStore } from '../../../../store/tripStore' +import { useSettingsStore } from '../../../../store/settingsStore' import { useTranslation } from '../../../../i18n' import { INNER_CLS, TileHeader } from './MTripSheetUi' import type { MTripSheetsProps } from '../MTripShell' @@ -16,6 +17,8 @@ import type { LucideIcon } from 'lucide-react' */ export default function MExportSheet({ planner, shell }: MTripSheetsProps) { const { t, locale } = useTranslation() + // The PDF is built outside React, so it cannot read this itself (#2066). + const timeFormat = useSettingsStore(s => s.settings.time_format) || '24h' const open = shell.sheet?.id === 'export' const dayNotes = useTripStore(s => s.dayNotes) const [subscribeOpen, setSubscribeOpen] = useState(false) @@ -46,6 +49,7 @@ export default function MExportSheet({ planner, shell }: MTripSheetsProps) { reservations: planner.reservations, t, locale, + timeFormat, }) } catch (e) { planner.toast.error(`${t('dayplan.pdfError')}: ${e instanceof Error ? e.message : String(e)}`) diff --git a/client/src/mobile/screens/trip/sheets/PlTimeFields.tsx b/client/src/mobile/screens/trip/sheets/PlTimeFields.tsx index e702933f1..3da0ba31c 100644 --- a/client/src/mobile/screens/trip/sheets/PlTimeFields.tsx +++ b/client/src/mobile/screens/trip/sheets/PlTimeFields.tsx @@ -1,6 +1,7 @@ import { useMemo } from 'react' import { AlertTriangle } from 'lucide-react' -import { Eyebrow, FIELD_CLS } from './PlSheetChrome' +import { Eyebrow } from './PlSheetChrome' +import CustomTimePicker from '../../../../components/shared/CustomTimePicker' import type { Assignment } from '../../../../types' import type { TripPlanner } from '../MTripShell' @@ -16,8 +17,6 @@ interface PlTimeFieldsProps { hasTimeError: boolean } -const TIME_CLS = `${FIELD_CLS} font-semibold [font-variant-numeric:tabular-nums]` - const WARNING_CLS = 'mt-2 flex items-start gap-[6px] rounded-[10px] bg-[rgba(232,161,58,.14)] px-[10px] py-[7px] font-geist text-[0.6875rem] leading-[1.4] text-[color:var(--m-st-pending)]' @@ -54,21 +53,11 @@ export default function PlTimeFields({
{t('places.startTime')} - onChange('place_time', e.target.value)} - className={TIME_CLS} - /> + onChange('place_time', v)} />
{t('places.endTime')} - onChange('end_time', e.target.value)} - className={TIME_CLS} - /> + onChange('end_time', v)} />
{hasTimeError && ( diff --git a/client/tests/unit/mobile/journey/MJourneyEntrySheet.test.tsx b/client/tests/unit/mobile/journey/MJourneyEntrySheet.test.tsx index df2e18dd7..a6c878221 100644 --- a/client/tests/unit/mobile/journey/MJourneyEntrySheet.test.tsx +++ b/client/tests/unit/mobile/journey/MJourneyEntrySheet.test.tsx @@ -285,7 +285,9 @@ function mountSheet(sheetEntry: JourneyEntry, opts: MountOptions = {}) { const multiFileInput = () => document.querySelector('input[type="file"][multiple]') as HTMLInputElement; const dateField = () => document.querySelector('input[type="date"]') as HTMLInputElement; -const timeField = () => document.querySelector('input[type="time"]') as HTMLInputElement; +// The time field is CustomTimePicker's text input, not a native time input — a +// native one paints 12h/24h from the browser locale and ignored the setting (#2067). +const timeField = () => document.querySelector('input[placeholder="00:00"], input[placeholder="2:30 PM"]') as HTMLInputElement; const jpeg = (name = 'a.jpg') => new File(['x'], name, { type: 'image/jpeg' }); const originalCreateObjectURL = URL.createObjectURL; diff --git a/client/tests/unit/mobile/trip/PlTimeFields.test.tsx b/client/tests/unit/mobile/trip/PlTimeFields.test.tsx index 5e06f9da0..36d21e983 100644 --- a/client/tests/unit/mobile/trip/PlTimeFields.test.tsx +++ b/client/tests/unit/mobile/trip/PlTimeFields.test.tsx @@ -3,8 +3,11 @@ import PlTimeFields from '../../../../src/mobile/screens/trip/sheets/PlTimeField import type { Assignment } from '../../../../src/types' import { buildPlanner } from '../../../helpers/mobileTrip' import { fireEvent, render, screen } from '../../../helpers/render' +import { seedStore } from '../../../helpers/store' +import { buildSettings } from '../../../helpers/factories' +import { useSettingsStore } from '../../../../src/store/settingsStore' -// FE-MOB-PLTIME-001 to FE-MOB-PLTIME-012 +// FE-MOB-PLTIME-001 to FE-MOB-PLTIME-013 const planner = buildPlanner() @@ -47,7 +50,12 @@ function setup({ hasTimeError={hasTimeError} />, ) - const inputs = Array.from(view.container.querySelectorAll('input[type="time"]')) as HTMLInputElement[] + // Both fields are CustomTimePicker text inputs now, so they answer to its + // 24h placeholder rather than to a native time input (#2067). + // Both fields are CustomTimePicker text inputs now, and they are the only text + // inputs this component renders — the placeholder itself changes with the + // user's format, so it cannot be the hook (#2067). + const inputs = Array.from(view.container.querySelectorAll('input[type="text"]')) as HTMLInputElement[] return { ...view, onChange, start: inputs[0], end: inputs[1] } } @@ -72,6 +80,25 @@ describe('PlTimeFields', () => { expect(onChange).toHaveBeenCalledWith('end_time', '12:45') }) + // #2067 — the desktop twin (PlaceFormModal) has used CustomTimePicker for ages; + // the phone kept a native time input, so the same form obeyed the setting on one + // shell and ignored it on the other. + // The normalisation back to 24h on blur belongs to CustomTimePicker and is + // covered by its own suite; what this file owns is that the phone shows the + // user's format at all, which the native input it replaced never did. + it('FE-MOB-PLTIME-013: a 12h user sees meridiem clocks, a 24h user does not', () => { + seedStore(useSettingsStore, { settings: buildSettings({ time_format: '12h' }) }) + const twelve = setup() + expect(twelve.start).toHaveValue('10:00 AM') + expect(twelve.end).toHaveValue('11:00 AM') + twelve.unmount() + + seedStore(useSettingsStore, { settings: buildSettings({ time_format: '24h' }) }) + const twentyFour = setup() + expect(twentyFour.start).toHaveValue('10:00') + expect(twentyFour.end).toHaveValue('11:00') + }) + it('FE-MOB-PLTIME-004: shows the end-before-start warning only when the sheet flags it', () => { setup() expect(screen.queryByText('places.endTimeBeforeStart')).not.toBeInTheDocument() diff --git a/server/src/nest/calendar/calendar.service.ts b/server/src/nest/calendar/calendar.service.ts index 39e138faa..5a007f52d 100644 --- a/server/src/nest/calendar/calendar.service.ts +++ b/server/src/nest/calendar/calendar.service.ts @@ -154,12 +154,15 @@ export class CalendarService { const reservations = this.db .prepare( `SELECT r.*, pl.lat AS place_lat, pl.lng AS place_lng, - sd.date AS stay_start_date, ed.date AS stay_end_date + sd.date AS stay_start_date, ed.date AS stay_end_date, + rd.date AS day_date, red.date AS end_day_date FROM reservations r LEFT JOIN places pl ON r.place_id = pl.id LEFT JOIN day_accommodations a ON r.accommodation_id = a.id LEFT JOIN days sd ON a.start_day_id = sd.id LEFT JOIN days ed ON a.end_day_id = ed.id + LEFT JOIN days rd ON r.day_id = rd.id + LEFT JOIN days red ON r.end_day_id = red.id WHERE r.trip_id = ?`, ) .all(tripId) as any[]; @@ -307,6 +310,21 @@ export class CalendarService { const isDate = (s: string | null | undefined) => !!s && /^\d{4}-\d{2}-\d{2}$/.test(s); const isTime = (s: string | null | undefined) => !!s && /^\d{2}:\d{2}/.test(s); + // "YYYY-MM-DD", full ISO, or a bare "HH:MM" → date/time parts. Endpoints carry + // local_date/local_time separately; reservation_time and reservation_end_time + // are combined strings, and the end is frequently just a bare clock alongside + // the reservation's own date. + const dateOf = (s: string | null | undefined): string | null => { + if (!s) return null; + const d = s.includes('T') ? s.split('T')[0] : s; + return isDate(d) ? d : null; + }; + const timeOf = (s: string | null | undefined): string | null => { + if (!s) return null; + const t = s.includes('T') ? s.split('T')[1] : s; + return t && isTime(t) ? t : null; + }; + // Build the DTSTART/DTEND lines for a reservation, or null when it has no // calendar-placeable time. Transports keep a per-side wall clock + IANA zone // on their endpoints, so consult a timed departure endpoint FIRST: transports @@ -391,11 +409,98 @@ export class CalendarService { if (first && isDate(first.local_date)) { return `DTSTART;VALUE=DATE:${fmtDate(first.local_date)}\r\n`; } + + // Last resort: the days the booking is pinned to. TransportModal writes + // reservation_time = NULL when its optional time pickers are left blank + // while still setting day_id/end_day_id, so for a hand-typed rental car — + // or bus, train, ferry, anything from that form — the day rows are the ONLY + // record of when it happens, and reading nothing else kept those bookings + // out of the subscribed calendar entirely (#2068). + if (isDate(r.day_date)) { + const lastDay = isDate(r.end_day_date) && r.end_day_date >= r.day_date ? r.end_day_date : r.day_date; + return `DTSTART;VALUE=DATE:${fmtDate(r.day_date)}\r\n` + + `DTEND;VALUE=DATE:${fmtDate(addDays(lastDay, 1))}\r\n`; + } return null; }; + // A booking that is a WINDOW rather than an activity: something is handed over + // and handed back, and the days in between carry nothing. Read as one block + // from drop-off to pick-up, a week of airport parking sat across the whole + // week of a subscribed calendar (#2068). The day plan already draws this + // distinction for parking (client/src/utils/dayMerge.ts, #1937); the exporter + // never learned it. A rental is the mirror image — you pick it up first. + const WINDOW_WORDING: Record = { + car: { start: 'Pickup', end: 'Drop-off' }, + parking: { start: 'Drop-off', end: 'Pickup' }, + }; + /** How long a hand-over reads as. Zero-length events collapse in most clients. */ + const HANDOVER_MINUTES = 60; + + const plusMinutes = (date: string, time: string, minutes: number): { date: string; time: string } => { + const [h, m] = time.slice(0, 5).split(':').map(Number); + const total = h * 60 + m + minutes; + const dayShift = Math.floor(total / (24 * 60)); + const rest = ((total % (24 * 60)) + 24 * 60) % (24 * 60); + return { + date: dayShift ? addDays(date, dayShift) : date, + time: `${String(Math.floor(rest / 60)).padStart(2, '0')}:${String(rest % 60).padStart(2, '0')}`, + }; + }; + + interface WindowSide { date: string; time: string | null; zone: string | null } + + // Which endpoint is which side. The ROLE decides, because an import can drop + // one (failed geocoding) and a surviving return endpoint must not masquerade + // as the pickup. Positional first/last is only a fallback when no role is + // present at all, and only with more than one endpoint, so a lone endpoint is + // never both sides (#1721). + const windowSidesOf = (r: any): { start: WindowSide | null; end: WindowSide | null } => { + const eps = endpointsMap.get(r.id); + const ordered = eps && eps.length > 0 ? [...eps].sort((a, b) => a.sequence - b.sequence) : []; + const roleFrom = ordered.find(e => e.role === 'from'); + const roleTo = ordered.find(e => e.role === 'to'); + const noRoles = !roleFrom && !roleTo; + const startEp = roleFrom ?? (noRoles && ordered.length > 1 ? ordered[0] : undefined); + const endEp = roleTo ?? (noRoles && ordered.length > 1 ? ordered[ordered.length - 1] : undefined); + + const fromEp = (ep: typeof startEp): WindowSide | null => + ep && isDate(ep.local_date) && isTime(ep.local_time) + ? { date: ep.local_date!, time: ep.local_time!, zone: ep.timezone || resolveTimeZone(ep.lat, ep.lng) } + : null; + + const placeZone = resolveTimeZone(r.place_lat, r.place_lng); + const start = fromEp(startEp) ?? (() => { + const date = dateOf(r.reservation_time) ?? (isDate(r.day_date) ? r.day_date : null); + return date ? { date, time: timeOf(r.reservation_time), zone: placeZone } : null; + })(); + const end = fromEp(endEp) ?? (() => { + const date = dateOf(r.reservation_end_time) + ?? (isDate(r.end_day_date) ? r.end_day_date : null) + ?? (timeOf(r.reservation_end_time) ? start?.date ?? null : null); + // The return side rarely carries a zone of its own. Inheriting the + // pickup's is what the single block did, and letting it float instead + // renders it in the subscriber's zone rather than the trip's (#1453). + return date ? { date, time: timeOf(r.reservation_end_time), zone: placeZone ?? start?.zone ?? null } : null; + })(); + return { start, end }; + }; + + // Split only when BOTH hand-overs resolve on DIFFERENT days. A same-day + // booking is one sitting and keeps its single event; a one-sided one — only + // the pickup was geocoded — keeps the block, which is then the only carrier + // of the return time at all. + const windowSplit = new Map(); + for (const r of reservations) { + if (!WINDOW_WORDING[r.type as string]) continue; + const { start, end } = windowSidesOf(r); + if (start && end && start.date !== end.date) windowSplit.set(r.id, { start, end }); + } + // Reservations as events for (const r of reservations) { + // The two hand-over events below stand in for this booking entirely. + if (windowSplit.has(r.id)) continue; const timeLines = buildReservationTimeLines(r); if (!timeLines) continue; const meta = r.metadata ? (typeof r.metadata === 'string' ? JSON.parse(r.metadata) : r.metadata) : {}; @@ -404,6 +509,18 @@ export class CalendarService { ev += timeLines; ev += `SUMMARY:${esc(r.title)}\r\n`; + const desc = describeReservation(r, meta); + if (desc) ev += `DESCRIPTION:${esc(desc)}\r\n`; + if (r.location) ev += `LOCATION:${esc(r.location)}\r\n`; + ev += `END:VEVENT\r\n`; + events.push(ev); + } + + // Everything a booking says about itself, minus its times. Hoisted out of the + // event loop because a window booking's two hand-over events carry the same + // lines: dropping the block must not drop the route or the confirmation with + // it (#2068). + function describeReservation(r: any, meta: any): string { let desc = r.type ? `Type: ${r.type}` : ''; if (r.confirmation_number) desc += `\nConfirmation: ${r.confirmation_number}`; if (meta.airline) desc += `\nAirline: ${meta.airline}`; @@ -433,11 +550,8 @@ export class CalendarService { } if (meta.train_number) desc += `\nTrain: ${meta.train_number}`; if (r.notes) desc += `\n${r.notes}`; - if (desc) ev += `DESCRIPTION:${esc(desc)}\r\n`; - if (r.location) ev += `LOCATION:${esc(r.location)}\r\n`; - ev += `END:VEVENT\r\n`; - events.push(ev); - } + return desc; + }; // Check-in and check-out as their own timed events, when the stay records the // clock (#1586). They are separate from the all-day stay above on purpose: an @@ -496,75 +610,59 @@ export class CalendarService { } } - // Pickup and drop-off as their own timed events — the car-rental analogue - // of check-in/check-out above (last unported piece of #1721). Additive, - // same as the check-in/check-out markers are additive to the all-day stay - // range: the rental's existing single-block event (from the endpoint or - // reservation_time branch of buildReservationTimeLines, above) is untouched. - // "YYYY-MM-DD", full ISO, or a bare "HH:MM" → date/time parts. Endpoints - // already carry local_date/local_time separately; reservation_time and - // reservation_end_time (the last-resort fallback below) are combined - // strings, and reservation_end_time is frequently just a bare clock - // alongside the reservation's own date. - const dateOf = (s: string | null | undefined): string | null => { - if (!s) return null; - const d = s.includes('T') ? s.split('T')[0] : s; - return isDate(d) ? d : null; - }; - const timeOf = (s: string | null | undefined): string | null => { - if (!s) return null; - const t = s.includes('T') ? s.split('T')[1] : s; - return t && isTime(t) ? t : null; - }; - + // The two hand-overs of a window booking, as their own events. + // + // For a rental this is the car-rental analogue of the check-in/check-out + // markers above (#1721): additive, sitting beside the booking's own block. + // For a booking whose two ends fall on DIFFERENT days it is not additive — + // `windowSplit` dropped the block, and these two ARE the booking (#2068), so + // they carry its description and a real duration instead of being points. + // + // Parking only ever appears here split. A same-day parking is one sitting and + // keeps the single event it has always had. for (const r of reservations) { - if (r.type !== 'car') continue; + const wording = WINDOW_WORDING[r.type as string]; + if (!wording) continue; + const split = windowSplit.get(r.id); + if (!split && r.type !== 'car') continue; - // Sides come from the endpoint role first: import can drop one endpoint - // (failed geocoding), and a surviving return endpoint must not masquerade - // as the pickup. Positional first/last by sequence is only a fallback - // when neither role is present at all, and only once there's more than - // one endpoint, so a lone endpoint is never treated as both sides. - const eps = endpointsMap.get(r.id); - const ordered = eps && eps.length > 0 ? [...eps].sort((a, b) => a.sequence - b.sequence) : []; - const roleFrom = ordered.find(e => e.role === 'from'); - const roleTo = ordered.find(e => e.role === 'to'); - const noRoles = !roleFrom && !roleTo; - const pickupEp = roleFrom ?? (noRoles && ordered.length > 1 ? ordered[0] : undefined); - const dropEp = roleTo ?? (noRoles && ordered.length > 1 ? ordered[ordered.length - 1] : undefined); + const { start, end } = split ?? windowSidesOf(r); + const meta = r.metadata ? (typeof r.metadata === 'string' ? JSON.parse(r.metadata) : r.metadata) : {}; + const desc = split ? describeReservation(r, meta) : ''; - const carMarker = (kind: 'pickup' | 'dropoff', summary: string, date: string, time: string, zone: string | null) => { + const handover = (kind: 'pickup' | 'dropoff', side: WindowSide, summary: string) => { let ev = `BEGIN:VEVENT\r\nUID:${uid(r.id, kind)}\r\nDTSTAMP:${now}\r\n`; - ev += dtLine('DTSTART', time, zone, `${date}T00:00`); + if (side.time) { + const ref = `${side.date}T00:00`; + ev += dtLine('DTSTART', side.time, side.zone, ref); + // Only where these events replace the block: adding a DTEND to the + // additive markers would change a feed that already went out. + if (split) { + const stop = plusMinutes(side.date, side.time, HANDOVER_MINUTES); + ev += dtLine('DTEND', stop.time, side.zone, `${stop.date}T00:00`); + } + } else if (split) { + // No clock anywhere — a rental typed into the planner with the optional + // time pickers left blank. One all-day event per hand-over still beats a + // block across the whole window. + ev += `DTSTART;VALUE=DATE:${fmtDate(side.date)}\r\n`; + ev += `DTEND;VALUE=DATE:${fmtDate(addDays(side.date, 1))}\r\n`; + } else { + return; + } ev += `SUMMARY:${esc(summary)}\r\n`; + if (desc) ev += `DESCRIPTION:${esc(desc)}\r\n`; if (r.location) ev += `LOCATION:${esc(r.location)}\r\n`; ev += `END:VEVENT\r\n`; events.push(ev); }; - // Per side: the chosen endpoint when it has a usable date and clock, - // else reservation_time (pickup) / reservation_end_time (drop-off) — - // imports frequently carry only that window. A side with no usable date - // and clock from either source emits nothing. - const pickupUsable = pickupEp && isDate(pickupEp.local_date) && isTime(pickupEp.local_time); - const pickupDate = pickupUsable ? pickupEp!.local_date : dateOf(r.reservation_time); - const pickupTime = pickupUsable ? pickupEp!.local_time : timeOf(r.reservation_time); - if (isDate(pickupDate) && isTime(pickupTime)) { - const zone = pickupUsable - ? (pickupEp!.timezone || resolveTimeZone(pickupEp!.lat, pickupEp!.lng)) - : resolveTimeZone(r.place_lat, r.place_lng); - carMarker('pickup', `Pickup: ${r.title}`, pickupDate!, pickupTime!, zone); - } - - const dropUsable = dropEp && isDate(dropEp.local_date) && isTime(dropEp.local_time); - const dropDate = dropUsable ? dropEp!.local_date : (dateOf(r.reservation_end_time) || dateOf(r.reservation_time)); - const dropTime = dropUsable ? dropEp!.local_time : timeOf(r.reservation_end_time); - if (isDate(dropDate) && isTime(dropTime)) { - const zone = dropUsable - ? (dropEp!.timezone || resolveTimeZone(dropEp!.lat, dropEp!.lng)) - : resolveTimeZone(r.place_lat, r.place_lng); - carMarker('dropoff', `Drop-off: ${r.title}`, dropDate!, dropTime!, zone); - } + // The wording is per type, the UID is per side: a rental is picked up first + // and dropped off at the end, a parking is the other way round, but the + // first side of the window keeps the same UID either way so an existing + // subscription does not grow a duplicate. + if (start) handover('pickup', start, `${wording.start}: ${r.title}`); + if (end) handover('dropoff', end, `${wording.end}: ${r.title}`); } // Every referenced zone gets a VTIMEZONE. They are emitted before the first diff --git a/server/tests/unit/nest/calendar.service.test.ts b/server/tests/unit/nest/calendar.service.test.ts index ff172f490..2a408c8b3 100644 --- a/server/tests/unit/nest/calendar.service.test.ts +++ b/server/tests/unit/nest/calendar.service.test.ts @@ -1059,6 +1059,139 @@ describe('car rentals', () => { // No 17-character value anywhere: that is what silently drops the zone. expect(ics).not.toMatch(/DTSTART[^\r\n]*\d{8}T\d{8}/); }); + + // ── Window bookings: two hand-overs, not one block (#2068) ───────────────── + + it('CAL-045: multi-day parking becomes a drop-off and a pick-up instead of one block', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const parking = createReservation(testDb, trip.id, { title: 'Airport P4', type: 'parking' }); + testDb.prepare('UPDATE reservations SET reservation_time=?, reservation_end_time=? WHERE id=?') + .run('2026-07-01T06:30', '2026-07-10T22:15', parking.id); + + const { ics } = svc.exportICS(trip.id); + + // The two hand-overs, an hour each. Parking is dropped off first. + expect(ics).toContain('SUMMARY:Drop-off: Airport P4'); + expect(ics).toContain('DTSTART:20260701T063000'); + expect(ics).toContain('DTEND:20260701T073000'); + expect(ics).toContain('SUMMARY:Pickup: Airport P4'); + expect(ics).toContain('DTSTART:20260710T221500'); + expect(ics).toContain('DTEND:20260710T231500'); + // And the block across the ten days in between is gone. + expect(ics).not.toContain('SUMMARY:Airport P4\r\n'); + expect(ics).not.toContain('DTEND:20260710T221500'); + }); + + it('CAL-046: a same-day parking is one sitting and keeps its single event', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const parking = createReservation(testDb, trip.id, { title: 'Garage', type: 'parking' }); + testDb.prepare('UPDATE reservations SET reservation_time=?, reservation_end_time=? WHERE id=?') + .run('2026-07-01T08:00', '2026-07-01T18:30', parking.id); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('SUMMARY:Garage\r\n'); + expect(ics).toContain('DTSTART:20260701T080000'); + expect(ics).toContain('DTEND:20260701T183000'); + expect(ics).not.toContain('Drop-off: Garage'); + expect(ics).not.toContain('Pickup: Garage'); + }); + + it('CAL-047: a rental typed with days but no clock finally reaches the feed', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const first = createDay(testDb, trip.id, { date: '2026-07-03' }); + const last = createDay(testDb, trip.id, { date: '2026-07-08' }); + const rental = createReservation(testDb, trip.id, { title: 'Sixt', type: 'car' }); + // The planner writes reservation_time = NULL when the optional time pickers + // are left blank, which is what made this booking invisible. + testDb.prepare('UPDATE reservations SET reservation_time=NULL, reservation_end_time=NULL, day_id=?, end_day_id=? WHERE id=?') + .run(first.id, last.id, rental.id); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('SUMMARY:Pickup: Sixt'); + expect(ics).toContain('DTSTART;VALUE=DATE:20260703'); + expect(ics).toContain('SUMMARY:Drop-off: Sixt'); + expect(ics).toContain('DTSTART;VALUE=DATE:20260708'); + }); + + it('CAL-048: a split rental keeps its zones and everything the block used to say', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const rental = createReservation(testDb, trip.id, { title: 'Hertz', type: 'car' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL, reservation_end_time=NULL, confirmation_number=?, location=? WHERE id=?') + .run('HZ-4471', 'Hertz Downtown', rental.id); + insertEndpoint(rental.id, 'from', 0, 'Paris Office', 48.8566, 2.3522, 'Europe/Paris', '09:00', '2026-07-07'); + insertEndpoint(rental.id, 'to', 1, 'Berlin Office', 52.5, 13.4, 'Europe/Berlin', '10:30', '2026-07-14'); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('DTSTART;TZID=Europe/Paris:20260707T090000'); + expect(ics).toContain('DTEND;TZID=Europe/Paris:20260707T100000'); + expect(ics).toContain('DTSTART;TZID=Europe/Berlin:20260714T103000'); + expect(ics).toContain('DTEND;TZID=Europe/Berlin:20260714T113000'); + // Dropping the block must not drop what it said: both hand-overs carry it. + // Long lines are folded at 75 octets, so unfold before reading them. + const unfolded = ics.replaceAll('\r\n ', ''); + const descriptions = unfolded.split('\r\n').filter(l => l.startsWith('DESCRIPTION:Type: car')); + expect(descriptions).toHaveLength(2); + expect(descriptions[0]).toContain('Confirmation: HZ-4471'); + expect(descriptions[0]).toContain('Route: Paris Office → Berlin Office'); + }); + + // #1453 — the block is the only carrier of the return time when just one side + // was geocoded, so a one-sided rental must keep it. + it('CAL-049: a one-sided rental keeps its block rather than losing the return', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const place = createPlace(testDb, trip.id, { name: 'Rental Desk', lat: 48.8566, lng: 2.3522 }); + const rental = createReservation(testDb, trip.id, { title: 'Avis', type: 'car' }); + testDb.prepare('UPDATE reservations SET reservation_time=?, reservation_end_time=?, place_id=? WHERE id=?') + .run('2026-07-02T10:00', '2026-07-09T10:00', place.id, rental.id); + + const { ics } = svc.exportICS(trip.id); + + // Both sides resolve off reservation_time/-_end_time, so this one does split, + // and the return keeps the pickup's zone instead of floating. + expect(ics).toContain('DTSTART;TZID=Europe/Paris:20260702T100000'); + expect(ics).toContain('DTSTART;TZID=Europe/Paris:20260709T100000'); + expect(ics).not.toMatch(/DTSTART:20260709T100000/); + }); + + it('CAL-050: a rental with only a pickup endpoint is left exactly as it was', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const rental = createReservation(testDb, trip.id, { title: 'Solo', type: 'car' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL, reservation_end_time=NULL WHERE id=?').run(rental.id); + insertEndpoint(rental.id, 'from', 0, 'Paris Office', 48.8566, 2.3522, 'Europe/Paris', '09:00', '2026-07-07'); + + const { ics } = svc.exportICS(trip.id); + + expect(ics).toContain('SUMMARY:Solo\r\n'); + expect(ics).toContain('SUMMARY:Pickup: Solo'); + expect(ics).not.toContain('Drop-off'); + }); + + it('CAL-051: any transport pinned to days without a clock reaches the feed too', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Road Trip' }); + const first = createDay(testDb, trip.id, { date: '2026-07-03' }); + const last = createDay(testDb, trip.id, { date: '2026-07-04' }); + const bus = createReservation(testDb, trip.id, { title: 'Night Bus', type: 'bus' }); + testDb.prepare('UPDATE reservations SET reservation_time=NULL, reservation_end_time=NULL, day_id=?, end_day_id=? WHERE id=?') + .run(first.id, last.id, bus.id); + + const { ics } = svc.exportICS(trip.id); + + // Not a window booking, so it stays one block — it just exists now. + expect(ics).toContain('SUMMARY:Night Bus'); + expect(ics).toContain('DTSTART;VALUE=DATE:20260703'); + expect(ics).toContain('DTEND;VALUE=DATE:20260705'); + expect(ics).not.toContain('Pickup: Night Bus'); + }); }); describe('folded quirk branches', () => {