From 6953ede0e3dc00c696aa545e3da1b1d044e03d01 Mon Sep 17 00:00:00 2001 From: Xheldon Date: Sat, 5 Sep 2026 11:30:10 +0800 Subject: [PATCH] fix(planner): link the place created for a hotel instead of minting a new one on every save resolveImportedPlace created the hotel's place through placesApi.create and read .id off the response, but that endpoint answers { place }, so the id was undefined: the accommodation was saved without a place_id and the new place was orphaned. Because the planner's places state never learned about it, the name match failed on the next edit too, and every save of the same hotel produced another copy (five in a row on one trip). Create the place through the store's addPlace instead. It unwraps the envelope, so the accommodation is linked, and it pushes the place into `places`, so a later save of the same hotel reuses it. The mobile sheet goes through the same handler and is fixed with it. The hook tests mocked placesApi.create with a bare { id } and therefore never saw the envelope; they now assert on addPlace, and a new case saves the same hotel twice and expects one place. Closes #2243 --- .../pages/tripPlanner/useTripPlanner.test.tsx | 49 +++++++++++++++---- .../src/pages/tripPlanner/useTripPlanner.ts | 12 +++-- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index 1b199e010..be200d2ed 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -13,7 +13,7 @@ import { resetAllStores, seedStore } from '../../../tests/helpers/store' import { buildUser, buildTrip, buildDay, buildPlace, buildAssignment, buildReservation } from '../../../tests/helpers/factories' import { addonsApi, accommodationsApi, authApi, tripsApi, assignmentsApi, - healthApi, airtrailApi, mapsApi, placesApi, + healthApi, airtrailApi, mapsApi, } from '../../api/client' import { accommodationRepo } from '../../repo/accommodationRepo' import { offlineDb } from '../../db/offlineDb' @@ -197,7 +197,6 @@ beforeEach(() => { vi.spyOn(airtrailApi, 'sync').mockResolvedValue({ changed: 0 }) vi.spyOn(mapsApi, 'reverse').mockResolvedValue({ name: '', address: '' } as never) vi.spyOn(mapsApi, 'search').mockResolvedValue({ places: [] } as never) - vi.spyOn(placesApi, 'create').mockResolvedValue({ id: 321 }) vi.mocked(accommodationRepo.list).mockResolvedValue({ accommodations: [] }) vi.mocked(getCached).mockReturnValue(undefined) }) @@ -1533,7 +1532,7 @@ describe('useTripPlanner — bookings and transports', () => { const payload = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number; venue?: unknown } } expect(payload.create_accommodation.place_id).toBe(4) expect(payload.create_accommodation.venue).toBeUndefined() - expect(placesApi.create).not.toHaveBeenCalled() + expect(actions.addPlace).not.toHaveBeenCalled() }) it('FE-TP-HOOK-080b: a venue name that only partially matches still links that place', async () => { @@ -1567,9 +1566,41 @@ describe('useTripPlanner — bookings and transports', () => { }) expect(mapsApi.search).toHaveBeenCalledWith('Ryokan Sakura Gion') - expect(placesApi.create).toHaveBeenCalledWith(42, { name: 'Ryokan Sakura', lat: 35.1, lng: 135.7, address: 'Gion' }) - const payload = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number } } - expect(payload.create_accommodation.place_id).toBe(321) + // Created through the store (addPlace), which unwraps the API's { place } + // envelope and pushes the place into `places` — not through placesApi + // directly, whose envelope used to be read as the place itself (#2243). + expect(actions.addPlace).toHaveBeenCalledWith(42, { name: 'Ryokan Sakura', lat: 35.1, lng: 135.7, address: 'Gion' }) + const payload = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number; venue?: unknown } } + expect(payload.create_accommodation.place_id).toBe(900) + expect(payload.create_accommodation.venue).toBeUndefined() + }) + + it('FE-TP-HOOK-081c: the place created for a new hotel is linked, so the next save of the same hotel reuses it (#2243)', async () => { + vi.mocked(mapsApi.search).mockResolvedValue({ + places: [{ lat: 35.1, lng: 135.7, address: 'Kyoto, JP' }], + } as never) + // The real addPlace puts the created place into the store; mirror that so + // the second save sees it the way the planner does after a create. + actions.addPlace.mockImplementation(async (_tripId: unknown, data: { name: string }) => { + const created = buildPlace({ id: 901, name: data.name, lat: 35.1, lng: 135.7 }) + useTripStore.setState(s => ({ places: [created, ...s.places] })) + return created + }) + seedTrip() + + const { result } = await renderPlanner() + const save = () => result.current.handleSaveReservation({ + title: 'Ryokan Sakura', type: 'hotel', + create_accommodation: { venue: { name: 'Ryokan Sakura' } }, + } as never) + await act(async () => { await save() }) + await act(async () => { await save() }) + + expect(actions.addPlace).toHaveBeenCalledTimes(1) + const first = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number } } + const second = actions.addReservation.mock.calls[1][1] as { create_accommodation: { place_id?: number } } + expect(first.create_accommodation.place_id).toBe(901) + expect(second.create_accommodation.place_id).toBe(901) }) it('FE-TP-HOOK-081b: a geocoded venue without a reviewed address adopts the hit address', async () => { @@ -1587,12 +1618,12 @@ describe('useTripPlanner — bookings and transports', () => { }) expect(mapsApi.search).toHaveBeenCalledWith('Ryokan Sakura') - expect(placesApi.create).toHaveBeenCalledWith(42, { name: 'Ryokan Sakura', lat: 35.1, lng: 135.7, address: 'Kyoto, JP' }) + expect(actions.addPlace).toHaveBeenCalledWith(42, { name: 'Ryokan Sakura', lat: 35.1, lng: 135.7, address: 'Kyoto, JP' }) }) it('FE-TP-HOOK-082: a failed geocode still creates the place, a failed create yields no link', async () => { vi.mocked(mapsApi.search).mockRejectedValue(new Error('overpass down')) - vi.mocked(placesApi.create).mockRejectedValue(new Error('quota')) + actions.addPlace.mockRejectedValue(new Error('quota')) seedTrip() const { result } = await renderPlanner() @@ -1603,7 +1634,7 @@ describe('useTripPlanner — bookings and transports', () => { } as never) }) - expect(placesApi.create).toHaveBeenCalled() + expect(actions.addPlace).toHaveBeenCalled() const payload = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number } } expect(payload.create_accommodation.place_id).toBeUndefined() }) diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 7aaae7170..753a8d0f3 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -8,7 +8,7 @@ import { useToast } from '../../components/shared/Toast' import { Map, Ticket, PackageCheck, Wallet, FolderOpen, Users, Train } from 'lucide-react' import { resolvePluginIcon } from '../../components/shared/PluginIcon' import { useTranslation, translateApiError } from '../../i18n' -import { addonsApi, accommodationsApi, authApi, tripsApi, assignmentsApi, healthApi, airtrailApi, mapsApi, placesApi } from '../../api/client' +import { addonsApi, accommodationsApi, authApi, tripsApi, assignmentsApi, healthApi, airtrailApi, mapsApi } from '../../api/client' import { parsedItemToDraft, isTransportItem, isUnplaceableItem, type BookingReviewDraft } from '../../components/Planner/parsedItemToDraft' import type { BookingImportPreviewItem } from '@trek/shared' import { accommodationRepo } from '../../repo/accommodationRepo' @@ -933,8 +933,14 @@ export function useTripPlanner() { } } catch { /* geocode failure is non-fatal — create the place without coords */ } try { - const place = await placesApi.create(tripId, { name: name || address || 'Accommodation', lat, lng, address } as never) - return (place as { id?: number })?.id ?? null + // Through the store, not placesApi directly: the API answers { place }, + // and reading .id off that wrapper linked nothing — every save of the + // hotel then minted another orphan place, because the store never + // learned about the previous one and the name match above could not + // find it. addPlace unwraps the response and puts the place into + // `places`, so the next save reuses it. + const place = await tripActions.addPlace(tripId, { name: name || address || 'Accommodation', lat, lng, address } as never) + return place?.id ?? null } catch { return null } }