mirror of
https://github.com/we-promise/sure.git
synced 2026-09-03 13:51:29 +00:00
Juan José MataandClaude Opus 5
8ca65ffd27
* Reset to main, keeping only the account matcher improvements Backs out the LLM-driven reconciliation work (PR #1382's approach and the two commits hardening it). That approach compares whole-statement aggregates, which is all-or-nothing: when 18 of 20 transactions have already synced the totals disagree, the import proceeds, and 18 duplicates are created. Reconciliation is a row-level problem and belongs in the import path, where TransactionImport already solves it via Account::ProviderImportAdapter. Kept from that work, because it stands on its own: - AccountMatcher gains a hint-based class-level entry point so callers without an AccountStatement row can score against the same rules. The instance path used by AccountStatement#assign_account_match is unchanged. - It also refuses to guess between equally-confident candidates rather than letting max_by take whichever the scan reached first. Account names are not unique within a family, so that tie was reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Reconcile PDF statement imports against transactions that already exist Reconciliation is a row-level problem. Comparing whole-statement totals is all-or-nothing: when 18 of 20 transactions have already synced the totals disagree, the import proceeds, and 18 duplicates are created. This matches each extracted transaction against what the account already holds, so only genuinely new transactions are ever offered for import and the rest are marked reconciled. TransactionImport#import! already does this for CSV via Account::ProviderImportAdapter. PdfImport#import! called it zero times and built one Transaction per row unconditionally -- the only import path in the repo with no duplicate protection. Reconciliation state follows Quicken's uncleared / cleared / reconciled, but only the last state is stored: - "Cleared" means the institution acknowledged the transaction, which is exactly what entries.source and entries.external_id already record. It stays accurate on its own, because the adapter stamps both onto a manual entry when a provider transaction claims it, so a hand-entered transaction that later appears in a download becomes cleared with no extra bookkeeping. Deriving it also keeps it non-editable, which is right: it is a fact about provenance, not an opinion. - "Reconciled" means a statement was matched against the transaction. Nothing can derive that, so entries gains reconciled_at and reconciled_by_statement_id. It is a judgement, so it can be set and unset, and it survives the statement being deleted (the FK nullifies, the timestamp stays). Matching: - find_duplicate_transaction grows include_provider_entries, which is what makes this work for Provider-backed accounts -- the existing where(external_id: nil) filter hid synced transactions from every import path, so this gap affected CSV imports equally. Default stays false: provider sync must not claim another provider's entry. - It also grows date_window, because a statement's posting date routinely differs by a day or two from the date a provider recorded. Nearest date wins. - Name is deliberately not matched on: statement descriptions and provider names for the same transaction rarely agree. The adapter makes the same choice for sync. - Candidates are built as real Import::Row objects so matching uses the same signed_amount and date_iso the import itself would write, rather than a second interpretation of signage that could drift. - Matching is per-account, so with no account assigned every row is offered and re-judged on assignment; reassigning also releases the previous account's reconciliations. - A row whose date or amount will not parse is offered for import rather than dropped, so nothing disappears silently. - import! re-checks at publish, since a sync can land between review and publish, and new transactions are born reconciled: the statement is their evidence. Provider-backed accounts are now offered in the import target picker. The manual-only restriction existed because importing into a synced account would duplicate what sync brought in, which is precisely what this removes. Also fixes a bug this uncovered on main: extract_transactions stored the extractor's symbol-keyed hash, while every reader digs with strings. jsonb keeps the hash as assigned until reload and ProcessPdfJob never reloads, so has_extracted_transactions? was false and PDF imports generated zero rows. The existing tests missed it -- one uses a YAML fixture, the other stubs the extractor with string keys. Supersedes #1382. Refs #1379. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Fix three review findings in statement reconciliation All three confirmed against the code before fixing. Publish-time recheck consumed the same entry twice. import! started its exclusion list empty, so a statement carrying two same-amount transactions against an account holding only one would re-match the surviving row against the entry row generation had already consumed -- silently dropping a genuinely new transaction instead of creating it. Seed the exclusions with the entries this statement already reconciled; newly synced entries are still caught, since only already-reconciled ones are excluded. A regeneration that emptied the row set left the import stranded. In the normal upload flow the account is assigned after extraction, so assign_account! regenerates -- and if everything then reconciled, rows_count went to zero while status stayed pending. _pdf_import.html.erb renders pending-with-no-rows as the processing screen, and process_with_ai_later cannot restart because ai_processed? is already true, so the import was stuck with no way forward. Status now follows the same rule ProcessPdfJob applies after initial processing, in both directions: no rows completes it, rows returning sends it back to pending. Guarded by data_committed? so a published import is never reopened. Unevaluatable rows went only to the Rails log. AGENTS.md asks for DebugLogEntry.capture on recoverable import failures so they surface in /settings/debug with structured context. Capture family, account, import, statement, row number and the raw date/amount that would not parse. Adds regression coverage for each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Fix row regeneration collision and over-broad reconciliation release test_unit caught one error in 6462 tests, and it was real. Row regeneration collided on the second call. insert_all! bypasses ActiveRecord, so the rows association is never populated with what it wrote. Calling generate_rows_from_extracted_data twice on the same in-memory record -- which assign_account! now does after ProcessPdfJob has already generated once -- made rows.destroy_all clear a stale empty collection, delete nothing, and then collide on (import_id, source_row_number). Reload before destroying, and reset the association after inserting so sync_mappings and the view read what was actually written. Releasing reconciliations was scoped to the statement, not the account. A statement is evidence for exactly one account at a time but can back more than one import, so reassigning an account cleared reconciliations another account still relied on. Scoped to the account being moved away from; a blank scope releases nothing, which is correct because nothing is reconciled while no account is assigned. The second finding was raised by CodeRabbit. Its other flagged risk -- entries being marked reconciled before the import is published -- is deliberate and stays: the statement is the evidence, reconciling is reversible via unmark_reconciled!, and deferring it to publish would leave a fully reconciled statement with nothing to publish and therefore nothing ever marked. Adds regression coverage for both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Address review nitpicks: model validation, scope placement, mock style, lock-safe DDL All four checked against the repo's own conventions before applying. Mirror the check constraint as a model validation. Assigning reconciled_by_statement without reconciled_at raised StatementInvalid rather than a validation error. CLAUDE.md Convention 5 asks for exactly this pairing -- the constraint in the database, an ActiveRecord validation for form-friendly errors. Stop hijacking the pending-scope comment. The reconciliation scopes were inserted directly under "Pending transaction scopes", so that header read as documentation for them and the provider note below read as a continuation of reconciled_by. Given the reconciliation scopes their own header. Use OpenStruct for the provider response double, per "Always prefer OpenStruct when creating mock instances". Verified OpenStruct.new(success?: true) responds to success?, and ostruct is already a dependency used elsewhere in test/. Make the migration lock-safe. entries is the largest table in the app: both indexes now build concurrently, and the check constraint is added unvalidated then validated separately so VALIDATE takes only SHARE UPDATE EXCLUSIVE instead of holding ACCESS EXCLUSIVE for a full scan. This follows existing practice -- 13 migrations already use disable_ddl_transaction! and 11 use algorithm: :concurrently, with add_offline_reason_to_securities combining add_column and a concurrent index in one migration exactly like this. The suggested follow-up migration for validation was not needed: validating in the same non- transactional migration gets the same lock behavior without a second file, and the repo has no validate: false precedent in 400 migrations. schema.rb is unchanged: a validated constraint and a concurrently-built index dump identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Add the two review-requested regression tests Covers the invalid-amount half of "malformed rows are offered, not dropped" -- the existing test only exercised an unparseable date. Asserts the raw value is stored verbatim rather than coerced to 0, so the review step shows the user what the statement actually said. Also covers the Entry validation added inf5ba646: assigning reconciled_by_statement without reconciled_at must fail model validation rather than reaching chk_entries_reconciled_at_present_when_statement_set and raising StatementInvalid. Both requested by CodeRabbit onf5ba646. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Assert both reconciliation fields when an account is reassigned reconciled? only reads reconciled_at, so the test proved the state was cleared but not that the statement evidence went with it, nor that the sibling account kept its own. Entry#unmark_reconciled! clears the pair, so assert the pair. Raised by CodeRabbit oneac82cf. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Release reconciliation on revert and guard account reassignment Two correctness findings from review. Import#revert destroyed the import's own entries but nothing else, so a statement import left stale evidence behind: entries it had only *matched* kept reconciled_at and reconciled_by_statement_id pointing at a statement that no longer claimed them. Worse, a statement that reconciled every line carries zero rows, so revert returned it to pending with rows_count 0 -- the exact combination the pdf import view renders as the processing screen, with no way to regenerate rows or re-trigger extraction. Import#revert now calls two hooks inside its transaction: revert_derived_state! for state a subclass keeps outside its own rows and entries, and status_after_revert for where the record lands. The base behavior is unchanged. PdfImport releases its reconciliations, re-judges every statement line against what the account actually holds now, and finishes as complete when there is nothing left to offer. PdfImport#assign_account! had no guard against an already-published import. A back-button or replayed PATCH ran release_reconciliations! and generate_rows_from_extracted_data unconditionally, releasing evidence and destroying the rows that documented what was published while the created entries stayed put -- only refresh_status_after_regeneration! checked data_committed?. It now takes the row lock, refuses when the import has committed data or a job owns the record, and returns false so the controller can explain rather than report a save that did not happen. An import that reconciled every line is still re-targetable: it is complete, but committed nothing of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Show what became of each statement transaction Row-level matching made the import's outcome invisible. A statement whose lines were all already on file finishes with rows_count 0 and renders the generic "Document analyzed" screen, which cannot be told apart from a statement nothing was extracted from. The user is told the import is done and nothing else. Two counts also became wrong rather than merely absent. The ready-for-review screen labels rows_count as "Transactions Extracted", but after matching that is the *unmatched* count: a 20-line statement against an account holding 18 of them read "2 transactions extracted". ready_for_review_description made the same claim in prose. Adds a summary dialog at GET /imports/:id/summary, linked from both the ready-for-review and complete screens, breaking the statement down into what was found, what was already recorded, what was imported, and what is still waiting. The counts are derived from the entries rather than stored, so they stay true if a later sync or edit changes the picture. Two of them need care. Entries this import creates are born reconciled, so already_recorded_count has to exclude them or it double-counts what the account genuinely already had. And publishing does not destroy rows -- they remain as the record of what was written -- so awaiting_review_count reports zero once the data is committed rather than repeating rows_count. The complete screen now explains a fully reconciled import instead of claiming to have found something, and the extracted count reports the statement's real size with the matched count beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Memoize the import outcome counts Rendering the summary dialog issued roughly fifteen queries for four numbers. already_recorded_count, imported_count and awaiting_review_count are each read two or three times per template, every read is its own COUNT, reconciled_anything? adds another by calling already_recorded_count internally, and awaiting_review_count consults data_committed? -- two more EXISTS queries -- on every invocation. Memoizing on the model rather than assigning locals in the template fixes the review screen too, which reads the same counts, and keeps the arithmetic out of the view. These report a finished outcome for display. Anything that re-judges the import recomputes from the entries directly, so a value cached for the life of the request is what the callers want. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q --------- Co-authored-by: Claude <noreply@anthropic.com>