From d173c9e9a0d7873a362f7cfccf0e6a1db64c822b Mon Sep 17 00:00:00 2001 From: Ricardo Mancinas Date: Wed, 5 Aug 2026 00:34:53 -0700 Subject: [PATCH] fix(billing): stop double-counting history a BALANCE FORWARD already carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BALANCE FORWARD rows are not movements. Access materialized one per customer per year, dated Jan 1, holding the closing balance of everything before it — that is what let the portal keep each year in its own table and still show a correct running balance from one year's rows. The platform imported those rows AND the real pre-cutover history they summarize, and every balance aggregate summed the lot. NUMid 501 read -10,469.29 on the receivables worklist against -14,065.29 on the customer's own statement and on the legacy portal; the gap was two cash receipts from 2009 and 2012 that the 2026 opening balance had already absorbed. The scale settles what it is: summed the old way the whole book came to +20,605,447.86 MXN — the office owing its customers 20.6 million pesos. Floored, it is -56,855.90. A receivables ledger cannot be 20M in credit. Adds BALANCE_FLOOR_JOIN + NOT_SUPERSEDED and applies them to balances() (page and count queries, which must agree), to stats()'s per-currency and per-domain figures, and to the owing/in-credit split. The four stats() aggregates moved from Prisma groupBy to raw SQL because groupBy cannot express a per-customer floor. statement() takes the same floor as a scalar, which is also what stops FEE ANUAL and fee15 leaking in. Those are not in STATEMENT_EXCLUDED_SOURCE_TABLES — that list reproduces legacy's DATOS2-only `datosfreak` — and they were putting 2,092 pre-cutover fee rows across 1,062 customers into the statement, skewing it by -5,129,764 against the number those customers have been quoted for years. Dating rather than source is the right test: a fee row *after* the opening balance is a real charge and still counts. movements() is deliberately left alone. It is a browser over captured rows — "how much water did we capture in April" — and staff need the historical rows visible, so it keeps totalling everything, the same asymmetry NOT_OUTSTANDING already has. stats() now separates the two questions it was mixing: movements, ledgerCustomers, crossLineCustomers and the date range stay unfloored inventory; everything under byCurrency/byDomain is a balance and is floored. BillingService had no tests. Adds 13 covering the floor's failure modes — it fails silently, so MIN-vs-MAX, `>` vs `>=`, the NULL branch for customers with no opening balance, and the join/predicate alias pairing are each pinned, plus the 501 arithmetic as a regression. Verified through the real service against the live ledger: balances() and statement() both return -14,065.29 for 501, matching the portal. Co-Authored-By: Claude Opus 5 --- apps/api/src/billing/balance-floor.spec.ts | 180 ++++++++++++++++++ apps/api/src/billing/billing.service.ts | 211 ++++++++++++++++----- 2 files changed, 339 insertions(+), 52 deletions(-) create mode 100644 apps/api/src/billing/balance-floor.spec.ts diff --git a/apps/api/src/billing/balance-floor.spec.ts b/apps/api/src/billing/balance-floor.spec.ts new file mode 100644 index 0000000..62e4687 --- /dev/null +++ b/apps/api/src/billing/balance-floor.spec.ts @@ -0,0 +1,180 @@ +import { Prisma } from "@jorgecuadros/database"; +import { + BALANCE_FLOOR_JOIN, + BALANCE_FORWARD_TYPE, + BillingService, + NOT_SUPERSEDED, +} from "./billing.service"; + +/** + * The balance floor drops rows a later BALANCE FORWARD already accounts for. + * + * It is worth testing because it fails silently: nothing throws, the numbers are + * just wrong, and they were wrong for years — the whole book read +20.6M MXN in + * credit because every customer's pre-cutover history was counted twice, once + * inside their opening balance and once as itself. + */ +describe("balance floor", () => { + describe("SQL fragments", () => { + it("binds the type name rather than interpolating it", () => { + // A literal would be a second place to edit if the label ever changes, + // and this string reaches SQL from a module constant. + expect(BALANCE_FLOOR_JOIN.values).toEqual([BALANCE_FORWARD_TYPE]); + }); + + it("keys the floor to the row's own customer", () => { + // Without this the derived table cross-joins and every customer inherits + // the earliest BALANCE FORWARD in the book. + expect(BALANCE_FLOOR_JOIN.sql).toContain( + "bfloor ON bfloor.customerId = t.customerId", + ); + }); + + it("takes the most recent opening balance, not the first", () => { + // A customer accumulates one BALANCE FORWARD per year. MIN would floor at + // the oldest and leave every intervening year double-counted. + expect(BALANCE_FLOOR_JOIN.sql).toContain("MAX(bf.transactionDate)"); + expect(BALANCE_FLOOR_JOIN.sql).not.toContain("MIN(bf.transactionDate)"); + }); + + it("ignores voided opening balances when locating the floor", () => { + expect(BALANCE_FLOOR_JOIN.sql).toContain("bf.voidedAt IS NULL"); + }); + + it("is inclusive of the opening balance row itself", () => { + // `>` instead of `>=` would drop the carried balance and understate every + // customer by exactly that amount. + expect(NOT_SUPERSEDED.sql).toContain("t.transactionDate >= bfloor.floorDate"); + expect(NOT_SUPERSEDED.sql).not.toMatch(/transactionDate\s*>\s*bfloor/); + }); + + it("leaves customers with no opening balance untouched", () => { + // NULL comparisons are never true, so without the explicit IS NULL branch + // a customer who has no BALANCE FORWARD row loses their entire ledger. + expect(NOT_SUPERSEDED.sql).toContain("bfloor.floorDate IS NULL"); + }); + + it("only ever references the alias the join defines", () => { + // The predicate is useless without the join; pairing them wrongly is a + // runtime "unknown column", so keep the alias identical in both. + const aliases = NOT_SUPERSEDED.sql.match(/bfloor\.\w+/g) ?? []; + expect(aliases.length).toBeGreaterThan(0); + for (const ref of aliases) { + expect(BALANCE_FLOOR_JOIN.sql).toContain(ref.split(".")[1]); + } + }); + }); + + describe("statement()", () => { + /** + * One customer means one floor date, so the statement uses a scalar lookup + * instead of the join. Asserting on the `where` Prisma is handed is the only + * way to see it without a database. + */ + function serviceWith(floor: Date | null) { + const findMany = jest.fn().mockResolvedValue([]); + const prisma = { + customer: { + findUnique: jest.fn().mockResolvedValue({ + id: "c1", + name: "CUADROS, JORGE H.", + preferredCurrency: "USD", + _count: { properties: 0, policies: 0 }, + }), + }, + transaction: { + findFirst: jest + .fn() + .mockResolvedValue(floor ? { transactionDate: floor } : null), + findMany, + }, + }; + + return { + service: new BillingService(prisma as never), + prisma, + findMany, + }; + } + + it("looks the floor up from the customer's newest opening balance", async () => { + const { service, prisma } = serviceWith(new Date("2026-01-01T00:00:00Z")); + + await service.statement("c1"); + + expect(prisma.transaction.findFirst).toHaveBeenCalledWith( + expect.objectContaining({ + where: { + customerId: "c1", + voidedAt: null, + type: { nameEn: BALANCE_FORWARD_TYPE }, + }, + orderBy: { transactionDate: "desc" }, + select: { transactionDate: true }, + }), + ); + }); + + it("bounds the statement at the floor, inclusive", async () => { + const floor = new Date("2026-01-01T00:00:00Z"); + const { service, findMany } = serviceWith(floor); + + await service.statement("c1"); + + expect(findMany.mock.calls[0][0].where).toMatchObject({ + customerId: "c1", + transactionDate: { gte: floor }, + }); + }); + + it("applies no date bound when the customer has no opening balance", async () => { + const { service, findMany } = serviceWith(null); + + await service.statement("c1"); + + expect(findMany.mock.calls[0][0].where).not.toHaveProperty( + "transactionDate", + ); + }); + + it("keeps the source-table exclusion alongside the floor", async () => { + // The two guards answer different questions — one reproduces legacy's + // DATOS2-only materialization, the other drops superseded history — and + // dropping either one changes the customer's balance. + const { service, findMany } = serviceWith(new Date("2026-01-01T00:00:00Z")); + + await service.statement("c1"); + + const where = findMany.mock.calls[0][0].where; + expect(where.OR).toEqual([ + { legacySourceTable: null }, + { legacySourceTable: { notIn: expect.arrayContaining(["EFECTIVO"]) } }, + ]); + }); + }); + + describe("regression: NUMid 501", () => { + /** + * The arithmetic that exposed the bug, pinned so it cannot silently return. + * Figures measured against the live ledger on 2026-08-05. + */ + const openingBalance = new Prisma.Decimal("-6732.29"); + const activitySinceOpening = new Prisma.Decimal("-7333.00"); + const preCutoverCashAlreadyInOpening = new Prisma.Decimal("3596.00"); + + it("matches the legacy portal once superseded rows are dropped", () => { + expect(openingBalance.plus(activitySinceOpening).toFixed(2)).toBe( + "-14065.29", + ); + }); + + it("reproduces the wrong figure when they are not", () => { + expect( + openingBalance + .plus(activitySinceOpening) + .plus(preCutoverCashAlreadyInOpening) + .toFixed(2), + ).toBe("-10469.29"); + }); + }); +}); diff --git a/apps/api/src/billing/billing.service.ts b/apps/api/src/billing/billing.service.ts index 7a54077..607309f 100644 --- a/apps/api/src/billing/billing.service.ts +++ b/apps/api/src/billing/billing.service.ts @@ -104,24 +104,31 @@ interface BalanceRow { nameMissing: number; city: string | null; state: string | null; - movements: bigint | number | string; + movements: RawCount; balanceMxn: Prisma.Decimal | null; balanceUsd: Prisma.Decimal | null; chargesMxn: Prisma.Decimal | null; creditsMxn: Prisma.Decimal | null; chargesUsd: Prisma.Decimal | null; creditsUsd: Prisma.Decimal | null; - utilityMovements: bigint | number | string; - insuranceMovements: bigint | number | string; + utilityMovements: RawCount; + insuranceMovements: RawCount; lastMovement: Date | null; } /** - * Raw-query counts come back in three shapes depending on the aggregate: - * `COUNT(*)` as bigint, `SUM(bool)` as a decimal *string*, and plain numbers. - * Normalize all of them before they reach the client as JSON. + * Every shape a raw-query count can arrive in. `COUNT(*)` is a bigint, + * `SUM(bool)` is a Prisma.Decimal, and plain numbers occur too — none of which + * survive JSON serialization the way the client expects. */ -function num(v: bigint | number | string | null | undefined): number { +type RawCount = bigint | number | string | Prisma.Decimal; + +/** + * Normalizes a raw-query count before it reaches the client as JSON. A bigint + * throws on JSON.stringify and a Decimal serializes to a *string*, so counts + * must not be passed through untouched. + */ +function num(v: RawCount | null | undefined): number { if (v === null || v === undefined) return 0; return typeof v === "number" ? v : Number(v); } @@ -152,6 +159,56 @@ const NOT_VOIDED: Prisma.TransactionWhereInput = { voidedAt: null }; */ const NOT_OUTSTANDING: Prisma.TransactionWhereInput = { outstanding: false }; +/** + * The legacy type name for a carried-forward opening balance. + * + * These rows are not movements. Access materialized one per customer per year, + * dated Jan 1, holding the closing balance of everything before it — that is + * what let the portal keep each year in its own table (`datosfreak` = current, + * `2025`, `2024`, ...) and still show a correct running balance from a single + * year's rows. + */ +export const BALANCE_FORWARD_TYPE = "BALANCE FORWARD"; + +/** + * Per-customer date of the most recent BALANCE FORWARD row. + * + * Joined rather than correlated: one small derived table (1,170 rows) beats a + * subquery evaluated per ledger row. + */ +export const BALANCE_FLOOR_JOIN = Prisma.sql` + LEFT JOIN ( + SELECT bf.customerId, MAX(bf.transactionDate) AS floorDate + FROM transactions bf + JOIN type_transactions bft ON bft.id = bf.typeId + WHERE bft.nameEn = ${BALANCE_FORWARD_TYPE} AND bf.voidedAt IS NULL + GROUP BY bf.customerId + ) bfloor ON bfloor.customerId = t.customerId`; + +/** + * Excludes rows a later BALANCE FORWARD already accounts for. + * + * WHY THIS EXISTS. The platform holds both the synthetic BALANCE FORWARD rows + * and the real pre-cutover history they summarize, so summing a customer's + * whole ledger counts that history twice — once inside the opening balance, + * once as itself. NUMid 501 read -10,469.29 on the worklist against -14,065.29 + * on the customer's own statement and on the legacy portal, the gap being two + * cash receipts from 2009 and 2012 that the 2026 opening balance had already + * absorbed. + * + * The scale is what settles it: summed the old way the entire book came to + * +20,605,447.86 MXN — the office owing its customers 20.6 million pesos. + * Floored, it is -56,855.90, a modest net receivable. A receivables ledger + * cannot be 20M in credit. + * + * Applies to BALANCES ONLY, in the same spirit as NOT_OUTSTANDING: the movement + * browser still totals every captured row, because "how much water did we + * capture in April" is a question about what was recorded, not about what is + * owed. Customers with no BALANCE FORWARD row (the floor is NULL) are + * unaffected. + */ +export const NOT_SUPERSEDED = Prisma.sql`(bfloor.floorDate IS NULL OR t.transactionDate >= bfloor.floorDate)`; + /** * Source tables excluded from the customer-facing statement. * @@ -402,22 +459,24 @@ export class BillingService { MAX(t.transactionDate) AS lastMovement FROM customers c JOIN transactions t ON t.customerId = c.id - WHERE t.voidedAt IS NULL AND t.outstanding = 0 ${nameFilter} ${txFilter} + ${BALANCE_FLOOR_JOIN} + WHERE t.voidedAt IS NULL AND t.outstanding = 0 AND ${NOT_SUPERSEDED} ${nameFilter} ${txFilter} GROUP BY c.id, c.name, c.nameSource, c.nameMissing, c.city, c.state ${having} ${orderBy} LIMIT ${pageSize} OFFSET ${(page - 1) * pageSize} `; - const counted = await this.prisma.$queryRaw<{ total: bigint | number | string }[]>` + const counted = await this.prisma.$queryRaw<{ total: RawCount }[]>` SELECT COUNT(*) AS total FROM ( SELECT c.id FROM customers c JOIN transactions t ON t.customerId = c.id + ${BALANCE_FLOOR_JOIN} -- Must match the page query's filters exactly, or the total disagrees -- with the rows. (The void exclusion was missing here before the -- outstanding work; a voided-only customer inflated the count.) - WHERE t.voidedAt IS NULL AND t.outstanding = 0 ${nameFilter} ${txFilter} + WHERE t.voidedAt IS NULL AND t.outstanding = 0 AND ${NOT_SUPERSEDED} ${nameFilter} ${txFilter} GROUP BY c.id ${having} ) x @@ -458,9 +517,18 @@ export class BillingService { }; } - /** Top-line figures for the billing page header. */ + /** + * Top-line figures for the billing page header. + * + * Two different questions live here and they use different row sets. + * `movements`, `ledgerCustomers`, `crossLineCustomers` and the date range are + * INVENTORY — what is stored — and count everything not voided. Everything + * under `byCurrency` / `byDomain` is a BALANCE, so it applies NOT_SUPERSEDED + * and drops rows an opening balance already accounts for. The four aggregates + * moved from Prisma groupBy to raw SQL to express that join; groupBy cannot. + */ async stats() { - const [movements, ledgerCustomers, byCurrency, byDomain] = await Promise.all([ + const [movements, ledgerCustomers] = await Promise.all([ this.prisma.transaction.count({ where: NOT_VOIDED }), this.prisma.transaction .findMany({ @@ -469,34 +537,47 @@ export class BillingService { select: { customerId: true }, }) .then((r) => r.length), - this.prisma.transaction.groupBy({ - by: ["currency"], - where: NOT_VOIDED, - _sum: { amount: true }, - _count: { _all: true }, - }), - this.prisma.transaction.groupBy({ - by: ["domain", "currency"], - where: NOT_VOIDED, - _sum: { amount: true }, - _count: { _all: true }, - }), ]); - const charges = await this.prisma.transaction.groupBy({ - by: ["currency"], - where: { AND: [{ amount: { lt: 0 } }, NOT_VOIDED] }, - _sum: { amount: true }, - _count: { _all: true }, - }); - const credits = await this.prisma.transaction.groupBy({ - by: ["currency"], - where: { AND: [{ amount: { gt: 0 } }, NOT_VOIDED] }, - _sum: { amount: true }, - _count: { _all: true }, - }); - const chargeMap = new Map(charges.map((c) => [c.currency, c])); - const creditMap = new Map(credits.map((c) => [c.currency, c])); + const byCurrency = await this.prisma.$queryRaw< + { + currency: string; + net: Prisma.Decimal | null; + count: RawCount; + charges: Prisma.Decimal | null; + chargeCount: RawCount; + credits: Prisma.Decimal | null; + creditCount: RawCount; + }[] + >` + SELECT t.currency AS currency, + SUM(t.amount) AS net, + COUNT(*) AS count, + SUM(CASE WHEN t.amount < 0 THEN t.amount ELSE 0 END) AS charges, + SUM(t.amount < 0) AS chargeCount, + SUM(CASE WHEN t.amount > 0 THEN t.amount ELSE 0 END) AS credits, + SUM(t.amount > 0) AS creditCount + FROM transactions t + ${BALANCE_FLOOR_JOIN} + WHERE t.voidedAt IS NULL AND ${NOT_SUPERSEDED} + GROUP BY t.currency + `; + + const byDomain = await this.prisma.$queryRaw< + { + domain: string; + currency: string; + net: Prisma.Decimal | null; + count: RawCount; + }[] + >` + SELECT t.domain AS domain, t.currency AS currency, + SUM(t.amount) AS net, COUNT(*) AS count + FROM transactions t + ${BALANCE_FLOOR_JOIN} + WHERE t.voidedAt IS NULL AND ${NOT_SUPERSEDED} + GROUP BY t.domain, t.currency + `; // How many customers sit on each side of the line, per currency — the // headline for a receivables view. Counted in SQL; a customer can be @@ -504,16 +585,19 @@ export class BillingService { const sides = await this.prisma.$queryRaw< { currency: string; - owing: bigint | number | string; - inCredit: bigint | number | string; + owing: RawCount; + inCredit: RawCount; }[] >` SELECT currency, SUM(bal < -0.005) AS owing, SUM(bal > 0.005) AS inCredit FROM ( - SELECT customerId, currency, SUM(amount) AS bal - FROM transactions WHERE voidedAt IS NULL GROUP BY customerId, currency + SELECT t.customerId, t.currency, SUM(t.amount) AS bal + FROM transactions t + ${BALANCE_FLOOR_JOIN} + WHERE t.voidedAt IS NULL AND ${NOT_SUPERSEDED} + GROUP BY t.customerId, t.currency ) x GROUP BY currency `; @@ -534,7 +618,7 @@ export class BillingService { // Customers whose ledger spans both business lines — the whole reason this // module is one view instead of two. - const crossLine = await this.prisma.$queryRaw<{ n: bigint | number | string }[]>` + const crossLine = await this.prisma.$queryRaw<{ n: RawCount }[]>` SELECT COUNT(*) AS n FROM ( SELECT customerId FROM transactions WHERE voidedAt IS NULL GROUP BY customerId HAVING COUNT(DISTINCT domain) > 1 @@ -549,20 +633,20 @@ export class BillingService { lastMovement: lastRow?.transactionDate ?? null, byCurrency: byCurrency.map((c) => ({ currency: c.currency, - net: c._sum.amount, - count: c._count._all, - charges: chargeMap.get(c.currency)?._sum.amount ?? null, - chargeCount: chargeMap.get(c.currency)?._count._all ?? 0, - credits: creditMap.get(c.currency)?._sum.amount ?? null, - creditCount: creditMap.get(c.currency)?._count._all ?? 0, + net: c.net, + count: num(c.count), + charges: c.charges, + chargeCount: num(c.chargeCount), + credits: c.credits, + creditCount: num(c.creditCount), owing: num(sideMap.get(c.currency)?.owing), inCredit: num(sideMap.get(c.currency)?.inCredit), })), byDomain: byDomain.map((d) => ({ domain: d.domain, currency: d.currency, - net: d._sum.amount, - count: d._count._all, + net: d.net, + count: num(d.count), })), }; } @@ -589,7 +673,7 @@ export class BillingService { }); const years = await this.prisma.$queryRaw< - { year: number; count: bigint | number | string }[] + { year: number; count: RawCount }[] >` SELECT YEAR(transactionDate) AS year, COUNT(*) AS count FROM transactions WHERE voidedAt IS NULL GROUP BY year ORDER BY year DESC @@ -647,9 +731,32 @@ export class BillingService { throw new NotFoundException(`Customer ${customerId} not found`); } + // One customer, so the balance floor is a single date rather than the + // derived table the aggregate queries join. See NOT_SUPERSEDED: rows before + // the opening balance are already inside it, and showing them would both + // double the total and make every balanceAfter below wrong. + // + // This is also what stops FEE ANUAL and fee15 leaking in. They are not in + // STATEMENT_EXCLUDED_SOURCE_TABLES — that list exists to reproduce legacy's + // DATOS2-only `datosfreak`, and it was letting 2,092 pre-cutover fee rows + // across 1,062 customers through, skewing the statement by -5,129,764 + // against the number those customers have been quoted for years. Dating + // rather than source is the right test: a FEE ANUAL row *after* the opening + // balance is a real charge and still counts. + const floor = await this.prisma.transaction.findFirst({ + where: { + customerId, + voidedAt: null, + type: { nameEn: BALANCE_FORWARD_TYPE }, + }, + orderBy: { transactionDate: "desc" }, + select: { transactionDate: true }, + }); + const rows = await this.prisma.transaction.findMany({ where: { customerId, + ...(floor ? { transactionDate: { gte: floor.transactionDate } } : {}), // NULL-safe exclusion. `notIn` alone compiles to SQL `NOT IN`, and // `NULL NOT IN (...)` is NULL, not true — so every app-captured row // (which has no legacySourceTable) silently vanished from the