diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 13870ed38..8875a6ece 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -8,7 +8,7 @@ "src/nest/atlas/atlas-geo.ts": 1005, "src/nest/atlas/atlas.service.ts": 1182, "src/nest/auth/auth.service.ts": 1165, - "src/nest/budget/budget.service.ts": 1703, + "src/nest/budget/budget.service.ts": 1702, "src/nest/collections/collections.service.ts": 1488, "src/nest/doc-sync/doc-sync.service.ts": 1060, "src/nest/doc-sync/providers/synology-drive.client.ts": 1160, @@ -18,6 +18,6 @@ "src/nest/plugins/plugin-runtime.service.ts": 1463, "src/nest/plugins/runtime/plugin-sdk.ts": 1094, "src/nest/reservations/reservations.service.ts": 1240, - "src/nest/trips/trips.service.ts": 1013, + "src/nest/trips/trips.service.ts": 1009, "src/nest/vacay/vacay.service.ts": 1440 } diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json index 8b6101c56..2e1bb7a4b 100644 --- a/server/scripts/tx-writes-baseline.json +++ b/server/scripts/tx-writes-baseline.json @@ -96,6 +96,5 @@ "src/nest/plugins/plugins.controller.ts#PluginsController.update": 2, "src/nest/plugins/plugins.controller.ts#PluginsController.updateConfig": 3, "src/nest/reservations/reservations.controller.ts#ReservationsController.update": 2, - "src/nest/trips/trips.service.ts#TripsService.update": 2, "src/nest/vacay/vacay.service.ts#VacayService.updatePlan": 2 } diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 37f6884b2..7c36f54dc 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -29,6 +29,9 @@ import type { TripMembersRepository } from '../../db/repositories/TripMembers.re type Trip = TripAccess; +/** A trip currency switch whose rates are already fetched, ready to write (#1543). */ +export type CurrencyRebase = { next: string; rates: Record | null }; + type SettlementRow = { id: number; trip_id: number; from_user_id: number; to_user_id: number; amount: number; currency: string | null; exchange_rate: number | null; @@ -538,18 +541,22 @@ export class BudgetService { * * Runs before the trip update, while the old currency is still in `trips`; a no-op without a change. */ - async rebaseTripCurrency( - tripId: string | number, - newCurrency: string | null | undefined, - ): Promise { - const next = (newCurrency || '').toUpperCase(); - if (!next) return; - const tripCurrency = await this.tripsRepo.getCurrency(tripId); - if (tripCurrency === undefined) return; - const prev = (tripCurrency || 'EUR').toUpperCase(); - if (prev === next) return; + async rebaseTripCurrency(tripId: string | number, newCurrency?: string | null): Promise { + const plan = await this.prepareCurrencyRebase(tripId, newCurrency); + if (plan) await this.applyCurrencyRebase(tripId, plan); + } - const rates = await this.exchangeRates.getRates(next); + /** rebaseTripCurrency's network half: the rates, fetched outside any transaction. Null when nothing changes. */ + async prepareCurrencyRebase(tripId: string | number, newCurrency?: string | null): Promise { + const next = (newCurrency || '').toUpperCase(); + if (!next) return null; + const tripCurrency = await this.tripsRepo.getCurrency(tripId); + if (tripCurrency === undefined || (tripCurrency || 'EUR').toUpperCase() === next) return null; + return { next, rates: await this.exchangeRates.getRates(next) }; + } + + /** rebaseTripCurrency's writes. The outgoing currency is read inside them, so the trip update can share the transaction. */ + async applyCurrencyRebase(tripId: string | number, { next, rates }: CurrencyRebase): Promise { // A row already denominated in the new base needs no conversion (rate 1). When no // live rate is available we also store 1 rather than a stale one: rate 1 means "not // frozen", so the settlement falls back to live rates instead of trusting a figure @@ -560,32 +567,23 @@ export class BudgetService { return r && r > 0 ? r : 1; }; - // D4 (rule 23) — one repository method per table, not a dynamic identifier: - // `budgetItemsRepo`'s and `budgetSettlementsRepo`'s own `pinCurrency`/ - // `listDistinctCurrencies`/`setExchangeRateForCurrency` trios. - const rebaseBudgetItems = async () => { + await this.uow.transactional(async () => { + const current = await this.tripsRepo.getCurrency(tripId); + const prev = (current || 'EUR').toUpperCase(); + if (current === undefined || prev === next) return; + // D4 (rule 23): each table's own pinCurrency/listDistinctCurrencies/setExchangeRateForCurrency, no dynamic identifier. await this.budgetItemsRepo.pinCurrency(tripId, prev); - const currencies = await this.budgetItemsRepo.listDistinctCurrencies(tripId); - for (const cur of currencies) { + for (const cur of await this.budgetItemsRepo.listDistinctCurrencies(tripId)) { await this.budgetItemsRepo.setExchangeRateForCurrency(tripId, cur, rateFor(cur.toUpperCase())); } - }; - const rebaseBudgetSettlements = async () => { await this.budgetSettlementsRepo.pinCurrency(tripId, prev); - const currencies = await this.budgetSettlementsRepo.listDistinctCurrencies(tripId); - for (const cur of currencies) { + for (const cur of await this.budgetSettlementsRepo.listDistinctCurrencies(tripId)) { await this.budgetSettlementsRepo.setExchangeRateForCurrency(tripId, cur, rateFor(cur.toUpperCase())); } - }; - - // Only priced places have anything to denominate; a currency on a free place would - // just be noise. `updated_at` doubles as the optimistic-concurrency token (#1135), - // so bumping it stops a client holding the pre-switch row from writing the pin away. - const pinPlaces = async () => { + // Only priced places are pinned. Their `updated_at` bump (the #1135 concurrency token) + // stops a client holding the pre-switch row from writing the pin away. await this.placesRepo.pinCurrencyForTrip(tripId, prev); - }; - - await this.uow.transactional(async () => { await rebaseBudgetItems(); await rebaseBudgetSettlements(); await pinPlaces(); }); + }); } async createBudgetItem( diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index afc67113d..55100ff95 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -42,7 +42,7 @@ import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import type { Trip, User } from '../../types'; import { DaysService } from '../days/days.service'; -import { BudgetService } from '../budget/budget.service'; +import { BudgetService, type CurrencyRebase } from '../budget/budget.service'; import { ReservationsService } from '../reservations/reservations.service'; import { VacayService } from '../vacay/vacay.service'; import { UnsplashService } from '../unsplash/unsplash.service'; @@ -496,17 +496,12 @@ export class TripsService { } /** - * The folded legacy updateTrip core — no currency rebase. The REST path goes - * through update() below; the plugin RPC host calls this directly (parity: - * the legacy host path never rebased). + * The folded legacy updateTrip core. The plugin RPC host calls it without `rebase` (the legacy + * host path never rebased); update() below passes one. TP24 to TP29 (inventory §11c): the budget + * rebase, the trip row (TP25), the leave entries that follow its dates and the rebuilt day grid + * are one transaction, so a failing `generateDays` or booking resync leaves all of them as they were. */ - /** - * TP24–TP29 (inventory §11c) — TP25's `UPDATE trips` write stays BEFORE - * the `:423` days-regeneration transaction (R5/§18.6, unchanged): if - * `generateDays` throws, the transaction rolls back the day rows but the - * trip keeps its new dates. Flagged, not fixed, per the ruling. - */ - async updateTrip(tripId: string | number, userId: number, data: UpdateTripData, userRole: string): Promise { + async updateTrip(tripId: string | number, userId: number, data: UpdateTripData, userRole: string, rebase: CurrencyRebase | null = null): Promise { const trip = await this.tripsRepo.findRaw(tripId) as (Trip & { reminder_days?: number }) | null; // TP24 if (!trip) throw new NotFoundError('Trip not found'); @@ -528,6 +523,8 @@ export class TripsService { // one write: a failure halfway used to leave a trip whose days missed its dates. let removedDays: DayGridRemoval[] = []; await this.uow.transactional(async () => { + // Before the row: the rebase reads the outgoing currency off it (#1543). + if (rebase) await this.budget.applyCurrencyRebase(tripId, rebase); await this.tripsRepo.updateTripRow(tripIdNum, { // TP25 title: newTitle, description: newDesc ?? null, @@ -613,11 +610,10 @@ export class TripsService { const trip = await this.getRaw(tripId); if (!trip) throw new NotFoundError('Trip not found'); this.resolveRange(trip, body); - // Re-anchor the budget while the outgoing currency is still on the trip row, - // otherwise the frozen FX rates and the currency-less expenses that inherit the - // trip's base are left pointing at a currency that no longer exists (#1543). - await this.budget.rebaseTripCurrency(tripId, body.currency); - return await this.updateTrip(tripId, userId, body, role); + // Re-anchor the budget so frozen rates and currency-less expenses do not point at a currency + // the trip left (#1543). The rates are fetched here, the rebase is written with the trip row. + const rebase = await this.budget.prepareCurrencyRebase(tripId, body.currency); + return await this.updateTrip(tripId, userId, body, role, rebase); } // ── Delete ───────────────────────────────────────────────────────────────── diff --git a/server/tests/e2e/trips.e2e.test.ts b/server/tests/e2e/trips.e2e.test.ts index 4e8e0004a..44ce9af45 100644 --- a/server/tests/e2e/trips.e2e.test.ts +++ b/server/tests/e2e/trips.e2e.test.ts @@ -83,7 +83,7 @@ describe('Trips e2e (real auth guard + temp SQLite)', () => { app = await build(); checkPermission = vi.spyOn(app.get(PermissionsService), 'checkPermission'); vi.spyOn(app.get(BudgetService), 'listBudgetItems').mockResolvedValue([]); - vi.spyOn(app.get(BudgetService), 'rebaseTripCurrency').mockResolvedValue(); + vi.spyOn(app.get(BudgetService), 'prepareCurrencyRebase').mockResolvedValue(null); server = app.getHttpServer(); }); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index b9cdb2d5c..0203f444f 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -584,6 +584,31 @@ describe('composite service paths (ex budget.bridge delegation)', () => { expect(row).toEqual({ currency: 'EUR', exchange_rate: RATES.RUB.EUR }); }); + it('BUDGET-SVC-DB-086: prepareCurrencyRebase writes nothing and applyCurrencyRebase reads the outgoing currency inside its write', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + testDb.prepare("UPDATE trips SET currency = 'EUR' WHERE id = ?").run(trip.id); + const item = await budget.createBudgetItem(trip.id, { name: 'Implicit', total_price: 100, members: [{ user_id: user.id }] }); + const row = () => testDb.prepare('SELECT currency, exchange_rate FROM budget_items WHERE id = ?').get(item.id); + const before = row(); + + expect(await budget.prepareCurrencyRebase(trip.id, 'eur')).toBeNull(); + expect(await budget.prepareCurrencyRebase(trip.id, '')).toBeNull(); + expect(await budget.prepareCurrencyRebase(999999, 'RUB')).toBeNull(); + const plan = await budget.prepareCurrencyRebase(trip.id, 'rub'); + expect(plan).toEqual({ next: 'RUB', rates: RATES.RUB }); + expect(row()).toEqual(before); + + // The trip took the new currency in the meantime: nothing is left to rebase. + testDb.prepare("UPDATE trips SET currency = 'RUB' WHERE id = ?").run(trip.id); + await budget.applyCurrencyRebase(trip.id, plan!); + expect(row()).toEqual(before); + + testDb.prepare("UPDATE trips SET currency = 'EUR' WHERE id = ?").run(trip.id); + await budget.applyCurrencyRebase(trip.id, plan!); + expect(row()).toEqual({ currency: 'EUR', exchange_rate: RATES.RUB.EUR }); + }); + it('BUDGET-SVC-DB-018: linkBudgetItemToReservation stamps the reservation id', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index faa33f704..0e9ec87c1 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -50,7 +50,7 @@ vi.mock('../../../src/websocket', () => ({ broadcast })); import { db as testDb } from '../../../src/db/database'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, createReservation, createPlace, createDay, createDayAssignment, createDayNote, addTripMember } from '../../helpers/factories'; +import { createUser, createTrip, createReservation, createPlace, createDay, createDayAssignment, createDayNote, addTripMember, createBudgetItem } from '../../helpers/factories'; import { createTestTourWaypointsRepo, createTour } from '../../helpers/tours-repos'; import { MAX_TRIP_DAYS, resolveDayGridRange, tripSpanDays } from '@trek/shared'; import { DaysService } from '../../../src/nest/days/days.service'; @@ -1397,33 +1397,73 @@ describe('TripsService wrapper helpers', () => { it('re-anchors the budget before the trip row leaves its old currency (#1543)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - const order: string[] = []; - const rebaseSpy = vi.spyOn(budgetSvc, 'rebaseTripCurrency').mockImplementation(async () => { order.push('rebase'); }); - const updateSpy = vi.spyOn(svc, 'updateTrip').mockImplementation(() => { order.push('update'); return {} as never; }); + testDb.prepare("UPDATE trips SET currency = 'EUR' WHERE id = ?").run(trip.id); + const plan = { next: 'RUB', rates: null }; + const seen: string[] = []; + const prepareSpy = vi.spyOn(budgetSvc, 'prepareCurrencyRebase').mockResolvedValue(plan); + const applySpy = vi.spyOn(budgetSvc, 'applyCurrencyRebase').mockImplementation(async () => { + seen.push((testDb.prepare('SELECT currency FROM trips WHERE id = ?').get(trip.id) as { currency: string }).currency); + }); try { await svc.update(trip.id, user.id, { currency: 'RUB' } as never, 'user'); - // The rebase reads the outgoing currency off the trip row, so it has to run first. - expect(rebaseSpy).toHaveBeenCalledWith(trip.id, 'RUB'); - expect(order).toEqual(['rebase', 'update']); + expect(prepareSpy).toHaveBeenCalledWith(trip.id, 'RUB'); + // The rebase reads the outgoing currency off the trip row, so it runs before the row moves. + expect(applySpy).toHaveBeenCalledWith(trip.id, plan); + expect(seen).toEqual(['EUR']); + expect(testDb.prepare('SELECT currency FROM trips WHERE id = ?').get(trip.id)).toEqual({ currency: 'RUB' }); } finally { - rebaseSpy.mockRestore(); - updateSpy.mockRestore(); + prepareSpy.mockRestore(); + applySpy.mockRestore(); + } + }); + + it('TRIP-SVC-092: a failing day rebuild leaves the budget on the old currency with the trip (#1543)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-07-01', end_date: '2026-07-03' }); + testDb.prepare("UPDATE trips SET currency = 'EUR' WHERE id = ?").run(trip.id); + const item = createBudgetItem(testDb, trip.id, { total_price: 100 }); + testDb.prepare("UPDATE budget_items SET currency = 'USD', exchange_rate = 1.1 WHERE id = ?").run(item.id); + // Rates come from the network in production; the plan is handed in so the real write runs. + const prepareSpy = vi.spyOn(budgetSvc, 'prepareCurrencyRebase').mockResolvedValue({ next: 'JPY', rates: { USD: 0.0067, EUR: 0.0062 } }); + const daysSpy = vi.spyOn(svc, 'generateDays').mockRejectedValue(new Error('boom')); + try { + await expect(svc.update(trip.id, user.id, { currency: 'JPY', end_date: '2026-07-05' }, 'user')).rejects.toThrow('boom'); + expect(testDb.prepare('SELECT currency, end_date FROM trips WHERE id = ?').get(trip.id)).toEqual({ currency: 'EUR', end_date: '2026-07-03' }); + expect(testDb.prepare('SELECT currency, exchange_rate FROM budget_items WHERE id = ?').get(item.id)).toEqual({ currency: 'USD', exchange_rate: 1.1 }); + } finally { + prepareSpy.mockRestore(); + daysSpy.mockRestore(); + } + }); + + it('TRIP-SVC-093: updateTrip, the plugin host path, never rebases the budget', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const prepareSpy = vi.spyOn(budgetSvc, 'prepareCurrencyRebase'); + const applySpy = vi.spyOn(budgetSvc, 'applyCurrencyRebase'); + try { + await svc.updateTrip(trip.id, user.id, { currency: 'USD' }, 'user'); + expect(prepareSpy).not.toHaveBeenCalled(); + expect(applySpy).not.toHaveBeenCalled(); + } finally { + prepareSpy.mockRestore(); + applySpy.mockRestore(); } }); it('TRIP-SVC-068: update refuses a bad range before the budget is rebased (#2403)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id, { start_date: '2026-07-01', end_date: '2026-07-07' }); - const rebaseSpy = vi.spyOn(budgetSvc, 'rebaseTripCurrency').mockResolvedValue(); + const prepareSpy = vi.spyOn(budgetSvc, 'prepareCurrencyRebase').mockResolvedValue(null); try { await expect(svc.update(trip.id, user.id, { currency: 'USD', end_date: '2036-07-01' }, 'user')) .rejects.toThrow(`A trip can span at most ${MAX_TRIP_DAYS} days`); await expect(svc.update(trip.id, user.id, { currency: 'USD', start_date: '2026-07-10' }, 'user')) .rejects.toThrow('End date must be after start date'); - expect(rebaseSpy).not.toHaveBeenCalled(); + expect(prepareSpy).not.toHaveBeenCalled(); await expect(svc.update(99999, user.id, { currency: 'USD' }, 'user')).rejects.toThrow('Trip not found'); } finally { - rebaseSpy.mockRestore(); + prepareSpy.mockRestore(); } });