diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 61580c363..1d85efb33 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -1833,11 +1833,10 @@ export class MapsService { const found = await trekPlacesById(placeId.slice('gers:'.length)).catch(() => null); if (!found) return { place: null }; - // What the index does not have, the free sources still do. The index - // carries no opening hours yet and never carries cuisine, wheelchair - // access or a menu link; OpenStreetMap has all of them for the same - // building. Without this the hours a user saw while adding the place - // disappeared from its card afterwards, which reads like data loss. + // What the index does not have, the free sources still do: cuisine, + // wheelchair access, a menu link. OpenStreetMap has all of them for the + // same building, and without this the details a user saw while adding the + // place disappeared from its card afterwards, which reads like data loss. // // Matched by name and coordinate, with the same two gates // resolveOsmIdentity applies everywhere: within range, and sharing a @@ -1849,16 +1848,42 @@ export class MapsService { }).catch(() => null); const record = toPlaceRecord(found); - if (!osm) return { place: record }; + // Hours the index read off the operator's own site, run through the same + // expansion OSM's go through — the client reads a list of weekday lines, + // not the raw syntax, and handing it two shapes for one field would be a + // bug on every card that shows it. + // + // Measured across seven countries, OpenStreetMap has hours for 27.5 + // percent of gastronomy; this covers part of the rest. It is the + // fallback, not the first choice: an OSM entry describes this exact + // object and gets corrected by people who walked past, where a chain's + // website often carries one set of hours for every branch. + const fromSite = + typeof found.hours?.osm === 'string' ? buildOsmDetails({ opening_hours: found.hours.osm }, '', '') : null; + if (!osm) { + return { + place: fromSite?.opening_hours + ? { + ...record, + opening_hours: fromSite.opening_hours, + open_now: fromSite.open_now, + opening_periods: fromSite.opening_periods, + } + : record, + }; + } const osmDetails = buildOsmDetails(osm.tags, '', ''); + const hoursFrom = osmDetails.opening_hours ? osmDetails : fromSite; return { place: { // Index first: its name, coordinate and contact details are the ones // the user picked. OSM only fills what is still missing. ...osmDetails, ...record, - opening_hours: osmDetails.opening_hours ?? null, + opening_hours: hoursFrom?.opening_hours ?? null, + open_now: hoursFrom?.open_now ?? null, + opening_periods: hoursFrom?.opening_periods ?? null, website: record.website ?? osmDetails.website ?? null, phone: record.phone ?? osmDetails.phone ?? null, osm_id: placeId, diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 137398dab..3c10d3460 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -49,7 +49,15 @@ export interface TrekPlace { }; brand: { name: string | null; wikidata: string | null } | null; source: string; - hours: unknown | null; + /** + * Opening hours in OSM syntax, plus where they came from. The index carries + * them for a small share of places; the rest are read from the schema.org + * data the operator publishes on their own site, in the same page fetch the + * description comes from. + */ + hours: { osm: string; source: string | null; sourceUrl: string | null } | null; + /** Quoted from the place's own site, when it publishes one for machines. */ + description?: { text: string; source: string | null; sourceUrl: string | null } | null; score?: number; } diff --git a/server/tests/unit/nest/maps.gers-details.test.ts b/server/tests/unit/nest/maps.gers-details.test.ts new file mode 100644 index 000000000..a5f4670f8 --- /dev/null +++ b/server/tests/unit/nest/maps.gers-details.test.ts @@ -0,0 +1,100 @@ +/** + * MAPS-GERS-001..007 — the details of a place that came from the index. + * + * The case that started this: L'Osteria Steinstrasse showed opening hours while + * it was being added and none on its card afterwards. The index is not the only + * source for a place it holds, and this is where the rest gets filled in. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { mockById } = vi.hoisted(() => ({ + mockById: vi.fn(async (_gers: string): Promise => null), +})); +vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({ + ...(await importOriginal()), + trekPlacesById: mockById, +})); + +vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KEY: '0'.repeat(64) })); + +import { MapsService } from '../../../src/nest/maps/maps.service'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; + +const PLACE = { + gers: 'abc-123', + name: "L'Osteria", + lat: 54.0879, + lng: 12.1408, + category: 'restaurant', + categoryPath: 'eat_and_drink>restaurant', + confidence: 1, + address: { freeform: 'Steinstrasse 9', locality: 'Rostock', postcode: '18055', region: null, country: 'DE' }, + contact: { website: 'https://losteria.net/', phone: null, email: null, socials: null }, + brand: null, + source: 'overture', + hours: null as { osm: string } | null, +}; + +function make(osmTags: Record | null) { + const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; + const svc = new MapsService(database, {} as PlacePhotoCacheService); + vi.spyOn(svc, 'resolveOsmIdentity').mockResolvedValue( + osmTags ? { tags: osmTags, osmUrl: 'https://www.openstreetmap.org/node/1', matchedName: "L'Osteria" } : null, + ); + return svc; +} + +beforeEach(() => { + mockById.mockReset(); + mockById.mockResolvedValue({ ...PLACE }); + vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +describe('MapsService.getPlaceDetails for a gers: id', () => { + it('MAPS-GERS-001: prefers the hours OpenStreetMap has for the same building', async () => { + // OSM describes this exact object and gets corrected by people who walked + // past. A chain's website often carries one set of hours for every branch. + mockById.mockResolvedValue({ ...PLACE, hours: { osm: 'Mo-Su 12:00-22:00' } }); + const out = await make({ opening_hours: 'Mo-Su 11:30-23:00' }).getPlaceDetails(1, 'gers:abc-123'); + // Expanded to weekday lines, which is the shape the card reads. + expect(out.place?.opening_hours).toEqual(expect.arrayContaining(['Monday: 11:30-23:00'])); + }); + + it('MAPS-GERS-002: falls back to the hours the operator publishes', async () => { + // OpenStreetMap has hours for 27.5 percent of gastronomy, measured across + // seven countries. This is what covers part of the rest. + mockById.mockResolvedValue({ ...PLACE, hours: { osm: 'Mo-Su 12:00-22:00' } }); + const out = await make({ cuisine: 'italian' }).getPlaceDetails(1, 'gers:abc-123'); + expect(out.place?.opening_hours).toEqual(expect.arrayContaining(['Monday: 12:00-22:00'])); + }); + + it('MAPS-GERS-003: still answers when OpenStreetMap knows nothing about it', async () => { + mockById.mockResolvedValue({ ...PLACE, hours: { osm: 'Mo-Su 12:00-22:00' } }); + const out = await make(null).getPlaceDetails(1, 'gers:abc-123'); + expect(out.place?.name).toBe("L'Osteria"); + // Same shape whether the hours came from OSM or from the operator's site. + expect(out.place?.opening_hours).toEqual(expect.arrayContaining(['Monday: 12:00-22:00'])); + }); + + it('MAPS-GERS-004: a place nobody has hours for shows none, not an empty string', async () => { + const out = await make(null).getPlaceDetails(1, 'gers:abc-123'); + expect(out.place?.opening_hours ?? null).toBeNull(); + }); + + it('MAPS-GERS-005: the index wins on the fields the user actually picked', async () => { + const out = await make({ name: 'Etwas anderes', opening_hours: 'Mo-Fr 09:00-17:00' }) + .getPlaceDetails(1, 'gers:abc-123'); + expect(out.place).toMatchObject({ name: "L'Osteria", osm_id: 'gers:abc-123', source: 'trek-places' }); + }); + + it('MAPS-GERS-006: an unknown id is null rather than a thrown request', async () => { + mockById.mockResolvedValue(null); + expect(await make(null).getPlaceDetails(1, 'gers:nope')).toEqual({ place: null }); + }); + + it('MAPS-GERS-007: a failing lookup is a null, not a 500', async () => { + mockById.mockRejectedValue(new Error('places api down')); + expect(await make(null).getPlaceDetails(1, 'gers:abc-123')).toEqual({ place: null }); + }); +}); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index e44582c11..209905b82 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -144,7 +144,27 @@ import type { SsrfResult } from '../../../src/utils/ssrfGuard'; // flowing exactly as they did for the legacy module. const svc = new MapsService(new DatabaseService(db as never), photoCacheStub); +/** + * Switch the TREK Places index off for one case. + * + * The index now answers search and autocomplete before Google is asked, which + * is the point of it. A case that is about the shape of the GOOGLE request has + * to take the index out of the way first, or it asserts against the index's URL + * and fails for the right reason. + * + * Spied on the method rather than driven through app_settings, because + * trekPlacesEnabled writes its key straight into the SQL and the db stub only + * sees bound parameters. The default and the 'false' path are covered in + * maps.area.test.ts, which does go through the setting. + */ +let indexSpy: { mockRestore: () => void } | null = null; +function indexOff(): void { + indexSpy = vi.spyOn(svc, 'trekPlacesEnabled').mockReturnValue(false); +} + afterEach(() => { + indexSpy?.mockRestore(); + indexSpy = null; vi.unstubAllGlobals(); mockDbGet.mockReset(); mockDbGet.mockReturnValue(undefined); @@ -1196,6 +1216,7 @@ describe('searchPlaces (fetch stubbed)', () => { // on its own. The body field and the query parameter are what Google's // reference specifies for each half. it('MAPS-039f: sends the session token in the autocomplete body', async () => { + indexOff(); mockDbGet.mockReturnValueOnce({ maps_api_key: 'ENCRYPTED' }).mockReturnValueOnce(null); const fetchMock = vi.fn().mockResolvedValue({ ok: true, json: async () => ({ suggestions: [] }) }); vi.stubGlobal('fetch', fetchMock); @@ -1208,6 +1229,7 @@ describe('searchPlaces (fetch stubbed)', () => { }); it('MAPS-039g: omits the field entirely when there is no session token', async () => { + indexOff(); mockDbGet.mockReturnValueOnce({ maps_api_key: 'ENCRYPTED' }).mockReturnValueOnce(null); const fetchMock = vi.fn().mockResolvedValue({ ok: true, json: async () => ({ suggestions: [] }) }); vi.stubGlobal('fetch', fetchMock); @@ -1335,6 +1357,7 @@ describe('searchPlaces (fetch stubbed)', () => { }); it('MAPS-183: drops permanently closed Google results and keeps the rest (#1341)', async () => { + indexOff(); mockDbGet.mockReturnValueOnce({ maps_api_key: 'some-key' }); const fetchMock = vi.fn().mockResolvedValue({ ok: true, @@ -1530,6 +1553,7 @@ describe('autocompletePlaces (fetch stubbed)', () => { }); it('MAPS-088: includes locationBias in Google request when provided', async () => { + indexOff(); mockDbGet.mockReturnValueOnce({ maps_api_key: 'test-key' }); const fetchMock = vi.fn().mockResolvedValue({ ok: true, @@ -1549,6 +1573,7 @@ describe('autocompletePlaces (fetch stubbed)', () => { }); it('MAPS-089: omits locationBias from Google request when not provided', async () => { + indexOff(); mockDbGet.mockReturnValueOnce({ maps_api_key: 'test-key' }); const fetchMock = vi.fn().mockResolvedValue({ ok: true,