diff --git a/client/src/components/Planner/PlaceFormModal.helpers.ts b/client/src/components/Planner/PlaceFormModal.helpers.ts index fea2de328..28861ecff 100644 --- a/client/src/components/Planner/PlaceFormModal.helpers.ts +++ b/client/src/components/Planner/PlaceFormModal.helpers.ts @@ -53,3 +53,61 @@ export const DEFAULT_FORM: PlaceFormData = { transport_mode: 'walking', website: '', } + +/** + * The fields a maps result owns. Everything else in the form belongs to the + * user and is never touched by picking a place. + */ +export const RESULT_FIELDS = [ + 'name', + 'address', + 'lat', + 'lng', + 'google_place_id', + 'google_ftid', + 'osm_id', + 'website', + 'phone', +] as const + +export type ResultField = (typeof RESULT_FIELDS)[number] + +/** + * Folds a picked search result into the form. + * + * The obvious version is `result.x || prev.x`, and it has a bug that is easy to + * miss and hard to spot as a user: that expression cannot tell "the user typed + * this" from "the previous search result wrote this". Pick Hamburg Airport, + * then pick Berlin Hauptbahnhof — which has no website in OpenStreetMap — and + * the airport's website is still sitting in the field. Save it and the station + * now links to an airport. + * + * So the caller tracks which fields it filled in itself. A field the last pick + * wrote belongs to the last place and is cleared when the new one says nothing + * about it; a field the user typed survives untouched. `autoFilled` is mutated + * in place — it is the caller's record of what it owns. + */ +export function mergeResult( + prev: PlaceFormData, + result: Record, + autoFilled: Set, +): PlaceFormData { + const next = { ...prev } as PlaceFormData & Record + + for (const field of RESULT_FIELDS) { + const raw = result[field] + const value = raw == null ? '' : String(raw) + + if (value) { + next[field] = value + autoFilled.add(field) + } else if (autoFilled.has(field)) { + // Belonged to the place that is no longer selected. + next[field] = '' + autoFilled.delete(field) + } + // Otherwise the user put it there; leave it alone. + } + + return next +} diff --git a/client/src/components/Planner/PlaceFormModal.test.tsx b/client/src/components/Planner/PlaceFormModal.test.tsx index d21d1d83b..04e225f39 100644 --- a/client/src/components/Planner/PlaceFormModal.test.tsx +++ b/client/src/components/Planner/PlaceFormModal.test.tsx @@ -1372,4 +1372,99 @@ describe('PlaceFormModal remaining branches', () => { expect(warning).toHaveTextContent('Spanning Event'); expect(warning).not.toHaveTextContent('Earlier Event'); }); + + /** + * FE-PLANNER-PLACEFORM-063..068 — what a second pick may overwrite. + * + * Search Hamburg Airport, then Berlin Hauptbahnhof, and the airport's website + * was still in the field: `result.website || prev.website` cannot tell "the + * user typed this" from "the last result wrote this". Saving then gave the + * station the airport's website. + */ + describe('picking a second place', () => { + const searchFor = async (user: ReturnType, place: Record) => { + server.use(http.post('/api/maps/search', () => HttpResponse.json({ places: [place] }))); + const searchInput = screen.getByPlaceholderText('Search places...'); + await user.clear(searchInput); + await user.type(searchInput, String(place.name)); + const searchRow = searchInput.closest('.flex') as HTMLElement; + await user.click(within(searchRow).getByRole('button')); + await user.click(await screen.findByText(String(place.name))); + }; + + const AIRPORT = { + name: 'Hamburg Airport', + address: 'Hamburg', + lat: '53.6323', + lng: '10.0067', + website: 'https://www.hamburg-airport.de/', + }; + const STATION = { name: 'Berlin Hauptbahnhof', address: 'Berlin', lat: '52.525', lng: '13.3694' }; + + const websiteInput = () => screen.getByPlaceholderText('https://...') as HTMLInputElement; + + it('FE-PLANNER-PLACEFORM-063: a place with no website clears the previous one', async () => { + const user = userEvent.setup(); + render(); + + await searchFor(user, AIRPORT); + expect(websiteInput().value).toBe('https://www.hamburg-airport.de/'); + + await searchFor(user, STATION); + expect(websiteInput().value).toBe(''); + }); + + it('FE-PLANNER-PLACEFORM-064: a website the user typed survives the next pick', async () => { + const user = userEvent.setup(); + render(); + + await user.type(websiteInput(), 'https://mine.example'); + await searchFor(user, STATION); + + expect(websiteInput().value).toBe('https://mine.example'); + }); + + it('FE-PLANNER-PLACEFORM-065: a picked value the user then edited also survives', async () => { + const user = userEvent.setup(); + render(); + + await searchFor(user, AIRPORT); + await user.clear(websiteInput()); + await user.type(websiteInput(), 'https://corrected.example'); + await searchFor(user, STATION); + + expect(websiteInput().value).toBe('https://corrected.example'); + }); + + it('FE-PLANNER-PLACEFORM-066: the ids of the previous place do not travel with the new one', async () => { + const user = userEvent.setup(); + const onSave = vi.fn(); + render(); + + await searchFor(user, { ...AIRPORT, google_place_id: 'ChIJham' }); + await searchFor(user, { ...STATION, osm_id: 'relation:3600565' }); + await user.click(screen.getByRole('button', { name: /^Add$/i })); + + await waitFor(() => expect(onSave).toHaveBeenCalled()); + const payload = onSave.mock.calls[0][0]; + expect(payload.osm_id).toBe('relation:3600565'); + expect(payload.google_place_id).toBeFalsy(); + }); + + it('FE-PLANNER-PLACEFORM-067: editing a saved place keeps its stored website', async () => { + // The dangerous direction: the form was filled from the database, so a + // search result that says nothing about a field must not empty it. + const user = userEvent.setup(); + render( + , + ); + expect(websiteInput().value).toBe('https://stored.example'); + + await searchFor(user, STATION); + expect(websiteInput().value).toBe('https://stored.example'); + }); + }); }); diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index 7e45b2613..eb9abecad 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -13,7 +13,7 @@ import { useToast } from '../shared/Toast' import { Search, Paperclip, X, AlertTriangle, Loader2, Plus } from 'lucide-react' import { useTranslation } from '../../i18n' import CustomTimePicker from '../shared/CustomTimePicker' -import { DEFAULT_FORM, isGoogleMapsUrl, type PlaceFormData } from './PlaceFormModal.helpers' +import { DEFAULT_FORM, isGoogleMapsUrl, mergeResult, type PlaceFormData, type ResultField } from './PlaceFormModal.helpers' import { getApiErrorMessage } from '../../utils/apiError' import type { Place, Category, Assignment } from '../../types' import { NumericInput } from '../shared/NumericInput' @@ -88,6 +88,10 @@ function usePlaceFormModal(props: PlaceFormModalProps) { const [duplicateWarning, setDuplicateWarning] = useState(null) // What the detail column is describing. Null until the user picks a result. const [detailsSelection, setDetailsSelection] = useState(null) + // Which fields the last picked search result wrote. Anything in here belongs + // to that place and goes when another is picked; anything outside it is the + // user's and survives. See mergeResult. + const autoFilledRef = useRef>(new Set()) const [pendingFiles, setPendingFiles] = useState([]) const fileRef = useRef(null) const searchInputRef = useRef(null) @@ -138,6 +142,18 @@ function usePlaceFormModal(props: PlaceFormModalProps) { } else { setForm(DEFAULT_FORM) } + // A fresh dialog owns nothing yet. The exception is a POI tapped on the map + // or a right-click place: those arrive prefilled from a place, so the same + // fields belong to it and a later search pick may clear them. An existing + // place being edited is the opposite — everything on that form came out of + // the database and none of it is a search result's to drop. + autoFilledRef.current = new Set( + !place && prefillCoords + ? (['name', 'address', 'lat', 'lng', 'website', 'phone', 'osm_id'] as ResultField[]).filter( + (field) => !!prefillCoords[field as keyof typeof prefillCoords], + ) + : [], + ) setPendingFiles([]) setDuplicateWarning(null) // The column follows whatever the dialog was opened with, not only a search @@ -246,6 +262,8 @@ function usePlaceFormModal(props: PlaceFormModalProps) { }, [mapsSearch, fetchSuggestions]) const handleChange = (field: string, value: string) => { + // Typed by hand, so the next pick must not clear it. + autoFilledRef.current.delete(field as ResultField) setForm(prev => ({ ...prev, [field]: value })) } @@ -282,18 +300,7 @@ function usePlaceFormModal(props: PlaceFormModalProps) { } const handleSelectMapsResult = (result) => { - setForm(prev => ({ - ...prev, - name: result.name || prev.name, - address: result.address || prev.address, - lat: result.lat || prev.lat, - lng: result.lng || prev.lng, - google_place_id: result.google_place_id || prev.google_place_id, - google_ftid: result.google_ftid || prev.google_ftid, - osm_id: result.osm_id || prev.osm_id, - website: result.website || prev.website, - phone: result.phone || prev.phone, - })) + setForm(prev => mergeResult(prev, result, autoFilledRef.current)) // The one point every pick flows through, so the detail column hangs here. // A new pick drops whatever hero image belonged to the previous place. const lat = Number(result.lat)