refactor(imports): back PDF imports with statements

app/controllers/account_statements_controller.rb

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 class AccountStatementsController < ApplicationController
-  before_action :set_statement, only: %i[show update destroy link unlink reject]
-  before_action :ensure_statement_manager!, only: %i[index create update destroy link unlink reject]
+  before_action :set_statement, only: %i[show update destroy extract link unlink reject]
+  before_action :ensure_statement_manager!, only: %i[index create update destroy extract link unlink reject]
 
   def index
     accessible_account_ids = Current.user.accessible_accounts.select(:id)
@@ -30,6 +30,7 @@ def show
     @accounts = Current.user.accessible_accounts.visible.alphabetically
     @can_manage_statement = @statement.manageable_by?(Current.user)
     @reconciliation_checks = @statement.reconciliation_checks
+    @latest_pdf_import = @statement.latest_reusable_pdf_import if @statement.pdf?
     @breadcrumbs = [
       [ t("breadcrumbs.home"), root_path ],
       [ t("account_statements.index.title"), account_statements_path ],
@@ -41,6 +42,7 @@ def show
   def create
     files = Array(statement_upload_params[:files]).reject(&:blank?).select { |file| file.respond_to?(:read) }
     account = target_account
+    return if performed?
 
     if files.empty?
       redirect_back_or_to account_statements_path, alert: t("account_statements.create.no_files")
@@ -56,22 +58,24 @@ def create
     files.each do |file|
       prepared_upload = AccountStatement.prepare_upload!(file)
       created << AccountStatement.create_from_prepared_upload!(family: Current.family, account: account, prepared_upload: prepared_upload)
-    rescue AccountStatement::InvalidUploadError
-      validation_errors << t("account_statements.create.invalid_file_type")
+    rescue AccountStatement::InvalidUploadError => e
+      validation_errors << invalid_upload_message(e)
     rescue AccountStatement::DuplicateUploadError => e
       duplicates << e.statement
     rescue ActiveRecord::RecordInvalid => e
       validation_errors << e.record.errors.full_messages.to_sentence
     end
 
-    redirect_to redirect_after_create(account, created.first || duplicates.first),
+    redirect_statement = created.first || duplicates.find { |statement| statement.viewable_by?(Current.user) }
+    redirect_to redirect_after_create(account, redirect_statement),
                 flash_for_upload(created:, duplicates:, validation_errors:)
   end
 
   def update
     return if @statement.account && !require_account_permission!(@statement.account)
 
-    target = statement_account_id.present? ? Current.user.accessible_accounts.find(statement_account_id) : nil
+    target = statement_account_id.present? ? target_account : nil
+    return if performed?
     return if target && !require_account_permission!(target)
 
     attrs = statement_params.to_h
@@ -100,7 +104,11 @@ def link
       return
     end
 
-    account = Current.user.accessible_accounts.find(account_id)
+    account = Current.user.accessible_accounts.find_by(id: account_id)
+    unless account
+      redirect_to account_statement_path(@statement), alert: t("accounts.not_authorized")
+      return
+    end
     return unless require_account_permission!(account)
 
     @statement.link_to_account!(account)
@@ -116,6 +124,7 @@ def unlink
 
   def reject
     return if @statement.account && !require_account_permission!(@statement.account)
+    return redirect_to(account_statement_path(@statement), alert: t("account_statements.reject.linked")) if @statement.account.present?
 
     @statement.reject_match!
     redirect_to account_statements_path, notice: t("account_statements.reject.success")
@@ -132,6 +141,23 @@ def destroy
     end
   end
 
+  def extract
+    unless @statement.manageable_by?(Current.user)
+      redirect_to account_statement_path(@statement), alert: t("accounts.not_authorized")
+      return
+    end
+
+    unless @statement.pdf?
+      redirect_to account_statement_path(@statement), alert: t("account_statements.extract.not_pdf")
+      return
+    end
+
+    pdf_import = PdfImport.create_from_statement!(statement: @statement)
+    pdf_import.process_with_ai_later
+
+    redirect_to import_path(pdf_import), notice: t("account_statements.extract.started")
+  end
+
   private
 
     def set_statement
@@ -146,7 +172,7 @@ def set_statement
     def ensure_statement_manager!
       return if AccountStatement.statement_manager?(Current.user)
 
-      redirect_to accounts_path, alert: t("accounts.not_authorized")
+      redirect_back_or_to accounts_path, alert: t("accounts.not_authorized")
     end
 
     def statement_upload_params
@@ -170,7 +196,11 @@ def target_account
       account_id = statement_account_id.presence
       return nil if account_id.blank?
 
-      Current.user.accessible_accounts.find(account_id)
+      account = Current.user.accessible_accounts.find_by(id: account_id)
+      return account if account
+
+      redirect_back_or_to account_statements_path, alert: t("accounts.not_authorized")
+      nil
     end
 
     def statement_account_id
@@ -191,6 +221,14 @@ def redirect_after_create(account, statement = nil)
       end
     end
 
+    def invalid_upload_message(error)
+      reason = error.respond_to?(:reason) ? error.reason : nil
+      key = reason.in?(%i[oversize unsupported_type empty invalid_pdf_header]) ? reason : :invalid_file_type
+
+      t("account_statements.create.upload_errors.#{key}",
+        default: t("account_statements.create.invalid_file_type"))
+    end
+
     def post_link_path(statement)
       statement.account ? account_path(statement.account, tab: "statements") : account_statement_path(statement)
     end

app/controllers/imports_controller.rb

@@ -2,9 +2,9 @@ class ImportsController < ApplicationController
   include SettingsHelper
 
   before_action :set_import, only: %i[show update publish destroy revert apply_template]
+  before_action :require_statement_import_permission!, only: %i[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?
@@ -13,7 +13,9 @@ def update
         redirect_back_or_to import_path(@import), alert: t("imports.update.invalid_account", default: "Account not found.")
         return
       end
-      @import.update!(account: account)
+      return if @import.account_statement.present? && !require_account_permission!(account)
+
+      @import.assign_account!(account)
     end
 
     redirect_to import_path(@import), notice: t("imports.update.account_saved", default: "Account saved.")
@@ -54,12 +56,7 @@ def create
       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
@@ -87,8 +84,6 @@ def create
         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")
@@ -132,24 +127,41 @@ def destroy
 
   private
     def set_import
-      @import = Current.family.imports.includes(:account).find(params[:id])
+      @import = Current.family.imports.includes(:account, :account_statement).find(params[:id])
+      raise ActiveRecord::RecordNotFound if @import.account_statement.present? && [email protected]_statement.viewable_by?(Current.user)
     end
 
     def import_params
       params.require(:import).permit(:import_file)
     end
 
+    def require_statement_import_permission!
+      return unless @import.account_statement.present?
+      return if @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)
+      unless AccountStatement.statement_manager?(Current.user)
+        redirect_to new_import_path, alert: t("accounts.not_authorized")
+        return
+      end
+
       if file.size > Import::MAX_PDF_SIZE
         redirect_to new_import_path, alert: t("imports.create.pdf_too_large", max_size: Import::MAX_PDF_SIZE / 1.megabyte)
         return
       end
 
-      pdf_import = Current.family.imports.create!(type: "PdfImport")
-      pdf_import.pdf_file.attach(file)
+      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)
@@ -174,11 +186,6 @@ def create_document_import(file)
       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
