From a58f8b98bcf3159794399187fffdd57167cd96d1 Mon Sep 17 00:00:00 2001 From: le king fu Date: Thu, 8 Oct 2026 21:05:14 -0400 Subject: [PATCH] fix(onboarding): count an import only once the file brought a transaction The "Import a first file" step counted `imported_files` rows in status `completed`. `executeImport` writes that row before its first insert, so an import that added nothing (a file holding only its header, or one whose rows were all in error in the preview) ticked the step: the Import card then said "Done." at rest and sent the user to /transactions, which had nothing to categorize and led back to the import. The counter now keeps a completed file only when a transaction points at it (`EXISTS` on `transactions.file_id`, covered by `idx_transactions_file`). Replayed with sqlite3 on `consolidated_schema.sql`: 0 after an import without a valid row, 0 before a successful import's first insert and 1 after, 0 once the file turns to `error`. Duplicates only change nothing: their transactions came with an earlier completed file, and a restore writes its own. The tests run without SQLite, so the rule is pinned on the query, and the Import page is pinned at rest after such an import: step 4 again, on the first source card, without "Done.". Review of #398 (PR #404), option A chosen by Max. Co-Authored-By: Claude Opus 5.5 --- src/pages/ImportPage.render.test.tsx | 17 ++++++++++++++++- src/services/importedFileService.ts | 5 +++-- src/services/onboardingService.test.ts | 16 +++++++++++++++- src/services/onboardingService.ts | 8 +++++++- src/shared/types/index.ts | 1 + src/utils/onboardingPath.ts | 4 ++-- 6 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/pages/ImportPage.render.test.tsx b/src/pages/ImportPage.render.test.tsx index 0ded6ae..13ddbaa 100644 --- a/src/pages/ImportPage.render.test.tsx +++ b/src/pages/ImportPage.render.test.tsx @@ -230,7 +230,8 @@ describe("ImportPage — the step card (#398)", () => { scannedSources: SOURCES, importReport: report(0), }); - // Duplicates only: the file row is `completed`, the checklist counts it. + // Duplicates only: this file brought no transaction, and the step stays + // checked by the earlier import that brought the ones it duplicates. harness.onboarding = path(IMPORTED, true); const html = render(); expect(sentence(html)).toBe("t(onboarding.card.import.failed[])"); @@ -238,6 +239,20 @@ describe("ImportPage — the step card (#398)", () => { expect(html).not.toContain("onboarding.card.next"); }); + it("after 'Done' on an import that added nothing, the wizard at rest: no 'Done.', step 4 again on the first source card", () => { + // The file row is written, but the import step only counts a file that + // brought a transaction: the re-read keeps the step to do (review of #398). + harness.wizard = wizard({ importFolder: "/home/max/Relevés", scannedSources: SOURCES }); + harness.onboarding = path({ ...ZERO, sources: 1 }, true); + const html = render(); + expect(html).toContain("t(onboarding.card.stepOf[n=4,total=6])"); + expect(html).not.toContain("t(onboarding.card.done)"); + expect(sentence(html)).toBe("t(onboarding.card.import.sourceList[])"); + const target = highlighted(html); + expect(target.inner).toContain("Desjardins"); + expect(target.inner).not.toContain("Tangerine"); + }); + it("after 'Done' or 'Cancel', the wizard at rest: the card still says 'Done.' and points at nothing", () => { harness.wizard = wizard({ importFolder: "/home/max/Relevés", scannedSources: SOURCES }); harness.onboarding = path(IMPORTED, true); diff --git a/src/services/importedFileService.ts b/src/services/importedFileService.ts index a559164..6e6bb95 100644 --- a/src/services/importedFileService.ts +++ b/src/services/importedFileService.ts @@ -39,8 +39,9 @@ export async function createImportedFile(file: { "SELECT id FROM imported_files WHERE source_id = $1 AND filename = $2", [file.source_id, file.filename] ); - // Both branches write the status that the "import a first file" step counts - // (`status = 'completed'`), so each one is announced (ADR 0022). + // Both branches write the status that the "import a first file" step reads + // (`status = 'completed'`, counted once the file brought a transaction), so + // each one is announced (ADR 0022). if (existing.length > 0) { await db.execute( `UPDATE imported_files SET file_hash = $1, row_count = $2, status = $3, notes = $4, import_date = CURRENT_TIMESTAMP WHERE id = $5`, diff --git a/src/services/onboardingService.test.ts b/src/services/onboardingService.test.ts index 883e953..104be6b 100644 --- a/src/services/onboardingService.test.ts +++ b/src/services/onboardingService.test.ts @@ -131,7 +131,6 @@ describe("getOnboardingSnapshot (#351, #396)", () => { expect(db.select).toHaveBeenCalledTimes(4); const sqls = db.select.mock.calls.map((c) => String(c[0])); const aggregate = sqls.find((s) => s.includes("import_sources"))!; - expect(aggregate).toContain("FROM imported_files WHERE status = 'completed'"); expect(aggregate).toContain("FROM transactions WHERE is_manually_categorized = 1"); expect(aggregate).toContain("FROM budget_entries"); expect(aggregate).toMatch(/\(SELECT COUNT\(\*\) FROM transactions\)\s+AS transactions/); @@ -144,6 +143,21 @@ describe("getOnboardingSnapshot (#351, #396)", () => { expect(sqls.filter((s) => s.includes("LIKE 'onboarding_skipped_%'"))).toHaveLength(1); }); + // The tests run without SQLite, so the rule is pinned on the query itself. + // `executeImport` writes the file row `completed` before its first insert: a + // file whose rows were all in error keeps that row and adds no transaction, + // and must not tick "Import a first file" (review of #398). + it("counts a completed import only once the file brought a transaction", async () => { + const { db } = useFakeDb(); + await getOnboardingSnapshot(); + const aggregate = db.select.mock.calls + .map((c) => String(c[0])) + .find((s) => s.includes("import_sources"))!; + expect(aggregate).toMatch( + /\(SELECT COUNT\(\*\) FROM imported_files f\s+WHERE f\.status = 'completed'\s+AND EXISTS \(SELECT 1 FROM transactions t WHERE t\.file_id = f\.id\)\)\s+AS imports/, + ); + }); + it("is a pure read: it never writes, even on a profile D9 would hide", async () => { const { db } = useFakeDb({ transactions: 60 }); const unsubscribe = subscribeDataChanged(() => { diff --git a/src/services/onboardingService.ts b/src/services/onboardingService.ts index d56d11d..bdd78cc 100644 --- a/src/services/onboardingService.ts +++ b/src/services/onboardingService.ts @@ -54,10 +54,16 @@ interface CountersRow { transactions: number; } +// `imports` counts a `completed` file only once it brought a transaction: +// `executeImport` writes the row `completed` before inserting, and a file whose +// rows were all in error (or that holds only its header) keeps that row with +// no transaction. The sub-query is covered by `idx_transactions_file`. const COUNTERS_SQL = ` SELECT (SELECT COUNT(*) FROM import_sources) AS sources, - (SELECT COUNT(*) FROM imported_files WHERE status = 'completed') AS imports, + (SELECT COUNT(*) FROM imported_files f + WHERE f.status = 'completed' + AND EXISTS (SELECT 1 FROM transactions t WHERE t.file_id = f.id)) AS imports, (SELECT COUNT(*) FROM transactions WHERE is_manually_categorized = 1) AS manual_categorized, (SELECT COUNT(*) FROM budget_entries) AS budget_entries, (SELECT COUNT(*) FROM transactions) AS transactions diff --git a/src/shared/types/index.ts b/src/shared/types/index.ts index d1873b1..2d13df2 100644 --- a/src/shared/types/index.ts +++ b/src/shared/types/index.ts @@ -960,6 +960,7 @@ export interface AccountReturn { /** Aggregated counters behind the getting-started checklist, read in one query. */ export interface OnboardingCounters { sources: number; + /** `completed` imported files that brought at least one transaction. */ imports: number; manualCategorized: number; budgetEntries: number; diff --git a/src/utils/onboardingPath.ts b/src/utils/onboardingPath.ts index eb6f0d4..bc60b98 100644 --- a/src/utils/onboardingPath.ts +++ b/src/utils/onboardingPath.ts @@ -280,8 +280,8 @@ function checked(checklist: Checklist | null, key: ChecklistItemKey): boolean { * are tried in order, and the first that applies wins: * 1. the folder is not read yet: no card (null); * 2. an import is running: "Importing…". It comes before anything the - * counters say, because `executeImport` writes the `imported_files` row - * `completed` before inserting a single transaction; + * counters say, because the import step ticks as soon as the first + * transaction of the file lands, long before the import ends; * 3. the report: "Done." when the import added transactions, the failure * sentence when it added none (insertion failed, duplicates only), or when * the count is missing: never "Done." without one;