From 724b52aa92a7e03ce228ac3138d1aa0b251c937e Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 29 Jul 2026 14:37:31 +0200 Subject: [PATCH] fix(plugins): broadcast itinerary.unassign to open sessions (#1705) assign broadcast, unassign did not, so the place stayed on the Plan tab until a manual refresh. It emits the same event as the REST route now. createPlace, updatePlace and deletePlace in the same file had the identical gap. --- .../plugins/host/plugin-host-deps.factory.ts | 37 +++++++++- .../plugins/plugin-host-deps.factory.test.ts | 73 +++++++++++++++++++ 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/server/src/nest/plugins/host/plugin-host-deps.factory.ts b/server/src/nest/plugins/host/plugin-host-deps.factory.ts index 78adcf4e4..ffedcacfc 100644 --- a/server/src/nest/plugins/host/plugin-host-deps.factory.ts +++ b/server/src/nest/plugins/host/plugin-host-deps.factory.ts @@ -15,11 +15,11 @@ import pathMod from 'node:path'; import { randomUUID } from 'node:crypto'; import { PermissionsService } from '../../permissions/permissions.service'; import { TripsService, NotFoundError, ValidationError } from '../../trips/trips.service'; -import { createPlace, updatePlace, deletePlace } from '../../../services/placeService'; +import { createPlace, updatePlace, deletePlace, getPlace } from '../../../services/placeService'; import { AddonsService } from '../../addons/addons.service'; import { isDemoEmail } from '../../../services/demo'; import { ADDON_IDS } from '../../../addons'; -import { listJourneys, listEntries as listJournalEntriesSvc, createEntry as createJournalEntrySvc, updateEntry as updateJournalEntrySvc, deleteEntry as deleteJournalEntrySvc, createJourney as createJourneySvc, deleteJourney as deleteJourneySvc } from '../../../services/journeyService'; +import { listJourneys, listEntries as listJournalEntriesSvc, createEntry as createJournalEntrySvc, updateEntry as updateJournalEntrySvc, deleteEntry as deleteJournalEntrySvc, createJourney as createJourneySvc, deleteJourney as deleteJourneySvc, onPlaceCreated, onPlaceUpdated, onPlaceDeleted } from '../../../services/journeyService'; import { listVisitedCountries, listManuallyVisitedRegions, listBucketList, markCountryVisited, unmarkCountryVisited, markRegionVisited, unmarkRegionVisited, createBucketItem as createBucketItemSvc, deleteBucketItem as deleteBucketItemSvc } from '../../../services/atlasService'; import { listCollections, getCollection, createCollection, updateCollection, savePlace as saveCollectionPlaceSvc, copyToTrip as copyCollectionToTripSvc, deletePlace as deleteCollectionPlaceSvc } from '../../../services/collectionsService'; import { BudgetService } from '../../budget/budget.service'; @@ -56,6 +56,13 @@ function mapCollectionError(fn: () => T): T { } } +// Journey mirroring is best-effort on every other path that triggers it +// (places.service wraps the same hooks, assignments.service wraps the reconcile), +// so a broken journey link can never turn a successful write into an RPC error. +function mirrorJourneys(run: () => void): void { + try { run(); } catch { /* non-fatal */ } +} + // --- #858 packing privacy: viewer-scoped fan-out mirrored from // packing.controller.broadcastUpdate/emitToViewers and PackingService's // broadcastItem/viewersOf/broadcastToViewers. A private item's events reach ONLY @@ -461,20 +468,34 @@ export class PluginHostDepsFactory { return { deleted: true }; }, // --- Places (place_edit). Delegate to the same placeService the REST/MCP paths - // use, then broadcast the same events so open web sessions update live. --- + // use, then broadcast the same events so open web sessions update live, and run + // the journey hooks places.controller runs on the same three writes: a journey + // linked to the trip carries a skeleton entry per day-assigned place, and + // without the hooks it keeps the old title/location — or a dead entry — until + // somebody reloads it. --- canEditPlaces: (tripId, userId) => this.canEditTripAs('place_edit', tripId, userId), createPlace: (tripId, input) => { const place = createPlace(String(tripId), input as Parameters[1]); this.realtime.broadcast(tripId, 'place:created', { place }); + mirrorJourneys(() => onPlaceCreated(Number(tripId), place.id)); return place; }, updatePlace: (tripId, placeId, input) => { const place = updatePlace(String(tripId), String(placeId), input as Parameters[2]); if (place === null) throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`); this.realtime.broadcast(tripId, 'place:updated', { place }); + mirrorJourneys(() => onPlaceUpdated(Number(placeId))); return place; }, deletePlace: (tripId, placeId) => { + // Scope the id to the trip before anything else: onPlaceDeleted keys on the + // place alone, so an id belonging to a foreign trip would detach that trip's + // journey entries even though the delete below refuses it. + if (!getPlace(String(tripId), String(placeId))) throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`); + // Ahead of the DELETE, like the REST route and the MCP tool: journey_entries + // .source_place_id is ON DELETE SET NULL, so afterwards the hook finds nothing + // left to detach and the entries linger as orphans. + mirrorJourneys(() => onPlaceDeleted(Number(placeId))); const deleted = deletePlace(String(tripId), String(placeId)); if (!deleted) throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`); this.realtime.broadcast(tripId, 'place:deleted', { placeId }); @@ -510,6 +531,7 @@ export class PluginHostDepsFactory { if (!this.assignments.placeExists(placeId, tripId)) throw new ForbiddenResource(`no place ${placeId} on trip ${tripId}`); const assignment = this.assignments.createAssignment(dayId, placeId, notes); this.realtime.broadcast(tripId, 'assignment:created', { assignment }); + this.assignments.reconcile(tripId); return assignment; }, unassignPlace: (tripId, assignmentId) => { @@ -517,8 +539,15 @@ export class PluginHostDepsFactory { if (!existing) throw new ForbiddenResource(`no assignment ${assignmentId} on trip ${tripId}`); this.assignments.deleteAssignment(assignmentId); // Same payload shape as the REST/MCP delete, so client stores can - // evict the assignment from the right day. + // evict the assignment from the right day. The dayId is what the client + // reducer keys the eviction on — without it nothing leaves the day. this.realtime.broadcast(tripId, 'assignment:deleted', { assignmentId, dayId: existing.day_id }); + // Create, delete, move and time re-mirror the linked journey skeletons + // afterwards, in the controller and in the MCP tool alike (reorder, + // transport and participants don't). Without it an open journey keeps + // the removed place until the next reload. No sid — a plugin has no + // originating socket. + this.assignments.reconcile(tripId); return { deleted: true }; }, // --- Trip creation (trip_create; owner = acting user). No broadcast — a new trip diff --git a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts index 2059c1d49..20f32e97a 100644 --- a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts +++ b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts @@ -79,6 +79,9 @@ vi.mock('../../../src/services/placeService', () => ({ createPlace: vi.fn((tid: string, body: Record) => ({ id: 10, trip_id: Number(tid), ...body })), updatePlace: vi.fn((_tid: string, pid: string) => (pid === '99' ? null : { id: Number(pid) })), deletePlace: vi.fn((_tid: string, pid: string) => pid !== '99'), + // Trip-scoped read the delete path uses to reject a foreign id before the + // journey hook runs — place 99 belongs to another trip. + getPlace: vi.fn((_tid: string, pid: string) => (pid === '99' ? null : { id: Number(pid) })), // trips.service (loaded for real since the trip fold) imports listPlaces for // its offline bundle — the mock must carry every named export it pulls. listPlaces: vi.fn(() => []), @@ -111,6 +114,7 @@ const assignmentsStub = { dayExists: vi.fn((dayId: number) => dayId === 3), placeExists: vi.fn((placeId: number) => placeId === 7), getAssignmentForTrip: vi.fn((id: number) => (id === 99 ? undefined : { id, day_id: 3 })), + reconcile: vi.fn(), } as unknown as AssignmentsService; // Packing is a constructor-injected stub (same behaviors as the old path mock). const packingStub = { @@ -235,6 +239,10 @@ vi.mock('../../../src/services/journeyService', () => ({ // Consumed by the real assignments.service.ts module (loaded un-mocked as a // factory constructor dep); the instance is stubbed, so this is never called. reconcileTripSkeletons: vi.fn(), + // The skeleton hooks the place writes fire (#1705). + onPlaceCreated: vi.fn(), + onPlaceUpdated: vi.fn(), + onPlaceDeleted: vi.fn(), listJourneys: vi.fn((uid: number) => [{ id: 1, owner: uid }]), // journeyId 88 = no access (listEntries self-gates to null); else returns entries listEntries: vi.fn((journeyId: number, uid: number) => (journeyId === 88 ? null : [{ id: 10, journey_id: journeyId, author_id: uid }])), @@ -288,6 +296,8 @@ const vacayStub = { import { PluginHostDepsFactory, type PluginCallRouter } from '../../../src/nest/plugins/host/plugin-host-deps.factory'; import { getPluginDataDb, closePluginDataDb } from '../../../src/nest/plugins/host/plugin-host-state'; import { db as mockDb } from '../../../src/db/database'; +import { onPlaceCreated, onPlaceUpdated, onPlaceDeleted } from '../../../src/services/journeyService'; +import { deletePlace as deletePlaceSvc } from '../../../src/services/placeService'; import type { BudgetService } from '../../../src/nest/budget/budget.service'; import type { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; import type { ReservationsService } from '../../../src/nest/reservations/reservations.service'; @@ -425,6 +435,47 @@ describe('host-deps factory — planner write + metadata deps', () => { expect((await call(h, 'places.delete', { tripId: 1, placeId: 99 })).error.code).toBe('RESOURCE_FORBIDDEN'); }); + // #1705: a place write from a plugin has to update a linked journey the same way + // the REST route does, otherwise the journey shows the old title (or an entry for + // a place that no longer exists) until it is reloaded. + it('places writes fire the journey hooks, delete ahead of the row, and skip a foreign id', async () => { + const h = host('db:write:places'); + const created = vi.mocked(onPlaceCreated); + const updated = vi.mocked(onPlaceUpdated); + const deleted = vi.mocked(onPlaceDeleted); + const removePlace = vi.mocked(deletePlaceSvc); + [created, updated, deleted, removePlace].forEach((m) => m.mockClear()); + + expect((await call(h, 'places.create', { tripId: 1, input: { name: 'P' } })).ok).toBe(true); + expect(created).toHaveBeenCalledWith(1, 10); + + expect((await call(h, 'places.update', { tripId: 1, placeId: 5, input: { name: 'Q' } })).ok).toBe(true); + expect(updated).toHaveBeenCalledWith(5); + + expect((await call(h, 'places.delete', { tripId: 1, placeId: 5 })).ok).toBe(true); + expect(deleted).toHaveBeenCalledWith(5); + // source_place_id is ON DELETE SET NULL — after the row is gone the hook has + // nothing left to detach, so the order is part of the fix. + expect(deleted.mock.invocationCallOrder[0]).toBeLessThan(removePlace.mock.invocationCallOrder[0]); + + // A place on another trip: refused before the hook can touch that trip's journeys. + deleted.mockClear(); + updated.mockClear(); + expect((await call(h, 'places.delete', { tripId: 1, placeId: 99 })).error.code).toBe('RESOURCE_FORBIDDEN'); + expect((await call(h, 'places.update', { tripId: 1, placeId: 99, input: {} })).error.code).toBe('RESOURCE_FORBIDDEN'); + expect(deleted).not.toHaveBeenCalled(); + expect(updated).not.toHaveBeenCalled(); + }); + + // A journey link that blows up must not turn a successful place write into an + // RPC error — every other caller of these hooks swallows them too. + it('places writes survive a throwing journey hook', async () => { + const h = host('db:write:places'); + const updated = vi.mocked(onPlaceUpdated); + updated.mockImplementationOnce(() => { throw new Error('journey db locked'); }); + expect((await call(h, 'places.update', { tripId: 1, placeId: 5, input: { name: 'Q' } })).ok).toBe(true); + }); + it('days + itinerary delegate; a day/place/assignment outside the trip is refused', async () => { const h = host('db:write:days', 'db:write:itinerary'); expect((await call(h, 'days.create', { tripId: 1, input: { notes: 'n' } })).ok).toBe(true); @@ -438,6 +489,28 @@ describe('host-deps factory — planner write + metadata deps', () => { expect((await call(h, 'itinerary.unassign', { tripId: 1, assignmentId: 99 })).error.code).toBe('RESOURCE_FORBIDDEN'); }); + // #1705: a plugin itinerary write has to reach open sessions exactly like the REST + // route. The delete payload needs the dayId the client reducer evicts by, and both + // directions run the same journey-skeleton reconcile the controller/MCP tool run. + it('itinerary writes carry the dayId the client evicts by and re-mirror linked journeys', async () => { + const h = host('db:write:itinerary'); + const reconcile = vi.mocked(assignmentsStub.reconcile); + + reconcile.mockClear(); + expect((await call(h, 'itinerary.assign', { tripId: 1, dayId: 3, placeId: 7 })).ok).toBe(true); + expect(reconcile).toHaveBeenCalledWith(1); + + reconcile.mockClear(); + expect((await call(h, 'itinerary.unassign', { tripId: 1, assignmentId: 30 })).ok).toBe(true); + expect(broadcast).toHaveBeenCalledWith(1, 'assignment:deleted', { assignmentId: 30, dayId: 3 }); + expect(reconcile).toHaveBeenCalledWith(1); + + // A refused unassign deletes nothing, so it must not touch the journeys either. + reconcile.mockClear(); + expect((await call(h, 'itinerary.unassign', { tripId: 1, assignmentId: 99 })).error.code).toBe('RESOURCE_FORBIDDEN'); + expect(reconcile).not.toHaveBeenCalled(); + }); + it('trips.update: archive/cover need their own permission; service errors map to RPC codes', async () => { const h = host('db:write:trips'); expect((await call(h, 'trips.update', { tripId: 1, input: { title: 'T' } })).ok).toBe(true);