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.
2026-10-18 21:16:47 +00:00 · 2026-07-29 14:37:31 +02:00
parent aaef3e3e76
commit 724b52aa92
2 changed files with 106 additions and 4 deletions
@@ -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<T>(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<typeof createPlace>[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<typeof updatePlace>[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
@@ -79,6 +79,9 @@ vi.mock('../../../src/services/placeService', () => ({
  createPlace: vi.fn((tid: string, body: Record<string, unknown>) => ({ 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);