diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 63ba869e9..d2c045003 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -7,7 +7,7 @@ "src/db/repositories/Users.repository.ts": 1445, "src/nest/atlas/atlas.service.ts": 1182, "src/nest/auth/auth.service.ts": 1143, - "src/nest/budget/budget.service.ts": 1622, + "src/nest/budget/budget.service.ts": 1617, "src/nest/collections/collections.service.ts": 1478, "src/nest/doc-sync/doc-sync.service.ts": 1060, "src/nest/doc-sync/providers/synology-drive.client.ts": 1160, diff --git a/server/src/nest/budget/budget-money.helpers.ts b/server/src/nest/budget/budget-money.helpers.ts index 345492234..1d18c76bf 100644 --- a/server/src/nest/budget/budget-money.helpers.ts +++ b/server/src/nest/budget/budget-money.helpers.ts @@ -1,4 +1,4 @@ -import { sumMinor } from '@trek/shared'; +import { splitEqualShares, sumMinor, toMinor } from '@trek/shared'; /** * Re-denominate whole trip-currency cents into whole display-currency cents @@ -54,6 +54,65 @@ export function allocateDisplayCents(cents: number[], factor: number, total?: nu return unit === 1 ? out : out.map((v) => v * unit); } +/** + * A custom split's member amounts as trip cents that add up to what the payers were + * credited. Converting each amount on its own rounds each one to a cent, so four + * quarters of a 136,000 ISK bill came to 4 x 249.38 = 997.52 EUR against the 997.51 + * the payer was credited: every such bill left the balances a cent off zero, a cent + * the settle-up list then had no payment for. When the amounts add up to the payers' + * total in the bill's own currency, as the write path insists, they are allocated + * against `creditCents` by largest remainder, ties to the lower user id. A split that + * does not add up (rows from before that check) still converts member by member. + */ +export function customSplitCents( + members: readonly { user_id: number; amount?: number | null }[], + convert: (amount: number) => number, + creditCents: number, + paidMinor: number, +): Record { + const shares: Record = {}; + const own = members.map((m) => m.amount ?? 0); + if (members.length === 0) return shares; + if (sumMinor(own) !== paidMinor) { + members.forEach((m, i) => (shares[m.user_id] = toMinor(convert(own[i])))); + return shares; + } + const exact = own.map((amount) => convert(amount) * 100); + const cents = exact.map((v) => Math.floor(v)); + let drift = creditCents - cents.reduce((a, c) => a + c, 0); + const rows = members.map((m, i) => ({ i, userId: m.user_id, frac: exact[i] - cents[i] })); + const up = [...rows].sort((a, b) => b.frac - a.frac || a.userId - b.userId); + const down = [...rows].sort((a, b) => a.frac - b.frac || a.userId - b.userId); + for (let k = 0; drift > 0; k++, drift--) cents[up[k % up.length].i] += 1; + for (let k = 0; drift < 0; k++, drift++) cents[down[k % down.length].i] -= 1; + members.forEach((m, i) => (shares[m.user_id] = cents[i])); + return shares; +} + +/** + * Each member's part of one expense for the per-person summary, in trip cents. A + * member with an amount of their own owes exactly that, the rest split the total + * evenly, as the query this replaced did. When every member has an amount, the + * amounts are converted as one set (customSplitCents) the way the settlement + * converts them, so the parts add up to the bill. + */ +export function summaryShareCents( + members: readonly { user_id: number; amount?: number | null }[], + convert: (amount: number) => number, + totalPrice: number, + itemId: number, +): Record { + const totalCents = toMinor(convert(totalPrice)); + const hasAmount = (m: { amount?: number | null }) => m.amount !== null && m.amount !== undefined; + if (members.length > 0 && members.every(hasAmount)) { + return customSplitCents(members, convert, totalCents, toMinor(totalPrice)); + } + const equal = splitEqualShares(totalCents, members, itemId); + return Object.fromEntries( + members.map((m) => [m.user_id, hasAmount(m) ? toMinor(convert(m.amount as number)) : equal[m.user_id] || 0]), + ); +} + /** * What `exchange_rate` holds on a row whose rate was never frozen. The column is * `REAL NOT NULL DEFAULT 1`, so without a migration 1 is the only way to store "no diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index f9416e91b..4469f92c3 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -1,7 +1,7 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; import type { BudgetFallbackFx, BudgetParticipantFinal, BudgetUnconverted, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; -import { currencyDecimals, splitEqualShares, toMinor } from '@trek/shared'; +import { currencyDecimals, splitEqualShares, sumMinor, toMinor } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; @@ -31,7 +31,7 @@ import { BUDGET_ITEM_TICKET, RESERVATION_METADATA } from '../../db/json-columns' import { decodeJson, decodeJsonResult, encodeJson } from '../../utils/json-column'; import { DomainError } from '../common/domain-error'; import { GUEST_HAS_PAYMENTS, shareAfterLeaving } from './budget-departure.helpers'; -import { allocateDisplayCents, isFrozenRate, NOT_FROZEN_RATE, quoteRatio, sumMoney, tripConverter } from './budget-money.helpers'; +import { allocateDisplayCents, customSplitCents, isFrozenRate, NOT_FROZEN_RATE, quoteRatio, summaryShareCents, sumMoney, tripConverter } from './budget-money.helpers'; type Trip = TripAccess; @@ -992,12 +992,9 @@ export class BudgetService { // A foreign row nothing can convert counts for nobody, as in the settlement. const convert = tripConverter(item.currency, item.exchange_rate, tripCurrency, rates); if (!convert) continue; - const toTripCents = (amount: number) => toMinor(convert(amount)); - // A member with an amount of their own owes exactly that; the rest split the - // total evenly, as the query this replaces did. - const equal = splitEqualShares(toTripCents(item.total_price || 0), own, item.id); + const shares = summaryShareCents(own, convert, item.total_price || 0, item.id); for (const m of own) { - const share = m.amount !== null && m.amount !== undefined ? toTripCents(m.amount) : (equal[m.user_id] || 0); + const share = shares[m.user_id] || 0; let p = people.get(m.user_id); if (!p) { p = { user_id: m.user_id, username: m.username, avatar: m.avatar, assigned: 0, paid: 0, items_count: 0 }; @@ -1184,7 +1181,6 @@ export class BudgetService { unconvertedCurrencies.add((item.currency || '').toUpperCase()); continue; } - const toTripCents = (amount: number): number => toMinor(convert(amount)); const members = allMembers.filter(m => m.budget_item_id === item.id); const payers = allPayers.filter(p => p.budget_item_id === item.id); if (members.length === 0) continue; // planning-only entry → doesn't affect balances @@ -1231,12 +1227,11 @@ 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); - const equalShares = !hasCustomSplit ? splitEqualShares(creditCents, members, item.id) : {}; + const shares = hasCustomSplit + ? customSplitCents(members, convert, creditCents, sumMinor(payers.map(p => p.amount))) + : splitEqualShares(creditCents, members, item.id); for (const m of members) { - const memberShare = hasCustomSplit && m.amount !== null && m.amount !== undefined - ? toTripCents(m.amount) - : (equalShares[m.user_id] || 0); - ensure(m.user_id, m).cents -= memberShare; + ensure(m.user_id, m).cents -= shares[m.user_id] || 0; } } diff --git a/server/tests/unit/nest/budget-money.helpers.test.ts b/server/tests/unit/nest/budget-money.helpers.test.ts new file mode 100644 index 000000000..436b803b7 --- /dev/null +++ b/server/tests/unit/nest/budget-money.helpers.test.ts @@ -0,0 +1,32 @@ +import { customSplitCents } from '../../../src/nest/budget/budget-money.helpers'; + +import { describe, expect, it } from 'vitest'; + +const isk = (amount: number) => amount / 136.34; + +describe('customSplitCents', () => { + it('allocates a split that adds up against the credited cents, ties to the lower user id', () => { + const members = [4, 1, 3, 2].map((user_id) => ({ user_id, amount: 34000 })); + expect(customSplitCents(members, isk, 99751, 13600000)).toEqual({ 1: 24938, 2: 24938, 3: 24938, 4: 24937 }); + }); + + it('takes cents back from the smallest fractions when the credit rounded down', () => { + const members = [ + { user_id: 1, amount: 0.5 }, + { user_id: 2, amount: 0.5 }, + ]; + expect(customSplitCents(members, (a) => a * 1.005, 99, 100)).toEqual({ 1: 49, 2: 50 }); + }); + + it('keeps the member-by-member rounding for a split that does not add up to the payers', () => { + const members = [ + { user_id: 1, amount: 34000 }, + { user_id: 2, amount: null }, + ]; + expect(customSplitCents(members, isk, 99751, 13600000)).toEqual({ 1: 24938, 2: 0 }); + }); + + it('splits nothing among nobody', () => { + expect(customSplitCents([], isk, 100, 100)).toEqual({}); + }); +}); diff --git a/server/tests/unit/nest/budget.foreign-custom-split.test.ts b/server/tests/unit/nest/budget.foreign-custom-split.test.ts new file mode 100644 index 000000000..41f7ac872 --- /dev/null +++ b/server/tests/unit/nest/budget.foreign-custom-split.test.ts @@ -0,0 +1,69 @@ +/** + * A custom split in a foreign currency adds up to what its payer was credited. + * BUDGET-SVC-DB-095 and 096. + * + * 136,000 ISK at 136.34 ISK per euro is 997.51 EUR, but each 34,000 ISK quarter + * is 249.376 and rounded up on its own, so four of them came to 997.52: the + * balances ended a cent off zero, a cent the settle-up list had no payment for. + */ +import { BudgetService } from '../../../src/nest/budget/budget.service'; +import { TripMembersModule } from '../../../src/nest/trip-members/trip-members.module'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { addTripMember, makeTrip } from '../../helpers/factories/trips'; +import { makeUser } from '../../helpers/factories/users'; +import { createTestModule, type TestModule } from '../../helpers/test-module'; +import { MikroORM } from '@mikro-orm/core'; + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +const db = createSnapshotTestDb(); +let t: TestModule; +let budget: BudgetService; +const orm = () => t.get(MikroORM); + +beforeAll(async () => { + t = await createTestModule({ db, imports: [TripMembersModule] }); + budget = t.get(BudgetService); +}); + +afterAll(async () => { + await t.close(); + db.close(); +}); + +async function seedQuarters() { + const users = []; + for (let i = 0; i < 4; i++) users.push((await makeUser(orm())).user); + const trip = await makeTrip(orm(), users[0].id); + for (const u of users.slice(1)) await addTripMember(orm(), trip.id, u.id); + await budget.createBudgetItem(trip.id, { + name: 'Sky Lagoon', + currency: 'ISK', + exchange_rate: 136.34, + payers: [{ user_id: users[0].id, amount: 136000 }], + members: users.map((u) => ({ user_id: u.id, amount: 34000 })), + }); + return { trip, users }; +} + +describe('a custom split in a foreign currency', () => { + it('BUDGET-SVC-DB-095: the balances add up to zero and settle-up clears every one of them', async () => { + const { trip, users } = await seedQuarters(); + const result = await budget.calculateSettlement(trip.id, { tripCurrency: 'EUR' }); + + const cents = result.balances.map((b) => Math.round(b.balance * 100)); + expect(cents.reduce((a, c) => a + c, 0)).toBe(0); + // The payer has the lowest id, so the tie hands them one of the three rounded-up quarters. + expect(result.balances.find((b) => b.user_id === users[0].id)!.balance).toBe(748.13); + const owed = result.flows + .filter((f) => f.to.user_id === users[0].id) + .reduce((a, f) => a + Math.round(f.amount * 100), 0); + expect(owed).toBe(74813); + }); + + it('BUDGET-SVC-DB-096: the per-person shares add up to the bill', async () => { + const { trip } = await seedQuarters(); + const people = await budget.getPerPersonSummary(trip.id); + expect(people.reduce((a, p) => a + Math.round(p.total_assigned * 100), 0)).toBe(99751); + }); +});