From df3fefe93483f671ae26c80f503a2eb65d556df8 Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 18 Oct 2026 19:16:14 -0400 Subject: [PATCH 1/7] feat: gather the category hierarchy rules in one module (#393) src/utils/categoryHierarchy.ts holds the depth, height and descent walks that buildParentOptions had inline, each with a set of the categories it has seen, so a parent_id cycle ends the walk. A category caught in a cycle, or under one, now has no depth instead of a made-up one, and the parent list of the category form never offers it: a child placed there would be hidden with it. The module also carries the orphan diagnostic, the parent change refusal and the root rule that the service, the Categories page and the budget use next. Co-Authored-By: Claude Opus 5.5 --- .../categories/parentOptions.test.ts | 7 + src/components/categories/parentOptions.ts | 77 +----- .../shared/CategoryCombobox.test.ts | 13 + src/utils/categoryHierarchy.test.ts | 220 ++++++++++++++++ src/utils/categoryHierarchy.ts | 237 ++++++++++++++++++ 5 files changed, 490 insertions(+), 64 deletions(-) create mode 100644 src/utils/categoryHierarchy.test.ts create mode 100644 src/utils/categoryHierarchy.ts diff --git a/src/components/categories/parentOptions.test.ts b/src/components/categories/parentOptions.test.ts index def71fa..eaaeb58 100644 --- a/src/components/categories/parentOptions.test.ts +++ b/src/components/categories/parentOptions.test.ts @@ -97,6 +97,13 @@ describe("buildParentOptions — editing a category", () => { expect(offered.every((id) => id === 1 || id === 2)).toBe(true); }); + it("never offers a member of a cycle, nor what hangs under it: a child there would be hidden (#393)", () => { + // 1 <-> 2 with 4 under 2; 3 and 5 are sound roots. Creating, then editing 5. + const cyclic = [node(1, 2), node(2, 1), node(4, 2), node(3, null), node(5, null)]; + expect(ids(buildParentOptions(cyclic))).toEqual([3, 5]); + expect(ids(buildParentOptions(cyclic, { editingId: 5 }))).toEqual([3]); + }); + it("terminates when the edited category is itself a member of a cycle", () => { // 1 <-> 2: the height walk under 1 meets 1 again and stops (height 1, not // unbounded). Only roots remain, so the root outside the cycle is offered. diff --git a/src/components/categories/parentOptions.ts b/src/components/categories/parentOptions.ts index 04fd296..2b5da06 100644 --- a/src/components/categories/parentOptions.ts +++ b/src/components/categories/parentOptions.ts @@ -1,7 +1,5 @@ import type { CategoryTreeNode } from "../../shared/types"; - -/** Deepest level a category may sit at: root (0), child (1), grandchild (2). */ -const MAX_PARENT_DEPTH = 1; +import { depthsOf, fitsUnder, subtreeHeight } from "../../utils/categoryHierarchy"; export interface ParentOptionsContext { /** Id of the category being edited; absent or null when creating. */ @@ -14,60 +12,9 @@ export interface ParentOptionsContext { currentParentId?: number | null; } -function depthsOf(categories: CategoryTreeNode[]): Map { - const byId = new Map(); - for (const c of categories) byId.set(c.id, c); - const depths = new Map(); - const depthOf = (id: number, seen: Set): number => { - const known = depths.get(id); - if (known !== undefined) return known; - if (seen.has(id)) return 0; // cycle: treat as a root rather than loop - seen.add(id); - const cat = byId.get(id); - const parentId = cat?.parent_id ?? null; - // A missing parent (orphan) counts as a root, like the tree and the combobox do. - const d = parentId === null || !byId.has(parentId) ? 0 : depthOf(parentId, seen) + 1; - depths.set(id, d); - return d; - }; - for (const c of categories) depthOf(c.id, new Set()); - return depths; -} - /** - * Height of the sub-tree under `rootId`: 0 for a leaf, 1 with children, 2 with - * grandchildren. Walks `parent_id` links; a cycle is not walked twice, and the - * walk can never take more steps than there are categories. - */ -function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number { - const childrenOf = new Map(); - for (const c of categories) { - if (c.parent_id === null) continue; - const bucket = childrenOf.get(c.parent_id) ?? []; - bucket.push(c.id); - childrenOf.set(c.parent_id, bucket); - } - let height = 0; - const seen = new Set([rootId]); - let frontier = [rootId]; - while (frontier.length > 0 && height < categories.length) { - const next: number[] = []; - for (const id of frontier) { - for (const child of childrenOf.get(id) ?? []) { - if (seen.has(child)) continue; - seen.add(child); - next.push(child); - } - } - if (next.length === 0) break; - height += 1; - frontier = next; - } - return height; -} - -/** - * Categories that may be chosen as the parent in the category form. + * Categories that may be chosen as the parent in the category form, and in the + * « Vérifier la hiérarchie » window (#393). * * - Roots and level-1 categories only: a level-2 parent would create a fourth * level, which nothing else in the app knows how to display. @@ -78,11 +25,15 @@ function subtreeHeight(categories: CategoryTreeNode[], rootId: number): number { * whole descent, which would create a cycle that hides the branch everywhere * (#393): a descendant sits at least one level below the edited category, * deeper than any depth still allowed. + * - A category caught in a `parent_id` cycle, or under one, has no depth and is + * never offered: a child placed under it would be hidden with it. * - The parent currently set on the form is always kept when it exists. * - * This list is the only guard: `updateCategory` writes any `parent_id` it is - * given, without a depth or cycle check (#393 will add one). The form therefore - * never offers a new parent that the rules above reject. + * The depth and height rules are those of `src/utils/categoryHierarchy.ts`, + * which `updateCategory` and `reattachCategory` apply again before writing + * (#393): the list only spares a refusal at save time. It does not leave out + * a parent that holds transactions; the service refuses it with a translated + * message. * * Ordering is left to the caller (`CategoryCombobox` sorts hierarchically). */ @@ -95,15 +46,13 @@ export function buildParentOptions( const depths = depthsOf(categories); const height = editingId !== null ? subtreeHeight(categories, editingId) : 0; - // Negative when the edited category has grandchildren: only the current parent survives. - const maxDepth = MAX_PARENT_DEPTH - height; return categories.filter((c) => { if (c.id === editingId) return false; // Kept even when the depth rule would drop it: the field must show what is - // stored. Keeping it changes nothing on save (it is already the parent); - // repairing such data is the job of #393. + // stored. Keeping it changes nothing on save (it is already the parent). if (c.id === currentParentId) return true; - return (depths.get(c.id) ?? 0) <= maxDepth; + const depth = depths.get(c.id); + return depth !== undefined && fitsUnder(depth, height); }); } diff --git a/src/components/shared/CategoryCombobox.test.ts b/src/components/shared/CategoryCombobox.test.ts index 854e52c..11cfa5b 100644 --- a/src/components/shared/CategoryCombobox.test.ts +++ b/src/components/shared/CategoryCombobox.test.ts @@ -99,6 +99,19 @@ describe("sortHierarchical", () => { expect(ordered).toEqual([1, 10, 500]); }); + it("leaves out the members of a parent_id cycle and what hangs under it, and ends (#393)", () => { + // 3 <-> 5, 8 under 5: no root leads to them, and their parents are in the + // list, so they are no orphans of it either. This is why a cycle hides a + // category from every picker; « Vérifier la hiérarchie » repairs it. + const input: Category[] = [ + cat(1, "Revenus", null, 1), + cat(3, "Épicerie", 5, 1), + cat(5, "Alimentation", 3, 1), + cat(8, "Boulangerie", 5, 2), + ]; + expect(sortHierarchical(input, displayName).map((c) => c.id)).toEqual([1]); + }); + it("is stable (does not duplicate) when called with already-ordered input", () => { const input: Category[] = [ cat(1, "Revenus", null, 1), diff --git a/src/utils/categoryHierarchy.test.ts b/src/utils/categoryHierarchy.test.ts new file mode 100644 index 0000000..de853a6 --- /dev/null +++ b/src/utils/categoryHierarchy.test.ts @@ -0,0 +1,220 @@ +// categoryHierarchy — the rules of the category hierarchy (#393): depths, +// heights and descent, the orphan diagnostic and the validation of a new +// parent. Every walk must end on a `parent_id` cycle: a test that loops runs +// into vitest's timeout and goes red. +import { describe, it, expect } from "vitest"; +import { + MAX_CATEGORY_DEPTH, + depthsOf, + descendantsOf, + displaysAsRoot, + findOrphans, + fitsUnder, + isActiveRow, + parentChangeRefusal, + subtreeHeight, + type HierarchyNode, + type HierarchyRow, +} from "./categoryHierarchy"; + +const n = (id: number, parent_id: number | null): HierarchyNode => ({ id, parent_id }); +/** An active row, or an inactive one with `active = false`. */ +const r = (id: number, parent_id: number | null, active = true): HierarchyRow => ({ + id, + parent_id, + is_active: active ? 1 : 0, +}); + +// Two roots. 1 -> 10 -> 100 and 1 -> 11; 2 -> 20. +const TREE = [n(1, null), n(10, 1), n(100, 10), n(11, 1), n(2, null), n(20, 2)]; + +const depthEntries = (nodes: HierarchyNode[]) => + [...depthsOf(nodes).entries()].sort(([a], [b]) => a - b); + +describe("depthsOf", () => { + it("gives 0 to a root, 1 to its child, 2 to a grandchild", () => { + expect(depthEntries(TREE)).toEqual([ + [1, 0], + [2, 0], + [10, 1], + [11, 1], + [20, 1], + [100, 2], + ]); + }); + + it("does not depend on the order of the list", () => { + expect(depthEntries([...TREE].reverse())).toEqual(depthEntries(TREE)); + }); + + it("treats a category whose parent is not in the list as a root", () => { + expect(depthEntries([n(10, 1), n(100, 10)])).toEqual([ + [10, 0], + [100, 1], + ]); + }); + + it("gives no depth to the members of a cycle nor to what hangs under it, and ends", () => { + // 1 <-> 2, 3 under 1, 4 under 3; 5 is a sound root. + const depths = depthsOf([n(3, 1), n(1, 2), n(2, 1), n(4, 3), n(5, null)]); + expect([...depths.entries()]).toEqual([[5, 0]]); + }); + + it("gives no depth to a category that is its own parent", () => { + expect(depthsOf([n(7, 7), n(8, 7)]).size).toBe(0); + }); + + it("ends on a three-member cycle reached from below", () => { + const depths = depthsOf([n(9, 3), n(3, 2), n(2, 1), n(1, 3)]); + expect(depths.size).toBe(0); + }); +}); + +describe("subtreeHeight and descendantsOf", () => { + it("count the levels and list the categories under a category", () => { + expect(subtreeHeight(TREE, 1)).toBe(2); + expect(subtreeHeight(TREE, 10)).toBe(1); + expect(subtreeHeight(TREE, 100)).toBe(0); + expect([...descendantsOf(TREE, 1)].sort((a, b) => a - b)).toEqual([10, 11, 100]); + expect(descendantsOf(TREE, 100).size).toBe(0); + }); + + it("end on a cycle, each category once, the start never listed", () => { + // 1 <-> 2, 3 under 2. + const cyclic = [n(1, 2), n(2, 1), n(3, 2)]; + expect(subtreeHeight(cyclic, 1)).toBe(2); + expect([...descendantsOf(cyclic, 1)].sort((a, b) => a - b)).toEqual([2, 3]); + expect(subtreeHeight([n(7, 7)], 7)).toBe(0); + expect(descendantsOf([n(7, 7)], 7).size).toBe(0); + }); +}); + +describe("fitsUnder", () => { + it("allows three levels, 0 to 2, counting the sub-tree", () => { + expect(MAX_CATEGORY_DEPTH).toBe(2); + expect(fitsUnder(0, 0)).toBe(true); + expect(fitsUnder(1, 0)).toBe(true); + expect(fitsUnder(0, 1)).toBe(true); + expect(fitsUnder(2, 0)).toBe(false); + expect(fitsUnder(1, 1)).toBe(false); + expect(fitsUnder(0, 2)).toBe(false); + }); +}); + +describe("displaysAsRoot", () => { + const ids = new Set([1, 2, 10]); + it("is a root without a parent, or with a parent missing from the list", () => { + expect(displaysAsRoot({ id: 1, parent_id: null }, ids)).toBe(true); + expect(displaysAsRoot({ id: 1 }, ids)).toBe(true); + expect(displaysAsRoot({ id: 30, parent_id: 99 }, ids)).toBe(true); + }); + + it("is not a root under a parent of the list, a cycle member included", () => { + expect(displaysAsRoot({ id: 10, parent_id: 1 }, ids)).toBe(false); + expect(displaysAsRoot({ id: 1, parent_id: 2 }, ids)).toBe(false); + }); +}); + +describe("isActiveRow", () => { + it("reads 1 (or true) as active, anything else as inactive, as `is_active = 1` does", () => { + expect(isActiveRow(r(1, null))).toBe(true); + expect(isActiveRow({ id: 1, parent_id: null, is_active: true })).toBe(true); + expect(isActiveRow(r(1, null, false))).toBe(false); + expect(isActiveRow({ id: 1, parent_id: null, is_active: false })).toBe(false); + }); +}); + +describe("findOrphans (#393)", () => { + it("finds nothing in a sound tree", () => { + expect(findOrphans(TREE.map((c) => r(c.id, c.parent_id ?? null)))).toEqual([]); + }); + + it("finds a category that is its own parent", () => { + expect(findOrphans([r(1, null), r(7, 7), r(8, 7)])).toEqual([{ id: 7, reason: "cycle" }]); + }); + + it("lists a two-member cycle once, by its smallest id, not what hangs under it", () => { + // 5 <-> 3, 8 under 5, 9 under 8. Read from below first. + const rows = [r(9, 8), r(8, 5), r(5, 3), r(3, 5), r(1, null)]; + expect(findOrphans(rows)).toEqual([{ id: 3, reason: "cycle" }]); + }); + + it("lists a three-member cycle once, by its smallest id, whatever the order of the rows", () => { + // 6 -> 4 -> 9 -> 6, 12 under 9. + const rows = [r(6, 4), r(4, 9), r(9, 6), r(12, 9), r(1, null)]; + expect(findOrphans(rows)).toEqual([{ id: 4, reason: "cycle" }]); + expect(findOrphans([...rows].reverse())).toEqual([{ id: 4, reason: "cycle" }]); + }); + + it("names a deactivated parent apart from a missing one, and only the head of each group", () => { + // 20's parent (2) is inactive; 21 under 20 follows it. 30's parent (99) does not exist. + const rows = [r(1, null), r(2, null, false), r(21, 20), r(20, 2), r(30, 99), r(31, 30)]; + expect(findOrphans(rows)).toEqual([ + { id: 20, reason: "inactive_parent" }, + { id: 30, reason: "missing_parent" }, + ]); + }); + + it("never lists an inactive category, nor a cycle among inactive ones", () => { + const rows = [r(1, null), r(2, 3, false), r(3, 2, false), r(4, 4, false)]; + expect(findOrphans(rows)).toEqual([]); + }); + + it("reports a loop that runs through a deactivated category as the deactivated parent", () => { + // 5 -> 6 (inactive) -> 5: the tree shows 5 at the root, its parent being inactive. + expect(findOrphans([r(5, 6), r(6, 5, false)])).toEqual([{ id: 5, reason: "inactive_parent" }]); + }); + + it("lists several groups, sorted by id", () => { + const rows = [r(40, 41), r(41, 40), r(15, 15), r(2, 77), r(1, null)]; + expect(findOrphans(rows)).toEqual([ + { id: 2, reason: "missing_parent" }, + { id: 15, reason: "cycle" }, + { id: 40, reason: "cycle" }, + ]); + }); +}); + +describe("parentChangeRefusal (#393)", () => { + it("accepts the root, always, even for a branch three levels deep or a cycle member", () => { + expect(parentChangeRefusal(TREE, 1, null)).toBeNull(); + expect(parentChangeRefusal([n(1, 2), n(2, 1)], 1, null)).toBeNull(); + }); + + it("refuses the category itself", () => { + expect(parentChangeRefusal(TREE, 11, 11)).toBe("self_parent"); + }); + + it("refuses a child and a grandchild: the move would close a loop", () => { + expect(parentChangeRefusal(TREE, 1, 10)).toBe("cycle"); + expect(parentChangeRefusal(TREE, 1, 100)).toBe("cycle"); + }); + + it("refuses a parent caught in another cycle: the category would be hidden with it", () => { + const nodes = [...TREE, n(50, 51), n(51, 50)]; + expect(parentChangeRefusal(nodes, 20, 50)).toBe("cycle"); + }); + + it("refuses a fourth level, counting the sub-tree of the category", () => { + // 100 is level 2: a child under it would be level 3. + expect(parentChangeRefusal(TREE, 20, 100)).toBe("too_deep"); + // Root 2 has a child (20): under 10 (level 1), 20 would land at level 3. + expect(parentChangeRefusal(TREE, 2, 10)).toBe("too_deep"); + // Root 1 has grandchildren: under any parent, 100 would land at level 3. + expect(parentChangeRefusal(TREE, 1, 2)).toBe("too_deep"); + }); + + it("accepts a valid move", () => { + expect(parentChangeRefusal(TREE, 20, 10)).toBeNull(); // a leaf under a level-1 parent + expect(parentChangeRefusal(TREE, 2, 1)).toBeNull(); // a root with one level under a root + expect(parentChangeRefusal(TREE, 11, 2)).toBeNull(); + }); + + it("moves a member of a cycle under a sound parent when the branch fits, and ends", () => { + // 1 <-> 2 with nothing under them; 3 is a sound root. + const nodes = [n(1, 2), n(2, 1), n(3, null)]; + expect(parentChangeRefusal(nodes, 1, 3)).toBeNull(); + // ... but never under the other member. + expect(parentChangeRefusal(nodes, 1, 2)).toBe("cycle"); + }); +}); diff --git a/src/utils/categoryHierarchy.ts b/src/utils/categoryHierarchy.ts new file mode 100644 index 0000000..c970aaf --- /dev/null +++ b/src/utils/categoryHierarchy.ts @@ -0,0 +1,237 @@ +// categoryHierarchy — the rules of the category hierarchy, in one place (#393). +// +// Three consumers read them: the parent list of the category form +// (`buildParentOptions`, #423), the orphan diagnostic of the Categories page +// (`findOrphans`) and the validation of a parent change in `updateCategory` +// and `reattachCategory` (`parentChangeRefusal`). Every walk, up a parent chain +// or down the children, keeps a set of the categories it has seen: a +// `parent_id` cycle stored in the profile ends the walk instead of looping. +// +// Levels: a root sits at depth 0, its children at 1, their children at 2. +// Three levels at most: the budget grid stops at the grandchildren, so a +// fourth level would drop out of it. + +/** What the hierarchy rules read of a category. */ +export interface HierarchyNode { + id: number; + /** `null` (or absent) for a root. */ + parent_id?: number | null; +} + +/** A row of `SELECT id, parent_id, is_active FROM categories`, active or not. */ +export interface HierarchyRow extends HierarchyNode { + /** `1` for an active category (SQLite's INTEGER flag). */ + is_active: number | boolean; +} + +/** Deepest depth a category may sit at: three levels, 0 to 2. */ +export const MAX_CATEGORY_DEPTH = 2; + +/** Whether the row is active, as the queries' `WHERE is_active = 1` reads it. */ +export function isActiveRow(row: HierarchyRow): boolean { + return row.is_active === 1 || row.is_active === true; +} + +/** + * Whether the category shows as a root among the categories `ids`: it has no + * parent, or a parent that is not in the list (deactivated, or gone). The + * tree, the category pickers and the budget all follow this rule (#393). + */ +export function displaysAsRoot(category: HierarchyNode, ids: ReadonlySet): boolean { + return category.parent_id == null || !ids.has(category.parent_id); +} + +/** + * Depth of each category among `categories`: 0 for a root, 1 for its child, 2 + * for a grandchild. A category whose parent is not in the list counts as a + * root, as in the tree. A category caught in a `parent_id` cycle, or under + * one, has no depth: no root leads to it, and it is left out of the map. + */ +export function depthsOf(categories: readonly HierarchyNode[]): Map { + const byId = new Map(); + for (const c of categories) byId.set(c.id, c); + const depths = new Map(); + /** Categories whose depth is known, or known to be none. */ + const settled = new Set(); + + for (const start of categories) { + if (settled.has(start.id)) continue; + // Walk up until a root, a settled category, or a category already seen on + // this walk: a cycle. + const path: number[] = []; + const seen = new Set(); + let node = start; + let topDepth: number | undefined; + for (;;) { + path.push(node.id); + seen.add(node.id); + const parent = node.parent_id == null ? undefined : byId.get(node.parent_id); + if (parent === undefined) { + topDepth = 0; + break; + } + if (settled.has(parent.id)) { + const above = depths.get(parent.id); + topDepth = above === undefined ? undefined : above + 1; + break; + } + if (seen.has(parent.id)) { + topDepth = undefined; + break; + } + node = parent; + } + // The last category of the path sits at `topDepth`, each one before it a + // level deeper; none has a depth when the walk ended in a cycle. + for (let i = path.length - 1, below = 0; i >= 0; i -= 1, below += 1) { + settled.add(path[i]); + if (topDepth !== undefined) depths.set(path[i], topDepth + below); + } + } + return depths; +} + +/** + * The categories under `rootId`, level by level. Each category is taken once, + * so a cycle ends the walk, and `rootId` itself is never listed. + */ +function levelsBelow(categories: readonly HierarchyNode[], rootId: number): number[][] { + const childrenOf = new Map(); + for (const c of categories) { + if (c.parent_id == null) continue; + const bucket = childrenOf.get(c.parent_id) ?? []; + bucket.push(c.id); + childrenOf.set(c.parent_id, bucket); + } + const levels: number[][] = []; + const seen = new Set([rootId]); + let frontier = [rootId]; + for (;;) { + const next: number[] = []; + for (const id of frontier) { + for (const child of childrenOf.get(id) ?? []) { + if (seen.has(child)) continue; + seen.add(child); + next.push(child); + } + } + if (next.length === 0) return levels; + levels.push(next); + frontier = next; + } +} + +/** Height of the sub-tree under `rootId`: 0 for a leaf, 1 with children, 2 with grandchildren. */ +export function subtreeHeight(categories: readonly HierarchyNode[], rootId: number): number { + return levelsBelow(categories, rootId).length; +} + +/** Every category under `rootId`, at any depth; never `rootId` itself, even inside a cycle. */ +export function descendantsOf(categories: readonly HierarchyNode[], rootId: number): Set { + return new Set(levelsBelow(categories, rootId).flat()); +} + +/** + * Whether a category whose sub-tree is `height` levels deep may sit under a + * parent at `parentDepth`: its deepest descendant must stay within the three + * levels. + */ +export function fitsUnder(parentDepth: number, height: number): boolean { + return parentDepth + 1 + height <= MAX_CATEGORY_DEPTH; +} + +/** Why a parent change is refused (the service adds `parent_has_transactions`). */ +export type HierarchyRefusal = "self_parent" | "cycle" | "too_deep"; + +/** + * Why category `id` may not move under `newParentId`, or `null` when it may. + * `active` are the active categories: depth and descent are counted on them, + * as the tree shows them. The caller has checked that `newParentId` is one of + * them. + * + * - The root (`null`) is always accepted: no place is shallower, and it is the + * repair of last resort for a category caught in a cycle. + * - `self_parent`: the category itself. + * - `cycle`: a category under it (the move would close a loop), or a category + * with no depth, caught in a cycle or under one (the move would hide it). + * - `too_deep`: its deepest descendant would land beyond the third level. + */ +export function parentChangeRefusal( + active: readonly HierarchyNode[], + id: number, + newParentId: number | null, +): HierarchyRefusal | null { + if (newParentId === null) return null; + if (newParentId === id) return "self_parent"; + if (descendantsOf(active, id).has(newParentId)) return "cycle"; + const parentDepth = depthsOf(active).get(newParentId); + if (parentDepth === undefined) return "cycle"; + if (!fitsUnder(parentDepth, subtreeHeight(active, id))) return "too_deep"; + return null; +} + +/** Why an active category is out of the hierarchy (#393). */ +export type OrphanReason = "cycle" | "inactive_parent" | "missing_parent"; + +export interface OrphanCategory { + /** The category to reattach; the categories under it follow it. */ + id: number; + reason: OrphanReason; +} + +/** + * The active categories the tree cannot place, one per group to reattach + * (#393), sorted by id: + * - `inactive_parent`, `missing_parent`: an active category whose parent is + * deactivated, or gone. The tree shows it as a root, out of its place. + * - `cycle`: for each `parent_id` cycle among active categories, its member + * with the smallest id. The cycle and everything under it are out of reach + * from the roots: hidden from the tree, the pickers, the budget and the + * reports. Reattaching that member breaks the cycle; the other members and + * their descendants come back under it. + * Only the head of each group is listed: a category under it follows it. + * + * `rows` are all the categories, inactive ones included: without them, a + * deactivated parent could not be told from a missing one. + */ +export function findOrphans(rows: readonly HierarchyRow[]): OrphanCategory[] { + const byId = new Map(); + for (const r of rows) byId.set(r.id, r); + const orphans: OrphanCategory[] = []; + /** Active categories whose group is known: visible, an orphan, or under one. */ + const settled = new Set(); + + for (const start of rows) { + if (!isActiveRow(start) || settled.has(start.id)) continue; + // Walk up the active parents until a root, a parent that is inactive or + // gone, a settled category, or a category already seen on this walk. + const path: number[] = []; + const seen = new Set(); + let node = start; + for (;;) { + path.push(node.id); + seen.add(node.id); + if (node.parent_id == null) break; + const parent = byId.get(node.parent_id); + if (parent === undefined) { + orphans.push({ id: node.id, reason: "missing_parent" }); + break; + } + if (!isActiveRow(parent)) { + orphans.push({ id: node.id, reason: "inactive_parent" }); + break; + } + if (settled.has(parent.id)) break; + if (seen.has(parent.id)) { + // The cycle runs from the parent to the end of the path; what comes + // before it on the path hangs under the cycle. + const members = path.slice(path.indexOf(parent.id)); + orphans.push({ id: Math.min(...members), reason: "cycle" }); + break; + } + node = parent; + } + for (const id of path) settled.add(id); + } + return orphans.sort((a, b) => a.id - b.id); +} -- 2.45.2 From db5ee65720b7f84243fc1493c30186ad48b00a1b Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 18 Oct 2026 19:16:21 -0400 Subject: [PATCH 2/7] fix: refuse a looping or too deep parent, and reattach orphan categories (#393) updateCategory checks a new parent before writing: the category itself, one of its descendants or a category caught in a cycle (cycle), a fourth level counting its sub-tree (too_deep), a parent holding transactions of its own (parent_has_transactions). The refusal is a CategoryHierarchyError that the Categories page shows by the translation of its code. The reads and the writes, the new parent's is_inputable included, run in one transaction through its handle; an unchanged parent is not checked, so a category stored before these rules can still be renamed. getCategoryDepth keeps the ids it has seen and ends on a cycle. A "Check hierarchy" button, between "All keywords" and "Re-initialize", opens a window that lists the orphan categories (caught in a parent loop, parent deleted, parent gone), one per group, with a new parent for each: the root by default, or a parent of the category form's list. reattachCategory applies the same rules; after it, the list and the tree reload and the Journal gets one line with the reason and the number of categories put back, no name. Co-Authored-By: Claude Opus 5.5 --- .../HierarchyCheckModal.render.test.tsx | 123 ++++++ .../categories/HierarchyCheckModal.tsx | 161 ++++++++ .../HierarchyCheckModal.wiring.test.ts | 137 ++++++ src/hooks/useCategories.test.ts | 214 ++++++++++ src/hooks/useCategories.ts | 142 ++++++- src/i18n/locales/en.json | 22 + src/i18n/locales/fr.json | 22 + src/pages/CategoriesPage.render.test.tsx | 63 ++- src/pages/CategoriesPage.tsx | 29 +- src/services/categoryService.test.ts | 391 ++++++++++++++++++ src/services/categoryService.ts | 181 +++++++- 11 files changed, 1458 insertions(+), 27 deletions(-) create mode 100644 src/components/categories/HierarchyCheckModal.render.test.tsx create mode 100644 src/components/categories/HierarchyCheckModal.tsx create mode 100644 src/components/categories/HierarchyCheckModal.wiring.test.ts create mode 100644 src/hooks/useCategories.test.ts create mode 100644 src/services/categoryService.test.ts diff --git a/src/components/categories/HierarchyCheckModal.render.test.tsx b/src/components/categories/HierarchyCheckModal.render.test.tsx new file mode 100644 index 0000000..c783b7d --- /dev/null +++ b/src/components/categories/HierarchyCheckModal.render.test.tsx @@ -0,0 +1,123 @@ +// HierarchyCheckModal — what the « Vérifier la hiérarchie » window shows (#393). +// No jsdom: react-dom/server renders in vitest's node environment. A closed +// CategoryCombobox renders its input only, without its portal (#429). +import { describe, it, expect, vi } from "vitest"; +import { renderToStaticMarkup } from "react-dom/server"; + +vi.mock("react-i18next", async () => (await import("../../test/i18nMock")).i18nMock()); + +import HierarchyCheckModal from "./HierarchyCheckModal"; +import type { CategoryTreeNode } from "../../shared/types"; +import type { OrphanCategory } from "../../utils/categoryHierarchy"; + +const node = (id: number, parent_id: number | null, name: string, i18n_key: string | null = null): CategoryTreeNode => ({ + id, + name, + parent_id, + color: null, + icon: null, + type: "expense", + is_active: true, + is_inputable: true, + sort_order: id, + keyword_count: 0, + i18n_key, + children: [], +}); + +// 1 is a root; 3 <-> 5 is a cycle; 20's parent is gone. +const CATEGORIES = [ + node(1, null, "Logement", "categoriesSeed.housing"), + node(3, 5, "Épicerie"), + node(5, 3, "Alimentation"), + node(20, 99, "Cadeaux"), +]; +const ORPHANS: OrphanCategory[] = [ + { id: 3, reason: "cycle" }, + { id: 20, reason: "missing_parent" }, +]; + +const render = (props: { orphans: OrphanCategory[] | null; error?: string | null; isSaving?: boolean }) => + renderToStaticMarkup( + , + ); + +const comboboxes = (html: string) => html.match(/]*role="combobox"[^>]*>/g) ?? []; + +describe("HierarchyCheckModal (#393)", () => { + it("is a modal dialog named after the button, with its explanation and a close button", () => { + const html = render({ orphans: [] }); + expect(html).toContain('role="dialog"'); + expect(html).toContain('aria-modal="true"'); + expect(html).toContain('aria-label="t(categories.hierarchy.check)"'); + expect(html).toContain("t(categories.hierarchy.intro)"); + expect(html).toContain("t(common.close)"); + }); + + it("says the check runs while the orphans are not known", () => { + const html = render({ orphans: null }); + expect(html).toContain("t(categories.hierarchy.checking)"); + expect(html).not.toContain("t(categories.hierarchy.none)"); + expect(comboboxes(html)).toHaveLength(0); + }); + + it("says « aucune » for a sound hierarchy, with no row", () => { + const html = render({ orphans: [] }); + expect(html).toContain("t(categories.hierarchy.none)"); + expect(html).not.toContain("categories.hierarchy.count"); + expect(html).not.toContain("t(categories.hierarchy.reattach)"); + }); + + it("counts the orphans, and gives each row its name, its reason, a new parent and « Rattacher »", () => { + const html = render({ orphans: ORPHANS }); + expect(html).toContain("t(categories.hierarchy.count[count=2])"); + expect(html).toContain(">Épicerie

"); + expect(html).toContain(">Cadeaux

"); + expect(html).toContain("t(categories.hierarchy.reasons.cycle)"); + expect(html).toContain("t(categories.hierarchy.reasons.missing_parent)"); + expect(html.split("t(categories.hierarchy.reattach)")).toHaveLength(3); + const inputs = comboboxes(html); + expect(inputs).toHaveLength(2); + expect(inputs[0]).toContain('aria-label="t(categories.hierarchy.newParentFor[name=Épicerie])"'); + expect(inputs[1]).toContain('aria-label="t(categories.hierarchy.newParentFor[name=Cadeaux])"'); + }); + + it("preselects the root on every row", () => { + for (const input of comboboxes(render({ orphans: ORPHANS }))) { + expect(input).toContain('value="t(categories.hierarchy.root)"'); + } + }); + + it("names a seed category in the app's language", () => { + const html = render({ orphans: [{ id: 1, reason: "inactive_parent" }] }); + expect(html).toContain(">t(categoriesSeed.housing[defaultValue=Logement])

"); + expect(html).toContain("t(categories.hierarchy.reasons.inactive_parent)"); + }); + + it("shows the translated error above the list, as an alert", () => { + const html = render({ orphans: ORPHANS, error: "Ce parent a des transactions" }); + expect(html).toContain('role="alert"'); + expect(html.indexOf("Ce parent a des transactions")).toBeLessThan(html.indexOf("categories.hierarchy.count")); + }); + + it("shows a failed check alone, without the « checking » line", () => { + const html = render({ orphans: null, error: "database is locked" }); + expect(html).toContain("database is locked"); + expect(html).not.toContain("t(categories.hierarchy.checking)"); + }); + + it("disables « Rattacher » while a reattachment runs", () => { + const html = render({ orphans: ORPHANS, isSaving: true }); + const buttons = html.match(/]*>t\(categories\.hierarchy\.reattach\)<\/button>/g) ?? []; + expect(buttons).toHaveLength(2); + for (const button of buttons) expect(button).toContain('disabled=""'); + expect(render({ orphans: ORPHANS })).not.toContain('disabled=""'); + }); +}); diff --git a/src/components/categories/HierarchyCheckModal.tsx b/src/components/categories/HierarchyCheckModal.tsx new file mode 100644 index 0000000..0f9c1fb --- /dev/null +++ b/src/components/categories/HierarchyCheckModal.tsx @@ -0,0 +1,161 @@ +import { useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { ListTree } from "lucide-react"; +import type { CategoryTreeNode } from "../../shared/types"; +import type { OrphanCategory } from "../../utils/categoryHierarchy"; +import CategoryCombobox from "../shared/CategoryCombobox"; +import { buildParentOptions } from "./parentOptions"; + +interface Props { + /** The orphan categories, one per group to reattach; `null` while the check runs. */ + orphans: OrphanCategory[] | null; + /** The translated text of a refused reattachment, or of a check that failed. */ + error: string | null; + /** The active categories, as the tree has them: names and parent choices. */ + categories: CategoryTreeNode[]; + isSaving: boolean; + onReattach: (orphan: OrphanCategory, parentId: number | null) => void; + onClose: () => void; +} + +/** The value of the « Racine » option of each row's combobox. */ +const ROOT = ""; + +/** + * « Vérifier la hiérarchie » (#393): the orphan categories of the profile, one + * row per group to reattach, each with its reason and a new parent. The root + * is preselected; the other choices are those of the category form + * (`buildParentOptions`, the row's category treated as the one being edited). + * The service applies the rules again and may refuse (a parent that holds + * transactions, for one): the translated refusal shows above the list. + * + * The comboboxes open their list in a portal (#429), so the scrolling list of + * rows never cuts it. + */ +export default function HierarchyCheckModal({ + orphans, + error, + categories, + isSaving, + onReattach, + onClose, +}: Props) { + const { t } = useTranslation(); + /** The parent picked on each row, by orphan id; no entry means the root. */ + const [choices, setChoices] = useState>({}); + + const optionsByOrphan = useMemo(() => { + const byOrphan = new Map(); + for (const orphan of orphans ?? []) { + byOrphan.set(orphan.id, buildParentOptions(categories, { editingId: orphan.id })); + } + return byOrphan; + }, [orphans, categories]); + + const rootOption = useMemo(() => [{ value: ROOT, label: t("categories.hierarchy.root") }], [t]); + + /** The row's parent: the one picked if it is still offered, else the root. */ + const choiceOf = (orphanId: number): number | null => { + const picked = choices[orphanId]; + if (picked === undefined) return null; + return optionsByOrphan.get(orphanId)?.some((c) => c.id === picked) ? picked : null; + }; + + const choose = (orphanId: number, parentId: number | null) => { + setChoices((previous) => { + const next = { ...previous }; + if (parentId === null) delete next[orphanId]; + else next[orphanId] = parentId; + return next; + }); + }; + + const nameOf = (id: number): string => { + const category = categories.find((c) => c.id === id); + if (!category) return "—"; + return category.i18n_key ? t(category.i18n_key, { defaultValue: category.name }) : category.name; + }; + + return ( +
+
+
+
+ +
+

{t("categories.hierarchy.check")}

+
+

{t("categories.hierarchy.intro")}

+ + {error && ( +
+ {error} +
+ )} + + {orphans === null ? ( + error ? null : ( +

{t("categories.hierarchy.checking")}

+ ) + ) : orphans.length === 0 ? ( +

{t("categories.hierarchy.none")}

+ ) : ( + <> +

{t("categories.hierarchy.count", { count: orphans.length })}

+
    + {orphans.map((orphan) => { + const name = nameOf(orphan.id); + const choice = choiceOf(orphan.id); + return ( +
  • +
    +

    {name}

    +

    + {t(`categories.hierarchy.reasons.${orphan.reason}`)} +

    +
    +
    + choose(orphan.id, id)} + extraOptions={rootOption} + activeExtra={choice === null ? ROOT : null} + onExtraSelect={() => choose(orphan.id, null)} + ariaLabel={t("categories.hierarchy.newParentFor", { name })} + compact + /> +
    + +
    • + ); + })} +
+ + )} + +
+ +
+
+
+ ); +} diff --git a/src/components/categories/HierarchyCheckModal.wiring.test.ts b/src/components/categories/HierarchyCheckModal.wiring.test.ts new file mode 100644 index 0000000..6c087a1 --- /dev/null +++ b/src/components/categories/HierarchyCheckModal.wiring.test.ts @@ -0,0 +1,137 @@ +// HierarchyCheckModal — what a row hands to « Rattacher » (#393). +// +// A static render clicks nothing. React's hooks come from the shared harness +// (src/test/hookHarness.ts, #409): the window runs as a plain function, its +// state survives from one call to the next, the test calls the props it hands +// to each CategoryCombobox and to its buttons, then renders again. +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { isValidElement, type ReactElement, type ReactNode } from "react"; + +vi.mock("react", async (orig) => (await import("../../test/hookHarness")).fakeReact(await orig())); +vi.mock("react-i18next", async () => (await import("../../test/i18nMock")).i18nMock()); +// Pass-through spy on the pure module, to pin what each row offers. +vi.mock("./parentOptions", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, buildParentOptions: vi.fn(actual.buildParentOptions) }; +}); + +import { hookHarness } from "../../test/hookHarness"; +import HierarchyCheckModal from "./HierarchyCheckModal"; +import CategoryCombobox from "../shared/CategoryCombobox"; +import { buildParentOptions } from "./parentOptions"; +import type { CategoryTreeNode } from "../../shared/types"; +import type { OrphanCategory } from "../../utils/categoryHierarchy"; + +const harness = hookHarness(); + +type Props = Record & { children?: ReactNode }; + +function find(node: ReactNode, match: (el: ReactElement) => boolean): ReactElement[] { + if (Array.isArray(node)) return node.flatMap((n) => find(n, match)); + if (!isValidElement(node)) return []; + const el = node as ReactElement; + return [...(match(el) ? [el] : []), ...find(el.props.children, match)]; +} + +const node = (id: number, parent_id: number | null): CategoryTreeNode => ({ + id, + name: `c${id}`, + parent_id, + color: null, + icon: null, + type: "expense", + is_active: true, + is_inputable: true, + sort_order: id, + keyword_count: 0, + i18n_key: null, + children: [], +}); + +// Roots 1 and 2 (2 has a child, 21); 3 <-> 5 is a cycle; 20's parent is gone. +const CATEGORIES = [node(1, null), node(2, null), node(21, 2), node(3, 5), node(5, 3), node(20, 99)]; +const CYCLE: OrphanCategory = { id: 3, reason: "cycle" }; +const GONE: OrphanCategory = { id: 20, reason: "missing_parent" }; + +const onReattach = vi.fn(); +const onClose = vi.fn(); + +const view = (orphans: OrphanCategory[] | null = [CYCLE, GONE], categories = CATEGORIES) => { + const tree = harness.render(() => + HierarchyCheckModal({ orphans, error: null, categories, isSaving: false, onReattach, onClose }), + ); + return { + comboboxes: find(tree, (el) => el.type === CategoryCombobox).map((el) => el.props), + reattach: find(tree, (el) => el.type === "button" && el.props.children === "t(categories.hierarchy.reattach)"), + close: find(tree, (el) => el.type === "button" && el.props.children === "t(common.close)"), + }; +}; + +const ids = (list: unknown) => (list as CategoryTreeNode[]).map((c) => c.id).sort((a, b) => a - b); + +/** Calls the `onClick` the window hands to a button. */ +const click = (button: ReactElement) => (button.props.onClick as () => void)(); + +beforeEach(() => { + harness.clear(); + onReattach.mockReset(); + onClose.mockReset(); + vi.mocked(buildParentOptions).mockClear(); +}); + +describe("HierarchyCheckModal — a row's new parent (#393)", () => { + it("offers the category form's parents, the row's category treated as the one edited, the root first", () => { + const { comboboxes } = view(); + expect(buildParentOptions).toHaveBeenCalledWith(CATEGORIES, { editingId: 3 }); + expect(buildParentOptions).toHaveBeenCalledWith(CATEGORIES, { editingId: 20 }); + // The cycle member carries the other member (5): roots only, its own cycle left out. + // 20, whose parent is gone, shows as a root in the tree, and is offered as one. + expect(ids(comboboxes[0].categories)).toEqual([1, 2, 20]); + // A leaf: roots and level-1 categories, never a cycle member nor itself. + expect(ids(comboboxes[1].categories)).toEqual([1, 2, 21]); + for (const props of comboboxes) { + expect(props.extraOptions).toEqual([{ value: "", label: "t(categories.hierarchy.root)" }]); + expect(props.value).toBeNull(); + expect(props.activeExtra).toBe(""); + } + }); + + it("« Rattacher » hands the root by default", () => { + click(view().reattach[1]); + expect(onReattach).toHaveBeenCalledWith(GONE, null); + }); + + it("hands the parent picked on the row, and the root again once it is picked back", () => { + (view().comboboxes[1].onChange as (id: number) => void)(21); + let current = view(); + expect(current.comboboxes[1].value).toBe(21); + expect(current.comboboxes[1].activeExtra).toBeNull(); + // The other row keeps the root. + expect(current.comboboxes[0].value).toBeNull(); + click(current.reattach[1]); + expect(onReattach).toHaveBeenLastCalledWith(GONE, 21); + + (current.comboboxes[1].onExtraSelect as (value: string) => void)(""); + current = view(); + expect(current.comboboxes[1].value).toBeNull(); + expect(current.comboboxes[1].activeExtra).toBe(""); + click(current.reattach[1]); + expect(onReattach).toHaveBeenLastCalledWith(GONE, null); + }); + + it("falls back to the root when the parent picked is no longer offered", () => { + (view().comboboxes[1].onChange as (id: number) => void)(21); + // After a reload, 21 is gone from the categories. + const current = view([CYCLE, GONE], CATEGORIES.filter((c) => c.id !== 21)); + expect(current.comboboxes[1].value).toBeNull(); + click(current.reattach[1]); + expect(onReattach).toHaveBeenCalledWith(GONE, null); + }); + + it("« Fermer » closes the window", () => { + const { close } = view(); + expect(close).toHaveLength(1); + click(close[0]); + expect(onClose).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/hooks/useCategories.test.ts b/src/hooks/useCategories.test.ts new file mode 100644 index 0000000..33668e4 --- /dev/null +++ b/src/hooks/useCategories.test.ts @@ -0,0 +1,214 @@ +// useCategories — the hierarchy check and its refusals (#393). +// +// React's hooks come from the shared harness (src/test/hookHarness.ts, #409): +// the hook runs as a plain function, its reducer is applied, `commit()` runs +// its mount effect. The service is mocked, except the refusal class and its +// i18n key, which the hook must recognize by `instanceof`. +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("react", async (orig) => (await import("../test/hookHarness")).fakeReact(await orig())); +vi.mock("../services/categoryService", async (importOriginal) => { + const actual = await importOriginal(); + return { + CategoryHierarchyError: actual.CategoryHierarchyError, + categoryHierarchyErrorKey: actual.categoryHierarchyErrorKey, + getAllCategoriesWithCounts: vi.fn(), + createCategory: vi.fn(), + updateCategory: vi.fn(), + deactivateCategory: vi.fn(), + getCategoryUsageCount: vi.fn(), + getChildrenUsageCount: vi.fn(), + getKeywordsByCategoryId: vi.fn(async () => []), + createKeyword: vi.fn(), + updateKeyword: vi.fn(), + deactivateKeyword: vi.fn(), + reinitializeCategories: vi.fn(), + hasDuplicateSortOrders: vi.fn(async () => false), + fixDuplicateSortOrders: vi.fn(), + getNextSortOrder: vi.fn(), + updateCategorySortOrders: vi.fn(), + findOrphanCategories: vi.fn(), + reattachCategory: vi.fn(), + }; +}); +vi.mock("../services/logService", () => ({ logInfo: vi.fn(), logWarn: vi.fn(), logError: vi.fn() })); + +import { hookHarness } from "../test/hookHarness"; +import { + useCategories, + categoriesErrorOf, + categoriesErrorText, + reattachLogLine, +} from "./useCategories"; +import { + CategoryHierarchyError, + findOrphanCategories, + getAllCategoriesWithCounts, + reattachCategory, + updateCategory, +} from "../services/categoryService"; +import { logInfo } from "../services/logService"; +import { markedT } from "../test/i18nMock"; +import type { OrphanCategory } from "../utils/categoryHierarchy"; + +const harness = hookHarness(); + +const ROWS = [ + { id: 1, name: "Logement", parent_id: null, color: null, icon: null, type: "expense", is_active: true, is_inputable: false, sort_order: 1, keyword_count: 0, i18n_key: null }, + { id: 10, name: "Loyer", parent_id: 1, color: null, icon: null, type: "expense", is_active: true, is_inputable: true, sort_order: 1, keyword_count: 0, i18n_key: null }, +]; + +const CYCLE: OrphanCategory = { id: 3, reason: "cycle" }; + +/** Lets the hook's promises settle. */ +const flush = async () => { + for (let i = 0; i < 20; i++) await Promise.resolve(); +}; + +/** Mounts the hook and lets its first load settle. */ +async function mount() { + harness.render(useCategories); + harness.commit(); + await flush(); + return harness.render(useCategories); +} + +const FORM = { name: "Logement", type: "expense" as const, color: "#4A90A4", parent_id: 10, is_inputable: false, sort_order: 1 }; + +beforeEach(() => { + harness.clear(); + vi.mocked(getAllCategoriesWithCounts).mockReset(); + vi.mocked(getAllCategoriesWithCounts).mockResolvedValue(ROWS as never); + vi.mocked(updateCategory).mockReset(); + vi.mocked(findOrphanCategories).mockReset(); + vi.mocked(reattachCategory).mockReset(); + vi.mocked(logInfo).mockReset(); +}); + +describe("categoriesErrorOf / categoriesErrorText (#393)", () => { + it("names a hierarchy refusal by the i18n key of its code, never by its message", () => { + const error = categoriesErrorOf(new CategoryHierarchyError("cycle", "raw text")); + expect(error).toEqual({ key: "categories.hierarchy.errors.cycle" }); + expect(categoriesErrorText(error, markedT)).toBe("t(categories.hierarchy.errors.cycle)"); + }); + + it("keeps the message of any other failure", () => { + const error = categoriesErrorOf(new Error("database is locked")); + expect(error).toEqual({ message: "database is locked" }); + expect(categoriesErrorText(error, markedT)).toBe("database is locked"); + expect(categoriesErrorOf("boom")).toEqual({ message: "boom" }); + }); +}); + +describe("useCategories — saving the form (#393)", () => { + it("shows a refused parent by its translated code, and keeps the form open", async () => { + vi.mocked(updateCategory).mockRejectedValue(new CategoryHierarchyError("cycle")); + let api = await mount(); + await api.selectCategory(1); + api = harness.render(useCategories); + api.startEditing(); + api = harness.render(useCategories); + await api.saveCategory(FORM); + const { state } = harness.render(useCategories); + expect(updateCategory).toHaveBeenCalledWith(1, FORM); + expect(state.error).toEqual({ key: "categories.hierarchy.errors.cycle" }); + expect(state.isSaving).toBe(false); + expect(state.editingCategory).not.toBeNull(); + }); + + it("shows any other failure by its message", async () => { + vi.mocked(updateCategory).mockRejectedValue(new Error("database is locked")); + let api = await mount(); + await api.selectCategory(1); + api = harness.render(useCategories); + await api.saveCategory(FORM); + expect(harness.render(useCategories).state.error).toEqual({ message: "database is locked" }); + }); +}); + +describe("useCategories — « Vérifier la hiérarchie » (#393)", () => { + it("opens the window while the check runs, then lists the orphans", async () => { + let resolve: (orphans: OrphanCategory[]) => void = () => undefined; + vi.mocked(findOrphanCategories).mockReturnValue(new Promise((r) => (resolve = r))); + const api = await mount(); + expect(api.state.hierarchyCheck).toBeNull(); + const done = api.checkHierarchy(); + expect(harness.render(useCategories).state.hierarchyCheck).toEqual({ orphans: null, error: null }); + resolve([CYCLE]); + await done; + expect(harness.render(useCategories).state.hierarchyCheck).toEqual({ orphans: [CYCLE], error: null }); + }); + + it("shows a check that failed in the window", async () => { + vi.mocked(findOrphanCategories).mockRejectedValue(new Error("database is locked")); + const api = await mount(); + await api.checkHierarchy(); + expect(harness.render(useCategories).state.hierarchyCheck).toEqual({ + orphans: null, + error: { message: "database is locked" }, + }); + }); + + it("reattaches, writes one Journal line without any name, then reloads the list and the tree", async () => { + vi.mocked(findOrphanCategories).mockResolvedValueOnce([CYCLE]).mockResolvedValueOnce([]); + vi.mocked(reattachCategory).mockResolvedValue(3); + let api = await mount(); + await api.checkHierarchy(); + api = harness.render(useCategories); + expect(getAllCategoriesWithCounts).toHaveBeenCalledTimes(1); + + await api.reattachOrphan(CYCLE, 1); + + expect(reattachCategory).toHaveBeenCalledWith(3, 1); + expect(logInfo).toHaveBeenCalledTimes(1); + const line = vi.mocked(logInfo).mock.calls[0][0] as string; + expect(line).toBe(reattachLogLine("cycle", 3)); + expect(line).toContain("3 categories"); + for (const row of ROWS) expect(line).not.toContain(row.name); + expect(findOrphanCategories).toHaveBeenCalledTimes(2); + expect(getAllCategoriesWithCounts).toHaveBeenCalledTimes(2); + const { state } = harness.render(useCategories); + expect(state.hierarchyCheck).toEqual({ orphans: [], error: null }); + expect(state.isSaving).toBe(false); + }); + + it("shows a refused reattachment in the window by its translated code, with no Journal line", async () => { + vi.mocked(findOrphanCategories).mockResolvedValue([CYCLE]); + vi.mocked(reattachCategory).mockRejectedValue(new CategoryHierarchyError("parent_has_transactions")); + let api = await mount(); + await api.checkHierarchy(); + api = harness.render(useCategories); + await api.reattachOrphan(CYCLE, 1); + const { state } = harness.render(useCategories); + expect(state.hierarchyCheck).toEqual({ + orphans: [CYCLE], + error: { key: "categories.hierarchy.errors.parent_has_transactions" }, + }); + expect(state.error).toBeNull(); + expect(state.isSaving).toBe(false); + expect(logInfo).not.toHaveBeenCalled(); + }); + + it("closes the window; a check that ends after it closed changes nothing", async () => { + let resolve: (orphans: OrphanCategory[]) => void = () => undefined; + vi.mocked(findOrphanCategories).mockReturnValue(new Promise((r) => (resolve = r))); + let api = await mount(); + const done = api.checkHierarchy(); + api = harness.render(useCategories); + api.closeHierarchyCheck(); + resolve([CYCLE]); + await done; + expect(harness.render(useCategories).state.hierarchyCheck).toBeNull(); + }); +}); + +describe("reattachLogLine (#393)", () => { + it("gives the reason code and the count, singular or plural", () => { + expect(reattachLogLine("inactive_parent", 1)).toBe( + "Categories: orphan category reattached (inactive_parent), 1 category back in the hierarchy", + ); + expect(reattachLogLine("cycle", 4)).toBe( + "Categories: orphan category reattached (cycle), 4 categories back in the hierarchy", + ); + }); +}); diff --git a/src/hooks/useCategories.ts b/src/hooks/useCategories.ts index 7217d47..f4c8f6c 100644 --- a/src/hooks/useCategories.ts +++ b/src/hooks/useCategories.ts @@ -20,7 +20,37 @@ import { fixDuplicateSortOrders, getNextSortOrder, updateCategorySortOrders, + findOrphanCategories, + reattachCategory, + CategoryHierarchyError, + categoryHierarchyErrorKey, } from "../services/categoryService"; +import { logInfo } from "../services/logService"; +import type { OrphanCategory } from "../utils/categoryHierarchy"; + +/** + * What a failed action shows: a refusal of the hierarchy rules by the i18n key + * of its code (#393), any other failure by its message. + */ +export type CategoriesError = { key: string } | { message: string }; + +export function categoriesErrorOf(e: unknown): CategoriesError { + if (e instanceof CategoryHierarchyError) return { key: categoryHierarchyErrorKey(e.code) }; + return { message: e instanceof Error ? e.message : String(e) }; +} + +/** The text of a categories error: the translation of its key, or its message. */ +export function categoriesErrorText(error: CategoriesError, t: (key: string) => string): string { + return "key" in error ? t(error.key) : error.message; +} + +/** The « Vérifier la hiérarchie » window (#393), while it is open. */ +export interface HierarchyCheckState { + /** The orphan categories, one per group to reattach; `null` while the check runs. */ + orphans: OrphanCategory[] | null; + /** A refused reattachment, or a check that failed. */ + error: CategoriesError | null; +} interface CategoriesState { categories: CategoryTreeNode[]; @@ -29,22 +59,29 @@ interface CategoriesState { keywords: Keyword[]; isLoading: boolean; isSaving: boolean; - error: string | null; + error: CategoriesError | null; editingCategory: CategoryFormData | null; isCreating: boolean; + /** `null` while the window is closed. */ + hierarchyCheck: HierarchyCheckState | null; } type CategoriesAction = | { type: "SET_LOADING"; payload: boolean } | { type: "SET_SAVING"; payload: boolean } - | { type: "SET_ERROR"; payload: string | null } + | { type: "SET_ERROR"; payload: CategoriesError | null } | { type: "SET_CATEGORIES"; payload: { flat: CategoryTreeNode[]; tree: CategoryTreeNode[] } } | { type: "SET_TREE"; payload: CategoryTreeNode[] } | { type: "SELECT_CATEGORY"; payload: number | null } | { type: "SET_KEYWORDS"; payload: Keyword[] } | { type: "START_CREATING" } | { type: "START_EDITING"; payload: CategoryFormData } - | { type: "CANCEL_EDITING" }; + | { type: "CANCEL_EDITING" } + | { type: "OPEN_HIERARCHY_CHECK" } + | { type: "SET_ORPHANS"; payload: OrphanCategory[] } + | { type: "START_REATTACH" } + | { type: "SET_HIERARCHY_ERROR"; payload: CategoriesError } + | { type: "CLOSE_HIERARCHY_CHECK" }; const initialState: CategoriesState = { categories: [], @@ -56,9 +93,16 @@ const initialState: CategoriesState = { error: null, editingCategory: null, isCreating: false, + hierarchyCheck: null, }; -function buildTree(flat: CategoryTreeNode[]): CategoryTreeNode[] { +/** + * The tree of the Categories page: each category under its parent, a category + * whose parent is not in the list at the root. A category caught in a + * `parent_id` cycle is under a parent and never at the root, so no root leads + * to it: « Vérifier la hiérarchie » finds it (#393). + */ +export function buildTree(flat: CategoryTreeNode[]): CategoryTreeNode[] { const map = new Map(); const roots: CategoryTreeNode[] = []; @@ -119,11 +163,43 @@ function reducer(state: CategoriesState, action: CategoriesAction): CategoriesSt return { ...state, isCreating: false, editingCategory: action.payload }; case "CANCEL_EDITING": return { ...state, editingCategory: null, isCreating: false }; + case "OPEN_HIERARCHY_CHECK": + return { ...state, hierarchyCheck: { orphans: null, error: null } }; + case "SET_ORPHANS": + // A check that ends after the window closed changes nothing. + return state.hierarchyCheck + ? { ...state, hierarchyCheck: { ...state.hierarchyCheck, orphans: action.payload } } + : state; + case "START_REATTACH": + return { + ...state, + isSaving: true, + hierarchyCheck: state.hierarchyCheck ? { ...state.hierarchyCheck, error: null } : null, + }; + case "SET_HIERARCHY_ERROR": + return { + ...state, + isSaving: false, + hierarchyCheck: state.hierarchyCheck ? { ...state.hierarchyCheck, error: action.payload } : null, + }; + case "CLOSE_HIERARCHY_CHECK": + return { ...state, hierarchyCheck: null }; default: return state; } } +/** + * The Journal line of a reattachment (#393): the reason code and the number of + * categories put back in place, never a category name. + */ +export function reattachLogLine(reason: OrphanCategory["reason"], restored: number): string { + return ( + `Categories: orphan category reattached (${reason}), ` + + `${restored} ${restored === 1 ? "category" : "categories"} back in the hierarchy` + ); +} + export function useCategories() { const [state, dispatch] = useReducer(reducer, initialState); const fetchIdRef = useRef(0); @@ -150,7 +226,7 @@ export function useCategories() { dispatch({ type: "SET_CATEGORIES", payload: { flat, tree } }); } catch (e) { if (fetchId !== fetchIdRef.current) return; - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, []); @@ -165,7 +241,7 @@ export function useCategories() { const kws = await getKeywordsByCategoryId(id); dispatch({ type: "SET_KEYWORDS", payload: kws }); } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } } }, []); @@ -212,7 +288,7 @@ export function useCategories() { } dispatch({ type: "SET_SAVING", payload: false }); } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, [state.isCreating, state.selectedCategoryId, loadCategories, selectCategory] @@ -237,7 +313,7 @@ export function useCategories() { dispatch({ type: "SET_SAVING", payload: false }); return { blocked: false, count: 0 }; } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); return { blocked: false, count: 0 }; } }, @@ -253,7 +329,7 @@ export function useCategories() { await loadCategories(); dispatch({ type: "SET_SAVING", payload: false }); } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, [loadCategories]); @@ -327,12 +403,49 @@ export function useCategories() { [state.tree, loadCategories] ); + /** Opens the « Vérifier la hiérarchie » window and runs the diagnostic (#393). */ + const checkHierarchy = useCallback(async () => { + dispatch({ type: "OPEN_HIERARCHY_CHECK" }); + try { + const orphans = await findOrphanCategories(); + dispatch({ type: "SET_ORPHANS", payload: orphans }); + } catch (e) { + dispatch({ type: "SET_HIERARCHY_ERROR", payload: categoriesErrorOf(e) }); + } + }, []); + + /** + * Reattaches an orphan under `parentId`, or to the root with `null` (#393). + * On success: one Journal line (counts, no name), then the orphan list and + * the tree reload. A refusal shows in the window, by its translated code. + */ + const reattachOrphan = useCallback( + async (orphan: OrphanCategory, parentId: number | null) => { + dispatch({ type: "START_REATTACH" }); + try { + const restored = await reattachCategory(orphan.id, parentId); + logInfo(reattachLogLine(orphan.reason, restored)); + const orphans = await findOrphanCategories(); + dispatch({ type: "SET_ORPHANS", payload: orphans }); + await loadCategories(); + dispatch({ type: "SET_SAVING", payload: false }); + } catch (e) { + dispatch({ type: "SET_HIERARCHY_ERROR", payload: categoriesErrorOf(e) }); + } + }, + [loadCategories] + ); + + const closeHierarchyCheck = useCallback(() => { + dispatch({ type: "CLOSE_HIERARCHY_CHECK" }); + }, []); + const loadKeywords = useCallback(async (categoryId: number) => { try { const kws = await getKeywordsByCategoryId(categoryId); dispatch({ type: "SET_KEYWORDS", payload: kws }); } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, []); @@ -344,7 +457,7 @@ export function useCategories() { await loadKeywords(state.selectedCategoryId); await loadCategories(); } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, [state.selectedCategoryId, loadKeywords, loadCategories] @@ -358,7 +471,7 @@ export function useCategories() { await loadKeywords(state.selectedCategoryId); } } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, [state.selectedCategoryId, loadKeywords] @@ -373,7 +486,7 @@ export function useCategories() { await loadCategories(); } } catch (e) { - dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e) }); + dispatch({ type: "SET_ERROR", payload: categoriesErrorOf(e) }); } }, [state.selectedCategoryId, loadKeywords, loadCategories] @@ -392,5 +505,8 @@ export function useCategories() { removeKeyword, reinitializeCategories, moveCategory, + checkHierarchy, + reattachOrphan, + closeHierarchyCheck, }; } diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index e3f9b6f..e31927a 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -442,6 +442,28 @@ "customColor": "Custom color", "allKeywords": "All Keywords", "allKeywordsEmpty": "No keywords yet", + "hierarchy": { + "check": "Check hierarchy", + "intro": "An orphan category is no longer in its place in the hierarchy: it is caught in a parent loop (a category placed under its own subcategory), which hides it along with its subcategories, or its parent was deleted or cannot be found. Reattach it to the root or to another parent: it comes back in the tree, the category menus, the budget and the reports, with its transactions, keywords and budget amounts.", + "checking": "Checking…", + "none": "No orphan categories: every category is in its place in the hierarchy.", + "count_one": "{{count}} orphan category", + "count_other": "{{count}} orphan categories", + "reasons": { + "cycle": "Caught in a parent loop", + "inactive_parent": "Its parent was deleted", + "missing_parent": "Its parent cannot be found" + }, + "newParentFor": "New parent for {{name}}", + "root": "Root (top level)", + "reattach": "Reattach", + "errors": { + "self_parent": "A category cannot be its own parent.", + "cycle": "This parent would create a parent loop: it sits under this category, or inside a loop. Choose another parent.", + "too_deep": "With this parent, the category or its subcategories would go beyond three levels. Choose a parent higher in the hierarchy.", + "parent_has_transactions": "This parent has transactions of its own: once a parent, it would no longer take input. Choose another parent, or recategorize its transactions first." + } + }, "help": { "title": "How to manage Categories", "tips": [ diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index 0ff9f8d..b106798 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -442,6 +442,28 @@ "customColor": "Couleur personnalisée", "allKeywords": "Tous les mots-clés", "allKeywordsEmpty": "Aucun mot-clé", + "hierarchy": { + "check": "Vérifier la hiérarchie", + "intro": "Une catégorie orpheline n'est plus à sa place dans la hiérarchie : elle est prise dans une boucle de parenté (une catégorie placée sous sa propre sous-catégorie), qui la cache avec ses sous-catégories, ou son parent a été supprimé ou est introuvable. Rattachez-la à la racine ou à un autre parent : elle revient dans l'arbre, les menus de catégories, le budget et les rapports, avec ses transactions, ses mots-clés et ses montants de budget.", + "checking": "Vérification en cours…", + "none": "Aucune catégorie orpheline : chaque catégorie est à sa place dans la hiérarchie.", + "count_one": "{{count}} catégorie orpheline", + "count_other": "{{count}} catégories orphelines", + "reasons": { + "cycle": "Prise dans une boucle de parenté", + "inactive_parent": "Son parent a été supprimé", + "missing_parent": "Son parent est introuvable" + }, + "newParentFor": "Nouveau parent de {{name}}", + "root": "Racine (niveau supérieur)", + "reattach": "Rattacher", + "errors": { + "self_parent": "Une catégorie ne peut pas être son propre parent.", + "cycle": "Ce parent créerait une boucle de parenté : il se trouve sous cette catégorie, ou dans une boucle. Choisissez un autre parent.", + "too_deep": "Avec ce parent, la catégorie ou ses sous-catégories dépasseraient trois niveaux. Choisissez un parent plus haut dans la hiérarchie.", + "parent_has_transactions": "Ce parent a des transactions à son nom : devenu parent, il ne serait plus saisissable. Choisissez un autre parent, ou reclassez d'abord ses transactions." + } + }, "help": { "title": "Comment gérer les Catégories", "tips": [ diff --git a/src/pages/CategoriesPage.render.test.tsx b/src/pages/CategoriesPage.render.test.tsx index 595b876..80df5bd 100644 --- a/src/pages/CategoriesPage.render.test.tsx +++ b/src/pages/CategoriesPage.render.test.tsx @@ -12,10 +12,16 @@ vi.mock("../services/db", () => ({ getDb: vi.fn(), withTransaction: vi.fn() })); vi.mock("react-i18next", async () => (await import("../test/i18nMock")).i18nMock()); vi.mock("../components/shared/PageHelp", () => ({ PageHelp: () => null })); -const harness = vi.hoisted(() => ({ onboarding: null as unknown })); +const harness = vi.hoisted(() => ({ + onboarding: null as unknown, + /** Overrides of the categories state, for the tests of #393. */ + state: {} as Record, +})); vi.mock("../contexts/OnboardingContext", () => ({ useOnboarding: () => harness.onboarding })); -vi.mock("../hooks/useCategories", () => ({ +vi.mock("../hooks/useCategories", async (importOriginal) => ({ + // The real error text helper: the page must translate a refusal's key. + categoriesErrorText: (await importOriginal()).categoriesErrorText, useCategories: () => ({ state: { categories: [], @@ -27,6 +33,8 @@ vi.mock("../hooks/useCategories", () => ({ isLoading: true, isSaving: false, error: null, + hierarchyCheck: null, + ...harness.state, }, }), })); @@ -88,6 +96,7 @@ const sentence = (html: string) => html.match(/<\/h2>

]*>(.*?)<\/p>/)?.[1] beforeEach(() => { harness.onboarding = path([]); + harness.state = {}; }); describe("CategoriesPage — the step card (#432)", () => { @@ -150,3 +159,53 @@ describe("CategoriesPage — the step card (#432)", () => { expect(value.skip).not.toHaveBeenCalled(); }); }); + +describe("CategoriesPage — « Vérifier la hiérarchie » (#393)", () => { + /** The opening tag of the header button whose label closes right after it. */ + const buttonTag = (html: string, label: string) => { + const end = html.indexOf(`${label}`); + expect(end).toBeGreaterThan(-1); + const start = html.lastIndexOf("", start) + 1); + }; + + it("sits between « All keywords » and « Reset », styled as « Reset »", () => { + const html = render(); + const allKeywords = html.indexOf("t(categories.allKeywords)"); + const check = html.indexOf("t(categories.hierarchy.check)"); + const reset = html.indexOf("t(categories.reinitialize)"); + expect(allKeywords).toBeLessThan(check); + expect(check).toBeLessThan(reset); + const classOf = (tag: string) => tag.match(/class="([^"]*)"/)?.[1]; + expect(classOf(buttonTag(html, "t(categories.hierarchy.check)"))).toBe( + classOf(buttonTag(html, "t(categories.reinitialize)")), + ); + }); + + it("is disabled while a save runs, as « Reset »", () => { + harness.state = { isSaving: true }; + expect(buttonTag(render(), "t(categories.hierarchy.check)")).toContain('disabled=""'); + }); + + it("opens the window only while the check is open", () => { + expect(render()).not.toContain('role="dialog"'); + harness.state = { hierarchyCheck: { orphans: [], error: null } }; + const html = render(); + expect(html).toContain('role="dialog"'); + expect(html).toContain("t(categories.hierarchy.none)"); + }); + + it("translates a refusal in the window by its key", () => { + harness.state = { + hierarchyCheck: { orphans: [], error: { key: "categories.hierarchy.errors.too_deep" } }, + }; + expect(render()).toContain("t(categories.hierarchy.errors.too_deep)"); + }); + + it("translates a refused save of the form by its key, and shows any other failure as is", () => { + harness.state = { error: { key: "categories.hierarchy.errors.parent_has_transactions" } }; + expect(render()).toContain(">t(categories.hierarchy.errors.parent_has_transactions)"); + harness.state = { error: { message: "database is locked" } }; + expect(render()).toContain(">database is locked"); + }); +}); diff --git a/src/pages/CategoriesPage.tsx b/src/pages/CategoriesPage.tsx index 54f489e..f620503 100644 --- a/src/pages/CategoriesPage.tsx +++ b/src/pages/CategoriesPage.tsx @@ -1,12 +1,13 @@ import { useState } from "react"; import { useTranslation } from "react-i18next"; import { useLocation } from "react-router-dom"; -import { Plus, RotateCcw, List, AlertTriangle } from "lucide-react"; +import { Plus, RotateCcw, List, AlertTriangle, ListTree } from "lucide-react"; import { PageHelp } from "../components/shared/PageHelp"; -import { useCategories } from "../hooks/useCategories"; +import { useCategories, categoriesErrorText } from "../hooks/useCategories"; import CategoryTree from "../components/categories/CategoryTree"; import CategoryDetailPanel from "../components/categories/CategoryDetailPanel"; import AllKeywordsPanel from "../components/categories/AllKeywordsPanel"; +import HierarchyCheckModal from "../components/categories/HierarchyCheckModal"; import OnboardingStepCard from "../components/onboarding/OnboardingStepCard"; import { useOnboarding } from "../contexts/OnboardingContext"; import { @@ -35,6 +36,9 @@ export default function CategoriesPage() { removeKeyword, reinitializeCategories, moveCategory, + checkHierarchy, + reattachOrphan, + closeHierarchyCheck, } = useCategories(); // The getting-started card (#432) points at "All keywords" while it is on @@ -79,6 +83,14 @@ export default function CategoriesPage() { {t("categories.allKeywords")} +