From 8fe2ae80af5392141f58d7fff3f1d6559ac7b560 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 22 Oct 2026 12:14:26 +0200 Subject: [PATCH] fix(realtime): budget, packing and todo writes send what REST sends --- .../src/nest/budget/budget-writes.service.ts | 57 +++ server/src/nest/budget/budget.controller.ts | 15 +- server/src/nest/budget/budget.mcp.ts | 37 +- server/src/nest/budget/budget.module.ts | 11 +- .../nest/packing/packing-writes.service.ts | 92 ++++- server/src/nest/packing/packing.controller.ts | 59 ++-- server/src/nest/packing/packing.mcp.ts | 73 ++-- server/src/nest/packing/packing.module.ts | 3 + server/src/nest/packing/packing.service.ts | 2 +- server/src/nest/todo/todo.controller.ts | 9 +- server/src/nest/todo/todo.mcp.ts | 4 +- server/src/nest/todo/todo.service.ts | 16 + .../broadcast-parity/money.cases.ts | 333 ++++++++++++++++-- .../unit/mcp/tools-budget-advanced.test.ts | 21 ++ .../unit/mcp/tools-packing-advanced.test.ts | 72 +++- server/tests/unit/mcp/tools-packing.test.ts | 3 + server/tests/unit/mcp/tools-todos.test.ts | 5 + .../tests/unit/nest/budget-items.mcp.test.ts | 2 +- .../tests/unit/nest/budget.controller.test.ts | 74 ++-- .../unit/nest/packing-writes.service.test.ts | 106 +++++- .../unit/nest/packing.controller.test.ts | 47 ++- .../tests/unit/nest/todo.controller.test.ts | 15 +- server/tests/unit/nest/todo.service.test.ts | 51 +++ 23 files changed, 916 insertions(+), 191 deletions(-) create mode 100644 server/src/nest/budget/budget-writes.service.ts diff --git a/server/src/nest/budget/budget-writes.service.ts b/server/src/nest/budget/budget-writes.service.ts new file mode 100644 index 000000000..ad6352885 --- /dev/null +++ b/server/src/nest/budget/budget-writes.service.ts @@ -0,0 +1,57 @@ +import type { User } from '../../types'; +import { DomainError } from '../common/domain-error'; +import { requireTripWrite, restTripWriter, type TripWriter } from '../common/trip-writer'; +import { PermissionsService } from '../permissions/permissions.service'; +import { RealtimeService } from '../realtime/realtime.service'; +import { BudgetService } from './budget.service'; +import { Injectable } from '@nestjs/common'; + +/** The fields a new expense takes, as BudgetService.create reads them. */ +export type BudgetItemCreateInput = Parameters[1]; + +/** + * Budget writes as use cases, one method each, for every surface: the trip + * gate, the write and the events it sends, after its commit. + * + * Only the create lives here so far. The MCP tool that adds an expense together + * with its split sent budget:members-updated on top of budget:created, which the + * REST route never sent for the same write; both now run this one method. The + * other budget writes still send from their adapters, and send the same there. + */ +@Injectable() +export class BudgetWritesService { + constructor( + private readonly budget: BudgetService, + private readonly permissions: PermissionsService, + private readonly realtime: RealtimeService, + ) {} + + /** The REST caller of a budget use case: its events skip the socket that sent the request. */ + restWriter(tripId: string, user: User, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** + * Add an expense. A booking or a place it links to must be on the same trip + * (#2084), and a booking mirrors the total of its expenses, so a linked one + * has its price worked out again before budget:created goes out. The item + * already carries its members, so no members-updated follows. + */ + async createItem(tripId: string | number, data: BudgetItemCreateInput, writer: TripWriter) { + await requireTripWrite( + { + access: { findAccessible: (id, userId) => this.budget.verifyTripAccess(id, userId) }, + permissions: this.permissions, + }, + 'budget_edit', + tripId, + writer, + ); + const refusal = await this.budget.linkRefusal(tripId, data); + if (refusal) throw new DomainError(400, refusal); + const item = await this.budget.create(String(tripId), data); + if (item.reservation_id) await this.budget.resyncReservationPrice(tripId, item.reservation_id, writer.socketId); + writer.events.emit('budget:created', { item }); + return item; + } +} diff --git a/server/src/nest/budget/budget.controller.ts b/server/src/nest/budget/budget.controller.ts index 370576c42..ec9f5feb5 100644 --- a/server/src/nest/budget/budget.controller.ts +++ b/server/src/nest/budget/budget.controller.ts @@ -5,6 +5,7 @@ import { JwtAuthGuard } from '../auth-core/jwt-auth.guard'; import { toRowId } from '../common/row-id'; import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; import { Trip } from '../permissions/trip.decorator'; +import { BudgetWritesService } from './budget-writes.service'; import { BudgetCreateItemDto, BudgetUpdateItemDto, @@ -57,7 +58,10 @@ import { // passes, so the HTTP and MCP paths cannot demand different rights. @UseGuards(JwtAuthGuard, TripAccessGuard) export class BudgetController { - constructor(private readonly budget: BudgetService) {} + constructor( + private readonly budget: BudgetService, + private readonly writes: BudgetWritesService, + ) {} @Get() async list(@CurrentUser() user: User, @Param('tripId') tripId: string) { @@ -207,12 +211,9 @@ export class BudgetController { @Body() body: BudgetCreateItemDto, @Headers('x-socket-id') socketId?: string, ) { - const refusal = await this.budget.linkRefusal(tripId, body); - if (refusal) throw new HttpException({ error: refusal }, 400); - const item = await this.budget.create(tripId, body); - // A booking mirrors the total of its expenses (#2084); a new one adds to it. - if (item.reservation_id) await this.budget.resyncReservationPrice(tripId, item.reservation_id, socketId); - this.budget.broadcast(tripId, 'budget:created', { item }, socketId); + // The use case answers the link refusal (#2084) and works out the price of a + // linked booking, the way the MCP tool adding an expense with its split does. + const item = await this.writes.createItem(tripId, body, this.writes.restWriter(tripId, user, socketId)); return { item }; } diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index 197eb4629..229b732d4 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -19,9 +19,10 @@ import { import { addonGate } from '../addons/addon-gate'; import { AddonsService } from '../addons/addons.service'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; -import { UnitOfWork } from '../database/unit-of-work'; +import { DomainError } from '../common/domain-error'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { TripMembershipService } from '../trip-membership/trip-membership.service'; +import { BudgetWritesService } from './budget-writes.service'; import { BudgetService } from './budget.service'; import { ExchangeRatesService } from './exchange-rates.service'; import { InjectRepository } from '@mikro-orm/nestjs'; @@ -97,7 +98,7 @@ export class BudgetMcp { private readonly membership: TripMembershipService, readonly addons: AddonsService, private readonly guards: McpToolGuardsService, - private readonly uow: UnitOfWork, + private readonly writes: BudgetWritesService, @InjectRepository(Places) private readonly places: PlacesRepository, @InjectRepository(Trips) private readonly trips: TripsRepository, ) {} @@ -512,33 +513,19 @@ export class BudgetMcp { }, ctx: McpContext, ) { + // Check access before the roster is read for the default split; createItem + // runs the full trip gate again, with the same refusal. if (!(await this.budget.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('budget_edit', tripId, ctx.userId))) return permissionDenied(); - if (place_id != null && !(await this.placeOnTrip(tripId, place_id))) - return errorResult('place_id does not belong to this trip.'); + const writer = await this.guards.tripWriter(tripId, ctx.userId); // Omitted userIds → default to the whole trip, matching create_budget_item. const members = userIds && userIds.length > 0 ? userIds : await this.resolveMemberIds(tripId, undefined); try { - const item = await this.uow.transactional(async () => { - const created = await this.budget.createBudgetItem(tripId, { - category, - name, - total_price, - note, - member_ids: members, - place_id, - }); - return (await this.budget.getBudgetItem(created.id, tripId))!; - }); - this.guards.safeBroadcast(tripId, 'budget:created', { item }); - if (members && members.length > 0) - this.guards.safeBroadcast(tripId, 'budget:members-updated', { - itemId: item.id, - members: item.members, - persons: item.persons, - }); - return ok({ item }); - } catch { + // The REST create's use case: the trip gate, the place check (#2084) and + // budget:created alone, since the item it sends already carries its members. + const data = { category, name, total_price, note, member_ids: members, place_id }; + return ok({ item: await this.writes.createItem(tripId, data, writer) }); + } catch (err) { + if (err instanceof DomainError) throw err; return errorResult('Failed to create budget item.'); } } diff --git a/server/src/nest/budget/budget.module.ts b/server/src/nest/budget/budget.module.ts index c29c2a7fe..42d5496a1 100644 --- a/server/src/nest/budget/budget.module.ts +++ b/server/src/nest/budget/budget.module.ts @@ -16,6 +16,7 @@ import { PermissionsModule } from '../permissions/permissions.module'; import { RealtimeModule } from '../realtime/realtime.module'; import { TripMembershipModule } from '../trip-membership/trip-membership.module'; import { BudgetItemsMcp } from './budget-items.mcp'; +import { BudgetWritesService } from './budget-writes.service'; import { BudgetController } from './budget.controller'; import { BudgetMcp } from './budget.mcp'; import { BudgetService } from './budget.service'; @@ -59,7 +60,15 @@ import { Module } from '@nestjs/common'; ]), ], controllers: [BudgetController], - providers: [BudgetService, ExchangeRatesService, BudgetMcp, BudgetItemsMcp, ExchangeRatesRpc, CostsRpc], + providers: [ + BudgetService, + BudgetWritesService, + ExchangeRatesService, + BudgetMcp, + BudgetItemsMcp, + ExchangeRatesRpc, + CostsRpc, + ], // For in-container consumers (CostsRpc, TripsService, // ReservationsService, BookingImportService). exports: [BudgetService, ExchangeRatesService], diff --git a/server/src/nest/packing/packing-writes.service.ts b/server/src/nest/packing/packing-writes.service.ts index 265b5383a..0bb8019b9 100644 --- a/server/src/nest/packing/packing-writes.service.ts +++ b/server/src/nest/packing/packing-writes.service.ts @@ -2,8 +2,9 @@ import type { User } from '../../types'; import { isUpdateConflict } from '../common/conflictResult'; import { DomainError } from '../common/domain-error'; import { requireTripWrite, type TripWriter } from '../common/trip-writer'; +import { MailerService } from '../notifications/mailer/mailer.service'; import { PermissionsService } from '../permissions/permissions.service'; -import { PackingService, isInvalidBagRef } from './packing.service'; +import { PackingService, isInvalidBagRef, type PackingVisibility } from './packing.service'; import { Injectable } from '@nestjs/common'; import type { TrekWsPayload } from '@trek/shared'; @@ -18,6 +19,12 @@ type PrivacyItem = { is_private?: number; owner_id?: number | null; recipients?: const itemNotFound = () => new DomainError(404, 'Item not found', { mcpMessage: 'Packing item not found.' }); // The payload named a bag that is not on this trip (#2154): 400, as on create. const bagNotFound = () => new DomainError(400, 'Bag not found', { mcpMessage: 'Bag not found.' }); +// The path named a bag that is not on this trip: 404, as every bag route answers it. +const bagMissing = () => new DomainError(404, 'Bag not found', { mcpMessage: 'Bag not found.' }); +const ownerOnly = () => + new DomainError(403, 'Only the owner can change sharing', { mcpMessage: 'Only the owner can change sharing.' }); +const templateMissing = () => + new DomainError(404, 'Template not found or empty', { mcpMessage: 'Template not found.' }); /** Writes that can move a bag's weight, the only ones worth a bag-totals ping (#2191). */ const WEIGHT_KEYS = ['weight_grams', 'quantity', 'bag_id']; @@ -36,12 +43,20 @@ const WEIGHT_KEYS = ['weight_grams', 'quantity', 'bag_id']; * (PackingService.broadcastUpdate), and its owner plus recipients on MCP. Which * of the two is right is a product decision; until it is made, each surface * keeps the routing it had. + * + * The list writes below the item ones (sharing, order, bag delete, category + * assignees, templates) came here where the two surfaces had drifted apart: + * the REST reorder told nobody, the MCP sharing change pinged bag totals it + * cannot move, the MCP bag delete reported success for a bag that was not + * there, the MCP assignee change told none of the tagged travellers, and a + * template applied over MCP always landed on the common list. */ @Injectable() export class PackingWritesService { constructor( private readonly packing: PackingService, private readonly permissions: PermissionsService, + private readonly mailer: MailerService, ) {} /** @@ -144,4 +159,79 @@ export class PackingWritesService { this.packing.broadcastBagTotals(String(tripId)); return deleted; } + + /** + * Move an item between the sharing tiers (#858). Only its owner may; an item + * nobody owns yet is claimed by the writer. Visibility moves no weight, so + * there is no bag-totals ping. + */ + async setSharing( + tripId: string | number, + itemId: number, + visibility: PackingVisibility, + recipientIds: number[], + writer: TripWriter, + ) { + await this.gate(tripId, writer); + const item = await this.packing.setItemSharing(tripId, itemId, writer.userId, visibility, recipientIds); + if (!item) throw itemNotFound(); + if ((item as { forbidden?: boolean }).forbidden) throw ownerOnly(); + // The viewer set just changed: off the whole room's screens first, then back + // to whoever may see it now (owner and recipients, or the room for Common). + writer.events.emit('packing:deleted', { itemId }); + writer.events.emit('packing:created', { item } as TrekWsPayload<'packing:created'>, this.packing.viewersOf(item)); + return item; + } + + /** The list's new order, for every other tab on both surfaces. */ + async reorder(tripId: string | number, orderedIds: number[], writer: TripWriter) { + await this.gate(tripId, writer); + await this.packing.reorderItems(tripId, orderedIds); + writer.events.emit('packing:reordered', { orderedIds }); + } + + /** Delete a bag; a bag that is not on the trip is refused and announced to nobody. */ + async deleteBag(tripId: string | number, bagId: number, writer: TripWriter) { + await this.gate(tripId, writer); + if (!(await this.packing.deleteBag(tripId, bagId))) throw bagMissing(); + writer.events.emit('packing:bag-deleted', { bagId }); + // bag_id is ON DELETE SET NULL, so everything that was in it just landed in + // the unassigned pile: both figures moved (#2191). + this.packing.broadcastBagTotals(String(tripId)); + } + + /** + * Who looks after a packing category. The tagged travellers are notified on + * every surface, the way the route always notified them. + */ + async setCategoryAssignees(tripId: string | number, category: string, userIds: number[], writer: TripWriter) { + await this.gate(tripId, writer); + const assignees = await this.packing.updateCategoryAssignees(tripId, category, userIds); + writer.events.emit('packing:assignees', { category, assignees }); + if (userIds.length > 0) { + // The notification names its actor by email. A writer carries only the id, + // so the address is read here; a guest never signs in, so never writes. + const email = await this.mailer.getUserEmail(writer.userId); + if (email) await this.packing.notifyTagged(String(tripId), { id: writer.userId, email }, category, userIds); + } + return assignees; + } + + /** + * Add a template's items to the list, on the common list or as the writer's + * own. Personal items reach their owner alone, common ones the room (#858). + */ + async applyTemplate( + tripId: string | number, + templateId: number, + visibility: 'common' | 'personal', + writer: TripWriter, + ) { + await this.gate(tripId, writer); + const items = await this.packing.applyTemplate(tripId, templateId, visibility, writer.userId); + if (!items) throw templateMissing(); + writer.events.emit('packing:template-applied', { items }, this.packing.viewersOf(items[0])); + this.packing.broadcastBagTotals(String(tripId)); + return items; + } } diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index 72f545199..ee5f96100 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -33,14 +33,6 @@ import { } from '@nestjs/common'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; -/** A packing item row carrying the privacy fields (#858) used to scope broadcasts. */ -type PackingItemRow = { - is_private?: number; - owner_id?: number | null; - recipients?: { user_id: number }[]; - [key: string]: unknown; -}; - /** * /api/trips/:tripId/packing — trip-scoped packing list (items, bags, templates, * assignees). @@ -132,9 +124,9 @@ export class PackingController { @CurrentUser() user: User, @Param('tripId') tripId: string, @Body() body: PackingReorderDto, - @Headers('x-socket-id') _socketId?: string, + @Headers('x-socket-id') socketId?: string, ) { - await this.packing.reorderItems(tripId, body.orderedIds); + await this.writes.reorder(tripId, body.orderedIds, this.writes.restWriter(tripId, user, socketId)); return { success: true }; } @@ -214,23 +206,14 @@ export class PackingController { if (itemId === null) { throw new HttpException({ error: 'Item not found' }, 404); } - const updated = await this.packing.setItemSharing( + // The use case answers the 404, the owner-only 403 and both events. + const updated = await this.writes.setSharing( tripId, itemId, - user.id, body.visibility, Array.isArray(body.recipient_ids) ? body.recipient_ids : [], + this.writes.restWriter(tripId, user, socketId), ); - if (!updated) { - throw new HttpException({ error: 'Item not found' }, 404); - } - if ((updated as { forbidden?: boolean }).forbidden) { - throw new HttpException({ error: 'Only the owner can change sharing' }, 403); - } - // The viewer set just changed: drop the item from the whole room, then re-add - // it for whoever can now see it (owner + recipients, or everyone if Common). - this.packing.broadcast(tripId, 'packing:deleted', { itemId }, socketId); - this.packing.emitToViewers(tripId, 'packing:created', { item: updated }, updated as PackingItemRow, socketId); return { item: updated }; } @@ -386,13 +369,8 @@ export class PackingController { if (bagIdNum === null) { throw new HttpException({ error: 'Bag not found' }, 404); } - if (!(await this.packing.deleteBag(tripId, bagIdNum))) { - throw new HttpException({ error: 'Bag not found' }, 404); - } - this.packing.broadcast(tripId, 'packing:bag-deleted', { bagId: bagIdNum }, socketId); - // bag_id is ON DELETE SET NULL, so everything that was in it just landed in - // the unassigned pile — both figures moved. - this.packing.broadcastBagTotals(tripId); + // The use case answers an unknown bag with the same 404 and sends nothing then. + await this.writes.deleteBag(tripId, bagIdNum, this.writes.restWriter(tripId, user, socketId)); return { success: true }; } @@ -414,18 +392,18 @@ export class PackingController { // Plan 4 Task 8b (U6) — :templateId is parsed ONCE here (toRowId, not // Number(): rule 15's NaN-into-SQL trap). A malformed id never matched // under the legacy affinity CAST either, so it 404s with the same body - // this handler's own not-found branch already produces below. + // the use case's own not-found refusal produces below. const templateIdNum = toRowId(templateId); if (templateIdNum === null) { throw new HttpException({ error: 'Template not found or empty' }, 404); } const visibility = body?.visibility === 'personal' ? 'personal' : 'common'; - const added = await this.packing.applyTemplate(tripId, templateIdNum, visibility, user.id); - if (!added) { - throw new HttpException({ error: 'Template not found or empty' }, 404); - } - this.packing.broadcastItem(tripId, 'packing:template-applied', { items: added }, added[0], socketId); - this.packing.broadcastBagTotals(tripId); + const added = await this.writes.applyTemplate( + tripId, + templateIdNum, + visibility, + this.writes.restWriter(tripId, user, socketId), + ); return { items: added, count: added.length }; } @@ -487,9 +465,12 @@ export class PackingController { @Headers('x-socket-id') socketId?: string, ) { const category = decodeURIComponent(categoryName); - const rows = await this.packing.updateCategoryAssignees(tripId, category, body.user_ids); - this.packing.broadcast(tripId, 'packing:assignees', { category, assignees: rows }, socketId); - await this.packing.notifyTagged(tripId, user, category, body.user_ids); + const rows = await this.writes.setCategoryAssignees( + tripId, + category, + body.user_ids, + this.writes.restWriter(tripId, user, socketId), + ); return { assignees: rows }; } } diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index c626cdc30..4ea575b05 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -18,6 +18,7 @@ import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { PackingWritesService } from './packing-writes.service'; import { PackingService } from './packing.service'; import { + packingApplyTemplateRequestSchema, packingCreateBagRequestSchema, packingCreateItemRequestSchema, packingSetSharingRequestSchema, @@ -277,16 +278,15 @@ export class PackingMcp { }: { tripId: number; itemId: number; visibility: PackingVisibility; recipient_ids?: number[] }, ctx: McpContext, ) { - if (!(await this.packing.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('packing_edit', tripId, ctx.userId))) return permissionDenied(); - const item = await this.packing.setItemSharing(tripId, itemId, ctx.userId, visibility, recipient_ids ?? []); - if (!item) return errorResult('Packing item not found.'); - if ((item as { forbidden?: boolean }).forbidden) return errorResult('Only the owner can change sharing.'); - // The viewer set just changed: drop the item from the whole room, then hand - // it back to whoever may see it now, as the REST route does. - this.guards.safeBroadcast(tripId, 'packing:deleted', { itemId }); - this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); - this.packing.broadcastBagTotals(String(tripId)); + // The use case sends what the REST route sends: no bag-totals ping, since + // who sees an item moves no weight. + const item = await this.writes.setSharing( + tripId, + itemId, + visibility, + recipient_ids ?? [], + await this.guards.tripWriter(tripId, ctx.userId), + ); return ok({ item }); } @@ -304,10 +304,7 @@ export class PackingMcp { access: { group: 'packing', mode: 'write' }, }) async reorderPackingItems({ tripId, orderedIds }: { tripId: number; orderedIds: number[] }, ctx: McpContext) { - if (!(await this.packing.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('packing_edit', tripId, ctx.userId))) return permissionDenied(); - await this.packing.reorderItems(tripId, orderedIds); - this.guards.safeBroadcast(tripId, 'packing:reordered', { orderedIds }); + await this.writes.reorder(tripId, orderedIds, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } @@ -446,15 +443,9 @@ export class PackingMcp { access: { group: 'packing', mode: 'write' }, }) async deletePackingBag({ tripId, bagId }: { tripId: number; bagId: number }, ctx: McpContext) { - if (!(await this.packing.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('packing_edit', tripId, ctx.userId))) return permissionDenied(); - await this.packing.deleteBag(tripId, bagId); - // { bagId } matches the REST route and the plugin host (the legacy - // registrar's { id } was the odd one out). - this.guards.safeBroadcast(tripId, 'packing:bag-deleted', { bagId }); - // packing_items.bag_id is ON DELETE SET NULL, so the contents land in the - // unassigned pile and both numbers move (#2191), as on REST and plugin RPC. - this.packing.broadcastBagTotals(String(tripId)); + // A bag that is not on the trip answers 'Bag not found.' and is announced to + // nobody, as on REST; it used to report success and send both events. + await this.writes.deleteBag(tripId, bagId, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } @@ -514,31 +505,43 @@ export class PackingMcp { { tripId, categoryName, userIds }: { tripId: number; categoryName: string; userIds: number[] }, ctx: McpContext, ) { - if (!(await this.packing.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('packing_edit', tripId, ctx.userId))) return permissionDenied(); - const assignees = await this.packing.updateCategoryAssignees(tripId, categoryName, userIds); - this.guards.safeBroadcast(tripId, 'packing:assignees', { category: categoryName, assignees }); + // The tagged travellers get the same notification the REST route sends them. + const assignees = await this.writes.setCategoryAssignees( + tripId, + categoryName, + userIds, + await this.guards.tripWriter(tripId, ctx.userId), + ); return ok({ assignees }); } @Tool({ name: 'apply_packing_template', - description: 'Apply a packing template to a trip (adds items from the template).', + description: + "Apply a packing template to a trip (adds items from the template), to the common list or as the caller's own items.", inputSchema: { tripId: idSchema, templateId: idSchema, + visibility: packingApplyTemplateRequestSchema + .unwrap() + .shape.visibility.describe( + "'common' (the default) adds the items to the list everyone shares, 'personal' to the caller's own list", + ), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, when: packingAddonOn, access: { group: 'packing', mode: 'write' }, }) - async applyPackingTemplate({ tripId, templateId }: { tripId: number; templateId: number }, ctx: McpContext) { - if (!(await this.packing.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('packing_edit', tripId, ctx.userId))) return permissionDenied(); - const items = await this.packing.applyTemplate(tripId, templateId); - if (items === null) return errorResult('Template not found.'); - this.guards.safeBroadcast(tripId, 'packing:template-applied', { items }); - this.packing.broadcastBagTotals(String(tripId)); + async applyPackingTemplate( + { tripId, templateId, visibility }: { tripId: number; templateId: number; visibility?: 'common' | 'personal' }, + ctx: McpContext, + ) { + const items = await this.writes.applyTemplate( + tripId, + templateId, + visibility ?? 'common', + await this.guards.tripWriter(tripId, ctx.userId), + ); return ok({ items, count: items.length }); } diff --git a/server/src/nest/packing/packing.module.ts b/server/src/nest/packing/packing.module.ts index de7ed1d33..89d33cbe0 100644 --- a/server/src/nest/packing/packing.module.ts +++ b/server/src/nest/packing/packing.module.ts @@ -11,6 +11,7 @@ import { PluginGuardsModule } from '../../nest-rpc/plugin-guards.module'; import { AddonsModule } from '../addons/addons.module'; import { AuditModule } from '../audit/audit.module'; import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; +import { MailerModule } from '../notifications/mailer/mailer.module'; import { NotificationsModule } from '../notifications/notifications.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { RealtimeModule } from '../realtime/realtime.module'; @@ -45,6 +46,8 @@ import { Module } from '@nestjs/common'; ]), McpSharedModule, NotificationsModule, + // The actor's email for the category assignee notification (PackingWritesService). + MailerModule, PermissionsModule, RealtimeModule, PluginGuardsModule, diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index f158c4cda..1889be4a1 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -891,7 +891,7 @@ export class PackingService { } /** Fire-and-forget tag notification, mirroring the legacy dynamic import. */ - async notifyTagged(tripId: string, actor: User, category: string, userIds: unknown): Promise { + async notifyTagged(tripId: string, actor: Pick, category: string, userIds: unknown): Promise { if (!Array.isArray(userIds) || userIds.length === 0) return; // Injected, not a lazy import of the old notifications bridge. The laziness bought // nothing the module graph does not already give — NotificationsModule diff --git a/server/src/nest/todo/todo.controller.ts b/server/src/nest/todo/todo.controller.ts index 797af4457..51def983a 100644 --- a/server/src/nest/todo/todo.controller.ts +++ b/server/src/nest/todo/todo.controller.ts @@ -65,8 +65,13 @@ export class TodoController { @RequirePermission('packing_edit') @Put('reorder') @ResponseContract(successResponseSchema) - async reorder(@CurrentUser() user: User, @Param('tripId') tripId: string, @Body() body: TodoReorderDto) { - await this.todo.reorderItems(tripId, body.orderedIds); + async reorder( + @CurrentUser() user: User, + @Param('tripId') tripId: string, + @Body() body: TodoReorderDto, + @Headers('x-socket-id') socketId?: string, + ) { + await this.todo.reorder(tripId, body.orderedIds, this.todo.restWriter(tripId, user, socketId)); return { success: true }; } diff --git a/server/src/nest/todo/todo.mcp.ts b/server/src/nest/todo/todo.mcp.ts index 68d123a21..73c3c9fdb 100644 --- a/server/src/nest/todo/todo.mcp.ts +++ b/server/src/nest/todo/todo.mcp.ts @@ -245,9 +245,7 @@ export class TodoMcp { access: { group: 'todos', mode: 'write' }, }) async reorderTodos({ tripId, orderedIds }: { tripId: number; orderedIds: number[] }, ctx: McpContext) { - if (!(await this.todos.verifyTripAccess(tripId, ctx.userId))) return noAccess(); - if (!(await this.guards.hasTripPermission('packing_edit', tripId, ctx.userId))) return permissionDenied(); - await this.todos.reorderItems(tripId, orderedIds); + await this.todos.reorder(tripId, orderedIds, await this.guards.tripWriter(tripId, ctx.userId)); return ok({ success: true }); } diff --git a/server/src/nest/todo/todo.service.ts b/server/src/nest/todo/todo.service.ts index c51f6c8e4..00d02278d 100644 --- a/server/src/nest/todo/todo.service.ts +++ b/server/src/nest/todo/todo.service.ts @@ -6,6 +6,7 @@ import type { TodoItemsRepository } from '../../db/repositories/TodoItems.reposi import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; import type { TripAccess } from '../../db/repositories/Trips.repository'; import type { User } from '../../types'; +import { requireTripWrite, 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'; @@ -157,6 +158,21 @@ export class TodoService { }); } + /** The REST caller of a todo use case: its events skip the socket that sent the request. */ + restWriter(tripId: string, user: User, socketId: string | undefined): TripWriter { + return restTripWriter(this.realtime, tripId, user, socketId); + } + + /** + * The list's new order, for every surface. Both used to save it and tell + * nobody, so another open list kept the old order until a reload. + */ + async reorder(tripId: string | number, orderedIds: number[], writer: TripWriter): Promise { + await requireTripWrite({ access: this.tripsRepo, permissions: this.permissions }, 'packing_edit', tripId, writer); + await this.reorderItems(tripId, orderedIds); + writer.events.emit('todo:reordered', { orderedIds }); + } + async getCategoryAssignees(tripId: string | number) { const rows = await this.todoCategoryAssigneesRepo.listForTrip(tripId); diff --git a/server/tests/integration/broadcast-parity/money.cases.ts b/server/tests/integration/broadcast-parity/money.cases.ts index 5cdd05f73..c4dc96738 100644 --- a/server/tests/integration/broadcast-parity/money.cases.ts +++ b/server/tests/integration/broadcast-parity/money.cases.ts @@ -1,14 +1,312 @@ +import { BudgetItems } from '../../../src/db/entities/BudgetItems.entity'; +import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; +import { updateRows } from '../../helpers/factories/rows'; import { addTripMember } from '../../helpers/factories/trips'; -import type { CaseGroup } from './types'; +import type { CaseGroup, Scene } from './types'; import { MikroORM } from '@mikro-orm/core'; +import { vi } from 'vitest'; + const TEMPLATES = "packing templates are the admin's global list: no trip room, and no screen shows them live"; +const budget = (s: Scene, rest = '') => `/api/trips/${s.trip.id}/budget${rest}`; +const packing = (s: Scene, rest = '') => `/api/trips/${s.trip.id}/packing${rest}`; +const todo = (s: Scene, rest = '') => `/api/trips/${s.trip.id}/todo${rest}`; + +/** The weight of every bag may have moved: every socket re-reads the totals, the writing one too. */ +const BAG_TOTALS = { event: 'packing:bag-totals', all: true } as const; + +/** A settle-up payment from the member to the owner, as both surfaces take it. */ +const payment = (s: Scene, amount: number) => ({ from_user_id: s.member.id, to_user_id: s.owner.id, amount }); + /** Money and lists: the budget with its settlements, packing and to-dos. */ export const money: CaseGroup = { name: 'money', prefix: 'BCP-MONEY', - cases: [], + cases: [ + // ── budget ──────────────────────────────────────────────────────────────── + { + tool: 'create_budget_item', + args: (s) => ({ tripId: s.trip.id, name: `Dinner ${s.n}`, total_price: 60 }), + rest: { method: 'post', path: (s) => budget(s), body: (s) => ({ name: `Dinner ${s.n}`, total_price: 60 }) }, + expect: ['budget:created'], + }, + { + tool: 'update_budget_item', + needs: ['budgetItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.budgetItem.id, name: `Lunch ${s.n}` }), + rest: { + method: 'put', + path: (s) => budget(s, `/${s.budgetItem.id}`), + body: (s) => ({ name: `Lunch ${s.n}` }), + }, + expect: ['budget:updated'], + }, + { + tool: 'delete_budget_item', + needs: ['budgetItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.budgetItem.id }), + rest: { method: 'delete', path: (s) => budget(s, `/${s.budgetItem.id}`) }, + expect: ['budget:deleted'], + }, + { + // The same write as POST /budget with member_ids, through the same use case: + // the item already carries its split, so no members-updated follows (G18). + tool: 'create_budget_item_with_members', + needs: ['member'], + args: (s) => ({ tripId: s.trip.id, name: `Taxi ${s.n}`, total_price: 40, userIds: [s.owner.id, s.member.id] }), + rest: { + method: 'post', + path: (s) => budget(s), + body: (s) => ({ name: `Taxi ${s.n}`, total_price: 40, member_ids: [s.owner.id, s.member.id] }), + }, + expect: ['budget:created'], + }, + { + tool: 'set_budget_item_members', + needs: ['budgetItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.budgetItem.id, userIds: [s.owner.id] }), + rest: { + method: 'put', + path: (s) => budget(s, `/${s.budgetItem.id}/members`), + body: (s) => ({ user_ids: [s.owner.id] }), + }, + expect: ['budget:members-updated'], + }, + { + tool: 'toggle_budget_member_paid', + needs: ['budgetItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.budgetItem.id, memberId: s.member.id, paid: true }), + rest: { + method: 'put', + path: (s) => budget(s, `/${s.budgetItem.id}/members/${s.member.id}/paid`), + body: () => ({ paid: true }), + }, + expect: ['budget:member-paid-updated'], + }, + { + tool: 'create_settlement', + needs: ['member'], + args: (s) => ({ tripId: s.trip.id, ...payment(s, 20) }), + rest: { method: 'post', path: (s) => budget(s, '/settlements'), body: (s) => payment(s, 20) }, + expect: ['budget:settlement-created'], + }, + { + tool: 'update_settlement', + needs: ['settlement'], + args: (s) => ({ tripId: s.trip.id, settlementId: s.settlement.id, ...payment(s, 25) }), + rest: { + method: 'put', + path: (s) => budget(s, `/settlements/${s.settlement.id}`), + body: (s) => payment(s, 25), + }, + expect: ['budget:settlement-updated'], + }, + { + tool: 'delete_settlement', + needs: ['settlement'], + args: (s) => ({ tripId: s.trip.id, settlementId: s.settlement.id }), + rest: { method: 'delete', path: (s) => budget(s, `/settlements/${s.settlement.id}`) }, + expect: ['budget:settlement-deleted'], + }, + { + // The expense in dollars has no rate frozen yet (1 reads as unfrozen); the + // server's quote is stubbed, so freezing it is the one change to announce. + tool: 'freeze_budget_rates', + needs: ['budgetItem'], + setup: async (s, app) => { + await updateRows( + app.get(MikroORM), + BudgetItems, + { id: s.budgetItem.id }, + { currency: 'USD', exchange_rate: 1 }, + ); + }, + stub: (app) => { + const rates = vi.spyOn(app.get(ExchangeRatesService), 'getRates').mockResolvedValue({ USD: 1.1 }); + return () => rates.mockRestore(); + }, + args: (s) => ({ tripId: s.trip.id }), + rest: { method: 'post', path: (s) => budget(s, '/freeze-rates'), body: () => ({}) }, + expect: ['budget:updated'], + }, + + // ── packing ─────────────────────────────────────────────────────────────── + { + tool: 'create_packing_item', + args: (s) => ({ tripId: s.trip.id, name: `Socks ${s.n}` }), + rest: { method: 'post', path: (s) => packing(s), body: (s) => ({ name: `Socks ${s.n}` }) }, + expect: ['packing:created', BAG_TOTALS], + }, + { + // Ticking an item off moves no weight, so no bag-totals ping (#2191). + tool: 'toggle_packing_item', + needs: ['packingItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.packingItem.id, checked: true }), + rest: { method: 'put', path: (s) => packing(s, `/${s.packingItem.id}`), body: () => ({ checked: true }) }, + expect: ['packing:updated'], + }, + { + tool: 'update_packing_item', + needs: ['packingItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.packingItem.id, name: `Wool socks ${s.n}` }), + rest: { + method: 'put', + path: (s) => packing(s, `/${s.packingItem.id}`), + body: (s) => ({ name: `Wool socks ${s.n}` }), + }, + expect: ['packing:updated'], + }, + { + tool: 'delete_packing_item', + needs: ['packingItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.packingItem.id }), + rest: { method: 'delete', path: (s) => packing(s, `/${s.packingItem.id}`) }, + expect: ['packing:deleted', BAG_TOTALS], + }, + { + // Off the whole room's screens, then back to whoever may see it now: the + // writer alone for a personal item. Visibility moves no weight, so the + // bag-totals ping the tool used to add is gone (G18). + tool: 'set_packing_item_sharing', + needs: ['packingItem'], + args: (s) => ({ tripId: s.trip.id, itemId: s.packingItem.id, visibility: 'personal' }), + rest: { + method: 'put', + path: (s) => packing(s, `/${s.packingItem.id}/sharing`), + body: () => ({ visibility: 'personal' }), + }, + expect: ['packing:deleted', { event: 'packing:created', only: 'viewers' }], + }, + { + // The route used to save the order and tell nobody (G14). + tool: 'reorder_packing_items', + needs: ['packingItem'], + args: (s) => ({ tripId: s.trip.id, orderedIds: [s.packingItem.id] }), + rest: { method: 'put', path: (s) => packing(s, '/reorder'), body: (s) => ({ orderedIds: [s.packingItem.id] }) }, + expect: ['packing:reordered'], + }, + { + tool: 'bulk_import_packing', + args: (s) => ({ tripId: s.trip.id, items: [{ name: `Tent ${s.n}` }] }), + rest: { method: 'post', path: (s) => packing(s, '/import'), body: (s) => ({ items: [{ name: `Tent ${s.n}` }] }) }, + expect: ['packing:created', BAG_TOTALS], + }, + { + tool: 'create_packing_bag', + args: (s) => ({ tripId: s.trip.id, name: `Carry-on ${s.n}` }), + rest: { method: 'post', path: (s) => packing(s, '/bags'), body: (s) => ({ name: `Carry-on ${s.n}` }) }, + expect: ['packing:bag-created'], + }, + { + tool: 'update_packing_bag', + needs: ['bag'], + args: (s) => ({ tripId: s.trip.id, bagId: s.bag.id, name: `Duffel ${s.n}` }), + rest: { method: 'put', path: (s) => packing(s, `/bags/${s.bag.id}`), body: (s) => ({ name: `Duffel ${s.n}` }) }, + expect: ['packing:bag-updated'], + }, + { + // Its items fall into the unassigned pile, so the totals move too. The G17 + // refusal (a bag from another trip sends nothing) is proven by PACK-LIST-004 + // and the delete_packing_bag test in tools-packing-advanced, not here. + tool: 'delete_packing_bag', + needs: ['bag'], + args: (s) => ({ tripId: s.trip.id, bagId: s.bag.id }), + rest: { method: 'delete', path: (s) => packing(s, `/bags/${s.bag.id}`) }, + expect: ['packing:bag-deleted', BAG_TOTALS], + }, + { + tool: 'set_bag_members', + needs: ['bag', 'member'], + args: (s) => ({ tripId: s.trip.id, bagId: s.bag.id, userIds: [s.member.id] }), + rest: { + method: 'put', + path: (s) => packing(s, `/bags/${s.bag.id}/members`), + body: (s) => ({ user_ids: [s.member.id] }), + }, + expect: ['packing:bag-members-updated'], + }, + { + // The tagged member hears of it on both surfaces; the MCP tool used to tell + // nobody (G23). The notification is sent without waiting on it, so it is + // read one macrotask after the write answered. + tool: 'set_packing_category_assignees', + needs: ['member'], + args: (s) => ({ tripId: s.trip.id, categoryName: 'Clothes', userIds: [s.member.id] }), + rest: { + method: 'put', + path: (s) => packing(s, '/category-assignees/Clothes'), + body: (s) => ({ user_ids: [s.member.id] }), + }, + settle: true, + expect: ['packing:assignees', { event: 'notification:new', user: 'others' }], + }, + { + // Applied as the writer's own items, which reach the writer alone: the MCP + // tool takes the visibility the route has taken since #1565 (G9). + tool: 'apply_packing_template', + needs: ['template'], + args: (s) => ({ tripId: s.trip.id, templateId: s.template.id, visibility: 'personal' }), + rest: { + method: 'post', + path: (s) => packing(s, `/apply-template/${s.template.id}`), + body: () => ({ visibility: 'personal' }), + }, + expect: [{ event: 'packing:template-applied', only: 'actor' }, BAG_TOTALS], + }, + + // ── to-dos ──────────────────────────────────────────────────────────────── + { + tool: 'create_todo', + args: (s) => ({ tripId: s.trip.id, name: `Book museum ${s.n}` }), + rest: { method: 'post', path: (s) => todo(s), body: (s) => ({ name: `Book museum ${s.n}` }) }, + expect: ['todo:created'], + }, + { + tool: 'update_todo', + needs: ['todo'], + args: (s) => ({ tripId: s.trip.id, itemId: s.todo.id, name: `Book the Louvre ${s.n}` }), + rest: { + method: 'put', + path: (s) => todo(s, `/${s.todo.id}`), + body: (s) => ({ name: `Book the Louvre ${s.n}` }), + }, + expect: ['todo:updated'], + }, + { + tool: 'toggle_todo', + needs: ['todo'], + args: (s) => ({ tripId: s.trip.id, itemId: s.todo.id, checked: true }), + rest: { method: 'put', path: (s) => todo(s, `/${s.todo.id}`), body: () => ({ checked: true }) }, + expect: ['todo:updated'], + }, + { + tool: 'delete_todo', + needs: ['todo'], + args: (s) => ({ tripId: s.trip.id, itemId: s.todo.id }), + rest: { method: 'delete', path: (s) => todo(s, `/${s.todo.id}`) }, + expect: ['todo:deleted'], + }, + { + // Both surfaces used to save the order and tell nobody (G16). + tool: 'reorder_todos', + needs: ['todo'], + args: (s) => ({ tripId: s.trip.id, orderedIds: [s.todo.id] }), + rest: { method: 'put', path: (s) => todo(s, '/reorder'), body: (s) => ({ orderedIds: [s.todo.id] }) }, + expect: ['todo:reordered'], + }, + { + tool: 'set_todo_category_assignees', + needs: ['member'], + args: (s) => ({ tripId: s.trip.id, categoryName: 'Booking', userIds: [s.member.id] }), + rest: { + method: 'put', + path: (s) => todo(s, '/category-assignees/Booking'), + body: (s) => ({ user_ids: [s.member.id] }), + }, + expect: ['todo:assignees'], + }, + ], exempt: [ { tool: 'save_packing_template', @@ -27,35 +325,4 @@ export const money: CaseGroup = { args: (s) => ({ templateId: s.template.id }), }, ], - pending: [ - 'create_budget_item', - 'delete_budget_item', - 'update_budget_item', - 'create_budget_item_with_members', - 'set_budget_item_members', - 'toggle_budget_member_paid', - 'create_settlement', - 'update_settlement', - 'delete_settlement', - 'freeze_budget_rates', - 'create_packing_item', - 'toggle_packing_item', - 'delete_packing_item', - 'update_packing_item', - 'set_packing_item_sharing', - 'reorder_packing_items', - 'create_packing_bag', - 'update_packing_bag', - 'delete_packing_bag', - 'set_bag_members', - 'set_packing_category_assignees', - 'apply_packing_template', - 'bulk_import_packing', - 'create_todo', - 'update_todo', - 'toggle_todo', - 'delete_todo', - 'reorder_todos', - 'set_todo_category_assignees', - ], }; diff --git a/server/tests/unit/mcp/tools-budget-advanced.test.ts b/server/tests/unit/mcp/tools-budget-advanced.test.ts index 10b02347e..2be87d63a 100644 --- a/server/tests/unit/mcp/tools-budget-advanced.test.ts +++ b/server/tests/unit/mcp/tools-budget-advanced.test.ts @@ -9,6 +9,7 @@ import { BudgetItems } from '../../../src/db/entities/BudgetItems.entity'; import { BudgetSettlements } from '../../../src/db/entities/BudgetSettlements.entity'; import { Trips } from '../../../src/db/entities/Trips.entity'; import type { TripAccess } from '../../../src/db/repositories/Trips.repository'; +import type { BudgetWritesService } from '../../../src/nest/budget/budget-writes.service'; import { BudgetController } from '../../../src/nest/budget/budget.controller'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; @@ -187,6 +188,24 @@ describe('Tool: create_budget_item_with_members', () => { const data = parseToolResult(result) as any; expect(data.item.members.map((m: any) => m.user_id).sort()).toEqual([user.id, member.id].sort()); expect(data.item.persons).toBe(2); + // What POST /budget sends for the same write: budget:created alone, its item + // already carrying the split (the members-updated that followed is gone). + expect(broadcastMock.mock.calls).toEqual([[trip.id, 'budget:created', { item: data.item, _source: 'mcp' }]]); + }); + }); + + it('refuses a trip the caller cannot see with the trip gate, and sends nothing', async () => { + const { user } = createUser(testDb); + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_budget_item_with_members', + arguments: { tripId: trip.id, name: 'Villa', total_price: 800 }, + }); + expect(result.isError).toBe(true); + expect((result.content as { text: string }[])[0].text).toBe('Trip not found or access denied.'); + expect(broadcastMock).not.toHaveBeenCalled(); }); }); @@ -287,6 +306,8 @@ describe('Settlement tools', () => { await createTestUnitOfWork(testDb), ...(await budgetRepoArgs(testDb)), ), + // The settlement route reads only; it never reaches the write use cases. + {} as BudgetWritesService, ); } diff --git a/server/tests/unit/mcp/tools-packing-advanced.test.ts b/server/tests/unit/mcp/tools-packing-advanced.test.ts index 47f4b6c07..96b2c9625 100644 --- a/server/tests/unit/mcp/tools-packing-advanced.test.ts +++ b/server/tests/unit/mcp/tools-packing-advanced.test.ts @@ -7,13 +7,14 @@ * live in tools-packing.test.ts. */ import { db as testDb } from '../../../src/db/database'; +import { Notifications } from '../../../src/db/entities/Notifications.entity'; import { PackingBags } from '../../../src/db/entities/PackingBags.entity'; import { PackingCategoryAssignees } from '../../../src/db/entities/PackingCategoryAssignees.entity'; import { PackingItems } from '../../../src/db/entities/PackingItems.entity'; import { PackingTemplateCategories } from '../../../src/db/entities/PackingTemplateCategories.entity'; import { PackingTemplateItems } from '../../../src/db/entities/PackingTemplateItems.entity'; import { PackingTemplates } from '../../../src/db/entities/PackingTemplates.entity'; -import { createUser, createAdmin, createTrip, createPackingItem } from '../../helpers/factories'; +import { addTripMember, createUser, createAdmin, createTrip, createPackingItem } from '../../helpers/factories'; import { addPackingBagMembers, makePackingBag, makePackingItem } from '../../helpers/factories/packing'; import { countRows, findRow, insertRow } from '../../helpers/factories/rows'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; @@ -285,6 +286,24 @@ describe('Tool: delete_packing_bag', () => { expect(result.isError).toBe(true); }); }); + + // It used to report success and send both events for a bag that was not there. + it('answers a bag that is not on the trip as not found, like REST, and sends nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const { id: bagId } = await makePackingBag(orm, otherTrip.id, { name: 'Elsewhere', color: '#000000' }); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'delete_packing_bag', + arguments: { tripId: trip.id, bagId }, + }); + expect(result.isError).toBe(true); + expect((result.content as { text: string }[])[0].text).toBe('Bag not found.'); + expect(broadcastMock).not.toHaveBeenCalled(); + expect(await findRow(orm, PackingBags, { id: bagId })).not.toBeNull(); + }); + }); }); // --------------------------------------------------------------------------- @@ -371,6 +390,26 @@ describe('Tool: set_packing_category_assignees', () => { }); }); + // The REST route always notified the tagged travellers; the tool now does too. + it('notifies the fellow traveller it tags, as the REST route does', async () => { + const { user } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, user.id); + addTripMember(testDb, trip.id, member.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'set_packing_category_assignees', + arguments: { tripId: trip.id, categoryName: 'Clothing', userIds: [member.id] }, + }); + expect(result.isError).toBeFalsy(); + // The send is fire and forget: it lands one turn of the event loop later. + await new Promise((resolve) => setImmediate(resolve)); + const sent = await findRow(orm, Notifications, { recipient: member.id }); + expect(sent?.text_key).toBe('notif.packing_tagged.text'); + expect(JSON.parse(String(sent?.text_params))).toMatchObject({ actor: user.email, category: 'Clothing' }); + }); + }); + it('clears assignees when passed empty array', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -435,6 +474,37 @@ describe('Tool: apply_packing_template', () => { arguments: { tripId: trip.id, templateId: 99999 }, }); expect(result.isError).toBe(true); + expect((result.content as { text: string }[])[0].text).toBe('Template not found.'); + expect(broadcastMock).not.toHaveBeenCalled(); + }); + }); + + // It always landed on the common list; REST has taken the visibility since #1565. + it("takes the visibility like REST: personal items are the caller's own and reach the caller alone", async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const templateId = await insertRow(orm, PackingTemplates, { name: 'Diving', createdByRef: user.id }); + const categoryId = await insertRow(orm, PackingTemplateCategories, { + template: templateId, + name: 'Gear', + sort_order: 0, + }); + await insertRow(orm, PackingTemplateItems, { category: categoryId, name: 'Mask', sort_order: 0 }); + + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'apply_packing_template', + arguments: { tripId: trip.id, templateId, visibility: 'personal' }, + }); + const data = parseToolResult(result) as { items: { is_private: number; owner_id: number }[] }; + expect(data.items).toEqual([expect.objectContaining({ is_private: 1, owner_id: user.id })]); + expect(broadcastMock).toHaveBeenCalledWith( + trip.id, + 'packing:template-applied', + expect.objectContaining({ items: data.items }), + undefined, + user.id, + ); }); }); }); diff --git a/server/tests/unit/mcp/tools-packing.test.ts b/server/tests/unit/mcp/tools-packing.test.ts index f9742d895..bbac74bd7 100644 --- a/server/tests/unit/mcp/tools-packing.test.ts +++ b/server/tests/unit/mcp/tools-packing.test.ts @@ -589,6 +589,9 @@ describe('Tool: set_packing_item_sharing', () => { }); expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:deleted', expect.any(Object)); expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:created', expect.any(Object), undefined, user.id); + // Exactly what the REST route sends: who sees an item moves no bag weight, + // so the bag-totals ping the tool used to add is gone. + expect(broadcastMock.mock.calls.map((c) => c[1])).toEqual(['packing:deleted', 'packing:created']); }); it('lets only the owner change sharing', async () => { diff --git a/server/tests/unit/mcp/tools-todos.test.ts b/server/tests/unit/mcp/tools-todos.test.ts index e315dc2da..cd7a4be33 100644 --- a/server/tests/unit/mcp/tools-todos.test.ts +++ b/server/tests/unit/mcp/tools-todos.test.ts @@ -367,6 +367,11 @@ describe('Tool: reorder_todos', () => { // item2 should now have sort_order 0 const updated = await findRow(orm, TodoItems, { id: item2.id }); expect(updated?.sort_order).toBe(0); + // Every open list, the asking tab's too, takes the new order live. + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'todo:reordered', { + orderedIds: [item2.id, item1.id], + _source: 'mcp', + }); }); }); diff --git a/server/tests/unit/nest/budget-items.mcp.test.ts b/server/tests/unit/nest/budget-items.mcp.test.ts index 59657575c..ef803e9b6 100644 --- a/server/tests/unit/nest/budget-items.mcp.test.ts +++ b/server/tests/unit/nest/budget-items.mcp.test.ts @@ -11,7 +11,7 @@ import { BudgetItems } from '../../../src/db/entities/BudgetItems.entity'; import type { McpContext } from '../../../src/nest-mcp'; import { getEntry, type ClassRef } from '../../../src/nest-mcp/metadata'; import { AddonsService } from '../../../src/nest/addons/addons.service'; -import { toolOutputForProvider } from '../../../src/nest/assistant/assistant-redact'; +import { toolOutputForProvider } from '../../../src/nest/assistant/assistant-output'; import { BudgetItemsMcp } from '../../../src/nest/budget/budget-items.mcp'; import { BudgetModule } from '../../../src/nest/budget/budget.module'; import { BudgetService } from '../../../src/nest/budget/budget.service'; diff --git a/server/tests/unit/nest/budget.controller.test.ts b/server/tests/unit/nest/budget.controller.test.ts index 0ad7f3d22..24282bf62 100644 --- a/server/tests/unit/nest/budget.controller.test.ts +++ b/server/tests/unit/nest/budget.controller.test.ts @@ -1,15 +1,44 @@ -import { BudgetController } from '../../../src/nest/budget/budget.controller'; -import type { BudgetService } from '../../../src/nest/budget/budget.service'; +import { BudgetWritesService } from '../../../src/nest/budget/budget-writes.service'; +import { BudgetController as RouteBudgetController } from '../../../src/nest/budget/budget.controller'; +import { BudgetService } from '../../../src/nest/budget/budget.service'; +import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { TRIP_PERMISSION_KEY } from '../../../src/nest/permissions/trip-access.guard'; +import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import type { User } from '../../../src/types'; import { HttpException } from '@nestjs/common'; import { HTTP_CODE_METADATA } from '@nestjs/common/constants'; +import { Test } from '@nestjs/testing'; import { describe, it, expect, vi } from 'vitest'; const user = { id: 1, role: 'user', email: 'u@example.test' } as User; const trip = { id: 5, user_id: 1 }; +/** Every route but the create, over the service mock alone. */ +class BudgetController extends RouteBudgetController { + constructor(svc: BudgetService) { + super(svc, {} as BudgetWritesService); + } +} + +/** + * The create route over the real use case (BudgetWritesService), built by Nest + * over the service mock with the permission granted: the 403 lives in + * trip-access.guard.test.ts. Its events leave through the returned realtime. + */ +async function createRoute(svc: BudgetService) { + const realtime = { broadcast: vi.fn() }; + const moduleRef = await Test.createTestingModule({ + providers: [ + BudgetWritesService, + { provide: BudgetService, useValue: svc }, + { provide: PermissionsService, useValue: { checkPermission: vi.fn().mockResolvedValue(true) } }, + { provide: RealtimeService, useValue: realtime }, + ], + }).compile(); + return { route: new RouteBudgetController(svc, moduleRef.get(BudgetWritesService)), realtime }; +} + function makeService(overrides: Partial = {}): BudgetService { return { verifyTripAccess: vi.fn().mockReturnValue(trip), @@ -281,15 +310,19 @@ describe('BudgetController (parity with the legacy /api/trips/:tripId/budget rou it('creates and broadcasts', async () => { const create = vi.fn().mockReturnValue({ id: 9, name: 'Hotel' }); - const broadcast = vi.fn(); const linkRefusal = vi.fn().mockReturnValue(null); const resyncReservationPrice = vi.fn(); - const svc = makeService({ create, broadcast, linkRefusal, resyncReservationPrice } as Partial); - expect(await new BudgetController(svc).create(user, '5', { name: 'Hotel', total_price: 200 }, 'sock')).toEqual({ + const svc = makeService({ create, linkRefusal, resyncReservationPrice } as Partial); + const { route, realtime } = await createRoute(svc); + expect(await route.create(user, '5', { name: 'Hotel', total_price: 200 }, 'sock')).toEqual({ item: { id: 9, name: 'Hotel' }, }); expect(linkRefusal).toHaveBeenCalledWith('5', { name: 'Hotel', total_price: 200 }); - expect(broadcast).toHaveBeenCalledWith('5', 'budget:created', { item: { id: 9, name: 'Hotel' } }, 'sock'); + // budget:created alone, skipping the tab that created it: no members-updated, + // since the item already carries its split. + expect(realtime.broadcast.mock.calls).toEqual([ + ['5', 'budget:created', { item: { id: 9, name: 'Hotel' } }, 'sock'], + ]); // Not linked to a booking, so no booking price to work out. expect(resyncReservationPrice).not.toHaveBeenCalled(); }); @@ -297,41 +330,35 @@ describe('BudgetController (parity with the legacy /api/trips/:tripId/budget rou it('an expense created on a booking adds to the price the booking mirrors (#2084)', async () => { const item = { id: 9, name: 'Seat', total_price: 15, reservation_id: 42 }; const resyncReservationPrice = vi.fn(); - const broadcast = vi.fn(); const svc = makeService({ create: vi.fn().mockReturnValue(item), resyncReservationPrice, - broadcast, } as Partial); - expect( - await new BudgetController(svc).create( - user, - '5', - { name: 'Seat', total_price: 15, reservation_id: 42 }, - 'sock', - ), - ).toEqual({ item }); + const { route, realtime } = await createRoute(svc); + expect(await route.create(user, '5', { name: 'Seat', total_price: 15, reservation_id: 42 }, 'sock')).toEqual({ + item, + }); expect(resyncReservationPrice).toHaveBeenCalledWith('5', 42, 'sock'); - expect(broadcast).toHaveBeenCalledWith('5', 'budget:created', { item }, 'sock'); + expect(realtime.broadcast).toHaveBeenCalledWith('5', 'budget:created', { item }, 'sock'); }); // #2084: an expense can only point at a booking or a place of its own trip. // The body and the status are the contract the MCP tool mirrors word for word. it('400s on a reservation_id from another trip, without writing or broadcasting', async () => { const create = vi.fn(); - const broadcast = vi.fn(); const resyncReservationPrice = vi.fn(); const linkRefusal = vi.fn().mockReturnValue('reservation_id does not belong to this trip.'); - const svc = makeService({ create, broadcast, linkRefusal, resyncReservationPrice } as Partial); + const svc = makeService({ create, linkRefusal, resyncReservationPrice } as Partial); + const { route, realtime } = await createRoute(svc); const body = { name: 'Hotel', total_price: 200, reservation_id: 4711 }; - expect(await thrownAsync(() => new BudgetController(svc).create(user, '5', body, 'sock'))).toEqual({ + expect(await thrownAsync(() => route.create(user, '5', body, 'sock'))).toEqual({ status: 400, body: { error: 'reservation_id does not belong to this trip.' }, }); expect(linkRefusal).toHaveBeenCalledWith('5', body); expect(create).not.toHaveBeenCalled(); expect(resyncReservationPrice).not.toHaveBeenCalled(); - expect(broadcast).not.toHaveBeenCalled(); + expect(realtime.broadcast).not.toHaveBeenCalled(); }); it('400s on a place_id from another trip, without writing', async () => { @@ -340,9 +367,8 @@ describe('BudgetController (parity with the legacy /api/trips/:tripId/budget rou create, linkRefusal: vi.fn().mockReturnValue('place_id does not belong to this trip.'), } as Partial); - expect( - await thrownAsync(() => new BudgetController(svc).create(user, '5', { name: 'Tickets', place_id: 4711 })), - ).toEqual({ + const { route } = await createRoute(svc); + expect(await thrownAsync(() => route.create(user, '5', { name: 'Tickets', place_id: 4711 }))).toEqual({ status: 400, body: { error: 'place_id does not belong to this trip.' }, }); diff --git a/server/tests/unit/nest/packing-writes.service.test.ts b/server/tests/unit/nest/packing-writes.service.test.ts index a4ee9b26b..38319eda1 100644 --- a/server/tests/unit/nest/packing-writes.service.test.ts +++ b/server/tests/unit/nest/packing-writes.service.test.ts @@ -1,5 +1,6 @@ import { DomainError } from '../../../src/nest/common/domain-error'; import { McpToolGuardsService } from '../../../src/nest/mcp-shared/mcp-tool-guards.service'; +import type { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; import { PackingWritesService } from '../../../src/nest/packing/packing-writes.service'; import type { PackingService } from '../../../src/nest/packing/packing.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; @@ -63,7 +64,8 @@ function setup(overrides: Record = {}, opts: { allowed?: boolea const permissions = { checkPermission: vi.fn().mockResolvedValue(opts.allowed ?? true), } as unknown as PermissionsService; - const writes = new PackingWritesService(packing, permissions); + const mailer = { getUserEmail: vi.fn().mockResolvedValue(user.email) } as unknown as MailerService; + const writes = new PackingWritesService(packing, permissions, mailer); const guards = new McpToolGuardsService( { getOwnerId: vi.fn().mockResolvedValue(1) } as never, { getRole: vi.fn().mockResolvedValue('user') } as never, @@ -80,7 +82,7 @@ function setup(overrides: Record = {}, opts: { allowed?: boolea await fn(writer); return { names: names(), calls: [...realtime.broadcast.mock.calls] }; }; - return { writes, packing, permissions, realtime, run }; + return { writes, packing, permissions, mailer, realtime, run }; } describe('packing item events: REST == MCP', () => { @@ -216,3 +218,103 @@ describe('packing item use cases: refusals', () => { expect(s.packing.updateItem).toHaveBeenCalledWith('5', 3, { name: 'x' }, ['name'], 'stale', 1); }); }); + +describe('packing list use cases: REST == MCP', () => { + const shared = { id: 3, is_private: 1, owner_id: 1, recipients: [{ user_id: 2 }] }; + const mcpText = (err: unknown) => (err as DomainError).mcpMessage ?? (err as DomainError).publicMessage; + + it('PACK-LIST-001: a sharing change drops the item from the room, re-adds it for its viewers, and pings no bag totals', async () => { + const s = setup({ setItemSharing: vi.fn().mockResolvedValue(shared) }); + const rest = await s.run('rest', (w) => s.writes.setSharing('5', 3, 'shared', [2], w)); + const mcp = await s.run('mcp', (w) => s.writes.setSharing(5, 3, 'shared', [2], w)); + // The MCP tool used to add a bag-totals ping the route never sent. + expect(mcp.names).toEqual(rest.names); + expect(rest.names).toEqual(['packing:deleted', 'packing:created', 'packing:created']); + expect(rest.calls.slice(1).map((c) => c[4])).toEqual([1, 2]); + expect(mcp.calls.slice(1).map((c) => c[4])).toEqual([1, 2]); + expect(s.packing.setItemSharing).toHaveBeenCalledWith(5, 3, 1, 'shared', [2]); + }); + + it("PACK-LIST-002: only the owner changes the sharing: REST's 403 and MCP's sentence, nothing sent", async () => { + const s = setup({ setItemSharing: vi.fn().mockResolvedValue({ forbidden: true }) }); + const err = await s.run('mcp', (w) => s.writes.setSharing(5, 3, 'personal', [], w)).catch((e: unknown) => e); + expect((err as DomainError).getStatus()).toBe(403); + expect((err as DomainError).toBody()).toEqual({ error: 'Only the owner can change sharing' }); + expect(mcpText(err)).toBe('Only the owner can change sharing.'); + expect(s.realtime.broadcast).not.toHaveBeenCalled(); + }); + + it('PACK-LIST-003: a reorder tells the other tabs on both surfaces, REST skipping the sender', async () => { + const s = setup({ reorderItems: vi.fn().mockResolvedValue(undefined) }); + const rest = await s.run('rest', (w) => s.writes.reorder('5', [3, 1], w)); + const mcp = await s.run('mcp', (w) => s.writes.reorder(5, [3, 1], w)); + expect(rest.calls).toEqual([['5', 'packing:reordered', { orderedIds: [3, 1] }, 'sock']]); + expect(mcp.calls).toEqual([[5, 'packing:reordered', { orderedIds: [3, 1], _source: 'mcp' }]]); + }); + + it('PACK-LIST-004: a bag delete sends the delete and the totals ping; a bag off the trip is a 404 and sends nothing', async () => { + const s = setup({ deleteBag: vi.fn().mockResolvedValue(true) }); + const rest = await s.run('rest', (w) => s.writes.deleteBag('5', 7, w)); + const mcp = await s.run('mcp', (w) => s.writes.deleteBag(5, 7, w)); + expect(mcp.names).toEqual(rest.names); + expect(rest.names).toEqual(['packing:bag-deleted', 'packing:bag-totals']); + + const gone = setup({ deleteBag: vi.fn().mockResolvedValue(false) }); + const err = await gone.run('mcp', (w) => gone.writes.deleteBag(5, 7, w)).catch((e: unknown) => e); + expect((err as DomainError).getStatus()).toBe(404); + expect((err as DomainError).toBody()).toEqual({ error: 'Bag not found' }); + expect(mcpText(err)).toBe('Bag not found.'); + expect(gone.realtime.broadcast).not.toHaveBeenCalled(); + }); + + it('PACK-LIST-005: category assignees go to the room and notify the tagged travellers on both surfaces', async () => { + const assignees = [{ user_id: 2 }]; + const s = setup({ updateCategoryAssignees: vi.fn().mockResolvedValue(assignees), notifyTagged: vi.fn() }); + const rest = await s.run('rest', (w) => s.writes.setCategoryAssignees('5', 'Clothes', [2], w)); + const mcp = await s.run('mcp', (w) => s.writes.setCategoryAssignees(5, 'Clothes', [2], w)); + expect(mcp.names).toEqual(rest.names); + expect(rest.calls).toEqual([['5', 'packing:assignees', { category: 'Clothes', assignees }, 'sock']]); + // The MCP tool used to tell none of them; both name the actor by its email. + const actor = { id: 1, email: user.email }; + expect(s.packing.notifyTagged).toHaveBeenNthCalledWith(1, '5', actor, 'Clothes', [2]); + expect(s.packing.notifyTagged).toHaveBeenNthCalledWith(2, '5', actor, 'Clothes', [2]); + expect(s.mailer.getUserEmail).toHaveBeenCalledWith(1); + }); + + it('PACK-LIST-006: clearing a category notifies nobody, and an actor without an address is not named', async () => { + const s = setup({ updateCategoryAssignees: vi.fn().mockResolvedValue([]), notifyTagged: vi.fn() }); + await s.run('mcp', (w) => s.writes.setCategoryAssignees(5, 'Clothes', [], w)); + expect(s.mailer.getUserEmail).not.toHaveBeenCalled(); + + vi.mocked(s.mailer.getUserEmail).mockResolvedValueOnce(null); + await s.run('mcp', (w) => s.writes.setCategoryAssignees(5, 'Clothes', [2], w)); + expect(s.packing.notifyTagged).not.toHaveBeenCalled(); + }); + + it('PACK-LIST-007: a template applied as personal items reaches its owner alone, on both surfaces', async () => { + const items = [{ id: 8, is_private: 1, owner_id: 1 }]; + const s = setup({ applyTemplate: vi.fn().mockResolvedValue(items) }); + const rest = await s.run('rest', (w) => s.writes.applyTemplate('5', 4, 'personal', w)); + const mcp = await s.run('mcp', (w) => s.writes.applyTemplate(5, 4, 'personal', w)); + expect(rest.calls).toEqual([ + ['5', 'packing:template-applied', { items }, 'sock', 1], + ['5', 'packing:bag-totals', {}, undefined], + ]); + expect(mcp.calls).toEqual([ + [5, 'packing:template-applied', { items, _source: 'mcp' }, undefined, 1], + ['5', 'packing:bag-totals', {}, undefined], + ]); + // The owner rides along on both, so a personal apply is the writer's own list. + expect(s.packing.applyTemplate).toHaveBeenNthCalledWith(1, '5', 4, 'personal', 1); + expect(s.packing.applyTemplate).toHaveBeenNthCalledWith(2, 5, 4, 'personal', 1); + }); + + it("PACK-LIST-008: a missing or empty template is REST's 404 and MCP's own sentence, nothing sent", async () => { + const s = setup({ applyTemplate: vi.fn().mockResolvedValue(null) }); + const err = await s.run('mcp', (w) => s.writes.applyTemplate(5, 4, 'common', w)).catch((e: unknown) => e); + expect((err as DomainError).getStatus()).toBe(404); + expect((err as DomainError).toBody()).toEqual({ error: 'Template not found or empty' }); + expect(mcpText(err)).toBe('Template not found.'); + expect(s.realtime.broadcast).not.toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/nest/packing.controller.test.ts b/server/tests/unit/nest/packing.controller.test.ts index 06b5d485e..c987ca115 100644 --- a/server/tests/unit/nest/packing.controller.test.ts +++ b/server/tests/unit/nest/packing.controller.test.ts @@ -1,3 +1,4 @@ +import type { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; import { PackingWritesService } from '../../../src/nest/packing/packing-writes.service'; import { PackingController as RoutePackingController } from '../../../src/nest/packing/packing.controller'; import type { PackingService } from '../../../src/nest/packing/packing.service'; @@ -62,11 +63,13 @@ function makeService(overrides: Partial = {}): PackingService { * The route over the real item use cases (PackingWritesService), with the * permission granted: the 403 lives in trip-access.guard.test.ts, and these * cases pin what the route answers and broadcasts through the service mock. + * The mailer answers the actor's email the assignee notification names. */ const allowAll = { checkPermission: vi.fn().mockResolvedValue(true) } as unknown as PermissionsService; +const mailer = { getUserEmail: vi.fn().mockResolvedValue(user.email) } as unknown as MailerService; class PackingController extends RoutePackingController { constructor(svc: PackingService) { - super(svc, new PackingWritesService(svc, allowAll)); + super(svc, new PackingWritesService(svc, allowAll, mailer)); } } @@ -377,11 +380,17 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r }); describe('PUT /reorder', () => { - it('reorders the items and reports success', async () => { + it('reorders the items, tells the other tabs and reports success', async () => { const reorderItems = vi.fn(); - const svc = makeService({ reorderItems } as Partial); - expect(await new PackingController(svc).reorder(user, '5', { orderedIds: [3, 1, 2] })).toEqual({ success: true }); + const broadcast = vi.fn(); + const svc = makeService({ reorderItems, broadcast } as Partial); + expect(await new PackingController(svc).reorder(user, '5', { orderedIds: [3, 1, 2] }, 'sock')).toEqual({ + success: true, + }); expect(reorderItems).toHaveBeenCalledWith('5', [3, 1, 2]); + // The route used to tell nobody; it now sends what the MCP tool always sent, + // skipping the tab that reordered. + expect(broadcast).toHaveBeenCalledWith('5', 'packing:reordered', { orderedIds: [3, 1, 2] }, 'sock'); }); }); @@ -669,12 +678,15 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r it('applies a template, broadcasts the added items and reports the count', async () => { const items = [{ id: 1 }, { id: 2 }, { id: 3 }]; const applyTemplate = vi.fn().mockReturnValue(items); - const broadcastItem = vi.fn(); - const svc = makeService({ applyTemplate, broadcastItem } as Partial); + const broadcast = vi.fn(); + const broadcastBagTotals = vi.fn(); + const svc = makeService({ applyTemplate, broadcast, broadcastBagTotals } as Partial); const res = await new PackingController(svc).applyTemplate(user, '5', '1', {}, 'sock'); expect(res).toEqual({ items, count: 3 }); expect(applyTemplate).toHaveBeenCalledWith('5', 1, 'common', user.id); - expect(broadcastItem).toHaveBeenCalledWith('5', 'packing:template-applied', { items }, items[0], 'sock'); + // Common items: the whole room, the applying tab left out. + expect(broadcast).toHaveBeenCalledWith('5', 'packing:template-applied', { items }, 'sock'); + expect(broadcastBagTotals).toHaveBeenCalledWith('5'); }); // #1565: the template must follow the tab the user is on, and a personal apply @@ -682,11 +694,13 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r it('applies a template into the personal list and keeps the broadcast to its owner', async () => { const items = [{ id: 1, is_private: 1, owner_id: user.id }]; const applyTemplate = vi.fn().mockReturnValue(items); - const broadcastItem = vi.fn(); - const svc = makeService({ applyTemplate, broadcastItem } as Partial); + const broadcast = vi.fn(); + const broadcastToViewers = vi.fn(); + const svc = makeService({ applyTemplate, broadcast, broadcastToViewers } as Partial); await new PackingController(svc).applyTemplate(user, '5', '1', { visibility: 'personal' }, 'sock'); expect(applyTemplate).toHaveBeenCalledWith('5', 1, 'personal', user.id); - expect(broadcastItem).toHaveBeenCalledWith('5', 'packing:template-applied', { items }, items[0], 'sock'); + expect(broadcastToViewers).toHaveBeenCalledWith('5', 'packing:template-applied', { items }, [user.id], 'sock'); + expect(broadcast).not.toHaveBeenCalledWith('5', 'packing:template-applied', expect.anything(), 'sock'); }); it('falls back to the common pool for an unknown visibility', async () => { @@ -783,7 +797,18 @@ describe('PackingController (parity with the legacy /api/trips/:tripId/packing r { category: 'Clothes', assignees: [{ user_id: 2 }] }, 'sock', ); - expect(notifyTagged).toHaveBeenCalledWith('5', user, 'Clothes', [2]); + // The actor is named by the email on its account row, as the MCP tool names it. + expect(notifyTagged).toHaveBeenCalledWith('5', { id: user.id, email: user.email }, 'Clothes', [2]); + }); + + it('notifies nobody when the category is cleared', async () => { + const notifyTagged = vi.fn(); + const svc = makeService({ + updateCategoryAssignees: vi.fn().mockReturnValue([]), + notifyTagged, + } as Partial); + await new PackingController(svc).updateCategoryAssignees(user, '5', 'Clothes', { user_ids: [] }, 'sock'); + expect(notifyTagged).not.toHaveBeenCalled(); }); }); }); diff --git a/server/tests/unit/nest/todo.controller.test.ts b/server/tests/unit/nest/todo.controller.test.ts index 08f546cb2..cb942d7fe 100644 --- a/server/tests/unit/nest/todo.controller.test.ts +++ b/server/tests/unit/nest/todo.controller.test.ts @@ -97,11 +97,16 @@ describe('TodoController (parity with the legacy /api/trips/:tripId/todo route)' }); }); - it('PUT /reorder succeeds with permission', async () => { - const reorderItems = vi.fn().mockResolvedValue(undefined); - const svc = makeService({ reorderItems } as Partial); - expect(await new TodoController(svc).reorder(user, '5', { orderedIds: [3, 1, 2] })).toEqual({ success: true }); - expect(reorderItems).toHaveBeenCalledWith('5', [3, 1, 2]); + it('PUT /reorder hands the order to the use case, with the requesting socket to skip', async () => { + const writer = { userId: user.id, surface: 'rest' }; + const reorder = vi.fn().mockResolvedValue(undefined); + const restWriter = vi.fn().mockReturnValue(writer); + const svc = makeService({ reorder, restWriter } as Partial); + expect(await new TodoController(svc).reorder(user, '5', { orderedIds: [3, 1, 2] }, 'sock')).toEqual({ + success: true, + }); + expect(restWriter).toHaveBeenCalledWith('5', user, 'sock'); + expect(reorder).toHaveBeenCalledWith('5', [3, 1, 2], writer); }); describe('category assignees', () => { diff --git a/server/tests/unit/nest/todo.service.test.ts b/server/tests/unit/nest/todo.service.test.ts index 17d5aa0af..a53b69f91 100644 --- a/server/tests/unit/nest/todo.service.test.ts +++ b/server/tests/unit/nest/todo.service.test.ts @@ -9,6 +9,7 @@ import { db as testDb } from '../../../src/db/database'; import { TodoCategoryAssignees } from '../../../src/db/entities/TodoCategoryAssignees.entity'; import { TodoItems } from '../../../src/db/entities/TodoItems.entity'; import { Users } from '../../../src/db/entities/Users.entity'; +import { DomainError } from '../../../src/nest/common/domain-error'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { TodoService } from '../../../src/nest/todo/todo.service'; @@ -253,6 +254,56 @@ describe('reorderItems', () => { }); }); +// ── reorder (the use case) ──────────────────────────────────────────────────── + +describe('reorder', () => { + function writerFor(userId: number) { + const events = { emit: vi.fn(), emitAll: vi.fn() }; + return { writer: { userId, role: 'user', surface: 'mcp' as const, events }, events }; + } + + it('TODO-SVC-025: saves the order, then tells the other tabs once with todo:reordered', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const a = (await svc.createItem(trip.id, { name: 'A' })) as { id: number }; + const b = (await svc.createItem(trip.id, { name: 'B' })) as { id: number }; + const { writer, events } = writerFor(user.id); + + await svc.reorder(trip.id, [b.id, a.id], writer); + + const rows = await findRows(await sharedTestOrm(testDb), TodoItems, { trip: trip.id }, { sort_order: 'asc' }); + expect(rows.map((r) => r.id)).toEqual([b.id, a.id]); + expect(events.emit.mock.calls).toEqual([['todo:reordered', { orderedIds: [b.id, a.id] }]]); + expect(events.emitAll).not.toHaveBeenCalled(); + }); + + it('TODO-SVC-026: a trip the writer cannot see is refused before the write, and nothing is sent', async () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const a = (await svc.createItem(trip.id, { name: 'A' })) as { id: number }; + const { writer, events } = writerFor(stranger.id); + + const err = await svc.reorder(trip.id, [a.id], writer).catch((e: unknown) => e); + expect(err).toBeInstanceOf(DomainError); + expect((err as DomainError).getStatus()).toBe(404); + expect(events.emit).not.toHaveBeenCalled(); + }); + + it("TODO-SVC-027: the REST writer skips the requesting socket, as every todo route's broadcast does", () => { + // The service holds a plain RealtimeService; its method lives on the prototype. + const broadcast = vi.spyOn(RealtimeService.prototype, 'broadcast').mockImplementation(() => undefined); + try { + const writer = svc.restWriter('5', { id: 1, role: 'user' } as never, 'sock'); + expect(writer).toMatchObject({ userId: 1, role: 'user', socketId: 'sock', surface: 'rest' }); + writer.events.emit('todo:reordered', { orderedIds: [2, 1] }); + expect(broadcast).toHaveBeenCalledWith('5', 'todo:reordered', { orderedIds: [2, 1] }, 'sock'); + } finally { + broadcast.mockRestore(); + } + }); +}); + // ── category assignees ──────────────────────────────────────────────────────── describe('getCategoryAssignees / updateCategoryAssignees', () => {