From 07e4a558e64e8f89500ef86a409d6bb07f695de4 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 3 Sep 2026 15:51:13 +0200 Subject: [PATCH] fix(bookings): the phone keeps a booking's stop and shows the files it already has (#2216, #2217) Editing a booking on the phone erased the stop it was linked to. The sheet seeded its assignment from the create-for-a-stop flag, which is null on an edit, and saved that straight back over a link the user had made on the desktop; the server writes the field whenever the key is present. It now seeds from the booking being edited, and the picker the desktop dialog has always had sits in the sheet too, so a stop can be seen, changed and cleared from a phone. The option list moved to its own module rather than being copied. Two more of the same shape, found on the way. The endpoint set was sent as an empty list on every save, and the server swaps the whole set when that key is present, so editing a transit booking dropped its stations; neither form edits endpoints, so an edit no longer sends the key at all. And the sheet's Files block listed only what was picked in this session, so a file uploaded from the desktop was invisible on the phone. It lists what is attached now, resolved the way the desktop resolves it. A place also shows the files of the bookings that sit on it, on the phone and on the desktop: a confirmation lives on the booking, and the place card was the one screen that could not reach it. --- .../src/components/Planner/PlaceInspector.tsx | 4 +- .../components/Planner/ReservationModal.tsx | 31 ++------ .../components/Planner/assignmentOptions.ts | 52 +++++++++++++ .../screens/trip/sheets/MPlaceSheet.tsx | 9 ++- .../screens/trip/sheets/MReservationSheet.tsx | 74 ++++++++++++++++++- client/src/utils/placeFiles.ts | 34 +++++++++ .../mobile/trip/MReservationSheet.test.tsx | 62 +++++++++++++++- client/tests/unit/utils/placeFiles.test.ts | 57 ++++++++++++++ 8 files changed, 289 insertions(+), 34 deletions(-) create mode 100644 client/src/components/Planner/assignmentOptions.ts create mode 100644 client/src/utils/placeFiles.ts create mode 100644 client/tests/unit/utils/placeFiles.test.ts diff --git a/client/src/components/Planner/PlaceInspector.tsx b/client/src/components/Planner/PlaceInspector.tsx index c895f25e1..504f4361a 100644 --- a/client/src/components/Planner/PlaceInspector.tsx +++ b/client/src/components/Planner/PlaceInspector.tsx @@ -12,6 +12,7 @@ import PlaceAvatarUpload from '../shared/PlaceAvatarUpload' import PlaceRating from '../shared/StarRating' import TrackColorPicker from '../shared/TrackColorPicker' import { resolveTrackColor, inheritedTrackColor } from '../Map/trackColors' +import { filesForPlace } from '../../utils/placeFiles' import GuestBadge from '../shared/GuestBadge' import StatusBadge from '../Collections/StatusBadge' import { mapsApi, pluginsApi } from '../../api/client' @@ -337,7 +338,8 @@ export default function PlaceInspector({ const selectedDay = days?.find(d => d.id === selectedDayId) const weekdayIndex = getWeekdayIndex(selectedDay?.date, placeTimeZone) - const placeFiles = (files || []).filter(f => String(f.place_id) === String(place.id) || (f.linked_place_ids || []).includes(place.id)) + // Its own files plus the ones on the bookings that hang on it (#2217). + const placeFiles = filesForPlace(files, place.id, reservations, selectedAssignmentId != null ? [selectedAssignmentId] : []) return (
a.order_index - b.order_index) - if (da.length === 0) continue - const dayLabel = day.title || t('dayplan.dayN', { n: day.day_number }) - const dateStr = day.date ? ` · ${formatDate(day.date, locale)}` : '' - const groupLabel = `${dayLabel}${dateStr}` - options.push({ value: `_header_${day.id}`, label: groupLabel, disabled: true, isHeader: true }) - for (let i = 0; i < da.length; i++) { - const place = da[i].place - if (!place) continue - const timeStr = place.place_time ? ` · ${place.place_time}${place.end_time ? ' – ' + place.end_time : ''}` : '' - options.push({ - value: da[i].id, - label: ` ${i + 1}. ${place.name}${timeStr}`, - searchLabel: place.name, - groupLabel, - dayDate: day.date || null, - }) - } - } - return options -} - interface ReservationModalProps { isOpen: boolean onClose: () => void @@ -279,7 +255,10 @@ export function ReservationModal({ isOpen, onClose, onSave, reservation, days, p // the picked trip place/activity directly on the reservation (#1353). place_id: form.type === 'hotel' ? null : (form.place_id || null), metadata: Object.keys(metadata).length > 0 ? metadata : null, - endpoints: [], + // Omitted on an edit: the server replaces the endpoint set whenever the + // key is present, and this form never edits endpoints, so sending an + // empty list would drop a transit booking's stations (#2216). + ...(reservation?.id ? {} : { endpoints: [] }), needs_review: false, } if (form.type === 'hotel' && (form.hotel_start_day || form.hotel_end_day)) { diff --git a/client/src/components/Planner/assignmentOptions.ts b/client/src/components/Planner/assignmentOptions.ts new file mode 100644 index 000000000..1a53dfd16 --- /dev/null +++ b/client/src/components/Planner/assignmentOptions.ts @@ -0,0 +1,52 @@ +import type { Assignment, Day, TranslationFn } from '../../types' + +/** Day and month only, the shape the booking dialog has always shown here. */ +function dayBadge(dateStr: string, locale?: string): string { + return new Date(dateStr + 'T00:00:00Z') + .toLocaleDateString(locale || undefined, { day: 'numeric', month: 'short', timeZone: 'UTC' }) +} + +export interface AssignmentOption { + value: number | string + label: string + searchLabel?: string + groupLabel?: string + dayDate?: string | null + disabled?: boolean + isHeader?: boolean +} + +/** + * The day plan as a flat option list: a disabled header per day, then its stops + * in plan order. Shared by the desktop booking dialog and the phone sheet so a + * booking can be linked to the same stop from either (#2216). + */ +export function buildAssignmentOptions( + days: Day[] | undefined, + assignments: Record | undefined, + t: TranslationFn, + locale?: string, +): AssignmentOption[] { + const options: AssignmentOption[] = [] + for (const day of (days || [])) { + const da = (assignments?.[String(day.id)] || []).slice().sort((a, b) => a.order_index - b.order_index) + if (da.length === 0) continue + const dayLabel = day.title || t('dayplan.dayN', { n: day.day_number }) + const dateStr = day.date ? ` · ${dayBadge(day.date, locale)}` : '' + const groupLabel = `${dayLabel}${dateStr}` + options.push({ value: `_header_${day.id}`, label: groupLabel, disabled: true, isHeader: true }) + for (let i = 0; i < da.length; i++) { + const place = da[i].place + if (!place) continue + const timeStr = place.place_time ? ` · ${place.place_time}${place.end_time ? ' – ' + place.end_time : ''}` : '' + options.push({ + value: da[i].id, + label: ` ${i + 1}. ${place.name}${timeStr}`, + searchLabel: place.name, + groupLabel, + dayDate: day.date || null, + }) + } + } + return options +} diff --git a/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx b/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx index 968c214a1..b3ca996a5 100644 --- a/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx +++ b/client/src/mobile/screens/trip/sheets/MPlaceSheet.tsx @@ -19,6 +19,7 @@ import { resolveTrackColor, inheritedTrackColor } from '../../../../components/M import { avatarSrc } from '../../../../utils/avatarSrc' import { safeHttpUrl } from '../../../../utils/safeUrl' import { openFile } from '../../../../utils/fileDownload' +import { filesForPlace } from '../../../../utils/placeFiles' import { getNavigationTargets, openNavigationTarget } from '../../../../components/Planner/placeNavigation' import { NavigationMenu } from '../../../../components/shared/NavigationMenu' import { getAssignmentReservations } from '../../../../utils/dayMerge' @@ -151,8 +152,12 @@ export default function MPlaceSheet({ planner, shell }: MTripSheetsProps) { setParticipantPickerOpen(false) } - const placeFiles = (planner.files || []).filter(f => - !f.deleted_at && (String(f.place_id) === String(place?.id) || (f.linked_place_ids || []).includes(place?.id ?? -1)), + // Its own files plus the ones on the bookings that hang on it (#2217). + const placeFiles = filesForPlace( + planner.files, + place?.id, + planner.reservations, + assignmentInDay ? [assignmentInDay.id] : [], ) const handleImagePick = async (e: React.ChangeEvent) => { diff --git a/client/src/mobile/screens/trip/sheets/MReservationSheet.tsx b/client/src/mobile/screens/trip/sheets/MReservationSheet.tsx index 81b037b88..ca219a584 100644 --- a/client/src/mobile/screens/trip/sheets/MReservationSheet.tsx +++ b/client/src/mobile/screens/trip/sheets/MReservationSheet.tsx @@ -1,5 +1,5 @@ -import { useEffect, useRef, useState } from 'react' -import { Check, FileText, Hotel, Link2, ParkingSquare, Plus, Ticket, Users, Utensils } from 'lucide-react' +import { useEffect, useMemo, useRef, useState } from 'react' +import { Check, ExternalLink, FileText, Hotel, Link2, ParkingSquare, Plus, Ticket, Users, Utensils } from 'lucide-react' import MSheet from '../../../components/MSheet' import { useAddonStore } from '../../../../store/addonStore' import { useTranslation } from '../../../../i18n' @@ -11,6 +11,8 @@ import CustomTimePicker from '../../../../components/shared/CustomTimePicker' import { CustomDatePicker } from '../../../../components/shared/CustomDateTimePicker' import { Eyebrow, FIELD_AREA_CLS, FIELD_CLS, FormSheetFooter, FormSheetHeader } from './PlSheetChrome' import PlFileAttach from './PlFileAttach' +import { buildAssignmentOptions } from '../../../../components/Planner/assignmentOptions' +import { openFile } from '../../../../utils/fileDownload' import GuestBadge from '../../../../components/shared/GuestBadge' import { SPLIT_COLORS } from '../../../../components/Budget/BudgetPanel.constants' import { useTripStore } from '../../../../store/tripStore' @@ -57,6 +59,7 @@ export default function MReservationSheet({ planner, onOpenExpense }: MReservati showReservationModal, setShowReservationModal, editingReservation, setEditingReservation, reservationPrefill, bookingForAssignmentId, setBookingForAssignmentId, + assignments, files, importReviewActive, advanceImportReview, handleSaveReservation, canUploadFiles, tripActions, } = planner @@ -67,6 +70,11 @@ export default function MReservationSheet({ planner, onOpenExpense }: MReservati const [form, setForm] = useState(EMPTY) const [pendingFiles, setPendingFiles] = useState([]) + const assignmentOptions = useMemo( + () => buildAssignmentOptions(days, assignments, t, locale), + [days, assignments, t, locale], + ) + // Travelers assigned to this booking (#1517) — seeded from the editing // reservation on open, persisted separately after the save resolves. const [travelerIds, setTravelerIds] = useState>(new Set()) @@ -77,10 +85,15 @@ export default function MReservationSheet({ planner, onOpenExpense }: MReservati const [snap, setSnap] = useState<{ res: typeof editingReservation; assignmentId: number | null }>( { res: null, assignmentId: null }, ) + // The stop this booking hangs on. Seeded from the booking being edited, not + // only from the create-for-a-stop flag, because saving used to write the flag + // straight back and an edit therefore erased the link (#2216). + const [assignmentId, setAssignmentId] = useState('') useEffect(() => { if (!showReservationModal) return setSnap({ res: editingReservation, assignmentId: bookingForAssignmentId ?? null }) + setAssignmentId(bookingForAssignmentId ?? editingReservation?.assignment_id ?? '') expenseIntentRef.current = false setPendingFiles([]) setTravelerIds(new Set((editingReservation?.travelers || []).map(tv => tv.user_id))) @@ -147,6 +160,16 @@ export default function MReservationSheet({ planner, onOpenExpense }: MReservati }, [showReservationModal]) const res = snap.res + // Files already on this booking. The sheet used to list only the ones picked + // in this session, so an upload from the desktop was invisible here (#2217). + const attachedFiles = res?.id + ? (files || []).filter(f => + !f.deleted_at && ( + String(f.reservation_id) === String(res.id) || + (f.linked_reservation_ids || []).includes(res.id) + ), + ) + : [] const isHotel = form.type === 'hotel' const set = (field: keyof typeof EMPTY, value: string | number) => setForm(prev => ({ ...prev, [field]: value })) @@ -233,11 +256,15 @@ export default function MReservationSheet({ planner, onOpenExpense }: MReservati location: isHotel ? form.hotel_address : form.location, confirmation_number: form.confirmation_number, notes: form.notes, url: form.url, - assignment_id: (isHotel && !form.accommodation_id) ? null : (snap.assignmentId || null), + assignment_id: (isHotel && !form.accommodation_id) ? null : (assignmentId || null), accommodation_id: isHotel ? (form.accommodation_id || null) : null, place_id: isHotel ? null : (form.place_id || null), metadata: Object.keys(metadata).length > 0 ? metadata : null, - endpoints: [], needs_review: false, + // Omitted on an edit for the same reason as the desktop dialog: the + // server swaps the whole endpoint set when the key is present, and this + // sheet never edits endpoints (#2216). + ...(snap.res?.id ? {} : { endpoints: [] }), + needs_review: false, } if (isHotel && (form.hotel_start_day || form.hotel_end_day)) { saveData.create_accommodation = { @@ -556,7 +583,46 @@ export default function MReservationSheet({ planner, onOpenExpense }: MReservati
)} + {/* LINK TO A STOP IN THE PLAN (#2216) */} + {!isHotel && assignmentOptions.length > 0 && ( + <> + {t('reservations.linkAssignment')} + { + setAssignmentId(value === '' ? '' : Number(value)) + const opt = assignmentOptions.find(o => o.value === value) + // Same courtesy as the desktop dialog: an undated booking takes + // the day of the stop it was just linked to. + if (opt?.dayDate) setForm(prev => (prev.reservation_time ? prev : { ...prev, reservation_time: opt.dayDate! })) + }} + placeholder={t('reservations.pickAssignment')} + options={[{ value: '', label: t('reservations.noAssignment') }, ...assignmentOptions]} + searchable + size="sm" + /> + + )} + {/* FILES */} + {attachedFiles.length > 0 && ( + <> + {t('files.title')} +
+ {attachedFiles.map(f => ( + + ))} +
+ + )} {canUploadFiles && ( + !f.deleted_at && (String(f.place_id) === String(placeId) || (f.linked_place_ids || []).includes(placeId)), + ) + const bookingIds = new Set( + (reservations || []) + .filter(r => String(r.place_id) === String(placeId) || (r.assignment_id != null && assignmentIds.includes(r.assignment_id))) + .map(r => r.id), + ) + if (bookingIds.size === 0) return own + const seen = new Set(own.map(f => f.id)) + return [ + ...own, + ...(files || []).filter(f => { + if (f.deleted_at || seen.has(f.id)) return false + if (f.reservation_id != null && bookingIds.has(Number(f.reservation_id))) return true + return (f.linked_reservation_ids || []).some(id => id != null && bookingIds.has(id)) + }), + ] +} diff --git a/client/tests/unit/mobile/trip/MReservationSheet.test.tsx b/client/tests/unit/mobile/trip/MReservationSheet.test.tsx index f40077a2a..460d1070b 100644 --- a/client/tests/unit/mobile/trip/MReservationSheet.test.tsx +++ b/client/tests/unit/mobile/trip/MReservationSheet.test.tsx @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import MReservationSheet from '../../../../src/mobile/screens/trip/sheets/MReservationSheet' import { useAddonStore } from '../../../../src/store/addonStore' import { useTripStore } from '../../../../src/store/tripStore' -import type { Accommodation, Day, Place, Reservation, TripMember } from '../../../../src/types' +import type { Accommodation, Assignment, Day, Place, Reservation, TripMember } from '../../../../src/types' import { buildPlanner } from '../../../helpers/mobileTrip' import { resetAllStores } from '../../../helpers/store' import { fireEvent, render, screen, waitFor } from '../../../helpers/render' @@ -76,6 +76,11 @@ const ACCOMMODATIONS = [ { id: 77, trip_id: 5, place_id: 101, start_day_id: 12, end_day_id: 13 }, ] as unknown as Accommodation[] +const ASSIGNMENT = { + id: 11, day_id: 12, order_index: 0, place_id: 102, + place: { id: 102, name: 'Cafe Central', place_time: '19:30' }, +} as unknown as Assignment + const MEMBERS = [ { id: 7, username: 'Ana', avatar_url: '/uploads/ana.png' }, { id: 8, username: 'bob', is_guest: true }, @@ -209,6 +214,61 @@ describe('MReservationSheet', () => { }) }) + it('FE-MOB-RESSH-054: editing a booking keeps the stop it was linked to (#2216)', async () => { + const linked = { ...DINNER, id: 71, assignment_id: 11, title: 'Zoo tickets' } as unknown as Reservation + const planner = makePlanner({ editingReservation: linked, assignments: { 12: [ASSIGNMENT] } }) + setup(planner) + + fireEvent.click(submitBtn()) + + await waitFor(() => expect(planner.handleSaveReservation).toHaveBeenCalled()) + // Saving without touching the picker used to send null and erase the link. + expect(savedPayload(planner).assignment_id).toBe(11) + // And the endpoint set is left alone rather than replaced by an empty one. + expect(savedPayload(planner)).not.toHaveProperty('endpoints') + }) + + it('FE-MOB-RESSH-055: the picker shows the plan and can unlink the booking (#2216)', async () => { + const linked = { ...DINNER, id: 71, assignment_id: 11 } as unknown as Reservation + const planner = makePlanner({ editingReservation: linked, assignments: { 12: [ASSIGNMENT] } }) + setup(planner) + + expect(screen.getByText('reservations.linkAssignment')).toBeInTheDocument() + const picker = selects().find(s => s.getAttribute('data-value') === '11') + expect(picker).toBeTruthy() + + pick(picker!, '') + fireEvent.click(submitBtn()) + await waitFor(() => expect(planner.handleSaveReservation).toHaveBeenCalled()) + expect(savedPayload(planner).assignment_id).toBeNull() + }) + + it('FE-MOB-RESSH-056: a booking created for a stop still saves that stop', async () => { + const planner = makePlanner({ bookingForAssignmentId: 11, assignments: { 12: [ASSIGNMENT] } }) + setup(planner) + type(titleField(), 'Zoo tickets') + fireEvent.click(submitBtn()) + + await waitFor(() => expect(planner.handleSaveReservation).toHaveBeenCalled()) + expect(savedPayload(planner).assignment_id).toBe(11) + // A create still declares the endpoint set, which is what the server expects. + expect(savedPayload(planner).endpoints).toEqual([]) + }) + + it('FE-MOB-RESSH-057: the files already on the booking are listed, not just this session\'s picks (#2217)', () => { + const linked = { ...DINNER, id: 71 } as unknown as Reservation + const files = [ + { id: 4, trip_id: 5, reservation_id: 71, original_name: 'voucher.pdf', url: '/uploads/voucher.pdf', deleted_at: null }, + { id: 5, trip_id: 5, reservation_id: 99, original_name: 'other.pdf', url: '/uploads/other.pdf', deleted_at: null }, + { id: 6, trip_id: 5, linked_reservation_ids: [71], original_name: 'linked.pdf', url: '/uploads/linked.pdf', deleted_at: null }, + ] + setup(makePlanner({ editingReservation: linked, files })) + + expect(screen.getByText('voucher.pdf')).toBeInTheDocument() + expect(screen.getByText('linked.pdf')).toBeInTheDocument() + expect(screen.queryByText('other.pdf')).not.toBeInTheDocument() + }) + it('FE-MOB-RESSH-007: the type chips switch the form between event and hotel layout', () => { setup() expect(screen.getByLabelText('reservations.meta.pickPlace')).toBeInTheDocument() diff --git a/client/tests/unit/utils/placeFiles.test.ts b/client/tests/unit/utils/placeFiles.test.ts new file mode 100644 index 000000000..e8f3547a3 --- /dev/null +++ b/client/tests/unit/utils/placeFiles.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import { filesForPlace } from '../../../src/utils/placeFiles' +import type { Reservation, TripFile } from '../../../src/types' + +// FE-UTIL-PLACEFILES-001 to FE-UTIL-PLACEFILES-006 + +const file = (over: Partial): TripFile => ({ + id: 1, trip_id: 5, original_name: 'a.pdf', url: '/uploads/a.pdf', + place_id: null, reservation_id: null, deleted_at: null, + ...over, +} as unknown as TripFile) + +const booking = (over: Partial): Reservation => ({ + id: 90, trip_id: 5, title: 'Table', type: 'restaurant', + place_id: null, assignment_id: null, + ...over, +} as unknown as Reservation) + +describe('filesForPlace', () => { + it('FE-UTIL-PLACEFILES-001: keeps the files the place owns, by column and by link', () => { + const own = file({ id: 1, place_id: 7 }) + const linked = file({ id: 2, linked_place_ids: [7] }) + const other = file({ id: 3, place_id: 8 }) + expect(filesForPlace([own, linked, other], 7, []).map(f => f.id)).toEqual([1, 2]) + }) + + it('FE-UTIL-PLACEFILES-002: adds the files of a booking that sits on the place (#2217)', () => { + const files = [file({ id: 4, reservation_id: 90 })] + const bookings = [booking({ id: 90, place_id: 7 })] + expect(filesForPlace(files, 7, bookings).map(f => f.id)).toEqual([4]) + }) + + it('FE-UTIL-PLACEFILES-003: reaches a booking through the stop it hangs on, not only through place_id', () => { + const files = [file({ id: 5, reservation_id: 90 })] + const bookings = [booking({ id: 90, assignment_id: 11 })] + expect(filesForPlace(files, 7, bookings, [11]).map(f => f.id)).toEqual([5]) + // Without that stop in the day, the booking is not this place's business. + expect(filesForPlace(files, 7, bookings, [12])).toEqual([]) + }) + + it('FE-UTIL-PLACEFILES-004: a file linked to the booking counts like one stored on it', () => { + const files = [file({ id: 6, linked_reservation_ids: [90] })] + expect(filesForPlace(files, 7, [booking({ id: 90, place_id: 7 })]).map(f => f.id)).toEqual([6]) + }) + + it('FE-UTIL-PLACEFILES-005: lists a file once when it is both the place\'s and the booking\'s', () => { + const both = file({ id: 7, place_id: 7, reservation_id: 90 }) + expect(filesForPlace([both], 7, [booking({ id: 90, place_id: 7 })]).map(f => f.id)).toEqual([7]) + }) + + it('FE-UTIL-PLACEFILES-006: drops deleted files and answers empty without a place', () => { + const gone = file({ id: 8, place_id: 7, deleted_at: '2026-09-01' }) + const bookingFileGone = file({ id: 9, reservation_id: 90, deleted_at: '2026-09-01' }) + expect(filesForPlace([gone, bookingFileGone], 7, [booking({ id: 90, place_id: 7 })])).toEqual([]) + expect(filesForPlace([file({ id: 10, place_id: 7 })], undefined, [])).toEqual([]) + }) +})