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); + }); +})