From 94c6efbbcc3d35d95e5f8f4add3c9106c4189e58 Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 08:00:39 +0200 Subject: [PATCH] refactor(trip-members): membership, guests and the trip read model through repositories --- server/src/db/dialect/sql-functions.ts | 23 ++ .../db/repositories/TripMembers.repository.ts | 192 +++++++++++++++- .../src/db/repositories/Trips.repository.ts | 92 ++++++++ .../src/db/repositories/Users.repository.ts | 132 ++++++++++- .../nest/trip-members/trip-members.module.ts | 15 +- .../nest/trip-members/trip-members.service.ts | 92 ++++---- .../trip-read-model/trip-read-model.module.ts | 12 +- .../trip-read-model.service.ts | 24 +- server/tests/helpers/mcp-test-controllers.ts | 11 +- server/tests/helpers/plugin-host.ts | 11 +- .../unit/db/dialect/sql-functions.test.ts | 27 +++ .../TripMembers.repository.test.ts | 206 ++++++++++++++++++ .../db/repositories/Trips.repository.test.ts | 83 +++++++ .../db/repositories/Users.repository.test.ts | 122 +++++++++++ .../tests/unit/nest/budget.service.db.test.ts | 5 +- .../unit/nest/trip-members.service.test.ts | 73 +++---- .../unit/nest/trip-read-model.service.test.ts | 74 ++++--- server/tests/unit/nest/trips.service.test.ts | 35 +-- 18 files changed, 1077 insertions(+), 152 deletions(-) diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index 4026cd124..4af5cadb4 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -266,3 +266,26 @@ export function maxOf(platform: Platform, ref: string, aliasName: string): RawQu return unsupported(platform); } +// --------------------------------------------------------------------------- +// Plan 3c Task 6 — `TripMembersRepository.listWithUserAndInviter` +// (`trip-members.service.ts:117`, TM2): the `role` column, a `CASE WHEN` +// comparing the joined user's id against the trip's owner id. +// --------------------------------------------------------------------------- + +/** + * `CASE WHEN = ? THEN ? ELSE ? END`, for use as a `.select()` value + * (chain `.as(aliasName)` at the call site, the same way `coalesce()`'s + * callers do — this file does not bake the alias into the SQL text itself + * for this one, unlike `countAll`/`minOf`/`maxOf`, because `.as()` already + * covers it and a `value`-bearing fragment cannot reuse `alias()`'s bare + * identifier validation the same way an aggregate's literal alias does). + * `value`/`whenTrue`/`whenFalse` are all bound as parameters, never + * interpolated — even though today's only caller (TM2's owner/member role + * label) passes fixed literal strings, parity is by VALUE, not by whether + * the string happens to be a source-code literal at the call site. + */ +export function caseWhenEquals(platform: Platform, ref: string, value: number, whenTrue: string, whenFalse: string): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw(`CASE WHEN ${column(ref)} = ? THEN ? ELSE ? END`, [value, whenTrue, whenFalse]); + return unsupported(platform); +} + diff --git a/server/src/db/repositories/TripMembers.repository.ts b/server/src/db/repositories/TripMembers.repository.ts index 800cd886e..6ab99b07c 100644 --- a/server/src/db/repositories/TripMembers.repository.ts +++ b/server/src/db/repositories/TripMembers.repository.ts @@ -1,7 +1,25 @@ import type { TripMembers } from '../entities/TripMembers.entity'; import { Trips } from '../entities/Trips.entity'; +import { caseWhenEquals, coalesce } from '../dialect/sql-functions'; import { TrekRepository } from './_shared/trek-repository'; +/** + * One row of `TripMembersService.listMembers` (`trip-members.service.ts:117-125`, + * TM2). `is_guest` stays the raw SQLite integer here — the `!!m.is_guest` + * boolean coercion is a service-layer default (D4), same split every other + * repository in this program keeps. + */ +export interface MemberWithUserAndInviterRow { + id: number; + username: string; + email: string; + avatar: string | null; + is_guest: number; + role: string; + added_at: string | null; + invited_by_username: string | null; +} + export class TripMembersRepository extends TrekRepository { /** * The user ids a trip may refer to: its members plus the owner. Byte-for-byte @@ -61,26 +79,182 @@ export class TripMembersRepository extends TrekRepository { /** * `SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?` - * (`trip-membership.service.ts:70`, TB5) — `joinTripAsMember`'s - * already-a-member check, the second leg of the non-transactional - * check-then-act sequence (§18.4: unchanged by this conversion). Both - * callers (`TripMembershipService.joinTripAsMember`) always pass real - * `number`s, so no raw-bind seam here. + * (`trip-membership.service.ts:70`, TB5 — `joinTripAsMember`'s + * already-a-member check, one leg of a non-transactional check-then-act + * sequence, §18.4: unchanged by this conversion; also `trip-members + * .service.ts:151`, TM5 — `addMember`'s own "already has access" check, + * the SAME non-transactional shape against TM6's insert). `TripMembershipService + * .joinTripAsMember`'s `tripId` is always a real `number`; `TripMembersService + * .addMember`'s is still the route's unconverted `tripId` + * (`string | number`) — raw-bind (D4's T5 escape hatch, the same seam + * `TripsRepository.findAccessible` documents) covers both without forking + * the statement into two methods. */ - async exists(trip_id: number, user_id: number): Promise { + async exists(trip_id: number | string, user_id: number): Promise { const row = await this.qb('m') .select(['m.id']) - .where({ trip: trip_id, user: user_id }) + .where('m.trip_id = ?', [trip_id]) + .andWhere({ user: user_id }) .execute<{ id: number } | undefined>('get', false); return !!row; } /** * `INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)` - * (`trip-membership.service.ts:72`, TB6) — the third leg of the same - * non-transactional sequence as `exists` above; `invitedBy` may be `null`. + * (`trip-membership.service.ts:72`, TB6 — the third leg of the same + * non-transactional sequence as `exists` above; also `trip-members + * .service.ts:155`, TM6 — `addMember`'s own insert, and `:227`, TM17 — + * `createGuest`'s membership row for the new guest, inside its own + * transaction). `invitedBy` may be `null`. `trip_id: number`, not + * `number | string`: unlike `exists`'s WHERE-comparison shape, `insert`'s + * typed `EntityData` requires the relation's real `Primary` type. + * This is still exact parity for `TripMembersService`'s two still-string + * `tripId` callers (TM6/TM17): by the time either call happens, the + * route's `tripId` has already matched a real trip through one of the + * four raw-bind access primitives earlier in the SAME request (the + * controller's inline `canAccessTrip` check for `addMember`; + * `TripOwnerGuard`'s `isOwner`/`findAccessible` for `createGuest`'s + * owner-only route) — which only succeeds for a string SQLite's own + * affinity rules already recognise as numeric, and `Number(...)` of + * exactly that class of string always agrees with SQLite's own + * numeric-affinity conversion. The two call sites document this and pass + * `Number(tripId)`, not a widened raw bind. */ async addMember(trip_id: number, user_id: number, invitedBy: number | null): Promise { await this.insert({ trip: trip_id, user: user_id, invited_by: invitedBy }); } + + /** + * `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` (`trip-members.service.ts:117-125`, TM2) — + * the self-join on `users` (once as `u` for the member, once as `ib` for + * the inviter) via `TripMembers.entity.ts`'s `invitedByRef` relation, + * which exists precisely for this join. Every joined-alias column is + * selected with an explicit inline `as ` (the `PlaceRatingsRepository + * .listForPlaces`/`AssignmentParticipantsRepository.insertIgnore` + * precedent — rule 19's trap: selecting a joined alias's bare `.id` + * without an explicit alias risks MikroORM's own `__` prefix + * for a relation-property select, verified NOT to reproduce the legacy's + * bare `id`/`email`/`avatar`/`is_guest`/`added_at` keys otherwise), never + * left to MikroORM's own column-collision aliasing. + */ + async listWithUserAndInviter(trip_id: number | string, owner_id: number): Promise { + const platform = this.getEntityManager().getPlatform(); + return this.qb('m') + .join('m.user', 'u') + .leftJoin('m.invitedByRef', 'ib') + .select([ + 'u.id as id', + coalesce(platform, 'u.display_name', 'u.username').as('username'), + 'u.email as email', + 'u.avatar as avatar', + 'u.is_guest as is_guest', + caseWhenEquals(platform, 'u.id', owner_id, 'owner', 'member').as('role'), + 'm.added_at as added_at', + coalesce(platform, 'ib.display_name', 'ib.username').as('invited_by_username'), + ]) + .where('m.trip_id = ?', [trip_id]) + .orderBy({ 'm.added_at': 'asc' }) + .execute('all', false); + } + + /** + * `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` + * (`trip-members.service.ts:235-237`, TM18) — security-sensitive: the + * scoping guard that keeps guest mutations trip-local (#1362). Rooted on + * `TripMembers` (this repository's entity) rather than `Users`: the + * legacy statement's `users u JOIN trip_members m` becomes `trip_members m + * JOIN users u` here, the same join and the same result set, only rooted + * the other way because that is this repository's entity. + * + * Raw-bind on `user_id` (not `.andWhere({ user: user_id })`) plus a + * `Number.isFinite` short-circuit guard — the same reason `remove()` below + * has both: `renameGuest`/`deleteGuest`'s callers still do a bare + * `Number.parseInt(userId)` on the route's `:userId`, unvalidated — a + * non-numeric id parses to `NaN`, and MikroORM/Kysely inline a non-finite + * `number` as the literal SQL token `NaN` even through a raw `?` + * placeholder (`no such column: NaN`, a 500 at PREPARE time — verified + * directly against a compiled boot, not assumed; a typed filter has the + * identical failure). The legacy statement bound it as a genuine + * better-sqlite3 parameter, matching zero rows (404 `'Guest not found'`, + * not a crash) — only skipping the query for a non-finite `user_id` + * reproduces that byte for byte. + */ + async isGuestOfTrip(trip_id: number | string, user_id: number): Promise { + if (!Number.isFinite(user_id)) return false; + const row = await this.qb('m') + .join('m.user', 'u') + .select(['u.id as id']) + .where('m.trip_id = ?', [trip_id]) + .andWhere('m.user_id = ?', [user_id]) + .andWhere({ 'u.is_guest': 1 }) + .execute<{ id: number } | undefined>('get', false); + return !!row; + } + + /** + * `INSERT OR IGNORE INTO trip_members (trip_id, user_id, invited_by) + * VALUES (?, ?, ?)` (`trip-members.service.ts:200`, TM15) — + * security-sensitive: `transferOwnership`'s third statement, giving the + * former owner membership back. `upsert` with `onConflictAction: 'ignore'` + * on the table's real unique key, named by entity property (`trip`/`user`, + * `AssignmentParticipantsRepository.insertIgnore`'s precedent for a + * relation-backed `onConflictFields` list) — `UNIQUE(trip_id, user_id)` + * per `TripMembersSchema`'s own `uniques` declaration, not assumed. + * `trip_id: number`, the same reasoning as `addMember`'s docstring above + * (an `upsert` is INSERT-shaped; the caller passes `Number(tripId)`, + * justified there — `transferOwnership`'s route is `TripOwnerGuard`-gated, + * so `tripId` has already matched a real trip through the raw-bind + * `isOwner`/`findAccessible` primitive earlier in the same request). + */ + async addIgnoringConflict(trip_id: number, user_id: number, invitedBy: number | null): Promise { + // `invitedByRef`, not `invited_by`: the latter is a `persist(false)` mirror + // column (like `trip_id`/`user_id`) that `upsert` — unlike `insert` above, + // per `addMember`'s test — silently drops when building its INSERT. + // `invitedByRef` is the real, joined-column-backed relation property. + await this.upsert( + { trip: trip_id, user: user_id, invitedByRef: invitedBy }, + { onConflictFields: ['trip', 'user'], onConflictAction: 'ignore' }, + ); + } + + /** + * `DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?` + * (`trip-members.service.ts:166`, TM8 `removeMember`; `:198`, TM14 + * `transferOwnership`'s "no longer a plain member" step — + * security-sensitive). Raw-bind on BOTH `trip_id` and `user_id` — not just + * `trip_id` (D4's T5 escape hatch, the same `number | string` seam + * `rosterUserIds`/`listUserIdsByTrip` above preserve): `TripMembersController + * .removeMember` still does a bare `Number.parseInt(userId)` on the route's + * `:userId` before calling this, unvalidated (rule 15's exact trap — a + * non-numeric id parses to `NaN`). + * + * **`Number.isFinite` guard, checked directly against a compiled boot, not + * assumed:** a typed `.andWhere({ user: user_id })` filter renders `NaN` as + * a literal, unquoted SQL token (`WHERE … AND user_id = NaN`), which + * SQLite's parser reads as a column reference and rejects at PREPARE time + * (`no such column: NaN`, a 500). Switching to a raw `m.user_id = ?` + * placeholder does NOT fix this — MikroORM/Kysely inline a non-finite + * `number` as that same literal token even through a `?` placeholder's + * bound-params array (reproduced directly: still `no such column: NaN` at + * prepare time, before any bind happens). The legacy statement bound + * `targetUserId` (`NaN`) straight into a real better-sqlite3 parameter, + * which silently matches zero rows and returns 200 `{ success: true }` — + * only skipping the query entirely for a non-finite `user_id` reproduces + * that, byte for byte, since neither the typed filter nor the raw + * placeholder route can pass `NaN` through the ORM layer intact. + */ + async remove(trip_id: number | string, user_id: number): Promise { + if (!Number.isFinite(user_id)) return; + await this.qb('m') + .delete() + .where('m.trip_id = ?', [trip_id]) + .andWhere('m.user_id = ?', [user_id]) + .execute('run'); + } } diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index d24a157df..cac53b4f4 100644 --- a/server/src/db/repositories/Trips.repository.ts +++ b/server/src/db/repositories/Trips.repository.ts @@ -1,4 +1,5 @@ import type { Trips } from '../entities/Trips.entity'; +import type { AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; /** What a trip-scoped request learns about the trip once access is verified. */ @@ -8,6 +9,32 @@ export interface TripAccess { currency: string | null; } +/** + * `SELECT * FROM trips WHERE id = ?`, every scalar column — the row + * `TripReadModelService.getTripSummary` (TR-B) and, once Task 7 lands, + * `TripsService.getRaw` (TP22) both need, `feed_token` included: the + * `withoutFeedToken()` JS strip that guards the credential stays in the + * SERVICE (TR-B's ruling), so this repository method must hand the column + * back intact for that strip to have something to delete. + */ +export interface TripRawRow { + id: number; + user_id: number; + title: string; + description: string | null; + start_date: string | null; + end_date: string | null; + currency: string | null; + cover_image: string | null; + is_archived: number | null; + reminder_days: number | null; + feed_token: string | null; + created_at: string | null; + updated_at: string | null; +} + +const _tripRawRowKeys: AssertRowKeys = true; + export class TripsRepository extends TrekRepository { /** * The `LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? @@ -132,4 +159,69 @@ export class TripsRepository extends TrekRepository { .execute<{ id: number }[]>('all', false); return rows.map((r) => r.id); } + + /** + * `SELECT title FROM trips WHERE id = ?` (`trip-members.service.ts:156`, + * TM7) — `addMember`'s notification-body read; the caller applies its own + * `?? 'Untitled'` fallback (D4). Raw-bind (D4's T5 escape hatch), the same + * `number | string` seam `findAccessible` documents. + */ + async getTitle(trip_id: number | string): Promise { + const row = await this.qb('t') + .select(['t.title']) + .where('t.id = ?', [trip_id]) + .execute<{ title: string } | undefined>('get', false); + return row?.title ?? null; + } + + /** + * `SELECT id, title, user_id FROM trips WHERE id = ?` + * (`trip-members.service.ts:180`, TM9) — `transferOwnership`'s first + * check-then-act read (miss → 404 `'Trip not found'`; wrong owner → 400 + * `'Only the owner can transfer ownership'`, both decided by the caller). + * Raw-bind, the same seam as `getTitle` above. + */ + async findIdTitleOwner(trip_id: number | string): Promise<{ id: number; title: string; user_id: number } | undefined> { + return this.qb('t') + .select(['t.id', 't.title', 't.user']) + .where('t.id = ?', [trip_id]) + .execute<{ id: number; title: string; user_id: number } | undefined>('get', false); + } + + /** + * `UPDATE trips SET user_id = ? WHERE id = ?` (`trip-members.service.ts:196`, + * TM13) — security-sensitive: the ownership handover itself, the first of + * `transferOwnership`'s three transactional statements. `qb().update()` + * rather than `nativeUpdate`: `nativeUpdate`'s typed `FilterQuery` rejects + * a `string` against `id`'s branded `number` type the same way `find`'s + * does, and `trip_id` here is still the route's unconverted string. + */ + async setOwner(trip_id: number | string, user_id: number): Promise { + await this.qb('t') + .update({ user: user_id }) + .where('t.id = ?', [trip_id]) + .execute('run'); + } + + /** + * `SELECT * FROM trips WHERE id = ?` — `TripReadModelService.getTripSummary`'s + * trip read (`trip-read-model.service.ts:52`, TR-B) and, once Task 7 lands, + * `TripsService.getRaw` (TP22, `trips.service.ts:389`) — one method, two + * sites, per the inventory's own note that they are the identical + * statement. `feed_token` comes back intact; `withoutFeedToken()` stays a + * JS strip in each CALLER (TR-B's ruling — this repository never blanks + * it itself, unlike `TRIP_SELECT`'s `NULL AS feed_token` SQL-side trick). + * `.select(['t.*'])` — MikroORM's QueryBuilder recognises the bare + * `.*` marker (verified against `QueryBuilder.js`'s own + * `prepareFields`, not assumed) and emits every column unprefixed, the + * same `SELECT t.*` shape the legacy statement used. Raw-bind, the same + * `number | string` seam `getTitle`/`findIdTitleOwner` above preserve. + */ + async findRaw(trip_id: number | string): Promise { + const row = await this.qb('t') + .select(['t.*']) + .where('t.id = ?', [trip_id]) + .execute('get', false); + return row ?? null; + } } diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 31159f280..768967da0 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -1,6 +1,6 @@ import { Users } from '../entities/Users.entity'; import { toRow, type AssertRowKeys } from './_shared/rows'; -import { columnIncrementedBy, currentTimestamp, lower, lowerParam } from '../dialect/sql-functions'; +import { coalesce, columnIncrementedBy, currentTimestamp, lower, lowerParam } from '../dialect/sql-functions'; import { TrekRepository } from './_shared/trek-repository'; /** @@ -78,6 +78,22 @@ export interface NewUserRow { avatar?: string | null; } +/** + * `TripMembersService.createGuest` (TM16, `trip-members.service.ts:222-224`) — + * a credential-less guest account (#1362). Additive, NOT a widening of + * `NewUserRow` above (Plan 3b Task 1's shape, covering only the AU10/O14 + * real-account column sets): this method's `password_hash`/`role`/`is_guest` + * are fixed literals the caller can never override, which is the whole + * point — a second, narrower method keeps the "credential-less account" + * invariant IN the method rather than in caller discipline (Task 6 brief's + * explicit ruling). + */ +export interface NewGuestUserRow { + username: string; + email: string; + display_name: string; +} + /** `UserProfileService.updateApiKeys` (UP3) — the four encrypted key columns, already resolved by the service. */ export interface ApiKeysPatch { maps_api_key: string | null; @@ -921,6 +937,120 @@ export class UsersRepository extends TrekRepository { return row ? { role: row.role, openweather_api_key: row.openweather_api_key ?? null } : null; } + // --------------------------------------------------------------------- + // TM3 — TripMembersService.listMembers's owner row + // --------------------------------------------------------------------- + + /** + * `SELECT id, COALESCE(display_name, username) AS username, email, avatar + * FROM users WHERE id = ?` (`trip-members.service.ts:129`, TM3). The + * legacy statement's own docstring records this COALESCE as a quirk fix on + * top of the 1:1 move (the ORIGINAL raw statement read the bare + * `username`) — kept exactly as the code already stands, not re-litigated. + */ + async findOwnerSummary(id: number): Promise<{ id: number; username: string; email: string; avatar: string | null } | null> { + const platform = this.getEntityManager().getPlatform(); + const row = await this.qb('u') + .select(['u.id', coalesce(platform, 'u.display_name', 'u.username').as('username'), 'u.email', 'u.avatar']) + .where({ id }) + .execute<{ id: number; username: string; email: string; avatar: string | null } | undefined>('get', false); + return row ? { id: row.id, username: row.username, email: row.email, avatar: row.avatar ?? null } : null; + } + + // --------------------------------------------------------------------- + // TM4 — TripMembersService.addMember's invite-target lookup + // --------------------------------------------------------------------- + + /** + * `SELECT id, username, email, avatar FROM users WHERE (email = ? OR + * username = ?) AND COALESCE(is_guest, 0) = 0` (`trip-members.service.ts:142-144`, + * TM4) — case-SENSITIVE (unlike `findByEmailCI`/`findIdByEmailCI` above; + * kept exactly as written, program rule 18 only applies where the legacy + * statement itself folds case), guests excluded so a trip-scoped guest can + * never be resolved (and re-attached to another trip) through the invite + * box. `identifier` is bound to both sides — the caller passes the SAME + * already-trimmed string for both the email and username branch, matching + * the legacy statement's `identifier.trim()` bound twice. `is_guest: 0` is + * exact parity for `COALESCE(is_guest, 0) = 0` (`countNonGuest`'s + * docstring above: the column is `NOT NULL DEFAULT 0`, so no row can store + * `NULL` there). + */ + async findInvitableByEmailOrUsername(identifier: string): Promise<{ id: number; username: string; email: string; avatar: string | null } | null> { + const row = await this.findOne( + { $or: [{ email: identifier }, { username: identifier }], is_guest: 0 }, + { fields: ['id', 'username', 'email', 'avatar'] }, + ); + return row ? { id: row.id, username: row.username, email: row.email, avatar: row.avatar ?? null } : null; + } + + // --------------------------------------------------------------------- + // TM10 — TripMembersService.transferOwnership's new-owner guest check + // --------------------------------------------------------------------- + + /** `SELECT id, email, is_guest FROM users WHERE id = ?` (`trip-members.service.ts:185`, TM10) — guest → 400 `'Cannot transfer ownership to a guest'` (caller's decision). */ + async findIdEmailGuest(id: number): Promise<{ id: number; email: string; is_guest: number } | null> { + const row = await this.findOne({ id }, { fields: ['id', 'email', 'is_guest'] }); + return row ? { id: row.id, email: row.email, is_guest: row.is_guest } : null; + } + + // --------------------------------------------------------------------- + // TM16 — TripMembersService.createGuest (Plan 3c Task 6, security-sensitive) + // --------------------------------------------------------------------- + + /** + * `INSERT INTO users (username, email, password_hash, role, is_guest, + * display_name) VALUES (?, ?, '', 'user', 1, ?)` (`trip-members.service.ts:222-224`, + * TM16) — security-sensitive: creates a credential-less account (#1362). + * `password_hash`/`role`/`is_guest` are fixed literals, never a caller + * input (see `NewGuestUserRow`'s docstring above for why this is its own + * method rather than a widened `insertUser`). Returns the generated id + * only — the legacy caller reads `res.lastInsertRowid` and never re-selects + * the row. + */ + async insertGuest(row: NewGuestUserRow): Promise { + return this.insert({ + username: row.username, + email: row.email, + password_hash: '', + role: 'user', + is_guest: 1, + display_name: row.display_name, + }); + } + + // --------------------------------------------------------------------- + // TM19 — TripMembersService.renameGuest (security-sensitive) + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET display_name = ?, updated_at = CURRENT_TIMESTAMP WHERE + * id = ? AND is_guest = 1` (`trip-members.service.ts:248`, TM19) — + * security-sensitive: the `AND is_guest = 1` is a SECOND guard (belt and + * braces) on top of `TripMembersRepository.isGuestOfTrip`'s trip-scoping + * check that runs before this is ever called (#1362) — pinned by a test + * that this method, called directly on a real non-guest id, updates the + * row count of exactly zero rows. + */ + async renameGuest(id: number, displayName: string): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id, is_guest: 1 }, { display_name: displayName, updated_at: currentTimestamp(platform) }); + } + + // --------------------------------------------------------------------- + // TM20 — TripMembersService.deleteGuest (security-sensitive: account erasure) + // --------------------------------------------------------------------- + + /** + * `DELETE FROM users WHERE id = ? AND is_guest = 1` (`trip-members.service.ts:268`, + * TM20) — security-sensitive: account erasure, the same belt-and-braces + * `is_guest = 1` predicate as `renameGuest` above. Cascades `trip_members` + * and every assignment/budget/packing join via the entity's FK delete + * rules, unchanged by this conversion. + */ + async deleteGuest(id: number): Promise { + await this.nativeDelete({ id, is_guest: 1 }); + } + // --------------------------------------------------------------------- // UC11 — account erasure (Plan 3b Task 1 review, "contract gaps" item 2: // named in the plan's Task 1 bullet, deliberately deferred to Task 5 — diff --git a/server/src/nest/trip-members/trip-members.module.ts b/server/src/nest/trip-members/trip-members.module.ts index 07c54a230..60cad5f78 100644 --- a/server/src/nest/trip-members/trip-members.module.ts +++ b/server/src/nest/trip-members/trip-members.module.ts @@ -1,4 +1,5 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { TripMembersController } from './trip-members.controller'; import { TripMembersService } from './trip-members.service'; import { DatabaseModule } from '../database/database.module'; @@ -8,6 +9,9 @@ import { AuditModule } from '../audit/audit.module'; import { AuthModule } from '../auth/auth.module'; import { BudgetModule } from '../budget/budget.module'; import { NotificationsModule } from '../notifications/notifications.module'; +import { Trips } from '../../db/entities/Trips.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import { Users } from '../../db/entities/Users.entity'; /** * The member/guest roster of a trip. @@ -17,9 +21,18 @@ import { NotificationsModule } from '../notifications/notifications.module'; * 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. + * + * `MikroOrmModule.forFeature([Trips, TripMembers, Users])` (Plan 3c Task 6): + * `DatabaseModule` stays too — `TripMembersService` still holds one documented + * raw-SQL survivor (`getTripForViewer`, pending Task 7) and the still-`DatabaseService` + * -routed `canAccessTrip` delegation, the same carve-out `DaysService`/every other + * converted domain in this program keeps. */ @Module({ - imports: [NotificationsModule, DatabaseModule, PermissionsModule, RealtimeModule, AuditModule, AuthModule, BudgetModule], + imports: [ + NotificationsModule, DatabaseModule, PermissionsModule, RealtimeModule, AuditModule, AuthModule, BudgetModule, + MikroOrmModule.forFeature([Trips, TripMembers, Users]), + ], controllers: [TripMembersController], providers: [TripMembersService], exports: [TripMembersService], diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 970f5a78c..b41fb2d65 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -1,5 +1,6 @@ import { Injectable } from '@nestjs/common'; import { randomUUID } from 'crypto'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { DatabaseService } from '../database/database.service'; import type { User } from '../../types'; import { avatarUrl } from '../common/avatarUrl'; @@ -13,6 +14,12 @@ import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { emitUserDeleted } from '../../plugin-user-lifecycle'; import { NotFoundError, ValidationError } from '../common/domain-errors'; import { NotificationsService } from '../notifications/notifications.service'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; export interface AddMemberResult { member: { id: number; username: string; email: string; avatar?: string | null; role: string; avatar_url: string | null }; @@ -68,8 +75,19 @@ export class TripMembersService { private readonly realtime: RealtimeService, private readonly notifications: NotificationsService, private readonly uow: UnitOfWork, + @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, + @InjectRepository(TripMembers) private readonly tripMembersRepo: TripMembersRepository, + @InjectRepository(Users) private readonly usersRepo: UsersRepository, ) {} + /** + * Task 7: `TripsService` still owns `TRIP_SELECT` and its own raw + * `this.dbs.connection` handle (`trips.service.ts:166-168`) — `getTripForViewer` + * below is this service's one documented raw-SQL survivor, importing the + * projection rather than copying it, unchanged until Task 7 converts + * `TripsService` itself and this call points at a repository method + * instead (`TripsRepository.findForViewer`, per the inventory's proposal). + */ private get db() { return this.dbs.connection; } @@ -88,7 +106,8 @@ export class TripMembersService { } /** 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. */ + * trip routes use, imported rather than copied so the two cannot drift. + * // Task 7: converts once TripsService itself does — see the class docstring above. */ async getTripForViewer(tripId: string | number, userId: number) { return this.db.prepare(`${TRIP_SELECT} WHERE t.id = :tripId`).get({ userId, tripId }); } @@ -113,24 +132,14 @@ export class TripMembersService { async 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 }[]; + const members = await this.tripMembersRepo.listWithUserAndInviter(tripId, tripOwnerId); // 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; + const owner = await this.usersRepo.findOwnerSummary(tripOwnerId); return { - owner: { ...owner, role: 'owner', is_guest: false, avatar_url: avatarUrl(owner) }, + 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) })), }; } @@ -140,31 +149,34 @@ export class TripMembersService { // 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; + const target = await this.usersRepo.findInvitableByEmailOrUsername(identifier.trim()); 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); + const existing = await this.tripMembersRepo.exists(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); + // `Number(tripId)`, not the raw string: `addMember`'s INSERT needs the + // real `Primary` type (its docstring). Safe here — `tripId` has + // already matched a real trip through the inline `canAccessTrip` check + // above this method's controller call, which only succeeds for a string + // SQLite's own affinity rules already recognise as numeric. + await this.tripMembersRepo.addMember(Number(tripId), target.id, invitedByUserId); - const tripInfo = this.db.prepare('SELECT title FROM trips WHERE id = ?').get(tripId) as { title: string } | undefined; + const tripTitle = await this.tripsRepo.getTitle(tripId); return { member: { ...target, role: 'member', avatar_url: avatarUrl(target) }, targetUserId: target.id, - tripTitle: tripInfo?.title || 'Untitled', + tripTitle: tripTitle || 'Untitled', }; } async removeMember(tripId: string | number, targetUserId: number): Promise { - this.db.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(tripId, targetUserId); + await this.tripMembersRepo.remove(tripId, targetUserId); } /** @@ -178,27 +190,31 @@ export class TripMembersService { newOwnerId: number, currentOwnerId: number, ): Promise { - 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; + const trip = await this.tripsRepo.findIdTitleOwner(tripId); 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; + const newOwner = await this.usersRepo.findIdEmailGuest(newOwnerId); 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); + const isMember = await this.tripMembersRepo.exists(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 fromEmail = (await this.usersRepo.getEmail(currentOwnerId)) || ''; + // `Number(tripId)` at the two writes below that need the real + // `Primary` type (`setOwner`'s docstring / `addIgnoringConflict`'s + // docstring) — safe: this route runs behind `TripOwnerGuard`, so `tripId` + // has already matched a real trip through its raw-bind `isOwner` check. await this.uow.transactional(async () => { - this.db.prepare('UPDATE trips SET user_id = ? WHERE id = ?').run(newOwnerId, tripId); + await this.tripsRepo.setOwner(tripId, newOwnerId); // 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); + await this.tripMembersRepo.remove(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); + await this.tripMembersRepo.addIgnoringConflict(Number(tripId), currentOwnerId, newOwnerId); }); return { tripTitle: trip.title, fromEmail, toEmail: newOwner.email }; @@ -219,12 +235,12 @@ export class TripMembersService { const email = `guest-${randomUUID()}@guests.invalid`; const username = `guest-${randomUUID()}`; + // `Number(tripId)` for the same reason `addMember`'s docstring gives: + // `createGuest`'s route runs behind `TripOwnerGuard`, so `tripId` has + // already matched a real trip through its raw-bind `isOwner` check. const guestId = await this.uow.transactional(async () => { - 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 newGuestId = Number(res.lastInsertRowid); - this.db.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(tripId, newGuestId, invitedByUserId); + const newGuestId = await this.usersRepo.insertGuest({ username, email, display_name: display }); + await this.tripMembersRepo.addMember(Number(tripId), newGuestId, invitedByUserId); return newGuestId; }); @@ -233,9 +249,7 @@ export class TripMembersService { /** Confirms a user id is a guest of THIS trip, so guest mutations stay trip-scoped. */ private async guestOfTrip(tripId: string | number, guestUserId: number): Promise { - 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); + return this.tripMembersRepo.isGuestOfTrip(tripId, guestUserId); } async renameGuest(tripId: string | number, guestUserId: number, name: string): Promise { @@ -246,7 +260,7 @@ export class TripMembersService { // 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); + await this.usersRepo.renameGuest(guestUserId, display); return true; } @@ -266,7 +280,7 @@ export class TripMembersService { await 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); + await this.usersRepo.deleteGuest(guestUserId); }); await 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 index cb01e34dd..575b93829 100644 --- a/server/src/nest/trip-read-model/trip-read-model.module.ts +++ b/server/src/nest/trip-read-model/trip-read-model.module.ts @@ -1,6 +1,7 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { TripReadModelService } from './trip-read-model.service'; -import { DatabaseModule } from '../database/database.module'; +import { Trips } from '../../db/entities/Trips.entity'; import { DaysModule } from '../days/days.module'; import { AccommodationsModule } from '../accommodations/accommodations.module'; import { BudgetModule } from '../budget/budget.module'; @@ -13,10 +14,15 @@ 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. */ + * have to carry it. Nothing imports this except trips. + * + * `MikroOrmModule.forFeature([Trips])` replaces `DatabaseModule` (Plan 3c + * Task 6): `TripReadModelService` is now SQL-free — its two sites + * (`getOwner`/`getTripSummary`'s trip read) both convert to `TripsRepository` + * methods, unlike `TripMembersModule`'s carve-out next door. */ @Module({ imports: [ - DatabaseModule, TripMembersModule, DaysModule, AccommodationsModule, BudgetModule, + MikroOrmModule.forFeature([Trips]), TripMembersModule, DaysModule, AccommodationsModule, BudgetModule, PackingModule, ReservationsModule, CollabModule, PlacesModule, TodoModule, FilesModule, ], providers: [TripReadModelService], 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 index b8eef8049..6c3a41d86 100644 --- a/server/src/nest/trip-read-model/trip-read-model.service.ts +++ b/server/src/nest/trip-read-model/trip-read-model.service.ts @@ -1,5 +1,7 @@ import { Injectable } from '@nestjs/common'; -import { DatabaseService } from '../database/database.service'; +import { InjectRepository } from '@mikro-orm/nestjs'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripsRepository } from '../../db/repositories/Trips.repository'; import { DaysService } from '../days/days.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; import { BudgetService } from '../budget/budget.service'; @@ -24,7 +26,7 @@ import { withoutFeedToken } from '../trips/trips.service'; @Injectable() export class TripReadModelService { constructor( - private readonly dbs: DatabaseService, + @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, private readonly members: TripMembersService, private readonly days: DaysService, private readonly accommodations: AccommodationsService, @@ -37,25 +39,15 @@ export class TripReadModelService { private readonly files: FilesService, ) {} - private get db() { - return this.dbs.connection; - } - - private async getOwner(tripId: string | number): Promise<{ 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) ────────────────────── async getTripSummary(tripId: number, viewerUserId?: number) { - const trip = withoutFeedToken( - this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Record | undefined, - ); + const trip = withoutFeedToken(await this.tripsRepo.findRaw(tripId)); if (!trip) return null; - const ownerRow = await this.getOwner(tripId); - if (!ownerRow) return null; - const { owner, members } = await this.members.listMembers(tripId, ownerRow.user_id); + const ownerId = await this.tripsRepo.getOwnerId(tripId); + if (ownerId === null) return null; + const { owner, members } = await this.members.listMembers(tripId, ownerId); const { days: rawDays } = await this.days.list(tripId); const days = rawDays.map(({ notes_items, ...day }) => ({ ...day, notes: notes_items })); diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 348cee4aa..09f5a4967 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -218,10 +218,15 @@ export async function createMcpTestRegistry(): Promise { journeyDomain, generalStorage, accommodationsService, await createTestUnitOfWork(dbService.connection), + await createTestPlacesRepo(dbService.connection), + await createTestTagsRepo(dbService.connection), + await createTestPlaceRatingsRepo(dbService.connection), + await createTestTripMembersRepo(dbService.connection), + await createTestDayAssignmentsRepo(dbService.connection), ); // Built after it: a hotel booking writes the stay's day stop through this one. const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService, notificationsStub(), new ReservationsReadRepository(dbService), accommodationsService, await createTestUnitOfWork(dbService.connection)); - const membersService = new TripMembersService(dbService, budgetService, new UserCleanupService(dbService, budgetService, await createTestUnitOfWork(dbService.connection), usersRepo), permissionsService, realtimeService, notificationsStub(), await createTestUnitOfWork(dbService.connection)); + const membersService = new TripMembersService(dbService, budgetService, new UserCleanupService(dbService, budgetService, await createTestUnitOfWork(dbService.connection), usersRepo), permissionsService, realtimeService, notificationsStub(), await createTestUnitOfWork(dbService.connection), await createTestTripsRepo(dbService.connection), await createTestTripMembersRepo(dbService.connection), usersRepo); const tripsService = new TripsService( dbService, reservationsService, @@ -236,7 +241,7 @@ export async function createMcpTestRegistry(): Promise { mcpOrm.em, ); const readModelService = new TripReadModelService( - dbService, membersService, daysService, accommodationsService, budgetService, + await createTestTripsRepo(dbService.connection), membersService, daysService, accommodationsService, budgetService, packingService, reservationsService, collabService, placesService, todoService, new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage, mcpOrm.em), ); @@ -282,7 +287,7 @@ export async function createMcpTestRegistry(): Promise { new FeedsMcp(new FeedsService(dbService, calendarService), dbService, new RuntimeEnvService(), guards), new TripInviteMcp(new TripInviteService(dbService, permissionsService, new TripMembershipService(await createTestTripsRepo(dbService.connection), await createTestTripMembersRepo(dbService.connection)), await createTestUnitOfWork(dbService.connection)), dbService, new RuntimeEnvService(), guards, new AuditService(auditLogRepo, usersRepo)), new MapsMcp(mapsService), - new PlacesMcp(placesService, mapsService, dbService, authService, journeyDomain, assignmentsService, guards, await createTestUnitOfWork(dbService.connection)), + new PlacesMcp(placesService, mapsService, await createTestTripsRepo(dbService.connection), authService, journeyDomain, assignmentsService, guards, await createTestUnitOfWork(dbService.connection)), new CollectionsMcp(new CollectionsService(dbService, permissionsService, realtimeService, notificationsStub(), generalStorage, await createTestUnitOfWork(dbService.connection)), dbService, authService, addonsService), new TransitMcp(new TransitService(new GoogleTransitProvider(dbService, appSettings, usersRepo)), daysService, reservationsService, dbService, authService, guards), new AtlasMcp(new AtlasService(dbService, await createTestUnitOfWork(dbService.connection)), addonsService, authService), diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index e4e672c39..a42967212 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -138,11 +138,18 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< const oauth = new PluginOAuthService(dbs); const accommodations = new AccommodationsService(dbs, permissions, realtime, assignments, await createTestUnitOfWork(dbs.connection)); // After it: deleting a place cancels the nights booked at it through this one. - const places = new PlacesService(dbs, permissions, realtime, new MapsService(dbs, photoCache, appSettings, usersRepo), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection)); + const places = new PlacesService( + dbs, permissions, realtime, new MapsService(dbs, photoCache, appSettings, usersRepo), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection), + await createTestPlacesRepo(dbs.connection), + await createTestTagsRepo(dbs.connection), + await createTestPlaceRatingsRepo(dbs.connection), + await createTestTripMembersRepo(dbs.connection), + await createTestDayAssignmentsRepo(dbs.connection), + ); // After accommodations: a hotel booking writes the stay's day stop through it. const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs), accommodations, await createTestUnitOfWork(dbs.connection)); const trips = new TripsService(dbs, reservations, days, permissions, budget, vacay, realtime, unsplash, generalStorage, await createTestUnitOfWork(dbs.connection), (await sharedTestOrm(dbs.connection)).em); - const members = new TripMembersService(dbs, budget, new UserCleanupService(dbs, budget, await createTestUnitOfWork(dbs.connection), usersRepo), permissions, realtime, notificationsStub(), await createTestUnitOfWork(dbs.connection)); + const members = new TripMembersService(dbs, budget, new UserCleanupService(dbs, budget, await createTestUnitOfWork(dbs.connection), usersRepo), permissions, realtime, notificationsStub(), await createTestUnitOfWork(dbs.connection), await createTestTripsRepo(dbs.connection), await createTestTripMembersRepo(dbs.connection), usersRepo); const guards = new PluginGuards(dbs, permissions, addons); const registry = createTestPluginRegistry([ diff --git a/server/tests/unit/db/dialect/sql-functions.test.ts b/server/tests/unit/db/dialect/sql-functions.test.ts index ccfcd0c8f..4cdb5d8ea 100644 --- a/server/tests/unit/db/dialect/sql-functions.test.ts +++ b/server/tests/unit/db/dialect/sql-functions.test.ts @@ -7,6 +7,7 @@ import { createUser } from '../../../helpers/factories'; import { Users } from '../../../../src/db/entities/Users.entity'; import { absDifference, + caseWhenEquals, coalesce, coalesceParam, columnIncrementedBy, @@ -428,4 +429,30 @@ describe('sql-functions (sqlite)', () => { const foreign = new FakePlatform(); expect(() => lowerTrim(foreign, 'u.name')).toThrow(/no implementation for platform FakePlatform/); }); + + // Plan 3c Task 6 — `TripMembersRepository.listWithUserAndInviter`'s `role` + // column (TM2): `CASE WHEN u.id = ? THEN 'owner' ELSE 'member' END`. + it("SQLF-030: caseWhenEquals renders CASE WHEN = ? THEN ? ELSE ? END, matching a hand-written statement", async () => { + const { user: a } = createUser(testDb); + const { user: b } = createUser(testDb); + const platform = t.em.getPlatform(); + + const rows = await t.em.createQueryBuilder(Users, 'u') + .select(['u.id', caseWhenEquals(platform, 'u.id', a.id, 'owner', 'member').as('role')]) + .where({ id: { $in: [a.id, b.id] } }) + .orderBy({ id: 'asc' }) + .execute('all', false); + expect(rows).toEqual([{ id: a.id, role: 'owner' }, { id: b.id, role: 'member' }]); + + const expected = testDb + .prepare('SELECT id, CASE WHEN id = ? THEN ? ELSE ? END as role FROM users WHERE id IN (?, ?) ORDER BY id ASC') + .all(a.id, 'owner', 'member', a.id, b.id); + expect(rows).toEqual(expected); + }); + + it('SQLF-031: an unknown platform fails closed for caseWhenEquals', () => { + class FakePlatform extends Platform {} + const foreign = new FakePlatform(); + expect(() => caseWhenEquals(foreign, 'u.id', 1, 'owner', 'member')).toThrow(/no implementation for platform FakePlatform/); + }); }); diff --git a/server/tests/unit/db/repositories/TripMembers.repository.test.ts b/server/tests/unit/db/repositories/TripMembers.repository.test.ts index 75c79f236..6b3092b0f 100644 --- a/server/tests/unit/db/repositories/TripMembers.repository.test.ts +++ b/server/tests/unit/db/repositories/TripMembers.repository.test.ts @@ -134,3 +134,209 @@ describe('TripMembersRepository — listUserIdsByTrip / exists / addMember (Plan expect(row2).toEqual({ invited_by: null }); }); }); + +// --------------------------------------------------------------------------- +// Plan 3c Task 6 — TripMembersService (TM2/TM8/TM14/TM15/TM18). Appended +// after Task 1's own suite above, per this task's file-ownership rule. +// --------------------------------------------------------------------------- + +/** TM2's statement, run raw on the same rows — the parity oracle every assertion below is checked against. */ +function legacyListWithUserAndInviter(tripId: number, ownerId: number): unknown { + return testDb.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(ownerId, tripId); +} + +describe('TripMembersRepository.listWithUserAndInviter (TM2, Plan 3c Task 6)', () => { + it('TMEMREPO-011: parity with the legacy statement on a fully seeded roster — a member with display_name, one without, a guest, an inviter with/without display_name, NULL avatars', async () => { + const { user: owner } = createUser(testDb, { username: 'owner-handle' }); + const trip = createTrip(testDb, owner.id); + + const { user: inviterWithName } = createUser(testDb, { username: 'inviter-1' }); + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Inviter One', inviterWithName.id); + const { user: inviterBare } = createUser(testDb, { username: 'inviter-2' }); + + const { user: memberWithDisplayName } = createUser(testDb, { username: 'member-1-handle' }); + testDb.prepare('UPDATE users SET display_name = ?, avatar = ? WHERE id = ?').run('Member One', 'm1.png', memberWithDisplayName.id); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(trip.id, memberWithDisplayName.id, inviterWithName.id); + + const { user: memberBare } = createUser(testDb, { username: 'member-2-handle' }); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(trip.id, memberBare.id, inviterBare.id); + + const { user: memberNoInviter } = createUser(testDb, { username: 'member-3-handle' }); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, NULL)').run(trip.id, memberNoInviter.id); + + const { user: guest } = createUser(testDb, { username: 'guest-handle' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(trip.id, guest.id, owner.id); + + testDb.prepare('UPDATE trip_members SET added_at = ? WHERE trip_id = ? AND user_id = ?').run('2026-01-01 00:00:00', trip.id, memberWithDisplayName.id); + testDb.prepare('UPDATE trip_members SET added_at = ? WHERE trip_id = ? AND user_id = ?').run('2026-01-02 00:00:00', trip.id, memberBare.id); + testDb.prepare('UPDATE trip_members SET added_at = ? WHERE trip_id = ? AND user_id = ?').run('2026-01-03 00:00:00', trip.id, memberNoInviter.id); + testDb.prepare('UPDATE trip_members SET added_at = ? WHERE trip_id = ? AND user_id = ?').run('2026-01-04 00:00:00', trip.id, guest.id); + + const rows = await tripMembers.listWithUserAndInviter(trip.id, owner.id); + expect(rows).toEqual(legacyListWithUserAndInviter(trip.id, owner.id)); + expect(rows).toEqual([ + { id: memberWithDisplayName.id, username: 'Member One', email: memberWithDisplayName.email, avatar: 'm1.png', is_guest: 0, role: 'member', added_at: '2026-01-01 00:00:00', invited_by_username: 'Inviter One' }, + { id: memberBare.id, username: 'member-2-handle', email: memberBare.email, avatar: null, is_guest: 0, role: 'member', added_at: '2026-01-02 00:00:00', invited_by_username: 'inviter-2' }, + { id: memberNoInviter.id, username: 'member-3-handle', email: memberNoInviter.email, avatar: null, is_guest: 0, role: 'member', added_at: '2026-01-03 00:00:00', invited_by_username: null }, + { id: guest.id, username: 'guest-handle', email: guest.email, avatar: null, is_guest: 1, role: 'member', added_at: '2026-01-04 00:00:00', invited_by_username: 'owner-handle' }, + ]); + }); + + it('TMEMREPO-012: the trip owner\'s own row (if ever a member) reports role "owner"', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, NULL)').run(trip.id, owner.id); + const rows = await tripMembers.listWithUserAndInviter(trip.id, owner.id); + expect(rows).toEqual([expect.objectContaining({ id: owner.id, role: 'owner' })]); + }); + + it('TMEMREPO-013: an owner-only trip (no members row) returns an empty array', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + expect(await tripMembers.listWithUserAndInviter(trip.id, owner.id)).toEqual([]); + }); + + // D-shape (rule 20): a `qb().execute('all', false)` projection, which + // never hydrates an entity into the identity map by construction — proven + // anyway, the `AssignmentParticipantsRepository`/`PlaceRatingsRepository` + // precedent shape. + it('TMEMREPO-014 (D-shape): a member added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + const { user: owner } = createUser(testDb); + const { user: fresh } = createUser(testDb, { username: 'fresh-member' }); + const trip = createTrip(testDb, owner.id); + await t.repo(TripMembers).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read + addTripMember(testDb, trip.id, fresh.id); + const rows = await tripMembers.listWithUserAndInviter(trip.id, owner.id); + expect(rows).toEqual([expect.objectContaining({ id: fresh.id, username: 'fresh-member' })]); + }); +}); + +describe('TripMembersRepository.isGuestOfTrip (TM18, security-sensitive, Plan 3c Task 6)', () => { + it('TMEMREPO-015: true only for a guest who is actually a member of THIS trip', async () => { + const { user: owner } = createUser(testDb); + const { user: guest } = createUser(testDb, { username: 'guest-scoped' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, guest.id); + + expect(await tripMembers.isGuestOfTrip(trip.id, guest.id)).toBe(true); + }); + + it('TMEMREPO-016: false for a real member (not a guest)', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + expect(await tripMembers.isGuestOfTrip(trip.id, member.id)).toBe(false); + }); + + it('TMEMREPO-017: false for a guest of a DIFFERENT trip — the scoping guard that keeps guest mutations trip-local', async () => { + const { user: ownerA } = createUser(testDb); + const { user: ownerB } = createUser(testDb); + const { user: guest } = createUser(testDb, { username: 'guest-of-a' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const tripA = createTrip(testDb, ownerA.id); + const tripB = createTrip(testDb, ownerB.id); + addTripMember(testDb, tripA.id, guest.id); + + expect(await tripMembers.isGuestOfTrip(tripA.id, guest.id)).toBe(true); + expect(await tripMembers.isGuestOfTrip(tripB.id, guest.id)).toBe(false); + }); + + it('TMEMREPO-018: false for a nonexistent user id', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + expect(await tripMembers.isGuestOfTrip(trip.id, 999999)).toBe(false); + }); +}); + +describe('TripMembersRepository.addIgnoringConflict / remove (TM8/TM14/TM15, security-sensitive, Plan 3c Task 6)', () => { + it('TMEMREPO-019: addIgnoringConflict inserts a fresh row', async () => { + const { user: owner } = createUser(testDb); + const { user: former } = createUser(testDb); + const { user: newOwner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await tripMembers.addIgnoringConflict(trip.id, former.id, newOwner.id); + const row = testDb.prepare('SELECT trip_id, user_id, invited_by FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, former.id); + expect(row).toEqual({ trip_id: trip.id, user_id: former.id, invited_by: newOwner.id }); + }); + + it('TMEMREPO-020: addIgnoringConflict is a true no-op (not a throw) when the (trip, user) row already exists', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + // The existing row's invited_by (NULL, from addTripMember) must survive — + // an upsert-merge would overwrite it, which is NOT what `INSERT OR IGNORE` does. + await expect(tripMembers.addIgnoringConflict(trip.id, member.id, owner.id)).resolves.toBeUndefined(); + const row = testDb.prepare('SELECT invited_by FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id); + expect(row).toEqual({ invited_by: null }); + expect((testDb.prepare('SELECT COUNT(*) AS n FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id) as { n: number }).n).toBe(1); + }); + + it('TMEMREPO-021: remove deletes exactly the (trip, user) row, leaving other members untouched', async () => { + const { user: owner } = createUser(testDb); + const { user: memberA } = createUser(testDb); + const { user: memberB } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, memberA.id); + addTripMember(testDb, trip.id, memberB.id); + + await tripMembers.remove(trip.id, memberA.id); + expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, memberA.id)).toBeUndefined(); + expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, memberB.id)).toBeDefined(); + }); + + it('TMEMREPO-022: remove on a missing row is a silent no-op', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await expect(tripMembers.remove(trip.id, 999999)).resolves.toBeUndefined(); + }); + + it('TMEMREPO-023: remove binds a string trip id raw, the same seam rosterUserIds/listUserIdsByTrip document', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await tripMembers.remove(String(trip.id), member.id); + expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeUndefined(); + }); +}); + +// Rule 15's exact trap, caught on a compiled boot (`DELETE /api/trips/:id/ +// members/abc` 500'd with `no such column: NaN` before this guard existed): +// `TripMembersController.removeMember`/`renameGuest`/`deleteGuest` still do a +// bare `Number.parseInt(userId)` on the route's `:userId`, unvalidated — a +// non-numeric id parses to `NaN`, which MikroORM/Kysely inline as the +// literal SQL token `NaN` even through a raw `?` placeholder. +describe('NaN user_id — rule 15 (a non-numeric route id parses to NaN, must not 500)', () => { + it('TMEMREPO-024: remove(tripId, NaN) is a silent no-op, matching the legacy better-sqlite3 bind', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + await expect(tripMembers.remove(trip.id, NaN)).resolves.toBeUndefined(); + // The real member row survives — NaN must not accidentally match anything. + expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeDefined(); + }); + + it('TMEMREPO-025: isGuestOfTrip(tripId, NaN) resolves false, matching the legacy 404 Guest not found', async () => { + const { user: owner } = createUser(testDb); + const { user: guest } = createUser(testDb); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, guest.id); + await expect(tripMembers.isGuestOfTrip(trip.id, NaN)).resolves.toBe(false); + }); +}); diff --git a/server/tests/unit/db/repositories/Trips.repository.test.ts b/server/tests/unit/db/repositories/Trips.repository.test.ts index 00198ff69..c132640d6 100644 --- a/server/tests/unit/db/repositories/Trips.repository.test.ts +++ b/server/tests/unit/db/repositories/Trips.repository.test.ts @@ -152,3 +152,86 @@ describe('TripsRepository.setEndDate (Plan 3c Task 2, DY35)', () => { expect((testDb.prepare('SELECT end_date FROM trips WHERE id = ?').get(trip.id) as { end_date: string | null }).end_date).toBeNull(); }); }); + +// --------------------------------------------------------------------------- +// Plan 3c Task 6 — TripMembersService (TM7/TM9/TM13) and TripReadModelService +// (TR-A/TR-B). Appended per this task's file-ownership rule (additive +// methods only, this repository is shared across tasks). +// --------------------------------------------------------------------------- + +describe('TripsRepository — TripMembersService / TripReadModelService (Plan 3c Task 6)', () => { + it('TRIPREPO-014: getTitle (TM7) reads the title, null for a missing trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Readable' }); + expect(await trips.getTitle(trip.id)).toBe('Readable'); + expect(await trips.getTitle(999999)).toBeNull(); + }); + + it('TRIPREPO-015: getTitle binds a string id raw, the same `0x10`/`007`-shaped id parity seam TripsRepository documents', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Stringy' }); + expect(await trips.getTitle(String(trip.id))).toBe('Stringy'); + expect(await trips.getTitle('not-a-number')).toBeNull(); + }); + + it('TRIPREPO-016: findIdTitleOwner (TM9) returns {id, title, user_id}, undefined for a missing trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Handover' }); + expect(await trips.findIdTitleOwner(trip.id)).toEqual({ id: trip.id, title: 'Handover', user_id: user.id }); + expect(await trips.findIdTitleOwner(999999)).toBeUndefined(); + }); + + it('TRIPREPO-017: setOwner (TM13, security-sensitive) writes user_id verbatim', async () => { + const { user: owner } = createUser(testDb); + const { user: newOwner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await trips.setOwner(trip.id, newOwner.id); + expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(newOwner.id); + }); + + it('TRIPREPO-018: setOwner binds a string trip id raw — the same seam its docstring documents', async () => { + const { user: owner } = createUser(testDb); + const { user: newOwner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await trips.setOwner(String(trip.id), newOwner.id); + expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(newOwner.id); + }); + + describe('findRaw (TR-B, shared with TripsService.getRaw once Task 7 lands)', () => { + it('TRIPREPO-019: every scalar column comes back, feed_token included (the JS strip is the caller\'s job)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Full Row', description: 'desc', start_date: '2026-01-01', end_date: '2026-01-05' }); + testDb.prepare('UPDATE trips SET feed_token = ?, cover_image = ? WHERE id = ?').run('secret-token', 'cover.png', trip.id); + const legacyRow = testDb.prepare('SELECT * FROM trips WHERE id = ?').get(trip.id); + const row = await trips.findRaw(trip.id); + expect(row).toEqual(legacyRow); + expect(row?.feed_token).toBe('secret-token'); + }); + + it('TRIPREPO-020: NULL-bearing nullable columns come back null, not undefined (rule 16); a missing trip is null', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const row = await trips.findRaw(trip.id); + expect(row).toMatchObject({ description: null, start_date: null, end_date: null, cover_image: null, feed_token: null }); + expect(await trips.findRaw(999999)).toBeNull(); + }); + + it('TRIPREPO-021: binds a string trip id raw, the same seam every other method here preserves', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Stringy Raw' }); + expect((await trips.findRaw(String(trip.id)))?.title).toBe('Stringy Raw'); + expect(await trips.findRaw('not-a-number')).toBeNull(); + }); + + // D-shape (rule 20): `findRaw` is a `qb().execute('get', false)` + // projection with an explicit `t.*` select, never hydrating an entity + // into the identity map by construction — proven anyway. + it('TRIPREPO-022 (D-shape): a title written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Before' }); + await t.repo(Trips).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read + testDb.prepare('UPDATE trips SET title = ? WHERE id = ?').run('After', trip.id); + expect((await trips.findRaw(trip.id))?.title).toBe('After'); + }); + }); +}); diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index ac9ecd099..69b232595 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -765,3 +765,125 @@ describe('UsersRepository', () => { expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(user.id)).toBeUndefined(); }); }); + +// --------------------------------------------------------------------------- +// Plan 3c Task 6 — TripMembersService's guest lifecycle (TM3/TM4/TM10/TM16/ +// TM19/TM20). Appended after the existing `describe('UsersRepository', ...)` +// block per this task's file-ownership rule (additive methods only). +// --------------------------------------------------------------------------- + +describe('UsersRepository — TripMembersService (Plan 3c Task 6)', () => { + describe('findOwnerSummary (TM3)', () => { + it('USERSREPO-059: prefers display_name over username, matching the legacy COALESCE', async () => { + const { user } = createUser(testDb, { username: 'raw-handle' }); + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Displayed Name', user.id); + expect(await users.findOwnerSummary(user.id)).toEqual({ + id: user.id, username: 'Displayed Name', email: user.email, avatar: null, + }); + }); + + it('USERSREPO-060: falls back to username when display_name is NULL; a missing user id is null, not a throw', async () => { + const { user } = createUser(testDb, { username: 'bare-handle' }); + expect(await users.findOwnerSummary(user.id)).toEqual({ + id: user.id, username: 'bare-handle', email: user.email, avatar: null, + }); + expect(await users.findOwnerSummary(999999)).toBeNull(); + }); + + it('USERSREPO-061: a NULL avatar comes back null, not undefined (rule 16)', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('me.png', user.id); + expect((await users.findOwnerSummary(user.id))?.avatar).toBe('me.png'); + }); + + // D-shape (rule 20): `findOwnerSummary` is a `qb().execute('get', false)` + // projection, which never hydrates an entity into the identity map by + // construction — proven anyway, the same "not required, proven + // regardless" shape `AssignmentParticipantsRepository`'s D-shape tests + // document. The FIRST, wider setup read passes `{ disableIdentityMap: + // false }` explicitly per the program rule. + it('USERSREPO-062 (D-shape): a display_name written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + const { user } = createUser(testDb, { username: 'fresh-owner' }); + await t.repo(Users).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Fresh Name', user.id); + expect(await users.findOwnerSummary(user.id)).toEqual({ + id: user.id, username: 'Fresh Name', email: user.email, avatar: null, + }); + }); + }); + + describe('findInvitableByEmailOrUsername (TM4)', () => { + it('USERSREPO-063: matches on email OR username, case-SENSITIVE (unlike findByEmailCI/findIdByEmailCI above)', async () => { + const { user: byEmail } = createUser(testDb, { email: 'invitee@example.test', username: 'invitee-handle' }); + expect(await users.findInvitableByEmailOrUsername('invitee@example.test')).toEqual({ + id: byEmail.id, username: 'invitee-handle', email: 'invitee@example.test', avatar: null, + }); + expect(await users.findInvitableByEmailOrUsername('invitee-handle')).toEqual({ + id: byEmail.id, username: 'invitee-handle', email: 'invitee@example.test', avatar: null, + }); + // Case-sensitive: an uppercased email must NOT match (program rule 18 + // only applies where the legacy statement itself folds case — TM4's does not). + expect(await users.findInvitableByEmailOrUsername('INVITEE@EXAMPLE.TEST')).toBeNull(); + }); + + it('USERSREPO-064: excludes guests — a trip-scoped guest can never be re-invited through the box', async () => { + const { user: guest } = createUser(testDb, { email: 'guest-look@example.test', username: 'guest-handle' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + expect(await users.findInvitableByEmailOrUsername('guest-look@example.test')).toBeNull(); + expect(await users.findInvitableByEmailOrUsername('guest-handle')).toBeNull(); + }); + + it('USERSREPO-065: no match returns null, not a throw', async () => { + expect(await users.findInvitableByEmailOrUsername('nobody@example.test')).toBeNull(); + }); + }); + + describe('findIdEmailGuest (TM10)', () => { + it('USERSREPO-066: reads id/email/is_guest; a missing user is null', async () => { + const { user } = createUser(testDb, { email: 'target@example.test' }); + expect(await users.findIdEmailGuest(user.id)).toEqual({ id: user.id, email: 'target@example.test', is_guest: 0 }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findIdEmailGuest(user.id)).toEqual({ id: user.id, email: 'target@example.test', is_guest: 1 }); + expect(await users.findIdEmailGuest(999999)).toBeNull(); + }); + }); + + describe('insertGuest (TM16, security-sensitive)', () => { + it('USERSREPO-067: writes the fixed literal columns and the given display_name; returns the generated id', async () => { + const id = await users.insertGuest({ username: 'guest-abc', email: 'guest-abc@guests.invalid', display_name: 'Ida' }); + const row = testDb.prepare('SELECT username, email, password_hash, role, is_guest, display_name FROM users WHERE id = ?').get(id); + expect(row).toEqual({ + username: 'guest-abc', email: 'guest-abc@guests.invalid', password_hash: '', role: 'user', is_guest: 1, display_name: 'Ida', + }); + }); + }); + + describe('renameGuest / deleteGuest (TM19/TM20, security-sensitive, belt-and-braces is_guest = 1)', () => { + it('USERSREPO-068: renameGuest updates display_name + updated_at only for an is_guest = 1 row', async () => { + const id = await users.insertGuest({ username: 'guest-rn', email: 'guest-rn@guests.invalid', display_name: 'Old' }); + await users.renameGuest(id, 'New Name'); + const row = testDb.prepare('SELECT display_name, updated_at FROM users WHERE id = ?').get(id) as { display_name: string; updated_at: string | null }; + expect(row.display_name).toBe('New Name'); + expect(row.updated_at).not.toBeNull(); + }); + + it('USERSREPO-069: renameGuest is a no-op on a real (non-guest) user id — the belt-and-braces predicate, pinned', async () => { + const { user } = createUser(testDb, { username: 'real-user' }); + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Untouched', user.id); + await users.renameGuest(user.id, 'Attempted Rename'); + expect((testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(user.id) as { display_name: string }).display_name).toBe('Untouched'); + }); + + it('USERSREPO-070: deleteGuest removes only an is_guest = 1 row', async () => { + const id = await users.insertGuest({ username: 'guest-del', email: 'guest-del@guests.invalid', display_name: 'Gone' }); + await users.deleteGuest(id); + expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(id)).toBeUndefined(); + }); + + it('USERSREPO-071: deleteGuest is a no-op on a real (non-guest) user id — the belt-and-braces predicate, pinned', async () => { + const { user } = createUser(testDb, { username: 'real-user-2' }); + await users.deleteGuest(user.id); + expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(user.id)).toBeDefined(); + }); + }); +}); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 54b901e09..04846456f 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -72,7 +72,7 @@ import { CollabService } from '../../../src/nest/collab/collab.service'; import { VacayService } from '../../../src/nest/vacay/vacay.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm, createTestTripsRepo, createTestTripMembersRepo } from '../../helpers/test-uow'; @@ -111,6 +111,9 @@ beforeAll(async () => { new RealtimeService(), notificationsStub(), await createTestUnitOfWork(testDb), + await createTestTripsRepo(testDb), + await createTestTripMembersRepo(testDb), + await createTestUsersRepo(testDb), ); createGuest = (...args) => membersSvc.createGuest(...args); deleteGuest = (...args) => membersSvc.deleteGuest(...args); diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts index 4acc825b0..81e9612c2 100644 --- a/server/tests/unit/nest/trip-members.service.test.ts +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -65,8 +65,10 @@ import { TripMembersService } from '../../../src/nest/trip-members/trip-members. import { NotFoundError, ValidationError } from '../../../src/nest/common/domain-errors'; import type { User } from '../../../src/types'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestTripsRepo, createTestTripMembersRepo, sharedTestOrm } from '../../helpers/test-uow'; import type { EntityManager } from '@mikro-orm/core'; +import type { TripsRepository } from '../../../src/db/repositories/Trips.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; // Plan 3c Task 0b: `dbsEm` is resolved once, at the top of the `beforeAll` // below, before any `dbs()` call — `canAccessTrip`/`isOwner`/`rosterUserIds`/ @@ -79,43 +81,22 @@ const dbs = () => new DatabaseService(testDb, dbsEm); let budgetSvc: BudgetService; let roster: TripMembersService; +let tripsRepo: TripsRepository; +let usersRepo: UsersRepository; beforeAll(async () => { dbsEm = (await sharedTestOrm(testDb)).em; + tripsRepo = await createTestTripsRepo(testDb); + usersRepo = await createTestUsersRepo(testDb); budgetSvc = new BudgetService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(dbs().connection)); - roster = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(notifySend), await createTestUnitOfWork(dbs().connection)); + roster = new TripMembersService( + dbs(), budgetSvc, + new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), usersRepo), + new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), + new RealtimeService(), notificationsStub(notifySend), await createTestUnitOfWork(dbs().connection), + tripsRepo, await createTestTripMembersRepo(testDb), usersRepo, + ); }); -/** - * A roster whose connection reports "no such row" for the first result of every - * statement containing `match`. Both fallbacks it serves sit behind a FOREIGN - * KEY, so the missing row cannot be produced through the real schema — freezing - * the read is the only way to keep those branches honest. Same Proxy shape as - * the failingConnection helper in trips.service.test.ts. - */ -async function rosterWithMissingRow(match: string) { - const conn = new Proxy(testDb, { - get(target, prop) { - if (prop === 'prepare') { - return (sql: string) => { - const stmt = target.prepare(sql); - if (!sql.includes(match)) return stmt; - return new Proxy(stmt, { - get(s, p) { - if (p === 'get') return () => undefined; - const v = (s as any)[p]; - return typeof v === 'function' ? v.bind(s) : v; - }, - }); - }; - } - const v = (target as any)[prop]; - return typeof v === 'function' ? v.bind(target) : v; - }, - }); - const frozen = { connection: conn, canAccessTrip: dbMock.canAccessTrip, isOwner: dbMock.isOwner } as unknown as DatabaseService; - return new TripMembersService(frozen, budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(notifySend), await createTestUnitOfWork(dbs().connection)); -} - beforeAll(() => { createTables(testDb); runMigrations(testDb); @@ -216,6 +197,11 @@ describe('TripMembersService delegation', () => { // ── addMember fallbacks ────────────────────────────────────────────────────── describe('addMember fallbacks', () => { + // R8 (Plan 3c program brief item 8): the SQL-text-keyed Proxy fault + // injection this test used to build (`rosterWithMissingRow`, keyed on the + // literal `'SELECT title FROM trips WHERE id = ?'` fragment) is rewritten + // onto a repository-level fault — `TripsRepository.getTitle` resolving + // `null`, the shape it already returns on a genuine miss. it("MEMBERS-SVC-007: addMember still reports a title when the trip row cannot be read ('Untitled')", async () => { const { user: owner } = createUser(testDb); const { user: invitee } = createUser(testDb); @@ -224,8 +210,13 @@ describe('addMember fallbacks', () => { // The title only feeds the invite notification, and it is read after the // membership is inserted — losing that row must not cost the invitee their // access or throw on an undefined title. - const broken = await rosterWithMissingRow('SELECT title FROM trips WHERE id = ?'); - const result = await broken.addMember(trip.id, invitee.email, owner.id, owner.id); + const spy = vi.spyOn(tripsRepo, 'getTitle').mockResolvedValueOnce(null); + let result; + try { + result = await roster.addMember(trip.id, invitee.email, owner.id, owner.id); + } finally { + spy.mockRestore(); + } expect(result.tripTitle).toBe('Untitled'); expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, invitee.id)).toBeDefined(); }); @@ -266,6 +257,9 @@ describe('transferOwnership guard rails', () => { expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(owner.id); }); + // R8: the same rewrite as MEMBERS-SVC-007 above, on `UsersRepository.getEmail` + // (TM12) instead of `TripsRepository.getTitle` — `findIdEmailGuest`'s own + // read for the new owner (TM10) is untouched, so only the fromEmail leg fails. it('MEMBERS-SVC-011: completes with an empty fromEmail when the former owner cannot be read', async () => { const { user: owner } = createUser(testDb); const { user: member } = createUser(testDb); @@ -275,8 +269,13 @@ describe('transferOwnership guard rails', () => { // fromEmail is audit detail only; an unreadable row must not abort the // handover halfway, which would leave the owner pointer and the member rows // disagreeing about who owns the trip. - const broken = await rosterWithMissingRow('SELECT email FROM users WHERE id = ?'); - const result = await broken.transferOwnership(trip.id, member.id, owner.id); + const spy = vi.spyOn(usersRepo, 'getEmail').mockResolvedValueOnce(null); + let result; + try { + result = await roster.transferOwnership(trip.id, member.id, owner.id); + } finally { + spy.mockRestore(); + } expect(result.fromEmail).toBe(''); expect(result.toEmail).toBe(member.email); expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(member.id); diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index beb21c48d..8e7d914ce 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -78,10 +78,11 @@ import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.serv import { makeStorageFixture } from '../../helpers/storage-fixture'; import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; +import type { TripsRepository } from '../../../src/db/repositories/Trips.repository'; import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm, createTestDaysRepo, createTestDayAssignmentsRepo, createTestDayNotesRepo, createTestTripsRepo, - createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, + createTestTripMembersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, } from '../../helpers/test-uow'; @@ -130,13 +131,18 @@ beforeAll(async () => { new JourneyDomainService(dbs(), new RealtimeService(), new TrekPhotosRepository(dbs()), await createTestUnitOfWork(dbs().connection)), makeStorageFixture('').storage, await accommodationsOver(dbs()), await createTestUnitOfWork(dbs().connection), + await createTestPlacesRepo(dbs().connection), + await createTestTagsRepo(dbs().connection), + await createTestPlaceRatingsRepo(dbs().connection), + await createTestTripMembersRepo(dbs().connection), + await createTestDayAssignmentsRepo(dbs().connection), ); - membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection)); + membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection), await createTestTripsRepo(dbs().connection), await createTestTripMembersRepo(dbs().connection), await createTestUsersRepo(dbs().connection)); }); -const buildReadModel = async (database: DatabaseService, roster: TripMembersService = membersSvc) => +const buildReadModel = async (tripsRepo: TripsRepository, roster: TripMembersService = membersSvc) => new TripReadModelService( - database, roster, daysSvc, accommodationsSvc, budgetSvc, + tripsRepo, roster, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection)), new ReservationsService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), await accommodationsOver(dbs()), await createTestUnitOfWork(dbs().connection)), new CollabService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), makeStorageFixture('').storage, new RateLimitService(), await createTestUnitOfWork(dbs().connection)), @@ -146,8 +152,10 @@ const buildReadModel = async (database: DatabaseService, roster: TripMembersServ ); let svc: Awaited>; +let tripsRepo: TripsRepository; beforeAll(async () => { - svc = await buildReadModel(dbs()); + tripsRepo = await createTestTripsRepo(dbs().connection); + svc = await buildReadModel(tripsRepo); }); beforeAll(() => { @@ -173,24 +181,13 @@ const addPackingItem = (tripId: number, name: string, checked: number) => testDb.prepare('INSERT INTO packing_items (trip_id, name, checked) VALUES (?, ?, ?)') .run(tripId, name, checked); -/** - * A DatabaseService whose connection cannot resolve the owner lookup, standing in - * for the trip row disappearing between the two SELECTs getTripSummary issues. - * Only the read model's own two statements go through this connection; the - * sibling services keep the real one. - */ -function ownerlessDbs(): DatabaseService { - const conn = new Proxy(testDb, { - get(target, prop) { - if (prop === 'prepare') { - return (sql: string) => - (sql.includes('SELECT user_id FROM trips') ? { get: () => undefined } : target.prepare(sql)); - } - const v = (target as any)[prop]; - return typeof v === 'function' ? v.bind(target) : v; - }, - }); - return { connection: conn } as unknown as DatabaseService; +// R8 (Plan 3c program brief item 8): the SQL-text-keyed Proxy fault injection +// this file used to build (`ownerlessDbs`, keyed on the literal +// `'SELECT user_id FROM trips'` fragment) is rewritten onto a repository-level +// fault: TR-A's `getOwnerId` failing while TR-B's `findRaw` still succeeds — +// the trip row disappearing between the two reads `getTripSummary` issues. +function forceMissingOwner(): ReturnType { + return vi.spyOn(tripsRepo, 'getOwnerId').mockResolvedValueOnce(null); } // ── Tests ───────────────────────────────────────────────────────────────────── @@ -208,17 +205,42 @@ describe('getTripSummary guards', () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); - // Trip row and owner id are two separate SELECTs; if the second one comes back + // Trip row and owner id are two separate reads; if the second one comes back // empty (trip deleted in between) the guard has to stop. Without it listMembers // runs with an undefined owner id and the summary reports an ownerless trip. - expect(await (await buildReadModel(ownerlessDbs())).getTripSummary(trip.id, owner.id)).toBeNull(); + const spy = forceMissingOwner(); + try { + expect(await svc.getTripSummary(trip.id, owner.id)).toBeNull(); + } finally { + spy.mockRestore(); + } - // Same trip through the real connection still aggregates — the null above is + // Same trip through the real repository still aggregates — the null above is // the missing owner row, not a broken fixture. expect(await svc.getTripSummary(trip.id, owner.id)).not.toBeNull(); }); }); +describe('getTripSummary — feed_token never reaches the wire (TR-B)', () => { + it('TRIP-READ-002b: withoutFeedToken strips feed_token even though findRaw hands it back intact', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + testDb.prepare('UPDATE trips SET feed_token = ? WHERE id = ?').run('secret-anon-feed-credential', trip.id); + + // TripsRepository.findRaw is a plain `SELECT *` — it hands feed_token back + // intact (proven directly, bypassing the service, so this test would fail + // if the repository ever started blanking the column itself). + expect((await tripsRepo.findRaw(trip.id))?.feed_token).toBe('secret-anon-feed-credential'); + + // getTripSummary's own withoutFeedToken() strip is the only thing standing + // between that credential and an MCP reader — assert it on the actual + // result object, not just that the call succeeds. + const summary = (await svc.getTripSummary(trip.id, owner.id))!; + expect(summary.trip).not.toHaveProperty('feed_token'); + expect(JSON.stringify(summary)).not.toContain('secret-anon-feed-credential'); + }); +}); + describe('getTripSummary shaping', () => { it('TRIP-READ-003: folds a falsy total_price into the budget total instead of poisoning it', async () => { const { user: owner } = createUser(testDb); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 4ca156d05..c1dd2cbf8 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -91,7 +91,7 @@ import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.se import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm, createTestDaysRepo, createTestDayAssignmentsRepo, createTestDayNotesRepo, createTestTripsRepo, - createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, + createTestTripMembersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, } from '../../helpers/test-uow'; @@ -168,6 +168,11 @@ beforeAll(async () => { new JourneyDomainService(dbs(), new RealtimeService(), new TrekPhotosRepository(dbs()), await createTestUnitOfWork(dbs().connection)), makeStorageFixture('').storage, await accommodationsOver(dbs()), await createTestUnitOfWork(dbs().connection), + await createTestPlacesRepo(dbs().connection), + await createTestTagsRepo(dbs().connection), + await createTestPlaceRatingsRepo(dbs().connection), + await createTestTripMembersRepo(dbs().connection), + await createTestDayAssignmentsRepo(dbs().connection), ); svc = new TripsService( dbs(), @@ -182,9 +187,9 @@ beforeAll(async () => { await createTestUnitOfWork(dbs().connection), (await sharedTestOrm(testDb)).em, ); - membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection)); + membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection), await createTestTripsRepo(dbs().connection), await createTestTripMembersRepo(dbs().connection), await createTestUsersRepo(dbs().connection)); readModelSvc = new TripReadModelService( - dbs(), membersSvc, daysSvc, accommodationsSvc, budgetSvc, + await createTestTripsRepo(dbs().connection), membersSvc, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection)), new ReservationsService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs()), accommodationsSvc, await createTestUnitOfWork(dbs().connection)), new CollabService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), coversFx.storage, new RateLimitService(), await createTestUnitOfWork(dbs().connection)), @@ -1252,15 +1257,6 @@ describe('quirk fixes', () => { ); } - /** Same frozen connection, for the guest deletion that now lives on the roster. */ - async function failingMembers(match: string) { - return new TripMembersService( - failingConnection(match), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), - new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), - notificationsStub(), await createTestUnitOfWork(dbs().connection), - ); - } - it('TRIP-SVC-051: remove is atomic — a failed trip DELETE keeps the journey entries intact', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -1279,14 +1275,25 @@ describe('quirk fixes', () => { expect(testDb.prepare('SELECT id FROM trips WHERE id = ?').get(trip.id)).toBeDefined(); }); + // R8 (Plan 3c program brief item 8): the SQL-text-keyed `failingConnection` + // Proxy this test used to build a `failingMembers(match)` service around is + // rewritten as a repository-level fault (Task 6 converted `deleteGuest` off + // `this.db.prepare(...)` entirely, onto `UsersRepository.deleteGuest` — the + // Proxy could never trigger any more, since no statement text runs through + // `this.db` inside `deleteGuest`'s transaction). it('TRIP-SVC-052: deleteGuest is atomic — a failed user DELETE rolls the budget re-split back', async () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); const { member: guest } = await membersSvc.createGuest(trip.id, 'Gia', owner.id); const item = await budgetSvc.createBudgetItem(trip.id, { name: 'Dinner', total_price: 80, member_ids: [owner.id, guest.id] }); - const broken = await failingMembers('DELETE FROM users WHERE id = ? AND is_guest = 1'); - await expect(broken.deleteGuest(trip.id, guest.id)).rejects.toThrow('boom'); + const usersRepo = await createTestUsersRepo(dbs().connection); + const spy = vi.spyOn(usersRepo, 'deleteGuest').mockRejectedValueOnce(new Error('boom')); + try { + await expect(membersSvc.deleteGuest(trip.id, guest.id)).rejects.toThrow('boom'); + } finally { + spy.mockRestore(); + } // Neither the guest nor their split membership was touched. expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(guest.id)).toBeDefined();