From 588ec8feb167e23ba7a6ecb90d8e707a10bdbf98 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 22 Oct 2026 12:14:26 +0200 Subject: [PATCH] fix(realtime): road trip vias and day boundaries send the same events everywhere --- .../src/nest/roadtrip/roadtrip.controller.ts | 59 +-- server/src/nest/roadtrip/roadtrip.mcp.ts | 53 +- server/src/nest/roadtrip/roadtrip.service.ts | 134 ++++- .../broadcast-parity/tours-roadtrip.cases.ts | 488 ++++++++++++++++-- server/tests/unit/mcp/tools-roadtrip.test.ts | 30 +- server/tests/unit/nest/day-boundaries.test.ts | 9 + .../tests/unit/nest/roadtrip-planning.test.ts | 37 +- .../unit/nest/roadtrip.controller.test.ts | 137 +++-- .../tests/unit/nest/roadtrip.service.test.ts | 130 ++++- shared/src/realtime/events.schema.ts | 7 +- 10 files changed, 863 insertions(+), 221 deletions(-) diff --git a/server/src/nest/roadtrip/roadtrip.controller.ts b/server/src/nest/roadtrip/roadtrip.controller.ts index d95ea0e37..c7fc178d3 100644 --- a/server/src/nest/roadtrip/roadtrip.controller.ts +++ b/server/src/nest/roadtrip/roadtrip.controller.ts @@ -1,6 +1,8 @@ import { ADDON_IDS } from '../../addons'; +import type { User } from '../../types'; import { AddonGuard } from '../addons/addon.guard'; import { RequireAddon } from '../addons/require-addon.decorator'; +import { CurrentUser } from '../auth-core/current-user.decorator'; import { JwtAuthGuard } from '../auth-core/jwt-auth.guard'; import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; import { @@ -37,6 +39,10 @@ import type { RoadtripDayTrack, RoadtripVia } from '@trek/shared'; * one road trip: they look at the same line on the same map, and a reshaped drive moves * every arrival time after it. Waiting for the other side's next reload is not "eventual * consistency", it is two people editing a route neither can see the other change. + * + * The events are the service's (RoadtripService.addVia and its siblings), the same for + * these routes and the road trip tools; a route hands it a writer that leaves out the + * socket that sent the request. */ @Controller('api/trips/:tripId/roadtrip') // AddonGuard FIRST: @RequireAddon is metadata and does nothing on its own, and the guard @@ -63,14 +69,14 @@ export class RoadtripController { @RequirePermission('day_edit') @Post('days/:dayId/vias') async create( + @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('dayId') dayId: string, @Body() body: RoadtripViaCreateDto, @Headers('x-socket-id') socketId?: string, ): Promise<{ via: RoadtripVia }> { await this.requireDay(dayId, tripId); - const via = await this.roadtrip.create(dayId, body); - await this.announce(tripId, dayId, socketId); + const via = await this.roadtrip.addVia(dayId, body, this.roadtrip.restWriter(tripId, user, socketId)); return { via }; } @@ -86,6 +92,7 @@ export class RoadtripController { @Post('days/:dayId/vias/batch') @HttpCode(200) async createMany( + @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('dayId') dayId: string, @Body() body: RoadtripViaBatchDto, @@ -98,20 +105,7 @@ export class RoadtripController { if (body.track && !(await this.roadtrip.trackExists(body.track.place_id, tripId))) { throw new HttpException({ error: 'Track not found' }, 404); } - const vias = await this.roadtrip.createMany(dayId, body); - await this.announce(tripId, dayId, socketId); - // A batch is how a day starts following a recorded track, so the track it now follows - // is news in its own right: the rail draws a badge for it and the map a line. - const tracks = await this.roadtrip.tracksForTrip(tripId); - this.roadtrip.broadcast( - tripId, - 'roadtripTrack:changed', - { - dayId, - track: tracks.find((t) => String(t.day_id) === String(dayId)) ?? null, - }, - socketId, - ); + const vias = await this.roadtrip.addVias(tripId, dayId, body, this.roadtrip.restWriter(tripId, user, socketId)); return { vias }; } @@ -126,14 +120,14 @@ export class RoadtripController { @Put('days/:dayId/vias') @HttpCode(200) async reanchor( + @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('dayId') dayId: string, @Body() body: RoadtripViaReanchorDto, @Headers('x-socket-id') socketId?: string, ): Promise<{ vias: RoadtripVia[] }> { await this.requireDay(dayId, tripId); - const vias = await this.roadtrip.reanchor(dayId, body); - await this.announce(tripId, dayId, socketId); + const vias = await this.roadtrip.reanchorVias(dayId, body, this.roadtrip.restWriter(tripId, user, socketId)); return { vias }; } @@ -141,6 +135,7 @@ export class RoadtripController { @Put('days/:dayId/vias/:id') @HttpCode(200) async update( + @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('dayId') dayId: string, @Param('id') id: string, @@ -148,48 +143,28 @@ export class RoadtripController { @Headers('x-socket-id') socketId?: string, ): Promise<{ via: RoadtripVia }> { await this.requireDay(dayId, tripId); - const via = await this.roadtrip.move(id, dayId, body.lat, body.lng, body.after_order_index); + const writer = this.roadtrip.restWriter(tripId, user, socketId); + const via = await this.roadtrip.moveVia(dayId, id, body, writer); if (!via) throw new HttpException({ error: 'Via not found' }, 404); - await this.announce(tripId, dayId, socketId); return { via }; } @RequirePermission('day_edit') @Delete('days/:dayId/vias/:id') async remove( + @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('dayId') dayId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ): Promise<{ success: true }> { await this.requireDay(dayId, tripId); - if (!(await this.roadtrip.remove(id, dayId))) { + if (!(await this.roadtrip.removeVia(dayId, id, this.roadtrip.restWriter(tripId, user, socketId)))) { throw new HttpException({ error: 'Via not found' }, 404); } - await this.announce(tripId, dayId, socketId); return { success: true }; } - /** - * Says what this day's drive is routed through now. - * - * Read back rather than assembled from what the write returned: a reanchor rewrites the - * whole set, a batch may clear legs before filling them, and one shape for all five - * routes means a client applies them all the same way. The originating socket is - * excluded, so the person dragging does not get their own point handed back mid-drag. - */ - private async announce(tripId: string, dayId: string, socketId: string | undefined): Promise { - this.roadtrip.broadcast( - tripId, - 'roadtripVia:changed', - { - dayId, - vias: await this.roadtrip.listForDay(dayId), - }, - socketId, - ); - } - /** * The guard proves the caller may reach the trip; this proves the day is part of it. * Without it a valid day id from someone else's trip would be editable through a trip diff --git a/server/src/nest/roadtrip/roadtrip.mcp.ts b/server/src/nest/roadtrip/roadtrip.mcp.ts index 7e06dd494..703089b88 100644 --- a/server/src/nest/roadtrip/roadtrip.mcp.ts +++ b/server/src/nest/roadtrip/roadtrip.mcp.ts @@ -41,6 +41,8 @@ const BOOKEND_VIA_NOTE = * the controller demands, so an assistant can never do through a tool what a person * cannot do through the UI. Reads go through the same day-belongs-to-trip check, so a * valid day id from another trip is not reachable by way of one the caller can see. + * The writes and their events are the service's use cases the routes call too; a tool + * call has no socket of its own, so the tab that asked hears the change like everyone else. */ @McpController() export class RoadtripMcp { @@ -99,8 +101,8 @@ export class RoadtripMcp { if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('day_edit', tripId, ctx.userId))) return permissionDenied(); if (!(await this.roadtrip.dayExists(dayId, tripId))) return noAccess(); - const via = await this.roadtrip.create(dayId, { after_order_index, lat, lng }); - await this.announce(tripId, dayId); + const writer = await this.guards.tripWriter(tripId, ctx.userId); + const via = await this.roadtrip.addVia(dayId, { after_order_index, lat, lng }, writer); return ok({ via }); } @@ -173,18 +175,8 @@ export class RoadtripMcp { // a place id from another trip, or a place that is not a track at all, become this // day's label. if (track && !(await this.roadtrip.trackExists(track.place_id, tripId))) return noAccess(); - const made = await this.roadtrip.createMany(dayId, { vias, replace_legs, track }); - await this.announce(tripId, dayId); - const tracks = await this.roadtrip.tracksForTrip(String(tripId)); - this.roadtrip.broadcast( - String(tripId), - 'roadtripTrack:changed', - { - dayId, - track: tracks.find((t) => String(t.day_id) === String(dayId)) ?? null, - }, - undefined, - ); + const writer = await this.guards.tripWriter(tripId, ctx.userId); + const made = await this.roadtrip.addVias(tripId, dayId, { vias, replace_legs, track }, writer); return ok({ vias: made }); } @@ -231,8 +223,8 @@ export class RoadtripMcp { if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('day_edit', tripId, ctx.userId))) return permissionDenied(); if (!(await this.roadtrip.dayExists(dayId, tripId))) return noAccess(); - const next = await this.roadtrip.reanchor(dayId, { vias, remove }); - await this.announce(tripId, dayId); + const writer = await this.guards.tripWriter(tripId, ctx.userId); + const next = await this.roadtrip.reanchorVias(dayId, { vias, remove }, writer); return ok({ vias: next }); } @@ -252,8 +244,8 @@ export class RoadtripMcp { if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('day_edit', tripId, ctx.userId))) return permissionDenied(); if (!(await this.roadtrip.dayExists(dayId, tripId))) return noAccess(); - if (!(await this.roadtrip.remove(viaId, dayId))) return noAccess(); - await this.announce(tripId, dayId); + const writer = await this.guards.tripWriter(tripId, ctx.userId); + if (!(await this.roadtrip.removeVia(dayId, viaId, writer))) return noAccess(); return ok({ success: true }); } @@ -270,29 +262,10 @@ export class RoadtripMcp { if (!(await this.tripsRepo.findAccessible(input.tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('day_edit', input.tripId, ctx.userId))) return permissionDenied(); if (!(await this.roadtrip.dayExists(input.dayId, input.tripId))) return noAccess(); - const via = await this.roadtrip.move(input.viaId, input.dayId, input.lat, input.lng, input.after_order_index); + const writer = await this.guards.tripWriter(input.tripId, ctx.userId); + const { lat, lng, after_order_index } = input; + const via = await this.roadtrip.moveVia(input.dayId, input.viaId, { lat, lng, after_order_index }, writer); if (!via) return noAccess(); - await this.announce(input.tripId, input.dayId); return ok({ via }); } - - /** - * Says what this day's drive is routed through now. - * - * The same announcement the REST routes make, because it is the same change: a via laid - * by an assistant moves the line on everybody's map exactly as one dragged by hand. - * No originating socket to exclude here — a tool call has no socket of its own, so the - * client that asked for it hears about it like everyone else. - */ - private async announce(tripId: number, dayId: number): Promise { - this.roadtrip.broadcast( - String(tripId), - 'roadtripVia:changed', - { - dayId, - vias: await this.roadtrip.listForDay(String(dayId)), - }, - undefined, - ); - } } diff --git a/server/src/nest/roadtrip/roadtrip.service.ts b/server/src/nest/roadtrip/roadtrip.service.ts index ca95c3b13..f1a921ba3 100644 --- a/server/src/nest/roadtrip/roadtrip.service.ts +++ b/server/src/nest/roadtrip/roadtrip.service.ts @@ -6,13 +6,31 @@ import type { DaysRepository } from '../../db/repositories/Days.repository'; import type { PlacesRepository } from '../../db/repositories/Places.repository'; import type { RoadtripDayTracksRepository } from '../../db/repositories/RoadtripDayTracks.repository'; import type { RoadtripViasRepository } from '../../db/repositories/RoadtripVias.repository'; +import type { User } from '../../types'; import { DomainError } from '../common/domain-error'; import { toRowId } from '../common/row-id'; +import { restTripWriter, type TripWriter } from '../common/trip-writer'; import { UnitOfWork } from '../database/unit-of-work'; import { RealtimeService } from '../realtime/realtime.service'; import { InjectRepository } from '@mikro-orm/nestjs'; -import { Injectable } from '@nestjs/common'; -import type { RoadtripDayTrack, RoadtripVia, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; +import { Injectable, Logger } from '@nestjs/common'; +import type { RoadtripDayTrack, RoadtripVia } from '@trek/shared'; + +/** A via as a write places it: after which stop of the day, and where. */ +type ViaInput = { after_order_index: number; lat: number; lng: number }; + +/** A chain of vias laid on one day, the legs it clears first and the track it was fitted to. */ +type ViaChainInput = { + vias: ViaInput[]; + replace_legs?: number[]; + track?: { place_id: number; stray_km?: number | null } | null; +}; + +/** The vias a re-anchoring moves to other legs, and those whose leg is gone. */ +type ViaReanchorInput = { vias: { id: number; after_order_index: number }[]; remove?: number[] }; + +/** Where a dragged via lands, and the leg it now follows when the drag crossed a stop. */ +type ViaMove = { lat: number; lng: number; after_order_index?: number }; /** * Via points: the places a day's drive is made to pass through without stopping. @@ -30,6 +48,8 @@ import type { RoadtripDayTrack, RoadtripVia, TrekWsPayload, TrekWsTripEventName */ @Injectable() export class RoadtripService { + private readonly logger = new Logger(RoadtripService.name); + constructor( private readonly realtime: RealtimeService, private readonly uow: UnitOfWork, @@ -39,24 +59,98 @@ export class RoadtripService { @InjectRepository(RoadtripDayTracks) private readonly tracksRepo: RoadtripDayTracksRepository, ) {} + /** The REST caller of a via write: its events leave out the socket that sent the request. */ + restWriter(tripId: string, user: User, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + /** - * Tells the trip's other clients what the drive looks like now. + * The via writes below, one use case each for the routes and the tools alike. The caller + * has checked that the day belongs to the trip and that the writer may edit its days; each + * writes, then tells the trip what the day's drive runs through now, after the commit. + */ + async addVia(dayId: string | number, input: ViaInput, writer: TripWriter): Promise { + const via = await this.create(dayId, input); + await this.announceDay(dayId, writer); + return via; + } + + /** + * A chain laid on one day, then the track the day follows now: a batch is how a day starts + * following a recorded track, so the rail draws a badge for it and the map a line. The + * track event goes out even when the batch named none, as it always has. + */ + async addVias( + tripId: string | number, + dayId: string | number, + input: ViaChainInput, + writer: TripWriter, + ): Promise { + const vias = await this.createMany(dayId, input); + await this.announceDay(dayId, writer); + try { + const tracks = await this.tracksForTrip(tripId); + writer.events.emit('roadtripTrack:changed', { + dayId: String(dayId), + track: tracks.find((t) => String(t.day_id) === String(dayId)) ?? null, + }); + } catch { + this.logger.warn(`Committed vias for day ${String(dayId)}: track notification failed`); + } + return vias; + } + + async reanchorVias(dayId: string | number, input: ViaReanchorInput, writer: TripWriter): Promise { + const vias = await this.reanchor(dayId, input); + await this.announceDay(dayId, writer); + return vias; + } + + /** Null, and nothing said, when the via is not on this day. */ + async moveVia( + dayId: string | number, + id: string | number, + to: ViaMove, + writer: TripWriter, + ): Promise { + const via = await this.move(id, dayId, to.lat, to.lng, to.after_order_index); + if (via) await this.announceDay(dayId, writer); + return via; + } + + /** False, and nothing said, when the via is not on this day. */ + async removeVia(dayId: string | number, id: string | number, writer: TripWriter): Promise { + const removed = await this.remove(id, dayId); + if (removed) await this.announceDay(dayId, writer); + return removed; + } + + /** + * Tells the trip what this day's drive is routed through now. * * Two people planning a road trip look at the same line on the same map, and a reshaped - * drive moves every arrival time after it — so this is not "how one person draws their + * drive moves every arrival time after it, so this is not "how one person draws their * route", which is what it was taken for when these routes were written silent. * - * The whole day's list goes out rather than the one point that changed: a via carries no - * identity anybody reads, the client holds them per day, and a drag is a burst of writes - * whose only interesting state is the one that lands last. + * The whole day's list goes out, read back rather than assembled from the write: a via + * carries no identity anybody reads, the client holds them per day, a reanchor rewrites + * the set and a batch may clear legs before filling them. REST leaves out the socket that + * made the change, so the person dragging does not get their point handed back mid-drag; + * a tool call has no socket, so the tab that asked hears it like everyone else. + * + * The day id goes out as text, the way the routes have always sent it, on every surface. + * Other senders of this event (assignments, accommodations, hike plans) still send a + * number, as their own routes always did; the client reads it through `Number()` either way. + * + * The write is committed by the time this runs, so a failed read or send is logged and + * swallowed: the caller gets the row it wrote, not a 500 for a change that landed. */ - broadcast( - tripId: string, - event: E, - payload: TrekWsPayload, - socketId: string | undefined, - ): void { - this.realtime.broadcast(tripId, event, payload, socketId); + private async announceDay(dayId: string | number, writer: TripWriter): Promise { + try { + writer.events.emit('roadtripVia:changed', { dayId: String(dayId), vias: await this.listForDay(dayId) }); + } catch { + this.logger.warn(`Committed vias for day ${String(dayId)}: realtime notification failed`); + } } /** @@ -110,6 +204,7 @@ export class RoadtripService { return await this.viasRepo.listForTrip(tripIdNum); } + /** Silent: writes and tells nobody. Callers that change a trip go through `addVia`. */ async create( dayId: string | number, input: { after_order_index: number; lat: number; lng: number; sequence?: number }, @@ -140,6 +235,8 @@ export class RoadtripService { * * `replace_legs` clears by leg rather than by day, so vias the traveller placed by hand * on other legs survive a track being laid on this one. + * + * Silent: writes and tells nobody. Callers that change a trip go through `addVias`. */ async createMany( dayId: string | number, @@ -202,7 +299,11 @@ export class RoadtripService { return await this.placesRepo.isTrackInTrip(placeId, tripIdNum); } - /** Moving a via is the whole edit; where it sits in the chain does not change. */ + /** + * Moving a via is the whole edit; where it sits in the chain does not change. + * + * Silent: writes and tells nobody. Callers that change a trip go through `moveVia`. + */ async move( id: string | number, dayId: string | number, @@ -243,6 +344,8 @@ export class RoadtripService { * Silent about ids it does not find. The caller computed this list from a snapshot of * the day, and a via somebody else deleted in the meantime is not a failure of the * re-anchoring; failing the batch over it would leave the rest of the day mis-pinned. + * + * Silent: writes and tells nobody. Callers that change a trip go through `reanchorVias`. */ async reanchor( dayId: string | number, @@ -300,6 +403,7 @@ export class RoadtripService { }); } + /** Silent: writes and tells nobody. Callers that change a trip go through `removeVia`. */ async remove(id: string | number, dayId: string | number): Promise { const idNum = toRowId(id); const dayIdNum = toRowId(dayId); diff --git a/server/tests/integration/broadcast-parity/tours-roadtrip.cases.ts b/server/tests/integration/broadcast-parity/tours-roadtrip.cases.ts index a337b8ac4..7fb53bff8 100644 --- a/server/tests/integration/broadcast-parity/tours-roadtrip.cases.ts +++ b/server/tests/integration/broadcast-parity/tours-roadtrip.cases.ts @@ -1,34 +1,464 @@ -import type { CaseGroup } from './types'; +import { CollectionTours } from '../../../src/db/entities/CollectionTours.entity'; +import { Places } from '../../../src/db/entities/Places.entity'; +import { TourChangeSteps } from '../../../src/db/entities/TourChangeSteps.entity'; +import { TourWaypoints } from '../../../src/db/entities/TourWaypoints.entity'; +import { Tours } from '../../../src/db/entities/Tours.entity'; +import { RoadtripRouterService } from '../../../src/nest/roadtrip/roadtrip-router.service'; +import { proposalIdOf } from '../../../src/nest/tours/tour-change-proposal'; +import { makePlace } from '../../helpers/factories/places'; +import { createRow, deleteRows, updateRows } from '../../helpers/factories/rows'; +import { addTourWaypoints, linkTourPacking, makeTour, makeTourActivity } from '../../helpers/factories/tours'; +import type { CaseGroup, Scene } from './types'; +import { MikroORM } from '@mikro-orm/core'; +import type { INestApplication } from '@nestjs/common'; +import type { TourChange } from '@trek/shared'; -/** Tours and road trips: city walks, hikes and their library, route vias, day boundaries and the Google route import. */ +import { vi } from 'vitest'; + +const ormOf = (app: INestApplication) => app.get(MikroORM); + +/** + * The row a case's setup made beyond its scene (a recorded track, a kept + * tour, a saved step), by the scene's number, for the arguments to point at. + */ +const madeFor = new Map(); + +function made(s: Scene): number { + const id = madeFor.get(s.n); + if (id === undefined) throw new Error(`scene ${s.n}: the case's setup made no row to point at`); + return id; +} + +/** + * No routing host in a test: the router that plans a city walk's and a + * hike's way on foot fails as a router that is down does, so each is saved + * with its points only, the way both tools save one when the router is away. + */ +function routerDown(app: INestApplication): () => void { + const route = vi.spyOn(app.get(RoadtripRouterService), 'route'); + route.mockRejectedValue(new Error('no routing host in the broadcast parity test')); + return () => route.mockRestore(); +} + +/** A hike's line with its heights, as the planner saves one. */ +const HIKE_LINE: [number, number, number][] = [ + [47, 11, 600], + [47.01, 11.01, 700], + [47.02, 11.02, 650], +]; + +/** The planner's save of a hike, for the create and edit routes. */ +const hikeBody = (name: string) => ({ + name, + route_geometry: HIKE_LINE, + waypoints: [ + { lat: 47, lng: 11, role: 'start', sequence: 0 }, + { lat: 47.02, lng: 11.02, role: 'end', sequence: 1 }, + ], + max_hiking_difficulty: 2, + duration_seconds: 3600, +}); + +/** The scene's hike with its line and its start and end, as the planner saves one. */ +async function planHike(s: Scene, app: INestApplication): Promise { + const orm = ormOf(app); + await updateRows(orm, Places, { id: s.tourPlace.id }, { route_geometry: JSON.stringify(HIKE_LINE) }); + await addTourWaypoints(orm, s.tourPlace.id, [ + { lat: 47, lng: 11 }, + { lat: 47.02, lng: 11.02 }, + ]); +} + +/** The stations of the scene's tour as a city walk: a gate, a market and a park. */ +const STATIONS = [ + { lat: 48.1, lng: 11.5, station_key: 'walk-gate', name: 'Gate' }, + { lat: 48.104, lng: 11.5, station_key: 'walk-market', name: 'Market' }, + { lat: 48.108, lng: 11.5, station_key: 'walk-park', name: 'Park' }, +]; + +/** The scene's tour made a city walk of three stations, without a line yet. */ +async function planWalk(s: Scene, app: INestApplication): Promise { + const orm = ormOf(app); + await updateRows(orm, Tours, { place: s.tourPlace.id }, { tourTypeRef: 'walk' }); + await addTourWaypoints(orm, s.tourPlace.id, STATIONS); +} + +/** The market left out of the walk. */ +const WITHOUT_MARKET: TourChange[] = [{ op: 'remove', key: 'walk-market' }]; + +/** The planner's save of the walk from the gate to the park, against the revision it was opened at. */ +const walkBody = (revision: number) => ({ + name: 'Old town', + tour_type: 'walk', + route_geometry: [ + [48.1, 11.5, 520], + [48.108, 11.5, 525], + ], + waypoints: [ + { lat: 48.1, lng: 11.5, role: 'start', sequence: 0, key: 'walk-gate', name: 'Gate' }, + { lat: 48.108, lng: 11.5, role: 'end', sequence: 1, key: 'walk-park', name: 'Park' }, + ], + expected_revision: revision, +}); + +/** + * The walk after a saved step took the market out: the step at revision 2 + * keeps the three stations as they were before it, and the walk holds two. + * Written row by row, since the save that records a step tells the trip. + */ +async function walkWithSavedStep(s: Scene, app: INestApplication): Promise { + const orm = ormOf(app); + await planWalk(s, app); + const before = STATIONS.map(({ station_key, ...station }, sequence) => ({ + ...station, + key: station_key, + role: sequence === 0 ? 'start' : sequence === STATIONS.length - 1 ? 'end' : 'via', + sequence, + })); + const step = await createRow(orm, TourChangeSteps, { + place: s.tourPlace.id, + proposal_id: `parity-step-${s.n}`, + base_revision: 1, + revision: 2, + changes: JSON.stringify(WITHOUT_MARKET), + before_state: JSON.stringify({ + route_geometry: null, + waypoints: before, + distance: null, + duration: null, + elevation_gain: null, + elevation_loss: null, + }), + }); + await deleteRows(orm, TourWaypoints, { place: s.tourPlace.id, station_key: 'walk-market' }); + await updateRows(orm, Tours, { place: s.tourPlace.id }, { revision: 2 }); + madeFor.set(s.n, step.id); +} + +/** A recorded track of the scene's trip (a GPX import, not planned over), for the hike to be done with. */ +async function recordedTrack(s: Scene, app: INestApplication): Promise { + const orm = ormOf(app); + const track = await makePlace(orm, s.trip.id, { + name: `Recorded ${s.n}`, + lat: 47, + lng: 11, + route_geometry: JSON.stringify(HIKE_LINE), + }); + await makeTour(orm, track.id, { origin: 'gpx' }); + return track.id; +} + +/** The scene's collection keeping a hike of its own, with its line and its two points. */ +async function keptTour(s: Scene, app: INestApplication): Promise { + const row = await createRow(ormOf(app), CollectionTours, { + collection: s.collection.id, + name: `Kept ridge ${s.n}`, + route_geometry: JSON.stringify(HIKE_LINE), + waypoints: JSON.stringify(hikeBody('').waypoints), + tourTypeRef: 'hike', + origin: 'trek', + source_trip_title: `Alps ${s.n}`, + source_tour_name: `Ridge ${s.n}`, + }); + madeFor.set(s.n, row.id); +} + +/** Two stops of a Google Maps directions link, reviewed. */ +const GOOGLE_STOPS = [ + { name: 'Brandenburger Tor', lat: 52.5163, lng: 13.3777 }, + { name: 'Alexanderplatz', lat: 52.5219, lng: 13.4132 }, +]; + +/** A GPX file with one track and its heights. */ +const gpxOf = (s: Scene) => + `Ridge ${s.n}800` + + '1000'; + +const roadtripDay = (s: Scene) => `/api/trips/${s.trip.id}/roadtrip/days/${s.days[0].id}`; +const tourPath = (s: Scene) => `/api/trips/${s.trip.id}/tours/${s.tourPlace.id}`; + +/** + * Tours and road trips: city walks, hikes and their library, the tours kept in + * collections, recorded tracks, the packing link, route vias, day boundaries, + * the road trip settings and the Google route import. Paddling, skiing and + * charging plan and read only; they have no write tool. + */ export const toursRoadtrip: CaseGroup = { name: 'tours-roadtrip', - prefix: 'BCP-TOURS', - cases: [], - exempt: [], - pending: [ - 'set_day_boundary', - 'import_google_maps_route', - 'update_roadtrip_settings', - 'add_route_via', - 'add_route_vias', - 'reanchor_route_vias', - 'remove_route_via', - 'update_route_via', - 'create_city_walk', - 'save_tour_to_collection', - 'take_over_collection_tour', - 'delete_collection_tour', - 'assign_tour_track', - 'unassign_tour_track', - 'set_tour_activity_excluded', - 'undo_tour_change', - 'save_tour_change', - 'take_over_tour', - 'link_tour_packing', - 'unlink_tour_packing', - 'create_hike', - 'update_tour', - 'import_tour_gpx', + prefix: 'BCP-TOUR', + cases: [ + { + tool: 'set_day_boundary', + needs: ['assignment'], + args: (s) => ({ + tripId: s.trip.id, + dayNumber: 1, + boundary: { day_number: 1, from_assignment_id: s.assignment.id, to_assignment_id: null, fraction: 1 }, + }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/roadtrip/day-boundaries`, + body: (s) => ({ day_number: 1, from_assignment_id: s.assignment.id, to_assignment_id: null, fraction: 1 }), + }, + expect: ['roadtripBoundary:changed'], + }, + { + tool: 'import_google_maps_route', + args: (s) => ({ tripId: s.trip.id, dayId: s.days[0].id, stops: GOOGLE_STOPS }), + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/roadtrip/google-maps-import`, + body: (s) => ({ dayId: s.days[0].id, stops: GOOGLE_STOPS }), + }, + // Each reviewed stop as a place and its visit, in the order of the link. + expect: ['place:created', 'assignment:created', 'place:created', 'assignment:created'], + }, + { + tool: 'update_roadtrip_settings', + args: (s) => ({ tripId: s.trip.id, settings: { roadtrip_hotel_bookends: true } }), + rest: { + method: 'put', + path: (s) => `/api/trips/${s.trip.id}/roadtrip/preferences`, + body: () => ({ roadtrip_hotel_bookends: true }), + }, + expect: ['roadtripPreferences:changed'], + }, + { + // G8: the day id goes out as text on both surfaces now, as the route always sent it. + tool: 'add_route_via', + args: (s) => ({ tripId: s.trip.id, dayId: s.days[0].id, after_order_index: 0, lat: 48.86, lng: 2.34 }), + rest: { + method: 'post', + path: (s) => `${roadtripDay(s)}/vias`, + body: () => ({ after_order_index: 0, lat: 48.86, lng: 2.34 }), + }, + expect: ['roadtripVia:changed'], + }, + { + tool: 'add_route_vias', + args: (s) => ({ + tripId: s.trip.id, + dayId: s.days[0].id, + vias: [{ after_order_index: 0, lat: 48.86, lng: 2.34 }], + }), + rest: { + method: 'post', + path: (s) => `${roadtripDay(s)}/vias/batch`, + body: () => ({ vias: [{ after_order_index: 0, lat: 48.86, lng: 2.34 }] }), + }, + // The vias first, then the track the day follows now (none here), as the batch always sent them. + expect: ['roadtripVia:changed', 'roadtripTrack:changed'], + }, + { + tool: 'reanchor_route_vias', + needs: ['via'], + args: (s) => ({ tripId: s.trip.id, dayId: s.days[0].id, vias: [{ id: s.via.id, after_order_index: 1 }] }), + rest: { + method: 'put', + path: (s) => `${roadtripDay(s)}/vias`, + body: (s) => ({ vias: [{ id: s.via.id, after_order_index: 1 }] }), + }, + expect: ['roadtripVia:changed'], + }, + { + tool: 'remove_route_via', + needs: ['via'], + args: (s) => ({ tripId: s.trip.id, dayId: s.days[0].id, viaId: s.via.id }), + rest: { method: 'delete', path: (s) => `${roadtripDay(s)}/vias/${s.via.id}` }, + expect: ['roadtripVia:changed'], + }, + { + tool: 'update_route_via', + needs: ['via'], + args: (s) => ({ tripId: s.trip.id, dayId: s.days[0].id, viaId: s.via.id, lat: 48.87, lng: 2.35 }), + rest: { + method: 'put', + path: (s) => `${roadtripDay(s)}/vias/${s.via.id}`, + body: () => ({ lat: 48.87, lng: 2.35 }), + }, + expect: ['roadtripVia:changed'], + }, + { + tool: 'create_city_walk', + args: (s) => ({ + tripId: s.trip.id, + name: 'Old town', + stations: [ + { lat: 48.1, lng: 11.5, name: 'Gate' }, + { lat: 48.108, lng: 11.5, name: 'Park' }, + ], + }), + // The planner saves a new walk through the tours route, with the line it planned in the browser. + rest: { method: 'post', path: (s) => `/api/trips/${s.trip.id}/tours`, body: () => walkBody(1) }, + stub: routerDown, + expect: ['tours:changed', 'place:created'], + }, + { + tool: 'create_hike', + args: (s) => ({ + tripId: s.trip.id, + name: 'Ridge walk', + waypoints: [ + { lat: 47, lng: 11 }, + { lat: 47.02, lng: 11.02 }, + ], + }), + rest: { method: 'post', path: (s) => `/api/trips/${s.trip.id}/tours`, body: () => hikeBody('Ridge walk') }, + stub: routerDown, + expect: ['tours:changed', 'place:created'], + }, + { + // Only the name changes, so the hike keeps its way and the router is not asked. + tool: 'update_tour', + needs: ['tour'], + setup: planHike, + args: (s) => ({ tripId: s.trip.id, placeId: s.tourPlace.id, expected_revision: 1, name: 'Renamed ridge' }), + rest: { + method: 'put', + path: tourPath, + body: () => ({ ...hikeBody('Renamed ridge'), expected_revision: 1 }), + }, + // Not needed while only the name changes, but a hike edit must never reach a real router. + stub: routerDown, + expect: ['tours:changed', 'place:updated'], + }, + { + tool: 'import_tour_gpx', + args: (s) => ({ tripId: s.trip.id, gpx: gpxOf(s), file_name: 'ridge.gpx' }), + // A multipart upload, which the parity harness does not send. + twin: 'POST /api/trips/:tripId/tours/import/gpx with the file', + expect: ['tours:changed', 'place:created'], + }, + { + tool: 'save_tour_change', + needs: ['tour'], + setup: planWalk, + args: (s) => ({ + tripId: s.trip.id, + placeId: s.tourPlace.id, + changes: WITHOUT_MARKET, + expected_revision: 1, + proposal_id: proposalIdOf({ tripId: s.trip.id, placeId: s.tourPlace.id, revision: 1, changes: WITHOUT_MARKET }), + confirmed: true, + }), + // The planner saves the changed walk through the same route write. + rest: { method: 'put', path: tourPath, body: () => walkBody(1) }, + stub: routerDown, + expect: ['tours:changed', 'place:updated'], + }, + { + tool: 'undo_tour_change', + needs: ['tour'], + setup: walkWithSavedStep, + args: (s) => ({ + tripId: s.trip.id, + placeId: s.tourPlace.id, + step_id: made(s), + expected_revision: 2, + confirmed: true, + }), + // Taking a step back writes the walk as it was through the route write the planner's save uses. + rest: { method: 'put', path: tourPath, body: () => walkBody(2) }, + expect: ['tours:changed', 'place:updated'], + }, + { + tool: 'take_over_tour', + needs: ['tour'], + setup: planHike, + args: (s) => ({ tripId: s.trip.id, source_trip_id: s.trip.id, source_place_id: s.tourPlace.id }), + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/tours/take-over`, + body: (s) => ({ source_trip_id: s.trip.id, source_place_id: s.tourPlace.id }), + }, + expect: ['tours:changed', 'place:created'], + }, + { + tool: 'assign_tour_track', + needs: ['tour'], + setup: async (s, app) => { + madeFor.set(s.n, await recordedTrack(s, app)); + }, + args: (s) => ({ tripId: s.trip.id, placeId: s.tourPlace.id, track_place_id: made(s), assignment_id: null }), + rest: { + method: 'put', + path: (s) => `${tourPath(s)}/activities`, + body: (s) => ({ track_place_id: made(s), assignment_id: null }), + }, + expect: ['tours:changed'], + }, + { + tool: 'unassign_tour_track', + needs: ['tour'], + setup: async (s, app) => { + const activity = await makeTourActivity(ormOf(app), s.tourPlace.id, await recordedTrack(s, app)); + madeFor.set(s.n, activity.id); + }, + args: (s) => ({ tripId: s.trip.id, placeId: s.tourPlace.id, activityId: made(s) }), + rest: { method: 'delete', path: (s) => `${tourPath(s)}/activities/${made(s)}` }, + expect: ['tours:changed'], + }, + { + tool: 'link_tour_packing', + needs: ['tour'], + args: (s) => ({ tripId: s.trip.id, placeId: s.tourPlace.id, category: null }), + rest: { method: 'put', path: (s) => `${tourPath(s)}/packing-link`, body: () => ({ category: null }) }, + expect: ['tours:changed'], + }, + { + tool: 'unlink_tour_packing', + needs: ['tour'], + setup: (s, app) => linkTourPacking(ormOf(app), s.tourPlace.id, null, s.owner.id), + args: (s) => ({ tripId: s.trip.id, placeId: s.tourPlace.id }), + rest: { method: 'delete', path: (s) => `${tourPath(s)}/packing-link` }, + expect: ['tours:changed'], + }, + { + // A collection is the user's, not the trip's: its people hear it on their own sockets. + tool: 'save_tour_to_collection', + needs: ['collection', 'tour'], + setup: planHike, + args: (s) => ({ collection_id: s.collection.id, source_trip_id: s.trip.id, source_place_id: s.tourPlace.id }), + rest: { + method: 'post', + path: (s) => `/api/addons/collections/${s.collection.id}/tours`, + body: (s) => ({ source_trip_id: s.trip.id, source_place_id: s.tourPlace.id }), + }, + expect: [{ event: 'collections:updated', user: 'actor' }], + }, + { + tool: 'take_over_collection_tour', + needs: ['collection'], + setup: keptTour, + args: (s) => ({ + tripId: s.trip.id, + collection_id: s.collection.id, + collection_tour_id: made(s), + include_waypoints: true, + }), + rest: { + method: 'post', + path: (s) => `/api/trips/${s.trip.id}/tours/from-collection`, + body: (s) => ({ collection_id: s.collection.id, collection_tour_id: made(s), include_waypoints: true }), + }, + expect: ['tours:changed', 'place:created'], + }, + { + tool: 'delete_collection_tour', + needs: ['collection'], + setup: keptTour, + args: (s) => ({ collection_id: s.collection.id, collection_tour_id: made(s) }), + rest: { method: 'delete', path: (s) => `/api/addons/collections/${s.collection.id}/tours/${made(s)}` }, + expect: [{ event: 'collections:updated', user: 'actor' }], + }, + ], + exempt: [ + { + tool: 'set_tour_activity_excluded', + reason: + "the user's own activity sums, read fresh when the tours statistics open; both surfaces are silent, and the registry has no event for them yet", + needs: ['tour'], + args: (s) => ({ place_id: s.tourPlace.id, excluded: true }), + }, ], }; diff --git a/server/tests/unit/mcp/tools-roadtrip.test.ts b/server/tests/unit/mcp/tools-roadtrip.test.ts index 8eb544c86..6790a4fd1 100644 --- a/server/tests/unit/mcp/tools-roadtrip.test.ts +++ b/server/tests/unit/mcp/tools-roadtrip.test.ts @@ -15,6 +15,7 @@ import { RoadtripPreferences } from '../../../src/db/entities/RoadtripPreference import { RoadtripVias } from '../../../src/db/entities/RoadtripVias.entity'; import { DEMO_EMAIL_PRIMARY } from '../../../src/nest/common/demo'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createUser, createTrip, createDay, createPlace } from '../../helpers/factories'; import { addTripMember } from '../../helpers/factories'; import { countRows, findRow, updateRows } from '../../helpers/factories/rows'; @@ -31,6 +32,8 @@ vi.mock('../../../src/db/database', async () => { }); let orm: TestOrm; +// Every tool here broadcasts through this one, so a case can read what went out. +const realtime = new FakeRealtimeService(); // The permissions cache is module-scoped, so a write through any instance is // what the tool's own check reads back. @@ -46,6 +49,7 @@ beforeAll(async () => { beforeEach(() => { resetTestDb(testDb); + realtime.reset(); // resetTestDb leaves the addons table alone, so each run restates it. The // addon ships disabled, which is why the tools were invisible before. setAddonEnabled(testDb, ADDON_IDS.ROADTRIP, true); @@ -57,7 +61,7 @@ afterAll(async () => { }); async function withHarness(userId: number, fn: (h: McpHarness) => Promise) { - const h = await createMcpHarness({ userId, withResources: false, scopes: null }); + const h = await createMcpHarness({ realtime, userId, withResources: false, scopes: null }); try { await fn(h); } finally { @@ -394,6 +398,30 @@ describe('road-trip MCP tools', () => { }); }); + it('MCP-ROADTRIP-015: adding a via tells the trip once, with the day id as text and no socket left out', async () => { + // G8: the tool used to send the day id as a number where the route sent text. It now runs + // the same use case through the MCP writer, so the payload matches the route's. + const { user, trip, day } = await scenario(); + + await withHarness(user.id, async (h) => { + const added = await h.client.callTool({ + name: 'add_route_via', + arguments: { tripId: trip.id, dayId: day.id, after_order_index: 0, lat: 53, lng: 10 }, + }); + expect(added.isError).toBeFalsy(); + const { via } = parseToolResult(added) as { via: { id: number } }; + + const sent = realtime.broadcastMock.mock.calls.filter(([, event]) => event === 'roadtripVia:changed'); + expect(sent).toHaveLength(1); + // Three arguments: room-wide, with no socket to leave out. + expect(sent[0]).toHaveLength(3); + const [tripId, , payload] = sent[0]; + expect(String(tripId)).toBe(String(trip.id)); + expect(payload).toMatchObject({ dayId: String(day.id), _source: 'mcp' }); + expect((payload.vias as { id: number }[]).map((v) => v.id)).toEqual([via.id]); + }); + }); + it('MCP-ROADTRIP-009: removing one that is not on the day is refused, not silently ignored', async () => { const { user, trip, day } = await scenario(); diff --git a/server/tests/unit/nest/day-boundaries.test.ts b/server/tests/unit/nest/day-boundaries.test.ts index f21e171a2..a7551f04f 100644 --- a/server/tests/unit/nest/day-boundaries.test.ts +++ b/server/tests/unit/nest/day-boundaries.test.ts @@ -157,9 +157,18 @@ it('MCP checks demo, trip access and edit permission before saving', async () => s.guards.hasTripPermission.mockReturnValue(false); await s.mcp.save(request, ctx); expect(await s.service.list(s.tripA.id)).toEqual([]); + // A refused call tells nobody anything. + expect(s.realtime.broadcast).not.toHaveBeenCalled(); s.guards.hasTripPermission.mockReturnValue(true); await s.mcp.save(request, ctx); expect(await s.service.list(s.tripA.id)).toEqual([boundary]); + // Clearing a boundary goes through `remove`, which the parity case never runs, so the + // event it sends is pinned here: once, with the list as it is now. + s.realtime.broadcast.mockClear(); await s.mcp.save({ ...request, boundary: null }, ctx); expect(await s.service.list(s.tripA.id)).toEqual([]); + expect(s.realtime.broadcast).toHaveBeenCalledTimes(1); + expect(s.realtime.broadcast).toHaveBeenCalledWith(String(s.tripA.id), 'roadtripBoundary:changed', { + boundaries: [], + }); }); diff --git a/server/tests/unit/nest/roadtrip-planning.test.ts b/server/tests/unit/nest/roadtrip-planning.test.ts index 7ff352f5c..b4936aa40 100644 --- a/server/tests/unit/nest/roadtrip-planning.test.ts +++ b/server/tests/unit/nest/roadtrip-planning.test.ts @@ -496,11 +496,11 @@ describe('Roadtrip MCP registration and search', () => { it('checks visit editing permission and ownership before moving a via', async () => { const service = { dayExists: vi.fn(() => true), - move: vi.fn(() => ({ id: 4 })), - listForDay: () => [], - broadcast: vi.fn(), + moveVia: vi.fn(() => ({ id: 4 })), }; - const guards = { hasTripPermission: vi.fn(() => false) }; + // The move and what it tells the trip are the service's; the tool hands it the MCP writer. + const writer = { userId: 5, surface: 'mcp' }; + const guards = { hasTripPermission: vi.fn(() => false), tripWriter: vi.fn(async () => writer) }; const mcp = new RoadtripMcp( service as never, { findAccessible: () => true } as never, @@ -509,15 +509,15 @@ describe('Roadtrip MCP registration and search', () => { ); const input = { tripId: 1, dayId: 2, viaId: 4, lat: 48, lng: 10 }; await mcp.updateVia(input, ctx); - expect(service.move).not.toHaveBeenCalled(); + expect(service.moveVia).not.toHaveBeenCalled(); guards.hasTripPermission.mockReturnValue(true); service.dayExists.mockReturnValue(false); await mcp.updateVia(input, ctx); - expect(service.move).not.toHaveBeenCalled(); + expect(service.moveVia).not.toHaveBeenCalled(); service.dayExists.mockReturnValue(true); await mcp.updateVia(input, ctx); - expect(service.move).toHaveBeenCalledWith(4, 2, 48, 10, undefined); - expect(service.broadcast).toHaveBeenCalled(); + expect(guards.tripWriter).toHaveBeenCalledWith(1, 5); + expect(service.moveVia).toHaveBeenCalledWith(2, 4, { lat: 48, lng: 10, after_order_index: undefined }, writer); }); }); @@ -534,15 +534,14 @@ describe('RoadtripMcp — non-member refusal, one case per tool (Plan 3d Task 7 listForTrip: vi.fn(async () => []), tracksForTrip: vi.fn(async () => []), trackExists: vi.fn(async () => true), - create: vi.fn(async () => ({ id: 1 })), - createMany: vi.fn(async () => []), - reanchor: vi.fn(async () => []), - remove: vi.fn(async () => true), - move: vi.fn(async () => ({ id: 1 })), - broadcast: vi.fn(), + addVia: vi.fn(async () => ({ id: 1 })), + addVias: vi.fn(async () => []), + reanchorVias: vi.fn(async () => []), + removeVia: vi.fn(async () => true), + moveVia: vi.fn(async () => ({ id: 1 })), }; const tripsRepo = { findAccessible: vi.fn(async () => findAccessible) }; - const guards = { hasTripPermission: vi.fn(async () => true) }; + const guards = { hasTripPermission: vi.fn(async () => true), tripWriter: vi.fn(async () => ({ userId: 5 })) }; const mcp = new RoadtripMcp(service as never, tripsRepo as never, guards as never, {} as never); return { mcp, service, tripsRepo }; } @@ -559,28 +558,28 @@ describe('RoadtripMcp — non-member refusal, one case per tool (Plan 3d Task 7 const { mcp, service } = build(false); const res = await mcp.addVia({ tripId: 10, dayId: 1, after_order_index: 0, lat: 48, lng: 10 }, ctx); expect(res.isError).toBe(true); - expect(service.create).not.toHaveBeenCalled(); + expect(service.addVia).not.toHaveBeenCalled(); }); it('add_route_vias refuses a non-member trip', async () => { const { mcp, service } = build(false); const res = await mcp.addVias({ tripId: 10, dayId: 1, vias: [] }, ctx); expect(res.isError).toBe(true); - expect(service.createMany).not.toHaveBeenCalled(); + expect(service.addVias).not.toHaveBeenCalled(); }); it('reanchor_route_vias refuses a non-member trip', async () => { const { mcp, service } = build(false); const res = await mcp.reanchorVias({ tripId: 10, dayId: 1, vias: [] }, ctx); expect(res.isError).toBe(true); - expect(service.reanchor).not.toHaveBeenCalled(); + expect(service.reanchorVias).not.toHaveBeenCalled(); }); it('remove_route_via refuses a non-member trip', async () => { const { mcp, service } = build(false); const res = await mcp.removeVia({ tripId: 10, dayId: 1, viaId: 4 }, ctx); expect(res.isError).toBe(true); - expect(service.remove).not.toHaveBeenCalled(); + expect(service.removeVia).not.toHaveBeenCalled(); }); }); diff --git a/server/tests/unit/nest/roadtrip.controller.test.ts b/server/tests/unit/nest/roadtrip.controller.test.ts index c333d5bbb..46dcd7a19 100644 --- a/server/tests/unit/nest/roadtrip.controller.test.ts +++ b/server/tests/unit/nest/roadtrip.controller.test.ts @@ -13,18 +13,27 @@ * AddonGuard again. `@RequireAddon` is metadata and does nothing on its own, so * dropping the guard from the list is a silent regression that opens all seven * routes on an instance that switched the addon off. + * + * What a write tells the trip is the service's (RoadtripService.addVia and its + * siblings, pinned in roadtrip.service.test.ts); here only that every write + * hands it the REST writer of this trip, user and socket. */ import { AddonGuard } from '../../../src/nest/addons/addon.guard'; import { JwtAuthGuard } from '../../../src/nest/auth-core/jwt-auth.guard'; +import type { TripWriter } from '../../../src/nest/common/trip-writer'; import { TripAccessGuard } from '../../../src/nest/permissions/trip-access.guard'; import { RoadtripController } from '../../../src/nest/roadtrip/roadtrip.controller'; import type { RoadtripService } from '../../../src/nest/roadtrip/roadtrip.service'; +import type { User } from '../../../src/types'; import { HttpException } from '@nestjs/common'; import { GUARDS_METADATA } from '@nestjs/common/constants'; import { describe, it, expect, vi } from 'vitest'; const VIA = { id: 5, day_id: 4, after_order_index: 0, sequence: 0, lat: 53, lng: 10 }; +const USER = { id: 3, role: 'user' } as User; +/** The writer restWriter hands back, as the use cases receive it. */ +const WRITER = { userId: 3, role: 'user', surface: 'rest' } as TripWriter; function svc(o: Partial = {}): RoadtripService { return { @@ -33,13 +42,12 @@ function svc(o: Partial = {}): RoadtripService { listForDay: vi.fn().mockReturnValue([VIA]), listForTrip: vi.fn().mockReturnValue([VIA]), tracksForTrip: vi.fn().mockReturnValue([]), - create: vi.fn().mockReturnValue(VIA), - createMany: vi.fn().mockReturnValue([VIA]), - reanchor: vi.fn().mockReturnValue([VIA]), - move: vi.fn().mockReturnValue(VIA), - remove: vi.fn().mockReturnValue(true), - // Every write announces the day's new shape to the trip's other clients. - broadcast: vi.fn(), + restWriter: vi.fn().mockReturnValue(WRITER), + addVia: vi.fn().mockReturnValue(VIA), + addVias: vi.fn().mockReturnValue([VIA]), + reanchorVias: vi.fn().mockReturnValue([VIA]), + moveVia: vi.fn().mockReturnValue(VIA), + removeVia: vi.fn().mockReturnValue(true), ...o, } as unknown as RoadtripService; } @@ -101,9 +109,9 @@ describe('RoadtripController — writes', () => { it('ROADTRIP-CTL-005: creating one checks the day first', async () => { const s = svc(); - expect(await new RoadtripController(s).create('7', '4', body)).toEqual({ via: VIA }); + expect(await new RoadtripController(s).create(USER, '7', '4', body)).toEqual({ via: VIA }); expect(s.dayExists).toHaveBeenCalledWith('4', '7'); - expect(s.create).toHaveBeenCalledWith('4', body); + expect(s.addVia).toHaveBeenCalledWith('4', body, WRITER); }); it('ROADTRIP-CTL-006: every write refuses a day from another trip', async () => { @@ -111,25 +119,25 @@ describe('RoadtripController — writes', () => { const c = new RoadtripController(s); const notFound = { status: 404, body: { error: 'Day not found' } }; - expect(await thrown(() => c.create('7', '999', body))).toEqual(notFound); - expect(await thrown(() => c.createMany('7', '999', { vias: [body] }))).toEqual(notFound); - expect(await thrown(() => c.reanchor('7', '999', { vias: [] }))).toEqual(notFound); - expect(await thrown(() => c.update('7', '999', '5', { lat: 1, lng: 2 }))).toEqual(notFound); - expect(await thrown(() => c.remove('7', '999', '5'))).toEqual(notFound); + expect(await thrown(() => c.create(USER, '7', '999', body))).toEqual(notFound); + expect(await thrown(() => c.createMany(USER, '7', '999', { vias: [body] }))).toEqual(notFound); + expect(await thrown(() => c.reanchor(USER, '7', '999', { vias: [] }))).toEqual(notFound); + expect(await thrown(() => c.update(USER, '7', '999', '5', { lat: 1, lng: 2 }))).toEqual(notFound); + expect(await thrown(() => c.remove(USER, '7', '999', '5'))).toEqual(notFound); // None of them reached the service. - expect(s.create).not.toHaveBeenCalled(); - expect(s.createMany).not.toHaveBeenCalled(); - expect(s.reanchor).not.toHaveBeenCalled(); - expect(s.move).not.toHaveBeenCalled(); - expect(s.remove).not.toHaveBeenCalled(); + expect(s.addVia).not.toHaveBeenCalled(); + expect(s.addVias).not.toHaveBeenCalled(); + expect(s.reanchorVias).not.toHaveBeenCalled(); + expect(s.moveVia).not.toHaveBeenCalled(); + expect(s.removeVia).not.toHaveBeenCalled(); }); it('ROADTRIP-CTL-007: a batch lays the whole chain in one call', async () => { const s = svc(); const batch = { vias: [body, body], replace_legs: [0] }; - expect(await new RoadtripController(s).createMany('7', '4', batch)).toEqual({ vias: [VIA] }); - expect(s.createMany).toHaveBeenCalledWith('4', batch); + expect(await new RoadtripController(s).createMany(USER, '7', '4', batch)).toEqual({ vias: [VIA] }); + expect(s.addVias).toHaveBeenCalledWith('7', '4', batch, WRITER); }); it('ROADTRIP-CTL-008: a track from another trip cannot become this day label', async () => { @@ -138,14 +146,16 @@ describe('RoadtripController — writes', () => { // would become a label that can never be drawn. const s = svc({ trackExists: vi.fn().mockReturnValue(false) }); expect( - await thrown(() => new RoadtripController(s).createMany('7', '4', { vias: [body], track: { place_id: 3 } })), + await thrown(() => + new RoadtripController(s).createMany(USER, '7', '4', { vias: [body], track: { place_id: 3 } }), + ), ).toEqual({ status: 404, body: { error: 'Track not found' } }); - expect(s.createMany).not.toHaveBeenCalled(); + expect(s.addVias).not.toHaveBeenCalled(); }); it('ROADTRIP-CTL-009: the track check runs against the trip, not the day', async () => { const s = svc(); - await new RoadtripController(s).createMany('7', '4', { vias: [body], track: { place_id: 3 } }); + await new RoadtripController(s).createMany(USER, '7', '4', { vias: [body], track: { place_id: 3 } }); expect(s.trackExists).toHaveBeenCalledWith(3, '7'); }); @@ -154,7 +164,7 @@ describe('RoadtripController — writes', () => { // check the existence of. Treating it as one would refuse the only way to // detach a track. const s = svc({ trackExists: vi.fn().mockReturnValue(false) }); - expect(await new RoadtripController(s).createMany('7', '4', { vias: [body], track: null })).toEqual({ + expect(await new RoadtripController(s).createMany(USER, '7', '4', { vias: [body], track: null })).toEqual({ vias: [VIA], }); expect(s.trackExists).not.toHaveBeenCalled(); @@ -163,27 +173,27 @@ describe('RoadtripController — writes', () => { it('ROADTRIP-CTL-011: re-anchoring goes through as one batch', async () => { const s = svc(); const plan = { vias: [{ id: 5, after_order_index: 1 }], remove: [6] }; - expect(await new RoadtripController(s).reanchor('7', '4', plan)).toEqual({ vias: [VIA] }); - expect(s.reanchor).toHaveBeenCalledWith('4', plan); + expect(await new RoadtripController(s).reanchor(USER, '7', '4', plan)).toEqual({ vias: [VIA] }); + expect(s.reanchorVias).toHaveBeenCalledWith('4', plan, WRITER); }); it('ROADTRIP-CTL-012: moving a via reports where it went', async () => { const s = svc(); - expect(await new RoadtripController(s).update('7', '4', '5', { lat: 54, lng: 11 })).toEqual({ via: VIA }); - // Undefined anchor, explicitly: a drag that stayed between the same two stops sends - // no new one, and the service must leave the existing pin alone rather than clear it. - expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11, undefined); + expect(await new RoadtripController(s).update(USER, '7', '4', '5', { lat: 54, lng: 11 })).toEqual({ via: VIA }); + // No anchor, explicitly: a drag that stayed between the same two stops sends no new + // one, and the service must leave the existing pin alone rather than clear it. + expect(s.moveVia).toHaveBeenCalledWith('4', '5', { lat: 54, lng: 11 }, WRITER); }); it('ROADTRIP-CTL-015: a new anchor is passed on, so a via dragged past a stop is re-pinned', async () => { const s = svc(); - await new RoadtripController(s).update('7', '4', '5', { lat: 54, lng: 11, after_order_index: 2 }); - expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11, 2); + await new RoadtripController(s).update(USER, '7', '4', '5', { lat: 54, lng: 11, after_order_index: 2 }); + expect(s.moveVia).toHaveBeenCalledWith('4', '5', { lat: 54, lng: 11, after_order_index: 2 }, WRITER); }); it('ROADTRIP-CTL-013: a via that is not on this day is a 404, not a silent no-op', async () => { - const s = svc({ move: vi.fn().mockReturnValue(null) }); - expect(await thrown(() => new RoadtripController(s).update('7', '4', '999', { lat: 1, lng: 2 }))).toEqual({ + const s = svc({ moveVia: vi.fn().mockReturnValue(null) }); + expect(await thrown(() => new RoadtripController(s).update(USER, '7', '4', '999', { lat: 1, lng: 2 }))).toEqual({ status: 404, body: { error: 'Via not found' }, }); @@ -191,13 +201,13 @@ describe('RoadtripController — writes', () => { it('ROADTRIP-CTL-014: removing one reports success', async () => { const s = svc(); - expect(await new RoadtripController(s).remove('7', '4', '5')).toEqual({ success: true }); - expect(s.remove).toHaveBeenCalledWith('5', '4'); + expect(await new RoadtripController(s).remove(USER, '7', '4', '5')).toEqual({ success: true }); + expect(s.removeVia).toHaveBeenCalledWith('4', '5', WRITER); }); - it('ROADTRIP-CTL-015: removing a via that is not there is a 404', async () => { - const s = svc({ remove: vi.fn().mockReturnValue(false) }); - expect(await thrown(() => new RoadtripController(s).remove('7', '4', '999'))).toEqual({ + it('ROADTRIP-CTL-016: removing a via that is not there is a 404', async () => { + const s = svc({ removeVia: vi.fn().mockReturnValue(false) }); + expect(await thrown(() => new RoadtripController(s).remove(USER, '7', '4', '999'))).toEqual({ status: 404, body: { error: 'Via not found' }, }); @@ -206,43 +216,28 @@ describe('RoadtripController — writes', () => { /** * Two people planning one road trip look at the same line on the same map, and a * reshaped drive moves every arrival time after it. These routes used to write in - * silence, which left the other side reading a route nobody could see change. + * silence, which left the other side reading a route nobody could see change. What + * each write sends is the service's; the route decides who sends it. */ describe('telling the trip', () => { - it('SRV-ROADTRIP-020: every write announces the new shape of the day', async () => { - const cases: [string, (c: RoadtripController) => unknown][] = [ - ['create', (c) => c.create('7', '4', { after_order_index: 0, lat: 1, lng: 2 }, 'sock')], - ['createMany', (c) => c.createMany('7', '4', { vias: [{ after_order_index: 0, lat: 1, lng: 2 }] }, 'sock')], - ['reanchor', (c) => c.reanchor('7', '4', { vias: [] }, 'sock')], - ['update', (c) => c.update('7', '4', '9', { lat: 1, lng: 2 }, 'sock')], - ['remove', (c) => c.remove('7', '4', '9', 'sock')], + it('SRV-ROADTRIP-020: every write hands its use case the writer of this trip, user and socket', async () => { + const cases: [string, keyof RoadtripService, (c: RoadtripController) => unknown][] = [ + ['create', 'addVia', (c) => c.create(USER, '7', '4', { after_order_index: 0, lat: 1, lng: 2 }, 'sock')], + [ + 'createMany', + 'addVias', + (c) => c.createMany(USER, '7', '4', { vias: [{ after_order_index: 0, lat: 1, lng: 2 }] }, 'sock'), + ], + ['reanchor', 'reanchorVias', (c) => c.reanchor(USER, '7', '4', { vias: [] }, 'sock')], + ['update', 'moveVia', (c) => c.update(USER, '7', '4', '9', { lat: 1, lng: 2 }, 'sock')], + ['remove', 'removeVia', (c) => c.remove(USER, '7', '4', '9', 'sock')], ]; - for (const [name, run] of cases) { + for (const [name, useCase, run] of cases) { const s = svc(); await run(new RoadtripController(s)); - expect(s.broadcast, name).toHaveBeenCalledWith('7', 'roadtripVia:changed', { dayId: '4', vias: [VIA] }, 'sock'); + expect(s.restWriter, name).toHaveBeenCalledWith('7', USER, 'sock'); + expect(vi.mocked(s[useCase]).mock.lastCall?.at(-1), name).toBe(WRITER); } }); - - it('SRV-ROADTRIP-021: the day list is read back, not assembled from the write', async () => { - // A reanchor rewrites the whole set and a batch may clear legs before filling them, - // so what the write returned is not what the day now holds. - const s = svc({ reanchor: vi.fn().mockReturnValue([]) } as Partial); - await new RoadtripController(s).reanchor('7', '4', { vias: [] }, 'sock'); - expect(s.broadcast).toHaveBeenCalledWith('7', 'roadtripVia:changed', { dayId: '4', vias: [VIA] }, 'sock'); - }); - - it('SRV-ROADTRIP-022: laying a track down says which track the day now follows', async () => { - const track = { day_id: 4, place_id: 3, name: 'B96' }; - const s = svc({ tracksForTrip: vi.fn().mockReturnValue([track]) } as Partial); - await new RoadtripController(s).createMany('7', '4', { vias: [], track: { place_id: 3 } }, 'sock'); - expect(s.broadcast).toHaveBeenCalledWith('7', 'roadtripTrack:changed', { dayId: '4', track }, 'sock'); - }); - - it('SRV-ROADTRIP-023: a refused write announces nothing', async () => { - const s = svc({ remove: vi.fn().mockReturnValue(false) } as Partial); - await expect(new RoadtripController(s).remove('7', '4', '9', 'sock')).rejects.toThrow(); - expect(s.broadcast).not.toHaveBeenCalled(); - }); }); }); diff --git a/server/tests/unit/nest/roadtrip.service.test.ts b/server/tests/unit/nest/roadtrip.service.test.ts index a3d79e389..5b098593c 100644 --- a/server/tests/unit/nest/roadtrip.service.test.ts +++ b/server/tests/unit/nest/roadtrip.service.test.ts @@ -2,15 +2,18 @@ import { Days } from '../../../src/db/entities/Days.entity'; import { Places } from '../../../src/db/entities/Places.entity'; import { RoadtripDayTracks } from '../../../src/db/entities/RoadtripDayTracks.entity'; import { RoadtripVias } from '../../../src/db/entities/RoadtripVias.entity'; +import type { TripWriter } from '../../../src/nest/common/trip-writer'; import { RoadtripService } from '../../../src/nest/roadtrip/roadtrip.service'; +import type { User } from '../../../src/types'; import { createSnapshotTestDb } from '../../helpers/db-mock'; import { createDay, createPlace, createTrip, createUser } from '../../helpers/factories'; import { updateRows } from '../../helpers/factories/rows'; import { resetTestDb } from '../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { createTestUnitOfWork } from '../../helpers/test-uow'; +import type { RoadtripVia } from '@trek/shared'; -import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; /** * Via points and tracks, against real rows through @@ -32,9 +35,9 @@ afterAll(async () => { testDb.close(); }); -async function makeService() { +async function makeService(realtime: { broadcast: (...args: unknown[]) => void } = { broadcast: () => {} }) { return new RoadtripService( - { broadcast: () => {} } as never, + realtime as never, await createTestUnitOfWork(testDb), t.repo(Days), t.repo(Places), @@ -374,3 +377,124 @@ describe('RoadtripService', () => { expect(rows.map((r) => r.sequence).sort((a, b) => a - b)).toEqual([0, 1]); }); }); + +interface Sent { + event: string; + payload: { dayId?: unknown; vias?: RoadtripVia[]; track?: unknown }; + all: boolean; +} + +/** A writer that keeps what a use case sends, in order, as a TripEventSink receives it. */ +function recordingWriter(): { sent: Sent[]; writer: TripWriter } { + const sent: Sent[] = []; + const keep = (event: string, payload: unknown, all: boolean) => { + sent.push({ event, payload: payload as Sent['payload'], all }); + }; + const writer: TripWriter = { + userId: 1, + role: 'user', + surface: 'mcp', + events: { + emit: (event, payload) => keep(event, payload, false), + emitAll: (event, payload) => keep(event, payload, true), + }, + }; + return { sent, writer }; +} + +/** + * Two people planning one road trip look at the same line on the same map, and a reshaped + * drive moves every arrival time after it. Each via write is one use case for the routes + * and the tools alike, and tells the trip what the day runs through now once it is written. + */ +describe('RoadtripService: telling the trip', () => { + let service: RoadtripService; + let f: Awaited>; + beforeEach(async () => { + service = await makeService(); + f = await fixture(); + }); + + it('SRV-ROADTRIP-021: every via write sends the day read back after it, with the day id as text', async () => { + const { sent, writer } = recordingWriter(); + const first = await service.addVia(f.dayA1.id, { after_order_index: 0, lat: 53, lng: 10 }, writer); + const second = await service.addVia(String(f.dayA1.id), { after_order_index: 1, lat: 54, lng: 11 }, writer); + const moved = await service.moveVia(f.dayA1.id, first.id, { lat: 53.5, lng: 10.5 }, writer); + // The reanchor answers nothing it was asked to move: what goes out is the day as it is now. + await service.reanchorVias(f.dayA1.id, { vias: [], remove: [second.id] }, writer); + await service.removeVia(f.dayA1.id, first.id, writer); + + expect(sent.map((s) => s.event)).toEqual(Array(5).fill('roadtripVia:changed')); + expect(sent.some((s) => s.all)).toBe(false); + // G8: the routes always sent the day id as the path spelled it; a tool sends it the same way. + expect(sent.map((s) => s.payload.dayId)).toEqual(Array(5).fill(String(f.dayA1.id))); + expect(sent.map((s) => s.payload.vias?.map((v) => v.id))).toEqual([ + [first.id], + [first.id, second.id], + [first.id, second.id], + [first.id], + [], + ]); + expect(sent[2]?.payload.vias?.[0]).toEqual(moved); + }); + + it('SRV-ROADTRIP-022: a chain says which track the day follows now, after its vias', async () => { + const { sent, writer } = recordingWriter(); + const chain = { vias: [{ after_order_index: 0, lat: 53, lng: 10 }], track: { place_id: f.track.id, stray_km: 2 } }; + const made = await service.addVias(f.tripA.id, f.dayA1.id, chain, writer); + // Left out, the track stays; the event still says which one the day follows. + await service.addVias(f.tripA.id, f.dayA1.id, { vias: [] }, writer); + await service.addVias(f.tripA.id, f.dayA1.id, { vias: [], track: null }, writer); + + const followed = { day_id: f.dayA1.id, place_id: f.track.id, stray_km: 2 }; + expect(sent.map((s) => [s.event, s.payload])).toEqual([ + ['roadtripVia:changed', { dayId: String(f.dayA1.id), vias: made }], + ['roadtripTrack:changed', { dayId: String(f.dayA1.id), track: followed }], + ['roadtripVia:changed', { dayId: String(f.dayA1.id), vias: made }], + ['roadtripTrack:changed', { dayId: String(f.dayA1.id), track: followed }], + ['roadtripVia:changed', { dayId: String(f.dayA1.id), vias: made }], + ['roadtripTrack:changed', { dayId: String(f.dayA1.id), track: null }], + ]); + }); + + it('SRV-ROADTRIP-023: a via that is not on the day changes nothing and tells nobody', async () => { + const { sent, writer } = recordingWriter(); + const other = await service.create(f.dayA2.id, { after_order_index: 0, lat: 53, lng: 10 }); + + expect(await service.moveVia(f.dayA1.id, other.id, { lat: 1, lng: 2 }, writer)).toBeNull(); + expect(await service.removeVia(f.dayA1.id, other.id, writer)).toBe(false); + expect(sent).toEqual([]); + expect(await service.listForDay(f.dayA2.id)).toEqual([other]); + }); + + it('SRV-ROADTRIP-024: the REST writer sends to the trip and leaves out the socket that made the change', async () => { + const calls: unknown[][] = []; + const rest = await makeService({ broadcast: (...args) => calls.push(args) }); + const user = { id: 1, role: 'user' } as User; + const tripId = String(f.tripA.id); + const dayId = String(f.dayA1.id); + + const via = await rest.addVia( + dayId, + { after_order_index: 0, lat: 53, lng: 10 }, + rest.restWriter(tripId, user, 'sock'), + ); + + expect(calls).toEqual([[tripId, 'roadtripVia:changed', { dayId, vias: [via] }, 'sock']]); + }); + + it('SRV-ROADTRIP-025: a read that fails after the commit is logged, and the write still answers', async () => { + const { sent, writer } = recordingWriter(); + vi.spyOn(service, 'listForDay').mockRejectedValueOnce(new Error('read failed')); + const via = await service.addVia(f.dayA1.id, { after_order_index: 0, lat: 53, lng: 10 }, writer); + expect(sent).toEqual([]); + + // The chain's track read fails on its own: the via event has gone, the track event is skipped. + vi.spyOn(service, 'tracksForTrip').mockRejectedValueOnce(new Error('read failed')); + const vias = await service.addVias(f.tripA.id, f.dayA1.id, { vias: [] }, writer); + expect(sent.map((s) => s.event)).toEqual(['roadtripVia:changed']); + + expect(vias).toEqual([via]); + expect(await service.listForDay(f.dayA1.id)).toEqual([via]); + }); +}); diff --git a/shared/src/realtime/events.schema.ts b/shared/src/realtime/events.schema.ts index 9d94b87bb..94c774526 100644 --- a/shared/src/realtime/events.schema.ts +++ b/shared/src/realtime/events.schema.ts @@ -110,6 +110,10 @@ export const TREK_WS_EVENTS = { // rather than a change to the itinerary everybody reads. That does not hold: two people // planning a road trip see the same line on the same map, and a reshaped drive changes // every arrival time after it. + // + // The via writes send dayId as text, the way the routes always did, on every surface. + // Assignments, accommodations and hike plans still send it as a number; the client + // reads it through Number(), so both forms stay valid here. 'roadtripVia:changed': { scope: 'trip', payload: z.object({ dayId: id, vias: z.array(entity) }), @@ -198,7 +202,8 @@ export const TREK_WS_EVENTS = { payload: z.union([z.object({ reservation: entity }), empty]), }, 'reservation:deleted': { scope: 'trip', payload: z.object({ reservationId: id }) }, - // DRIFT: REST sends { positions, day_id }; the MCP tool sends { positions, dayId }. + // REST sends { positions, day_id }; the MCP tool sends { positions, day_id, dayId }, + // keeping the dayId key it always sent next to REST's. 'reservation:positions': { scope: 'trip', payload: z.union([