diff --git a/server/src/db/repositories/DayAccommodations.repository.ts b/server/src/db/repositories/DayAccommodations.repository.ts index e3ad93a2f..9f91171fc 100644 --- a/server/src/db/repositories/DayAccommodations.repository.ts +++ b/server/src/db/repositories/DayAccommodations.repository.ts @@ -1,5 +1,5 @@ import type { DayAccommodations } from '../entities/DayAccommodations.entity'; -import { coalesceParam, columnRef } from '../dialect/sql-functions'; +import { coalesceOverride, columnRef } from '../dialect/sql-functions'; import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; @@ -127,18 +127,19 @@ export class DayAccommodationsRepository extends TrekRepository { const platform = this.getEntityManager().getPlatform(); await this.qb() .update({ - check_in: coalesceParam(platform, 'check_in', check_in), - check_in_end: coalesceParam(platform, 'check_in_end', check_in_end), - check_out: coalesceParam(platform, 'check_out', check_out), + check_in: coalesceOverride(platform, check_in, 'check_in'), + check_in_end: coalesceOverride(platform, check_in_end, 'check_in_end'), + check_out: coalesceOverride(platform, check_out, 'check_out'), }) .where({ id }) .execute('run'); @@ -148,7 +149,7 @@ export class DayAccommodationsRepository extends TrekRepository { const platform = this.getEntityManager().getPlatform(); await this.qb() - .update({ confirmation: coalesceParam(platform, 'confirmation', confirmation) }) + .update({ confirmation: coalesceOverride(platform, confirmation, 'confirmation') }) .where({ id }) .execute('run'); } diff --git a/server/src/db/repositories/Reservations.repository.ts b/server/src/db/repositories/Reservations.repository.ts index e72c068a8..fa12e266f 100644 --- a/server/src/db/repositories/Reservations.repository.ts +++ b/server/src/db/repositories/Reservations.repository.ts @@ -1,7 +1,7 @@ import type { Reservations } from '../entities/Reservations.entity'; import { DayAssignments } from '../entities/DayAssignments.entity'; import { Days } from '../entities/Days.entity'; -import { castIntegerKysely, coalesceParam, columnRef, concatKysely, dayDistance, startsWithIsoDateKysely, substringKysely } from '../dialect/sql-functions'; +import { castIntegerKysely, coalesceOverride, columnRef, concatKysely, dayDistance, startsWithIsoDateKysely, substringKysely } from '../dialect/sql-functions'; import { publicReservationExpr, publicStayExists, type ReservationVisibilityKyselyDB } from './_shared/reservation-visibility'; import type { DayAssignmentRow } from './DayAssignments.repository'; import { TrekRepository } from './_shared/trek-repository'; @@ -960,15 +960,16 @@ export class ReservationsRepository extends TrekRepository { * reservations SET metadata = ?, confirmation_number = COALESCE(?, * confirmation_number) WHERE id = ?`. Runs AFTER `updateAccommodation`'s * own transaction commits (§18.6 — R5 class, flagged not fixed). - * `coalesceParam` for the value-side COALESCE (`DayAccommodationsRepository - * .patchConfirmation`'s precedent). + * `coalesceOverride` for the value-side COALESCE — the bound `?` FIRST, + * the column the fallback, so the new value wins + * (`DayAccommodationsRepository.patchConfirmation`'s precedent). */ async setMetadataAndConfirmation(id: number, metadata: string, confirmation: string | null): Promise { const platform = this.getEntityManager().getPlatform(); await this.qb() .update({ metadata, - confirmation_number: coalesceParam(platform, 'confirmation_number', confirmation), + confirmation_number: coalesceOverride(platform, confirmation, 'confirmation_number'), }) .where({ id }) .execute('run'); diff --git a/server/src/db/repositories/_shared/collection-role.ts b/server/src/db/repositories/_shared/collection-role.ts index 7271c5934..b065a6c7b 100644 --- a/server/src/db/repositories/_shared/collection-role.ts +++ b/server/src/db/repositories/_shared/collection-role.ts @@ -1,4 +1,3 @@ -import type { ExpressionBuilder, ExpressionWrapper, SqlBool } from 'kysely'; import type { CollectionMembers } from '../../entities/CollectionMembers.entity'; import type { Collections } from '../../entities/Collections.entity'; import type { TrekRepository } from './trek-repository'; @@ -42,17 +41,14 @@ import type { TrekRepository } from './trek-repository'; * copy-to-trip/labels/invites surface go through the SAME service methods, * never re-deriving the predicate. * - * **Kysely-callable form** ({@link collectionVisibleExpr}): an inline - * boolean `EXISTS`/`OR` predicate for a caller already building a raw, - * multi-join statement through `this.kysely()` (the shape Task 2's dynamic - * OR-condition methods — `matchingCollectionPlaces`/`findMembership`, - * CL58/CL69 — are the most likely future consumers of), modelled on - * `packing-visibility.ts`'s `packingVisibleToActorExpr`: an owner-id equality - * plus a correlated `EXISTS` against `collection_members`, mirroring CL2's - * own UNION shape as a boolean OR instead of a value-returning UNION SELECT. - * This form answers "is this collection visible to this user", not "what is - * their role" — a caller needing the actual role (for an edit/delete gate) - * always uses {@link resolveCollectionRole}, never this boolean form. + * A Kysely-callable boolean-`EXISTS`/`OR` twin of this predicate (for a + * caller already building a raw, multi-join statement through + * `this.kysely()`, modelled on `packing-visibility.ts`'s + * `packingVisibleToActorExpr`) was drafted alongside this QB form but had no + * consumer by Plan 3h's whole-plan review — Task 2's own dynamic OR-condition + * methods (`matchingCollectionPlaces`/`findMembership`, CL58/CL69) never + * needed it — and was deleted there (L5) rather than shipped untested. Add it + * back, with a consumer or its own parity test, if a future task needs it. * * Proven against five actor cases (owner, admin member, editor member, * viewer member, an invited-but-still-pending user, a complete stranger) by @@ -89,40 +85,3 @@ export async function resolveCollectionRole( if (!member) return null; return member.role === 'admin' || member.role === 'viewer' ? (member.role as 'admin' | 'viewer') : 'editor'; } - -/** - * `collections`/`collection_members`'s REAL table names, the shape - * {@link collectionVisibleExpr} needs. A consumer's own local Kysely `DB` - * interface satisfies this by structural extension, the - * `ReservationVisibilityKyselyDB`/`PackingVisibilityKyselyDB` precedent. - */ -export interface CollectionVisibilityKyselyDB { - collections: { id: number; owner_id: number }; - collection_members: { collection_id: number; user_id: number; status: string }; -} - -/** - * The Kysely-expression twin of CL2's UNION, as a boolean OR: owner-id - * equality, or a correlated `EXISTS` against an accepted `collection_members` - * row. `collections` is a genuine `p.manyToOne(Users)` FK - * (`Collections.entity.ts`), but this form takes the raw `owner_id` column - * directly rather than joining through the relation — matching CL2's own - * `owner_id = ?` text, and avoiding a second join the caller's own outer - * query may not want. - */ -export function collectionVisibleExpr( - eb: ExpressionBuilder, - userId: number, -): ExpressionWrapper { - return eb.or([ - eb('collections.owner_id', '=', userId), - eb.exists( - eb - .selectFrom('collection_members as cm') - .select('cm.collection_id') - .whereRef('cm.collection_id', '=', 'collections.id') - .where('cm.user_id', '=', userId) - .where('cm.status', '=', 'accepted'), - ), - ]); -} diff --git a/server/src/nest/doc-sync/doc-sync.controller.ts b/server/src/nest/doc-sync/doc-sync.controller.ts index 69b479538..e28de7ce2 100644 --- a/server/src/nest/doc-sync/doc-sync.controller.ts +++ b/server/src/nest/doc-sync/doc-sync.controller.ts @@ -14,6 +14,7 @@ import { Req, UseGuards, } from '@nestjs/common'; +import { MikroORM } from '@mikro-orm/core'; import type { Request } from 'express'; import type { User } from '../../types'; import { ADDON_IDS } from '../../addons'; @@ -23,6 +24,7 @@ import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import { TripAccessGuard } from '../permissions/trip-access.guard'; import { RealtimeService } from '../realtime/realtime.service'; +import { withRequestContext } from '../database/request-context'; import { docFailed } from './document-provider'; import { DocSyncConfigService, type LinkRow } from './doc-sync-config.service'; import { DocSyncService } from './doc-sync.service'; @@ -70,6 +72,7 @@ export class DocSyncController { private readonly sync: DocSyncService, private readonly registry: DocumentProviderRegistry, private readonly realtime: RealtimeService, + private readonly orm: MikroORM, ) {} /** @@ -316,7 +319,24 @@ export class DocSyncController { // A first run right away, so the user sees something happen instead of // waiting out a poll interval and wondering whether it worked. - void this.sync.syncLink(res.data, { full: true }); + // + // L1 (Plan 3h Task 7 review) — a third R9-class detached chain the + // inventory missed: `syncLink` runs ~36 ORM/Kysely statements, detached + // after this response goes out, on the request's own EntityManager fork. + // Wrapped in its own `withRequestContext` fork for the same reason the + // webhook nudge timer (`DocSyncWebhookController#schedule`) and the + // import-job runner (`ImportJobsService#start`) are: insurance against + // the request-scoped fork being gone by the time this runs, per 3f's own + // measurement that `AsyncLocalStorage` survives a detached chain intact + // in this codebase today, not a fix for an observed failure. A failure + // here is not a failure of the binding (the next poll carries it), so it + // is logged rather than thrown — same posture as the webhook + // subscription failure just above. + void withRequestContext(this.orm, () => this.sync.syncLink(res.data, { full: true })).catch((err: unknown) => { + this.logger.error( + `link ${res.data.id}: the first run after creation failed (${err instanceof Error ? err.message : String(err)}), the next poll carries it`, + ); + }); return await this.config.publicLink((await this.config.getLink(res.data.id)) ?? res.data, publicOrigin(req)); } diff --git a/server/src/nest/doc-sync/doc-sync.service.ts b/server/src/nest/doc-sync/doc-sync.service.ts index 3aa1af8d5..ddc99e572 100644 --- a/server/src/nest/doc-sync/doc-sync.service.ts +++ b/server/src/nest/doc-sync/doc-sync.service.ts @@ -979,9 +979,14 @@ export class DocSyncService { * queries (a 3rd near-duplicate listing shape alongside {@link issues}'s * own `LIMIT 200` fixed-state-list) stay two repository methods, per R2's * own instruction not to unify near-duplicate reads with different shapes. + * `state` is checked for TRUTHINESS, not `undefined` (Plan 3h Task 7 + * review, M2): legacy's own gate (`doc-sync.controller.ts`, pre-R2) read + * `state ? : `, so `?state=` (an empty string) + * returns every item — an `=== undefined` check would instead filter on + * `state = ''`, which matches nothing. */ async itemsForTrip(tripId: number, state?: string) { - return state === undefined ? this.items.listForTrip(tripId) : this.items.listForTripByState(tripId, state); + return state ? this.items.listForTripByState(tripId, state) : this.items.listForTrip(tripId); } /** Per-trip view for the UI: what is synced, what needs attention. */ diff --git a/server/tests/integration/doc-sync-webhook-request-context.test.ts b/server/tests/integration/doc-sync-webhook-request-context.test.ts index 1aa48654a..d40db24e0 100644 --- a/server/tests/integration/doc-sync-webhook-request-context.test.ts +++ b/server/tests/integration/doc-sync-webhook-request-context.test.ts @@ -11,21 +11,32 @@ * now forks its OWN fresh request context via `withRequestContext` — insurance, * not a fix for an observed failure. * - * This test drives the REAL `DocSyncWebhookController.nudge()` (not a - * hand-built double) through a real HTTP request against a compiled - * `buildApp()` boot — `nudge()` itself needs the per-request `EntityManager` - * fork Nest's own middleware provides, so only the schedule()`'s DETACHED - * TIMER body (which runs from a genuinely bare context, after the response - * has already gone out) is what this test's assertion is about — and asserts - * that body never logs `cannotUseGlobalContext` — the same - * regression class `boot-sweeps-request-context.test.ts` guards for the seven - * cron boot sweeps and `storage-health-notifier-request-context.test.ts` - * guards for Plan 3f's own R3 listener. + * Plan 3h Task 7 review, M3: the original version of this test drove the + * REAL `DocSyncWebhookController.nudge()` through a real HTTP request, on the + * theory that only the timer body — not `nudge()` itself — was the "bare + * context" under test. That theory was wrong in a way that made the ratchet + * non-load-bearing: the HTTP request driving `nudge()` gives Nest's own + * middleware an `EntityManager` fork, and `AsyncLocalStorage` carries that + * fork into the `setTimeout` callback regardless of whether `schedule()` + * wraps it in its own `withRequestContext` — so unwrapping it left the test + * green (proven; recorded in the review's mutation log as MD3). The request + * was never gone by the time the timer fired; it just felt that way because + * the response had already been sent. + * + * The fix: call the controller's `schedule()` directly from the test's own + * top-level async function body, which sits in NO `AsyncLocalStorage` + * context at all — a genuinely bare one. `schedule()` itself needs no + * `EntityManager` (it only touches the in-memory `pending` map and + * `setTimeout`), so this call is safe outside any request; the `reload` + * callback it schedules (`() => config.getLink(linkId)`) is what needs the + * fork, and it only runs once the timer fires, inside `schedule()`'s own + * `withRequestContext` — exactly the wrap under test. The link id comes from + * a raw `testDb` read (no ORM, no context needed) rather than through + * `nudge()`/`getLinkByToken`, so nothing before the `schedule()` call ever + * touches the EntityManager. */ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import type { INestApplication } from '@nestjs/common'; -import request from 'supertest'; -import type { Server } from 'http'; vi.mock('../../src/db/database', async () => { const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); @@ -46,19 +57,23 @@ import { db as testDb } from '../../src/db/database'; import { buildApp } from '../../src/bootstrap'; import { createUser, createTrip } from '../helpers/factories'; import { WEBHOOK_NUDGE_DEBOUNCE_SECONDS } from '../../src/nest/doc-sync/doc-sync.constants'; +import { DocSyncWebhookController } from '../../src/nest/doc-sync/doc-sync-webhook.controller'; +import { DocSyncConfigService } from '../../src/nest/doc-sync/doc-sync-config.service'; describe('DocSyncWebhookController#schedule() runs its detached timer body inside its own request context', () => { let app: INestApplication; - let server: Server; - let token: string; + let controller: DocSyncWebhookController; + let config: DocSyncConfigService; + let linkId: number; beforeAll(async () => { app = await buildApp(); - server = app.getHttpServer(); + controller = app.get(DocSyncWebhookController); + config = app.get(DocSyncConfigService); - // The addon gate and the provider both have to be genuinely on: nudge() - // returns before ever scheduling anything otherwise (isSwitchedOff's own - // early-out), and this ratchet needs schedule() to actually fire. + // The addon gate and the provider both have to be genuinely on: schedule()'s + // own reload callback (`syncIsOn`) skips the run otherwise, before ever + // reaching the assertion below. testDb.exec("UPDATE addons SET enabled = 1 WHERE id = 'documents'"); testDb.exec("UPDATE document_providers SET enabled = 1 WHERE id = 'paperless'"); @@ -70,15 +85,17 @@ describe('DocSyncWebhookController#schedule() runs its detached timer body insid VALUES (?, 'paperless', ?, 'https://paperless.example.com', NULL, '{}')`, ) .run(trip.id, owner.id); - token = 'webhook-ctx-ratchet-token'; - testDb + const linkInfo = testDb .prepare( `INSERT INTO trip_document_links (trip_id, connection_id, provider_id, remote_scope_key, remote_label, direction, delete_policy, conflict_policy, sync_enabled, webhook_token, created_by) - VALUES (?, ?, 'paperless', 'tag:1', 'Japan', 'both', 'unlink', 'manual', 1, ?, ?)`, + VALUES (?, ?, 'paperless', 'tag:1', 'Japan', 'both', 'unlink', 'manual', 1, 'webhook-ctx-ratchet-token', ?)`, ) - .run(trip.id, connInfo.lastInsertRowid, token, owner.id); + .run(trip.id, connInfo.lastInsertRowid, owner.id); + // A plain better-sqlite3 read — no ORM, no request context — so nothing + // before the `schedule()` call below ever touches the EntityManager. + linkId = Number(linkInfo.lastInsertRowid); }); afterAll(async () => { @@ -87,22 +104,16 @@ describe('DocSyncWebhookController#schedule() runs its detached timer body insid }); it( - 'WEBHOOK-CTX-001: nudging from a bare (non-request) context never logs cannotUseGlobalContext once the debounce timer fires', + 'WEBHOOK-CTX-001: schedule(), called from a genuinely bare (non-request) context, never logs cannotUseGlobalContext once its debounce timer fires', async () => { const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); try { - // A REAL HTTP request through the compiled app — not a hand-built - // double, and not a direct method call either: `nudge()` itself - // needs the per-request `EntityManager` fork Nest's own middleware - // provides (its own `getLinkByToken` read would otherwise throw - // `cannotUseGlobalContext` before ever reaching `schedule()` — this - // is what a direct `app.get(...).nudge(...)` call, tried first, - // actually surfaced: confirmation this test needs the real request - // path, not a shortcut around it). Only the DETACHED TIMER body - // that fires after this response is the "bare context" under test. - const res = await request(server).post(`/api/docsync/webhook/${token}`).send({}); - expect(res.status).toBe(200); - expect(res.body).toEqual({ received: true }); + // Calling the controller's own private `schedule()` directly, bracket + // notation only (no `any`/`as unknown as`): this test function body is + // not inside ANY AsyncLocalStorage context, unlike a real HTTP request + // through Nest's middleware — so the timer this starts is the real + // thing the docstring above describes, not one riding a caller's fork. + controller['schedule'](linkId, () => config.getLink(linkId)); // Let the real debounce timer fire, then drain the microtasks its // async body queues (real timers here — the detached body is a diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index d0595538b..d914b82dd 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -1155,3 +1155,65 @@ describe('PlacesRepository.setSource (DWS7)', () => { expect(row.updated_at).toBe(before.updated_at); }); }); + +// --------------------------------------------------------------------------- +// Plan 3h Task 7 review, M1 coverage — Plan 3h Task 4's `maps.service.ts` +// photo-fetch (MAP9), untested at the repository level (the service test +// stubs this method). +// --------------------------------------------------------------------------- + +describe('PlacesRepository.setImageUrlIfUnset (MAP9)', () => { + it('MAP9-001: fills a NULL image_url, matching the legacy $or guard', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'No photo yet' }); + testDb.prepare("UPDATE places SET google_place_id = 'ChIJ_shared' WHERE id = ?").run(place.id); + + const n = await places.setImageUrlIfUnset('ChIJ_shared', '/uploads/photo-cache/new.jpg'); + + expect(n).toBe(1); + const row = testDb.prepare('SELECT image_url FROM places WHERE id = ?').get(place.id) as { image_url: string | null }; + expect(row.image_url).toBe('/uploads/photo-cache/new.jpg'); + }); + + it('MAP9-002: fills an EMPTY-STRING image_url too (the second half of the $or)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Blank photo' }); + testDb.prepare("UPDATE places SET google_place_id = 'ChIJ_blank', image_url = '' WHERE id = ?").run(place.id); + + const n = await places.setImageUrlIfUnset('ChIJ_blank', '/uploads/photo-cache/filled.jpg'); + + expect(n).toBe(1); + const row = testDb.prepare('SELECT image_url FROM places WHERE id = ?').get(place.id) as { image_url: string | null }; + expect(row.image_url).toBe('/uploads/photo-cache/filled.jpg'); + }); + + it('MAP9-003: an already-set image_url is left untouched — never clobbers a custom image', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Custom photo' }); + testDb.prepare("UPDATE places SET google_place_id = 'ChIJ_custom', image_url = '/uploads/custom.jpg' WHERE id = ?").run(place.id); + + const n = await places.setImageUrlIfUnset('ChIJ_custom', '/uploads/photo-cache/should-not-land.jpg'); + + expect(n).toBe(0); + const row = testDb.prepare('SELECT image_url FROM places WHERE id = ?').get(place.id) as { image_url: string | null }; + expect(row.image_url).toBe('/uploads/custom.jpg'); + }); + + it('MAP9-004: unscoped by trip — every empty-image row sharing the google_place_id across trips is filled in one statement', async () => { + const { user } = createUser(testDb); + const tripA = createTrip(testDb, user.id); + const tripB = createTrip(testDb, user.id); + const placeA = createPlace(testDb, tripA.id, { name: 'A' }); + const placeB = createPlace(testDb, tripB.id, { name: 'B' }); + testDb.prepare("UPDATE places SET google_place_id = 'ChIJ_shared2' WHERE id IN (?, ?)").run(placeA.id, placeB.id); + + const n = await places.setImageUrlIfUnset('ChIJ_shared2', '/uploads/photo-cache/both.jpg'); + + expect(n).toBe(2); + expect((testDb.prepare('SELECT image_url FROM places WHERE id = ?').get(placeA.id) as { image_url: string }).image_url).toBe('/uploads/photo-cache/both.jpg'); + expect((testDb.prepare('SELECT image_url FROM places WHERE id = ?').get(placeB.id) as { image_url: string }).image_url).toBe('/uploads/photo-cache/both.jpg'); + }); +}); diff --git a/server/tests/unit/db/repositories/Reservations.repository.test.ts b/server/tests/unit/db/repositories/Reservations.repository.test.ts index a196d0e75..c884fb120 100644 --- a/server/tests/unit/db/repositories/Reservations.repository.test.ts +++ b/server/tests/unit/db/repositories/Reservations.repository.test.ts @@ -505,3 +505,64 @@ describe('ReservationsRepository — Task 2 reads (listForTrip / findWithJoins / expect(typed.map((r) => r.id).sort((a, b) => a - b)).toEqual([dinner.id, hotel.id].sort((a, b) => a - b)); }); }); + +// --------------------------------------------------------------------------- +// Plan 3h Task 7 review, M1 coverage — the AirTrail leaf's writes/reads, +// untested at the repository level (the airtrail-link/sync service tests +// stub this repository). +// --------------------------------------------------------------------------- + +describe('ReservationsRepository — AirTrail leaf (ATL1/ATL2/ATL4, airports)', () => { + it('ATL1 linkAirtrailSingleFlight — stamps every AirTrail column and switches sync on, matching the legacy statement', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const res = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + + await reservationsRepo.linkAirtrailSingleFlight(res.id, 'AT-42', user.id, 'hash-1', '2026-09-24T10:00:00Z'); + + const row = testDb.prepare( + "SELECT external_source, external_id, external_owner_user_id, sync_enabled, external_hash, external_synced_at FROM reservations WHERE id = ?", + ).get(res.id); + expect(row).toEqual({ + external_source: 'airtrail', external_id: 'AT-42', external_owner_user_id: user.id, + sync_enabled: 1, external_hash: 'hash-1', external_synced_at: '2026-09-24T10:00:00Z', + }); + }); + + it('ATL2 setAirtrailSyncDisabled — turns sync_enabled off, matching the legacy statement, and leaves every other column alone', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const res = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + await reservationsRepo.linkAirtrailSingleFlight(res.id, 'AT-42', user.id, 'hash-1', '2026-09-24T10:00:00Z'); + + await reservationsRepo.setAirtrailSyncDisabled(res.id); + + const row = testDb.prepare('SELECT sync_enabled, external_id FROM reservations WHERE id = ?').get(res.id); + expect(row).toEqual({ sync_enabled: 0, external_id: 'AT-42' }); + }); + + it('ATL4 findAirtrailLinked — matches the legacy statement, scoped to external_source = \'airtrail\' (a plain flight misses)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const linked = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + await reservationsRepo.linkAirtrailSingleFlight(linked.id, 'AT-7', user.id, 'hash-2', '2026-09-24T11:00:00Z'); + const plain = createReservation(testDb, trip.id, { title: 'Dinner', type: 'restaurant' }); + + const legacy = testDb.prepare( + "SELECT id, trip_id, external_id, external_owner_user_id, sync_enabled FROM reservations WHERE id = ? AND external_source = 'airtrail'", + ).get(linked.id); + expect(await reservationsRepo.findAirtrailLinked(linked.id)).toEqual(legacy); + expect(await reservationsRepo.findAirtrailLinked(plain.id)).toBeUndefined(); + }); + + it('markNeedsReview (airports) — sets needs_review, matching the legacy statement', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const res = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + expect((testDb.prepare('SELECT needs_review FROM reservations WHERE id = ?').get(res.id) as { needs_review: number }).needs_review).toBe(0); + + await reservationsRepo.markNeedsReview(res.id); + + expect((testDb.prepare('SELECT needs_review FROM reservations WHERE id = ?').get(res.id) as { needs_review: number }).needs_review).toBe(1); + }); +}); diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index 866a142fa..4c9f6dfbe 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -282,6 +282,26 @@ describe('updateAccommodation', () => { const reservation = testDb.prepare('SELECT confirmation_number FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; expect(reservation.confirmation_number).toBe('RES-9'); }); + + it('H1 (Plan 3h Task 7 review): a new confirmation on the stay OVERRIDES the linked reservation\'s existing, different confirmation — the new value wins, matching legacy COALESCE(?, confirmation_number)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id) as any; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }) as any; + const { accommodation: accom } = (await svc.createAccommodation(trip.id, { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, + })) as any; + // The linked reservation already carries a DIFFERENT, non-null confirmation — + // if the stay-side update used `coalesceParam`'s (existing-wins) direction + // instead of `coalesceOverride`'s (new-wins), it would keep this value. + testDb.prepare('UPDATE reservations SET confirmation_number = ? WHERE accommodation_id = ?').run('RES-OLD', accom.id); + + const existing = (await svc.getAccommodation(accom.id, trip.id))!; + await svc.updateAccommodation(accom.id, existing as any, { confirmation: 'ACC-CHANGED' }); + + const reservation = testDb.prepare('SELECT confirmation_number FROM reservations WHERE accommodation_id = ?').get(accom.id) as any; + expect(reservation.confirmation_number).toBe('ACC-CHANGED'); + }); }); describe('deleteAccommodation', () => { diff --git a/server/tests/unit/nest/booking-import/import-jobs.service.test.ts b/server/tests/unit/nest/booking-import/import-jobs.service.test.ts index eea3d75cb..e2bdec7bd 100644 --- a/server/tests/unit/nest/booking-import/import-jobs.service.test.ts +++ b/server/tests/unit/nest/booking-import/import-jobs.service.test.ts @@ -7,6 +7,7 @@ import { ImportJobsService } from '../../../../src/nest/booking-import/import-jo import { RealtimeService } from '../../../../src/nest/realtime/realtime.service'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { Users } from '../../../../src/db/entities/Users.entity'; // R9 (Plan 3h Task 4): `run()` now forks its own `withRequestContext`, so the // service needs a real `MikroORM` — the `StorageHealthNotifierService` @@ -58,7 +59,18 @@ describe('ImportJobsService', () => { }); it('IMPORTJOBS-CTX-001 (R9 ratchet): start() is called from a bare, non-request context (like every test above) and the detached run() still resolves cleanly — the withRequestContext wrap around it means the missing ambient context never reaches BookingImportService.preview as cannotUseGlobalContext', async () => { - const preview = vi.fn(async () => ({ items: [{ id: 'ctx' }] })); + // Plan 3h Task 7 review, M3: a plain `vi.fn` double for `preview` never + // touches the ORM at all, so this ratchet stayed green even with the + // wrap removed (the mutation log's MD4). `preview` now does a REAL read + // through `t`'s `allowGlobalContext: false` EntityManager — the same + // instance `beforeAll` built specifically so a passing suite proves the + // wrap load-bearing, not merely harmless — so it throws + // "global EntityManager"/`cannotUseGlobalContext` unless `run()`'s own + // `withRequestContext` fork is actually live when this executes. + const preview = vi.fn(async () => { + await t.repo(Users).findOne({ id: -1 }); + return { items: [{ id: 'ctx' }] }; + }); const svc = makeService(preview); const id = svc.start('7', files(1), 'no-ai', 42); diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index 83bea83e2..0b9e56421 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -218,6 +218,26 @@ describe('saved places + dedup', () => { expect(res.place!.tags?.map((t) => t.name)).toContain('food'); }); + it('CL49 (Plan 3h Task 7 review, M1 coverage): updatePlace with tag_ids rewrites the tag set (CollectionPlacesRepository.deleteTags)', async () => { + const u = createUser(testDb).user; + const food = createTag(testDb, u.id, { name: 'food' }); + const views = createTag(testDb, u.id, { name: 'views' }); + const col = await svc.createCollection(u.id, { name: 'Tagged' }); + const res = await svc.savePlace(u.id, { collection_id: col.id, name: 'Ramen', tag_ids: [food.id] }); + expect(res.place!.tags?.map((t) => t.name)).toEqual(['food']); + + // A second tag_ids write must DROP the old tag rather than accumulate — + // only possible if the old assignment rows were actually deleted first. + await svc.updatePlace(u.id, res.place!.id, { tag_ids: [views.id] }); + const stored = (await svc.getCollection(u.id, col.id)).places.find((p) => p.id === res.place!.id)!; + expect(stored.tags?.map((t) => t.name)).toEqual(['views']); + + // An empty tag_ids array clears every tag. + await svc.updatePlace(u.id, res.place!.id, { tag_ids: [] }); + const cleared = (await svc.getCollection(u.id, col.id)).places.find((p) => p.id === res.place!.id)!; + expect(cleared.tags ?? []).toEqual([]); + }); + it('COLLECTIONS-SVC-013: savePlace rejects an inaccessible collection (404)', async () => { const a = createUser(testDb).user; const b = createUser(testDb).user; @@ -240,6 +260,22 @@ describe('saved places + dedup', () => { expect(result.place).toBeDefined(); }); + it('CL27 (Plan 3h Task 7 review, M1 coverage): an UNNAMED candidate falls back to the coordinate dedup branch (CollectionsRepository.findDuplicateByCoords)', async () => { + // `placeMatchStrategies` only offers `coords` when the name normalizes away + // to nothing (blank/whitespace); a single space satisfies the DTO's + // `min(1)` but still normalizes to null, so it reaches the coords branch + // this repository method serves. + const u = createUser(testDb).user; + const col = await svc.createCollection(u.id, { name: 'Unnamed pins' }); + await svc.savePlace(u.id, { collection_id: col.id, name: ' ', lat: 48.8566, lng: 2.3522 }); + + const withinTolerance = await svc.savePlace(u.id, { collection_id: col.id, name: ' ', lat: 48.85665, lng: 2.35215 }); + expect(withinTolerance.duplicate).toBe(true); + + const outsideTolerance = await svc.savePlace(u.id, { collection_id: col.id, name: ' ', lat: 48.9, lng: 2.5 }); + expect(outsideTolerance.duplicate).toBeFalsy(); + }); + it('COLLECTIONS-SVC-101: a provider id still recognises a renamed place a name/coords search would miss', async () => { // google_place_id/google_ftid/osm_id are stored on every collection_places row // but were never read back for dedup, so a renamed place with no matching diff --git a/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts index 5a7435389..56f3e3594 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts @@ -78,6 +78,7 @@ import type { } from '../../../../src/nest/doc-sync/doc-sync.dto'; import type { User } from '../../../../src/types'; import { createTestUnitOfWork, createTestTripsRepo } from '../../../helpers/test-uow'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createTestDocumentConnectionsRepo, createTestDocumentProviderFieldsRepo, @@ -156,6 +157,7 @@ const realtime = { broadcast: vi.fn() }; // createTestUnitOfWork is async — module-scope construction cannot await it. let config: DocSyncConfigService; let controller: DocSyncController; +let t: TestOrm; /** Only what publicOrigin reads: header lookup plus the connection's own scheme. */ function makeReq(headers: Record = {}, protocol = 'http'): Request { @@ -210,6 +212,11 @@ let otherTripId: number; beforeAll(async () => { createTables(testDb); runMigrations(testDb); + // L1 (Plan 3h Task 7 review): createLink's fire-and-forget syncLink call + // is now wrapped in its own withRequestContext fork, which needs a real + // MikroORM instance — sync itself stays a spy, this ORM never resolves a + // repository through it. + t = await createTestOrm(testDb); config = new DocSyncConfigService( await createTestTripsRepo(testDb), await createTestDocumentProvidersRepo(testDb), @@ -220,7 +227,7 @@ beforeAll(async () => { registry, await createTestUnitOfWork(testDb), ); - controller = new DocSyncController(config, sync as unknown as DocSyncService, registry, realtime as unknown as RealtimeService); + controller = new DocSyncController(config, sync as unknown as DocSyncService, registry, realtime as unknown as RealtimeService, t.orm); const o = createUser(testDb, { username: 'owner', email: 'owner@test.local' }).user; const m = createUser(testDb, { username: 'member', email: 'member@test.local' }).user; const a = createUser(testDb, { username: 'admin', email: 'admin@test.local', role: 'admin' }).user; @@ -232,7 +239,8 @@ beforeAll(async () => { testDb.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(tripId, m.id); }); -afterAll(() => { +afterAll(async () => { + await t.close(); testDb.close(); }); diff --git a/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts index f17bb82b4..8f0bd7c3e 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts @@ -434,7 +434,13 @@ describe('DocSyncService', () => { const tieBreakFirst = await makeLink({ lastSyncAt: null }); // same ties; id is the ONLY thing that can separate these two const midStale = await makeLink({ lastSyncAt: sqlTime('-2 hours') }); // next=NULL, last=2h ago — after the NULL/NULL pair, before the recent one const midRecent = await makeLink({ lastSyncAt: sqlTime('-1 minute') }); // next=NULL, last=1m ago — last of the NULL-next-attempt group - const backedOff = await makeLink({ nextAttemptAt: sqlTime('-1 hour'), lastSyncAt: sqlTime('-1 minute') }); // next_attempt_at is NON-null — sorts after every NULL row regardless of last_sync_at + // Plan 3h Task 7 review, M4: this row's `last_sync_at` used to be + // `-1 minute` — the newest of every row in this test — so it was + // ALSO the sort-first row under `last_sync_at`-before-`next_attempt_at` + // (the swapped-key mutation), leaving the test green either way. `-3 + // hours` is older than every other row's `last_sync_at`, so only the + // correct key order (next_attempt_at first) still sorts this row last. + const backedOff = await makeLink({ nextAttemptAt: sqlTime('-1 hour'), lastSyncAt: sqlTime('-3 hours') }); // next_attempt_at is NON-null — sorts after every NULL row regardless of last_sync_at const ids = (await service.dueLinks()).map((l) => l.id); const expected = [tieBreakSecond.id, tieBreakFirst.id].sort((a, b) => a - b); @@ -1176,6 +1182,41 @@ describe('DocSyncService', () => { }); }); + describe('itemsForTrip', () => { + it('M2 (Plan 3h Task 7 review): an empty `?state=` returns every item, not none — legacy truthiness, not an `undefined` check', async () => { + const link = await makeLink(); + const fileId = makeFile(); + const withFile = seedItem(link, { remoteId: 'r1', state: 'synced', fileId }); + const withoutFile = seedItem(link, { remoteId: 'r2', state: 'conflict' }); + + // `state === undefined ? listForTrip : listForTripByState` (the R2 + // regression this ratchet pins) would treat `''` as a real filter + // value and match nothing; the legacy/fixed gate is `state ? + // listForTripByState : listForTrip`, so an empty string behaves the + // same as omitting the query param entirely. + const withEmptyState = await service.itemsForTrip(tripId, ''); + const withNoState = await service.itemsForTrip(tripId); + + expect(withEmptyState.map((r) => r.id).sort()).toEqual([withFile, withoutFile].sort()); + expect(withNoState.map((r) => r.id).sort()).toEqual([withFile, withoutFile].sort()); + // The file-joined row carries the LEFT JOIN trip_files column; the + // file-less row must not, so `listForTrip`'s join shape is exercised + // by both a hit and a miss (M1's coverage gap on these two methods). + expect(withNoState.find((r) => r.id === withFile)?.file_name).toBe('boarding.pdf'); + expect(withNoState.find((r) => r.id === withoutFile)?.file_name).toBeNull(); + }); + + it('a real state value still narrows to that one state', async () => { + const link = await makeLink(); + seedItem(link, { remoteId: 'r1', state: 'synced' }); + const conflicted = seedItem(link, { remoteId: 'r2', state: 'conflict' }); + + const rows = await service.itemsForTrip(tripId, 'conflict'); + + expect(rows.map((r) => r.id)).toEqual([conflicted]); + }); + }); + describe('resolveConflict', () => { /** A conflicted pairing: the provider moved on, and TREK holds its own copy. */ function seedConflict(link: LinkRow): { itemId: number; fileId: number } { diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index bb02d4426..3c8c01063 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -883,6 +883,45 @@ describe('ReservationsService — legacy branch parity (coverage of the folded c expect(row).toEqual({ check_in: '16:00', confirmation: 'KEEP' }); }); + it('H1 (Plan 3h Task 7 review) — patchTimes: updating a linked reservation OVERRIDES the stay\'s existing non-null check-in/end/out — the new value wins, matching legacy COALESCE(?, col)', async () => { + const { trip } = ownerTrip({ start_date: '2030-05-01', end_date: '2030-05-02' }); + const place = createPlace(testDb, trip.id); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number }[]; + // The stay already holds a DIFFERENT, non-null value on every column the + // sync touches — if `coalesceParam`'s (existing-wins) direction were used + // instead of `coalesceOverride`'s (new-wins), the stay would keep these. + const acc = createDayAccommodation(testDb, trip.id, place.id, days[0].id, days[1].id, { + check_in: '14:00', check_out: '10:00', + }); + testDb.prepare('UPDATE day_accommodations SET check_in_end = ? WHERE id = ?').run('10:00', acc.id); + const res = createReservation(testDb, trip.id, { title: 'Hotel', type: 'hotel' }); + testDb.prepare("UPDATE reservations SET accommodation_id = ?, metadata = ? WHERE id = ?") + .run(String(acc.id), JSON.stringify({ check_in_time: '16:30', check_in_end_time: '11:15', check_out_time: '09:00' }), res.id); + const current = (await svc.getReservation(String(res.id), String(trip.id)))!; + // Touch an unrelated field so the metadata sync runs off the stored + // (already new) metadata, same shape as RESV-SVC-025. + await svc.update(String(res.id), String(trip.id), { notes: 'touch' }, current); + const row = testDb.prepare('SELECT check_in, check_in_end, check_out FROM day_accommodations WHERE id = ?').get(acc.id); + expect(row).toEqual({ check_in: '16:30', check_in_end: '11:15', check_out: '09:00' }); + }); + + it('H1 (Plan 3h Task 7 review) — patchConfirmation: updating a linked reservation OVERRIDES the stay\'s existing non-null confirmation — the new value wins, matching legacy COALESCE(?, confirmation)', async () => { + const { trip } = ownerTrip({ start_date: '2030-05-01', end_date: '2030-05-02' }); + const place = createPlace(testDb, trip.id); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number }[]; + // The stay already holds a DIFFERENT, non-null confirmation — if + // `coalesceParam`'s (existing-wins) direction were used instead of + // `coalesceOverride`'s (new-wins), the stay would keep it. + const acc = createDayAccommodation(testDb, trip.id, place.id, days[0].id, days[1].id, { confirmation: 'ACC-OLD' }); + const res = createReservation(testDb, trip.id, { title: 'Hotel', type: 'hotel' }); + testDb.prepare("UPDATE reservations SET accommodation_id = ?, metadata = ?, confirmation_number = 'RES-NEW' WHERE id = ?") + .run(String(acc.id), JSON.stringify({}), res.id); + const current = (await svc.getReservation(String(res.id), String(trip.id)))!; + await svc.update(String(res.id), String(trip.id), { notes: 'touch' }, current); + const row = testDb.prepare('SELECT confirmation FROM day_accommodations WHERE id = ?').get(acc.id); + expect(row).toEqual({ confirmation: 'RES-NEW' }); + }); + it('RESV-SVC-026: resync derives the end day too, keeping the stored one when the end time is out of range', async () => { const { trip } = ownerTrip({ start_date: '2030-05-01', end_date: '2030-05-03' }); const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number }[];