From 37ffb2c3b492580ee3482d3facd9abc865e4f5c0 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 2 Jun 2026 12:52:28 +0200 Subject: [PATCH] =?UTF-8?q?fix(audit):=20financial=20=E2=80=94=20M19=20(gr?= =?UTF-8?q?oup-by-currency=20accumulation,=20full-precision=20rates),=20M2?= =?UTF-8?q?3=20(invoice=20money=20rounding=20+=200%=20discount),=20L25=20(?= =?UTF-8?q?no=20silent=20unconverted/stale=20FX),=20L26=20(companyNotes=20?= =?UTF-8?q?updatedAt)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M23 numeric(12,2) schema precision deferred to a migration. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/services/currency.ts | 37 ++- src/lib/services/invoices.ts | 54 +++- src/lib/services/notes.service.ts | 2 +- src/lib/services/reports/currency.ts | 84 +++++- src/lib/services/reports/financial.service.ts | 243 ++++++++++++------ 5 files changed, 312 insertions(+), 108 deletions(-) diff --git a/src/lib/services/currency.ts b/src/lib/services/currency.ts index bfa85337..9ea728a9 100644 --- a/src/lib/services/currency.ts +++ b/src/lib/services/currency.ts @@ -5,12 +5,31 @@ import { CodedError } from '@/lib/errors'; import { logger } from '@/lib/logger'; import { fetchWithTimeout } from '@/lib/fetch-with-timeout'; +/** + * Reject FX rates older than this when normalising money for reports (audit + * L25). A rate this stale almost certainly means the refresh job has been + * failing silently; using it would mis-state revenue against a months-old + * cross-rate. 14 days gives ample slack over the daily refresh cadence while + * still catching a genuinely-stalled poller. + */ +export const RATE_MAX_AGE_MS = 14 * 24 * 60 * 60 * 1000; + export async function getRate(from: string, to: string): Promise { if (from === to) return 1; const rate = await db.query.currencyRates.findFirst({ where: and(eq(currencyRates.baseCurrency, from), eq(currencyRates.targetCurrency, to)), }); - return rate ? Number(rate.rate) : null; + if (!rate) return null; + // Staleness guard (L25): a rate the refresh job hasn't touched in + // RATE_MAX_AGE_MS is treated as unavailable rather than silently used. + if (rate.fetchedAt && Date.now() - new Date(rate.fetchedAt).getTime() > RATE_MAX_AGE_MS) { + logger.warn( + { from, to, fetchedAt: rate.fetchedAt }, + 'Currency rate is stale beyond RATE_MAX_AGE_MS; treating as unavailable', + ); + return null; + } + return Number(rate.rate); } export async function convert( @@ -49,26 +68,34 @@ export async function refreshRates(): Promise { }); } - // Store inverse rates for common conversions + // Store inverse rates for common conversions. Persist at full precision — + // pre-rounding the inverse to 6dp (former behaviour) meant X→USD and + // USD→X were not exact reciprocals, compounding into report totals (M19). + // The `rate` column is unbounded `numeric`, so full precision is safe. for (const [currency, rate] of Object.entries(rates)) { - const inverse = 1 / rate; + const inverse = String(1 / rate); await db .insert(currencyRates) .values({ baseCurrency: currency, targetCurrency: 'USD', - rate: String(inverse.toFixed(6)), + rate: inverse, source: 'frankfurter', fetchedAt: new Date(), }) .onConflictDoUpdate({ target: [currencyRates.baseCurrency, currencyRates.targetCurrency], - set: { rate: String(inverse.toFixed(6)), fetchedAt: new Date(), source: 'frankfurter' }, + set: { rate: inverse, fetchedAt: new Date(), source: 'frankfurter' }, }); } logger.info({ rateCount: Object.keys(rates).length }, 'Currency rates refreshed'); } catch (err) { + // Log AND rethrow (L25): swallowing here let a months-stale rate persist + // unnoticed. The HTTP route wraps this in errorResponse(); the BullMQ + // maintenance worker marks the job failed and surfaces it via its + // `failed` handler — either way the failure is now visible. logger.error({ err }, 'Failed to refresh currency rates'); + throw err; } } diff --git a/src/lib/services/invoices.ts b/src/lib/services/invoices.ts index 7d8ce659..b9fd5819 100644 --- a/src/lib/services/invoices.ts +++ b/src/lib/services/invoices.ts @@ -23,6 +23,21 @@ import type { ListInvoicesInput, } from '@/lib/validators/invoices'; +// ─── Money helper (audit M23) ─────────────────────────────────────────────── + +/** + * Render a money amount as a fixed-2dp string for persistence into the + * `numeric` money columns. Rounds at the cent before stringifying so JS-float + * artefacts (e.g. `0.1 + 0.2 = 0.30000000000000004`) never reach the DB. + * + * NOTE: the underlying columns are still unbounded `numeric` (no `(12,2)` + * scale) — tightening the schema needs a migration and is tracked as a + * deferred follow-up; this guards the write side in the meantime. + */ +function money(amount: number): string { + return (Math.round(amount * 100) / 100).toFixed(2); +} + // ─── Auto-numbering (BR-041) ─────────────────────────────────────────────── async function generateInvoiceNumber(portId: string, tx: typeof db): Promise { @@ -261,7 +276,11 @@ export async function createInvoice(portId: string, data: CreateInvoiceInput, me .limit(1); if (setting) { - discountPct = Number(setting.value) || 2; + // A legitimately-configured 0% discount must be honoured, not coerced + // back to the 2% default the old `|| 2` produced (M23). Only fall back + // to 2 when the stored value is missing or non-numeric. + const parsed = Number(setting.value); + discountPct = Number.isNaN(parsed) ? 2 : parsed; } else { discountPct = 2; } @@ -319,12 +338,12 @@ export async function createInvoice(portId: string, data: CreateInvoiceInput, me dueDate: data.dueDate, paymentTerms: data.paymentTerms ?? 'net30', currency: data.currency ?? 'USD', - subtotal: String(subtotal), + subtotal: money(subtotal), discountPct: String(discountPct), - discountAmount: String(discountAmount), + discountAmount: money(discountAmount), feePct: String(feePct), - feeAmount: String(feeAmount), - total: String(total), + feeAmount: money(feeAmount), + total: money(total), status: 'draft', paymentStatus: 'unpaid', interestId: data.interestId ?? null, @@ -346,8 +365,8 @@ export async function createInvoice(portId: string, data: CreateInvoiceInput, me invoiceId: newInvoice.id, description: li.description, quantity: String(li.quantity ?? 1), - unitPrice: String(li.unitPrice), - total: String((li.quantity ?? 1) * li.unitPrice), + unitPrice: money(li.unitPrice), + total: money((li.quantity ?? 1) * li.unitPrice), sortOrder: idx, })), ); @@ -450,7 +469,14 @@ export async function updateInvoice( ), ) .limit(1); - discountPct = setting ? Number(setting.value) || 2 : 2; + // `?? 2` semantics (honour a configured 0%), but `Number()` yields NaN + // for a non-numeric stored value, so guard with `Number.isNaN` (M23). + if (setting) { + const parsed = Number(setting.value); + discountPct = Number.isNaN(parsed) ? 2 : parsed; + } else { + discountPct = 2; + } } const discountAmount = (subtotal * discountPct) / 100; @@ -458,12 +484,12 @@ export async function updateInvoice( const feePct = Number(existing.feePct) || 0; const total = subtotal - discountAmount + feeAmount; - updateData.subtotal = String(subtotal); + updateData.subtotal = money(subtotal); updateData.discountPct = String(discountPct); - updateData.discountAmount = String(discountAmount); + updateData.discountAmount = money(discountAmount); updateData.feePct = String(feePct); - updateData.feeAmount = String(feeAmount); - updateData.total = String(total); + updateData.feeAmount = money(feeAmount); + updateData.total = money(total); // Replace line items await tx.delete(invoiceLineItems).where(eq(invoiceLineItems.invoiceId, id)); @@ -473,8 +499,8 @@ export async function updateInvoice( invoiceId: id, description: li.description, quantity: String(li.quantity ?? 1), - unitPrice: String(li.unitPrice), - total: String((li.quantity ?? 1) * li.unitPrice), + unitPrice: money(li.unitPrice), + total: money((li.quantity ?? 1) * li.unitPrice), sortOrder: idx, })), ); diff --git a/src/lib/services/notes.service.ts b/src/lib/services/notes.service.ts index 626d849b..b1e1d0f2 100644 --- a/src/lib/services/notes.service.ts +++ b/src/lib/services/notes.service.ts @@ -929,7 +929,7 @@ export async function create( .from(userProfiles) .where(eq(userProfiles.userId, authorId)) .limit(1); - return { ...note, authorName: profile[0]?.displayName ?? null, updatedAt: note.createdAt }; + return { ...note, authorName: profile[0]?.displayName ?? null }; } if (entityType === 'clients') { const [note] = await db diff --git a/src/lib/services/reports/currency.ts b/src/lib/services/reports/currency.ts index 19f85edb..a87d2cec 100644 --- a/src/lib/services/reports/currency.ts +++ b/src/lib/services/reports/currency.ts @@ -2,7 +2,8 @@ import { eq } from 'drizzle-orm'; import { db } from '@/lib/db'; import { ports } from '@/lib/db/schema/ports'; -import { convert } from '@/lib/services/currency'; +import { getRate } from '@/lib/services/currency'; +import { logger } from '@/lib/logger'; /** * Port's default currency for money normalisation. Falls back to USD @@ -18,15 +19,84 @@ export async function resolvePortCurrency(portId: string): Promise { } /** - * Convert `amount` from `from` → `to`. Returns the amount unchanged when - * the currencies match or a rate is unavailable (so a missing FX rate - * degrades to "report in source units" rather than dropping the figure). + * Convert `amount` from `from` → `to`, rounding the result to 2dp. + * + * Used for one-off per-row figures in report TABLES (recent payments, + * expense ledger, …) where a single normalised display value is wanted and + * the cent-rounding is correct because the value is shown standalone, not + * accumulated. For SUMS use {@link CurrencyAccumulator} instead — accumulating + * per-row `normalizeAmount` results compounds rounding drift (audit M19). + * + * On a missing/stale rate this returns `null` rather than the former silent + * `?? amount` fallback that added an unconverted foreign amount straight into + * a port-currency figure (audit L25). Callers decide how to degrade. */ -export async function normalizeAmount(amount: number, from: string, to: string): Promise { +export async function normalizeAmount( + amount: number, + from: string, + to: string, +): Promise { if (!amount) return amount; const f = from.toUpperCase(); const t = to.toUpperCase(); if (f === t) return amount; - const converted = await convert(amount, f, t); - return converted?.result ?? amount; + const rate = await getRate(f, t); + if (rate == null) { + logger.warn({ from: f, to: t }, 'Report normalizeAmount: FX rate unavailable; skipping figure'); + return null; + } + return Number((amount * rate).toFixed(2)); +} + +/** + * Sums money amounts in their SOURCE currency, grouped by currency, then + * converts each currency bucket to the target currency exactly ONCE at + * settle time — rounding only the final per-target figure (audit M19). + * + * This avoids two correctness bugs of the old per-row `normalizeAmount`-then- + * accumulate pattern: + * 1. cents-rounding every row before adding compounded ±0.5¢×N drift; + * 2. a missing/stale rate silently added the raw foreign amount into the + * port-currency total (audit L25) — here an unconvertible bucket is + * skipped and counted, never folded in at the wrong scale. + */ +export class CurrencyAccumulator { + /** sourceCurrency (upper) → summed raw amount in that source currency */ + private readonly buckets = new Map(); + + /** Add a raw amount in its own currency. No conversion happens here. */ + add(amount: number, currency: string): void { + if (!amount) return; + const c = currency.toUpperCase(); + this.buckets.set(c, (this.buckets.get(c) ?? 0) + amount); + } + + /** + * Convert every bucket to `target` once, sum, and round only the final + * figure. Unconvertible buckets (no/stale rate) are skipped and counted in + * `unconvertible` rather than added at the wrong scale. + */ + async settle(target: string): Promise<{ total: number; unconvertible: number }> { + const t = target.toUpperCase(); + let total = 0; + let unconvertible = 0; + for (const [currency, sum] of this.buckets) { + if (sum === 0) continue; + if (currency === t) { + total += sum; + continue; + } + const rate = await getRate(currency, t); + if (rate == null) { + unconvertible += 1; + logger.warn( + { from: currency, to: t, amount: sum }, + 'Report aggregate: FX rate unavailable; bucket excluded from total', + ); + continue; + } + total += sum * rate; + } + return { total: Number(total.toFixed(2)), unconvertible }; + } } diff --git a/src/lib/services/reports/financial.service.ts b/src/lib/services/reports/financial.service.ts index 23b61bc2..a9feed4c 100644 --- a/src/lib/services/reports/financial.service.ts +++ b/src/lib/services/reports/financial.service.ts @@ -7,7 +7,7 @@ import { berths } from '@/lib/db/schema/berths'; import { payments } from '@/lib/db/schema/pipeline'; import { expenses } from '@/lib/db/schema/financial'; import { activeInterestsWhere } from '@/lib/services/active-interest'; -import { resolvePortCurrency, normalizeAmount } from './currency'; +import { resolvePortCurrency, normalizeAmount, CurrencyAccumulator } from './currency'; import { agingBucket, AGING_BUCKETS, @@ -94,23 +94,37 @@ async function getDepositPositions(portId: string): Promise { .from(payments) .where(and(eq(payments.portId, portId), eq(payments.paymentType, 'deposit'))); - const collectedByInterest = new Map(); + // Accumulate collected deposits per interest in their SOURCE currency, then + // settle each interest's buckets to the target currency once (M19) — never + // cents-rounding mid-accumulation. + const collectedAccByInterest = new Map(); for (const r of collectedRows) { - const amount = await normalizeAmount( - Number(r.amount ?? 0), - r.currency ?? targetCurrency, - targetCurrency, - ); - collectedByInterest.set(r.interestId, (collectedByInterest.get(r.interestId) ?? 0) + amount); + let acc = collectedAccByInterest.get(r.interestId); + if (!acc) { + acc = new CurrencyAccumulator(); + collectedAccByInterest.set(r.interestId, acc); + } + acc.add(Number(r.amount ?? 0), r.currency ?? targetCurrency); + } + const collectedByInterest = new Map(); + for (const [interestId, acc] of collectedAccByInterest) { + const { total } = await acc.settle(targetCurrency); + collectedByInterest.set(interestId, total); } const positions: DepositPosition[] = []; for (const r of rows) { - const expected = await normalizeAmount( - Number(r.expected ?? 0), - r.expectedCurrency ?? targetCurrency, - targetCurrency, - ); + // A single expected figure per deal — `normalizeAmount` is fine here; on a + // missing rate it returns null, in which case we fall back to the raw + // amount labelled in source units (the figure can't be dropped silently as + // it gates the outstanding/pipeline KPIs). 0-expected deals are excluded + // upstream by the IS NOT NULL filter. + const expected = + (await normalizeAmount( + Number(r.expected ?? 0), + r.expectedCurrency ?? targetCurrency, + targetCurrency, + )) ?? Number(r.expected ?? 0); const collected = collectedByInterest.get(r.interestId) ?? 0; const remaining = Math.max(0, expected - collected); const daysOutstanding = r.createdAt @@ -151,28 +165,42 @@ async function sumPaymentsInRange( ), ); - const acc = { total: 0, deposit: 0, balance: 0, refund: 0 }; + // Sum in source currency per type, convert each currency bucket once at the + // end (M19). Refunds accumulate as MAGNITUDES (audit H12): `Math.abs` is + // applied to the source amount before bucketing — conversion preserves + // magnitude (rates are positive) — so a refund always subtracts and can + // never inflate revenue regardless of its stored sign. + const depositAcc = new CurrencyAccumulator(); + const balanceAcc = new CurrencyAccumulator(); + const refundMagAcc = new CurrencyAccumulator(); for (const r of rows) { - const amount = await normalizeAmount( - Number(r.amount ?? 0), - r.currency ?? targetCurrency, - targetCurrency, - ); + const raw = Number(r.amount ?? 0); + const cur = r.currency ?? targetCurrency; if (r.paymentType === 'refund') { - // Refund sign convention (audit H12): treat a refund as a magnitude - // deduction regardless of the stored sign. New rows store negatives, but - // legacy rows may be positive — `Math.abs` makes both subtract so a - // refund can never INFLATE collected revenue. - const magnitude = Math.abs(amount); - acc.total -= magnitude; - acc.refund -= magnitude; + refundMagAcc.add(Math.abs(raw), cur); + } else if (r.paymentType === 'deposit') { + depositAcc.add(raw, cur); + } else if (r.paymentType === 'balance') { + balanceAcc.add(raw, cur); } else { - acc.total += amount; - if (r.paymentType === 'deposit') acc.deposit += amount; - else if (r.paymentType === 'balance') acc.balance += amount; + // Any other non-refund type still contributes to total via its own + // bucket; fold it into deposit so `total` stays = deposit+balance−refund. + depositAcc.add(raw, cur); } } - return acc; + + const [deposit, balance, refundMag] = await Promise.all([ + depositAcc.settle(targetCurrency), + balanceAcc.settle(targetCurrency), + refundMagAcc.settle(targetCurrency), + ]); + + return { + total: Number((deposit.total + balance.total - refundMag.total).toFixed(2)), + deposit: deposit.total, + balance: balance.total, + refund: -refundMag.total, + }; } export async function getFinancialKpis(portId: string, range: DateRange): Promise { @@ -215,14 +243,11 @@ async function sumExpensesInRange( lte(expenses.expenseDate, range.to), ), ); - let total = 0; + const acc = new CurrencyAccumulator(); for (const r of rows) { - total += await normalizeAmount( - Number(r.amount ?? 0), - r.currency ?? targetCurrency, - targetCurrency, - ); + acc.add(Number(r.amount ?? 0), r.currency ?? targetCurrency); } + const { total } = await acc.settle(targetCurrency); return total; } @@ -257,27 +282,47 @@ export async function getRevenueByMonth( ), ); - const byMonth = new Map(); - for (const key of monthRange(range.from, range.to)) byMonth.set(key, { deposit: 0, balance: 0 }); + // One trio of source-currency accumulators per month; convert each once at + // settle time (M19). Deposit bar = deposit − refund magnitude (audit H12): + // `Math.abs` on the source amount so legacy positive-stored refunds also + // subtract; conversion preserves magnitude. + type MonthAcc = { + deposit: CurrencyAccumulator; + balance: CurrencyAccumulator; + refundMag: CurrencyAccumulator; + }; + const byMonth = new Map(); + for (const key of monthRange(range.from, range.to)) + byMonth.set(key, { + deposit: new CurrencyAccumulator(), + balance: new CurrencyAccumulator(), + refundMag: new CurrencyAccumulator(), + }); for (const r of rows) { if (!r.receivedAt) continue; - const key = monthKey(new Date(r.receivedAt)); - const bucket = byMonth.get(key); - if (!bucket) continue; - const amount = await normalizeAmount( - Number(r.amount ?? 0), - r.paymentCurrency ?? currency, - currency, - ); - if (r.paymentType === 'balance') bucket.balance += amount; - else if (r.paymentType === 'refund') - // Refund sign convention (audit H12): net the refund magnitude out of the - // deposit bar (it previously dropped refunds, overstating monthly - // revenue). `Math.abs` so legacy positive-stored refunds also subtract. - bucket.deposit -= Math.abs(amount); - else bucket.deposit += amount; // deposit + other → deposit bar + const acc = byMonth.get(monthKey(new Date(r.receivedAt))); + if (!acc) continue; + const raw = Number(r.amount ?? 0); + const cur = r.paymentCurrency ?? currency; + if (r.paymentType === 'balance') acc.balance.add(raw, cur); + else if (r.paymentType === 'refund') acc.refundMag.add(Math.abs(raw), cur); + else acc.deposit.add(raw, cur); // deposit + other → deposit bar } - return Array.from(byMonth.entries()).map(([month, v]) => ({ month, ...v })); + + const out: RevenueByMonthRow[] = []; + for (const [month, acc] of byMonth) { + const [deposit, balance, refundMag] = await Promise.all([ + acc.deposit.settle(currency), + acc.balance.settle(currency), + acc.refundMag.settle(currency), + ]); + out.push({ + month, + deposit: Number((deposit.total - refundMag.total).toFixed(2)), + balance: balance.total, + }); + } + return out; } export interface CollectionFunnelRow { @@ -375,8 +420,22 @@ export interface CashFlowRow { /** Monthly inflow (payments received) vs outflow (expenses booked). */ export async function getCashFlow(portId: string, range: DateRange): Promise { const currency = await resolvePortCurrency(portId); - const byMonth = new Map(); - for (const key of monthRange(range.from, range.to)) byMonth.set(key, { inflow: 0, outflow: 0 }); + // Per-month source-currency accumulators; convert each bucket once at settle + // (M19). Inflow = non-refund payments − refund magnitudes (audit H12); the + // refund magnitude rides its own accumulator so it converts at its own + // currency's rate before being subtracted. + type FlowAcc = { + inflow: CurrencyAccumulator; + refundMag: CurrencyAccumulator; + outflow: CurrencyAccumulator; + }; + const byMonth = new Map(); + for (const key of monthRange(range.from, range.to)) + byMonth.set(key, { + inflow: new CurrencyAccumulator(), + refundMag: new CurrencyAccumulator(), + outflow: new CurrencyAccumulator(), + }); const paymentRows = await db .select({ @@ -395,14 +454,12 @@ export async function getCashFlow(portId: string, range: DateRange): Promise ({ month, ...v })); + const out: CashFlowRow[] = []; + for (const [month, acc] of byMonth) { + const [inflow, refundMag, outflow] = await Promise.all([ + acc.inflow.settle(currency), + acc.refundMag.settle(currency), + acc.outflow.settle(currency), + ]); + out.push({ + month, + inflow: Number((inflow.total - refundMag.total).toFixed(2)), + outflow: outflow.total, + }); + } + return out; } export interface ExpenseBreakdownRow { @@ -456,15 +522,23 @@ export async function getExpenseBreakdown( lte(expenses.expenseDate, range.to), ), ); - const byCategory = new Map(); + // One source-currency accumulator per category; settle each once (M19). + const byCategory = new Map(); for (const r of rows) { const cat = r.category ?? 'Uncategorised'; - const amount = await normalizeAmount(Number(r.amount ?? 0), r.currency ?? currency, currency); - byCategory.set(cat, (byCategory.get(cat) ?? 0) + amount); + let acc = byCategory.get(cat); + if (!acc) { + acc = new CurrencyAccumulator(); + byCategory.set(cat, acc); + } + acc.add(Number(r.amount ?? 0), r.currency ?? currency); } - return Array.from(byCategory.entries()) - .map(([category, total]) => ({ category, total })) - .sort((a, b) => b.total - a.total); + const out: ExpenseBreakdownRow[] = []; + for (const [category, acc] of byCategory) { + const { total } = await acc.settle(currency); + out.push({ category, total }); + } + return out.sort((a, b) => b.total - a.total); } // ─── Tables ────────────────────────────────────────────────────────────────── @@ -540,7 +614,11 @@ export async function getRecentPayments( clientName: r.clientName, mooring: r.mooring, paymentType: r.paymentType, - amount: await normalizeAmount(Number(r.amount ?? 0), r.paymentCurrency ?? currency, currency), + // Single displayed figure: on a missing/stale rate fall back to the raw + // source amount (logged in normalizeAmount) rather than dropping the row. + amount: + (await normalizeAmount(Number(r.amount ?? 0), r.paymentCurrency ?? currency, currency)) ?? + Number(r.amount ?? 0), currency, }); } @@ -588,7 +666,8 @@ export async function getRefundLog(portId: string, range: DateRange): Promise