From c49224d4f74ff347f3d434feaad97f27ddb95ba1 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Sat, 5 Sep 2026 01:38:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(domain):=20review=20round=201=20=E2=80=94?= =?UTF-8?q?=20name=20the=20cost,=20cover=20the=20anchored=20throw,=20pin?= =?UTF-8?q?=20two=20reachable-but-untested=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1 (Critical): a month-only startDate on an anchored everyNMonths cost with no charges yet reached addMonthsToDate directly and threw a bare parsing error naming no cost, several frames below foldBalances. anchoredOccurrences now checks its starting cursor's granularity before the walk begins and throws a cost-named, actionable error instead. No new validation at either write boundary — that stays out of scope here. stepFrom's docstring no longer asserts a guarantee ("required to carry a day-granular startDate") that nothing in the code enforces; it now says what actually protects it. I1 (Important): the anchored walk's own termination throw had zero test coverage — both surviving "termination" tests use the cost() fixture, whose default anchoring is calendar, so they only ever exercised gridOccurrences. Proven by mutation (replacing the anchored throw with break left every test passing). Added a test that actually walks anchoredOccurrences, and corrected the inline comment that falsely claimed coverage existed. I2 (Important): confirmationsFor's throw is reachable from data the JSON importer currently accepts (a calendar-cost purchase missing occurrenceDate), not just a hypothetical programming error — its docstring said "programming error upstream," which isn't true yet. Docstring corrected, and the throw is now pinned by a test instead of only asserted in prose. I3 (Important): nothing asserted that editing a charge's date backwards re-anchors the series instead of throwing — exactly the behaviour deleting requireConfirmationAdvances made structurally safe, and exactly the gap left by deleting round-1's "a date more than one cycle before the slot it claims is refused" test. Added the inverse: same fixture, but asserting the edit now succeeds, the fold still folds cleanly, and the series re-anchors to the edited date. Minor: round-1 deleted ExpectedBand.test.ts's "stays suppressed once X3 no longer applies" (X1-only), leaving its underlying scenario (day-granular anchored cost, slot beyond the displayed month, X3 already satisfied) completely untested even though still reachable. Added a test pinning what expectedGroups actually does with it now that X1 is gone: the row is offered, not suppressed — so a future reintroduction of suppression here is a visible, deliberate change instead of a silent one. Left untouched, as instructed: ExpectedBand.tsx's stale X1 docstring and the Settings anchoring dropdown (both belong to later tasks). --- src/domain/occurrences.test.ts | 56 ++++++++++++++++++++++++-- src/domain/occurrences.ts | 56 +++++++++++++++++++------- src/store/actions.test.ts | 53 ++++++++++++++++++++++++ src/ui/components/ExpectedBand.test.ts | 27 +++++++++++++ 4 files changed, 174 insertions(+), 18 deletions(-) diff --git a/src/domain/occurrences.test.ts b/src/domain/occurrences.test.ts index 39fe380..68db045 100644 --- a/src/domain/occurrences.test.ts +++ b/src/domain/occurrences.test.ts @@ -150,9 +150,30 @@ describe("occurrencesOf — confirmation (calendar)", () => { expect(found[0]!.confirmedBy).toBe("pY"); expect(found[1]!.confirmedBy).toBeNull(); }); + + test("REVIEW ROUND 2 / I2: a calendar-cost purchase with no occurrenceDate throws, naming the purchase", () => { + // Reachable from legal-looking imported data, not just a programming + // error: the JSON importer currently accepts a purchase referencing a + // recurring cost with no occurrenceDate, so a file built that way loads + // clean and throws on the very next fold. Pinned here so the throw's + // behaviour (and its message naming the purchase, not a bare "undefined") + // doesn't drift silently. + const rent = cost({ id: "rent", anchoring: "calendar" }); + const noSlot: Purchase = { + id: "bad1", + date: "2026-01-05", + description: "Rent", + total: { amount: 8000, currency: "DKK" }, + splitMode: "percent", + splits: [{ postId: "housing", value: 100, absorbsRemainder: true }], + schedule: null, + source: { recurringId: "rent" }, // no occurrenceDate + }; + expect(() => occurrencesOf(rent, [noSlot], "2026-02")).toThrow(/bad1/); + }); }); -describe("occurrencesOf — termination", () => { +describe("occurrencesOf — termination (grid walk)", () => { // CONTROLLER RULING: the round-1 version of this block also had "a // confirmation dated absurdly early throws rather than walking backwards", // which modelled the OLD lastCharge slot-rebasing mechanism (a confirmation @@ -163,8 +184,15 @@ describe("occurrencesOf — termination", () => { // before startDate cannot pull the series backwards" below) — so the throw // that test asserted is no longer reachable by the scenario it built. // Deleted rather than adapted, because there is no reachable input left that - // exercises the same path; the anchored walk's own termination throw is - // still covered by the two tests kept below, which only need n < 1. + // exercises the same path. + // + // CORRECTION (round 2 review): the two tests below both build their cost + // with `cost({...})`, whose fixture default is `anchoring: "calendar"` — so + // they only ever exercise `gridOccurrences`'s termination throw, not + // `anchoredOccurrences`'s. A prior version of this comment claimed the + // anchored throw was "still covered by the two tests kept below"; that was + // false. The anchored walk's own termination throw has its own test, in + // "the anchored walk (lastCharge)" below. test("a recurrence that cannot advance throws rather than hanging", () => { const broken = cost({ recurrence: { kind: "everyNDays", n: 0 }, startDate: "2026-01-01" }); expect(() => occurrencesOf(broken, none, "2026-02")).toThrow(/did not advance/); @@ -309,6 +337,28 @@ describe("the anchored walk (lastCharge)", () => { expect(dates).toEqual(["2026-10-12", "2026-11-11"]); }); + test("REVIEW ROUND 2 / I1: a broken recurrence throws through the ANCHORED walk too, not just the grid one", () => { + // Proven missing by mutation: replacing the anchored walk's termination + // throw with a `break` left every existing test passing, because the two + // tests in "occurrencesOf — termination (grid walk)" both default to + // `anchoring: "calendar"` via the `cost()` fixture and so only exercise + // `gridOccurrences`. `phone()` here defaults to `anchoring: "lastCharge"`, + // so this one actually walks `anchoredOccurrences`. + const broken = phone({ recurrence: { kind: "everyNDays", n: 0 } }); + expect(() => occurrencesOf(broken, [], "2026-11")).toThrow(/did not advance/); + }); + + test("REVIEW ROUND 2 / C1: a month-only startDate throws a cost-named error, not a bare date-parsing one", () => { + // No charges, so the walk's starting cursor is `cost.startDate` directly + // — "2026-09", month-only. Before this fix, that reached `stepFrom` -> + // `addMonthsToDate` and threw "Not a day-granular IsoDate: 2026-09", + // naming no cost, three frames below foldBalances. Now `anchoredOccurrences` + // catches it first and names the bill. + const untimed = phone({ startDate: "2026-09" }); + expect(() => occurrencesOf(untimed, [], "2026-11")).toThrow(/Phone plan/); + expect(() => occurrencesOf(untimed, [], "2026-11")).not.toThrow(/day-granular IsoDate/); + }); + test("unconfirmed across many months, a day-granular anchored cost just keeps stepping", () => { // CONTROLLER RULING: replaces round-1's "unconfirmed, it behaves exactly // like a calendar series" (old describe "lastCharge anchoring: the phone diff --git a/src/domain/occurrences.ts b/src/domain/occurrences.ts index bd51699..80c8e9e 100644 --- a/src/domain/occurrences.ts +++ b/src/domain/occurrences.ts @@ -42,6 +42,9 @@ function isBefore(a: IsoDate, b: IsoDate): boolean { return a < b; } +/** "YYYY-MM-DD" vs "YYYY-MM" — whether a date carries a day at all. */ +const DAY_GRANULAR_DATE = /^\d{4}-\d{2}-\d{2}$/; + /** Moves forward to the next occurrence of `weekday`, or stays put if already on it. */ function snapForward(date: IsoDate, weekday: number): IsoDate { const delta = (weekday - weekdayOf(date) + 7) % 7; @@ -58,13 +61,16 @@ function snapForward(date: IsoDate, weekday: number): IsoDate { * step is day-precise and clamps (`addMonthsToDate`). The other two kinds are * day-granular either way and ignore the argument. * - * The anchored walk (`anchoredOccurrences`) only ever calls this with a - * day-granular `date`: its cursor starts at `cost.startDate` or at a real - * charge's date, and an anchored `everyNMonths` cost is required to carry a - * day-granular `startDate` for exactly this reason. If a month-only date ever - * reaches this branch, `addMonthsToDate` throwing is CORRECT — something - * upstream produced a cursor this function was never meant to receive, and a - * visible error beats silently treating "2026-09" as "2026-09-01". + * `anchoredOccurrences` checks its starting cursor is day-granular before + * ever calling this for an `everyNMonths` cost, and throws an error naming + * the cost if it is not. That check exists BECAUSE nothing currently + * guarantees a day-granular `startDate` on an anchored `everyNMonths` cost — + * neither write boundary rejects a month-only one, and a charge's own date + * can be month-only too (`Purchase.date`'s documented shape). Without that + * check, a month-only date reaching this branch would still throw, but from + * `addMonthsToDate` with a bare parsing message that names no cost — a + * failure the owner cannot act on, several frames below `foldBalances`, that + * would take every money route down with it. * * Adding a recurrence kind means adding a case here and a variant to * `Recurrence`. Nothing else in the app switches on the kind. @@ -96,14 +102,17 @@ export function stepFrom( * * Calendar-only. A `lastCharge` cost has no slots (`Purchase.source`'s doc in * types.ts) — its charges are walked by `chargesFor`/`anchoredOccurrences` - * off `Purchase.date` directly, and never reach this function. A purchase - * that DOES reach here (its `recurringId` matches) but carries no - * `occurrenceDate` is therefore not a data problem to skip past quietly: it - * means some caller attached a `lastCharge`-shaped purchase (or one with a - * dangling/mismatched source) to a cost this function is walking as - * `calendar`, which is a programming error upstream. Throwing says so - * directly instead of the old scaffolding `continue`, which would have - * silently dropped the confirmation and left the slot looking unpaid. + * off `Purchase.date` directly, and never reach this function BY DESIGN. + * + * A purchase that DOES reach here (its `recurringId` names a `calendar` cost) + * but carries no `occurrenceDate` is not only a hypothetical programming + * error: the JSON importer currently accepts a purchase missing one, and + * import replaces the whole dataset, so a file built that way loads clean and + * throws on the very next fold — the app goes down immediately, from data the + * import boundary let in. Throwing here, naming the purchase, is still better + * than the old scaffolding `continue`, which would have silently dropped the + * confirmation and left the slot looking unpaid; closing the import boundary + * itself is a separate piece of work. */ export function confirmationsFor( purchases: readonly Purchase[], @@ -183,6 +192,23 @@ function anchoredOccurrences( // `max(latest, startDate)`: a charge back-dated before the cost existed must // not pull the series behind its own start. const anchor = latest === null ? null : isBefore(latest, cost.startDate) ? cost.startDate : latest; + const startingPoint = anchor ?? cost.startDate; + + // `stepFrom`'s `everyNMonths`/`lastCharge` branch hands its date straight + // to `addMonthsToDate`, which requires "YYYY-MM-DD" and would otherwise + // throw a bare parsing error naming no cost at all, several frames below + // `foldBalances` — taking every money route down with a message the owner + // cannot act on. Nothing at either write boundary rejects a month-only + // `startDate` on an anchored `everyNMonths` cost today (and a charge's own + // date can be month-only too, per `Purchase.date`'s documented shape), so + // check here, before the walk starts, and name the cost. + if (cost.recurrence.kind === "everyNMonths" && !DAY_GRANULAR_DATE.test(startingPoint)) { + throw new Error( + `Recurring cost "${cost.name}" is anchored (lastCharge) with a monthly recurrence, so every ` + + `date it steps from must be day-granular ("YYYY-MM-DD") — got "${startingPoint}". Give it a ` + + `specific day: on its startDate if it has no charges yet, or on its latest charge otherwise.`, + ); + } let cursor = anchor === null ? cost.startDate : stepFrom(anchor, cost.recurrence, cost.anchoring); diff --git a/src/store/actions.test.ts b/src/store/actions.test.ts index 4536547..63fd846 100644 --- a/src/store/actions.test.ts +++ b/src/store/actions.test.ts @@ -833,3 +833,56 @@ describe("confirmOccurrence", () => { // always succeeds — because the anchored walk always re-derives its cursor // from whichever charge is latest, never from a `Purchase.date` a caller is // trying to move "behind" a slot that no longer exists. + +// REVIEW ROUND 2 / I3: that claim ("now behaves identically — the edit always +// succeeds") was asserted above but never actually pinned by a test — the +// deleted test #10 ("a date more than one cycle before the slot it claims is +// refused") half-covered this shape and was removed along with it, leaving a +// silent gap. Inverted rather than left out: same fixture the old refused-edit +// test used, but now asserting the edit SUCCEEDS, the fold still folds +// cleanly afterwards, and the series re-anchors to the edited date rather +// than the original confirmation date. +describe("editing a charge's date backwards re-anchors instead of throwing", () => { + test("moving a confirmed charge's date more than one cycle earlier re-anchors the series to it", () => { + const data = createSeedDataset("2026-08"); + data.posts[0]!.id = "housing"; + const car = actions.addRecurringCost(data, { + name: "Car insurance", + archived: false, + amount: { amount: 900, currency: "DKK" }, + startDate: "2026-08-14", + recurrence: { kind: "everyNDays", n: 30 }, + anchoring: "lastCharge", + splitMode: "percent", + splits: [{ postId: "housing", value: 100, absorbsRemainder: true }], + }); + + const purchase = actions.confirmOccurrence(data, car.id, "2026-09-13", { + date: "2026-09-13", + }); + expect(() => occurrencesByMonth(data, "2026-12")).not.toThrow(); + + // Under the old guard, an edit like this ("more than one cycle before the + // slot it claims") was refused. Now it just succeeds. (2026-08-20, not + // something before the cost's own 2026-08-14 startDate — an edit that far + // back would hit the DELIBERATE "can't pull the series before startDate" + // clamp tested in occurrences.test.ts, which is a different behaviour + // than the one this test is pinning.) + actions.updatePurchase(data, purchase.id, { date: "2026-08-20" }); + expect(data.purchases.find((p) => p.id === purchase.id)!.date).toBe("2026-08-20"); + + // The fold still works... + expect(() => occurrencesByMonth(data, "2026-12")).not.toThrow(); + + // ...and the series re-anchors to the EDITED date: the next projected + // occurrence is one cycle (30 days) after 2026-08-20, not after the + // original 2026-09-13. + const byMonth = occurrencesByMonth(data, "2026-12"); + const dates = [...byMonth.values()] + .flat() + .filter((o) => o.recurringId === car.id) + .map((o) => o.date) + .sort(); + expect(dates[0]).toBe("2026-09-19"); // 2026-08-20 + 30 days + }); +}); diff --git a/src/ui/components/ExpectedBand.test.ts b/src/ui/components/ExpectedBand.test.ts index 8b4f026..ce25d06 100644 --- a/src/ui/components/ExpectedBand.test.ts +++ b/src/ui/components/ExpectedBand.test.ts @@ -298,6 +298,33 @@ describe("expectedGroups — X3 (a cost with an outstanding slot never also show // throws — is still true, and now trivially so: nothing in the anchored walk // can throw from a confirmation's date any more. describe("expectedGroups — coming-up confirmations never break the fold (formerly X1)", () => { + test("REVIEW ROUND 2 (Minor): a day-granular anchored cost's future slot IS now offered once this month is settled — X1's suppression is gone, not replaced", () => { + // This is deleted test #8's scenario ("stays suppressed even once X3 no + // longer applies") — still perfectly reachable, and now completely + // untested. Pinning what actually happens today rather than leaving the + // gap silent: September's slot is settled on time, so X3 has nothing left + // to suppress, and there is no anchored-model X1 replacement, so the + // October row (39 days from `today`, more than one 30-day cycle) IS + // offered. If a future task reintroduces some suppression here, this test + // will fail loudly and force that change to be deliberate. + const data = baseDataset(); + data.recurring = [ + cost({ + id: "car", + name: "Car insurance", + amount: { amount: 349, currency: "DKK" }, + startDate: "2026-08-14", + recurrence: { kind: "everyNDays", n: 30 }, + anchoring: "lastCharge", + }), + ]; + confirmOccurrence(data, "car", "2026-09-13", { date: "2026-09-13" }); + + const groups = expectedGroups(data, "2026-09", "2026-09-04"); + expect(groups.pending).toEqual([]); + expect(groups.comingUp.map((o) => o.date)).toEqual(["2026-10-13"]); + }); + test("regression: whatever expectedGroups offers as 'coming up', confirming it all today and folding never throws (demo Car insurance shape)", () => { const data = baseDataset(); data.recurring = [