From 0afdb1d0fde51ed8baf265b44d3ccc54637fe962 Mon Sep 17 00:00:00 2001 From: MkDev11 <94194147+MkDev11@users.noreply.github.com> Date: Mon, 2 Feb 2026 10:27:02 -0500 Subject: [PATCH] Feature/pdf import transaction rows (#846) * Add import row generation from PDF extracted data - Add generate_rows_from_extracted_data method to PdfImport - Add import! method to create transactions from PDF rows - Update ProcessPdfJob to generate rows after extraction - Update configured?, cleaned?, publishable? for PDF workflow - Add column_keys, required_column_keys, mapping_steps - Set bank statements to pending status for user review - Add tests for new functionality Closes #844 * Add tests for BankStatementExtractor - Test transaction extraction from PDF content - Test deduplication across chunk boundaries - Test amount normalization for various formats - Test graceful handling of malformed JSON responses - Test error handling for empty/nil PDF content * Fix supports_pdf_processing? to validate effective model The validation was always checking @default_model, but process_pdf allows overriding the model via parameter. This could cause a vision-capable override model to be rejected, or a non-vision-capable override to pass validation only to fail during processing. Changes: - supports_pdf_processing? now accepts optional model parameter - process_pdf passes effective model to validation - Raise Provider::Openai::Error inside with_provider_response for consistent error handling Addresses review feedback from PR#808 * Fix insert_all! bug: explicitly set import_id Rails insert_all! on associations does NOT auto-set the foreign key. Added import_id explicitly and use Import::Row.insert_all! directly. Also reload rows before counting to ensure accurate count. * Fix pending status showing as processing for bank statements with rows When bank statement PDF imports have extracted rows, show a 'Ready for Review' screen with a link to the confirm path instead of the 'Processing' spinner. This addresses the PR feedback that users couldn't reach the review flow even though rows were created. * Gate publishable? on account.present? to prevent import failure PDF imports are created without an account, and import! raises if account is missing. This prevents users from hitting publish and having the job fail. * Wrap generate_rows_from_extracted_data in transaction for atomicity - Clear rows and reset count even when no transactions extracted - Use transaction block to prevent partial updates on failure - Use mapped_rows.size instead of reload for count * Localize transactions count string with i18n helper * Add AccountMapping step for PDF imports when account is nil PDF imports need account selection before publishing. This adds Import::AccountMapping to mapping_steps when account is nil, matching the behavior of TransactionImport and TradeImport. Addresses PR#846 feedback about account selection for PDF imports. * Only include CategoryMapping when rows have non-empty categories PDF extraction doesn't extract categories from bank statements, so the CategoryMapping step would show empty. Now we only include CategoryMapping if rows actually have non-empty category values. This prevents showing an empty mapping step for PDF imports. * Fix PDF import UI flow and account selection - Add direct account selection in PDF import UI instead of AccountMapping - AccountMapping designed for CSV imports with multiple account values - PDF imports need single account for all transactions - Add update action and route for imports controller - Fix controller to handle pdf_import param format from form_with - Show Publish button when import is publishable (account set) - Fix stepper nav: Upload/Configure/Clean non-clickable for PDF imports - Redirect PDF imports from configuration step (auto-configured) - Improve AI prompt to recognize M-PESA/mobile money as bank statements - Fix migration ordering for import_rows table columns * Add guard for invalid account_id in imports#update Prevents silently clearing account when invalid ID is passed. Returns error message instead of confusing 'Account saved' notice. * Localize step names in import nav and add account guard - Use t() helper for all step names (Upload, Configure, Clean, Map, Confirm) - Add guard for invalid account_id in imports#update - Prevents silently clearing account when invalid ID is passed * Make category column migrations idempotent Check if columns exist before adding to prevent duplicate column errors when migrations are re-run with new timestamps. * Add match_path for PDF import step highlighting Fixes step detection when path is nil by using separate match_path for current step highlighting while keeping links disabled. * Rename category migrations and update to Rails 7.2 - Rename class to EnsureCategoryFieldsOnImportRows to avoid conflicts - Rename class to EnsureCategoryIconOnImportRows - Update migration version from 7.1 to 7.2 per guidelines - Rename files to match class names - Add match_path for PDF import step highlighting * Use primary (black) style for Create Account and Save buttons * Remove match_path from auto-completed PDF steps Only step 4 (Confirm) needs match_path for active-step detection. Steps 1-3 are purely informational and always complete. * Add fallback for document type translation Handles nil or unexpected document_type values gracefully. Also removes match_path from auto-completed PDF steps. * Use index-based step number for mobile indicator Fixes 'Step 5 of 4' issue when Map step is dynamically removed. * Fix hostings_controller_test: use blank? instead of nil Setting returns empty string not nil for unset values. * Localize step progress label and use design token * Fix button styling: use design system Tailwind classes btn--primary and btn--secondary CSS classes don't exist. Use actual design system classes from DS::Buttonish. * Fix CRLF line endings in tags_controller_test.rb --------- Co-authored-by: mkdev11 --- .../import/configurations_controller.rb | 2 + app/controllers/imports_controller.rb | 18 +- app/jobs/process_pdf_job.rb | 13 +- app/models/pdf_import.rb | 87 +++++++- app/models/provider/openai.rb | 9 +- app/models/provider/openai/pdf_processor.rb | 2 +- app/views/imports/_nav.html.erb | 40 ++-- app/views/imports/_pdf_import.html.erb | 67 +++++- config/routes.rb | 2 +- ...0000_add_category_fields_to_import_rows.rb | 7 - ...000001_add_category_icon_to_import_rows.rb | 5 - ...9_ensure_category_fields_on_import_rows.rb | 7 + ...220_ensure_category_icon_on_import_rows.rb | 5 + .../settings/hostings_controller_test.rb | 2 +- test/fixtures/imports.yml | 21 ++ test/models/pdf_import_test.rb | 84 +++++++- .../openai/bank_statement_extractor_test.rb | 197 ++++++++++++++++++ 17 files changed, 515 insertions(+), 53 deletions(-) delete mode 100644 db/migrate/20240701000000_add_category_fields_to_import_rows.rb delete mode 100644 db/migrate/20240701000001_add_category_icon_to_import_rows.rb create mode 100644 db/migrate/20240925112219_ensure_category_fields_on_import_rows.rb create mode 100644 db/migrate/20240925112220_ensure_category_icon_on_import_rows.rb create mode 100644 test/models/provider/openai/bank_statement_extractor_test.rb diff --git a/app/controllers/import/configurations_controller.rb b/app/controllers/import/configurations_controller.rb index 12e47a477..6602e3fbe 100644 --- a/app/controllers/import/configurations_controller.rb +++ b/app/controllers/import/configurations_controller.rb @@ -4,6 +4,8 @@ class Import::ConfigurationsController < ApplicationController before_action :set_import def show + # PDF imports are auto-configured from AI extraction, skip to clean step + redirect_to import_clean_path(@import) if @import.is_a?(PdfImport) end def update diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 88a346838..f1a217529 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -1,7 +1,23 @@ class ImportsController < ApplicationController include SettingsHelper - before_action :set_import, only: %i[show publish destroy revert apply_template] + before_action :set_import, only: %i[show update publish destroy revert apply_template] + + 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 = Current.family.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 + @import.update!(account: account) + end + + redirect_to import_path(@import), notice: t("imports.update.account_saved", default: "Account saved.") + end def publish @import.publish_later diff --git a/app/jobs/process_pdf_job.rb b/app/jobs/process_pdf_job.rb index 25c31f11f..8fb4fccef 100644 --- a/app/jobs/process_pdf_job.rb +++ b/app/jobs/process_pdf_job.rb @@ -5,18 +5,22 @@ class ProcessPdfJob < ApplicationJob return unless pdf_import.is_a?(PdfImport) return unless pdf_import.pdf_uploaded? return if pdf_import.status == "complete" - return if pdf_import.ai_processed? && (!pdf_import.bank_statement? || pdf_import.has_extracted_transactions?) + return if pdf_import.ai_processed? && (!pdf_import.bank_statement? || pdf_import.rows_count > 0) pdf_import.update!(status: :importing) begin pdf_import.process_with_ai - # For bank statements, extract transactions + # For bank statements, extract transactions and generate import rows if pdf_import.bank_statement? Rails.logger.info("ProcessPdfJob: Extracting transactions for bank statement import #{pdf_import.id}") pdf_import.extract_transactions Rails.logger.info("ProcessPdfJob: Extracted #{pdf_import.extracted_transactions.size} transactions") + + pdf_import.generate_rows_from_extracted_data + pdf_import.sync_mappings + Rails.logger.info("ProcessPdfJob: Generated #{pdf_import.rows_count} import rows") end # Find the user who created this import (first admin or any user in the family) @@ -26,7 +30,10 @@ class ProcessPdfJob < ApplicationJob pdf_import.send_next_steps_email(user) end - pdf_import.update!(status: :complete) + # Bank statements with rows go to pending for user review/publish + # Non-bank statements are marked complete (no further action needed) + final_status = pdf_import.bank_statement? && pdf_import.rows_count > 0 ? :pending : :complete + pdf_import.update!(status: final_status) rescue StandardError => e sanitized_error = sanitize_error_message(e) Rails.logger.error("PDF processing failed for import #{pdf_import.id}: #{e.class.name} - #{sanitized_error}") diff --git a/app/models/pdf_import.rb b/app/models/pdf_import.rb index 8b25e8bfa..0e0250462 100644 --- a/app/models/pdf_import.rb +++ b/app/models/pdf_import.rb @@ -3,6 +3,34 @@ class PdfImport < Import validates :document_type, inclusion: { in: DOCUMENT_TYPES }, allow_nil: true + def import! + raise "Account required for PDF import" unless account.present? + + transaction do + mappings.each(&:create_mappable!) + + new_transactions = rows.map do |row| + category = mappings.categories.mappable_for(row.category) + + Transaction.new( + category: category, + entry: Entry.new( + account: account, + date: row.date_iso, + amount: row.signed_amount, + name: row.name, + currency: row.currency, + notes: row.notes, + import: self, + import_locked: true + ) + ) + end + + Transaction.import!(new_transactions, recursive: true) if new_transactions.any? + end + end + def pdf_uploaded? pdf_file.attached? end @@ -71,6 +99,34 @@ class PdfImport < Import extracted_data&.dig("transactions") || [] end + def generate_rows_from_extracted_data + transaction do + rows.destroy_all + + unless has_extracted_transactions? + update_column(:rows_count, 0) + return + end + + currency = account&.currency || family.currency + + mapped_rows = extracted_transactions.map do |txn| + { + import_id: id, + date: format_date_for_import(txn["date"]), + amount: txn["amount"].to_s, + name: txn["name"].to_s, + category: txn["category"].to_s, + notes: txn["notes"].to_s, + currency: currency + } + end + + Import::Row.insert_all!(mapped_rows) if mapped_rows.any? + update_column(:rows_count, mapped_rows.size) + end + end + def send_next_steps_email(user) PdfImportMailer.with( user: user, @@ -83,19 +139,19 @@ class PdfImport < Import end def configured? - ai_processed? + ai_processed? && rows_count > 0 end def cleaned? - ai_processed? + configured? && rows.all?(&:valid?) end def publishable? - false + account.present? && bank_statement? && cleaned? && mappings.all?(&:valid?) end def column_keys - [] + %i[date amount name category notes] end def requires_csv_workflow? @@ -107,4 +163,27 @@ class PdfImport < Import pdf_file.download end + + def required_column_keys + %i[date amount] + end + + def mapping_steps + base = [] + # Only include CategoryMapping if rows have non-empty categories + base << Import::CategoryMapping if rows.where.not(category: [ nil, "" ]).exists? + # Note: PDF imports use direct account selection in the UI, not AccountMapping + # AccountMapping is designed for CSV imports where rows have different account values + base + end + + private + + def format_date_for_import(date_str) + return "" if date_str.blank? + + Date.parse(date_str).strftime(date_format) + rescue ArgumentError + date_str.to_s + end end diff --git a/app/models/provider/openai.rb b/app/models/provider/openai.rb index 08ac224f9..6ec10333d 100644 --- a/app/models/provider/openai.rb +++ b/app/models/provider/openai.rb @@ -118,21 +118,20 @@ class Provider::Openai < Provider # Can be disabled via ENV for OpenAI-compatible endpoints that don't support vision # Only vision-capable models (gpt-4o, gpt-4-turbo, gpt-4.1, etc.) support PDF input - def supports_pdf_processing? + def supports_pdf_processing?(model: @default_model) return false unless ENV.fetch("OPENAI_SUPPORTS_PDF_PROCESSING", "true").to_s.downcase.in?(%w[true 1 yes]) # Custom providers manage their own model capabilities return true if custom_provider? - # Check if the configured model supports vision/PDF input - VISION_CAPABLE_MODEL_PREFIXES.any? { |prefix| @default_model.start_with?(prefix) } + # Check if the specified model supports vision/PDF input + VISION_CAPABLE_MODEL_PREFIXES.any? { |prefix| model.start_with?(prefix) } end def process_pdf(pdf_content:, model: "", family: nil) - raise "Model does not support PDF/vision processing" unless supports_pdf_processing? - with_provider_response do effective_model = model.presence || @default_model + raise Error, "Model does not support PDF/vision processing: #{effective_model}" unless supports_pdf_processing?(model: effective_model) trace = create_langfuse_trace( name: "openai.process_pdf", diff --git a/app/models/provider/openai/pdf_processor.rb b/app/models/provider/openai/pdf_processor.rb index b99caa77c..f65510e87 100644 --- a/app/models/provider/openai/pdf_processor.rb +++ b/app/models/provider/openai/pdf_processor.rb @@ -42,7 +42,7 @@ class Provider::Openai::PdfProcessor For each document, you must determine: 1. **Document Type**: Classify the document as one of the following: - - `bank_statement`: A bank account statement showing transactions, balances, and account activity + - `bank_statement`: A bank account statement showing transactions, balances, and account activity. This includes mobile money statements (like M-PESA, Venmo, PayPal, Cash App), digital wallet statements, and any statement showing a list of financial transactions with dates and amounts. - `credit_card_statement`: A credit card statement showing charges, payments, and balances - `investment_statement`: An investment/brokerage statement showing holdings, trades, or portfolio performance - `financial_document`: General financial documents like tax forms, receipts, invoices, or financial reports diff --git a/app/views/imports/_nav.html.erb b/app/views/imports/_nav.html.erb index 898c78255..26435d8b6 100644 --- a/app/views/imports/_nav.html.erb +++ b/app/views/imports/_nav.html.erb @@ -1,18 +1,29 @@ <%# locals: (import:) %> -<% steps = [ - { name: "Upload", path: import_upload_path(import), is_complete: import.uploaded?, step_number: 1 }, - { name: "Configure", path: import_configuration_path(import), is_complete: import.configured?, step_number: 2 }, - { name: "Clean", path: import_clean_path(import), is_complete: import.cleaned?, step_number: 3 }, - { name: "Map", path: import_confirm_path(import), is_complete: import.publishable?, step_number: 4 }, - { name: "Confirm", path: import_path(import), is_complete: import.complete?, step_number: 5 } -].reject { |step| step[:name] == "Map" && import.mapping_steps.empty? } %> +<% steps = if import.is_a?(PdfImport) + # PDF imports have a simplified flow: Upload -> Confirm + # Upload/Configure/Clean are always complete for processed PDF imports + [ + { name: t("imports.steps.upload", default: "Upload"), path: nil, is_complete: import.pdf_uploaded?, step_number: 1 }, + { name: t("imports.steps.configure", default: "Configure"), path: nil, is_complete: import.configured?, step_number: 2 }, + { name: t("imports.steps.clean", default: "Clean"), path: nil, is_complete: import.cleaned?, step_number: 3 }, + { name: t("imports.steps.confirm", default: "Confirm"), path: import_path(import), is_complete: import.complete?, step_number: 4 } + ] +else + [ + { name: t("imports.steps.upload", default: "Upload"), path: import_upload_path(import), is_complete: import.uploaded?, step_number: 1 }, + { name: t("imports.steps.configure", default: "Configure"), path: import_configuration_path(import), is_complete: import.configured?, step_number: 2 }, + { name: t("imports.steps.clean", default: "Clean"), path: import_clean_path(import), is_complete: import.cleaned?, step_number: 3 }, + { name: t("imports.steps.map", default: "Map"), key: "Map", path: import_confirm_path(import), is_complete: import.publishable?, step_number: 4 }, + { name: t("imports.steps.confirm", default: "Confirm"), path: import_path(import), is_complete: import.complete?, step_number: 5 } + ].reject { |step| step[:key] == "Map" && import.mapping_steps.empty? } +end %> <% content_for :mobile_import_progress do %> - <% active_step = steps.detect { |s| request.path.eql?(s[:path]) } %> - <% if active_step.present? %> + <% active_step_index = steps.index { |s| request.path.eql?(s[:match_path] || s[:path]) } %> + <% if active_step_index %>
- Step <%= active_step[:step_number] %> of <%= steps.size %> + <%= t("imports.steps.progress", step: active_step_index + 1, total: steps.size, default: "Step %{step} of %{total}") %>
<% end %> <% end %> @@ -20,7 +31,7 @@