diff --git a/roadmap.md b/roadmap.md index 7505d3080..2e530e74b 100644 --- a/roadmap.md +++ b/roadmap.md @@ -376,21 +376,22 @@ gate-verifiable, worth re-checking after every phase: `node-cron` out of `package.json`. `index.ts` no longer boots any cron and its `shutdown()` rides `nestApp.close()`. The db-boot airport backfill was already gone (`9c46f31c`); ws/MCP wiring remain its hand-booted seams. -- 🟑 (re-checked 2026-08-11, after the bridge fold) **8** `*.bridge.ts` files - remain, each shrunk to the exports its consumers import and carrying a - truthful header. The honest split: **5 pinned by one seam**, the - pre-`app.init()` MCP/OAuth mount (`oauth`, `audit`, `addons`, `auth`, - `permissions` β€” these die together if that mount ever moves behind the - container), and **3 verified-permanent cycle-dodges** (trips ← - budget.mcp/packing.mcp/costs.rpc β€” TripsModule/TripReadModelModule/ - TripMembersModule all import the budget/packing domains; assignments ← - places.mcp β€” DaysModule β†’ PlacesModule β†’ AssignmentsModule β†’ DaysModule; - airtrail ← reservations.controller). The 2026-08-10 endgame executed: the 4 - dead ones deleted (`8e4261ed`), `journey.bridge` + `src/mcp/resources.ts` - retired with the journey-resource migration, `budget.bridge` deleted - (UserCleanupService injects BudgetService; BudgetModule dropped AuthModule), - and the backup-restore / systemNotices in-container edges folded - (`permissions-cache.ts`, threaded `addonEnabled`). +- 🟑 (re-checked 2026-08-11, after the bridge fold) **5** `*.bridge.ts` files + remain β€” `oauth`, `audit`, `addons`, `auth`, `permissions` β€” each shrunk to + the exports its consumers import, all pinned by the ONE remaining seam (the + pre-`app.init()` MCP/OAuth mount) and all dying together if that mount ever + moves behind the container. The 2026-08-10 endgame executed and then some: + the 4 dead ones deleted (`8e4261ed`); `journey.bridge` + + `src/mcp/resources.ts` retired with the journey-resource migration; + `budget.bridge` deleted (UserCleanupService injects BudgetService); + the backup-restore / systemNotices in-container edges folded + (`permissions-cache.ts`, threaded `addonEnabled`); and the three + "verified-permanent" cycle-dodges eliminated by restructuring rather than + forwardRef β€” `trips.bridge` via leaf TripMembership reads + prompt + relocation, `assignments.bridge` via an `AssignmentsDomainModule` split, + `airtrail.bridge` via `ReservationsReadRepository` + `AirtrailLinkService` + in `AirtrailCoreModule`. `notifications.instance.ts` died with the last of + them. - βœ… (2026-08-11) `src/mcp/resources.ts` gone β€” the 4 journey resources are decorator-registered on `journey.mcp.ts`; `journey.bridge` died with it. - ❌ The `db` proxy is still imported outside DatabaseModule: diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 39bd7c728..a9e3dd7f0 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -32,7 +32,7 @@ npx vitest watch tests/unit/nest/auth-guard.test.ts ## Architecture -Nest owns everything: the strangler migration **completed 2026-08-10**. Legacy `src/services/` is **deleted** β€” `eslint.config.mjs` errors on any import of it β€” and every domain is a DI module under `src/nest//`. `src/nest/README.md` is the per-module blueprint and records the migration's design decisions. What still runs outside the container (tracked in `roadmap.md`): the MCP/OAuth transport mounts before `app.init()` β€” that seam is why five `*.bridge.ts` files survive (plus three permanent cycle-dodge bridges; each bridge's header states its pin and deletion trigger). The websocket seam is gone: `/ws` is a Nest gateway (`src/nest/realtime/`), and `src/websocket.ts` is a re-export stub. The scheduler seam is gone: every cron is a domain provider on `@nestjs/schedule` (see `src/nest/scheduling/` below). +Nest owns everything: the strangler migration **completed 2026-08-10**. Legacy `src/services/` is **deleted** β€” `eslint.config.mjs` errors on any import of it β€” and every domain is a DI module under `src/nest//`. `src/nest/README.md` is the per-module blueprint and records the migration's design decisions. What still runs outside the container (tracked in `roadmap.md`): the MCP/OAuth transport mounts before `app.init()` β€” that seam is why five `*.bridge.ts` files survive (`addons`, `audit`, `auth`, `oauth`, `permissions`; each header states the pin, and all die together if that mount migrates). The cycle-dodge bridges are gone β€” folded via leaf-module splits (`AssignmentsDomainModule`, `ReservationsReadRepository`/`AirtrailCoreModule`, the `TripMembershipService` reads). The websocket seam is gone too: `/ws` is a Nest gateway (`src/nest/realtime/`), and `src/websocket.ts` is a re-export stub. The scheduler seam is gone: every cron is a domain provider on `@nestjs/schedule` (see `src/nest/scheduling/` below). - **`src/index.ts`** β€” process entry: creates upload/data dirs, builds the app via `bootstrap.ts`, wires the WebSocket server onto the same HTTP server, prints the startup banner. Default port **3001**. - **`src/bootstrap.ts` β€” `buildApp()`** β€” the single shared app builder for both prod and the test harness. **Registration order is load-bearing**: `applyGlobalMiddleware` β†’ `applyPlatformUploads` β†’ `applyPlatformTransport` β†’ `applyPlatformStatic` all run on the raw Express instance **before `app.init()`**, because Nest throws `NotFoundException` on unmatched routes and won't fall through to anything registered after init. `bodyParser: false` keeps the raw `/mcp` body unparsed for the MCP SDK. diff --git a/server/src/nest/README.md b/server/src/nest/README.md index caa4fed8e..deb05f661 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -243,16 +243,32 @@ through. `@trek/nest-mcp` hands the gate its declaring instance now, and `AddonsService`. `reservations.mcp.ts` (assignments) and `atlas.mcp.ts` / `journey.mcp.ts` (auth) went the same way. -Two in-container uses are left, both verified-real module cycles that -injection cannot fix without forwardRef. Keep this list current: an entry that -outlives its cause reads as a standing exception and invites the next one. +No in-container bridge uses are left. The last four folded 2026-08-11, and +their folds are the playbook for any edge that reappears β€” plain injection +would have closed a real cycle in every case, so each cut the knot with a +finer-grained module instead: -- `places.mcp.ts` β†’ `assignments.bridge`: a real cycle, `DaysModule β†’ - PlacesModule β†’ AssignmentsModule β†’ DaysModule`. - `budget.mcp.ts` / `packing.mcp.ts` / `costs.rpc.ts` β†’ `trips.bridge`: - `TripsModule`, `TripReadModelModule` and `TripMembersModule` all import the - budget/packing domains, so injecting any of the backing services back - closes a loop. + everything that owns the hydrated trip reads (TripsService, + TripReadModelService, TripMembersService) lives in modules importing the + budget/packing domains, so the consumers moved instead β€” the id-level + membership reads went to the dependency-free `TripMembershipModule` + (`getOwnerId` / `listMemberUserIds` / `listAccessibleTripIds`) and the two + cross-domain prompts moved above the read model onto + `trips/trip-prompts.mcp.ts`. `trips.bridge` (102 lines, ~25 hand-built + services) is deleted. +- `places.mcp.ts` β†’ `assignments.bridge`: the cycle `DaysModule β†’ + PlacesModule β†’ AssignmentsModule β†’ DaysModule` is real, but only + AssignmentsMcp needs DaysService β€” the service itself does not, so it moved + to a leaf `AssignmentsDomainModule` (the journey-domain split) that + PlacesModule imports. `assignments.bridge` is deleted. +- `reservations.controller.ts` β†’ `airtrail.bridge`: the pull genuinely needs + ReservationsService, so `AirtrailModule` keeps it β€” but the write-back push + only needs the hydrated reservation READ. That read became + `ReservationsReadRepository` (leaf, the trek_photos pattern), the link + lifecycle + push became `AirtrailLinkService` in `AirtrailCoreModule`, and + the controller injects it. `airtrail.bridge` is deleted, and + `notifications.instance.ts` β€” whose last consumer it was β€” died with it. Two more were on this list and folded 2026-08-11: @@ -283,12 +299,12 @@ cases that exercised them kept their assertions and point at the service. four legacy resources became `@Resource`/`@ResourceTemplate` methods on `journey.mcp.ts`. -Eight bridges remain: five pinned by the pre-`app.init()` MCP/OAuth mount +Five bridges remain, all pinned by the one pre-`app.init()` MCP/OAuth mount (`addons`, `audit`, `auth`, `oauth`, `permissions` β€” shrunk to the exports that -seam consumes) and three verified-permanent cycle-dodges (`assignments`, -`airtrail`, `trips`). When one loses its last outside-container consumer, -delete it in the same change rather than leaving it as a courtesy: an unused -bridge reads as a supported entry point. +seam consumes) and all dying together if that mount ever moves behind the +container. When one loses its last outside-container consumer, delete it in +the same change rather than leaving it as a courtesy: an unused bridge reads +as a supported entry point. The nine fire-and-forget notification senders inject too. They were lazy `import('../notifications/notifications.bridge').then(({ send }) => …)` calls @@ -306,11 +322,12 @@ harnesses (`new AuthService(db, permissions, atlas)` where the class takes six). That is why `tests/` is not in the build's `include`, and cleaning it up is its own piece of work. -`notifications.instance.ts` holds the out-of-container `NotificationsService` -for the surviving cycle-dodge bridges (`trips.bridge` and `airtrail.bridge`; -it lives exactly as long as either does). The `notifications.bridge.ts` send -shim it used to back died with the scheduler migration β€” the reminder crons -inject the container singleton now (`ReminderJobsService`). +`notifications.instance.ts` held the out-of-container `NotificationsService` +for the cycle-dodge bridges and died 2026-08-11 with the last of them +(`airtrail.bridge`) β€” every production consumer injects the container +singleton now. The module-scoped channel registry it leaned on stays (the +no-Nest test harnesses still construct instances; CHOVR-015 pins the +sharing). - `app-config/` β€” the `@nestjs/config` binding (`AppConfigModule`, global). Never read `process.env` in a module (ESLint enforces this): inject a boot-stable namespace via its `registerAs` token (`@Inject(mcpConfig.KEY) … ConfigType<…>`) @@ -855,8 +872,8 @@ SQL, statuses, bodies, and error strings. The plugin RPC host is **no longer a bridge consumer**: since Option A of `src/nest/plugins/DI-MIGRATION.md` it injects domain services via `PluginHostDepsFactory`, so a migrated domain adds `exports: [XService]` + a `PluginsModule` import instead of a bridge entry. -Only the pre-`app.init()` MCP/OAuth mount and the three verified module-cycle -seams still need bridges β€” nothing on the websocket path imports one, and the +Only the pre-`app.init()` MCP/OAuth mount still needs bridges β€” the module- +cycle seams are folded, nothing on the websocket path imports one, and the crons all inject now (`*.job.ts` providers on `scheduling/CronRegistrarService`). 1. **Move the SQL** into `.service.ts` as methods over an injected diff --git a/server/src/nest/integrations/airtrail-core.module.ts b/server/src/nest/integrations/airtrail-core.module.ts new file mode 100644 index 000000000..a77585f09 --- /dev/null +++ b/server/src/nest/integrations/airtrail-core.module.ts @@ -0,0 +1,24 @@ +import { Module } from '@nestjs/common'; +import { AddonsModule } from '../addons/addons.module'; +import { AuditModule } from '../audit/audit.module'; +import { RealtimeModule } from '../realtime/realtime.module'; +import { ReservationsReadModule } from '../reservations/reservations-read.module'; +import { AirtrailClient } from './airtrail.client'; +import { AirtrailLinkService } from './airtrail-link.service'; +import { AirtrailService } from './airtrail.service'; + +/** + * The AirTrail pieces ReservationsModule may inject: the HTTP client, the + * credential/settings service and the link lifecycle (incl. the write-back + * trigger the reservations controller fires). Deliberately does NOT import + * ReservationsModule β€” reservation reads go through the leaf + * ReservationsReadModule β€” so ReservationsModule β†’ AirtrailCoreModule carries + * no cycle. The pull (AirtrailSyncService), which does need + * ReservationsService, lives one level up in AirtrailModule. + */ +@Module({ + imports: [AuditModule, AddonsModule, RealtimeModule, ReservationsReadModule], + providers: [AirtrailClient, AirtrailService, AirtrailLinkService], + exports: [AirtrailClient, AirtrailService, AirtrailLinkService], +}) +export class AirtrailCoreModule {} diff --git a/server/src/nest/integrations/airtrail-link.service.ts b/server/src/nest/integrations/airtrail-link.service.ts new file mode 100644 index 000000000..1312f7201 --- /dev/null +++ b/server/src/nest/integrations/airtrail-link.service.ts @@ -0,0 +1,150 @@ +import { Injectable } from '@nestjs/common'; +import { ADDON_IDS } from '../../addons'; +import { DatabaseService } from '../database/database.service'; +import { RealtimeService } from '../realtime/realtime.service'; +import { AddonsService } from '../addons/addons.service'; +import { ReservationsReadRepository } from '../reservations/reservations-read.repository'; +import { logError } from '../audit/audit-log.logger'; +import { AirtrailAuthError, type AirtrailCreds, type AirtrailFlightRaw } from './airtrail.client'; +import { AirtrailClient } from './airtrail.client'; +import { AirtrailService } from './airtrail.service'; +import { canonicalHash } from './airtrail.mapper'; +import { buildSavePayload } from './airtrail-sync.helpers'; + +/** + * The AirTrail link lifecycle β€” the enablement gate, the detach policy, the + * multi-leg guard (#1535) and the TREK β†’ AirTrail write-back (#1240) β€” split + * out of AirtrailSyncService so ReservationsModule can inject it: it reads + * reservations through the leaf ReservationsReadRepository, never through + * ReservationsService, so AirtrailCoreModule stays off the + * ReservationsModule β†’ AirtrailModule β†’ ReservationsModule loop that used to + * force airtrail.bridge. The pull half stays in AirtrailSyncService, which + * genuinely needs ReservationsService (remote changes apply through the real + * update path) and delegates these shared pieces back here. + */ +@Injectable() +export class AirtrailLinkService { + constructor( + private readonly db: DatabaseService, + private readonly realtime: RealtimeService, + private readonly addons: AddonsService, + private readonly reads: ReservationsReadRepository, + private readonly client: AirtrailClient, + private readonly airtrail: AirtrailService, + ) {} + + /** Global on/off: the addon must be enabled and sync not explicitly turned off. */ + syncGloballyEnabled(): boolean { + if (!this.addons.isAddonEnabled(ADDON_IDS.AIRTRAIL)) return false; + const row = this.db.get<{ value: string }>("SELECT value FROM app_settings WHERE key = 'airtrail_sync_enabled'"); + return row?.value !== 'false'; + } + + broadcastUpdated(tripId: number, reservationId: number): void { + try { + const reservation = this.reads.getReservationWithJoins(reservationId); + if (reservation) this.realtime.broadcast(String(tripId), 'reservation:updated', { reservation } as never, undefined); + } catch { + /* broadcast failure is non-fatal */ + } + } + + detach(tripId: number, reservationId: number): void { + this.db.run('UPDATE reservations SET sync_enabled = 0 WHERE id = ?', reservationId); + this.broadcastUpdated(tripId, reservationId); + } + + /** + * True when the reservation has grown into a multi-leg booking locally (extra + * stops / metadata.legs) β€” a shape the single AirTrail flight it is linked to + * cannot represent. Syncing such a row in either direction would corrupt one + * side: a pull flattens the layover chain back to fromβ†’to, a push rewrites the + * AirTrail flight to span the whole route (#1535). + */ + hasLocalMultiLegShape(reservationId: number, metadataJson: string | null | undefined): boolean { + try { + const meta = metadataJson ? JSON.parse(metadataJson) : {}; + if (Array.isArray(meta?.legs) && meta.legs.length > 1) return true; + } catch { + /* malformed metadata β€” fall through to the endpoint count */ + } + const row = this.db.get<{ n: number }>( + 'SELECT COUNT(*) AS n FROM reservation_endpoints WHERE reservation_id = ?', + reservationId, + ) as { n: number }; + return row.n > 2; + } + + /** + * Push a locally-edited linked reservation back to AirTrail using the importer's + * (owner's) credentials β€” even if a different member made the edit. If the owner + * is gone or the flight no longer exists in AirTrail, the link is detached so the + * next pull's AirTrail-wins policy can't silently revert the local edit. + */ + async pushReservationToAirtrail(reservationId: number, tripId: number): Promise { + if (!this.syncGloballyEnabled()) return; + + const row = this.db.get<{ + id: number; trip_id: number; external_id: string; external_owner_user_id: number | null; sync_enabled: number; + }>( + "SELECT id, trip_id, external_id, external_owner_user_id, sync_enabled FROM reservations WHERE id = ? AND external_source = 'airtrail'", + reservationId, + ); + if (!row || !row.sync_enabled) return; + + // An edit that turned this linked flight into a multi-leg booking severs the + // 1:1 mapping to the AirTrail flight: pushing would rewrite that flight to the + // full span, and the next pull would flatten the layover again. Detach β€” the + // merge is a deliberate local restructuring, like a joined import (#1535). + const reservation = this.reads.getReservationWithJoins(row.id); + if (!reservation) return; + if (this.hasLocalMultiLegShape(row.id, (reservation as { metadata?: string | null }).metadata)) { + this.detach(tripId, row.id); + return; + } + + // AirTrail is read-only by default (#1240). Only push when the flight's owner has + // explicitly opted in. A no-op skip (not a detach): the link stays active so the + // inbound, AirTrail-wins pull keeps the reservation up to date. + if (!row.external_owner_user_id || !this.airtrail.isAirtrailWriteEnabled(row.external_owner_user_id)) return; + + const creds: AirtrailCreds | null = this.airtrail.getAirtrailCredentials(row.external_owner_user_id); + if (!creds) { + this.detach(tripId, row.id); // owner disconnected β€” cannot push, so stop syncing + return; + } + + let existing: AirtrailFlightRaw | null; + try { + existing = await this.client.getFlight(creds, Number(row.external_id)); + } catch (err) { + if (err instanceof AirtrailAuthError) this.detach(tripId, row.id); + else logError(`AirTrail push: get failed for reservation ${row.id}: ${err instanceof Error ? err.message : err}`); + return; + } + if (!existing) { + this.detach(tripId, row.id); // gone in AirTrail β†’ treat like a remote delete + return; + } + + const payload = buildSavePayload(reservation, existing); + if (!payload) return; + + try { + await this.client.saveFlight(creds, payload); + // Self-write suppression: re-read the saved flight and store its hash so the + // next poll doesn't treat our own write as an inbound change. + const saved = await this.client.getFlight(creds, Number(row.external_id)); + if (saved) { + this.db.run( + 'UPDATE reservations SET external_hash = ?, external_synced_at = ? WHERE id = ?', + canonicalHash(saved), + new Date().toISOString(), + row.id, + ); + } + } catch (err) { + logError(`AirTrail push failed for reservation ${row.id}: ${err instanceof Error ? err.message : err}`); + } + } +} diff --git a/server/src/nest/integrations/airtrail-sync.service.ts b/server/src/nest/integrations/airtrail-sync.service.ts index ea6b30ea5..afdf5156f 100644 --- a/server/src/nest/integrations/airtrail-sync.service.ts +++ b/server/src/nest/integrations/airtrail-sync.service.ts @@ -1,36 +1,34 @@ import { Injectable } from '@nestjs/common'; -import { ADDON_IDS } from '../../addons'; import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { AddonsService } from '../addons/addons.service'; import { ReservationsService } from '../reservations/reservations.service'; import { logError, logInfo } from '../audit/audit-log.logger'; -import { AirtrailAuthError, type AirtrailCreds, type AirtrailFlightRaw } from './airtrail.client'; +import { AirtrailAuthError, type AirtrailFlightRaw } from './airtrail.client'; import { AirtrailClient } from './airtrail.client'; import { AirtrailService } from './airtrail.service'; +import { AirtrailLinkService } from './airtrail-link.service'; import { canonicalHash, mapFlightToReservation } from './airtrail.mapper'; -import { buildSavePayload } from './airtrail-sync.helpers'; export { buildSavePayload } from './airtrail-sync.helpers'; /** - * Two-way AirTrail sync: the background pull that reconciles linked reservations - * against the owner's current flights, and the push that writes a local edit - * back. + * The AirTrail β†’ TREK pull: the background poll that reconciles linked + * reservations against the owner's current flights. Remote changes apply + * through the REAL reservation update path (endpoint restamping and all), + * which is why this service injects ReservationsService and lives in + * AirtrailModule. The shared link lifecycle β€” enablement gate, detach policy, + * multi-leg guard β€” and the TREK β†’ AirTrail push moved to AirtrailLinkService + * (AirtrailCoreModule) so the reservations controller can inject the write-back + * trigger without a module cycle; this class delegates to it. * - * Folded out of services/airtrail/airtrailSync.ts. Every rule is unchanged β€” the - * snapshot-hash change detection, the detach-instead-of-delete policy, the - * multi-leg guard (#1535), the write opt-in (#1240) and the self-write - * suppression. What moved is the wiring: the db singleton, the raw websocket - * broadcast and three bridges (addons, reservations) were module imports and are - * injected now. + * Folded out of services/airtrail/airtrailSync.ts. Every rule is unchanged β€” + * the snapshot-hash change detection, the detach-instead-of-delete policy, the + * multi-leg guard (#1535) and the self-write suppression. */ @Injectable() export class AirtrailSyncService { constructor( private readonly db: DatabaseService, - private readonly realtime: RealtimeService, - private readonly addons: AddonsService, + private readonly link: AirtrailLinkService, private readonly reservations: ReservationsService, private readonly client: AirtrailClient, private readonly airtrail: AirtrailService, @@ -40,46 +38,8 @@ export class AirtrailSyncService { * than a module-level flag, because the service is a container singleton. */ private running = false; - /** Global on/off: the addon must be enabled and sync not explicitly turned off. */ syncGloballyEnabled(): boolean { - if (!this.addons.isAddonEnabled(ADDON_IDS.AIRTRAIL)) return false; - const row = this.db.get<{ value: string }>("SELECT value FROM app_settings WHERE key = 'airtrail_sync_enabled'"); - return row?.value !== 'false'; - } - - private broadcastUpdated(tripId: number, reservationId: number): void { - try { - const reservation = this.reservations.getReservationWithJoins(reservationId); - if (reservation) this.realtime.broadcast(String(tripId), 'reservation:updated', { reservation } as never, undefined); - } catch { - /* broadcast failure is non-fatal */ - } - } - - private detach(tripId: number, reservationId: number): void { - this.db.run('UPDATE reservations SET sync_enabled = 0 WHERE id = ?', reservationId); - this.broadcastUpdated(tripId, reservationId); - } - - /** - * True when the reservation has grown into a multi-leg booking locally (extra - * stops / metadata.legs) β€” a shape the single AirTrail flight it is linked to - * cannot represent. Syncing such a row in either direction would corrupt one - * side: a pull flattens the layover chain back to fromβ†’to, a push rewrites the - * AirTrail flight to span the whole route (#1535). - */ - private hasLocalMultiLegShape(reservationId: number, metadataJson: string | null | undefined): boolean { - try { - const meta = metadataJson ? JSON.parse(metadataJson) : {}; - if (Array.isArray(meta?.legs) && meta.legs.length > 1) return true; - } catch { - /* malformed metadata β€” fall through to the endpoint count */ - } - const row = this.db.get<{ n: number }>( - 'SELECT COUNT(*) AS n FROM reservation_endpoints WHERE reservation_id = ?', - reservationId, - ) as { n: number }; - return row.n > 2; + return this.link.syncGloballyEnabled(); } // ── AirTrail β†’ TREK (poll) ─────────────────────────────────────────────────── @@ -113,7 +73,7 @@ export class AirtrailSyncService { for (const row of linked) { const flight = byId.get(String(row.external_id)); if (!flight) { - this.detach(row.trip_id, row.id); // deleted in AirTrail β†’ keep row, stop syncing + this.link.detach(row.trip_id, row.id); // deleted in AirTrail β†’ keep row, stop syncing changed++; continue; } @@ -123,10 +83,10 @@ export class AirtrailSyncService { const current = this.reservations.getReservation(row.id, row.trip_id); if (!current) continue; - if (this.hasLocalMultiLegShape(row.id, (current as any).metadata)) { + if (this.link.hasLocalMultiLegShape(row.id, (current as any).metadata)) { // The user connected this flight into a multi-leg booking; applying the // remote single-flight shape would flatten it. Stop syncing instead. - this.detach(row.trip_id, row.id); + this.link.detach(row.trip_id, row.id); changed++; continue; } @@ -138,7 +98,7 @@ export class AirtrailSyncService { new Date().toISOString(), row.id, ); - this.broadcastUpdated(row.trip_id, row.id); + this.link.broadcastUpdated(row.trip_id, row.id); changed++; } catch (err) { logError(`AirTrail sync: failed to update reservation ${row.id}: ${err instanceof Error ? err.message : err}`); @@ -150,7 +110,7 @@ export class AirtrailSyncService { /** Background poll across every connected owner (scheduler). */ async runAirtrailSync(): Promise { if (this.running) return; - if (!this.syncGloballyEnabled()) return; + if (!this.link.syncGloballyEnabled()) return; this.running = true; let changed = 0; try { @@ -172,7 +132,7 @@ export class AirtrailSyncService { * background poll. */ async runAirtrailSyncForUser(userId: number): Promise<{ changed: number }> { - if (!this.syncGloballyEnabled()) return { changed: 0 }; + if (!this.link.syncGloballyEnabled()) return { changed: 0 }; try { return { changed: await this.syncOwner(userId) }; } catch (err) { @@ -180,79 +140,4 @@ export class AirtrailSyncService { return { changed: 0 }; } } - - // ── TREK β†’ AirTrail (push) ─────────────────────────────────────────────────── - - /** - * Push a locally-edited linked reservation back to AirTrail using the importer's - * (owner's) credentials β€” even if a different member made the edit. If the owner - * is gone or the flight no longer exists in AirTrail, the link is detached so the - * next pull's AirTrail-wins policy can't silently revert the local edit. - */ - async pushReservationToAirtrail(reservationId: number, tripId: number): Promise { - if (!this.syncGloballyEnabled()) return; - - const row = this.db.get<{ - id: number; trip_id: number; external_id: string; external_owner_user_id: number | null; sync_enabled: number; - }>( - "SELECT id, trip_id, external_id, external_owner_user_id, sync_enabled FROM reservations WHERE id = ? AND external_source = 'airtrail'", - reservationId, - ); - if (!row || !row.sync_enabled) return; - - // An edit that turned this linked flight into a multi-leg booking severs the - // 1:1 mapping to the AirTrail flight: pushing would rewrite that flight to the - // full span, and the next pull would flatten the layover again. Detach β€” the - // merge is a deliberate local restructuring, like a joined import (#1535). - const reservation = this.reservations.getReservationWithJoins(row.id); - if (!reservation) return; - if (this.hasLocalMultiLegShape(row.id, (reservation as { metadata?: string | null }).metadata)) { - this.detach(tripId, row.id); - return; - } - - // AirTrail is read-only by default (#1240). Only push when the flight's owner has - // explicitly opted in. A no-op skip (not a detach): the link stays active so the - // inbound, AirTrail-wins pull keeps the reservation up to date. - if (!row.external_owner_user_id || !this.airtrail.isAirtrailWriteEnabled(row.external_owner_user_id)) return; - - const creds: AirtrailCreds | null = this.airtrail.getAirtrailCredentials(row.external_owner_user_id); - if (!creds) { - this.detach(tripId, row.id); // owner disconnected β€” cannot push, so stop syncing - return; - } - - let existing: AirtrailFlightRaw | null; - try { - existing = await this.client.getFlight(creds, Number(row.external_id)); - } catch (err) { - if (err instanceof AirtrailAuthError) this.detach(tripId, row.id); - else logError(`AirTrail push: get failed for reservation ${row.id}: ${err instanceof Error ? err.message : err}`); - return; - } - if (!existing) { - this.detach(tripId, row.id); // gone in AirTrail β†’ treat like a remote delete - return; - } - - const payload = buildSavePayload(reservation, existing); - if (!payload) return; - - try { - await this.client.saveFlight(creds, payload); - // Self-write suppression: re-read the saved flight and store its hash so the - // next poll doesn't treat our own write as an inbound change. - const saved = await this.client.getFlight(creds, Number(row.external_id)); - if (saved) { - this.db.run( - 'UPDATE reservations SET external_hash = ?, external_synced_at = ? WHERE id = ?', - canonicalHash(saved), - new Date().toISOString(), - row.id, - ); - } - } catch (err) { - logError(`AirTrail push failed for reservation ${row.id}: ${err instanceof Error ? err.message : err}`); - } - } } diff --git a/server/src/nest/integrations/airtrail.bridge.ts b/server/src/nest/integrations/airtrail.bridge.ts deleted file mode 100644 index 0632060a7..000000000 --- a/server/src/nest/integrations/airtrail.bridge.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { PermissionsService } from '../permissions/permissions.service'; -import { AddonsService } from '../addons/addons.service'; -import { AuditService } from '../audit/audit.service'; -import { BudgetService } from '../budget/budget.service'; -import { ExchangeRatesService } from '../budget/exchange-rates.service'; -import { ReservationsService } from '../reservations/reservations.service'; -import { AirtrailClient } from './airtrail.client'; -import { AirtrailService } from './airtrail.service'; -import { AirtrailSyncService } from './airtrail-sync.service'; -import { notificationsInstance } from '../notifications/notifications.instance'; - -/** - * The ONE airtrail edge that cannot be an injection, and why. - * - * `AirtrailSyncService` needs `ReservationsService`: the pull applies remote - * changes through the real reservation update path, endpoint restamping and all, - * and reimplementing that against raw SQL would be strictly worse than the - * coupling. So `AirtrailModule` imports `ReservationsModule`. - * - * The write-back trigger runs in the other direction β€” `reservations.controller` - * fires it after a local edit β€” and injecting `AirtrailSyncService` there would - * close `ReservationsModule -> AirtrailModule -> ReservationsModule`. A - * `forwardRef` would compile and is exactly the shape this migration has been - * removing, so it stays a seam instead, documented rather than accidental. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton, the same pattern the other bridges use. - */ -const dbs = () => new DatabaseService(db); -const realtime = new RealtimeService(); -const permissions = new PermissionsService(dbs()); -const client = new AirtrailClient(); -const airtrail = new AirtrailService(dbs(), new AuditService(dbs()), client); -const reservations = new ReservationsService( - dbs(), - permissions, - new BudgetService(dbs(), permissions, new ExchangeRatesService(), realtime), - realtime, - notificationsInstance(), -); -const sync = new AirtrailSyncService( - dbs(), - realtime, - new AddonsService(dbs()), - reservations, - client, - airtrail, -); - -/** Fire-and-forget write-back after a local edit to a linked flight (#1240). */ -export function pushReservationToAirtrail(reservationId: number, tripId: number): Promise { - return sync.pushReservationToAirtrail(reservationId, tripId); -} diff --git a/server/src/nest/integrations/airtrail.module.ts b/server/src/nest/integrations/airtrail.module.ts index 0a6c73c9f..19ed7bd90 100644 --- a/server/src/nest/integrations/airtrail.module.ts +++ b/server/src/nest/integrations/airtrail.module.ts @@ -1,7 +1,6 @@ import { Module } from '@nestjs/common'; import { AirtrailController } from './airtrail.controller'; -import { AirtrailClient } from './airtrail.client'; -import { AirtrailService } from './airtrail.service'; +import { AirtrailCoreModule } from './airtrail-core.module'; import { AirtrailSyncService } from './airtrail-sync.service'; import { AirtrailSyncJob } from './airtrail-sync.job'; import { AirtrailImportService } from './airtrail-import.service'; @@ -17,18 +16,17 @@ import { ReservationsModule } from '../reservations/reservations.module'; * /api/trips/:tripId/reservations/import/airtrail. * * The logic used to be plain functions over the better-sqlite3 singleton in - * services/airtrail/*, the last thing left in that directory. It is four - * providers now: the HTTP client, the credential/settings service, the two-way - * sync and the importer. - * - * It imports ReservationsModule because the pull applies remote changes through - * the real reservation update path. The trigger in the other direction stays a - * seam on purpose - see airtrail.bridge.ts. + * services/airtrail/*, the last thing left in that directory. The client, the + * credential/settings service and the link lifecycle (incl. the write-back + * push) live in AirtrailCoreModule so ReservationsModule can inject them; + * this module holds what genuinely needs ReservationsService β€” the pull + * (remote changes apply through the real reservation update path), its cron + * and the importer. That split is what retired airtrail.bridge. */ @Module({ - imports: [PermissionsModule, AddonsModule, AuditModule, ReservationsModule, SchedulingModule], + imports: [AirtrailCoreModule, PermissionsModule, AddonsModule, AuditModule, ReservationsModule, SchedulingModule], controllers: [AirtrailController], - providers: [AirtrailClient, AirtrailService, AirtrailSyncService, AirtrailSyncJob, AirtrailImportService], + providers: [AirtrailSyncService, AirtrailSyncJob, AirtrailImportService], exports: [AirtrailSyncService, AirtrailImportService], }) export class AirtrailModule {} diff --git a/server/src/nest/notifications/channel-registry.ts b/server/src/nest/notifications/channel-registry.ts index 50712c1b5..42577778e 100644 --- a/server/src/nest/notifications/channel-registry.ts +++ b/server/src/nest/notifications/channel-registry.ts @@ -3,14 +3,16 @@ import type { ExternalChannel } from './notification-events'; /** * The live set of notification channels. * - * Deliberately a module singleton and not a provider. Two things read it from - * outside the container: notifications.instance.ts hands its own - * NotificationsService to the surviving cycle-dodge bridges - * (trips/reservations/airtrail/packing), and PluginRuntimeService pushes its - * channel getter in at onModuleInit. Make this a provider and that instance + * Deliberately a module singleton and not a provider: PluginRuntimeService + * pushes its channel getter in at onModuleInit, and every + * separately-constructed NotificationsService (the no-Nest test harnesses; + * CHOVR-015 pins this) must see it. Make this a provider and such an instance * gets a second, empty registry - plugin channels would then go quiet with no * error anywhere. The built-ins are registered from NotificationsService's - * constructor, so every path that can dispatch has them. + * constructor, so every path that can dispatch has them. (The last production + * out-of-container consumer, notifications.instance.ts, died with the bridge + * fold - a provider registry is thinkable now, but the harnesses still hang + * off the module scope.) */ /** Namespace for plugin channel ids, so a plugin can never claim `email`. */ diff --git a/server/src/nest/notifications/channels/builtins.ts b/server/src/nest/notifications/channels/builtins.ts index aa5573f99..d55e13d9d 100644 --- a/server/src/nest/notifications/channels/builtins.ts +++ b/server/src/nest/notifications/channels/builtins.ts @@ -24,8 +24,8 @@ export interface BuiltinChannelDeps { * * They take their dependencies as an argument rather than importing them, * because the registry they land in is a module singleton while the transports - * are providers. Both NotificationsService instances - the container singleton - * and the one notifications.instance.ts constructs - register from their + * are providers. Every NotificationsService instance - the container singleton + * and any separately-constructed one - registers from its * constructor, so the registry is populated on every path that can dispatch. * It used to happen through a side-effect import in the preferences service, * which meant the built-ins existed only because somebody happened to import diff --git a/server/src/nest/notifications/notifications.instance.ts b/server/src/nest/notifications/notifications.instance.ts deleted file mode 100644 index 938e262df..000000000 --- a/server/src/nest/notifications/notifications.instance.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { db } from '../../db/database'; -import { DatabaseService } from '../database/database.service'; -import { RealtimeService } from '../realtime/realtime.service'; -import { MailerService } from './mailer/mailer.service'; -import { NotificationPreferencesService } from './notification-preferences.service'; -import { NtfyService } from './transports/ntfy.service'; -import { WebhookService } from './transports/webhook.service'; -import { NotificationsService } from './notifications.service'; - -/** - * The out-of-container NotificationsService, for the bridges that hand-build - * their collaborators (airtrail.bridge is the last one β€” trips.bridge died - * 2026-08-11; this file lives exactly as long as it does). The reminder - * crons that used to reach it through a dedicated send bridge inject the - * container singleton now (ReminderJobsService); that bridge is gone. - * - * One instance, module-scoped, because the channel registry is: the built-ins - * registered in the constructor and the channels the plugin runtime pushes in - * have to be visible on every path. A second copy would silence plugin channels - * for whichever caller got it. - * - * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto - * the shared better-sqlite3 singleton. - */ -const dbs = new DatabaseService(db); -const mailer = new MailerService(dbs); -const notifications = new NotificationsService( - dbs, - new RealtimeService(), - mailer, - new WebhookService(dbs), - new NtfyService(dbs), - new NotificationPreferencesService(dbs, mailer), -); - -export function notificationsInstance(): NotificationsService { - return notifications; -} diff --git a/server/src/nest/notifications/notifications.service.ts b/server/src/nest/notifications/notifications.service.ts index 06220b509..9f3eec369 100644 --- a/server/src/nest/notifications/notifications.service.ts +++ b/server/src/nest/notifications/notifications.service.ts @@ -293,12 +293,12 @@ function shouldSendToUser( * no longer execute the action twice; a handler failure releases the claim). * The channel transports and the preference matrix are injected providers * since the notifications fold; the channel registry stays a module singleton - * because notifications.instance.ts and the plugin runtime both reach it from - * outside the container. + * so every separately-constructed instance (the no-Nest test harnesses β€” the + * last production out-of-container consumer, notifications.instance.ts, died + * with the bridge fold) sees the channels the plugin runtime pushes in. * - * Non-Nest consumers (the surviving cycle-dodge bridges) go through - * `notifications.instance.ts`; the plugin RPC host injects this service via - * `HostSurfaceRpc`. The reminder crons inject it too (ReminderJobsService). + * Every production consumer injects this service now: the plugin RPC host via + * `HostSurfaceRpc`, the reminder crons via ReminderJobsService. */ @Injectable() export class NotificationsService { @@ -310,10 +310,10 @@ export class NotificationsService { private readonly ntfy: NtfyService, private readonly prefs: NotificationPreferencesService, ) { - // The registry is a module singleton shared with notifications.instance.ts - // and the plugin runtime, so registering from here means every path that can - // dispatch has the built-ins β€” including the instance the surviving bridges - // use, which never runs onModuleInit. registerChannel is an idempotent Map.set. + // The registry is a module singleton shared with the plugin runtime and any + // separately-constructed instance (which never runs onModuleInit), so + // registering from here means every path that can dispatch has the + // built-ins. registerChannel is an idempotent Map.set. registerBuiltinChannels({ mailer, webhook, ntfy }); } diff --git a/server/src/nest/realtime/realtime.gateway.ts b/server/src/nest/realtime/realtime.gateway.ts index 6eb24698b..b9c39d5d1 100644 --- a/server/src/nest/realtime/realtime.gateway.ts +++ b/server/src/nest/realtime/realtime.gateway.ts @@ -34,8 +34,8 @@ const HEARTBEAT_INTERVAL = 30_000; * than an interval nobody stops. * * What did NOT move is the socket registry. It stays module-scoped in - * ws-state.ts because the out-of-container bridges construct - * `new RealtimeService()` and must see the same rooms; see the note there. + * ws-state.ts because out-of-container code (the MCP seam's safeBroadcast, + * auth.bridge's graph) must see the same rooms; see the note there. * * The wire protocol is unchanged, down to the frame names. TrekWsAdapter maps * `{ type }` onto @SubscribeMessage, because the stock adapter dispatches on diff --git a/server/src/nest/realtime/realtime.service.ts b/server/src/nest/realtime/realtime.service.ts index 5ff37275a..57f4c5072 100644 --- a/server/src/nest/realtime/realtime.service.ts +++ b/server/src/nest/realtime/realtime.service.ts @@ -21,8 +21,9 @@ import { broadcast, broadcastToUser, getOnlineUserIds } from '../../websocket'; * the functions at construction, and never dereference an export the * method wasn't asked for β€” many test mocks provide `broadcast` only. * - * The service is deliberately dependency-free so module-level bridge code - * (the airtrail.bridge.ts pattern) can construct it with a bare `new RealtimeService()`. + * The service is deliberately dependency-free so out-of-container code (the + * auth.bridge graph, the no-Nest test harnesses) can construct it with a bare + * `new RealtimeService()`. */ @Injectable() export class RealtimeService { diff --git a/server/src/nest/realtime/ws-state.ts b/server/src/nest/realtime/ws-state.ts index 51cb7b155..2c0627018 100644 --- a/server/src/nest/realtime/ws-state.ts +++ b/server/src/nest/realtime/ws-state.ts @@ -6,12 +6,13 @@ import { User } from '../../types'; * The socket registry: rooms, per-socket identity, and the three fan-out * primitives. * - * MODULE state, not provider state, and that is load-bearing. The - * out-of-container bridges and notifications.instance.ts build `new - * RealtimeService()` by hand, outside the container, because they run outside - * it. If the rooms lived on a provider instance, those broadcasts would go to - * an empty map: no error, no log, just a client that stops updating. Same - * reasoning as the geo throttle cursor and the permissions cache. + * MODULE state, not provider state, and that is load-bearing. Out-of-container + * code still reaches the rooms: the MCP seam's safeBroadcast imports + * src/websocket directly, auth.bridge hand-builds a RealtimeService, and the + * no-Nest test harnesses construct more. If the rooms lived on a provider + * instance, those broadcasts would go to an empty map: no error, no log, just + * a client that stops updating. Same reasoning as the geo throttle cursor and + * the permissions cache. * * The gateway next door owns the connection lifecycle and the handshake and * writes into here. This file deliberately knows nothing about auth. diff --git a/server/src/nest/reservations/reservations-read.module.ts b/server/src/nest/reservations/reservations-read.module.ts new file mode 100644 index 000000000..38716f3de --- /dev/null +++ b/server/src/nest/reservations/reservations-read.module.ts @@ -0,0 +1,13 @@ +import { Module } from '@nestjs/common'; +import { ReservationsReadRepository } from './reservations-read.repository'; + +/** + * A leaf on purpose (the trip-membership precedent): AirtrailCoreModule needs + * the single-reservation hydration reads without dragging ReservationsModule + * in β€” which would re-close the cycle that airtrail.bridge used to dodge. + */ +@Module({ + providers: [ReservationsReadRepository], + exports: [ReservationsReadRepository], +}) +export class ReservationsReadModule {} diff --git a/server/src/nest/reservations/reservations-read.repository.ts b/server/src/nest/reservations/reservations-read.repository.ts new file mode 100644 index 000000000..8c5174a67 --- /dev/null +++ b/server/src/nest/reservations/reservations-read.repository.ts @@ -0,0 +1,62 @@ +import { Injectable } from '@nestjs/common'; +import { DatabaseService } from '../database/database.service'; +import { avatarUrl } from '../common/avatarUrl'; +import type { ReservationRow, ReservationEndpoint, ReservationTraveler } from './reservations.service'; + +/** The one traveler projection every reservation read shares (avatar_url included). */ +export function toTraveler(r: { user_id: number; username: string; avatar: string | null; is_guest?: number | null }): ReservationTraveler { + return { user_id: r.user_id, username: r.username, avatar: r.avatar, is_guest: r.is_guest ?? null, avatar_url: avatarUrl(r) }; +} + +/** + * The single-reservation hydration reads, split out of ReservationsService (the + * trek_photos repository precedent) so consumers outside ReservationsModule can + * reach them without importing the module. The concrete case: the AirTrail + * write-back (AirtrailLinkService) needs the hydrated row for its payload and + * its update broadcast, and injecting ReservationsService there would close + * ReservationsModule β†’ AirtrailCoreModule β†’ ReservationsModule β€” the cycle + * that used to force airtrail.bridge. ReservationsService injects this and + * delegates, so there is exactly one copy of each query. + */ +@Injectable() +export class ReservationsReadRepository { + constructor(private readonly db: DatabaseService) {} + + getReservationWithJoins(id: string | number): ReservationRow | undefined { + const row = this.db.get(` + SELECT r.*, d.day_number, p.name as place_name, r.assignment_id, + ap.place_id as accommodation_place_id, acc_p.name as accommodation_name, + ap.start_day_id as accommodation_start_day_id, ap.end_day_id as accommodation_end_day_id + FROM reservations r + LEFT JOIN days d ON r.day_id = d.id + LEFT JOIN places p ON r.place_id = p.id + LEFT JOIN day_accommodations ap ON r.accommodation_id = ap.id + LEFT JOIN places acc_p ON ap.place_id = acc_p.id + WHERE r.id = ? + `, id); + if (!row) return undefined; + row.endpoints = this.loadEndpoints(row.id); + row.travelers = this.loadTravelers(row.id); + // accommodation_id is a TEXT column; the integer FK reads back as a numeric + // string (e.g. "14.0"). Normalize to an int so clients can parse it. + row.accommodation_id = row.accommodation_id == null ? null : Math.trunc(Number(row.accommodation_id)); + return row; + } + + loadEndpoints(reservationId: number): ReservationEndpoint[] { + return this.db.all( + 'SELECT * FROM reservation_endpoints WHERE reservation_id = ? ORDER BY sequence', + reservationId + ); + } + + loadTravelers(reservationId: number | string): ReservationTraveler[] { + const rows = this.db.all(` + SELECT rt.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar, u.is_guest + FROM reservation_travelers rt + JOIN users u ON rt.user_id = u.id + WHERE rt.reservation_id = ? + `, reservationId); + return rows.map(r => toTraveler(r)); + } +} diff --git a/server/src/nest/reservations/reservations.controller.ts b/server/src/nest/reservations/reservations.controller.ts index 40c8cd053..13b5987b9 100644 --- a/server/src/nest/reservations/reservations.controller.ts +++ b/server/src/nest/reservations/reservations.controller.ts @@ -15,7 +15,7 @@ import { ReservationsService } from './reservations.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; -import { pushReservationToAirtrail } from '../integrations/airtrail.bridge'; +import { AirtrailLinkService } from '../integrations/airtrail-link.service'; import { ReservationCreateDto, ReservationUpdateDto, @@ -46,7 +46,11 @@ type ReservationBody = Record & { // passes, so the HTTP and MCP paths cannot demand different rights. @UseGuards(JwtAuthGuard, TripAccessGuard) export class ReservationsController { - constructor(private readonly reservations: ReservationsService) {} + constructor( + private readonly reservations: ReservationsService, + // Injected from AirtrailCoreModule β€” the split that retired airtrail.bridge. + private readonly airtrailLink: AirtrailLinkService, + ) {} @@ -113,7 +117,7 @@ export class ReservationsController { // Push a locally-edited AirTrail flight back to AirTrail (fire-and-forget, // under the importer's credentials β€” see airtrailSync). #214 if ((reservation as any)?.external_source === 'airtrail' && (reservation as any)?.sync_enabled) { - void pushReservationToAirtrail(Number((reservation as any).id), Number(tripId)).catch(() => {}); + void this.airtrailLink.pushReservationToAirtrail(Number((reservation as any).id), Number(tripId)).catch(() => {}); } this.reservations.notifyBookingChange(tripId, user.id, body.title || cur.title, body.type || cur.type || ''); return { reservation }; diff --git a/server/src/nest/reservations/reservations.module.ts b/server/src/nest/reservations/reservations.module.ts index 434c5388f..4701863e2 100644 --- a/server/src/nest/reservations/reservations.module.ts +++ b/server/src/nest/reservations/reservations.module.ts @@ -1,4 +1,6 @@ import { Module } from '@nestjs/common'; +import { ReservationsReadModule } from './reservations-read.module'; +import { AirtrailCoreModule } from '../integrations/airtrail-core.module'; import { BudgetModule } from '../budget/budget.module'; import { DaysModule } from '../days/days.module'; import { PermissionsModule } from '../permissions/permissions.module'; @@ -22,7 +24,7 @@ import { NotificationsModule } from '../notifications/notifications.module'; @Module({ // DaysModule: ReservationsMcp injects DaysService for its nine getDay calls. // BudgetModule: ReservationsService + ReservationsMcp inject BudgetService (budget-sync seam). - imports: [NotificationsModule, DaysModule, AssignmentsModule, PermissionsModule, BudgetModule, AuthModule, RealtimeModule, PluginGuardsModule], + imports: [NotificationsModule, DaysModule, AssignmentsModule, PermissionsModule, BudgetModule, AuthModule, RealtimeModule, PluginGuardsModule, ReservationsReadModule, AirtrailCoreModule], controllers: [ReservationsController, UpcomingReservationsController], providers: [ReservationsService, ReservationsMcp, ReservationsRpc], // For in-container consumers (ReservationsRpc, TripsService, BookingImportService). diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 33cd14788..9d3b08f57 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -3,7 +3,7 @@ import { DatabaseService, type TripAccess } from '../database/database.service'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; -import { avatarUrl } from '../common/avatarUrl'; +import { ReservationsReadRepository, toTraveler } from './reservations-read.repository'; import type { Reservation, User } from '../../types'; import { BudgetService } from '../budget/budget.service'; import { typeToCostCategory } from '@trek/shared'; @@ -136,6 +136,7 @@ export class ReservationsService { private readonly budget: BudgetService, private readonly realtime: RealtimeService, private readonly notifications: NotificationsService, + private readonly reads: ReservationsReadRepository, ) {} verifyTripAccess(tripId: string | number, userId: number) { @@ -194,17 +195,6 @@ export class ReservationsService { return map; } - private loadEndpoints(reservationId: number): ReservationEndpoint[] { - return this.db.all( - 'SELECT * FROM reservation_endpoints WHERE reservation_id = ? ORDER BY sequence', - reservationId - ); - } - - private toTraveler(r: { user_id: number; username: string; avatar: string | null; is_guest?: number | null }): ReservationTraveler { - return { user_id: r.user_id, username: r.username, avatar: r.avatar, is_guest: r.is_guest ?? null, avatar_url: avatarUrl(r) }; - } - /** Users assignable on a trip: its members (guests included) plus the owner. */ private assignableUserIds(tripId: string | number): Set { const members = this.db.all<{ user_id: number }>('SELECT user_id FROM trip_members WHERE trip_id = ?', tripId); @@ -226,20 +216,14 @@ export class ReservationsService { const map = new Map(); for (const row of rows) { const list = map.get(row.reservation_id) ?? []; - list.push(this.toTraveler(row)); + list.push(toTraveler(row)); map.set(row.reservation_id, list); } return map; } loadTravelers(reservationId: number | string): ReservationTraveler[] { - const rows = this.db.all(` - SELECT rt.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar, u.is_guest - FROM reservation_travelers rt - JOIN users u ON rt.user_id = u.id - WHERE rt.reservation_id = ? - `, reservationId); - return rows.map(r => this.toTraveler(r)); + return this.reads.loadTravelers(reservationId); } /** @@ -430,24 +414,7 @@ export class ReservationsService { } getReservationWithJoins(id: string | number) { - const row = this.db.get(` - SELECT r.*, d.day_number, p.name as place_name, r.assignment_id, - ap.place_id as accommodation_place_id, acc_p.name as accommodation_name, - ap.start_day_id as accommodation_start_day_id, ap.end_day_id as accommodation_end_day_id - FROM reservations r - LEFT JOIN days d ON r.day_id = d.id - LEFT JOIN places p ON r.place_id = p.id - LEFT JOIN day_accommodations ap ON r.accommodation_id = ap.id - LEFT JOIN places acc_p ON ap.place_id = acc_p.id - WHERE r.id = ? - `, id); - if (!row) return undefined; - row.endpoints = this.loadEndpoints(row.id); - row.travelers = this.loadTravelers(row.id); - // accommodation_id is a TEXT column; the integer FK reads back as a numeric - // string (e.g. "14.0"). Normalize to an int so clients can parse it. - row.accommodation_id = row.accommodation_id == null ? null : Math.trunc(Number(row.accommodation_id)); - return row; + return this.reads.getReservationWithJoins(id); } /** The accommodation insert, the reservation insert, the endpoint save and diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index 78deb6afe..3af7e828c 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -41,14 +41,6 @@ vi.mock('../../src/db/database', () => ({ })); vi.mock('../../src/websocket', () => ({ broadcast: vi.fn() })); const { notificationSend } = vi.hoisted(() => ({ notificationSend: vi.fn().mockResolvedValue(undefined) })); -// ReservationsService injects NotificationsService now instead of reaching the -// bridge through a lazy import, so the e2e app resolves the real provider out of -// its own container β€” the module mock above never sees the send. The override -// below is what intercepts it. -vi.mock('../../src/nest/notifications/notifications.instance', () => ({ - notificationsInstance: () => ({ send: notificationSend }), -})); - import { PermissionsService } from '../../src/nest/permissions/permissions.service'; // Since the permissions DI migration, the check is a spy on the container's diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index c5ec91aa7..abfebb25a 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -35,6 +35,7 @@ import { PlacesMcp } from '../../src/nest/places/places.mcp'; import { PlacesService } from '../../src/nest/places/places.service'; import { ReservationsMcp } from '../../src/nest/reservations/reservations.mcp'; import { ReservationsService } from '../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../src/nest/reservations/reservations-read.repository'; import { TagsMcp } from '../../src/nest/tags/tags.mcp'; import { TagsService } from '../../src/nest/tags/tags.service'; import { SettingsService } from '../../src/nest/settings/settings.service'; @@ -120,7 +121,7 @@ export function createMcpTestRegistry(): McpRegistry { placePhotoCache, journeyDomain, ); - const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService, notificationsStub()); + const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService, notificationsStub(), new ReservationsReadRepository(dbService)); const accommodationsService = new AccommodationsService(dbService, permissionsService, realtimeService); const membersService = new TripMembersService(dbService, budgetService, new UserCleanupService(dbService, budgetService), permissionsService, realtimeService, notificationsStub()); const tripsService = new TripsService( diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 19c5721e3..9cd78c18a 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -14,6 +14,7 @@ import { PluginOAuthService } from '../../src/nest/plugins/oauth/plugin-oauth.se import { BudgetService } from '../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../src/nest/budget/exchange-rates.service'; import { ReservationsService } from '../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../src/nest/reservations/reservations-read.repository'; import { TagsService } from '../../src/nest/tags/tags.service'; import { CategoriesService } from '../../src/nest/categories/categories.service'; import { TodoService } from '../../src/nest/todo/todo.service'; @@ -90,7 +91,7 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF const todos = new TodoService(dbs, permissions, realtime); const packing = new PackingService(dbs, permissions, realtime, notificationsStub()); const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService()); - const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub()); + const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs)); const collab = new CollabService(dbs, permissions, realtime, notificationsStub()); const vacay = new VacayService(dbs, realtime, notificationsStub()); const days = new DaysService(dbs, permissions, realtime, queryHelpers); diff --git a/server/tests/unit/nest/calendar.service.test.ts b/server/tests/unit/nest/calendar.service.test.ts index c2d149323..db4c4ed1e 100644 --- a/server/tests/unit/nest/calendar.service.test.ts +++ b/server/tests/unit/nest/calendar.service.test.ts @@ -49,6 +49,7 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; import { CalendarService, foldICS } from '../../../src/nest/calendar/calendar.service'; @@ -62,7 +63,7 @@ const budgetSvc = new BudgetService(dbs(), new PermissionsService(dbs()), new Ex // Named `svc` so the moved cases below read exactly as they did on TripsService. const svc = new CalendarService( dbs(), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), ); beforeAll(() => { diff --git a/server/tests/unit/nest/channel-override.test.ts b/server/tests/unit/nest/channel-override.test.ts index e9d18a1e5..5d180bbee 100644 --- a/server/tests/unit/nest/channel-override.test.ts +++ b/server/tests/unit/nest/channel-override.test.ts @@ -27,13 +27,17 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin, setAppSetting, setNotificationChannels } from '../../helpers/factories'; -// The send dispatcher is DI-native since the notifications fold; drive it -// through the same outside-container entry point the surviving cycle-dodge -// bridges (trips/reservations/airtrail/packing) use. -import { notificationsInstance } from '../../../src/nest/notifications/notifications.instance'; +// The send dispatcher is DI-native since the notifications fold +// (notifications.instance.ts died with the last cycle-dodge bridge); a +// hand-constructed instance still shares the module-scoped channel registry, +// which is exactly what CHOVR-015 pins. import type { NotificationPayload } from '../../../src/nest/notifications/notifications.service'; +import { makeNotificationsService } from '../../helpers/notifications'; -const send = (payload: NotificationPayload) => notificationsInstance().send(payload); +// One instance, built at module load like the old import-time singleton: its +// constructor is what registers the built-in channels the registry cases read. +const notifications = makeNotificationsService(new DatabaseService(testDb)); +const send = (payload: NotificationPayload) => notifications.send(payload); import { NtfyService } from '../../../src/nest/notifications/transports/ntfy.service'; import { WebhookService } from '../../../src/nest/notifications/transports/webhook.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; @@ -214,16 +218,15 @@ describe('a live plugin channel needs no second opt-in', () => { * The seam itself, end to end. * * PluginRuntimeService pushes its channel getter into the registry at - * onModuleInit; notifications.instance.ts constructs its own - * NotificationsService for the surviving cycle-dodge bridges - * (trips/reservations/airtrail/packing). Both only meet because the registry is + * onModuleInit; a separately-constructed NotificationsService (as every + * no-Nest harness builds) only sees it because the registry is * a module singleton. Make it a provider and that instance gets an empty one β€” * plugin channels then go quiet with no error anywhere, which is exactly the * kind of failure nothing else here would catch. Every case above sets the * source by hand; this one goes through the runtime. */ describe('the plugin channel source reaches the outside-container instance', () => { - it('CHOVR-015 β€” a channel the runtime publishes is delivered by notificationsInstance().send', async () => { + it('CHOVR-015 β€” a channel the runtime publishes is delivered by a separately built instance', async () => { const { user } = createUser(testDb, { username: 'recipient' }); setNotificationChannels(testDb, 'none'); const delivered: Array<{ userId: number; title: string }> = []; diff --git a/server/tests/unit/nest/notifications.service.test.ts b/server/tests/unit/nest/notifications.service.test.ts index 38d49f906..47ffab34b 100644 --- a/server/tests/unit/nest/notifications.service.test.ts +++ b/server/tests/unit/nest/notifications.service.test.ts @@ -76,7 +76,6 @@ import { createUser, createAdmin, setAppSetting, setNotificationChannels, disabl import { DatabaseService } from '../../../src/nest/database/database.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { NotificationsService, type NotificationPayload } from '../../../src/nest/notifications/notifications.service'; -import { notificationsInstance } from '../../../src/nest/notifications/notifications.instance'; import { setPluginChannelSource } from '../../../src/nest/notifications/channel-registry'; // The channel interface lives in notification-events; channel-registry only imports // it for its own signatures and never re-exported it. @@ -718,21 +717,5 @@ describe('send() β€” plugin notification channels', () => { }); }); -// ───────────────────────────────────────────────────────────────────────────── -// Outside-container delegation (notifications.instance.ts β€” the entry point the -// surviving cycle-dodge bridges use; its old send bridge died with the -// reminder-cron migration) -// ───────────────────────────────────────────────────────────────────────────── - -describe('notifications.instance', () => { - it('NSVC-020 β€” send delegates to the DI service over the shared db proxy', async () => { - const { user } = createUser(testDb); - setNotificationChannels(testDb, 'none'); - - await notificationsInstance().send({ event: 'trip_invite', actorId: null, scope: 'user', targetId: user.id, params: { trip: 'Lisbon', actor: 'Alice', invitee: 'Bob', tripId: '7' } }); - - expect(countAllNotifications()).toBe(1); - expect(broadcastMock).toHaveBeenCalledTimes(1); - expect(getInAppNotifications(user.id)[0].navigate_target).toBe('/trips/7'); - }); -}); +// NSVC-020 pinned notifications.instance.ts, which died with the last +// cycle-dodge bridge β€” every consumer injects the container singleton now. diff --git a/server/tests/unit/nest/reservations.controller.test.ts b/server/tests/unit/nest/reservations.controller.test.ts index 73bede14f..840435c9d 100644 --- a/server/tests/unit/nest/reservations.controller.test.ts +++ b/server/tests/unit/nest/reservations.controller.test.ts @@ -2,11 +2,15 @@ import { describe, it, expect, vi } from 'vitest'; import { HttpException } from '@nestjs/common'; import { ReservationsController } from '../../../src/nest/reservations/reservations.controller'; import type { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import type { AirtrailLinkService } from '../../../src/nest/integrations/airtrail-link.service'; import type { User } from '../../../src/types'; const user = { id: 1, role: 'user', email: 'u@example.test' } as User; const trip = { id: 5, user_id: 1 }; +// Fire-and-forget write-back trigger, injected since airtrail.bridge died. +const airtrailLink = { pushReservationToAirtrail: vi.fn().mockResolvedValue(undefined) } as unknown as AirtrailLinkService; + function makeService(overrides: Partial = {}): ReservationsService { return { verifyTripAccess: vi.fn().mockReturnValue(trip), @@ -32,7 +36,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese it('GET / returns reservations', () => { const svc = makeService({ list: vi.fn().mockReturnValue([{ id: 1 }]) } as Partial); - expect(new ReservationsController(svc).list(user, '5')).toEqual({ reservations: [{ id: 1 }] }); + expect(new ReservationsController(svc, airtrailLink).list(user, '5')).toEqual({ reservations: [{ id: 1 }] }); }); describe('POST /', () => { @@ -45,7 +49,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const broadcast = vi.fn(); const syncBudgetOnCreate = vi.fn(); const notifyBookingChange = vi.fn(); const svc = makeService({ create, broadcast, syncBudgetOnCreate, notifyBookingChange } as Partial); const body = { title: 'Hotel', type: 'lodging', create_budget_entry: { total_price: 200 } }; - expect(new ReservationsController(svc).create(user, '5', body, 'sock')).toEqual({ reservation: { id: 9 } }); + expect(new ReservationsController(svc, airtrailLink).create(user, '5', body, 'sock')).toEqual({ reservation: { id: 9 } }); expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:created', {}, 'sock'); expect(syncBudgetOnCreate).toHaveBeenCalledWith('5', 9, 'Hotel', 'lodging', { total_price: 200 }, 'sock'); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:created', { reservation: { id: 9 } }, 'sock'); @@ -61,7 +65,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const updatePositions = vi.fn(); const broadcast = vi.fn(); const svc = makeService({ updatePositions, broadcast } as Partial); const positions = [{ id: 1, day_plan_position: 0 }]; - expect(new ReservationsController(svc).updatePositions(user, '5', { positions, day_id: 3 }, 'sock')).toEqual({ success: true }); + expect(new ReservationsController(svc, airtrailLink).updatePositions(user, '5', { positions, day_id: 3 }, 'sock')).toEqual({ success: true }); expect(updatePositions).toHaveBeenCalledWith('5', positions, 3); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:positions', { positions, day_id: 3 }, 'sock'); }); @@ -70,7 +74,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese describe('PUT /:id', () => { it('404 when the reservation is missing', () => { const svc = makeService({ getReservation: vi.fn().mockReturnValue(undefined) } as Partial); - expect(thrown(() => new ReservationsController(svc).update(user, '5', '9', { title: 'X' }))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); + expect(thrown(() => new ReservationsController(svc, airtrailLink).update(user, '5', '9', { title: 'X' }))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); }); it('updates, syncs budget with current fallbacks, broadcasts + notifies', () => { @@ -78,7 +82,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const update = vi.fn().mockReturnValue({ reservation: { id: 9 }, accommodationChanged: true }); const broadcast = vi.fn(); const syncBudgetOnUpdate = vi.fn(); const notifyBookingChange = vi.fn(); const svc = makeService({ getReservation, update, broadcast, syncBudgetOnUpdate, notifyBookingChange } as Partial); - new ReservationsController(svc).update(user, '5', '9', { create_budget_entry: { total_price: 50 } }, 'sock'); + new ReservationsController(svc, airtrailLink).update(user, '5', '9', { create_budget_entry: { total_price: 50 } }, 'sock'); expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:updated', {}, 'sock'); expect(syncBudgetOnUpdate).toHaveBeenCalledWith('5', '9', '', undefined, 'Old', 'lodging', { total_price: 50 }, 'sock'); expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Old', 'lodging'); @@ -91,7 +95,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese it('404 when the reservation is off-trip / missing', () => { const svc = makeService({ setTravelers: vi.fn().mockReturnValue(null) } as Partial); - expect(thrown(() => new ReservationsController(svc).updateTravelers(user, '5', '9', { user_ids: [1] }))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); + expect(thrown(() => new ReservationsController(svc, airtrailLink).updateTravelers(user, '5', '9', { user_ids: [1] }))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); }); it('assigns travelers, broadcasts, and returns { travelers, reservation }', () => { @@ -100,7 +104,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const setTravelers = vi.fn().mockReturnValue({ travelers, reservation }); const broadcast = vi.fn(); const svc = makeService({ setTravelers, broadcast } as Partial); - expect(new ReservationsController(svc).updateTravelers(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ travelers, reservation }); + expect(new ReservationsController(svc, airtrailLink).updateTravelers(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ travelers, reservation }); expect(setTravelers).toHaveBeenCalledWith('9', '5', [2]); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:travelers-updated', { reservationId: 9, travelers }, 'sock'); }); @@ -109,14 +113,14 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese describe('DELETE /:id', () => { it('404 when nothing deleted', () => { const svc = makeService({ remove: vi.fn().mockReturnValue({ deleted: undefined, accommodationDeleted: false, deletedBudgetItemId: null }) } as Partial); - expect(thrown(() => new ReservationsController(svc).remove(user, '5', '9'))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); + expect(thrown(() => new ReservationsController(svc, airtrailLink).remove(user, '5', '9'))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); }); it('broadcasts the accommodation + budget cascade then reservation:deleted', () => { const remove = vi.fn().mockReturnValue({ deleted: { id: 9, title: 'Hotel', type: 'lodging', accommodation_id: 3 }, accommodationDeleted: true, deletedBudgetItemId: 7 }); const broadcast = vi.fn(); const notifyBookingChange = vi.fn(); const svc = makeService({ remove, broadcast, notifyBookingChange } as Partial); - expect(new ReservationsController(svc).remove(user, '5', '9', 'sock')).toEqual({ success: true }); + expect(new ReservationsController(svc, airtrailLink).remove(user, '5', '9', 'sock')).toEqual({ success: true }); expect(broadcast).toHaveBeenCalledWith('5', 'accommodation:deleted', { accommodationId: 3 }, 'sock'); expect(broadcast).toHaveBeenCalledWith('5', 'budget:deleted', { itemId: 7 }, 'sock'); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:deleted', { reservationId: 9 }, 'sock'); diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index f63d33db8..68b20d1b0 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -42,12 +42,6 @@ const permissionsStub = { checkPermission } as unknown as PermissionsService; const budget = { createBudgetItem: vi.fn(), updateBudgetItem: vi.fn(), deleteBudgetItem: vi.fn(), linkBudgetItemToReservation: vi.fn() }; const { notif } = vi.hoisted(() => ({ notif: { send: vi.fn().mockResolvedValue(undefined) } })); -// The bridge builds its own collaborators, so its NotificationsService comes -// from notifications.instance rather than from the module above β€” RESV-BRIDGE-009 -// asserts on what the bridge sends, so that is the module to intercept. -vi.mock('../../../src/nest/notifications/notifications.instance', () => ({ - notificationsInstance: () => notif, -})); import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; @@ -56,6 +50,7 @@ import { createUser, createTrip, createReservation, createBudgetItem, createPlac import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; import type { BudgetService } from '../../../src/nest/budget/budget.service'; // Was reservations.bridge, deleted along with the other three that had no // consumer outside the container. The cases below kept their assertions and @@ -80,7 +75,7 @@ const bridge = { import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; -const svc = new ReservationsService(new DatabaseService(testDb), permissionsStub, budget as unknown as BudgetService, new RealtimeService(), notificationsStub(notif.send)); +const svc = new ReservationsService(new DatabaseService(testDb), permissionsStub, budget as unknown as BudgetService, new RealtimeService(), notificationsStub(notif.send), new ReservationsReadRepository(new DatabaseService(testDb))); beforeAll(() => { createTables(testDb); runMigrations(testDb); }); beforeEach(() => { diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index 9c12dd82f..54ea00bdf 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -56,6 +56,7 @@ import { TodoService } from '../../../src/nest/todo/todo.service'; import { PackingService } from '../../../src/nest/packing/packing.service'; import { FilesService } from '../../../src/nest/files/files.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; import { CollabService } from '../../../src/nest/collab/collab.service'; @@ -96,7 +97,7 @@ const buildReadModel = (database: DatabaseService, roster: TripMembersService = new TripReadModelService( database, roster, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 8110f923d..c05ebf75b 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -62,6 +62,7 @@ import { TodoService } from '../../../src/nest/todo/todo.service'; import { PackingService } from '../../../src/nest/packing/packing.service'; import { FilesService } from '../../../src/nest/files/files.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; import { CollabService } from '../../../src/nest/collab/collab.service'; @@ -111,7 +112,7 @@ const createAccommodation = accommodationsSvc.createAccommodation.bind(accommoda const svc = new TripsService( dbs(), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), daysSvc, new PermissionsService(dbs()), budgetSvc, @@ -123,7 +124,7 @@ const membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupServi const readModelSvc = new TripReadModelService( dbs(), membersSvc, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), @@ -918,7 +919,7 @@ describe('quirk fixes', () => { const fdbs = failingConnection(match); return new TripsService( fdbs, - new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub()), + new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), daysSvc, new PermissionsService(dbs()), budgetSvc, diff --git a/server/tests/unit/services/airtrailImport.test.ts b/server/tests/unit/services/airtrailImport.test.ts index ecf436fc8..4c36d5b5a 100644 --- a/server/tests/unit/services/airtrailImport.test.ts +++ b/server/tests/unit/services/airtrailImport.test.ts @@ -18,6 +18,7 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; import type { AirtrailClient } from '../../../src/nest/integrations/airtrail.client'; import type { AirtrailService } from '../../../src/nest/integrations/airtrail.service'; import { notificationsStub } from '../../helpers/notifications'; @@ -41,6 +42,7 @@ function makeImportService(): AirtrailImportService { new BudgetService(dbs(), permissions, new ExchangeRatesService(), realtime), realtime, notificationsStub(), + new ReservationsReadRepository(dbs()), ), { listFlights } as unknown as AirtrailClient, { diff --git a/server/tests/unit/services/airtrailWriteGate.test.ts b/server/tests/unit/services/airtrailWriteGate.test.ts index 8961fd9b6..1bfd2ed9b 100644 --- a/server/tests/unit/services/airtrailWriteGate.test.ts +++ b/server/tests/unit/services/airtrailWriteGate.test.ts @@ -16,12 +16,14 @@ vi.mock('../../../src/nest/integrations/airtrail.mapper', () => ({ entityCode: (e: any) => e?.icao || e?.iata || null, })); -import { AirtrailSyncService } from '../../../src/nest/integrations/airtrail-sync.service'; +import { AirtrailLinkService } from '../../../src/nest/integrations/airtrail-link.service'; import { AirtrailAuthError } from '../../../src/nest/integrations/airtrail.client'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import type { AddonsService } from '../../../src/nest/addons/addons.service'; +import { AirtrailSyncService } from '../../../src/nest/integrations/airtrail-sync.service'; import type { ReservationsService } from '../../../src/nest/reservations/reservations.service'; +import type { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; import type { AirtrailClient } from '../../../src/nest/integrations/airtrail.client'; import type { AirtrailService } from '../../../src/nest/integrations/airtrail.service'; @@ -41,7 +43,7 @@ const getAirtrailCredentials = vi.fn(); let dbGet: (sql: string) => unknown; let dbAll: (sql: string) => unknown[]; -function makeService(): AirtrailSyncService { +function makeServices(): { link: AirtrailLinkService; sync: AirtrailSyncService } { const db = { get: (sql: string) => dbGet(sql), all: (sql: string) => dbAll(sql), @@ -51,17 +53,29 @@ function makeService(): AirtrailSyncService { }, } as unknown as DatabaseService; - return new AirtrailSyncService( + const client = { getFlight, listFlights, saveFlight } as unknown as AirtrailClient; + const airtrail = { isAirtrailWriteEnabled, getAirtrailCredentials } as unknown as AirtrailService; + // The push and the shared link lifecycle live on AirtrailLinkService since the + // core/pull split (which retired airtrail.bridge); the pull delegates to it. + const link = new AirtrailLinkService( db, { broadcast: vi.fn() } as unknown as RealtimeService, { isAddonEnabled: vi.fn(() => true) } as unknown as AddonsService, - { getReservation, getReservationWithJoins, update: updateReservation } as unknown as ReservationsService, - { getFlight, listFlights, saveFlight } as unknown as AirtrailClient, - { isAirtrailWriteEnabled, getAirtrailCredentials } as unknown as AirtrailService, + { getReservationWithJoins } as unknown as ReservationsReadRepository, + client, + airtrail, ); + const sync = new AirtrailSyncService( + db, + link, + { getReservation, update: updateReservation } as unknown as ReservationsService, + client, + airtrail, + ); + return { link, sync }; } -let svc: AirtrailSyncService; +let svc: { link: AirtrailLinkService; sync: AirtrailSyncService }; beforeEach(() => { vi.clearAllMocks(); @@ -74,7 +88,7 @@ beforeEach(() => { return undefined; }; dbAll = () => []; - svc = makeService(); + svc = makeServices(); getAirtrailCredentials.mockReturnValue({ baseUrl: 'https://at.example', apiKey: 'k', allowInsecureTls: false }); // GET returns AirTrail-owned detail TREK doesn't model β€” must survive the writeback. @@ -96,7 +110,7 @@ beforeEach(() => { describe('pushReservationToAirtrail write gate (#1240)', () => { it('does nothing β€” and does not detach β€” when the owner has not opted in', async () => { isAirtrailWriteEnabled.mockReturnValue(false); - await svc.pushReservationToAirtrail(5, 9); + await svc.link.pushReservationToAirtrail(5, 9); expect(getFlight).not.toHaveBeenCalled(); expect(saveFlight).not.toHaveBeenCalled(); expect(runSpy).not.toHaveBeenCalled(); // no detach, no hash write β€” pure no-op @@ -104,7 +118,7 @@ describe('pushReservationToAirtrail write gate (#1240)', () => { it('writes back, preserving AirTrail-owned fields, when the owner has opted in', async () => { isAirtrailWriteEnabled.mockReturnValue(true); - await svc.pushReservationToAirtrail(5, 9); + await svc.link.pushReservationToAirtrail(5, 9); expect(saveFlight).toHaveBeenCalledTimes(1); const payload = saveFlight.mock.calls[0][1] as Record; expect(payload.departureTerminal).toBe('7'); // spread preserved the unmanaged field @@ -119,7 +133,7 @@ describe('pushReservationToAirtrail write gate (#1240)', () => { if (sql.includes('FROM reservations')) return { ...linkedRow }; return undefined; }; - await svc.pushReservationToAirtrail(5, 9); + await svc.link.pushReservationToAirtrail(5, 9); // Pushing would rewrite the single AirTrail flight to span the whole route. expect(saveFlight).not.toHaveBeenCalled(); expect(runSpy).toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), [5]); @@ -133,7 +147,7 @@ describe('pushReservationToAirtrail write gate (#1240)', () => { metadata: JSON.stringify({ legs: [{ from: 'BRU', to: 'HEL' }, { from: 'HEL', to: 'JFK' }] }), endpoints: [], }); - await svc.pushReservationToAirtrail(5, 9); + await svc.link.pushReservationToAirtrail(5, 9); expect(saveFlight).not.toHaveBeenCalled(); expect(runSpy).toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), [5]); }); @@ -141,7 +155,7 @@ describe('pushReservationToAirtrail write gate (#1240)', () => { it('detaches when the owner key stopped working, rather than retrying forever', async () => { isAirtrailWriteEnabled.mockReturnValue(true); getFlight.mockRejectedValue(new AirtrailAuthError('invalid key')); - await svc.pushReservationToAirtrail(5, 9); + await svc.link.pushReservationToAirtrail(5, 9); expect(saveFlight).not.toHaveBeenCalled(); expect(runSpy).toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), [5]); }); @@ -149,7 +163,7 @@ describe('pushReservationToAirtrail write gate (#1240)', () => { it('detaches when the flight is gone from AirTrail β€” the same as a remote delete', async () => { isAirtrailWriteEnabled.mockReturnValue(true); getFlight.mockResolvedValue(null); - await svc.pushReservationToAirtrail(5, 9); + await svc.link.pushReservationToAirtrail(5, 9); expect(saveFlight).not.toHaveBeenCalled(); expect(runSpy).toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), [5]); }); @@ -164,7 +178,7 @@ describe('inbound sync multi-leg guard (#1535)', () => { }; dbAll = (sql: string) => sql.includes('sync_enabled = 1') ? [{ id: 5, trip_id: 9, external_id: '42', external_hash: 'stale' }] : []; - svc = makeService(); + svc = makeServices(); } it('detaches instead of flattening when a linked reservation grew extra stops locally', async () => { @@ -175,7 +189,7 @@ describe('inbound sync multi-leg guard (#1535)', () => { listFlights.mockResolvedValue([{ id: 42 }]); getReservation.mockReturnValue({ id: 5, metadata: JSON.stringify({}) }); - const { changed } = await svc.runAirtrailSyncForUser(7); + const { changed } = await svc.sync.runAirtrailSyncForUser(7); expect(updateReservation).not.toHaveBeenCalled(); expect(runSpy).toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), [5]); expect(changed).toBe(1); @@ -186,7 +200,7 @@ describe('inbound sync multi-leg guard (#1535)', () => { listFlights.mockResolvedValue([{ id: 42 }]); getReservation.mockReturnValue({ id: 5, metadata: JSON.stringify({}) }); - await svc.runAirtrailSyncForUser(7); + await svc.sync.runAirtrailSyncForUser(7); expect(updateReservation).toHaveBeenCalledTimes(1); expect(runSpy).not.toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), expect.anything()); }); @@ -194,7 +208,7 @@ describe('inbound sync multi-leg guard (#1535)', () => { it('detaches a flight that vanished from AirTrail, keeping the TREK row', async () => { withLinkedRow(2); listFlights.mockResolvedValue([]); // the linked id is no longer there - const { changed } = await svc.runAirtrailSyncForUser(7); + const { changed } = await svc.sync.runAirtrailSyncForUser(7); expect(updateReservation).not.toHaveBeenCalled(); expect(runSpy).toHaveBeenCalledWith(expect.stringContaining('sync_enabled = 0'), [5]); expect(changed).toBe(1); @@ -203,7 +217,7 @@ describe('inbound sync multi-leg guard (#1535)', () => { it('leaves a disconnected owner alone rather than detaching their rows', async () => { withLinkedRow(2); getAirtrailCredentials.mockReturnValue(null); - const { changed } = await svc.runAirtrailSyncForUser(7); + const { changed } = await svc.sync.runAirtrailSyncForUser(7); expect(listFlights).not.toHaveBeenCalled(); expect(runSpy).not.toHaveBeenCalled(); expect(changed).toBe(0);