diff --git a/client/src/components/Roadtrip/useRoadtripVias.ts b/client/src/components/Roadtrip/useRoadtripVias.ts index 4dc8c6f0c..b995ac08d 100644 --- a/client/src/components/Roadtrip/useRoadtripVias.ts +++ b/client/src/components/Roadtrip/useRoadtripVias.ts @@ -66,7 +66,7 @@ export interface RoadtripVias { /** Absent leaves the day's track alone, null clears it, an object records a new one. */ track?: { place_id: number; stray_km?: number | null } | null, ) => Promise - move: (dayId: number, id: number, lat: number, lng: number) => Promise + move: (dayId: number, id: number, lat: number, lng: number, afterOrderIndex?: number) => Promise remove: (dayId: number, id: number) => Promise /** * Correct a day's anchors after its stops changed shape. @@ -171,9 +171,16 @@ export function useRoadtripVias(tripId: number | string | null, active: boolean) await reload() }, [tripId, reload]) - const move = useCallback(async (dayId: number, id: number, lat: number, lng: number) => { + const move = useCallback(async (dayId: number, id: number, lat: number, lng: number, afterOrderIndex?: number) => { if (!tripId) return - await roadtripApi.moveVia(tripId, dayId, id, { lat, lng }) + // The anchor rides along when the caller worked out a new one: a via dragged past the + // stop it used to sit before belongs to the next leg now, and saying only where it is + // leaves it claiming the old one. + await roadtripApi.moveVia(tripId, dayId, id, { + lat, + lng, + ...(afterOrderIndex === undefined ? {} : { after_order_index: afterOrderIndex }), + }) await reload() }, [tripId, reload]) diff --git a/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx index e9df4a853..60ba14115 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx @@ -1060,13 +1060,65 @@ describe('useTripPlanner road trip: shaping the drive on the map', () => { const { result } = await renderRoadtrip() await act(async () => { await result.current.moveRoadtripVia(5, 3, 53.2, 11.7) }) - expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 53.2, 11.7) + expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 53.2, 11.7, 0) rt.vias.move.mockRejectedValue(new Error('stale')) await act(async () => { await result.current.moveRoadtripVia(5, 3, 53.2, 11.7) }) expect(toasts.some(t => t.type === 'error' && t.message === 'stale')).toBe(true) }) + it('FE-TP-ROAD-053: a via dragged past a stop is re-pinned to the leg it landed on', async () => { + // The bug this exists for. A drag used to send only the new coordinates, so a via + // pulled beyond the stop it used to precede kept claiming the earlier leg: the route + // ran out to the point and back before carrying on, which reads as the drag doing + // nothing at all. Three stops so there are two legs to land between. + seedTrip({ days: [buildDay({ id: 5, day_number: 1 })] }) + rt.corridor.day = { dayId: 5, dayNumber: 1 } + rt.routes.days = [{ + dayId: 5, + stops: [{ lat: 53.55, lng: 9.99 }, { lat: 53.85, lng: 11.45 }, { lat: 52.52, lng: 13.4 }], + geometry: [[53.55, 9.99], [53.87, 10.7], [53.85, 11.45], [53.87, 11.53], [52.52, 13.4]], + }] + const { result } = await renderRoadtrip() + + // Dropped on the first leg, then dragged past the middle stop onto the second. + await act(async () => { await result.current.addRoadtripVia(53.87, 10.7) }) + expect(rt.vias.add).toHaveBeenCalledWith(5, 0, 53.87, 10.7) + + await act(async () => { await result.current.moveRoadtripVia(5, 9, 53.87, 11.53) }) + expect(rt.vias.move).toHaveBeenCalledWith(5, 9, 53.87, 11.53, 1) + }) + + it('FE-TP-ROAD-054: a drag is measured against its own day, however close another one runs', async () => { + // Placing a via lets the nearest day win, which is right for a click on the map. + // A drag is not that: the via already belongs to a day, and handing it to a + // neighbouring day whose road happens to pass closer would make it vanish from the + // one it was dragged in. + twoRoutedDays() + const { result } = await renderRoadtrip() + + // Right on day 6's line, but dragged within day 5. + await act(async () => { await result.current.moveRoadtripVia(5, 3, 47.9, 12.3) }) + + expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 47.9, 12.3, expect.any(Number)) + // Day 6 never saw it. + expect(rt.vias.move).not.toHaveBeenCalledWith(6, expect.anything(), expect.anything(), expect.anything(), expect.anything()) + }) + + it('FE-TP-ROAD-055: a drag far off the road still moves, without an anchor to offer', async () => { + // No distance guard on a drag, unlike on a click: pulling a via well away from the + // current road is the whole point of the gesture. A day with no line to measure + // against simply sends no anchor, and the existing pin stays as it was. + seedTrip({ days: [buildDay({ id: 5, day_number: 1 })] }) + rt.corridor.day = { dayId: 5, dayNumber: 1 } + rt.routes.days = [{ dayId: 5, stops: [], geometry: [] }] + const { result } = await renderRoadtrip() + + await act(async () => { await result.current.moveRoadtripVia(5, 3, 41.9, 12.5) }) + + expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 41.9, 12.5, undefined) + }) + it('FE-TP-ROAD-052: removing a via lets the drive take the direct road again', async () => { twoRoutedDays() const { result } = await renderRoadtrip() diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index b180a6fc2..dce17731d 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -1148,10 +1148,19 @@ export function useTripPlanner() { * third stop" means the same thing everywhere. The day is the one whose line was hit, * found by trying each day's geometry and keeping the closest. */ - const addRoadtripVia = useCallback(async (lat: number, lng: number) => { - if (!can('day_edit', trip)) return + /** + * Which stop of which day a point belongs behind, measured along the drive. + * + * Shared by placing a via and by dragging one, because it is the same question both + * times and the answer has to be recomputed both times. A drag used to send only the + * new coordinates, so a via pulled past the stop it used to precede kept claiming the + * earlier leg: the route then ran out to the point and back before carrying on, which + * looks exactly like a drag that did nothing. + */ + const anchorFor = useCallback((lat: number, lng: number, onlyDayId?: number) => { let best: { dayId: number; afterIndex: number; offRouteKm: number } | null = null for (const day of roadtripRoutes.days) { + if (onlyDayId !== undefined && day.dayId !== onlyDayId) continue if (day.geometry.length < 2) continue const spine = day.geometry.map(([la, ln]) => ({ lat: la, lng: ln })) const hit = projectOntoRoute({ lat, lng }, spine) @@ -1162,6 +1171,12 @@ export function useTripPlanner() { const afterIndex = Math.max(0, insertIndexForAlong(stopsAlong, hit.alongKm) - 1) best = { dayId: day.dayId, afterIndex, offRouteKm: hit.offRouteKm } } + return best + }, [roadtripRoutes.days]) + + const addRoadtripVia = useCallback(async (lat: number, lng: number) => { + if (!can('day_edit', trip)) return + const best = anchorFor(lat, lng) // A click that landed on some other line is not a via anywhere. if (!best || best.offRouteKm > 2) return try { @@ -1169,17 +1184,25 @@ export function useTripPlanner() { } catch (err: unknown) { toast.error(err instanceof Error ? err.message : t('common.unknownError')) } - }, [roadtripRoutes.days, roadtripVias, can, trip, toast, t]) + }, [anchorFor, roadtripVias, can, trip, toast, t]) /** Dragging a via redraws the route through its new position. */ const moveRoadtripVia = useCallback(async (dayId: number, id: number, lat: number, lng: number) => { if (!can('day_edit', trip)) return + // Measured against this day only. A drag is a drag WITHIN a day: letting the nearest + // day win, the way placing one does, would hand the via to a neighbouring day whose + // road happens to pass closer, and it would vanish from the day it was dragged in. + // + // No distance guard either. Dragging a via well off the current road is the whole + // point of dragging it, and refusing that would be refusing the gesture; the anchor + // just says which leg gets bent, and the router answers the rest. + const anchor = anchorFor(lat, lng, dayId) try { - await roadtripVias.move(dayId, id, lat, lng) + await roadtripVias.move(dayId, id, lat, lng, anchor?.afterIndex) } catch (err: unknown) { toast.error(err instanceof Error ? err.message : t('common.unknownError')) } - }, [roadtripVias, can, trip, toast, t]) + }, [anchorFor, roadtripVias, can, trip, toast, t]) /** Removing a via lets the drive take the direct road again. */ const removeRoadtripVia = useCallback(async (dayId: number, id: number) => { diff --git a/server/src/nest/roadtrip/roadtrip.controller.ts b/server/src/nest/roadtrip/roadtrip.controller.ts index abe3e1363..6bfd54994 100644 --- a/server/src/nest/roadtrip/roadtrip.controller.ts +++ b/server/src/nest/roadtrip/roadtrip.controller.ts @@ -108,7 +108,7 @@ export class RoadtripController { @Body() body: RoadtripViaUpdateDto, ): { via: RoadtripVia } { this.requireDay(dayId, tripId); - const via = this.roadtrip.move(id, dayId, body.lat, body.lng); + const via = this.roadtrip.move(id, dayId, body.lat, body.lng, body.after_order_index); if (!via) throw new HttpException({ error: 'Via not found' }, 404); return { via }; } diff --git a/server/src/nest/roadtrip/roadtrip.service.ts b/server/src/nest/roadtrip/roadtrip.service.ts index 720f8b853..cd5ae43f1 100644 --- a/server/src/nest/roadtrip/roadtrip.service.ts +++ b/server/src/nest/roadtrip/roadtrip.service.ts @@ -154,14 +154,34 @@ export class RoadtripService { } /** Moving a via is the whole edit; where it sits in the chain does not change. */ - move(id: string | number, dayId: string | number, lat: number, lng: number): RoadtripVia | null { + move( + id: string | number, + dayId: string | number, + lat: number, + lng: number, + afterOrderIndex?: number, + ): RoadtripVia | null { const existing = this.db.get<{ id: number }>( 'SELECT id FROM roadtrip_vias WHERE id = ? AND day_id = ?', id, dayId, ); if (!existing) return null; - this.db.run('UPDATE roadtrip_vias SET lat = ?, lng = ? WHERE id = ?', lat, lng, id); + // The anchor moves with the point when the caller worked out a new one. It is not a + // property of the via but of where the via sits along the drive, so dragging one past + // a stop changes which leg it belongs to — and leaving it behind is what made the + // route run out to the point and back instead of bending through it. + if (afterOrderIndex === undefined) { + this.db.run('UPDATE roadtrip_vias SET lat = ?, lng = ? WHERE id = ?', lat, lng, id); + } else { + this.db.run( + 'UPDATE roadtrip_vias SET lat = ?, lng = ?, after_order_index = ? WHERE id = ?', + lat, + lng, + afterOrderIndex, + id, + ); + } return this.byId(Number(id)); } diff --git a/server/tests/unit/nest/roadtrip.controller.test.ts b/server/tests/unit/nest/roadtrip.controller.test.ts index aaed155f5..22b3cf385 100644 --- a/server/tests/unit/nest/roadtrip.controller.test.ts +++ b/server/tests/unit/nest/roadtrip.controller.test.ts @@ -160,7 +160,15 @@ describe('RoadtripController — writes', () => { it('ROADTRIP-CTL-012: moving a via reports where it went', () => { const s = svc(); expect(new RoadtripController(s).update('7', '4', '5', { lat: 54, lng: 11 })).toEqual({ via: VIA }); - expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11); + // Undefined anchor, explicitly: a drag that stayed between the same two stops sends + // no new one, and the service must leave the existing pin alone rather than clear it. + expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11, undefined); + }); + + it('ROADTRIP-CTL-015: a new anchor is passed on, so a via dragged past a stop is re-pinned', () => { + const s = svc(); + new RoadtripController(s).update('7', '4', '5', { lat: 54, lng: 11, after_order_index: 2 }); + expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11, 2); }); it('ROADTRIP-CTL-013: a via that is not on this day is a 404, not a silent no-op', () => { diff --git a/server/tests/unit/nest/roadtrip.service.test.ts b/server/tests/unit/nest/roadtrip.service.test.ts index dfa73c02c..e047f7c93 100644 --- a/server/tests/unit/nest/roadtrip.service.test.ts +++ b/server/tests/unit/nest/roadtrip.service.test.ts @@ -82,7 +82,10 @@ describe('RoadtripService', () => { expect(service.listForTrip(7).map(v => v.day_id)).toEqual([1, 2]); }); - it('ROADTRIP-SVC-005: moving a via changes where it is, not where it sits in the chain', () => { + it('ROADTRIP-SVC-005: a move with no new anchor leaves the chain alone', () => { + // The drag stayed between the same two stops, so there is nothing to re-pin. The + // sequence is never touched by a move either way: it orders the vias that share one + // anchor, and dragging one does not reorder its neighbours. const via = service.create(1, { after_order_index: 1, lat: 53, lng: 10 }); const moved = service.move(via.id, 1, 52.5, 11.5); @@ -90,6 +93,26 @@ describe('RoadtripService', () => { expect(moved).toMatchObject({ lat: 52.5, lng: 11.5, after_order_index: 1, sequence: via.sequence }); }); + it('ROADTRIP-SVC-007: a via dragged past a stop is re-pinned to the leg it landed on', () => { + // The bug this exists for: a drag used to carry only the coordinates, so a via pulled + // beyond the stop it used to precede kept claiming the earlier leg. The route then ran + // out to the point and back before carrying on, which reads as the drag doing nothing. + const via = service.create(1, { after_order_index: 0, lat: 53.87, lng: 10.7 }); + + const moved = service.move(via.id, 1, 53.87, 11.53, 1); + + expect(moved).toMatchObject({ lat: 53.87, lng: 11.53, after_order_index: 1 }); + }); + + it('ROADTRIP-SVC-008: a re-pin through the wrong day changes nothing at all', () => { + // The day check has to guard the anchor as well as the coordinates, or an id from + // another day could be renumbered through a day the caller does happen to reach. + const via = service.create(1, { after_order_index: 0, lat: 53, lng: 10 }); + + expect(service.move(via.id, 2, 51, 12, 3)).toBeNull(); + expect(service.listForDay(1)[0]).toMatchObject({ lat: 53, lng: 10, after_order_index: 0 }); + }); + it('ROADTRIP-SVC-006: a via cannot be moved or removed through the wrong day', () => { const via = service.create(1, { after_order_index: 0, lat: 53, lng: 10 }); diff --git a/shared/src/roadtrip/roadtrip.schema.ts b/shared/src/roadtrip/roadtrip.schema.ts index 1eac31b1c..8efe3b05e 100644 --- a/shared/src/roadtrip/roadtrip.schema.ts +++ b/shared/src/roadtrip/roadtrip.schema.ts @@ -43,10 +43,20 @@ export const roadtripViaCreateRequestSchema = z.object({ }); export type RoadtripViaCreateRequest = z.infer; -/** Moving a via is the whole edit — dragging it somewhere else on the map. */ +/** + * Moving a via is the whole edit — dragging it somewhere else on the map. + * + * `after_order_index` travels with the drag because the anchor is not a property of the + * point, it is a property of where the point sits along the drive. Without it a via + * dragged past the stop it used to precede keeps claiming the earlier leg, and the route + * runs out to it and back again before carrying on — which reads as the drag having done + * nothing at all. Optional so an older client, and a drag that did not move the via + * across a stop, still send just the coordinates. + */ export const roadtripViaUpdateRequestSchema = z.object({ lat: latSchema, lng: lngSchema, + after_order_index: z.number().int().min(0).optional(), }); export type RoadtripViaUpdateRequest = z.infer;