Stop a picked place leaving its website on the next one

This commit is contained in:
Maurice
2026-08-10 10:25:31 +02:00
committed by Maurice
parent 880e258438
commit e1a52ba43c
3 changed files with 173 additions and 13 deletions
@@ -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<string, unknown>,
autoFilled: Set<ResultField>,
): PlaceFormData {
const next = { ...prev } as PlaceFormData & Record<string, string | undefined>
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
}
@@ -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<typeof userEvent.setup>, place: Record<string, unknown>) => {
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(<PlaceFormModal {...defaultProps} />);
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(<PlaceFormModal {...defaultProps} />);
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(<PlaceFormModal {...defaultProps} />);
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(<PlaceFormModal {...defaultProps} onSave={onSave} />);
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(
<PlaceFormModal
{...defaultProps}
place={{ id: 7, name: 'Old', website: 'https://stored.example', lat: 1, lng: 2 } as never}
/>,
);
expect(websiteInput().value).toBe('https://stored.example');
await searchFor(user, STATION);
expect(websiteInput().value).toBe('https://stored.example');
});
});
});
@@ -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<string | null>(null)
// What the detail column is describing. Null until the user picks a result.
const [detailsSelection, setDetailsSelection] = useState<PlaceDetailsSelection | null>(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<Set<ResultField>>(new Set())
const [pendingFiles, setPendingFiles] = useState([])
const fileRef = useRef(null)
const searchInputRef = useRef<HTMLInputElement>(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)