mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(trips): write the budget currency rebase with the trip row
TripsService.update rebased the budget onto the new currency in its own transaction and only then wrote the trip row in a second one. When the day grid rebuild, the booking resync or the leave shift threw after the rebase had committed, budget items and settlements stayed pinned and re-frozen against a currency the trip never took (#1543). rebaseTripCurrency is split into prepareCurrencyRebase, which fetches the rates before any transaction opens, and applyCurrencyRebase, which reads the outgoing currency and writes the rebase. update() fetches first and hands the plan to updateTrip, which applies it inside the transaction that writes the row, the leave shift and the day grid. updateTrip called without a plan (the plugin RPC host) still never rebases, and rebaseTripCurrency keeps working as before. The updateTrip docstring no longer claims the row is written outside the day regeneration transaction. TripsService.update leaves the lint:tx baseline, and the size baselines shrink with the files.
This commit is contained in:
Maurice
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<string, number> | 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<void> {
|
||||
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<void> {
|
||||
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<CurrencyRebase | null> {
|
||||
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<void> {
|
||||
// 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(
|
||||
|
||||
@@ -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<UpdateTripResult> {
|
||||
async updateTrip(tripId: string | number, userId: number, data: UpdateTripData, userRole: string, rebase: CurrencyRebase | null = null): Promise<UpdateTripResult> {
|
||||
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 ─────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user