From ce19efd47651615b9b0c75a893e63ceb941c409e Mon Sep 17 00:00:00 2001 From: le king fu Date: Thu, 13 Aug 2026 14:31:49 -0400 Subject: [PATCH] feat(import): make the preview mandatory and show what the amounts mean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A confidence score reports how many rows were READ, never what they say: the `all-positive` fixture scores a perfect 100 % while every credit is imported as an expense, because each of those rows is perfectly readable. Nothing between that score and the database looked at the signs — the preview was an optional modal of 20 rows with no totals, and the final confirmation listed the delimiter and the date format but neither the amount mode nor the sign convention. The `file-preview` step had been declared in `ImportWizardStep` since the beginning and no dispatch ever aimed at it. It is a real step now, traversed at every import and gated by nothing — in particular not by the detection score, which is sign-blind by construction. - `useImportWizard`: `parseAndPreview` parses and stops at the preview, replacing `parsePreview` and the `parseAndCheckDuplicates` that jumped straight to the duplicates ("skips preview step"); `checkDuplicates`, dead code until now, is the preview's next button, so the rows the user validated are the rows that get checked. - `summarizeParsedRows`: the recap, pure and tested — outflows and their total, inflows and theirs, rows in error. Totals stay SIGNED, since magnitudes would hide the one thing the recap exists to expose. Computed over the whole file, never over the twenty rows displayed. - `flipSignFormat` + "Inverser les signes": the correction lands on the CONFIGURATION, so it is persisted with the source and the next file from that bank reads right on its own. In debit/credit mode it swaps the two column indices rather than toggling a convention `mapRow` ignores there, where a toggle would have been inert. - `ImportConfirmation` states the amount mode, the sign convention (in the mode that applies it) and the column mapping, named by header. - `FilePreviewModal` removed: it was the redundant surface, and editing the table alone would have mutated a still-live copy of it. The `all-positive` KNOWN DEFECT marker is dropped rather than deleted. Its three original expectations still hold — an unsigned file carries no direction and detection cannot invent one — and two cases were added: the recap tell (six outflows, zero inflows) and the honest limit, that flipping this particular file only produces its mirror image. 1088 vitest (1054 before), tsc and vite build clean. No DB migration. Resolves #329 --- src/components/import/FilePreviewModal.tsx | 61 ---- src/components/import/FilePreviewTable.tsx | 98 +++++- src/components/import/ImportConfirmation.tsx | 70 ++++ src/hooks/useImportWizard.ts | 67 +++- src/i18n/locales/en.json | 10 +- src/i18n/locales/fr.json | 10 +- src/pages/ImportPage.tsx | 94 +++--- src/utils/csvAutoDetect.test.ts | 147 ++++++++- src/utils/importFormat.test.ts | 327 ++++++++++++++++++- src/utils/importFormat.ts | 119 +++++++ 10 files changed, 842 insertions(+), 161 deletions(-) delete mode 100644 src/components/import/FilePreviewModal.tsx diff --git a/src/components/import/FilePreviewModal.tsx b/src/components/import/FilePreviewModal.tsx deleted file mode 100644 index 07057de..0000000 --- a/src/components/import/FilePreviewModal.tsx +++ /dev/null @@ -1,61 +0,0 @@ -import { useEffect } from "react"; -import { createPortal } from "react-dom"; -import { useTranslation } from "react-i18next"; -import { X } from "lucide-react"; -import FilePreviewTable from "./FilePreviewTable"; -import type { ParsedRow } from "../../shared/types"; - -interface FilePreviewModalProps { - rows: ParsedRow[]; - totalCount: number; - onClose: () => void; -} - -export default function FilePreviewModal({ - rows, - totalCount, - onClose, -}: FilePreviewModalProps) { - const { t } = useTranslation(); - - useEffect(() => { - function handleEscape(e: KeyboardEvent) { - if (e.key === "Escape") onClose(); - } - document.addEventListener("keydown", handleEscape); - return () => document.removeEventListener("keydown", handleEscape); - }, [onClose]); - - return createPortal( -
{ if (e.target === e.currentTarget) onClose(); }} - > -
- {/* Header */} -
-

{t("import.preview.title")}

