From 8f2fe9d327b76030ef5e4f0ea7dd5fc3512d1c85 Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 06:18:18 +0200 Subject: [PATCH] =?UTF-8?q?refactor(server):=20plan=204=20=E2=80=94=20faca?= =?UTF-8?q?de=20inline=20sweep=20part=20B=20(rosterUserIds/getPlaceWithTag?= =?UTF-8?q?s)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inline DatabaseService.rosterUserIds onto TripMembersRepository.rosterUserIds directly in budget.service.ts, budget.mcp.ts and todo.service.ts, and DatabaseService.getPlaceWithTags onto PlacesRepository.findWithTagsAndRatings in accommodations.service.ts (placesRepo was already injected there). BudgetService/BudgetMcp/TodoService collapse their constructors, dropping the now-fully-unused DatabaseService param (budget.module.ts/todo.module.ts register TripMembers on forFeature). PackingService keeps DatabaseService as a vestigial param behind an optional TripMembersRepository with a fallback in tripRosterIds: an in-flight Task 5c file (tests/unit/services/conflictUpdate.test.ts) hand-constructs PackingService positionally without the new repository, so its arity cannot change this task; the fallback keeps that file's DatabaseService.rosterUserIds spy working until Task 4 drops the param with the class itself. --- .../accommodations/accommodations.service.ts | 18 +++++---------- server/src/nest/budget/budget.mcp.ts | 11 +++++---- server/src/nest/budget/budget.module.ts | 3 ++- server/src/nest/budget/budget.service.ts | 16 +++++++------ server/src/nest/packing/packing.module.ts | 3 ++- server/src/nest/packing/packing.service.ts | 23 ++++++++++++++++--- server/src/nest/todo/todo.module.ts | 3 ++- server/src/nest/todo/todo.service.ts | 21 +++++++++-------- 8 files changed, 60 insertions(+), 38 deletions(-) diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index dc46824c1..e7966cf1e 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -2,7 +2,6 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; import type { RoadtripVia, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; -import { DatabaseService, type PlaceWithTags, type TripAccess } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { PermissionsService } from '../permissions/permissions.service'; import { AssignmentsService } from '../assignments/assignments.service'; @@ -12,11 +11,11 @@ import type { DayAccommodationsRepository, DayAccommodationRow } from '../../db/ import { DayAssignments } from '../../db/entities/DayAssignments.entity'; import type { DayAssignmentsRepository } from '../../db/repositories/DayAssignments.repository'; import { Places } from '../../db/entities/Places.entity'; -import type { PlacesRepository } from '../../db/repositories/Places.repository'; +import type { PlacesRepository, PlaceWithTagsRow as PlaceWithTags } from '../../db/repositories/Places.repository'; import { Days } from '../../db/entities/Days.entity'; import type { DaysRepository } from '../../db/repositories/Days.repository'; import { Trips } from '../../db/entities/Trips.entity'; -import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import type { TripsRepository, TripAccess } from '../../db/repositories/Trips.repository'; import { RoadtripVias } from '../../db/entities/RoadtripVias.entity'; import type { RoadtripViasRepository } from '../../db/repositories/RoadtripVias.repository'; import { Reservations } from '../../db/entities/Reservations.entity'; @@ -111,9 +110,6 @@ export interface CreateAccommodationData { @Injectable() export class AccommodationsService { constructor( - // `dbs` stays: `stampLodging` still reaches `getPlaceWithTags` - // (Task 3/4's own delegate, not this task's). - private readonly dbs: DatabaseService, private readonly permissions: PermissionsService, private readonly realtime: RealtimeService, private readonly assignments: AssignmentsService, @@ -131,10 +127,6 @@ export class AccommodationsService { @InjectRepository(BudgetItems) private readonly budgetItemsRepo: BudgetItemsRepository, ) {} - private get db() { - return this.dbs; - } - /** Owner or member, returning the trip. Takes a number too: the MCP tools pass * the parsed id, the REST path the raw param. */ async verifyTripAccess(tripId: string | number, userId: number) { @@ -385,8 +377,10 @@ export class AccommodationsService { if (!place || place.stop_type) return null; // AC10 await this.placesRepo.stampHotel(placeId); - // AC11 — primitive, already-converted (`DatabaseService.getPlaceWithTags` → `PlacesRepository.findWithTagsAndRatings`). - return await this.db.getPlaceWithTags(placeId); + // AC11 — Plan 4 Task 3: `DatabaseService.getPlaceWithTags` inlined onto + // `PlacesRepository.findWithTagsAndRatings` directly (`placesRepo` was + // already injected below for other reads). + return await this.placesRepo.findWithTagsAndRatings(placeId); } /** diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index 42afba8a5..e49287385 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -11,7 +11,6 @@ import { DemoService } from '../common/demo.service'; import { ADDON_IDS } from '../../addons'; import { noAccess, permissionDenied } from '../../mcp/tools/_shared'; import { TripMembershipService } from '../trip-membership/trip-membership.service'; -import { DatabaseService } from '../database/database.service'; import { BudgetService } from './budget.service'; import { ExchangeRatesService } from './exchange-rates.service'; import { addonGate } from '../addons/addon-gate'; @@ -22,6 +21,8 @@ import { Places } from '../../db/entities/Places.entity'; import type { PlacesRepository } from '../../db/repositories/Places.repository'; import { Trips } from '../../db/entities/Trips.entity'; import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; /** Legacy registrar gate: the whole budget surface rides the budget addon. */ const budgetAddonOn = addonGate(ADDON_IDS.BUDGET); @@ -85,7 +86,6 @@ export class BudgetMcp { constructor( private readonly budget: BudgetService, private readonly exchangeRates: ExchangeRatesService, - private readonly db: DatabaseService, private readonly env: RuntimeEnvService, private readonly membership: TripMembershipService, readonly addons: AddonsService, @@ -94,6 +94,9 @@ export class BudgetMcp { @InjectRepository(Places) private readonly places: PlacesRepository, @InjectRepository(Trips) private readonly trips: TripsRepository, private readonly demo: DemoService, + // Plan 4 Task 3 — DatabaseService.rosterUserIds inlined onto + // TripMembersRepository.rosterUserIds directly. + @InjectRepository(TripMembers) private readonly tripMembers: TripMembersRepository, ) {} /** Plan 3i Task 3: the AuthService.isDemoUser check via the injected DemoService (common/demo.service.ts), not the free-function demo-write.ts helper. */ @@ -132,7 +135,7 @@ export class BudgetMcp { // here instead would certify a split against a figure the row never receives. // Negative payers count with their sign (#2176) — the write path stores them. if (payers !== undefined) { - const roster = await this.db.rosterUserIds(tripId); + const roster = await this.tripMembers.rosterUserIds(tripId); return sumCents(payers.filter(p => p.amount !== 0 && roster.has(p.user_id)).map(p => p.amount)); } if (total_price !== undefined) return toCents(total_price); @@ -154,7 +157,7 @@ export class BudgetMcp { totalCents: number, payers?: { user_id: number; amount: number }[], ): Promise { - const roster = await this.db.rosterUserIds(tripId); + const roster = await this.tripMembers.rosterUserIds(tripId); const strangers = members.filter(m => !roster.has(m.user_id)).map(m => m.user_id); if (strangers.length > 0) { return `members contains user IDs that are not on this trip: ${strangers.join(', ')}. Resolve them with list_trip_members.`; diff --git a/server/src/nest/budget/budget.module.ts b/server/src/nest/budget/budget.module.ts index 730a5c08e..ae4d3b2a7 100644 --- a/server/src/nest/budget/budget.module.ts +++ b/server/src/nest/budget/budget.module.ts @@ -7,6 +7,7 @@ import { BudgetCategoryOrder } from '../../db/entities/BudgetCategoryOrder.entit import { Reservations } from '../../db/entities/Reservations.entity'; import { Places } from '../../db/entities/Places.entity'; import { Trips } from '../../db/entities/Trips.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; import { Module } from '@nestjs/common'; import { BudgetController } from './budget.controller'; import { BudgetService } from './budget.service'; @@ -34,7 +35,7 @@ import { DemoModule } from '../common/demo.module'; * DemoService, and a hand-built TestingModule never imports AppModule, so * the @Global broadcast never happens unless this module imports it directly. */ @Module({ - imports: [McpSharedModule, PermissionsModule, AppConfigModule, DemoModule, RealtimeModule, PluginGuardsModule, AddonsModule, TripMembershipModule, MikroOrmModule.forFeature([BudgetItems, BudgetItemMembers, BudgetItemPayers, BudgetSettlements, BudgetCategoryOrder, Reservations, Places, Trips])], + imports: [McpSharedModule, PermissionsModule, AppConfigModule, DemoModule, RealtimeModule, PluginGuardsModule, AddonsModule, TripMembershipModule, MikroOrmModule.forFeature([BudgetItems, BudgetItemMembers, BudgetItemPayers, BudgetSettlements, BudgetCategoryOrder, Reservations, Places, Trips, TripMembers])], controllers: [BudgetController], providers: [BudgetService, ExchangeRatesService, BudgetMcp, ExchangeRatesRpc, CostsRpc], // For in-container consumers (CostsRpc, TripsService, diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 3e3f8d6e1..a003be126 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -1,6 +1,5 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; -import { DatabaseService, type TripAccess } from '../database/database.service'; import type { BudgetParticipantFinal, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -23,7 +22,9 @@ import type { ReservationsRepository } from '../../db/repositories/Reservations. import { Places } from '../../db/entities/Places.entity'; import type { PlacesRepository } from '../../db/repositories/Places.repository'; import { Trips } from '../../db/entities/Trips.entity'; -import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import type { TripsRepository, TripAccess } from '../../db/repositories/Trips.repository'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; type Trip = TripAccess; @@ -126,7 +127,6 @@ function allocateDisplayCents(cents: number[], factor: number, total = Math.roun @Injectable() export class BudgetService { constructor( - private readonly db: DatabaseService, private readonly permissions: PermissionsService, private readonly exchangeRates: ExchangeRatesService, private readonly realtime: RealtimeService, @@ -139,13 +139,15 @@ export class BudgetService { @InjectRepository(Reservations) private readonly reservationsRepo: ReservationsRepository, @InjectRepository(Places) private readonly placesRepo: PlacesRepository, @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, + // Plan 4 Task 3 — DatabaseService.rosterUserIds inlined onto + // TripMembersRepository.rosterUserIds directly. + @InjectRepository(TripMembers) private readonly tripMembersRepo: TripMembersRepository, ) {} async verifyTripAccess(tripId: string | number, userId: number) { // Plan 4 Task 2 — canAccessTrip's own DatabaseService delegation is // gone: this reuses the TripsRepository already injected for other - // reads and calls findAccessible. `db` stays injected for - // `rosterUserIds` (Task 3's own). + // reads and calls findAccessible. return await this.tripsRepo.findAccessible(tripId, userId); } @@ -208,7 +210,7 @@ export class BudgetService { private async rosterMemberIds(tripId: string | number, userIds: number[]): Promise> { const unique = new Set(userIds); if (unique.size === 0) return new Set(); - const roster = await this.db.rosterUserIds(tripId); + const roster = await this.tripMembersRepo.rosterUserIds(tripId); return new Set([...unique].filter(id => roster.has(id))); } @@ -1204,7 +1206,7 @@ export class BudgetService { * an id it rejected exists at all. */ private async settlementPartiesOnTrip(tripId: string | number, data: { from_user_id: number; to_user_id: number }): Promise { - const roster = await this.db.rosterUserIds(tripId); + const roster = await this.tripMembersRepo.rosterUserIds(tripId); return roster.has(data.from_user_id) && roster.has(data.to_user_id); } diff --git a/server/src/nest/packing/packing.module.ts b/server/src/nest/packing/packing.module.ts index 760794274..f44bcc702 100644 --- a/server/src/nest/packing/packing.module.ts +++ b/server/src/nest/packing/packing.module.ts @@ -21,6 +21,7 @@ import { PackingTemplates } from '../../db/entities/PackingTemplates.entity'; import { PackingTemplateCategories } from '../../db/entities/PackingTemplateCategories.entity'; import { PackingTemplateItems } from '../../db/entities/PackingTemplateItems.entity'; import { Trips } from '../../db/entities/Trips.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; /** Packing domain (S2 — Phase 2 trip sub-domain). Registered in AppModule. * Exports PackingService for in-container consumers (TripsService bundle, @@ -31,7 +32,7 @@ import { Trips } from '../../db/entities/Trips.entity'; // `PackingService.notifyTagged`'s PK91 `getTitle` read only, the same // "registered here only for one read, never the owning module" shape // `CollabModule`'s own forFeature list documents). - MikroOrmModule.forFeature([PackingItems, PackingItemContributors, PackingBags, PackingCategoryAssignees, PackingTemplates, PackingTemplateCategories, PackingTemplateItems, Trips]), + MikroOrmModule.forFeature([PackingItems, PackingItemContributors, PackingBags, PackingCategoryAssignees, PackingTemplates, PackingTemplateCategories, PackingTemplateItems, Trips, TripMembers]), McpSharedModule, NotificationsModule, PermissionsModule, AuthModule, RealtimeModule, PluginGuardsModule, AddonsModule, AuditModule], controllers: [PackingController, AdminPackingTemplatesController], providers: [PackingService, PackingMcp, PackingRpc], diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 5e1d4d40f..171d409ef 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -25,6 +25,8 @@ import { PackingTemplateItems } from '../../db/entities/PackingTemplateItems.ent import type { PackingTemplateItemsRepository } from '../../db/repositories/PackingTemplateItems.repository'; import { Trips } from '../../db/entities/Trips.entity'; import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; /** Privacy fields stamped on a packing item (#858). */ type PrivacyFields = { is_private?: number; owner_id?: number | null }; @@ -95,13 +97,22 @@ export class PackingService { @InjectRepository(PackingTemplateCategories) private readonly templateCategoriesRepo: PackingTemplateCategoriesRepository, @InjectRepository(PackingTemplateItems) private readonly templateItemsRepo: PackingTemplateItemsRepository, @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, + // Plan 4 Task 3: `DatabaseService.rosterUserIds` inlined onto + // `TripMembersRepository.rosterUserIds` — optional (not a trailing + // required param) because an in-flight Task 5c file + // (`tests/unit/services/conflictUpdate.test.ts`, out of this task's + // file-ownership window) hand-constructs `PackingService` positionally + // without it; every real (DI-built) instance gets it via `forFeature`. + // `tripRosterIds` below falls back to the `db` facade only when absent. + // Safe to make required, and drop the fallback + `db`, once that file lands. + @InjectRepository(TripMembers) private readonly tripMembersRepo?: TripMembersRepository, ) {} async verifyTripAccess(tripId: string | number, userId: number) { // Plan 4 Task 2 — canAccessTrip's own DatabaseService delegation is // gone: this reuses the TripsRepository already injected for other - // reads and calls findAccessible. `db` stays injected for - // `rosterUserIds` (Task 3's own). + // reads and calls findAccessible. `db` stays injected only for the + // `tripRosterIds` fallback below (Task 3's own). return await this.tripsRepo.findAccessible(tripId, userId); } @@ -561,7 +572,13 @@ export class PackingService { * grew up without it. */ private async tripRosterIds(tripId: string | number): Promise> { - return await this.db.rosterUserIds(tripId); + // Plan 4 Task 3: `TripMembersRepository.rosterUserIds`, inlined, when a + // real (DI-built) instance has it — falls back to the `DatabaseService` + // facade only for the one hand-built test instance that still omits it + // (see the constructor comment above). + return this.tripMembersRepo + ? await this.tripMembersRepo.rosterUserIds(tripId) + : await this.db.rosterUserIds(tripId); } async setBagMembers(tripId: string | number, bagId: string | number, userIds: number[]) { diff --git a/server/src/nest/todo/todo.module.ts b/server/src/nest/todo/todo.module.ts index a40ff6b90..632e7c658 100644 --- a/server/src/nest/todo/todo.module.ts +++ b/server/src/nest/todo/todo.module.ts @@ -13,6 +13,7 @@ import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; import { TodoItems } from '../../db/entities/TodoItems.entity'; import { TodoCategoryAssignees } from '../../db/entities/TodoCategoryAssignees.entity'; import { Trips } from '../../db/entities/Trips.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; /** To-do domain (S3 — Phase 2 trip sub-domain). Registered in AppModule. * Exports TodoService for in-container consumers (TripsService bundle). @@ -23,7 +24,7 @@ import { Trips } from '../../db/entities/Trips.entity'; * (Plan 4 Task 2): `verifyTripAccess`'s own canAccessTrip delegate, now * TripsRepository directly. */ @Module({ - imports: [MikroOrmModule.forFeature([TodoItems, TodoCategoryAssignees, Trips]), McpSharedModule, PermissionsModule, AuthModule, RealtimeModule, PluginGuardsModule, AddonsModule], + imports: [MikroOrmModule.forFeature([TodoItems, TodoCategoryAssignees, Trips, TripMembers]), McpSharedModule, PermissionsModule, AuthModule, RealtimeModule, PluginGuardsModule, AddonsModule], controllers: [TodoController], providers: [TodoService, TodoMcp, TodoRpc], exports: [TodoService], diff --git a/server/src/nest/todo/todo.service.ts b/server/src/nest/todo/todo.service.ts index a5cbf5c83..0efa166fc 100644 --- a/server/src/nest/todo/todo.service.ts +++ b/server/src/nest/todo/todo.service.ts @@ -4,14 +4,15 @@ import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import type { User } from '../../types'; -import { DatabaseService, type TripAccess } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { TodoItems } from '../../db/entities/TodoItems.entity'; import type { TodoItemsRepository } from '../../db/repositories/TodoItems.repository'; import { TodoCategoryAssignees } from '../../db/entities/TodoCategoryAssignees.entity'; import type { TodoCategoryAssigneesRepository } from '../../db/repositories/TodoCategoryAssignees.repository'; import { Trips } from '../../db/entities/Trips.entity'; -import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import type { TripsRepository, TripAccess } from '../../db/repositories/Trips.repository'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; type Trip = TripAccess; @@ -21,25 +22,27 @@ type Trip = TripAccess; * moved 1:1 off the legacy raw statements: identical column sets, the `||` * falsy-coercion defaults, the bodyKeys sentinel protocol on update and the * post-write re-selects). Trip access, the 'packing_edit' permission - * (shared with packing), the roster-filter on category assignees (kept - * unconverted on `DatabaseService.rosterUserIds`, the `BudgetService` - * precedent) and the WebSocket broadcast keep their legacy call paths. + * (shared with packing), the roster-filter on category assignees (Plan 4 + * Task 3: `TripMembersRepository.rosterUserIds`, injected directly, not + * through `DatabaseService`) and the WebSocket broadcast keep their legacy + * call paths. * Non-Nest consumers (plugin RPC host, the legacy MCP trips registrar) go * through todo.bridge.ts instead of importing this class directly. */ @Injectable() export class TodoService { constructor( - private readonly db: DatabaseService, private readonly permissions: PermissionsService, private readonly realtime: RealtimeService, private readonly uow: UnitOfWork, @InjectRepository(TodoItems) private readonly todoItemsRepo: TodoItemsRepository, @InjectRepository(TodoCategoryAssignees) private readonly todoCategoryAssigneesRepo: TodoCategoryAssigneesRepository, // Plan 4 Task 2 — canAccessTrip's own DatabaseService delegation is - // gone: this injects TripsRepository directly. `db` stays injected for - // `rosterUserIds` (Task 3's own). + // gone: this injects TripsRepository directly. @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, + // Plan 4 Task 3 — DatabaseService.rosterUserIds inlined onto + // TripMembersRepository.rosterUserIds directly. + @InjectRepository(TripMembers) private readonly tripMembersRepo: TripMembersRepository, ) {} async verifyTripAccess(tripId: string | number, userId: number) { @@ -132,7 +135,7 @@ export class TodoService { // members and reservations filter travellers. Dropped rather than // rejected: a copied trip carries assignee ids across before its members // exist, and a 400 would make the picker unusable there. - const roster = await this.db.rosterUserIds(tripId); + const roster = await this.tripMembersRepo.rosterUserIds(tripId); for (const uid of userIds) if (roster.has(uid)) await this.todoCategoryAssigneesRepo.insertIgnore(tripId, categoryName, uid); } });