@@ -239,10 +246,4 @@ def create_sure_import(file)
 
       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

app/jobs/process_pdf_job.rb

@@ -1,11 +1,13 @@
 class ProcessPdfJob < ApplicationJob
+  PROCESSING_CLAIM_TTL = 30.minutes
+
   queue_as :medium_priority
 
   def perform(pdf_import)
     return unless pdf_import.is_a?(PdfImport)
-    return unless pdf_import.pdf_uploaded?
+    return reset_processing_claim(pdf_import) unless pdf_import.pdf_uploaded?
     return if pdf_import.status == "complete"
-    return if pdf_import.ai_processed? && (!pdf_import.statement_with_transactions? || pdf_import.rows_count > 0)
+    return reset_processing_claim(pdf_import) if pdf_import.ai_processed? && (!pdf_import.statement_with_transactions? || pdf_import.rows_count > 0)
 
     pdf_import.update!(status: :importing)
 
@@ -62,7 +64,7 @@ def sanitize_error_message(error)
     end
 
     def upload_to_vector_store(pdf_import, document_type:)
-      filename = pdf_import.pdf_file.filename.to_s
+      filename = pdf_import.pdf_filename
       file_content = pdf_import.pdf_file_content
 
       family_document = pdf_import.family.upload_document(
@@ -85,4 +87,16 @@ def resolve_document_type(pdf_import, process_result)
     def statement_with_transactions?(document_type)
       document_type.in?(%w[bank_statement credit_card_statement])
     end
+
+    def reset_processing_claim(pdf_import)
+      pdf_import.with_lock do
+        if pdf_import.importing? && processing_claim_stale?(pdf_import)
+          pdf_import.update!(status: :pending)
+        end
+      end
+    end
+
+    def processing_claim_stale?(pdf_import)
+      pdf_import.updated_at <= PROCESSING_CLAIM_TTL.ago
+    end
 end