From e677d58f9112bffbbf1f70e9c97ba2e4760ef504 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 26 Oct 2026 14:08:56 +0100 Subject: [PATCH] fix(places): a track is a duplicate only of a track, never of a sight of that name A GPX or KML import skipped every row whose name was already a place of the trip. A watch names a hike after where it went, so "Hokitika Gorge" was refused as already in the trip because the gorge itself was a planned place, with no track to show for it (#2810). A track now matches only the tracks of that name; a pin still matches every place of its name. The Places filter also hid its "Tracks" choice whenever Tours was on, on the grounds that tracks are tours then. A track imported before Tours is a plain place and stays in the list, so the choice now counts the tracks that are no tour. --- .../components/Planner/usePlacesPool.test.ts | 8 ++++-- .../src/components/Planner/usePlacesPool.ts | 8 ++++-- .../src/db/repositories/Places.repository.ts | 6 +++++ server/src/nest/places/places.helpers.ts | 18 ++++++++++--- server/src/nest/places/places.service.ts | 12 ++++----- server/tests/e2e/tours.e2e.test.ts | 15 +++++++++++ .../db/repositories/Places.repository.test.ts | 12 +++++++++ server/tests/unit/nest/places.helpers.test.ts | 27 +++++++++++++++++++ 8 files changed, 92 insertions(+), 14 deletions(-) diff --git a/client/src/components/Planner/usePlacesPool.test.ts b/client/src/components/Planner/usePlacesPool.test.ts index 1dc445f81..269890b9f 100644 --- a/client/src/components/Planner/usePlacesPool.test.ts +++ b/client/src/components/Planner/usePlacesPool.test.ts @@ -48,13 +48,17 @@ afterEach(() => { }); describe('places pool helpers', () => { - it('FE-PLANNER-POOL-001: the Tracks choice needs a track and stays away while tours are on', () => { + it('FE-PLANNER-POOL-001: the Tracks choice needs a track that is no tour while tours are on (#2810)', () => { const track = buildPlace({ route_geometry: '[[1,2],[3,4]]' }); + const tour = buildPlace({ route_geometry: '[[1,2],[3,4]]' }); + tour.tour_place_id = tour.id; const pin = buildPlace(); expect(poolHasTracks([pin], false)).toBe(false); expect(poolHasTracks([pin, track], false)).toBe(true); expect(poolHasTracks([pin, track], undefined)).toBe(true); - expect(poolHasTracks([pin, track], true)).toBe(false); + expect(poolHasTracks([pin, tour], true)).toBe(false); + expect(poolHasTracks([pin, tour], false)).toBe(true); + expect(poolHasTracks([pin, tour, track], true)).toBe(true); }); it('FE-PLANNER-POOL-002: the show choices list all, unplanned and planned, then tracks when there are some', () => { diff --git a/client/src/components/Planner/usePlacesPool.ts b/client/src/components/Planner/usePlacesPool.ts index 71d9f7ddf..04920f262 100644 --- a/client/src/components/Planner/usePlacesPool.ts +++ b/client/src/components/Planner/usePlacesPool.ts @@ -12,9 +12,13 @@ export type PlacesFilter = 'all' | 'unplanned' | 'planned' | 'tracks'; type Translate = ReturnType['t']; -/** Whether the "Tracks" choice belongs in the pool: never while tours are on, since tracks are tours then. */ +/** + * Whether the "Tracks" choice belongs in the pool: a place with a line that is + * no tour. While tours are on a tour lists under Tours, but a track imported + * before them (4.x) is a plain place and stays here (#2810). + */ export function poolHasTracks(places: Place[], toursEnabled: boolean | undefined): boolean { - return !toursEnabled && places.some((p) => p.route_geometry); + return places.some((p) => p.route_geometry && !(toursEnabled && p.tour_place_id === p.id)); } /** The "show" choices; tracks only once a place has one. */ diff --git a/server/src/db/repositories/Places.repository.ts b/server/src/db/repositories/Places.repository.ts index 2b9d8c2fb..f1fa5d469 100644 --- a/server/src/db/repositories/Places.repository.ts +++ b/server/src/db/repositories/Places.repository.ts @@ -590,6 +590,12 @@ export class PlacesRepository extends TrekRepository { .execute<{ name: string; description: string | null; address: string | null; lat: number | null; lng: number | null; route_geometry: string | null; category: string | null }[]>('all', false); } + /** The names of the trip's places that carry a line, which a track to import is matched against (#2810). */ + async listTrackNames(trip_id: number): Promise { + const rows = await this.find({ trip: trip_id, route_geometry: { $ne: null } }, { fields: ['name'] }); + return rows.map((row) => row.name); + } + /** * PL24 (`PlacesService.buildDedupSet`'s read) — `SELECT name, lat, lng, * google_place_id, google_ftid, osm_id, amap_poi_id FROM places WHERE diff --git a/server/src/nest/places/places.helpers.ts b/server/src/nest/places/places.helpers.ts index 78a143416..491e555e4 100644 --- a/server/src/nest/places/places.helpers.ts +++ b/server/src/nest/places/places.helpers.ts @@ -86,11 +86,16 @@ export async function reclaimPhotoCache( export interface DedupSet { names: Set; + /** The names of the places with a line: a track to import matches only these (#2810). Absent where none is read. */ + trackNames?: Set; coords: Array<{ lat: number; lng: number }>; /** Provider ids (google_place_id, google_ftid, osm_id, amap_poi_id) of the places already in the trip. */ externalIds: Set; } +/** A candidate for the dedup set; `track` when it is a line (a GPX track or route, a KML path). */ +export type DedupCandidate = PlaceMatchCandidate & { track?: boolean }; + /** * Returns true if a candidate place is already represented in the dedup set. * @@ -104,12 +109,14 @@ export interface DedupSet { * JavaScript's is not, and only unnamed rows contribute coordinates to a dedup * set. `findDuplicatePlace` spells out where the two still answer differently. */ -export function isPlaceDuplicate(candidate: PlaceMatchCandidate, dedup: DedupSet): boolean { +export function isPlaceDuplicate(candidate: DedupCandidate, dedup: DedupSet): boolean { for (const strategy of placeMatchStrategies(candidate)) { if (strategy.by === 'externalId') { if (dedup.externalIds.has(strategy.id)) return true; } else if (strategy.by === 'name') { - if (dedup.names.has(strategy.name)) return true; + // A track is the same as a track of that name, never as a sight of that name: + // a watch names a hike after the place it went to (#2810). + if ((candidate.track ? dedup.trackNames : dedup.names)?.has(strategy.name)) return true; } else if ( dedup.coords.some( (c) => @@ -132,6 +139,7 @@ export function dedupSetOf( rows: ReadonlyArray< { name: string | null; lat: number | null; lng: number | null } & Parameters[0] >, + trackNames: ReadonlyArray = [], ): DedupSet { const names = new Set(); const coords: Array<{ lat: number; lng: number }> = []; @@ -144,15 +152,17 @@ export function dedupSetOf( coords.push({ lat: row.lat, lng: row.lng }); } } - return { names, coords, externalIds }; + const tracks = trackNames.map(normalizePlaceName).filter((name): name is string => !!name); + return { names, trackNames: new Set(tracks), coords, externalIds }; } /** Record a newly inserted place so subsequent candidates in the same batch are checked against it. */ -export function trackInsertedInDedupSet(place: PlaceMatchCandidate, dedup: DedupSet): void { +export function trackInsertedInDedupSet(place: DedupCandidate, dedup: DedupSet): void { for (const id of externalIdsOf(place)) dedup.externalIds.add(id); const normalizedName = normalizePlaceName(place.name); if (normalizedName) { dedup.names.add(normalizedName); + if (place.track) (dedup.trackNames ??= new Set()).add(normalizedName); } else if (place.lat != null && place.lng != null) { dedup.coords.push({ lat: place.lat, lng: place.lng }); } diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 49414aecb..1fb206035 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -751,14 +751,14 @@ export class PlacesService { /** Build a lookup of names/coords for places already in a trip. */ private async buildDedupSet(tripId: string): Promise { // PL24 — `PlacesRepository.listDedupInputs` replaces the raw statement (Task 5 review L6 ruling). - return dedupSetOf(await this.placesRepo.listDedupInputs(tripId)); + return dedupSetOf(await this.placesRepo.listDedupInputs(tripId), await this.placesRepo.listTrackNames(toRowId(tripId) ?? -1)); } /** Which prepared GPX rows the import would skip as already in the trip: its own rule, read only. */ async gpxDuplicates(tripId: string, rows: PreparedGpxPlace[]): Promise { const dedup = await this.buildDedupSet(tripId); return rows.map((wp) => { - const candidate = { name: wp.name, lat: wp.lat, lng: wp.lng }; + const candidate = { name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry }; if (isPlaceDuplicate(candidate, dedup)) return true; trackInsertedInDedupSet(candidate, dedup); return false; @@ -925,7 +925,7 @@ export class PlacesService { // `findWithTagsAndRatings` INSIDE the caller's transaction, so the read // sees the still-uncommitted row. for (const wp of waypoints) { - if (isPlaceDuplicate({ name: wp.name, lat: wp.lat, lng: wp.lng }, dedup)) { + if (isPlaceDuplicate({ name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry }, dedup)) { skipped++; continue; } @@ -960,7 +960,7 @@ export class PlacesService { }); const place = (await this.placesRepo.findWithTagsAndRatings(placeId))!; created.push(place); - trackInsertedInDedupSet({ name: wp.name, lat: wp.lat, lng: wp.lng }, dedup); + trackInsertedInDedupSet({ name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry }, dedup); } return { places: created, count: created.length, skipped }; @@ -1033,7 +1033,7 @@ export class PlacesService { const fallbackName = `Placemark ${fallbackIndex}`; const name = parsedPlacemark.name || fallbackName; - if (isPlaceDuplicate({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng }, dedup)) { + if (isPlaceDuplicate({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng, track: isPath }, dedup)) { summary.skippedCount += 1; dupCount++; fallbackIndex += 1; @@ -1079,7 +1079,7 @@ export class PlacesService { const place = (await this.placesRepo.findWithTagsAndRatings(placeId))!; created.push(place); - trackInsertedInDedupSet({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng }, dedup); + trackInsertedInDedupSet({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng, track: isPath }, dedup); summary.createdCount += 1; fallbackIndex += 1; } diff --git a/server/tests/e2e/tours.e2e.test.ts b/server/tests/e2e/tours.e2e.test.ts index d0b7d1fc6..7088a5d6f 100644 --- a/server/tests/e2e/tours.e2e.test.ts +++ b/server/tests/e2e/tours.e2e.test.ts @@ -565,6 +565,21 @@ describe('Tours e2e (real guards + temp SQLite)', () => { expect((await importGpx(OWNER, TRACK_GPX)).body).toEqual({ tours: [], caution: false, skipped: 1 }); }); + it('imports a hike named after a sight of the trip instead of skipping it as a duplicate (#2810)', async () => { + // A watch names the hike after where it went, and that place is already planned. + const sight = (await makePlace(orm, 5, { name: 'alpine loop', lat: 47, lng: 11 })).id; + + const preview = await previewGpx(OWNER, TRACK_GPX); + expect(preview.body.tracks[0]).toMatchObject({ name: 'Alpine loop', duplicate: false }); + + const res = await importGpx(OWNER, TRACK_GPX); + expect(res.status).toBe(201); + expect(res.body).toMatchObject({ skipped: 0 }); + expect(res.body.tours).toHaveLength(1); + expect(res.body.tours[0].place_id).not.toBe(sight); + expect(await placeColumns(sight, 'route_geometry')).toEqual({ route_geometry: null }); + }); + it('400 without a file, and for a file with no track or route', async () => { const missing = await importGpx(OWNER, null); expect(missing.status).toBe(400); diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index 1b307c3e9..311d9394c 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -1587,4 +1587,16 @@ describe('PlacesRepository.isTour / listForTrip tour_place_id', () => { expect(byId.get(tour.id)).toBe(tour.id); expect(byId.get(plain.id)).toBeNull(); }); + + it("PLACEREPO-TOUR-005: listTrackNames lists the names of the trip's places with a line (#2810)", async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const other = createTrip(testDb, user.id); + const line = '[[49,12],[49.01,12.01]]'; + const tracks = [createPlace(testDb, trip.id, { name: 'Garmin hike' }), createPlace(testDb, trip.id, { name: 'Ridge walk' }), createPlace(testDb, other.id, { name: 'Elsewhere' })]; + await updateRows(t.orm, Places, { id: { $in: tracks.map((p) => p.id) } }, { route_geometry: line }); + createPlace(testDb, trip.id, { name: 'Hokitika Gorge' }); + + expect((await places.listTrackNames(trip.id)).sort()).toEqual(['Garmin hike', 'Ridge walk']); + }); }); diff --git a/server/tests/unit/nest/places.helpers.test.ts b/server/tests/unit/nest/places.helpers.test.ts index 59b48be4b..e02231bdf 100644 --- a/server/tests/unit/nest/places.helpers.test.ts +++ b/server/tests/unit/nest/places.helpers.test.ts @@ -5,6 +5,7 @@ */ import { COORD_DEDUP_TOLERANCE, + dedupSetOf, externalIdsOf, isPlaceDuplicate, mapWithConcurrency, @@ -25,6 +26,32 @@ vi.mock('../../../src/db/database', () => ({ const emptyDedup = (): DedupSet => ({ names: new Set(), coords: [], externalIds: new Set() }); describe('isPlaceDuplicate / trackInsertedInDedupSet', () => { + it('matches a track only against the tracks of that name, never a sight of that name (#2810)', () => { + // The watch named the hike after the gorge, and the gorge is already a place of the trip. + const dedup = dedupSetOf( + [ + { name: 'Hokitika Gorge', lat: -42.95, lng: 171.0 }, + { name: '008', lat: 1, lng: 2 }, + ], + [' 008 ', null], + ); + expect(isPlaceDuplicate({ name: 'hokitika gorge', lat: -42.9, lng: 171.0, track: true }, dedup)).toBe(false); + expect(isPlaceDuplicate({ name: '008', lat: 1, lng: 2, track: true }, dedup)).toBe(true); + // A pin still matches every place of its name, a track included. + expect(isPlaceDuplicate({ name: 'Hokitika Gorge', lat: null, lng: null }, dedup)).toBe(true); + expect(isPlaceDuplicate({ name: '008', lat: null, lng: null }, dedup)).toBe(true); + + // A track imported earlier in the same file counts for the next one. + trackInsertedInDedupSet({ name: 'Hokitika Gorge', lat: -42.9, lng: 171.0, track: true }, dedup); + expect(isPlaceDuplicate({ name: 'Hokitika gorge ', lat: null, lng: null, track: true }, dedup)).toBe(true); + // A set built by hand without tracks matches no track by name. + const bare = emptyDedup(); + trackInsertedInDedupSet({ name: 'Ridge', lat: 1, lng: 2 }, bare); + expect(isPlaceDuplicate({ name: 'Ridge', lat: 1, lng: 2, track: true }, bare)).toBe(false); + trackInsertedInDedupSet({ name: 'Lake', lat: 1, lng: 2, track: true }, bare); + expect([...(bare.trackNames ?? [])]).toEqual(['lake']); + }); + it('matches a named place case- and whitespace-insensitively', () => { const dedup = emptyDedup(); trackInsertedInDedupSet({ name: ' Eiffel Tower ', lat: 1, lng: 2 }, dedup);