From 7a604e0e0df48c408b86d5271627d6a8efaac844 Mon Sep 17 00:00:00 2001 From: le king fu Date: Thu, 13 Aug 2026 13:16:52 -0400 Subject: [PATCH] fix(import): persist the import format and restore it faithfully The root bug of this chantier. `import_sources` carried no `amount_mode` and no `sign_convention` until v17, so restoring a configured source re-inferred the mode from `mapping.debitAmount !== undefined` and wrote `signConvention: "negative_expense"` outright (useImportWizard.ts:321-323). A credit-card statement configured for positive expenses came back on the default convention at its second import and `parseFilesInternal` negated every amount: expenses landed as income, with no error shown anywhere. The format is a read value now, not a guessed one. Two types and a codec, not one composed type. The four carriers are structurally incompatible -- `ImportSource.has_header` is declared boolean, `ImportConfigTemplate.has_header` is a number, `SourceConfig` is camelCase on a parsed mapping -- so the guarantee cannot come from a shared shape. It comes from `src/utils/importFormat.ts` being the single conversion point between `ImportFormatRow` (persisted: snake_case, mapping as JSON, `has_header` normalized to 0/1) and `ImportFormat` (domain), and from its completeness test. That test is enforced on two levels, and both were mutation-checked: `FORMAT_FIELD_PAIRS` is typed `Record`, so a field added to the format fails to BUILD until it is listed; the test then compares each codec's real output keys against that table, so a field listed but not wired fails the TEST. Dropping `sign_convention` from `formatToRow` -- the shape of the original bug -- fails 14 tests. `formatFromRow` validates rather than falls back. The v17 CHECK admits `absolute_indicator` so the third amount mode ships without another migration, but the app cannot map one: falling through to the `single` branch would read the wrong column for every row, and anything other than `positive_expense` would silently mean `negative_expense`. It raises an `ImportFormatError` carrying an i18n key, and the wizard opens on a fresh configuration so "reconfigure this source" stays an action the user can actually take. Also here: - The config write moves from `checkDuplicatesInternal` to `executeImport`, so an import abandoned at the duplicate step leaves no configuration behind. It is the only write point in the hook and a guard test holds that. - Switching amount mode prunes the abandoned mode's columns, so the mode owns the mapping rather than the reverse. The column the ` merely DISPLAYS + // would make that error unreachable, so an absent column stays absent. + it("does not invent the column the select displays by default", () => { + const cleared = clearMappingForMode({ date: 0, description: 1 }, "single"); + expect("amount" in cleared).toBe(false); + const pair = clearMappingForMode({ date: 0, description: 1 }, "debit_credit"); + expect("debitAmount" in pair).toBe(false); + expect("creditAmount" in pair).toBe(false); + }); + + it("emits no undefined-valued keys, so the persisted JSON stays clean", () => { + const row = formatToRow({ + ...SAMPLE_FORMAT, + columnMapping: clearMappingForMode(full, "single"), + }); + expect(row.column_mapping).toBe('{"date":0,"description":1,"amount":2}'); + }); + + it("is idempotent", () => { + const once = clearMappingForMode(full, "debit_credit"); + expect(clearMappingForMode(once, "debit_credit")).toEqual(once); + }); +}); + +// --------------------------------------------------------------------------- +// Static guards on useImportWizard. +// +// The restore path and the config write point live inside `useCallback`s of a +// React hook and the repository has no jsdom (see FilterPanel.test.tsx), so the +// only way to assert on them is to read the source. Same technique as the guard +// link 1 left on `parseFilesInternal` in csvAutoDetect.test.ts. +// --------------------------------------------------------------------------- + +const WIZARD_SRC = readFileSync( + resolve(import.meta.dirname, "..", "hooks", "useImportWizard.ts"), + "utf-8" +); + +/** The body of a `const = useCallback(...)` block, up to the next one. */ +function callbackBody(name: string, nextName: string): string { + const start = WIZARD_SRC.indexOf(`const ${name} =`); + const end = WIZARD_SRC.indexOf(`const ${nextName} =`, start); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + return WIZARD_SRC.slice(start, end); +} + +/** + * The block of `selectSource` that rebuilds a stored format. Scoped tightly on + * purpose: the fresh-source arm below it legitimately names `encoding` while + * auto-detecting one. + */ +function restoreBranch(): string { + const body = callbackBody("selectSource", "loadHeadersWithConfig"); + const start = body.indexOf("let restored: SourceConfig | null = null;"); + const end = body.indexOf("if (restored) {", start); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + return body.slice(start, end); +} + +describe("useImportWizard — restore reads, never re-derives (#324)", () => { + it("routes the restore through the codec", () => { + expect(restoreBranch()).toContain("...formatFromRow(existing)"); + }); + + it("no longer re-infers the amount mode from the mapping shape", () => { + expect(WIZARD_SRC).not.toContain("mapping.debitAmount !== undefined"); + }); + + // The spread alone is not enough: a literal AFTER it silently wins, which is + // how `signConvention: "negative_expense"` overrode the stored value. The + // restore must name no format field of its own. + it("names no format field of its own in the restore", () => { + const branch = restoreBranch(); + for (const field of Object.keys(FORMAT_FIELD_PAIRS)) { + expect(branch, `restore hardcodes ${field}`).not.toContain(`${field}:`); + } + }); + + it("restores the template provenance instead of blanking it", () => { + expect(callbackBody("selectSource", "loadHeadersWithConfig")).toContain( + "existing?.template_id ?? null" + ); + }); +}); + +describe("useImportWizard — the config write point (#324)", () => { + it("writes nothing at the duplicate step", () => { + const body = callbackBody("checkDuplicatesInternal", "checkDuplicates"); + expect(body).not.toContain("createSource("); + expect(body).not.toContain("updateSource("); + }); + + it("writes the format at import time, through the codec", () => { + const body = callbackBody("executeImport", "goToStep"); + expect(body).toContain("formatToRow(config)"); + expect(body).toContain("createSource("); + expect(body).toContain("updateSource("); + }); + + it("keeps a single write point in the whole hook", () => { + expect(WIZARD_SRC.match(/await createSource\(/g)).toHaveLength(1); + expect(WIZARD_SRC.match(/await updateSource\(/g)).toHaveLength(1); + }); +}); diff --git a/src/utils/importFormat.ts b/src/utils/importFormat.ts new file mode 100644 index 0000000..7fe86e9 --- /dev/null +++ b/src/utils/importFormat.ts @@ -0,0 +1,187 @@ +/** + * The import format codec — the SINGLE conversion point between the persisted + * shape of a CSV format (`ImportFormatRow`, shared by `import_sources` and + * `import_config_templates`) and its domain shape (`ImportFormat`). + * + * Why a codec and not a shared type: the two shapes cannot be unified. The + * persisted rows are snake_case, store the column mapping as JSON, and have no + * boolean type; the wizard works in camelCase on a parsed mapping. See the + * comment block on `ImportFormatRow` in `shared/types`. + * + * The property this file exists to hold: a field cannot be added to the format + * and then silently skipped on the way to or from the database. That is + * enforced on two levels — `FORMAT_FIELD_PAIRS` is typed against + * `keyof ImportFormat`, so the build breaks until a new field is listed, and + * `importFormat.test.ts` compares the codec's actual output keys against that + * table, so listing a field without wiring it fails the test. Losing + * `sign_convention` on the way out is what this chantier is repairing (#324). + */ + +import type { + AmountMode, + ColumnMapping, + ImportFormat, + ImportFormatRow, + ImportFormatRowInput, + SignConvention, +} from "../shared/types"; + +/** + * Values the application can actually map. The database `CHECK` on + * `import_sources.amount_mode` deliberately admits `absolute_indicator` so the + * third mode ships without another migration — but until it is implemented, + * reading one must be a visible error, never a silent fall-through to the + * `single` branch (which would read the wrong column for every row). + */ +export const AMOUNT_MODES: readonly AmountMode[] = ["single", "debit_credit"]; +export const SIGN_CONVENTIONS: readonly SignConvention[] = [ + "negative_expense", + "positive_expense", +]; + +/** + * The field correspondence table, domain key -> persisted key. + * + * Typed as `Record`: adding a field + * to `ImportFormat` makes this literal stop satisfying the mapped type and the + * build fails until the field is listed here. The test then verifies that both + * codec directions actually emit every listed field. + */ +export const FORMAT_FIELD_PAIRS: Record = { + delimiter: "delimiter", + encoding: "encoding", + dateFormat: "date_format", + skipLines: "skip_lines", + hasHeader: "has_header", + columnMapping: "column_mapping", + amountMode: "amount_mode", + signConvention: "sign_convention", +}; + +/** i18n keys for the ways a stored format can be unreadable. */ +export type ImportFormatErrorKey = + | "import.errors.unsupportedAmountMode" + | "import.errors.unsupportedSignConvention" + | "import.errors.invalidColumnMapping"; + +/** + * Thrown when a persisted format cannot be decoded. It carries an i18n key so + * the wizard can surface it verbatim instead of falling back to a default + * format — a wrong default is precisely how the original bug wrote reversed + * amounts without an error. + */ +export class ImportFormatError extends Error { + readonly i18nKey: ImportFormatErrorKey; + + constructor(i18nKey: ImportFormatErrorKey, detail: string) { + super(`${i18nKey}: ${detail}`); + this.name = "ImportFormatError"; + this.i18nKey = i18nKey; + } +} + +/** Domain format -> persisted row. `has_header` is normalized to 0/1 here. */ +export function formatToRow(format: ImportFormat): ImportFormatRow { + return { + delimiter: format.delimiter, + encoding: format.encoding, + date_format: format.dateFormat, + skip_lines: format.skipLines, + has_header: format.hasHeader ? 1 : 0, + column_mapping: JSON.stringify(format.columnMapping), + amount_mode: format.amountMode, + sign_convention: format.signConvention, + }; +} + +/** + * Persisted row -> domain format. + * + * Every enumerated field is validated against the application's whitelist and + * an unknown value raises rather than falls back, because both fall-backs are + * silent data corruption: an unmapped `amount_mode` reads the wrong column, and + * anything other than `positive_expense` would mean `negative_expense` and flip + * every sign. + */ +export function formatFromRow(row: ImportFormatRowInput): ImportFormat { + if (!AMOUNT_MODES.includes(row.amount_mode)) { + throw new ImportFormatError( + "import.errors.unsupportedAmountMode", + String(row.amount_mode) + ); + } + if (!SIGN_CONVENTIONS.includes(row.sign_convention)) { + throw new ImportFormatError( + "import.errors.unsupportedSignConvention", + String(row.sign_convention) + ); + } + + let columnMapping: ColumnMapping; + try { + columnMapping = JSON.parse(row.column_mapping) as ColumnMapping; + } catch { + throw new ImportFormatError( + "import.errors.invalidColumnMapping", + row.column_mapping + ); + } + if ( + columnMapping === null || + typeof columnMapping !== "object" || + typeof columnMapping.date !== "number" || + typeof columnMapping.description !== "number" + ) { + throw new ImportFormatError( + "import.errors.invalidColumnMapping", + row.column_mapping + ); + } + + return { + delimiter: row.delimiter, + encoding: row.encoding, + dateFormat: row.date_format, + skipLines: row.skip_lines, + hasHeader: !!row.has_header, + columnMapping, + amountMode: row.amount_mode, + signConvention: row.sign_convention, + }; +} + +/** + * Drop the columns belonging to the mode being left behind, so the amount mode + * is the source of truth for the mapping rather than the reverse. + * + * Before v17 the mode was re-inferred from `mapping.debitAmount !== undefined`, + * which made a stale key from an abandoned mode silently decide how amounts + * were read. The mode is persisted now, but a mapping still carrying both + * shapes would keep the two disagreeing — so switching mode prunes. + * + * The column the `