fix: remove double sign negation for previous year actuals (#34)

Transaction amounts are already signed in the DB (expenses negative,
income positive). Remove Math.abs() normalization and stop multiplying
by sign at display time to avoid double negation.

Extract buildPrevYearTotalMap as a testable exported function and
rewrite tests to import the real function instead of reimplementing it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
medic-bot 2026-03-11 12:02:58 -04:00
parent a764ae0d38
commit 21bf1173ea
3 changed files with 40 additions and 42 deletions

View file

@ -150,7 +150,7 @@ export default function BudgetTable({ rows, onUpdatePlanned, onSplitEvenly }: Bu
monthTotals[m] += row.months[m] * sign; monthTotals[m] += row.months[m] * sign;
} }
annualTotal += row.annual * sign; annualTotal += row.annual * sign;
prevYearTotal += row.previousYearTotal * sign; prevYearTotal += row.previousYearTotal; // actuals are already signed in the DB
} }
const totalCols = 15; // category + prev year + annual + 12 months const totalCols = 15; // category + prev year + annual + 12 months
@ -197,7 +197,7 @@ export default function BudgetTable({ rows, onUpdatePlanned, onSplitEvenly }: Bu
</div> </div>
</td> </td>
<td className={`py-2 px-2 text-right text-xs ${isIntermediateParent ? "font-medium" : "font-semibold"} text-[var(--muted-foreground)]`}> <td className={`py-2 px-2 text-right text-xs ${isIntermediateParent ? "font-medium" : "font-semibold"} text-[var(--muted-foreground)]`}>
{formatSigned(row.previousYearTotal * sign)} {formatSigned(row.previousYearTotal)}
</td> </td>
<td className={`py-2 px-2 text-right text-xs ${isIntermediateParent ? "font-medium" : "font-semibold"}`}> <td className={`py-2 px-2 text-right text-xs ${isIntermediateParent ? "font-medium" : "font-semibold"}`}>
{formatSigned(row.annual * sign)} {formatSigned(row.annual * sign)}
@ -230,7 +230,7 @@ export default function BudgetTable({ rows, onUpdatePlanned, onSplitEvenly }: Bu
{/* Previous year total — read-only */} {/* Previous year total — read-only */}
<td className="py-2 px-2 text-right text-[var(--muted-foreground)]"> <td className="py-2 px-2 text-right text-[var(--muted-foreground)]">
<span className="text-xs px-1 py-0.5"> <span className="text-xs px-1 py-0.5">
{formatSigned(row.previousYearTotal * sign)} {formatSigned(row.previousYearTotal)}
</span> </span>
</td> </td>
{/* Annual total — editable */} {/* Annual total — editable */}
@ -340,7 +340,7 @@ export default function BudgetTable({ rows, onUpdatePlanned, onSplitEvenly }: Bu
sectionMonthTotals[m] += row.months[m] * sign; sectionMonthTotals[m] += row.months[m] * sign;
} }
sectionAnnualTotal += row.annual * sign; sectionAnnualTotal += row.annual * sign;
sectionPrevYearTotal += row.previousYearTotal * sign; sectionPrevYearTotal += row.previousYearTotal; // actuals are already signed in the DB
} }
return ( return (
<Fragment key={type}> <Fragment key={type}>

View file

@ -1,35 +1,23 @@
import { describe, it, expect } from "vitest"; import { describe, it, expect } from "vitest";
import { buildPrevYearTotalMap } from "./useBudget";
/** /**
* Unit tests for the previous-year actuals normalization logic used in useBudget. * Unit tests for the previous-year actuals normalization logic used in useBudget.
* *
* Transaction amounts in the database use signed values (expenses are negative). * Transaction amounts in the database use signed values (expenses are negative,
* Budget entries are always stored as positive values, with sign applied at display time. * income is positive). The buildPrevYearTotalMap function preserves these signs
* When building the previousYearTotal map from raw actuals, we must normalize with * as-is, because the budget display layer does NOT apply a sign multiplier to
* Math.abs() to match the budget convention. Without this, expenses would show * previous year actuals (unlike planned budget amounts).
* inverted signs (negative × -1 = positive instead of positive × -1 = negative).
*/ */
describe("previous year actuals normalization", () => { describe("buildPrevYearTotalMap", () => {
// Simulates the normalization logic from useBudget.ts it("should preserve negative sign for expense actuals", () => {
function buildPrevYearTotalMap(
actuals: Array<{ category_id: number | null; actual: number }>
): Map<number, number> {
const prevYearTotalMap = new Map<number, number>();
for (const a of actuals) {
if (a.category_id != null)
prevYearTotalMap.set(a.category_id, Math.abs(a.actual));
}
return prevYearTotalMap;
}
it("should store absolute values for expense actuals (negative in DB)", () => {
const actuals = [{ category_id: 1, actual: -500 }]; // expense: negative in DB const actuals = [{ category_id: 1, actual: -500 }]; // expense: negative in DB
const map = buildPrevYearTotalMap(actuals); const map = buildPrevYearTotalMap(actuals);
expect(map.get(1)).toBe(500); expect(map.get(1)).toBe(-500);
}); });
it("should store absolute values for income actuals (positive in DB)", () => { it("should preserve positive sign for income actuals", () => {
const actuals = [{ category_id: 2, actual: 3000 }]; // income: positive in DB const actuals = [{ category_id: 2, actual: 3000 }]; // income: positive in DB
const map = buildPrevYearTotalMap(actuals); const map = buildPrevYearTotalMap(actuals);
expect(map.get(2)).toBe(3000); expect(map.get(2)).toBe(3000);
@ -47,16 +35,16 @@ describe("previous year actuals normalization", () => {
expect(map.get(3)).toBe(0); expect(map.get(3)).toBe(0);
}); });
it("should display correctly with sign multiplier applied", () => { it("should handle multiple categories", () => {
// Simulate the display logic: previousYearTotal * sign const actuals = [
const signFor = (type: string) => (type === "expense" ? -1 : 1); { category_id: 1, actual: -200 },
{ category_id: 2, actual: 1500 },
// Expense category: actual was -500 in DB → normalized to 500 → displayed as 500 * -1 = -500 { category_id: 3, actual: -75.5 },
const expenseActual = Math.abs(-500); ];
expect(expenseActual * signFor("expense")).toBe(-500); const map = buildPrevYearTotalMap(actuals);
expect(map.size).toBe(3);
// Income category: actual was 3000 in DB → normalized to 3000 → displayed as 3000 * 1 = 3000 expect(map.get(1)).toBe(-200);
const incomeActual = Math.abs(3000); expect(map.get(2)).toBe(1500);
expect(incomeActual * signFor("income")).toBe(3000); expect(map.get(3)).toBe(-75.5);
}); });
}); });

View file

@ -63,6 +63,21 @@ function reducer(state: BudgetState, action: BudgetAction): BudgetState {
const TYPE_ORDER: Record<string, number> = { expense: 0, income: 1, transfer: 2 }; const TYPE_ORDER: Record<string, number> = { expense: 0, income: 1, transfer: 2 };
/**
* Build a map of category_id -> annual actual total from raw actuals.
* Transaction amounts are already signed (expenses negative, income positive),
* so they are stored as-is without normalization.
*/
export function buildPrevYearTotalMap(
actuals: Array<{ category_id: number | null; actual: number }>
): Map<number, number> {
const prevYearTotalMap = new Map<number, number>();
for (const a of actuals) {
if (a.category_id != null) prevYearTotalMap.set(a.category_id, a.actual);
}
return prevYearTotalMap;
}
export function useBudget() { export function useBudget() {
const [state, dispatch] = useReducer(reducer, undefined, initialState); const [state, dispatch] = useReducer(reducer, undefined, initialState);
const fetchIdRef = useRef(0); const fetchIdRef = useRef(0);
@ -90,12 +105,7 @@ export function useBudget() {
} }
// Build a map for previous year actuals: categoryId -> annual actual total // Build a map for previous year actuals: categoryId -> annual actual total
// Use Math.abs because transaction amounts are stored with sign (expenses negative), const prevYearTotalMap = buildPrevYearTotalMap(prevYearActuals);
// but budget entries are always positive — the sign is applied at display time.
const prevYearTotalMap = new Map<number, number>();
for (const a of prevYearActuals) {
if (a.category_id != null) prevYearTotalMap.set(a.category_id, Math.abs(a.actual));
}
// Helper: build months array from entryMap // Helper: build months array from entryMap
const buildMonths = (catId: number) => { const buildMonths = (catId: number) => {