- -
- - {/* Body */} -
- - {totalCount > rows.length && ( -

- {t("import.preview.moreRows", { - count: totalCount - rows.length, - })} -

- )} -
-
-
, - document.body - ); -} diff --git a/src/components/import/FilePreviewTable.tsx b/src/components/import/FilePreviewTable.tsx index 444516e..ac7e933 100644 --- a/src/components/import/FilePreviewTable.tsx +++ b/src/components/import/FilePreviewTable.tsx @@ -1,16 +1,24 @@ import { useTranslation } from "react-i18next"; -import { AlertCircle } from "lucide-react"; +import { AlertCircle, ArrowDownLeft, ArrowUpRight, RefreshCw } from "lucide-react"; import type { ParsedRow } from "../../shared/types"; -import { isRowErrorKey } from "../../utils/importFormat"; +import { isRowErrorKey, summarizeParsedRows } from "../../utils/importFormat"; + +/** Rows rendered in the table. The recap above it always covers the whole file. */ +const DISPLAYED_ROWS = 20; interface FilePreviewTableProps { + /** ALL parsed rows — the recap is meaningless on a truncated sample. */ rows: ParsedRow[]; + onFlipSigns?: () => void; + isFlipping?: boolean; } export default function FilePreviewTable({ rows, + onFlipSigns, + isFlipping = false, }: FilePreviewTableProps) { - const { t } = useTranslation(); + const { t, i18n } = useTranslation(); if (rows.length === 0) { return ( @@ -20,7 +28,13 @@ export default function FilePreviewTable({ ); } - const errorCount = rows.filter((r) => r.error).length; + const totals = summarizeParsedRows(rows); + const displayedRows = rows.slice(0, DISPLAYED_ROWS); + + const currency = new Intl.NumberFormat( + i18n.language === "fr" ? "fr-CA" : "en-CA", + { style: "currency", currency: "CAD" } + ); // Row errors are i18n keys (#325); anything else reaches us verbatim. const errorText = (error: string) => @@ -36,15 +50,77 @@ export default function FilePreviewTable({ {t("import.preview.rowCount", { count: rows.length })} - {errorCount > 0 && ( + {totals.errorCount > 0 && ( - {t("import.preview.errorCount", { count: errorCount })} + {t("import.preview.errorCount", { count: totals.errorCount })} )} + {/* + The signed recap (#329) — the last control before the database, and the + only one that looks at what the amounts MEAN. A statement showing zero + inflows next to a payroll line is wrong on its face, whatever produced + it. Computed over every row, never over the twenty displayed below. + */} +
+
+
+
+

+ + {t("import.preview.outflowCount", { count: totals.outflowCount })} +

+

+ {currency.format(totals.outflowTotal)} +

+
+
+

+ + {t("import.preview.inflowCount", { count: totals.inflowCount })} +

+

+ {currency.format(totals.inflowTotal)} +

+
+
+

+ + {t("import.preview.errorRows")} +

+

0 + ? "text-[var(--negative)]" + : "text-[var(--muted-foreground)]" + }`} + > + {totals.errorCount} +

+
+
+ + {onFlipSigns && ( +
+ +

+ {t("import.preview.flipSignsHint")} +

+
+ )} +
+
+
@@ -67,7 +143,7 @@ export default function FilePreviewTable({ - {rows.map((row) => ( + {displayedRows.map((row) => (
+ + {rows.length > displayedRows.length && ( +

+ {t("import.preview.moreRows", { + count: rows.length - displayedRows.length, + })} +

+ )} ); } diff --git a/src/components/import/ImportConfirmation.tsx b/src/components/import/ImportConfirmation.tsx index 3c9aa88..41d5bf1 100644 --- a/src/components/import/ImportConfirmation.tsx +++ b/src/components/import/ImportConfirmation.tsx @@ -5,6 +5,8 @@ import type { SourceConfig, ScannedFile, DuplicateCheckResult } from "../../shar interface ImportConfirmationProps { sourceName: string; config: SourceConfig; + /** Header labels of the parsed file, used to name the mapped columns. */ + headers?: string[]; selectedFiles: ScannedFile[]; duplicateResult: DuplicateCheckResult; excludedCount: number; @@ -13,6 +15,7 @@ interface ImportConfirmationProps { export default function ImportConfirmation({ sourceName, config, + headers = [], selectedFiles, duplicateResult, excludedCount, @@ -22,6 +25,32 @@ export default function ImportConfirmation({ const rowsToImport = duplicateResult.newRows.length + duplicateResult.duplicateRows.length - excludedCount; + /** + * A mapped column, named by its header when the file has one. An index alone + * ("2") tells the reader nothing about whether the right column was picked. + */ + const columnLabel = (index: number | undefined): string => { + if (index === undefined) return t("import.confirm.columnUnmapped"); + const header = headers[index]?.trim(); + return header ? `${index} — ${header}` : String(index); + }; + + // The mapping as the amount mode actually reads it: naming a debit column on + // a single-amount format would describe an import that is not happening. + const mappedColumns: Array<[string, number | undefined]> = + config.amountMode === "debit_credit" + ? [ + ["import.config.dateColumn", config.columnMapping.date], + ["import.config.descriptionColumn", config.columnMapping.description], + ["import.config.debitColumn", config.columnMapping.debitAmount], + ["import.config.creditColumn", config.columnMapping.creditAmount], + ] + : [ + ["import.config.dateColumn", config.columnMapping.date], + ["import.config.descriptionColumn", config.columnMapping.description], + ["import.config.amountColumn", config.columnMapping.amount], + ]; + return (

@@ -73,6 +102,47 @@ export default function ImportConfirmation({ {t("import.config.skipLines")}:{" "} {config.skipLines}

+ {/* + The amount mode and the sign convention (#329). They decide what + every amount MEANS, and they were the two settings this last + checkpoint did not show — the most fragile pair, invisible. + */} +
+ {t("import.config.amountMode")}:{" "} + {config.amountMode === "debit_credit" + ? t("import.config.debitCredit") + : t("import.config.singleAmount")} +
+ {/* + Single-amount mode only: `mapRow` computes `credit - debit` on + magnitudes in debit/credit mode and never reads the convention + there, so stating one would describe a rule that is not applied. + */} + {config.amountMode === "single" && ( +
+ + {t("import.config.signConvention")}: + {" "} + {config.signConvention === "positive_expense" + ? t("import.config.positiveExpense") + : t("import.config.negativeExpense")} +
+ )} + + + + {/* Column mapping */} +
+

+ {t("import.config.columnMapping")} +

+
+ {mappedColumns.map(([labelKey, index]) => ( +
+ {t(labelKey)}:{" "} + {columnLabel(index)} +
+ ))}
diff --git a/src/hooks/useImportWizard.ts b/src/hooks/useImportWizard.ts index dd0fd64..b2fa11e 100644 --- a/src/hooks/useImportWizard.ts +++ b/src/hooks/useImportWizard.ts @@ -46,6 +46,7 @@ import { } from "../utils/csvAutoDetect"; import { detectAmountSeparators, + flipSignFormat, formatFromRow, formatToRow, ImportFormatError, @@ -580,9 +581,16 @@ export function useImportWizard() { } }, [state.selectedSource, state.importedFilesBySource]); - // Internal helper: parses selected files and returns rows + headers - const parseFilesInternal = useCallback(async (): Promise<{ rows: ParsedRow[]; headers: string[] }> => { - const config = state.sourceConfig; + // Internal helper: parses selected files and returns rows + headers. + // + // `configOverride` exists for the sign flip: it re-parses under a format that + // has just been dispatched and is therefore not yet readable in `state`. + // Reading the stale one would redisplay the exact table the user asked to + // correct, which reads as "the button did nothing". + const parseFilesInternal = useCallback(async ( + configOverride?: SourceConfig + ): Promise<{ rows: ParsedRow[]; headers: string[] }> => { + const config = configOverride ?? state.sourceConfig; const allRows: ParsedRow[] = []; let headers: string[] = []; @@ -644,8 +652,15 @@ export function useImportWizard() { return { rows: allRows, headers }; }, [state.selectedFiles, state.sourceConfig]); - // Parse files and store preview (does NOT change wizard step) - const parsePreview = useCallback(async () => { + // Parse the selected files and STOP at the preview. + // + // Every import goes through that step now (#329). It used to be an optional + // modal nobody had to open, next to a button that went straight to the + // duplicate check — so a file read under a wrong convention reached the + // database without anyone ever seeing a total. The step is unconditional on + // purpose: the detection score does not gate it, because a perfect score says + // nothing about the sign of what was read. + const parseAndPreview = useCallback(async () => { if (state.selectedFiles.length === 0) return; dispatch({ type: "SET_LOADING", payload: true }); @@ -657,6 +672,7 @@ export function useImportWizard() { type: "SET_PARSED_PREVIEW", payload: result, }); + dispatch({ type: "SET_STEP", payload: "file-preview" }); } catch (e) { dispatch({ type: "SET_ERROR", @@ -746,7 +762,12 @@ export function useImportWizard() { dispatch({ type: "SET_STEP", payload: "duplicate-check" }); }, [state.selectedFiles]); - // Check duplicates using already-parsed preview data + // Leave the preview for the duplicate check, on the rows it just displayed. + // + // Dead code until #329: the wizard jumped from the configuration straight to + // the duplicates, re-parsing on the way. It is the preview step's "next" + // button now, so the rows the user validated are the rows that get checked — + // no second parse can quietly read the file differently in between. const checkDuplicates = useCallback(async () => { dispatch({ type: "SET_LOADING", payload: true }); dispatch({ type: "SET_ERROR", payload: null }); @@ -761,27 +782,45 @@ export function useImportWizard() { } }, [state.parsedPreview, checkDuplicatesInternal]); - // Parse files then check duplicates in one step (skips preview step) - const parseAndCheckDuplicates = useCallback(async () => { - if (state.selectedFiles.length === 0) return; + /** + * Read the file the other way round, and remember it. + * + * The correction lands on the CONFIGURATION (`flipSignFormat`, which knows + * that debit/credit flips by swapping columns rather than by toggling a + * convention `mapRow` ignores there), so it is persisted with the source at + * import time and the next file from that bank reads right on its own. + * + * Headers are deliberately NOT reloaded: a flip touches neither delimiter, + * encoding, skipped lines nor header flag, and `loadHeadersWithConfig` + * dispatches an empty row list — racing it against this re-parse is a coin + * toss between the corrected table and a blank one. + */ + const flipSignConvention = useCallback(async () => { + const flipped: SourceConfig = { + ...state.sourceConfig, + ...flipSignFormat(state.sourceConfig), + }; + dispatch({ type: "SET_SOURCE_CONFIG", payload: flipped }); + // Detection did not produce this format. Keeping its badge would vouch for + // a configuration it never measured — the same rule as a manual edit. + dispatch({ type: "SET_DETECTION_SCORE", payload: null }); dispatch({ type: "SET_LOADING", payload: true }); dispatch({ type: "SET_ERROR", payload: null }); try { - const result = await parseFilesInternal(); + const result = await parseFilesInternal(flipped); dispatch({ type: "SET_PARSED_PREVIEW", payload: result, }); - await checkDuplicatesInternal(result.rows); } catch (e) { dispatch({ type: "SET_ERROR", payload: e instanceof Error ? e.message : String(e), }); } - }, [state.selectedFiles, parseFilesInternal, checkDuplicatesInternal]); + }, [state.sourceConfig, parseFilesInternal]); const executeImport = useCallback(async () => { if (!state.duplicateResult) return; @@ -1063,9 +1102,9 @@ export function useImportWizard() { updateConfig, toggleFile, selectAllFiles, - parsePreview, + parseAndPreview, checkDuplicates, - parseAndCheckDuplicates, + flipSignConvention, executeImport, goToStep, reset, diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index 2e303e9..3cfae2d 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -127,7 +127,12 @@ "description": "Description", "amount": "Amount", "raw": "Raw data", - "moreRows": "... and {{count}} more row(s)" + "moreRows": "... and {{count}} more row(s)", + "outflowCount": "{{count}} outflow(s)", + "inflowCount": "{{count}} inflow(s)", + "errorRows": "Rows in error", + "flipSigns": "Flip the signs", + "flipSignsHint": "Corrects the source configuration, not just this preview: the correction is remembered for the next imports." }, "duplicates": { "title": "Duplicate Detection", @@ -149,7 +154,8 @@ "files": "Files", "settings": "Settings", "rowsToImport": "Rows to import", - "rowsSummary": "{{count}} row(s) to import, {{skipped}} duplicate(s) skipped" + "rowsSummary": "{{count}} row(s) to import, {{skipped}} duplicate(s) skipped", + "columnUnmapped": "not mapped" }, "progress": { "title": "Import in Progress", diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index 8e76eca..56a1eb0 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -127,7 +127,12 @@ "description": "Description", "amount": "Montant", "raw": "Données brutes", - "moreRows": "... et {{count}} ligne(s) supplémentaire(s)" + "moreRows": "... et {{count}} ligne(s) supplémentaire(s)", + "outflowCount": "{{count}} sortie(s)", + "inflowCount": "{{count}} entrée(s)", + "errorRows": "Lignes en erreur", + "flipSigns": "Inverser les signes", + "flipSignsHint": "Corrige la configuration de la source, pas seulement cet aperçu : la correction est mémorisée pour les prochains imports." }, "duplicates": { "title": "Détection des doublons", @@ -149,7 +154,8 @@ "files": "Fichiers", "settings": "Paramètres", "rowsToImport": "Lignes à importer", - "rowsSummary": "{{count}} ligne(s) à importer, {{skipped}} doublon(s) ignoré(s)" + "rowsSummary": "{{count}} ligne(s) à importer, {{skipped}} doublon(s) ignoré(s)", + "columnUnmapped": "non associée" }, "progress": { "title": "Import en cours", diff --git a/src/pages/ImportPage.tsx b/src/pages/ImportPage.tsx index 056238e..2a704b3 100644 --- a/src/pages/ImportPage.tsx +++ b/src/pages/ImportPage.tsx @@ -1,17 +1,16 @@ -import { useState, useCallback } from "react"; import { useTranslation } from "react-i18next"; import { useImportWizard } from "../hooks/useImportWizard"; import ImportFolderConfig from "../components/import/ImportFolderConfig"; import SourceList from "../components/import/SourceList"; import SourceConfigPanel from "../components/import/SourceConfigPanel"; +import FilePreviewTable from "../components/import/FilePreviewTable"; import DuplicateCheckPanel from "../components/import/DuplicateCheckPanel"; import ImportConfirmation from "../components/import/ImportConfirmation"; import ImportProgress from "../components/import/ImportProgress"; import ImportReportPanel from "../components/import/ImportReportPanel"; import WizardNavigation from "../components/import/WizardNavigation"; import ImportHistoryPanel from "../components/import/ImportHistoryPanel"; -import FilePreviewModal from "../components/import/FilePreviewModal"; -import { AlertCircle, Eye, X, ChevronLeft } from "lucide-react"; +import { AlertCircle } from "lucide-react"; import { PageHelp } from "../components/shared/PageHelp"; export default function ImportPage() { @@ -24,8 +23,9 @@ export default function ImportPage() { updateConfig, toggleFile, selectAllFiles, - parsePreview, - parseAndCheckDuplicates, + parseAndPreview, + checkDuplicates, + flipSignConvention, executeImport, goToStep, reset, @@ -38,13 +38,6 @@ export default function ImportPage() { setSkipAllDuplicates, } = useImportWizard(); - const [showPreviewModal, setShowPreviewModal] = useState(false); - - const handlePreview = useCallback(async () => { - await parsePreview(); - setShowPreviewModal(true); - }, [parsePreview]); - const nextDisabled = state.selectedFiles.length === 0 || !state.sourceConfig.name; return ( @@ -111,41 +104,36 @@ export default function ImportPage() { detectionScore={state.detectionScore} isLoading={state.isLoading} /> -
-
- -
-
- - - -
-
+ {/* + One way forward, and it goes through the preview (#329). The pair of + buttons this replaces offered "Aperçu" as an optional detour and + "Vérifier les doublons" as the real path, so the totals were the one + screen an import never had to show. + */} + goToStep("source-list")} + onNext={parseAndPreview} + onCancel={reset} + nextLabel={t("import.wizard.preview")} + nextDisabled={nextDisabled || state.isLoading} + /> + + )} + + {state.step === "file-preview" && ( +
+ + goToStep("source-config")} + onNext={checkDuplicates} + onCancel={reset} + nextLabel={t("import.wizard.checkDuplicates")} + nextDisabled={state.isLoading} + />
)} @@ -159,7 +147,7 @@ export default function ImportPage() { onIncludeAll={() => setSkipAllDuplicates(false)} /> goToStep("source-config")} + onBack={() => goToStep("file-preview")} onNext={() => goToStep("confirm")} onCancel={reset} nextLabel={t("import.wizard.confirm")} @@ -172,6 +160,7 @@ export default function ImportPage() { )} - - {/* Preview modal */} - {showPreviewModal && state.parsedPreview.length > 0 && ( - setShowPreviewModal(false)} - /> - )} ); } diff --git a/src/utils/csvAutoDetect.test.ts b/src/utils/csvAutoDetect.test.ts index 87571ec..72fb8e6 100644 --- a/src/utils/csvAutoDetect.test.ts +++ b/src/utils/csvAutoDetect.test.ts @@ -23,7 +23,12 @@ import { preprocessQuotedCSV, } from "./csvAutoDetect"; import type { DecimalSeparator } from "./amountParser"; -import { detectAmountSeparators, mapRow } from "./importFormat"; +import { + detectAmountSeparators, + flipSignFormat, + mapRow, + summarizeParsedRows, +} from "./importFormat"; import { CSV_FIXTURE_NAMES, readCsvFixture, @@ -182,11 +187,24 @@ function mapCorpusRow( return row.parsed ?? { error: row.error! }; } -/** Full pipeline: raw file text -> signed amounts, as the wizard runs it. */ -function parseFixtureEndToEnd(name: CsvFixtureName) { +/** The shape `autoDetectConfig` returns: a format minus the encoding. */ +type DetectedConfig = NonNullable>; + +/** + * Full pipeline: raw file text -> signed amounts, as the wizard runs it. + * + * `configure` rewrites the detected configuration before anything is read, so a + * correction the user makes in the preview — the sign flip (#329) — can be + * replayed over the whole file exactly as the wizard replays it. + */ +function parseFixtureEndToEnd( + name: CsvFixtureName, + configure?: (config: DetectedConfig) => DetectedConfig +) { const rawContent = readCsvFixture(name); - const cfg = autoDetectConfig(rawContent); - if (!cfg) throw new Error(`autoDetectConfig returned null for ${name}`); + const detected = autoDetectConfig(rawContent); + if (!detected) throw new Error(`autoDetectConfig returned null for ${name}`); + const cfg = configure ? configure(detected) : detected; const format = { ...cfg, encoding: "utf-8" }; const data = Papa.parse(preprocessQuotedCSV(rawContent), { @@ -207,12 +225,36 @@ function parseFixtureEndToEnd(name: CsvFixtureName) { } /** Signed amounts only — the number that actually reaches the database. */ -function amountsOf(name: CsvFixtureName): (number | string)[] { - return parseFixtureEndToEnd(name).rows.map((r) => +function amountsOf( + name: CsvFixtureName, + configure?: (config: DetectedConfig) => DetectedConfig +): (number | string)[] { + return parseFixtureEndToEnd(name, configure).rows.map((r) => "error" in r ? r.error : r.amount ); } +/** The recap the mandatory preview step displays for a fixture (#329). */ +function recapOf( + name: CsvFixtureName, + configure?: (config: DetectedConfig) => DetectedConfig +) { + const { rows } = parseFixtureEndToEnd(name, configure); + return summarizeParsedRows( + rows.map((r, rowIndex) => + "error" in r + ? { rowIndex, raw: [], parsed: null, error: r.error } + : { rowIndex, raw: [], parsed: r } + ) + ); +} + +/** The sign flip as the preview's button applies it: onto the configuration. */ +const flipped = (config: DetectedConfig): DetectedConfig => ({ + ...config, + ...flipSignFormat(config), +}); + /** The amounts every well-detected fixture in the corpus must produce. */ const REFERENCE_AMOUNTS = [-84.32, 1250, -142.18, -56.75, 300, -6.95]; @@ -478,15 +520,19 @@ describe("autoDetectConfig — no header row (#326)", () => { }); }); -describe("autoDetectConfig — KNOWN DEFECT: all-positive amounts (#326, fixed by #329)", () => { +describe("autoDetectConfig — all-positive amounts: detected wrong, now SURFACED (#326, #329)", () => { // `detectSingleAmount` (csvAutoDetect.ts:507-530) infers the convention from // the share of negative values alone: no negatives -> `positive_expense` -> // every amount is negated at parse time. On a file that mixes expenses and // income and signs neither, the income is imported as an expense. // // Nothing in the file distinguishes the two cases, so detection cannot be - // "fixed" here. #329 surfaces it instead: a confidence score, a mandatory - // preview showing signed totals, and a "flip the signs" button. + // fixed here — the three expectations below still hold and are meant to. What + // #329 changed is that the mistake can no longer reach the database unseen: + // the preview step is traversed at every import, its recap states the two + // directions, and the flip button corrects the CONFIGURATION. The last two + // cases are that repair, and they are the reason this block no longer carries + // a KNOWN DEFECT marker. it("infers positive_expense from the absence of negatives", () => { const cfg = autoDetectConfig(readCsvFixture("all-positive"))!; @@ -497,21 +543,81 @@ describe("autoDetectConfig — KNOWN DEFECT: all-positive amounts (#326, fixed b it("negates the income rows along with the expenses", () => { expect(amountsOf("all-positive")).toEqual([ -84.32, // expense — correct - -1250, // DEFECT — payroll deposit, should be +1250 + -1250, // WRONG — payroll deposit, should be +1250 -142.18, // expense — correct -56.75, // expense — correct - -300, // DEFECT — incoming transfer, should be +300 + -300, // WRONG — incoming transfer, should be +300 -6.95, // expense — correct ]); }); - it("produces a total that is plainly wrong, which the preview will expose", () => { + it("produces a total that is plainly wrong, which the preview exposes", () => { const total = amountsOf("all-positive").reduce( (a, b) => a + (typeof b === "number" ? b : 0), 0 ); expect(total).toBeCloseTo(-1840.2, 2); // truth is +1259.80 }); + + it("shows a bank statement with SIX outflows and ZERO inflows", () => { + // The tell, and the whole reason the step is mandatory: no month of a real + // account has zero inflows. The score sees nothing here — every row reads. + const recap = recapOf("all-positive"); + expect(recap.outflowCount).toBe(6); + expect(recap.inflowCount).toBe(0); + expect(recap.inflowTotal).toBe(0); + expect(recap.outflowTotal).toBeCloseTo(-1840.2, 2); + expect(recap.errorCount).toBe(0); + }); + + it("flips to the mirror image — this file needs more than a flip", () => { + // Stated plainly, because it is the limit of the control: the file carries + // no direction at all, so NO convention can split it correctly. Flipping + // turns six outflows into six inflows and the recap stays implausible, + // which is the honest outcome — the preview tells the user the file is + // unreadable as it stands, in both directions. What the flip does repair is + // a file that is uniformly reversed (the debit/credit case below). + expect(amountsOf("all-positive", flipped)).toEqual([ + 84.32, 1250, 142.18, 56.75, 300, 6.95, + ]); + + const recap = recapOf("all-positive", flipped); + expect(recap.inflowCount).toBe(6); + expect(recap.outflowCount).toBe(0); + }); +}); + +describe("the sign flip on a whole fixture (#329)", () => { + it("reverses a correctly-read signed file, and back again", () => { + expect(amountsOf("signed-amount", flipped)).toEqual( + REFERENCE_AMOUNTS.map((a) => -a) + ); + expect( + amountsOf("signed-amount", (cfg) => flipped(flipped(cfg))) + ).toEqual(REFERENCE_AMOUNTS); + }); + + it("is NOT inert in debit/credit mode", () => { + // The review finding this branch answers: `mapRow` computes `credit - debit` + // on magnitudes and never reads the sign convention there, so a button that + // toggled the convention would leave a two-column file mapped backwards + // exactly as wrong as it found it. The columns swap instead. + const cfg = autoDetectConfig(readCsvFixture("debit-credit"))!; + expect(cfg.amountMode).toBe("debit_credit"); + expect(amountsOf("debit-credit")).toEqual(REFERENCE_AMOUNTS); + expect(amountsOf("debit-credit", flipped)).toEqual( + REFERENCE_AMOUNTS.map((a) => -a) + ); + }); + + it("leaves the recap of a well-read file plausible", () => { + const recap = recapOf("signed-amount"); + expect(recap.outflowCount).toBe(4); + expect(recap.inflowCount).toBe(2); + expect(recap.inflowTotal).toBeCloseTo(1550, 2); + expect(recap.outflowTotal).toBeCloseTo(-290.2, 2); + expect(recap.errorCount).toBe(0); + }); }); describe("preprocessQuotedCSV + autoDetectConfig — whole-line-quoted file (#326)", () => { @@ -867,14 +973,21 @@ describe("detectImportFormat — confidence score (#328)", () => { expect(scoreOf("no-header").totalRows).toBe(6); }); - it("says nothing about the SIGN of what it read (#329)", () => { - // `all-positive` is the corpus' known defect: unsigned magnitudes, detected - // as `negative_expense`, so every credit imports as an expense. Every row + it("says nothing about the SIGN of what it read — the recap does (#329)", () => { + // `all-positive` carries unsigned magnitudes, detected as + // `positive_expense`, so every credit imports as an expense. Every row // still PARSES, so the score is a perfect 100 % — which is precisely why the - // threshold blocks nothing and the preview step stays mandatory. + // threshold gates nothing and the preview step is traversed unconditionally. const score = scoreOf("all-positive"); expect(score.ratio).toBe(1); expect(score.confident).toBe(true); + + // The control that DOES see it, on the same file and the same rows: the + // signed recap of the mandatory preview step. This is no longer a promise + // written in a comment — it is the assertion below. + const recap = recapOf("all-positive"); + expect(recap.inflowCount).toBe(0); + expect(recap.outflowCount).toBe(score.readRows); }); it("carries no score on a refused format", () => { diff --git a/src/utils/importFormat.test.ts b/src/utils/importFormat.test.ts index d933426..eb1e189 100644 --- a/src/utils/importFormat.test.ts +++ b/src/utils/importFormat.test.ts @@ -6,12 +6,13 @@ // wizard restores what was saved instead of re-deriving it. import { describe, it, expect } from "vitest"; -import { readFileSync } from "node:fs"; +import { existsSync, readFileSync } from "node:fs"; import { resolve } from "node:path"; import type { ImportFormat, ImportFormatRow, ImportFormatRowInput, + ParsedRow, } from "../shared/types"; import { AMOUNT_MODES, @@ -21,11 +22,15 @@ import { SIGN_CONVENTIONS, clearMappingForMode, detectAmountSeparators, + flipSignFormat, formatFromRow, formatToRow, isRowErrorKey, mapRow, + summarizeParsedRows, } from "./importFormat"; +import fr from "../i18n/locales/fr.json"; +import en from "../i18n/locales/en.json"; // --------------------------------------------------------------------------- // Samples. Every field deliberately differs from the wizard's default config, @@ -601,3 +606,323 @@ describe("useImportWizard — the config write point (#324)", () => { expect(WIZARD_SRC.match(/await updateSource\(/g)).toHaveLength(1); }); }); + +// --------------------------------------------------------------------------- +// The signed recap and the sign flip (#329) +// --------------------------------------------------------------------------- + +/** A parsed row carrying an amount. */ +function row(rowIndex: number, amount: number): ParsedRow { + return { + rowIndex, + raw: ["01/01/2025", "LABEL", String(amount)], + parsed: { date: "2025-01-01", description: "LABEL", amount }, + }; +} + +/** A row that produced no amount at all. */ +function errorRow(rowIndex: number): ParsedRow { + return { + rowIndex, + raw: ["", "LABEL", "x"], + parsed: null, + error: ROW_ERROR_KEYS.invalidDate, + }; +} + +describe("summarizeParsedRows — the recap that reads MEANING (#329)", () => { + it("counts and totals the two directions separately", () => { + expect( + summarizeParsedRows([row(0, -84.32), row(1, 1250), row(2, -56.75)]) + ).toEqual({ + outflowCount: 2, + outflowTotal: -141.07, + inflowCount: 1, + inflowTotal: 1250, + errorCount: 0, + }); + }); + + it("keeps the totals SIGNED, as they will reach the ledger", () => { + // Magnitudes would hide the one thing the recap exists to expose: a file + // read backwards looks identical once the signs are dropped. + const totals = summarizeParsedRows([row(0, -10), row(1, 40)]); + expect(totals.outflowTotal).toBeLessThan(0); + expect(totals.inflowTotal).toBeGreaterThan(0); + }); + + it("counts rows with no parsed amount as errors, and nothing else", () => { + const totals = summarizeParsedRows([errorRow(0), row(1, -10), errorRow(2)]); + expect(totals.errorCount).toBe(2); + expect(totals.outflowCount).toBe(1); + expect(totals.inflowCount).toBe(0); + }); + + it("files a zero amount under neither direction", () => { + // Deliberate: a 0,00 row is its own anomaly. Filing it silently under + // outflows or inflows would be a small lie of the same family as the big + // one this recap exists to catch. + const totals = summarizeParsedRows([row(0, 0), row(1, -10)]); + expect(totals.outflowCount).toBe(1); + expect(totals.inflowCount).toBe(0); + expect(totals.errorCount).toBe(0); + }); + + it("returns all zeroes on an empty file", () => { + expect(summarizeParsedRows([])).toEqual({ + outflowCount: 0, + outflowTotal: 0, + inflowCount: 0, + inflowTotal: 0, + errorCount: 0, + }); + }); +}); + +describe("flipSignFormat — the correction lands on the FORMAT (#329)", () => { + const SINGLE = { + ...SAMPLE_FORMAT, + amountMode: "single" as const, + signConvention: "negative_expense" as const, + columnMapping: { date: 0, description: 1, amount: 2 }, + }; + const DEBIT_CREDIT = { + ...SAMPLE_FORMAT, + amountMode: "debit_credit" as const, + columnMapping: { date: 0, description: 1, debitAmount: 2, creditAmount: 3 }, + }; + + it("toggles the convention in single-amount mode", () => { + expect(flipSignFormat(SINGLE).signConvention).toBe("positive_expense"); + expect( + flipSignFormat({ ...SINGLE, signConvention: "positive_expense" }) + .signConvention + ).toBe("negative_expense"); + }); + + it("is its own inverse in single-amount mode", () => { + const once = { ...SINGLE, ...flipSignFormat(SINGLE) }; + expect({ ...once, ...flipSignFormat(once) }).toEqual(SINGLE); + }); + + it("leaves the mapping alone in single-amount mode", () => { + expect(flipSignFormat(SINGLE).columnMapping).toEqual(SINGLE.columnMapping); + }); + + it("swaps the two columns in debit/credit mode", () => { + // `mapRow` ignores the sign convention there — toggling it would be inert, + // which is the review finding this branch answers. + expect(flipSignFormat(DEBIT_CREDIT).columnMapping).toEqual({ + date: 0, + description: 1, + debitAmount: 3, + creditAmount: 2, + }); + }); + + it("does not touch the convention in debit/credit mode", () => { + expect(flipSignFormat(DEBIT_CREDIT).signConvention).toBe( + DEBIT_CREDIT.signConvention + ); + }); + + it("moves a half-mapped column to the other role", () => { + const half = { + ...DEBIT_CREDIT, + columnMapping: { date: 0, description: 1, debitAmount: 2 }, + }; + expect(flipSignFormat(half).columnMapping).toEqual({ + date: 0, + description: 1, + creditAmount: 2, + }); + }); + + it("never materializes an unmapped column as a key", () => { + // `mapRow` distinguishes "not mapped" from "mapped to column 0"; an + // explicit `undefined` would make that distinction unreachable. + const half = { + ...DEBIT_CREDIT, + columnMapping: { date: 0, description: 1, creditAmount: 3 }, + }; + const mapping = flipSignFormat(half).columnMapping; + expect(Object.keys(mapping)).not.toContain("creditAmount"); + expect(mapping.debitAmount).toBe(3); + }); + + it("flips the sign of every row it is applied to (single)", () => { + const raw = ["05/01/2025", "EPICERIE", "84.32"]; + const before = mapRow(raw, { ...SINGLE, dateFormat: "DD/MM/YYYY" }); + const after = mapRow(raw, { + ...SINGLE, + dateFormat: "DD/MM/YYYY", + ...flipSignFormat(SINGLE), + }); + expect(before.parsed!.amount).toBe(84.32); + expect(after.parsed!.amount).toBe(-84.32); + }); + + it("flips the sign of every row it is applied to (debit/credit)", () => { + const raw = ["05/01/2025", "EPICERIE", "84.32", ""]; + const format = { ...DEBIT_CREDIT, dateFormat: "DD/MM/YYYY" }; + expect(mapRow(raw, format).parsed!.amount).toBe(-84.32); + expect( + mapRow(raw, { ...format, ...flipSignFormat(format) }).parsed!.amount + ).toBe(84.32); + }); +}); + +// --------------------------------------------------------------------------- +// Static guards: the preview step is traversed at EVERY import (#329). +// +// The step lives in a React page and the repository has no jsdom, so the wiring +// is asserted on the source — same technique as the guards above. +// --------------------------------------------------------------------------- + +const PAGE_SRC = readFileSync( + resolve(import.meta.dirname, "..", "pages", "ImportPage.tsx"), + "utf-8" +); + +describe("the preview step is mandatory (#329)", () => { + it("has a transition that targets it", () => { + // `file-preview` was declared in `ImportWizardStep` from the beginning and + // no dispatch ever aimed at it — the optional modal had supplanted it. + expect(callbackBody("parseAndPreview", "checkDuplicatesInternal")).toContain( + 'dispatch({ type: "SET_STEP", payload: "file-preview" })' + ); + }); + + it("leaves no callback that parses straight to the duplicate check", () => { + // `parseAndCheckDuplicates` jumped from the configuration to the duplicates + // ("skips preview step"). Keeping it exported would leave a live bypass. + expect(WIZARD_SRC).not.toContain("parseAndCheckDuplicates"); + expect(WIZARD_SRC).not.toContain("skips preview step"); + }); + + it("renders the step and reaches it from the configuration", () => { + expect(PAGE_SRC).toContain('state.step === "file-preview"'); + expect(PAGE_SRC).toContain("onNext={parseAndPreview}"); + expect(PAGE_SRC).toContain("onNext={checkDuplicates}"); + }); + + it("walks back through the preview from the duplicate step", () => { + expect(PAGE_SRC).toContain('onBack={() => goToStep("file-preview")}'); + }); + + it("shows the recap over the WHOLE file, not the displayed sample", () => { + // The table truncates its rows; a recap computed on the truncation would + // state a total that is not the file's. + expect(PAGE_SRC).toContain("rows={state.parsedPreview}"); + expect(PAGE_SRC).not.toContain("state.parsedPreview.slice("); + }); + + it("no longer carries the optional preview modal", () => { + expect(PAGE_SRC).not.toContain("FilePreviewModal"); + expect( + existsSync( + resolve( + import.meta.dirname, + "..", + "components", + "import", + "FilePreviewModal.tsx" + ) + ) + ).toBe(false); + }); +}); + +describe("the sign flip is wired to the configuration (#329)", () => { + const FLIP = () => callbackBody("flipSignConvention", "executeImport"); + + it("rewrites the config and re-parses under the new one", () => { + const body = FLIP(); + expect(body).toContain("flipSignFormat(state.sourceConfig)"); + expect(body).toContain('dispatch({ type: "SET_SOURCE_CONFIG", payload: flipped })'); + // The flipped format is PASSED to the parse: `state` has not re-rendered + // yet, so re-reading it would redisplay the table the user asked to fix. + expect(body).toContain("parseFilesInternal(flipped)"); + }); + + it("drops the detection score, which never measured this format", () => { + expect(FLIP()).toContain( + 'dispatch({ type: "SET_DETECTION_SCORE", payload: null })' + ); + }); + + it("does not reload headers, which a flip cannot change", () => { + // `loadHeadersWithConfig` dispatches an empty row list; racing it against + // the re-parse is a coin toss between the corrected table and a blank one. + expect(FLIP()).not.toContain("loadHeadersWithConfig"); + }); +}); + +describe("the confirmation states what decides the amounts (#329)", () => { + const CONFIRM_SRC = readFileSync( + resolve( + import.meta.dirname, + "..", + "components", + "import", + "ImportConfirmation.tsx" + ), + "utf-8" + ); + + it("shows the amount mode and the column mapping", () => { + expect(CONFIRM_SRC).toContain("import.config.amountMode"); + expect(CONFIRM_SRC).toContain("import.config.columnMapping"); + }); + + it("shows the sign convention, in the mode that applies it", () => { + expect(CONFIRM_SRC).toContain("import.config.signConvention"); + expect(CONFIRM_SRC).toContain('config.amountMode === "single" && ('); + }); + + it("names the columns the amount mode actually reads", () => { + // Naming a debit column on a single-amount format would describe an import + // that is not happening, so the list is chosen by mode. + const mapping = CONFIRM_SRC.slice( + CONFIRM_SRC.indexOf("const mappedColumns"), + CONFIRM_SRC.indexOf("return (") + ); + expect(mapping).toContain('config.amountMode === "debit_credit"'); + for (const key of [ + "dateColumn", + "descriptionColumn", + "amountColumn", + "debitColumn", + "creditColumn", + ]) { + expect(mapping, key).toContain(`import.config.${key}`); + } + }); +}); + +describe("every new preview string exists in both languages (#329)", () => { + it("carries the recap and the flip button", () => { + for (const key of [ + "outflowCount", + "inflowCount", + "errorRows", + "flipSigns", + "flipSignsHint", + ] as const) { + expect(fr.import.preview[key].length, `fr.${key}`).toBeGreaterThan(0); + expect(en.import.preview[key].length, `en.${key}`).toBeGreaterThan(0); + } + }); + + it("interpolates the two counts in both languages", () => { + for (const key of ["outflowCount", "inflowCount"] as const) { + expect(fr.import.preview[key], `fr.${key}`).toContain("{{count}}"); + expect(en.import.preview[key], `en.${key}`).toContain("{{count}}"); + } + }); + + it("carries the unmapped-column label of the confirmation", () => { + expect(fr.import.confirm.columnUnmapped.length).toBeGreaterThan(0); + expect(en.import.confirm.columnUnmapped.length).toBeGreaterThan(0); + }); +}); diff --git a/src/utils/importFormat.ts b/src/utils/importFormat.ts index 16c7d11..696797f 100644 --- a/src/utils/importFormat.ts +++ b/src/utils/importFormat.ts @@ -352,3 +352,122 @@ export function detectAmountSeparators( } return result; } + +// --------------------------------------------------------------------------- +// The signed recap and the sign flip (#329) +// --------------------------------------------------------------------------- + +/** + * What a whole parsed file amounts to, before a single row is written. + * + * This is the chantier's last control, and the only one that looks at MEANING. + * A confidence score reports how many rows were read, never what they say: the + * `all-positive` fixture scores a perfect 100 % while every credit imports as + * an expense (#328), because each of those rows is perfectly readable. A + * statement whose recap shows six outflows and zero inflows is wrong on its + * face, whatever produced it — a bad detection, a stale template, a bank that + * changed its export. + * + * Totals are SIGNED, exactly as the amounts that would reach the ledger: + * `outflowTotal` is negative or zero, `inflowTotal` positive or zero. Showing + * magnitudes would hide the one thing the recap exists to expose. + * + * A row whose amount is exactly zero is neither an outflow nor an inflow, so + * the two counts do not have to add up to the row count displayed beside them. + * That is deliberate: a zero amount is its own anomaly and filing it silently + * under one direction would be a small lie of the same family as the big one. + */ +export interface PreviewTotals { + outflowCount: number; + /** Negative or zero. */ + outflowTotal: number; + inflowCount: number; + /** Positive or zero. */ + inflowTotal: number; + /** Rows that produced no amount at all — they will not be imported. */ + errorCount: number; +} + +/** Add up a parsed file into the recap the preview step displays. PURE. */ +export function summarizeParsedRows( + rows: readonly ParsedRow[] +): PreviewTotals { + const totals: PreviewTotals = { + outflowCount: 0, + outflowTotal: 0, + inflowCount: 0, + inflowTotal: 0, + errorCount: 0, + }; + + for (const row of rows) { + // No parsed amount is exactly the set of rows carrying an `error`; counting + // the absence of a number rather than the presence of a message keeps this + // count and the amounts it sits next to reading the same rows. + if (!row.parsed) { + totals.errorCount++; + continue; + } + const amount = row.parsed.amount; + if (amount < 0) { + totals.outflowCount++; + totals.outflowTotal += amount; + } else if (amount > 0) { + totals.inflowCount++; + totals.inflowTotal += amount; + } + } + + return totals; +} + +/** + * Flip the direction a file's amounts are read in. + * + * It acts on the FORMAT, never on the parsed rows: the correction has to be + * memorised with the source, so the next import of the same bank reads right on + * its own. Flipping the rows alone would repair one import and let the next one + * reintroduce the same reversal. + * + * The operation differs by mode, and getting that wrong makes the button inert: + * - `single` — the sign convention is what decides, so it toggles. + * - `debit_credit` — `mapRow` computes `credit - debit` on magnitudes and + * never reads the sign convention there (#325); the direction is carried by + * WHICH column holds which role. Toggling the convention would change + * nothing at all, so the two column indices swap instead. + * + * A half-mapped debit/credit format swaps too: moving the single mapped column + * to the other role is precisely the correction a file whose one amount column + * was taken for the wrong side needs. Unmapped stays unmapped — `mapRow` + * distinguishes "not mapped" from "mapped to column 0", so the key must not + * materialize as `undefined`. + * + * It returns the two fields a flip decides rather than a whole format, so the + * caller spreads it onto whatever carries them (`SourceConfig` in the wizard, + * a detected configuration in the tests) and keeps its own extra fields. Both + * fields always come back, so the pair can never be applied half way. + */ +export function flipSignFormat( + format: Readonly< + Pick + > +): Pick { + if (format.amountMode === "debit_credit") { + const { debitAmount, creditAmount, ...rest } = format.columnMapping; + return { + signConvention: format.signConvention, + columnMapping: { + ...rest, + ...(creditAmount !== undefined ? { debitAmount: creditAmount } : {}), + ...(debitAmount !== undefined ? { creditAmount: debitAmount } : {}), + }, + }; + } + return { + columnMapping: format.columnMapping, + signConvention: + format.signConvention === "negative_expense" + ? "positive_expense" + : "negative_expense", + }; +}