diff --git a/src/hooks/useBudget.test.ts b/src/hooks/useBudget.test.ts index 44e5236..822eefc 100644 --- a/src/hooks/useBudget.test.ts +++ b/src/hooks/useBudget.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import type { BudgetYearRow, Category, BudgetEntry } from "../shared/types"; import { buildBudgetYearRows } from "./useBudget"; import { sumLeavesForType } from "../components/budget/budgetTableResults"; +import { reorderRows } from "../utils/reorderRows"; import { type CollapseAccessors, visibleRows, @@ -9,10 +10,11 @@ import { } from "../utils/collapsibleRows"; // The Budget grid's rows are built by `useBudget` (the "3rd builder" the -// abandoned v1 collapse plan ignored). Its final sort is LEVEL-ORDER (depth -// ascending / BFS), not depth-first. These tests pin that invariant: the -// shipped ancestor-walk collapse (issue #288) is order-INDEPENDENT, but the v1 -// depth-cursor algorithm assumed DFS and would have broken exactly here. +// abandoned v1 collapse plan ignored). Since issue #424 they are emitted +// DEPTH-FIRST: each parent row is followed directly by its whole subtree, so an +// expanded subcategory shows under its parent and `reorderRows` (subtotals at +// the bottom) attaches each subtotal to its own rows. The collapse (ancestor +// walk, issue #288) does not depend on the order. function cat( id: number, @@ -62,29 +64,261 @@ const ACCESSORS: CollapseAccessors = { isParent: (row) => row.is_parent, }; -describe("buildBudgetYearRows — level-order emission (issue #289 invariant)", () => { - const rows = buildBudgetYearRows(CATEGORIES, [], []); +/** + * Depth-first invariant: every non-root row comes after its parent row, and + * every row between a parent row and the end of its subtree is one of its + * descendants. A "(direct)" leaf carries its own category as parent: it is + * attached to the subtotal row of that category (the `p:` key of the grid). + */ +function expectDepthFirst(rows: BudgetYearRow[]) { + const parentIndex = new Map(); + rows.forEach((r, i) => { + if (r.is_parent) parentIndex.set(r.category_id, i); + }); + // Ancestor ids of a row, climbing the parent subtotal rows. + const ancestorsOf = (r: BudgetYearRow): number[] => { + const out: number[] = []; + let pid = r.parent_id; + while (pid != null && !out.includes(pid)) { + out.push(pid); + const idx = parentIndex.get(pid); + if (idx === undefined) break; + pid = rows[idx].parent_id; + } + return out; + }; + rows.forEach((r, i) => { + if (r.parent_id != null) { + const p = parentIndex.get(r.parent_id); + expect(p, `${r.category_name} has a parent row`).toBeDefined(); + expect(p!, `${r.category_name} comes after its parent`).toBeLessThan(i); + } + }); + rows.forEach((r, i) => { + if (!r.is_parent) return; + const depth = r.depth ?? 0; + // The subtree runs until the next row of this depth or shallower. + let j = i + 1; + while (j < rows.length && (rows[j].depth ?? 0) > depth) { + expect(ancestorsOf(rows[j]), `${rows[j].category_name} is under ${r.category_name}`).toContain(r.category_id); + j++; + } + // ...and every descendant of the parent lies inside that range. + rows.forEach((d, k) => { + if (k === i) return; + if (ancestorsOf(d).includes(r.category_id)) { + expect(k, `${d.category_name} lies inside the subtree of ${r.category_name}`).toBeGreaterThan(i); + expect(k, `${d.category_name} lies inside the subtree of ${r.category_name}`).toBeLessThan(j); + } + }); + }); +} - it("emits income before expense, then each group in LEVEL order (BFS), not DFS", () => { - // Salary (income) first; then the Housing expense group depth-ascending: - // root subtotal, then the depth-1 rows (parent Utilities before leaf Rent), - // then the depth-2 grandchildren. - expect(rows.map((r) => r.category_id)).toEqual([10, 1, 3, 2, 4, 5]); +const names = (rows: BudgetYearRow[]) => rows.map((r) => r.category_name); + +// Shape of the "Revenus" group of a new profile (consolidated_schema.sql): one +// depth-1 leaf ("Autres revenus") and three intermediate parents. +const REVENUS: Category[] = [ + cat(1000, "Revenus", "income", { is_inputable: false, sort_order: 1 }), + cat(1010, "Emploi", "income", { parent_id: 1000, is_inputable: false, sort_order: 1 }), + cat(1011, "Paie régulière", "income", { parent_id: 1010, sort_order: 1 }), + cat(1012, "Primes & bonus", "income", { parent_id: 1010, sort_order: 2 }), + cat(1013, "Travail autonome", "income", { parent_id: 1010, sort_order: 3 }), + cat(1020, "Gouvernemental", "income", { parent_id: 1000, is_inputable: false, sort_order: 2 }), + cat(1021, "Remboursement impôt", "income", { parent_id: 1020, sort_order: 1 }), + cat(1022, "Allocations familiales", "income", { parent_id: 1020, sort_order: 2 }), + cat(1023, "Crédits TPS/TVQ", "income", { parent_id: 1020, sort_order: 3 }), + cat(1024, "Assurance-emploi / RQAP", "income", { parent_id: 1020, sort_order: 4 }), + cat(1030, "Revenus de placement", "income", { parent_id: 1000, is_inputable: false, sort_order: 3 }), + cat(1031, "Intérêts & dividendes", "income", { parent_id: 1030, sort_order: 1 }), + cat(1032, "Gains en capital", "income", { parent_id: 1030, sort_order: 2 }), + cat(1033, "Revenus locatifs", "income", { parent_id: 1030, sort_order: 3 }), + cat(1090, "Autres revenus", "income", { parent_id: 1000, sort_order: 9 }), +]; + +// Shape of "Finances & obligations" of a new profile: leaves before AND after +// intermediate parents ("Retrait cash" sorts after "Impôts & taxes"). +const FINANCES: Category[] = [ + cat(1900, "Finances & obligations", "expense", { is_inputable: false, sort_order: 10 }), + cat(1910, "Frais bancaires", "expense", { parent_id: 1900, is_inputable: false, sort_order: 1 }), + cat(1911, "Frais de compte", "expense", { parent_id: 1910, sort_order: 1 }), + cat(1912, "Intérêts & frais de crédit", "expense", { parent_id: 1910, sort_order: 2 }), + cat(1913, "Frais de change", "expense", { parent_id: 1910, sort_order: 3 }), + cat(1920, "Impôts & taxes", "expense", { parent_id: 1900, is_inputable: false, sort_order: 2 }), + cat(1921, "Impôt fédéral", "expense", { parent_id: 1920, sort_order: 1 }), + cat(1922, "Impôt provincial", "expense", { parent_id: 1920, sort_order: 2 }), + cat(1923, "Acomptes provisionnels", "expense", { parent_id: 1920, sort_order: 3 }), + cat(1930, "Dons & cotisations", "expense", { parent_id: 1900, is_inputable: false, sort_order: 3 }), + cat(1931, "Dons de charité", "expense", { parent_id: 1930, sort_order: 1 }), + cat(1932, "Cotisations professionnelles", "expense", { parent_id: 1930, sort_order: 2 }), + cat(1933, "Cotisations syndicales", "expense", { parent_id: 1930, sort_order: 3 }), + cat(1940, "Cadeaux", "expense", { parent_id: 1900, sort_order: 4 }), + cat(1945, "Retrait cash", "expense", { parent_id: 1900, sort_order: 5 }), + cat(1946, "Achats divers non catégorisés", "expense", { parent_id: 1900, sort_order: 6 }), +]; + +// Shape of the older taxonomy (default profile): an inputable root, so a +// "(direct)" row, and one intermediate parent (Assurances, migration v7). +const RECURRENTES: Category[] = [ + cat(2, "Dépenses récurrentes", "expense", { sort_order: 2 }), + cat(20, "Loyer", "expense", { parent_id: 2, sort_order: 1 }), + cat(21, "Électricité", "expense", { parent_id: 2, sort_order: 2 }), + cat(31, "Assurances", "expense", { parent_id: 2, is_inputable: false, sort_order: 12 }), + cat(310, "Assurance-auto", "expense", { parent_id: 31, sort_order: 1 }), + cat(311, "Assurance-habitation", "expense", { parent_id: 31, sort_order: 2 }), + cat(312, "Assurance-vie", "expense", { parent_id: 31, sort_order: 3 }), + cat(32, "Pharmacie", "expense", { parent_id: 2, sort_order: 13 }), + cat(33, "Taxes municipales", "expense", { parent_id: 2, sort_order: 14 }), +]; + +describe("buildBudgetYearRows — depth-first emission (issue #424)", () => { + it("emits income before expense, each group depth-first: Utilities is followed by its own leaves", () => { + const rows = buildBudgetYearRows(CATEGORIES, [], []); + // Salary (income) first; then Housing, its children sorted by name with + // leaves and sub-groups mixed: Rent, then Utilities and its leaves. + expect(rows.map((r) => r.category_id)).toEqual([10, 1, 2, 3, 4, 5]); + expectDepthFirst(rows); }); - it("keeps depth non-decreasing within a top group — the property that would have killed the v1 (DFS-assuming) collapse algorithm", () => { - const expenseGroup = rows.filter((r) => r.category_type === "expense"); - const depths = expenseGroup.map((r) => r.depth ?? 0); - for (let i = 1; i < depths.length; i++) { - expect(depths[i]).toBeGreaterThanOrEqual(depths[i - 1]); - } + it("Revenus: the depth-1 leaf sorts first by name, each sub-group carries its own leaves", () => { + const rows = buildBudgetYearRows(REVENUS, [], []); + expect(names(rows)).toEqual([ + "Revenus", + "Autres revenus", + "Emploi", + "Paie régulière", + "Primes & bonus", + "Travail autonome", + "Gouvernemental", + "Allocations familiales", + "Assurance-emploi / RQAP", + "Crédits TPS/TVQ", + "Remboursement impôt", + "Revenus de placement", + "Gains en capital", + "Intérêts & dividendes", + "Revenus locatifs", + ]); + expectDepthFirst(rows); + }); - // Explicit DFS-killer: the depth-2 grandchildren (Hydro/Internet) come AFTER - // the depth-1 leaf sibling (Rent). A depth-first emission would interleave - // them directly under Utilities, i.e. BEFORE Rent. - const leafIdx = (id: number) => rows.findIndex((r) => r.category_id === id && !r.is_parent); - expect(leafIdx(4)).toBeGreaterThan(leafIdx(2)); // Hydro after Rent - expect(leafIdx(5)).toBeGreaterThan(leafIdx(2)); // Internet after Rent + it("Finances & obligations: leaves before and after sub-groups, sorted by name together", () => { + const rows = buildBudgetYearRows(FINANCES, [], []); + expect(names(rows)).toEqual([ + "Finances & obligations", + "Achats divers non catégorisés", + "Cadeaux", + "Dons & cotisations", + "Cotisations professionnelles", + "Cotisations syndicales", + "Dons de charité", + "Frais bancaires", + "Frais de change", + "Frais de compte", + "Intérêts & frais de crédit", + "Impôts & taxes", + "Acomptes provisionnels", + "Impôt fédéral", + "Impôt provincial", + "Retrait cash", + ]); + expectDepthFirst(rows); + }); + + it("older taxonomy: the (direct) row leads, Assurances is followed by its three leaves", () => { + const rows = buildBudgetYearRows(RECURRENTES, [], []); + expect(names(rows)).toEqual([ + "Dépenses récurrentes", + "Dépenses récurrentes (direct)", + "Assurances", + "Assurance-auto", + "Assurance-habitation", + "Assurance-vie", + "Électricité", + "Loyer", + "Pharmacie", + "Taxes municipales", + ]); + expectDepthFirst(rows); + }); + + it("top groups are sorted by type, then sort_order, then name — whatever the input order", () => { + const all = [ + ...FINANCES, + ...RECURRENTES, + ...REVENUS, + cat(9000, "Divers", "expense", { sort_order: 2 }), + cat(9100, "Virements", "transfer", { sort_order: 0 }), + ]; + const rows = buildBudgetYearRows([...all].reverse(), [], []); + const roots = rows.filter((r) => (r.depth ?? 0) === 0).map((r) => r.category_name); + expect(roots).toEqual([ + "Revenus", // income + "Dépenses récurrentes", // expense, sort_order 2, name before "Divers" + "Divers", // expense, sort_order 2 (lone leaf) + "Finances & obligations", // expense, sort_order 10 + "Virements", // transfer, last despite sort_order 0 + ]); + expectDepthFirst(rows); + }); + + it("subtotals at the bottom: each subtotal follows its own rows (reorderRows on the builder output)", () => { + const rows = reorderRows(buildBudgetYearRows([...FINANCES, ...RECURRENTES], [], []), false); + expect(names(rows)).toEqual([ + "Dépenses récurrentes (direct)", + "Assurance-auto", + "Assurance-habitation", + "Assurance-vie", + "Assurances", + "Électricité", + "Loyer", + "Pharmacie", + "Taxes municipales", + "Dépenses récurrentes", + "Achats divers non catégorisés", + "Cadeaux", + "Cotisations professionnelles", + "Cotisations syndicales", + "Dons de charité", + "Dons & cotisations", + "Frais de change", + "Frais de compte", + "Intérêts & frais de crédit", + "Frais bancaires", + "Acomptes provisionnels", + "Impôt fédéral", + "Impôt provincial", + "Impôts & taxes", + "Retrait cash", + "Finances & obligations", + ]); + }); + + it("expanding a single intermediate parent shows its leaves right under it", () => { + const rows = buildBudgetYearRows(REVENUS, [], []); + const flipped = new Set(["p:1000", "p:1010"]); // Revenus + Emploi expanded + const isCollapsed = (r: BudgetYearRow) => isCollapsedFor(flipped, ACCESSORS.keyOf(r), false); + const visible = visibleRows(rows, ACCESSORS, isCollapsed); + expect(names(visible)).toEqual([ + "Revenus", + "Autres revenus", + "Emploi", + "Paie régulière", + "Primes & bonus", + "Travail autonome", + "Gouvernemental", + "Revenus de placement", + ]); + }); + + it("subtotals are unchanged by the order: each subtotal sums its own leaves", () => { + const ENTRIES = [entry(1011, 100), entry(1012, 20), entry(1021, 7), entry(1033, 3), entry(1090, 1000)]; + const rows = buildBudgetYearRows(REVENUS, ENTRIES, []); + const annualOf = (id: number) => rows.find((r) => r.category_id === id && r.is_parent)!.annual; + expect(annualOf(1000)).toBe(1130); + expect(annualOf(1010)).toBe(120); + expect(annualOf(1020)).toBe(7); + expect(annualOf(1030)).toBe(3); }); }); @@ -102,9 +336,9 @@ describe("buildBudgetYearRows + multi-level collapse (issue #289 end-to-end)", ( const flipped = new Set(["p:1"]); // Housing expanded const isCollapsed = (r: BudgetYearRow) => isCollapsedFor(flipped, ACCESSORS.keyOf(r), false); const visible = visibleRows(rows, ACCESSORS, isCollapsed); - // Housing's direct children: Utilities subtotal + Rent leaf. Hydro/Internet - // remain hidden under the collapsed Utilities. - expect(visible.map((r) => r.category_id)).toEqual([10, 1, 3, 2]); + // Housing's direct children, by name: Rent leaf + Utilities subtotal. + // Hydro/Internet remain hidden under the collapsed Utilities. + expect(visible.map((r) => r.category_id)).toEqual([10, 1, 2, 3]); }); }); diff --git a/src/hooks/useBudget.ts b/src/hooks/useBudget.ts index a476293..45a37b8 100644 --- a/src/hooks/useBudget.ts +++ b/src/hooks/useBudget.ts @@ -88,16 +88,22 @@ const TYPE_ORDER: Record = { income: 0, expense: 1, transfer: 2 * Assembles the flat, hierarchical `BudgetYearRow[]` the grid renders from the * raw category tree, this year's budget entries, and last year's actual totals. * - * ORDERING INVARIANT (issue #289 — the multi-level-collapse consumer). The - * final `rows.sort` below orders each top category group by DEPTH ASCENDING - * (level-order / BFS), NOT depth-first: within a group every depth-0 row - * precedes every depth-1 row, which precedes every depth-2 row. The abandoned - * v1 collapse algorithm wrongly assumed a depth-first emission order and broke - * exactly here; the shipped algorithm (ancestor walk — see `collapsibleRows.ts`) - * is order-INDEPENDENT, so this sort needs NO change. The level-order property - * is pinned by `useBudget.test.ts` so a refactor can't silently reintroduce the - * v1 assumption — do not reorder into DFS without revisiting the collapse - * consumer (`BudgetTable`). + * ORDERING INVARIANT (issue #424, replacing the level-order of #289). Rows are + * emitted DEPTH-FIRST: every parent row is followed directly by its whole + * subtree, so an expanded subcategory shows right under its parent and + * `reorderRows` (subtotals at the bottom) can attach each subtotal to its own + * rows. The mechanism is fixed by blocks, sorted BEFORE they are emitted: + * - inside a top group, one block per child: a lone leaf, or a sub-group + * `[subtotal, ...its children]` (what `buildLevel2Group` returns); + * - the blocks of a group are sorted once: the "(direct)" block first, then by + * `localeCompare` of the leaf or subtotal name, leaves and sub-groups mixed + * (the sibling order of the Budget vs Actual report, `budgetService.ts`); + * - top groups (`[parent, ...]` block or lone leaf) are sorted by type + * (`TYPE_ORDER`), then `sort_order`, then name, and concatenated. + * No comparator ever runs on rows inside a group: a depth sort there would + * bring back the level order that pushed subcategories to the end of their + * group. The collapse consumer (`collapsibleRows.ts`, ancestor walk) does not + * depend on the order; the depth-first property is pinned by `useBudget.test.ts`. * * Pure (no DB, no React) so the invariant is unit-testable without a renderer — * the repo has no jsdom, so hook logic that must be tested is extracted here, @@ -136,8 +142,7 @@ export function buildBudgetYearRows( return { months, annual, previousYearTotal }; }; - // Index categories by id and group children by parent_id - const catById = new Map(allCategories.map((c) => [c.id, c])); + // Group children by parent_id const childrenByParent = new Map(); for (const cat of allCategories) { if (cat.parent_id) { @@ -146,7 +151,8 @@ export function buildBudgetYearRows( } } - const rows: BudgetYearRow[] = []; + // One entry per top group, sorted then flattened at the end (issue #424). + const topGroups: Array<{ cat: Category; rows: BudgetYearRow[] }> = []; // Build rows for an intermediate parent (level 1 or 2 with children) function buildLevel2Group(cat: typeof allCategories[0], grandparentId: number): BudgetYearRow[] { @@ -246,7 +252,7 @@ export function buildBudgetYearRows( if (inputableChildren.length === 0 && intermediateParents.length === 0 && cat.is_inputable) { // Standalone leaf (no children) — regular editable row const { months, annual, previousYearTotal } = buildMonths(cat.id); - rows.push({ + topGroups.push({ cat, rows: [{ category_id: cat.id, category_name: cat.name, category_color: cat.color || "#9ca3af", @@ -257,14 +263,21 @@ export function buildBudgetYearRows( months, annual, previousYearTotal, - }); + }] }); } else if (inputableChildren.length > 0 || intermediateParents.length > 0) { - const allChildRows: BudgetYearRow[] = []; + // One block per child: a lone leaf, or a sub-group [subtotal, ...children]. + // `name` is the leaf or subtotal name the blocks are sorted by. + const blocks: Array<{ isDirect: boolean; name: string; rows: BudgetYearRow[] }> = []; + const pushBlock = (blockRows: BudgetYearRow[], isDirect = false) => { + if (blockRows.length > 0) { + blocks.push({ isDirect, name: blockRows[0].category_name, rows: blockRows }); + } + }; // If parent is also inputable, create a "(direct)" fake-child row if (cat.is_inputable) { const { months, annual, previousYearTotal } = buildMonths(cat.id); - allChildRows.push({ + pushBlock([{ category_id: cat.id, category_name: `${cat.name} (direct)`, category_color: cat.color || "#9ca3af", @@ -275,7 +288,7 @@ export function buildBudgetYearRows( months, annual, previousYearTotal, - }); + }], true); } for (const child of inputableChildren) { @@ -283,7 +296,7 @@ export function buildBudgetYearRows( if (grandchildren.length === 0) { // Simple leaf at depth 1 const { months, annual, previousYearTotal } = buildMonths(child.id); - allChildRows.push({ + pushBlock([{ category_id: child.id, category_name: child.name, category_color: child.color || cat.color || "#9ca3af", @@ -294,19 +307,27 @@ export function buildBudgetYearRows( months, annual, previousYearTotal, - }); + }]); } else { // Intermediate parent at depth 1 with grandchildren - allChildRows.push(...buildLevel2Group(child, cat.id)); + pushBlock(buildLevel2Group(child, cat.id)); } } // Non-inputable intermediate parents for (const ip of intermediateParents) { - allChildRows.push(...buildLevel2Group(ip, cat.id)); + pushBlock(buildLevel2Group(ip, cat.id)); } - if (allChildRows.length === 0) continue; + if (blocks.length === 0) continue; + + // Sort the blocks once, before emission: "(direct)" first, then by name, + // leaves and sub-groups mixed. Rows inside a block keep their order. + blocks.sort((a, b) => { + if (a.isDirect !== b.isDirect) return a.isDirect ? -1 : 1; + return a.name.localeCompare(b.name); + }); + const allChildRows = blocks.flatMap((b) => b.rows); // Parent subtotal row: sum of leaf rows only (avoid double-counting) const leafRows = allChildRows.filter((r) => !r.is_parent); @@ -319,67 +340,41 @@ export function buildBudgetYearRows( parentPrevYear += cr.previousYearTotal; } - rows.push({ - category_id: cat.id, - category_name: cat.name, - category_color: cat.color || "#9ca3af", - category_type: cat.type, - parent_id: null, - is_parent: true, - depth: 0, - months: parentMonths, - annual: parentAnnual, - previousYearTotal: parentPrevYear, + topGroups.push({ + cat, + rows: [ + { + category_id: cat.id, + category_name: cat.name, + category_color: cat.color || "#9ca3af", + category_type: cat.type, + parent_id: null, + is_parent: true, + depth: 0, + months: parentMonths, + annual: parentAnnual, + previousYearTotal: parentPrevYear, + }, + ...allChildRows, + ], }); - - // Sort children alphabetically, but keep "(direct)" first - allChildRows.sort((a, b) => { - if (a.category_id === cat.id && !a.is_parent) return -1; - if (b.category_id === cat.id && !b.is_parent) return 1; - return a.category_name.localeCompare(b.category_name); - }); - - rows.push(...allChildRows); } // else: non-inputable parent with no inputable children — skip } - // Sort by type, then within each type: keep hierarchy groups together - function getTopGroupId(r: BudgetYearRow): number { - if ((r.depth ?? 0) === 0) return r.category_id; - if (r.is_parent && r.parent_id === null) return r.category_id; - let pid = r.parent_id; - while (pid !== null) { - const pCat = catById.get(pid); - if (!pCat || !pCat.parent_id) return pid; - pid = pCat.parent_id; - } - return r.category_id; - } - - rows.sort((a, b) => { - const typeA = TYPE_ORDER[a.category_type] ?? 9; - const typeB = TYPE_ORDER[b.category_type] ?? 9; + // Sort the top groups by type, then sort_order, then name, and concatenate + // them. Each group keeps its depth-first row order (issue #424). + topGroups.sort((a, b) => { + const typeA = TYPE_ORDER[a.cat.type] ?? 9; + const typeB = TYPE_ORDER[b.cat.type] ?? 9; if (typeA !== typeB) return typeA - typeB; - const groupA = getTopGroupId(a); - const groupB = getTopGroupId(b); - if (groupA !== groupB) { - const catA = catById.get(groupA); - const catB = catById.get(groupB); - const orderA = catA?.sort_order ?? 999; - const orderB = catB?.sort_order ?? 999; - if (orderA !== orderB) return orderA - orderB; - return (catA?.name ?? "").localeCompare(catB?.name ?? ""); - } - // Same group: sort by depth, then parent before children at same depth - if (a.is_parent !== b.is_parent && (a.depth ?? 0) === (b.depth ?? 0)) return a.is_parent ? -1 : 1; - if ((a.depth ?? 0) !== (b.depth ?? 0)) return (a.depth ?? 0) - (b.depth ?? 0); - if (a.parent_id && a.category_id === a.parent_id) return -1; - if (b.parent_id && b.category_id === b.parent_id) return 1; - return a.category_name.localeCompare(b.category_name); + const orderA = a.cat.sort_order ?? 999; + const orderB = b.cat.sort_order ?? 999; + if (orderA !== orderB) return orderA - orderB; + return a.cat.name.localeCompare(b.cat.name); }); - return rows; + return topGroups.flatMap((g) => g.rows); } /** diff --git a/src/utils/collapsibleRows.test.ts b/src/utils/collapsibleRows.test.ts index cd30aed..68edc12 100644 --- a/src/utils/collapsibleRows.test.ts +++ b/src/utils/collapsibleRows.test.ts @@ -54,7 +54,9 @@ const dfsTree: Row[] = [ ]; // The EXACT same tree, emitted level-order (BFS) — the order the budget grid -// produces. The ancestor-walk must mask identically to the DFS order. +// produced before issue #424 (it is depth-first since). Kept as the proof that +// the ancestor walk does not depend on the order: it must mask identically to +// the DFS order. const bfsTree: Row[] = [ r(1, null, true, 0), // Housing r(2, null, true, 0), // Salary diff --git a/src/utils/collapsibleRows.ts b/src/utils/collapsibleRows.ts index ef61a76..7f757c5 100644 --- a/src/utils/collapsibleRows.ts +++ b/src/utils/collapsibleRows.ts @@ -11,7 +11,8 @@ * row until we reach a root (null), an ancestor outside this section (⇒ visible), * or a collapsed ancestor (⇒ hidden). This is independent of the order rows * arrive in — the fix for the v1 adjacency algorithm, which assumed a - * depth-first order the budget grid (level-ordered) does not emit. + * depth-first order the budget grid did not emit at the time. The grid has been + * depth-first since issue #424, but the walk keeps not relying on it. * * The accessors are supplied by each consumer so every table can key + climb the * exact hierarchy it renders. Keys are prefixed `p:` so they are injective: a diff --git a/src/utils/reorderRows.test.ts b/src/utils/reorderRows.test.ts new file mode 100644 index 0000000..a2609f4 --- /dev/null +++ b/src/utils/reorderRows.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect } from "vitest"; +import { reorderRows } from "./reorderRows"; + +// Minimal row: the two fields `reorderRows` reads, plus a name to assert on. +interface Row { + name: string; + is_parent?: boolean; + depth?: number; +} + +const leaf = (name: string, depth: number): Row => ({ name, is_parent: false, depth }); +const sub = (name: string, depth: number): Row => ({ name, is_parent: true, depth }); +const names = (rows: Row[]) => rows.map((r) => r.name); + +// Two levels, depth-first: a root group whose leaves sit before AND after a +// sibling sub-group, then a lone root leaf. +// Finances +// Cadeaux +// Impôts (sub-group) +// Fédéral +// Provincial +// Retrait cash <- follows the sibling sub-group "Impôts" +// Divers (root leaf) <- follows the root group "Finances" +const TWO_LEVELS: Row[] = [ + sub("Finances", 0), + leaf("Cadeaux", 1), + sub("Impôts", 1), + leaf("Fédéral", 2), + leaf("Provincial", 2), + leaf("Retrait cash", 1), + leaf("Divers", 0), +]; + +// Three levels, as the Compare report tree allows (depth is not capped at 2). +// A +// A1 (sub-group) +// A1a (sub-group) +// x +// y <- follows the sibling sub-group "A1a" +// z <- follows the sibling sub-group "A1" +const THREE_LEVELS: Row[] = [ + sub("A", 0), + sub("A1", 1), + sub("A1a", 2), + leaf("x", 3), + leaf("y", 2), + leaf("z", 1), +]; + +describe("reorderRows — subtotals at the bottom (issue #424)", () => { + it("two levels: a leaf that follows a sibling sub-group stays outside its block", () => { + expect(names(reorderRows(TWO_LEVELS, false))).toEqual([ + "Cadeaux", + "Fédéral", + "Provincial", + "Impôts", + "Retrait cash", + "Finances", + "Divers", + ]); + }); + + it("three levels: each subtotal follows its own rows only", () => { + expect(names(reorderRows(THREE_LEVELS, false))).toEqual(["x", "A1a", "y", "A1", "z", "A"]); + }); + + it("consecutive sibling sub-groups each close on their own leaves", () => { + const rows: Row[] = [ + sub("Revenus", 0), + leaf("Autres", 1), + sub("Emploi", 1), + leaf("Paie", 2), + sub("Gouv", 1), + leaf("Alloc", 2), + ]; + expect(names(reorderRows(rows, false))).toEqual(["Autres", "Paie", "Emploi", "Alloc", "Gouv", "Revenus"]); + }); + + it("keeps every row exactly once", () => { + const out = reorderRows(TWO_LEVELS, false); + expect(out).toHaveLength(TWO_LEVELS.length); + expect(new Set(out)).toEqual(new Set(TWO_LEVELS)); + }); +}); + +describe("reorderRows — subtotals on top", () => { + it("returns the list unchanged", () => { + expect(reorderRows(TWO_LEVELS, true)).toBe(TWO_LEVELS); + expect(reorderRows(THREE_LEVELS, true)).toBe(THREE_LEVELS); + }); +}); diff --git a/src/utils/reorderRows.ts b/src/utils/reorderRows.ts index 17eaab4..59b0bbb 100644 --- a/src/utils/reorderRows.ts +++ b/src/utils/reorderRows.ts @@ -3,6 +3,11 @@ * Recursively moves subtotal (parent) rows below their children * at every depth level when "subtotals on bottom" is enabled. * + * Expects a depth-first list (each parent followed by its whole subtree), the + * order `buildBudgetYearRows` and the report tree builders emit. A subtotal's + * block ends at the next row of its own depth or shallower, so a leaf that + * follows a sibling sub-group stays outside that sub-group (issue #424). + * * The generic constraint only requires the two fields the algorithm actually * reads (`is_parent`, `depth`) so it works for both the budget rows (required * `is_parent`) and the Compare `CategoryDelta` rows (optional `is_parent`). @@ -25,6 +30,15 @@ export function reorderRows< currentChildren = []; } currentParent = row; + } else if (currentParent && (row.depth ?? 0) <= parentDepth) { + // A row at this level (or above) that is not a parent of this level + // closes the current block: it is a sibling of the subtotal, not one of + // its children (issue #424 — a leaf after a sibling sub-group used to + // fall into that sub-group's block). + result.push(...reorderGroup(currentChildren, parentDepth + 1), currentParent); + currentChildren = []; + currentParent = null; + result.push(row); } else if (currentParent) { currentChildren.push(row); } else {