diff --git a/src/components/import/FilePreviewTable.tsx b/src/components/import/FilePreviewTable.tsx index 05a83c7..444516e 100644 --- a/src/components/import/FilePreviewTable.tsx +++ b/src/components/import/FilePreviewTable.tsx @@ -1,6 +1,7 @@ import { useTranslation } from "react-i18next"; import { AlertCircle } from "lucide-react"; import type { ParsedRow } from "../../shared/types"; +import { isRowErrorKey } from "../../utils/importFormat"; interface FilePreviewTableProps { rows: ParsedRow[]; @@ -21,6 +22,10 @@ export default function FilePreviewTable({ const errorCount = rows.filter((r) => r.error).length; + // Row errors are i18n keys (#325); anything else reaches us verbatim. + const errorText = (error: string) => + isRowErrorKey(error) ? t(error) : error; + return (
@@ -77,7 +82,7 @@ export default function FilePreviewTable({ {row.parsed?.date || ( - {row.error || "—"} + {row.error ? errorText(row.error) : "—"} )} diff --git a/src/components/import/ImportReportPanel.tsx b/src/components/import/ImportReportPanel.tsx index e20d505..169bd8c 100644 --- a/src/components/import/ImportReportPanel.tsx +++ b/src/components/import/ImportReportPanel.tsx @@ -7,6 +7,7 @@ import { FileText, } from "lucide-react"; import type { ImportReport } from "../../shared/types"; +import { isRowErrorKey } from "../../utils/importFormat"; interface ImportReportPanelProps { report: ImportReport; @@ -104,7 +105,11 @@ export default function ImportReportPanel({ {report.errors.map((err, i) => ( {err.rowIndex + 1} - {err.message} + + {/* Row errors are i18n keys (#325); an exception message + that reached this list is NOT one and stays verbatim. */} + {isRowErrorKey(err.message) ? t(err.message) : err.message} + ))} diff --git a/src/hooks/useImportWizard.ts b/src/hooks/useImportWizard.ts index 15e8059..79a207f 100644 --- a/src/hooks/useImportWizard.ts +++ b/src/hooks/useImportWizard.ts @@ -39,16 +39,17 @@ import { updateTemplate, deleteTemplate as deleteTemplateService, } from "../services/importConfigTemplateService"; -import { parseDate } from "../utils/dateParser"; -import { parseFrenchAmount } from "../utils/amountParser"; import { preprocessQuotedCSV, autoDetectConfig as runAutoDetect, } from "../utils/csvAutoDetect"; import { + detectAmountSeparators, formatFromRow, formatToRow, ImportFormatError, + mapRow, + ROW_ERROR_KEYS, } from "../utils/importFormat"; /** Error text for the banner: an i18n key when we have one, the raw message otherwise. */ @@ -510,66 +511,32 @@ export function useImportWizard() { headers = firstDataRow.map((_, i) => `Col ${i}`); } + // Data rows of THIS file, kept aside so the decimal separator of each + // amount column is arbitrated over the whole column before any row is + // read. `1.234` alone is ambiguous; the column is not. + const dataRows: string[][] = []; for (let i = startIdx; i < data.length; i++) { const raw = data[i]; if (raw.length <= 1 && raw[0]?.trim() === "") continue; + dataRows.push(raw); + } + const decimalSeparators = detectAmountSeparators(dataRows, config); + for (const raw of dataRows) { try { - const date = parseDate( - raw[config.columnMapping.date]?.trim() || "", - config.dateFormat + allRows.push( + mapRow(raw, config, { + rowIndex: allRows.length, + sourceFilename: file.filename, + decimalSeparators, + }) ); - const description = - raw[config.columnMapping.description]?.trim() || ""; - - let amount: number; - if (config.amountMode === "debit_credit") { - const debit = parseFrenchAmount( - raw[config.columnMapping.debitAmount ?? 0] || "" - ); - const credit = parseFrenchAmount( - raw[config.columnMapping.creditAmount ?? 0] || "" - ); - amount = isNaN(credit) ? -(isNaN(debit) ? 0 : debit) : credit; - } else { - amount = parseFrenchAmount( - raw[config.columnMapping.amount ?? 0] || "" - ); - if (config.signConvention === "positive_expense" && !isNaN(amount)) { - amount = -amount; - } - } - - if (!date) { - allRows.push({ - rowIndex: allRows.length, - raw, - parsed: null, - error: "Invalid date", - sourceFilename: file.filename, - }); - } else if (isNaN(amount)) { - allRows.push({ - rowIndex: allRows.length, - raw, - parsed: null, - error: "Invalid amount", - sourceFilename: file.filename, - }); - } else { - allRows.push({ - rowIndex: allRows.length, - raw, - parsed: { date, description, amount }, - sourceFilename: file.filename, - }); - } } catch { allRows.push({ rowIndex: allRows.length, raw, parsed: null, - error: "Parse error", + error: ROW_ERROR_KEYS.parseError, sourceFilename: file.filename, }); } @@ -842,7 +809,10 @@ export function useImportWizard() { // Count errors from parsing const parseErrors = state.parsedPreview.filter((r) => r.error); for (const err of parseErrors) { - errors.push({ rowIndex: err.rowIndex, message: err.error || "Parse error" }); + errors.push({ + rowIndex: err.rowIndex, + message: err.error || ROW_ERROR_KEYS.parseError, + }); } const report: ImportReport = { diff --git a/src/hooks/useSnapshotEditor.ts b/src/hooks/useSnapshotEditor.ts index 67cb5e6..ac2887f 100644 --- a/src/hooks/useSnapshotEditor.ts +++ b/src/hooks/useSnapshotEditor.ts @@ -161,7 +161,9 @@ export function holdingsFromServiceHoldings( * column indices from `analyzeHoldingsCsv`. Behavior: * - Symbols are normalized (UPPER/TRIM) like manual entry (SecurityPicker) so * an imported title collapses onto the same `balance_securities` row. - * - Numbers are parsed with `parseFrenchAmount` (handles `1 234,56`, `1,234.56`). + * - Numbers are parsed with `parseFrenchAmount` (handles `1 234,56`, `1,234.56`, + * `(140,10)`); a cell it cannot read is left EMPTY, or kept verbatim for the + * quantity, so validation refuses the row instead of storing a wrong number. * - unit_price + book_cost are OPTIONAL: when the mapping's column is null (no * price/cost column) the field stays empty; the user fetches/types it later. * - Duplicate symbols WITHIN the CSV are merged into one draft to respect the @@ -179,7 +181,13 @@ export function holdingsFromCsvRows( const order: string[] = []; const bySymbol = new Map< string, - { symbol: string; qty: number; book: number | null; price: string } + { + symbol: string; + qty: number | null; + qtyRaw: string; + book: number | null; + price: string; + } >(); for (const row of rows) { @@ -188,8 +196,14 @@ export function holdingsFromCsvRows( const symbol = normalizeSecuritySymbol(rawSymbol); if (!symbol) continue; - const qtyParsed = parseFrenchAmount((row[mapping.quantity] ?? "").trim()); - const qty = isNaN(qtyParsed) ? 0 : qtyParsed; + // An unreadable quantity used to be coerced to 0, which SAVED a + // zero-value position without a word (#325). It stays `null` now and the + // draft keeps the offending text, so `buildDetailedLines` raises the + // existing `snapshot_priced_quantity_required` and the user sees the cell + // to correct. + const qtyRaw = (row[mapping.quantity] ?? "").trim(); + const qtyParsed = parseFrenchAmount(qtyRaw); + const qty = isNaN(qtyParsed) ? null : qtyParsed; let price = ""; if (mapping.unit_price !== null) { @@ -205,12 +219,25 @@ export function holdingsFromCsvRows( const existing = bySymbol.get(symbol); if (existing) { - existing.qty += qty; + // One unreadable lot taints the merged quantity: summing it as 0 would + // hide the bad cell behind a plausible total. + if (existing.qty === null || qty === null) { + existing.qty = null; + if (!existing.qtyRaw) existing.qtyRaw = qtyRaw; + } else { + existing.qty += qty; + } if (book !== null) existing.book = (existing.book ?? 0) + book; if (!existing.price && price) existing.price = price; // first non-empty } else { order.push(symbol); - bySymbol.set(symbol, { symbol, qty, book, price }); + bySymbol.set(symbol, { + symbol, + qty, + qtyRaw: qty === null ? qtyRaw : "", + book, + price, + }); } } @@ -219,7 +246,7 @@ export function holdingsFromCsvRows( return { ...makeEmptyHolding(defaultAssetType), symbol: a.symbol, - quantity: String(a.qty), + quantity: a.qty !== null ? String(a.qty) : a.qtyRaw, unit_price: a.price, book_cost: a.book !== null ? String(a.book) : "", }; diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index fe4e08c..cc74728 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -191,6 +191,12 @@ "unsupportedSignConvention": "The sign convention saved for this source is not recognized. Reconfigure the source before importing.", "invalidColumnMapping": "The column mapping saved for this source cannot be read. Reconfigure the source before importing." }, + "rowErrors": { + "invalidDate": "Unreadable date", + "invalidAmount": "Unreadable amount", + "amountColumnNotMapped": "Amount column not mapped", + "parseError": "Unreadable row" + }, "help": { "title": "How to import bank statements", "tips": [ diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index 2e097d0..4aa1c97 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -191,6 +191,12 @@ "unsupportedSignConvention": "La convention de signe enregistrée pour cette source n'est pas reconnue. Reconfigurez la source avant d'importer.", "invalidColumnMapping": "Le mapping de colonnes enregistré pour cette source est illisible. Reconfigurez la source avant d'importer." }, + "rowErrors": { + "invalidDate": "Date illisible", + "invalidAmount": "Montant illisible", + "amountColumnNotMapped": "Colonne de montant non mappée", + "parseError": "Ligne illisible" + }, "help": { "title": "Comment importer des relevés bancaires", "tips": [ diff --git a/src/utils/amountParser.test.ts b/src/utils/amountParser.test.ts index 1132c01..983e8e4 100644 --- a/src/utils/amountParser.test.ts +++ b/src/utils/amountParser.test.ts @@ -1,34 +1,27 @@ -// amountParser — characterization tests (issue #326). +// amountParser — characterization tests (#326), hardened (#325). // // `parseFrenchAmount` had no test at all, on a codebase of 871. It is called // from 11 sites (8 in `csvAutoDetect.ts`, 3 in `useSnapshotEditor.ts`) and sits -// under every imported amount, so this file pins what it does TODAY, before -// issue #325 hardens it. +// under every imported amount, so #326 pinned what it did before #325 touched +// it. The `KNOWN DEFECT` blocks #326 left here have since flipped: the +// expectations were updated in place and the markers dropped, per the standing +// rule (update, never delete). // -// ┌── HOW TO READ THIS FILE ─────────────────────────────────────────────────┐ -// │ Two kinds of test live here, and the difference is deliberate: │ -// │ │ -// │ `describe("… contract")` — behaviour that must SURVIVE the rewrite. │ -// │ Breaking one of these is a regression. │ -// │ │ -// │ `describe("… KNOWN DEFECT")` — behaviour that is WRONG today and is │ -// │ expected to change in #325. Every such │ -// │ assertion states the right answer in a │ -// │ comment. When #325 lands, these tests are │ -// │ meant to fail; the fix is to update the │ -// │ expectation, not to delete the test. │ -// └──────────────────────────────────────────────────────────────────────────┘ -// -// The defect that motivated the whole chantier: `parseFrenchAmount` ends on +// The defect that motivated the whole chantier: `parseFrenchAmount` ended on // `parseFloat`, which stops at the first invalid character instead of rejecting -// the string. A trailing unit or sign therefore yields a magnitude that is off -// by a factor of 100 — and it passes `isNaN`, so it is counted as a VALID row -// everywhere downstream. `"100,00 CAD"` does not fail; it imports as 10 000. +// the string. A trailing unit or sign therefore yielded a magnitude off by a +// factor of 100 — and it passed `isNaN`, so it counted as a VALID row +// everywhere downstream. `"100,00 CAD"` did not fail; it imported as 10 000. +// Validation is anchored now and such a cell is NaN, i.e. a visible row error. import { describe, it, expect } from "vitest"; -import { parseFrenchAmount } from "./amountParser"; +import { detectDecimalSeparator, parseFrenchAmount } from "./amountParser"; import { autoDetectConfig } from "./csvAutoDetect"; -import { holdingsFromCsvRows } from "../hooks/useSnapshotEditor"; +import { + buildDetailedLines, + holdingsFromCsvRows, +} from "../hooks/useSnapshotEditor"; +import { BalanceServiceError } from "../services/balance.service"; import { readCsvFixture } from "../__fixtures__/csv"; describe("parseFrenchAmount — separator contract (#326)", () => { @@ -76,99 +69,141 @@ describe("parseFrenchAmount — separator contract (#326)", () => { }); }); -describe("parseFrenchAmount — KNOWN DEFECT: parseFloat prefix scan (#326, fixed by #325)", () => { - // `parseFloat` returns the longest valid PREFIX instead of rejecting the - // whole string. Every expectation below is the bug, not the intent. +describe("parseFrenchAmount — anchored validation (#325, was a KNOWN DEFECT of #326)", () => { + // FIXED. `parseFloat` used to return the longest valid PREFIX instead of + // rejecting the string; validation is anchored over the whole normalized + // value now, so any residual character yields NaN. // - // #325 must anchor the validation (regex over the whole normalized string) - // and return NaN on any residual character. All of these then become NaN, - // except the accounting-parenthesis and trailing-sign forms, which #325 - // explicitly adds support for. + // Note on the two currency/indicator cases: link 1 wrote "should be 1234.56" + // and "should be 100" in the titles, while the block header it wrote just + // above said "all of these then become NaN, except the accounting-parenthesis + // and trailing-sign forms". The header is what #325 implements, for two + // reasons the titles missed. `CR`/`DB` carry a DIRECTION, so returning a + // magnitude for both would replace a loud failure with a silent SIGN error — + // and the D/C-indicator shape is refused upstream by design (#328). And + // "100,00 CAD" is structurally identical to "2025 Montant": rescuing the + // first by whitelisting a trailing word re-blinds `detectHeader` on the + // second, which is the very defect measured below. - it("returns a x100 magnitude on a trailing sign — should be -50", () => { + it("reads a trailing sign as a negative amount", () => { // "50,00-" is the trailing-minus convention of several bank exports. - // The comma stops looking like a decimal separator once "-" trails it - // (the /,\d{1,2}$/ probe fails), so the comma is dropped as a thousands - // separator and "5000-" parses as 5000. - expect(parseFrenchAmount("50,00-")).toBe(5000); + expect(parseFrenchAmount("50,00-")).toBe(-50); + expect(parseFrenchAmount("1 234,56-")).toBe(-1234.56); + expect(parseFrenchAmount("50,00+")).toBe(50); }); - it("returns a x100 magnitude on a trailing indicator — should be 1234.56", () => { - expect(parseFrenchAmount("1 234,56 CR")).toBe(123456); - expect(parseFrenchAmount("1 234,56 DB")).toBe(123456); + it("rejects a trailing direction indicator instead of guessing a sign", () => { + expect(parseFrenchAmount("1 234,56 CR")).toBeNaN(); + expect(parseFrenchAmount("1 234,56 DB")).toBeNaN(); }); - it("returns a x100 magnitude on a trailing currency code — should be 100", () => { - expect(parseFrenchAmount("100,00 CAD")).toBe(10000); - expect(parseFrenchAmount("84,32 USD")).toBe(8432); + it("rejects a trailing currency code", () => { + expect(parseFrenchAmount("100,00 CAD")).toBeNaN(); + expect(parseFrenchAmount("84,32 USD")).toBeNaN(); }); - it("accepts a numeric prefix of a text label — should be NaN", () => { - // This is what blinds `detectHeader`: a header cell that STARTS with - // digits reads as a number, so the header row is taken for data. - expect(parseFrenchAmount("2024 Montant")).toBe(2024); - expect(parseFrenchAmount("5%")).toBe(5); + it("rejects the numeric prefix of a text label", () => { + // This is what used to blind `detectHeader`: a header cell STARTING with + // digits read as a number, so the header row was taken for data. + expect(parseFrenchAmount("2024 Montant")).toBeNaN(); + expect(parseFrenchAmount("5%")).toBeNaN(); }); - it("accepts JavaScript number literals a bank never emits — should be NaN", () => { - expect(parseFrenchAmount("1e3")).toBe(1000); - expect(parseFrenchAmount("Infinity")).toBe(Infinity); + it("rejects JavaScript number literals a bank never emits", () => { + expect(parseFrenchAmount("1e3")).toBeNaN(); + expect(parseFrenchAmount("Infinity")).toBeNaN(); + expect(parseFrenchAmount("0x1F")).toBeNaN(); }); - it("keeps only the first two groups of a malformed number — should be NaN", () => { - expect(parseFrenchAmount("1,2,3")).toBe(1.2); + it("rejects a malformed number instead of keeping its first two groups", () => { + expect(parseFrenchAmount("1,2,3")).toBeNaN(); + expect(parseFrenchAmount("1.2.3")).toBeNaN(); + expect(parseFrenchAmount("1,23,456")).toBeNaN(); // groups must be 3 digits + }); + + it("rejects two signs, wherever they sit", () => { + expect(parseFrenchAmount("-50,00-")).toBeNaN(); + expect(parseFrenchAmount("(-50,00)")).toBeNaN(); }); }); -describe("parseFrenchAmount — KNOWN DEFECT: unsupported accounting forms (#326, fixed by #325)", () => { - it("rejects accounting parentheses — should be -50", () => { - // "(50,00)" is the standard accounting notation for a negative amount. - // Today it is NaN, so the row is dropped as "Invalid amount" rather than - // being imported with the wrong sign — a loud failure, unlike the ones - // above, but still a failure. - expect(parseFrenchAmount("(50,00)")).toBeNaN(); - expect(parseFrenchAmount("(1 234,56)")).toBeNaN(); +describe("parseFrenchAmount — accounting forms (#325, was a KNOWN DEFECT of #326)", () => { + it("reads accounting parentheses as a negative amount", () => { + expect(parseFrenchAmount("(50,00)")).toBe(-50); + expect(parseFrenchAmount("(1 234,56)")).toBe(-1234.56); + expect(parseFrenchAmount("(1,234.56)")).toBe(-1234.56); + }); + + it("still rejects an unbalanced parenthesis", () => { + expect(parseFrenchAmount("(50,00")).toBeNaN(); + expect(parseFrenchAmount("50,00)")).toBeNaN(); }); }); -describe("parseFrenchAmount — KNOWN DEFECT: separator arbitrated per cell (#326, fixed by #325)", () => { - // The French-vs-English decision is taken on each cell in isolation - // (`/,\d{1,2}$/`), never at column level. Two cells of the SAME column can - // therefore be read under two different conventions. - // - // #325 arbitrates the separator per column, which resolves both cases. +describe("parseFrenchAmount — column-level arbitration (#325, was a KNOWN DEFECT of #326)", () => { + // The French-vs-English decision used to be taken on each cell in isolation + // and NOTHING else, so two cells of the same column could be read under two + // different conventions. The isolated readings below are unchanged — they + // are the best a lone cell allows — but a caller that knows the column now + // passes its verdict and settles the ambiguity. - it("reads a 3-digit group after the comma as a thousands separator", () => { + it("keeps the documented reading when no column context is given", () => { // "12,345" is 12.345 in a column of decimals, 12345 in a column of - // thousands. Nothing in the cell alone can tell. + // thousands. Nothing in the cell ALONE can tell. expect(parseFrenchAmount("12,345")).toBe(12345); - }); - - it("reads a dot as a decimal separator when no comma is present", () => { - // In a French column, "1.234" means 1234. Read alone it yields 1.234 — - // a x1000 error in the opposite direction from the cases above. expect(parseFrenchAmount("1.234")).toBe(1.234); expect(parseFrenchAmount("1.234,56")).toBe(1234.56); // ...unless a comma follows }); + + it("obeys the column verdict when there is one", () => { + expect(parseFrenchAmount("12,345", { decimalSeparator: "," })).toBe(12.345); + expect(parseFrenchAmount("12,345", { decimalSeparator: "." })).toBe(12345); + expect(parseFrenchAmount("1.234", { decimalSeparator: "," })).toBe(1234); + expect(parseFrenchAmount("1.234", { decimalSeparator: "." })).toBe(1.234); + }); + + it("still rejects a value that contradicts the column verdict", () => { + // A grouping separator groups by three, always. + expect(parseFrenchAmount("1.23", { decimalSeparator: "," })).toBeNaN(); + expect(parseFrenchAmount("1,23", { decimalSeparator: "." })).toBeNaN(); + }); + + it("arbitrates a column from a decisive sibling cell", () => { + // "1.234" alone is ambiguous; "84,32" in the same column is not. + expect(detectDecimalSeparator(["1.234", "84,32", "-6,95"])).toBe(","); + expect(detectDecimalSeparator(["1,234", "84.32", "-6.95"])).toBe("."); + // Mixed notation settles on the last separator of each decisive cell. + expect(detectDecimalSeparator(["1.234,56"])).toBe(","); + expect(detectDecimalSeparator(["1,234.56"])).toBe("."); + }); + + it("returns no verdict when the column gives no evidence", () => { + expect(detectDecimalSeparator([])).toBeUndefined(); + expect(detectDecimalSeparator(["1.234", "5.678"])).toBeUndefined(); + expect(detectDecimalSeparator(["", " ", "N/A"])).toBeUndefined(); + // One vote each way is a tie, not a majority. + expect(detectDecimalSeparator(["84,32", "84.32"])).toBeUndefined(); + }); }); -describe("parseFrenchAmount — call-site fallout (#326)", () => { +describe("parseFrenchAmount — call-site fallout (#326, hardened by #325)", () => { // The /review-spec revision of #326 asks the corpus to reach the holdings // call sites too, because #325 hardens the parser GLOBALLY. These pin what // the shared parser does to its two most exposed consumers. - it("blinds detectHeader when a header cell starts with digits", () => { + it("no longer blinds detectHeader when a header cell starts with digits", () => { // `detectHeader` (csvAutoDetect.ts:224) treats "parses as a number" as - // proof of a data row. "2025 Montant" parses as 2025, so the header is - // taken for data. + // proof of a data row. "2025 Montant" used to parse as 2025, so the header + // was taken for data; anchoring makes it NaN and the header is recognised. const cfg = autoDetectConfig(readCsvFixture("header-numeric-label"))!; - expect(cfg.hasHeader).toBe(false); // DEFECT — should be true + expect(cfg.hasHeader).toBe(true); }); - it("leaks the x100 magnitude into the holdings CSV import (#245)", () => { + it("no longer leaks a x100 magnitude into the holdings CSV import (#245)", () => { // `holdingsFromCsvRows` (useSnapshotEditor.ts:191-202) shares the parser. - // A price column carrying its currency code silently multiplies every - // position by 100 — a $150.25 share is stored at $15,025. + // A price column carrying its currency code used to multiply every + // position by 100 — a $150.25 share stored at $15,025. It is refused now, + // and an empty price is what `buildDetailedLines` rejects on save. const drafts = holdingsFromCsvRows( [ ["AAPL", "10", "150,25 CAD", "1 200,00"], @@ -176,19 +211,43 @@ describe("parseFrenchAmount — call-site fallout (#326)", () => { ], { symbol: 0, quantity: 1, unit_price: 2, book_cost: 3 } ); - expect(drafts[0].unit_price).toBe("15025"); // DEFECT — should be "150.25" - expect(drafts[0].book_cost).toBe("1200"); // clean cell, correct today - expect(drafts[1].unit_price).toBe("300.5"); // clean cell, correct today - expect(drafts[1].book_cost).toBe("140000"); // DEFECT — should be "1400" + expect(drafts[0].unit_price).toBe(""); // refused, not 15025 + expect(drafts[0].book_cost).toBe("1200"); // clean cell + expect(drafts[1].unit_price).toBe("300.5"); // clean cell + expect(drafts[1].book_cost).toBe(""); // refused, not 140000 }); - it("drops an accounting-parenthesis price instead of reading it", () => { + it("reads an accounting-parenthesis price instead of dropping it", () => { const drafts = holdingsFromCsvRows( [["GOOG", "2", "(140,10)", "280,20"]], { symbol: 0, quantity: 1, unit_price: 2, book_cost: 3 } ); - // NaN price is swallowed to an empty string — no error surfaces. - expect(drafts[0].unit_price).toBe(""); // DEFECT — should be "-140.10" + expect(drafts[0].unit_price).toBe("-140.1"); expect(drafts[0].quantity).toBe("2"); }); + + it("keeps an unreadable quantity verbatim so the save refuses it", () => { + // It used to be coerced to 0, which SAVED a zero-value position in + // silence. `buildDetailedLines` throws on the raw text instead. + const drafts = holdingsFromCsvRows( + [["GOOG", "2 parts", "140,10", "280,20"]], + { symbol: 0, quantity: 1, unit_price: 2, book_cost: 3 } + ); + expect(drafts[0].quantity).toBe("2 parts"); + expect(() => + buildDetailedLines({ 7: drafts }, new Set([7])) + ).toThrowError(BalanceServiceError); + }); + + it("taints the merged quantity when one lot of a symbol is unreadable", () => { + const drafts = holdingsFromCsvRows( + [ + ["AAPL", "6", "150,00", "700"], + ["AAPL", "quatre", "151,00", "500"], + ], + { symbol: 0, quantity: 1, unit_price: 2, book_cost: 3 } + ); + expect(drafts).toHaveLength(1); + expect(drafts[0].quantity).toBe("quatre"); // NOT "6" + }); }); diff --git a/src/utils/amountParser.ts b/src/utils/amountParser.ts index bdd79c4..91b6c81 100644 --- a/src/utils/amountParser.ts +++ b/src/utils/amountParser.ts @@ -1,26 +1,192 @@ /** - * Parse a French-formatted amount string to a number. - * Handles formats like: 1.234,56 / 1234,56 / -1 234.56 / 1 234,56 + * Amount parsing for imported files (#325). + * + * The function used to end on `parseFloat`, which returns the longest valid + * PREFIX of a string instead of rejecting it. `"100,00 CAD"` therefore came out + * as 10 000 and `"1 234,56 CR"` as 123 456 — a factor-100 error that passes + * `isNaN`, so the row counted as VALID everywhere downstream. Validation is + * ANCHORED now: after normalisation the whole remaining string must match a + * numeric grammar, and any residual character yields `NaN`. + * + * `NaN` is the only safe answer for a cell we cannot read with certainty. A + * trailing `CR` / `DB` carries a DIRECTION, not noise, so guessing a magnitude + * for it would trade a loud failure for a silent sign error — the exact bug + * class this chantier exists to remove. The absolute-amount + D/C-indicator + * shape is refused, by design, upstream (#328). + * + * Two accounting forms ARE legitimate and supported: parentheses `(50,00)` and + * a trailing sign `50,00-`, both meaning a negative amount. + * + * Separator arbitration: `1.234` means 1234 in a French column and 1.234 in an + * English one, and nothing INSIDE the cell can tell. Callers that know the + * column pass `decimalSeparator` (see `detectDecimalSeparator`); callers that + * do not get the documented per-cell default. */ -export function parseFrenchAmount(raw: string): number { + +/** Which character separates the decimals in a given column. */ +export type DecimalSeparator = "," | "."; + +export interface AmountParseOptions { + /** + * Decimal separator arbitrated at COLUMN level. When set, the other character + * is read as a grouping separator, whatever a single cell looks like. + */ + decimalSeparator?: DecimalSeparator; +} + +/** Currency symbols and every flavour of space are noise, never data. */ +const NOISE = /[€$£\s\u00A0]/g; + +/** Digits, optionally grouped in threes by `sep`. Anchored. */ +function groupedRe(sep: "," | "."): RegExp { + const s = sep === "." ? "\\." : ","; + return new RegExp(`^\\d{1,3}(?:${s}\\d{3})+$`); +} + +/** Digits, one `sep`, then decimals. Anchored. `limit` caps the decimal count. */ +function decimalRe(sep: "," | ".", limit?: number): RegExp { + const s = sep === "." ? "\\." : ","; + const tail = limit === undefined ? "+" : `{1,${limit}}`; + return new RegExp(`^\\d+${s}\\d${tail}$`); +} + +/** Grouped digits then decimals, e.g. `1.234,56`. Anchored. */ +function groupedDecimalRe(group: "," | ".", dec: "," | "."): RegExp { + const g = group === "." ? "\\." : ","; + const d = dec === "." ? "\\." : ","; + return new RegExp(`^\\d{1,3}(?:${g}\\d{3})+${d}\\d+$`); +} + +/** + * Read a fully normalised body (digits plus `.` and `,` only, no sign, no + * spaces) under a known decimal separator. Returns `NaN` when the body does not + * match the grammar exactly. + */ +function readWithSeparator(body: string, decimal: DecimalSeparator): number { + const group: DecimalSeparator = decimal === "," ? "." : ","; + const hasDecimal = body.includes(decimal); + const hasGroup = body.includes(group); + + let normalized: string | null = null; + if (hasDecimal && hasGroup) { + if (groupedDecimalRe(group, decimal).test(body)) normalized = body; + } else if (hasDecimal) { + if (decimalRe(decimal).test(body)) normalized = body; + } else if (hasGroup) { + if (groupedRe(group).test(body)) normalized = body; + } else if (/^\d+$/.test(body)) { + normalized = body; + } + if (normalized === null) return NaN; + + const stripped = normalized.split(group).join(""); + return Number(decimal === "," ? stripped.replace(",", ".") : stripped); +} + +/** + * Read a body with no column context. The rules reproduce the documented + * per-cell behaviour, now anchored: + * - both separators present -> the LAST one is the decimal; + * - a single `,` followed by one or two digits -> French decimal; + * - a single `.` -> English decimal (`1.234` reads as 1.234 alone); + * - otherwise the separator must group digits in perfect threes. + */ +function readPerCell(body: string): number { + const lastComma = body.lastIndexOf(","); + const lastDot = body.lastIndexOf("."); + + if (lastComma >= 0 && lastDot >= 0) { + return readWithSeparator(body, lastComma > lastDot ? "," : "."); + } + if (lastComma >= 0) { + if (decimalRe(",", 2).test(body)) return readWithSeparator(body, ","); + return groupedRe(",").test(body) ? readWithSeparator(body, ".") : NaN; + } + if (lastDot >= 0) { + if (decimalRe(".").test(body)) return readWithSeparator(body, "."); + return groupedRe(".").test(body) ? readWithSeparator(body, ",") : NaN; + } + return /^\d+$/.test(body) ? Number(body) : NaN; +} + +/** + * Parse an amount cell to a number, or `NaN` when it cannot be read with + * certainty. Handles `1.234,56`, `1 234,56`, `1,234.56`, `-84,32`, `(50,00)` + * and `50,00-`. + */ +export function parseFrenchAmount( + raw: string, + options: AmountParseOptions = {} +): number { if (!raw || typeof raw !== "string") return NaN; let cleaned = raw.trim(); + let negative = false; - // Remove currency symbols and whitespace - cleaned = cleaned.replace(/[€$£\s\u00A0]/g, ""); - - // Detect if comma is decimal separator (French style) - // Pattern: digits followed by comma followed by exactly 1-2 digits at end - const frenchPattern = /,\d{1,2}$/; - if (frenchPattern.test(cleaned)) { - // French format: remove dots (thousand sep), replace comma with dot (decimal) - cleaned = cleaned.replace(/\./g, "").replace(",", "."); - } else { - // English format or no decimal: remove commas (thousand sep) - cleaned = cleaned.replace(/,/g, ""); + // Accounting parentheses. Only a balanced, outermost pair counts; `(50,00` + // falls through and fails the anchored grammar below. + const parens = /^\((.*)\)$/.exec(cleaned); + if (parens) { + negative = true; + cleaned = parens[1].trim(); } - const result = parseFloat(cleaned); - return isNaN(result) ? NaN : result; + cleaned = cleaned.replace(NOISE, ""); + if (!cleaned) return NaN; + + // One sign, leading or trailing, never both, never inside parentheses. + const leading = /^[+-]/.test(cleaned); + const trailing = /[+-]$/.test(cleaned); + if (leading && trailing) return NaN; + if (leading || trailing) { + if (negative) return NaN; // `(-50,00)` is not a form any bank emits + negative = cleaned[leading ? 0 : cleaned.length - 1] === "-"; + cleaned = leading ? cleaned.slice(1) : cleaned.slice(0, -1); + } + + // Anchored gate: nothing but digits and separators may remain. This is what + // rejects `2024Montant`, `1e3`, `Infinity`, `5%` and `100,00CAD`. + if (!/^[\d.,]+$/.test(cleaned) || !/\d/.test(cleaned)) return NaN; + + const value = options.decimalSeparator + ? readWithSeparator(cleaned, options.decimalSeparator) + : readPerCell(cleaned); + + if (!Number.isFinite(value)) return NaN; + return negative ? -value : value; +} + +/** + * Arbitrate the decimal separator of a COLUMN from all of its cells. + * + * A cell is decisive when it carries both separators (the last one is the + * decimal) or exactly one separator followed by one or two digits — a grouping + * separator is always followed by three. Ambiguous cells such as `1.234` vote + * for nothing. Returns `undefined` when the column gives no verdict, in which + * case callers keep the per-cell default. + */ +export function detectDecimalSeparator( + cells: Iterable +): DecimalSeparator | undefined { + let comma = 0; + let dot = 0; + + for (const cell of cells) { + if (!cell || typeof cell !== "string") continue; + const body = cell.trim().replace(NOISE, "").replace(/^[+-]|[+-]$/g, ""); + if (!body) continue; + + const lastComma = body.lastIndexOf(","); + const lastDot = body.lastIndexOf("."); + if (lastComma >= 0 && lastDot >= 0) { + if (lastComma > lastDot) comma++; + else dot++; + continue; + } + if (lastComma >= 0 && decimalRe(",", 2).test(body)) comma++; + else if (lastDot >= 0 && decimalRe(".", 2).test(body)) dot++; + } + + if (comma === dot) return undefined; + return comma > dot ? "," : "."; } diff --git a/src/utils/csvAutoDetect.test.ts b/src/utils/csvAutoDetect.test.ts index dc1c807..328ef90 100644 --- a/src/utils/csvAutoDetect.test.ts +++ b/src/utils/csvAutoDetect.test.ts @@ -20,8 +20,8 @@ import { autoDetectConfig, preprocessQuotedCSV, } from "./csvAutoDetect"; -import { parseFrenchAmount } from "./amountParser"; -import { parseDate } from "./dateParser"; +import type { DecimalSeparator } from "./amountParser"; +import { detectAmountSeparators, mapRow } from "./importFormat"; import { CSV_FIXTURE_NAMES, readCsvFixture, @@ -157,44 +157,25 @@ describe("analyzeHoldingsCsv (#245)", () => { // └──────────────────────────────────────────────────────────────────────────┘ /** - * MIRROR of the row-mapping rule in `useImportWizard.parseFilesInternal` - * (`useImportWizard.ts:489-542`), copied verbatim. + * The row-mapping rule is `mapRow` (`src/utils/importFormat.ts`) since #325. * - * It is duplicated here rather than imported because the rule currently lives - * inside a `useCallback` of a React hook and the repository has no jsdom. - * Issue #325 extracts it as a pure `mapRow(raw, format)` in - * `src/utils/importFormat.ts` — AT THAT POINT THIS MIRROR MUST BE DELETED and - * the tests re-pointed at the real `mapRow`. Until then, the guard test - * "production rule still matches the mirror" below fails the build if the - * production expression drifts, so the mirror cannot silently start lying. + * It used to be duplicated here as a hand copy called `mapCorpusRow`, because + * the rule lived inside a `useCallback` of a React hook and the repository has + * no jsdom; a static guard test pinned the five production expressions so the + * copy could not silently start lying. #325 lifted the rule out as a pure + * function, so the mirror and its guard are gone and these tests run the real + * thing — which is the whole point of the extraction. + * + * The shape below keeps the corpus assertions readable: a row is either its + * three parsed fields or its error. */ function mapCorpusRow( raw: string[], - cfg: NonNullable> + cfg: NonNullable>, + decimalSeparators?: ReadonlyMap ): { date: string; description: string; amount: number } | { error: string } { - const date = parseDate( - raw[cfg.columnMapping.date]?.trim() || "", - cfg.dateFormat - ); - const description = raw[cfg.columnMapping.description]?.trim() || ""; - - let amount: number; - if (cfg.amountMode === "debit_credit") { - const debit = parseFrenchAmount(raw[cfg.columnMapping.debitAmount ?? 0] || ""); - const credit = parseFrenchAmount( - raw[cfg.columnMapping.creditAmount ?? 0] || "" - ); - amount = isNaN(credit) ? -(isNaN(debit) ? 0 : debit) : credit; - } else { - amount = parseFrenchAmount(raw[cfg.columnMapping.amount ?? 0] || ""); - if (cfg.signConvention === "positive_expense" && !isNaN(amount)) { - amount = -amount; - } - } - - if (!date) return { error: "Invalid date" }; - if (isNaN(amount)) return { error: "Invalid amount" }; - return { date, description, amount }; + const row = mapRow(raw, { ...cfg, encoding: "utf-8" }, { decimalSeparators }); + return row.parsed ?? { error: row.error! }; } /** Full pipeline: raw file text -> signed amounts, as the wizard runs it. */ @@ -202,6 +183,7 @@ function parseFixtureEndToEnd(name: CsvFixtureName) { const rawContent = readCsvFixture(name); const cfg = autoDetectConfig(rawContent); if (!cfg) throw new Error(`autoDetectConfig returned null for ${name}`); + const format = { ...cfg, encoding: "utf-8" }; const data = Papa.parse(preprocessQuotedCSV(rawContent), { delimiter: cfg.delimiter, @@ -209,12 +191,14 @@ function parseFixtureEndToEnd(name: CsvFixtureName) { }).data as string[][]; const startIdx = cfg.skipLines + (cfg.hasHeader ? 1 : 0); - const rows = []; + const dataRows: string[][] = []; for (let i = startIdx; i < data.length; i++) { const raw = data[i]; if (raw.length <= 1 && raw[0]?.trim() === "") continue; - rows.push(mapCorpusRow(raw, cfg)); + dataRows.push(raw); } + const decimalSeparators = detectAmountSeparators(dataRows, format); + const rows = dataRows.map((raw) => mapCorpusRow(raw, cfg, decimalSeparators)); return { config: cfg, rows }; } @@ -242,27 +226,21 @@ describe("corpus integrity (#326)", () => { } }); - it("production rule still matches the mirror in mapCorpusRow", () => { - // Static contract, in the style of __integration__/transactions-transfer-icon. - // `mapCorpusRow` is a hand copy of a rule that lives inside a React hook; - // this is what stops it drifting. #325 extracts the rule as `mapRow` — - // when it does, these matchers fail, and that IS the signal to delete the - // mirror and import the real function. + it("runs the production rule, with no mirror left to drift (#325)", () => { + // The guard that used to live here pinned five expressions of + // `parseFilesInternal` so the hand-written mirror could not drift. Both are + // gone: the wizard delegates to `mapRow` and these tests call it directly. const SRC = readFileSync( resolve(import.meta.dirname, "..", "hooks", "useImportWizard.ts"), "utf-8" ); - expect(SRC).toContain( - "amount = isNaN(credit) ? -(isNaN(debit) ? 0 : debit) : credit;" - ); - expect(SRC).toContain( - 'if (config.signConvention === "positive_expense" && !isNaN(amount)) {' - ); - // The `?? 0` fallbacks silently read column 0 when a mapping is missing. - // #325 replaces them with an explicit row error. - expect(SRC).toContain("raw[config.columnMapping.debitAmount ?? 0]"); - expect(SRC).toContain("raw[config.columnMapping.creditAmount ?? 0]"); - expect(SRC).toContain("raw[config.columnMapping.amount ?? 0]"); + expect(SRC).toContain("mapRow(raw, config, {"); + // The `?? 0` fallbacks silently read column 0 — usually the date — when a + // mapping was incomplete. They must never come back. + expect(SRC).not.toContain("columnMapping.debitAmount ?? 0"); + expect(SRC).not.toContain("columnMapping.creditAmount ?? 0"); + expect(SRC).not.toContain("columnMapping.amount ?? 0"); + expect(SRC).not.toContain("isNaN(credit)"); }); }); @@ -358,15 +336,16 @@ describe("autoDetectConfig — KNOWN DEFECT: debit/credit order guessed by posit }); }); -describe("autoDetectConfig — KNOWN DEFECT: unused column filled with 0,00 (#326, fixed by #325)", () => { - // When the unused half of a debit/credit pair carries "0,00" instead of an - // empty cell, detection still gets the mode right — `isSparseComplementary` - // treats a 0 as absent. The PARSING is what breaks: the rule branches on - // `isNaN(credit)` (useImportWizard.ts:509), and "0,00" parses to 0, not NaN. - // Every debit row therefore imports as 0,00 and the expense vanishes. +describe("autoDetectConfig — unused column filled with 0,00 (#326, fixed by #325)", () => { + // FIXED. When the unused half of a debit/credit pair carries "0,00" instead + // of an empty cell, detection always got the mode right — + // `isSparseComplementary` treats a 0 as absent. The PARSING was what broke: + // the rule branched on `isNaN(credit)`, and "0,00" parses to 0, not NaN, so + // every debit row imported as 0,00 and the expense vanished — silently, since + // 0 passes validation. // - // #325 replaces the nullity comparison with `amount = credit - debit` on - // magnitudes and treats a 0,00 cell as absent. + // The rule is `credit - debit` on magnitudes now, for which a 0,00 cell needs + // no special case: zero is the identity of the subtraction. it("still detects the debit_credit mode correctly", () => { const cfg = autoDetectConfig(readCsvFixture("unused-column-zero"))!; @@ -375,17 +354,21 @@ describe("autoDetectConfig — KNOWN DEFECT: unused column filled with 0,00 (#32 expect(cfg.columnMapping.creditAmount).toBe(3); }); - it("swallows every debit to zero end to end", () => { - expect(amountsOf("unused-column-zero")).toEqual( - // DEFECT — should be REFERENCE_AMOUNTS - [0, 1250, 0, 0, 300, 0] + it("parses end to end to the reference amounts", () => { + expect(amountsOf("unused-column-zero")).toEqual(REFERENCE_AMOUNTS); + }); + + it("keeps every expense, and none of them is zero", () => { + const amounts = amountsOf("unused-column-zero"); + expect(amounts.filter((a) => a === 0)).toHaveLength(0); + expect(amounts.filter((a) => typeof a === "number" && a < 0)).toHaveLength( + 4 ); }); - it("keeps the credits intact, so only the expenses disappear", () => { - const amounts = amountsOf("unused-column-zero"); - expect(amounts.filter((a) => a === 0)).toHaveLength(4); - expect(amounts.filter((a) => a !== 0)).toEqual([1250, 300]); + it("reads the same file identically to its empty-cell twin", () => { + // Same six transactions, written with empty cells instead of 0,00. + expect(amountsOf("unused-column-zero")).toEqual(amountsOf("debit-credit")); }); }); @@ -431,29 +414,27 @@ describe("autoDetectConfig — header carrying a number (#326)", () => { }); }); -describe("autoDetectConfig — KNOWN DEFECT: header cell starting with digits (#326, fixed by #328)", () => { - // "2025 Montant" normalizes to "2025Montant"; parseFloat returns the 2025 - // prefix, `hasNumber` flips true, and the header row is taken for data. +describe("autoDetectConfig — header cell starting with digits (#326, fixed by #325)", () => { + // FIXED, by the other of the two routes link 1 named. "2025 Montant" + // normalizes to "2025Montant"; `parseFloat` returned the 2025 prefix, + // `hasNumber` flipped true, and the header row was taken for data. // - // #328 adds a lexical signal to `detectHeader`, and #325 anchors the parser - // so a numeric prefix no longer reads as a number. Either fix closes this. + // Link 1's note: "#328 adds a lexical signal to `detectHeader`, and #325 + // anchors the parser so a numeric prefix no longer reads as a number. Either + // fix closes this." #325 landed first — the anchored parser returns NaN for + // that cell, so `detectHeader` sees a row with no number and no date. + // #328 keeps its lexical signal for header cells that carry a BARE number. - it("takes the header row for a data row", () => { + it("recognises the header row", () => { const cfg = autoDetectConfig(readCsvFixture("header-numeric-label"))!; - expect(cfg.hasHeader).toBe(false); // DEFECT — should be true - // The column mapping survives because the six real rows outvote the header. + expect(cfg.hasHeader).toBe(true); expect(cfg.columnMapping).toEqual({ date: 0, description: 1, amount: 2 }); }); - it("emits the header row as an error row instead of skipping it", () => { + it("skips the header instead of emitting it as an error row", () => { const { rows } = parseFixtureEndToEnd("header-numeric-label"); - expect(rows).toHaveLength(7); // DEFECT — should be 6 - // The header survives as far as the date check, which is the only reason - // it fails loudly rather than importing 2025,00 as a transaction. - expect(rows[0]).toEqual({ error: "Invalid date" }); - expect(amountsOf("header-numeric-label").slice(1)).toEqual( - REFERENCE_AMOUNTS - ); + expect(rows).toHaveLength(6); + expect(amountsOf("header-numeric-label")).toEqual(REFERENCE_AMOUNTS); }); }); diff --git a/src/utils/importFormat.test.ts b/src/utils/importFormat.test.ts index 8f40519..d933426 100644 --- a/src/utils/importFormat.test.ts +++ b/src/utils/importFormat.test.ts @@ -17,10 +17,14 @@ import { AMOUNT_MODES, FORMAT_FIELD_PAIRS, ImportFormatError, + ROW_ERROR_KEYS, SIGN_CONVENTIONS, clearMappingForMode, + detectAmountSeparators, formatFromRow, formatToRow, + isRowErrorKey, + mapRow, } from "./importFormat"; // --------------------------------------------------------------------------- @@ -304,6 +308,217 @@ describe("clearMappingForMode", () => { }); }); +// --------------------------------------------------------------------------- +// mapRow — the row rule, lifted out of the wizard (#325) +// --------------------------------------------------------------------------- + +const DC_FORMAT: ImportFormat = { + delimiter: ";", + encoding: "utf-8", + dateFormat: "DD/MM/YYYY", + skipLines: 0, + hasHeader: true, + columnMapping: { date: 0, description: 1, debitAmount: 2, creditAmount: 3 }, + amountMode: "debit_credit", + signConvention: "negative_expense", +}; + +const SINGLE_FORMAT: ImportFormat = { + ...DC_FORMAT, + columnMapping: { date: 0, description: 1, amount: 2 }, + amountMode: "single", +}; + +/** The amount of a row that parsed, or the error key of one that did not. */ +function outcome(row: ReturnType): number | string { + return row.parsed ? row.parsed.amount : row.error!; +} + +describe("mapRow — debit/credit is a subtraction, not a nullity test (#325)", () => { + it("reads a debit as negative and a credit as positive", () => { + expect( + outcome(mapRow(["05/01/2025", "EPICERIE", "84,32", ""], DC_FORMAT)) + ).toBe(-84.32); + expect( + outcome(mapRow(["15/01/2025", "PAIE", "", "1250,00"], DC_FORMAT)) + ).toBe(1250); + }); + + it("treats a 0,00 cell in the unused column as absent", () => { + // The bug: `isNaN(credit)` was false for "0,00", so the credit won and + // every debit imported as zero. + expect( + outcome(mapRow(["05/01/2025", "EPICERIE", "84,32", "0,00"], DC_FORMAT)) + ).toBe(-84.32); + expect( + outcome(mapRow(["15/01/2025", "PAIE", "0,00", "1250,00"], DC_FORMAT)) + ).toBe(1250); + }); + + it("takes both columns as magnitudes, whatever sign they carry", () => { + expect( + outcome(mapRow(["05/01/2025", "EPICERIE", "-84,32", ""], DC_FORMAT)) + ).toBe(-84.32); + expect( + outcome(mapRow(["15/01/2025", "PAIE", "", "-1250,00"], DC_FORMAT)) + ).toBe(1250); + }); + + it("nets a row that fills both columns", () => { + expect( + outcome(mapRow(["05/01/2025", "AJUST", "40,00", "100,00"], DC_FORMAT)) + ).toBe(60); + }); + + it("errors when NEITHER column is readable, instead of importing 0", () => { + expect(outcome(mapRow(["05/01/2025", "X", "", ""], DC_FORMAT))).toBe( + ROW_ERROR_KEYS.invalidAmount + ); + expect(outcome(mapRow(["05/01/2025", "X", "n/a", "-"], DC_FORMAT))).toBe( + ROW_ERROR_KEYS.invalidAmount + ); + }); + + it("ignores the sign convention, which only applies to a single column", () => { + const flipped: ImportFormat = { + ...DC_FORMAT, + signConvention: "positive_expense", + }; + expect( + outcome(mapRow(["05/01/2025", "EPICERIE", "84,32", ""], flipped)) + ).toBe(-84.32); + }); +}); + +describe("mapRow — an unmapped amount column is an error, not column 0 (#325)", () => { + // The `?? 0` fallbacks read column 0 — usually the date — for every row. + + it("refuses a single-amount format with no amount column", () => { + const { amount: _drop, ...mapping } = SINGLE_FORMAT.columnMapping; + const row = mapRow(["05/01/2025", "EPICERIE", "-84,32"], { + ...SINGLE_FORMAT, + columnMapping: mapping, + }); + expect(row.parsed).toBeNull(); + expect(row.error).toBe(ROW_ERROR_KEYS.amountColumnNotMapped); + }); + + it("refuses a debit/credit format with neither column mapped", () => { + const row = mapRow(["05/01/2025", "EPICERIE", "84,32", ""], { + ...DC_FORMAT, + columnMapping: { date: 0, description: 1 }, + }); + expect(row.error).toBe(ROW_ERROR_KEYS.amountColumnNotMapped); + }); + + it("accepts a debit/credit format with only ONE column mapped", () => { + // Half a pair is a legitimate shape (a card statement with debits only). + expect( + outcome( + mapRow(["05/01/2025", "EPICERIE", "84,32"], { + ...DC_FORMAT, + columnMapping: { date: 0, description: 1, debitAmount: 2 }, + }) + ) + ).toBe(-84.32); + }); + + it("reports the format error before any per-row problem", () => { + const row = mapRow(["not-a-date", "EPICERIE", "-84,32"], { + ...SINGLE_FORMAT, + columnMapping: { date: 0, description: 1 }, + }); + expect(row.error).toBe(ROW_ERROR_KEYS.amountColumnNotMapped); + }); +}); + +describe("mapRow — the rest of the contract (#325)", () => { + it("keeps the date check ahead of the amount check", () => { + expect(outcome(mapRow(["nope", "X", "abc"], SINGLE_FORMAT))).toBe( + ROW_ERROR_KEYS.invalidDate + ); + expect(outcome(mapRow(["05/01/2025", "X", "abc"], SINGLE_FORMAT))).toBe( + ROW_ERROR_KEYS.invalidAmount + ); + }); + + it("applies positive_expense to a single amount column", () => { + expect( + outcome( + mapRow(["05/01/2025", "EPICERIE", "84,32"], { + ...SINGLE_FORMAT, + signConvention: "positive_expense", + }) + ) + ).toBe(-84.32); + }); + + it("carries rowIndex, raw and sourceFilename through", () => { + const raw = ["05/01/2025", "EPICERIE", "-84,32"]; + const row = mapRow(raw, SINGLE_FORMAT, { + rowIndex: 41, + sourceFilename: "releve.csv", + }); + expect(row.rowIndex).toBe(41); + expect(row.raw).toBe(raw); + expect(row.sourceFilename).toBe("releve.csv"); + expect(row.parsed).toEqual({ + date: "2025-01-05", + description: "EPICERIE", + amount: -84.32, + }); + }); + + it("defaults rowIndex to 0 and omits sourceFilename when not given", () => { + const row = mapRow(["05/01/2025", "X", "-1,00"], SINGLE_FORMAT); + expect(row.rowIndex).toBe(0); + expect("sourceFilename" in row).toBe(false); + }); + + it("reports every failure as an i18n key", () => { + for (const raw of [ + ["nope", "X", "-1,00"], + ["05/01/2025", "X", "abc"], + ]) { + const row = mapRow(raw, SINGLE_FORMAT); + expect(isRowErrorKey(row.error!)).toBe(true); + } + expect(isRowErrorKey("boom: sqlite is locked")).toBe(false); + }); +}); + +describe("detectAmountSeparators — the column decides (#325)", () => { + const rows = [ + ["05/01/2025", "EPICERIE", "1.234", ""], + ["15/01/2025", "PAIE", "", "84,32"], + ]; + + it("arbitrates each amount column the format declares", () => { + const seps = detectAmountSeparators(rows, DC_FORMAT); + // Column 2 holds only the ambiguous "1.234" — no verdict of its own. + expect(seps.get(2)).toBeUndefined(); + expect(seps.get(3)).toBe(","); + }); + + it("turns an ambiguous cell into the column's reading", () => { + const french = [ + ["05/01/2025", "A", "1.234"], + ["15/01/2025", "B", "84,32"], + ]; + const seps = detectAmountSeparators(french, SINGLE_FORMAT); + expect(seps.get(2)).toBe(","); + expect( + outcome(mapRow(french[0], SINGLE_FORMAT, { decimalSeparators: seps })) + ).toBe(1234); + // Without the column verdict, the same cell reads as 1.234. + expect(outcome(mapRow(french[0], SINGLE_FORMAT))).toBe(1.234); + }); + + it("looks at no column the format does not use", () => { + expect(detectAmountSeparators(rows, SINGLE_FORMAT).has(3)).toBe(false); + }); +}); + // --------------------------------------------------------------------------- // Static guards on useImportWizard. // diff --git a/src/utils/importFormat.ts b/src/utils/importFormat.ts index 7fe86e9..16c7d11 100644 --- a/src/utils/importFormat.ts +++ b/src/utils/importFormat.ts @@ -23,8 +23,15 @@ import type { ImportFormat, ImportFormatRow, ImportFormatRowInput, + ParsedRow, SignConvention, } from "../shared/types"; +import { + detectDecimalSeparator, + parseFrenchAmount, + type DecimalSeparator, +} from "./amountParser"; +import { parseDate } from "./dateParser"; /** * Values the application can actually map. The database `CHECK` on @@ -185,3 +192,163 @@ export function clearMappingForMode( ...(mapping.amount !== undefined ? { amount: mapping.amount } : {}), }; } + +// --------------------------------------------------------------------------- +// Row mapping — one CSV row + one format -> one `ParsedRow` (#325) +// --------------------------------------------------------------------------- + +/** + * i18n keys for the ways a single row can fail. They used to be raw English + * literals rendered straight into the preview table; they are keys now so the + * strings live in the locale files like every other displayed text. Resolve + * them with `isRowErrorKey` before calling `t()` — an `ImportReport` also + * carries raw exception messages, which must never be fed to the translator. + */ +export const ROW_ERROR_KEYS = { + invalidDate: "import.rowErrors.invalidDate", + invalidAmount: "import.rowErrors.invalidAmount", + amountColumnNotMapped: "import.rowErrors.amountColumnNotMapped", + parseError: "import.rowErrors.parseError", +} as const; + +export type RowErrorKey = (typeof ROW_ERROR_KEYS)[keyof typeof ROW_ERROR_KEYS]; + +const ROW_ERROR_VALUES: readonly string[] = Object.values(ROW_ERROR_KEYS); + +/** True when the string is one of ours and can safely be translated. */ +export function isRowErrorKey(value: string): value is RowErrorKey { + return ROW_ERROR_VALUES.includes(value); +} + +export interface MapRowOptions { + /** Position of the row in the whole import. Defaults to 0. */ + rowIndex?: number; + /** File the row came from, carried through to the report. */ + sourceFilename?: string; + /** + * Decimal separator arbitrated at COLUMN level, keyed by column index — see + * `detectAmountSeparators`. A column absent from the map keeps the per-cell + * default. + */ + decimalSeparators?: ReadonlyMap; +} + +/** + * Map one raw CSV row to a `ParsedRow` under a given format. PURE: no React, no + * I/O, no throw — every failure comes back as `error` on the row. + * + * This rule used to live inside a `useCallback` of `useImportWizard`, which is + * why the corpus tests had to keep a hand-written mirror of it and why the + * detection score (#328) and the signed preview (#329) would each have had to + * re-implement it. One implementation, three consumers. + * + * The two-column rule is `credit - debit` on MAGNITUDES. It used to be + * `isNaN(credit) ? -debit : credit`, so a bank writing `0,00` in the unused + * column — which is common — made every debit import as 0,00 and pass + * validation, because `isNaN(0)` is false. Subtraction needs no special case + * for a `0,00` cell: zero is the identity. `Math.abs` implements the documented + * convention (both columns hold positive numbers) even when an export negates + * its debits. + * + * A row whose amount is unreadable in BOTH columns is an error, never a 0: an + * amount nobody could read must not enter the ledger as a free transaction. + */ +export function mapRow( + raw: string[], + format: ImportFormat, + options: MapRowOptions = {} +): ParsedRow { + const mapping = format.columnMapping; + const base = { + rowIndex: options.rowIndex ?? 0, + raw, + ...(options.sourceFilename !== undefined + ? { sourceFilename: options.sourceFilename } + : {}), + }; + const fail = (error: RowErrorKey): ParsedRow => ({ + ...base, + parsed: null, + error, + }); + + const readAmount = (col: number): number => + parseFrenchAmount(raw[col]?.trim() ?? "", { + decimalSeparator: options.decimalSeparators?.get(col), + }); + + // 1. Configuration. An unmapped amount column used to fall back to `?? 0`, + // reading column 0 — usually the date — for every row of the file. That is + // a format error affecting the whole import, so it is reported before any + // per-row problem. + const debitMapped = mapping.debitAmount !== undefined; + const creditMapped = mapping.creditAmount !== undefined; + if (format.amountMode === "debit_credit") { + if (!debitMapped && !creditMapped) { + return fail(ROW_ERROR_KEYS.amountColumnNotMapped); + } + } else if (mapping.amount === undefined) { + return fail(ROW_ERROR_KEYS.amountColumnNotMapped); + } + + // 2. Date. Kept ahead of the amount value, as it has always been. + const date = parseDate(raw[mapping.date]?.trim() || "", format.dateFormat); + if (!date) return fail(ROW_ERROR_KEYS.invalidDate); + + // 3. Amount. + let amount: number; + if (format.amountMode === "debit_credit") { + const debit = debitMapped ? readAmount(mapping.debitAmount!) : NaN; + const credit = creditMapped ? readAmount(mapping.creditAmount!) : NaN; + if (isNaN(debit) && isNaN(credit)) { + return fail(ROW_ERROR_KEYS.invalidAmount); + } + amount = + (isNaN(credit) ? 0 : Math.abs(credit)) - + (isNaN(debit) ? 0 : Math.abs(debit)); + } else { + amount = readAmount(mapping.amount!); + if (isNaN(amount)) return fail(ROW_ERROR_KEYS.invalidAmount); + if (format.signConvention === "positive_expense") amount = -amount; + } + + return { + ...base, + parsed: { + date, + description: raw[mapping.description]?.trim() || "", + amount, + }, + }; +} + +/** + * Arbitrate the decimal separator of every amount column the format declares, + * over the whole set of data rows. + * + * `1.234` is 1234 in a French column and 1.234 in an English one, and no rule + * applied to that cell ALONE can tell — but a sibling cell reading `84,32` + * settles it for the column. Detection deliberately stays out of this (it runs + * before a column is known to be an amount column at all); the verdict is + * applied where the value actually becomes a transaction. + */ +export function detectAmountSeparators( + rows: readonly string[][], + format: ImportFormat +): Map { + const mapping = format.columnMapping; + const columns = + format.amountMode === "debit_credit" + ? [mapping.debitAmount, mapping.creditAmount] + : [mapping.amount]; + + const result = new Map(); + for (const col of columns) { + if (col === undefined) continue; + const verdict = detectDecimalSeparator( + rows.map((row) => row[col] ?? "") + ); + if (verdict) result.set(col, verdict); + } + return result; +}