diff --git a/client/src/components/Budget/CostsPanel.test.tsx b/client/src/components/Budget/CostsPanel.test.tsx index f7d5b5ab5..0c099d0be 100644 --- a/client/src/components/Budget/CostsPanel.test.tsx +++ b/client/src/components/Budget/CostsPanel.test.tsx @@ -744,7 +744,7 @@ describe('CostsPanel — settlements in the ledger', () => { type Flow = { from: { user_id: number; username: string }; to: { user_id: number; username: string }; amount: number } type Balance = { user_id: number; username: string; avatar_url: string | null; balance: number } -type Payment = { id: number; from_user_id: number; to_user_id: number; amount: number; currency?: string | null; created_at?: string; settled_at?: string | null } +type Payment = { id: number; from_user_id: number; to_user_id: number; amount: number; currency?: string | null; exchange_rate?: number; created_at?: string; settled_at?: string | null } // `members` here is the wire shape the panel reads; `paid` is only set by the server. type MemberFixture = { user_id: number; username?: string; amount?: number; paid?: number } diff --git a/client/src/components/Map/RouteCalculator.test.ts b/client/src/components/Map/RouteCalculator.test.ts index e7348e037..dc92b877d 100644 --- a/client/src/components/Map/RouteCalculator.test.ts +++ b/client/src/components/Map/RouteCalculator.test.ts @@ -975,3 +975,77 @@ describe('calculateAlternatives', () => { .rejects.toMatchObject({ name: 'RoutingRefusedError', status: 429 }) }) }) + +// ── turning round at a stop ──────────────────────────────────────────────────── + +/** + * Gouffre de Padirac snaps onto `Route du Puits au Salvage`, which leads nowhere else. + * OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked, and it + * refuses the ENTIRE request when it cannot obey, so a road-trip day lost every leg it + * had over one cave car park, while each of its pairs routed perfectly on its own. + */ +describe('turning round at a stop', () => { + const askedFor = async ( + call: (points: { lat: number; lng: number }[]) => Promise, + count: number, + ): Promise => { + let asked = '' + server.use(http.get(`${FOSSGIS.driving}/:coords`, ({ request }) => { + asked = request.url + return HttpResponse.json(buildOsrmRouteResponse()) + })) + await call(freshWaypoints(count)) + return asked + } + + it('FE-COMP-ROUTECALCULATOR-055: a drive that stops on the way may turn round where it stopped', async () => { + expect(await askedFor(p => calculateRoute(p), 3)).toContain('continue_straight=false') + expect(await askedFor(p => calculateRouteWithLegs(p), 3)).toContain('continue_straight=false') + expect(await askedFor(p => calculateSegments(p), 3)).toContain('continue_straight=false') + }) + + it('FE-COMP-ROUTECALCULATOR-056: two points have no middle, so the request is the one it always was', async () => { + expect(await askedFor(p => calculateRoute(p), 2)).not.toContain('continue_straight') + expect(await askedFor(p => calculateRouteWithLegs(p), 2)).not.toContain('continue_straight') + expect(await askedFor(p => calculateSegments(p), 2)).not.toContain('continue_straight') + }) + + it('FE-COMP-ROUTECALCULATOR-057: a router that has never heard of the permission is asked without it, once and then always', async () => { + // OSRM refuses an unknown parameter outright instead of ignoring it, and says which + // it is refusing: `InvalidQuery` names the query string, `NoRoute` names the road. + // Without this a router that predates the parameter would refuse every drive with a + // stop in the middle, which is a far worse fault than the one it fixes. + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: 'https://old-osrm.example.org' } })) + const asked: string[] = [] + server.use(http.get('https://old-osrm.example.org/route/v1/driving/:coords', ({ request }) => { + asked.push(request.url) + return new URL(request.url).searchParams.has('continue_straight') + ? HttpResponse.json({ code: 'InvalidQuery', message: 'Query string malformed' }, { status: 400 }) + : HttpResponse.json(buildOsrmRouteResponse()) + })) + + const first = await calculateRoute(freshWaypoints(3)) + expect(first.coordinates).toHaveLength(2) + expect(asked).toHaveLength(2) + expect(asked[1]).not.toContain('continue_straight') + + // Remembered per host, so the second drive costs one request rather than two. + await calculateRoute(freshWaypoints(3)) + expect(asked).toHaveLength(3) + expect(asked[2]).not.toContain('continue_straight') + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) + }) + + it('FE-COMP-ROUTECALCULATOR-058: a refusal about the road is still a refusal, not a second request', async () => { + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: 'https://noroute-osrm.example.org' } })) + let calls = 0 + server.use(http.get('https://noroute-osrm.example.org/route/v1/driving/:coords', () => { + calls += 1 + return HttpResponse.json({ code: 'NoRoute', message: 'No route found between points' }, { status: 400 }) + })) + + await expect(calculateRoute(freshWaypoints(3))).rejects.toThrow() + expect(calls).toBe(1) + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) + }) +}) diff --git a/client/src/components/Map/RouteCalculator.ts b/client/src/components/Map/RouteCalculator.ts index 3638696c5..d201ee2de 100644 --- a/client/src/components/Map/RouteCalculator.ts +++ b/client/src/components/Map/RouteCalculator.ts @@ -53,6 +53,31 @@ function routeBaseFor(profile: 'driving' | 'walking' | 'cycling'): string { return `${withoutTrailingSlashes(configured)}/route/v1/${OSRM_PROFILE_PATH[profile]}` } +/** + * Permission to turn around at a stop, for requests that have stops to turn around at. + * + * OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked otherwise, + * and a refusal is not local: the whole request comes back `400 NoRoute`, every leg of it, + * however many were fine. A stop on a dead end is enough. Gouffre de Padirac snaps onto + * `Route du Puits au Salvage`, which goes nowhere else, so a day that visits it in the + * middle lost all its legs at once while each of its pairs routed perfectly on its own. + * That is also why the fault looked like it depended on how the stops were spread across + * days: a pair on a day boundary is asked for on its own, and a request with nothing in + * the middle has nothing to refuse. + * + * Correct as well as convenient here. TREK's waypoints are places somebody stops at, not + * shape hints, and the way out of a dead end IS the way back in, and the constraint was + * buying a detour even where it did not refuse outright. + * + * Empty for two waypoints, which have no middle, so the many two-point callers + * (alternatives, the day connectors, booking geometry) keep the URL they had. + */ +const U_TURN_PARAM = '&continue_straight=false' + +function uTurnParam(waypoints: readonly Waypoint[]): string { + return waypoints.length > 2 ? U_TURN_PARAM : '' +} + /** * `fetch`, with the request counted. * @@ -79,10 +104,16 @@ async function routedFetch( let km = 0 for (let i = 1; i < waypoints.length; i++) km += haversineKm(waypoints[i - 1], waypoints[i]) const sample = { profile, surface: kind, selfHosted, waypoints: waypoints.length, km } + const asked = withoutUTurnIfRefused(url) try { - const response = await fetch(url, { signal }) + const response = await fetch(asked, { signal }) countRoute({ ...sample, failed: !response.ok }) - return response + if (response.ok || !asked.includes(U_TURN_PARAM)) return response + const plain = await droppedUTurn(asked, response) + if (!plain) return response + const second = await fetch(plain, { signal }) + countRoute({ ...sample, failed: !second.ok }) + return second } catch (err) { if (!(err instanceof DOMException && err.name === 'AbortError')) { countRoute({ ...sample, failed: true }) @@ -91,6 +122,49 @@ async function routedFetch( } } +/** + * Hosts that answered `InvalidQuery` to `continue_straight`, by origin. + * + * OSRM refuses an unknown query parameter outright rather than ignoring it, so a router + * that has never heard of this one would refuse EVERY request with a stop in the middle, + * which is a far worse fault than the one it fixes. `osrm-routed` has understood it + * since 5.6, so this is about something else answering under `routing_base_url`, and the + * first real request is the probe: there is no separate one. + * + * By origin rather than per profile, because a router either knows the parameter or does + * not; and in a Set rather than a setting, so pointing the instance at a router that does + * know it starts asking again without a reload. Same shape and the same reasoning as + * `excludeUnsupported` below. + */ +const uTurnUnsupported = new Set() + +const originOf = (url: string): string => { + try { return new URL(url).origin } catch { return url } +} + +/** The url as it should go out, with the permission dropped for a host that refused it. */ +function withoutUTurnIfRefused(url: string): string { + if (!url.includes(U_TURN_PARAM)) return url + return uTurnUnsupported.has(originOf(url)) ? url.replace(U_TURN_PARAM, '') : url +} + +/** + * The same url without the u-turn permission, once it is clear the host objects to the + * PARAMETER and not to the route. Null when the refusal was about the route itself. + * + * OSRM says which it is: `InvalidQuery` names the query string, `NoRoute` names the road. + * Reading the body is safe here because this response is about to be thrown away either + * way, and it only happens on a 400 for a request that carried the parameter. + */ +async function droppedUTurn(url: string, response: Response): Promise { + if (response.status !== 400) return null + let code: unknown + try { code = (await response.clone().json())?.code } catch { return null } + if (code !== 'InvalidQuery') return null + uTurnUnsupported.add(originOf(url)) + return url.replace(U_TURN_PARAM, '') +} + /** * A routing request the host refused, as opposed to one it answered with "no route". * @@ -153,7 +227,7 @@ export async function calculateRoute( } const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${routeBaseFor(profile)}/${coords}?overview=full&geometries=geojson&steps=false` + const url = `${routeBaseFor(profile)}/${coords}?overview=full&geometries=geojson&steps=false${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'route', profile, waypoints) if (!response.ok) { @@ -360,7 +434,7 @@ export async function calculateSegments( if (!waypoints || waypoints.length < 2) return [] const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${routeBaseFor('driving')}/${coords}?overview=false&geometries=geojson&steps=false&annotations=distance,duration` + const url = `${routeBaseFor('driving')}/${coords}?overview=false&geometries=geojson&steps=false&annotations=distance,duration${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'segments', 'driving', waypoints) if (!response.ok) throw new RoutingRefusedError(response.status, retryAfterMs(response)) @@ -488,7 +562,7 @@ export async function calculateRouteWithLegs( // (plugins name their own modes), which no comparison narrows to the three OSRM knows. const osrmProfile: 'driving' | 'walking' | 'cycling' = profile === 'walking' ? 'walking' : profile === 'cycling' ? 'cycling' : 'driving' - const url = `${routeBaseFor(osrmProfile)}/${coords}?overview=full&geometries=geojson&annotations=distance,duration` + const url = `${routeBaseFor(osrmProfile)}/${coords}?overview=full&geometries=geojson&annotations=distance,duration${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'legs', osrmProfile, waypoints) if (!response.ok) throw new RoutingRefusedError(response.status, retryAfterMs(response)) diff --git a/client/src/components/Planner/PlaceFormModal.test.tsx b/client/src/components/Planner/PlaceFormModal.test.tsx index acf47eb97..32ac8a7aa 100644 --- a/client/src/components/Planner/PlaceFormModal.test.tsx +++ b/client/src/components/Planner/PlaceFormModal.test.tsx @@ -1772,6 +1772,16 @@ describe('PlaceFormModal as a road trip service stop', () => { expect(screen.getAllByText('Add as a stop').length).toBeGreaterThan(0); }); + it('FE-PLANNER-PLACEFORM-085: it opens on the kind the corridor was looking for, dwell and all', () => { + // The panel seeds itself to charging on an electric car, and a form opening on a + // fuel pump under a picker that says Charging is the fault this carries the answer to. + render(); + + expect(screen.getByRole('button', { name: 'Charging' })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: /30 min/ })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: 'Fuel' })).toHaveAttribute('aria-pressed', 'false'); + }); + it('FE-PLANNER-PLACEFORM-076: picking a kind also picks how long that kind takes', async () => { const onSave = vi.fn().mockResolvedValue({ id: 9 }); render(); diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index 901547203..eb0979136 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -254,10 +254,15 @@ function usePlaceFormModal(props: PlaceFormModalProps) { // path exists to avoid. Read out of the closure rather than watched, because the // mode is fixed for the life of one opening and a rebuilt drive must not reset a // half-filled form. + // What the corridor panel was looking for when the button was pressed, because + // that is the traveller's own answer to what they are adding; only a panel with + // nothing selected falls back to the constant. The dwell follows the kind, the + // same way picking one by hand moves it. + const kind = serviceStop.defaultKind ?? DEFAULT_SERVICE_KIND setForm({ ...DEFAULT_FORM, - stop_type: DEFAULT_SERVICE_KIND, - duration_minutes: STOP_KIND_BY_KEY[DEFAULT_SERVICE_KIND].defaultMinutes, + stop_type: kind, + duration_minutes: STOP_KIND_BY_KEY[kind].defaultMinutes, }) } else { setForm(DEFAULT_FORM) diff --git a/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx b/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx index fdf90b226..3129b6b45 100644 --- a/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx +++ b/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx @@ -441,6 +441,35 @@ describe('RoadtripCorridorPanel', () => { // nothing is added until a place has been found and saved in there. fireEvent.click(manual) expect(onAddManual).toHaveBeenCalledTimes(1) + // The kind, never the MouseEvent: the categories above the button are the answer to + // what is being added, and a form that ignored them opened every manual stop on fuel. + expect(onAddManual).toHaveBeenCalledWith('fuel') + }) + + it('FE-ROADTRIP-PANEL-035: the manual add opens on what the panel is looking for', () => { + const open = (categories: string[]): unknown => { + const onAddManual = vi.fn() + const { unmount } = wrap( + , + ) + fireEvent.click(searchRow()[1]) + unmount() + return onAddManual.mock.calls[0][0] + } + + // An electric car seeds the panel to charging, and this is the case the fault was + // reported on: the panel said Charging and the form opened on Fuel. + expect(open(['charging'])).toBe('charging') + expect(open(['restaurant'])).toBe('restaurant') + // Multi-select: the first the panel LISTS wins, rather than a kind nobody switched on. + expect(open(['sights', 'rest_area'])).toBe('rest_area') + // Nothing selected is no answer, and the form falls back on its own. + expect(open([])).toBeNull() }) it('FE-ROADTRIP-PANEL-026: with no drive on the trip there is nowhere to add one by hand', () => { diff --git a/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx b/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx index 505b5f82c..850044776 100644 --- a/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx +++ b/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx @@ -13,9 +13,10 @@ import CustomSelect from '../shared/CustomSelect' import RoadtripCategoryPicker from './RoadtripCategoryPicker' import { serviceColor } from './roadtripModel' import { alongLabel, offRouteLabel } from './corridorSearchModel' -import { CORRIDOR_CATEGORY_BY_KEY } from './stopKinds' +import { CORRIDOR_CATEGORY_BY_KEY, manualStopKindFor } from './stopKinds' import { FS } from './typeScale' import { CORRIDOR_CATEGORY_KEYS, CORRIDOR_SECTION_KM, CORRIDOR_WIDTHS_KM, type RoadtripCorridor } from './useRoadtripCorridor' +import type { RoadtripStopType } from '@trek/shared' import type { CorridorPoi } from './useCorridorPois' import type { RoadtripRoutes } from './useRoadtripRoutes' @@ -38,8 +39,12 @@ interface RoadtripCorridorPanelProps { /** * Opens the place form for a stop the search never found. Given only to somebody who * may add places, which is what keeps the button off a reader's panel. + * + * Carries the kind the panel is looking for, so the form opens on it rather than on a + * constant: the categories above the button are the traveller's own answer to "what am + * I adding", and ignoring them made every manual stop start life as a fuel stop. */ - onAddManual?: () => void + onAddManual?: (kind: RoadtripStopType | null) => void } /** @@ -476,7 +481,9 @@ export default function RoadtripCorridorPanel({ {onAddPoi && onAddManual && corridor.day ? (