From 3a8191fc0746d2bd92c8e72e819b7648718133f2 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 5 Nov 2026 13:02:28 +0100 Subject: [PATCH] fix(costs): keep display-currency payments settled, count them as typed in the plan and the PDF, and file every booking type under its Costs key A display-currency payment whose halves pointed opposite ways no longer stayed settled (#1445), the plan sidebar and the PDF total still went through the trip currency, and a zero-decimal display currency was not rounded. Every booking type now maps to one Costs key on every path; a cost the old map filed under 'other' still moves with a type change. --- .../src/components/Planner/DayPlanSidebar.tsx | 2 +- .../src/components/Planner/planCosts.test.ts | 13 +++++ client/src/components/Planner/planCosts.ts | 14 ++++- .../src/nest/budget/budget-money.helpers.ts | 2 +- server/src/nest/budget/budget.service.ts | 58 ++++++++++++++----- .../src/nest/reservations/reservations.mcp.ts | 6 +- .../nest/reservations/reservations.service.ts | 13 ++++- server/src/nest/transit/transit.mcp.ts | 4 +- .../tests/unit/mcp/tools-reservations.test.ts | 15 +++++ .../unit/nest/budget.service.calc.test.ts | 55 ++++++++++++++++++ .../unit/nest/reservations.service.test.ts | 28 +++++++++ shared/src/budget/budget.schema.spec.ts | 2 +- shared/src/budget/budget.schema.ts | 30 ---------- shared/src/budget/cost-category.spec.ts | 25 +++++++- shared/src/budget/cost-category.ts | 18 ++++++ 15 files changed, 227 insertions(+), 58 deletions(-) diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index 97497c36c..0b5999051 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -1122,7 +1122,7 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { const budgetItems = useTripStore(s => s.budgetItems) const costsEnabled = useAddonStore(s => s.isEnabled('budget')) const totalCostLabel = useMemo(() => { - const costs = planCosts({ days, assignments, reservations, budgetItems, costsEnabled, tripCurrency: currency }) + const costs = planCosts({ days, assignments, reservations, budgetItems, costsEnabled, tripCurrency: currency, displayCurrency: costBase }) return formatMoneySum(costs.total, costBase, locale, fxRates) }, [days, assignments, reservations, budgetItems, costsEnabled, currency, costBase, locale, fxRates]) diff --git a/client/src/components/Planner/planCosts.test.ts b/client/src/components/Planner/planCosts.test.ts index 2a09eb023..fa5199615 100644 --- a/client/src/components/Planner/planCosts.test.ts +++ b/client/src/components/Planner/planCosts.test.ts @@ -65,6 +65,19 @@ describe('planCosts (#2551)', () => { expect(costs.byDay.get(2)).toEqual([{ amount: 100, currency: 'USD' }, { amount: 30, currency: 'EUR' }]) }) + it('counts an expense entered in the display currency as typed, not through the trip currency (#2965)', () => { + // A yen trip read in euros: the flight was typed in euros, frozen at 0.008 EUR per yen. + const flight = buildBudgetItem({ total_price: 1000, currency: 'EUR', exchange_rate: 0.008, place_id: 51 }) + const hotel = buildBudgetItem({ total_price: 20000, currency: null, place_id: 51 }) + const read = { ...base, tripCurrency: 'JPY', budgetItems: [flight, hotel], costsEnabled: true } + expect(planCosts({ ...read, displayCurrency: 'eur' }).total).toEqual([ + { amount: 1000, currency: 'EUR' }, + { amount: 20000, currency: 'JPY' }, + ]) + // Without a display currency (the PDF), the euros still count at their frozen rate. + expect(planCosts(read).total).toEqual([{ amount: 1000 / 0.008 + 20000, currency: 'JPY' }]) + }) + it('nets a refund against the expenses of its day', () => { const items = [ buildBudgetItem({ total_price: 200, currency: null, place_id: 50 }), diff --git a/client/src/components/Planner/planCosts.ts b/client/src/components/Planner/planCosts.ts index 155ebea82..a59f0b185 100644 --- a/client/src/components/Planner/planCosts.ts +++ b/client/src/components/Planner/planCosts.ts @@ -1,6 +1,6 @@ import type { AssignmentsMap, BudgetItem, Day, Reservation } from '../../types' import type { MoneyEntry } from '../../utils/formatters' -import { bookedInTrip } from '../../hooks/useExchangeRates' +import { bookedInTrip, inDisplayCurrency } from '../../hooks/useExchangeRates' import { daySpan, startKey } from './bookings/bookingsModel' /** What the plan's cost figures add up: per day, and for the whole trip. */ @@ -22,18 +22,22 @@ export interface PlanCosts { * An expense counts on the day its booking starts, else the first day its place is * planned on, else the day of its date. One that belongs to no day still counts in * the total. A foreign amount booked at a frozen rate counts at that rate, as Costs - * counts it. + * counts it. One entered in `displayCurrency`, when the caller reads the plan in one, + * counts as typed, as Costs shows it (#2965): booked into the trip currency and back + * at today's rate it drifted off the amount entered. The PDF passes none, since it + * adds everything up in the trip currency. * * With Costs off there are no expenses, and a place's own price (set through the * API) still counts, once, on the first day the place is planned on. */ -export function planCosts({ days, assignments, reservations, budgetItems, costsEnabled, tripCurrency }: { +export function planCosts({ days, assignments, reservations, budgetItems, costsEnabled, tripCurrency, displayCurrency }: { days: Day[] assignments: AssignmentsMap reservations: Reservation[] budgetItems: BudgetItem[] costsEnabled: boolean tripCurrency: string + displayCurrency?: string | null }): PlanCosts { const trip = (tripCurrency || 'EUR').toUpperCase() const firstDayOfPlace = new Map() @@ -84,6 +88,10 @@ export function planCosts({ days, assignments, reservations, budgetItems, costsE for (const item of budgetItems) { const amount = item.total_price || 0 if (amount === 0) continue + if (displayCurrency && inDisplayCurrency(item.currency, trip, displayCurrency)) { + add(dayOfExpense(item), { amount, currency: displayCurrency.toUpperCase() }) + continue + } const booked = bookedInTrip(amount, item.currency, item.exchange_rate, trip) const entry = booked != null ? { amount: booked, currency: trip } diff --git a/server/src/nest/budget/budget-money.helpers.ts b/server/src/nest/budget/budget-money.helpers.ts index 97666c0b7..b17a11e59 100644 --- a/server/src/nest/budget/budget-money.helpers.ts +++ b/server/src/nest/budget/budget-money.helpers.ts @@ -236,5 +236,5 @@ export function settlementFx(opts: { // inverse of the display factor, so such a transfer reads back as entered. const settleConverterFor = (sCurrency?: string | null, sRate?: number | null) => sCurrency ? tripConverter(sCurrency, sRate, tripCurrency, rates) : (amount: number) => amount / displayFactor; - return { currency, displayFactor, toDisplay, converterFor, settleConverterFor, isDirect }; + return { currency, displayFactor, displayUnit, toDisplay, converterFor, settleConverterFor, isDirect }; } diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index e63fc65ff..b7465a358 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -91,7 +91,8 @@ export function splitLegacyTicketNote( * queries instead of a full listSettlements() scan, settlement usernames use * COALESCE(display_name, username) like every item query, and updateMembers no * longer double-applies avatarUrl. - */@Injectable() + */ +@Injectable() export class BudgetService { constructor( private readonly permissions: PermissionsService, @@ -1082,7 +1083,7 @@ export class BudgetService { tripId: string | number, opts: { base?: string; rates?: Record | null; tripCurrency?: string; baseRate?: number } = {}, ) { - const { currency, displayFactor, toDisplay, converterFor, settleConverterFor, isDirect } = settlementFx(opts); + const { currency, displayFactor, displayUnit, toDisplay, converterFor, settleConverterFor, isDirect } = settlementFx(opts); const unconvertedItems: number[] = []; const unconvertedSettlements: number[] = []; const unconvertedCurrencies = new Set(); @@ -1136,6 +1137,9 @@ export class BudgetService { // convert is reported whole and moves nothing, credits and shares alike. const inDisplay = isDirect(item.currency); const convert = inDisplay ? (amount: number) => amount : converterFor(item.currency, item.exchange_rate); + // The direct half skips toDisplay, so it is kept in whole steps of the display + // currency here instead: whole won or yen, as the rest of the answer (#2604). + const unit = inDisplay ? displayUnit : 1; if (!convert) { unconvertedItems.push(item.id); unconvertedCurrencies.add((item.currency || '').toUpperCase()); @@ -1169,7 +1173,7 @@ export class BudgetService { let creditCents = 0; for (const p of payers) { const exact = convert(p.amount) * 100; - const paid = Math.round(exact); + const paid = Math.round(exact / unit) * unit; const payer = ensure(p.user_id, p); if (inDisplay) { payer.direct += paid; @@ -1194,12 +1198,13 @@ export class BudgetService { // here since #2225. An item whose payers net to exactly zero still does, and // divides zero, which is what it is worth. const hasCustomSplit = members.some(m => m.amount !== null && m.amount !== undefined); + // Split in those steps too (creditCents is a whole number of them), then back to hundredths. const shares = hasCustomSplit - ? customSplitCents(members, convert, creditCents, sumMinor(payers.map(p => p.amount))) - : splitEqualShares(creditCents, members, item.id); + ? customSplitCents(members, amount => convert(amount) / unit, creditCents / unit, sumMinor(payers.map(p => p.amount))) + : splitEqualShares(creditCents / unit, members, item.id); for (const m of members) { const member = ensure(m.user_id, m); - if (inDisplay) member.direct -= shares[m.user_id] || 0; + if (inDisplay) member.direct -= (shares[m.user_id] || 0) * unit; else member.cents -= shares[m.user_id] || 0; } } @@ -1230,12 +1235,12 @@ export class BudgetService { // goes through the trip currency at its frozen rate, which keeps a debt // in the trip currency settled when rates move (#1445). const total = Math.round(s.amount * 100); - const room = isDirect(s.currency) ? Math.max(0, Math.min(-from.direct, to.direct)) : 0; - const paidDirect = total > 0 ? Math.min(total, room) : 0; + const inDisplay = isDirect(s.currency); + const room = inDisplay ? Math.max(0, Math.min(-from.direct, to.direct)) : 0; + // In whole steps of the display currency, like the direct half it pays (#2604). + const paidDirect = total > 0 ? Math.floor(Math.min(total, room) / displayUnit) * displayUnit : 0; from.direct += paidDirect; to.direct -= paidDirect; - reimbursedDirect[s.to_user_id] = (reimbursedDirect[s.to_user_id] || 0) + paidDirect; - reimbursedDirect[s.from_user_id] = (reimbursedDirect[s.from_user_id] || 0) - paidDirect; // Rounded to a trip cent per transfer, so recording one in a display currency // can't leave a sliver of a cent behind to accumulate over a trip's lifetime. const rest = total === 0 ? 0 : (total - paidDirect) / total; @@ -1243,15 +1248,42 @@ export class BudgetService { const inTrip = Math.round(exact); from.cents += inTrip; to.cents -= inTrip; + // When the two halves point opposite ways between this pair (the payer is owed + // in display-currency expenses but owes more in the trip currency: one paid a + // euro train, the other a yen hotel), the transfer settles the net of both. + // The display-currency credit then moves into the trip half at this + // transfer's frozen rate, against the trip debt it was netted with. Left where + // it was, it stayed fixed while the yen it offset floated with the live rate, + // and a settled trip reopened as soon as the rate moved (#1445). Only as much + // moves as both halves hold, so neither side ends up owing the other way. + let back = 0; + let backExact = 0; + if (inDisplay && total > 0) { + const directRoom = Math.min(from.direct, -to.direct); + const tripRoom = Math.min(-from.cents, to.cents); + if (directRoom > 0 && tripRoom > 0) { + const fits = Math.round(tripRoom / convert(1) / displayUnit) * displayUnit; + back = Math.max(0, Math.min(directRoom, fits)); + backExact = convert(back / 100) * 100; + const backTrip = Math.round(backExact); + from.direct -= back; + to.direct += back; + from.cents += backTrip; + to.cents -= backTrip; + } + } + const movedDirect = paidDirect - back; + reimbursedDirect[s.to_user_id] = (reimbursedDirect[s.to_user_id] || 0) + movedDirect; + reimbursedDirect[s.from_user_id] = (reimbursedDirect[s.from_user_id] || 0) - movedDirect; // Net of the transfers in both directions: sending one back is a reimbursement // received in reverse, and netting them is what keeps the final budget's // subtraction equal to the balance it is taken from. - const shown = finalUnit(exact); + const shown = finalUnit(exact) + finalUnit(backExact); reimbursedCents[s.to_user_id] = (reimbursedCents[s.to_user_id] || 0) + shown; reimbursedCents[s.from_user_id] = (reimbursedCents[s.from_user_id] || 0) - shown; const moved = { settlement_id: s.id, from_user_id: s.from_user_id, to_user_id: s.to_user_id }; - (movedRows[s.to_user_id] ??= []).push({ ...moved, cents: shown, direct: paidDirect }); - (movedRows[s.from_user_id] ??= []).push({ ...moved, cents: -shown, direct: -paidDirect }); + (movedRows[s.to_user_id] ??= []).push({ ...moved, cents: shown, direct: movedDirect }); + (movedRows[s.from_user_id] ??= []).push({ ...moved, cents: -shown, direct: -movedDirect }); } // Into the display currency as one set, then simplify — balances and flows are diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index 4594397d5..cd8389ab7 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -17,7 +17,7 @@ import { AssignmentsService } from '../assignments/assignments.service'; import { idSchema, transportLegInputSchema, reservationUrlSchema, type TransportLegInput, bookingPriceSchema, bookingBudgetCategorySchema, TRANSPORT_RESERVATION_TYPES, MCP_CREATABLE_TRANSPORT_TYPES, LEG_RESERVATION_TYPES, BOOKING_RESERVATION_TYPES, - RESERVATION_STATUSES, type ReservationStatus, + RESERVATION_STATUSES, type ReservationStatus, typeToCostCategory, } from '@trek/shared'; import { RESERVATION_METADATA } from '../../db/json-columns'; import { decodeJson } from '../../utils/json-column'; @@ -409,7 +409,7 @@ export class ReservationsMcp { notes, day_id, place_id, assignment_id, create_accommodation: createAccommodation, metadata, - }, linkedCost(price, budget_category || type), await this.guards.tripWriter(tripId, ctx.userId)); + }, linkedCost(price, budget_category || typeToCostCategory(type)), await this.guards.tripWriter(tripId, ctx.userId)); return ok(sameFare ? { reservation, warning: sameFareWarning(sameFare) } : { reservation }); } @@ -749,7 +749,7 @@ export class ReservationsMcp { metadata: Object.keys(meta).length > 0 ? meta : undefined, endpoints: transportEndpoints, needs_review, - }, linkedCost(price, budget_category || type), await this.guards.tripWriter(tripId, ctx.userId)); + }, linkedCost(price, budget_category || typeToCostCategory(type)), await this.guards.tripWriter(tripId, ctx.userId)); return ok(sameFare ? { reservation, warning: sameFareWarning(sameFare) } : { reservation }); } diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index e94390194..f45bae7e1 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -10,7 +10,7 @@ import { keepMirroredPrice } from './reservation-metadata'; import { endpointRows, groupDayPositions, groupEndpointsByReservation } from './reservations.helpers'; import type { Reservation, User } from '../../types'; import { BudgetService } from '../budget/budget.service'; -import { typeToCostCategory } from '@trek/shared'; +import { costCategoryOf, typeToCostCategory } from '@trek/shared'; import { NotificationsService } from '../notifications/notifications.service'; import { AccommodationsService, noStayMirror, type AccommodationMirror } from '../accommodations/accommodations.service'; import { toRowId, legacyBoundIntegerText } from '../common/row-id'; @@ -157,6 +157,9 @@ type AccommodationTimesMeta = { * transaction, the linked-cost side effects and the booking notification, so * REST, MCP and the plugin RPC (ReservationsRpc) stay thin. */ +/** The booking types whose cost the old type map filed under 'other' (see syncBudgetOnUpdate). */ +const FILED_AS_OTHER_BEFORE = new Set(['cable_car', 'event', 'tour', 'cruise', 'bicycle', 'transit', 'transport_other']); + @Injectable() export class ReservationsService { constructor( @@ -1148,8 +1151,14 @@ export class ReservationsService { const oldCat = typeToCostCategory(currentType); const newCat = typeToCostCategory(type); // RS51. Every linked expense (#2084), each only while it still has the derived category. + // Read through costCategoryOf, so a label stored before the fixed keys ("Flight") + // counts as the key it stands for and moves with the type too. + // Before 5.1.1 these types filed their cost under 'other', so an 'other' on one of + // them is still the derived category and moves with the type as it did. + const derived = (category: string | null) => + costCategoryOf(category) === oldCat || (category === 'other' && FILED_AS_OTHER_BEFORE.has(currentType)); const linked = oldCat === newCat ? [] : await this.budgetItemsRepo.listIdAndCategoryByReservation(tripId, id); - for (const item of linked.filter(i => i.category === oldCat)) { + for (const item of linked.filter(i => derived(i.category))) { const updated = await this.budget.updateBudgetItem(item.id, tripId, { category: newCat }); this.sendCost(tripId, { event: 'budget:updated', payload: { item: updated } }, socketId, collect); } diff --git a/server/src/nest/transit/transit.mcp.ts b/server/src/nest/transit/transit.mcp.ts index ce4adc027..20ecb0d8a 100644 --- a/server/src/nest/transit/transit.mcp.ts +++ b/server/src/nest/transit/transit.mcp.ts @@ -284,8 +284,8 @@ export class TransitMcp { } // The fare files the linked expense in the booking's own transaction and // rides along in the metadata, as create_transport's price does. `transit` - // is no Costs category (typeToCostCategory would file it under other), so a - // fare defaults to the one every rail and bus booking maps to. + // is no Costs category of its own, so a fare defaults to the one every rail + // and bus booking maps to (typeToCostCategory('transit') says the same). const linkedCost = price != null && price > 0 ? { total_price: price, category: budget_category || 'transport' } : undefined; const { reservation } = await this.writes.createBooking( diff --git a/server/tests/unit/mcp/tools-reservations.test.ts b/server/tests/unit/mcp/tools-reservations.test.ts index 1ab2ea541..586e29ef1 100644 --- a/server/tests/unit/mcp/tools-reservations.test.ts +++ b/server/tests/unit/mcp/tools-reservations.test.ts @@ -93,6 +93,21 @@ describe('Tool: create_reservation', () => { }); }); + it('files the cost of a priced booking under its Costs key when no budget_category is given', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'create_reservation', + arguments: { tripId: trip.id, title: 'Jazz night', type: 'event', price: 40 }, + }); + const data = parseToolResult(result) as { reservation: { id: number } }; + const item = await findRow(orm, BudgetItems, { reservation: data.reservation.id }); + // Not a separate "event" bucket next to other: the key the web form files it under. + expect(item?.category).toBe('activities'); + }); + }); + it('creates a parking reservation (#1444)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/budget.service.calc.test.ts b/server/tests/unit/nest/budget.service.calc.test.ts index f455b6ea8..46c22965a 100644 --- a/server/tests/unit/nest/budget.service.calc.test.ts +++ b/server/tests/unit/nest/budget.service.calc.test.ts @@ -1437,6 +1437,27 @@ describe('calculateSettlement: currencies shown in whole units (#2604)', () => { } }); + it('a bill in a won display currency on a euro trip is split in whole won, and settle-up clears it', async () => { + // Read in won, a won bill counts as typed (#2965) and skips the conversion that + // rounds to whole won, so it has to be split in whole won itself: 3,333.33 each + // printed as -3,333 / -3,333 beside +6,667 again. + const fx = { base: 'KRW', tripCurrency: 'EUR', rates: { EUR: 1, KRW: 1500 } }; + const items = [{ ...makeItem(1, 10000), currency: 'KRW', exchange_rate: 1500 } as BudgetItem]; + const members = [makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol')]; + const payers = [makePayer(1, 1, 10000, 'alice')]; + await setupDb(items, members, payers); + + const result = await budget.calculateSettlement(1, fx); + expect(result.currency).toBe('KRW'); + expectWholeAndSquare(result); + expect(result.balances.map((b) => b.balance).sort((x, y) => x - y)).toEqual([-3334, -3333, 6667]); + + const after = await settleEverything(items, members, payers, fx, 'KRW'); + expectWholeAndSquare(after); + expect(after.balances.every((b) => b.balance === 0)).toBe(true); + expect(after.flows).toEqual([]); + }); + it('a two-decimal display currency keeps its cents', async () => { // The euro case the rest of this file pins, read once more beside the won ones: // 10.00 three ways stays 3.34 / 3.33 / 3.33, not whole euros. @@ -1629,6 +1650,40 @@ describe('calculateSettlement: bills in the display currency (#2965)', () => { expect(centSum(later.balances.map((b) => b.balance))).toBe(0); }); + it('settled when the halves point opposite ways, it stays settled when the yen moves', async () => { + // Bob paid a 100 EUR train for both, Alice a 100,000 yen hotel for both: Alice + // owes Bob 50 EUR in euros, Bob owes Alice 50,000 yen. Bob pays the net in euros. + // The euro credit used to stay in euros beside the yen it was netted with, and + // the balance reopened as soon as the rate moved. + const items = () => [ + { ...makeItem(1, 100), currency: 'EUR', exchange_rate: 1 / 176.37 } as BudgetItem, + { ...makeItem(2, 100000) } as BudgetItem, + ]; + const members = () => [...pair(), makeMember(2, 1, 'alice'), makeMember(2, 2, 'bob')]; + const payers = () => [makePayer(1, 2, 100, 'bob'), makePayer(2, 1, 100000, 'alice')]; + await setupDb(items(), members(), payers()); + const offer = (await budget.calculateSettlement(1, today)).flows; + expect(offer).toHaveLength(1); + expect(offer[0].from.user_id).toBe(2); + await setupDb(items(), members(), payers(), [makeSettlementRow(5, 2, 1, offer[0].amount, 'EUR', 1 / 176.73)]); + const results = await Promise.all( + [1 / 176.73, 1 / 160, 1 / 195].map((eurPerYen) => + budget.calculateSettlement(1, { ...today, rates: { JPY: 1, EUR: eurPerYen } }), + ), + ); + for (const result of results) { + expect(result.balances.map((b) => b.balance)).toEqual([0, 0]); + expect(result.flows).toEqual([]); + for (const f of result.finalBudgets) { + expect(f.sources.moved.reduce((a, r) => a + r.cents, 0)).toBe(Math.round(f.reimbursed * 100)); + expect(f.final).toBeCloseTo(f.expenses - f.reimbursed - f.pending, 2); + } + } + // What Bob sent is what Alice received, read on the day it was sent. + const alice = results[0].finalBudgets.find((f) => f.user_id === 1); + expect(alice?.reimbursed).toBe(offer[0].amount); + }); + it('read in the trip currency nothing changes: the bill goes through its frozen rate', async () => { await setupDb(flight(), pair(), paid()); const result = await budget.calculateSettlement(1, { base: 'JPY', tripCurrency: 'JPY', rates: null }); diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index 5e661d008..364f8fde4 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -725,6 +725,34 @@ describe('ReservationsService (DI-native, real SQL)', () => { expect(broadcast).not.toHaveBeenCalled(); }); + it('RESV-SVC-044: a type change also re-files a legacy label and a cable car cost the MCP filed under transport', async () => { + const { trip, res, item: legacy } = await linkedItem({ name: 'Fare', category: 'Flight' }); + budget.updateBudgetItem.mockImplementation((id: number) => ({ id, category: 'transport' })); + // "Flight" was stored before the fixed keys; it still stands for flights. + await svc.syncBudgetOnUpdate(String(trip.id), String(res.id), 'X', 'train', 'X', 'flight', undefined, 'sock'); + expect(budget.updateBudgetItem).toHaveBeenCalledWith(legacy.id, String(trip.id), { category: 'transport' }); + + budget.updateBudgetItem.mockClear(); + const { trip: t2, res: r2, item: gondola } = await linkedItem({ name: 'Gondola', category: 'transport' }); + budget.updateBudgetItem.mockImplementation((id: number) => ({ id, category: 'flights' })); + // A cable car files under transport on every path now, so the move reaches it. + await svc.syncBudgetOnUpdate(String(t2.id), String(r2.id), 'X', 'flight', 'X', 'cable_car', undefined, 'sock'); + expect(budget.updateBudgetItem).toHaveBeenCalledWith(gondola.id, String(t2.id), { category: 'flights' }); + }); + + it('RESV-SVC-045: an "other" cost filed by the old type map still moves with the type', async () => { + const { trip, res, item } = await linkedItem({ name: 'Ticket', category: 'other' }); + budget.updateBudgetItem.mockImplementation((id: number) => ({ id, category: 'flights' })); + await svc.syncBudgetOnUpdate(String(trip.id), String(res.id), 'X', 'flight', 'X', 'event', undefined, 'sock'); + expect(budget.updateBudgetItem).toHaveBeenCalledWith(item.id, String(trip.id), { category: 'flights' }); + + budget.updateBudgetItem.mockClear(); + const { trip: t2, res: r2 } = await linkedItem({ name: 'Snacks', category: 'other' }); + // A flight never filed under 'other': there it was picked by hand and stays. + await svc.syncBudgetOnUpdate(String(t2.id), String(r2.id), 'X', 'train', 'X', 'flight', undefined, 'sock'); + expect(budget.updateBudgetItem).not.toHaveBeenCalled(); + }); + it('updates an existing linked item when a price is provided', async () => { const { trip, res, item } = await linkedItem(); budget.updateBudgetItem.mockReturnValue({ id: item.id }); diff --git a/shared/src/budget/budget.schema.spec.ts b/shared/src/budget/budget.schema.spec.ts index 7f36ee5d3..4bbe5119d 100644 --- a/shared/src/budget/budget.schema.spec.ts +++ b/shared/src/budget/budget.schema.spec.ts @@ -12,8 +12,8 @@ import { budgetItemsPageSchema, BUDGET_ITEMS_PAGE_DEFAULT, COST_CATEGORIES, - typeToCostCategory, } from './budget.schema'; +import { typeToCostCategory } from './cost-category'; import { describe, it, expect } from 'vitest'; diff --git a/shared/src/budget/budget.schema.ts b/shared/src/budget/budget.schema.ts index ff6014df8..5febac67a 100644 --- a/shared/src/budget/budget.schema.ts +++ b/shared/src/budget/budget.schema.ts @@ -54,36 +54,6 @@ export const COST_CATEGORIES = [ ] as const; export type CostCategory = (typeof COST_CATEGORIES)[number]; -/** - * Maps a reservation `type` (flight, train, hotel, …) to one of the fixed Costs - * categories, so an expense created from a booking lands in the right bucket - * instead of a free-text/localized label. Unknown types fall back to `other`. - */ -const RESERVATION_TYPE_TO_COST_CATEGORY: Record = { - flight: 'flights', - plane: 'flights', - train: 'transport', - bus: 'transport', - car: 'transport', - 'car-rental': 'transport', - ferry: 'transport', - boat: 'transport', - taxi: 'transport', - transfer: 'transport', - transport: 'transport', - hotel: 'accommodation', - accommodation: 'accommodation', - lodging: 'accommodation', - parking: 'parking', - restaurant: 'food', - activity: 'activities', -}; - -export function typeToCostCategory(type: string | null | undefined): CostCategory { - if (!type) return 'other'; - return RESERVATION_TYPE_TO_COST_CATEGORY[type.trim().toLowerCase()] || 'other'; -} - /** * One payer of an expense — a row of budget_item_payers. `amount` is in the * expense's own currency (budget_items.currency). Several payers can split who diff --git a/shared/src/budget/cost-category.spec.ts b/shared/src/budget/cost-category.spec.ts index 95c5e7bbf..4f4b98f3a 100644 --- a/shared/src/budget/cost-category.spec.ts +++ b/shared/src/budget/cost-category.spec.ts @@ -1,5 +1,7 @@ -// COST-CATEGORY-001 to COST-CATEGORY-005: a typed category read as one of the fixed keys. -import { COST_CATEGORY_HINT, costCategoryOf, storedCostCategory } from './cost-category'; +// COST-CATEGORY-001 to COST-CATEGORY-007: a typed category read as one of the fixed keys. +import { COST_CATEGORIES } from './budget.schema'; +import { COST_CATEGORY_HINT, costCategoryOf, storedCostCategory, typeToCostCategory } from './cost-category'; +import { RESERVATION_TYPES } from '../reservation/reservation-types'; import { describe, expect, it } from 'vitest'; @@ -37,4 +39,23 @@ describe('costCategoryOf', () => { for (const key of ['accommodation', 'food', 'transport', 'flights', 'other']) expect(COST_CATEGORY_HINT).toContain(key); }); + + it('COST-CATEGORY-006: every booking type is filed under a key, the same one on every path', () => { + for (const { type } of RESERVATION_TYPES) { + expect(COST_CATEGORIES).toContain(typeToCostCategory(type)); + // The MCP stores the raw type through storedCostCategory; it must land where the form puts it. + expect(storedCostCategory(type)).toBe(typeToCostCategory(type)); + } + expect(typeToCostCategory('event')).toBe('activities'); + expect(typeToCostCategory('cable_car')).toBe('transport'); + expect(typeToCostCategory('car-rental')).toBe('transport'); + expect(typeToCostCategory('car rental')).toBe('transport'); + }); + + it('COST-CATEGORY-007: a type no key stands for is filed under other', () => { + expect(typeToCostCategory('spaceship')).toBe('other'); + expect(typeToCostCategory('')).toBe('other'); + expect(typeToCostCategory(null)).toBe('other'); + expect(typeToCostCategory(undefined)).toBe('other'); + }); }); diff --git a/shared/src/budget/cost-category.ts b/shared/src/budget/cost-category.ts index b9879fde9..48fa1ee92 100644 --- a/shared/src/budget/cost-category.ts +++ b/shared/src/budget/cost-category.ts @@ -4,6 +4,8 @@ import { COST_CATEGORIES, type CostCategory } from './budget.schema'; * Free-text and legacy categories (and booking type labels) mapped to the fixed * keys. Bookings used to store labels like "Flight"/"Train"/"Other", and an * assistant writing through the MCP says "Food" where the key is "food". + * Every reservation type is in here too, so a booking's cost lands on the same + * key whether the web form, the import or the MCP wrote it. */ const CATEGORY_ALIASES: Record = { flight: 'flights', @@ -14,11 +16,16 @@ const CATEGORY_ALIASES: Record = { bus: 'transport', car: 'transport', 'car rental': 'transport', + 'car-rental': 'transport', + bicycle: 'transport', + cruise: 'transport', ferry: 'transport', boat: 'transport', taxi: 'transport', transfer: 'transport', cable_car: 'transport', + transit: 'transport', + transport_other: 'transport', transport: 'transport', transportation: 'transport', hotel: 'accommodation', @@ -34,6 +41,8 @@ const CATEGORY_ALIASES: Record = { groceries: 'groceries', activity: 'activities', activities: 'activities', + event: 'activities', + tour: 'activities', sightseeing: 'sightseeing', sights: 'sightseeing', shop: 'shopping', @@ -66,6 +75,15 @@ export function costCategoryOf(category: string | null | undefined): CostCategor return CATEGORY_ALIASES[category.trim().toLowerCase()] ?? null; } +/** + * The Costs category a booking of this type is filed under, `other` for a type + * no key stands for. Reads the same table as costCategoryOf, so a cost created + * with a booking and the type-change resync never disagree about it. + */ +export function typeToCostCategory(type: string | null | undefined): CostCategory { + return costCategoryOf(type) ?? 'other'; +} + /** * The category an expense is stored under: the fixed key when the text names one, * so "Food" and "food" are one category, and the text itself otherwise.