mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
refactor(places): one door onto the place-matching rule
Two consumers decide whether a candidate place is one the trip already has, and they spelled the rule out separately: the bulk importers ask an in-memory set (isPlaceDuplicate), while findDuplicatePlace asks the database because its caller needs the matched row's id. The two had drifted — the SQL copy falls back to a coordinate match for a NAMED candidate, which the in-memory copy deliberately never does, since that would merge the restaurant and the bar at the same address. The order now lives once in @trek/shared as placeMatchStrategies(), and each consumer is a mechanical interpreter of it: provider id (the only part that survives a rename, #1550), then name, then coordinates and only when there is no name. Adds PlacesService.findMatchingPlaceId as the public door for an importer that has to link to the existing place rather than merely skip the candidate. findDuplicatePlace stays private behind it — it also returns google_ftid for the bulk importer's backfill, which is that caller's business and not part of "which place is this?". No behaviour change for either existing caller. The coordinate branch is currently unreachable from the one call site, which sits inside an isPlaceDuplicate() guard that has already matched on name and returned; the drift only bites once something calls it standalone. COORD_DEDUP_TOLERANCE and externalIdsOf move to shared and are re-exported from places.helpers, so collections.service and places.service keep their imports.
This commit is contained in:
@@ -35,6 +35,7 @@ export * from './day/day.schema';
|
||||
export * from './day/note-colors';
|
||||
export * from './assignment/assignment.schema';
|
||||
export * from './place/place.schema';
|
||||
export * from './place/place-match';
|
||||
export * from './place/track-colors';
|
||||
export * from './collection/collection.schema';
|
||||
export * from './trip/trip.schema';
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
import { COORD_DEDUP_TOLERANCE, externalIdsOf, normalizePlaceName, placeMatchStrategies } from './place-match';
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
|
||||
describe('normalizePlaceName', () => {
|
||||
it('trims and lowercases', () => {
|
||||
expect(normalizePlaceName(' Eiffel Tower ')).toBe('eiffel tower');
|
||||
});
|
||||
|
||||
it('treats blank, whitespace, null and undefined alike as no name', () => {
|
||||
expect(normalizePlaceName('')).toBeNull();
|
||||
expect(normalizePlaceName(' ')).toBeNull();
|
||||
expect(normalizePlaceName(null)).toBeNull();
|
||||
expect(normalizePlaceName(undefined)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('externalIdsOf', () => {
|
||||
it('collects the provider ids that carry a value, trimmed', () => {
|
||||
expect(externalIdsOf({ google_place_id: ' ChIJ_a ', google_ftid: '0x1:0x2', osm_id: null })).toEqual([
|
||||
'ChIJ_a',
|
||||
'0x1:0x2',
|
||||
]);
|
||||
});
|
||||
|
||||
it('ignores blank ids', () => {
|
||||
expect(externalIdsOf({ google_ftid: ' ', osm_id: '' })).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps provider order: place_id, ftid, osm', () => {
|
||||
expect(externalIdsOf({ osm_id: 'node/42', google_ftid: 'f', google_place_id: 'p' })).toEqual(['p', 'f', 'node/42']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('placeMatchStrategies', () => {
|
||||
it('puts every provider id first, one strategy each, in provider order', () => {
|
||||
expect(placeMatchStrategies({ name: 'Trattoria', google_place_id: 'p', osm_id: 'node/42' })).toEqual([
|
||||
{ by: 'externalId', id: 'p' },
|
||||
{ by: 'externalId', id: 'node/42' },
|
||||
{ by: 'name', name: 'trattoria' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('never offers coordinates for a NAMED candidate, even when it has them', () => {
|
||||
// The whole point of the rule: widening the coordinate check to named places
|
||||
// would merge the restaurant and the bar in the same building.
|
||||
const strategies = placeMatchStrategies({ name: 'Ground Floor Diner', lat: 52.52, lng: 13.405 });
|
||||
|
||||
expect(strategies).toEqual([{ by: 'name', name: 'ground floor diner' }]);
|
||||
expect(strategies.some((s) => s.by === 'coords')).toBe(false);
|
||||
});
|
||||
|
||||
it('falls back to coordinates only when there is no name', () => {
|
||||
expect(placeMatchStrategies({ name: null, lat: 48.85, lng: 2.35 })).toEqual([
|
||||
{ by: 'coords', lat: 48.85, lng: 2.35, tolerance: COORD_DEDUP_TOLERANCE },
|
||||
]);
|
||||
});
|
||||
|
||||
it('offers provider ids then coordinates for an unnamed candidate that has both', () => {
|
||||
expect(placeMatchStrategies({ name: ' ', google_ftid: 'f', lat: 1, lng: 2 })).toEqual([
|
||||
{ by: 'externalId', id: 'f' },
|
||||
{ by: 'coords', lat: 1, lng: 2, tolerance: COORD_DEDUP_TOLERANCE },
|
||||
]);
|
||||
});
|
||||
|
||||
it('offers nothing for a candidate with no name, no ids and no coordinates', () => {
|
||||
expect(placeMatchStrategies({ name: null, lat: null, lng: null })).toEqual([]);
|
||||
});
|
||||
|
||||
it('needs both halves of a coordinate pair', () => {
|
||||
expect(placeMatchStrategies({ name: null, lat: 48.85, lng: null })).toEqual([]);
|
||||
expect(placeMatchStrategies({ name: null, lat: null, lng: 2.35 })).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,84 @@
|
||||
/**
|
||||
* When two places are the same place — the single source of the rule.
|
||||
*
|
||||
* There are two consumers with nothing in common but this decision: the bulk
|
||||
* place importers ask it of an in-memory set they already built for the trip
|
||||
* (`isPlaceDuplicate`), and the booking importer asks it of the database and
|
||||
* needs the matched row's id so it can link to it (`findMatchingPlaceId`). Both
|
||||
* used to spell the rule out themselves, in the same order, and the SQL copy had
|
||||
* quietly drifted: it reached for coordinates on a NAMED candidate, which the
|
||||
* in-memory copy deliberately never does.
|
||||
*
|
||||
* So the order lives here as data, and each consumer is a mechanical interpreter
|
||||
* of it. Adding a way to recognise a place means adding a strategy here once.
|
||||
*
|
||||
* The order, and why:
|
||||
*
|
||||
* 1. Provider id — the only part of a place that survives the user editing it.
|
||||
* Re-importing a Google Maps list used to duplicate every place someone had
|
||||
* renamed (#1550). One strategy per id rather than one for all of them, so
|
||||
* an earlier provider still wins when two ids point at different rows.
|
||||
* 2. Name — exact after trim + lowercase.
|
||||
* 3. Coordinates — **only when there is no name.** Widening the coordinate
|
||||
* check to named places would merge the restaurant and the bar at the same
|
||||
* address, which is a worse failure than the duplicate it would prevent.
|
||||
*/
|
||||
|
||||
/** ≈ 11 m. Two coordinate-only places inside this box are treated as one place. */
|
||||
export const COORD_DEDUP_TOLERANCE = 0.0001;
|
||||
|
||||
/** The fields a place can be recognised by. Both a parsed candidate and a stored row fit. */
|
||||
export interface PlaceMatchCandidate {
|
||||
name?: string | null;
|
||||
lat?: number | null;
|
||||
lng?: number | null;
|
||||
google_place_id?: string | null;
|
||||
google_ftid?: string | null;
|
||||
osm_id?: string | null;
|
||||
}
|
||||
|
||||
/** One way to look for an existing place, to be tried in order. */
|
||||
export type PlaceMatchStrategy =
|
||||
| { by: 'externalId'; id: string }
|
||||
| { by: 'name'; name: string }
|
||||
| { by: 'coords'; lat: number; lng: number; tolerance: number };
|
||||
|
||||
/** Trim + lowercase, or null when there is no usable name. */
|
||||
export function normalizePlaceName(name: string | null | undefined): string | null {
|
||||
const trimmed = typeof name === 'string' ? name.trim() : '';
|
||||
return trimmed === '' ? null : trimmed.toLowerCase();
|
||||
}
|
||||
|
||||
/** The provider ids a candidate carries, trimmed, in provider order, blanks dropped. */
|
||||
export function externalIdsOf(candidate: PlaceMatchCandidate): string[] {
|
||||
return [candidate.google_place_id, candidate.google_ftid, candidate.osm_id]
|
||||
.filter((id): id is string => typeof id === 'string' && id.trim() !== '')
|
||||
.map((id) => id.trim());
|
||||
}
|
||||
|
||||
/**
|
||||
* How to look for `candidate`, most reliable first. An empty list means the
|
||||
* candidate carries nothing to recognise it by, so it cannot match anything.
|
||||
*/
|
||||
export function placeMatchStrategies(candidate: PlaceMatchCandidate): PlaceMatchStrategy[] {
|
||||
const strategies: PlaceMatchStrategy[] = externalIdsOf(candidate).map((id) => ({
|
||||
by: 'externalId' as const,
|
||||
id,
|
||||
}));
|
||||
|
||||
const name = normalizePlaceName(candidate.name);
|
||||
if (name) {
|
||||
strategies.push({ by: 'name', name });
|
||||
return strategies;
|
||||
}
|
||||
|
||||
if (candidate.lat != null && candidate.lng != null) {
|
||||
strategies.push({
|
||||
by: 'coords',
|
||||
lat: candidate.lat,
|
||||
lng: candidate.lng,
|
||||
tolerance: COORD_DEDUP_TOLERANCE,
|
||||
});
|
||||
}
|
||||
return strategies;
|
||||
}
|
||||
Reference in New Issue
Block a user