From db982cb13ec2dfd8fbf197240061b55731b5fe04 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 01:12:21 +0200 Subject: [PATCH 1/3] feat(reports): add splitKind to distinguish deposit-driven splits from line splits Adds SourceReportInvoice.splitKind ('lines' | 'deposits' | 'both' | null), derived alongside isSplit in the existing step-f UNION at zero extra query cost, so the report legend can distinguish a genuine line split from an invoice whose lines all sit in one source but carries a deposit tagged to another. Previously both cases were byte-identical in the response and collapsed to the generic split label instead of the deposit-reduced one. Also removes the unsound budgetLines.length > 0 gate on isDepositReduced, and fixes a second, previously unreported defect: isDepositReduced was over-inclusive, firing on line-split invoices carrying an untagged deposit even though untagged deposits are apportioned pro-rata rather than claimed separately. Fixes #1911 Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude backend-developer Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude e2e-test-engineer Co-Authored-By: Claude product-owner --- .../agent-memory/e2e-test-engineer/MEMORY.md | 1 + .../issue-1911-splitkind-e2e.md | 84 ++++ .claude/agent-memory/product-owner/MEMORY.md | 78 ++-- .../product-owner/bank-report-wizard.md | 137 ++++++ .../qa-integration-tester/MEMORY.md | 1 + .../story-1911-splitkind.md | 116 +++++ .../reports/ReportInvoiceList.test.tsx | 1 + .../reportContent/buildReportContent.test.ts | 160 +++++-- .../lib/reportContent/buildReportContent.ts | 26 +- client/src/lib/reportExclusions.test.ts | 1 + client/src/lib/reportPdf/merge.test.ts | 1 + client/src/lib/reportPdf/realRender.test.ts | 278 +++++++++++- .../ReportWizardPage.aiGeneration.test.tsx | 4 +- .../ReportWizardPage.test.tsx | 10 + .../ReportWizardPage/wizardReducer.test.ts | 1 + e2e/pages/ReportWizardPage.ts | 47 +- .../reportWizardEditableContent.spec.ts | 212 +++++++-- server/src/routes/sourceReports.test.ts | 3 + .../src/services/sourceReportService.test.ts | 410 ++++++++++++++++++ server/src/services/sourceReportService.ts | 31 +- shared/src/lib/reportMath.test.ts | 1 + shared/src/types/sourceReport.ts | 8 + wiki | 2 +- 23 files changed, 1462 insertions(+), 151 deletions(-) create mode 100644 .claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md create mode 100644 .claude/agent-memory/qa-integration-tester/story-1911-splitkind.md diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index ddaab131a..d8696067a 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -32,6 +32,7 @@ - [issue-1959-inline-meta-and-labels.md](issue-1959-inline-meta-and-labels.md) — #1959 reversed #1923's †/‡ footnotes → inline `(partial)`/`(less deposit)` and merged area+attachments into one `.usageMetaText` line; POM renames, rewritten scenarios 2/17/18/20 + AI 8, attachment-tier facts, untested column toggles. - [claim-deposit-scope-1922.md](claim-deposit-scope-1922.md) — PR #1922 invoice/deposit claim-scope split: `handleMarkClaimed`'s two-array submit, server-truth success-banner counts, the three "deposit surfaces the invoice" shapes, `claimNothingClaimable` guard. - [issue-1973-column-visibility-pdf.md](issue-1973-column-visibility-pdf.md) — column-visibility toggles wired to the PDF (supersedes #1966): reducer reset facts (use-case change resets `hiddenColumns`, `SET_ATTACH_DOCUMENTS` doesn't), Scenarios 28-33, size-diff PDF-consequence proof pattern, `goBack()` viewport-independent multi-step walk, new `columnToggleGroup`/`usageHiddenAttachmentsWarning` POM locators. +- [issue-1911-splitkind-e2e.md](issue-1911-splitkind-e2e.md) — #1911 `splitKind` field: incoming spec said "no E2E changes needed", was wrong — fixed Scenario 18's mis-seeded fixture AND found Scenario 17 independently affected (zero-contribution-line case) by cross-referencing unit-test ACs; new AC-3.2 regression-guard sibling test; POM docblock corrections. ## Open follow-ups to flag to orchestrator diff --git a/.claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md b/.claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md new file mode 100644 index 000000000..14287609f --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/issue-1911-splitkind-e2e.md @@ -0,0 +1,84 @@ +--- +name: issue-1911-splitkind-e2e +description: Issue #1911 (SourceReportInvoice.splitKind) E2E fallout in reportWizardEditableContent.spec.ts — dev-team-lead's "no E2E changes needed" spec conclusion was wrong; two scenarios (17 AND 18) were affected, found by cross-referencing server/client unit-test ACs, not by running the browser suite. +metadata: + type: project +--- + +## What #1911 actually changed (semantics, not just a field addition) + +`server/src/services/sourceReportService.ts` step f: `isSplit`/`isDepositReduced` used to be +derived client-side from `invoice.isSplit(raw) && budgetLines.length>0` / +`invoice.isSplit(raw) && deposits.length>0 && !ownTagged`. Both gates were unsound (claim reports +drop zero-contribution budget lines; a foreign-tagged deposit never appears in `deposits[]` at +all). #1911 replaced them with a purely server-derived `splitKind: 'lines'|'deposits'|'both'|null` +(SQL computes `has_foreign_line_source`/`has_foreign_deposit_source` — "this arm contains a source +≠ the reported one", untagged deposits excluded from the deposit arm entirely since the query +filters `budget_source_id IS NOT NULL`). `buildReportContent.ts`: `row.isSplit ⟺ splitKind ∈ +{'lines','both'}`, `row.isDepositReduced ⟺ splitKind ∈ {'deposits','both'}`. `row.isDeposit` +(constituted-deposit badge) trigger is UNCHANGED and now independent of the other two — all three +can co-occur on one row (old code's implicit either/or is gone). + +## Two E2E scenarios were affected, not one — the incoming spec only caught the second + +`e2e/tests/budget/reportWizardEditableContent.spec.ts`: + +- **Scenario 18** ("split + deposit-reduced labels"): `invoice3`'s deposit was seeded + `budgetSourceId: null` (untagged) with a comment claiming that produces `isDepositReduced: true` + — true under the OLD buggy code, false under the fix (this was the literal bug #1911 exists to + fix). Retagged to `otherSourceId` (a source ≠ the reported one) to get the genuine + `splitKind: 'both'` shape. +- **Scenario 17** ("constituted-deposit row … carries NO marker/label"): NOT flagged by the + incoming spec at all, found by grepping every `createDepositViaApi(...budgetSourceId...)` call + site per the task's own hint ("tagged to the reported source" is one of the two shapes to + check). Its invoice has budget lines ENTIRELY on a different source (A) and a deposit tagged to + the reported source (B) itself. Under the OLD code: `isSplit(row)` was gated by + `budgetLines.length>0` for B, which is 0 → false → no `(partial)`. Under NEW code: + `has_foreign_line_source` is true (A's line is foreign to B) regardless of whether B itself has + ANY line contribution → `splitKind: 'lines'` → `isSplit(row)` **true**. This is the "AC 3.1 + zero-contribution-line regression case" explicitly called out in + `client/src/lib/reportContent/buildReportContent.test.ts` (search that phrase) — a unit test + already asserted this new behavior; the E2E suite just hadn't been told. Net: the row now shows + BOTH the "Deposit" badge AND `(partial)`, plus one footnote entry (previously zero). + +**How I found it without a live browser**: cross-referenced `server/src/services/ +sourceReportService.test.ts` (search `Story #1891 regression: invoice with lines only for source B ++ a deposit tagged to source A → isSplit true in both A and B reports`, line ~346) against the +E2E fixture shape — that unit test's `resultB.invoices[0].isSplit` assertion is `true`, which is +the DB-level raw `isSplit` (unchanged by #1911) that Scenario 17's OLD stale comment claimed was +`false`. Then confirmed via `buildReportContent.test.ts`'s "AC 3.1 (regression, #1898/claim +zero-contribution-line drop)" test that the row-level flag inherits this. Static/unit-test +cross-referencing caught a bug the incoming E2E spec missed — worth doing whenever a spec claims +"no E2E changes needed" for a semantic (not just additive) server change. + +## Regression-guard test added + +A new sibling `test()` inside Scenario 18's `describe` block (not folded into the existing test, +so `footnoteItems` can assert count 1 cleanly — the existing test's count is 2 for unrelated +reasons): a split invoice (lines on two sources) + an UNTAGGED deposit must show `(partial)` but +never `(less deposit)`, with exactly one footnote entry. This is literally the fixture shape +Scenario 18's `invoice3` used to have (before being retagged) — without a standalone guard, the +AC 3.2 over-inclusive bug could regress silently since every other test in the file now uses +either no deposit, an own-tagged deposit, or a genuinely-foreign-tagged one. + +## POM docblock corrections + +`e2e/pages/ReportWizardPage.ts` had TWO now-false invariants baked into JSDoc comments (found by +reading the file, not just the test): `depositBadge()`'s doc claimed a constituted-deposit row +"carries no inline note of its own" and `inlineNote()`'s doc claimed such a row "gets NEITHER — +it gets the inline depositBadge instead". Both corrected with an "Issue #1911" note; also added a +dedicated "Issue #1911" paragraph to the class docstring (same location/style as the existing +"Issue #1965" paragraph) so a future reader hits the corrected model before writing a new fixture +against stale assumptions. + +## Reusable lesson + +When a dev-team-lead spec says a server change is "purely additive, no E2E changes needed" for a +field that DRIVES existing conditional rendering, don't take it on faith — grep every fixture that +feeds the changed derivation (here: every `createDepositViaApi` call with `budgetSourceId` null or +equal to the reported/requested source) and check its assertions against the NEW derivation logic, +not just the ONE scenario the spec happened to mention. The unit test suite (already written and +green) is a fast, authoritative way to derive "what SHOULD this fixture shape now assert" without +needing a live browser. + +See [[story-1879-report-wizard]], [[issue-1959-inline-meta-and-labels]]. diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index c90014d75..64eae132d 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -21,9 +21,9 @@ ## Active / Planned Epics -- EPIC-04 (#4) Household Items [Must] — IN PROGRESS, 11 stories (#387-#394, #413, #415, #467). Full detail + per-story review notes in [epic-04-planning.md](epic-04-planning.md) +- EPIC-04 (#4) Household Items [Must] — IN PROGRESS, 11 stories (#387-#394, #413, #415, #467). See [epic-04-planning.md](epic-04-planning.md) - EPIC-16 (#752) Floor Plans & Utility Tracking (2.5D) [Should, Future] — 22 stories (#753-#775), all Backlog, 6 phases. See [epic-16-planning.md](epic-16-planning.md) -- Backlog-only (not started): EPIC-07 Reporting [#7], EPIC-09 Dashboard [#9], EPIC-10 UX Polish [#10]. Deps: 07←05,08; 09←02,03,05; 10←all. +- Backlog-only (not started): EPIC-07 Reporting [#7], EPIC-09 Dashboard [#9], EPIC-10 UX Polish [#10]. Deps: 07←05,08; 09←02,03,05; 10←all. See [epic-09-planning.md](epic-09-planning.md), [epic-17-planning.md](epic-17-planning.md), [epic-18-planning.md](epic-18-planning.md) ## Completed Sprint Story References @@ -31,22 +31,25 @@ ## Standalone bugs & stories (no active parent epic) -Full detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) (budget/invoice) and [standalone-diary-bugs.md](standalone-diary-bugs.md) / [standalone-photo-stories.md](standalone-photo-stories.md). Growing budget/invoice cluster (20+) → propose new "Budget/Invoice UX Polish" epic next planning cycle. Auto-itemize is standalone (no parent epic): stories #1545-#1547 + bug fixes. - -- Budget/invoice batches: #1369-#1373 (2026-04-28), #1389-#1390 (2026-04-29), #1401 (2026-05-10), #1421-#1425 (2026-05-15), #1439-#1441 (2026-05-17), #1553 (2026-05-22) -- Auto-itemize: #1545/#1546/#1547 mini-epic (2026-05-21), #1600 (2026-05-26), **#1833 duplicate budget lines on commit retry (2026-07-07)** -- **Auth rate limits: #1970** — PR #1989 APPROVED round 3 (2026-08-04), M1 resolved (`5446b29a`), all 7 ACs met → **Done on merge**. Follow-ups #1990/#1991/#1992 stay open. Detail in [auth-rate-limits-1970.md](auth-rate-limits-1970.md) — incl. verifying "proves X reached the route" assertions **by local mutation + revert**, and checking a numeric-header probe is deterministic (which request in the window it observes). -- Diary: #1426 critical photo data loss (2026-05-15) -- Photo: #1723 lightbox picker UX (2026-06-16) -- **DataTable: #1955** two-column toggle race silently hides 2nd column, all 6 DataTable pages (Should Have, S, Backlog, 2026-08-02). See [datatable-column-preference-race.md](datatable-column-preference-race.md) — records that **fast clicking is the SAFE case** (I judged this backwards; debounce `clearTimeout` coalesces rapid input, the >500ms reading-pace gap is the reachable one) and that #1920's E2E-only fix (`InvoicesPage.enableColumn()` awaits the PATCH) makes CI green **without** fixing production — don't close #1955 on a green shard. -- **#1957** latent cross-file E2E test-isolation hazard (shared-admin `user_preferences` writes under `fullyParallel`, `LocaleContext.syncWithServer` actively flips a victim test's locale) — Should Have, bug, Backlog, 2026-08-02/03, filed from `/fix-e2e` work on PR #1956. Scoped as an audit + per-spec sweep, not a single-file fix — found a second live instance (`diary-uat-fixes.spec.ts` vs `dashboard.spec.ts`, key `dashboard.hiddenCards`) while researching it. Distinct from #1955 (production race) and #1920 (E2E workaround for #1955). Detail in [e2e-shared-admin-preference-hazard.md](e2e-shared-admin-preference-hazard.md). -- **PR #2008 (#2003 ADR-034 overflow enforcement + #1980 legend assertions)** — **REJECTED round 1** (`664bf048`, 2026-08-05), verdict in a comment (human-authored PR). Three criticals: **C1** the new `max(horizontalRatio) <= 1` assertions are the constant `0` — pdfmake records the write cursor's _left_ edge (`DocumentContext.js:515-529`), so a left-margin-anchored full-width table can never exceed 0; proven by 3 mutations (`WORST_CASE_CHAR_ADVANCE_EM 1.04→0.1` and `VENDOR_WIDTH 45→600` both stayed green; inverting the bound to `-1` printed `Received: 0` at all 8 sites in all states). **C2** the ADR pointer claims the assertion is "non-vacuous", refuted by the PR's own helper comment. **C3** the new E2E test is red twice (NBSP: `locator.textContent()` doesn't normalize, `toContainText()` does). New reusable patterns: **invert an inequality's bound to read the measured value**, **a synthetic revert-test proves the helper not the assertion**, **a comment conceding non-coverage IS the finding**, **a documented ADR bar can be unmeasurable — escalate as a rule correction**. → **APPROVED round 2** (`4f4b93e3`): metric replaced by per-cell `_minWidth <= widths[i]._calcWidth` (8 assertions, claim/budget-overview × en/de), ADR-034 rule #1 corrected (3rd correction + Deviation Log), **#2003's body amended with a supersession block** so the dead `<= 1` bullet isn't read as live spec. Round-2 patterns: **verify a vacuity fix by re-running the exact mutations that stayed green last round** (both went green→all-8-fail); **"moves with the input" is the positive signal** (`212.93`/`7.10`/~`33`; legend `_minWidth` 36.80 en vs 54.86 de); **when a fix removes a vacuous assertion, check it was REPLACED not just deleted** (#1980 AC1 went vacuous→**absent**, carried as Medium — re-grading my own round-1 Medium upward would move goalposts); **probe that a measurement exists before demanding it**; **prove an E2E failure is a flake via a byte-identical-subtree green run** (`git diff -- e2e/` empty + all-16-green 16 min earlier + shard 3/16 green last round rules out a shard-boundary shift). #2003 Done on merge; **#1980 Done only if M1 lands**. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"PR #2008". -- **PR #2007 (#2001 remove `TFunction` from `reportPdf/*`)** — **APPROVED round 1** (`df7d2c73`, 2026-08-05), all 9 ACs met, **Done on merge** (every AC machine-checkable, no UAT). One Medium non-blocking: the fix re-opened the same hazard class in a new form (`labels.skipReasonLabels[reason as 'a'|'b'] ?? reason` — unchecked cast + silent fallback echoes a raw identifier if a third `SkippedDocument['reason']` is added). **Pattern: when a story's purpose is "close the class", check whether the fix introduces a fresh instance of it.** Verification: grep ACs get run directory-wide _and_ one step past the AC text (also checked `i18n`/`useTranslation`/`toLocale`/`Intl.`); AC7 discrimination proven by 3 mutations in a throwaway `/tmp` worktree (symlink root `node_modules`) — **mutate the i18n _key_, not just the value**; call-time→build-time refactors need a `useMemo` dep-array staleness check. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"#2001 reviewed". -- **#2005** now **CLOSED** — all 16 E2E shards green on `df7d2c73`; the promotion blocker below is cleared. E2E shard 8/16 red across four beta PRs — dashboard "New Invoice" (Scenario 13, #1735) opens no modal in either Paperless branch, fails on retry too (bug, Must Have, Todo, 2026-08-05, filed from PR #2004 round-5 sign-off). **Blocks the next `beta`→`main` promotion** (`E2E Gates` is main-only). Proved PR-independent by checking the head commits of #1999/#2000/#2002 — not `beta`, which never runs full E2E. AC1 is a _classification_ AC so the dev-team-lead keeps the Test Failure Debugging Protocol call. Detail in [bank-report-wizard.md](bank-report-wizard.md) §"round 5". -- **PR #2004 (#1888 + #1910)** — **APPROVED, M1 closed round 5** (`6a3eb7ec`, 2026-08-05): `lang={lang}` on the `