diff --git a/server/src/nest/trip-members/trip-members.controller.ts b/server/src/nest/trip-members/trip-members.controller.ts new file mode 100644 index 000000000..f95a11dd8 --- /dev/null +++ b/server/src/nest/trip-members/trip-members.controller.ts @@ -0,0 +1,171 @@ +import { + Body, + Controller, + Delete, + Get, + Headers, + HttpCode, + HttpException, + Param, + Post, + Put, + Req, + UseGuards, +} from '@nestjs/common'; +import type { Request } from 'express'; +import type { User } from '../../types'; +import { TripMembersService } from './trip-members.service'; +import { JwtAuthGuard } from '../auth/jwt-auth.guard'; +import { CurrentUser } from '../auth/current-user.decorator'; +import { getClientIp } from '../audit/client-ip'; +import { AuditService } from '../audit/audit.service'; +import { NotFoundError, ValidationError } from '../common/domain-errors'; +import { TripAddMemberDto, TripTransferOwnershipDto, TripCreateGuestDto, TripRenameGuestDto } from '../trips/trips.dto'; + +/** + * /api/trips/:id/{members,transfer,guests} — who is on a trip. + * + * Same prefix as TripsController but a separate class in a separate module: + * every route here is at least one segment deeper than the @Get(':id') it sits + * beside, so there is no ambiguity, and TripInviteLinkController has served + * /api/trips/:tripId/invite-link from its own module for as long. + * + * No class-level TripAccessGuard on purpose. Guards run before pipes, so one + * here would answer the trip 404 before the body pipe could answer its 400, and + * the e2e cases pin that order. The inline canAccessTrip checks are the ones + * that moved, unchanged. + */ +@Controller('api/trips') +@UseGuards(JwtAuthGuard) +export class TripMembersController { + constructor(private readonly roster: TripMembersService, private readonly audit: AuditService) {} + + @Get(':id/members') + members(@CurrentUser() user: User, @Param('id') id: string) { + const access = this.roster.canAccessTrip(id, user.id); + if (!access) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + const { owner, members } = this.roster.listMembers(id, access.user_id); + return { owner, members, current_user_id: user.id }; + } + + @Post(':id/members') + @HttpCode(201) + addMember(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripAddMemberDto) { + const { identifier } = body; + const access = this.roster.canAccessTrip(id, user.id); + if (!access) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + if (!this.roster.can('member_manage', user.role, access.user_id, user.id, access.user_id !== user.id)) { + throw new HttpException({ error: 'No permission to manage members' }, 403); + } + try { + const result = this.roster.addMember(id, identifier, access.user_id, user.id); + this.roster.notifyInvite(id, user, result.targetUserId, result.tripTitle, result.member.email); + return { member: result.member }; + } catch (e: unknown) { + if (e instanceof NotFoundError) throw new HttpException({ error: e.message }, 404); + if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); + throw e; + } + } + + @Delete(':id/members/:userId') + removeMember(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { + const access = this.roster.canAccessTrip(id, user.id); + if (!access) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + const targetId = parseInt(userId); + if (targetId !== user.id && !this.roster.can('member_manage', user.role, access.user_id, user.id, access.user_id !== user.id)) { + throw new HttpException({ error: 'No permission to remove members' }, 403); + } + this.roster.removeMember(id, targetId); + return { success: true }; + } + + @Post(':id/transfer') + transferOwnership( + @CurrentUser() user: User, + @Param('id') id: string, + @Body() body: TripTransferOwnershipDto, + @Req() req: Request, + @Headers('x-socket-id') socketId?: string, + ) { + const { newOwnerId } = body; + const access = this.roster.canAccessTrip(id, user.id); + if (!access) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + // Owner-only: handing over a trip is reserved for its actual owner, not just + // anyone who can manage members. + if (access.user_id !== user.id) { + throw new HttpException({ error: 'Only the owner can transfer ownership' }, 403); + } + try { + const result = this.roster.transferOwnership(id, newOwnerId, user.id); + this.audit.writeAudit({ userId: user.id, action: 'trip.transfer_ownership', ip: getClientIp(req), details: { tripId: Number(id), trip: result.tripTitle, from: result.fromEmail, to: result.toEmail } }); + // Nudge everyone viewing the trip to re-read it so the new ownership and the + // recomputed permissions take effect live. + const updatedTrip = this.roster.getTripForViewer(id, user.id); + this.roster.broadcast(id, 'trip:updated', { trip: updatedTrip }, socketId); + return { success: true }; + } catch (e: unknown) { + if (e instanceof NotFoundError) throw new HttpException({ error: e.message }, 404); + if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); + throw e; + } + } + + /** Loads the trip or throws 404, then asserts the caller is its owner (guest CRUD, #1362). */ + private requireOwner(id: string, user: User): void { + const access = this.roster.canAccessTrip(id, user.id); + if (!access) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + if (access.user_id !== user.id) { + throw new HttpException({ error: 'Only the owner can manage guests' }, 403); + } + } + + @Post(':id/guests') + @HttpCode(201) + createGuest(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripCreateGuestDto) { + this.requireOwner(id, user); + // 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 this.roster.createGuest(id, body.name, user.id); + } catch (e: unknown) { + if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); + throw e; + } + } + + @Put(':id/guests/:userId') + renameGuest(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string, @Body() body: TripRenameGuestDto) { + this.requireOwner(id, user); + try { + if (!this.roster.renameGuest(id, parseInt(userId), body.name)) { + throw new HttpException({ error: 'Guest not found' }, 404); + } + return { success: true }; + } catch (e: unknown) { + if (e instanceof HttpException) throw e; + if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); + throw e; + } + } + + @Delete(':id/guests/:userId') + deleteGuest(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { + this.requireOwner(id, user); + if (!this.roster.deleteGuest(id, parseInt(userId))) { + 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 new file mode 100644 index 000000000..33ec65537 --- /dev/null +++ b/server/src/nest/trip-members/trip-members.module.ts @@ -0,0 +1,26 @@ +import { Module } from '@nestjs/common'; +import { TripMembersController } from './trip-members.controller'; +import { TripMembersService } from './trip-members.service'; +import { DatabaseModule } from '../database/database.module'; +import { PermissionsModule } from '../permissions/permissions.module'; +import { RealtimeModule } from '../realtime/realtime.module'; +import { AuditModule } from '../audit/audit.module'; +import { AuthModule } from '../auth/auth.module'; +import { BudgetModule } from '../budget/budget.module'; + +/** + * The member/guest roster of a trip. + * + * A sink module: it imports auth (for the plugin-data erasure a deleted guest + * triggers) and budget (for the expense re-split that goes with it), and nothing + * imports it back except trips. That is what keeps it legal — trip-membership/ + * next door cannot do the same, because AuthModule imports *that* one, so giving + * it these dependencies would close the cycle. + */ +@Module({ + imports: [DatabaseModule, PermissionsModule, RealtimeModule, AuditModule, AuthModule, BudgetModule], + controllers: [TripMembersController], + providers: [TripMembersService], + exports: [TripMembersService], +}) +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 new file mode 100644 index 000000000..7824ebb95 --- /dev/null +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -0,0 +1,269 @@ +import { Injectable } from '@nestjs/common'; +import { randomUUID } from 'crypto'; +import { DatabaseService } from '../database/database.service'; +import type { User } from '../../types'; +import { avatarUrl } from '../common/avatarUrl'; +import { UserCleanupService } from '../auth/user-cleanup.service'; +import { BudgetService } from '../budget/budget.service'; +import { PermissionsService } from '../permissions/permissions.service'; +import { RealtimeService } from '../realtime/realtime.service'; +import { TRIP_SELECT } from '../trips/trips.service'; +import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; +import { emitUserDeleted } from '../../plugin-user-lifecycle'; +import { NotFoundError, ValidationError } from '../common/domain-errors'; + +export interface AddMemberResult { + member: { id: number; username: string; email: string; avatar?: string | null; role: string; avatar_url: string | null }; + targetUserId: number; + tripTitle: string; +} + +export interface TransferOwnershipResult { + tripTitle: string; + fromEmail: string; + toEmail: string; +} + +// ── Guest members (#1362) ─────────────────────────────── +// +// A guest is a credential-less users row (is_guest=1) joined into trip_members, so +// it is assignable everywhere a real member is (budget splits, packing, to-dos, day +// participants) yet can never authenticate (the auth/global-list guards exclude +// is_guest=1). The display name lives in users.username so every existing JOIN that +// renders a member name shows the guest correctly; a synthetic, non-deliverable +// email keeps the UNIQUE/NOT NULL constraints satisfied. + +export interface GuestMember { + id: number; + username: string; + email: string; + role: 'member'; + is_guest: true; + avatar_url: null; +} + +/** + * Who is on a trip: real members, the owner handover, and the credential-less + * guests (#1362). + * + * Its own domain because it was one of three places that could put somebody on + * a trip, and because it is the reason TripsService reached for the auth and + * budget domains at all: deleting a guest erases their plugin data and re-splits + * the expenses they were part of. + * + * NOT the same module as trip-membership/. That one is a deliberate leaf with no + * imports, and AuthModule imports it — so it can never depend on auth or budget + * without closing a cycle. This module is a sink: it imports both and nothing + * imports it back except trips. + */ +@Injectable() +export class TripMembersService { + constructor( + private readonly dbs: DatabaseService, + private readonly budget: BudgetService, + private readonly userCleanup: UserCleanupService, + private readonly permissions: PermissionsService, + private readonly realtime: RealtimeService, + ) {} + + private get db() { + return this.dbs.connection; + } + + canAccessTrip(tripId: string | number, userId: number) { + return this.dbs.canAccessTrip(tripId, userId) as { user_id: number } | null | undefined; + } + + can(action: string, role: string, ownerId: number | null, userId: number, isMember: boolean): boolean { + return this.permissions.checkPermission(action, role, ownerId, userId, isMember); + } + + broadcast(tripId: string, event: E, payload: TrekWsPayload, socketId: string | undefined): void { + this.realtime.broadcast(tripId, event, payload, socketId); + } + + /** The trip in list shape, for the re-read a handover broadcasts. Same query the + * trip routes use, imported rather than copied so the two cannot drift. */ + getTripForViewer(tripId: string | number, userId: number) { + return this.db.prepare(`${TRIP_SELECT} WHERE t.id = :tripId`).get({ userId, tripId }); + } + + /** Fire-and-forget trip-invite notification (mirrors the route's dynamic import). */ + notifyInvite(tripId: string, actor: User, targetUserId: number, tripTitle: string, inviteeEmail: string): void { + import('../notifications/notifications.bridge').then(({ send }) => { + send({ + event: 'trip_invite', + actorId: actor.id, + scope: 'user', + targetId: targetUserId, + params: { trip: tripTitle, actor: actor.email, invitee: inviteeEmail, tripId: String(tripId) }, + }).catch(() => {}); + }); + } + + // ── Members ─────────────────────────────────────────────────────────────── + + listMembers(tripId: string | number, tripOwnerId: number) { + // u.is_guest rides along (#1362) so guests stay assignable everywhere a member is, + // while the UI can badge them and suppress owner-only actions. The owner is never a guest. + const members = this.db.prepare(` + SELECT u.id, COALESCE(u.display_name, u.username) AS username, u.email, u.avatar, u.is_guest, + CASE WHEN u.id = ? THEN 'owner' ELSE 'member' END as role, + m.added_at, + COALESCE(ib.display_name, ib.username) as invited_by_username + FROM trip_members m + JOIN users u ON u.id = m.user_id + LEFT JOIN users ib ON ib.id = m.invited_by + WHERE m.trip_id = ? + ORDER BY m.added_at ASC + `).all(tripOwnerId, tripId) as { id: number; username: string; email: string; avatar: string | null; is_guest: number; role: string; added_at: string; invited_by_username: string | null }[]; + + // Quirk fix on top of the 1:1 move: the owner row prefers display_name like + // every member row does (the legacy query read the raw username only). + const owner = this.db.prepare('SELECT id, COALESCE(display_name, username) AS username, email, avatar FROM users WHERE id = ?').get(tripOwnerId) as Pick; + + return { + owner: { ...owner, role: 'owner', is_guest: false, avatar_url: avatarUrl(owner) }, + members: members.map(m => ({ ...m, is_guest: !!m.is_guest, avatar_url: avatarUrl(m) })), + }; + } + + addMember(tripId: string | number, identifier: string, tripOwnerId: number, invitedByUserId: number): AddMemberResult { + if (!identifier) throw new ValidationError('Email or username required'); + + // Guests (#1362) are not invitable accounts — exclude them so a trip-scoped guest + // can never be resolved (and re-attached to another trip) through the invite box. + const target = this.db.prepare( + 'SELECT id, username, email, avatar FROM users WHERE (email = ? OR username = ?) AND COALESCE(is_guest, 0) = 0' + ).get(identifier.trim(), identifier.trim()) as Pick | undefined; + + if (!target) throw new NotFoundError('User not found'); + + if (target.id === tripOwnerId) + throw new ValidationError('Trip owner is already a member'); + + const existing = this.db.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(tripId, target.id); + if (existing) throw new ValidationError('User already has access'); + + this.db.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, target.id, invitedByUserId); + + const tripInfo = this.db.prepare('SELECT title FROM trips WHERE id = ?').get(tripId) as { title: string } | undefined; + + return { + member: { ...target, role: 'member', avatar_url: avatarUrl(target) }, + targetUserId: target.id, + tripTitle: tripInfo?.title || 'Untitled', + }; + } + + removeMember(tripId: string | number, targetUserId: number): void { + this.db.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(tripId, targetUserId); + } + + /** + * Hand a trip over to one of its existing members (#973). The new owner must + * already be a member; afterwards they hold `trips.user_id` and the former owner + * becomes a regular member, so nobody loses access. Runs in a transaction so the + * owner pointer and the membership rows never diverge. + */ + transferOwnership( + tripId: string | number, + newOwnerId: number, + currentOwnerId: number, + ): TransferOwnershipResult { + const trip = this.db.prepare('SELECT id, title, user_id FROM trips WHERE id = ?').get(tripId) as { id: number; title: string; user_id: number } | undefined; + if (!trip) throw new NotFoundError('Trip not found'); + if (trip.user_id !== currentOwnerId) throw new ValidationError('Only the owner can transfer ownership'); + if (newOwnerId === currentOwnerId) throw new ValidationError('You already own this trip'); + + const newOwner = this.db.prepare('SELECT id, email, is_guest FROM users WHERE id = ?').get(newOwnerId) as { id: number; email: string; is_guest?: number } | undefined; + if (!newOwner) throw new NotFoundError('User not found'); + // A guest (#1362) can never log in, so it must never become the owner of a trip. + if (newOwner.is_guest) throw new ValidationError('Cannot transfer ownership to a guest'); + + const isMember = this.db.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(tripId, newOwnerId); + if (!isMember) throw new ValidationError('New owner must be a trip member'); + + const fromEmail = (this.db.prepare('SELECT email FROM users WHERE id = ?').get(currentOwnerId) as { email: string } | undefined)?.email || ''; + + const run = this.db.transaction(() => { + this.db.prepare('UPDATE trips SET user_id = ? WHERE id = ?').run(newOwnerId, tripId); + // The new owner is no longer a plain member… + this.db.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(tripId, newOwnerId); + // …and the former owner keeps access as a member. + this.db.prepare('INSERT OR IGNORE INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, currentOwnerId, newOwnerId); + }); + run(); + + return { tripTitle: trip.title, fromEmail, toEmail: newOwner.email }; + } + + // ── Guest members (#1362) ─────────────────────────────────────────────────── + + /** username is UNIQUE across all users — keep the typed name but disambiguate guests + * that happen to share it (e.g. two "Anna"s) with a numeric suffix. */ + createGuest(tripId: string | number, name: string, invitedByUserId: number): { member: GuestMember } { + const display = (name || '').trim(); + if (!display) throw new ValidationError('Guest name is required'); + if (display.length > 50) throw new ValidationError('Guest name must be 50 characters or fewer'); + + // The human name lives in display_name (not unique — two trips can each have a + // "Jake", #1446); username is a uuid handle only for the UNIQUE constraint and is + // never shown (member views COALESCE display_name over it). + const email = `guest-${randomUUID()}@guests.invalid`; + const username = `guest-${randomUUID()}`; + + const create = this.db.transaction(() => { + const res = this.db.prepare( + "INSERT INTO users (username, email, password_hash, role, is_guest, display_name) VALUES (?, ?, '', 'user', 1, ?)" + ).run(username, email, display); + const guestId = Number(res.lastInsertRowid); + this.db.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, guestId, invitedByUserId); + return guestId; + }); + const guestId = create(); + + return { member: { id: guestId, username: display, email, role: 'member', is_guest: true, avatar_url: null } }; + } + + /** Confirms a user id is a guest of THIS trip, so guest mutations stay trip-scoped. */ + private guestOfTrip(tripId: string | number, guestUserId: number): boolean { + return !!this.db.prepare( + 'SELECT u.id FROM users u JOIN trip_members m ON m.user_id = u.id WHERE u.id = ? AND m.trip_id = ? AND u.is_guest = 1' + ).get(guestUserId, tripId); + } + + renameGuest(tripId: string | number, guestUserId: number, name: string): boolean { + const display = (name || '').trim(); + if (!display) throw new ValidationError('Guest name is required'); + if (display.length > 50) throw new ValidationError('Guest name must be 50 characters or fewer'); + if (!this.guestOfTrip(tripId, guestUserId)) return false; + + // Rename only the display name — no global-uniqueness dedup, so a rename to a name + // another trip's guest already uses no longer produces "Name 2" (#1446). + this.db.prepare('UPDATE users SET display_name = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ? AND is_guest = 1').run(display, guestUserId); + return true; + } + + deleteGuest(tripId: string | number, guestUserId: number): boolean { + if (!this.guestOfTrip(tripId, guestUserId)) return false; + // A guest is still a user id a plugin may hold data for, so erase that too — the + // host-side per-user tables + a durable own-db erasure per granted plugin — exactly + // like a full account deletion (otherwise a deleted guest's plugin data lingers). + this.userCleanup.erasePluginUserData(guestUserId); + // Quirk fix on top of the 1:1 move: the budget re-split and the user delete + // run in one transaction, so a failure mid-flow can't leave the expense + // divisors re-derived for a guest that still exists (or vice versa). The + // plugin-side erasure/notification keep their order around it. + this.db.transaction(() => { + // Re-split the expenses they were part of before the cascade takes their member + // rows away — the divisor is denormalized and cannot follow a foreign key (#1553). + this.budget.removeUserFromBudgetItems(guestUserId); + // Deleting the guest's users row cascades its membership and every assignment join + // (trip_members, budget/packing/assignment links) via the ON DELETE foreign keys. + this.db.prepare('DELETE FROM users WHERE id = ? AND is_guest = 1').run(guestUserId); + })(); + emitUserDeleted(guestUserId); // deliver the erasure to any active plugin now + return true; + } +} diff --git a/server/src/nest/trip-read-model/trip-read-model.module.ts b/server/src/nest/trip-read-model/trip-read-model.module.ts new file mode 100644 index 000000000..cb01e34dd --- /dev/null +++ b/server/src/nest/trip-read-model/trip-read-model.module.ts @@ -0,0 +1,25 @@ +import { Module } from '@nestjs/common'; +import { TripReadModelService } from './trip-read-model.service'; +import { DatabaseModule } from '../database/database.module'; +import { DaysModule } from '../days/days.module'; +import { AccommodationsModule } from '../accommodations/accommodations.module'; +import { BudgetModule } from '../budget/budget.module'; +import { PackingModule } from '../packing/packing.module'; +import { ReservationsModule } from '../reservations/reservations.module'; +import { CollabModule } from '../collab/collab.module'; +import { PlacesModule } from '../places/places.module'; +import { TodoModule } from '../todo/todo.module'; +import { FilesModule } from '../files/files.module'; +import { TripMembersModule } from '../trip-members/trip-members.module'; + +/** Where the trip read aggregates keep their fan-out, so the write path does not + * have to carry it. Nothing imports this except trips. */ +@Module({ + imports: [ + DatabaseModule, TripMembersModule, DaysModule, AccommodationsModule, BudgetModule, + PackingModule, ReservationsModule, CollabModule, PlacesModule, TodoModule, FilesModule, + ], + providers: [TripReadModelService], + exports: [TripReadModelService], +}) +export class TripReadModelModule {} diff --git a/server/src/nest/trip-read-model/trip-read-model.service.ts b/server/src/nest/trip-read-model/trip-read-model.service.ts new file mode 100644 index 000000000..12feb55e3 --- /dev/null +++ b/server/src/nest/trip-read-model/trip-read-model.service.ts @@ -0,0 +1,115 @@ +import { Injectable } from '@nestjs/common'; +import { DatabaseService } from '../database/database.service'; +import { DaysService } from '../days/days.service'; +import { AccommodationsService } from '../accommodations/accommodations.service'; +import { BudgetService } from '../budget/budget.service'; +import { PackingService } from '../packing/packing.service'; +import { ReservationsService } from '../reservations/reservations.service'; +import { CollabService } from '../collab/collab.service'; +import { PlacesService } from '../places/places.service'; +import { TodoService } from '../todo/todo.service'; +import { FilesService } from '../files/files.service'; +import { TripMembersService } from '../trip-members/trip-members.service'; + +/** + * The two read aggregates over a trip: the MCP summary and the offline bundle. + * + * They were the reason TripsService injected nine other domain services while + * its write path needs a handful. Reading is the fan-out; the writing core is + * not. Split apart, the aggregate root talks to database, permissions, realtime + * and the four domains its own updates touch, and everything that only ever + * appeared inside a summary lives here. + */ +@Injectable() +export class TripReadModelService { + constructor( + private readonly dbs: DatabaseService, + private readonly members: TripMembersService, + private readonly days: DaysService, + private readonly accommodations: AccommodationsService, + private readonly budget: BudgetService, + private readonly packing: PackingService, + private readonly reservations: ReservationsService, + private readonly collab: CollabService, + private readonly places: PlacesService, + private readonly todo: TodoService, + private readonly files: FilesService, + ) {} + + private get db() { + return this.dbs.connection; + } + + private getOwner(tripId: string | number): { user_id: number } | undefined { + return this.db.prepare('SELECT user_id FROM trips WHERE id = ?').get(tripId) as { user_id: number } | undefined; + } + + // ── Trip summary (used by MCP get_trip_summary tool) ────────────────────── + + getTripSummary(tripId: number, viewerUserId?: number) { + const trip = this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Record | undefined; + if (!trip) return null; + + const ownerRow = this.getOwner(tripId); + if (!ownerRow) return null; + const { owner, members } = this.members.listMembers(tripId, ownerRow.user_id); + + const { days: rawDays } = this.days.list(tripId); + const days = rawDays.map(({ notes_items, ...day }) => ({ ...day, notes: notes_items })); + + const accommodations = this.accommodations.list(tripId); + + const budgetItems = this.budget.listBudgetItems(tripId); + const budget = { + items: budgetItems, + item_count: budgetItems.length, + total: budgetItems.reduce((sum, i) => sum + (i.total_price || 0), 0), + currency: trip.currency, + }; + + // Thread the viewer so another member's private/personal packing items (#858) + // stay hidden — without it listItems returns the UNFILTERED list. + const packingItems = this.packing.listItems(tripId, viewerUserId); + const packing = { + items: packingItems, + total: packingItems.length, + checked: (packingItems as { checked: number }[]).filter(i => i.checked).length, + }; + + const reservations = this.reservations.list(tripId); + const collab_notes = this.collab.listNotes(tripId); + + return { + trip, + members: { owner, collaborators: members }, + days, + accommodations, + budget, + packing, + reservations, + collab_notes, + }; + } + + // ── Bundle / notifications (route helpers) ──────────────────────────────── + + /** Aggregates every trip sub-collection for offline caching (legacy /:id/bundle). */ + bundle(tripId: string, trip: { user_id: number }, viewerId: number) { + const { days } = this.days.list(tripId); + const { owner, members } = this.members.listMembers(tripId, trip.user_id); + return { + trip, + days, + places: this.places.list(String(tripId), {}), + // Scope to the requesting member so other members' private packing items + // (#858) never land in this viewer's offline cache. + packingItems: this.packing.listItems(tripId, viewerId), + todoItems: this.todo.listItems(tripId), + budgetItems: this.budget.listBudgetItems(tripId), + reservations: this.reservations.list(tripId), + files: this.files.listFiles(tripId, false), + accommodations: this.accommodations.list(tripId), + members: [owner, ...(members || [])].filter(Boolean), + }; + } +} diff --git a/server/src/nest/trips/trips.bridge.ts b/server/src/nest/trips/trips.bridge.ts index b6c03186a..092a66d1c 100644 --- a/server/src/nest/trips/trips.bridge.ts +++ b/server/src/nest/trips/trips.bridge.ts @@ -22,6 +22,8 @@ import { PlacesService } from '../places/places.service'; import { MapsService } from '../maps/maps.service'; import { UserCleanupService } from '../auth/user-cleanup.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; +import { TripMembersService } from '../trip-members/trip-members.service'; +import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; /** * Non-Nest entry point for the trip domain — for the two consumers that cannot @@ -44,22 +46,35 @@ const dbs = () => new DatabaseService(db); const photoCache = new PlacePhotoCacheService(dbs(), new RuntimeEnvService()); const journeyDomain = new JourneyDomainService(dbs(), new RealtimeService(), new TrekPhotosRepository(dbs())); const budget = new BudgetService(dbs(), new PermissionsService(dbs()), new ExchangeRatesService(), new RealtimeService()); +const permissions = new PermissionsService(dbs()); +const realtime = new RealtimeService(); +const days = new DaysService(dbs(), permissions, realtime, new QueryHelpersService(dbs())); +const accommodations = new AccommodationsService(dbs(), permissions, realtime); +const reservations = new ReservationsService(dbs(), permissions, budget, realtime); +const places = new PlacesService(dbs(), permissions, realtime, new MapsService(dbs(), photoCache), new QueryHelpersService(dbs()), new UnsplashService(dbs(), new RuntimeEnvService()), photoCache, journeyDomain); const trips = new TripsService( dbs(), - new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new ReservationsService(dbs(), new PermissionsService(dbs()), budget, new RealtimeService()), - new DaysService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new QueryHelpersService(dbs())), - new PermissionsService(dbs()), + reservations, + days, + permissions, budget, - new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new VacayService(dbs(), new RealtimeService()), - new RealtimeService(), - new PlacesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new MapsService(dbs(), photoCache), new QueryHelpersService(dbs()), new UnsplashService(dbs(), new RuntimeEnvService()), photoCache, journeyDomain), + new VacayService(dbs(), realtime), + realtime, new UnsplashService(dbs(), new RuntimeEnvService()), - new UserCleanupService(dbs()), - new AccommodationsService(dbs(), new PermissionsService(dbs()), new RealtimeService()), +); +const members = new TripMembersService(dbs(), budget, new UserCleanupService(dbs()), permissions, realtime); +const readModel = new TripReadModelService( + dbs(), + members, + days, + accommodations, + budget, + new PackingService(dbs(), permissions, realtime), + reservations, + new CollabService(dbs(), permissions, realtime), + places, + new TodoService(dbs(), permissions, realtime), + new FilesService(dbs(), permissions, realtime), ); export function getTripOwner(tripId: string | number) { @@ -67,11 +82,11 @@ export function getTripOwner(tripId: string | number) { } export function listMembers(tripId: string | number, tripOwnerId: number) { - return trips.listMembers(tripId, tripOwnerId); + return members.listMembers(tripId, tripOwnerId); } export function getTripSummary(tripId: number, viewerUserId?: number) { - return trips.getTripSummary(tripId, viewerUserId); + return readModel.getTripSummary(tripId, viewerUserId); } /** diff --git a/server/src/nest/trips/trips.controller.ts b/server/src/nest/trips/trips.controller.ts index 18b00a2c0..cae1d961b 100644 --- a/server/src/nest/trips/trips.controller.ts +++ b/server/src/nest/trips/trips.controller.ts @@ -36,6 +36,7 @@ import { NotFoundError, ValidationError } from './trips.service'; import { TripCreateDto, TripUpdateDto, TripCopyDto, TripAddMemberDto, TripTransferOwnershipDto, TripCreateGuestDto, TripRenameGuestDto } from './trips.dto'; import { UnsplashService } from '../unsplash/unsplash.service'; import { CalendarService } from '../calendar/calendar.service'; +import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; const MAX_COVER_SIZE = 20 * 1024 * 1024; const coversDir = path.join(__dirname, '../../../uploads/covers'); @@ -73,7 +74,7 @@ const addDays = (d: Date, n: number) => { const r = new Date(d); r.setDate(r.get export class TripsController { // calendar last: the hand-wired construction sites in the tests stay positional, // so a new dependency does not touch the ones that never reach the ICS route. - constructor(private readonly trips: TripsService, private readonly audit: AuditService, private readonly env: RuntimeEnvService, private readonly unsplash: UnsplashService, private readonly calendar: CalendarService) {} + constructor(private readonly trips: TripsService, private readonly audit: AuditService, private readonly env: RuntimeEnvService, private readonly unsplash: UnsplashService, private readonly calendar: CalendarService, private readonly readModel: TripReadModelService) {} @Get() list(@CurrentUser() user: User, @Query('archived') archived?: string) { @@ -253,142 +254,13 @@ export class TripsController { return { success: true }; } - @Get(':id/members') - members(@CurrentUser() user: User, @Param('id') id: string) { - const access = this.trips.canAccessTrip(id, user.id); - if (!access) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - const { owner, members } = this.trips.listMembers(id, access.user_id); - return { owner, members, current_user_id: user.id }; - } - - @Post(':id/members') - @HttpCode(201) - addMember(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripAddMemberDto) { - const { identifier } = body; - const access = this.trips.canAccessTrip(id, user.id); - if (!access) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - if (!this.trips.can('member_manage', user.role, access.user_id, user.id, access.user_id !== user.id)) { - throw new HttpException({ error: 'No permission to manage members' }, 403); - } - try { - const result = this.trips.addMember(id, identifier, access.user_id, user.id); - this.trips.notifyInvite(id, user, result.targetUserId, result.tripTitle, result.member.email); - return { member: result.member }; - } catch (e: unknown) { - if (e instanceof NotFoundError) throw new HttpException({ error: e.message }, 404); - if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); - throw e; - } - } - - @Delete(':id/members/:userId') - removeMember(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { - const access = this.trips.canAccessTrip(id, user.id); - if (!access) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - const targetId = parseInt(userId); - if (targetId !== user.id && !this.trips.can('member_manage', user.role, access.user_id, user.id, access.user_id !== user.id)) { - throw new HttpException({ error: 'No permission to remove members' }, 403); - } - this.trips.removeMember(id, targetId); - return { success: true }; - } - - @Post(':id/transfer') - transferOwnership( - @CurrentUser() user: User, - @Param('id') id: string, - @Body() body: TripTransferOwnershipDto, - @Req() req: Request, - @Headers('x-socket-id') socketId?: string, - ) { - const { newOwnerId } = body; - const access = this.trips.canAccessTrip(id, user.id); - if (!access) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - // Owner-only: handing over a trip is reserved for its actual owner, not just - // anyone who can manage members. - if (access.user_id !== user.id) { - throw new HttpException({ error: 'Only the owner can transfer ownership' }, 403); - } - try { - const result = this.trips.transferOwnership(id, newOwnerId, user.id); - this.audit.writeAudit({ userId: user.id, action: 'trip.transfer_ownership', ip: getClientIp(req), details: { tripId: Number(id), trip: result.tripTitle, from: result.fromEmail, to: result.toEmail } }); - // Nudge everyone viewing the trip to re-read it so the new ownership and the - // recomputed permissions take effect live. - const updatedTrip = this.trips.get(id, user.id); - this.trips.broadcast(id, 'trip:updated', { trip: updatedTrip }, socketId); - return { success: true }; - } catch (e: unknown) { - if (e instanceof NotFoundError) throw new HttpException({ error: e.message }, 404); - if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); - throw e; - } - } - - /** Loads the trip or throws 404, then asserts the caller is its owner (guest CRUD, #1362). */ - private requireOwner(id: string, user: User): void { - const access = this.trips.canAccessTrip(id, user.id); - if (!access) { - throw new HttpException({ error: 'Trip not found' }, 404); - } - if (access.user_id !== user.id) { - throw new HttpException({ error: 'Only the owner can manage guests' }, 403); - } - } - - @Post(':id/guests') - @HttpCode(201) - createGuest(@CurrentUser() user: User, @Param('id') id: string, @Body() body: TripCreateGuestDto) { - this.requireOwner(id, user); - // 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 this.trips.createGuest(id, body.name, user.id); - } catch (e: unknown) { - if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); - throw e; - } - } - - @Put(':id/guests/:userId') - renameGuest(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string, @Body() body: TripRenameGuestDto) { - this.requireOwner(id, user); - try { - if (!this.trips.renameGuest(id, parseInt(userId), body.name)) { - throw new HttpException({ error: 'Guest not found' }, 404); - } - return { success: true }; - } catch (e: unknown) { - if (e instanceof HttpException) throw e; - if (e instanceof ValidationError) throw new HttpException({ error: e.message }, 400); - throw e; - } - } - - @Delete(':id/guests/:userId') - deleteGuest(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { - this.requireOwner(id, user); - if (!this.trips.deleteGuest(id, parseInt(userId))) { - throw new HttpException({ error: 'Guest not found' }, 404); - } - return { success: true }; - } - @Get(':id/bundle') bundle(@CurrentUser() user: User, @Param('id') id: string) { const trip = this.trips.get(id, user.id) as { user_id: number } | undefined; if (!trip) { throw new HttpException({ error: 'Trip not found' }, 404); } - return this.trips.bundle(id, trip, user.id); + return this.readModel.bundle(id, trip, user.id); } @Get(':id/export.ics') diff --git a/server/src/nest/trips/trips.mcp.ts b/server/src/nest/trips/trips.mcp.ts index a135b73ff..052b75465 100644 --- a/server/src/nest/trips/trips.mcp.ts +++ b/server/src/nest/trips/trips.mcp.ts @@ -7,6 +7,8 @@ import { import { z } from 'zod'; import { AuthService } from '../auth/auth.service'; import { CalendarService } from '../calendar/calendar.service'; +import { TripMembersService } from '../trip-members/trip-members.service'; +import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; import { isAddonEnabled, getCollabFeatures } from '../addons/addons.bridge'; import { ADDON_IDS } from '../../addons'; import { safeBroadcast, MAX_MCP_TRIP_DAYS, noAccess, hasTripPermission, permissionDenied } from '../../mcp/tools/_shared'; @@ -67,6 +69,8 @@ export class TripsMcp { // Appended, not inserted: the hand-wired MCP test harnesses build this // positionally, so an earlier slot would silently shift every one of them. private readonly calendar: CalendarService, + private readonly members: TripMembersService, + private readonly readModel: TripReadModelService, ) {} // --- TRIPS --- @@ -204,7 +208,7 @@ export class TripsMcp { }) async getTripSummary({ tripId }: { tripId: number }, ctx: McpContext) { if (!this.trips.canAccessTrip(tripId, ctx.userId)) return noAccess(); - const summary = this.trips.getTripSummary(tripId, ctx.userId); + const summary = this.readModel.getTripSummary(tripId, ctx.userId); if (!summary) return noAccess(); const R = canReadTrips(ctx.scopes); // Addon availability gates @@ -275,7 +279,7 @@ export class TripsMcp { if (!this.trips.canAccessTrip(tripId, ctx.userId)) return noAccess(); const ownerRow = this.trips.getOwner(tripId); if (!ownerRow) return noAccess(); - const { owner, members } = this.trips.listMembers(tripId, ownerRow.user_id); + const { owner, members } = this.members.listMembers(tripId, ownerRow.user_id); return ok({ owner, members }); } @@ -296,7 +300,7 @@ export class TripsMcp { if (!ownerRow || ownerRow.user_id !== ctx.userId) return { content: [{ type: 'text' as const, text: 'Only the trip owner can add members.' }], isError: true }; try { - const result = this.trips.addMember(tripId, identifier, ownerRow.user_id, ctx.userId); + const result = this.members.addMember(tripId, identifier, ownerRow.user_id, ctx.userId); safeBroadcast(tripId, 'member:added', { member: result.member }); return ok({ member: result.member }); } catch (err) { @@ -321,7 +325,7 @@ export class TripsMcp { const ownerRow = this.trips.getOwner(tripId); if (!ownerRow || ownerRow.user_id !== ctx.userId) return { content: [{ type: 'text' as const, text: 'Only the trip owner can remove members.' }], isError: true }; - this.trips.removeMember(tripId, memberId); + this.members.removeMember(tripId, memberId); safeBroadcast(tripId, 'member:removed', { userId: memberId }); return ok({ success: true }); } @@ -407,7 +411,7 @@ export class TripsMcp { if (id === null || !this.trips.canAccessTrip(id, ctx.userId)) return accessDenied(uri.href); const ownerRow = this.trips.getOwner(id); if (!ownerRow) return accessDenied(uri.href); - const { owner, members } = this.trips.listMembers(id, ownerRow.user_id); + const { owner, members } = this.members.listMembers(id, ownerRow.user_id); return jsonContent(uri.href, { owner, members }); } @@ -425,7 +429,7 @@ export class TripsMcp { if (!this.trips.canAccessTrip(tripId, ctx.userId)) { return { messages: [{ role: 'user' as const, content: { type: 'text' as const, text: 'Trip not found or access denied.' } }] }; } - const summary = this.trips.getTripSummary(tripId, ctx.userId); + const summary = this.readModel.getTripSummary(tripId, ctx.userId); if (!summary) { return { messages: [{ role: 'user' as const, content: { type: 'text' as const, text: 'Trip not found.' } }] }; } diff --git a/server/src/nest/trips/trips.module.ts b/server/src/nest/trips/trips.module.ts index f105ac3a8..bd0403e6b 100644 --- a/server/src/nest/trips/trips.module.ts +++ b/server/src/nest/trips/trips.module.ts @@ -22,11 +22,13 @@ import { TripsMcp } from './trips.mcp'; import { AuthModule } from '../auth/auth.module'; import { CalendarModule } from '../calendar/calendar.module'; import { AccommodationsModule } from '../accommodations/accommodations.module'; +import { TripMembersModule } from '../trip-members/trip-members.module'; +import { TripReadModelModule } from '../trip-read-model/trip-read-model.module'; /** Trips aggregate root (C1 — Phase 3). Uses exact strangler prefixes so it does * not capture the nested sub-domain mounts (collab, files, ...). */ @Module({ - imports: [TodoModule, PackingModule, FilesModule, ReservationsModule, DaysModule, PermissionsModule, AuditModule, BudgetModule, CollabModule, VacayModule, PlacesModule, AuthModule, AppConfigModule, UnsplashModule, RealtimeModule, PluginGuardsModule, TripMembershipModule, CalendarModule, AccommodationsModule], + imports: [TodoModule, PackingModule, FilesModule, ReservationsModule, DaysModule, PermissionsModule, AuditModule, BudgetModule, CollabModule, VacayModule, PlacesModule, AuthModule, AppConfigModule, UnsplashModule, RealtimeModule, PluginGuardsModule, TripMembershipModule, CalendarModule, AccommodationsModule, TripMembersModule, TripReadModelModule], controllers: [TripsController], providers: [TripsService, TripsMcp, TripsRpc], // Exported for FeedsModule (ICS feeds) and PluginsModule (RPC host injection). diff --git a/server/src/nest/trips/trips.rpc.ts b/server/src/nest/trips/trips.rpc.ts index 2e95e6674..552e89196 100644 --- a/server/src/nest/trips/trips.rpc.ts +++ b/server/src/nest/trips/trips.rpc.ts @@ -9,6 +9,7 @@ import { DatabaseService } from '../database/database.service'; import { ReservationsService } from '../reservations/reservations.service'; import { DaysService } from '../days/days.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; +import { TripMembersService } from '../trip-members/trip-members.service'; import { TripMembershipService } from '../trip-membership/trip-membership.service'; import { TripsService, NotFoundError, ValidationError } from './trips.service'; @@ -40,6 +41,7 @@ export class TripsRpc { private readonly guards: PluginGuards, // Appended: the hand-wired plugin-host harnesses build this positionally. private readonly accommodations: AccommodationsService, + private readonly roster: TripMembersService, ) {} @PluginMethod('trips.getById', { permission: 'db:read:trips' }) @@ -183,7 +185,7 @@ export class TripsRpc { // Ownership transfer is a separate, deliberate action. const trip = this.db.prepare('SELECT user_id FROM trips WHERE id = ?').get(tripId) as { user_id: number } | undefined; if (trip && trip.user_id === targetUserId) throw new ForbiddenResource('cannot remove the trip owner'); - this.trips.removeMember(tripId, targetUserId); + this.roster.removeMember(tripId, targetUserId); return { removed: true }; } } diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index 1cf3a7bcf..761a0bf7b 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -1,24 +1,14 @@ import { Injectable } from '@nestjs/common'; import path from 'path'; import fs from 'fs'; -import { randomUUID } from 'crypto'; import { DatabaseService } from '../database/database.service'; import type { ActiveTrip, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import type { Trip, User } from '../../types'; -import { avatarUrl } from '../common/avatarUrl'; -import { UserCleanupService } from '../auth/user-cleanup.service'; -import { emitUserDeleted } from '../../plugin-user-lifecycle'; -import { PlacesService } from '../places/places.service'; import { DaysService } from '../days/days.service'; -import { AccommodationsService } from '../accommodations/accommodations.service'; -import { PackingService } from '../packing/packing.service'; -import { TodoService } from '../todo/todo.service'; import { BudgetService } from '../budget/budget.service'; import { ReservationsService } from '../reservations/reservations.service'; -import { FilesService } from '../files/files.service'; -import { CollabService } from '../collab/collab.service'; import { VacayService } from '../vacay/vacay.service'; import { UnsplashService } from '../unsplash/unsplash.service'; import { NotFoundError, ValidationError } from '../common/domain-errors'; @@ -111,7 +101,10 @@ export interface GuestMember { } /** - * Trip aggregate root, DI-native. The SQL moved 1:1 from the legacy + * Trip aggregate root, DI-native. Membership, the calendar export and the two + * read aggregates live in their own domains now; what is left is the write core + * plus day generation, which is why the constructor is eight parameters instead + * of fourteen. The SQL moved 1:1 from the legacy * services/tripService.ts: identical statements, the `||` falsy-coercion * defaults, the post-write TRIP_SELECT re-selects and the mixed * named/positional parameter styles are all preserved byte-for-byte. @@ -127,21 +120,13 @@ export interface GuestMember { export class TripsService { constructor( private readonly dbs: DatabaseService, - private readonly todo: TodoService, - private readonly packing: PackingService, - private readonly files: FilesService, private readonly reservations: ReservationsService, private readonly days: DaysService, private readonly permissions: PermissionsService, private readonly budget: BudgetService, - private readonly collab: CollabService, private readonly vacay: VacayService, private readonly realtime: RealtimeService, - private readonly places: PlacesService, private readonly unsplash: UnsplashService, - private readonly userCleanup: UserCleanupService, - // Appended: the hand-wired construction sites stay positional. - private readonly accommodations: AccommodationsService, ) {} private get db() { @@ -691,253 +676,6 @@ export class TripsService { return this.db.prepare(`${TRIP_SELECT} WHERE t.id = :tripId`).get({ userId, tripId: newTripId }); } - // ── Members ─────────────────────────────────────────────────────────────── - - listMembers(tripId: string | number, tripOwnerId: number) { - // u.is_guest rides along (#1362) so guests stay assignable everywhere a member is, - // while the UI can badge them and suppress owner-only actions. The owner is never a guest. - const members = this.db.prepare(` - SELECT u.id, COALESCE(u.display_name, u.username) AS username, u.email, u.avatar, u.is_guest, - CASE WHEN u.id = ? THEN 'owner' ELSE 'member' END as role, - m.added_at, - COALESCE(ib.display_name, ib.username) as invited_by_username - FROM trip_members m - JOIN users u ON u.id = m.user_id - LEFT JOIN users ib ON ib.id = m.invited_by - WHERE m.trip_id = ? - ORDER BY m.added_at ASC - `).all(tripOwnerId, tripId) as { id: number; username: string; email: string; avatar: string | null; is_guest: number; role: string; added_at: string; invited_by_username: string | null }[]; - - // Quirk fix on top of the 1:1 move: the owner row prefers display_name like - // every member row does (the legacy query read the raw username only). - const owner = this.db.prepare('SELECT id, COALESCE(display_name, username) AS username, email, avatar FROM users WHERE id = ?').get(tripOwnerId) as Pick; - - return { - owner: { ...owner, role: 'owner', is_guest: false, avatar_url: avatarUrl(owner) }, - members: members.map(m => ({ ...m, is_guest: !!m.is_guest, avatar_url: avatarUrl(m) })), - }; - } - - addMember(tripId: string | number, identifier: string, tripOwnerId: number, invitedByUserId: number): AddMemberResult { - if (!identifier) throw new ValidationError('Email or username required'); - - // Guests (#1362) are not invitable accounts — exclude them so a trip-scoped guest - // can never be resolved (and re-attached to another trip) through the invite box. - const target = this.db.prepare( - 'SELECT id, username, email, avatar FROM users WHERE (email = ? OR username = ?) AND COALESCE(is_guest, 0) = 0' - ).get(identifier.trim(), identifier.trim()) as Pick | undefined; - - if (!target) throw new NotFoundError('User not found'); - - if (target.id === tripOwnerId) - throw new ValidationError('Trip owner is already a member'); - - const existing = this.db.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(tripId, target.id); - if (existing) throw new ValidationError('User already has access'); - - this.db.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, target.id, invitedByUserId); - - const tripInfo = this.db.prepare('SELECT title FROM trips WHERE id = ?').get(tripId) as { title: string } | undefined; - - return { - member: { ...target, role: 'member', avatar_url: avatarUrl(target) }, - targetUserId: target.id, - tripTitle: tripInfo?.title || 'Untitled', - }; - } - - removeMember(tripId: string | number, targetUserId: number): void { - this.db.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(tripId, targetUserId); - } - - /** - * Hand a trip over to one of its existing members (#973). The new owner must - * already be a member; afterwards they hold `trips.user_id` and the former owner - * becomes a regular member, so nobody loses access. Runs in a transaction so the - * owner pointer and the membership rows never diverge. - */ - transferOwnership( - tripId: string | number, - newOwnerId: number, - currentOwnerId: number, - ): TransferOwnershipResult { - const trip = this.db.prepare('SELECT id, title, user_id FROM trips WHERE id = ?').get(tripId) as { id: number; title: string; user_id: number } | undefined; - if (!trip) throw new NotFoundError('Trip not found'); - if (trip.user_id !== currentOwnerId) throw new ValidationError('Only the owner can transfer ownership'); - if (newOwnerId === currentOwnerId) throw new ValidationError('You already own this trip'); - - const newOwner = this.db.prepare('SELECT id, email, is_guest FROM users WHERE id = ?').get(newOwnerId) as { id: number; email: string; is_guest?: number } | undefined; - if (!newOwner) throw new NotFoundError('User not found'); - // A guest (#1362) can never log in, so it must never become the owner of a trip. - if (newOwner.is_guest) throw new ValidationError('Cannot transfer ownership to a guest'); - - const isMember = this.db.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(tripId, newOwnerId); - if (!isMember) throw new ValidationError('New owner must be a trip member'); - - const fromEmail = (this.db.prepare('SELECT email FROM users WHERE id = ?').get(currentOwnerId) as { email: string } | undefined)?.email || ''; - - const run = this.db.transaction(() => { - this.db.prepare('UPDATE trips SET user_id = ? WHERE id = ?').run(newOwnerId, tripId); - // The new owner is no longer a plain member… - this.db.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(tripId, newOwnerId); - // …and the former owner keeps access as a member. - this.db.prepare('INSERT OR IGNORE INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, currentOwnerId, newOwnerId); - }); - run(); - - return { tripTitle: trip.title, fromEmail, toEmail: newOwner.email }; - } - - // ── Guest members (#1362) ─────────────────────────────────────────────────── - - /** username is UNIQUE across all users — keep the typed name but disambiguate guests - * that happen to share it (e.g. two "Anna"s) with a numeric suffix. */ - createGuest(tripId: string | number, name: string, invitedByUserId: number): { member: GuestMember } { - const display = (name || '').trim(); - if (!display) throw new ValidationError('Guest name is required'); - if (display.length > 50) throw new ValidationError('Guest name must be 50 characters or fewer'); - - // The human name lives in display_name (not unique — two trips can each have a - // "Jake", #1446); username is a uuid handle only for the UNIQUE constraint and is - // never shown (member views COALESCE display_name over it). - const email = `guest-${randomUUID()}@guests.invalid`; - const username = `guest-${randomUUID()}`; - - const create = this.db.transaction(() => { - const res = this.db.prepare( - "INSERT INTO users (username, email, password_hash, role, is_guest, display_name) VALUES (?, ?, '', 'user', 1, ?)" - ).run(username, email, display); - const guestId = Number(res.lastInsertRowid); - this.db.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, guestId, invitedByUserId); - return guestId; - }); - const guestId = create(); - - return { member: { id: guestId, username: display, email, role: 'member', is_guest: true, avatar_url: null } }; - } - - /** Confirms a user id is a guest of THIS trip, so guest mutations stay trip-scoped. */ - private guestOfTrip(tripId: string | number, guestUserId: number): boolean { - return !!this.db.prepare( - 'SELECT u.id FROM users u JOIN trip_members m ON m.user_id = u.id WHERE u.id = ? AND m.trip_id = ? AND u.is_guest = 1' - ).get(guestUserId, tripId); - } - - renameGuest(tripId: string | number, guestUserId: number, name: string): boolean { - const display = (name || '').trim(); - if (!display) throw new ValidationError('Guest name is required'); - if (display.length > 50) throw new ValidationError('Guest name must be 50 characters or fewer'); - if (!this.guestOfTrip(tripId, guestUserId)) return false; - - // Rename only the display name — no global-uniqueness dedup, so a rename to a name - // another trip's guest already uses no longer produces "Name 2" (#1446). - this.db.prepare('UPDATE users SET display_name = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ? AND is_guest = 1').run(display, guestUserId); - return true; - } - - deleteGuest(tripId: string | number, guestUserId: number): boolean { - if (!this.guestOfTrip(tripId, guestUserId)) return false; - // A guest is still a user id a plugin may hold data for, so erase that too — the - // host-side per-user tables + a durable own-db erasure per granted plugin — exactly - // like a full account deletion (otherwise a deleted guest's plugin data lingers). - this.userCleanup.erasePluginUserData(guestUserId); - // Quirk fix on top of the 1:1 move: the budget re-split and the user delete - // run in one transaction, so a failure mid-flow can't leave the expense - // divisors re-derived for a guest that still exists (or vice versa). The - // plugin-side erasure/notification keep their order around it. - this.db.transaction(() => { - // Re-split the expenses they were part of before the cascade takes their member - // rows away — the divisor is denormalized and cannot follow a foreign key (#1553). - this.budget.removeUserFromBudgetItems(guestUserId); - // Deleting the guest's users row cascades its membership and every assignment join - // (trip_members, budget/packing/assignment links) via the ON DELETE foreign keys. - this.db.prepare('DELETE FROM users WHERE id = ? AND is_guest = 1').run(guestUserId); - })(); - emitUserDeleted(guestUserId); // deliver the erasure to any active plugin now - return true; - } - - // ── Trip summary (used by MCP get_trip_summary tool) ────────────────────── - - getTripSummary(tripId: number, viewerUserId?: number) { - const trip = this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Record | undefined; - if (!trip) return null; - - const ownerRow = this.getOwner(tripId); - if (!ownerRow) return null; - const { owner, members } = this.listMembers(tripId, ownerRow.user_id); - - const { days: rawDays } = this.days.list(tripId); - const days = rawDays.map(({ notes_items, ...day }) => ({ ...day, notes: notes_items })); - - const accommodations = this.accommodations.list(tripId); - - const budgetItems = this.budget.listBudgetItems(tripId); - const budget = { - items: budgetItems, - item_count: budgetItems.length, - total: budgetItems.reduce((sum, i) => sum + (i.total_price || 0), 0), - currency: trip.currency, - }; - - // Thread the viewer so another member's private/personal packing items (#858) - // stay hidden — without it listItems returns the UNFILTERED list. - const packingItems = this.packing.listItems(tripId, viewerUserId); - const packing = { - items: packingItems, - total: packingItems.length, - checked: (packingItems as { checked: number }[]).filter(i => i.checked).length, - }; - - const reservations = this.reservations.list(tripId); - const collab_notes = this.collab.listNotes(tripId); - - return { - trip, - members: { owner, collaborators: members }, - days, - accommodations, - budget, - packing, - reservations, - collab_notes, - }; - } - - // ── Bundle / notifications (route helpers) ──────────────────────────────── - - /** Aggregates every trip sub-collection for offline caching (legacy /:id/bundle). */ - bundle(tripId: string, trip: { user_id: number }, viewerId: number) { - const { days } = this.days.list(tripId); - const { owner, members } = this.listMembers(tripId, trip.user_id); - return { - trip, - days, - places: this.places.list(String(tripId), {}), - // Scope to the requesting member so other members' private packing items - // (#858) never land in this viewer's offline cache. - packingItems: this.packing.listItems(tripId, viewerId), - todoItems: this.todo.listItems(tripId), - budgetItems: this.budget.listBudgetItems(tripId), - reservations: this.reservations.list(tripId), - files: this.files.listFiles(tripId, false), - accommodations: this.accommodations.list(tripId), - members: [owner, ...(members || [])].filter(Boolean), - }; - } - - /** Fire-and-forget trip-invite notification (mirrors the route's dynamic import). */ - notifyInvite(tripId: string, actor: User, targetUserId: number, tripTitle: string, inviteeEmail: string): void { - import('../notifications/notifications.bridge').then(({ send }) => { - send({ - event: 'trip_invite', - actorId: actor.id, - scope: 'user', - targetId: targetUserId, - params: { trip: tripTitle, actor: actor.email, invitee: inviteeEmail, tripId: String(tripId) }, - }).catch(() => {}); - }); - } } // Defined in common/ so calendar and maps can raise them without importing the diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 6caac2857..1f6b71caf 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -60,6 +60,8 @@ import { UserCleanupService } from '../../src/nest/auth/user-cleanup.service'; import { CalendarService } from '../../src/nest/calendar/calendar.service'; import { AccommodationsService } from '../../src/nest/accommodations/accommodations.service'; import { AccommodationsMcp } from '../../src/nest/accommodations/accommodations.mcp'; +import { TripMembersService } from '../../src/nest/trip-members/trip-members.service'; +import { TripReadModelService } from '../../src/nest/trip-read-model/trip-read-model.service'; import { PlacePhotoCacheService } from '../../src/nest/place-photos/place-photo-cache.service'; import { RuntimeEnvService } from '../../src/nest/app-config/runtime-env.service'; import { makeNotificationsService, makeNotificationPreferencesService } from './notifications'; @@ -97,25 +99,21 @@ export function createMcpTestRegistry(): McpRegistry { ); const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService); const accommodationsService = new AccommodationsService(dbService, permissionsService, realtimeService); + const membersService = new TripMembersService(dbService, budgetService, new UserCleanupService(dbService), permissionsService, realtimeService); const tripsService = new TripsService( dbService, - todoService, - packingService, - new FilesService(dbService, permissionsService, realtimeService), - new ReservationsService(dbService, permissionsService, budgetService, realtimeService), + reservationsService, daysService, permissionsService, budgetService, - collabService, new VacayService(dbService, realtimeService), realtimeService, - placesService, - // These two were missing: the call passed 12 arguments to a 14-parameter - // constructor, so unsplash and userCleanup were undefined here. tsconfig - // has include:['src'], so tsc never saw it. new UnsplashService(dbService, new RuntimeEnvService()), - new UserCleanupService(dbService), - accommodationsService, + ); + const readModelService = new TripReadModelService( + dbService, membersService, daysService, accommodationsService, budgetService, + packingService, reservationsService, collabService, placesService, todoService, + new FilesService(dbService, permissionsService, realtimeService), ); const calendarService = new CalendarService(dbService, reservationsService); return createTestRegistry( @@ -136,7 +134,7 @@ export function createMcpTestRegistry(): McpRegistry { new AssignmentsMcp(new AssignmentsService(dbService, permissionsService, realtimeService, queryHelpersService, journeyDomain), daysService, authService), new CollabMcp(collabService, authService), new VacayMcp(new VacayService(dbService, realtimeService), authService), - new TripsMcp(tripsService, todoService, collabService, authService, calendarService), + new TripsMcp(tripsService, todoService, collabService, authService, calendarService, membersService, readModelService), new ShareMcp(new ShareService(dbService, new SettingsService(dbService), permissionsService, queryHelpersService), authService), new MapsMcp(mapsService), new PlacesMcp(placesService, mapsService, dbService, authService, journeyDomain), diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 2cd694971..ad07a8583 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -51,6 +51,7 @@ import { PlacesRpc } from '../../src/nest/places/places.rpc'; import { DaysRpc } from '../../src/nest/days/days.rpc'; import { AccommodationsRpc } from '../../src/nest/accommodations/accommodations.rpc'; import { AccommodationsService } from '../../src/nest/accommodations/accommodations.service'; +import { TripMembersService } from '../../src/nest/trip-members/trip-members.service'; import { ItineraryRpc } from '../../src/nest/assignments/itinerary.rpc'; import { TripsRpc } from '../../src/nest/trips/trips.rpc'; import { CostsRpc } from '../../src/nest/budget/costs.rpc'; @@ -99,7 +100,8 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF const accommodations = new AccommodationsService(dbs, permissions, realtime); // Was 12 arguments for a 14-parameter constructor; unsplash and userCleanup were // undefined here and tsconfig only typechecks src, so nothing said so. - const trips = new TripsService(dbs, todos, packing, files, reservations, days, permissions, budget, collab, vacay, realtime, places, undefined as never, undefined as never, accommodations); + const trips = new TripsService(dbs, reservations, days, permissions, budget, vacay, realtime, undefined as never); + const members = new TripMembersService(dbs, budget, undefined as never, permissions, realtime); const guards = new PluginGuards(dbs, permissions, addons); const registry = createTestPluginRegistry([ @@ -115,7 +117,7 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF new DaysRpc(days, realtime, guards), new AccommodationsRpc(accommodations, realtime, guards), new ItineraryRpc(assignments, realtime, guards), - new TripsRpc(trips, reservations, days, membership, dbs, realtime, guards, accommodations), + new TripsRpc(trips, reservations, days, membership, dbs, realtime, guards, accommodations, members), new CostsRpc(budget, dbs, realtime, guards), new ReservationsRpc(reservations, realtime, guards), new CollabRpc(collab, realtime, guards), diff --git a/server/tests/helpers/rpc-host-deps.ts b/server/tests/helpers/rpc-host-deps.ts index 6ad6fe354..bd2405720 100644 --- a/server/tests/helpers/rpc-host-deps.ts +++ b/server/tests/helpers/rpc-host-deps.ts @@ -71,7 +71,7 @@ export function allRpcControllers(): object[] { new PlacesRpc(anyService(), anyService(), anyService(), anyService()), new DaysRpc(anyService(), anyService(), anyService()), new ItineraryRpc(anyService(), anyService(), anyService()), - new TripsRpc(anyService(), anyService(), anyService(), anyService(), anyService(), anyService(), anyService(), anyService()), + new TripsRpc(anyService(), anyService(), anyService(), anyService(), anyService(), anyService(), anyService(), anyService(), anyService()), new CostsRpc(anyService(), anyService(), anyService(), anyService()), new ReservationsRpc(anyService(), anyService(), anyService()), new AccommodationsRpc(anyService(), anyService(), anyService()), diff --git a/server/tests/unit/mcp/tools-prompts.test.ts b/server/tests/unit/mcp/tools-prompts.test.ts index c1b7bb50b..3449a89fd 100644 --- a/server/tests/unit/mcp/tools-prompts.test.ts +++ b/server/tests/unit/mcp/tools-prompts.test.ts @@ -83,9 +83,21 @@ import type { CollabService } from '../../../src/nest/collab/collab.service'; // getTripSummary is the same controllable mock the legacy path used. const tripsStub = { canAccessTrip: (tripId: number, userId: number) => dbMock.canAccessTrip(tripId, userId), - getTripSummary: (tripId: number, viewerUserId?: number) => mockGetTripSummary(tripId, viewerUserId), } as unknown as TripsService; -const tripsMcp = new TripsMcp(tripsStub, { listItems: () => [] } as unknown as TodoService, { listPolls: () => [], countMessages: () => 0 } as unknown as CollabService); +// getTripSummary moved to TripReadModelService with the trip split; the mock is +// the same controllable one, one constructor slot further along. +const readModelStub = { + getTripSummary: (tripId: number, viewerUserId?: number) => mockGetTripSummary(tripId, viewerUserId), +} as never; +const tripsMcp = new TripsMcp( + tripsStub, + { listItems: () => [] } as unknown as TodoService, + { listPolls: () => [], countMessages: () => 0 } as unknown as CollabService, + undefined as never, + undefined as never, + undefined as never, + readModelStub, +); // The three remaining prompts moved to their domains' @McpController classes: // packing-list, budget-overview and the static-token notice. Built over the same diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 8c03a129b..9cfc4c87c 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -63,7 +63,7 @@ import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.se import * as bridge from '../../../src/nest/budget/budget.bridge'; import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; -import { TripsService } from '../../../src/nest/trips/trips.service'; +import { TripMembersService } from '../../../src/nest/trip-members/trip-members.service'; import { TodoService } from '../../../src/nest/todo/todo.service'; import { PackingService } from '../../../src/nest/packing/packing.service'; import { FilesService } from '../../../src/nest/files/files.service'; @@ -80,29 +80,20 @@ const budget = new BudgetService( new RealtimeService(), ); -// Guest fixtures come from the DI-native TripsService since the trip fold (was -// an import of the deleted services/tripService); deleteGuest routes through -// the SAME BudgetService domain SQL (removeUserFromBudgetItems) under test. +// Guest fixtures come from TripMembersService since the trip split (they were on +// TripsService before, and on the deleted services/tripService before that); +// deleteGuest routes through the SAME BudgetService domain SQL +// (removeUserFromBudgetItems) under test. const dbs = () => new DatabaseService(testDb); -const tripsSvc = new TripsService( +const membersSvc = new TripMembersService( dbs(), - new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new ReservationsService(dbs(), new PermissionsService(dbs()), budget, new RealtimeService()), - new DaysService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new QueryHelpersService(dbs())), - new PermissionsService(dbs()), budget, - new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new VacayService(dbs(), new RealtimeService()), - new RealtimeService(), - undefined as never, // places — not exercised here - undefined as never, // unsplash — not exercised here new UserCleanupService(dbs()), - undefined as never, // accommodations — not exercised here + new PermissionsService(dbs()), + new RealtimeService(), ); -const createGuest = tripsSvc.createGuest.bind(tripsSvc); -const deleteGuest = tripsSvc.deleteGuest.bind(tripsSvc); +const createGuest = membersSvc.createGuest.bind(membersSvc); +const deleteGuest = membersSvc.deleteGuest.bind(membersSvc); beforeAll(() => { createTables(testDb); diff --git a/server/tests/unit/nest/trip-members.controller.test.ts b/server/tests/unit/nest/trip-members.controller.test.ts new file mode 100644 index 000000000..5b21d302f --- /dev/null +++ b/server/tests/unit/nest/trip-members.controller.test.ts @@ -0,0 +1,174 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { HttpException } from '@nestjs/common'; +import type { Request } from 'express'; + +vi.mock('../../../src/nest/audit/client-ip', () => ({ getClientIp: vi.fn(() => '1.2.3.4') })); +vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logDebug: vi.fn(), logError: vi.fn(), logWarn: vi.fn() })); + +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'; +import type { AuditService } from '../../../src/nest/audit/audit.service'; +import { NotFoundError, ValidationError } from '../../../src/nest/common/domain-errors'; +import type { User } from '../../../src/types'; +import { tripTransferOwnershipRequestSchema } from '@trek/shared'; +import { expectRegisteredController } from '../../helpers/module-providers'; + +/** + * The member/guest routes, moved out of trips.controller.test.ts with the + * controller they cover. Every assertion is unchanged: the routes kept their + * paths, their status codes and their error bodies. + */ +const user = { id: 1, role: 'user', email: 'u@example.test' } as User; +const req = { headers: {} } as Request; + +const writeAudit = vi.fn(); +const audit = { writeAudit } as unknown as AuditService; +const tc = (s: TripMembersService) => new TripMembersController(s, audit); + +function svc(o: Partial = {}): TripMembersService { + return { + canAccessTrip: vi.fn().mockReturnValue({ user_id: 1 }), + can: vi.fn().mockReturnValue(true), + broadcast: vi.fn(), + notifyInvite: vi.fn(), + getTripForViewer: vi.fn().mockReturnValue({ id: 9 }), + ...o, + } as unknown as TripMembersService; +} + +function thrown(fn: () => unknown): { status: number; body: unknown } { + try { fn(); } catch (err) { + expect(err).toBeInstanceOf(HttpException); + const e = err as HttpException; + return { status: e.getStatus(), body: e.getResponse() }; + } + throw new Error('expected throw'); +} + +beforeEach(() => vi.clearAllMocks()); + +describe('TripMembersController', () => { +describe('members', () => { + it('GET 404 without access, else owner+members+current_user_id', () => { + expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).members(user, '9'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + const s = svc({ listMembers: vi.fn().mockReturnValue({ owner: { id: 1 }, members: [] }) } as Partial); + expect(tc(s).members(user, '9')).toEqual({ owner: { id: 1 }, members: [], current_user_id: 1 }); + }); + + it('POST 403 without member_manage, else adds + notifies', () => { + expect(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' } }); + 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(tc(s).addMember(user, '9', { identifier: 'bob@x.y' })).toEqual({ member: { id: 2, email: 'bob@x.y' } }); + expect(notifyInvite).toHaveBeenCalledWith('9', user, 2, 'T', 'bob@x.y'); + }); + + it('POST 404 without trip access', () => { + const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); + expect(thrown(() => tc(s).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + }); + + it('POST maps NotFoundError to 404, ValidationError to 400, re-throws others', () => { + const nf = svc({ addMember: vi.fn().mockImplementation(() => { throw new NotFoundError('no user'); }) } as Partial); + expect(thrown(() => tc(nf).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 404, body: { error: 'no user' } }); + const ve = svc({ addMember: vi.fn().mockImplementation(() => { throw new ValidationError('already a member'); }) } as Partial); + expect(thrown(() => tc(ve).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 400, body: { error: 'already a member' } }); + const other = svc({ addMember: vi.fn().mockImplementation(() => { throw new Error('boom'); }) } as Partial); + expect(() => tc(other).addMember(user, '9', { identifier: 'bob@x.y' })).toThrow('boom'); + }); + + it('DELETE 404 without trip access', () => { + const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); + expect(thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + }); + + it('DELETE self needs no permission; removing others needs member_manage', () => { + 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(tc(s).removeMember(user, '9', '1')).toEqual({ success: true }); + expect(thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 403, body: { error: 'No permission to remove members' } }); + }); +}); + +describe('POST /:id/transfer (#973)', () => { + it('404 without trip access', () => { + const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); + expect(thrown(() => tc(s).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + }); + + it('403 when the requester is not the owner', () => { + // access.user_id (5) differs from the requesting user (1) + const s = svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) }); + expect(thrown(() => tc(s).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 403, body: { error: 'Only the owner can transfer ownership' } }); + }); + + it('a non-numeric newOwnerId 400s in the ZodValidationPipe', () => { + // The hand-rolled 'newOwnerId is required' 400 moved into the global + // pipe (trips DTO ratchet) — the schema rejects non-numeric ids. + expect(tripTransferOwnershipRequestSchema.safeParse({ newOwnerId: 'nope' }).success).toBe(false); + expect(tripTransferOwnershipRequestSchema.safeParse({}).success).toBe(false); + }); + + it('transfers, audits and broadcasts the refreshed trip', () => { + const transferOwnership = vi.fn().mockReturnValue({ tripTitle: 'Roadtrip', fromEmail: 'a@x.y', toEmail: 'b@x.y' }); + const getTripForViewer = vi.fn().mockReturnValue({ id: 9, user_id: 2 }); + const broadcast = vi.fn(); + const s = svc({ transferOwnership, getTripForViewer, broadcast } as Partial); + expect(tc(s).transferOwnership(user, '9', { newOwnerId: 2 }, req, 'sock')).toEqual({ success: true }); + expect(transferOwnership).toHaveBeenCalledWith('9', 2, user.id); + expect(broadcast).toHaveBeenCalledWith('9', 'trip:updated', { trip: { id: 9, user_id: 2 } }, 'sock'); + }); + + it('maps NotFoundError to 404 and ValidationError to 400', () => { + const nf = svc({ transferOwnership: vi.fn().mockImplementation(() => { throw new NotFoundError('User not found'); }) } as Partial); + expect(thrown(() => tc(nf).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 404, body: { error: 'User not found' } }); + const ve = svc({ transferOwnership: vi.fn().mockImplementation(() => { throw new ValidationError('New owner must be a trip member'); }) } as Partial); + expect(thrown(() => tc(ve).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 400, body: { error: 'New owner must be a trip member' } }); + }); +}); + +describe('guests (#1362)', () => { + it('404 without access, 403 for a non-owner, 400 without a name; else creates', () => { + expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).createGuest(user, '9', { name: 'Anna' }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + // access.user_id (5) ≠ requester (1) → not the owner + expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) })).createGuest(user, '9', { name: 'Anna' }))).toEqual({ status: 403, body: { error: 'Only the owner can manage guests' } }); + // A whitespace-only name still 400s with the legacy body — the service + // throws after trimming (the schema only enforces 1..50 chars). + const wsGuest = svc({ createGuest: vi.fn().mockImplementation(() => { throw new ValidationError('Guest name is required'); }) } as Partial); + expect(thrown(() => tc(wsGuest).createGuest(user, '9', { name: ' ' }))).toEqual({ status: 400, body: { error: 'Guest name is required' } }); + const createGuest = vi.fn().mockReturnValue({ member: { id: 7, username: 'Anna', is_guest: true } }); + const s = svc({ createGuest } as Partial); + expect(tc(s).createGuest(user, '9', { name: 'Anna' })).toEqual({ member: { id: 7, username: 'Anna', is_guest: true } }); + expect(createGuest).toHaveBeenCalledWith('9', 'Anna', user.id); + }); + + it('rename: 403 non-owner, 404 when the guest is missing, else success', () => { + expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) })).renameGuest(user, '9', '7', { name: 'Bob' }))).toEqual({ status: 403, body: { error: 'Only the owner can manage guests' } }); + const miss = svc({ renameGuest: vi.fn().mockReturnValue(false) } as Partial); + expect(thrown(() => tc(miss).renameGuest(user, '9', '7', { name: 'Bob' }))).toEqual({ status: 404, body: { error: 'Guest not found' } }); + const ok = svc({ renameGuest: vi.fn().mockReturnValue(true) } as Partial); + expect(tc(ok).renameGuest(user, '9', '7', { name: 'Bob' })).toEqual({ success: true }); + }); + + it('delete: 403 non-owner, 404 when the guest is missing, else success', () => { + expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) })).deleteGuest(user, '9', '7'))).toEqual({ status: 403, body: { error: 'Only the owner can manage guests' } }); + const miss = svc({ deleteGuest: vi.fn().mockReturnValue(false) } as Partial); + expect(thrown(() => tc(miss).deleteGuest(user, '9', '7'))).toEqual({ status: 404, body: { error: 'Guest not found' } }); + const ok = svc({ deleteGuest: vi.fn().mockReturnValue(true) } as Partial); + expect(tc(ok).deleteGuest(user, '9', '7')).toEqual({ success: true }); + }); + + it('maps a ValidationError from createGuest to 400', () => { + const ve = svc({ createGuest: vi.fn().mockImplementation(() => { throw new ValidationError('Guest name must be 50 characters or fewer'); }) } as Partial); + expect(thrown(() => tc(ve).createGuest(user, '9', { name: 'x'.repeat(60) }))).toEqual({ status: 400, body: { error: 'Guest name must be 50 characters or fewer' } }); + }); +}); + + it('MEMBERS-CTRL-001: the module registers the controller, so the routes cannot go missing', () => { + expectRegisteredController(TripMembersModule, TripMembersController); + expect(Reflect.getMetadata('path', TripMembersController)).toBe('api/trips'); + }); +}); diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index 21336a4d4..338c0d3a3 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -19,6 +19,7 @@ const unsplashStub = { import { TripsController } from '../../../src/nest/trips/trips.controller'; import type { TripsService } from '../../../src/nest/trips/trips.service'; import type { CalendarService } from '../../../src/nest/calendar/calendar.service'; +import type { TripReadModelService } from '../../../src/nest/trip-read-model/trip-read-model.service'; import { NotFoundError, ValidationError } from '../../../src/nest/trips/trips.service'; import type { User } from '../../../src/types'; import { activeTripResponseSchema, tripCreateRequestSchema, tripTransferOwnershipRequestSchema } from '@trek/shared'; @@ -32,8 +33,10 @@ const writeAudit = vi.fn(); const audit = { writeAudit } as unknown as AuditService; // calendar is optional so the call sites that never reach the ICS route stay as // they were; the two that do pass a stub. -const tc = (s: TripsService, calendar?: Partial) => - new TripsController(s, audit, new RuntimeEnvService(), unsplashStub, (calendar ?? {}) as CalendarService); +// calendar and readModel are optional so the call sites that reach neither the +// ICS route nor the bundle stay as they were; the ones that do pass a stub. +const tc = (s: TripsService, calendar?: Partial, readModel?: Partial) => + new TripsController(s, audit, new RuntimeEnvService(), unsplashStub, (calendar ?? {}) as CalendarService, (readModel ?? {}) as TripReadModelService); function svc(o: Partial = {}): TripsService { return { @@ -248,129 +251,11 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { }); }); - describe('members', () => { - it('GET 404 without access, else owner+members+current_user_id', () => { - expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).members(user, '9'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - const s = svc({ listMembers: vi.fn().mockReturnValue({ owner: { id: 1 }, members: [] }) } as Partial); - expect(tc(s).members(user, '9')).toEqual({ owner: { id: 1 }, members: [], current_user_id: 1 }); - }); - - it('POST 403 without member_manage, else adds + notifies', () => { - expect(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' } }); - 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(tc(s).addMember(user, '9', { identifier: 'bob@x.y' })).toEqual({ member: { id: 2, email: 'bob@x.y' } }); - expect(notifyInvite).toHaveBeenCalledWith('9', user, 2, 'T', 'bob@x.y'); - }); - - it('POST 404 without trip access', () => { - const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => tc(s).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); - - it('POST maps NotFoundError to 404, ValidationError to 400, re-throws others', () => { - const nf = svc({ addMember: vi.fn().mockImplementation(() => { throw new NotFoundError('no user'); }) } as Partial); - expect(thrown(() => tc(nf).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 404, body: { error: 'no user' } }); - const ve = svc({ addMember: vi.fn().mockImplementation(() => { throw new ValidationError('already a member'); }) } as Partial); - expect(thrown(() => tc(ve).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 400, body: { error: 'already a member' } }); - const other = svc({ addMember: vi.fn().mockImplementation(() => { throw new Error('boom'); }) } as Partial); - expect(() => tc(other).addMember(user, '9', { identifier: 'bob@x.y' })).toThrow('boom'); - }); - - it('DELETE 404 without trip access', () => { - const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); - - it('DELETE self needs no permission; removing others needs member_manage', () => { - 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(tc(s).removeMember(user, '9', '1')).toEqual({ success: true }); - expect(thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 403, body: { error: 'No permission to remove members' } }); - }); - }); - - describe('POST /:id/transfer (#973)', () => { - it('404 without trip access', () => { - const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); - expect(thrown(() => tc(s).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - }); - - it('403 when the requester is not the owner', () => { - // access.user_id (5) differs from the requesting user (1) - const s = svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) }); - expect(thrown(() => tc(s).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 403, body: { error: 'Only the owner can transfer ownership' } }); - }); - - it('a non-numeric newOwnerId 400s in the ZodValidationPipe', () => { - // The hand-rolled 'newOwnerId is required' 400 moved into the global - // pipe (trips DTO ratchet) — the schema rejects non-numeric ids. - expect(tripTransferOwnershipRequestSchema.safeParse({ newOwnerId: 'nope' }).success).toBe(false); - expect(tripTransferOwnershipRequestSchema.safeParse({}).success).toBe(false); - }); - - it('transfers, audits and broadcasts the refreshed trip', () => { - const transferOwnership = vi.fn().mockReturnValue({ tripTitle: 'Roadtrip', fromEmail: 'a@x.y', toEmail: 'b@x.y' }); - const get = vi.fn().mockReturnValue({ id: 9, user_id: 2 }); - const broadcast = vi.fn(); - const s = svc({ transferOwnership, get, broadcast } as Partial); - expect(tc(s).transferOwnership(user, '9', { newOwnerId: 2 }, req, 'sock')).toEqual({ success: true }); - expect(transferOwnership).toHaveBeenCalledWith('9', 2, user.id); - expect(broadcast).toHaveBeenCalledWith('9', 'trip:updated', { trip: { id: 9, user_id: 2 } }, 'sock'); - }); - - it('maps NotFoundError to 404 and ValidationError to 400', () => { - const nf = svc({ transferOwnership: vi.fn().mockImplementation(() => { throw new NotFoundError('User not found'); }) } as Partial); - expect(thrown(() => tc(nf).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 404, body: { error: 'User not found' } }); - const ve = svc({ transferOwnership: vi.fn().mockImplementation(() => { throw new ValidationError('New owner must be a trip member'); }) } as Partial); - expect(thrown(() => tc(ve).transferOwnership(user, '9', { newOwnerId: 2 }, req))).toEqual({ status: 400, body: { error: 'New owner must be a trip member' } }); - }); - }); - - describe('guests (#1362)', () => { - it('404 without access, 403 for a non-owner, 400 without a name; else creates', () => { - expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).createGuest(user, '9', { name: 'Anna' }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - // access.user_id (5) ≠ requester (1) → not the owner - expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) })).createGuest(user, '9', { name: 'Anna' }))).toEqual({ status: 403, body: { error: 'Only the owner can manage guests' } }); - // A whitespace-only name still 400s with the legacy body — the service - // throws after trimming (the schema only enforces 1..50 chars). - const wsGuest = svc({ createGuest: vi.fn().mockImplementation(() => { throw new ValidationError('Guest name is required'); }) } as Partial); - expect(thrown(() => tc(wsGuest).createGuest(user, '9', { name: ' ' }))).toEqual({ status: 400, body: { error: 'Guest name is required' } }); - const createGuest = vi.fn().mockReturnValue({ member: { id: 7, username: 'Anna', is_guest: true } }); - const s = svc({ createGuest } as Partial); - expect(tc(s).createGuest(user, '9', { name: 'Anna' })).toEqual({ member: { id: 7, username: 'Anna', is_guest: true } }); - expect(createGuest).toHaveBeenCalledWith('9', 'Anna', user.id); - }); - - it('rename: 403 non-owner, 404 when the guest is missing, else success', () => { - expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) })).renameGuest(user, '9', '7', { name: 'Bob' }))).toEqual({ status: 403, body: { error: 'Only the owner can manage guests' } }); - const miss = svc({ renameGuest: vi.fn().mockReturnValue(false) } as Partial); - expect(thrown(() => tc(miss).renameGuest(user, '9', '7', { name: 'Bob' }))).toEqual({ status: 404, body: { error: 'Guest not found' } }); - const ok = svc({ renameGuest: vi.fn().mockReturnValue(true) } as Partial); - expect(tc(ok).renameGuest(user, '9', '7', { name: 'Bob' })).toEqual({ success: true }); - }); - - it('delete: 403 non-owner, 404 when the guest is missing, else success', () => { - expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue({ user_id: 5 }) })).deleteGuest(user, '9', '7'))).toEqual({ status: 403, body: { error: 'Only the owner can manage guests' } }); - const miss = svc({ deleteGuest: vi.fn().mockReturnValue(false) } as Partial); - expect(thrown(() => tc(miss).deleteGuest(user, '9', '7'))).toEqual({ status: 404, body: { error: 'Guest not found' } }); - const ok = svc({ deleteGuest: vi.fn().mockReturnValue(true) } as Partial); - expect(tc(ok).deleteGuest(user, '9', '7')).toEqual({ success: true }); - }); - - it('maps a ValidationError from createGuest to 400', () => { - const ve = svc({ createGuest: vi.fn().mockImplementation(() => { throw new ValidationError('Guest name must be 50 characters or fewer'); }) } as Partial); - expect(thrown(() => tc(ve).createGuest(user, '9', { name: 'x'.repeat(60) }))).toEqual({ status: 400, body: { error: 'Guest name must be 50 characters or fewer' } }); - }); - }); - it('GET /:id/bundle 404 then aggregates', () => { expect(thrown(() => tc(svc({ get: vi.fn().mockReturnValue(undefined) } as Partial)).bundle(user, '9'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); const bundle = vi.fn().mockReturnValue({ trip: { id: 9 }, days: [] }); - const s = svc({ get: vi.fn().mockReturnValue({ user_id: 1 }), bundle } as Partial); - expect(tc(s).bundle(user, '9')).toEqual({ trip: { id: 9 }, days: [] }); + const s = svc({ get: vi.fn().mockReturnValue({ user_id: 1 }) } as Partial); + expect(tc(s, undefined, { bundle }).bundle(user, '9')).toEqual({ trip: { id: 9 }, days: [] }); }); describe('POST /:id/cover', () => { diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 481975f52..0d1a41560 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -7,6 +7,11 @@ * summary/list/create/delete/copy SQL; 051–053 pin the post-fold quirk fixes * (transactional deletes, owner display_name)). Uses a real in-memory SQLite * DB so SQL logic is exercised faithfully. + * + * The membership and read-aggregate cases now drive TripMembersService and + * TripReadModelService, which is where that code went when the aggregate root + * was split. They stayed in this file, over the same in-memory DB, so the diff + * shows the move rather than a rewrite. */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -65,6 +70,8 @@ import { VacayService } from '../../../src/nest/vacay/vacay.service'; import { TripsService } from '../../../src/nest/trips/trips.service'; import { PlacesService } from '../../../src/nest/places/places.service'; import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; +import { TripMembersService } from '../../../src/nest/trip-members/trip-members.service'; +import { TripReadModelService } from '../../../src/nest/trip-read-model/trip-read-model.service'; import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; import { MapsService } from '../../../src/nest/maps/maps.service'; import { getTripOwner, listMembers as bridgeListMembers } from '../../../src/nest/trips/trips.bridge'; @@ -82,21 +89,25 @@ const createAccommodation = accommodationsSvc.createAccommodation.bind(accommoda const svc = new TripsService( dbs(), - new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService()), new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService()), daysSvc, new PermissionsService(dbs()), budgetSvc, - new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService()), new VacayService(dbs(), new RealtimeService()), new RealtimeService(), - placesSvc, undefined as never, // unsplash — not exercised here - new UserCleanupService(dbs()), - accommodationsSvc, ); +const membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs()), new PermissionsService(dbs()), new RealtimeService()); +const readModelSvc = new TripReadModelService( + dbs(), membersSvc, daysSvc, accommodationsSvc, budgetSvc, + new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService()), + new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + placesSvc, + new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), + new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService()), +); + beforeAll(() => { createTables(testDb); @@ -492,7 +503,7 @@ describe('transferOwnership (#973)', () => { const trip = createTrip(testDb, owner.id); addTripMember(testDb, trip.id, member.id); - const result = svc.transferOwnership(trip.id, member.id, owner.id); + const result = membersSvc.transferOwnership(trip.id, member.id, owner.id); expect(result.toEmail).toBe(member.email); const updated = testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }; @@ -510,20 +521,20 @@ describe('transferOwnership (#973)', () => { const trip = createTrip(testDb, owner.id); addTripMember(testDb, trip.id, member.id); // member (not the owner) attempts the transfer - expect(() => svc.transferOwnership(trip.id, member.id, member.id)).toThrow(); + expect(() => membersSvc.transferOwnership(trip.id, member.id, member.id)).toThrow(); }); it('TRIP-SVC-022: rejects a transfer to someone who is not a member', () => { const { user: owner } = createUser(testDb); const { user: stranger } = createUser(testDb); const trip = createTrip(testDb, owner.id); - expect(() => svc.transferOwnership(trip.id, stranger.id, owner.id)).toThrow('New owner must be a trip member'); + expect(() => membersSvc.transferOwnership(trip.id, stranger.id, owner.id)).toThrow('New owner must be a trip member'); }); it('TRIP-SVC-023: rejects transferring to yourself', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - expect(() => svc.transferOwnership(trip.id, owner.id, owner.id)).toThrow('You already own this trip'); + expect(() => membersSvc.transferOwnership(trip.id, owner.id, owner.id)).toThrow('You already own this trip'); }); }); @@ -532,7 +543,7 @@ describe('guest members (#1362)', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - const { member } = svc.createGuest(trip.id, ' Anna ', owner.id); + const { member } = membersSvc.createGuest(trip.id, ' Anna ', owner.id); expect(member.username).toBe('Anna'); expect(member.is_guest).toBe(true); @@ -547,7 +558,7 @@ describe('guest members (#1362)', () => { expect(m).toBeTruthy(); // Surfaces in listMembers with is_guest=true and the typed display name. - const { members } = svc.listMembers(trip.id, owner.id) as any; + const { members } = membersSvc.listMembers(trip.id, owner.id) as any; const guest = members.find((x: any) => x.id === member.id); expect(guest.username).toBe('Anna'); expect(guest.is_guest).toBe(true); @@ -556,8 +567,8 @@ describe('guest members (#1362)', () => { it('TRIP-SVC-031: the same guest name is allowed, not suffixed (#1446)', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - const a = svc.createGuest(trip.id, 'Sam', owner.id); - const b = svc.createGuest(trip.id, 'Sam', owner.id); + const a = membersSvc.createGuest(trip.id, 'Sam', owner.id); + const b = membersSvc.createGuest(trip.id, 'Sam', owner.id); // both keep the plain display name; only the internal (uuid) username differs expect(a.member.username).toBe('Sam'); expect(b.member.username).toBe('Sam'); @@ -571,26 +582,26 @@ describe('guest members (#1362)', () => { const { user: other } = createUser(testDb); const otherTrip = createTrip(testDb, other.id); const trip = createTrip(testDb, owner.id); - const { member } = svc.createGuest(trip.id, 'Bob', owner.id); + const { member } = membersSvc.createGuest(trip.id, 'Bob', owner.id); - expect(svc.renameGuest(trip.id, member.id, 'Robert')).toBe(true); + expect(membersSvc.renameGuest(trip.id, member.id, 'Robert')).toBe(true); expect((testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(member.id) as any).display_name).toBe('Robert'); // A real user cannot be renamed through the guest path… - expect(svc.renameGuest(trip.id, owner.id, 'Hacked')).toBe(false); + expect(membersSvc.renameGuest(trip.id, owner.id, 'Hacked')).toBe(false); // …and a guest cannot be renamed from a different trip. - expect(svc.renameGuest(otherTrip.id, member.id, 'Nope')).toBe(false); + expect(membersSvc.renameGuest(otherTrip.id, member.id, 'Nope')).toBe(false); }); it('TRIP-SVC-033: deleteGuest removes the user (cascading membership), guest-only + trip-scoped', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - const { member } = svc.createGuest(trip.id, 'Carol', owner.id); + const { member } = membersSvc.createGuest(trip.id, 'Carol', owner.id); // Real members are not deletable via the guest path. - expect(svc.deleteGuest(trip.id, owner.id)).toBe(false); + expect(membersSvc.deleteGuest(trip.id, owner.id)).toBe(false); - expect(svc.deleteGuest(trip.id, member.id)).toBe(true); + expect(membersSvc.deleteGuest(trip.id, member.id)).toBe(true); expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(member.id)).toBeUndefined(); expect(testDb.prepare('SELECT id FROM trip_members WHERE user_id = ?').get(member.id)).toBeUndefined(); }); @@ -598,12 +609,12 @@ describe('guest members (#1362)', () => { it('TRIP-SVC-034: a guest is never invitable (addMember) nor a transfer target', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - const { member } = svc.createGuest(trip.id, 'Dora', owner.id); + const { member } = membersSvc.createGuest(trip.id, 'Dora', owner.id); // The synthetic username/email must not resolve through the invite box. - expect(() => svc.addMember(trip.id, 'Dora', owner.id, owner.id)).toThrow('User not found'); + expect(() => membersSvc.addMember(trip.id, 'Dora', owner.id, owner.id)).toThrow('User not found'); // Ownership can never be handed to a guest. - expect(() => svc.transferOwnership(trip.id, member.id, owner.id)).toThrow('Cannot transfer ownership to a guest'); + expect(() => membersSvc.transferOwnership(trip.id, member.id, owner.id)).toThrow('Cannot transfer ownership to a guest'); }); }); @@ -639,7 +650,7 @@ describe('folded trip CRUD', () => { testDb.prepare("INSERT INTO budget_items (trip_id, category, name, total_price) VALUES (?, 'food', 'Dinner', 40)").run(trip.id); testDb.prepare("INSERT INTO packing_items (trip_id, name, checked) VALUES (?, 'Socks', 1)").run(trip.id); - const summary = svc.getTripSummary(trip.id, owner.id)!; + const summary = readModelSvc.getTripSummary(trip.id, owner.id)!; expect(summary).toBeTruthy(); expect((summary.trip as any).id).toBe(trip.id); expect(summary.members.owner.id).toBe(owner.id); @@ -653,7 +664,7 @@ describe('folded trip CRUD', () => { expect(summary.collab_notes).toEqual([]); // Missing trips return null instead of throwing. - expect(svc.getTripSummary(99999)).toBeNull(); + expect(readModelSvc.getTripSummary(99999)).toBeNull(); }); it('TRIP-SVC-043: list returns owned + shared trips with is_owner, honoring the archived filter', () => { @@ -781,14 +792,14 @@ describe('TripsService wrapper helpers', () => { testDb.prepare("INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, 'Secret', 1, ?)").run(trip.id, owner.id); testDb.prepare("INSERT INTO packing_items (trip_id, name) VALUES (?, 'Shared')").run(trip.id); - const result = svc.bundle(String(trip.id), { user_id: owner.id }, viewer.id) as any; + const result = readModelSvc.bundle(String(trip.id), { user_id: owner.id }, viewer.id) as any; expect(result.days).toHaveLength(2); expect(result.members.map((m: any) => m.id).sort()).toEqual([owner.id, viewer.id].sort()); expect(result.packingItems.map((p: any) => p.name)).toEqual(['Shared']); }); it('notifyInvite is fire-and-forget (no throw)', () => { - expect(() => svc.notifyInvite('9', { id: 1, email: 'a@b.c' } as never, 2, 'T', 'b@x.y')).not.toThrow(); + expect(() => membersSvc.notifyInvite('9', { id: 1, email: 'a@b.c' } as never, 2, 'T', 'b@x.y')).not.toThrow(); }); }); @@ -820,17 +831,17 @@ describe('folded quirk branches', () => { const { user: invitee } = createUser(testDb); const trip = createTrip(testDb, owner.id, { title: 'Joinable' }); - const result = svc.addMember(trip.id, invitee.email, owner.id, owner.id); + const result = membersSvc.addMember(trip.id, invitee.email, owner.id, owner.id); expect(result.member.id).toBe(invitee.id); expect(result.tripTitle).toBe('Joinable'); expect(result.targetUserId).toBe(invitee.id); // Duplicate + owner + missing identifier reject with the byte-identical errors. - expect(() => svc.addMember(trip.id, invitee.email, owner.id, owner.id)).toThrow('User already has access'); - expect(() => svc.addMember(trip.id, owner.email, owner.id, owner.id)).toThrow('Trip owner is already a member'); - expect(() => svc.addMember(trip.id, '', owner.id, owner.id)).toThrow('Email or username required'); + expect(() => membersSvc.addMember(trip.id, invitee.email, owner.id, owner.id)).toThrow('User already has access'); + expect(() => membersSvc.addMember(trip.id, owner.email, owner.id, owner.id)).toThrow('Trip owner is already a member'); + expect(() => membersSvc.addMember(trip.id, '', owner.id, owner.id)).toThrow('Email or username required'); - svc.removeMember(trip.id, invitee.id); + membersSvc.removeMember(trip.id, invitee.id); expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, invitee.id)).toBeUndefined(); }); @@ -886,7 +897,7 @@ describe('folded quirk branches', () => { describe('quirk fixes', () => { /** A TripsService whose connection throws when preparing SQL matching `match`. */ - function failingTrips(match: string) { + function failingConnection(match: string) { const conn = new Proxy(testDb, { get(target, prop) { if (prop === 'prepare') { @@ -899,23 +910,28 @@ describe('quirk fixes', () => { return typeof v === 'function' ? v.bind(target) : v; }, }); - const fdbs = { connection: conn, canAccessTrip: dbMock.canAccessTrip, isOwner: dbMock.isOwner } as unknown as import('../../../src/nest/database/database.service').DatabaseService; + return { connection: conn, canAccessTrip: dbMock.canAccessTrip, isOwner: dbMock.isOwner } as unknown as import('../../../src/nest/database/database.service').DatabaseService; + } + + function failingTrips(match: string) { + const fdbs = failingConnection(match); return new TripsService( fdbs, - new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService()), new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService()), daysSvc, new PermissionsService(dbs()), budgetSvc, - new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService()), new VacayService(dbs(), new RealtimeService()), new RealtimeService(), - placesSvc, - undefined as never, // unsplash — not exercised here - new UserCleanupService(dbs()), - new AccommodationsService(fdbs, new PermissionsService(dbs()), new RealtimeService()), + undefined as never, + ); + } + + /** Same frozen connection, for the guest deletion that now lives on the roster. */ + function failingMembers(match: string) { + return new TripMembersService( + failingConnection(match), budgetSvc, new UserCleanupService(dbs()), + new PermissionsService(dbs()), new RealtimeService(), ); } @@ -940,10 +956,10 @@ describe('quirk fixes', () => { it('TRIP-SVC-052: deleteGuest is atomic — a failed user DELETE rolls the budget re-split back', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - const { member: guest } = svc.createGuest(trip.id, 'Gia', owner.id); + const { member: guest } = membersSvc.createGuest(trip.id, 'Gia', owner.id); const item = budgetSvc.createBudgetItem(trip.id, { name: 'Dinner', total_price: 80, member_ids: [owner.id, guest.id] }); - const broken = failingTrips('DELETE FROM users WHERE id = ? AND is_guest = 1'); + const broken = failingMembers('DELETE FROM users WHERE id = ? AND is_guest = 1'); expect(() => broken.deleteGuest(trip.id, guest.id)).toThrow('boom'); // Neither the guest nor their split membership was touched. @@ -956,7 +972,7 @@ describe('quirk fixes', () => { const { user: owner } = createUser(testDb, { username: 'owner-handle' }); testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Olive Displayed', owner.id); const trip = createTrip(testDb, owner.id); - const { owner: row } = svc.listMembers(trip.id, owner.id); + const { owner: row } = membersSvc.listMembers(trip.id, owner.id); expect(row.username).toBe('Olive Displayed'); }); }); diff --git a/server/tests/unit/plugins/rpc-fallbacks.test.ts b/server/tests/unit/plugins/rpc-fallbacks.test.ts index 209086fb8..d5682dcb8 100644 --- a/server/tests/unit/plugins/rpc-fallbacks.test.ts +++ b/server/tests/unit/plugins/rpc-fallbacks.test.ts @@ -145,7 +145,7 @@ describe('service errors are translated into the RPC taxonomy', () => { create: vi.fn(() => { if (createThrows) throw createThrows; return { trip: { id: 99 } }; }), removeMember: vi.fn(), } as never; - const rpc = new TripsRpc(trips, {} as never, {} as never, {} as never, db, realtime(), guards, {} as never); + const rpc = new TripsRpc(trips, {} as never, {} as never, {} as never, db, realtime(), guards, {} as never, {} as never); return new PluginRpcHost('p', ALL, makeDeps(), createTestPluginRegistry([rpc])); } @@ -246,7 +246,7 @@ describe('every schema-validated method rejects a payload its schema refuses', ( new PlacesRpc(anything, anything, realtime(), guards), new DaysRpc(anything, realtime(), guards), new PackingRpc(anything, realtime(), guards), - new TripsRpc(anything, anything, anything, anything, db, realtime(), guards, anything), + new TripsRpc(anything, anything, anything, anything, db, realtime(), guards, anything, anything), new AccommodationsRpc(anything, realtime(), guards), new ReservationsRpc(anything, realtime(), guards), ]); diff --git a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts index fbaf61fd1..824e4100d 100644 --- a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts +++ b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts @@ -32,7 +32,7 @@ const tripsRegistry = () => { } as unknown as DatabaseService; const guards = new PluginGuards(db, {} as never, {} as never); return createTestPluginRegistry([ - new TripsRpc({} as never, {} as never, {} as never, {} as never, db, {} as never, guards, {} as never), + new TripsRpc({} as never, {} as never, {} as never, {} as never, db, {} as never, guards, {} as never, {} as never), ]); }; diff --git a/server/tests/unit/trips/trips.rpc.test.ts b/server/tests/unit/trips/trips.rpc.test.ts index 207f51ab7..f5fc0c655 100644 --- a/server/tests/unit/trips/trips.rpc.test.ts +++ b/server/tests/unit/trips/trips.rpc.test.ts @@ -19,6 +19,7 @@ import type { TripsService } from '../../../src/nest/trips/trips.service'; import type { ReservationsService } from '../../../src/nest/reservations/reservations.service'; import type { DaysService } from '../../../src/nest/days/days.service'; import type { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; +import type { TripMembersService } from '../../../src/nest/trip-members/trip-members.service'; import type { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import type { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; @@ -50,6 +51,10 @@ export function build(opts: { allow?: (action: string) => boolean; updateThrows? const accommodations = { list: vi.fn(() => [{ id: 11 }]), } as unknown as AccommodationsService & Record>; + // Membership left TripsService for its own domain. + const roster = { + removeMember: vi.fn(), + } as unknown as TripMembersService & Record>; const membership = { joinTripAsMember: vi.fn(() => ({ joined: true })) } as unknown as TripMembershipService & Record>; const db = { canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), @@ -65,10 +70,10 @@ export function build(opts: { allow?: (action: string) => boolean; updateThrows? const permissions = { checkPermission: vi.fn((a: string) => (opts.allow ? opts.allow(a) : true)) } as unknown as PermissionsService; const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const guards = new PluginGuards(db, permissions, { isAddonEnabled: vi.fn(() => true) } as unknown as AddonsService); - const rpc = new TripsRpc(trips, reservations, days, membership, db, realtime, guards, accommodations); + const rpc = new TripsRpc(trips, reservations, days, membership, db, realtime, guards, accommodations, roster); const host = (...grants: string[]) => new PluginRpcHost('p', new Set(grants.length ? grants : ALL_TRIP_GRANTS), makeDeps(), createTestPluginRegistry([rpc])); - return { trips, reservations, days, accommodations, membership, realtime, permissions, host }; + return { trips, reservations, days, accommodations, roster, membership, realtime, permissions, host }; } describe('TripsRpc reads', () => { @@ -192,13 +197,13 @@ describe('TripsRpc membership', () => { // The stub reports user 42 as the owner of trip 1. 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.trips.removeMember).not.toHaveBeenCalled(); + expect(f.roster.removeMember).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.trips.removeMember).toHaveBeenCalledWith(1, 7); + expect(f.roster.removeMember).toHaveBeenCalledWith(1, 7); }); it('TRIPS-RPC-018 without member_manage nothing changes', async () => { diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 7d5dc0c5c..f01618522 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -112,6 +112,8 @@ export default defineConfig({ 'src/nest/transit/**/*.ts': { statements: 92, branches: 83, functions: 97, lines: 94 }, 'src/nest/trip-invite/**/*.ts': { statements: 91, branches: 93, functions: 93, lines: 89 }, 'src/nest/trip-membership/**/*.ts': { statements: 99, branches: 86, functions: 99, lines: 99 }, + 'src/nest/trip-members/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, + 'src/nest/trip-read-model/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, 'src/nest/trips/**/*.ts': { statements: 92, branches: 82, functions: 91, lines: 94 }, 'src/nest/unsplash/**/*.ts': { statements: 97, branches: 82, functions: 99, lines: 96 }, 'src/nest/vacay/**/*.ts': { statements: 82, branches: 66, functions: 89, lines: 86 },