From 9522e4ce4ab69f94e824e8d7d0f2a199ff78bf51 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 20 Aug 2026 17:25:51 +0200 Subject: [PATCH] fix(budget): keep an uneven split from corrupting the expense total MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 163.21 across two people is 81.61 and 81.60. Both are exact to the cent — the client splits properly — but the server discarded the total it was sent and re-derived it by adding those two as doubles, which lands on 163.20999999999998. That is the number that went into the row. It looked like a display quirk because everything routed through Intl came out right; only the places that print a number as it stands showed it. The stored value was wrong all along, which is why reopening the expense showed it back, and why the price stamped onto a linked booking carried it too. The domain already had this rule and followed it everywhere else — the settlement maths converts to whole cents before it adds anything. The payer sum was the one arithmetic still done in euros, in both the create and the update path, so it now goes through the same rule. The client side is what makes rows that are already stored wrong stop looking wrong: the expense form and the mobile cost sheet seed their total from a cent-clean value, and the three panels that print a booking's price verbatim tidy it when it parses as a number. Free text an import wrote there passes through untouched rather than becoming NaN. Closes #1964 --- client/src/components/Budget/CostsPanel.tsx | 4 +- .../components/Planner/ReservationsPanel.tsx | 4 +- .../mobile/screens/trip/sheets/MCostSheet.tsx | 4 +- .../mobile/screens/trip/tabs/MBookingsTab.tsx | 4 +- .../screens/trip/tabs/MTransportsTab.tsx | 4 +- client/src/utils/formatters.ts | 28 ++++++ client/tests/unit/utils/cleanAmount.test.ts | 63 +++++++++++++ server/src/nest/budget/budget.service.ts | 29 +++++- .../tests/unit/nest/budget.service.db.test.ts | 88 +++++++++++++++++++ 9 files changed, 214 insertions(+), 14 deletions(-) create mode 100644 client/tests/unit/utils/cleanAmount.test.ts diff --git a/client/src/components/Budget/CostsPanel.tsx b/client/src/components/Budget/CostsPanel.tsx index 4bca99248..b00d754ed 100644 --- a/client/src/components/Budget/CostsPanel.tsx +++ b/client/src/components/Budget/CostsPanel.tsx @@ -10,7 +10,7 @@ import { useTranslation } from '../../i18n' import { budgetApi } from '../../api/client' import { useExchangeRates } from '../../hooks/useExchangeRates' import { useIsMobile } from '../../hooks/useIsMobile' -import { formatMoney, currencyDecimals, currencyLocale, localizeAmountInput } from '../../utils/formatters' +import { formatMoney, currencyDecimals, currencyLocale, localizeAmountInput, cleanAmount } from '../../utils/formatters' import { downloadBlob } from '../../utils/fileDownload' import Modal from '../shared/Modal' import CustomSelect from '../shared/CustomSelect' @@ -1020,7 +1020,7 @@ export function ExpenseModal({ tripId, base, people, me, editing, prefill, onClo const [day, setDay] = useState(editing?.expense_date || new Date().toISOString().slice(0, 10)) const [note, setNote] = useState(() => readUserNote(editing)) const [total, setTotal] = useState(() => { - if (editing) return editing.total_price ? String(editing.total_price) : '' + if (editing) return editing.total_price ? String(cleanAmount(editing.total_price)) : '' if (prefill?.amount != null) return String(prefill.amount) return '' }) diff --git a/client/src/components/Planner/ReservationsPanel.tsx b/client/src/components/Planner/ReservationsPanel.tsx index d096d98d8..fe97ad203 100644 --- a/client/src/components/Planner/ReservationsPanel.tsx +++ b/client/src/components/Planner/ReservationsPanel.tsx @@ -22,7 +22,7 @@ import type { ViewContribution } from '../../api/client' import { usePluginViewContributions, PluginCardFooter } from '../Plugins/PluginContributions' import { usePluginStore, type ActivePlugin } from '../../store/pluginStore' import PluginFrame from '../Plugins/PluginFrame' -import { splitReservationDateTime, formatTime } from '../../utils/formatters' +import { splitReservationDateTime, formatTime, cleanAmountText } from '../../utils/formatters' import { getFlightLegs, getTrainLegs } from '../../utils/flightLegs' import EmptyState from '../shared/EmptyState' import { TravelerAvatarRow, TravelerFilterAvatars } from './TravelerPicker' @@ -378,7 +378,7 @@ function ReservationCard({ r, tripId, onEdit, onDelete, files = [], onNavigateTo if (meta.train_number) cells.push({ label: t('reservations.meta.trainNumber'), value: meta.train_number }) if (meta.platform) cells.push({ label: t('reservations.meta.platform'), value: meta.platform }) if (meta.seat) cells.push({ label: t('reservations.meta.seat'), value: meta.seat + (meta.class ? ` · ${meta.class}` : '') }) - if (meta.price != null && meta.price !== '') cells.push({ label: t('reservations.price'), value: `${meta.price}${meta.priceCurrency ? ' ' + meta.priceCurrency : ''}` }) + if (meta.price != null && meta.price !== '') cells.push({ label: t('reservations.price'), value: `${cleanAmountText(meta.price)}${meta.priceCurrency ? ' ' + meta.priceCurrency : ''}` }) if (meta.check_in_time) cells.push({ label: t('reservations.meta.checkIn'), value: formatTime(meta.check_in_time, locale, timeFormat) + (meta.check_in_end_time ? ` – ${formatTime(meta.check_in_end_time, locale, timeFormat)}` : '') }) if (meta.check_out_time) cells.push({ label: t('reservations.meta.checkOut'), value: formatTime(meta.check_out_time, locale, timeFormat) }) if (cells.length === 0) return null diff --git a/client/src/mobile/screens/trip/sheets/MCostSheet.tsx b/client/src/mobile/screens/trip/sheets/MCostSheet.tsx index fc57e938d..8e3108bcb 100644 --- a/client/src/mobile/screens/trip/sheets/MCostSheet.tsx +++ b/client/src/mobile/screens/trip/sheets/MCostSheet.tsx @@ -9,7 +9,7 @@ import { useTranslation } from '../../../../i18n' import { useToast } from '../../../../components/shared/Toast' import { useTripStore } from '../../../../store/tripStore' import { useExchangeRates } from '../../../../hooks/useExchangeRates' -import { formatMoney, localizeAmountInput } from '../../../../utils/formatters' +import { formatMoney, localizeAmountInput, cleanAmount } from '../../../../utils/formatters' import { SYMBOLS, SPLIT_COLORS, currenciesWith } from '../../../../components/Budget/BudgetPanel.constants' import { COST_CATEGORY_LIST, catMeta } from '../../../../components/Budget/costsCategories' import { calculateTicketShares, hasTicketSplit, NOTE_MAX, readTicketItems, readUserNote, splitEqualShares, writeTicketItems, type TicketItem } from '../../../../components/Budget/CostsPanel.helpers' @@ -74,7 +74,7 @@ export default function MCostSheet({ tripId, base, people, me, editing, prefill, const [currency, setCurrency] = useState((editing?.currency || base).toUpperCase()) const [day, setDay] = useState(editing?.expense_date || new Date().toISOString().slice(0, 10)) const [total, setTotal] = useState(() => { - if (editing) return editing.total_price ? String(editing.total_price) : '' + if (editing) return editing.total_price ? String(cleanAmount(editing.total_price)) : '' if (prefill?.amount != null) return String(prefill.amount) return '' }) diff --git a/client/src/mobile/screens/trip/tabs/MBookingsTab.tsx b/client/src/mobile/screens/trip/tabs/MBookingsTab.tsx index 1d76e03d1..c71856d00 100644 --- a/client/src/mobile/screens/trip/tabs/MBookingsTab.tsx +++ b/client/src/mobile/screens/trip/tabs/MBookingsTab.tsx @@ -2,7 +2,7 @@ import { useState } from 'react' import { FileText, MapPin, Pencil, Trash2 } from 'lucide-react' import MDancingTrek from '../../../components/MDancingTrek' import { RES_ICONS } from '../../../../components/Planner/DayPlanSidebar.constants' -import { splitReservationDateTime, formatTime } from '../../../../utils/formatters' +import { splitReservationDateTime, formatTime, cleanAmountText } from '../../../../utils/formatters' import { openFile } from '../../../../utils/fileDownload' import { useTranslation } from '../../../../i18n' import type { Reservation } from '../../../../types' @@ -122,7 +122,7 @@ function BookingCard({ res, planner, canEdit, compact }: { if (meta.platform) metaCells.push({ label: t('reservations.meta.platform'), value: meta.platform }) if (meta.seat) metaCells.push({ label: t('reservations.meta.seat'), value: meta.seat + (meta.class ? ` · ${meta.class}` : '') }) if (meta.price != null && meta.price !== '') { - metaCells.push({ label: t('reservations.price'), value: `${meta.price}${meta.priceCurrency ? ` ${meta.priceCurrency}` : ''}` }) + metaCells.push({ label: t('reservations.price'), value: `${cleanAmountText(meta.price)}${meta.priceCurrency ? ` ${meta.priceCurrency}` : ''}` }) } if (meta.check_in_time) { metaCells.push({ diff --git a/client/src/mobile/screens/trip/tabs/MTransportsTab.tsx b/client/src/mobile/screens/trip/tabs/MTransportsTab.tsx index 4fc00c983..56b16be5a 100644 --- a/client/src/mobile/screens/trip/tabs/MTransportsTab.tsx +++ b/client/src/mobile/screens/trip/tabs/MTransportsTab.tsx @@ -2,7 +2,7 @@ import { useState } from 'react' import { FileText, Pencil, Trash2 } from 'lucide-react' import MDancingTrek from '../../../components/MDancingTrek' import { RES_ICONS } from '../../../../components/Planner/DayPlanSidebar.constants' -import { splitReservationDateTime, formatTime } from '../../../../utils/formatters' +import { splitReservationDateTime, formatTime, cleanAmountText } from '../../../../utils/formatters' import { openFile } from '../../../../utils/fileDownload' import { useTranslation } from '../../../../i18n' import type { Reservation } from '../../../../types' @@ -128,7 +128,7 @@ function TransportCard({ res, planner, shell, canEdit, compact }: { if (meta.platform) metaCells.push({ label: t('reservations.meta.platform'), value: meta.platform }) if (meta.seat) metaCells.push({ label: t('reservations.meta.seat'), value: meta.seat + (meta.class ? ` · ${meta.class}` : '') }) if (meta.price != null && meta.price !== '') { - metaCells.push({ label: t('reservations.price'), value: `${meta.price}${meta.priceCurrency ? ` ${meta.priceCurrency}` : ''}` }) + metaCells.push({ label: t('reservations.price'), value: `${cleanAmountText(meta.price)}${meta.priceCurrency ? ` ${meta.priceCurrency}` : ''}` }) } const files = (planner.files || []).filter( diff --git a/client/src/utils/formatters.ts b/client/src/utils/formatters.ts index cbbc86ed8..75c51f1f2 100644 --- a/client/src/utils/formatters.ts +++ b/client/src/utils/formatters.ts @@ -85,6 +85,34 @@ export function localizeAmountInput(value: string | number | null | undefined, c return currencyDecimalSeparator(currency) === ',' ? s.replace('.', ',') : s } +/** + * The same amount, without the float noise an uneven split leaves behind (#1964). + * + * A total that does not divide evenly is split into parts that are each exact + * to the cent, but adding those parts back up in floating point does not land + * on the total: 81.61 + 81.60 is 163.20999999999998. Anything that reaches + * formatMoney is fine, since Intl rounds — this is for the places that print a + * number as it stands, which is where the reporter saw it: the expense form + * when reopened, and the price stamped onto a linked booking. + * + * In cents, matching the server's own money arithmetic rather than the + * currency's decimal count, so the two agree on what a clean value is. + */ +export function cleanAmount(value: number): number { + return Math.round(value * 100) / 100 +} + +/** + * A price held as free text, tidied only when it really is a number. Import + * writes whatever the confirmation said into this field, so anything that does + * not parse is passed through untouched rather than mangled into NaN. + */ +export function cleanAmountText(value: string | number | null | undefined): string { + if (value == null) return '' + const n = Number(value) + return Number.isFinite(n) && String(value).trim() !== '' ? String(cleanAmount(n)) : String(value) +} + /** * Locale- and currency-correct money formatting via Intl: the symbol position, * thousands/decimal separators and decimal count all follow the user's locale diff --git a/client/tests/unit/utils/cleanAmount.test.ts b/client/tests/unit/utils/cleanAmount.test.ts new file mode 100644 index 000000000..cba664662 --- /dev/null +++ b/client/tests/unit/utils/cleanAmount.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from 'vitest'; +import { cleanAmount, cleanAmountText } from '../../../src/utils/formatters'; + +/** + * Money that has been through an uneven split (#1964). + * + * A total that does not divide evenly is split into parts that are each exact + * to the cent, but adding those parts back in floating point does not return + * the total: 81.61 + 81.60 is 163.20999999999998. The server writes the clean + * value now, so these two are for what is already stored, and for the places + * that print a number as it stands rather than through Intl — the expense form + * when reopened, and the price stamped onto a linked booking. + */ + +describe('cleanAmount', () => { + it('gives back the total the parts were meant to add up to', () => { + expect(cleanAmount(81.61 + 81.60)).toBe(163.21); + expect(cleanAmount(33.34 + 33.33 + 33.33)).toBe(100); + }); + + it('leaves a value that was already clean untouched', () => { + expect(cleanAmount(163.21)).toBe(163.21); + expect(cleanAmount(25)).toBe(25); + expect(cleanAmount(0)).toBe(0); + }); + + it('keeps a genuine half-cent from being invented or lost', () => { + // Rounding to cents is the point; a third of a cent is not money. + expect(cleanAmount(0.005)).toBe(0.01); + expect(cleanAmount(0.004)).toBe(0); + }); + + it('handles a negative the same way, since a refund is money too', () => { + expect(cleanAmount(-81.61 - 81.60)).toBe(-163.21); + }); +}); + +describe('cleanAmountText', () => { + it('tidies a numeric price string', () => { + expect(cleanAmountText('163.20999999999998')).toBe('163.21'); + expect(cleanAmountText(163.20999999999998)).toBe('163.21'); + }); + + /* + * The booking price field holds whatever the confirmation said. Import writes + * free text into it, so anything that is not a number has to come back out + * exactly as it went in rather than as NaN. + */ + it('passes free text through untouched', () => { + expect(cleanAmountText('included in fare')).toBe('included in fare'); + expect(cleanAmountText('EUR 40,00')).toBe('EUR 40,00'); + }); + + it('answers empty for nothing at all', () => { + expect(cleanAmountText(null)).toBe(''); + expect(cleanAmountText(undefined)).toBe(''); + expect(cleanAmountText('')).toBe(''); + }); + + it('does not turn whitespace into a zero', () => { + expect(cleanAmountText(' ')).toBe(' '); + }); +}); diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 0239033ad..9bd4f95c4 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -52,6 +52,23 @@ export function splitLegacyTicketNote( * converted(Σ). `factor === 1` (the display currency IS the trip currency, the * common case) is the identity and never touches a float. */ +/** + * Add money in whole cents, not in floats (#1964). + * + * A total that does not divide evenly is split into parts that are each exact + * to the cent — 163.21 across two people is 81.61 and 81.60 — but adding those + * two doubles gives 163.20999999999998, and that is what got written back over + * the clean total the client sent. It then surfaced anywhere the number is + * printed without Intl doing the rounding: the expense form when reopened, the + * mobile cost sheet, and the price stamped onto a linked booking. + * + * This is the same rule the settlement maths in this file already follows + * (toTripCents), applied to the one arithmetic that had been left in euros. + */ +function sumMoney(amounts: number[]): number { + return amounts.reduce((a, v) => a + Math.round(v * 100), 0) / 100; +} + function allocateDisplayCents(cents: number[], factor: number): number[] { if (factor === 1) return [...cents]; const exact = cents.map(c => c * factor); @@ -145,12 +162,13 @@ export class BudgetService { this.db.run('DELETE FROM budget_item_payers WHERE budget_item_id = ?', itemId); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_payers (budget_item_id, user_id, amount) VALUES (?, ?, ?)'); const known = this.rosterMemberIds(tripId, payers.map(p => p.user_id)); - let total = 0; + const accepted: number[] = []; for (const p of payers) { if (!(p.amount > 0) || !known.has(p.user_id)) continue; insert.run(itemId, p.user_id, p.amount); - total += p.amount; + accepted.push(p.amount); } + const total = sumMoney(accepted); this.db.run('UPDATE budget_items SET total_price = ? WHERE id = ?', total, itemId); return total; } @@ -348,7 +366,7 @@ export class BudgetService { // total_price is derived from explicit payers when given; otherwise the caller // value (planning entries, or a bill no one has paid yet). - const payerTotal = (data.payers || []).reduce((a, p) => a + (p.amount > 0 ? p.amount : 0), 0); + const payerTotal = sumMoney((data.payers || []).filter(p => p.amount > 0).map(p => p.amount)); const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0); const knownMembers = data.members ? this.rosterMemberIds(tripId, data.members.map(m => m.user_id)) : null; @@ -1013,7 +1031,10 @@ export class BudgetService { ); if (!reservation) return; const meta = reservation.metadata ? JSON.parse(reservation.metadata) : {}; - meta.price = String(totalPrice); + // Cent-clean, so a booking never inherits float noise from the expense + // it is linked to — and so a row stamped before #1964 heals on the next + // edit. The panels print this string as it stands. + meta.price = String(Math.round(totalPrice * 100) / 100); this.db.run('UPDATE reservations SET metadata = ? WHERE id = ?', JSON.stringify(meta), reservation.id); const updatedRes = this.db.get('SELECT * FROM reservations WHERE id = ?', reservation.id); this.realtime.broadcast(tripId, 'reservation:updated', { reservation: updatedRes }, socketId); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 6becc5019..7cb55d652 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -685,3 +685,91 @@ describe('post-fold quirk fixes', () => { expect(cat).toBeUndefined(); }); }); + +/** + * The total an uneven split writes back (#1964). + * + * The client splits in whole cents, which is right — 163.21 across two people + * is 81.61 and 81.60. The server then discarded the total it was sent and + * re-derived it by adding those two as doubles, which lands on + * 163.20999999999998, and that is what went into the row. + * + * Everything that reaches Intl was fine, which is why it looked like a display + * quirk in only some places. It was not: the stored number was wrong, and the + * expense form showed it back the moment the item was reopened. + */ +describe('an expense whose split leaves a remainder', () => { + const totalOf = (itemId: number) => + (testDb.prepare('SELECT total_price FROM budget_items WHERE id = ?').get(itemId) as { total_price: number }).total_price; + + it('stores the total the parts add up to, not the float they land on', () => { + const { user: alice } = createUser(testDb); + const { user: bob } = createUser(testDb); + const trip = createTrip(testDb, alice.id); + addTripMember(testDb, trip.id, bob.id); + + const item = budget.createBudgetItem(trip.id, { + name: 'Flight', + payers: [{ user_id: alice.id, amount: 81.61 }, { user_id: bob.id, amount: 81.60 }], + members: [{ user_id: alice.id }, { user_id: bob.id }], + }); + + // The assertion that fails on the old code, with exactly the reported number. + expect(totalOf(item.id)).toBe(163.21); + expect(String(totalOf(item.id))).toBe('163.21'); + }); + + it('does the same when the payers are replaced on an update', () => { + const { user: alice } = createUser(testDb); + const { user: bob } = createUser(testDb); + const trip = createTrip(testDb, alice.id); + addTripMember(testDb, trip.id, bob.id); + + const item = budget.createBudgetItem(trip.id, { name: 'Hotel', total_price: 10, members: [{ user_id: alice.id }] }); + budget.updateBudgetItem(item.id, trip.id, { + payers: [{ user_id: alice.id, amount: 81.61 }, { user_id: bob.id, amount: 81.60 }], + }); + + expect(totalOf(item.id)).toBe(163.21); + }); + + /* + * Three ways is the harder case: 100.00 becomes 33.34 + 33.33 + 33.33, and + * two of those additions drift. + */ + it('holds across a three-way split too', () => { + const { user: alice } = createUser(testDb); + const { user: bob } = createUser(testDb); + const { user: carol } = createUser(testDb); + const trip = createTrip(testDb, alice.id); + addTripMember(testDb, trip.id, bob.id); + addTripMember(testDb, trip.id, carol.id); + + const item = budget.createBudgetItem(trip.id, { + name: 'Dinner', + payers: [ + { user_id: alice.id, amount: 33.34 }, + { user_id: bob.id, amount: 33.33 }, + { user_id: carol.id, amount: 33.33 }, + ], + members: [{ user_id: alice.id }, { user_id: bob.id }, { user_id: carol.id }], + }); + + expect(totalOf(item.id)).toBe(100); + }); + + it('leaves a total that needs no cleaning exactly as it was', () => { + const { user: alice } = createUser(testDb); + const { user: bob } = createUser(testDb); + const trip = createTrip(testDb, alice.id); + addTripMember(testDb, trip.id, bob.id); + + const item = budget.createBudgetItem(trip.id, { + name: 'Taxi', + payers: [{ user_id: alice.id, amount: 12.5 }, { user_id: bob.id, amount: 12.5 }], + members: [{ user_id: alice.id }, { user_id: bob.id }], + }); + + expect(totalOf(item.id)).toBe(25); + }); +})