From ce7b5bf453ca77669c14eabf6057091ec8d5edd7 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 22 Oct 2026 12:14:27 +0200 Subject: [PATCH] fix(realtime): trip, member, chat, file, settings, tag and notification writes in parity --- .../nest/assistant/assistant-share.service.ts | 20 +- server/src/nest/collab/collab.controller.ts | 39 +- server/src/nest/collab/collab.mcp.ts | 12 +- server/src/nest/collab/collab.service.ts | 77 +++- server/src/nest/files/files.controller.ts | 20 +- server/src/nest/files/files.mcp.ts | 4 +- server/src/nest/files/files.service.ts | 60 ++- .../notifications/notifications.controller.ts | 24 +- .../nest/notifications/notifications.mcp.ts | 7 +- .../notifications/notifications.service.ts | 72 +-- .../nest/settings/settings-writes.service.ts | 41 ++ .../src/nest/settings/settings.controller.ts | 13 +- server/src/nest/settings/settings.mcp.ts | 9 +- server/src/nest/settings/settings.module.ts | 13 +- server/src/nest/tags/tag-writes.service.ts | 46 ++ server/src/nest/tags/tags.controller.ts | 34 +- server/src/nest/tags/tags.mcp.ts | 15 +- server/src/nest/tags/tags.module.ts | 7 +- .../trip-member-writes.service.ts | 88 ++++ .../trip-members/trip-members.controller.ts | 47 +- .../nest/trip-members/trip-members.module.ts | 6 +- .../nest/trip-members/trip-members.service.ts | 15 +- server/src/nest/trips/trip-writes.service.ts | 84 ++++ server/src/nest/trips/trips.controller.ts | 62 ++- server/src/nest/trips/trips.helpers.ts | 25 +- server/src/nest/trips/trips.mcp.ts | 88 ++-- server/src/nest/trips/trips.module.ts | 3 +- server/src/nest/trips/trips.rpc.ts | 10 +- server/src/nest/trips/trips.service.ts | 4 +- .../broadcast-parity/collab-trip.cases.ts | 436 ++++++++++++++++-- .../unit/mcp/tools-collab-polls-chat.test.ts | 35 +- server/tests/unit/mcp/tools-files.test.ts | 52 +++ server/tests/unit/mcp/tools-notes.test.ts | 28 ++ .../unit/mcp/tools-notifications.test.ts | 60 +++ server/tests/unit/mcp/tools-prompts.test.ts | 6 +- server/tests/unit/mcp/tools-settings.test.ts | 28 ++ .../unit/mcp/tools-tags-maps-weather.test.ts | 27 ++ .../tests/unit/mcp/tools-trip-members.test.ts | 94 +++- server/tests/unit/mcp/tools-trips.test.ts | 92 +++- .../assistant/assistant-share.service.test.ts | 49 +- .../tests/unit/nest/collab.controller.test.ts | 64 +-- server/tests/unit/nest/collab.service.test.ts | 108 ++++- .../tests/unit/nest/files.controller.test.ts | 36 +- .../nest/notifications.controller.test.ts | 18 +- .../unit/nest/settings.controller.test.ts | 53 ++- .../settings/settings-writes.service.test.ts | 88 ++++ .../tests/unit/nest/tags.controller.test.ts | 41 +- .../unit/nest/trip-members.controller.test.ts | 75 ++- .../unit/nest/trip-members.service.test.ts | 15 + .../tests/unit/nest/trips.controller.test.ts | 72 ++- server/tests/unit/trips/trips.rpc.test.ts | 15 + 51 files changed, 2055 insertions(+), 382 deletions(-) create mode 100644 server/src/nest/settings/settings-writes.service.ts create mode 100644 server/src/nest/tags/tag-writes.service.ts create mode 100644 server/src/nest/trip-members/trip-member-writes.service.ts create mode 100644 server/src/nest/trips/trip-writes.service.ts create mode 100644 server/tests/unit/nest/settings/settings-writes.service.test.ts diff --git a/server/src/nest/assistant/assistant-share.service.ts b/server/src/nest/assistant/assistant-share.service.ts index 91dcdd88f..253b0fa9b 100644 --- a/server/src/nest/assistant/assistant-share.service.ts +++ b/server/src/nest/assistant/assistant-share.service.ts @@ -11,9 +11,6 @@ import { InjectRepository } from '@mikro-orm/nestjs'; import { Injectable } from '@nestjs/common'; import { COLLAB_MESSAGE_ORIGIN_ASSISTANT } from '@trek/shared'; -/** How much of the message the chat notification previews, as for one typed in the chat. */ -const PREVIEW_CHARS = 80; - /** The row a message id on the wire (`msg-`) names, or null for anything else. */ export function messageRowOf(messageId: string): number | null { const match = /^msg-([1-9]\d{0,15})$/.exec(messageId); @@ -26,7 +23,9 @@ export function messageRowOf(messageId: string): number | null { * as shared from the assistant (`origin`), which only this path sets: the * mark says the user shared an answer the assistant gave them in this trip, * so the server checks that answer, not the words. It reaches the chat like a - * typed message, broadcast and notified the same way. + * typed message, broadcast and notified the same way, with one difference: the + * asking tab is not left out. It never put the message into its own chat, so it + * hears it like every other tab (the chat drops a message it already holds). */ @Injectable() export class AssistantShareService { @@ -48,7 +47,8 @@ export class AssistantShareService { threadId: number, messageId: string, text: string, - socketId: string | undefined, + // The asking tab, which the route hands on; deliberately not skipped (see the class comment). + _askingSocketId: string | undefined, ) { const collabOn = await this.addons.isAddonEnabled(ADDON_IDS.COLLAB); if (!collabOn || !(await this.addons.getCollabFeatures()).chat) throw aiRefusal('forbidden', 'Collab chat is off'); @@ -59,13 +59,13 @@ export class AssistantShareService { const row = rowId === null ? null : await this.messages.findInThread(rowId, threadId); if (row?.role !== 'assistant') throw aiRefusal('not_found', 'Message not found'); - const created = await this.collab.createMessage(tripId, user.id, text, null, [], COLLAB_MESSAGE_ORIGIN_ASSISTANT); + const created = await this.collab.postMessage( + tripId, + { text, origin: COLLAB_MESSAGE_ORIGIN_ASSISTANT }, + this.collab.restWriter(String(tripId), user, undefined), + ); const message = created.message; if (!message) throw aiRefusal('generic', 'The message could not be posted'); - this.collab.broadcast(String(tripId), 'collab:message:created', { message }, socketId); - const posted = message.text; - const preview = posted.length > PREVIEW_CHARS ? `${posted.substring(0, PREVIEW_CHARS)}...` : posted; - await this.collab.notifyCollab(String(tripId), user, preview); return { message }; } } diff --git a/server/src/nest/collab/collab.controller.ts b/server/src/nest/collab/collab.controller.ts index af6d8d881..8768ab306 100644 --- a/server/src/nest/collab/collab.controller.ts +++ b/server/src/nest/collab/collab.controller.ts @@ -167,15 +167,11 @@ export class CollabController { @Body() body: CollabNoteCreateDto, @Headers('x-socket-id') socketId?: string, ) { - const note = await this.collab.createNote(tripId, user.id, { - title: body.title, - content: body.content, - category: body.category, - color: body.color, - website: body.website, - }); - this.collab.broadcast(tripId, 'collab:note:created', { note }, socketId); - await this.collab.notifyCollab(tripId, user); + const note = await this.collab.postNote( + tripId, + { title: body.title, content: body.content, category: body.category, color: body.color, website: body.website }, + this.collab.restWriter(tripId, user, socketId), + ); return { note }; } @@ -525,25 +521,24 @@ export class CollabController { } const replyTo = body.reply_to === undefined || body.reply_to === '' || body.reply_to === null ? null : Number(body.reply_to); - const result = await this.collab.createMessage( + const result = await this.collab.postMessage( tripId, - user.id, - text, - Number.isFinite(replyTo as number) ? replyTo : null, - uploaded.map((file) => ({ - filename: file.filename, - originalname: file.originalname, - size: file.size, - mimetype: file.mimetype, - })), + { + text, + replyTo: Number.isFinite(replyTo as number) ? replyTo : null, + files: uploaded.map((file) => ({ + filename: file.filename, + originalname: file.originalname, + size: file.size, + mimetype: file.mimetype, + })), + }, + this.collab.restWriter(tripId, user, socketId), ); if (result.error === 'reply_not_found') { for (const name of committed) await this.storage.delete('files', name).catch(() => {}); throw new HttpException({ error: 'Reply target message not found' }, 400); } - this.collab.broadcast(tripId, 'collab:message:created', { message: result.message }, socketId); - const preview = text || 'sent an image'; - await this.collab.notifyCollab(tripId, user, preview.length > 80 ? preview.substring(0, 80) + '...' : preview); return { message: result.message }; } diff --git a/server/src/nest/collab/collab.mcp.ts b/server/src/nest/collab/collab.mcp.ts index 5e8230530..88bb7f5d3 100644 --- a/server/src/nest/collab/collab.mcp.ts +++ b/server/src/nest/collab/collab.mcp.ts @@ -129,8 +129,12 @@ export class CollabMcp { ) { if (!(await this.collab.verifyTripAccess(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('collab_edit', tripId, ctx.userId))) return permissionDenied(); - const note = await this.collab.createNote(tripId, ctx.userId, { title, content, category, color, website, pinned }); - this.guards.safeBroadcast(tripId, 'collab:note:created', { note }); + // Through the route's own use case, so the fellow travellers get the same notification. + const note = await this.collab.postNote( + tripId, + { title, content, category, color, website, pinned }, + await this.guards.tripWriter(tripId, ctx.userId), + ); return ok({ note }); } @@ -364,9 +368,9 @@ export class CollabMcp { ) { if (!(await this.collab.verifyTripAccess(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('collab_edit', tripId, ctx.userId))) return permissionDenied(); - const result = await this.collab.createMessage(tripId, ctx.userId, text, replyTo ?? null); + const writer = await this.guards.tripWriter(tripId, ctx.userId); + const result = await this.collab.postMessage(tripId, { text, replyTo: replyTo ?? null }, writer); if (result.error) return errorResult(result.error); - this.guards.safeBroadcast(tripId, 'collab:message:created', { message: result.message }); return ok({ message: result.message }); } diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index 002fdd8a4..242a18e73 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -21,6 +21,7 @@ import { checkSsrf, createOutboundDispatcher } from '../../utils/ssrfGuard'; import { avatarUrl } from '../common/avatarUrl'; import { RateLimitService } from '../common/rate-limit.service'; import { toRowId } from '../common/row-id'; +import { restTripWriter, type TripWriter } from '../common/trip-writer'; import { UnitOfWork } from '../database/unit-of-work'; import { NotificationsService } from '../notifications/notifications.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -70,6 +71,20 @@ export interface LinkPreviewResult { rateLimited?: boolean; } +/** What a chat notification shows of a message: its first 80 characters, or that it was an image. */ +function chatPreview(text: string): string { + const shown = text.trim() || 'sent an image'; + return shown.length > 80 ? `${shown.substring(0, 80)}...` : shown; +} + +/** A chat message as postMessage takes it; only the assistant's share sets `origin`. */ +export interface ChatMessageInput { + text: string; + replyTo?: number | null; + files?: Array<{ filename: string; originalname: string; size: number; mimetype: string }>; + origin?: string | null; +} + /** * Outbound fetches one user may trigger per minute. Deliberately generous: the * client asks for a preview per rendered message (`LIMIT 100`) and per note with @@ -140,13 +155,13 @@ function scrapeOpenGraph(html: string): Omit { * numeric value feeds both the trip-scoping SELECT and the write that * follows it. * - * **`deleteMessage`'s pre-existing `username` gap.** The legacy statement - * behind `findInTrip` (CB51) is `SELECT * FROM collab_messages WHERE id = ? - * AND trip_id = ?` — no join to `users` — yet the legacy code returned - * `{ username: message.username }`, a column that statement never selected. - * That has always evaluated to `undefined` at runtime; preserved exactly - * below (parity is law), not fixed — flagged in the task-5 report as a - * pre-existing defect for a ruling, the same as `§17`'s other "surprises". + * **`deleteMessage`'s `username`.** The legacy statement behind `findInTrip` + * (CB51) never joined `users`, so the legacy `{ username: message.username }` + * was always `undefined` and REST fell back to the caller's own name. The + * author's name is now read through the joined shape before the delete, so + * MCP (which has no session user to fall back on) names the author too. REST + * sends the same bytes as before: only the author may delete a message, so + * the author's name is the caller's name. */ @Injectable() export class CollabService { @@ -204,6 +219,45 @@ export class CollabService { this.realtime.broadcast(tripId, event, payload, socketId); } + /* ------------------------------------------------------------------ */ + /* Posting (every surface) */ + /* ------------------------------------------------------------------ */ + + /** REST's writer for a post: the trip room minus the tab that sent the request. */ + restWriter(tripId: string | number, user: Pick, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** + * A note posted to the trip: every tab gets it, every fellow traveller a + * notification. The route always notified; through this, MCP does too. + */ + async postNote(tripId: string | number, data: Parameters[2], writer: TripWriter) { + const note = await this.createNote(tripId, writer.userId, data); + writer.events.emit('collab:note:created', { note }); + await this.notifyCollab(String(tripId), await this.actorOf(writer)); + return note; + } + + /** + * A chat message posted to the trip, sent and notified like a typed one + * wherever it came from. A reply to a message that is gone comes back as + * `{ error }` with nothing sent, for the caller to refuse in its own words. + */ + async postMessage(tripId: string | number, input: ChatMessageInput, writer: TripWriter) { + const { text, replyTo = null, files = [], origin = null } = input; + const result = await this.createMessage(tripId, writer.userId, text, replyTo, files, origin); + if (!result.message) return result; + writer.events.emit('collab:message:created', { message: result.message }); + await this.notifyCollab(String(tripId), await this.actorOf(writer), chatPreview(text)); + return result; + } + + /** The writer as a notification names them: by id, and by the address the route read off its session. */ + private async actorOf(writer: TripWriter): Promise> { + return { id: writer.userId, email: await this.notifications.actorEmail(writer.userId) }; + } + /* ------------------------------------------------------------------ */ /* Reactions */ /* ------------------------------------------------------------------ */ @@ -686,6 +740,7 @@ export class CollabService { const message = await this.messagesRepo.findInTrip(idNum, tripIdNum); if (!message) return { error: 'not_found' }; if (Number(message.user_id) !== Number(userId)) return { error: 'not_owner' }; + const author = await this.messagesRepo.findWithReply(message.id); const attachments = await this.messagesRepo.listFilenamesForMessage(message.id, tripIdNum); await this.uow.transactional(async () => { @@ -700,10 +755,8 @@ export class CollabService { /* best effort */ }); } - // `message.username` — see the class docstring's "pre-existing `username` - // gap" note: `findInTrip` never joins `users`, so this has always been - // `undefined` at runtime. Preserved exactly, not fixed. - return { username: undefined }; + // Read before the delete; see the class docstring's `username` note. + return { username: author?.username }; } /* ------------------------------------------------------------------ */ @@ -854,7 +907,7 @@ export class CollabService { } /** Fire-and-forget collab notification (mirrors the legacy route's dynamic import). */ - async notifyCollab(tripId: string, actor: User, preview?: string): Promise { + async notifyCollab(tripId: string, actor: Pick, preview?: string): Promise { // Injected, not a lazy import of the old notifications bridge. The laziness bought // nothing the module graph does not already give — NotificationsModule // reaches nothing in this direction — and it hid the edge while handing the diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index f299d9264..905b346d6 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -374,6 +374,7 @@ export class FilesController { @Param('tripId') tripId: string, @Param('id') id: string, @Body() body: FileLinkDto, + @Headers('x-socket-id') socketId?: string, ) { if (!(await this.files.can('file_edit', trip, user))) { throw new HttpException({ error: 'No permission' }, 403); @@ -388,12 +389,16 @@ export class FilesController { place_id: body.place_id, budget_item_id: body.budget_item_id, }); - const links = await this.files.createFileLink(id, { - reservation_id: body.reservation_id, - assignment_id: body.assignment_id, - place_id: body.place_id, - budget_item_id: body.budget_item_id, - }); + const links = await this.files.linkFile( + id, + { + reservation_id: body.reservation_id, + assignment_id: body.assignment_id, + place_id: body.place_id, + budget_item_id: body.budget_item_id, + }, + this.files.restWriter(tripId, user, socketId), + ); return { success: true, links }; } @@ -406,6 +411,7 @@ export class FilesController { @Param('tripId') tripId: string, @Param('id') id: string, @Param('linkId') linkId: string, + @Headers('x-socket-id') socketId?: string, ) { if (!(await this.files.can('file_edit', trip, user))) { throw new HttpException({ error: 'No permission' }, 403); @@ -417,7 +423,7 @@ export class FilesController { if (!file) { throw new HttpException({ error: 'File not found' }, 404); } - await this.files.deleteFileLink(linkId, id); + await this.files.unlinkFile(id, linkId, this.files.restWriter(tripId, user, socketId)); return { success: true }; } diff --git a/server/src/nest/files/files.mcp.ts b/server/src/nest/files/files.mcp.ts index 74a792f89..6534c1417 100644 --- a/server/src/nest/files/files.mcp.ts +++ b/server/src/nest/files/files.mcp.ts @@ -268,7 +268,7 @@ export class FilesMcp { } const foreign = await this.files.findForeignLinkTarget(tripId, targets); if (foreign) return errorResult(`Linked item does not belong to this trip (${foreign}).`); - const links = await this.files.createFileLink(fileId, targets); + const links = await this.files.linkFile(fileId, targets, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true, links }); } @@ -294,7 +294,7 @@ export class FilesMcp { // against :tripId first, exactly as the REST route does it. Otherwise a member of // any trip could drop a link row belonging to a foreign trip's file. if (!(await this.files.getFileById(fileId, tripId))) return errorResult('File not found.'); - await this.files.deleteFileLink(linkId, fileId); + await this.files.unlinkFile(fileId, linkId, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } diff --git a/server/src/nest/files/files.service.ts b/server/src/nest/files/files.service.ts index 74c45a8c1..fc3de2bcc 100644 --- a/server/src/nest/files/files.service.ts +++ b/server/src/nest/files/files.service.ts @@ -19,6 +19,7 @@ import { verifyJwtAndLoadUser } from '../auth-core/jwt-verify'; import { avatarUrl } from '../common/avatarUrl'; 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 { PermissionsService } from '../permissions/permissions.service'; import { RealtimeService } from '../realtime/realtime.service'; @@ -47,6 +48,18 @@ function formatFile(file: TripFileJoinRow) { }; } +/** A file as the file list shows it: the row plus the ids of everything its links point at. */ +function withLinkedIds(file: TripFileJoinRow, fileLinks: FileLinkTargetRow[]) { + return { + ...formatFile(file), + linked_reservation_ids: fileLinks.filter((l) => l.reservation_id).map((l) => l.reservation_id), + linked_place_ids: fileLinks.filter((l) => l.place_id).map((l) => l.place_id), + // MCP's link_trip_file and plugins can pin a file to one stop of the plan. + linked_assignment_ids: fileLinks.filter((l) => l.assignment_id).map((l) => l.assignment_id), + linked_budget_item_ids: fileLinks.filter((l) => l.budget_item_id).map((l) => l.budget_item_id), + }; +} + export interface FileLink { file_id: number; reservation_id: number | null; @@ -343,17 +356,7 @@ export class FilesService { linksMap[link.file_id].push(link); } - return files.map((f) => { - const fileLinks = linksMap[f.id] || []; - return { - ...formatFile(f), - linked_reservation_ids: fileLinks.filter((l) => l.reservation_id).map((l) => l.reservation_id), - linked_place_ids: fileLinks.filter((l) => l.place_id).map((l) => l.place_id), - // MCP's link_trip_file and plugins can pin a file to one stop of the plan. - linked_assignment_ids: fileLinks.filter((l) => l.assignment_id).map((l) => l.assignment_id), - linked_budget_item_ids: fileLinks.filter((l) => l.budget_item_id).map((l) => l.budget_item_id), - }; - }); + return files.map((f) => withLinkedIds(f, linksMap[f.id] || [])); } /** R2: `createFile`'s `trip_files` insert and its conditional `file_links` insert (FL9+FL10) run inside one transaction. */ @@ -546,4 +549,39 @@ export class FilesService { async getFileLinks(fileId: string | number) { return await this.fileLinksRepo.listForFileWithReservationTitle(toRowId(fileId) ?? -1); } + + // --------------------------------------------------------------------------- + // Link changes, for every surface + // --------------------------------------------------------------------------- + + /** REST's writer for a file use case: the trip room minus the tab that sent the request. */ + restWriter(tripId: string | number, user: Pick, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** + * Attaches a file to one more booking, place, stop or expense. Every tab of the + * trip gets the file again with all its links, the way the file list reads it, + * so the attachment shows without a reload. The callers have checked the file + * and the targets against the trip. + */ + async linkFile(fileId: string | number, targets: Parameters[1], writer: TripWriter) { + const links = await this.createFileLink(fileId, targets); + await this.announceLinks(fileId, writer); + return links; + } + + /** Removes one attachment of a file; the file stays, and every tab gets it with the links it has left. */ + async unlinkFile(fileId: string | number, linkId: string | number, writer: TripWriter): Promise { + await this.deleteFileLink(linkId, fileId); + await this.announceLinks(fileId, writer); + } + + private async announceLinks(fileId: string | number, writer: TripWriter): Promise { + const idNum = toRowId(fileId) ?? -1; + const row = await this.tripFilesRepo.findByIdWithJoins(idNum); + if (!row) return; + const file = withLinkedIds(row, await this.fileLinksRepo.listForFiles([idNum])); + writer.events.emit('file:updated', { file }); + } } diff --git a/server/src/nest/notifications/notifications.controller.ts b/server/src/nest/notifications/notifications.controller.ts index b2dc88cad..e87defa35 100644 --- a/server/src/nest/notifications/notifications.controller.ts +++ b/server/src/nest/notifications/notifications.controller.ts @@ -19,6 +19,7 @@ import { Controller, Delete, Get, + Headers, HttpCode, HttpException, Param, @@ -149,8 +150,11 @@ export class NotificationsController { } @Put('in-app/read-all') - async readAll(@CurrentUser() user: User): Promise<{ success: boolean; count: number }> { - return { success: true, count: await this.notifications.markAllRead(user.id) }; + async readAll( + @CurrentUser() user: User, + @Headers('x-socket-id') socketId?: string, + ): Promise<{ success: boolean; count: number }> { + return { success: true, count: await this.notifications.markAllRead(user.id, socketId) }; } @Delete('in-app/all') @@ -159,18 +163,26 @@ export class NotificationsController { } @Put('in-app/:id/read') - async markRead(@CurrentUser() user: User, @Param('id') idParam: string): Promise<{ success: boolean }> { + async markRead( + @CurrentUser() user: User, + @Param('id') idParam: string, + @Headers('x-socket-id') socketId?: string, + ): Promise<{ success: boolean }> { const id = this.parseId(idParam); - if (!(await this.notifications.markRead(id, user.id))) { + if (!(await this.notifications.markRead(id, user.id, socketId))) { throw new HttpException({ error: 'Not found' }, 404); } return { success: true }; } @Put('in-app/:id/unread') - async markUnread(@CurrentUser() user: User, @Param('id') idParam: string): Promise<{ success: boolean }> { + async markUnread( + @CurrentUser() user: User, + @Param('id') idParam: string, + @Headers('x-socket-id') socketId?: string, + ): Promise<{ success: boolean }> { const id = this.parseId(idParam); - if (!(await this.notifications.markUnread(id, user.id))) { + if (!(await this.notifications.markUnread(id, user.id, socketId))) { throw new HttpException({ error: 'Not found' }, 404); } return { success: true }; diff --git a/server/src/nest/notifications/notifications.mcp.ts b/server/src/nest/notifications/notifications.mcp.ts index 820bbfac0..ac7f6853f 100644 --- a/server/src/nest/notifications/notifications.mcp.ts +++ b/server/src/nest/notifications/notifications.mcp.ts @@ -85,7 +85,8 @@ export class NotificationsMcp { access: { group: 'notifications', mode: 'write' }, }) async markNotificationRead({ notificationId }: { notificationId: number }, ctx: McpContext) { - const success = await this.notifications.markRead(notificationId, ctx.userId); + // No socket to leave out: every tab of the user, the asking one included, follows the bell. + const success = await this.notifications.markRead(notificationId, ctx.userId, undefined); if (!success) return { content: [{ type: 'text' as const, text: 'Notification not found.' }], isError: true }; return ok({ success: true }); } @@ -100,7 +101,7 @@ export class NotificationsMcp { access: { group: 'notifications', mode: 'write' }, }) async markNotificationUnread({ notificationId }: { notificationId: number }, ctx: McpContext) { - const success = await this.notifications.markUnread(notificationId, ctx.userId); + const success = await this.notifications.markUnread(notificationId, ctx.userId, undefined); if (!success) return { content: [{ type: 'text' as const, text: 'Notification not found.' }], isError: true }; return ok({ success: true }); } @@ -113,7 +114,7 @@ export class NotificationsMcp { access: { group: 'notifications', mode: 'write' }, }) async markAllNotificationsRead(_input: Record, ctx: McpContext) { - const count = await this.notifications.markAllRead(ctx.userId); + const count = await this.notifications.markAllRead(ctx.userId, undefined); return ok({ success: true, count }); } diff --git a/server/src/nest/notifications/notifications.service.ts b/server/src/nest/notifications/notifications.service.ts index 931e29e76..1da3483c5 100644 --- a/server/src/nest/notifications/notifications.service.ts +++ b/server/src/nest/notifications/notifications.service.ts @@ -1,6 +1,6 @@ import { readEnv, getAppUrl } from '../../app-config'; import { Notifications } from '../../db/entities/Notifications.entity'; -import type { NotificationsRepository } from '../../db/repositories/Notifications.repository'; +import type { NotificationJoinRow, NotificationsRepository } from '../../db/repositories/Notifications.repository'; import { logDebug, logError } from '../audit/audit-log.logger'; import { avatarUrl } from '../common/avatarUrl'; import { UnitOfWork } from '../database/unit-of-work'; @@ -94,6 +94,20 @@ interface NotificationRow { type RespondResult = { success: boolean; error?: string; notification?: NotificationRow }; +/** + * A stored notification as the bell reads it: the time as ISO UTC, the sender's + * avatar as a URL. The repository row's `type`/`scope`/`response` are plain + * TEXT; narrowing them to the unions above is the same unchecked boundary the + * legacy `db.all(...)` generic drew. + */ +function withSender(row: NotificationJoinRow): NotificationRow { + return { + ...row, + created_at: toUtcIso(row.created_at!), + sender_avatar: avatarUrl({ avatar: row.sender_avatar }), + } as NotificationRow; +} + // ── Event config map ─────────────────────────────────────────────────────── interface EventNotifConfig { @@ -612,34 +626,45 @@ export class NotificationsService { const total = await this.notificationsRepo.countForRecipient(userId, unreadOnly); const unread_count = await this.notificationsRepo.countUnreadForRecipient(userId); - // The repository row's `type`/`scope`/`response` are the physical TEXT - // columns (plain `string`); this service's own `NotificationRow` narrows - // them to the literal unions it writes — the same unchecked boundary the - // legacy `db.all(...)` generic drew (SQLite has no enum - // type to check against either way). - const mapped = rows.map((r) => ({ - ...r, - created_at: toUtcIso(r.created_at!), - sender_avatar: avatarUrl({ avatar: r.sender_avatar }), - })) as NotificationRow[]; - - return { notifications: mapped, total, unread_count }; + return { notifications: rows.map(withSender), total, unread_count }; } async unreadCount(userId: number): Promise { return await this.notificationsRepo.countUnreadForRecipient(userId); } - async markRead(notificationId: number, userId: number): Promise { - return (await this.notificationsRepo.setRead(notificationId, userId, 1)) > 0; + /** + * The read state of one of the user's notifications. The user's other tabs and + * devices get the row as it now reads (`socketId`, REST's requesting tab, is + * left out: it changed its bell itself), so their bell and count follow. + */ + async markRead(notificationId: number, userId: number, socketId: string | undefined): Promise { + return this.setReadState(notificationId, userId, 1, socketId); } - async markUnread(notificationId: number, userId: number): Promise { - return (await this.notificationsRepo.setRead(notificationId, userId, 0)) > 0; + async markUnread(notificationId: number, userId: number, socketId: string | undefined): Promise { + return this.setReadState(notificationId, userId, 0, socketId); } - async markAllRead(userId: number): Promise { - return await this.notificationsRepo.markAllRead(userId); + /** Every notification of the user read at once; the other tabs read their list again when anything changed. */ + async markAllRead(userId: number, socketId: string | undefined): Promise { + const count = await this.notificationsRepo.markAllRead(userId); + if (count > 0) this.realtime.broadcastToUser(userId, { type: 'notification:read-all' }, socketId); + return count; + } + + private async setReadState(id: number, userId: number, read: 0 | 1, socketId: string | undefined) { + if ((await this.notificationsRepo.setRead(id, userId, read)) === 0) return false; + const row = await this.notificationsRepo.findWithSenderById(id); + if (row) { + this.realtime.broadcastToUser(userId, { type: 'notification:updated', notification: withSender(row) }, socketId); + } + return true; + } + + /** The address a notification names its actor by, as the routes read it off their session user. */ + async actorEmail(userId: number): Promise { + return (await this.mailer.getUserEmail(userId)) ?? ''; } async deleteOne(notificationId: number, userId: number): Promise { @@ -691,14 +716,7 @@ export class NotificationsService { return { success: false, error: err instanceof Error ? err.message : 'Action failed' }; } - const updated = (await this.notificationsRepo.findWithSenderById(notificationId))!; - - // Same unchecked-narrowing boundary as `listInApp`'s `mapped` above. - const mappedUpdated = { - ...updated, - created_at: toUtcIso(updated.created_at!), - sender_avatar: avatarUrl({ avatar: updated.sender_avatar }), - } as NotificationRow; + const mappedUpdated = withSender((await this.notificationsRepo.findWithSenderById(notificationId))!); this.realtime.broadcastToUser(userId, { type: 'notification:updated', notification: mappedUpdated }); diff --git a/server/src/nest/settings/settings-writes.service.ts b/server/src/nest/settings/settings-writes.service.ts new file mode 100644 index 000000000..507bf0c03 --- /dev/null +++ b/server/src/nest/settings/settings-writes.service.ts @@ -0,0 +1,41 @@ +import { RealtimeService } from '../realtime/realtime.service'; +import { SettingsService } from './settings.service'; +import { Injectable } from '@nestjs/common'; +import { MASKED_SETTING_VALUE } from '@trek/shared'; + +/** + * A user's own settings saved, for every surface, and their other tabs and + * devices told which keys changed so they read their settings again. The event + * names keys only, never a value: some keys hold secrets, and the receiving tab + * reads the values through its own session anyway. + * + * `socketId` is REST's requesting tab, which applied the change itself and is + * left out. MCP passes none, so the asking tab follows a change the assistant made. + */ +@Injectable() +export class SettingsWritesService { + constructor( + private readonly settings: SettingsService, + private readonly realtime: RealtimeService, + ) {} + + async saveOne(userId: number, key: string, value: unknown, socketId: string | undefined): Promise { + await this.settings.upsertSetting(userId, key, value); + this.announce(userId, [key], socketId); + } + + /** Answers how many rows were written. A masked secret echoed back is skipped, and named in no event. */ + async saveMany(userId: number, values: Record, socketId: string | undefined): Promise { + const written = await this.settings.bulkUpsertSettings(userId, values); + this.announce( + userId, + Object.keys(values).filter((key) => values[key] !== MASKED_SETTING_VALUE), + socketId, + ); + return written; + } + + private announce(userId: number, keys: string[], socketId: string | undefined): void { + if (keys.length > 0) this.realtime.broadcastToUser(userId, { type: 'settings:updated', keys }, socketId); + } +} diff --git a/server/src/nest/settings/settings.controller.ts b/server/src/nest/settings/settings.controller.ts index d2977717a..30373503b 100644 --- a/server/src/nest/settings/settings.controller.ts +++ b/server/src/nest/settings/settings.controller.ts @@ -7,9 +7,10 @@ import { CurrentUser } from '../auth-core/current-user.decorator'; import { JwtAuthGuard } from '../auth-core/jwt-auth.guard'; import { isManagedLockedKey, splitManagedKeys } from '../common/managed'; import { ResponseContract } from '../common/response-contract'; +import { SettingsWritesService } from './settings-writes.service'; import { AdminDefaultUserSettingsDto, SettingUpsertDto, SettingsBulkDto } from './settings.dto'; import { SettingsService, isAdminOnlyEndpointSetting } from './settings.service'; -import { Body, Controller, Get, HttpCode, HttpException, Post, Put, Req, UseGuards } from '@nestjs/common'; +import { Body, Controller, Get, Headers, HttpCode, HttpException, Post, Put, Req, UseGuards } from '@nestjs/common'; import { MASKED_SETTING_VALUE, PLANNER_TRIP_VIEWS_KEY, @@ -40,6 +41,8 @@ export class SettingsController { constructor( private readonly settings: SettingsService, private readonly env: RuntimeEnvService, + // The writes, which tell the user's other tabs which keys changed. + private readonly writes: SettingsWritesService, ) {} /** @@ -87,7 +90,7 @@ export class SettingsController { @Put() @ResponseContract(settingUpsertResponseSchema) - async upsert(@CurrentUser() user: User, @Body() body: SettingUpsertDto) { + async upsert(@CurrentUser() user: User, @Body() body: SettingUpsertDto, @Headers('x-socket-id') socketId?: string) { this.assertMayWriteInstanceEndpoint({ [body.key]: body.value }); this.assertValidValues({ [body.key]: body.value }); // assertMayWriteInstanceEndpoint only covers llm_base_url and provider 'local'. @@ -100,19 +103,19 @@ export class SettingsController { if (body.value === MASKED_SETTING_VALUE) { return { success: true, key: body.key, unchanged: true }; } - await this.settings.upsertSetting(user.id, body.key, body.value); + await this.writes.saveOne(user.id, body.key, body.value, socketId); return { success: true, key: body.key, value: body.value }; } @Post('bulk') @HttpCode(200) // Express answers bulk with res.json (200), not the POST-default 201. @ResponseContract(settingsBulkResponseSchema) - async bulk(@CurrentUser() user: User, @Body() body: SettingsBulkDto) { + async bulk(@CurrentUser() user: User, @Body() body: SettingsBulkDto, @Headers('x-socket-id') socketId?: string) { this.assertMayWriteInstanceEndpoint(body.settings); this.assertValidValues(body.settings); const { allowed, blocked } = splitManagedKeys(body.settings, this.env.isManaged()); try { - const updated = await this.settings.bulkUpsertSettings(user.id, allowed); + const updated = await this.writes.saveMany(user.id, allowed, socketId); return { success: true, updated, ...(blocked.length ? { managed_keys: blocked } : {}) }; } catch (err) { console.error('Error saving settings:', err); diff --git a/server/src/nest/settings/settings.mcp.ts b/server/src/nest/settings/settings.mcp.ts index 84c2260e3..46f93053e 100644 --- a/server/src/nest/settings/settings.mcp.ts +++ b/server/src/nest/settings/settings.mcp.ts @@ -7,6 +7,7 @@ import { errorResult, ok, } from '../../nest-mcp'; +import { SettingsWritesService } from './settings-writes.service'; import { SettingsService } from './settings.service'; import { MASKED_SETTING_VALUE, @@ -110,7 +111,10 @@ const KEY_LIST = DISPLAY_PREFERENCE_KEYS.join(', '); */ @McpController() export class SettingsMcp { - constructor(private readonly settings: SettingsService) {} + constructor( + private readonly settings: SettingsService, + private readonly writes: SettingsWritesService, + ) {} /** * getUserSettings resolves admin defaults and the managed-install token @@ -182,7 +186,8 @@ export class SettingsMcp { validated[key] = parsed.data; } - const updated = await this.settings.bulkUpsertSettings(ctx.userId, validated); + // Every tab of the user, the asking one included, reads its settings again. + const updated = await this.writes.saveMany(ctx.userId, validated, undefined); // Read back rather than echoing the input: an admin default or the managed // token injection can still shape what the user ends up seeing. return ok({ success: true, updated, settings: await this.readDisplayPreferences(ctx.userId) }); diff --git a/server/src/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index bc5ac09a8..b4138e784 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -2,7 +2,9 @@ import { AppSettings } from '../../db/entities/AppSettings.entity'; import { Settings } from '../../db/entities/Settings.entity'; import { AppConfigModule } from '../app-config/app-config.module'; import { AuditModule } from '../audit/audit.module'; +import { RealtimeModule } from '../realtime/realtime.module'; import { TripMembershipModule } from '../trip-membership/trip-membership.module'; +import { SettingsWritesService } from './settings-writes.service'; import { AdminDefaultUserSettingsController, SettingsController } from './settings.controller'; import { SettingsMcp } from './settings.mcp'; import { SettingsService } from './settings.service'; @@ -26,9 +28,16 @@ import { Module } from '@nestjs/common'; // `@InjectRepository(Users)`. TripMembershipModule is here for the planner // view row (#2639), which keeps only trips the user can still open: the // trips table belongs to another domain, so the ids come from its service. - imports: [AppConfigModule, AuditModule, TripMembershipModule, MikroOrmModule.forFeature([AppSettings, Settings])], + // RealtimeModule for SettingsWritesService, which tells a user's other tabs which keys changed. + imports: [ + AppConfigModule, + AuditModule, + TripMembershipModule, + RealtimeModule, + MikroOrmModule.forFeature([AppSettings, Settings]), + ], controllers: [SettingsController, AdminDefaultUserSettingsController], - providers: [SettingsService, SettingsMcp], + providers: [SettingsService, SettingsWritesService, SettingsMcp], exports: [SettingsService], }) export class SettingsModule {} diff --git a/server/src/nest/tags/tag-writes.service.ts b/server/src/nest/tags/tag-writes.service.ts new file mode 100644 index 000000000..f5856f52c --- /dev/null +++ b/server/src/nest/tags/tag-writes.service.ts @@ -0,0 +1,46 @@ +import { RealtimeService } from '../realtime/realtime.service'; +import { TagsService } from './tags.service'; +import { Injectable } from '@nestjs/common'; +import type { Tag } from '@trek/shared'; + +/** + * A user's tag writes, for REST and MCP alike, each followed by `tags:changed` + * to the user's sockets so every tag picker they have open reads the list again. + * A ping rather than the tag: a delete has nothing left to send. + * + * `socketId` is REST's requesting tab, which updated its own list and is left + * out; MCP passes none. The callers keep their ownership checks. + */ +@Injectable() +export class TagWritesService { + constructor( + private readonly tags: TagsService, + private readonly realtime: RealtimeService, + ) {} + + async create(userId: number, name: string, color: string | undefined, socketId: string | undefined): Promise { + const tag = await this.tags.create(userId, name, color); + this.announce(userId, socketId); + return tag; + } + + async update( + userId: number, + id: string | number, + changes: { name?: string; color?: string }, + socketId: string | undefined, + ): Promise { + const tag = await this.tags.update(id, changes.name, changes.color); + this.announce(userId, socketId); + return tag; + } + + async remove(userId: number, id: string | number, socketId: string | undefined): Promise { + await this.tags.remove(id); + this.announce(userId, socketId); + } + + private announce(userId: number, socketId: string | undefined): void { + this.realtime.broadcastToUser(userId, { type: 'tags:changed' }, socketId); + } +} diff --git a/server/src/nest/tags/tags.controller.ts b/server/src/nest/tags/tags.controller.ts index 73fb1194a..1adfaf0f0 100644 --- a/server/src/nest/tags/tags.controller.ts +++ b/server/src/nest/tags/tags.controller.ts @@ -1,9 +1,10 @@ import type { User } from '../../types'; import { CurrentUser } from '../auth-core/current-user.decorator'; import { JwtAuthGuard } from '../auth-core/jwt-auth.guard'; +import { TagWritesService } from './tag-writes.service'; import { TagCreateDto, TagUpdateDto } from './tags.dto'; import { TagsService } from './tags.service'; -import { Body, Controller, Delete, Get, HttpException, Param, Post, Put, UseGuards } from '@nestjs/common'; +import { Body, Controller, Delete, Get, Headers, HttpException, Param, Post, Put, UseGuards } from '@nestjs/common'; import type { Tag, TagListResponse } from '@trek/shared'; /** @@ -18,7 +19,11 @@ import type { Tag, TagListResponse } from '@trek/shared'; @Controller('api/tags') @UseGuards(JwtAuthGuard) export class TagsController { - constructor(private readonly tags: TagsService) {} + constructor( + private readonly tags: TagsService, + // The writes, which tell the user's other tabs that their tags changed. + private readonly writes: TagWritesService, + ) {} @Get() async list(@CurrentUser() user: User): Promise { @@ -26,7 +31,11 @@ export class TagsController { } @Post() - async create(@CurrentUser() user: User, @Body() body: TagCreateDto): Promise<{ tag: Tag }> { + async create( + @CurrentUser() user: User, + @Body() body: TagCreateDto, + @Headers('x-socket-id') socketId?: string, + ): Promise<{ tag: Tag }> { // Cast rather than validate: the legacy route bound whatever arrived // straight into SQLite, and a contract that rejected a numeric name here // would be a new 400 for anything already sending one. @@ -34,24 +43,33 @@ export class TagsController { if (!name) { throw new HttpException({ error: 'Tag name is required' }, 400); } - return { tag: await this.tags.create(user.id, name, color) }; + return { tag: await this.writes.create(user.id, name, color, socketId) }; } @Put(':id') - async update(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TagUpdateDto): Promise<{ tag: Tag }> { + async update( + @CurrentUser() user: User, + @Param('id') id: string, + @Body() body: TagUpdateDto, + @Headers('x-socket-id') socketId?: string, + ): Promise<{ tag: Tag }> { const { name, color } = body as { name?: string; color?: string }; if (!(await this.tags.getByIdAndUser(id, user.id))) { throw new HttpException({ error: 'Tag not found' }, 404); } - return { tag: await this.tags.update(id, name, color) }; + return { tag: await this.writes.update(user.id, id, { name, color }, socketId) }; } @Delete(':id') - async remove(@CurrentUser() user: User, @Param('id') id: string): Promise<{ success: boolean }> { + async remove( + @CurrentUser() user: User, + @Param('id') id: string, + @Headers('x-socket-id') socketId?: string, + ): Promise<{ success: boolean }> { if (!(await this.tags.getByIdAndUser(id, user.id))) { throw new HttpException({ error: 'Tag not found' }, 404); } - await this.tags.remove(id); + await this.writes.remove(user.id, id, socketId); return { success: true }; } } diff --git a/server/src/nest/tags/tags.mcp.ts b/server/src/nest/tags/tags.mcp.ts index d5363a747..ae3615b20 100644 --- a/server/src/nest/tags/tags.mcp.ts +++ b/server/src/nest/tags/tags.mcp.ts @@ -9,6 +9,7 @@ import { errorResult, ok, } from '../../nest-mcp'; +import { TagWritesService } from './tag-writes.service'; import { TagsService } from './tags.service'; import { idSchema } from '@trek/shared'; @@ -22,7 +23,11 @@ import { z } from 'zod'; */ @McpController() export class TagsMcp { - constructor(private readonly tags: TagsService) {} + // The writes go through TagWritesService with no socket to leave out: every tab of the caller reads its tags again. + constructor( + private readonly tags: TagsService, + private readonly writes: TagWritesService, + ) {} @Tool({ name: 'list_tags', @@ -47,7 +52,7 @@ export class TagsMcp { access: { group: 'places', mode: 'write' }, }) async createTag({ name, color }: { name: string; color?: string }, ctx: McpContext) { - const tag = await this.tags.create(ctx.userId, name, color); + const tag = await this.writes.create(ctx.userId, name, color, undefined); return ok({ tag }); } @@ -64,8 +69,8 @@ export class TagsMcp { }) async updateTag({ tagId, name, color }: { tagId: number; name?: string; color?: string }, ctx: McpContext) { if (!(await this.tags.getByIdAndUser(tagId, ctx.userId))) return errorResult('Tag not found.'); - const tag = await this.tags.update(tagId, name, color); - if (!(await tag)) return errorResult('Tag not found.'); + const tag = await this.writes.update(ctx.userId, tagId, { name, color }, undefined); + if (!tag) return errorResult('Tag not found.'); return ok({ tag }); } @@ -80,7 +85,7 @@ export class TagsMcp { }) async deleteTag({ tagId }: { tagId: number }, ctx: McpContext) { if (!(await this.tags.getByIdAndUser(tagId, ctx.userId))) return errorResult('Tag not found.'); - await this.tags.remove(tagId); + await this.writes.remove(ctx.userId, tagId, undefined); return ok({ success: true }); } } diff --git a/server/src/nest/tags/tags.module.ts b/server/src/nest/tags/tags.module.ts index 1ee1501e4..439023120 100644 --- a/server/src/nest/tags/tags.module.ts +++ b/server/src/nest/tags/tags.module.ts @@ -1,4 +1,6 @@ import { Tags } from '../../db/entities/Tags.entity'; +import { RealtimeModule } from '../realtime/realtime.module'; +import { TagWritesService } from './tag-writes.service'; import { TagsController } from './tags.controller'; import { TagsMcp } from './tags.mcp'; import { TagsRpc } from './tags.rpc'; @@ -15,12 +17,13 @@ import { Module } from '@nestjs/common'; * precedent comment). */ @Module({ - imports: [MikroOrmModule.forFeature([Tags])], + // RealtimeModule for TagWritesService, which tells a user's other tabs that their tags changed. + imports: [RealtimeModule, MikroOrmModule.forFeature([Tags])], controllers: [TagsController], // TagsRpc must stay in providers: the plugin RPC registry discovers marked // PROVIDERS only, and a missing entry here would leave tags.* answering // PERMISSION_DENIED with no other symptom. - providers: [TagsService, TagsMcp, TagsRpc], + providers: [TagsService, TagWritesService, TagsMcp, TagsRpc], // For in-container consumers (TagsRpc). exports: [TagsService], }) diff --git a/server/src/nest/trip-members/trip-member-writes.service.ts b/server/src/nest/trip-members/trip-member-writes.service.ts new file mode 100644 index 000000000..58a735728 --- /dev/null +++ b/server/src/nest/trip-members/trip-member-writes.service.ts @@ -0,0 +1,88 @@ +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; +import type { User } from '../../types'; +import { BudgetService } from '../budget/budget.service'; +import { restTripWriter, type TripWriter } from '../common/trip-writer'; +import { RealtimeService } from '../realtime/realtime.service'; +import { TripMembersService } from './trip-members.service'; +import { InjectRepository } from '@mikro-orm/nestjs'; +import { Injectable } from '@nestjs/common'; + +/** + * The roster writes of a trip, one method per use case, for REST and MCP alike. + * + * Each runs the write through TripMembersService, whose methods stay silent + * building blocks, and sends what the room needs once it is committed. Before + * this only the MCP tools announced a member coming or going, and only REST sent + * the invitation, so a tab never learnt of a member added in the planner, and a + * person added through MCP was never told. + * + * The callers keep their own gates (trip access, member_manage, owner only), + * which already agreed. + */ +@Injectable() +export class TripMemberWritesService { + constructor( + private readonly roster: TripMembersService, + private readonly budget: BudgetService, + private readonly realtime: RealtimeService, + @InjectRepository(Users) private readonly users: UsersRepository, + ) {} + + /** REST's writer for a roster change: the room minus the tab that sent the request. */ + restWriter(tripId: string | number, user: Pick, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** Puts an account on the trip: the room gets the member, the person an invitation. */ + async addMember(tripId: string | number, identifier: string, tripOwnerId: number, writer: TripWriter) { + const result = await this.roster.addMember(tripId, identifier, tripOwnerId, writer.userId); + writer.events.emit('member:added', { member: result.member }); + // The invitation names who sent it by address, as the route always did with its session user. + const actor = { id: writer.userId, email: (await this.users.getEmail(writer.userId)) ?? '' }; + this.roster.notifyInvite(String(tripId), actor, result.targetUserId, result.tripTitle, result.member.email); + return result; + } + + /** Takes somebody off the trip, the writer themselves included when they leave. */ + async removeMember(tripId: number, targetUserId: number, writer: TripWriter): Promise { + await this.roster.removeMember(tripId, targetUserId); + writer.events.emit('member:removed', { userId: targetUserId }); + } + + /** A companion without an account. No invitation: a guest has no inbox. */ + async createGuest(tripId: string | number, name: string, writer: TripWriter) { + const created = await this.roster.createGuest(tripId, name, writer.userId); + writer.events.emit('member:added', { member: created.member }); + return created; + } + + /** False for somebody who is no guest of this trip. The event carries the id and the new name, never the row. */ + async renameGuest(tripId: string | number, guestId: number, name: string, writer: TripWriter): Promise { + if (!(await this.roster.renameGuest(tripId, guestId, name))) return false; + writer.events.emit('member:updated', { member: { id: guestId, username: name.trim() } }); + return true; + } + + /** + * False for somebody who is no guest of this trip. Deleting a guest hands their + * share of every expense they were part of to the rest of its split, and clears + * them as the legacy payer of any expense, so each of those expenses goes out + * again after the member, as it now reads. + */ + async deleteGuest(tripId: string | number, guestId: number, writer: TripWriter): Promise { + // Read first: the rows that name the guest go with the guest. + const shared = (await this.budget.listBudgetItems(tripId)) + .filter( + (item) => item.paid_by_user_id === guestId || item.members?.some((member) => member.user_id === guestId), + ) + .map((item) => item.id); + if (!(await this.roster.deleteGuest(tripId, guestId))) return false; + writer.events.emit('member:removed', { userId: guestId }); + const resplit = await Promise.all(shared.map((itemId) => this.budget.getBudgetItem(itemId, tripId))); + for (const item of resplit) { + if (item) writer.events.emit('budget:updated', { item }); + } + return true; + } +} diff --git a/server/src/nest/trip-members/trip-members.controller.ts b/server/src/nest/trip-members/trip-members.controller.ts index f09598516..78e19126a 100644 --- a/server/src/nest/trip-members/trip-members.controller.ts +++ b/server/src/nest/trip-members/trip-members.controller.ts @@ -7,6 +7,7 @@ import { NotFoundError, ValidationError } from '../common/domain-errors'; import { toRowId } from '../common/row-id'; import { RequireTripOwner, TripOwnerGuard } from '../permissions/trip-owner.guard'; import { TripAddMemberDto, TripTransferOwnershipDto, TripCreateGuestDto, TripRenameGuestDto } from '../trips/trips.dto'; +import { TripMemberWritesService } from './trip-member-writes.service'; import { TripMembersService } from './trip-members.service'; import { Body, @@ -49,6 +50,8 @@ export class TripMembersController { constructor( private readonly roster: TripMembersService, private readonly audit: AuditService, + // The roster changes, which announce themselves to the room the way the MCP tools do. + private readonly writes: TripMemberWritesService, ) {} @Get(':id/members') @@ -63,7 +66,12 @@ export class TripMembersController { @Post(':id/members') @HttpCode(201) - async addMember(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripAddMemberDto) { + async addMember( + @CurrentUser() user: User, + @Param('id') id: string, + @Body() body: TripAddMemberDto, + @Headers('x-socket-id') socketId?: string, + ) { const { identifier } = body; const access = await this.roster.canAccessTrip(id, user.id); if (!access) { @@ -73,8 +81,8 @@ export class TripMembersController { throw new HttpException({ error: 'No permission to manage members' }, 403); } try { - const result = await this.roster.addMember(id, identifier, access.user_id, user.id); - this.roster.notifyInvite(id, user, result.targetUserId, result.tripTitle, result.member.email); + const writer = this.writes.restWriter(id, user, socketId); + const result = await this.writes.addMember(id, identifier, access.user_id, writer); return { member: result.member }; } catch (e: unknown) { if (e instanceof NotFoundError) throw new HttpException({ error: e.message }, 404); @@ -84,7 +92,12 @@ export class TripMembersController { } @Delete(':id/members/:userId') - async removeMember(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { + async removeMember( + @CurrentUser() user: User, + @Param('id') id: string, + @Param('userId') userId: string, + @Headers('x-socket-id') socketId?: string, + ) { // Plan 4 Task 8a — parsed ONCE here (toRowId, not Number(): rule 15's // NaN-into-SQL trap), and the parsed number is what both the access // check and the removal itself use below — this route has no class-level @@ -105,7 +118,7 @@ export class TripMembersController { ) { throw new HttpException({ error: 'No permission to remove members' }, 403); } - await this.roster.removeMember(tripId, targetId); + await this.writes.removeMember(tripId, targetId, this.writes.restWriter(tripId, user, socketId)); return { success: true }; } @@ -144,12 +157,16 @@ export class TripMembersController { @HttpCode(201) @UseGuards(TripOwnerGuard) @RequireTripOwner('Only the owner can manage guests', { param: 'id' }) - async createGuest(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripCreateGuestDto) { + async createGuest( + @CurrentUser() user: User, + @Param('id') id: string, + @Body() body: TripCreateGuestDto, + @Headers('x-socket-id') socketId?: string, + ) { // Whitespace-only names still 400 with the legacy body — the service throws // ValidationError('Guest name is required') after trimming. try { - // No notifyInvite: a guest has no inbox. - return await this.roster.createGuest(id, body.name, user.id); + return await this.writes.createGuest(id, body.name, this.writes.restWriter(id, user, socketId)); } catch (e: unknown) { if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); throw e; @@ -164,9 +181,11 @@ export class TripMembersController { @Param('id') id: string, @Param('userId') userId: string, @Body() body: TripRenameGuestDto, + @Headers('x-socket-id') socketId?: string, ) { try { - if (!(await this.roster.renameGuest(id, Number.parseInt(userId), body.name))) { + const writer = this.writes.restWriter(id, user, socketId); + if (!(await this.writes.renameGuest(id, Number.parseInt(userId), body.name, writer))) { throw new HttpException({ error: 'Guest not found' }, 404); } return { success: true }; @@ -180,8 +199,14 @@ export class TripMembersController { @Delete(':id/guests/:userId') @UseGuards(TripOwnerGuard) @RequireTripOwner('Only the owner can manage guests', { param: 'id' }) - async deleteGuest(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { - if (!(await this.roster.deleteGuest(id, Number.parseInt(userId)))) { + async deleteGuest( + @CurrentUser() user: User, + @Param('id') id: string, + @Param('userId') userId: string, + @Headers('x-socket-id') socketId?: string, + ) { + const writer = this.writes.restWriter(id, user, socketId); + if (!(await this.writes.deleteGuest(id, Number.parseInt(userId), writer))) { throw new HttpException({ error: 'Guest not found' }, 404); } return { success: true }; diff --git a/server/src/nest/trip-members/trip-members.module.ts b/server/src/nest/trip-members/trip-members.module.ts index 0d80157a9..02a5d7b05 100644 --- a/server/src/nest/trip-members/trip-members.module.ts +++ b/server/src/nest/trip-members/trip-members.module.ts @@ -7,6 +7,7 @@ import { BudgetModule } from '../budget/budget.module'; import { NotificationsModule } from '../notifications/notifications.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { RealtimeModule } from '../realtime/realtime.module'; +import { TripMemberWritesService } from './trip-member-writes.service'; import { TripMembersController } from './trip-members.controller'; import { TripMembersService } from './trip-members.service'; import { MikroOrmModule } from '@mikro-orm/nestjs'; @@ -37,7 +38,8 @@ import { Module } from '@nestjs/common'; MikroOrmModule.forFeature([Trips, TripMembers, Users]), ], controllers: [TripMembersController], - providers: [TripMembersService], - exports: [TripMembersService], + providers: [TripMembersService, TripMemberWritesService], + // The writes for TripsMcp, whose roster tools announce through the same use cases as these routes. + exports: [TripMembersService, TripMemberWritesService], }) export class TripMembersModule {} diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 8f417bece..e380a49a0 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -118,7 +118,13 @@ export class TripMembersService { } /** Fire-and-forget trip-invite notification (mirrors the route's dynamic import). */ - notifyInvite(tripId: string, actor: User, targetUserId: number, tripTitle: string, inviteeEmail: string): void { + notifyInvite( + tripId: string, + actor: Pick, + targetUserId: number, + tripTitle: string, + inviteeEmail: string, + ): void { // Injected, not a lazy import of the old notifications bridge. The laziness bought // nothing the module graph does not already give — NotificationsModule // reaches nothing in this direction — and it hid the edge while handing the @@ -151,6 +157,13 @@ export class TripMembersService { }; } + /** One member as the roster lists them, for a write that joined them by id (the plugin RPC). */ + async findMember(tripId: number, userId: number) { + const ownerId = await this.tripsRepo.getOwnerId(tripId); + if (ownerId === null) return undefined; + return (await this.listMembers(tripId, ownerId)).members.find((m) => m.id === userId); + } + async addMember( tripId: string | number, identifier: string, diff --git a/server/src/nest/trips/trip-writes.service.ts b/server/src/nest/trips/trip-writes.service.ts new file mode 100644 index 000000000..b33c1f9b6 --- /dev/null +++ b/server/src/nest/trips/trip-writes.service.ts @@ -0,0 +1,84 @@ +import type { User } from '../../types'; +import { DomainError } from '../common/domain-error'; +import { restTripWriter, type TripWriter } from '../common/trip-writer'; +import { PermissionsService } from '../permissions/permissions.service'; +import { RealtimeService } from '../realtime/realtime.service'; +import { TripsService, type DeleteTripInfo } from './trips.service'; +import { Injectable } from '@nestjs/common'; + +/** The fields a new trip takes, as TripsService.create reads them. */ +type TripCreateInput = Parameters[1]; + +/** + * Who adds a trip to their own list. `role` is for the trip_create check; REST's + * `socketId` is the tab that asked, which shows the new trip from its own answer + * and is left out of the ping. MCP has none, so every tab of the user hears it. + */ +export interface TripListWriter { + userId: number; + role: string; + socketId: string | undefined; +} + +/** + * The trip writes whose events every surface sends the same way: a trip created + * or copied tells its owner's other tabs that the trip list changed, and a + * deleted trip tells the trip room. TripsService holds the writes themselves and + * sends nothing; the controller and the MCP tools are adapters over this. + * + * The update broadcast stays with its callers, which already sent it alike. What + * they share for it is the permission list (tripUpdatePermissions in + * trips.helpers.ts). + */ +@Injectable() +export class TripWritesService { + constructor( + private readonly trips: TripsService, + private readonly permissions: PermissionsService, + private readonly realtime: RealtimeService, + ) {} + + /** REST's writer for a trip: the room minus the tab that sent the request. */ + restWriter(tripId: string | number, user: Pick, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** + * Creates a trip with its days. REST checks trip_create before its own date + * checks, so the gate here only ever refuses an MCP caller. + */ + async create(input: TripCreateInput, writer: TripListWriter) { + await this.requireCreate(writer); + const created = await this.trips.create(writer.userId, input); + this.announceList(writer); + return created; + } + + /** Copies a trip the writer can open into a new one they own, and returns its id. */ + async copy(sourceTripId: string | number, title: string | undefined, writer: TripListWriter): Promise { + await this.requireCreate(writer); + const newTripId = await this.trips.copy(sourceTripId, writer.userId, title); + this.announceList(writer); + return newTripId; + } + + /** Deletes a trip and tells everyone in its room, once the delete is committed. */ + async remove(tripId: string | number, writer: TripWriter): Promise { + const info = await this.trips.remove(tripId, writer.userId, writer.role); + writer.events.emit('trip:deleted', { id: info.tripId }); + return info; + } + + private async requireCreate(writer: TripListWriter): Promise { + if (!(await this.permissions.checkPermission('trip_create', writer.role, null, writer.userId, false))) { + throw new DomainError(403, 'No permission to create trips', { + mcpMessage: 'You do not have permission to create trips.', + }); + } + } + + /** A ping, not the trip: the dashboard of every other tab reads its list again. */ + private announceList(writer: TripListWriter): void { + this.realtime.broadcastToUser(writer.userId, { type: 'trips:changed' }, writer.socketId); + } +} diff --git a/server/src/nest/trips/trips.controller.ts b/server/src/nest/trips/trips.controller.ts index ab00cbbb0..75d9f510a 100644 --- a/server/src/nest/trips/trips.controller.ts +++ b/server/src/nest/trips/trips.controller.ts @@ -11,6 +11,7 @@ import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write'; import { StorageService } from '../storage/storage.service'; import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; import { UnsplashService } from '../unsplash/unsplash.service'; +import { TripWritesService } from './trip-writes.service'; import { TripCreateDto, TripUpdateDto, @@ -20,6 +21,7 @@ import { TripCreateGuestDto, TripRenameGuestDto, } from './trips.dto'; +import { tripUpdatePermissions } from './trips.helpers'; import { TripsService } from './trips.service'; import { NotFoundError, ValidationError } from './trips.service'; import { @@ -81,6 +83,7 @@ export class TripsController { private readonly calendar: CalendarService, private readonly readModel: TripReadModelService, private readonly storage: StorageService, + private readonly tripWrites: TripWritesService, ) {} @Get() @@ -120,7 +123,12 @@ export class TripsController { @Post() @HttpCode(201) - async create(@CurrentUser() user: User, @Body() body: TripCreateDto, @Req() req: Request) { + async create( + @CurrentUser() user: User, + @Body() body: TripCreateDto, + @Req() req: Request, + @Headers('x-socket-id') socketId?: string, + ) { if (!(await this.trips.can('trip_create', user.role, null, user.id, false))) { throw new HttpException({ error: 'No permission to create trips' }, 403); } @@ -133,15 +141,10 @@ export class TripsController { } const parsedDayCount = day_count ? Math.min(Math.max(Number(day_count) || 7, 1), MAX_TRIP_DAYS) : undefined; try { - const { trip, tripId, reminderDays } = await this.trips.create(user.id, { - title, - description, - start_date, - end_date, - currency, - reminder_days, - day_count: parsedDayCount, - }); + const { trip, tripId, reminderDays } = await this.tripWrites.create( + { title, description, start_date, end_date, currency, reminder_days, day_count: parsedDayCount }, + { userId: user.id, role: user.role, socketId }, + ); await this.audit.writeAudit({ userId: user.id, action: 'trip.create', @@ -179,25 +182,13 @@ export class TripsController { } const ownerId = access.user_id; const isMember = ownerId !== user.id; - if ( - body.is_archived !== undefined && - !(await this.trips.can('trip_archive', user.role, ownerId, user.id, isMember)) - ) { - throw new HttpException({ error: 'No permission to archive/unarchive this trip' }, 403); - } - if ( - body.cover_image !== undefined && - !(await this.trips.can('trip_cover_upload', user.role, ownerId, user.id, isMember)) - ) { - throw new HttpException({ error: 'No permission to change cover image' }, 403); - } - const editFields = ['title', 'description', 'start_date', 'end_date', 'currency', 'reminder_days', 'day_count']; - if ( - editFields.some((f) => body[f] !== undefined) && - !(await this.trips.can('trip_edit', user.role, ownerId, user.id, isMember)) - ) { - throw new HttpException({ error: 'No permission to edit this trip' }, 403); - } + // A body the caller may only partly write is refused on the first right it lacks, in the list's order. + const needed = tripUpdatePermissions(body); + const granted = await Promise.all( + needed.map(({ action }) => this.trips.can(action, user.role, ownerId, user.id, isMember)), + ); + const lacking = needed.find((_, i) => !granted[i]); + if (lacking) throw new HttpException({ error: lacking.refusal }, 403); // A chosen Unsplash cover arrives as an images.unsplash.com hot-link; download // it into uploads/covers so the cover survives offline + CDN link-rot (#1277). if (this.unsplash.isUnsplashCoverUrl(body.cover_image)) { @@ -282,7 +273,13 @@ export class TripsController { @Post(':id/copy') @HttpCode(201) - async copy(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripCopyDto, @Req() req: Request) { + async copy( + @CurrentUser() user: User, + @Param('id') id: string, + @Body() body: TripCopyDto, + @Req() req: Request, + @Headers('x-socket-id') socketId?: string, + ) { if (!(await this.trips.can('trip_create', user.role, null, user.id, false))) { throw new HttpException({ error: 'No permission to create trips' }, 403); } @@ -291,7 +288,7 @@ export class TripsController { } const { title } = body; try { - const newTripId = await this.trips.copy(id, user.id, title); + const newTripId = await this.tripWrites.copy(id, title, { userId: user.id, role: user.role, socketId }); await this.audit.writeAudit({ userId: user.id, action: 'trip.copy', @@ -325,7 +322,7 @@ export class TripsController { if (!(await this.trips.can('trip_delete', user.role, owner.user_id, user.id, owner.user_id !== user.id))) { throw new HttpException({ error: 'No permission to delete this trip' }, 403); } - const info = await this.trips.remove(id, user.id, user.role); + const info = await this.tripWrites.remove(id, this.tripWrites.restWriter(id, user, socketId)); await this.audit.writeAudit({ userId: user.id, action: 'trip.delete', @@ -334,7 +331,6 @@ export class TripsController { }); if (info.isAdminDelete && info.ownerEmail) logInfo(`Admin ${user.email} deleted trip "${info.title}" owned by ${info.ownerEmail}`); - this.trips.broadcast(String(info.tripId), 'trip:deleted', { id: info.tripId }, socketId); return { success: true }; } diff --git a/server/src/nest/trips/trips.helpers.ts b/server/src/nest/trips/trips.helpers.ts index 2744b98bd..c0b6603b9 100644 --- a/server/src/nest/trips/trips.helpers.ts +++ b/server/src/nest/trips/trips.helpers.ts @@ -1,5 +1,5 @@ import { ValidationError } from '../common/domain-errors'; -import { MAX_TRIP_DAYS, tripSpanDays } from '@trek/shared'; +import { MAX_TRIP_DAYS, tripSpanDays, type PermissionKey } from '@trek/shared'; /** * Pure helpers of the trips domain, moved out of trips.service.ts. Nothing @@ -33,3 +33,26 @@ export function withoutFeedToken(row: T): T { if (row && typeof row === 'object') delete (row as { feed_token?: unknown }).feed_token; return row; } + +/** The fields of a trip update that count as an edit, as against archiving it or changing its cover. */ +const TRIP_EDIT_FIELDS = ['title', 'description', 'start_date', 'end_date', 'currency', 'reminder_days', 'day_count']; + +/** + * The permissions a trip update needs, in the order PUT /api/trips/:id checks + * them, each with the refusal that route answers. Archiving and the cover have + * their own action; any other field present is an edit. update_trip runs the + * same list, so an MCP caller cannot archive a trip or swap its cover on the + * strength of trip_edit alone. + */ +export function tripUpdatePermissions(body: object): { action: PermissionKey; refusal: string }[] { + const sent = (field: string) => (body as Record)[field] !== undefined; + const needed: { action: PermissionKey; refusal: string }[] = []; + if (sent('is_archived')) { + needed.push({ action: 'trip_archive', refusal: 'No permission to archive/unarchive this trip' }); + } + if (sent('cover_image')) { + needed.push({ action: 'trip_cover_upload', refusal: 'No permission to change cover image' }); + } + if (TRIP_EDIT_FIELDS.some(sent)) needed.push({ action: 'trip_edit', refusal: 'No permission to edit this trip' }); + return needed; +} diff --git a/server/src/nest/trips/trips.mcp.ts b/server/src/nest/trips/trips.mcp.ts index 4f9e50305..2fc22dca5 100644 --- a/server/src/nest/trips/trips.mcp.ts +++ b/server/src/nest/trips/trips.mcp.ts @@ -23,8 +23,11 @@ import { DomainError } from '../common/domain-error'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { tripIdPromptArg } from '../mcp-shared/prompt-args'; import { TodoService } from '../todo/todo.service'; +import { TripMemberWritesService } from '../trip-members/trip-member-writes.service'; import { TripMembersService } from '../trip-members/trip-members.service'; import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; +import { TripWritesService, type TripListWriter } from './trip-writes.service'; +import { tripUpdatePermissions } from './trips.helpers'; import { TripsService, NotFoundError, ValidationError } from './trips.service'; import { idSchema, MAX_TRIP_DAYS } from '@trek/shared'; @@ -89,8 +92,19 @@ export class TripsMcp { private readonly readModel: TripReadModelService, private readonly addons: AddonsService, private readonly guards: McpToolGuardsService, + private readonly tripWrites: TripWritesService, + private readonly memberWrites: TripMemberWritesService, ) {} + /** + * The caller adding a trip to their own list. Only the admin role changes what + * the trip_create check answers. No socket to leave out: every tab of the + * caller hears that the list changed. + */ + private async listWriter(userId: number): Promise { + return { userId, role: (await this.guards.isAdminUser(userId)) ? 'admin' : 'user', socketId: undefined }; + } + // --- TRIPS --- @Tool({ @@ -174,15 +188,10 @@ export class TripsMcp { return { content: [{ type: 'text' as const, text: 'End date must be after start date.' }], isError: true }; } try { - const { trip } = await this.trips.create(ctx.userId, { - title, - description, - start_date, - end_date, - currency, - day_count, - reminder_days, - }); + const { trip } = await this.tripWrites.create( + { title, description, start_date, end_date, currency, day_count, reminder_days }, + await this.listWriter(ctx.userId), + ); return ok({ trip }); } catch (err) { if (err instanceof ValidationError) return errorResult(err.message); @@ -276,7 +285,19 @@ export class TripsMcp { ctx: McpContext, ) { if (!(await this.trips.canAccessTrip(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('trip_edit', tripId, ctx.userId))) return permissionDenied(); + // updateTrip reads every field with a `!== undefined` presence sentinel, so a + // dated trip only goes back to dateless once an explicit null reaches it. A flag + // rather than a nullable start_date/end_date: clients that serialise an omitted + // optional as null would otherwise wipe the dates of every trip they touched. + const dates: { start_date?: string | null; end_date?: string | null } = clear_dates + ? { start_date: null, end_date: null } + : { start_date, end_date }; + const fields = { title, description, ...dates, currency, is_archived, cover_image, day_count, reminder_days }; + // The rights PUT /api/trips/:id asks for, field by field: archiving and the cover have their own. + const granted = await Promise.all( + tripUpdatePermissions(fields).map(({ action }) => this.guards.hasTripPermission(action, tripId, ctx.userId)), + ); + if (granted.includes(false)) return permissionDenied(); if (clear_dates && (start_date || end_date)) return errorResult('clear_dates cannot be combined with start_date or end_date.'); if (start_date) { @@ -292,21 +313,16 @@ export class TripsMcp { if (Number.isNaN(d.getTime()) || d.toISOString().slice(0, 10) !== end_date) return { content: [{ type: 'text' as const, text: 'end_date is not a valid calendar date.' }], isError: true }; } - // updateTrip reads every field with a `!== undefined` presence sentinel, so a - // dated trip only goes back to dateless once an explicit null reaches it. A flag - // rather than a nullable start_date/end_date: clients that serialise an omitted - // optional as null would otherwise wipe the dates of every trip they touched. - const dates: { start_date?: string | null; end_date?: string | null } = clear_dates - ? { start_date: null, end_date: null } - : { start_date, end_date }; // update() re-anchors the budget before the trip row moves off the old - // currency (#1543) and then runs the legacy updateTrip core. + // currency (#1543) and then runs the legacy updateTrip core. The caller's own + // role, as REST hands it on, so an admin's edit of a trip they do not own is one. + const { role } = await this.guards.tripWriter(tripId, ctx.userId); try { const { updatedTrip, removedDays } = await this.trips.update( tripId, ctx.userId, - { title, description, ...dates, currency, is_archived, cover_image, day_count, reminder_days, date_shift_mode }, - 'user', + { ...fields, date_shift_mode }, + role, ); this.guards.safeBroadcast(tripId, 'trip:updated', { trip: updatedTrip }); // Only when days went, so the answer to a rename or a longer trip stays as it was. @@ -351,7 +367,8 @@ export class TripsMcp { }) async deleteTrip({ tripId }: { tripId: number }, ctx: McpContext) { if (!(await this.trips.isOwner(tripId, ctx.userId))) return noAccess(); - await this.trips.remove(tripId, ctx.userId, 'user'); + // trip:deleted goes to the room as on REST, so open tabs leave the trip. + await this.tripWrites.remove(tripId, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true, tripId }); } @@ -488,9 +505,10 @@ export class TripsMcp { // in the permission matrix, and POST /api/trips/:id/members has always honoured // that setting (and the admin bypass inside checkPermission) where this did not. if (!(await this.guards.hasTripPermission('member_manage', tripId, ctx.userId))) return permissionDenied(); + const writer = await this.guards.tripWriter(tripId, ctx.userId); try { - const result = await this.members.addMember(tripId, identifier, ownerRow.user_id, ctx.userId); - this.guards.safeBroadcast(tripId, 'member:added', { member: result.member }); + // Invited the way the planner invites: the room hears of the member, the person gets the notification. + const result = await this.memberWrites.addMember(tripId, identifier, ownerRow.user_id, writer); return ok({ member: result.member }); } catch (err) { const msg = @@ -516,8 +534,7 @@ export class TripsMcp { // requirement: the same self-removal bypass DELETE /api/trips/:id/members/:userId has. if (memberId !== ctx.userId && !(await this.guards.hasTripPermission('member_manage', tripId, ctx.userId))) return permissionDenied(); - await this.members.removeMember(tripId, memberId); - this.guards.safeBroadcast(tripId, 'member:removed', { userId: memberId }); + await this.memberWrites.removeMember(tripId, memberId, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } @@ -539,8 +556,7 @@ export class TripsMcp { // owner calling this would get a success that changed nothing. Say so instead. if (ownerRow.user_id === ctx.userId) return errorResult('You own this trip, so you cannot leave it. Hand it to another member first, or delete it.'); - await this.members.removeMember(tripId, ctx.userId); - this.guards.safeBroadcast(tripId, 'member:removed', { userId: ctx.userId }); + await this.memberWrites.removeMember(tripId, ctx.userId, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } @@ -570,10 +586,9 @@ export class TripsMcp { const ownerRow = await this.trips.getOwner(tripId); if (!ownerRow || ownerRow.user_id !== ctx.userId) return { content: [{ type: 'text' as const, text: 'Only the trip owner can manage guests.' }], isError: true }; + const writer = await this.guards.tripWriter(tripId, ctx.userId); try { - // No notifyInvite: a guest has no inbox. - const { member } = await this.members.createGuest(tripId, name, ctx.userId); - this.guards.safeBroadcast(tripId, 'member:added', { member }); + const { member } = await this.memberWrites.createGuest(tripId, name, writer); return ok({ member }); } catch (err) { const msg = err instanceof ValidationError ? err.message : 'Failed to create guest.'; @@ -597,8 +612,9 @@ export class TripsMcp { const ownerRow = await this.trips.getOwner(tripId); if (!ownerRow || ownerRow.user_id !== ctx.userId) return { content: [{ type: 'text' as const, text: 'Only the trip owner can manage guests.' }], isError: true }; + const writer = await this.guards.tripWriter(tripId, ctx.userId); try { - if (!(await this.members.renameGuest(tripId, guestId, name))) + if (!(await this.memberWrites.renameGuest(tripId, guestId, name, writer))) return { content: [{ type: 'text' as const, text: 'Guest not found.' }], isError: true }; } catch (err) { const msg = err instanceof ValidationError ? err.message : 'Failed to rename guest.'; @@ -623,9 +639,9 @@ export class TripsMcp { const ownerRow = await this.trips.getOwner(tripId); if (!ownerRow || ownerRow.user_id !== ctx.userId) return { content: [{ type: 'text' as const, text: 'Only the trip owner can manage guests.' }], isError: true }; - if (!(await this.members.deleteGuest(tripId, guestId))) + // The member goes, then every expense the guest shared, split anew. + if (!(await this.memberWrites.deleteGuest(tripId, guestId, await this.guards.tripWriter(tripId, ctx.userId)))) return { content: [{ type: 'text' as const, text: 'Guest not found.' }], isError: true }; - this.guards.safeBroadcast(tripId, 'member:removed', { userId: guestId }); return ok({ success: true }); } @@ -643,10 +659,12 @@ export class TripsMcp { async copyTrip({ tripId, title }: { tripId: number; title?: string }, ctx: McpContext) { if (!(await this.trips.canAccessTrip(tripId, ctx.userId))) return noAccess(); try { - const newTripId = await this.trips.copy(tripId, ctx.userId, title); + const newTripId = await this.tripWrites.copy(tripId, title, await this.listWriter(ctx.userId)); const newTrip = await this.trips.canAccessTrip(newTripId, ctx.userId); return ok({ trip: { id: newTripId, ...newTrip } }); - } catch { + } catch (err) { + // A refusal (no trip_create) keeps its own words; anything else stays the generic failure. + if (err instanceof DomainError) throw err; return { content: [{ type: 'text' as const, text: 'Failed to copy trip.' }], isError: true }; } } diff --git a/server/src/nest/trips/trips.module.ts b/server/src/nest/trips/trips.module.ts index d160afccd..08c8e9832 100644 --- a/server/src/nest/trips/trips.module.ts +++ b/server/src/nest/trips/trips.module.ts @@ -25,6 +25,7 @@ import { TripReadModelModule } from '../trip-read-model/trip-read-model.module'; import { UnsplashModule } from '../unsplash/unsplash.module'; import { VacayModule } from '../vacay/vacay.module'; import { TripPromptsMcp } from './trip-prompts.mcp'; +import { TripWritesService } from './trip-writes.service'; import { TripsController } from './trips.controller'; import { MAX_COVER_SIZE, TRIP_COVER_FILE_FILTER } from './trips.controller'; import { TripsMcp } from './trips.mcp'; @@ -73,7 +74,7 @@ import { MulterModule } from '@nestjs/platform-express'; SettingsModule, ], controllers: [TripsController], - providers: [TripsService, TripsMcp, TripPromptsMcp, TripsRpc], + providers: [TripsService, TripWritesService, TripsMcp, TripPromptsMcp, TripsRpc], // Exported for FeedsModule (ICS feeds) and PluginsModule (RPC host injection). exports: [TripsService], }) diff --git a/server/src/nest/trips/trips.rpc.ts b/server/src/nest/trips/trips.rpc.ts index 7322d077d..ca40f027f 100644 --- a/server/src/nest/trips/trips.rpc.ts +++ b/server/src/nest/trips/trips.rpc.ts @@ -204,7 +204,14 @@ export class TripsRpc { const target = await this.em.getRepository(Users).findIdAndEmail(targetUserId); if (!target) throw new ForbiddenResource(`no user ${targetUserId}`); // The acting user is recorded as the inviter. - return this.membership.joinTripAsMember(tripId, targetUserId, actor); + const result = await this.membership.joinTripAsMember(tripId, targetUserId, actor); + // The open planners learn of the member as they do from REST and MCP. A + // plugin has no socket of its own, so the whole room hears it. + if (result.joined) { + const member = await this.roster.findMember(tripId, targetUserId); + if (member) this.realtime.broadcast(tripId, 'member:added', { member }); + } + return result; } @PluginMethod('trips.removeMember', { permission: 'db:write:members' }) @@ -218,6 +225,7 @@ export class TripsRpc { const ownerId = await this.em.getRepository(Trips).getOwnerId(tripId); // RP6 — TripsRepository.getOwnerId if (ownerId !== null && ownerId === targetUserId) throw new ForbiddenResource('cannot remove the trip owner'); await this.roster.removeMember(tripId, targetUserId); + this.realtime.broadcast(tripId, 'member:removed', { userId: targetUserId }); return { removed: true }; } } diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index c9910d116..f4df10137 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -142,8 +142,8 @@ export interface GuestMember { * the member rows, and create() defaults the currency to the owner's display * currency instead of the legacy EUR literal. Auth (canAccessTrip), per-field permission checks and * audit logging stay in the controller (1:1 with the legacy route); - * trip:updated / trip:deleted broadcasts stay in the controller too — this - * service emits none. + * trip:updated goes out from its callers, trip:deleted and the trips:changed ping + * from TripWritesService; this service emits none. */ @Injectable() export class TripsService { diff --git a/server/tests/integration/broadcast-parity/collab-trip.cases.ts b/server/tests/integration/broadcast-parity/collab-trip.cases.ts index e7f2e5d9b..66cdaf87a 100644 --- a/server/tests/integration/broadcast-parity/collab-trip.cases.ts +++ b/server/tests/integration/broadcast-parity/collab-trip.cases.ts @@ -1,17 +1,417 @@ +import { DocumentConnections } from '../../../src/db/entities/DocumentConnections.entity'; +import { DocumentProviders } from '../../../src/db/entities/DocumentProviders.entity'; +import { TripDocumentLinks } from '../../../src/db/entities/TripDocumentLinks.entity'; +import type { DocumentProvider, DocumentProviderCapabilities } from '../../../src/nest/doc-sync/document-provider'; +import { DocumentProviderRegistry } from '../../../src/nest/doc-sync/document-provider.registry'; +import { NotificationsService } from '../../../src/nest/notifications/notifications.service'; +import { addBudgetItemMember } from '../../helpers/factories/budget'; +import { linkFile } from '../../helpers/factories/files'; +import { insertRow, updateRows } from '../../helpers/factories/rows'; +import { addTripMember } from '../../helpers/factories/trips'; import type { CaseGroup, Scene } from './types'; +import { MikroORM } from '@mikro-orm/core'; +import type { INestApplication } from '@nestjs/common'; + +import { Readable } from 'node:stream'; +import { vi } from 'vitest'; /** * Working together on a trip: collab, the trip itself and its members, files, * and the account-wide settings, notifications and tags the planner reads. + * + * The collab posts and the invitation also notify fellow travellers + * (notification:new, sent after the write without being awaited). A scene's + * trip has nobody else on it for a post, so nothing is sent there; the + * invitation's send is stubbed, and the notifications are held by the tool + * tests instead (MCP-COLLAB-001/002, MCP-MEMBERS-001). */ const TOKEN_ONLY = 'manages a link or feed token: nothing on any screen follows it live, so there is nothing to send'; const onTrip = (s: Scene) => ({ tripId: s.trip.id }); const noArgs = () => ({}); +/** A route under the scene's trip. */ +const tripPath = (s: Scene, rest = '') => `/api/trips/${s.trip.id}${rest}`; +const orm = (app: INestApplication) => app.get(MikroORM); + +/** Ids a setup made for a scene, by the scene's number. */ +const fileLinkOf = new Map(); +const docLinkOf = new Map(); +const sceneId = (ids: Map, s: Scene) => { + const id = ids.get(s.n); + if (id === undefined) throw new Error(`scene ${s.n} has no row from its setup`); + return id; +}; + +/** The invitation goes out after the answer, so it is stubbed rather than read late. */ +function stubNotifications(app: INestApplication) { + const send = vi.spyOn(app.get(NotificationsService), 'send').mockResolvedValue({ attempted: 0, delivered: 0 }); + return () => send.mockRestore(); +} + +/** + * A document store with one PDF in the bound folder, in place of the provider + * the registry would call over the network. The sync pulls it in, which is + * the run that tells the trip something changed. + */ +const CAPABILITIES: DocumentProviderCapabilities = { + push: 'none', + stableId: true, + remoteTrash: true, + replaceInPlace: true, + contentHashInListing: true, + maxUploadBytes: null, + acceptedMimeTypes: null, + canCreateScope: true, +}; +const PDF = Buffer.from('%PDF-1.4 parity'); +const SCOPE = { scopeKey: 'tag:1', label: 'Parity', remoteRootId: '1', remoteRootPath: '/TREK/parity' }; +const ok = (data: T) => ({ success: true as const, data }); +const documentStore = { + id: 'paperless', + capabilities: () => CAPABILITIES, + probe: async () => ok({ account: 'parity', capabilities: CAPABILITIES }), + listScopes: async () => ok([SCOPE]), + createScope: async () => ok(SCOPE), + resolveScope: async () => ok(SCOPE), + list: async () => + ok({ + documents: [ + { + remoteId: 'r1', + name: 'boarding.pdf', + size: PDF.length, + mimeType: 'application/pdf', + remoteVersion: 'v1', + contentHash: null, + remoteModifiedAt: '2027-05-01T10:00:00Z', + isDeleted: false, + }, + ], + cursor: null, + cursorUnchanged: false, + truncated: false, + }), + // A stream of its own per call: each scene pulls the document once. + fetch: async () => + ok({ body: Readable.from([PDF]), size: PDF.length, mimeType: 'application/pdf', remoteVersion: 'v1' }), + push: async () => ok({ remoteId: 'r-pushed', remoteVersion: 'v1', remoteModifiedAt: null, deduplicated: false }), + rename: async () => ok({ remoteVersion: 'v2' }), + trash: async () => ok(undefined), +} as unknown as DocumentProvider; + +function stubDocumentStore(app: INestApplication) { + const get = vi.spyOn(app.get(DocumentProviderRegistry), 'get').mockReturnValue(documentStore); + return () => get.mockRestore(); +} + +/** The trip bound to that store by its owner, with the provider switched on by the admin. */ +async function bindDocumentStore(s: Scene, app: INestApplication) { + await updateRows(orm(app), DocumentProviders, { id: 'paperless' }, { enabled: 1 }); + const connection = await insertRow(orm(app), DocumentConnections, { + trip: s.trip.id, + provider: 'paperless', + ownerUser: s.owner.id, + base_url: 'https://paperless.example.com', + secrets: null, + settings: '{}', + }); + const link = await insertRow(orm(app), TripDocumentLinks, { + trip: s.trip.id, + connection, + provider_id: 'paperless', + remote_scope_key: `tag:${s.n}`, + remote_root_id: '1', + remote_root_path: '/TREK/parity', + remote_label: 'Parity', + direction: 'both', + delete_policy: 'unlink', + conflict_policy: 'manual', + sync_enabled: 1, + failure_count: 0, + next_attempt_at: null, + createdByRef: s.owner.id, + }); + docLinkOf.set(s.n, link); +} + +const userPing = { user: 'actor' } as const; export const collabTrip: CaseGroup = { name: 'collab-trip', prefix: 'BCP-COLLAB', - cases: [], + cases: [ + // ── Collab ──────────────────────────────────────────────────────────── + { + tool: 'create_collab_note', + args: (s) => ({ tripId: s.trip.id, title: 'Ideas' }), + rest: { method: 'post', path: (s) => tripPath(s, '/collab/notes'), body: () => ({ title: 'Ideas' }) }, + expect: ['collab:note:created'], + }, + { + tool: 'update_collab_note', + needs: ['collabNote'], + args: (s) => ({ tripId: s.trip.id, noteId: s.collabNote.id, title: 'Plans' }), + rest: { + method: 'put', + path: (s) => tripPath(s, `/collab/notes/${s.collabNote.id}`), + body: () => ({ title: 'Plans' }), + }, + expect: ['collab:note:updated'], + }, + { + tool: 'delete_collab_note', + needs: ['collabNote'], + args: (s) => ({ tripId: s.trip.id, noteId: s.collabNote.id }), + rest: { method: 'delete', path: (s) => tripPath(s, `/collab/notes/${s.collabNote.id}`) }, + expect: ['collab:note:deleted'], + }, + { + tool: 'create_collab_poll', + args: (s) => ({ tripId: s.trip.id, question: 'Where to eat?', options: ['Pasta', 'Sushi'] }), + rest: { + method: 'post', + path: (s) => tripPath(s, '/collab/polls'), + body: () => ({ question: 'Where to eat?', options: ['Pasta', 'Sushi'] }), + }, + expect: ['collab:poll:created'], + }, + { + tool: 'vote_collab_poll', + needs: ['poll'], + args: (s) => ({ tripId: s.trip.id, pollId: s.poll.id, optionIndex: 0 }), + rest: { + method: 'post', + path: (s) => tripPath(s, `/collab/polls/${s.poll.id}/vote`), + body: () => ({ option_index: 0 }), + }, + expect: ['collab:poll:voted'], + }, + { + tool: 'close_collab_poll', + needs: ['poll'], + args: (s) => ({ tripId: s.trip.id, pollId: s.poll.id }), + rest: { method: 'put', path: (s) => tripPath(s, `/collab/polls/${s.poll.id}/close`) }, + expect: ['collab:poll:closed'], + }, + { + tool: 'delete_collab_poll', + needs: ['poll'], + args: (s) => ({ tripId: s.trip.id, pollId: s.poll.id }), + rest: { method: 'delete', path: (s) => tripPath(s, `/collab/polls/${s.poll.id}`) }, + expect: ['collab:poll:deleted'], + }, + { + tool: 'send_collab_message', + args: (s) => ({ tripId: s.trip.id, text: 'Dinner at 8?' }), + rest: { method: 'post', path: (s) => tripPath(s, '/collab/messages'), body: () => ({ text: 'Dinner at 8?' }) }, + expect: ['collab:message:created'], + }, + { + tool: 'delete_collab_message', + needs: ['collabMessage'], + args: (s) => ({ tripId: s.trip.id, messageId: s.collabMessage.id }), + rest: { method: 'delete', path: (s) => tripPath(s, `/collab/messages/${s.collabMessage.id}`) }, + expect: ['collab:message:deleted'], + }, + { + tool: 'react_collab_message', + needs: ['collabMessage'], + args: (s) => ({ tripId: s.trip.id, messageId: s.collabMessage.id, emoji: '👍' }), + rest: { + method: 'post', + path: (s) => tripPath(s, `/collab/messages/${s.collabMessage.id}/react`), + body: () => ({ emoji: '👍' }), + }, + expect: ['collab:message:reacted'], + }, + + // ── Files and the document store ────────────────────────────────────── + { + tool: 'upload_trip_file', + args: (s) => ({ tripId: s.trip.id, filename: 'ticket.pdf', content: PDF.toString('base64') }), + twin: 'POST /api/trips/:tripId/files, a multipart upload', + expect: ['file:created'], + }, + { + tool: 'update_trip_file', + needs: ['file'], + args: (s) => ({ tripId: s.trip.id, fileId: s.file.id, description: 'Boarding pass' }), + rest: { + method: 'put', + path: (s) => tripPath(s, `/files/${s.file.id}`), + body: () => ({ description: 'Boarding pass' }), + }, + expect: ['file:updated'], + }, + { + // The file goes out again with every link it now carries, as the file list reads it. + tool: 'link_trip_file', + needs: ['file', 'place'], + args: (s) => ({ tripId: s.trip.id, fileId: s.file.id, place_id: s.place.id }), + rest: { + method: 'post', + path: (s) => tripPath(s, `/files/${s.file.id}/link`), + body: (s) => ({ place_id: s.place.id }), + }, + expect: ['file:updated'], + }, + { + tool: 'unlink_trip_file', + needs: ['file', 'place'], + setup: async (s, app) => { + fileLinkOf.set(s.n, (await linkFile(orm(app), s.file.id, { place: s.place.id })).id); + }, + args: (s) => ({ tripId: s.trip.id, fileId: s.file.id, linkId: sceneId(fileLinkOf, s) }), + rest: { method: 'delete', path: (s) => tripPath(s, `/files/${s.file.id}/link/${sceneId(fileLinkOf, s)}`) }, + expect: ['file:updated'], + }, + { + // Sent by the run itself (DocSyncService.syncLink), the same for the scheduler, + // the route and the tool, to every socket: a pulled document is news to all. + tool: 'sync_trip_documents', + setup: bindDocumentStore, + stub: stubDocumentStore, + args: (s) => ({ tripId: s.trip.id }), + rest: { + method: 'post', + path: (s) => tripPath(s, `/docsync/links/${sceneId(docLinkOf, s)}/sync`), + body: () => ({}), + }, + expect: [ + { event: 'file:created', all: true }, + { event: 'docsync:changed', all: true }, + ], + }, + + // ── The user's own notifications, settings and tags ──────────────────── + { + tool: 'mark_notification_read', + needs: ['notification'], + args: (s) => ({ notificationId: s.notification.id }), + rest: { method: 'put', path: (s) => `/api/notifications/in-app/${s.notification.id}/read` }, + expect: [{ event: 'notification:updated', ...userPing }], + }, + { + tool: 'mark_notification_unread', + needs: ['notification'], + args: (s) => ({ notificationId: s.notification.id }), + rest: { method: 'put', path: (s) => `/api/notifications/in-app/${s.notification.id}/unread` }, + expect: [{ event: 'notification:updated', ...userPing }], + }, + { + tool: 'mark_all_notifications_read', + needs: ['notification'], + args: noArgs, + rest: { method: 'put', path: () => '/api/notifications/in-app/read-all' }, + expect: [{ event: 'notification:read-all', ...userPing }], + }, + { + tool: 'update_display_settings', + args: () => ({ settings: { temperature_unit: 'fahrenheit' } }), + rest: { + method: 'post', + path: () => '/api/settings/bulk', + body: () => ({ settings: { temperature_unit: 'fahrenheit' } }), + }, + expect: [{ event: 'settings:updated', ...userPing }], + }, + { + tool: 'create_tag', + args: () => ({ name: 'Beach' }), + rest: { method: 'post', path: () => '/api/tags', body: () => ({ name: 'Beach' }) }, + expect: [{ event: 'tags:changed', ...userPing }], + }, + { + tool: 'update_tag', + needs: ['tag'], + args: (s) => ({ tagId: s.tag.id, name: 'Coast' }), + rest: { method: 'put', path: (s) => `/api/tags/${s.tag.id}`, body: () => ({ name: 'Coast' }) }, + expect: [{ event: 'tags:changed', ...userPing }], + }, + { + tool: 'delete_tag', + needs: ['tag'], + args: (s) => ({ tagId: s.tag.id }), + rest: { method: 'delete', path: (s) => `/api/tags/${s.tag.id}` }, + expect: [{ event: 'tags:changed', ...userPing }], + }, + + // ── The trip and who is on it ───────────────────────────────────────── + { + // The new trip is the owner's: their other tabs read their trip list again. + tool: 'create_trip', + args: () => ({ title: 'Lisbon' }), + rest: { method: 'post', path: () => '/api/trips', body: () => ({ title: 'Lisbon' }) }, + expect: [{ event: 'trips:changed', ...userPing }], + }, + { + tool: 'copy_trip', + args: onTrip, + rest: { method: 'post', path: (s) => tripPath(s, '/copy'), body: () => ({}) }, + expect: [{ event: 'trips:changed', ...userPing }], + }, + { + tool: 'update_trip', + args: (s) => ({ tripId: s.trip.id, title: 'Lisbon in May' }), + rest: { method: 'put', path: (s) => tripPath(s), body: () => ({ title: 'Lisbon in May' }) }, + expect: ['trip:updated'], + }, + { + tool: 'delete_trip', + args: onTrip, + rest: { method: 'delete', path: (s) => tripPath(s) }, + expect: ['trip:deleted'], + }, + { + // The admin of the scene has an account and is on no trip of it. + tool: 'add_trip_member', + args: (s) => ({ tripId: s.trip.id, identifier: s.admin.username }), + rest: { method: 'post', path: (s) => tripPath(s, '/members'), body: (s) => ({ identifier: s.admin.username }) }, + stub: stubNotifications, + expect: ['member:added'], + }, + { + tool: 'remove_trip_member', + needs: ['member'], + args: (s) => ({ tripId: s.trip.id, memberId: s.member.id }), + rest: { method: 'delete', path: (s) => tripPath(s, `/members/${s.member.id}`) }, + expect: ['member:removed'], + }, + { + // Somebody on the trip who is not its owner leaves it: the admin, put on it first. + tool: 'leave_trip', + actor: 'admin', + setup: async (s, app) => { + await addTripMember(orm(app), s.trip.id, s.admin.id, s.owner.id); + }, + args: onTrip, + rest: { method: 'delete', path: (s) => tripPath(s, `/members/${s.admin.id}`) }, + expect: ['member:removed'], + }, + { + tool: 'create_trip_guest', + args: (s) => ({ tripId: s.trip.id, name: 'Anna' }), + rest: { method: 'post', path: (s) => tripPath(s, '/guests'), body: () => ({ name: 'Anna' }) }, + expect: ['member:added'], + }, + { + tool: 'rename_trip_guest', + needs: ['guest'], + args: (s) => ({ tripId: s.trip.id, guestId: s.guest.id, name: 'Anna B.' }), + rest: { method: 'put', path: (s) => tripPath(s, `/guests/${s.guest.id}`), body: () => ({ name: 'Anna B.' }) }, + expect: ['member:updated'], + }, + { + // The guest shares the dinner: it goes out again, split among the rest, after the member. + tool: 'delete_trip_guest', + needs: ['guest', 'budgetItem'], + setup: async (s, app) => { + await addBudgetItemMember(orm(app), s.budgetItem.id, s.guest.id); + }, + args: (s) => ({ tripId: s.trip.id, guestId: s.guest.id }), + rest: { method: 'delete', path: (s) => tripPath(s, `/guests/${s.guest.id}`) }, + expect: ['member:removed', 'budget:updated'], + }, + ], exempt: [ { tool: 'create_share_link', reason: TOKEN_ONLY, args: onTrip }, { tool: 'delete_share_link', reason: TOKEN_ONLY, args: onTrip, first: { tool: 'create_share_link', args: onTrip } }, @@ -49,38 +449,4 @@ export const collabTrip: CaseGroup = { first: { tool: 'enable_all_trips_calendar_feed', args: noArgs }, }, ], - pending: [ - 'create_collab_note', - 'update_collab_note', - 'delete_collab_note', - 'create_collab_poll', - 'vote_collab_poll', - 'close_collab_poll', - 'delete_collab_poll', - 'send_collab_message', - 'delete_collab_message', - 'react_collab_message', - 'sync_trip_documents', - 'upload_trip_file', - 'update_trip_file', - 'link_trip_file', - 'unlink_trip_file', - 'mark_notification_read', - 'mark_notification_unread', - 'mark_all_notifications_read', - 'update_display_settings', - 'create_tag', - 'update_tag', - 'delete_tag', - 'create_trip', - 'update_trip', - 'delete_trip', - 'add_trip_member', - 'remove_trip_member', - 'leave_trip', - 'create_trip_guest', - 'rename_trip_guest', - 'delete_trip_guest', - 'copy_trip', - ], }; diff --git a/server/tests/unit/mcp/tools-collab-polls-chat.test.ts b/server/tests/unit/mcp/tools-collab-polls-chat.test.ts index 534f68a31..ca0d715e8 100644 --- a/server/tests/unit/mcp/tools-collab-polls-chat.test.ts +++ b/server/tests/unit/mcp/tools-collab-polls-chat.test.ts @@ -11,11 +11,13 @@ import { db as testDb } from '../../../src/db/database'; import { CollabPollVotes } from '../../../src/db/entities/CollabPollVotes.entity'; import { CollabPolls } from '../../../src/db/entities/CollabPolls.entity'; import { TripMembers } from '../../../src/db/entities/TripMembers.entity'; +import { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { createUser, createTrip } from '../../helpers/factories'; import { makeCollabMessage } from '../../helpers/factories/collab'; import { countRows, findRow, insertRow } from '../../helpers/factories/rows'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, parseResourceResult, type McpHarness } from '../../helpers/mcp-harness'; +import { bootTestApp } from '../../helpers/test-app'; import { resetTestDb } from '../../helpers/test-db'; import { setAddonEnabled } from '../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; @@ -408,7 +410,12 @@ describe('Tool: delete_collab_message', () => { }); const data = parseToolResult(result) as any; expect(data.success).toBe(true); - expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'collab:message:deleted', expect.any(Object)); + // Named like REST names it: the author, who is the only one allowed to delete. + expect(broadcastMock).toHaveBeenCalledWith( + trip.id, + 'collab:message:deleted', + expect.objectContaining({ messageId: Number(msgId), username: user.username }), + ); }); }); @@ -528,3 +535,29 @@ describe('Resource: trek://trips/{tripId}/collab/messages', () => { }); }); }); + +describe('send_collab_message, as the chat posts it', () => { + it('MCP-COLLAB-001: notifies the fellow travellers with the preview a typed message gets, after the room heard it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const send = vi + .spyOn((await bootTestApp(realtime)).get(NotificationsService), 'send') + .mockResolvedValue({ attempted: 0, delivered: 0 }); + try { + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'send_collab_message', arguments: { tripId: trip.id, text: 'Dinner at 8?' } }); + }); + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'collab:message:created', expect.any(Object)); + expect(send).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'collab_message', + actorId: user.id, + targetId: trip.id, + params: expect.objectContaining({ actor: user.email, preview: 'Dinner at 8?' }), + }), + ); + } finally { + send.mockRestore(); + } + }); +}); diff --git a/server/tests/unit/mcp/tools-files.test.ts b/server/tests/unit/mcp/tools-files.test.ts index 70b4668c8..51123dcb4 100644 --- a/server/tests/unit/mcp/tools-files.test.ts +++ b/server/tests/unit/mcp/tools-files.test.ts @@ -829,6 +829,8 @@ describe('Tool: link_trip_file', () => { }); expect(result.isError).toBe(true); expect(JSON.stringify(result.content)).toContain('File not found.'); + // Nothing was linked, so no tab is told about the other trip's file. + expect(broadcastMock).not.toHaveBeenCalledWith(expect.anything(), 'file:updated', expect.anything()); }); }); @@ -992,6 +994,56 @@ describe('Tool: unlink_trip_file', () => { }); }); +describe('Link changes reach every tab', () => { + /** The one file:updated a link change sent, and the file it carried. */ + function sentFile() { + expect(broadcastMock.mock.calls.map((call) => call[1])).toEqual(['file:updated']); + return broadcastMock.mock.calls[0]![2].file as { + id: number; + linked_place_ids: number[]; + linked_reservation_ids: number[]; + }; + } + + it('MCP-FILES-001: link_trip_file sends the file again with every link it now carries', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const reservation = createReservation(testDb, trip.id); + const place = createPlace(testDb, trip.id); + const file = await insertFile(trip.id); + await linkFile(orm, file.id, { reservation: reservation.id }); + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'link_trip_file', + arguments: { tripId: trip.id, fileId: file.id, place_id: place.id }, + }); + }); + expect(sentFile()).toMatchObject({ + id: file.id, + linked_reservation_ids: [reservation.id], + linked_place_ids: [place.id], + }); + }); + + it('MCP-FILES-002: unlink_trip_file sends the file with the links it has left', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const reservation = createReservation(testDb, trip.id); + const place = createPlace(testDb, trip.id); + const file = await insertFile(trip.id); + await linkFile(orm, file.id, { reservation: reservation.id }); + await linkFile(orm, file.id, { place: place.id }); + const doomed = (await linkRows(file.id))[0]!.id; + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'unlink_trip_file', + arguments: { tripId: trip.id, fileId: file.id, linkId: doomed }, + }); + }); + expect(sentFile()).toMatchObject({ id: file.id, linked_reservation_ids: [], linked_place_ids: [place.id] }); + }); +}); + // --------------------------------------------------------------------------- // list_trip_file_links // --------------------------------------------------------------------------- diff --git a/server/tests/unit/mcp/tools-notes.test.ts b/server/tests/unit/mcp/tools-notes.test.ts index 36258783c..b546fbbf1 100644 --- a/server/tests/unit/mcp/tools-notes.test.ts +++ b/server/tests/unit/mcp/tools-notes.test.ts @@ -9,11 +9,13 @@ import { db as testDb } from '../../../src/db/database'; import { CollabNotes } from '../../../src/db/entities/CollabNotes.entity'; import { DayNotes } from '../../../src/db/entities/DayNotes.entity'; import { TripFiles } from '../../../src/db/entities/TripFiles.entity'; +import { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { createUser, createTrip, createDay, createDayNote, createCollabNote } from '../../helpers/factories'; import { makeTripFile } from '../../helpers/factories/files'; import { countRows, findRow, updateRows } from '../../helpers/factories/rows'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, parseResourceResult, type McpHarness } from '../../helpers/mcp-harness'; +import { bootTestApp } from '../../helpers/test-app'; import { resetTestDb } from '../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; @@ -797,3 +799,29 @@ describe('Resource: trek://trips/{tripId}/collab-notes', () => { }); }); }); + +describe('create_collab_note, as the Collab tab posts it', () => { + it('MCP-COLLAB-002: notifies the fellow travellers like the route does, after the room heard of the note', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const send = vi + .spyOn((await bootTestApp(realtime)).get(NotificationsService), 'send') + .mockResolvedValue({ attempted: 0, delivered: 0 }); + try { + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'create_collab_note', arguments: { tripId: trip.id, title: 'Ideas' } }); + }); + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'collab:note:created', expect.any(Object)); + expect(send).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'collab_message', + actorId: user.id, + targetId: trip.id, + params: expect.objectContaining({ actor: user.email }), + }), + ); + } finally { + send.mockRestore(); + } + }); +}); diff --git a/server/tests/unit/mcp/tools-notifications.test.ts b/server/tests/unit/mcp/tools-notifications.test.ts index 5a006d87f..2e393f8d7 100644 --- a/server/tests/unit/mcp/tools-notifications.test.ts +++ b/server/tests/unit/mcp/tools-notifications.test.ts @@ -11,10 +11,13 @@ */ import { db as testDb } from '../../../src/db/database'; import { Notifications } from '../../../src/db/entities/Notifications.entity'; +import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { createUser } from '../../helpers/factories'; import { makeNotification } from '../../helpers/factories/notifications'; import { countRows, findRow, updateRows } from '../../helpers/factories/rows'; +import type { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, parseResourceResult, type McpHarness } from '../../helpers/mcp-harness'; +import { bootTestApp } from '../../helpers/test-app'; import { resetTestDb } from '../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; @@ -276,3 +279,60 @@ describe('Resource: trek://notifications/in-app', () => { }); }); }); + +describe('Read state reaches every tab of the user', () => { + /** The fake the harness container was booted with, emptied for the case. */ + async function userMessages() { + const realtime = (await bootTestApp()).get(RealtimeService) as FakeRealtimeService; + realtime.broadcastToUserMock.mockClear(); + return realtime.broadcastToUserMock; + } + + it('MCP-NOTIF-001: mark_notification_read and _unread send the row as it now reads, to every tab', async () => { + const { user } = createUser(testDb); + const notif = await createNotification(user.id); + const sent = await userMessages(); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'mark_notification_read', arguments: { notificationId: notif.id } }); + await h.client.callTool({ name: 'mark_notification_unread', arguments: { notificationId: notif.id } }); + }); + expect(sent.mock.calls.map(([to, message, skip]) => [to, message.type, skip])).toEqual([ + [user.id, 'notification:updated', undefined], + [user.id, 'notification:updated', undefined], + ]); + const states = sent.mock.calls.map(([, message]) => message.notification as { id: number; is_read: number }); + expect(states).toEqual([ + expect.objectContaining({ id: notif.id, is_read: 1 }), + expect.objectContaining({ id: notif.id, is_read: 0 }), + ]); + }); + + it('MCP-NOTIF-002: mark_all_notifications_read pings once when something changed, and not for nothing', async () => { + const { user } = createUser(testDb); + await createNotification(user.id); + const sent = await userMessages(); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'mark_all_notifications_read', arguments: {} }); + await h.client.callTool({ name: 'mark_all_notifications_read', arguments: {} }); + }); + expect(sent.mock.calls).toEqual([[user.id, { type: 'notification:read-all' }, undefined]]); + }); + + it("MCP-NOTIF-003: marking somebody else's notification is refused and sends nothing to either user", async () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const notif = await createNotification(owner.id); + const sent = await userMessages(); + await withHarness(stranger.id, async (h) => { + const read = await h.client.callTool({ name: 'mark_notification_read', arguments: { notificationId: notif.id } }); + const unread = await h.client.callTool({ + name: 'mark_notification_unread', + arguments: { notificationId: notif.id }, + }); + expect(read.isError).toBe(true); + expect(unread.isError).toBe(true); + }); + expect(sent).not.toHaveBeenCalled(); + expect((await findRow(orm, Notifications, { id: notif.id }))!.is_read).toBe(0); + }); +}); diff --git a/server/tests/unit/mcp/tools-prompts.test.ts b/server/tests/unit/mcp/tools-prompts.test.ts index de299e403..8b25c7ca3 100644 --- a/server/tests/unit/mcp/tools-prompts.test.ts +++ b/server/tests/unit/mcp/tools-prompts.test.ts @@ -133,6 +133,9 @@ beforeAll(async () => { readModelStub, addonsStub, promptGuards, + // The trip and roster writes: no prompt reaches a write. + undefined as never, + undefined as never, ); promptPackingService = new PackingService( new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), @@ -164,7 +167,8 @@ beforeAll(async () => { new TripMembershipService(await createTestTripsRepo(testDb), await createTestTripMembersRepo(testDb)), addonsStub, promptGuards, - await createTestUnitOfWork(testDb), + // BudgetWritesService: a prompt writes nothing. + {} as never, await createTestPlacesRepo(testDb), await createTestTripsRepo(testDb), ); diff --git a/server/tests/unit/mcp/tools-settings.test.ts b/server/tests/unit/mcp/tools-settings.test.ts index 8c1561b15..3743621ef 100644 --- a/server/tests/unit/mcp/tools-settings.test.ts +++ b/server/tests/unit/mcp/tools-settings.test.ts @@ -14,6 +14,7 @@ import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; import { Settings } from '../../../src/db/entities/Settings.entity'; import { encrypt_api_key } from '../../../src/nest/common/crypto/apiKeyCrypto'; import { MANAGED_LOCKED_SETTING_KEYS } from '../../../src/nest/common/managed'; +import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { DISPLAY_PREFERENCE_KEYS } from '../../../src/nest/settings/settings.mcp'; import { isAdminOnlyEndpointSetting, @@ -23,7 +24,9 @@ import { import { createUser } from '../../helpers/factories'; import { countRows, findRow, insertRow } from '../../helpers/factories/rows'; import { setUserSetting } from '../../helpers/factories/settings'; +import type { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, type McpHarness } from '../../helpers/mcp-harness'; +import { bootTestApp } from '../../helpers/test-app'; import { resetTestDb } from '../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { MASKED_SETTING_VALUE } from '@trek/shared'; @@ -574,3 +577,28 @@ describe('Settings tools, scope gating', () => { expect(names).not.toContain('update_display_settings'); }); }); + +describe('update_display_settings reaches every tab', () => { + it('MCP-SETTINGS-001: tells every tab of the user which keys changed, never their values', async () => { + const { user } = createUser(testDb); + // The fake the harness container was booted with, which records every user message. + const realtime = (await bootTestApp()).get(RealtimeService) as FakeRealtimeService; + realtime.broadcastToUserMock.mockClear(); + await withHarness(user.id, async (h) => { + await update(h, { temperature_unit: 'fahrenheit', week_start: 'sunday' }); + }); + expect(realtime.broadcastToUserMock.mock.calls).toEqual([ + [user.id, { type: 'settings:updated', keys: ['temperature_unit', 'week_start'] }, undefined], + ]); + }); + + it('MCP-SETTINGS-002: a refused change sends nothing', async () => { + const { user } = createUser(testDb); + const realtime = (await bootTestApp()).get(RealtimeService) as FakeRealtimeService; + realtime.broadcastToUserMock.mockClear(); + await withHarness(user.id, async (h) => { + expect((await update(h, { temperature_unit: 'kelvin' })).isError).toBe(true); + }); + expect(realtime.broadcastToUserMock).not.toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts index 39ab0b654..0ec550467 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -273,6 +273,33 @@ describe('Tool: delete_tag', () => { }); }); +describe('Tag writes reach every tab of the user', () => { + it('MCP-TAGS-001: create_tag, update_tag and delete_tag each ping the tag pickers of every tab', async () => { + const { user } = createUser(testDb); + realtime.broadcastToUserMock.mockClear(); + await withHarness(user.id, async (h) => { + const created = parseToolResult( + await h.client.callTool({ name: 'create_tag', arguments: { name: 'Beach' } }), + ) as { tag: { id: number } }; + await h.client.callTool({ name: 'update_tag', arguments: { tagId: created.tag.id, name: 'Coast' } }); + await h.client.callTool({ name: 'delete_tag', arguments: { tagId: created.tag.id } }); + }); + const ping = [user.id, { type: 'tags:changed' }, undefined]; + expect(realtime.broadcastToUserMock.mock.calls).toEqual([ping, ping, ping]); + }); + + it('MCP-TAGS-002: a tag of another user is refused without a ping', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const foreign = await makeTag(orm, other.id); + realtime.broadcastToUserMock.mockClear(); + await withHarness(user.id, async (h) => { + expect((await h.client.callTool({ name: 'delete_tag', arguments: { tagId: foreign.id } })).isError).toBe(true); + }); + expect(realtime.broadcastToUserMock).not.toHaveBeenCalled(); + }); +}); + // --------------------------------------------------------------------------- // tags scope gating (places:read / places:write, registration-time) // --------------------------------------------------------------------------- diff --git a/server/tests/unit/mcp/tools-trip-members.test.ts b/server/tests/unit/mcp/tools-trip-members.test.ts index 17b966513..d563df96d 100644 --- a/server/tests/unit/mcp/tools-trip-members.test.ts +++ b/server/tests/unit/mcp/tools-trip-members.test.ts @@ -5,19 +5,22 @@ * copy_trip, export_trip_ics, get_share_link, create_share_link, delete_share_link. */ import { db as testDb } from '../../../src/db/database'; +import { BudgetItems } from '../../../src/db/entities/BudgetItems.entity'; import { ShareTokens } from '../../../src/db/entities/ShareTokens.entity'; import { TripMembers } from '../../../src/db/entities/TripMembers.entity'; import { Trips } from '../../../src/db/entities/Trips.entity'; import { Users } from '../../../src/db/entities/Users.entity'; import { GUEST_HAS_PAYMENTS } from '../../../src/nest/budget/budget-departure.helpers'; +import { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { invalidatePermissionsCache } from '../../../src/nest/permissions/permissions-cache'; import { createUser, createAdmin, createTrip, addTripMember } from '../../helpers/factories'; -import { addBudgetItemPayer, makeBudgetItem } from '../../helpers/factories/budget'; -import { countRows, findRow } from '../../helpers/factories/rows'; +import { addBudgetItemMember, addBudgetItemPayer, makeBudgetItem } from '../../helpers/factories/budget'; +import { countRows, findRow, updateRows } from '../../helpers/factories/rows'; import { setAppSetting } from '../../helpers/factories/settings'; import { makeShareToken } from '../../helpers/factories/trips'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, type McpHarness } from '../../helpers/mcp-harness'; +import { bootTestApp } from '../../helpers/test-app'; import { resetTestDb } from '../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; @@ -770,6 +773,7 @@ describe('Tool: rename_trip_guest', () => { const otherTrip = createTrip(testDb, owner.id); await withHarness(owner.id, async (h) => { const guestId = await makeGuest(h, otherTrip.id, 'Anna'); + broadcastMock.mockClear(); const result = await h.client.callTool({ name: 'rename_trip_guest', arguments: { tripId: ownTrip.id, guestId, name: 'Renamed' }, @@ -777,6 +781,8 @@ describe('Tool: rename_trip_guest', () => { expect(result.isError).toBe(true); const row = await userRow(guestId); expect(row.display_name).toBe('Anna'); + // A refused rename announces nothing, on either trip. + expect(broadcastMock).not.toHaveBeenCalledWith(expect.anything(), 'member:updated', expect.anything()); }); }); @@ -856,6 +862,7 @@ describe('Tool: delete_trip_guest', () => { expect(JSON.stringify(result.content)).toContain(GUEST_HAS_PAYMENTS); expect(await findRow(orm, Users, { id: guestId })).not.toBeNull(); expect(broadcastMock).not.toHaveBeenCalledWith(trip.id, 'member:removed', expect.anything()); + expect(broadcastMock).not.toHaveBeenCalledWith(trip.id, 'budget:updated', expect.anything()); }); }); @@ -973,3 +980,86 @@ describe('Guest tools stay owner-only', () => { expect(await countRows(orm, Users, { is_guest: 1 })).toBe(1); }); }); + +// --------------------------------------------------------------------------- +// The roster use cases REST shares (TripMemberWritesService): what each tool +// sends besides its member event. +// --------------------------------------------------------------------------- + +describe('Roster writes, as REST makes them', () => { + it('MCP-MEMBERS-001: add_trip_member sends the invitation the planner sends, naming the inviter by address', async () => { + const { user: owner } = createUser(testDb); + const { user: invitee } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const send = vi + .spyOn((await bootTestApp(realtime)).get(NotificationsService), 'send') + .mockResolvedValue({ attempted: 0, delivered: 0 }); + try { + await withHarness(owner.id, async (h) => { + await h.client.callTool({ name: 'add_trip_member', arguments: { tripId: trip.id, identifier: invitee.email } }); + }); + expect(send).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'trip_invite', + actorId: owner.id, + scope: 'user', + targetId: invitee.id, + params: expect.objectContaining({ actor: owner.email, invitee: invitee.email, tripId: String(trip.id) }), + }), + ); + } finally { + send.mockRestore(); + } + }); + + it('MCP-MEMBERS-002: rename_trip_guest tells the room the new name, and nothing else of the row', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await withHarness(owner.id, async (h) => { + const guestId = await makeGuest(h, trip.id, 'Anna'); + broadcastMock.mockClear(); + await h.client.callTool({ + name: 'rename_trip_guest', + arguments: { tripId: trip.id, guestId, name: ' Anna B. ' }, + }); + expect(broadcastMock.mock.calls).toEqual([ + [trip.id, 'member:updated', { member: { id: guestId, username: 'Anna B.' }, _source: 'mcp' }], + ]); + }); + }); + + it('MCP-MEMBERS-003: delete_trip_guest sends each expense the guest shared again, split among the rest, after the member', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await withHarness(owner.id, async (h) => { + const guestId = await makeGuest(h, trip.id, 'Anna'); + const shared = await makeBudgetItem(orm, trip.id, { total_price: 60 }); + await addBudgetItemMember(orm, shared.id, owner.id); + await addBudgetItemMember(orm, shared.id, guestId); + await makeBudgetItem(orm, trip.id, { total_price: 10 }); + broadcastMock.mockClear(); + await h.client.callTool({ name: 'delete_trip_guest', arguments: { tripId: trip.id, guestId } }); + expect(broadcastMock.mock.calls.map((call) => call[1])).toEqual(['member:removed', 'budget:updated']); + const [, , payload] = broadcastMock.mock.calls[1]!; + const item = payload.item as { id: number; members: { user_id: number }[] }; + expect(item.id).toBe(shared.id); + expect(item.members.map((m) => m.user_id)).toEqual([owner.id]); + }); + }); + + it('MCP-MEMBERS-004: delete_trip_guest also sends an expense the guest was only the legacy payer of, cleared', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await withHarness(owner.id, async (h) => { + const guestId = await makeGuest(h, trip.id, 'Anna'); + const legacy = await makeBudgetItem(orm, trip.id, { total_price: 40 }); + await updateRows(orm, BudgetItems, { id: legacy.id }, { paidByUser: guestId }); + broadcastMock.mockClear(); + await h.client.callTool({ name: 'delete_trip_guest', arguments: { tripId: trip.id, guestId } }); + expect(broadcastMock.mock.calls.map((call) => call[1])).toEqual(['member:removed', 'budget:updated']); + const item = broadcastMock.mock.calls[1]![2].item as { id: number; paid_by_user_id: number | null }; + expect(item.id).toBe(legacy.id); + expect(item.paid_by_user_id).toBeNull(); + }); + }); +}); diff --git a/server/tests/unit/mcp/tools-trips.test.ts b/server/tests/unit/mcp/tools-trips.test.ts index 9723a72de..09c8c9b4b 100644 --- a/server/tests/unit/mcp/tools-trips.test.ts +++ b/server/tests/unit/mcp/tools-trips.test.ts @@ -13,6 +13,7 @@ import { Trips } from '../../../src/db/entities/Trips.entity'; import { VacayEntries } from '../../../src/db/entities/VacayEntries.entity'; import { VacayUserYears } from '../../../src/db/entities/VacayUserYears.entity'; import { VacayYears } from '../../../src/db/entities/VacayYears.entity'; +import { invalidatePermissionsCache } from '../../../src/nest/permissions/permissions-cache'; import { createUser, createTrip, @@ -29,7 +30,7 @@ import { } from '../../helpers/factories'; import { makePackingItem } from '../../helpers/factories/packing'; import { countRows, findRow, findRows, insertRow, insertRows, updateRows } from '../../helpers/factories/rows'; -import { setUserSetting } from '../../helpers/factories/settings'; +import { setAppSetting, setUserSetting } from '../../helpers/factories/settings'; import { makeVacayEntry, makeVacayPlan, addVacayPlanMember } from '../../helpers/factories/vacay'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { createMcpHarness, parseToolResult, parseResourceResult, type McpHarness } from '../../helpers/mcp-harness'; @@ -1090,3 +1091,92 @@ describe('scope gating', () => { for (const t of [...WRITE_TOOLS, ...READ_TOOLS, 'delete_trip', ...SHARE_TOOLS]) expect(names).not.toContain(t); }); }); + +// --------------------------------------------------------------------------- +// The trip writes REST shares (TripWritesService) and the rights PUT +// /api/trips/:id checks field by field. +// --------------------------------------------------------------------------- + +describe('Trip writes, as REST makes them', () => { + /** Lower or raise a configurable action the way the admin permission panel does. */ + async function setPermission(action: string, level: string): Promise { + await setAppSetting(orm, `perm_${action}`, level); + await invalidatePermissionsCache(); + } + + beforeEach(() => { + realtime.broadcastToUserMock.mockClear(); + }); + + afterEach(async () => { + await invalidatePermissionsCache(); + }); + + it('MCP-TRIPS-001: create_trip and copy_trip tell every tab of the owner that the trip list changed', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const created = parseToolResult( + await h.client.callTool({ name: 'create_trip', arguments: { title: 'Lisbon' } }), + ) as { trip: { id: number } }; + await h.client.callTool({ name: 'copy_trip', arguments: { tripId: created.trip.id } }); + }); + expect(realtime.broadcastToUserMock.mock.calls).toEqual([ + [user.id, { type: 'trips:changed' }, undefined], + [user.id, { type: 'trips:changed' }, undefined], + ]); + }); + + it('MCP-TRIPS-002: create_trip and copy_trip are refused without trip_create, as the routes refuse them', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await setPermission('trip_create', 'admin'); + await withHarness(user.id, async (h) => { + const created = await h.client.callTool({ name: 'create_trip', arguments: { title: 'Lisbon' } }); + const copied = await h.client.callTool({ name: 'copy_trip', arguments: { tripId: trip.id } }); + for (const result of [created, copied]) { + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toContain('You do not have permission to create trips.'); + } + }); + expect(await countRows(orm, Trips, { user: user.id })).toBe(1); + expect(realtime.broadcastToUserMock).not.toHaveBeenCalled(); + }); + + it('MCP-TRIPS-003: delete_trip tells the room the trip is gone, as the route does', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + await h.client.callTool({ name: 'delete_trip', arguments: { tripId: trip.id } }); + }); + expect(broadcastMock.mock.calls).toEqual([[trip.id, 'trip:deleted', { id: trip.id, _source: 'mcp' }]]); + }); + + it('MCP-TRIPS-004: update_trip asks trip_archive and trip_cover_upload for those fields, not trip_edit alone', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { title: 'Before' }); + addTripMember(testDb, trip.id, member.id); + await setPermission('trip_edit', 'trip_member'); + await withHarness(member.id, async (h) => { + const archive = await h.client.callTool({ + name: 'update_trip', + arguments: { tripId: trip.id, is_archived: true }, + }); + const cover = await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, cover_image: null } }); + expect(archive.isError).toBe(true); + expect(cover.isError).toBe(true); + expect((await tripRow(trip.id)).is_archived).toBe(0); + const rename = await h.client.callTool({ name: 'update_trip', arguments: { tripId: trip.id, title: 'After' } }); + expect(rename.isError).toBeFalsy(); + }); + await setPermission('trip_archive', 'trip_member'); + await withHarness(member.id, async (h) => { + const archive = await h.client.callTool({ + name: 'update_trip', + arguments: { tripId: trip.id, is_archived: true }, + }); + expect(archive.isError).toBeFalsy(); + }); + expect(await tripRow(trip.id)).toMatchObject({ title: 'After', is_archived: 1 }); + }); +}); diff --git a/server/tests/unit/nest/assistant/assistant-share.service.test.ts b/server/tests/unit/nest/assistant/assistant-share.service.test.ts index 8c5b878ec..2d115769a 100644 --- a/server/tests/unit/nest/assistant/assistant-share.service.test.ts +++ b/server/tests/unit/nest/assistant/assistant-share.service.test.ts @@ -2,8 +2,9 @@ * Sharing an assistant answer in the trip chat, over the real thread and * message repositories on a migrated test db: the chat must be on, the thread * the user's on this trip, the message an answer in it; then it is posted as - * the user's own message with the assistant's origin, broadcast and notified - * like a typed one. + * the user's own message with the assistant's origin, through the chat's own + * posting use case, which broadcasts and notifies like a typed message + * (collab.service.test.ts holds that, the preview included). * * Replaced: the addon switches and the collab chat itself. */ @@ -33,11 +34,14 @@ const addons = { getCollabFeatures: vi.fn(async () => ({ chat: true, notes: true, polls: true })), }; const collab = { - createMessage: vi.fn(async (_trip: number, userId: number, text: string) => ({ - message: { id: 90, user_id: userId, text: text.trim(), origin: 'assistant' }, + restWriter: vi.fn((tripId: string, user: User, socketId: string | undefined) => ({ + tripId, + userId: user.id, + socketId, + })), + postMessage: vi.fn(async (_trip: number, input: { text: string; origin?: string | null }, w: { userId: number }) => ({ + message: { id: 90, user_id: w.userId, text: input.text.trim(), origin: input.origin }, })), - broadcast: vi.fn(), - notifyCollab: vi.fn(async () => undefined), }; beforeAll(async () => { @@ -81,25 +85,24 @@ function shareAs(user: User, tripId: number, threadId: number, messageId: string } describe('AssistantShareService', () => { - it('ASTSHR-001: posts the answer as the user, marked as from the assistant, broadcast and notified', async () => { + it('ASTSHR-001: posts the answer as the user, marked as from the assistant, through the chat posting use case', async () => { const c = await conversation(); const service = t.get(AssistantShareService); - const shared = await service.share(c.user, c.tripId, c.threadId, `msg-${c.answer.id}`, 'x'.repeat(100), 'sock-1'); - expect(collab.createMessage).toHaveBeenCalledWith(c.tripId, c.user.id, 'x'.repeat(100), null, [], 'assistant'); - expect(shared).toEqual({ message: expect.objectContaining({ id: 90, origin: 'assistant' }) }); - expect(collab.broadcast).toHaveBeenCalledWith( - String(c.tripId), - 'collab:message:created', - { message: shared.message }, - 'sock-1', + const shared = await service.share(c.user, c.tripId, c.threadId, `msg-${c.answer.id}`, 'x'.repeat(100), undefined); + expect(collab.postMessage).toHaveBeenCalledWith( + c.tripId, + { text: 'x'.repeat(100), origin: 'assistant' }, + { tripId: String(c.tripId), userId: c.user.id, socketId: undefined }, ); - expect(collab.notifyCollab).toHaveBeenCalledWith(String(c.tripId), c.user, `${'x'.repeat(80)}...`); + expect(shared).toEqual({ message: expect.objectContaining({ id: 90, origin: 'assistant' }) }); }); - it('ASTSHR-002: a short answer is previewed whole', async () => { + it('ASTSHR-002: the asking tab hears its shared message too: the socket the route names is not left out', async () => { const c = await conversation(); - await shareAs(c.user, c.tripId, c.threadId, `msg-${c.answer.id}`); - expect(collab.notifyCollab).toHaveBeenCalledWith(String(c.tripId), c.user, 'Hi'); + await t.get(AssistantShareService).share(c.user, c.tripId, c.threadId, `msg-${c.answer.id}`, 'Hi', 'sock-1'); + // It never put the message into its own chat, so a writer skipping it would leave the chat without it. + expect(collab.restWriter).toHaveBeenCalledWith(String(c.tripId), c.user, undefined); + expect(collab.postMessage.mock.calls[0]?.[2]).toMatchObject({ socketId: undefined }); }); it('ASTSHR-003: refused while the collab addon or its chat is off', async () => { @@ -109,7 +112,7 @@ describe('AssistantShareService', () => { await expect(share()).rejects.toMatchObject({ status: 403, message: 'Collab chat is off' }); addons.getCollabFeatures.mockResolvedValueOnce({ chat: false, notes: true, polls: true }); await expect(share()).rejects.toMatchObject({ status: 403, code: 'forbidden' }); - expect(collab.createMessage).not.toHaveBeenCalled(); + expect(collab.postMessage).not.toHaveBeenCalled(); }); it('ASTSHR-004: a thread of another user or trip, a question, an answer elsewhere or no id is not found', async () => { @@ -121,14 +124,14 @@ describe('AssistantShareService', () => { await expect(shareAs(c.user, c.tripId, c.threadId, `msg-${c.question.id}`)).rejects.toMatchObject(notFound); await expect(shareAs(c.user, c.tripId, c.threadId, `msg-${other.answer.id}`)).rejects.toMatchObject(notFound); await expect(shareAs(c.user, c.tripId, c.threadId, 'msg-x')).rejects.toBeInstanceOf(DomainError); - expect(collab.createMessage).not.toHaveBeenCalled(); + expect(collab.postMessage).not.toHaveBeenCalled(); }); it('ASTSHR-005: a message the chat could not post is a refusal, never a silent success', async () => { const c = await conversation(); - collab.createMessage.mockResolvedValueOnce({ message: undefined } as never); + collab.postMessage.mockResolvedValueOnce({ message: undefined } as never); await expect(shareAs(c.user, c.tripId, c.threadId, `msg-${c.answer.id}`)).rejects.toMatchObject({ status: 400 }); - expect(collab.broadcast).not.toHaveBeenCalled(); + expect(collab.postMessage).toHaveBeenCalledTimes(1); }); it('ASTSHR-006: reads a wire message id as its row', () => { diff --git a/server/tests/unit/nest/collab.controller.test.ts b/server/tests/unit/nest/collab.controller.test.ts index cac3d2285..44451552c 100644 --- a/server/tests/unit/nest/collab.controller.test.ts +++ b/server/tests/unit/nest/collab.controller.test.ts @@ -4,7 +4,8 @@ import { collabChatImageFilter, collabNoteFileFilter, } from '../../../src/nest/collab/collab.controller'; -import type { CollabService } from '../../../src/nest/collab/collab.service'; +import type { ChatMessageInput, CollabService } from '../../../src/nest/collab/collab.service'; +import type { TripWriter } from '../../../src/nest/common/trip-writer'; import { TripAccessGuard, TRIP_PERMISSION_KEY } from '../../../src/nest/permissions/trip-access.guard'; import type { StorageService } from '../../../src/nest/storage/storage.service'; import type { User } from '../../../src/types'; @@ -17,15 +18,30 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const user = { id: 1, username: 'u', role: 'user', email: 'u@example.test' } as User; +/** The writer a route builds for its request, as the stub hands it back. */ +const restWriter = (socketId?: string) => ({ userId: 1, role: 'user', socketId }); + function svc(o: Partial = {}): CollabService { - return { + const s = { verifyTripAccess: vi.fn().mockResolvedValue({ user_id: 1 }), canEdit: vi.fn().mockResolvedValue(true), canUploadFiles: vi.fn().mockResolvedValue(true), broadcast: vi.fn(), notifyCollab: vi.fn(), + restWriter: vi.fn((_tripId: string, _user: User, socketId: string | undefined) => restWriter(socketId)), ...o, } as unknown as CollabService; + // The two posting use cases over the stubbed building blocks, so a case can stub + // createNote or createMessage as before. What the real ones send and notify is + // held by collab.service.test.ts and the broadcast parity suite. + const post = s as unknown as Record<'postNote' | 'postMessage', unknown>; + post.postNote ??= vi.fn((tripId: string, data: Parameters[2], w: TripWriter) => + s.createNote(tripId, w.userId, data), + ); + post.postMessage ??= vi.fn((tripId: string, input: ChatMessageInput, w: TripWriter) => + s.createMessage(tripId, w.userId, input.text, input.replyTo ?? null, input.files ?? [], input.origin ?? null), + ); + return s; } const storageStub = { @@ -53,23 +69,18 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou expect(await new CollabController(s, storageStub).listNotes(user, '5')).toEqual({ notes: [{ id: 1 }] }); }); - it('POST creates + broadcasts + notifies (empty title now 400s in the Zod pipe)', async () => { + it('POST creates through the posting use case, which broadcasts and notifies (empty title now 400s in the Zod pipe)', async () => { const createNote = vi.fn().mockReturnValue({ id: 9 }); - const broadcast = vi.fn(); - const notifyCollab = vi.fn(); - const s = svc({ createNote, broadcast, notifyCollab } as Partial); + const s = svc({ createNote } as Partial); expect( await new CollabController(s, storageStub).createNote(user, '5', { title: 'T', content: 'c' }, 'sock'), ).toEqual({ note: { id: 9 } }); - expect(createNote).toHaveBeenCalledWith('5', 1, { - title: 'T', - content: 'c', - category: undefined, - color: undefined, - website: undefined, - }); - expect(broadcast).toHaveBeenCalledWith('5', 'collab:note:created', { note: { id: 9 } }, 'sock'); - expect(notifyCollab).toHaveBeenCalledWith('5', user); + const fields = { title: 'T', content: 'c', category: undefined, color: undefined, website: undefined }; + expect(createNote).toHaveBeenCalledWith('5', 1, fields); + expect(s.restWriter).toHaveBeenCalledWith('5', user, 'sock'); + expect(s.postNote).toHaveBeenCalledWith('5', fields, restWriter('sock')); + // Nothing goes out from the route itself, or it would go out twice. + expect(s.broadcast).not.toHaveBeenCalled(); }); it('PUT 404 when missing, else updates + broadcasts', async () => { @@ -292,18 +303,13 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou ).createMessage(user, '5', { text: 'hi', reply_to: 99 }, undefined), ), ).toEqual({ status: 400, body: { error: 'Reply target message not found' } }); - const broadcast = vi.fn(); - const notifyCollab = vi.fn(); - const s = svc({ - createMessage: vi.fn().mockResolvedValue({ message: { id: 3 } }), - broadcast, - notifyCollab, - } as Partial); + const s = svc({ createMessage: vi.fn().mockResolvedValue({ message: { id: 3 } }) } as Partial); expect( await new CollabController(s, storageStub).createMessage(user, '5', { text: 'hello' }, undefined, 'sock'), ).toEqual({ message: { id: 3 } }); - expect(broadcast).toHaveBeenCalledWith('5', 'collab:message:created', { message: { id: 3 } }, 'sock'); - expect(notifyCollab).toHaveBeenCalledWith('5', user, 'hello'); + // The posting use case sends and notifies; the route hands it the trimmed text and its writer. + expect(s.postMessage).toHaveBeenCalledWith('5', { text: 'hello', replyTo: null, files: [] }, restWriter('sock')); + expect(s.broadcast).not.toHaveBeenCalled(); }); it('refuses a caller who cannot reach the trip or cannot write, from inside the handler', async () => { @@ -380,12 +386,12 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou expect(broadcast).toHaveBeenCalledWith('5', 'collab:message:deleted', { messageId: 3, username: 'bob' }, 'sock'); }); - it("U5: deleteMessage broadcast falls back to the caller's own username when the service's username is undefined (the real, pre-existing shape)", async () => { + it("U5: deleteMessage broadcast falls back to the caller's own username when the service's username is undefined", async () => { const broadcast = vi.fn(); - // `CollabService.deleteMessage` really does return `{ username: undefined }` - // (findInTrip never joins users) — the controller's `result.username || - // user.username` fallback is what keeps the broadcast correct, and only - // works because the route already requires caller === message owner. + // `CollabService.deleteMessage` returned `{ username: undefined }` for + // years (findInTrip never joined users). It now reads the author's name, + // but the controller keeps its `result.username || user.username` + // fallback, which is correct because only the owner may delete. const s = svc({ deleteMessage: vi.fn().mockResolvedValue({ username: undefined }), broadcast, diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index 88201c0a1..8652c3811 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -25,6 +25,8 @@ import { dbNow } from '../../../src/db/types/db-timestamp.type'; import { CollabService } from '../../../src/nest/collab/collab.service'; import { avatarUrl } from '../../../src/nest/common/avatarUrl'; import { RateLimitService } from '../../../src/nest/common/rate-limit.service'; +import type { TripWriter } from '../../../src/nest/common/trip-writer'; +import type { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { @@ -37,6 +39,7 @@ import { } from '../../helpers/collab-repos'; import { createUser, createTrip } from '../../helpers/factories'; import { countRows, findRow, findRows, insertRow, updateRows } from '../../helpers/factories/rows'; +import { FakeRealtimeService } from '../../helpers/fake-realtime'; import { notificationsStub } from '../../helpers/notifications'; import { makeStorageFixture } from '../../helpers/storage-fixture'; import { resetTestDb } from '../../helpers/test-db'; @@ -111,13 +114,18 @@ let pollVotesRepo: CollabPollVotesRepository; let linksRepo: CollabLinksRepository; let messagesRepo: CollabMessagesRepository; -async function buildCollabService(storage = collabFx.storage, rl = rateLimit): Promise { +async function buildCollabService( + storage = collabFx.storage, + rl = rateLimit, + notifications = notificationsStub(), + realtime: RealtimeService = new RealtimeService(), +): Promise { // Plan 4 Task 2/4 — CollabService's own DatabaseService param is gone: // canAccessTrip now reads through the TripsRepository passed at the end. return new CollabService( new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), - new RealtimeService(), - notificationsStub(), + realtime, + notifications, storage, rl, await createTestUnitOfWork(testDb), @@ -396,6 +404,92 @@ describe('createMessage', () => { }); }); +// ── posting (every surface) ─────────────────────────────────────────────────── + +describe('postNote / postMessage', () => { + /** A CollabService whose broadcasts and notifications a case reads. */ + async function postingSvc() { + const realtime = new FakeRealtimeService(); + const send = vi.fn(async (_payload: { params: Record }) => ({ attempted: 0, delivered: 0 })); + const notifications = { + send, + actorEmail: vi.fn(async () => 'owner@collab.test'), + } as unknown as NotificationsService; + const service = await buildCollabService(collabFx.storage, rateLimit, notifications, realtime); + return { service, realtime, send }; + } + + /** The MCP shape of a writer: every socket, no socket left out. */ + const mcpWriter = (service: CollabService, tripId: number, userId: number): TripWriter => ({ + ...service.restWriter(tripId, { id: userId, role: 'user' }, undefined), + surface: 'mcp', + }); + + it('COLLAB-SVC-050: a posted note goes to the room minus the asking tab, then notifies the trip as its writer', async () => { + const { user1, trip } = setup(); + const { service, realtime, send } = await postingSvc(); + const note = await service.postNote( + trip.id, + { title: 'Ideas' }, + service.restWriter(String(trip.id), { id: user1.id, role: 'user' }, 'sock-1'), + ); + expect(realtime.broadcastMock.mock.calls).toEqual([[String(trip.id), 'collab:note:created', { note }, 'sock-1']]); + expect(send).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'collab_message', + actorId: user1.id, + scope: 'trip', + targetId: trip.id, + params: expect.objectContaining({ actor: 'owner@collab.test', tripId: String(trip.id) }), + }), + ); + }); + + it('COLLAB-SVC-051: a posted message is sent and notified with an 80 character preview, from any surface', async () => { + const { user1, trip } = setup(); + const { service, realtime, send } = await postingSvc(); + const long = 'x'.repeat(100); + const result = await service.postMessage(trip.id, { text: ` ${long} ` }, mcpWriter(service, trip.id, user1.id)); + expect(result.message!.text).toBe(long); + expect(realtime.broadcastMock).toHaveBeenCalledWith( + trip.id, + 'collab:message:created', + { message: result.message }, + undefined, + ); + expect(send).toHaveBeenCalledWith( + expect.objectContaining({ params: expect.objectContaining({ preview: `${'x'.repeat(80)}...` }) }), + ); + }); + + it('COLLAB-SVC-052: a short message is previewed whole, and an image without text says so', async () => { + const { user1, trip } = setup(); + const { service, send } = await postingSvc(); + const writer = mcpWriter(service, trip.id, user1.id); + await service.postMessage(trip.id, { text: 'Hi' }, writer); + await service.postMessage( + trip.id, + { text: '', files: [{ filename: 'a.jpg', originalname: 'a.jpg', size: 3, mimetype: 'image/jpeg' }] }, + writer, + ); + const previews = send.mock.calls.map(([payload]) => payload.params.preview); + expect(previews).toEqual(['Hi', 'sent an image']); + }); + + it('COLLAB-SVC-053: a reply to a message that is gone sends and notifies nothing', async () => { + const { user1, trip } = setup(); + const { service, realtime, send } = await postingSvc(); + const result = await service.postMessage( + trip.id, + { text: 'Hi', replyTo: 9999 }, + mcpWriter(service, trip.id, user1.id), + ); + expect(result).toEqual({ error: 'reply_not_found' }); + expect(realtime.broadcastMock).not.toHaveBeenCalled(); + expect(send).not.toHaveBeenCalled(); + }); +}); + // ── deleteMessage ownership check ───────────────────────────────────────────── describe('deleteMessage', () => { @@ -419,11 +513,9 @@ describe('deleteMessage', () => { const result = await svc.deleteMessage(trip.id, r.message!.id, user1.id); expect(result.error).toBeUndefined(); - // U5 (pre-existing, pinned not fixed): findInTrip never joins users, so - // this has always been undefined at runtime — the controller falls back - // to the caller's own username (CollabController.deleteMessage, correct - // because only the owner may delete their own message). - expect(result.username).toBeUndefined(); + // The author's name, read before the delete: MCP has no session user to + // fall back on, and REST's fallback names the same person anyway. + expect(result.username).toBe(user1.username); const row = await findRow(orm, CollabMessages, { id: r.message!.id }); expect(row?.deleted).toBe(1); diff --git a/server/tests/unit/nest/files.controller.test.ts b/server/tests/unit/nest/files.controller.test.ts index 4b6f8edf3..6ea73076e 100644 --- a/server/tests/unit/nest/files.controller.test.ts +++ b/server/tests/unit/nest/files.controller.test.ts @@ -336,22 +336,40 @@ describe('FilesController (parity with the legacy /api/trips/:tripId/files route ), ), ).toEqual({ status: 404, body: { error: 'File not found' } }); - const createFileLink = vi.fn().mockReturnValue([{ id: 1 }]); - const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), createFileLink } as Partial); - expect(await fc(s).link(user, trip, '5', '9', { reservation_id: 2 })).toEqual({ + // Through the link use case, which sends the file with its links to the room minus the asking tab. + const writer = { socketId: 'sock' }; + const restWriter = vi.fn().mockReturnValue(writer); + const linkFile = vi.fn().mockReturnValue([{ id: 1 }]); + const s = fsvc({ + getFileById: vi.fn().mockReturnValue({ id: 9 }), + linkFile, + restWriter, + } as Partial); + expect(await fc(s).link(user, trip, '5', '9', { reservation_id: 2 }, 'sock')).toEqual({ success: true, links: [{ id: 1 }], }); + expect(restWriter).toHaveBeenCalledWith('5', user, 'sock'); + expect(linkFile).toHaveBeenCalledWith( + '9', + { reservation_id: 2, assignment_id: undefined, place_id: undefined, budget_item_id: undefined }, + writer, + ); }); it('DELETE /:id/link/:linkId removes the link; GET /:id/links lists', async () => { - const deleteFileLink = vi.fn(); + const writer = { socketId: 'sock' }; + const unlinkFile = vi.fn(); expect( await fc( - fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), deleteFileLink } as Partial), - ).unlink(user, trip, '5', '9', '3'), + fsvc({ + getFileById: vi.fn().mockReturnValue({ id: 9 }), + unlinkFile, + restWriter: vi.fn().mockReturnValue(writer), + } as Partial), + ).unlink(user, trip, '5', '9', '3', 'sock'), ).toEqual({ success: true }); - expect(deleteFileLink).toHaveBeenCalledWith('3', '9'); + expect(unlinkFile).toHaveBeenCalledWith('9', '3', writer); const s = fsvc({ getFileById: vi.fn().mockReturnValue({ id: 9 }), getFileLinks: vi.fn().mockReturnValue([{ id: 1 }]), @@ -363,7 +381,7 @@ describe('FilesController (parity with the legacy /api/trips/:tripId/files route const foreign = () => fsvc({ getFileById: vi.fn().mockReturnValue(undefined), - deleteFileLink: vi.fn(), + unlinkFile: vi.fn(), getFileLinks: vi.fn(), } as Partial); const unlinkSvc = foreign(); @@ -371,7 +389,7 @@ describe('FilesController (parity with the legacy /api/trips/:tripId/files route status: 404, body: { error: 'File not found' }, }); - expect(unlinkSvc.deleteFileLink).not.toHaveBeenCalled(); + expect(unlinkSvc.unlinkFile).not.toHaveBeenCalled(); const listSvc = foreign(); expect(await rejected(fc(listSvc).links(user, trip, '5', '9'))).toEqual({ status: 404, diff --git a/server/tests/unit/nest/notifications.controller.test.ts b/server/tests/unit/nest/notifications.controller.test.ts index 421425373..3c564fa85 100644 --- a/server/tests/unit/nest/notifications.controller.test.ts +++ b/server/tests/unit/nest/notifications.controller.test.ts @@ -199,7 +199,9 @@ describe('NotificationsController (parity with the legacy /api/notifications rou describe('bulk + single mutations', () => { it('read-all returns success + count', async () => { const markAllRead = vi.fn().mockReturnValue(3); - expect(await makeController({ markAllRead }).readAll(user)).toEqual({ success: true, count: 3 }); + expect(await makeController({ markAllRead }).readAll(user, 'sock')).toEqual({ success: true, count: 3 }); + // The service tells the user's other tabs; the one that asked is left out. + expect(markAllRead).toHaveBeenCalledWith(4, 'sock'); }); it('delete-all returns success + count', async () => { @@ -224,8 +226,18 @@ describe('NotificationsController (parity with the legacy /api/notifications rou it('mark-read success', async () => { const markRead = vi.fn().mockReturnValue(true); - expect(await makeController({ markRead }).markRead(user, '5')).toEqual({ success: true }); - expect(markRead).toHaveBeenCalledWith(5, 4); + expect(await makeController({ markRead }).markRead(user, '5', 'sock')).toEqual({ success: true }); + expect(markRead).toHaveBeenCalledWith(5, 4, 'sock'); + }); + + it('mark-unread hands the socket on, and 404s when nothing changed', async () => { + const markUnread = vi.fn().mockReturnValueOnce(true).mockReturnValueOnce(false); + expect(await makeController({ markUnread }).markUnread(user, '5', 'sock')).toEqual({ success: true }); + expect(markUnread).toHaveBeenCalledWith(5, 4, 'sock'); + expect(await thrown(() => makeController({ markUnread }).markUnread(user, '6'))).toEqual({ + status: 404, + body: { error: 'Not found' }, + }); }); it('delete single success', async () => { diff --git a/server/tests/unit/nest/settings.controller.test.ts b/server/tests/unit/nest/settings.controller.test.ts index 44cf1dae4..41bb7ed2f 100644 --- a/server/tests/unit/nest/settings.controller.test.ts +++ b/server/tests/unit/nest/settings.controller.test.ts @@ -1,4 +1,5 @@ import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; +import type { SettingsWritesService } from '../../../src/nest/settings/settings-writes.service'; import { SettingsController } from '../../../src/nest/settings/settings.controller'; import type { SettingsService } from '../../../src/nest/settings/settings.service'; import type { User } from '../../../src/types'; @@ -30,12 +31,34 @@ async function thrown(fn: () => unknown): Promise<{ status: number; body: unknow beforeEach(() => vi.clearAllMocks()); +/** + * SettingsWritesService over the stubbed service: the writes go through it, so + * it hands each on and records the socket the route read. The settings:updated + * it sends is held by tools-settings.test.ts and the broadcast parity suite. + */ +function writesFor(s: SettingsService) { + return { + saveOne: vi.fn((userId: number, key: string, value: unknown, _socketId: string | undefined) => + s.upsertSetting(userId, key, value), + ), + saveMany: vi.fn((userId: number, values: Record, _socketId: string | undefined) => + s.bulkUpsertSettings(userId, values), + ), + }; +} +let writes: ReturnType; + +function controller(s: SettingsService) { + writes = writesFor(s); + const env = { isManaged: () => false } as unknown as RuntimeEnvService; + return new SettingsController(s, env, writes as unknown as SettingsWritesService); +} + describe('SettingsController', () => { it('GET / returns the settings', async () => { expect( - await new SettingsController( + await controller( svc({ getUserSettings: vi.fn().mockReturnValue({ theme: 'dark' }) } as Partial), - { isManaged: () => false } as unknown as RuntimeEnvService, ).list(user), ).toEqual({ settings: { theme: 'dark' } }); }); @@ -45,28 +68,27 @@ describe('SettingsController', () => { it('PUT / no-ops on the masked sentinel without writing', async () => { const upsertSetting = vi.fn(); - const c = new SettingsController(svc({ upsertSetting } as Partial), { - isManaged: () => false, - } as unknown as RuntimeEnvService); + const c = controller(svc({ upsertSetting } as Partial)); expect(await c.upsert(user, { key: 'immich_api_key', value: '••••••••' })).toEqual({ success: true, key: 'immich_api_key', unchanged: true, }); expect(upsertSetting).not.toHaveBeenCalled(); + expect(writes.saveOne).not.toHaveBeenCalled(); }); it('PUT / writes a real value', async () => { const upsertSetting = vi.fn(); - const c = new SettingsController(svc({ upsertSetting } as Partial), { - isManaged: () => false, - } as unknown as RuntimeEnvService); - expect(await c.upsert(user, { key: 'theme', value: 'dark' })).toEqual({ + const c = controller(svc({ upsertSetting } as Partial)); + expect(await c.upsert(user, { key: 'theme', value: 'dark' }, 'sock')).toEqual({ success: true, key: 'theme', value: 'dark', }); expect(upsertSetting).toHaveBeenCalledWith(1, 'theme', 'dark'); + // Through the writes, which tell the user's other tabs and leave out this one. + expect(writes.saveOne).toHaveBeenCalledWith(1, 'theme', 'dark', 'sock'); }); it('POST /bulk 500 on a write error, else returns the count', async () => { @@ -75,21 +97,22 @@ describe('SettingsController', () => { vi.spyOn(console, 'error').mockImplementation(() => {}); expect( await thrown(() => - new SettingsController( + controller( svc({ bulkUpsertSettings: vi.fn(() => { throw new Error('db'); }), } as Partial), - { isManaged: () => false } as unknown as RuntimeEnvService, ).bulk(user, { settings: { a: 1 } }), ), ).toEqual({ status: 500, body: { error: 'Error saving settings' } }); expect( - await new SettingsController( - svc({ bulkUpsertSettings: vi.fn().mockReturnValue(3) } as Partial), - { isManaged: () => false } as unknown as RuntimeEnvService, - ).bulk(user, { settings: { a: 1, b: 2, c: 3 } }), + await controller(svc({ bulkUpsertSettings: vi.fn().mockReturnValue(3) } as Partial)).bulk( + user, + { settings: { a: 1, b: 2, c: 3 } }, + 'sock', + ), ).toEqual({ success: true, updated: 3 }); + expect(writes.saveMany).toHaveBeenCalledWith(1, { a: 1, b: 2, c: 3 }, 'sock'); }); }); diff --git a/server/tests/unit/nest/settings/settings-writes.service.test.ts b/server/tests/unit/nest/settings/settings-writes.service.test.ts new file mode 100644 index 000000000..2c9cb74f0 --- /dev/null +++ b/server/tests/unit/nest/settings/settings-writes.service.test.ts @@ -0,0 +1,88 @@ +/** + * SettingsWritesService: the save goes through SettingsService, and the user's + * other tabs hear which keys changed. Keys only, never a value, and a masked + * secret echoed back by a settings form is named in no event, because nothing + * about it changed. + * + * Replaced: SettingsService (its writes have their own suite) and the realtime + * transport, which the test module fakes. + */ +import { SettingsWritesService } from '../../../../src/nest/settings/settings-writes.service'; +import { SettingsService } from '../../../../src/nest/settings/settings.service'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { createTestModule, type TestModule } from '../../../helpers/test-module'; +import { MASKED_SETTING_VALUE } from '@trek/shared'; + +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +const db = createSnapshotTestDb(); +let t: TestModule; +let writes: SettingsWritesService; + +const settings = { + upsertSetting: vi.fn(async (_userId: number, _key: string, _value: unknown) => undefined), + bulkUpsertSettings: vi.fn(async (_userId: number, values: Record) => { + return Object.values(values).filter((value) => value !== MASKED_SETTING_VALUE).length; + }), +}; + +beforeAll(async () => { + t = await createTestModule({ + db, + imports: [], + providers: [SettingsWritesService, { provide: SettingsService, useValue: settings }], + }); + writes = t.get(SettingsWritesService); +}); + +beforeEach(() => { + vi.clearAllMocks(); +}); + +afterAll(async () => { + await t.close(); +}); + +describe('SettingsWritesService', () => { + it('SET-WR-001: saveOne writes the key and names it to the other tabs, leaving the asking one out', async () => { + await writes.saveOne(7, 'temperature_unit', 'celsius', 'sock-1'); + + expect(settings.upsertSetting).toHaveBeenCalledWith(7, 'temperature_unit', 'celsius'); + expect(t.realtime.broadcastToUserMock).toHaveBeenCalledTimes(1); + expect(t.realtime.broadcastToUserMock).toHaveBeenCalledWith( + 7, + { type: 'settings:updated', keys: ['temperature_unit'] }, + 'sock-1', + ); + }); + + it('SET-WR-002: saveMany leaves a masked secret out of the keys and never sends a value', async () => { + const written = await writes.saveMany(7, { maps_api_key: MASKED_SETTING_VALUE, week_start: 'monday' }, 'sock-2'); + + expect(written).toBe(1); + expect(t.realtime.broadcastToUserMock).toHaveBeenCalledTimes(1); + expect(t.realtime.broadcastToUserMock).toHaveBeenCalledWith( + 7, + { type: 'settings:updated', keys: ['week_start'] }, + 'sock-2', + ); + }); + + it('SET-WR-003: saveMany with nothing but masked secrets sends no event at all', async () => { + const written = await writes.saveMany(7, { maps_api_key: MASKED_SETTING_VALUE }, undefined); + + expect(written).toBe(0); + expect(settings.bulkUpsertSettings).toHaveBeenCalledTimes(1); + expect(t.realtime.broadcastToUserMock).not.toHaveBeenCalled(); + }); + + it('SET-WR-004: without a socket (the MCP path) every tab of the user hears it', async () => { + await writes.saveMany(7, { week_start: 'sunday' }, undefined); + + expect(t.realtime.broadcastToUserMock).toHaveBeenCalledWith( + 7, + { type: 'settings:updated', keys: ['week_start'] }, + undefined, + ); + }); +}); diff --git a/server/tests/unit/nest/tags.controller.test.ts b/server/tests/unit/nest/tags.controller.test.ts index e9d7a4f7a..05885275b 100644 --- a/server/tests/unit/nest/tags.controller.test.ts +++ b/server/tests/unit/nest/tags.controller.test.ts @@ -1,3 +1,4 @@ +import type { TagWritesService } from '../../../src/nest/tags/tag-writes.service'; import { TagsController } from '../../../src/nest/tags/tags.controller'; import type { TagsService } from '../../../src/nest/tags/tags.service'; import type { User } from '../../../src/types'; @@ -9,8 +10,28 @@ import { describe, it, expect, vi } from 'vitest'; const user = { id: 5 } as User; +/** + * TagWritesService over the stubbed service: the writes go through it, so it + * hands each on and records the socket the route read. The tags:changed it + * sends is held by tools-tags-maps-weather.test.ts and the broadcast parity suite. + */ +function writesFor(svc: Partial) { + return { + create: vi.fn((userId: number, name: string, color: string | undefined, _socketId: string | undefined) => + svc.create!(userId, name, color), + ), + update: vi.fn( + (_userId: number, id: string, changes: { name?: string; color?: string }, _socketId: string | undefined) => + svc.update!(id, changes.name, changes.color), + ), + remove: vi.fn((_userId: number, id: string, _socketId: string | undefined) => svc.remove!(id)), + }; +} +let writes: ReturnType; + function makeController(svc: Partial) { - return new TagsController(svc as TagsService); + writes = writesFor(svc); + return new TagsController(svc as TagsService, writes as unknown as TagWritesService); } const tag: Tag = { id: 1, user_id: 5, name: 'Beach', color: '#10b981' }; @@ -45,10 +66,12 @@ describe('TagsController (parity with the legacy /api/tags route)', () => { it('creates a tag for the caller', async () => { const create = vi.fn().mockReturnValue(tag); - expect(await makeController({ create }).create(user, anyBody({ name: 'Beach', color: '#10b981' }))).toEqual({ - tag, - }); + expect( + await makeController({ create }).create(user, anyBody({ name: 'Beach', color: '#10b981' }), 'sock'), + ).toEqual({ tag }); expect(create).toHaveBeenCalledWith(5, 'Beach', '#10b981'); + // Through the writes, which tell the caller's other tabs and leave out this one. + expect(writes.create).toHaveBeenCalledWith(5, 'Beach', '#10b981', 'sock'); }); }); @@ -69,10 +92,11 @@ describe('TagsController (parity with the legacy /api/tags route)', () => { it('updates an owned tag', async () => { const getByIdAndUser = vi.fn().mockReturnValue(tag); const update = vi.fn().mockReturnValue({ ...tag, name: 'Hike' }); - expect(await makeController({ getByIdAndUser, update }).update(user, '1', anyBody({ name: 'Hike' }))).toEqual({ - tag: { ...tag, name: 'Hike' }, - }); + expect( + await makeController({ getByIdAndUser, update }).update(user, '1', anyBody({ name: 'Hike' }), 'sock'), + ).toEqual({ tag: { ...tag, name: 'Hike' } }); expect(update).toHaveBeenCalledWith('1', 'Hike', undefined); + expect(writes.update).toHaveBeenCalledWith(5, '1', { name: 'Hike', color: undefined }, 'sock'); }); }); @@ -90,8 +114,9 @@ describe('TagsController (parity with the legacy /api/tags route)', () => { it('deletes an owned tag', async () => { const getByIdAndUser = vi.fn().mockReturnValue(tag); const remove = vi.fn(); - expect(await makeController({ getByIdAndUser, remove }).remove(user, '1')).toEqual({ success: true }); + expect(await makeController({ getByIdAndUser, remove }).remove(user, '1', 'sock')).toEqual({ success: true }); expect(remove).toHaveBeenCalledWith('1'); + expect(writes.remove).toHaveBeenCalledWith(5, '1', 'sock'); }); }); }); diff --git a/server/tests/unit/nest/trip-members.controller.test.ts b/server/tests/unit/nest/trip-members.controller.test.ts index 890574c99..32c874ca5 100644 --- a/server/tests/unit/nest/trip-members.controller.test.ts +++ b/server/tests/unit/nest/trip-members.controller.test.ts @@ -2,6 +2,8 @@ import type { AuditService } from '../../../src/nest/audit/audit.service'; import { GUEST_HAS_PAYMENTS } from '../../../src/nest/budget/budget-departure.helpers'; import { DomainError } from '../../../src/nest/common/domain-error'; import { NotFoundError, ValidationError } from '../../../src/nest/common/domain-errors'; +import type { TripWriter } from '../../../src/nest/common/trip-writer'; +import type { TripMemberWritesService } from '../../../src/nest/trip-members/trip-member-writes.service'; import { TripMembersController } from '../../../src/nest/trip-members/trip-members.controller'; import { TripMembersModule } from '../../../src/nest/trip-members/trip-members.module'; import type { TripMembersService } from '../../../src/nest/trip-members/trip-members.service'; @@ -32,7 +34,38 @@ const req = { headers: {} } as Request; const writeAudit = vi.fn(); const audit = { writeAudit } as unknown as AuditService; -const tc = (s: TripMembersService) => new TripMembersController(s, audit); + +/** + * TripMemberWritesService over the stubbed roster: the roster routes hand their + * writes to it, so it passes each on to the stub and records the writer the + * route built. What the real one sends (member:added/removed/updated, the + * invitation, the re-split expenses) is held by tools-trip-members.test.ts and + * the broadcast parity suite. + */ +function writesFor(s: TripMembersService) { + return { + restWriter: vi.fn( + (_tripId: string | number, u: User, socketId: string | undefined) => + ({ userId: u.id, role: u.role, socketId }) as TripWriter, + ), + addMember: vi.fn((tripId: string, identifier: string, ownerId: number, w: TripWriter) => + s.addMember(tripId, identifier, ownerId, w.userId), + ), + removeMember: vi.fn((tripId: number, userId: number, _w: TripWriter) => s.removeMember(tripId, userId)), + createGuest: vi.fn((tripId: string, name: string, w: TripWriter) => s.createGuest(tripId, name, w.userId)), + renameGuest: vi.fn((tripId: string, guestId: number, name: string, _w: TripWriter) => + s.renameGuest(tripId, guestId, name), + ), + deleteGuest: vi.fn((tripId: string, guestId: number, _w: TripWriter) => s.deleteGuest(tripId, guestId)), + }; +} +let writes: ReturnType; +const tc = (s: TripMembersService) => { + writes = writesFor(s); + return new TripMembersController(s, audit, writes as unknown as TripMemberWritesService); +}; +/** The writer a route builds for the user of these cases. */ +const restWriter = (socketId?: string) => ({ userId: 1, role: 'user', socketId }); function svc(o: Partial = {}): TripMembersService { return { @@ -70,24 +103,24 @@ describe('TripMembersController', () => { expect(await tc(s).members(user, '9')).toEqual({ owner: { id: 1 }, members: [], current_user_id: 1 }); }); - it('POST 403 without member_manage, else adds + notifies', async () => { + it('POST 403 without member_manage, else adds through the roster writes', async () => { expect( await thrown(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).addMember(user, '9', { identifier: 'bob@x.y' }), ), ).toEqual({ status: 403, body: { error: 'No permission to manage members' } }); + expect(writes.addMember).not.toHaveBeenCalled(); const addMember = vi .fn() .mockReturnValue({ member: { id: 2, email: 'bob@x.y' }, targetUserId: 2, tripTitle: 'T' }); - const notifyInvite = vi.fn(); - const s = svc({ addMember, notifyInvite } as Partial); - expect(await tc(s).addMember(user, '9', { identifier: 'bob@x.y' })).toEqual({ + const s = svc({ addMember } as Partial); + expect(await tc(s).addMember(user, '9', { identifier: 'bob@x.y' }, 'sock')).toEqual({ member: { id: 2, email: 'bob@x.y' }, }); - expect(notifyInvite).toHaveBeenCalledWith('9', user, 2, 'T', 'bob@x.y'); - // Task 6 review, L1: the pre-existing REST↔MCP asymmetry (§13b) — these five - // controller mutations broadcast NOTHING while the MCP twins all emit - // member:added/member:removed. Pinned here rather than "fixed". + // The use case announces member:added and sends the invitation, as the MCP + // tool does through the same method; the route only builds its writer. + expect(writes.restWriter).toHaveBeenCalledWith('9', user, 'sock'); + expect(writes.addMember).toHaveBeenCalledWith('9', 'bob@x.y', 1, restWriter('sock')); expect(s.broadcast).not.toHaveBeenCalled(); }); @@ -138,13 +171,15 @@ describe('TripMembersController', () => { const removeMember = vi.fn(); const s = svc({ can: vi.fn().mockReturnValue(false), removeMember } as Partial); // self-removal (targetId === user.id) bypasses the permission check - expect(await tc(s).removeMember(user, '9', '1')).toEqual({ success: true }); + expect(await tc(s).removeMember(user, '9', '1', 'sock')).toEqual({ success: true }); + // The parsed trip id, as the removal itself takes it; member:removed goes out from the use case. + expect(writes.removeMember).toHaveBeenCalledWith(9, 1, restWriter('sock')); + expect(removeMember).toHaveBeenCalledWith(9, 1); expect(await thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 403, body: { error: 'No permission to remove members' }, }); - // Task 6 review, L1: see the addMember case above. - expect(s.broadcast).not.toHaveBeenCalled(); + expect(writes.removeMember).not.toHaveBeenCalled(); }); }); @@ -211,12 +246,12 @@ describe('TripMembersController', () => { }); const createGuest = vi.fn().mockReturnValue({ member: { id: 7, username: 'Anna', is_guest: true } }); const s = svc({ createGuest } as Partial); - expect(await tc(s).createGuest(user, '9', { name: 'Anna' })).toEqual({ + expect(await tc(s).createGuest(user, '9', { name: 'Anna' }, 'sock')).toEqual({ member: { id: 7, username: 'Anna', is_guest: true }, }); expect(createGuest).toHaveBeenCalledWith('9', 'Anna', user.id); - // Task 6 review, L1: see the addMember case above. - expect(s.broadcast).not.toHaveBeenCalled(); + // member:added goes out from the use case, to the room minus the asking tab. + expect(writes.createGuest).toHaveBeenCalledWith('9', 'Anna', restWriter('sock')); }); it('rename: 404 when the guest is missing, else success', async () => { @@ -226,9 +261,8 @@ describe('TripMembersController', () => { body: { error: 'Guest not found' }, }); const ok = svc({ renameGuest: vi.fn().mockReturnValue(true) } as Partial); - expect(await tc(ok).renameGuest(user, '9', '7', { name: 'Bob' })).toEqual({ success: true }); - // Task 6 review, L1: see the addMember case above. - expect(ok.broadcast).not.toHaveBeenCalled(); + expect(await tc(ok).renameGuest(user, '9', '7', { name: 'Bob' }, 'sock')).toEqual({ success: true }); + expect(writes.renameGuest).toHaveBeenCalledWith('9', 7, 'Bob', restWriter('sock')); }); it('delete: 404 when the guest is missing, else success', async () => { @@ -238,9 +272,8 @@ describe('TripMembersController', () => { body: { error: 'Guest not found' }, }); const ok = svc({ deleteGuest: vi.fn().mockResolvedValue(true) } as Partial); - expect(await tc(ok).deleteGuest(user, '9', '7')).toEqual({ success: true }); - // Task 6 review, L1: see the addMember case above. - expect(ok.broadcast).not.toHaveBeenCalled(); + expect(await tc(ok).deleteGuest(user, '9', '7', 'sock')).toEqual({ success: true }); + expect(writes.deleteGuest).toHaveBeenCalledWith('9', 7, restWriter('sock')); }); it('passes the 409 of a guest who moved money on as it stands, the text the MCP tool answers too (#2604)', async () => { diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts index e0c6bb87b..4380e2fc3 100644 --- a/server/tests/unit/nest/trip-members.service.test.ts +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -432,6 +432,21 @@ describe('listMembers shaping', () => { expect(byId.get(bare.id)!.is_guest).toBe(false); expect(ownerRow.is_guest).toBe(false); }); + + it('MEMBERS-SVC-020: findMember returns one roster row by id, and nothing for the owner, a stranger or a missing trip', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + + const row = await roster.findMember(trip.id, member.id); + expect(row).toMatchObject({ id: member.id, role: 'member', is_guest: false, avatar_url: null }); + // The owner is not a trip_members row, so the plugin path never announces them. + expect(await roster.findMember(trip.id, owner.id)).toBeUndefined(); + expect(await roster.findMember(trip.id, stranger.id)).toBeUndefined(); + expect(await roster.findMember(999999, member.id)).toBeUndefined(); + }); }); // ── Task 6 review items (M1, L2) ───────────────────────────────────────────── diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index b509fecec..cace9ba70 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -2,7 +2,9 @@ import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.serv import type { AuditService } from '../../../src/nest/audit/audit.service'; import type { CalendarService } from '../../../src/nest/calendar/calendar.service'; import { DomainError } from '../../../src/nest/common/domain-error'; +import type { TripWriter } from '../../../src/nest/common/trip-writer'; import type { TripReadModelService } from '../../../src/nest/trip-read-model/trip-read-model.service'; +import type { TripListWriter, TripWritesService } from '../../../src/nest/trips/trip-writes.service'; import { TripsController, MAX_COVER_SIZE, TRIP_COVER_FILE_FILTER } from '../../../src/nest/trips/trips.controller'; import type { TripsService } from '../../../src/nest/trips/trips.service'; import { NotFoundError, ValidationError } from '../../../src/nest/trips/trips.service'; @@ -52,7 +54,30 @@ const audit = { writeAudit } as unknown as AuditService; const storageStub = { put: vi.fn().mockResolvedValue(undefined), } as unknown as import('../../../src/nest/storage/storage.service').StorageService; -const tc = (s: TripsService, calendar?: Partial, readModel?: Partial) => +/** + * TripWritesService over the stubbed TripsService: create, copy and delete go + * through it, so it hands each on to the stub and records the writer the route + * built. What the real one sends is held by tools-trips.test.ts and the + * broadcast parity suite, on the booted app. + */ +function writesFor(s: TripsService) { + return { + restWriter: vi.fn( + (_tripId: string, u: User, socketId: string | undefined) => + ({ userId: u.id, role: u.role, socketId }) as TripWriter, + ), + create: vi.fn((input: Parameters[1], w: TripListWriter) => s.create(w.userId, input)), + copy: vi.fn((id: string, title: string | undefined, w: TripListWriter) => s.copy(id, w.userId, title)), + remove: vi.fn((id: string, w: TripWriter) => s.remove(id, w.userId, w.role)), + }; +} + +const tc = ( + s: TripsService, + calendar?: Partial, + readModel?: Partial, + writes = writesFor(s), +) => new TripsController( s, audit, @@ -61,6 +86,7 @@ const tc = (s: TripsService, calendar?: Partial, readModel?: Pa (calendar ?? {}) as CalendarService, (readModel ?? {}) as TripReadModelService, storageStub, + writes as unknown as TripWritesService, ); function svc(o: Partial = {}): TripsService { @@ -290,6 +316,19 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = trip: { id: 9 }, }); }); + + it('creates through the trip writes as the session user, naming the tab that asked', async () => { + const s = svc({ + create: vi.fn().mockReturnValue({ trip: { id: 9 }, tripId: 9, reminderDays: 0 }), + } as Partial); + const writes = writesFor(s); + await tc(s, undefined, undefined, writes).create(user, { title: 'T' }, req, 'sock'); + expect(writes.create).toHaveBeenCalledWith(expect.objectContaining({ title: 'T' }), { + userId: 1, + role: 'user', + socketId: 'sock', + }); + }); }); it('GET /:id 404 when missing', async () => { @@ -440,7 +479,11 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = copy: vi.fn().mockReturnValue(42), getCopiedTrip: vi.fn().mockReturnValue({ id: 42 }), } as Partial); - expect(await tc(s).copy(user, '9', { title: 'Copy' }, req)).toEqual({ trip: { id: 42 } }); + const writes = writesFor(s); + expect(await tc(s, undefined, undefined, writes).copy(user, '9', { title: 'Copy' }, req, 'sock')).toEqual({ + trip: { id: 42 }, + }); + expect(writes.copy).toHaveBeenCalledWith('9', 'Copy', { userId: 1, role: 'user', socketId: 'sock' }); }); }); @@ -487,20 +530,27 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = expect(remove).toHaveBeenCalledWith('9', 1, 'admin'); }); - it('deletes, audits and broadcasts', async () => { + it('deletes through the trip writes, which announce it to the room minus the asking tab, and audits', async () => { const remove = vi.fn().mockReturnValue({ tripId: 9, title: 'T', isAdminDelete: false }); - const broadcast = vi.fn(); - const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 1 }), remove, broadcast } as Partial); - expect(await tc(s).remove(user, '9', req, 'sock')).toEqual({ success: true }); - expect(broadcast).toHaveBeenCalledWith('9', 'trip:deleted', { id: 9 }, 'sock'); + const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 1 }), remove } as Partial); + const writes = writesFor(s); + expect(await tc(s, undefined, undefined, writes).remove(user, '9', req, 'sock')).toEqual({ success: true }); + expect(writes.restWriter).toHaveBeenCalledWith('9', user, 'sock'); + expect(writes.remove).toHaveBeenCalledWith('9', { userId: 1, role: 'user', socketId: 'sock' }); + expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'trip.delete' })); + // The broadcast left the controller for the use case: nothing goes out from here twice. + expect(s.broadcast).not.toHaveBeenCalled(); }); it('admin delete logs the owner', async () => { const remove = vi.fn().mockReturnValue({ tripId: 9, title: 'T', isAdminDelete: true, ownerEmail: 'owner@x.y' }); - const broadcast = vi.fn(); - const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), remove, broadcast } as Partial); - expect(await tc(s).remove(user, '9', req)).toEqual({ success: true }); - expect(broadcast).toHaveBeenCalledWith('9', 'trip:deleted', { id: 9 }, undefined); + const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), remove } as Partial); + const writes = writesFor(s); + expect(await tc(s, undefined, undefined, writes).remove(user, '9', req)).toEqual({ success: true }); + expect(writes.restWriter).toHaveBeenCalledWith('9', user, undefined); + expect(writeAudit).toHaveBeenCalledWith( + expect.objectContaining({ details: expect.objectContaining({ owner: 'owner@x.y' }) }), + ); }); }); diff --git a/server/tests/unit/trips/trips.rpc.test.ts b/server/tests/unit/trips/trips.rpc.test.ts index 996926f1b..edbe075bf 100644 --- a/server/tests/unit/trips/trips.rpc.test.ts +++ b/server/tests/unit/trips/trips.rpc.test.ts @@ -71,6 +71,7 @@ export function build(opts: { allow?: (action: string) => boolean; updateThrows? // Membership left TripsService for its own domain. const roster = { removeMember: vi.fn(), + findMember: vi.fn(async (_tripId: number, userId: number) => ({ id: userId, username: 'bob', role: 'member' })), } as unknown as TripMembersService & Record>; const membership = { joinTripAsMember: vi.fn(() => ({ joined: true })) } as unknown as TripMembershipService & Record>; @@ -328,6 +329,18 @@ describe('TripsRpc membership', () => { const f = build(); expect((await f.host().dispatch(req('trips.addMember', { tripId: 1, userId: 7 }), 42)).ok).toBe(true); expect(f.membership.joinTripAsMember).toHaveBeenCalledWith(1, 7, 42); + // The whole room hears it: a plugin has no socket to leave out. + expect(f.realtime.broadcast).toHaveBeenCalledWith(1, 'member:added', { + member: { id: 7, username: 'bob', role: 'member' }, + }); + }); + + it('TRIPS-RPC-021 a join that changes nothing announces nothing', async () => { + const f = build(); + f.membership.joinTripAsMember.mockResolvedValueOnce({ joined: false, tripId: 1 }); + expect((await f.host().dispatch(req('trips.addMember', { tripId: 1, userId: 7 }), 42)).ok).toBe(true); + expect(f.roster.findMember).not.toHaveBeenCalled(); + expect(f.realtime.broadcast).not.toHaveBeenCalled(); }); it('TRIPS-RPC-014 member management is its own grant, separate from every other write', async () => { @@ -352,12 +365,14 @@ describe('TripsRpc membership', () => { const res = (await f.host().dispatch(req('trips.removeMember', { tripId: 1, userId: 42 }), 42)) as RpcError; expect(res.error.message).toBe('cannot remove the trip owner'); expect(f.roster.removeMember).not.toHaveBeenCalled(); + expect(f.realtime.broadcast).not.toHaveBeenCalled(); }); it('TRIPS-RPC-017 a non-owner member is removed', async () => { const f = build(); expect((await f.host().dispatch(req('trips.removeMember', { tripId: 1, userId: 7 }), 42)).ok).toBe(true); expect(f.roster.removeMember).toHaveBeenCalledWith(1, 7); + expect(f.realtime.broadcast).toHaveBeenCalledWith(1, 'member:removed', { userId: 7 }); }); it('TRIPS-RPC-018 without member_manage nothing changes', async () => {