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", + }; +}