From a2b637824508b5d21f188b89ebafd7c4602441c5 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 20 Oct 2026 00:53:00 +0000 Subject: [PATCH] fix(tours): keep a city walk's visits through a save from the planner, and count a loop's start once (CW-02, CW-03, CW-09) A walk saved from the planner kept its stops without the keys the server gave them, so the next save from the same open planner handed every stop a new key and dropped the visits checked off in the day plan meanwhile. The planner now takes over the keys a save answers, point by point, without planning the route again, so a stop keeps its key and its visits from one save to the next. The edits of a stop's dialog now reach the planner: a changed name, stay, importance or protection counted as no change and was dropped. It is a step of its own that can be undone, Save stays available, and the route is not planned again for it. A save that takes stops out of a walk, or makes it another type, tells every client of the trip which visits are left on each day the walk is on, the client that saved included, so the progress in the day row no longer counts a stop the walk has lost. The way back of a loop is the start again and no stop of its own. Closing a loop gives it only the start's spot and the place it stands for, and the planner's stays, the stops of the tour detail, the stop count of the tour list, the visits and the progress an assistant reads leave it out, so a loop through three stops counts three, the start's stay counts once and the way back cannot be checked off on its own. A loop saved before keeps working. The rule lives in one helper the server and the client share, and the tour list takes its stop count from the walk's points through it. The tests that seed a walk for the assistant's progress go through the ORM factories. --- .../Tours/planner/TourWaypointList.test.tsx | 18 +- .../Tours/planner/TourWaypointList.tsx | 7 +- .../Tours/planner/stationModel.test.ts | 33 ++++ .../components/Tours/planner/stationModel.ts | 9 +- .../planner/tourPlannerWaypoints.test.ts | 47 ++++- .../Tours/planner/tourPlannerWaypoints.ts | 63 +++++-- .../Tours/planner/tourRouteActions.test.ts | 32 ++-- .../Tours/planner/tourRouteActions.ts | 42 +++-- .../useTourPlanner.routeTools.test.tsx | 174 ++++++++++++++++-- .../Tours/planner/useTourPlanner.ts | 14 +- .../components/Tours/planner/useTourRoute.ts | 50 ++++- .../src/components/Tours/poi/poiInsertion.ts | 4 +- .../components/Tours/walk/WalkStations.tsx | 4 +- .../src/pages/tripPlanner/useTourMapPoints.ts | 2 +- .../TourStationVisits.repository.ts | 10 +- .../src/db/repositories/Tours.repository.ts | 12 +- server/src/nest/tours/tour-visits.service.ts | 7 +- server/src/nest/tours/tours.service.ts | 65 ++++++- server/tests/e2e/tours-walk.e2e.test.ts | 12 +- .../TourStationVisits.repository.test.ts | 8 +- .../db/repositories/Tours.repository.test.ts | 28 --- .../unit/nest/tour-visits.service.test.ts | 34 ++++ server/tests/unit/nest/tours.mcp.test.ts | 12 +- ...s.permissions.boundaries.mock-only.test.ts | 2 +- .../nest/tours.service.permissions.test.ts | 2 +- .../unit/nest/tours.walk.service.test.ts | 75 +++++++- shared/src/index.ts | 1 + shared/src/tours/tour-walk.spec.ts | 35 ++++ shared/src/tours/tour-walk.ts | 33 ++++ 29 files changed, 684 insertions(+), 151 deletions(-) create mode 100644 client/src/components/Tours/planner/stationModel.test.ts create mode 100644 shared/src/tours/tour-walk.spec.ts create mode 100644 shared/src/tours/tour-walk.ts diff --git a/client/src/components/Tours/planner/TourWaypointList.test.tsx b/client/src/components/Tours/planner/TourWaypointList.test.tsx index fb0e3c36c..01f076779 100644 --- a/client/src/components/Tours/planner/TourWaypointList.test.tsx +++ b/client/src/components/Tours/planner/TourWaypointList.test.tsx @@ -1,4 +1,4 @@ -// FE-TOUR-WAYPOINT-LIST-001 to FE-TOUR-WAYPOINT-LIST-007: shaping points, the water router's answers +// FE-TOUR-WAYPOINT-LIST-001 to FE-TOUR-WAYPOINT-LIST-008: shaping points, the water router's answers // and a city walk's stops in the list (P2A-03, P2A-05, PAD-11, CW-01). import userEvent from '@testing-library/user-event'; import type { PaddleRouteLeg } from '@trek/shared'; @@ -216,4 +216,20 @@ describe('TourWaypointList', () => { ); expect(screen.queryByRole('button', { name: 'Edit stop' })).not.toBeInTheDocument(); }); + + it('FE-TOUR-WAYPOINT-LIST-008: the way back of a loop walk is no stop: no pills and no stop to edit', async () => { + const back = { ...START, id: 'back', role: 'end' as const }; + const controller = planner({ + tourType: 'walk', + isLoop: true, + waypoints: [START, STOP, back], + selectedWaypointId: 'back', + }); + render(); + const rows = screen.getAllByTestId('tour-planner-waypoint-row'); + expect(within(at(rows, 0)).getByTestId('station-pills')).toBeInTheDocument(); + expect(within(at(rows, 2)).queryByTestId('station-pills')).not.toBeInTheDocument(); + await userEvent.click(within(at(rows, 2)).getByRole('button', { name: 'More actions' })); + expect(screen.queryByRole('button', { name: 'Edit stop' })).not.toBeInTheDocument(); + }); }); diff --git a/client/src/components/Tours/planner/TourWaypointList.tsx b/client/src/components/Tours/planner/TourWaypointList.tsx index 3310d2e6d..3201bd82d 100644 --- a/client/src/components/Tours/planner/TourWaypointList.tsx +++ b/client/src/components/Tours/planner/TourWaypointList.tsx @@ -475,7 +475,7 @@ function WaypointRowActions({ const menu = useContextMenu(); const paddle = usePaddleWaypointActions(planner); const [stationOpen, setStationOpen] = useState(false); - const station = planner.tourType === 'walk' && !point.shaping; + const station = planner.tourType === 'walk' && !point.shaping && !(planner.isLoop && index === lastIndex); const items: MenuItems = [ ...(station ? [ @@ -882,6 +882,7 @@ export default function TourWaypointList({ const isDropTarget = dropTarget?.id === point.id && draggedWaypointId !== point.id; const loopEnd = planner.isLoop && index === lastIndex; const linkedPlace = tripPlaces.linkedPlaceOf(point); + const placeName = linkedPlace?.name ?? placeNames[point.id]; return (
  • tripPlaces.unlink(point.id) : undefined} - stationName={linkedPlace?.name ?? placeNames[point.id]} + stationName={placeName} /> )} diff --git a/client/src/components/Tours/planner/stationModel.test.ts b/client/src/components/Tours/planner/stationModel.test.ts new file mode 100644 index 000000000..ca9fe9cab --- /dev/null +++ b/client/src/components/Tours/planner/stationModel.test.ts @@ -0,0 +1,33 @@ +// FE-WALK-TIMES-001 to FE-WALK-TIMES-002: a city walk's walking time and stays kept apart (CW-02). +import { describe, expect, it } from 'vitest'; +import { walkTimes } from './stationModel'; +import type { TourPlannerWaypoint } from './tourPlannerWaypoints'; + +const cathedral: TourPlannerWaypoint = { id: 'a', lat: 48.1, lng: 11.5, role: 'start', stayMinutes: 30 }; +const steer: TourPlannerWaypoint = { id: 's', lat: 48.105, lng: 11.505, role: 'via', shaping: true, stayMinutes: 99 }; +const market: TourPlannerWaypoint = { id: 'b', lat: 48.11, lng: 11.51, role: 'via', stayMinutes: 0 }; +const park: TourPlannerWaypoint = { id: 'c', lat: 48.13, lng: 11.53, role: 'end' }; + +describe('walkTimes', () => { + it('FE-WALK-TIMES-001: adds the stays entered to the walking time and says what is not known', () => { + expect(walkTimes([cathedral, steer, market, park], 45)).toEqual({ + walkingMinutes: 45, + stayMinutes: 30, + unknownStays: 1, + totalMinutes: 75, + partial: true, + }); + expect(walkTimes([cathedral, market], null)).toMatchObject({ totalMinutes: null, partial: false }); + }); + + it("FE-WALK-TIMES-002: the way back of a loop is its start again, so the start's stay counts once", () => { + const back: TourPlannerWaypoint = { ...cathedral, id: 'back', role: 'end' }; + expect(walkTimes([cathedral, market, { ...park, role: 'via' }, back], 45)).toEqual({ + walkingMinutes: 45, + stayMinutes: 30, + unknownStays: 1, + totalMinutes: 75, + partial: true, + }); + }); +}); diff --git a/client/src/components/Tours/planner/stationModel.ts b/client/src/components/Tours/planner/stationModel.ts index 6a062a2df..1a103b0f0 100644 --- a/client/src/components/Tours/planner/stationModel.ts +++ b/client/src/components/Tours/planner/stationModel.ts @@ -1,3 +1,4 @@ +import { walkStationsOf } from '@trek/shared'; import type { TourPlannerWaypoint } from './tourPlannerWaypoints'; /** @@ -18,13 +19,9 @@ export interface WalkTimes { partial: boolean; } -/** The stations of a city walk: its points that are no shaping points. */ -function stationsOf(points: readonly TourPlannerWaypoint[]): TourPlannerWaypoint[] { - return points.filter((point) => !point.shaping); -} - export function walkTimes(points: readonly TourPlannerWaypoint[], walkingMinutes: number | null): WalkTimes { - const stations = stationsOf(points); + // A shaping point is no station, and the way back of a loop is its start again: its stay counts once. + const stations = walkStationsOf(points); const stayMinutes = stations.reduce((sum, station) => sum + (station.stayMinutes ?? 0), 0); const unknownStays = stations.filter((station) => station.stayMinutes == null).length; return { diff --git a/client/src/components/Tours/planner/tourPlannerWaypoints.test.ts b/client/src/components/Tours/planner/tourPlannerWaypoints.test.ts index d2863dd64..f6fcfa443 100644 --- a/client/src/components/Tours/planner/tourPlannerWaypoints.test.ts +++ b/client/src/components/Tours/planner/tourPlannerWaypoints.test.ts @@ -1,14 +1,16 @@ -// FE-TOUR-WAYPOINTS-001 to FE-TOUR-WAYPOINTS-007: the editor's waypoint model, shaping points, drawn legs -// and a city walk's stations (P2A-03, PAD-11, CW-01 to CW-04). +// FE-TOUR-WAYPOINTS-001 to FE-TOUR-WAYPOINTS-009: the editor's waypoint model, shaping points, drawn legs +// and a city walk's stations with the keys a save answers (P2A-03, PAD-11, CW-01 to CW-04). import type { TourWaypoint } from '@trek/shared'; import { describe, expect, it } from 'vitest'; import { normalizePlannerWaypoints, plannerWaypointsOf, + routeExtrasKey, sameWaypointSnapshot, savedWaypointsOf, tourWaypointKey, withDrawnLegsRouted, + withSavedKeys, withShaping, withShapingPoint, type TourPlannerWaypoint, @@ -146,4 +148,45 @@ describe('tourPlannerWaypoints', () => { expect(savedWaypointsOf(points, 'hike')).toEqual(plain); expect(savedWaypointsOf(points)).toEqual(plain); }); + + it("FE-TOUR-WAYPOINTS-008: a station's own fields make another step of the history, but no other route", () => { + const edits: Array> = [ + { name: 'Cathedral' }, + { stayMinutes: 0 }, + { importance: 'optional' }, + { protected: true }, + { openingHours: 'Mo-Fr 10:00-17:00' }, + ]; + for (const edit of edits) { + const edited = [A, { ...B, ...edit }, C]; + expect(sameWaypointSnapshot([A, B, C], edited)).toBe(false); + expect(tourWaypointKey(edited)).toBe(tourWaypointKey([A, B, C])); + expect(routeExtrasKey(edited)).toBe(routeExtrasKey([A, B, C])); + } + // The key a save answered is no change of the draft at all. + expect(sameWaypointSnapshot([A, B, C], [A, { ...B, key: 'stop-b' }, C])).toBe(true); + expect(routeExtrasKey([A, { ...B, linkedPlaceId: 4 }, C])).not.toBe(routeExtrasKey([A, B, C])); + }); + + it('FE-TOUR-WAYPOINTS-009: the keys a save answered go onto the points by position, and nothing new answers the same list', () => { + const points = [A, { ...B, key: 'stop-b' }, C]; + const keyed = withSavedKeys(points, [ + { lat: A.lat, lng: A.lng, role: 'start', sequence: 0, key: 'stop-a' }, + { lat: B.lat, lng: B.lng, role: 'via', sequence: 1, key: 'stop-b' }, + { lat: C.lat, lng: C.lng, role: 'end', sequence: 2, key: 'stop-c' }, + ]); + expect(keyed.map((point) => point.key)).toEqual(['stop-a', 'stop-b', 'stop-c']); + expect(keyed[1]).toBe(points[1]); + expect(keyed.map((point) => point.id)).toEqual(['a', 'b', 'c']); + + expect( + withSavedKeys( + keyed, + keyed.map((point, sequence) => ({ ...point, sequence })) + ) + ).toBe(keyed); + // An answer without keys, as an offline save gives, or of another length leaves the points as they are. + expect(withSavedKeys(points, savedWaypointsOf([A, B, C]))).toBe(points); + expect(withSavedKeys(points, [])).toBe(points); + }); }); diff --git a/client/src/components/Tours/planner/tourPlannerWaypoints.ts b/client/src/components/Tours/planner/tourPlannerWaypoints.ts index 68021dc15..f383197e8 100644 --- a/client/src/components/Tours/planner/tourPlannerWaypoints.ts +++ b/client/src/components/Tours/planner/tourPlannerWaypoints.ts @@ -126,21 +126,58 @@ export function tourWaypointKey(points: TourPlannerWaypoint[]): string { } /** - * The line, and what was planned along it that draws no line: a portage's - * time, a lock, a point the router set, the place a point stands for. + * What was planned along the line that draws no line: a portage's time, a + * lock, a point the router set, the place a point stands for. With + * {@link tourWaypointKey} it is what a route is planned from. */ +export function routeExtrasKey(points: TourPlannerWaypoint[]): string { + return JSON.stringify( + points.map((point) => [ + point.fixedLeg?.kind === 'portage' ? (point.fixedLeg.extra_minutes ?? null) : null, + point.lock ?? null, + !!point.auto, + !!point.carryNext, + point.linkedPlaceId ?? null, + ]) + ); +} + +/** CW-01 to CW-04: what a city walk's station holds besides its spot, none of which the route depends on. */ +function stationFieldsKey(points: TourPlannerWaypoint[]): string { + return JSON.stringify( + points.map((point) => [ + point.name ?? null, + point.stayMinutes ?? null, + point.importance ?? null, + !!point.protected, + point.openingHours ?? null, + ]) + ); +} + +/** The same step of the history: the line, what was planned along it, and every station's own fields. */ export function sameWaypointSnapshot(a: TourPlannerWaypoint[], b: TourPlannerWaypoint[]): boolean { - const extras = (points: TourPlannerWaypoint[]) => - JSON.stringify( - points.map((point) => [ - point.fixedLeg?.kind === 'portage' ? (point.fixedLeg.extra_minutes ?? null) : null, - point.lock ?? null, - !!point.auto, - !!point.carryNext, - point.linkedPlaceId ?? null, - ]) - ); - return tourWaypointKey(a) === tourWaypointKey(b) && extras(a) === extras(b); + return ( + tourWaypointKey(a) === tourWaypointKey(b) && + routeExtrasKey(a) === routeExtrasKey(b) && + stationFieldsKey(a) === stationFieldsKey(b) + ); +} + +/** + * CW-03: the keys a save was answered with, taken over by position. The server + * keeps the points of a save in their order and gives one sent without a key + * a key of its own; sent back with the next save, it keeps the visits made on + * that station. An answer of another length is left alone, and nothing new + * answers the same array. + */ +export function withSavedKeys(points: TourPlannerWaypoint[], saved: readonly TourWaypoint[]): TourPlannerWaypoint[] { + if (saved.length !== points.length) return points; + const keyed = points.map((point, index) => { + const key = saved[index]?.key; + return key && key !== point.key ? { ...point, key } : point; + }); + return keyed.some((point, index) => point !== points[index]) ? keyed : points; } /** P2A-03: the points with a shaping point set into the leg after `afterId`; unchanged when there is no such leg. */ diff --git a/client/src/components/Tours/planner/tourRouteActions.test.ts b/client/src/components/Tours/planner/tourRouteActions.test.ts index 976832fe3..9294b0707 100644 --- a/client/src/components/Tours/planner/tourRouteActions.test.ts +++ b/client/src/components/Tours/planner/tourRouteActions.test.ts @@ -1,9 +1,10 @@ +import { isClosedLoop } from '@trek/shared'; import { describe, expect, it } from 'vitest'; +import type { TourPlannerWaypoint } from './tourPlannerWaypoints'; import { alignFreeLegs, closeLoop, freeLegBetween, - isClosedLoop, portageLegBetween, reverseRoute, setPortageExtraMinutes, @@ -27,18 +28,6 @@ const FERRATA = { ] as [number, number][], }; -describe('isClosedLoop', () => { - it('reads a loop from the points: the end on the spot of the start', () => { - expect(isClosedLoop([A, B, { ...A, id: 'a2' }])).toBe(true); - expect(isClosedLoop([A, B, C])).toBe(false); - }); - - it('is no loop with two points on one spot, or none', () => { - expect(isClosedLoop([A, { ...A, id: 'a2' }])).toBe(false); - expect(isClosedLoop([])).toBe(false); - }); -}); - describe('closeLoop (PR1-01)', () => { it('adds one end on the start, under a new id, and keeps every stop', () => { const closed = closeLoop([A, B, C], 'back'); @@ -57,6 +46,23 @@ describe('closeLoop (PR1-01)', () => { const closed = closeLoop([{ ...A, fixedLeg: freeLegBetween(A, B) }, B], 'back'); expect(closed[2].fixedLeg).toBeUndefined(); }); + + it("takes none of a city walk's start station onto the way back, only its spot and the place it stands for", () => { + const cathedral: TourPlannerWaypoint = { + ...A, + role: 'start', + key: 'stop-cathedral', + name: 'Cathedral', + stayMinutes: 30, + importance: 'must', + protected: true, + openingHours: 'Mo-Su 09:00-18:00', + linkedPlaceId: 7, + }; + const closed = closeLoop([cathedral, { ...B, role: 'via' }, { ...C, role: 'end' }], 'back'); + expect(closed[3]).toEqual({ id: 'back', lat: A.lat, lng: A.lng, role: 'start', linkedPlaceId: 7 }); + expect(closed[0]).toBe(cathedral); + }); }); describe('reverseRoute (PR1-02)', () => { diff --git a/client/src/components/Tours/planner/tourRouteActions.ts b/client/src/components/Tours/planner/tourRouteActions.ts index 1cb1b3e46..c48b103e1 100644 --- a/client/src/components/Tours/planner/tourRouteActions.ts +++ b/client/src/components/Tours/planner/tourRouteActions.ts @@ -1,4 +1,4 @@ -import type { PaddleLock, TourFixedLeg } from '@trek/shared'; +import { isClosedLoop, type PaddleLock, type TourFixedLeg } from '@trek/shared'; /** * The route tools of the tour planner as plain list operations: reversing the @@ -22,22 +22,14 @@ export interface RoutePoint { carryNext?: boolean; } -/** Closer than this, in degrees, two waypoints stand on the same spot (about a centimetre). */ -const SAME_SPOT_DEG = 1e-7; - -function sameSpot(a: { lat: number; lng: number }, b: { lat: number; lng: number }): boolean { - return Math.abs(a.lat - b.lat) < SAME_SPOT_DEG && Math.abs(a.lng - b.lng) < SAME_SPOT_DEG; -} - -/** - * Whether the tour ends where it starts. Read from the points themselves, so a - * saved loop opens as a loop again without a flag of its own; a tour of two - * points on one spot is no loop, there is nothing to return from. - */ -export function isClosedLoop(points: readonly { lat: number; lng: number }[]): boolean { - const first = points[0]; - const last = points[points.length - 1]; - return points.length >= 3 && !!first && !!last && sameSpot(first, last); +/** What makes the start a station of a city walk (CW-01 to CW-04), and its saved key (CW-03). */ +interface StationParts { + key?: string; + name?: string | null; + stayMinutes?: number | null; + importance?: string | null; + protected?: boolean; + openingHours?: string | null; } /** @@ -56,11 +48,23 @@ export function withPointAppended(points: readonly T[], newId: string): T[] { - const first = points[0]; + const first: (T & StationParts) | undefined = points[0]; if (!first || points.length < 2 || isClosedLoop(points)) return [...points]; - const { fixedLeg: _leg, lock: _lock, ...start } = first; + const { + fixedLeg: _leg, + lock: _lock, + key: _key, + name: _name, + stayMinutes: _stay, + importance: _importance, + protected: _protected, + openingHours: _hours, + ...start + } = first; return [...points, { ...start, id: newId } as T]; } diff --git a/client/src/components/Tours/planner/useTourPlanner.routeTools.test.tsx b/client/src/components/Tours/planner/useTourPlanner.routeTools.test.tsx index e73a578e0..f6ca6f217 100644 --- a/client/src/components/Tours/planner/useTourPlanner.routeTools.test.tsx +++ b/client/src/components/Tours/planner/useTourPlanner.routeTools.test.tsx @@ -1,18 +1,28 @@ import { act, renderHook } from '@testing-library/react'; +import type { TourCreateRequest, TourCreateResponse } from '@trek/shared'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { routeWalkingTour, enrichTourElevations, createTour, detailTour, updateTour, paddlePlan, paddleForget } = - vi.hoisted(() => ({ - routeWalkingTour: vi.fn(), - enrichTourElevations: vi.fn(), - createTour: vi.fn(), - detailTour: vi.fn(), - updateTour: vi.fn(), - paddlePlan: vi.fn(), - paddleForget: vi.fn(), - })); +const { + routeWalkingTour, + routeCityWalk, + enrichTourElevations, + createTour, + detailTour, + updateTour, + paddlePlan, + paddleForget, +} = vi.hoisted(() => ({ + routeWalkingTour: vi.fn(), + routeCityWalk: vi.fn(), + enrichTourElevations: vi.fn(), + createTour: vi.fn(), + detailTour: vi.fn(), + updateTour: vi.fn(), + paddlePlan: vi.fn(), + paddleForget: vi.fn(), +})); -vi.mock('./tourRouting', () => ({ routeWalkingTour, enrichTourElevations })); +vi.mock('./tourRouting', () => ({ routeWalkingTour, routeCityWalk, enrichTourElevations })); vi.mock('../../../repo/tourRepo', () => ({ tourRepo: { create: createTour, detail: detailTour, update: updateTour } })); vi.mock('../../../repo/paddleRouteRepo', () => ({ paddleRouteRepo: { plan: paddlePlan, forget: paddleForget } })); vi.mock('../useTourPermissions', () => ({ @@ -95,6 +105,7 @@ describe('useTourPlanner route tools', () => { vi.useFakeTimers(); localStorage.clear(); routeWalkingTour.mockReset().mockResolvedValue(routed); + routeCityWalk.mockReset().mockResolvedValue(routed); paddlePlan.mockReset().mockImplementation(async (_trip: number, legs: LegAsked[]) => onWater(legs)); paddleForget.mockReset(); enrichTourElevations.mockReset().mockResolvedValue(enriched); @@ -526,4 +537,145 @@ describe('useTourPlanner route tools', () => { ); }); }); + + describe("a city walk's keys and stops (CW-02, CW-03)", () => { + /** The server's answer to a save: the points in their order, a key sent kept, one missing made up. */ + const answered = (body: TourCreateRequest, revision: number): TourCreateResponse => ({ + tour: { ...savedTour, tour_type: 'walk', revision }, + waypoints: body.waypoints.map((point, index) => ({ ...point, key: point.key ?? `new-${revision}-${index}` })), + }); + /** The keys of the points a create or an update sent, the body being its last argument. */ + const keysSent = (call: unknown[] | undefined) => + (call?.[call.length - 1] as TourCreateRequest).waypoints.map((point) => point.key); + + /** A walk through these points, named and routed, ready to save. */ + async function walkPlanned(points: Array<[number, number]>) { + createTour.mockImplementation(async (_trip: number, body: TourCreateRequest) => answered(body, 1)); + updateTour.mockImplementation(async (_trip: number, _place: number, body: TourCreateRequest) => + answered(body, (updateTour.mock.calls.length as number) + 1) + ); + const hook = plannerWith(points); + act(() => hook.result.current.setTourType('walk')); + act(() => hook.result.current.setName('Old town')); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + return hook; + } + + it('sends the keys a save answered with the next save, and plans no route again for them', async () => { + const { result } = await walkPlanned([ + [48, 11], + [48.01, 11.02], + [48.02, 11], + ]); + const routings = routeCityWalk.mock.calls.length; + await act(async () => { + await result.current.save(); + }); + expect(keysSent(createTour.mock.calls[0])).toEqual([undefined, undefined, undefined]); + expect(result.current.waypoints.map((point) => point.key)).toEqual(['new-1-0', 'new-1-1', 'new-1-2']); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + expect(routeCityWalk).toHaveBeenCalledTimes(routings); + expect(result.current.status).toBe('saved'); + expect(result.current.hasUnsavedChanges).toBe(false); + expect(result.current.canUndo).toBe(false); + + act(() => result.current.setName('Old town at dusk')); + await act(async () => { + await result.current.save(); + }); + expect(keysSent(updateTour.mock.calls[0])).toEqual(['new-1-0', 'new-1-1', 'new-1-2']); + }); + + it("closes a walk without the start's key, and the way back keeps the key it was given", async () => { + const { result } = await walkPlanned([ + [48, 11], + [48.01, 11.02], + [48.02, 11], + ]); + await act(async () => { + await result.current.save(); + }); + act(() => result.current.returnToStart()); + expect(result.current.waypoints.map((point) => point.key)).toEqual(['new-1-0', 'new-1-1', 'new-1-2', undefined]); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + await act(async () => { + await result.current.save(); + }); + expect(result.current.waypoints.map((point) => point.key)).toEqual(['new-1-0', 'new-1-1', 'new-1-2', 'new-2-3']); + + act(() => result.current.setName('Round the old town')); + await act(async () => { + await result.current.save(); + }); + expect(keysSent(updateTour.mock.calls[1])).toEqual(['new-1-0', 'new-1-1', 'new-1-2', 'new-2-3']); + }); + + it("takes a stop's stay from its dialog as a step of its own and saves it, without routing the walk again", async () => { + const { result } = await walkPlanned([ + [48, 11], + [48.01, 11.02], + [48.02, 11], + ]); + const routings = routeCityWalk.mock.calls.length; + const stop = must(result.current.waypoints[1]); + act(() => + result.current.editWaypoints((points) => + points.map((point) => (point.id === stop.id ? { ...point, name: 'Market', stayMinutes: 45 } : point)) + ) + ); + expect(result.current.waypoints[1]).toMatchObject({ name: 'Market', stayMinutes: 45 }); + expect(result.current.stationStays?.stayMinutes).toBe(45); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + expect(routeCityWalk).toHaveBeenCalledTimes(routings); + expect(result.current.status).toBe('ready'); + expect(result.current.canSave).toBe(true); + + act(() => result.current.undo()); + expect(result.current.waypoints[1]?.stayMinutes).toBeUndefined(); + act(() => result.current.redo()); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + expect(routeCityWalk).toHaveBeenCalledTimes(routings); + await act(async () => { + await result.current.save(); + }); + expect((createTour.mock.calls[0]?.[1] as TourCreateRequest).waypoints[1]).toMatchObject({ + name: 'Market', + stay_minutes: 45, + }); + }); + + it('names a point of a paddle tour without planning the water again', async () => { + const { result } = plannerWith([ + [48, 11], + [48.01, 11.02], + [48.02, 11], + ]); + act(() => result.current.setTourType('kayak')); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + const plans = paddlePlan.mock.calls.length; + act(() => + result.current.editWaypoints((points) => + points.map((point, index) => (index === 1 ? { ...point, name: 'Boathouse' } : point)) + ) + ); + await act(async () => { + await vi.advanceTimersByTimeAsync(450); + }); + expect(paddlePlan).toHaveBeenCalledTimes(plans); + expect(result.current.waypoints[1]?.name).toBe('Boathouse'); + expect(result.current.status).toBe('ready'); + }); + }); }); diff --git a/client/src/components/Tours/planner/useTourPlanner.ts b/client/src/components/Tours/planner/useTourPlanner.ts index fb6968664..50a7f09ce 100644 --- a/client/src/components/Tours/planner/useTourPlanner.ts +++ b/client/src/components/Tours/planner/useTourPlanner.ts @@ -1,13 +1,13 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' -import { hikingPreferencesOf, paddleSpeedKmhSchema, tourWebsiteSchema, type BikeProfile, type HikingPreferences, type PlannableTourType, type TourCreateResponse, type TourFixedLeg, type TourListItem, type TourMaxHikingDifficulty } from '@trek/shared' +import { hikingPreferencesOf, isClosedLoop, paddleSpeedKmhSchema, tourWebsiteSchema, type BikeProfile, type HikingPreferences, type PlannableTourType, type TourCreateResponse, type TourFixedLeg, type TourListItem, type TourMaxHikingDifficulty } from '@trek/shared' import { paddleRouteRepo } from '../../../repo/paddleRouteRepo' import { tourRepo } from '../../../repo/tourRepo' import { randomId } from '../../../utils/randomId' import { analyzeRouteGeometry, type RouteProfileFocus } from '../../../utils/routeGeometry' -import { closeLoop, isClosedLoop, reverseRoute, toggleFreeLeg, withPointAppended } from './tourRouteActions' +import { closeLoop, reverseRoute, toggleFreeLeg, withPointAppended } from './tourRouteActions' import { walkTimes } from './stationModel' import { hasCompleteElevation, useTourRoute } from './useTourRoute' -import { normalizePlannerWaypoints, plannerWaypointsOf, ridablePoints, sameWaypointSnapshot, savedWaypointsOf, unlinkedPoint, type TourPlannerWaypoint } from './tourPlannerWaypoints' +import { normalizePlannerWaypoints, plannerWaypointsOf, ridablePoints, sameWaypointSnapshot, savedWaypointsOf, unlinkedPoint, withSavedKeys, type TourPlannerWaypoint } from './tourPlannerWaypoints' import { openedTourFields, tourPlannerFacts } from './tourPlannerFacts' import { usePlannerHistory } from './usePlannerHistory' import { useTourDraftStorage } from './useTourDraftStorage' @@ -798,6 +798,12 @@ export function useTourPlanner({ tripId, active = true, onSaved, canEdit: editPe ? await tourRepo.create(tripId, payload) : await tourRepo.update(tripId, targetPlaceId, payload) if (stillCurrent()) { + // CW-03: the keys the server gave new points stay with them, so the next save keeps their visits. + const keyed = withSavedKeys(waypointsRef.current, result.waypoints) + if (keyed !== waypointsRef.current) { + waypointsRef.current = keyed + setWaypoints(keyed) + } forgetDraft() adoptRevision(result.tour.revision) if (options.name) setNameState(options.name.trim()) @@ -827,7 +833,7 @@ export function useTourPlanner({ tripId, active = true, onSaved, canEdit: editPe } }, [adoptRevision, refuseRevision, revisionBaseRef, clearHistory, breakAdditionalMinutes, canSave, derivedFromPlaceId, - description, savedDurationSeconds, editingPlaceId, enrichedGeometry, invalidateDetailRequest, name, onSaved, plannedDurationMinutes, forgetDraft, tripId, typeFields, waypoints, website]) + description, savedDurationSeconds, editingPlaceId, enrichedGeometry, invalidateDetailRequest, name, onSaved, plannedDurationMinutes, forgetDraft, tourType, tripId, typeFields, waypoints, website]) return { tripId, canEdit, canAssign, diff --git a/client/src/components/Tours/planner/useTourRoute.ts b/client/src/components/Tours/planner/useTourRoute.ts index d2e6c8f0c..01058d300 100644 --- a/client/src/components/Tours/planner/useTourRoute.ts +++ b/client/src/components/Tours/planner/useTourRoute.ts @@ -6,10 +6,24 @@ import type { PlannableTourType, TourMaxHikingDifficulty, } from '@trek/shared'; -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { + useCallback, + useEffect, + useEffectEvent, + useMemo, + useRef, + useState, + type Dispatch, + type SetStateAction, +} from 'react'; import { randomId } from '../../../utils/randomId'; import { planPaddleStep, type PaddleTourRoute } from './paddleRouting'; -import { normalizePlannerWaypoints, tourWaypointKey, type TourPlannerWaypoint } from './tourPlannerWaypoints'; +import { + normalizePlannerWaypoints, + routeExtrasKey, + tourWaypointKey, + type TourPlannerWaypoint, +} from './tourPlannerWaypoints'; import { isTourRoute, type TourRouteUnreachable } from './tourRouteOutcome'; import { enrichTourElevations, @@ -37,7 +51,7 @@ export interface TourRouteInputs { retryToken: number; /** The planner's error, so a point found unreachable is only named while that is the error. */ error: string | null; - setStatus: (status: TourPlannerStatus) => void; + setStatus: Dispatch>; setError: (error: string | null) => void; onCleared: () => void; /** Points the router set itself (a paddle tour's), laid in without a step of the history. */ @@ -53,6 +67,8 @@ interface PlannedRoute { distanceMeters: number | null; durationSeconds: number | null; forKey: string | null; + /** The run it was planned in: its points with what was planned along them, its choices and the retry. */ + forRun: string | null; } const NO_ROUTE: PlannedRoute = { @@ -63,12 +79,21 @@ const NO_ROUTE: PlannedRoute = { distanceMeters: null, durationSeconds: null, forKey: null, + forRun: null, }; +/** A route planned whole: a paddle route without a failed leg, a route on land with its heights. */ +function routeComplete(planned: PlannedRoute): boolean { + return planned.paddleRoute + ? planned.paddleRoute.failures.length === 0 + : hasCompleteElevation(planned.enrichedGeometry); +} + /** One planning run: what it plans from, whether it went stale, and where it writes. */ interface RouteRun { inputs: Omit; plannedKey: string; + runKey: string; signal: AbortSignal; stale: () => boolean; setPlanned: (next: PlannedRoute | ((current: PlannedRoute) => PlannedRoute)) => void; @@ -76,7 +101,7 @@ interface RouteRun { } // PAD-11: a paddle tour is planned on the water by the server's router; its heights are no concern. -async function routePaddle({ inputs, plannedKey, signal, stale, setPlanned }: RouteRun) { +async function routePaddle({ inputs, plannedKey, runKey, signal, stale, setPlanned }: RouteRun) { const { waypoints, tripId, paddleBoat, paddleLakeStyle, setStatus, setError } = inputs; setStatus('routing'); const profile = { boat: paddleBoat ?? 'kayak', lakeStyle: paddleLakeStyle } as const; @@ -92,6 +117,7 @@ async function routePaddle({ inputs, plannedKey, signal, stale, setPlanned }: Ro distanceMeters: planned.distanceMeters, durationSeconds: null, forKey: plannedKey, + forRun: runKey, }); const failed = planned.failures.length > 0; setStatus(failed ? 'routing-failed' : 'ready'); @@ -99,7 +125,7 @@ async function routePaddle({ inputs, plannedKey, signal, stale, setPlanned }: Ro } // A hike or a bike tour on land, then the heights along it. -async function routeOnLand({ inputs, plannedKey, signal, stale, setPlanned, setUnreachable }: RouteRun) { +async function routeOnLand({ inputs, plannedKey, runKey, signal, stale, setPlanned, setUnreachable }: RouteRun) { const { waypoints, tourType, bikeProfile, maxHikingDifficulty, hikingPreferences, setStatus, setError } = inputs; setStatus('routing'); const routed = @@ -124,6 +150,7 @@ async function routeOnLand({ inputs, plannedKey, signal, stale, setPlanned, setU distanceMeters: routed.distanceMeters, durationSeconds: routed.durationSeconds, forKey: plannedKey, + forRun: runKey, }); setStatus('enriching-elevation'); @@ -183,6 +210,11 @@ export function useTourRoute(inputs: TourRouteInputs) { // What a route was planned for besides its points: a route of another type or profile is not this one's. const choices = [tourType, bikeProfile, maxHikingDifficulty, hikingPreferences, paddleBoat, paddleLakeStyle]; const plannedKey = `${waypointKey}#${JSON.stringify(choices)}`; + // CW-02, CW-03: a station's own fields and the keys a save answered are no part of what a route is planned from. + const extrasKey = useMemo(() => routeExtrasKey(waypoints), [waypoints]); + const runKey = `${plannedKey}#${extrasKey}#${retryToken}`; + // The route shown was planned whole for exactly this run, so a change that leaves it as it is plans nothing again. + const routeStands = useEffectEvent(() => planned.forRun === runKey && routeComplete(planned)); useEffect(() => { if (waypoints.length < 2) { @@ -193,6 +225,10 @@ export function useTourRoute(inputs: TourRouteInputs) { setError(null); return; } + if (routeStands()) { + setStatus((current) => (current === 'dirty' ? 'ready' : current)); + return; + } const generation = ++generationRef.current; const controller = new AbortController(); @@ -214,6 +250,7 @@ export function useTourRoute(inputs: TourRouteInputs) { patchWaypoints, }, plannedKey, + runKey, signal: controller.signal, stale, setPlanned, @@ -244,6 +281,7 @@ export function useTourRoute(inputs: TourRouteInputs) { patchWaypoints, plannedKey, retryToken, + runKey, setError, setStatus, tourType, @@ -251,7 +289,7 @@ export function useTourRoute(inputs: TourRouteInputs) { waypoints, ]); - const { forKey, ...shown } = planned; + const { forKey, forRun: _run, ...shown } = planned; return { ...shown, /** The route shown was planned for these points and these choices. */ diff --git a/client/src/components/Tours/poi/poiInsertion.ts b/client/src/components/Tours/poi/poiInsertion.ts index d87ac157b..25eb6273b 100644 --- a/client/src/components/Tours/poi/poiInsertion.ts +++ b/client/src/components/Tours/poi/poiInsertion.ts @@ -1,5 +1,5 @@ -import { nearnessToLine } from '@trek/shared'; -import { isClosedLoop, withPointAppended, type RoutePoint } from '../planner/tourRouteActions'; +import { isClosedLoop, nearnessToLine } from '@trek/shared'; +import { withPointAppended, type RoutePoint } from '../planner/tourRouteActions'; type Point = RoutePoint & { role: string; shaping?: boolean }; diff --git a/client/src/components/Tours/walk/WalkStations.tsx b/client/src/components/Tours/walk/WalkStations.tsx index d25537bb9..7e696ee52 100644 --- a/client/src/components/Tours/walk/WalkStations.tsx +++ b/client/src/components/Tours/walk/WalkStations.tsx @@ -1,4 +1,4 @@ -import type { TourListItem, TourVisitStatus } from '@trek/shared'; +import { walkStationsOf, type TourListItem, type TourVisitStatus } from '@trek/shared'; import { AlertTriangle, Check, CheckCheck, Clock, Redo2, SkipForward, Undo2 } from 'lucide-react'; import { useEffect, useState } from 'react'; import { useTranslation } from '../../../i18n'; @@ -25,7 +25,7 @@ function useWalkStations(tripId: number | string, placeId: number): TourPlannerW .detail(tripId, placeId, controller.signal) .then((detail) => { if (controller.signal.aborted) return; - const points = plannerWaypointsOf(detail.waypoints, freshId).filter((point) => !point.shaping); + const points = walkStationsOf(plannerWaypointsOf(detail.waypoints, freshId)); setStations({ placeId, points }); }) .catch(() => { diff --git a/client/src/pages/tripPlanner/useTourMapPoints.ts b/client/src/pages/tripPlanner/useTourMapPoints.ts index 11877cd17..ba32cec51 100644 --- a/client/src/pages/tripPlanner/useTourMapPoints.ts +++ b/client/src/pages/tripPlanner/useTourMapPoints.ts @@ -1,7 +1,7 @@ +import { isClosedLoop } from '@trek/shared'; import { useMemo } from 'react'; import type { useOutsidePressGuard } from '../../components/Ferratas/useOutsidePressGuard'; import { paddleRolesById } from '../../components/Tours/planner/paddlePlanOf'; -import { isClosedLoop } from '../../components/Tours/planner/tourRouteActions'; import type { TourPlannerController } from '../../components/Tours/planner/useTourPlanner'; import { useWaypointPlaceNames } from '../../components/Tours/planner/useWaypointPlaceNames'; import { useTranslation } from '../../i18n'; diff --git a/server/src/db/repositories/TourStationVisits.repository.ts b/server/src/db/repositories/TourStationVisits.repository.ts index d3e806e61..69c8b7053 100644 --- a/server/src/db/repositories/TourStationVisits.repository.ts +++ b/server/src/db/repositories/TourStationVisits.repository.ts @@ -106,11 +106,15 @@ export class TourStationVisitsRepository extends TrekRepository { + /** + * Drops the visits of stations a walk no longer has, on every day it is on; + * answers the day assignments that lost one, each once. + */ + async dropStationsNotIn(place_id: number, keep: readonly string[]): Promise { const assignments = this.db().selectFrom('day_assignments').select('id').where('place_id', '=', place_id); let query = this.db().deleteFrom('tour_station_visits').where('assignment_id', 'in', assignments); if (keep.length > 0) query = query.where('station_key', 'not in', [...keep]); - await query.execute(); + const dropped = await query.returning('assignment_id').execute(); + return [...new Set(dropped.map((row) => row.assignment_id))].sort((a, b) => a - b); } } diff --git a/server/src/db/repositories/Tours.repository.ts b/server/src/db/repositories/Tours.repository.ts index f67b6d0c1..b3b2408f3 100644 --- a/server/src/db/repositories/Tours.repository.ts +++ b/server/src/db/repositories/Tours.repository.ts @@ -44,8 +44,6 @@ export interface TourListRow { revision: number; /** MF-08: a hike's routing preferences, still JSON text. */ hiking_preferences: string | null; - /** CW-03: the points that are no shaping points, the stations of a city walk. */ - station_count: number; } /** LIB-02: a tour of another trip as the take-over list reads it, with the trip it is in. */ @@ -257,12 +255,6 @@ export class ToursRepository extends TrekRepository { eb .exists(eb.selectFrom('tour_waypoints as tw').select('tw.id').whereRef('tw.place_id', '=', 'p.id')) .as('has_waypoints'), - eb - .selectFrom('tour_waypoints as sw') - .select((sub) => sub.fn.countAll().as('n')) - .whereRef('sw.place_id', '=', 'p.id') - .where('sw.shaping', '=', 0) - .as('station_count'), ]); } @@ -788,11 +780,10 @@ function toLibraryRow & { has_wa } function toListRow< - R extends Omit & { + R extends Omit & { planned: unknown; has_waypoints: unknown; has_recorded_track: unknown; - station_count: unknown; }, >(r: R): TourListRow { return { @@ -800,6 +791,5 @@ function toListRow< planned: Number(r.planned), has_waypoints: Number(r.has_waypoints), has_recorded_track: Number(r.has_recorded_track), - station_count: Number(r.station_count), }; } diff --git a/server/src/nest/tours/tour-visits.service.ts b/server/src/nest/tours/tour-visits.service.ts index 5797135a0..0957385d9 100644 --- a/server/src/nest/tours/tour-visits.service.ts +++ b/server/src/nest/tours/tour-visits.service.ts @@ -10,7 +10,7 @@ import type { ToursRepository } from '../../db/repositories/Tours.repository'; import { PlacesService } from '../places/places.service'; import { InjectRepository } from '@mikro-orm/nestjs'; import { Injectable, NotFoundException } from '@nestjs/common'; -import type { TourVisit, TourVisitListResponse, TourVisitUpdateRequest } from '@trek/shared'; +import { walkStationsOf, type TourVisit, type TourVisitListResponse, type TourVisitUpdateRequest } from '@trek/shared'; /** A city walk on one of its days: the assignment, the walk and its stations in order. */ export interface WalkOnDay { @@ -52,7 +52,7 @@ export class TourVisitsService { assignmentId, dayId: assignment.day_id, placeId: assignment.place_id, - stations: points.filter((point) => !point.shaping), + stations: walkStationsOf(points), visits: await this.visitsRepo.listForAssignment(assignmentId), }; } @@ -138,6 +138,7 @@ export interface WalkProgress { walking_minutes: number | null; } -function toVisit(row: TourStationVisitRow): TourVisit { +/** A visit as the routes and the trip's socket send it. */ +export function toVisit(row: TourStationVisitRow): TourVisit { return { assignment_id: row.assignment_id, station_key: row.station_key, status: row.status }; } diff --git a/server/src/nest/tours/tours.service.ts b/server/src/nest/tours/tours.service.ts index f93532308..997accb7c 100644 --- a/server/src/nest/tours/tours.service.ts +++ b/server/src/nest/tours/tours.service.ts @@ -7,7 +7,11 @@ import { TOUR_HIKING_PREFERENCES, TOUR_TRACK_MARKS } from '../../db/json-columns import type { PlacesRepository } from '../../db/repositories/Places.repository'; import type { TourStationVisitsRepository } from '../../db/repositories/TourStationVisits.repository'; import type { TourTypesRepository } from '../../db/repositories/TourTypes.repository'; -import type { TourWaypointsRepository } from '../../db/repositories/TourWaypoints.repository'; +import type { + TourWaypointRow, + TourWaypointsRepository, + TourWaypointWithPlaceRow, +} from '../../db/repositories/TourWaypoints.repository'; import type { TourDayRow, TourListRow, @@ -23,6 +27,7 @@ import { PlaceImportService } from '../place-import/place-import.service'; import { PlacesService } from '../places/places.service'; import { TourPaddleCheckService } from './tour-paddle-check.service'; import { advanceTourRevision } from './tour-revision'; +import { toVisit } from './tour-visits.service'; import { computeTourMetrics, localHashOf, @@ -52,6 +57,7 @@ import { type TourProvenanceSource, type TourWaypoint, type WandererDifficulty, + walkStationsOf, wandererDifficultyToSac, } from '@trek/shared'; @@ -110,7 +116,8 @@ export class ToursService { private readonly paddleChecks: TourPaddleCheckService, ) {} - private toItem(r: TourListRow): TourListItem { + /** A tour as it is listed; a city walk with the stations its points make (CW-03). */ + private toItem(r: TourListRow, points: readonly TourWaypointRow[] = []): TourListItem { return { place_id: r.place_id, name: r.name, @@ -142,7 +149,7 @@ export class ToursService { has_waypoints: Boolean(r.has_waypoints), revision: r.revision, hiking_preferences: decodeJson(TOUR_HIKING_PREFERENCES, r.hiking_preferences, 'tour'), - station_count: r.tour_type === 'walk' ? r.station_count : null, + station_count: r.tour_type === 'walk' ? walkStationsOf(points).length : null, }; } @@ -218,12 +225,28 @@ export class ToursService { const tid = toRowId(tripId); if (tid === null) return []; const rows = await this.toursRepo.listForTrip(tid); + const walkPoints = await this.walkPointsOf(tid, rows); return await this.paddleChecks.forTrip( tid, - rows.map((r) => this.toItem(r)), + rows.map((r) => this.toItem(r, walkPoints.get(r.place_id) ?? [])), ); } + /** CW-03: the points of the trip's city walks by tour, read only where the trip has a walk. */ + private async walkPointsOf( + tripId: number, + rows: readonly TourListRow[], + ): Promise> { + const byPlace = new Map(); + if (!rows.some((row) => row.tour_type === 'walk')) return byPlace; + for (const point of await this.waypointsRepo.listForTrip(tripId)) { + const points = byPlace.get(point.place_id); + if (points) points.push(point); + else byPlace.set(point.place_id, [point]); + } + return byPlace; + } + /** The days every tour of the trip sits on, for the MCP read tools. */ async listTourDays(tripId: number): Promise { return await this.toursRepo.listTourDays(tripId); @@ -257,7 +280,7 @@ export class ToursService { } } - const checked = await this.paddleChecks.forTour(this.toItem(row), stored, place?.route_geometry ?? null); + const checked = await this.paddleChecks.forTour(this.toItem(row, stored), stored, place?.route_geometry ?? null); return { tour: checked.item, waypoints, @@ -349,7 +372,11 @@ export class ToursService { } catch { this.logger.warn(`Committed Tour ${placeId}: place notification failed`); } - const checked = await this.paddleChecks.forTour(this.toItem(row), waypoints, place.route_geometry ?? null); + const checked = await this.paddleChecks.forTour( + this.toItem(row, waypoints), + waypoints, + place.route_geometry ?? null, + ); return { tour: checked.item, waypoints, @@ -573,7 +600,7 @@ export class ToursService { const start = input.route_geometry[0]; const fields = this.routeFields(input); - await this.uow.transactional(async () => { + const visitsDropped = await this.uow.transactional(async () => { // The trip predicate is the cross-trip boundary. It runs inside the write // so an id from another accessible trip cannot be moved into the current // one, and a place deleted meanwhile answers 404 instead of failing a @@ -597,8 +624,8 @@ export class ToursService { const waypoints = keyedWaypoints(await this.linkedWaypoints(tid, input.waypoints)); await this.waypointsRepo.insertForPlace(pid, waypoints); // CW-03: a station that is gone takes its visits with it; a tour that is no city walk keeps none. - const stations = input.tour_type === 'walk' ? waypoints.map((point) => point.key) : []; - await this.visitsRepo.dropStationsNotIn(pid, stations); + const stations = input.tour_type === 'walk' ? walkStationsOf(waypoints).map((point) => point.key) : []; + return await this.visitsRepo.dropStationsNotIn(pid, stations); }); const result = await this.getTour(tripId, placeId); @@ -610,9 +637,28 @@ export class ToursService { } catch { this.logger.warn(`Committed Tour ${placeId}: place notification failed`); } + await this.announceVisits(tripId, visitsDropped); return result; } + /** + * CW-03: the days whose visits an edit took along, told to every client of + * the trip, the one that saved included, so no day plan goes on counting a + * station the walk no longer has. + */ + private async announceVisits(tripId: string, assignmentIds: readonly number[]): Promise { + await Promise.all( + assignmentIds.map(async (assignmentId) => { + const visits = (await this.visitsRepo.listForAssignment(assignmentId)).map(toVisit); + try { + this.places.broadcast(tripId, 'tours:visits', { assignmentId, visits }, undefined); + } catch { + this.logger.warn(`Committed Tour visits of assignment ${assignmentId}: realtime notification failed`); + } + }), + ); + } + /** PR1-10: the names of the places the points stand for, for a reader that wants them spelled out. */ async linkedPlaceNames(tripId: number, waypoints: readonly TourWaypoint[]): Promise> { return await this.waypointsRepo.placeNames( @@ -735,7 +781,6 @@ export class ToursService { has_waypoints: 0, revision: 1, hiking_preferences: null, - station_count: 0, }), ); } diff --git a/server/tests/e2e/tours-walk.e2e.test.ts b/server/tests/e2e/tours-walk.e2e.test.ts index 0a552df4e..edefd189f 100644 --- a/server/tests/e2e/tours-walk.e2e.test.ts +++ b/server/tests/e2e/tours-walk.e2e.test.ts @@ -357,12 +357,22 @@ describe('City walks e2e (real guards + temp SQLite)', () => { expect(reduced).toEqual([200, 200, 200]); const withoutMarket = inOrder([waypoints[0], waypoints[3]]); + realtime.broadcastMock.mockClear(); const edit = await request(server) .put(`/api/trips/5/tours/${placeId}`) .set('Cookie', cookie()) + .set('X-Socket-Id', 'sock-1') .send(walkBody({ waypoints: withoutMarket, expected_revision: 1 })); expect(edit.status).toBe(200); - expect((await visitsOf()).map((visit) => visit.station_key).sort()).toEqual([cathedral, park].sort()); + const left = await visitsOf(); + expect(left.map((visit) => visit.station_key).sort()).toEqual([cathedral, park].sort()); + // The client that saved still holds the market's visit too, so its own socket hears of the change as well. + expect(realtime.broadcastMock).toHaveBeenCalledWith( + '5', + 'tours:visits', + { assignmentId, visits: left }, + undefined, + ); const plain = withoutMarket.map(({ stay_minutes: _s, importance: _i, protected: _p, ...point }) => point); const hike = await request(server) diff --git a/server/tests/unit/db/repositories/TourStationVisits.repository.test.ts b/server/tests/unit/db/repositories/TourStationVisits.repository.test.ts index 7692844ba..953f68de9 100644 --- a/server/tests/unit/db/repositories/TourStationVisits.repository.test.ts +++ b/server/tests/unit/db/repositories/TourStationVisits.repository.test.ts @@ -124,7 +124,7 @@ describe('TourStationVisitsRepository', () => { ]); }); - it('TSVREPO-006: dropStationsNotIn keeps the named stations on every day, drops the rest, and leaves other walks alone', async () => { + it('TSVREPO-006: dropStationsNotIn keeps the named stations on every day, drops the rest, leaves other walks alone and names the days', async () => { const { walk, onDay1, onDay2, foreign } = await fixture(); await markStationVisit(t, onDay1.id, 'stop-a', 'visited'); await markStationVisit(t, onDay1.id, 'stop-b', 'visited'); @@ -132,11 +132,13 @@ describe('TourStationVisitsRepository', () => { await markStationVisit(t, onDay2.id, 'stop-b', 'visited'); await markStationVisit(t, foreign.id, 'stop-b', 'visited'); - await repo.dropStationsNotIn(walk.id, ['stop-a']); + expect(await repo.dropStationsNotIn(walk.id, ['stop-a'])).toEqual([onDay1.id, onDay2.id].sort((a, b) => a - b)); expect((await repo.listForAssignment(onDay1.id)).map((v) => v.station_key)).toEqual(['stop-a']); expect((await repo.listForAssignment(onDay2.id)).map((v) => v.station_key)).toEqual(['stop-a']); + expect(await repo.dropStationsNotIn(walk.id, ['stop-a'])).toEqual([]); - await repo.dropStationsNotIn(walk.id, []); + await markStationVisit(t, onDay2.id, 'stop-c', 'skipped'); + expect(await repo.dropStationsNotIn(walk.id, [])).toEqual([onDay1.id, onDay2.id].sort((a, b) => a - b)); expect(await repo.listForAssignment(onDay1.id)).toEqual([]); expect(await repo.listForAssignment(onDay2.id)).toEqual([]); expect(await repo.listForAssignment(foreign.id)).toEqual([ diff --git a/server/tests/unit/db/repositories/Tours.repository.test.ts b/server/tests/unit/db/repositories/Tours.repository.test.ts index d7d490f3a..5beac8bce 100644 --- a/server/tests/unit/db/repositories/Tours.repository.test.ts +++ b/server/tests/unit/db/repositories/Tours.repository.test.ts @@ -11,7 +11,6 @@ import type { ToursRepository } from '../../../../src/db/repositories/Tours.repo import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { createDay, createDayAssignment, createPlace, createTrip, createUser } from '../../../helpers/factories'; import { insertRow, updateRows } from '../../../helpers/factories/rows'; -import { addTourWaypoints, makeTour } from '../../../helpers/factories/tours'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createTour } from '../../../helpers/tours-repos'; @@ -91,7 +90,6 @@ describe('ToursRepository reads', () => { max_hiking_difficulty: 5, planned: 0, has_waypoints: 1, - station_count: 1, derived_from_place_id: null, origin: 'trek', boat_type: null, @@ -123,7 +121,6 @@ describe('ToursRepository reads', () => { max_hiking_difficulty: 2, planned: 1, has_waypoints: 0, - station_count: 0, derived_from_place_id: null, origin: 'trek', boat_type: null, @@ -193,30 +190,6 @@ describe('ToursRepository reads', () => { expect(await repo.isOnDay(day1.id, plain.id)).toBe(false); expect(await repo.isOnDay(day1.id, tour.id, onDay1.id)).toBe(false); }); - - it('TOURREPO-013: station_count counts the points that are no shaping points, 0 for a tour without any (CW-03)', async () => { - const { trip } = fixture(); - const walk = createPlace(testDb, trip.id, { name: 'Old town' }); - const bare = createPlace(testDb, trip.id, { name: 'Bare' }); - await makeTour(t, walk.id, { tourTypeRef: 'walk' }); - await makeTour(t, bare.id, { tourTypeRef: 'walk' }); - await addTourWaypoints(t, walk.id, [ - { lat: 48.1, lng: 11.5, station_key: 'stop-a' }, - { lat: 48.11, lng: 11.51, shaping: 1 }, - { lat: 48.12, lng: 11.52, station_key: 'stop-b' }, - { lat: 48.13, lng: 11.53, station_key: 'stop-c' }, - ]); - - const counts = new Map((await repo.listForTrip(trip.id)).map((row) => [row.place_id, row.station_count])); - expect(counts).toEqual( - new Map([ - [walk.id, 3], - [bare.id, 0], - ]), - ); - expect((await repo.findInTrip(trip.id, walk.id))?.station_count).toBe(3); - expect((await repo.findInTrip(trip.id, bare.id))?.station_count).toBe(0); - }); }); describe('ToursRepository writes', () => { @@ -235,7 +208,6 @@ describe('ToursRepository writes', () => { wanderer_ref: null, planned: 0, has_waypoints: 0, - station_count: 0, derived_from_place_id: null, origin: 'trek', boat_type: null, diff --git a/server/tests/unit/nest/tour-visits.service.test.ts b/server/tests/unit/nest/tour-visits.service.test.ts index 2fd9a8b65..a69e8d03c 100644 --- a/server/tests/unit/nest/tour-visits.service.test.ts +++ b/server/tests/unit/nest/tour-visits.service.test.ts @@ -271,6 +271,40 @@ describe('TourVisitsService', () => { expect(await countRows(orm(), TourStationVisits, {})).toBe(before); }); + it('TOUR-VISITS-007: the way back of a loop is no station: its start is read once and its key is refused', async () => { + const { trip, day1 } = await fixture(); + const loop = await makePlace(orm(), trip.id, { name: 'Round' }); + await makeTour(orm(), loop.id, { tourTypeRef: 'walk', duration: 60 }); + const cathedral = { + lat: 48.1, + lng: 11.5, + name: 'Cathedral', + stay_minutes: 30, + importance: 'must', + protected: 1, + } as const; + await addTourWaypoints(orm(), loop.id, [ + { ...cathedral, station_key: 'stop-cathedral' }, + { lat: 48.11, lng: 11.51, station_key: 'stop-market' }, + { lat: 48.13, lng: 11.53, station_key: 'stop-park', stay_minutes: 45 }, + // A way back saved as a copy of the start, the way loops were closed at first. + { ...cathedral, station_key: 'stop-back' }, + ]); + const onDay1 = await makeDayAssignment(orm(), day1.id, loop.id); + await markStationVisit(orm(), onDay1.id, 'stop-cathedral', 'visited'); + + const progress = await visits.progress(trip.id, loop.id, day1.id); + expect(progress.stations.map((station) => [station.name, station.status])).toEqual([ + ['Cathedral', 'visited'], + [null, 'planned'], + [null, 'planned'], + ]); + expect(progress.open).toEqual({ stations: 2, stay_minutes_known: 45, stays_unknown: 1 }); + await expect(visits.update(String(trip.id), onDay1.id, 'stop-back', { status: 'visited' })).rejects.toEqual( + new NotFoundException('Station not found'), + ); + }); + it('TOUR-VISITS-006: progress of a walk without stations or walking time, and of a walk not on that day', async () => { const { trip, day3, walk, other, otherDay, otherWalk, foreign } = await fixture(); expect(await visits.progress(other.id, otherWalk.id, otherDay.id)).toEqual({ diff --git a/server/tests/unit/nest/tours.mcp.test.ts b/server/tests/unit/nest/tours.mcp.test.ts index f491d5323..ef1bb618f 100644 --- a/server/tests/unit/nest/tours.mcp.test.ts +++ b/server/tests/unit/nest/tours.mcp.test.ts @@ -21,6 +21,8 @@ import { createTrip, createUser, } from '../../helpers/factories'; +import { makeDayAssignment } from '../../helpers/factories/itinerary'; +import { makePlace } from '../../helpers/factories/places'; import { countRows, insertRows, updateRows } from '../../helpers/factories/rows'; import { addTourWaypoints, makeTour, markStationVisit } from '../../helpers/factories/tours'; import { resetTestDb } from '../../helpers/test-db'; @@ -180,15 +182,15 @@ describe('ToursMcp', () => { /** CW-09: a city walk on day 1 with three stops, the first visited and the second skipped that day. */ async function walkFixture() { const base = fixture(); - const walk = createPlace(db, base.trip.id, { name: 'Old town' }); const orm = await createTestOrm(db); + const walk = await makePlace(orm, base.trip.id, { name: 'Old town' }); await makeTour(orm, walk.id, { tourTypeRef: 'walk', duration: 80 }); await addTourWaypoints(orm, walk.id, [ { lat: 48.1, lng: 11.5, station_key: 'stop-cathedral', name: 'Cathedral', stay_minutes: 30 }, { lat: 48.11, lng: 11.51, station_key: 'stop-market', stay_minutes: 20 }, { lat: 48.12, lng: 11.52, station_key: 'stop-park', importance: 'must' }, ]); - const onDay1 = createDayAssignment(db, base.day1.id, walk.id); + const onDay1 = await makeDayAssignment(orm, base.day1.id, walk.id); await markStationVisit(orm, onDay1.id, 'stop-cathedral', 'visited'); await markStationVisit(orm, onDay1.id, 'stop-market', 'skipped'); await orm.close(); @@ -221,8 +223,10 @@ describe('ToursMcp', () => { expect( read(await mcp.getCityWalkProgress({ tripId: trip.id, placeId: walk.id, dayId: day1.id }, ctx(outsider.id))), ).toEqual({ error: 'Trip not found or access denied.' }); - const cafe = createPlace(db, trip.id, { name: 'Plain' }); - createDayAssignment(db, day1.id, cafe.id); + const orm = await createTestOrm(db); + const cafe = await makePlace(orm, trip.id, { name: 'Plain' }); + await makeDayAssignment(orm, day1.id, cafe.id); + await orm.close(); const notThere = [ { placeId: walk.id, dayId: day2.id }, { placeId: ridge.id, dayId: day1.id }, diff --git a/server/tests/unit/nest/tours.permissions.boundaries.mock-only.test.ts b/server/tests/unit/nest/tours.permissions.boundaries.mock-only.test.ts index 993b92d40..ee2f6f4f9 100644 --- a/server/tests/unit/nest/tours.permissions.boundaries.mock-only.test.ts +++ b/server/tests/unit/nest/tours.permissions.boundaries.mock-only.test.ts @@ -308,7 +308,7 @@ function tourRepositoryFixture(ownerTripId: number) { toursRepo as unknown as ToursRepository, { isEnabled: vi.fn(async () => true) } as unknown as TourTypesRepository, waypointsRepo as unknown as TourWaypointsRepository, - { dropStationsNotIn: vi.fn(async () => undefined) } as unknown as TourStationVisitsRepository, + { dropStationsNotIn: vi.fn(async () => []) } as unknown as TourStationVisitsRepository, placesRepo as unknown as PlacesRepository, buildPlaceImportService(), noPaddleChecks(), diff --git a/server/tests/unit/nest/tours.service.permissions.test.ts b/server/tests/unit/nest/tours.service.permissions.test.ts index fd90f1cfb..08e686108 100644 --- a/server/tests/unit/nest/tours.service.permissions.test.ts +++ b/server/tests/unit/nest/tours.service.permissions.test.ts @@ -122,7 +122,7 @@ function makeService() { insertForPlace: inTx(undefined), deleteForPlace: inTx(undefined), }; - const visitsRepo = { dropStationsNotIn: inTx(undefined) }; + const visitsRepo = { dropStationsNotIn: inTx([]) }; const placesRepo = { insertTourPlace: inTx(42), updateTourRoute: inTx(true), diff --git a/server/tests/unit/nest/tours.walk.service.test.ts b/server/tests/unit/nest/tours.walk.service.test.ts index 81ffe7ab8..ca15c994e 100644 --- a/server/tests/unit/nest/tours.walk.service.test.ts +++ b/server/tests/unit/nest/tours.walk.service.test.ts @@ -44,7 +44,10 @@ beforeAll(async () => { tours = t.get(ToursService); visits = t.get(TourVisitsService); }); -beforeEach(() => resetTestDb(db)); +beforeEach(() => { + resetTestDb(db); + t.realtime.broadcastMock.mockReset(); +}); afterAll(async () => { await t.close(); db.close(); @@ -141,17 +144,39 @@ describe('ToursService and city walks', () => { // The market is left out; the others come back with their keys, in another order. const kept = inOrder([park, cathedral]); - await tours.updateTour(tripId, String(placeId), walkBody({ waypoints: kept, expected_revision: 1 })); + await tours.updateTour(tripId, String(placeId), walkBody({ waypoints: kept, expected_revision: 1 }), 'sock-1'); const statusOf = async (assignmentId: number) => (await visits.walkOnDay(tripId, assignmentId)).visits.map((visit) => [visit.station_key, visit.status]); expect(await statusOf(onDay1.id)).toEqual([[cathedral.key, 'visited']]); expect(await statusOf(onDay2.id)).toEqual([[park.key, 'visited']]); expect(await storedKeys(placeId)).toEqual([park.key, cathedral.key]); + // Every client of the trip hears of both days, the one that saved too. + const visitsOf = (assignmentId: number, key: string | undefined) => ({ + assignmentId, + visits: [{ assignment_id: assignmentId, station_key: key, status: 'visited' }], + }); + expect(t.realtime.broadcastMock).toHaveBeenCalledWith( + tripId, + 'tours:visits', + visitsOf(onDay1.id, cathedral.key), + undefined, + ); + expect(t.realtime.broadcastMock).toHaveBeenCalledWith( + tripId, + 'tours:visits', + visitsOf(onDay2.id, park.key), + undefined, + ); + + // A save that takes no station away has no visits to tell of. + t.realtime.broadcastMock.mockClear(); + await tours.updateTour(tripId, String(placeId), walkBody({ waypoints: kept, expected_revision: 2 })); + expect(t.realtime.broadcastMock.mock.calls.map(([, event]) => event)).not.toContain('tours:visits'); await tours.updateTour( tripId, String(placeId), - walkBody({ tour_type: 'hike', waypoints: kept.map(plainPoint), expected_revision: 2 }), + walkBody({ tour_type: 'hike', waypoints: kept.map(plainPoint), expected_revision: 3 }), ); expect(await countRows(orm(), TourStationVisits, {})).toBe(0); expect(await storedKeys(placeId)).toEqual([park.key, cathedral.key]); @@ -185,6 +210,50 @@ describe('ToursService and city walks', () => { expect(new Set(keys).size).toBe(3); }); + it('TOURS-WALK-006: the way back of a loop is no station: counted once, and its visits go with the next save', async () => { + const { tripId, day1 } = await tripWithDays(); + // A loop saved before the way back stopped copying the start: its own key, the start's name, stay and protection. + const back = { ...STATIONS[0], key: 'stop-back' }; + const created = await tours.createTour(tripId, walkBody({ waypoints: inOrder([...STATIONS, back]) })); + const placeId = created.tour.place_id; + expect(created.tour.station_count).toBe(3); + expect((await tours.listTours(tripId)).map((item) => item.station_count)).toEqual([3]); + expect((await tours.getTour(tripId, String(placeId))).tour.station_count).toBe(3); + + const onDay1 = await makeDayAssignment(orm(), day1.id, placeId); + const walk = await visits.walkOnDay(tripId, onDay1.id); + expect(walk.stations.map((station) => station.key)).toEqual([ + 'stop-cathedral', + created.waypoints[2].key, + created.waypoints[3].key, + ]); + await markStationVisit(orm(), onDay1.id, 'stop-cathedral', 'visited'); + await markStationVisit(orm(), onDay1.id, 'stop-back', 'visited'); + + // A telling that fails leaves the committed save as it is. + t.realtime.broadcastMock.mockImplementation((_trip: string, event: string) => { + if (event === 'tours:visits') throw new Error('socket gone'); + }); + const saved = await tours.updateTour( + tripId, + String(placeId), + walkBody({ waypoints: created.waypoints, expected_revision: 1 }), + ); + expect(saved.tour).toMatchObject({ station_count: 3, revision: 2 }); + expect((await visits.walkOnDay(tripId, onDay1.id)).visits).toEqual([ + { assignment_id: onDay1.id, station_key: 'stop-cathedral', status: 'visited' }, + ]); + expect(t.realtime.broadcastMock).toHaveBeenCalledWith( + tripId, + 'tours:visits', + { + assignmentId: onDay1.id, + visits: [{ assignment_id: onDay1.id, station_key: 'stop-cathedral', status: 'visited' }], + }, + undefined, + ); + }); + it('TOURS-WALK-005: a GPX file taken in as a city walk has no stations yet', async () => { const { tripId } = await tripWithDays(); const gpx = ` diff --git a/shared/src/index.ts b/shared/src/index.ts index 2ad328716..83b6dc966 100644 --- a/shared/src/index.ts +++ b/shared/src/index.ts @@ -60,6 +60,7 @@ export * from './tours/tour-library.schema'; export * from './tours/tour-poi-search.schema'; export * from './tours/tour-trail-grade.schema'; export * from './tours/tour-walk.schema'; +export * from './tours/tour-walk'; export * from './tours/tours.metrics'; export * from './tours/tour-gpx-import.schema'; export * from './tours/paddle-plan.schema'; diff --git a/shared/src/tours/tour-walk.spec.ts b/shared/src/tours/tour-walk.spec.ts new file mode 100644 index 000000000..b20ead7d3 --- /dev/null +++ b/shared/src/tours/tour-walk.spec.ts @@ -0,0 +1,35 @@ +import { isClosedLoop, walkStationsOf } from './tour-walk'; + +import { describe, expect, it } from 'vitest'; + +const cathedral = { lat: 48.1, lng: 11.5, name: 'Cathedral' }; +const market = { lat: 48.11, lng: 11.51, name: 'Market' }; +const park = { lat: 48.13, lng: 11.53, name: 'Park' }; +const steer = { lat: 48.105, lng: 11.505, shaping: true }; + +describe('isClosedLoop (PR1-01)', () => { + it('is a tour of three points or more whose last stands on the spot of its first', () => { + expect(isClosedLoop([cathedral, market, { ...cathedral }])).toBe(true); + expect(isClosedLoop([cathedral, market, { lat: cathedral.lat + 5e-8, lng: cathedral.lng - 5e-8 }])).toBe(true); + expect(isClosedLoop([cathedral, market, park])).toBe(false); + expect(isClosedLoop([cathedral, market, { lat: cathedral.lat + 1e-6, lng: cathedral.lng }])).toBe(false); + }); + + it('is no loop with two points on one spot, or with none', () => { + expect(isClosedLoop([cathedral, { ...cathedral }])).toBe(false); + expect(isClosedLoop([])).toBe(false); + }); +}); + +describe('walkStationsOf (CW-02, CW-03)', () => { + it('lists the stops in order and leaves a shaping point out', () => { + expect(walkStationsOf([cathedral, steer, market, park])).toEqual([cathedral, market, park]); + }); + + it('leaves the way back of a loop out, so its start is one station', () => { + const back = { ...cathedral, name: 'Cathedral again' }; + expect(walkStationsOf([cathedral, steer, market, park, back])).toEqual([cathedral, market, park]); + expect(walkStationsOf([cathedral, { ...cathedral }])).toEqual([cathedral, cathedral]); + expect(walkStationsOf([])).toEqual([]); + }); +}); diff --git a/shared/src/tours/tour-walk.ts b/shared/src/tours/tour-walk.ts new file mode 100644 index 000000000..1f370e2d4 --- /dev/null +++ b/shared/src/tours/tour-walk.ts @@ -0,0 +1,33 @@ +/** Closer than this, in degrees, two points of a tour stand on the same spot (about a centimetre). */ +const SAME_SPOT_DEG = 1e-7; + +interface Spot { + lat: number; + lng: number; +} + +function sameSpot(a: Spot, b: Spot): boolean { + return Math.abs(a.lat - b.lat) < SAME_SPOT_DEG && Math.abs(a.lng - b.lng) < SAME_SPOT_DEG; +} + +/** + * PR1-01: whether a tour ends where it starts. Read from the points + * themselves, so a saved loop opens as a loop again without a flag of its + * own; a tour of two points on one spot is no loop, there is nothing to + * return from. + */ +export function isClosedLoop(points: readonly Spot[]): boolean { + const first = points[0]; + const last = points[points.length - 1]; + return points.length >= 3 && !!first && !!last && sameSpot(first, last); +} + +/** + * CW-02, CW-03: the stations of a city walk in their order. A shaping point + * only steers the route, and the way back of a loop is its start again, so + * neither is a station: nothing to stay at, check off or count twice. + */ +export function walkStationsOf(points: readonly T[]): T[] { + const route = isClosedLoop(points) ? points.slice(0, -1) : points; + return route.filter((point) => !point.shaping); +}