From 3a67a8e2797cf3a12d0272299cbc755bc401968d Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 23 Sep 2026 13:41:31 +0200 Subject: [PATCH] fix(planner): trip places in every manual location field, never at 0,0 The trip's places went into the field's search results as they were. A place without a pin was picked as 0,0, because Number(null) is a finite 0, and a highlight left over from a longer search could point past the shorter list and throw on Enter. The phone's form, the train stations and the car stops were not offered the places at all. The places are now prepared once per form (named, with a real coordinate, each once) and handed to every location field of the manual tab, on the desktop and on the phone. The field shows them while it is empty or holds fewer than three characters, narrowed by what was typed, and derives that list rather than writing it into the search results, so a late search answer cannot replace it and a place already picked does not reopen it. --- .../Planner/LocationSelect.test.tsx | 83 ++++++++++++++++++- .../src/components/Planner/LocationSelect.tsx | 34 +++++--- .../Planner/TransportModal.test.tsx | 34 +++++++- .../src/components/Planner/TransportModal.tsx | 12 ++- .../components/Planner/locationPicks.test.ts | 32 +++++++ .../src/components/Planner/locationPicks.ts | 33 ++++++++ .../trip/sheets/MTransportFormSheet.tsx | 12 ++- .../mobile/trip/MTransportFormSheet.test.tsx | 18 +++- 8 files changed, 234 insertions(+), 24 deletions(-) create mode 100644 client/src/components/Planner/locationPicks.test.ts create mode 100644 client/src/components/Planner/locationPicks.ts diff --git a/client/src/components/Planner/LocationSelect.test.tsx b/client/src/components/Planner/LocationSelect.test.tsx index c83ca1204..22ec2eb15 100644 --- a/client/src/components/Planner/LocationSelect.test.tsx +++ b/client/src/components/Planner/LocationSelect.test.tsx @@ -1,4 +1,4 @@ -// FE-PLANNER-LOCSEL-001 to FE-PLANNER-LOCSEL-019 +// FE-PLANNER-LOCSEL-001 to FE-PLANNER-LOCSEL-024 import { useState } from 'react'; import { delay, http, HttpResponse } from 'msw'; import userEvent from '@testing-library/user-event'; @@ -29,11 +29,15 @@ function searchRoute(places: SearchHit[] | (() => Response | Promise)) // Controlled host — `value` drives the input text and mutes the search for the // already-picked name, exactly like ReservationModal wires it up. -function Host({ initial = null, onPick }: { initial?: LocationPoint | null; onPick?: (l: LocationPoint | null) => void }) { +function Host({ initial = null, onPick, places }: { initial?: LocationPoint | null; onPick?: (l: LocationPoint | null) => void; places?: LocationPoint[] }) { const [value, setValue] = useState(initial); - return { setValue(l); onPick?.(l); }} />; + return { setValue(l); onPick?.(l); }} places={places} />; } +/** The trip's own places, as the transport forms hand them over (#2468). */ +const HOTEL = { name: 'Bach Suites Saigon', lat: 10.78, lng: 106.7, address: 'District 1' }; +const EXPO = { name: 'Textile Exhibition', lat: 10.8, lng: 106.66, address: null }; + describe('LocationSelect', () => { it('FE-PLANNER-LOCSEL-001: falls back to the translated placeholder', () => { render(); @@ -263,4 +267,77 @@ describe('LocationSelect', () => { await waitFor(() => expect(screen.queryByText('Gare du Nord')).not.toBeInTheDocument()); }); + + it('FE-PLANNER-LOCSEL-020: an empty field offers the trip places, and picking one needs no search', async () => { + const user = userEvent.setup(); + let calls = 0; + server.use(searchRoute(() => { calls++; return HttpResponse.json({ places: [GARE] }); })); + const onPick = vi.fn(); + + render(); + await user.click(screen.getByRole('textbox')); + + expect(screen.getByText('Bach Suites Saigon')).toBeInTheDocument(); + expect(screen.getByText('Textile Exhibition')).toBeInTheDocument(); + await user.click(screen.getByText('Textile Exhibition')); + + expect(onPick).toHaveBeenLastCalledWith({ name: 'Textile Exhibition', lat: 10.8, lng: 106.66, address: null }); + await settle(); + expect(calls).toBe(0); + }); + + it('FE-PLANNER-LOCSEL-021: one or two typed characters narrow the trip places by name', async () => { + const user = userEvent.setup(); + + render(); + await user.type(screen.getByRole('textbox'), 'te'); + + expect(screen.getByText('Textile Exhibition')).toBeInTheDocument(); + // "Bach Suites" has a "te" inside "Suites", so it stays; a name without it goes. + expect(screen.getByText('Bach Suites Saigon')).toBeInTheDocument(); + await user.type(screen.getByRole('textbox'), '{backspace}{backspace}ex'); + expect(screen.getByText('Textile Exhibition')).toBeInTheDocument(); + expect(screen.queryByText('Bach Suites Saigon')).not.toBeInTheDocument(); + }); + + it('FE-PLANNER-LOCSEL-022: from the third character on the map search answers instead of the trip places', async () => { + const user = userEvent.setup(); + server.use(searchRoute([GARE])); + + render(); + await user.type(screen.getByRole('textbox'), 'Gare'); + + expect(await screen.findByText('Gare du Nord')).toBeInTheDocument(); + expect(screen.queryByText('Bach Suites Saigon')).not.toBeInTheDocument(); + }); + + it('FE-PLANNER-LOCSEL-023: a place already picked does not reopen the trip places when the field is focused', async () => { + const user = userEvent.setup(); + + render(); + await user.click(screen.getByRole('textbox')); + + expect(screen.queryByText('Bach Suites Saigon')).not.toBeInTheDocument(); + }); + + it('FE-PLANNER-LOCSEL-024: shortening the query after moving down the search results picks nothing stale', async () => { + const user = userEvent.setup(); + server.use(searchRoute([GARE, LYON, { ...GARE, name: 'Gare de l Est', osm_id: 'n3' }])); + const onPick = vi.fn(); + + render(); + const input = screen.getByRole('textbox'); + await user.type(input, 'Gare'); + await screen.findByText('Gare de Lyon'); + await user.keyboard('{ArrowDown}{ArrowDown}{ArrowDown}'); + + // Back under three characters: the trip places are shown and nothing is highlighted, + // so Enter does nothing rather than reaching for the third row of the old list. + await user.type(input, '{backspace}{backspace}{backspace}'); + await user.keyboard('{Enter}'); + expect(onPick).not.toHaveBeenCalledWith(expect.objectContaining({ lat: expect.any(Number) })); + + await user.keyboard('{ArrowDown}{Enter}'); + expect(onPick).toHaveBeenLastCalledWith(expect.objectContaining({ name: 'Bach Suites Saigon' })); + }); }); diff --git a/client/src/components/Planner/LocationSelect.tsx b/client/src/components/Planner/LocationSelect.tsx index c0985b26c..4fc08c450 100644 --- a/client/src/components/Planner/LocationSelect.tsx +++ b/client/src/components/Planner/LocationSelect.tsx @@ -16,7 +16,12 @@ interface Props { onChange: (loc: LocationPoint | null) => void placeholder?: string style?: React.CSSProperties - places?: { name: string; lat: number; lng: number }[] + /** + * The trip's own places, offered while the field is empty or holds fewer than three + * characters, the way the transit search offers them (#2468). Typing narrows them by + * name; from the third character on the map search answers instead. + */ + places?: LocationPoint[] } export default function LocationSelect({ value, onChange, placeholder, style, places }: Props) { @@ -47,7 +52,7 @@ export default function LocationSelect({ value, onChange, placeholder, style, pl if (debounceRef.current) clearTimeout(debounceRef.current) const trimmed = query.trim() if (trimmed.length < 3 || (value && trimmed === value.name)) { - setResults(trimmed.length < 3 && places ? places : []) + setResults([]) return } debounceRef.current = setTimeout(async () => { @@ -82,11 +87,20 @@ export default function LocationSelect({ value, onChange, placeholder, style, pl setResults([]) } + // The trip's places stand in for the search below three characters. Derived rather + // than written into `results`, so a search answer that lands after the field was + // shortened cannot replace them, and a place already picked does not reopen them. + const typed = query.trim() + const showPicks = !value && typed.length < 3 && !!places?.length + const rows = showPicks + ? (places ?? []).filter(p => !typed || p.name.toLowerCase().includes(typed.toLowerCase())) + : results + const onKey = (e: React.KeyboardEvent) => { - if (!open || results.length === 0) return - if (e.key === 'ArrowDown') { e.preventDefault(); setHighlight(h => Math.min(h + 1, results.length - 1)) } + if (!open || rows.length === 0) return + if (e.key === 'ArrowDown') { e.preventDefault(); setHighlight(h => Math.min(h + 1, rows.length - 1)) } else if (e.key === 'ArrowUp') { e.preventDefault(); setHighlight(h => Math.max(h - 1, 0)) } - else if (e.key === 'Enter' && highlight >= 0) { e.preventDefault(); pick(results[highlight]) } + else if (e.key === 'Enter' && rows[highlight]) { e.preventDefault(); pick(rows[highlight]) } else if (e.key === 'Escape') setOpen(false) } @@ -98,7 +112,7 @@ export default function LocationSelect({ value, onChange, placeholder, style, pl type="text" value={query} placeholder={placeholder ?? t('reservations.searchLocation')} - onChange={(e) => { setQuery(e.target.value); setOpen(true); if (value) onChange(null) }} + onChange={(e) => { setQuery(e.target.value); setOpen(true); setHighlight(-1); if (value) onChange(null) }} onFocus={() => setOpen(true)} onKeyDown={onKey} className="bg-transparent text-content" @@ -111,14 +125,14 @@ export default function LocationSelect({ value, onChange, placeholder, style, pl )} - {open && (loading || results.length > 0) && ( + {open && ((loading && !showPicks) || rows.length > 0) && (
- {loading && results.length === 0 && ( + {loading && !showPicks && rows.length === 0 && (
{t('common.loading')}
)} - {results.map((r, i) => ( + {rows.map((r, i) => (
diff --git a/client/src/components/Planner/locationPicks.test.ts b/client/src/components/Planner/locationPicks.test.ts new file mode 100644 index 000000000..5fb864ddd --- /dev/null +++ b/client/src/components/Planner/locationPicks.test.ts @@ -0,0 +1,32 @@ +// FE-PLANNER-LOCPICKS-001 to FE-PLANNER-LOCPICKS-003 +import { toLocationPicks } from './locationPicks'; + +describe('toLocationPicks', () => { + it('FE-PLANNER-LOCPICKS-001: a trip place becomes a pick with its name, coordinates and address', () => { + expect(toLocationPicks([{ name: ' Bach Suites Saigon ', lat: '10.78', lng: 106.7, address: 'District 1' }])).toEqual([ + { name: 'Bach Suites Saigon', lat: 10.78, lng: 106.7, address: 'District 1' }, + ]); + }); + + it('FE-PLANNER-LOCPICKS-002: a place without a name or a real coordinate is not offered, never as 0,0', () => { + expect(toLocationPicks([ + { name: 'No coordinates', lat: null, lng: null }, + { name: 'Half a coordinate', lat: 10, lng: null }, + { name: 'Blank coordinate', lat: '', lng: '' }, + { name: 'Not a number', lat: 'north', lng: 106.7 }, + { name: ' ', lat: 10, lng: 106 }, + { name: null, lat: 10, lng: 106 }, + ])).toEqual([]); + expect(toLocationPicks(undefined)).toEqual([]); + }); + + it('FE-PLANNER-LOCPICKS-003: the same place added twice is offered once, a namesake elsewhere stays', () => { + const picks = toLocationPicks([ + { name: 'Hotel', lat: 10, lng: 106 }, + { name: 'Hotel', lat: 10, lng: 106 }, + { name: 'Hotel', lat: 11, lng: 106 }, + ]); + expect(picks.map(p => `${p.name}@${p.lat}`)).toEqual(['Hotel@10', 'Hotel@11']); + expect(picks[0].address).toBeNull(); + }); +}); diff --git a/client/src/components/Planner/locationPicks.ts b/client/src/components/Planner/locationPicks.ts new file mode 100644 index 000000000..0070a2057 --- /dev/null +++ b/client/src/components/Planner/locationPicks.ts @@ -0,0 +1,33 @@ +import type { LocationPoint } from './LocationSelect' + +/** A trip place as far as a location field can offer it. */ +interface PlaceLike { + name?: string | null + lat?: number | string | null + lng?: number | string | null + address?: string | null +} + +/** + * The trip's places as ready-made picks for a location field. + * + * Only a place with a name and a real coordinate is offered. A place without one would + * be picked as 0,0 (`Number(null)` is a finite 0), which puts the endpoint of a booking + * in the Gulf of Guinea. The same place added twice is offered once. + */ +export function toLocationPicks(places: readonly PlaceLike[] | null | undefined): LocationPoint[] { + const picks: LocationPoint[] = [] + const seen = new Set() + for (const place of places ?? []) { + const name = place.name?.trim() + if (!name || place.lat == null || place.lng == null || place.lat === '' || place.lng === '') continue + const lat = Number(place.lat) + const lng = Number(place.lng) + if (!Number.isFinite(lat) || !Number.isFinite(lng)) continue + const key = `${name}:${lat}:${lng}` + if (seen.has(key)) continue + seen.add(key) + picks.push({ name, lat, lng, address: place.address ?? null }) + } + return picks +} diff --git a/client/src/mobile/screens/trip/sheets/MTransportFormSheet.tsx b/client/src/mobile/screens/trip/sheets/MTransportFormSheet.tsx index 8644d3244..9dd067cf5 100644 --- a/client/src/mobile/screens/trip/sheets/MTransportFormSheet.tsx +++ b/client/src/mobile/screens/trip/sheets/MTransportFormSheet.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react' +import { useEffect, useMemo, useRef, useState } from 'react' import { Bike, Bus, Car, CarTaxiFront, Check, ChevronDown, ChevronUp, Plane, Plus, Route, Sailboat, Ship, Train, TrainFront, TramFront, Trash2, X } from 'lucide-react' import MSheet from '../../../components/MSheet' import { useAddonStore } from '../../../../store/addonStore' @@ -11,6 +11,7 @@ import CustomTimePicker from '../../../../components/shared/CustomTimePicker' import { BookingCodeInput } from '../../../../components/shared/BookingCode' import AirportSelect, { type Airport } from '../../../../components/Planner/AirportSelect' import LocationSelect, { type LocationPoint } from '../../../../components/Planner/LocationSelect' +import { toLocationPicks } from '../../../../components/Planner/locationPicks' import TransitSearchPanel from '../../../../components/Planner/TransitSearchPanel' import { Eyebrow, FIELD_AREA_CLS, FIELD_CLS, FormSheetFooter, FormSheetHeader } from './PlSheetChrome' import PlFileAttach from './PlFileAttach' @@ -158,6 +159,8 @@ export default function MTransportFormSheet({ planner, onOpenExpense }: MTranspo const isBudgetEnabled = useAddonStore(s => s.isEnabled('budget')) const tripHasDates = Boolean(trip?.start_date && trip?.end_date) + // The trip's places, offered by every location field of the manual tab (#2468). + const locationPicks = useMemo(() => toLocationPicks(places), [places]) const [form, setForm] = useState({ ...EMPTY }) const [automated, setAutomated] = useState(false) @@ -850,7 +853,7 @@ export default function MTransportFormSheet({ planner, onOpenExpense }: MTranspo
{roleLabel}
- updateWp({ location: l || null })} /> + updateWp({ location: l || null })} places={locationPicks} />
{!isFirst && !isLast && (
diff --git a/client/tests/unit/mobile/trip/MTransportFormSheet.test.tsx b/client/tests/unit/mobile/trip/MTransportFormSheet.test.tsx index af6a9002b..80bdb9e1d 100644 --- a/client/tests/unit/mobile/trip/MTransportFormSheet.test.tsx +++ b/client/tests/unit/mobile/trip/MTransportFormSheet.test.tsx @@ -65,9 +65,10 @@ vi.mock('../../../../src/components/Planner/AirportSelect', () => ({ })) vi.mock('../../../../src/components/Planner/LocationSelect', () => ({ - default: ({ value, onChange }: { value: { name: string } | null; onChange: (l: unknown) => void }) => ( + default: ({ value, onChange, places }: { value: { name: string } | null; onChange: (l: unknown) => void; places?: { name: string }[] }) => ( p.name).join('|')} value={value?.name ?? ''} onChange={e => onChange(LOCATIONS[e.target.value] ?? null)} /> @@ -1021,4 +1022,19 @@ describe('MTransportFormSheet', () => { expect(payload.confirmation_number).toBe('BOOK1') }) }) + + it('FE-MOB-TRFRM-053: the manual From and To fields offer the trip places that have a location, each once (#2468)', () => { + renderSheet(makePlanner({ + places: [ + { id: 1, name: 'Fushimi Inari', lat: 34.97, lng: 135.77 }, + { id: 2, name: 'No pin yet', lat: null, lng: null }, + { id: 3, name: 'Fushimi Inari', lat: 34.97, lng: 135.77 }, + ], + })) + fireEvent.click(screen.getByRole('button', { name: 'reservations.type.bus' })) + + const fields = locationInputs() + expect(fields).toHaveLength(2) + for (const field of fields) expect(field).toHaveAttribute('data-picks', 'Fushimi Inari') + }) })