Files
sure/app/controllers/imports_controller.rb
T
Juan José MataandClaude Opus 5 8ca65ffd27 Reconcile PDF statement imports against transactions that already exist (#3105)
* 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 in f5ba646: 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 on f5ba646.

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 on eac82cf.

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>
2026-08-22 03:14:03 +02:00

289 lines
9.5 KiB
Ruby

class ImportsController < ApplicationController
include SettingsHelper
before_action :set_import, only: %i[show update publish destroy revert apply_template cancel summary]
before_action :require_statement_import_permission!, only: %i[update publish destroy revert apply_template cancel]
def update
# Handle both pdf_import[account_id] and import[account_id] param formats
account_id = params.dig(:pdf_import, :account_id) || params.dig(:import, :account_id)
if account_id.present?
account = accessible_accounts.find_by(id: account_id)
unless account
redirect_back_or_to import_path(@import), alert: t("imports.update.invalid_account", default: "Account not found.")
return
end
return if @import.account_statement.present? && !require_account_permission!(account)
if @import.is_a?(PdfImport)
# Refused for an import whose data already landed -- see
# PdfImport#reassignable?. A replayed or back-button PATCH gets the
# explanation rather than a silent unwind.
unless @import.assign_account!(account)
redirect_back_or_to import_path(@import), alert: t("imports.update.account_locked")
return
end
else
@import.update!(account: account)
end
end
redirect_to import_path(@import), notice: t("imports.update.account_saved", default: "Account saved.")
end
def publish
@import.publish_later
redirect_to import_path(@import), notice: t(".started")
rescue Import::MaxRowCountExceededError
redirect_back_or_to import_path(@import), alert: t(".max_rows_exceeded", max: @import.max_row_count)
end
# Statement imports only: what became of each extracted transaction. Without it
# a statement whose lines all matched finishes on the generic complete screen,
# which cannot distinguish "everything was already recorded" from "nothing was
# found".
def summary
raise ActiveRecord::RecordNotFound unless @import.is_a?(PdfImport)
end
def cancel
if @import.force_fail!
redirect_to imports_path, notice: t(".cancelled")
else
redirect_to imports_path, alert: t(".not_cancellable")
end
end
def index
@pagy, @imports = pagy(Current.family.imports.where(type: Import::TYPES).ordered, limit: safe_per_page)
@breadcrumbs = [
[ t("breadcrumbs.home"), root_path ],
[ t("breadcrumbs.imports"), imports_path ]
]
respond_to do |format|
format.html { render layout: "settings" }
end
end
def new
@pending_import = Current.family.imports.ordered.pending.first
@document_upload_extensions = document_upload_supported_extensions
end
def create
file = import_params[:import_file]
if file.present? && document_upload_request?
create_document_import(file)
return
end
if file.present? && sure_import_request?
create_sure_import(file)
return
end
# Handle PDF file uploads - process with AI
if file.present? && Import::ALLOWED_PDF_MIME_TYPES.include?(file.content_type)
unless valid_pdf_file?(file)
redirect_to new_import_path, alert: t("imports.create.invalid_pdf")
return
end
create_pdf_import(file)
return
end
type = params.dig(:import, :type).to_s
type = "TransactionImport" unless Import::TYPES.include?(type)
account = accessible_accounts.find_by(id: params.dig(:import, :account_id))
import = Current.family.imports.create!(
type: type,
account: account,
date_format: Current.family.date_format,
)
if file.present?
if file.size > Import::MAX_CSV_SIZE
import.destroy
redirect_to new_import_path, alert: t("imports.create.file_too_large", max_size: Import::MAX_CSV_SIZE / 1.megabyte)
return
end
unless Import::ALLOWED_CSV_MIME_TYPES.include?(file.content_type)
import.destroy
redirect_to new_import_path, alert: t("imports.create.invalid_file_type")
return
end
# Stream reading is not fully applicable here as we store the raw string in the DB,
# but we have validated size beforehand to prevent memory exhaustion from massive files.
import.update!(raw_file_str: file.read)
redirect_to import_configuration_path(import), notice: t("imports.create.csv_uploaded")
else
redirect_to import_upload_path(import)
end
end
def show
unless @import.requires_csv_workflow?
redirect_to import_upload_path(@import), alert: t("imports.show.finalize_upload") unless @import.uploaded?
return
end
if !@import.uploaded?
redirect_to import_upload_path(@import), alert: t("imports.show.finalize_upload")
elsif !@import.publishable?
next_path = @import.mapping_steps.empty? ? import_clean_path(@import) : import_confirm_path(@import)
redirect_to next_path, alert: t("imports.show.finalize_mappings")
end
end
def revert
@import.revert_later
redirect_to imports_path, notice: t(".started")
end
def apply_template
if @import.suggested_template
@import.apply_template!(@import.suggested_template)
redirect_to import_configuration_path(@import), notice: t(".template_applied")
else
redirect_to import_configuration_path(@import), alert: t(".no_template_found")
end
end
def destroy
@import.destroy
redirect_to imports_path, notice: t(".deleted")
end
private
def set_import
@import = Current.family.imports.includes(:account, :account_statement).find(params[:id])
raise ActiveRecord::RecordNotFound if @import.account_statement.present? && !@import.account_statement.viewable_by?(Current.user)
end
def import_params
params.require(:import).permit(:import_file)
end
def require_statement_import_permission!
return if @import.account_statement.blank? || @import.account_statement.manageable_by?(Current.user)
redirect_target = @import.account || @import.account_statement
redirect_back_or_to redirect_target, alert: t("accounts.not_authorized")
end
def create_pdf_import(file)
return redirect_to new_import_path, alert: t("accounts.not_authorized") unless AccountStatement.statement_manager?(Current.user)
return redirect_to new_import_path, alert: t("imports.create.pdf_too_large", max_size: Import::MAX_PDF_SIZE / 1.megabyte) if file.size > Import::MAX_PDF_SIZE
pdf_import = PdfImport.create_from_upload!(family: Current.family, file: file, user: Current.user)
pdf_import.process_with_ai_later
redirect_to import_path(pdf_import), notice: t("imports.create.pdf_processing")
rescue AccountStatement::DuplicateUploadError
redirect_to new_import_path, alert: t("imports.create.duplicate_pdf_unavailable")
rescue AccountStatement::InvalidUploadError
redirect_to new_import_path, alert: t("imports.create.invalid_pdf")
end
def create_document_import(file)
adapter = VectorStore.adapter
unless adapter
redirect_to new_import_path, alert: t("imports.create.document_provider_not_configured")
return
end
if file.size > Import::MAX_PDF_SIZE
redirect_to new_import_path, alert: t("imports.create.document_too_large", max_size: Import::MAX_PDF_SIZE / 1.megabyte)
return
end
filename = file.original_filename.to_s
ext = File.extname(filename).downcase
supported_extensions = adapter.supported_extensions.map(&:downcase)
unless supported_extensions.include?(ext)
redirect_to new_import_path, alert: t("imports.create.invalid_document_file_type")
return
end
if ext == ".pdf"
unless valid_pdf_file?(file)
redirect_to new_import_path, alert: t("imports.create.invalid_pdf")
return
end
create_pdf_import(file)
return
end
family_document = Current.family.upload_document(
file_content: file.read,
filename: filename
)
if family_document
redirect_to new_import_path, notice: t("imports.create.document_uploaded")
else
redirect_to new_import_path, alert: t("imports.create.document_upload_failed")
end
end
def document_upload_supported_extensions
adapter = VectorStore.adapter
return [] unless adapter
adapter.supported_extensions.map(&:downcase).uniq.sort
end
def document_upload_request?
params.dig(:import, :type) == "DocumentImport"
end
def sure_import_request?
params.dig(:import, :type) == "SureImport"
end
def create_sure_import(file)
if file.size > SureImport::MAX_NDJSON_SIZE
redirect_to new_import_path, alert: t("imports.create.file_too_large", max_size: SureImport::MAX_NDJSON_SIZE / 1.megabyte)
return
end
ext = File.extname(file.original_filename.to_s).downcase
unless ext.in?(%w[.ndjson .json])
redirect_to new_import_path, alert: t("imports.create.invalid_ndjson_file_type")
return
end
content = file.read
file.rewind
unless SureImport.valid_ndjson_first_line?(content)
redirect_to new_import_path, alert: t("imports.create.invalid_ndjson_file_type")
return
end
import = Current.family.imports.create!(type: "SureImport")
import.ndjson_file.attach(
io: StringIO.new(content),
filename: file.original_filename,
content_type: file.content_type
)
import.sync_ndjson_rows_count!
redirect_to import_path(import), notice: t("imports.create.ndjson_uploaded")
end
def valid_pdf_file?(file)
header = file.read(5)
file.rewind
header&.start_with?("%PDF-")
end
end