fix: follow-ups of the lot-v018 stack reviews #482

Merged
maximus merged 25 commits from review-followups-lot-v018 into main 2026-10-21 00:31:46 +00:00
2 changed files with 79 additions and 31 deletions
Showing only changes of commit 1386abd91e - Show all commits

View file

@ -69,7 +69,7 @@ const row = (id: number, parent_id: number | null, extra: Partial<Row> = {}): Ro
...extra, ...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. */ /** What categories hold in their own name, for the checks of a parent's content. */
interface Content { interface Content {
@ -130,7 +130,7 @@ function useFakeDb(initial: Row[], transactions: Record<number, number> = {}, co
const select = async (sql: string, params: unknown[] = []) => { const select = async (sql: string, params: unknown[] = []) => {
if (sql === HIERARCHY_SQL) { 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)) { if (CONTENT_SQL.test(sql)) {
// The query as written, its `$1` bound by name. // 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))], ["createCategory (sub-category)", () => createCategory(newChild(11))],
]; ];
const HOLDINGS: Array<[string, Record<number, number>, Content]> = [ // What 11, a leaf that takes input, holds; the code that refuses it.
["parent_has_transactions", { 11: 2 }, {}], const HOLDINGS: Array<[string, string, Record<number, number>, Content]> = [
["parent_has_budget", {}, { budget: [[11, 25]] }], ["transactions", "parent_has_transactions", { 11: 2 }, {}],
["parent_has_keywords", {}, { keywords: [[11, 1]] }], ["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) => { 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); const db = useFakeDb(TREE, transactions, content);
expect((await refusal(act())).code).toBe(code); expect((await refusal(act())).code).toBe(code);
expect(db.writes()).toEqual(["BEGIN", "ROLLBACK"]); 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(); const writes = db.writes();
expect([writes[0], writes[writes.length - 1]]).toEqual(["BEGIN", "COMMIT"]); 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 () => { 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"]); 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 () => { it("checks a category that still takes input even when it has children: it would stop taking input", async () => {
// 1 already has children (10, 11), and a keyword stored before these rules. // 1 has children (10, 11) yet takes input (the fake's default), and holds a keyword.
useFakeDb(TREE, {}, { keywords: [[1, 1]] }); useFakeDb(TREE, {}, { keywords: [[1, 1]] });
expect((await refusal(createCategory(newChild(1)))).code).toBe("parent_has_keywords"); expect((await refusal(createCategory(newChild(1)))).code).toBe("parent_has_keywords");
}); });

View file

@ -44,14 +44,21 @@ export function categoryHierarchyErrorKey(code: CategoryHierarchyErrorCode): str
} }
/** /**
* Every category, active or not, with what the hierarchy rules read (#393). * Every category, active or not, with what the hierarchy rules read (#393),
* The other category queries keep active rows only, which would turn a * and whether it takes input: a category that does would become a parent
* deactivated parent into a missing one. * (`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<HierarchyRow[]> { /** A row of `HIERARCHY_ROWS_SQL`. */
return db.select<HierarchyRow[]>(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<CategoryCheckRow[]> {
return db.select<CategoryCheckRow[]>(HIERARCHY_ROWS_SQL);
} }
/** The orphan categories of the profile, one per group to reattach (`findOrphans`, #393). */ /** 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<void> {
} }
/** /**
* What a parent may not hold in its own name (#393, decision of 2026-10-19): * What a category may not hold to become a parent (#393, decision of
* once a parent, it stops taking input, yet its direct transactions would stay * 2026-10-19): once a parent, it stops taking input, yet its direct
* on it, a non-zero budget amount (in a year of the budget or in a template) * transactions would stay on it, a non-zero budget amount (of any year, or in
* would drop out of the budget grid, and its active keywords would keep sorting * 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 * 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 * 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. * 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` * Whether `parent` would become a parent now: it still takes input, which
* lists, whatever it is today (a leaf, or already a parent), through the * `markAsParent` would take away (`is_inputable = 1`, read the same way).
* handle it is given. Throws before any write.
*/ */
async function checkParentContent(db: Database, parentId: number): Promise<void> { 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<void> {
const parent = rows.find((r) => r.id === parentId);
if (parent === undefined || !becomesParent(parent)) return;
for (const { code, sql } of PARENT_CONTENT_CHECKS) { for (const { code, sql } of PARENT_CONTENT_CHECKS) {
const found = await db.select<Array<{ cnt: number }>>(sql, [parentId]); const found = await db.select<Array<{ cnt: number }>>(sql, [parentId]);
if ((found[0]?.cnt ?? 0) > 0) throw new CategoryHierarchyError(code); if ((found[0]?.cnt ?? 0) > 0) throw new CategoryHierarchyError(code);
@ -108,12 +133,13 @@ async function checkParentContent(db: Database, parentId: number): Promise<void>
/** /**
* Checks moving category `id` under `parentId` (#393), on rows read in the * Checks moving category `id` under `parentId` (#393), on rows read in the
* caller's transaction and through its handle: the hierarchy rules of * caller's transaction and through its handle: the hierarchy rules of
* `parentChangeRefusal`, then what the parent holds (`checkParentContent`). * `parentChangeRefusal`, then what the parent holds when it would become one
* The root is always accepted. Throws before any write. * (`checkParentContent`). The root is always accepted. Throws before any
* write.
*/ */
async function checkNewParent( async function checkNewParent(
db: Database, db: Database,
rows: HierarchyRow[], rows: CategoryCheckRow[],
id: number, id: number,
parentId: number | null parentId: number | null
): Promise<void> { ): Promise<void> {
@ -125,7 +151,7 @@ async function checkNewParent(
} }
const refusal = parentChangeRefusal(active, id, parentId); const refusal = parentChangeRefusal(active, id, parentId);
if (refusal !== null) throw new CategoryHierarchyError(refusal); 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. */ /** Runs `body` between BEGIN and COMMIT on the transaction handle, ROLLBACK on failure. */
@ -199,9 +225,9 @@ export async function getCategoryDepth(categoryId: number): Promise<number> {
/** /**
* Creates a category. Under a parent: the depth is checked first, outside any * Creates a category. Under a parent: the depth is checked first, outside any
* transaction (`getCategoryDepth`); then, in one transaction through its * transaction (`getCategoryDepth`); then, in one transaction through its
* handle, what the parent holds (#393, `checkParentContent`: * handle, what the parent holds when it would become one (#393,
* `CategoryHierarchyError` on a refusal, nothing written), the new row, and * `checkParentContent`: `CategoryHierarchyError` on a refusal, nothing
* the parent's `is_inputable`. * written), the new row, and the parent's `is_inputable`.
*/ */
export async function createCategory(data: { export async function createCategory(data: {
name: string; name: string;
@ -223,7 +249,7 @@ export async function createCategory(data: {
return withTransaction((db) => return withTransaction((db) =>
inTransaction(db, async () => { 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( const result = await db.execute(
`INSERT INTO categories (name, type, color, parent_id, is_inputable, sort_order) VALUES ($1, $2, $3, $4, $5, $6)`, `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] [data.name, data.type, data.color, parentId, data.is_inputable ? 1 : 0, data.sort_order]