All checks were successful
PR Check — Frontend / frontend (pull_request) Successful in 1m44s
Three ways an imported amount could be silently wrong, all of them passing validation, all of them fixed here. The two-column rule was `isNaN(credit) ? -debit : credit`, so the credit always won. Many banks write `0,00` in the unused column rather than leaving it empty, and `isNaN(0)` is false -- every debit of such a file imported as 0,00 and the expense simply vanished, with no error anywhere. The rule is `credit - debit` on magnitudes now, which needs no special case for a `0,00` cell (zero is the identity of the subtraction) and implements the documented convention even when an export negates its debits. A row unreadable in BOTH columns is an error instead of a free 0,00 transaction. `parseFrenchAmount` ended on `parseFloat`, which returns the longest valid PREFIX instead of rejecting. Measured before the fix: `"50,00-"` -> 5000, `"1 234,56 CR"` -> 123456, `"100,00 CAD"` -> 10000. A factor-100 error, and it passes `isNaN`, so those rows counted as VALID everywhere downstream -- which would have defeated the signed preview (#329), the safety net of the whole chantier. Validation is anchored over the whole normalized string now and any residual character yields NaN. NaN, not a rescued magnitude, for a trailing `CR`/`DB` or currency code. Two reasons: `CR`/`DB` carry a DIRECTION, so returning a magnitude for both would trade a loud failure for a silent SIGN error (the D/C-indicator shape is refused upstream by design, #328); and `"100,00 CAD"` is structurally identical to `"2025 Montant"`, so whitelisting a trailing word to rescue the first re-blinds `detectHeader` on the second. Two accounting forms ARE legitimate and supported: parentheses `(50,00)` and a trailing sign `50,00-`. The `?? 0` fallbacks read column 0 -- usually the date -- when the mapping was incomplete. An unmapped amount column is an explicit row error now, reported ahead of any per-row problem since it is a format error affecting every row. `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. `detectDecimalSeparator` arbitrates from the decisive siblings of the column and `parseFrenchAmount` takes the verdict as an option. Detection deliberately stays out of it: it runs before a column is known to be an amount column at all, so the verdict is applied where the value actually becomes a transaction. The rule itself moves out of the hook as a pure `mapRow(raw, format)` in `importFormat.ts`. That is what lets the corpus tests run the REAL rule -- the hand-written mirror in `csvAutoDetect.test.ts` and the static guard pinning five `parseFilesInternal` expressions are both deleted -- and what stops the detection score (#328) and the signed preview (#329) each re-implementing it. Hardening is global: the parser is shared by 11 call sites, 8 in `csvAutoDetect.ts` and 3 in the holdings CSV import (#245), where a price cell `150,25 CAD` used to store 15025. It is refused now and `buildDetailedLines` raises on the empty price. An unreadable QUANTITY was worse -- coerced to 0, so a zero-value position saved in silence; the draft keeps the offending text instead and the existing `snapshot_priced_quantity_required` fires. Row errors become i18n keys (`import.rowErrors.*`) rather than the raw English literals rendered straight into the preview table, since this adds a user-visible string. The report table also carries raw exception messages, so both render sites resolve through `isRowErrorKey` and never feed `t()` anything that is not ours. Test churn, per link 1's handoff (update the expectation, drop the marker, never delete the test): the three `#325` KNOWN DEFECT blocks in `amountParser.test.ts` flip, plus `unused-column-zero` in `csvAutoDetect.test.ts`. One block tagged `#328` flips too -- `header-numeric-label`, whose own comment reads "#328 adds a lexical signal to detectHeader, and #325 anchors the parser [...] either fix closes this". The anchored parser landed first. The other `#328` blocks (`debit-credit-reversed`, `absolute-indicator`) and the `#329` block are verified unchanged. CHANGELOG and docs stay centralized in the last link of the stack, as the plan specifies. 989 vitest (963 before), tsc + vite build clean, cargo check clean. No DB migration. Resolves #325
253 lines
12 KiB
TypeScript
253 lines
12 KiB
TypeScript
// 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 #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).
|
|
//
|
|
// 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 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 { detectDecimalSeparator, parseFrenchAmount } from "./amountParser";
|
|
import { autoDetectConfig } from "./csvAutoDetect";
|
|
import {
|
|
buildDetailedLines,
|
|
holdingsFromCsvRows,
|
|
} from "../hooks/useSnapshotEditor";
|
|
import { BalanceServiceError } from "../services/balance.service";
|
|
import { readCsvFixture } from "../__fixtures__/csv";
|
|
|
|
describe("parseFrenchAmount — separator contract (#326)", () => {
|
|
it("reads the French decimal comma", () => {
|
|
expect(parseFrenchAmount("1234,56")).toBe(1234.56);
|
|
expect(parseFrenchAmount("12,5")).toBe(12.5);
|
|
expect(parseFrenchAmount("0,00")).toBe(0);
|
|
});
|
|
|
|
it("reads French thousand separators (space, non-breaking space, dot)", () => {
|
|
expect(parseFrenchAmount("1 234,56")).toBe(1234.56);
|
|
expect(parseFrenchAmount("1\u00A0234,56")).toBe(1234.56); // non-breaking space
|
|
expect(parseFrenchAmount("1.234,56")).toBe(1234.56);
|
|
});
|
|
|
|
it("reads English notation", () => {
|
|
expect(parseFrenchAmount("1234.56")).toBe(1234.56);
|
|
expect(parseFrenchAmount("1,234.56")).toBe(1234.56);
|
|
});
|
|
|
|
it("keeps the sign of a leading minus", () => {
|
|
expect(parseFrenchAmount("-84,32")).toBe(-84.32);
|
|
expect(parseFrenchAmount(" -84,32 ")).toBe(-84.32);
|
|
});
|
|
|
|
it("strips currency symbols on either side", () => {
|
|
expect(parseFrenchAmount("1 250,00 $")).toBe(1250);
|
|
expect(parseFrenchAmount("$1,250.00")).toBe(1250);
|
|
expect(parseFrenchAmount("€84,32")).toBe(84.32);
|
|
expect(parseFrenchAmount("£84,32")).toBe(84.32);
|
|
});
|
|
|
|
it("rejects blank and non-numeric input", () => {
|
|
expect(parseFrenchAmount("")).toBeNaN();
|
|
expect(parseFrenchAmount(" ")).toBeNaN();
|
|
expect(parseFrenchAmount("abc")).toBeNaN();
|
|
expect(parseFrenchAmount("-")).toBeNaN();
|
|
expect(parseFrenchAmount("--5")).toBeNaN();
|
|
// Letter-leading labels are rejected — this is what keeps `detectHeader`
|
|
// working on a header such as "Solde 2024" (see the twin defect below).
|
|
expect(parseFrenchAmount("Solde 2024")).toBeNaN();
|
|
// Non-string input is guarded before any parsing.
|
|
expect(parseFrenchAmount(undefined as unknown as string)).toBeNaN();
|
|
expect(parseFrenchAmount(42 as unknown as string)).toBeNaN();
|
|
});
|
|
});
|
|
|
|
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.
|
|
//
|
|
// 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("reads a trailing sign as a negative amount", () => {
|
|
// "50,00-" is the trailing-minus convention of several bank exports.
|
|
expect(parseFrenchAmount("50,00-")).toBe(-50);
|
|
expect(parseFrenchAmount("1 234,56-")).toBe(-1234.56);
|
|
expect(parseFrenchAmount("50,00+")).toBe(50);
|
|
});
|
|
|
|
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("rejects a trailing currency code", () => {
|
|
expect(parseFrenchAmount("100,00 CAD")).toBeNaN();
|
|
expect(parseFrenchAmount("84,32 USD")).toBeNaN();
|
|
});
|
|
|
|
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("rejects JavaScript number literals a bank never emits", () => {
|
|
expect(parseFrenchAmount("1e3")).toBeNaN();
|
|
expect(parseFrenchAmount("Infinity")).toBeNaN();
|
|
expect(parseFrenchAmount("0x1F")).toBeNaN();
|
|
});
|
|
|
|
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 — 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 — 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("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.
|
|
expect(parseFrenchAmount("12,345")).toBe(12345);
|
|
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, 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("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" 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(true);
|
|
});
|
|
|
|
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 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"],
|
|
["MSFT", "5", "300,50", "1 400,00 CAD"],
|
|
],
|
|
{ symbol: 0, quantity: 1, unit_price: 2, book_cost: 3 }
|
|
);
|
|
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("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 }
|
|
);
|
|
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"
|
|
});
|
|
});
|