diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index d7824f7c0..b4970d22d 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -279,10 +279,17 @@ export function maxOf(platform: Platform, ref: string, aliasName: string): RawQu * 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. + * `value`/`whenTrue`/`whenFalse` are all passed through `raw()`'s own + * parameter escaping, never spliced into the SQL text by this function — + * 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. (Task 6 review's + * I1: downstream, MikroORM still formats the FINAL query with every + * parameter inlined as an escaped literal in the SQL text it hands + * better-sqlite3 — not as a separate bound-params array — so a non-finite + * `value` renders as the bare token `NaN`/`Infinity` and fails at PREPARE + * time regardless of this function's own escaping; callers must guard a + * non-finite `value` before calling.) */ 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]); diff --git a/server/src/db/repositories/Days.repository.ts b/server/src/db/repositories/Days.repository.ts index ac22f0d71..beb9e0f9b 100644 --- a/server/src/db/repositories/Days.repository.ts +++ b/server/src/db/repositories/Days.repository.ts @@ -1,4 +1,7 @@ import type { Days } from '../entities/Days.entity'; +import { DayAssignments } from '../entities/DayAssignments.entity'; +import { DayNotes } from '../entities/DayNotes.entity'; +import { DayAccommodations } from '../entities/DayAccommodations.entity'; import { toRow, type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; @@ -199,4 +202,66 @@ export class DaysRepository extends TrekRepository { .execute<{ id: number } | undefined>('get', false); return !!row; } + + // --------------------------------------------------------------------------- + // Plan 3c Task 7 (`TripsService.generateDays`) — appended per the task's own + // file-ownership rule ("additive methods on Days/Places/Users repositories"). + // --------------------------------------------------------------------------- + + /** TP3 — `UPDATE days SET date = NULL WHERE id = ?` (nullifies rather than deletes: assignments/notes/accommodations survive). */ + async clearDate(id: number): Promise { + await this.nativeUpdate({ id }, { date: null }); + } + + /** + * TP12 (`trips.service.ts::generateDays`'s `isEmptyDay`) — the legacy + * statement is `SELECT NOT EXISTS(...) AND NOT EXISTS(...) AND NOT + * EXISTS(...) AS empty` with no `FROM` clause and `@id` named params, the + * single most dialect-specific statement in the cluster (inventory §17b). + * Expressed here as three `count()`s (R6's own "three counts" alternative + * to a Kysely `selectNoFrom`), one per table the legacy statement's three + * `NOT EXISTS` correlate against — `day_assignments` and `day_notes` are + * this plan's own tables; `day_accommodations` is Plan 3d's, read (never + * written) here through the entity's already-generated `TrekRepository` + * (`this.getEntityManager().getRepository(DayAccommodations)`, the same + * cross-repository-read shape `TripMembersRepository.rosterUserIds` + * documents for `Trips`) — no domain method of `days`' own is added to + * that repository, so ownership of `day_accommodations`' write logic + * stays with Plan 3d. + */ + async isEmptyDay(day_id: number): Promise { + const em = this.getEntityManager(); + const [assignments, notes, accommodations] = await Promise.all([ + em.getRepository(DayAssignments).count({ day: day_id }), + em.getRepository(DayNotes).count({ day: day_id }), + em.getRepository(DayAccommodations).count({ $or: [{ startDay: day_id }, { endDay: day_id }] }), + ]); + return assignments === 0 && notes === 0 && accommodations === 0; + } + + /** + * TP6 (`trips.service.ts::generateDays`'s trailing-empty trim) — the + * legacy statement orders candidate days `ORDER BY d.day_number DESC` and + * takes the first `limit` that pass all three `NOT EXISTS` checks (§17b). + * Reproduced here by reading every day of the trip in the same + * `day_number DESC` order and filtering with {@link isEmptyDay} per row, + * stopping once `limit` matches are collected — same result set as the + * single correlated statement (both examine days in day_number-descending + * order and take the first N that are empty), documented per R6 as the + * "three counts, comment which" alternative to a Kysely `selectNoFrom`. + */ + async listTrailingEmptyIds(trip_id: number, limit: number): Promise { + if (limit <= 0) return []; + const candidates = await this.qb('d') + .select(['d.id']) + .where({ trip: trip_id }) + .orderBy({ day_number: 'desc' }) + .execute<{ id: number }[]>('all', false); + const empty: number[] = []; + for (const c of candidates) { + if (empty.length >= limit) break; + if (await this.isEmptyDay(c.id)) empty.push(c.id); + } + return empty; + } } diff --git a/server/src/db/repositories/TripMembers.repository.ts b/server/src/db/repositories/TripMembers.repository.ts index 6ab99b07c..77b5f8833 100644 --- a/server/src/db/repositories/TripMembers.repository.ts +++ b/server/src/db/repositories/TripMembers.repository.ts @@ -176,14 +176,18 @@ export class TripMembersRepository extends TrekRepository { * `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. + * non-numeric id parses to `NaN`. **Mechanism (Task 6 review's I1, + * captured by wrapping `better-sqlite3.prepare`, not assumed): MikroORM + * formats every query with ALL parameters inlined as escaped literals — + * the params array that reaches better-sqlite3 is always empty — so a + * non-finite `NaN`/`Infinity` renders as a bare, unquoted SQL token and + * SQLite fails at PREPARE time (`no such column: NaN`, a 500), before any + * statement runs. A raw `?` placeholder does not avoid this (a typed + * filter has the identical failure) because the inlining happens during + * formatting, not at the bind step.** 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; @@ -239,10 +243,14 @@ export class TripMembersRepository extends TrekRepository { * 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 + * placeholder does NOT fix this (Task 6 review's I1, captured by wrapping + * `better-sqlite3.prepare`): MikroORM formats every query with ALL + * parameters inlined as escaped literals — the params array that reaches + * better-sqlite3 is always empty — so `NaN` renders as that same bare + * token regardless of whether the predicate was written as a typed filter + * or a raw `?` placeholder (reproduced directly: still `no such column: + * NaN` at prepare time, before any bind happens — the inlining happens + * during formatting, not at the bind step). 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 @@ -257,4 +265,29 @@ export class TripMembersRepository extends TrekRepository { .andWhere('m.user_id = ?', [user_id]) .execute('run'); } + + // --------------------------------------------------------------------------- + // Plan 3c Task 7 (`trips.rpc.ts::members`) — additive: no other repository + // is rooted at `trip_members` joined to `users`, and RP3's shape is + // deliberately NOT `listWithUserAndInviter`'s (TM2's COALESCE'd + // `username`), so it gets its own method here rather than a variant + // parameter on TM2's. + // --------------------------------------------------------------------------- + + /** + * RP3 (`trips.rpc.ts::members`) — `SELECT u.id, u.username, u.display_name, + * u.avatar FROM trip_members tm JOIN users u ON u.id = tm.user_id WHERE + * tm.trip_id = ?`. Emits raw `username` AND `display_name` as SEPARATE + * fields — deliberately NOT `listWithUserAndInviter`'s (TM2) COALESCE'd + * single `username` (the ruling: "do not harmonise" — a plugin reading + * `trips.members` and the REST/MCP roster are two different wire shapes on + * purpose, inventory §18.10). + */ + async listRawUsernameAndDisplayName(trip_id: number | string): Promise<{ id: number; username: string; display_name: string | null; avatar: string | null }[]> { + return await this.qb('m') + .join('m.user', 'u') + .select(['u.id', 'u.username', 'u.display_name', 'u.avatar']) + .where('m.trip_id = ?', [trip_id]) + .execute<{ id: number; username: string; display_name: string | null; avatar: string | null }[]>('all', false); + } } diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index cac53b4f4..6b14e7b8e 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 { currentTimestamp } from '../dialect/sql-functions'; import type { AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; @@ -35,6 +36,66 @@ export interface TripRawRow { const _tripRawRowKeys: AssertRowKeys = true; +/** + * `TRIP_SELECT`'s output row (inventory §11a) — every `TripRawRow` column + * plus the four computed/joined columns the projection adds. `feed_token` + * is always `null` here (SQL-side blanking via `NULL AS feed_token`/its + * Kysely equivalent, never a JS strip) — see `TripsRepository + * .tripSelectQuery`'s docstring. + */ +export interface TripSelectRow extends Omit { + feed_token: null; + day_count: number; + place_count: number; + is_owner: number; + owner_username: string; + shared_count: number; +} + +/** The narrow `trips`/`users`/`days`/`places`/`trip_members` shape `tripSelectQuery` needs (`this.kysely()`'s typed `DB` argument). */ +interface TripSelectKyselyDB { + trips: { + // `number | string`, not a bare `number`: the raw-bind seam (D4's T5 + // escape hatch) — `findForViewer`'s `WHERE t.id = ?` accepts the route's + // unconverted id with no coercion, the same seam every QB-based method + // in this file documents. Widening the TYPE (not spelling `sql\`...\``, + // which `no-restricted-syntax` bans in `src/db/repositories/**`) is what + // lets `.where('t.id', '=', trip_id)` accept a `string` here. + id: number | string; + 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; + }; + users: { id: number; username: string }; + trip_members: { id: number; trip_id: number; user_id: number }; + days: { id: number; trip_id: number }; + places: { id: number; trip_id: number }; +} + +/** TP21's output row (inventory §11c) — `relevance` is the computed sort key, never sent to the client (the controller destructures `{id, title, start_date, end_date}` only). */ +export interface ActiveTripRow { + id: number; + title: string; + start_date: string | null; + end_date: string | null; + relevance: number; +} + +/** The narrow `trips`/`trip_members` shape `activeTrip` needs. */ +interface ActiveTripKyselyDB { + trips: { id: number; title: string; start_date: string | null; end_date: string | null; user_id: number; is_archived: number | null }; + trip_members: { id: number; trip_id: number; user_id: number }; +} + export class TripsRepository extends TrekRepository { /** * The `LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? @@ -224,4 +285,215 @@ export class TripsRepository extends TrekRepository { .execute('get', false); return row ?? null; } + + // --------------------------------------------------------------------------- + // Plan 3c Task 7 (`TripsService`) — `TRIP_SELECT` (inventory §11a) becomes + // ONE builder here (`tripSelectQuery`), expressed through `this.kysely()` + // (a typed one-table-plus-joins DB interface, the same escape hatch + // `DayAssignmentsRepository.reanchorToDay`/`DayAssignmentsRepository + // .listForTimeSort` use): three correlated scalar `COUNT(*)` subqueries a + // QueryBuilder select list cannot express, a `CASE WHEN` projected column, + // and `NULL AS feed_token` placed AFTER `t.*` (via `.selectAll('t')` then + // a later `.select()` of the literal) so the duplicate key wins in the row + // object exactly the way the legacy statement's column order does — not by + // hand-listing every `trips` column (§18.5: that loses the "blanked once, + // survives a schema change" guarantee the legacy comment describes). + // Verified directly with `.compile()` against a real in-memory DB before + // wiring in (task report, TRIP_SELECT SQL captures) — a fully seeded trip + // (owner, member, stranger; day/place counts; `feed_token` set in the DB) + // produces `feed_token: null`, `is_owner`, `owner_username`, all four + // counts, byte-identical in shape to a raw run of the legacy statement. + // --------------------------------------------------------------------------- + + /** + * `this.kysely()`'s typed `DB` argument for every `TRIP_SELECT`-shaped + * query below — narrowed to the columns those queries read or join + * through, per `WebauthnChallengesRepository.claimChallenge`'s docstring + * (entity-metadata inference is not what a hand-written statement wants). + */ + private tripSelectQuery(user_id: number) { + return this.kysely() + .selectFrom('trips as t') + .innerJoin('users as u', 'u.id', 't.user_id') + .selectAll('t') + .select((eb) => [ + eb.val(null).as('feed_token'), + eb.selectFrom('days as d').select((eb2) => eb2.fn.countAll().as('c')).whereRef('d.trip_id', '=', 't.id').as('day_count'), + eb.selectFrom('places as p').select((eb2) => eb2.fn.countAll().as('c')).whereRef('p.trip_id', '=', 't.id').as('place_count'), + eb.case().when('t.user_id', '=', user_id).then(1).else(0).end().as('is_owner'), + 'u.username as owner_username', + eb.selectFrom('trip_members as tm').select((eb2) => eb2.fn.countAll().as('c')).whereRef('tm.trip_id', '=', 't.id').as('shared_count'), + ]); + } + + /** + * TP20 (`trips.service.ts::get`) — `TRIP_SELECT` scoped to one trip AND + * the access predicate (`t.user_id = :userId OR m.user_id IS NOT NULL`), + * i.e. "the trip if this viewer may see it, else nothing". Also serves + * TP19 (`create`'s post-insert re-select) and TP29 (`updateTrip`'s + * post-write re-select): both call sites are only ever reached after the + * acting user has already passed an access check for this exact trip + * (creator = owner; `updateTrip`'s callers all gate on `canAccessTrip`/ + * `requireTripEdit` first), so the access predicate here is always + * trivially satisfied in those two contexts and the result is identical + * to the legacy's unscoped `WHERE t.id = :tripId` re-select. Also serves + * `TripMembersService.getTripForViewer` (TM1, Task 6 review's "for Task + * 7" note) — same reasoning: every caller already holds access. + * + * Raw-bind (`number | string`, D4's T5 escape hatch — `TripSelectKyselyDB`'s + * `trips.id` is typed `number | string` for exactly this): the legacy + * statement bound the route's unconverted `tripId` with no `Number()`/ + * `toRowId` conversion, matching `findAccessible`'s documented seam. + */ + async findForViewer(trip_id: number | string, user_id: number): Promise { + const row = await this.tripSelectQuery(user_id) + .leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id)) + .where('t.id', '=', trip_id) + .where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])) + .executeTakeFirst(); + return row as TripSelectRow | undefined; + } + + /** + * TP16/TP17 (`trips.service.ts::list`) — `TRIP_SELECT` + the access join, + * optionally filtered by `is_archived`, `ORDER BY t.created_at DESC`. + * `archived === null` reproduces TP16 (no filter); any other value (0 or + * 1, the controller's own coercion) reproduces TP17. + */ + async listForUser(user_id: number, archived: number | null): Promise { + let query = this.tripSelectQuery(user_id) + .leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id)) + .where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])); + if (archived !== null) query = query.where('t.is_archived', '=', archived); + const rows = await query.orderBy('t.created_at', 'desc').execute(); + return rows as unknown as TripSelectRow[]; + } + + /** + * TP21 (`trips.service.ts::activeTrip`) — the triple `CASE WHEN … + * relevance` projection and the double-`CASE WHEN` `ORDER BY` (one ASC, + * one DESC). The legacy statement's `ORDER BY` items reference the + * `relevance` SELECT-list alias (`CASE WHEN relevance < 2 …`, `CASE WHEN + * relevance = 2 …`); reproduced here by recomputing the equivalent + * boolean expression inline at each of the four sites (the `.select()` + * and three `.orderBy()` calls below) rather than referencing the alias, + * since Kysely's typed `ORDER BY` can't name a computed SELECT alias and + * `sql`-tagged templates are banned in `src/db/repositories/**` — and a + * shared private helper can't be typed across these four callbacks either + * (`leftJoin`'s alias widens each callback's own `ExpressionBuilder` + * table-set in a way a separate generic method signature can't match + * structurally). Semantically identical to the legacy statement — SQLite + * evaluates both forms to the same three-way ordering, verified directly + * against `.compile()`/real rows before wiring in (task report, + * TRIP_SELECT SQL captures): `running` = `relevance = 0`, `upcoming` = + * `relevance = 1`, `running OR upcoming` = `relevance < 2`. + */ + async activeTrip(user_id: number, today: string): Promise { + const row = await this.kysely() + .selectFrom('trips as t') + .leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id)) + .select(['t.id', 't.title', 't.start_date', 't.end_date']) + .select((eb) => [ + eb.case() + .when(eb.and([eb('t.start_date', 'is not', null), eb('t.end_date', 'is not', null), eb('t.start_date', '<=', today), eb('t.end_date', '>=', today)])) + .then(0) + .when(eb.and([eb('t.start_date', 'is not', null), eb('t.start_date', '>=', today)])) + .then(1) + .else(2) + .end() + .as('relevance'), + ]) + .where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])) + .where('t.is_archived', '=', 0) + .orderBy((eb) => + eb.case() + .when(eb.and([eb('t.start_date', 'is not', null), eb('t.end_date', 'is not', null), eb('t.start_date', '<=', today), eb('t.end_date', '>=', today)])) + .then(0) + .when(eb.and([eb('t.start_date', 'is not', null), eb('t.start_date', '>=', today)])) + .then(1) + .else(2) + .end(), 'asc') + .orderBy((eb) => + eb.case() + .when(eb.or([ + eb.and([eb('t.start_date', 'is not', null), eb('t.end_date', 'is not', null), eb('t.start_date', '<=', today), eb('t.end_date', '>=', today)]), + eb.and([eb('t.start_date', 'is not', null), eb('t.start_date', '>=', today)]), + ])) + .then(eb.ref('t.start_date')) + .end(), 'asc') + .orderBy((eb) => + eb.case() + .when(eb.not(eb.or([ + eb.and([eb('t.start_date', 'is not', null), eb('t.end_date', 'is not', null), eb('t.start_date', '<=', today), eb('t.end_date', '>=', today)]), + eb.and([eb('t.start_date', 'is not', null), eb('t.start_date', '>=', today)]), + ]))) + .then(eb.ref('t.start_date')) + .end(), 'desc') + .limit(1) + .executeTakeFirst(); + return row as ActiveTripRow | undefined; + } + + /** + * TP18 (`trips.service.ts::create`'s INSERT) — the column set of the + * legacy statement, written verbatim (the `||`/clamp defaults are the + * service's own, decided before this call, per the program's D2 split). + * Returns `em.insert()`'s PK (R6's `lastInsertRowid` replacement). + */ + async insertTrip(input: { + user_id: number; + title: string; + description: string | null; + start_date: string | null; + end_date: string | null; + currency: string; + reminder_days: number; + }): Promise { + return await this.insert({ + user: input.user_id, + title: input.title, + description: input.description, + start_date: input.start_date, + end_date: input.end_date, + currency: input.currency, + reminder_days: input.reminder_days, + }); + } + + /** + * TP25 (`trips.service.ts::updateTrip`) — `UPDATE trips SET title=?, + * description=?, start_date=?, end_date=?, currency=?, is_archived=?, + * cover_image=?, reminder_days=?, updated_at=CURRENT_TIMESTAMP WHERE + * id=?`. The `||`/`!== undefined` pre-image fallbacks are the SERVICE's + * own (R5/§18.6 — this write stays OUTSIDE the days-regeneration + * transaction, unchanged); this takes the final, already-decided values + * and writes them verbatim, `updated_at` stamped unconditionally like the + * legacy statement. Named `updateTripRow` (not `updateTrip`, the + * deliverable list's own name) to keep it unambiguous next to + * `TripsService.updateTrip`, which calls it. + */ + async updateTripRow(id: number, data: { + title: string; + description: string | null; + start_date: string | null; + end_date: string | null; + currency: string; + is_archived: number; + cover_image: string | null; + reminder_days: number; + }): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { ...data, updated_at: currentTimestamp(platform) }); + } + + /** TP35 (`trips.service.ts::updateCoverImage`) — `UPDATE trips SET cover_image=?, updated_at=CURRENT_TIMESTAMP WHERE id=?`. */ + async setCoverImage(id: number, cover_image: string): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { cover_image, updated_at: currentTimestamp(platform) }); + } + + /** TP34 (`trips.service.ts::remove`) — `DELETE FROM trips WHERE id = ?` (security-sensitive: cascades the whole trip). */ + async deleteById(id: number): Promise { + await this.nativeDelete({ id }); + } } diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index b41fb2d65..0a1c904d6 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -9,7 +9,6 @@ import { UnitOfWork } from '../database/unit-of-work'; import { BudgetService } from '../budget/budget.service'; import { PermissionsService } from '../permissions/permissions.service'; import { RealtimeService } from '../realtime/realtime.service'; -import { TRIP_SELECT } from '../trips/trips.service'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { emitUserDeleted } from '../../plugin-user-lifecycle'; import { NotFoundError, ValidationError } from '../common/domain-errors'; @@ -80,18 +79,6 @@ export class TripMembersService { @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; - } - async canAccessTrip(tripId: string | number, userId: number) { const access = await this.dbs.canAccessTrip(tripId, userId); return access as { user_id: number } | null | undefined; @@ -107,9 +94,13 @@ 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. - * // Task 7: converts once TripsService itself does — see the class docstring above. */ + * Plan 3c Task 7: repointed to `TripsRepository.findForViewer` — the + * SQL-side `NULL AS feed_token` blanking survives the move (Task 6 + * review's "for Task 7" note): `findForViewer`'s row always carries + * `feed_token: null`, which is load-bearing for the `trip:updated` + * broadcast a transfer sends. */ async getTripForViewer(tripId: string | number, userId: number) { - return this.db.prepare(`${TRIP_SELECT} WHERE t.id = :tripId`).get({ userId, tripId }); + return this.tripsRepo.findForViewer(tripId, userId); } /** Fire-and-forget trip-invite notification (mirrors the route's dynamic import). */ diff --git a/server/src/nest/trips/trips.rpc.ts b/server/src/nest/trips/trips.rpc.ts index e0cd6f84f..8b7f3ebc0 100644 --- a/server/src/nest/trips/trips.rpc.ts +++ b/server/src/nest/trips/trips.rpc.ts @@ -1,3 +1,4 @@ +import { EntityManager } from '@mikro-orm/core'; import { tripCreateRequestSchema, tripUpdateRequestSchema } from '@trek/shared'; import { PluginController, PluginMethod } from '../plugins/host/rpc-kit/decorators'; import { PluginGuards } from '../plugins/host/plugin-guards.service'; @@ -5,12 +6,15 @@ import { BadParams, ForbiddenResource } from '../plugins/host/rpc-errors'; import { num, schemaMessage } from '../plugins/host/rpc-params'; import type { PluginRpcContext } from '../plugins/host/rpc-kit/types'; import { RealtimeService } from '../realtime/realtime.service'; -import { DatabaseService } from '../database/database.service'; import { ReservationsService } from '../reservations/reservations.service'; import { DaysService } from '../days/days.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; import { TripMembersService } from '../trip-members/trip-members.service'; import { TripMembershipService } from '../trip-membership/trip-membership.service'; +import { Trips } from '../../db/entities/Trips.entity'; +import { Places } from '../../db/entities/Places.entity'; +import { Users } from '../../db/entities/Users.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; import { TripsService, NotFoundError, ValidationError, withoutFeedToken } from './trips.service'; const TRIP_EDIT_ACTION = 'trip_edit'; @@ -36,20 +40,30 @@ export class TripsRpc { private readonly reservations: ReservationsService, private readonly days: DaysService, private readonly membership: TripMembershipService, - private readonly db: DatabaseService, + // Plan 3c Task 7: `DatabaseService` DROPPED — RP1–RP6 were its only uses + // in this class, and every one now resolves through `em.getRepository(...)` + // below (same precedent as `TripReadModelService`, Task 6: "DatabaseService + // is dropped from its constructor entirely" once a service goes fully + // SQL-free). Every call site that hand-constructs `TripsRpc` positionally + // drops its `db` fake/instance argument in the same slot. private readonly realtime: RealtimeService, private readonly guards: PluginGuards, // Appended: the hand-wired plugin-host harnesses build this positionally. private readonly accommodations: AccommodationsService, private readonly roster: TripMembersService, + // Plan 3c Task 7: RP1–RP6's repository calls all resolve through this, + // the same `this.em.getRepository(...)` shape `TripsService.canAccessTrip`/ + // `.isOwner` used since Task 0b — `EntityManager` is `@Global()`, so no + // module needs new wiring. Appended last, per this class's own convention. + private readonly em: EntityManager, ) {} @PluginMethod('trips.getById', { permission: 'db:read:trips' }) async getById(params: Record, ctx: PluginRpcContext): Promise { - return this.guards.tripRead(params, ctx, () => + return this.guards.tripRead(params, ctx, async () => // db:read:trips is a read grant on the trip, not on the credential that - // publishes it anonymously — see withoutFeedToken. - withoutFeedToken(this.db.prepare('SELECT * FROM trips WHERE id = ?').get(num(params.tripId, 'tripId'))), + // publishes it anonymously — see withoutFeedToken. RP1 — TripsRepository.findRaw. + withoutFeedToken(await this.em.getRepository(Trips).findRaw(num(params.tripId, 'tripId'))), ); } @@ -58,10 +72,9 @@ export class TripsRpc { // The trip's place POOL. Places carry no itinerary position of their own // (day_id/order_index live on day_assignments), so order by created_at like the // REST list does. trips.getDays is the day-ordered itinerary. - return this.guards.tripRead(params, ctx, () => - this.db - .prepare('SELECT * FROM places WHERE trip_id = ? ORDER BY created_at DESC') - .all(num(params.tripId, 'tripId')), + // RP2 — PlacesRepository.listForTripOrdered (Task 4/5's repository, Task 7's own additive method). + return this.guards.tripRead(params, ctx, async () => + await this.em.getRepository(Places).listForTripOrdered(num(params.tripId, 'tripId')), ); } @@ -106,10 +119,12 @@ export class TripsRpc { @PluginMethod('trips.members', { permission: 'db:read:trips' }) async members(params: Record, ctx: PluginRpcContext): Promise { - return this.guards.tripRead(params, ctx, () => - this.db - .prepare('SELECT u.id, u.username, u.display_name, u.avatar FROM trip_members tm JOIN users u ON u.id = tm.user_id WHERE tm.trip_id = ?') - .all(num(params.tripId, 'tripId')) as unknown[], + // RP3 — TripMembersRepository.listRawUsernameAndDisplayName: raw `username` + // AND `display_name` as separate fields, deliberately NOT TM2's COALESCE + // (inventory §18.10 — a plugin's roster shape and the REST/MCP roster + // shape are two different wire shapes on purpose; do not harmonise). + return this.guards.tripRead(params, ctx, async () => + await this.em.getRepository(TripMembers).listRawUsernameAndDisplayName(num(params.tripId, 'tripId')), ); } @@ -130,11 +145,11 @@ export class TripsRpc { if ('cover_image' in input && !(await this.guards.canEditAs('trip_cover_upload', tripId, actor))) { throw new ForbiddenResource(`no permission to change the cover of trip ${tripId}`); } - const user = this.db.prepare('SELECT role FROM users WHERE id = ?').get(actor) as { role?: string } | undefined; + const role = await this.em.getRepository(Users).getRole(actor); // RP4 — UsersRepository.getRole try { // The no-rebase core, parity with the legacy host path, which never // re-anchored the budget currency. - const result = await this.trips.updateTrip(tripId, actor, input as Parameters[2], user?.role ?? 'user'); + const result = await this.trips.updateTrip(tripId, actor, input as Parameters[2], role ?? 'user'); this.realtime.broadcast(tripId, 'trip:updated', { trip: result.updatedTrip }); return result.updatedTrip; } catch (e) { @@ -172,7 +187,8 @@ export class TripsRpc { const targetUserId = num(params.userId, 'userId'); const actor = this.guards.requireActor(ctx, 'trip member'); await this.guards.requireTripEdit(tripId, actor, MEMBER_MANAGE_ACTION); - const target = this.db.prepare('SELECT id FROM users WHERE id = ?').get(targetUserId) as { id: number } | undefined; + // RP5 — UsersRepository.findIdAndEmail (existence check only; the `email` field is unused here). + const target = await this.em.getRepository(Users).findIdAndEmail(targetUserId); if (!target) throw new ForbiddenResource(`no user ${targetUserId}`); // The acting user is recorded as the inviter. return this.membership.joinTripAsMember(tripId, targetUserId, actor); @@ -186,8 +202,8 @@ export class TripsRpc { await this.guards.requireTripEdit(tripId, actor, MEMBER_MANAGE_ACTION); // Never remove the OWNER through this path: that would orphan the trip. // Ownership transfer is a separate, deliberate action. - const trip = this.db.prepare('SELECT user_id FROM trips WHERE id = ?').get(tripId) as { user_id: number } | undefined; - if (trip && trip.user_id === targetUserId) throw new ForbiddenResource('cannot remove the trip owner'); + const ownerId = await this.em.getRepository(Trips).getOwnerId(tripId); // RP6 — TripsRepository.getOwnerId + if (ownerId !== null && ownerId === targetUserId) throw new ForbiddenResource('cannot remove the trip owner'); await this.roster.removeMember(tripId, targetUserId); return { removed: true }; } diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index 0a4d37fc5..28131ba9e 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -3,6 +3,8 @@ import path from 'path'; import { EntityManager } from '@mikro-orm/core'; import { DatabaseService } from '../database/database.service'; import { Trips } from '../../db/entities/Trips.entity'; +import { Days } from '../../db/entities/Days.entity'; +import { Users } from '../../db/entities/Users.entity'; import { MAX_TRIP_DAYS, tripSpanDays, type ActiveTrip, type TrekWsPayload, type TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -172,10 +174,26 @@ export class TripsService { private readonly em: EntityManager, ) {} + // Plan 3c Task 7: the raw better-sqlite3 handle now backs only the three + // documented survivors — `remove`'s TP32/TP33 (journey_entries, Plan 3g) + // and `copy`/`getCopiedTrip` (Task 8's own file). Every other method below + // reads/writes through `tripsRepo`/`daysRepo` (or a sibling repository + // reached the same way `TripsService.canAccessTrip`/`.isOwner` already + // did since Task 0b: `this.em.getRepository(...)`, not a new constructor + // parameter — the shared per-request `EntityManager` caches repositories, + // so this is the same instance a hand-constructed test spies on). private get db() { return this.dbs.connection; } + private get tripsRepo() { + return this.em.getRepository(Trips); + } + + private get daysRepo() { + return this.em.getRepository(Days); + } + async canAccessTrip(tripId: string | number, userId: number) { const access = await this.em.getRepository(Trips).findAccessible(tripId, userId); return access as { user_id: number } | null | undefined; @@ -195,46 +213,49 @@ export class TripsService { // ── Day generation ──────────────────────────────────────────────────────── + /** + * TP1–TP13 (inventory §11b), all through `DaysRepository` — statement order + * and the two-phase renumber (negative pass then positive pass, sequential + * `for…of`, never `Promise.all`: order is load-bearing against + * `UNIQUE(trip_id, day_number)`, the same discipline Task 2's own + * `DaysService` conversion kept) preserved byte-for-byte. Inherits the + * caller's transaction — `updateTrip`'s `:423` `uow.transactional` block — + * or runs with none at all when called from `create` (R5/§18.6, unchanged + * by this conversion; no transaction is opened HERE either way). + * + * TP1/TP4/TP8/TP13 (four legacy sites, one shared repository method): + * `listOrderedForReorder` orders `ASC` by `day_number` where TP1's own + * statement has no `ORDER BY` at all — harmless: TP1's result only ever + * feeds `.filter()`/re-`.sort()` calls below, never relies on read order. + */ async generateDays(tripId: number | bigint | string, startDate: string | null, endDate: string | null, dayCount?: number) { - const existing = this.db.prepare('SELECT id, day_number, date FROM days WHERE trip_id = ?').all(tripId) as { id: number; day_number: number; date: string | null }[]; - const setDayNumber = this.db.prepare('UPDATE days SET day_number = ? WHERE id = ?'); + const trip_id = Number(tripId); + const existing = await this.daysRepo.listOrderedForReorder(trip_id); // TP1 - // Helper: two-phase renumber to avoid UNIQUE(trip_id, day_number) collisions - function renumber(days: { id: number }[]) { - days.forEach((d, i) => setDayNumber.run(-(i + 1), d.id)); - days.forEach((d, i) => setDayNumber.run(i + 1, d.id)); - } + // Two-phase renumber to avoid UNIQUE(trip_id, day_number) collisions. + const renumber = async (days: { id: number }[]) => { + for (let i = 0; i < days.length; i++) await this.daysRepo.setDayNumber(days[i].id, -(i + 1)); // TP2 + for (let i = 0; i < days.length; i++) await this.daysRepo.setDayNumber(days[i].id, i + 1); // TP2 + }; if (!startDate || !endDate) { // Nullify all dated days instead of deleting them — preserves assignments/notes/accommodations const withDates = existing.filter(d => d.date); - if (withDates.length > 0) { - const nullify = this.db.prepare('UPDATE days SET date = NULL WHERE id = ?'); - for (const d of withDates) nullify.run(d.id); - } + for (const d of withDates) await this.daysRepo.clearDate(d.id); // TP3 + // Now all days are dateless — adjust count toward dayCount target - const allDays = this.db.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(tripId) as { id: number }[]; + const allDays = await this.daysRepo.listOrderedForReorder(trip_id); // TP4 const targetCount = Math.min(Math.max(dayCount ?? (allDays.length || 7), 1), MAX_TRIP_DAYS); const needed = targetCount - allDays.length; if (needed > 0) { - const insert = this.db.prepare('INSERT INTO days (trip_id, day_number, date) VALUES (?, ?, NULL)'); - for (let i = 0; i < needed; i++) insert.run(tripId, allDays.length + i + 1); + for (let i = 0; i < needed; i++) await this.daysRepo.insertDay({ trip_id, day_number: allDays.length + i + 1, date: null }); // TP5 } else if (needed < 0) { // Only trim trailing empty days to avoid destroying content - const candidates = this.db.prepare( - `SELECT d.id FROM days d - WHERE d.trip_id = ? - AND NOT EXISTS (SELECT 1 FROM day_assignments da WHERE da.day_id = d.id) - AND NOT EXISTS (SELECT 1 FROM day_notes dn WHERE dn.day_id = d.id) - AND NOT EXISTS (SELECT 1 FROM day_accommodations dac WHERE dac.start_day_id = d.id OR dac.end_day_id = d.id) - ORDER BY d.day_number DESC - LIMIT ?` - ).all(tripId, -needed) as { id: number }[]; - const del = this.db.prepare('DELETE FROM days WHERE id = ?'); - for (const d of candidates) del.run(d.id); + const candidates = await this.daysRepo.listTrailingEmptyIds(trip_id, -needed); // TP6 + for (const id of candidates) await this.daysRepo.deleteById(id); // TP7 } - const remaining = this.db.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(tripId) as { id: number }[]; - renumber(remaining); + const remaining = await this.daysRepo.listOrderedForReorder(trip_id); // TP8 + await renumber(remaining); return; } @@ -257,10 +278,7 @@ export class TripsService { // Phase 1: stamp all existing days with negative day_numbers to free up slots const allExisting = [...dated, ...dateless]; - allExisting.forEach((d, i) => setDayNumber.run(-(i + 1), d.id)); - - const assignDay = this.db.prepare('UPDATE days SET date = ?, day_number = ? WHERE id = ?'); - const insert = this.db.prepare('INSERT INTO days (trip_id, day_number, date) VALUES (?, ?, ?)'); + for (let i = 0; i < allExisting.length; i++) await this.daysRepo.setDayNumber(allExisting[i].id, -(i + 1)); // TP2 let datelessIdx = 0; @@ -268,21 +286,20 @@ export class TripsService { const date = targetDates[i]; if (i < dated.length) { // Positional remap: existing dated day i gets new date — keeps all children - assignDay.run(date, i + 1, dated[i].id); + await this.daysRepo.setDayNumberAndDate(dated[i].id, i + 1, date); // TP9 } else if (datelessIdx < dateless.length) { // Reuse a dateless day — keeps its assignments, notes, etc. - assignDay.run(date, i + 1, dateless[datelessIdx].id); + await this.daysRepo.setDayNumberAndDate(dateless[datelessIdx].id, i + 1, date); // TP9 datelessIdx++; } else { - insert.run(tripId, i + 1, date); + await this.daysRepo.insertDay({ trip_id, day_number: i + 1, date }); // TP10 } } // Overflow dated days (trip shrunk): delete them (issue #909). // Cascade removes their assignments, notes, and accommodations. - const del = this.db.prepare('DELETE FROM days WHERE id = ?'); for (let i = targetDates.length; i < dated.length; i++) { - del.run(dated[i].id); + await this.daysRepo.deleteById(dated[i].id); // TP11 } // Any remaining unused dateless days: drop the empty placeholders so day_count @@ -290,45 +307,28 @@ export class TripsService { // notes, accommodations) — mirrors the dateless-path trimming above (#1083). // Base must be max(targetDates.length, dated.length) to avoid colliding with // positives already assigned by the main loop or the overflow loop above. - const isEmptyDay = this.db.prepare( - `SELECT NOT EXISTS (SELECT 1 FROM day_assignments da WHERE da.day_id = @id) - AND NOT EXISTS (SELECT 1 FROM day_notes dn WHERE dn.day_id = @id) - AND NOT EXISTS (SELECT 1 FROM day_accommodations dac WHERE dac.start_day_id = @id OR dac.end_day_id = @id) AS empty` - ); const maxAssigned = Math.max(targetDates.length, dated.length); let keptDateless = 0; for (let i = datelessIdx; i < dateless.length; i++) { - const empty = (isEmptyDay.get({ id: dateless[i].id }) as { empty: number }).empty; + const empty = await this.daysRepo.isEmptyDay(dateless[i].id); // TP12 if (empty) { - del.run(dateless[i].id); + await this.daysRepo.deleteById(dateless[i].id); // TP11 } else { - setDayNumber.run(maxAssigned + keptDateless + 1, dateless[i].id); + await this.daysRepo.setDayNumber(dateless[i].id, maxAssigned + keptDateless + 1); // TP2 keptDateless++; } } // Final renumber to compact and eliminate any gaps/negatives - const remaining = this.db.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(tripId) as { id: number }[]; - renumber(remaining); + const remaining = await this.daysRepo.listOrderedForReorder(trip_id); // TP13 + await renumber(remaining); } // ── Trip CRUD ───────────────────────────────────────────────────────────── + /** TP16/TP17 — `TripsRepository.listForUser` (`TRIP_SELECT` + the access join, ONE builder, inventory §11a/§11c). */ async list(userId: number, archived: number | null) { - if (archived === null) { - return this.db.prepare(` - ${TRIP_SELECT} - LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId - WHERE (t.user_id = :userId OR m.user_id IS NOT NULL) - ORDER BY t.created_at DESC - `).all({ userId }); - } - return this.db.prepare(` - ${TRIP_SELECT} - LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId - WHERE (t.user_id = :userId OR m.user_id IS NOT NULL) AND t.is_archived = :archived - ORDER BY t.created_at DESC - `).all({ userId, archived }); + return this.tripsRepo.listForUser(userId, archived); } async create(userId: number, data: CreateTripData) { @@ -337,24 +337,25 @@ export class TripsService { ? (Number(data.reminder_days) >= 0 && Number(data.reminder_days) <= 30 ? Number(data.reminder_days) : 3) : 3; - const result = this.db.prepare(` - INSERT INTO trips (user_id, title, description, start_date, end_date, currency, reminder_days) - VALUES (?, ?, ?, ?, ?, ?, ?) - `).run(userId, data.title, data.description || null, data.start_date || null, data.end_date || null, data.currency || 'EUR', rd); + const tripId = await this.tripsRepo.insertTrip({ // TP18 + user_id: userId, + title: data.title, + description: data.description || null, + start_date: data.start_date || null, + end_date: data.end_date || null, + currency: data.currency || 'EUR', + reminder_days: rd, + }); - const tripId = result.lastInsertRowid; await this.generateDays(tripId, data.start_date || null, data.end_date || null, data.day_count); - const trip = this.db.prepare(`${TRIP_SELECT} WHERE t.id = :tripId`).get({ userId, tripId }); - return { trip, tripId: Number(tripId), reminderDays: rd }; + const trip = await this.tripsRepo.findForViewer(tripId, userId); // TP19 — the creator always owns it, so the access predicate is trivially satisfied + return { trip, tripId, reminderDays: rd }; } + /** TP20 — `TripsRepository.findForViewer`: `TRIP_SELECT` scoped to one trip AND the access predicate. */ async get(tripId: string | number, userId: number) { - return this.db.prepare(` - ${TRIP_SELECT} - LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId - WHERE t.id = :tripId AND (t.user_id = :userId OR m.user_id IS NOT NULL) - `).get({ userId, tripId }) as Trip | undefined; + return await this.tripsRepo.findForViewer(tripId, userId) as Trip | undefined; } /** @@ -368,34 +369,25 @@ export class TripsService { * a startup redirect, so it reads four columns of one row instead of every * trip with its per-trip day/place counts. */ + /** TP21 — `TripsRepository.activeTrip`: the triple `CASE WHEN … relevance` projection and the double-`CASE WHEN` `ORDER BY`. */ async activeTrip(userId: number, today = new Date().toISOString().slice(0, 10)) { - return this.db.prepare(` - SELECT t.id, t.title, t.start_date, t.end_date, - CASE - WHEN t.start_date IS NOT NULL AND t.end_date IS NOT NULL AND t.start_date <= :today AND t.end_date >= :today THEN 0 - WHEN t.start_date IS NOT NULL AND t.start_date >= :today THEN 1 - ELSE 2 - END AS relevance - FROM trips t - LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId - WHERE (t.user_id = :userId OR m.user_id IS NOT NULL) AND t.is_archived = 0 - ORDER BY relevance ASC, - CASE WHEN relevance < 2 THEN t.start_date END ASC, - CASE WHEN relevance = 2 THEN t.start_date END DESC - LIMIT 1 - `).get({ userId, today }) as ActiveTrip & { relevance: number } | undefined; + return await this.tripsRepo.activeTrip(userId, today) as ActiveTrip & { relevance: number } | undefined; } + /** TP22 — `TripsRepository.findRaw` (shared with `TripReadModelService.getTripSummary`, TR-B, per the inventory's own note that they are the identical statement). */ async getRaw(tripId: string | number): Promise { - return this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Trip | undefined; + const row = await this.tripsRepo.findRaw(tripId); + return (row ?? undefined) as Trip | undefined; } async searchCoverImages(query: string, userId: number) { return this.unsplash.searchUnsplashPhotos(query, 9, await this.unsplash.getUnsplashKey(userId)); } + /** TP23 — `TripsRepository.getOwnerId` (already existed, TB1), reshaped to the legacy `{user_id}` row shape (matches `TripReadModelService`'s own TR-A reshape). */ 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; + const ownerId = await this.tripsRepo.getOwnerId(tripId); + return ownerId === null ? undefined : { user_id: ownerId }; } /** @@ -403,8 +395,14 @@ export class TripsService { * through update() below; the plugin RPC host calls this directly (parity: * the legacy host path never rebased). */ + /** + * TP24–TP29 (inventory §11c) — TP25's `UPDATE trips` write stays BEFORE + * the `:423` days-regeneration transaction (R5/§18.6, unchanged): if + * `generateDays` throws, the transaction rolls back the day rows but the + * trip keeps its new dates. Flagged, not fixed, per the ruling. + */ async updateTrip(tripId: string | number, userId: number, data: UpdateTripData, userRole: string): Promise { - const trip = this.db.prepare('SELECT * FROM trips WHERE id = ?').get(tripId) as Trip & { reminder_days?: number } | undefined; + const trip = await this.tripsRepo.findRaw(tripId) as (Trip & { reminder_days?: number }) | null; // TP24 if (!trip) throw new NotFoundError('Trip not found'); const { title, description, currency, is_archived, cover_image, reminder_days } = data; @@ -420,11 +418,17 @@ export class TripsService { ? (Number(reminder_days) >= 0 && Number(reminder_days) <= 30 ? Number(reminder_days) : oldReminder) : oldReminder; - this.db.prepare(` - UPDATE trips SET title=?, description=?, start_date=?, end_date=?, - currency=?, is_archived=?, cover_image=?, reminder_days=?, updated_at=CURRENT_TIMESTAMP - WHERE id=? - `).run(newTitle, newDesc, newStart || null, newEnd || null, newCurrency, newArchived, newCover, newReminder, tripId); + const tripIdNum = Number(tripId); // safe: `trip` above only resolved through the raw-bind seam on a real row (Task 6 review's own ruling on this exact conversion) + await this.tripsRepo.updateTripRow(tripIdNum, { // TP25 + title: newTitle, + description: newDesc ?? null, + start_date: newStart || null, + end_date: newEnd || null, + currency: newCurrency, + is_archived: newArchived ?? 0, + cover_image: newCover ?? null, + reminder_days: newReminder, + }); if (trip.start_date && trip.end_date && newStart && newStart !== trip.start_date) await this.vacay.shiftOwnerEntriesForTripWindow(trip.user_id, trip.start_date, trip.end_date, newStart); @@ -433,18 +437,14 @@ export class TripsService { await this.uow.transactional(async () => { // Accommodations have no absolute date columns, so their pre-change dates must be // snapshotted before generateDays re-dates the day rows in place. - const prevDateByDayId = new Map( - (this.db.prepare('SELECT id, date FROM days WHERE trip_id = ?').all(tripId) as { id: number; date: string | null }[]) - .map(d => [d.id, d.date]), - ); + const prevDays = await this.daysRepo.listOrderedForReorder(tripIdNum); // TP26 + const prevDateByDayId = new Map(prevDays.map(d => [d.id, d.date])); await this.generateDays(tripId, newStart || null, newEnd || null, dayCount); if (data.date_shift_mode === 'shift_all') { // Explicit "shift everything": bookings stay glued to their (re-dated) day rows, // so re-stamp reservation_time to follow — same rules as reorderDays/insertDay. - const newDateByDayId = new Map( - (this.db.prepare('SELECT id, date FROM days WHERE trip_id = ?').all(tripId) as { id: number; date: string | null }[]) - .map(d => [d.id, d.date]), - ); + const newDays = await this.daysRepo.listOrderedForReorder(tripIdNum); // TP27 + const newDateByDayId = new Map(newDays.map(d => [d.id, d.date])); await this.days.restampReservationDates(tripId, prevDateByDayId, newDateByDayId); } else { // Default: generateDays re-dates day rows positionally; re-anchor dated bookings to @@ -466,10 +466,14 @@ export class TripsService { const isAdminEdit = userRole === 'admin' && trip.user_id !== userId; let ownerEmail: string | undefined; if (Object.keys(changes).length > 0 && isAdminEdit) { - ownerEmail = (this.db.prepare('SELECT email FROM users WHERE id = ?').get(trip.user_id) as { email: string } | undefined)?.email; + ownerEmail = (await this.em.getRepository(Users).getEmail(trip.user_id)) ?? undefined; // TP28 } - const updatedTrip = this.db.prepare(`${TRIP_SELECT} WHERE t.id = :tripId`).get({ userId, tripId }); + // TP29 — every caller of updateTrip has already passed an access check for + // this exact trip (canAccessTrip / requireTripEdit), so the access + // predicate `findForViewer` applies is always trivially satisfied here — + // same reasoning as `create`'s TP19 re-select above. + const updatedTrip = await this.tripsRepo.findForViewer(tripId, userId); return { updatedTrip, changes, isAdminEdit, ownerEmail, newTitle, newReminder, oldReminder }; } @@ -508,14 +512,19 @@ export class TripsService { // ── Delete ───────────────────────────────────────────────────────────────── + /** + * TP30–TP34 (inventory §11c) — TP32/TP33 (`journey_entries`) stay raw + * inside the transaction (`// Plan 3g`, that table's owning plan); every + * other statement moved through `TripsRepository`. + */ async remove(tripId: string | number, userId: number, userRole: string): Promise { - const trip = this.db.prepare('SELECT title, user_id FROM trips WHERE id = ?').get(tripId) as { title: string; user_id: number } | undefined; + const trip = await this.tripsRepo.findIdTitleOwner(tripId); // TP30 (the `id` field this method also selects is unused here) if (!trip) throw new NotFoundError('Trip not found'); const isAdminDelete = userRole === 'admin' && trip.user_id !== userId; let ownerEmail: string | undefined; if (isAdminDelete) { - ownerEmail = (this.db.prepare('SELECT email FROM users WHERE id = ?').get(trip.user_id) as { email: string } | undefined)?.email; + ownerEmail = (await this.em.getRepository(Users).getEmail(trip.user_id)) ?? undefined; // TP31 } // Quirk fix on top of the 1:1 move: the three-statement delete runs in a @@ -527,14 +536,14 @@ export class TripsService { this.db.prepare(` DELETE FROM journey_entries WHERE source_trip_id = ? AND type = 'skeleton' - `).run(tripId); + `).run(tripId); // TP32 — Plan 3g // Detach filled entries (keep user's written content, just remove trip link) this.db.prepare(` UPDATE journey_entries SET source_trip_id = NULL, source_place_id = NULL, source_assignment_id = NULL WHERE source_trip_id = ? - `).run(tripId); + `).run(tripId); // TP33 — Plan 3g - this.db.prepare('DELETE FROM trips WHERE id = ?').run(tripId); + await this.tripsRepo.deleteById(Number(tripId)); // TP34 — safe: `trip` above only resolved through the raw-bind seam on a real row }); return { tripId: Number(tripId), title: trip.title, ownerId: trip.user_id, isAdminDelete, ownerEmail }; @@ -553,8 +562,9 @@ export class TripsService { }); } + /** TP35 — `TripsRepository.setCoverImage`. */ async updateCoverImage(tripId: string | number, coverUrl: string): Promise { - this.db.prepare('UPDATE trips SET cover_image=?, updated_at=CURRENT_TIMESTAMP WHERE id=?').run(coverUrl, tripId); + await this.tripsRepo.setCoverImage(Number(tripId), coverUrl); // safe: only called after `getRaw`/`getOwner` matched the same id through the raw-bind seam } // ── Copy / duplicate ───────────────────────────────────────────────────── diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 0b75b3372..c616e24b1 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -166,7 +166,7 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< new DaysRpc(days, realtime, guards), new AccommodationsRpc(accommodations, realtime, guards), new ItineraryRpc(assignments, realtime, guards), - new TripsRpc(trips, reservations, days, membership, dbs, realtime, guards, accommodations, members), + new TripsRpc(trips, reservations, days, membership, realtime, guards, accommodations, members, (await sharedTestOrm(dbs.connection)).em), new CostsRpc(budget, dbs, realtime, guards, membership), new ReservationsRpc(reservations, realtime, guards), new CollabRpc(collab, realtime, guards), diff --git a/server/tests/unit/db/repositories/Days.repository.test.ts b/server/tests/unit/db/repositories/Days.repository.test.ts index 8add508a4..16a360085 100644 --- a/server/tests/unit/db/repositories/Days.repository.test.ts +++ b/server/tests/unit/db/repositories/Days.repository.test.ts @@ -2,7 +2,7 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vites import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; -import { createDay, createTrip, createUser } from '../../../helpers/factories'; +import { createDay, createDayAccommodation, createDayAssignment, createDayNote, createPlace, createTrip, createUser } from '../../../helpers/factories'; import { Days } from '../../../../src/db/entities/Days.entity'; import type { DaysRepository } from '../../../../src/db/repositories/Days.repository'; @@ -226,4 +226,73 @@ describe('DaysRepository — Task 2 additions', () => { expect(await days.existsInTrip('not-a-number', trip.id)).toBe(false); }); }); + + // ── Plan 3c Task 7 (TripsService.generateDays: TP3, TP6, TP12) ───────────── + + describe('clearDate (TP3)', () => { + it('DAYREPO-021: nullifies the date without deleting the row — assignments/notes/accommodations survive', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id, { date: '2026-01-01' }); + const place = createPlace(testDb, trip.id); + createDayAssignment(testDb, day.id, place.id); + await days.clearDate(day.id); + const row = testDb.prepare('SELECT date FROM days WHERE id = ?').get(day.id) as { date: string | null }; + expect(row.date).toBeNull(); + expect(testDb.prepare('SELECT id FROM day_assignments WHERE day_id = ?').get(day.id)).toBeDefined(); + }); + }); + + describe('isEmptyDay (TP12) — three counts across day_assignments/day_notes/day_accommodations', () => { + it('DAYREPO-022: true for a day with nothing on it; false once it holds an assignment, a note, or an accommodation (start OR end)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const empty = createDay(testDb, trip.id); + const withAssignment = createDay(testDb, trip.id); + const withNote = createDay(testDb, trip.id); + const withAccomStart = createDay(testDb, trip.id); + const withAccomEnd = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id); + createDayAssignment(testDb, withAssignment.id, place.id); + createDayNote(testDb, withNote.id, trip.id); + createDayAccommodation(testDb, trip.id, place.id, withAccomStart.id, withAccomStart.id); + createDayAccommodation(testDb, trip.id, place.id, withAccomEnd.id, withAccomEnd.id); + + expect(await days.isEmptyDay(empty.id)).toBe(true); + expect(await days.isEmptyDay(withAssignment.id)).toBe(false); + expect(await days.isEmptyDay(withNote.id)).toBe(false); + expect(await days.isEmptyDay(withAccomStart.id)).toBe(false); + expect(await days.isEmptyDay(withAccomEnd.id)).toBe(false); + }); + }); + + describe('listTrailingEmptyIds (TP6) — ORDER BY day_number DESC, first `limit` empty days', () => { + it('DAYREPO-023: a non-empty day in the middle is skipped, not a stopping point — matches the legacy WHERE-filter-then-LIMIT semantics', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const d1 = createDay(testDb, trip.id, { day_number: 1 }); // empty + const d2 = createDay(testDb, trip.id, { day_number: 2 }); // holds a note — excluded, not a stop + const d3 = createDay(testDb, trip.id, { day_number: 3 }); // empty + const d4 = createDay(testDb, trip.id, { day_number: 4 }); // empty + createDayNote(testDb, d2.id, trip.id); + + // limit=2: stops as soon as 2 empty days are collected, in day_number + // DESC order — d2 is never even reached. + expect(await days.listTrailingEmptyIds(trip.id, 2)).toEqual([d4.id, d3.id]); + + // limit=3: d4, d3 collected; d2 examined and SKIPPED (not empty, and + // not a stopping point either); d1 examined and collected — matching + // the legacy `WHERE NOT EXISTS(...) ×3 ORDER BY day_number DESC LIMIT + // 3` exactly (it filters every day up front, then takes the top 3 of + // what's left — d2 was never a candidate to begin with). + expect(await days.listTrailingEmptyIds(trip.id, 3)).toEqual([d4.id, d3.id, d1.id]); + }); + + it('DAYREPO-024: a limit of 0 returns nothing without querying', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + createDay(testDb, trip.id); + expect(await days.listTrailingEmptyIds(trip.id, 0)).toEqual([]); + }); + }); }); diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index 489e40791..7ec14c57b 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -466,7 +466,7 @@ describe('PlacesRepository.listForTrip (PL3) — filter fragments', () => { expect(rows.map((r) => r.id)).toEqual([newer.id, older.id]); }); - it('PLACEREPO-028: searchPattern matches name/address/description (already-escaped, wrapped by the caller)', async () => { + it('PLACEREPO-028: searchPattern matches name/address/description (already-escaped, wrapped by the caller — NOT a test of the ESCAPE clause itself: `%Eiffel%` has no literal `%`/`_` to escape. `ESCAPE \'\\\'` is proven by `PLACE-SVC-068`/PLACEREPO-028b below, which do carry a literal wildcard character)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const hit = createPlace(testDb, trip.id, { name: 'Eiffel Tower' }); @@ -476,6 +476,19 @@ describe('PlacesRepository.listForTrip (PL3) — filter fragments', () => { void miss; }); + it('PLACEREPO-028b (M2 — the ESCAPE clause this repository method actually relies on): a literal `%` in the already-escaped pattern matches literally, not as a wildcard', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // escapeLikePattern (the service) would have produced this pattern for a + // literal search term "50%": the `%` the user typed is backslash-escaped, + // and the wrapping `%…%` are the real wildcards this method adds. + const hit = createPlace(testDb, trip.id, { name: '50% off tour' }); + const miss = createPlace(testDb, trip.id, { name: '50X off tour' }); + const rows = await places.listForTrip(String(trip.id), { searchPattern: '%50\\%%' }); + expect(rows.map((r) => r.id)).toEqual([hit.id]); + void miss; + }); + it('PLACEREPO-029: category filters to that category_id only', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -533,6 +546,99 @@ describe('PlacesRepository.listForTrip (PL3) — filter fragments', () => { }); }); +/** + * PL3 parity (Task 4 review M2/rule 19): the base `94c6efbbc` raw statement + * (`places.service.ts:180-217` at that commit — reproduced verbatim below, + * the same `let query += …` composition, the same param order), run over + * every `search × category × tag × assignment` combination + * (2 × 2 × 2 × 4 = 32, undefined/present for the first three, {undefined, + * 'all', 'assigned', 'unassigned'} for the fourth). Each cell asserts + * `toEqual` on the FULL row set from both statements, not just the id list — + * a dropped/renamed column would pass an id-only comparison silently. + */ +describe('PlacesRepository.listForTrip (PL3) — toEqual(legacy) over all 32 filter combinations', () => { + function legacyListForTrip( + tripId: string, + filters: { searchPattern?: string; category?: string; tag?: string; assignment?: 'all' | 'unassigned' | 'assigned' }, + ): unknown[] { + let query = ` + SELECT DISTINCT p.*, c.name as category_name, c.color as category_color, c.icon as category_icon + FROM places p + LEFT JOIN categories c ON p.category_id = c.id + WHERE p.trip_id = ? + `; + const params: (string | number)[] = [tripId]; + + if (filters.searchPattern) { + query += " AND (p.name LIKE ? ESCAPE '\\' OR p.address LIKE ? ESCAPE '\\' OR p.description LIKE ? ESCAPE '\\')"; + params.push(filters.searchPattern, filters.searchPattern, filters.searchPattern); + } + if (filters.category) { + query += ' AND p.category_id = ?'; + params.push(filters.category); + } + if (filters.tag) { + query += ' AND p.id IN (SELECT place_id FROM place_tags WHERE tag_id = ?)'; + params.push(filters.tag); + } + if (filters.assignment === 'unassigned') { + query += ' AND p.id NOT IN (SELECT da.place_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE d.trip_id = ?)'; + params.push(tripId); + } else if (filters.assignment === 'assigned') { + query += ' AND p.id IN (SELECT da.place_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE d.trip_id = ?)'; + params.push(tripId); + } + query += ' ORDER BY p.created_at DESC'; + return testDb.prepare(query).all(...params); + } + + it('PLACEREPO-032b (PL3, M2): every row, every column, on all 32 combinations, byte-identical to the legacy raw statement', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const cat = createCategory(testDb, { name: 'Museums' }); + const tag = createTag(testDb, user.id, { name: 'Must see' }); + const day = createDay(testDb, trip.id); + + // p1: category + tag + assigned + matches the search term. + const p1 = createPlace(testDb, trip.id, { name: 'Central Park', category_id: cat.id }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(p1.id, tag.id); + createDayAssignment(testDb, day.id, p1.id); + // p2: category, no tag, unassigned, matches the search term (a near-miss on 3 of 4 axes). + createPlace(testDb, trip.id, { name: 'Central Station', category_id: cat.id }); + // p3: no category, tag + assigned, does NOT match the search term. + const p3 = createPlace(testDb, trip.id, { name: 'Louvre' }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(p3.id, tag.id); + createDayAssignment(testDb, day.id, p3.id); + // p4: no category, no tag, unassigned, does NOT match the search term. + createPlace(testDb, trip.id, { name: 'Eiffel Tower' }); + // Another trip's place, named to collide with the search term — must + // never appear in either statement's output (both scope by trip_id). + createPlace(testDb, otherTrip.id, { name: 'Central Perk' }); + + const searchOptions: (string | undefined)[] = [undefined, '%Central%']; + const categoryOptions: (string | undefined)[] = [undefined, String(cat.id)]; + const tagOptions: (string | undefined)[] = [undefined, String(tag.id)]; + const assignmentOptions: (('all' | 'unassigned' | 'assigned') | undefined)[] = [undefined, 'all', 'assigned', 'unassigned']; + + let combinations = 0; + for (const searchPattern of searchOptions) { + for (const category of categoryOptions) { + for (const tag_ of tagOptions) { + for (const assignment of assignmentOptions) { + combinations++; + const filters = { searchPattern, category, tag: tag_, assignment }; + const actual = await places.listForTrip(String(trip.id), filters); + const legacy = legacyListForTrip(String(trip.id), filters); + expect(actual).toEqual(legacy); + } + } + } + } + expect(combinations).toBe(32); + }); +}); + // --------------------------------------------------------------------------- // Plan 3c Task 5 — the four importers' `insertPlace` column sets, the // colorize read+write transaction, the ftid backfill, and the @@ -793,3 +899,29 @@ describe('PlacesRepository.fillIfEmpty (PL43/PL44/PL46) — three call shapes ov expect((testDb.prepare('SELECT address FROM places WHERE id = ?').get(place.id) as { address: string | null }).address).toBeNull(); }); }); + +// ── Plan 3c Task 7 (trips.rpc.ts::getPlaces, RP2) — additive ──────────────── + +describe('PlacesRepository.listForTripOrdered (RP2)', () => { + it('PLACEREPO-046: every place of the trip, ORDER BY created_at DESC, scoped to the trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const p1 = createPlace(testDb, trip.id, { name: 'First' }); + testDb.prepare('UPDATE places SET created_at = ? WHERE id = ?').run('2026-01-01 00:00:00', p1.id); + const p2 = createPlace(testDb, trip.id, { name: 'Second' }); + testDb.prepare('UPDATE places SET created_at = ? WHERE id = ?').run('2026-02-01 00:00:00', p2.id); + createPlace(testDb, otherTrip.id, { name: 'Elsewhere' }); + + const legacy = testDb.prepare('SELECT * FROM places WHERE trip_id = ? ORDER BY created_at DESC').all(trip.id); + const rows = await places.listForTripOrdered(trip.id); + expect(rows).toEqual(legacy); + expect(rows.map((r) => r.id)).toEqual([p2.id, p1.id]); + }); + + it('PLACEREPO-047: an empty trip returns an empty array, not a throw', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect(await places.listForTripOrdered(trip.id)).toEqual([]); + }); +}); diff --git a/server/tests/unit/db/repositories/TripMembers.repository.test.ts b/server/tests/unit/db/repositories/TripMembers.repository.test.ts index 6b3092b0f..d32c415bd 100644 --- a/server/tests/unit/db/repositories/TripMembers.repository.test.ts +++ b/server/tests/unit/db/repositories/TripMembers.repository.test.ts @@ -340,3 +340,41 @@ describe('NaN user_id — rule 15 (a non-numeric route id parses to NaN, must no await expect(tripMembers.isGuestOfTrip(trip.id, NaN)).resolves.toBe(false); }); }); + +// ── Plan 3c Task 7 (trips.rpc.ts::members, RP3) — additive ────────────────── + +describe('TripMembersRepository.listRawUsernameAndDisplayName (RP3)', () => { + it('TMEMREPO-026: byte-identical to the legacy statement — raw username AND display_name, NOT TM2\'s COALESCE', async () => { + const { user: owner } = createUser(testDb); + const { user: named } = createUser(testDb, { username: 'bare-name' }); + testDb.prepare('UPDATE users SET display_name = ?, avatar = ? WHERE id = ?').run('Displayed', 'a.png', named.id); + const { user: bare } = createUser(testDb, { username: 'no-display' }); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, named.id); + addTripMember(testDb, trip.id, bare.id); + + const legacy = testDb.prepare( + 'SELECT u.id, u.username, u.display_name, u.avatar FROM trip_members tm JOIN users u ON u.id = tm.user_id WHERE tm.trip_id = ?', + ).all(trip.id); + const rows = await tripMembers.listRawUsernameAndDisplayName(trip.id); + expect(rows).toEqual(legacy); + const byId = new Map(rows.map((r) => [r.id, r])); + // Raw username, not COALESCEd with display_name — TM2's shape is different on purpose (§18.10). + expect(byId.get(named.id)).toEqual({ id: named.id, username: 'bare-name', display_name: 'Displayed', avatar: 'a.png' }); + expect(byId.get(bare.id)).toEqual({ id: bare.id, username: 'no-display', display_name: null, avatar: null }); + }); + + it('TMEMREPO-027: an owner-only trip (no trip_members rows) returns an empty array', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + expect(await tripMembers.listRawUsernameAndDisplayName(trip.id)).toEqual([]); + }); + + it('TMEMREPO-028: raw-bind — a string trip id binds unconverted, same as a real number', 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.listRawUsernameAndDisplayName(String(trip.id))).map((r) => r.id)).toEqual([member.id]); + }); +}); diff --git a/server/tests/unit/db/repositories/Trips.repository.test.ts b/server/tests/unit/db/repositories/Trips.repository.test.ts index c132640d6..c2dc30165 100644 --- a/server/tests/unit/db/repositories/Trips.repository.test.ts +++ b/server/tests/unit/db/repositories/Trips.repository.test.ts @@ -2,7 +2,7 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { CAN_ACCESS_TRIP_SQL, resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; -import { addTripMember, createTrip, createUser } from '../../../helpers/factories'; +import { addTripMember, createDay, createPlace, createTrip, createUser } from '../../../helpers/factories'; import { Trips } from '../../../../src/db/entities/Trips.entity'; import type { TripsRepository } from '../../../../src/db/repositories/Trips.repository'; @@ -235,3 +235,251 @@ describe('TripsRepository — TripMembersService / TripReadModelService (Plan 3c }); }); }); + +// ── Plan 3c Task 7 — TRIP_SELECT's ONE builder, generateDays helpers, CRUD ─── +// +// `TRIP_SELECT` (trips.service.ts): the shared projection every one of +// `findForViewer`/`listForUser`/`activeTrip` builds on (`tripSelectQuery`). +// Byte-diffed here against the legacy statement run raw, on a FULLY seeded +// trip (days, places, a second member, an owner) with every nullable column +// both null and set (rule 19). +const LEGACY_TRIP_SELECT = ` + SELECT t.*, + NULL AS feed_token, + (SELECT COUNT(*) FROM days d WHERE d.trip_id = t.id) as day_count, + (SELECT COUNT(*) FROM places p WHERE p.trip_id = t.id) as place_count, + CASE WHEN t.user_id = :userId THEN 1 ELSE 0 END as is_owner, + u.username as owner_username, + (SELECT COUNT(*) FROM trip_members tm WHERE tm.trip_id = t.id) as shared_count + FROM trips t + JOIN users u ON u.id = t.user_id +`; + +describe('TripsRepository.findForViewer / listForUser / activeTrip (Plan 3c Task 7, TRIP_SELECT)', () => { + describe('findForViewer (TP20, also serves TP19/TP29 and TripMembersService.getTripForViewer)', () => { + it('TRIPREPO-023: byte-identical to the legacy TRIP_SELECT statement on a FULLY seeded trip (days, places, a member, feed_token set in the DB)', async () => { + const { user: owner } = createUser(testDb, { username: 'owner-handle' }); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { + title: 'Full Row', description: 'A real description', start_date: '2026-03-01', end_date: '2026-03-05', + }); + testDb.prepare('UPDATE trips SET feed_token = ?, cover_image = ? WHERE id = ?').run('super-secret', 'cover.png', trip.id); + addTripMember(testDb, trip.id, member.id); + createDay(testDb, trip.id, { date: '2026-03-01' }); + createDay(testDb, trip.id, { date: '2026-03-02' }); + createPlace(testDb, trip.id); + createPlace(testDb, trip.id); + createPlace(testDb, trip.id); + + const legacyOwner = testDb.prepare(` + ${LEGACY_TRIP_SELECT} + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId + WHERE t.id = :tripId AND (t.user_id = :userId OR m.user_id IS NOT NULL) + `) + .get({ userId: owner.id, tripId: trip.id }); + const row = await trips.findForViewer(trip.id, owner.id); + expect(row).toEqual(legacyOwner); + // createTrip's own start_date/end_date span already generates 5 days; + // the two explicit createDay calls above add 2 more, for 7 total. + expect(row).toMatchObject({ + day_count: 7, place_count: 3, shared_count: 1, is_owner: 1, owner_username: 'owner-handle', feed_token: null, + }); + // The real column is set in the DB — the wire value is blanked by the + // SQL-side `NULL AS feed_token` trick, not merely absent from a + // hand-picked column list (§18.5). + expect((testDb.prepare('SELECT feed_token FROM trips WHERE id = ?').get(trip.id) as { feed_token: string }).feed_token).toBe('super-secret'); + + const legacyMember = testDb.prepare(` + ${LEGACY_TRIP_SELECT} + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId + WHERE t.id = :tripId AND (t.user_id = :userId OR m.user_id IS NOT NULL) + `) + .get({ userId: member.id, tripId: trip.id }); + const memberRow = await trips.findForViewer(trip.id, member.id); + expect(memberRow).toEqual(legacyMember); + expect(memberRow).toMatchObject({ is_owner: 0, day_count: 7, place_count: 3, shared_count: 1 }); + }); + + it('TRIPREPO-024: NULL-seeded nullable columns come back null on the wire; a stranger and a nonexistent trip both get undefined', async () => { + const { user: owner } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); // description/start_date/end_date/cover_image all NULL by default + + const legacy = testDb.prepare(` + ${LEGACY_TRIP_SELECT} + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId + WHERE t.id = :tripId AND (t.user_id = :userId OR m.user_id IS NOT NULL) + `) + .get({ userId: owner.id, tripId: trip.id }); + const row = await trips.findForViewer(trip.id, owner.id); + expect(row).toEqual(legacy); + expect(row).toMatchObject({ description: null, start_date: null, end_date: null, cover_image: null, feed_token: null }); + + expect(await trips.findForViewer(trip.id, stranger.id)).toBeUndefined(); + expect(await trips.findForViewer(999999, owner.id)).toBeUndefined(); + }); + + it('TRIPREPO-025: binds a string trip id raw, the seam every other method here preserves', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { title: 'Stringy' }); + expect((await trips.findForViewer(String(trip.id), owner.id))?.title).toBe('Stringy'); + }); + + // Rule 21: `findForViewer` is a Kysely read, which never consults the + // identity map — the "fresh after a raw UPDATE" shape applies instead of + // a D-shape identity-map test. + it('TRIPREPO-026 (fresh after a raw UPDATE): a title changed outside the ORM is visible on the next read', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { title: 'Stale' }); + expect((await trips.findForViewer(trip.id, owner.id))?.title).toBe('Stale'); + testDb.prepare('UPDATE trips SET title = ? WHERE id = ?').run('Fresh', trip.id); + expect((await trips.findForViewer(trip.id, owner.id))?.title).toBe('Fresh'); + }); + }); + + describe('listForUser (TP16/TP17)', () => { + it('TRIPREPO-027: byte-identical to the legacy statement, archived === null (TP16, no filter)', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const owned = createTrip(testDb, owner.id, { title: 'Owned' }); + const shared = createTrip(testDb, member.id, { title: 'Shared' }); + addTripMember(testDb, shared.id, owner.id); + const archived = createTrip(testDb, owner.id, { title: 'Archived' }); + testDb.prepare('UPDATE trips SET is_archived = 1 WHERE id = ?').run(archived.id); + + const legacy = testDb.prepare(` + ${LEGACY_TRIP_SELECT} + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId + WHERE (t.user_id = :userId OR m.user_id IS NOT NULL) + ORDER BY t.created_at DESC + `).all({ userId: owner.id }); + const rows = await trips.listForUser(owner.id, null); + expect(rows).toEqual(legacy); + expect(rows.map((r) => r.id).sort()).toEqual([owned.id, shared.id, archived.id].sort()); + }); + + it('TRIPREPO-028: byte-identical to the legacy statement, archived filtered (TP17)', async () => { + const { user: owner } = createUser(testDb); + const active = createTrip(testDb, owner.id, { title: 'Active' }); + const archived = createTrip(testDb, owner.id, { title: 'Archived' }); + testDb.prepare('UPDATE trips SET is_archived = 1 WHERE id = ?').run(archived.id); + + const legacyArchived = testDb.prepare(` + ${LEGACY_TRIP_SELECT} + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId + WHERE (t.user_id = :userId OR m.user_id IS NOT NULL) AND t.is_archived = :archived + ORDER BY t.created_at DESC + `).all({ userId: owner.id, archived: 1 }); + expect(await trips.listForUser(owner.id, 1)).toEqual(legacyArchived); + expect((await trips.listForUser(owner.id, 1)).map((r) => r.id)).toEqual([archived.id]); + expect((await trips.listForUser(owner.id, 0)).map((r) => r.id)).toEqual([active.id]); + }); + }); + + describe('activeTrip (TP21) — the triple CASE WHEN relevance projection and the double-CASE WHEN ORDER BY', () => { + const LEGACY_ACTIVE_TRIP = ` + SELECT t.id, t.title, t.start_date, t.end_date, + CASE + WHEN t.start_date IS NOT NULL AND t.end_date IS NOT NULL AND t.start_date <= :today AND t.end_date >= :today THEN 0 + WHEN t.start_date IS NOT NULL AND t.start_date >= :today THEN 1 + ELSE 2 + END AS relevance + FROM trips t + LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = :userId + WHERE (t.user_id = :userId OR m.user_id IS NOT NULL) AND t.is_archived = 0 + ORDER BY relevance ASC, + CASE WHEN relevance < 2 THEN t.start_date END ASC, + CASE WHEN relevance = 2 THEN t.start_date END DESC + LIMIT 1 + `; + + it('TRIPREPO-029: a running-today trip (relevance 0) wins over an upcoming or past one, matching the legacy statement result AND ordering', async () => { + const { user } = createUser(testDb); + const today = '2026-06-10'; + createTrip(testDb, user.id, { title: 'Past', start_date: '2026-01-01', end_date: '2026-01-05' }); + const running = createTrip(testDb, user.id, { title: 'Running', start_date: '2026-06-08', end_date: '2026-06-12' }); + createTrip(testDb, user.id, { title: 'Future', start_date: '2026-07-01', end_date: '2026-07-05' }); + + const legacy = testDb.prepare(LEGACY_ACTIVE_TRIP).get({ userId: user.id, today }); + const row = await trips.activeTrip(user.id, today); + expect(row).toEqual(legacy); + expect(row).toMatchObject({ id: running.id, title: 'Running', relevance: 0 }); + }); + + it('TRIPREPO-030: no running trip — the next upcoming one wins, earliest first (relevance 1)', async () => { + const { user } = createUser(testDb); + const today = '2026-06-10'; + const soonest = createTrip(testDb, user.id, { title: 'Soonest', start_date: '2026-07-01', end_date: '2026-07-05' }); + createTrip(testDb, user.id, { title: 'Later', start_date: '2026-08-01', end_date: '2026-08-05' }); + + const legacy = testDb.prepare(LEGACY_ACTIVE_TRIP).get({ userId: user.id, today }); + const row = await trips.activeTrip(user.id, today); + expect(row).toEqual(legacy); + expect(row).toMatchObject({ id: soonest.id, relevance: 1 }); + }); + + it('TRIPREPO-031: nothing running or upcoming — the most recently started trip wins (relevance 2), archived trips excluded', async () => { + const { user } = createUser(testDb); + const today = '2026-06-10'; + createTrip(testDb, user.id, { title: 'Long ago', start_date: '2026-01-01', end_date: '2026-01-05' }); + const recent = createTrip(testDb, user.id, { title: 'Recent', start_date: '2026-05-01', end_date: '2026-05-05' }); + const archived = createTrip(testDb, user.id, { title: 'Archived-but-recent', start_date: '2026-06-01', end_date: '2026-06-05' }); + testDb.prepare('UPDATE trips SET is_archived = 1 WHERE id = ?').run(archived.id); + + const legacy = testDb.prepare(LEGACY_ACTIVE_TRIP).get({ userId: user.id, today }); + const row = await trips.activeTrip(user.id, today); + expect(row).toEqual(legacy); + expect(row).toMatchObject({ id: recent.id, relevance: 2 }); + }); + + it('TRIPREPO-032: nothing at all returns undefined', async () => { + const { user } = createUser(testDb); + expect(await trips.activeTrip(user.id, '2026-06-10')).toBeUndefined(); + }); + }); +}); + +describe('TripsRepository.insertTrip / updateTripRow / setCoverImage / deleteById (Plan 3c Task 7)', () => { + it('TRIPREPO-033: insertTrip (TP18) writes the given column set verbatim and returns the generated id', async () => { + const { user } = createUser(testDb); + const id = await trips.insertTrip({ + user_id: user.id, title: 'New Trip', description: null, start_date: null, end_date: null, currency: 'EUR', reminder_days: 3, + }); + const row = testDb.prepare('SELECT * FROM trips WHERE id = ?').get(id) as Record; + expect(row).toMatchObject({ user_id: user.id, title: 'New Trip', currency: 'EUR', reminder_days: 3 }); + }); + + it('TRIPREPO-034: updateTripRow (TP25) writes every column and stamps updated_at', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Before' }); + await trips.updateTripRow(trip.id, { + title: 'After', description: 'd', start_date: '2026-01-01', end_date: '2026-01-05', + currency: 'USD', is_archived: 1, cover_image: 'c.png', reminder_days: 7, + }); + const row = testDb.prepare('SELECT * FROM trips WHERE id = ?').get(trip.id) as Record; + expect(row).toMatchObject({ + title: 'After', description: 'd', start_date: '2026-01-01', end_date: '2026-01-05', + currency: 'USD', is_archived: 1, cover_image: 'c.png', reminder_days: 7, + }); + // CURRENT_TIMESTAMP has one-second resolution — a same-second before/after + // comparison is flaky, not a real assertion; the column being non-null and + // in the CURRENT_TIMESTAMP text shape is what `updateTripRow` promises. + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); + }); + + it('TRIPREPO-035: setCoverImage (TP35) writes cover_image and stamps updated_at', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await trips.setCoverImage(trip.id, '/uploads/covers/x.jpg'); + expect((testDb.prepare('SELECT cover_image FROM trips WHERE id = ?').get(trip.id) as { cover_image: string }).cover_image).toBe('/uploads/covers/x.jpg'); + }); + + it('TRIPREPO-036: deleteById (TP34, security-sensitive) removes exactly the given trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const other = createTrip(testDb, user.id); + await trips.deleteById(trip.id); + expect(testDb.prepare('SELECT id FROM trips WHERE id = ?').get(trip.id)).toBeUndefined(); + expect(testDb.prepare('SELECT id FROM trips WHERE id = ?').get(other.id)).toBeDefined(); + }); +}); diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index 69b232595..ba8c74570 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -836,6 +836,17 @@ describe('UsersRepository — TripMembersService (Plan 3c Task 6)', () => { it('USERSREPO-065: no match returns null, not a throw', async () => { expect(await users.findInvitableByEmailOrUsername('nobody@example.test')).toBeNull(); }); + + // Task 6 review, L3: `findInvitableByEmailOrUsername` is `findOne`-based + // (unlike the qb-based projections above), so rule 21's D-shape test + // applies for real — this is the guest-exclusion guard (#1362), so + // staleness here would re-open the bug the guard exists to close. + it('USERSREPO-072 (D-shape): an is_guest flip written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + const { user } = createUser(testDb, { email: 'flip@example.test', username: 'flip-handle' }); + await t.repo(Users).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findInvitableByEmailOrUsername('flip@example.test')).toBeNull(); + }); }); describe('findIdEmailGuest (TM10)', () => { diff --git a/server/tests/unit/nest/trip-members.controller.test.ts b/server/tests/unit/nest/trip-members.controller.test.ts index 348ca51d6..5a4f93e4e 100644 --- a/server/tests/unit/nest/trip-members.controller.test.ts +++ b/server/tests/unit/nest/trip-members.controller.test.ts @@ -63,6 +63,10 @@ describe('members', () => { const s = svc({ addMember, notifyInvite } as Partial); expect(await tc(s).addMember(user, '9', { identifier: 'bob@x.y' })).toEqual({ member: { id: 2, email: 'bob@x.y' } }); expect(notifyInvite).toHaveBeenCalledWith('9', user, 2, 'T', 'bob@x.y'); + // Task 6 review, L1: the pre-existing REST↔MCP asymmetry (§13b) — these five + // controller mutations broadcast NOTHING while the MCP twins all emit + // member:added/member:removed. Pinned here rather than "fixed". + expect(s.broadcast).not.toHaveBeenCalled(); }); it('POST 404 without trip access', async () => { @@ -90,6 +94,8 @@ describe('members', () => { // self-removal (targetId === user.id) bypasses the permission check expect(await tc(s).removeMember(user, '9', '1')).toEqual({ success: true }); expect(await thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 403, body: { error: 'No permission to remove members' } }); + // Task 6 review, L1: see the addMember case above. + expect(s.broadcast).not.toHaveBeenCalled(); }); }); @@ -136,6 +142,8 @@ describe('guests (#1362)', () => { const s = svc({ createGuest } as Partial); expect(await tc(s).createGuest(user, '9', { name: 'Anna' })).toEqual({ member: { id: 7, username: 'Anna', is_guest: true } }); expect(createGuest).toHaveBeenCalledWith('9', 'Anna', user.id); + // Task 6 review, L1: see the addMember case above. + expect(s.broadcast).not.toHaveBeenCalled(); }); it('rename: 404 when the guest is missing, else success', async () => { @@ -143,6 +151,8 @@ describe('guests (#1362)', () => { expect(await thrown(() => tc(miss).renameGuest(user, '9', '7', { name: 'Bob' }))).toEqual({ status: 404, body: { error: 'Guest not found' } }); const ok = svc({ renameGuest: vi.fn().mockReturnValue(true) } as Partial); expect(await tc(ok).renameGuest(user, '9', '7', { name: 'Bob' })).toEqual({ success: true }); + // Task 6 review, L1: see the addMember case above. + expect(ok.broadcast).not.toHaveBeenCalled(); }); it('delete: 404 when the guest is missing, else success', async () => { @@ -150,6 +160,8 @@ describe('guests (#1362)', () => { expect(await thrown(() => tc(miss).deleteGuest(user, '9', '7'))).toEqual({ status: 404, body: { error: 'Guest not found' } }); const ok = svc({ deleteGuest: vi.fn().mockResolvedValue(true) } as Partial); expect(await tc(ok).deleteGuest(user, '9', '7')).toEqual({ success: true }); + // Task 6 review, L1: see the addMember case above. + expect(ok.broadcast).not.toHaveBeenCalled(); }); it('maps a ValidationError from createGuest to 400', async () => { diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts index 81e9612c2..87bd6f182 100644 --- a/server/tests/unit/nest/trip-members.service.test.ts +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -1,5 +1,7 @@ /** - * Unit tests for TripMembersService — MEMBERS-SVC-001 through MEMBERS-SVC-015. + * Unit tests for TripMembersService — MEMBERS-SVC-001 through MEMBERS-SVC-018 + * (016-018 added by the Task 6 review's M1/L2 items: transferOwnership/ + * createGuest transaction-rollback proofs and an addMember concurrency case). * * The roster cases that came over with the split still run in * tests/unit/nest/trips.service.test.ts (TRIP-SVC-020…023, 030…034, 049, 052, @@ -69,6 +71,7 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, c 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'; +import type { TripMembersRepository } from '../../../src/db/repositories/TripMembers.repository'; // Plan 3c Task 0b: `dbsEm` is resolved once, at the top of the `beforeAll` // below, before any `dbs()` call — `canAccessTrip`/`isOwner`/`rosterUserIds`/ @@ -83,17 +86,24 @@ let budgetSvc: BudgetService; let roster: TripMembersService; let tripsRepo: TripsRepository; let usersRepo: UsersRepository; +// Task 6 review, M1: hoisted (was constructed inline) so the mutation-rollback +// tests below can `vi.spyOn` the SAME instance the service holds — the shared +// EM caches repositories, so a fresh `createTestTripMembersRepo(testDb)` call +// returns this identical object (Task 6 review's own confirmation for `tripsRepo`/ +// `usersRepo`'s existing spies applies here too). +let tripMembersRepo: TripMembersRepository; beforeAll(async () => { dbsEm = (await sharedTestOrm(testDb)).em; tripsRepo = await createTestTripsRepo(testDb); usersRepo = await createTestUsersRepo(testDb); + tripMembersRepo = await createTestTripMembersRepo(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), usersRepo), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(notifySend), await createTestUnitOfWork(dbs().connection), - tripsRepo, await createTestTripMembersRepo(testDb), usersRepo, + tripsRepo, tripMembersRepo, usersRepo, ); }); @@ -160,9 +170,9 @@ describe('TripMembersService delegation', () => { // The handover broadcast hands the raw :id route param straight in, so the // named-parameter query has to keep matching a string id against the INTEGER // column — and the payload the clients re-read must carry their own is_owner. - const asOwner = await roster.getTripForViewer(String(trip.id), owner.id) as Record; + const asOwner = await roster.getTripForViewer(String(trip.id), owner.id) as unknown as Record; expect(asOwner).toMatchObject({ id: trip.id, title: 'Handover', is_owner: 1, owner_username: owner.username, shared_count: 1 }); - const asMember = await roster.getTripForViewer(trip.id, member.id) as Record; + const asMember = await roster.getTripForViewer(trip.id, member.id) as unknown as Record; expect(asMember.is_owner).toBe(0); expect(await roster.getTripForViewer(999999, owner.id)).toBeUndefined(); }); @@ -375,3 +385,77 @@ describe('listMembers shaping', () => { expect(ownerRow.is_guest).toBe(false); }); }); + +// ── Task 6 review items (M1, L2) ───────────────────────────────────────────── +// +// task-6-review.md M1: neither `transferOwnership` nor `createGuest` had a +// test proving their `uow.transactional` block actually rolls back when its +// LAST statement fails — a mutation moving `addIgnoringConflict`/`addMember` +// outside the transaction survived the whole suite. Shaped like TRIP-SVC-052 +// (`vi.spyOn` the repository the shared EM caches, not a hand-rolled fault +// injection). L2: `addMember`'s TM5→TM6 check-then-act (§18.4) had no +// concurrency test of its own — `joinTripAsMember`'s TRIP-JOIN-005 covers the +// identical shape one method over; this copies it. + +describe('Task 6 review items — rollback and concurrency', () => { + it('MEMBERS-SVC-016 (mutation-proved): transferOwnership rolls back when the final INSERT rejects — the owner pointer and the new owner\'s membership both survive', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + await roster.addMember(trip.id, member.email, owner.id, owner.id); + + const spy = vi.spyOn(tripMembersRepo, 'addIgnoringConflict').mockRejectedValueOnce(new Error('boom')); + try { + await expect(roster.transferOwnership(trip.id, member.id, owner.id)).rejects.toThrow('boom'); + } finally { + spy.mockRestore(); + } + + // setOwner (TM13) ran, then remove (TM14) ran, then addIgnoringConflict (TM15) + // rejected — a fix that moved TM15 outside the transaction would leave TM13/TM14 + // committed while this assertion still expects them rolled back. + const tripRow = testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }; + expect(tripRow.user_id).toBe(owner.id); + expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeDefined(); + }); + + it('MEMBERS-SVC-017 (mutation-proved): createGuest rolls back when the membership INSERT rejects — no orphan guest user row', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const before = (testDb.prepare('SELECT COUNT(*) AS n FROM users').get() as { n: number }).n; + + const spy = vi.spyOn(tripMembersRepo, 'addMember').mockRejectedValueOnce(new Error('boom')); + try { + await expect(roster.createGuest(trip.id, 'Rollback Rae', owner.id)).rejects.toThrow('boom'); + } finally { + spy.mockRestore(); + } + + // insertGuest (TM16) ran, then addMember (TM17) rejected — a fix that moved + // TM17 outside the transaction would leave the guest's `users` row behind. + const after = (testDb.prepare('SELECT COUNT(*) AS n FROM users').get() as { n: number }).n; + expect(after).toBe(before); + }); + + it('MEMBERS-SVC-018 (concurrency, §18.4, copies TRIP-JOIN-005): two concurrent addMember calls for the same (trip, user) race the TM5→TM6 check-then-act window', async () => { + const { user: owner } = createUser(testDb); + const { user: invitee } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + const results = await Promise.allSettled([ + roster.addMember(trip.id, invitee.email, owner.id, owner.id), + roster.addMember(trip.id, invitee.email, owner.id, owner.id), + ]); + // Today's actual outcome, run under real concurrency, not assumed: the + // method performs no locking/serialization of its own (TM5 `exists` → + // TM6 `addMember`, unchanged by this conversion, §18.4). Whichever call + // loses the race either throws the app-level 'User already has access' + // (TM5 caught it) or rejects on the table's own UNIQUE(trip_id, user_id) + // constraint (TM6 raced past TM5) — both are acceptable, unchanged + // outcomes; what must hold is that at most one membership row exists. + const fulfilled = results.filter((r) => r.status === 'fulfilled'); + expect(fulfilled.length).toBeGreaterThanOrEqual(1); + const count = testDb.prepare('SELECT COUNT(*) as n FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, invitee.id) as { n: number }; + expect(count.n).toBe(1); + }); +}); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 3d341bc35..798a1039b 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -1224,41 +1224,18 @@ describe('folded quirk branches', () => { // ── Post-fold quirk fixes (transactions, owner display name) ───────────────── describe('quirk fixes', () => { - /** A TripsService whose connection throws when preparing SQL matching `match`. */ - function failingConnection(match: string) { - const conn = new Proxy(testDb, { - get(target, prop) { - if (prop === 'prepare') { - return (sql: string) => { - if (sql.includes(match)) throw new Error('boom'); - return target.prepare(sql); - }; - } - const v = (target as any)[prop]; - return typeof v === 'function' ? v.bind(target) : v; - }, - }); - return { connection: conn, canAccessTrip: dbMock.canAccessTrip, isOwner: dbMock.isOwner } as unknown as import('../../../src/nest/database/database.service').DatabaseService; - } + // Plan 3c Task 7 (R8): the SQL-text-keyed `failingConnection`/`failingTrips` + // Proxy this test used to build a one-off TripsService around is rewritten + // as a repository-level fault, the same shape TRIP-SVC-052 below already + // uses for `deleteGuest` — `remove`'s TP32-TP34 (journey_entries stay raw; + // `deleteById` is TP34, `TripsRepository`'s) are the statements this test + // pins, and TP34 is now a repository method to `vi.spyOn`, not SQL text to + // pattern-match. The Proxy could never trigger any more: no statement text + // matching `'DELETE FROM trips WHERE id = ?'` runs through `this.db` + // inside `remove`'s transaction any more (TP32/TP33 are the only survivors + // there, and their own text is untouched). - async function failingTrips(match: string) { - const fdbs = failingConnection(match); - return new TripsService( - fdbs, - 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)), - daysSvc, - new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), - budgetSvc, - new VacayService(dbs(), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection)), - new RealtimeService(), - undefined as never, - coversFx.storage, - await createTestUnitOfWork(dbs().connection), - (await sharedTestOrm(testDb)).em, - ); - } - - it('TRIP-SVC-051: remove is atomic — a failed trip DELETE keeps the journey entries intact', async () => { + it('TRIP-SVC-051 (mutation-proved): remove is atomic — a failed trip DELETE keeps the journey entries intact', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const journeyId = Number(testDb.prepare( @@ -1268,8 +1245,13 @@ describe('quirk fixes', () => { "INSERT INTO journey_entries (journey_id, source_trip_id, author_id, type, title, entry_date, created_at, updated_at) VALUES (?, ?, ?, 'skeleton', 'S', '2025-06-01', 0, 0)", ).run(journeyId, trip.id, user.id); - const broken = await failingTrips('DELETE FROM trips WHERE id = ?'); - await expect(broken.remove(trip.id, user.id, 'user')).rejects.toThrow('boom'); + const tripsRepo = await createTestTripsRepo(dbs().connection); + const spy = vi.spyOn(tripsRepo, 'deleteById').mockRejectedValueOnce(new Error('boom')); + try { + await expect(svc.remove(trip.id, user.id, 'user')).rejects.toThrow('boom'); + } finally { + spy.mockRestore(); + } // The skeleton cleanup rolled back with the failed delete. expect(testDb.prepare("SELECT id FROM journey_entries WHERE type = 'skeleton'").get()).toBeDefined(); diff --git a/server/tests/unit/plugins/rpc-fallbacks.test.ts b/server/tests/unit/plugins/rpc-fallbacks.test.ts index 4d9a9be8a..33c4232b9 100644 --- a/server/tests/unit/plugins/rpc-fallbacks.test.ts +++ b/server/tests/unit/plugins/rpc-fallbacks.test.ts @@ -23,6 +23,7 @@ import { ReservationsRpc } from '../../../src/nest/reservations/reservations.rpc import { DaysRpc } from '../../../src/nest/days/days.rpc'; import { VacayRpc } from '../../../src/nest/vacay/vacay.rpc'; import { NotFoundError, ValidationError } from '../../../src/nest/trips/trips.service'; +import type { EntityManager } from '@mikro-orm/core'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import type { AddonsService } from '../../../src/nest/addons/addons.service'; @@ -43,8 +44,23 @@ function guardsFor() { canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 42 })), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }), all: () => [] })), } as unknown as DatabaseService; + // Plan 3c Task 7: TripsRpc's RP1-RP6 resolve `EntityManager.getRepository(...)` + // now, not `db.prepare(...)` — a generic fake repository (same shape regardless + // of which entity class was requested) covers every RP method these fallback + // cases exercise, none of which assert on the repository's specific return value. + const em = { + getRepository: () => ({ + getRole: vi.fn(async () => 'user'), + findIdAndEmail: vi.fn(async () => ({ id: 2, email: 'x@example.test' })), + getOwnerId: vi.fn(async () => 1), + findRaw: vi.fn(async () => ({ id: 1, title: 'Japan', feed_token: null })), + listForTripOrdered: vi.fn(async () => []), + listRawUsernameAndDisplayName: vi.fn(async () => []), + }), + } as unknown as EntityManager; return { db, + em, guards: new PluginGuards( db, { checkPermission: vi.fn(() => true) } as unknown as PermissionsService, @@ -141,14 +157,14 @@ describe('a service that says no becomes a refusal, not a crash', () => { describe('service errors are translated into the RPC taxonomy', () => { function tripsHost(updateThrows?: Error, createThrows?: Error) { - const { guards, db } = guardsFor(); + const { guards, em } = guardsFor(); const trips = { list: vi.fn(() => []), updateTrip: vi.fn(() => { if (updateThrows) throw updateThrows; return { updatedTrip: { id: 1 } }; }), create: vi.fn(() => { if (createThrows) throw createThrows; return { trip: { id: 99 } }; }), removeMember: vi.fn(), } as never; - const rpc = new TripsRpc(trips, {} as never, {} as never, {} as never, db, realtime(), guards, {} as never, {} as never); + const rpc = new TripsRpc(trips, {} as never, {} as never, {} as never, realtime(), guards, {} as never, {} as never, em); return new PluginRpcHost('p', ALL, makeDeps(), createTestPluginRegistry([rpc])); } @@ -245,13 +261,13 @@ const SCHEMA_REJECTS: Array<[string, Record]> = [ describe('every schema-validated method rejects a payload its schema refuses', () => { function everything() { - const { guards, db } = guardsFor(); + const { guards, em } = guardsFor(); const anything = new Proxy({}, { get: () => vi.fn(() => ({ id: 1, trip: { id: 1 }, updatedTrip: { id: 1 }, reservation: { id: 1 } })) }) as never; const registry = createTestPluginRegistry([ new PlacesRpc(anything, anything, realtime(), guards), new DaysRpc(anything, realtime(), guards), new PackingRpc(anything, realtime(), guards), - new TripsRpc(anything, anything, anything, anything, db, realtime(), guards, anything, anything), + new TripsRpc(anything, anything, anything, anything, realtime(), guards, anything, anything, em), new AccommodationsRpc(anything, realtime(), guards), new ReservationsRpc(anything, realtime(), guards), ]); diff --git a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts index 468af158e..718d1e63f 100644 --- a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts +++ b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts @@ -17,6 +17,7 @@ import type { RealtimeService } from '../../../src/nest/realtime/realtime.servic import { TripsRpc } from '../../../src/nest/trips/trips.rpc'; import { DbRpc } from '../../../src/nest/plugins/host/rpc/db.rpc'; import { PluginGuards } from '../../../src/nest/plugins/host/plugin-guards.service'; +import type { EntityManager } from '@mikro-orm/core'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; @@ -39,8 +40,13 @@ const tripsRegistry = () => { prepare: () => ({ get: () => ({ id: 1, title: 'Japan' }), all: () => [] }), } as unknown as DatabaseService; const guards = new PluginGuards(db, {} as never, {} as never); + // Plan 3c Task 7: trips.getById (RP1) now reads through + // `EntityManager.getRepository(Trips).findRaw(...)`, not `db.prepare(...)`. + const em = { + getRepository: () => ({ findRaw: async () => ({ id: 1, title: 'Japan', feed_token: null }) }), + } as unknown as EntityManager; return createTestPluginRegistry([ - new TripsRpc({} as never, {} as never, {} as never, {} as never, db, {} as never, guards, {} as never, {} as never), + new TripsRpc({} as never, {} as never, {} as never, {} as never, {} as never, guards, {} as never, {} as never, em), ]); }; diff --git a/server/tests/unit/trips/trips.rpc.test.ts b/server/tests/unit/trips/trips.rpc.test.ts index 052272584..7fdd81459 100644 --- a/server/tests/unit/trips/trips.rpc.test.ts +++ b/server/tests/unit/trips/trips.rpc.test.ts @@ -8,6 +8,7 @@ * ACCESS, which is why it sits behind its own permission rather than any other write. */ import { describe, it, expect, vi } from 'vitest'; +import type { EntityManager } from '@mikro-orm/core'; import { expectRegisteredProvider } from '../../helpers/module-providers'; import { PluginRpcHost } from '../../../src/nest/plugins/host/rpc-host'; import { createTestPluginRegistry } from '../../../src/nest/plugins/host/rpc-kit/testing'; @@ -28,6 +29,10 @@ import type { PermissionsService } from '../../../src/nest/permissions/permissio import type { AddonsService } from '../../../src/nest/addons/addons.service'; import type { RpcRequest, RpcError } from '../../../src/nest/plugins/protocol/envelope'; import { makeDeps } from '../../helpers/rpc-host-deps'; +import { Trips } from '../../../src/db/entities/Trips.entity'; +import { Places } from '../../../src/db/entities/Places.entity'; +import { Users } from '../../../src/db/entities/Users.entity'; +import { TripMembers } from '../../../src/db/entities/TripMembers.entity'; const req = (method: string, params: Record = {}): RpcRequest => ({ k: 'req', id: 'x', method, params }); @@ -60,21 +65,50 @@ export function build(opts: { allow?: (action: string) => boolean; updateThrows? removeMember: vi.fn(), } as unknown as TripMembersService & Record>; const membership = { joinTripAsMember: vi.fn(() => ({ joined: true })) } as unknown as TripMembershipService & Record>; + // `PluginGuards.canEditAs` (nest/plugins/host/, out of this task's scope) + // still calls `db.prepare('SELECT role FROM users WHERE id = ?')` for its + // own per-field permission role lookup — unrelated to RP4's now-converted + // `SELECT role FROM users WHERE id = ?` inside `TripsRpc.update` itself. const db = { canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), - prepare: vi.fn((sql: string) => ({ - get: (arg: number) => { - if (sql.includes('FROM users WHERE')) return arg === 404 ? undefined : { id: arg, role: 'user' }; - if (sql.includes('SELECT user_id FROM trips')) return { user_id: 42 }; - return { id: 1, title: 'Japan' }; - }, - all: () => [{ id: 7, name: 'Place' }], - })), + prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService; + // Plan 3c Task 7: RP1-RP6's SQL-text-keyed `db.prepare` stub is gone — + // every raw statement moved to a repository method, so this is a + // repository-level stub, keyed on the entity CLASS `em.getRepository(...)` + // was asked for, matching the shape the legacy `db.prepare(sql)` branch + // covered: `Trips.findRaw` (RP1/getById), `Places.listForTripOrdered` + // (RP2/getPlaces), `TripMembers.listRawUsernameAndDisplayName` + // (RP3/members), `Users.getRole` (RP4/update) and `.findIdAndEmail` + // (RP5/addMember — id 404 is the "unknown user" case every test below + // exercises), `Trips.getOwnerId` (RP6/removeMember — trip 1's owner is 42). + const em = { + getRepository: vi.fn((entity: unknown) => { + if (entity === Trips) { + return { + findRaw: vi.fn(async (id: number) => ({ id, title: 'Japan', feed_token: null })), + getOwnerId: vi.fn(async () => 42), + }; + } + if (entity === Places) { + return { listForTripOrdered: vi.fn(async () => [{ id: 7, name: 'Place' }]) }; + } + if (entity === Users) { + return { + getRole: vi.fn(async () => 'user'), + findIdAndEmail: vi.fn(async (id: number) => (id === 404 ? null : { id, email: `u${id}@example.test` })), + }; + } + if (entity === TripMembers) { + return { listRawUsernameAndDisplayName: vi.fn(async () => [{ id: 7, username: 'bob', display_name: null, avatar: null }]) }; + } + throw new Error(`build(): unexpected entity ${String(entity)}`); + }), + } as unknown as EntityManager; const permissions = { checkPermission: vi.fn((a: string) => (opts.allow ? opts.allow(a) : true)) } as unknown as PermissionsService; const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const guards = new PluginGuards(db, permissions, { isAddonEnabled: vi.fn(() => true) } as unknown as AddonsService); - const rpc = new TripsRpc(trips, reservations, days, membership, db, realtime, guards, accommodations, roster); + const rpc = new TripsRpc(trips, reservations, days, membership, realtime, guards, accommodations, roster, em); const host = (...grants: string[]) => new PluginRpcHost('p', new Set(grants.length ? grants : ALL_TRIP_GRANTS), makeDeps(), createTestPluginRegistry([rpc])); return { trips, reservations, days, accommodations, roster, membership, realtime, permissions, host };