From 78548ca2ba18a9406e290f165de5d59d170f8913 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 29 Aug 2026 16:04:00 +0200 Subject: [PATCH] fix(collections): ask the same question at all three call sites savePlace was not the only caller of findDuplicateCollectionPlace. The bulk copy already carries google_place_id/google_ftid/osm_id into the row it writes but asked without them, and the import picker never selected them at all, so a renamed place was recognised on one path and not on the other two. The picker answering differently from the import is the drift the method exists to prevent: a row shown as new would come back refused. COLLECTIONS-SVC-102 and -103 cover the bulk copy and the picker. --- .../nest/collections/collections.service.ts | 17 ++++++++-- .../unit/nest/collections.service.test.ts | 32 +++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 200ed40d0..2c8b651e4 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -610,8 +610,10 @@ export class CollectionsService { const rows = this.db.all<{ place_id: number; name: string; address: string | null; lat: number | null; lng: number | null; category_id: number | null; image_url: string | null; day_number: number | null; date: string | null; + google_place_id: string | null; google_ftid: string | null; osm_id: string | null; }>(` SELECT p.id AS place_id, p.name, p.address, p.lat, p.lng, p.category_id, p.image_url, + p.google_place_id, p.google_ftid, p.osm_id, (SELECT MIN(d.day_number) FROM day_assignments da JOIN days d ON d.id = da.day_id WHERE da.place_id = p.id AND d.trip_id = p.trip_id) AS day_number, @@ -627,7 +629,9 @@ export class CollectionsService { return { places: rows.map(r => ({ ...r, - already_in_list: this.findDuplicateCollectionPlace(collectionId, { name: r.name, lat: r.lat, lng: r.lng }) != null, + // Asked with the same candidate the save will use, or the picker marks a + // place as new and the save then refuses it as a duplicate. + already_in_list: this.findDuplicateCollectionPlace(collectionId, r) != null, scheduled: r.day_number != null, })), }; @@ -660,7 +664,16 @@ export class CollectionsService { const name = p.name as string; const lat = (p.lat as number | null) ?? null; const lng = (p.lng as number | null) ?? null; - if (!force && this.findDuplicateCollectionPlace(collectionId, { name, lat, lng })) { + // The provider ids go with it: they are already carried into the insert + // below, so leaving them out here would recognise less than the row that + // gets written knows about. + const candidate = { + name, lat, lng, + google_place_id: (p.google_place_id as string | null) ?? null, + google_ftid: (p.google_ftid as string | null) ?? null, + osm_id: (p.osm_id as string | null) ?? null, + }; + if (!force && this.findDuplicateCollectionPlace(collectionId, candidate)) { skipped.push({ id: placeId, name }); continue; } diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index 60309ee88..2ec3c1f38 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -220,6 +220,38 @@ describe('saved places + dedup', () => { expect(result.duplicate).toBe(true); expect(result.duplicateOf?.name).toBe('Original Name'); }); + + it('COLLECTIONS-SVC-102: the bulk import recognises a renamed place by its provider id too', () => { + // savePlace was not the only caller. The bulk copy carries the provider ids + // into the row it writes, so asking without them would recognise less than + // the row it just wrote already knows. + const u = createUser(testDb).user; + const col = svc.createCollection(u.id, { name: 'Rome' }); + const trip = createTrip(testDb, u.id); + const place = createPlace(testDb, trip.id, { name: 'Trattoria da Enzo' }); + testDb.prepare('UPDATE places SET google_ftid = ? WHERE id = ?').run('0x1:0x2', place.id); + svc.savePlace(u.id, { collection_id: col.id, name: 'Dinner Tuesday', lat: 41.88, lng: 12.47, google_ftid: '0x1:0x2' }); + + const out = svc.saveFromTripPlaces(u.id, col.id, trip.id, [place.id]); + + expect(out.copied).toBe(0); + expect(out.skipped.map(s => s.name)).toEqual(['Trattoria da Enzo']); + }); + + it('COLLECTIONS-SVC-103: the import picker marks that same place as already saved', () => { + // The dialog and the import have to agree: a row shown as new that the import + // then refuses is the drift this method exists to prevent. + const u = createUser(testDb).user; + const col = svc.createCollection(u.id, { name: 'Rome' }); + const trip = createTrip(testDb, u.id); + const place = createPlace(testDb, trip.id, { name: 'Trattoria da Enzo' }); + testDb.prepare('UPDATE places SET google_ftid = ? WHERE id = ?').run('0x1:0x2', place.id); + svc.savePlace(u.id, { collection_id: col.id, name: 'Dinner Tuesday', lat: 41.88, lng: 12.47, google_ftid: '0x1:0x2' }); + + const listed = svc.importablePlaces(u.id, col.id, trip.id).places.find(p => p.place_id === place.id); + + expect(listed?.already_in_list).toBe(true); + }); }); // ── save-from-trip provenance + IDOR ─────────────────────────────────────────