diff --git a/server/src/db/repositories/PlaceRegions.repository.ts b/server/src/db/repositories/PlaceRegions.repository.ts index aad5bc9ba..b6545911a 100644 --- a/server/src/db/repositories/PlaceRegions.repository.ts +++ b/server/src/db/repositories/PlaceRegions.repository.ts @@ -10,6 +10,15 @@ export interface PlaceRegionRow { region_name: string; } +/** A place's location next to the country its cached region holds, null while it has no cached region. */ +interface PlaceCountryRow { + id: number; + lat: number | null; + lng: number | null; + address: string | null; + country_code: string | null; +} + type PlaceRegionsKyselyDB = Pick; /** @@ -18,7 +27,7 @@ type PlaceRegionsKyselyDB = Pick { } /** - * AT40 (`lastTrip`) — `SELECT pr.country_code, COUNT(DISTINCT p.id) AS - * places FROM place_regions pr JOIN places p ON p.id = pr.place_id WHERE - * p.trip_id = ? AND pr.country_code IS NOT NULL GROUP BY pr.country_code - * ORDER BY places DESC, pr.country_code ASC`. + * AT40 (`lastTrip`/`nextTrip`) — `SELECT p.id, p.lat, p.lng, p.address, + * pr.country_code FROM places p LEFT JOIN place_regions pr ON pr.place_id = + * p.id WHERE p.trip_id = ? ORDER BY p.id`. A place without a cached region + * comes back with a null country, for the caller to resolve from its + * location (#2875). */ - async countPlacesByCountryForTrip(tripId: number): Promise<{ country_code: string; places: number }[]> { - const rows = await this.db() - .selectFrom('place_regions as pr') - .innerJoin('places as p', 'p.id', 'pr.place_id') - .select((eb) => ['pr.country_code', eb.fn.count('p.id').distinct().as('places')]) + async listPlaceCountriesForTrip(tripId: number): Promise { + return await this.db() + .selectFrom('places as p') + .leftJoin('place_regions as pr', 'pr.place_id', 'p.id') + .select(['p.id', 'p.lat', 'p.lng', 'p.address', 'pr.country_code']) .where('p.trip_id', '=', tripId) - .where('pr.country_code', 'is not', null) - .groupBy('pr.country_code') - .orderBy('places', 'desc') - .orderBy('pr.country_code', 'asc') + .orderBy('p.id') .execute(); - return rows.map((r) => ({ country_code: r.country_code, places: Number(r.places) })); } /** - * AT44 (`getTravelStats`) — `SELECT DISTINCT pr.country_code FROM - * place_regions pr JOIN places p ON p.id = pr.place_id JOIN trips t ON - * p.trip_id = t.id LEFT JOIN trip_members tm ON t.id = tm.trip_id WHERE - * (t.user_id = ? OR tm.user_id = ?) AND pr.country_code IS NOT NULL AND + * AT44 (`getTravelStats`) — `SELECT DISTINCT p.id, p.lat, p.lng, p.address, + * pr.country_code FROM places p JOIN trips t ON p.trip_id = t.id LEFT JOIN + * trip_members tm ON t.id = tm.trip_id LEFT JOIN place_regions pr ON + * pr.place_id = p.id WHERE (t.user_id = ? OR tm.user_id = ?) AND * COALESCE(t.start_date, t.end_date) IS NOT NULL AND COALESCE(t.start_date, - * t.end_date) <= date('now')` — "only started trips count" (#1048). - * `today` (`date('now')`, UTC) is resolved by the caller once - * (`todayUtc()`, the same value `stats()`/`lastTrip` already compute) and - * bound here, the `TripsRepository.activeTrip(user_id, today)` precedent - * for a literal `date('now')` comparison. + * t.end_date) <= ? ORDER BY p.id` — "only started trips count" (#1048). + * Every place comes back, a place without a cached region with a null + * country (#2875). `today` (UTC) is resolved by the caller once + * (`todayUtc()`) and bound here. */ - async listVisitedCountryCodesForUser(userId: number, today: string): Promise { - const rows = await this.db() - .selectFrom('place_regions as pr') - .innerJoin('places as p', 'p.id', 'pr.place_id') + async listStartedTripPlaceCountriesForUser(userId: number, today: string): Promise { + return await this.db() + .selectFrom('places as p') .innerJoin('trips as t', 't.id', 'p.trip_id') .leftJoin('trip_members as tm', 'tm.trip_id', 't.id') - .select('pr.country_code') + .leftJoin('place_regions as pr', 'pr.place_id', 'p.id') + .select(['p.id', 'p.lat', 'p.lng', 'p.address', 'pr.country_code']) .distinct() .where((eb) => eb.or([eb('t.user_id', '=', userId), eb('tm.user_id', '=', userId)])) - .where('pr.country_code', 'is not', null) .where((eb) => eb(eb.fn.coalesce('t.start_date', 't.end_date'), 'is not', null)) .where((eb) => eb(eb.fn.coalesce('t.start_date', 't.end_date'), '<=', today)) + .orderBy('p.id') .execute(); - return rows.map((r) => r.country_code); } } diff --git a/server/src/nest/atlas/atlas.service.ts b/server/src/nest/atlas/atlas.service.ts index b9e95a3e4..47ee26905 100644 --- a/server/src/nest/atlas/atlas.service.ts +++ b/server/src/nest/atlas/atlas.service.ts @@ -39,6 +39,7 @@ import { KNOWN_COUNTRIES } from './known-countries'; import { cityFromAddress } from './city-from-address'; import { transferEndpointIds } from './transfer-endpoints'; import { countryVisitDates } from './visit-dates'; +import { placeCountryResolver } from './place-countries'; import { haversineKm } from '../common/geo'; /** The part of a place that its cached region is derived from. */ @@ -213,21 +214,12 @@ export class AtlasService { const cached = placeIds.length > 0 ? await this.placeRegions.listCountryCodesForPlaceIds(placeIds) : []; // AT3 const cachedMap = new Map(cached.map((r) => [r.place_id, r.country_code])); + const countryOf = placeCountryResolver(); const uncachedForGeocode: PlaceRow[] = []; for (const p of places) { - const fromDb = cachedMap.get(p.id); - if (fromDb) { - out.set(p.id, fromDb); - continue; - } - const sync = resolveCountryCodeSync(p); - if (sync) { - out.set(p.id, sync); - continue; - } - if (p.lat && p.lng && !geocodingInFlight.has(p.id)) { - uncachedForGeocode.push(p); - } + const code = countryOf(p, cachedMap.get(p.id)); + if (code) out.set(p.id, code); + else if (p.lat && p.lng && !geocodingInFlight.has(p.id)) uncachedForGeocode.push(p); } this.cacheRegionsInBackground(uncachedForGeocode); // AT4 @@ -1005,11 +997,9 @@ export class AtlasService { * side of a work trip), and without the second key which one is "last" would be * whatever the storage engine happened to return first. * - * Countries come from `place_regions`, Atlas's own cache, most-visited first. - * No fallback to `getCountryFromCoords` here on purpose: this answers a - * dashboard widget, and a point-in-polygon scan over 4MB of boundaries is too - * much to spend on a label. An unresolved trip reports an empty list, which the - * caller renders as "no country" rather than as a wrong one. + * Countries are resolved per place the way the Atlas map resolves them, most + * visited first. A trip none of whose places can be placed reports an empty + * list, which the caller renders as "no country" rather than as a wrong one. */ async lastTrip(userId: number): Promise<{ title: string; start_date: string | null; end_date: string | null; countries: string[] } | null> { const trip = await this.trips.lastStartedTrip(userId, todayUtc()); // AT39 @@ -1047,10 +1037,17 @@ export class AtlasService { }; } - /** The countries a trip's places resolved to in `place_regions`, most-visited first. */ + /** The countries a trip's places sit in, most-visited first, ties by code. */ private async tripCountries(tripId: number): Promise { - const rows = await this.placeRegions.countPlacesByCountryForTrip(tripId); // AT40 - return rows.map((r) => r.country_code.toUpperCase()); + const countryOf = placeCountryResolver(); + const placesPerCountry = new Map(); + for (const place of await this.placeRegions.listPlaceCountriesForTrip(tripId)) { // AT40 + const code = countryOf(place, place.country_code)?.toUpperCase(); + if (code) placesPerCountry.set(code, (placesPerCountry.get(code) ?? 0) + 1); + } + return [...placesPerCountry] + .sort(([codeA, a], [codeB, b]) => b - a || (codeA < codeB ? -1 : 1)) + .map(([code]) => code); } async getTravelStats(userId: number) { @@ -1076,17 +1073,20 @@ export class AtlasService { if (cityPart) cities.add(cityPart); }); - // Visited countries — same source the Atlas page uses: ISO-2 codes from - // auto-resolved place regions plus countries the user marked manually. + // Visited countries: the ones the user marked by hand, plus the countries of + // their places, resolved the way the Atlas map resolves them (#2875). const countryCodes = new Set(); const manualCountries = await this.visitedCountries.listCodesForUser(userId); // AT43 manualCountries.forEach(code => { if (code) countryCodes.add(code.toUpperCase()); }); // Only trips that have already started count as visited — a country you have merely // booked a trip to isn't stamped in the passport yet, and one you jotted down without - // any dates even less so (#1048). date('now') is UTC, matching tripVisitStatus. - const placeRegionCodes = await this.placeRegions.listVisitedCountryCodesForUser(userId, today); // AT44 - placeRegionCodes.forEach(code => { if (code) countryCodes.add(code.toUpperCase()); }); + // any dates even less so (#1048). `today` is UTC, matching tripVisitStatus. + const countryOf = placeCountryResolver(); + for (const place of await this.placeRegions.listStartedTripPlaceCountriesForUser(userId, today)) { // AT44 + const code = countryOf(place, place.country_code); + if (code) countryCodes.add(code.toUpperCase()); + } // Transport bookings don't create a place row, so their geocoded endpoints never // reached place_regions — a country reached only by a flight/train (no lodging or diff --git a/server/src/nest/atlas/place-countries.ts b/server/src/nest/atlas/place-countries.ts new file mode 100644 index 000000000..d3336db9b --- /dev/null +++ b/server/src/nest/atlas/place-countries.ts @@ -0,0 +1,29 @@ +import type { PlaceRow } from '../../db/repositories/Places.repository'; +import { resolveCountryCodeSync } from './atlas-geo'; + +type LocatedPlace = Pick; + +/** + * The one way a place gets its country, for every view that counts countries + * (#2875): the country of its cached region first, the bundled borders for a + * place that has no cached region. place_regions is a region cache, and a place + * in a country the region bundle does not cover (Vatican City) or one whose + * background lookup has not finished holds no row there, so a count that read the + * cache alone lost countries the Atlas map shows. + * + * The border lookup is a point-in-polygon test, so each resolver runs it once per + * location: the places of a city trip mostly share their coordinates or address. + */ +export function placeCountryResolver(): (place: LocatedPlace, cached: string | null | undefined) => string | null { + const byLocation = new Map(); + return (place, cached) => { + if (cached) return cached; + const key = `${place.lat},${place.lng},${place.address ?? ''}`; + let code = byLocation.get(key); + if (code === undefined) { + code = resolveCountryCodeSync(place); + byLocation.set(key, code); + } + return code; + }; +} diff --git a/server/tests/unit/db/repositories/PlaceRegions.repository.test.ts b/server/tests/unit/db/repositories/PlaceRegions.repository.test.ts index 28068fe4a..4926c7aa2 100644 --- a/server/tests/unit/db/repositories/PlaceRegions.repository.test.ts +++ b/server/tests/unit/db/repositories/PlaceRegions.repository.test.ts @@ -120,104 +120,57 @@ describe('PlaceRegionsRepository.listDistinctRegionCodesForCountryAndPlaces (AT2 }); }); -describe('PlaceRegionsRepository.countPlacesByCountryForTrip (AT40)', () => { - it('PLACEREGREPO-007: matches the legacy GROUP BY statement, ordered by count DESC then country ASC, NULL country excluded', async () => { +describe('PlaceRegionsRepository.listPlaceCountriesForTrip (AT40)', () => { + it('PLACEREGREPO-007: every place of the trip with its cached country, null where none is cached, other trips left out', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const other = createTrip(testDb, user.id); - const fr1 = createPlace(testDb, trip.id); - const fr2 = createPlace(testDb, trip.id); - const de1 = createPlace(testDb, trip.id); - const noRegion = createPlace(testDb, trip.id); - await insertPlaceRegion(fr1.id, 'FR', 'FR-IDF', 'Île-de-France'); - await insertPlaceRegion(fr2.id, 'FR', 'FR-PAC', 'Provence'); - await insertPlaceRegion(de1.id, 'DE', 'DE-BY', 'Bavaria'); - // A different trip's place must not leak in. + const cached = createPlace(testDb, trip.id, { lat: 48.85, lng: 2.35 }); + const uncached = createPlace(testDb, trip.id, { lat: 41.9022, lng: 12.4539 }); + await insertPlaceRegion(cached.id, 'FR', 'FR-IDF', 'Île-de-France'); const foreign = createPlace(testDb, other.id); await insertPlaceRegion(foreign.id, 'FR', 'FR-IDF', 'Île-de-France'); - void noRegion; - // test-sql-allow: the raw statement is the legacy oracle this parity test holds the repository to. - const legacy = testDb - .prepare( - ` - SELECT pr.country_code, COUNT(DISTINCT p.id) AS places - FROM place_regions pr JOIN places p ON p.id = pr.place_id - WHERE p.trip_id = ? AND pr.country_code IS NOT NULL - GROUP BY pr.country_code ORDER BY places DESC, pr.country_code ASC`, - ) - .all(trip.id); - const typed = await repo.countPlacesByCountryForTrip(trip.id); - expect(typed).toEqual(legacy); - expect(typed).toEqual([ - { country_code: 'FR', places: 2 }, - { country_code: 'DE', places: 1 }, + expect(await repo.listPlaceCountriesForTrip(trip.id)).toEqual([ + { id: cached.id, lat: 48.85, lng: 2.35, address: null, country_code: 'FR' }, + { id: uncached.id, lat: 41.9022, lng: 12.4539, address: null, country_code: null }, ]); }); - it('PLACEREGREPO-008: empty array for a trip with no geocoded places', async () => { + it('PLACEREGREPO-008: empty array for a trip without places', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - expect(await repo.countPlacesByCountryForTrip(trip.id)).toEqual([]); + expect(await repo.listPlaceCountriesForTrip(trip.id)).toEqual([]); }); }); -describe('PlaceRegionsRepository.listVisitedCountryCodesForUser (AT44)', () => { - it('PLACEREGREPO-009: matches the legacy statement — owner and member trips count, only started trips, deduped, NULL country excluded', async () => { +describe('PlaceRegionsRepository.listStartedTripPlaceCountriesForUser (AT44)', () => { + it('PLACEREGREPO-009: owner and member see the places of started trips once each, cached or not', async () => { const { user: owner } = createUser(testDb); const { user: member } = createUser(testDb); + const { user: other } = createUser(testDb); const { user: stranger } = createUser(testDb); const started = createTrip(testDb, owner.id, { start_date: '2020-01-01', end_date: '2020-01-10' }); const notStarted = createTrip(testDb, owner.id, { start_date: '2099-01-01', end_date: '2099-01-10' }); + const dateless = createTrip(testDb, owner.id); + // Two members: the trip_members join must not repeat a place per member. addTripMember(testDb, started.id, member.id); + addTripMember(testDb, started.id, other.id); - const startedPlace = createPlace(testDb, started.id); - await insertPlaceRegion(startedPlace.id, 'FR', 'FR-IDF', 'Île-de-France'); - const startedPlace2 = createPlace(testDb, started.id); - await insertPlaceRegion(startedPlace2.id, 'FR', 'FR-PAC', 'Provence'); // same country, dedup target - const notStartedPlace = createPlace(testDb, notStarted.id); - await insertPlaceRegion(notStartedPlace.id, 'DE', 'DE-BY', 'Bavaria'); // not started, excluded + const cached = createPlace(testDb, started.id, { lat: 48.85, lng: 2.35 }); + await insertPlaceRegion(cached.id, 'FR', 'FR-IDF', 'Île-de-France'); + const uncached = createPlace(testDb, started.id, { lat: 41.9022, lng: 12.4539 }); + const future = createPlace(testDb, notStarted.id); + await insertPlaceRegion(future.id, 'DE', 'DE-BY', 'Bavaria'); + createPlace(testDb, dateless.id); const today = '2026-01-01'; - // test-sql-allow: the raw statement is the legacy oracle this parity test holds the repository to. - const legacy = testDb - .prepare( - ` - SELECT DISTINCT pr.country_code - FROM place_regions pr - JOIN places p ON p.id = pr.place_id - JOIN trips t ON p.trip_id = t.id - LEFT JOIN trip_members tm ON t.id = tm.trip_id - WHERE (t.user_id = ? OR tm.user_id = ?) - AND pr.country_code IS NOT NULL - AND COALESCE(t.start_date, t.end_date) IS NOT NULL - AND COALESCE(t.start_date, t.end_date) <= ?`, - ) - .all(owner.id, owner.id, today); - - const typedOwner = await repo.listVisitedCountryCodesForUser(owner.id, today); - expect(typedOwner).toEqual(legacy.map((r: unknown) => (r as { country_code: string }).country_code)); - expect(typedOwner).toEqual(['FR']); - - // test-sql-allow: the raw statement is the legacy oracle this parity test holds the repository to. - const legacyMember = testDb - .prepare( - ` - SELECT DISTINCT pr.country_code - FROM place_regions pr - JOIN places p ON p.id = pr.place_id - JOIN trips t ON p.trip_id = t.id - LEFT JOIN trip_members tm ON t.id = tm.trip_id - WHERE (t.user_id = ? OR tm.user_id = ?) - AND pr.country_code IS NOT NULL - AND COALESCE(t.start_date, t.end_date) IS NOT NULL - AND COALESCE(t.start_date, t.end_date) <= ?`, - ) - .all(member.id, member.id, today); - const typedMember = await repo.listVisitedCountryCodesForUser(member.id, today); - expect(typedMember).toEqual(legacyMember.map((r: unknown) => (r as { country_code: string }).country_code)); - expect(typedMember).toEqual(['FR']); - - expect(await repo.listVisitedCountryCodesForUser(stranger.id, today)).toEqual([]); + const expected = [ + { id: cached.id, lat: 48.85, lng: 2.35, address: null, country_code: 'FR' }, + { id: uncached.id, lat: 41.9022, lng: 12.4539, address: null, country_code: null }, + ]; + expect(await repo.listStartedTripPlaceCountriesForUser(owner.id, today)).toEqual(expected); + expect(await repo.listStartedTripPlaceCountriesForUser(member.id, today)).toEqual(expected); + expect(await repo.listStartedTripPlaceCountriesForUser(stranger.id, today)).toEqual([]); }); }); diff --git a/server/tests/unit/nest/travel-stats.service.test.ts b/server/tests/unit/nest/travel-stats.service.test.ts index a2cf563ce..3e6fa7db2 100644 --- a/server/tests/unit/nest/travel-stats.service.test.ts +++ b/server/tests/unit/nest/travel-stats.service.test.ts @@ -272,6 +272,116 @@ describe('getTravelStats', () => { }); }); +/** + * The passport card counted a place's country only once its region sat in + * place_regions, while the Atlas map falls back to the borders. A place in + * Vatican City never gets a region row (the region bundle has none for it, and + * Nominatim names no region there), so the map showed VA and the card did not. + */ +describe('place countries without a cached region (#2875)', () => { + const PAST_START = '2023-05-01'; + const PAST_END = '2023-05-10'; + const VATICAN = { lat: 41.9022, lng: 12.4539 }; + + async function addPlace( + tripId: number, + name: string, + values: { lat?: number | null; lng?: number | null; address?: string | null }, + ) { + return insertRow(await sharedTestOrm(testDb), Places, { trip: tripId, name }, { lat: null, lng: null, ...values }); + } + + async function cacheRegion(placeId: number, countryCode: string, regionCode: string) { + await upsertRow(await sharedTestOrm(testDb), PlaceRegions, { + place: placeId, + country_code: countryCode, + region_code: regionCode, + region_name: regionCode, + }); + } + + it('TRAVEL-2875-001: a place in Vatican City stamps VA on the passport card', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Rome', start_date: PAST_START, end_date: PAST_END }); + await addPlace(trip.id, "St. Peter's Basilica", VATICAN); + + expect((await atlas.getTravelStats(user.id)).countries).toContain('VA'); + }); + + it('TRAVEL-2875-002: any place still waiting for its region lookup counts by its borders', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Paris', start_date: PAST_START, end_date: PAST_END }); + await addPlace(trip.id, 'Louvre', { lat: 48.8606, lng: 2.3376 }); + + expect((await atlas.getTravelStats(user.id)).countries).toEqual(['FR']); + }); + + it('TRAVEL-2875-003: the passport card and the Atlas map list the same visited countries', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Rome', start_date: PAST_START, end_date: PAST_END }); + await addPlace(trip.id, "St. Peter's Basilica", VATICAN); + const colosseumId = await addPlace(trip.id, 'Colosseum', { lat: 41.8902, lng: 12.4922 }); + await cacheRegion(colosseumId, 'IT', 'IT-62'); + + const atlasVisited = (await atlas.stats(user.id)).countries + .filter((c) => c.status === 'visited') + .map((c) => c.code) + .sort(); + expect(atlasVisited).toEqual(['IT', 'VA']); + expect([...(await atlas.getTravelStats(user.id)).countries].sort()).toEqual(atlasVisited); + }); + + it('TRAVEL-2875-004: a Vatican place on a trip that has not started stays off the card (#1048)', async () => { + const { user } = createUser(testDb); + const start = new Date(Date.now() + 30 * 86_400_000).toISOString().slice(0, 10); + const trip = createTrip(testDb, user.id, { title: 'Rome, next month', start_date: start, end_date: start }); + await addPlace(trip.id, "St. Peter's Basilica", VATICAN); + + expect((await atlas.getTravelStats(user.id)).countries).not.toContain('VA'); + }); + + it('TRAVEL-2875-005: a place without coordinates counts by a country its address spells out, never by a bare code (#2111)', async () => { + const { user } = createUser(testDb); + const named = createTrip(testDb, user.id, { title: 'Named', start_date: PAST_START, end_date: PAST_END }); + await addPlace(named.id, 'Vatican Museums', { address: 'Viale Vaticano, Vatican City' }); + + expect((await atlas.getTravelStats(user.id)).countries).toEqual(['VA']); + + const { user: other } = createUser(testDb); + const bare = createTrip(testDb, other.id, { title: 'Bare', start_date: PAST_START, end_date: PAST_END }); + await addPlace(bare.id, 'Somewhere in Virginia', { address: 'Richmond, VA' }); + + expect((await atlas.getTravelStats(other.id)).countries).toEqual([]); + }); + + it('TRAVEL-2875-006: the last trip names Vatican City when that is all it went to', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { + title: 'Day in the Vatican', + start_date: PAST_START, + end_date: PAST_END, + }); + await addPlace(trip.id, "St. Peter's Basilica", VATICAN); + await addPlace(trip.id, 'Sistine Chapel', VATICAN); + + expect((await atlas.lastTrip(user.id))?.countries).toEqual(['VA']); + }); + + it('TRAVEL-2875-007: the last trip ranks cached and resolved places together, most places first', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Rome', start_date: PAST_START, end_date: PAST_END }); + const colosseumId = await addPlace(trip.id, 'Colosseum', { lat: 41.8902, lng: 12.4922 }); + await cacheRegion(colosseumId, 'it', 'IT-62'); + await addPlace(trip.id, 'Pantheon', { lat: 41.8986, lng: 12.4769 }); + await addPlace(trip.id, "St. Peter's Basilica", VATICAN); + await addPlace(trip.id, 'Sistine Chapel', VATICAN); + await addPlace(trip.id, 'Vatican Museums', VATICAN); + + // The cached lower-case "it" and the resolved IT are one country with two places. + expect((await atlas.lastTrip(user.id))?.countries).toEqual(['VA', 'IT']); + }); +}); + describe('travel-stats quirk fixes', () => { it('AUTH-DB-089: getTravelStats keeps a place at lat 0 / lng 0 (equator, prime meridian)', async () => { const { user } = createUser(testDb);