From 1386abd91ec5e5303d208ed6369b505b5c4bcc44 Mon Sep 17 00:00:00 2001 From: le king fu Date: Tue, 20 Oct 2026 19:40:04 -0400 Subject: [PATCH] fix(categories): check a parent's content only when it would become one (#393) Decision of Max of 2026-10-20 (review 25921): transactions, non-zero budget amounts and active keywords are checked only while the target still takes input (is_inputable = 1, the condition of markAsParent), that is when it really becomes a parent and stops taking input. An existing parent, no longer inputable, accepts a new or reattached subcategory whatever it holds, as in every released version: the refusal protected nothing there. All three codes and all three entry points follow the rule; the hierarchy rows now carry is_inputable, and createCategory reads them through its transaction's handle. Also pins a negative amount, in the grid and in a template, as a refusal (review 25921, R1/R2): the grid stores what is typed. Co-Authored-By: Claude Opus 5.5 --- src/services/categoryService.test.ts | 40 ++++++++++++---- src/services/categoryService.ts | 70 +++++++++++++++++++--------- 2 files changed, 79 insertions(+), 31 deletions(-) diff --git a/src/services/categoryService.test.ts b/src/services/categoryService.test.ts index 6c5a0a3..b240cb0 100644 --- a/src/services/categoryService.test.ts +++ b/src/services/categoryService.test.ts @@ -69,7 +69,7 @@ const row = (id: number, parent_id: number | null, extra: Partial = {}): Ro ...extra, }); -const HIERARCHY_SQL = "SELECT id, parent_id, is_active FROM categories"; +const HIERARCHY_SQL = "SELECT id, parent_id, is_active, is_inputable FROM categories"; /** What categories hold in their own name, for the checks of a parent's content. */ interface Content { @@ -130,7 +130,7 @@ function useFakeDb(initial: Row[], transactions: Record = {}, co const select = async (sql: string, params: unknown[] = []) => { if (sql === HIERARCHY_SQL) { - return rows.map(({ id, parent_id, is_active }) => ({ id, parent_id, is_active })); + return rows.map(({ id, parent_id, is_active, is_inputable }) => ({ id, parent_id, is_active, is_inputable })); } if (CONTENT_SQL.test(sql)) { // The query as written, its `$1` bound by name. @@ -377,14 +377,22 @@ describe("a parent's content: transactions, budget amounts, keywords (#393, deci ["createCategory (sub-category)", () => createCategory(newChild(11))], ]; - const HOLDINGS: Array<[string, Record, Content]> = [ - ["parent_has_transactions", { 11: 2 }, {}], - ["parent_has_budget", {}, { budget: [[11, 25]] }], - ["parent_has_keywords", {}, { keywords: [[11, 1]] }], + // What 11, a leaf that takes input, holds; the code that refuses it. + const HOLDINGS: Array<[string, string, Record, Content]> = [ + ["transactions", "parent_has_transactions", { 11: 2 }, {}], + ["a budget amount", "parent_has_budget", {}, { budget: [[11, 25]] }], + // The grid stores what is typed: a negative amount counts too. + ["a negative budget amount", "parent_has_budget", {}, { budget: [[11, -25]] }], + ["a negative template amount", "parent_has_budget", {}, { templates: [[11, -40]] }], + ["an active keyword", "parent_has_keywords", {}, { keywords: [[11, 1]] }], ]; + // An existing parent no longer takes input: 11, with 110 under it, holding + // transactions, amounts and a keyword stored before these rules. + const EXISTING_PARENT = [...TREE.map((r) => (r.id === 11 ? { ...r, is_inputable: 0 } : r)), row(110, 11)]; + describe.each(ENTRY_POINTS)("%s", (_name, act) => { - it.each(HOLDINGS)("refuses %s, writing nothing", async (code, transactions, content) => { + it.each(HOLDINGS)("refuses a leaf that takes input and holds %s (%s), writing nothing", async (_holding, code, transactions, content) => { const db = useFakeDb(TREE, transactions, content); expect((await refusal(act())).code).toBe(code); expect(db.writes()).toEqual(["BEGIN", "ROLLBACK"]); @@ -406,6 +414,20 @@ describe("a parent's content: transactions, budget amounts, keywords (#393, deci const writes = db.writes(); expect([writes[0], writes[writes.length - 1]]).toEqual(["BEGIN", "COMMIT"]); }); + + it("accepts an existing parent, which no longer takes input, whatever it holds (decision of 2026-10-20)", async () => { + const db = useFakeDb(EXISTING_PARENT, { 11: 2 }, { + budget: [[11, 25]], + templates: [[11, 10]], + keywords: [[11, 1]], + }); + await act(); + // Nothing to protect: markAsParent changes nothing for it, and no content is read. + expect(db.rows().find((r) => r.id === 11)?.is_inputable).toBe(0); + expect(db.log.some((l) => CONTENT_SQL.test(l.sql))).toBe(false); + const writes = db.writes(); + expect([writes[0], writes[writes.length - 1]]).toEqual(["BEGIN", "COMMIT"]); + }); }); it("refuses a budget amount of a template, even with no amount in the budget itself", async () => { @@ -454,8 +476,8 @@ describe("a parent's content: transactions, budget amounts, keywords (#393, deci expect(db.writes()).toEqual(["BEGIN", "INSERT INTO categories (name, type, color, parent_id, is_inputable, sort_order) VALUES ($1, $2, $3, $4, $5, $6)", "COMMIT"]); }); - it("checks a parent that is already a parent the same way", async () => { - // 1 already has children (10, 11), and a keyword stored before these rules. + it("checks a category that still takes input even when it has children: it would stop taking input", async () => { + // 1 has children (10, 11) yet takes input (the fake's default), and holds a keyword. useFakeDb(TREE, {}, { keywords: [[1, 1]] }); expect((await refusal(createCategory(newChild(1)))).code).toBe("parent_has_keywords"); }); diff --git a/src/services/categoryService.ts b/src/services/categoryService.ts index 0ba748e..e16e27e 100644 --- a/src/services/categoryService.ts +++ b/src/services/categoryService.ts @@ -44,14 +44,21 @@ export function categoryHierarchyErrorKey(code: CategoryHierarchyErrorCode): str } /** - * Every category, active or not, with what the hierarchy rules read (#393). - * The other category queries keep active rows only, which would turn a - * deactivated parent into a missing one. + * Every category, active or not, with what the hierarchy rules read (#393), + * and whether it takes input: a category that does would become a parent + * (`checkParentContent`). The other category queries keep active rows only, + * which would turn a deactivated parent into a missing one. */ -const HIERARCHY_ROWS_SQL = "SELECT id, parent_id, is_active FROM categories"; +const HIERARCHY_ROWS_SQL = "SELECT id, parent_id, is_active, is_inputable FROM categories"; -async function readHierarchyRows(db: Database): Promise { - return db.select(HIERARCHY_ROWS_SQL); +/** A row of `HIERARCHY_ROWS_SQL`. */ +interface CategoryCheckRow extends HierarchyRow { + /** `1` while the category takes input (SQLite's INTEGER flag). */ + is_inputable: number | boolean; +} + +async function readHierarchyRows(db: Database): Promise { + return db.select(HIERARCHY_ROWS_SQL); } /** The orphan categories of the profile, one per group to reattach (`findOrphans`, #393). */ @@ -69,10 +76,11 @@ async function markAsParent(db: Database, parentId: number): Promise { } /** - * What a parent may not hold in its own name (#393, decision of 2026-10-19): - * once a parent, it stops taking input, yet its direct transactions would stay - * on it, a non-zero budget amount (in a year of the budget or in a template) - * would drop out of the budget grid, and its active keywords would keep sorting + * What a category may not hold to become a parent (#393, decision of + * 2026-10-19): once a parent, it stops taking input, yet its direct + * transactions would stay on it, a non-zero budget amount (of any year, or in + * a template; negative ones included, the grid stores what is typed) would + * drop out of the budget grid, and its active keywords would keep sorting * transactions into it. A deleted keyword (`is_active = 0`) sorts nothing and * no screen shows it, so it does not count. Checked in this order, and the * first one found is the refusal: transactions first, as before these two. @@ -94,11 +102,28 @@ const PARENT_CONTENT_CHECKS: ReadonlyArray<{ code: CategoryHierarchyErrorCode; s ]; /** - * Refuses `parentId` as a parent when it holds what `PARENT_CONTENT_CHECKS` - * lists, whatever it is today (a leaf, or already a parent), through the - * handle it is given. Throws before any write. + * Whether `parent` would become a parent now: it still takes input, which + * `markAsParent` would take away (`is_inputable = 1`, read the same way). */ -async function checkParentContent(db: Database, parentId: number): Promise { +function becomesParent(parent: CategoryCheckRow): boolean { + return parent.is_inputable === 1 || parent.is_inputable === true; +} + +/** + * Refuses `parentId` as a parent when it would become one now (it still takes + * input) and holds what `PARENT_CONTENT_CHECKS` lists, through the handle it + * is given; `rows` are read through the same handle. A category that no + * longer takes input, an existing parent for one, accepts a child whatever it + * holds, as before these checks: `markAsParent` changes nothing for it + * (decision of Max of 2026-10-20, review 25921). Throws before any write. + */ +async function checkParentContent( + db: Database, + rows: CategoryCheckRow[], + parentId: number +): Promise { + const parent = rows.find((r) => r.id === parentId); + if (parent === undefined || !becomesParent(parent)) return; for (const { code, sql } of PARENT_CONTENT_CHECKS) { const found = await db.select>(sql, [parentId]); if ((found[0]?.cnt ?? 0) > 0) throw new CategoryHierarchyError(code); @@ -108,12 +133,13 @@ async function checkParentContent(db: Database, parentId: number): Promise /** * Checks moving category `id` under `parentId` (#393), on rows read in the * caller's transaction and through its handle: the hierarchy rules of - * `parentChangeRefusal`, then what the parent holds (`checkParentContent`). - * The root is always accepted. Throws before any write. + * `parentChangeRefusal`, then what the parent holds when it would become one + * (`checkParentContent`). The root is always accepted. Throws before any + * write. */ async function checkNewParent( db: Database, - rows: HierarchyRow[], + rows: CategoryCheckRow[], id: number, parentId: number | null ): Promise { @@ -125,7 +151,7 @@ async function checkNewParent( } const refusal = parentChangeRefusal(active, id, parentId); if (refusal !== null) throw new CategoryHierarchyError(refusal); - await checkParentContent(db, parentId); + await checkParentContent(db, rows, parentId); } /** Runs `body` between BEGIN and COMMIT on the transaction handle, ROLLBACK on failure. */ @@ -199,9 +225,9 @@ export async function getCategoryDepth(categoryId: number): Promise { /** * Creates a category. Under a parent: the depth is checked first, outside any * transaction (`getCategoryDepth`); then, in one transaction through its - * handle, what the parent holds (#393, `checkParentContent`: - * `CategoryHierarchyError` on a refusal, nothing written), the new row, and - * the parent's `is_inputable`. + * handle, what the parent holds when it would become one (#393, + * `checkParentContent`: `CategoryHierarchyError` on a refusal, nothing + * written), the new row, and the parent's `is_inputable`. */ export async function createCategory(data: { name: string; @@ -223,7 +249,7 @@ export async function createCategory(data: { return withTransaction((db) => inTransaction(db, async () => { - if (parentId !== null) await checkParentContent(db, parentId); + if (parentId !== null) await checkParentContent(db, await readHierarchyRows(db), parentId); const result = await db.execute( `INSERT INTO categories (name, type, color, parent_id, is_inputable, sort_order) VALUES ($1, $2, $3, $4, $5, $6)`, [data.name, data.type, data.color, parentId, data.is_inputable ? 1 : 0, data.sort_order]