diff --git a/app/controllers/pending_duplicate_merges_controller.rb b/app/controllers/pending_duplicate_merges_controller.rb index 80c592f3b..9b690daa5 100644 --- a/app/controllers/pending_duplicate_merges_controller.rb +++ b/app/controllers/pending_duplicate_merges_controller.rb @@ -52,6 +52,10 @@ class PendingDuplicateMergesController < ApplicationController else redirect_back_or_to transactions_path, alert: "Could not merge transactions" end + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotDestroyed, + ActiveRecord::Deadlocked, ActiveRecord::LockWaitTimeout => e + Rails.logger.error("Failed to manually merge pending transaction: #{e.message}") + redirect_back_or_to transactions_path, alert: t("transactions.merge_duplicate.failure") end private diff --git a/app/controllers/transactions_controller.rb b/app/controllers/transactions_controller.rb index ce7e32438..944744d06 100644 --- a/app/controllers/transactions_controller.rb +++ b/app/controllers/transactions_controller.rb @@ -180,7 +180,9 @@ class TransactionsController < ApplicationController end redirect_to transactions_path - rescue ActiveRecord::RecordNotDestroyed, ActiveRecord::RecordInvalid => e + rescue ActiveRecord::RecordNotFound, ActiveRecord::RecordInvalid, + ActiveRecord::RecordNotDestroyed, ActiveRecord::Deadlocked, + ActiveRecord::LockWaitTimeout => e Rails.logger.error("Failed to merge duplicate transaction #{params[:id]}: #{e.message}") flash[:alert] = t("transactions.merge_duplicate.failure") redirect_to transactions_path diff --git a/app/models/enable_banking_account/transactions/processor.rb b/app/models/enable_banking_account/transactions/processor.rb index 831539610..fb50333e0 100644 --- a/app/models/enable_banking_account/transactions/processor.rb +++ b/app/models/enable_banking_account/transactions/processor.rb @@ -15,6 +15,7 @@ class EnableBankingAccount::Transactions::Processor Rails.logger.info "EnableBankingAccount::Transactions::Processor - Processing #{total_count} transactions for enable_banking_account #{enable_banking_account.id}" imported_count = 0 + skipped_count = 0 failed_count = 0 errors = [] @@ -22,8 +23,43 @@ class EnableBankingAccount::Transactions::Processor Account::ProviderImportAdapter.new(enable_banking_account.current_account) end + # Pre-fetch external_ids that were manually merged and must not be re-imported. + # One query per sync; O(1) Set lookup per transaction — avoids N+1. + # Uses a lateral jsonb_array_elements join to extract only the ID strings in SQL, + # avoiding loading full extra blobs into Ruby. Handles both Array (current) and + # Hash (legacy) formats via jsonb_typeof. + excluded_ids = if enable_banking_account.current_account + Transaction.joins(:entry) + .where(entries: { account_id: enable_banking_account.current_account.id }) + .where("transactions.extra ? 'manual_merge'") + .joins( + Arel.sql(<<~SQL.squish) + CROSS JOIN LATERAL jsonb_array_elements( + CASE jsonb_typeof(transactions.extra->'manual_merge') + WHEN 'array' THEN transactions.extra->'manual_merge' + WHEN 'object' THEN jsonb_build_array(transactions.extra->'manual_merge') + ELSE '[]'::jsonb + END + ) AS merge_elem + SQL + ) + .pluck(Arel.sql("merge_elem->>'merged_from_external_id'")) + .compact + .to_set + else + Set.new + end + enable_banking_account.raw_transactions_payload.each_with_index do |transaction_data, index| begin + ext_id = EnableBankingEntry::Processor.compute_external_id(transaction_data) + + if ext_id && excluded_ids.include?(ext_id) + Rails.logger.info("EnableBankingAccount::Transactions::Processor - Skipping re-import of manually merged pending transaction: #{ext_id}") + skipped_count += 1 + next + end + result = EnableBankingEntry::Processor.new( transaction_data, enable_banking_account: enable_banking_account, @@ -56,6 +92,7 @@ class EnableBankingAccount::Transactions::Processor success: failed_count == 0, total: total_count, imported: imported_count, + skipped: skipped_count, failed: failed_count, errors: errors } diff --git a/app/models/enable_banking_entry/processor.rb b/app/models/enable_banking_entry/processor.rb index 1cc2309db..e08d37acc 100644 --- a/app/models/enable_banking_entry/processor.rb +++ b/app/models/enable_banking_entry/processor.rb @@ -10,6 +10,12 @@ class EnableBankingEntry::Processor # transaction_amount: { amount, currency }, # creditor_name, debtor_name, remittance_information, ... # } + def self.compute_external_id(raw_transaction_data) + data = raw_transaction_data.with_indifferent_access + id = data[:transaction_id].presence || data[:entry_reference].presence + id ? "enable_banking_#{id}" : nil + end + def initialize(enable_banking_transaction, enable_banking_account:, import_adapter: nil) @enable_banking_transaction = enable_banking_transaction @enable_banking_account = enable_banking_account @@ -17,8 +23,12 @@ class EnableBankingEntry::Processor end def process + # Cache a safe diagnostic id upfront — used in all logging paths so rescue + # blocks never call the potentially-raising private external_id method. + safe_id = self.class.compute_external_id(@enable_banking_transaction) || "unknown" + unless account.present? - Rails.logger.warn "EnableBankingEntry::Processor - No linked account for enable_banking_account #{enable_banking_account.id}, skipping transaction #{external_id}" + Rails.logger.warn "EnableBankingEntry::Processor - No linked account for enable_banking_account #{enable_banking_account.id}, skipping transaction #{safe_id}" return nil end @@ -35,13 +45,13 @@ class EnableBankingEntry::Processor extra: extra ) rescue ArgumentError => e - Rails.logger.error "EnableBankingEntry::Processor - Validation error for transaction #{external_id}: #{e.message}" + Rails.logger.error "EnableBankingEntry::Processor - Validation error for transaction #{safe_id}: #{e.message}" raise rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e - Rails.logger.error "EnableBankingEntry::Processor - Failed to save transaction #{external_id}: #{e.message}" + Rails.logger.error "EnableBankingEntry::Processor - Failed to save transaction #{safe_id}: #{e.message}" raise StandardError.new("Failed to import transaction: #{e.message}") rescue => e - Rails.logger.error "EnableBankingEntry::Processor - Unexpected error processing transaction #{external_id}: #{e.class} - #{e.message}" + Rails.logger.error "EnableBankingEntry::Processor - Unexpected error processing transaction #{safe_id}: #{e.class} - #{e.message}" Rails.logger.error e.backtrace.join("\n") raise StandardError.new("Unexpected error importing transaction: #{e.message}") end @@ -64,9 +74,9 @@ class EnableBankingEntry::Processor end def external_id - id = data[:transaction_id].presence || data[:entry_reference].presence + id = self.class.compute_external_id(data) raise ArgumentError, "Enable Banking transaction missing required field 'transaction_id'" unless id - "enable_banking_#{id}" + id end def name @@ -220,7 +230,8 @@ class EnableBankingEntry::Processor end def log_invalid_currency(currency_value) - Rails.logger.warn("Invalid currency code '#{currency_value}' in Enable Banking transaction #{external_id}, falling back to account currency") + safe_id = self.class.compute_external_id(data) || "unknown" + Rails.logger.warn("Invalid currency code '#{currency_value}' in Enable Banking transaction #{safe_id}, falling back to account currency") end def date diff --git a/app/models/entry.rb b/app/models/entry.rb index 042ea6957..aaf079059 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -48,27 +48,20 @@ class Entry < ApplicationRecord # Pending transaction scopes - check Transaction.extra for provider pending flags # Works with any provider that stores pending status in extra["provider_name"]["pending"] scope :pending, -> { + conditions = Transaction::PENDING_PROVIDERS.map { |p| "(transactions.extra -> '#{p}' ->> 'pending')::boolean = true" } joins("INNER JOIN transactions ON transactions.id = entries.entryable_id AND entries.entryable_type = 'Transaction'") - .where(<<~SQL.squish) - (transactions.extra -> 'simplefin' ->> 'pending')::boolean = true - OR (transactions.extra -> 'plaid' ->> 'pending')::boolean = true - OR (transactions.extra -> 'lunchflow' ->> 'pending')::boolean = true - SQL + .where(conditions.join(" OR ")) } scope :excluding_pending, -> { # For non-Transaction entries (Trade, Valuation), always include - # For Transaction entries, exclude if pending flag is true + # For Transaction entries, exclude if any provider marks it pending where(<<~SQL.squish) entries.entryable_type != 'Transaction' OR NOT EXISTS ( SELECT 1 FROM transactions t WHERE t.id = entries.entryable_id - AND ( - (t.extra -> 'simplefin' ->> 'pending')::boolean = true - OR (t.extra -> 'plaid' ->> 'pending')::boolean = true - OR (t.extra -> 'lunchflow' ->> 'pending')::boolean = true - ) + AND (#{Transaction::PENDING_CHECK_SQL}) ) SQL } @@ -148,6 +141,10 @@ class Entry < ApplicationRecord def self.reconcile_pending_duplicates(account: nil, dry_run: false, date_window: 8, amount_tolerance: 0.25) stats = { checked: 0, reconciled: 0, details: [] } + not_pending_sql = Transaction::PENDING_PROVIDERS + .map { |p| "(transactions.extra -> '#{p}' ->> 'pending')::boolean IS NOT TRUE" } + .join(" AND ") + # Get pending entries to check scope = Entry.pending.where(excluded: false) scope = scope.where(account: account) if account @@ -164,11 +161,7 @@ class Entry < ApplicationRecord .where(currency: pending_entry.currency) .where(amount: pending_entry.amount) .where(date: pending_entry.date..(pending_entry.date + date_window.days)) # Posted must be ON or AFTER pending date - .where(<<~SQL.squish) - (transactions.extra -> 'simplefin' ->> 'pending')::boolean IS NOT TRUE - AND (transactions.extra -> 'plaid' ->> 'pending')::boolean IS NOT TRUE - AND (transactions.extra -> 'lunchflow' ->> 'pending')::boolean IS NOT TRUE - SQL + .where(not_pending_sql) .limit(2) # Only need to know if 0, 1, or 2+ candidates .to_a # Load limited records to avoid COUNT(*) on .size @@ -211,11 +204,7 @@ class Entry < ApplicationRecord .where(currency: pending_entry.currency) .where(date: pending_entry.date..(pending_entry.date + fuzzy_date_window.days)) # Posted ON or AFTER pending .where("ABS(entries.amount) BETWEEN ? AND ?", min_amount, max_amount) - .where(<<~SQL.squish) - (transactions.extra -> 'simplefin' ->> 'pending')::boolean IS NOT TRUE - AND (transactions.extra -> 'plaid' ->> 'pending')::boolean IS NOT TRUE - AND (transactions.extra -> 'lunchflow' ->> 'pending')::boolean IS NOT TRUE - SQL + .where(not_pending_sql) # Match by name similarity (first 3 words) name_words = pending_entry.name.downcase.gsub(/[^a-z0-9\s]/, "").split.first(3).join(" ") @@ -253,10 +242,12 @@ class Entry < ApplicationRecord pending_transaction.update!( extra: existing_extra.merge( "potential_posted_match" => { - "entry_id" => fuzzy_match.id, - "reason" => "fuzzy_amount_match", + "entry_id" => fuzzy_match.id, + "reason" => "fuzzy_amount_match", "posted_amount" => fuzzy_match.amount.to_s, - "detected_at" => Date.current.to_s + "confidence" => "medium", + "dismissed" => false, + "detected_at" => Date.current.to_s } ) ) diff --git a/app/models/transaction.rb b/app/models/transaction.rb index e20370ead..6ebc807be 100644 --- a/app/models/transaction.rb +++ b/app/models/transaction.rb @@ -95,6 +95,13 @@ class Transaction < ApplicationRecord # Providers that support pending transaction flags PENDING_PROVIDERS = %w[simplefin plaid lunchflow enable_banking].freeze + # Pre-computed SQL fragment for subqueries that check if a transaction (aliased as "t") is pending. + # Stored as a constant so static analysis can verify it contains no user input. + PENDING_CHECK_SQL = PENDING_PROVIDERS + .map { |p| "(t.extra -> '#{p}' ->> 'pending')::boolean = true" } + .join(" OR ") + .freeze + # Pending transaction scopes - filter based on provider pending flags in extra JSONB # Works with any provider that stores pending status in extra["provider_name"]["pending"] scope :pending, -> { @@ -140,7 +147,7 @@ class Transaction < ApplicationRecord PENDING_PROVIDERS.any? do |provider| ActiveModel::Type::Boolean.new.cast(extra_data.dig(provider, "pending")) end - rescue + rescue StandardError false end @@ -176,22 +183,106 @@ class Transaction < ApplicationRecord potential_posted_match_data&.dig("dismissed") == true end - # Merge this pending transaction with its suggested posted match - # This DELETES the pending entry since the posted version is canonical + # Merge this pending transaction with its suggested posted match. + # The pending entry is destroyed; the posted entry survives with attributes inherited from both sides. + # Attribute inheritance: Date + Category from pending, Name + Merchant from posted (booked). def merge_with_duplicate! + return false unless pending? return false unless has_potential_duplicate? posted_entry = potential_duplicate_entry return false unless posted_entry - pending_entry_id = entry.id - pending_entry_name = entry.name + pending_entry = entry - # Delete this pending entry completely (no need to keep it around) - entry.destroy! + # Guard: cross-account merges are never valid + if posted_entry.account_id != pending_entry.account_id + Rails.logger.warn("merge_with_duplicate! rejected: posted_entry #{posted_entry.id} belongs to different account than pending entry #{pending_entry.id}") + return false + end - Rails.logger.info("User merged pending entry #{pending_entry_id} (#{pending_entry_name}) with posted entry #{posted_entry.id}") - true + pending_entry_id = pending_entry.id + merge_succeeded = false + + ApplicationRecord.transaction(requires_new: true) do + # Row-level locks prevent concurrent merges on the same pair of entries. + # If a concurrent request already destroyed the pending entry, lock! raises + # RecordNotFound — treat that as an idempotent success. + begin + pending_entry.lock! + rescue ActiveRecord::RecordNotFound + Rails.logger.info("Pending entry #{pending_entry_id} already destroyed (concurrent merge), skipping") + return true + end + + begin + posted_entry.lock! + rescue ActiveRecord::RecordNotFound + Rails.logger.info("Posted entry #{posted_entry.id} deleted concurrently; aborting merge") + raise ActiveRecord::Rollback + end + + # Capture after lock! (which reloads) to guarantee DB-fresh values and avoid + # stale in-memory cached associations (e.g., loaded via touch: true). + external_id = pending_entry.external_id + pending_entry_date = pending_entry.date + + # Batch all changes to the surviving posted Transaction into a single update! + # to avoid firing after_save callbacks twice on the same row. + # Lock the Transaction row so concurrent merges into the same posted entry + # cannot race on reading/writing extra (e.g., the manual_merge array). + posted_tx = posted_entry.entryable + posted_tx.lock! if posted_tx.is_a?(Transaction) + if posted_tx.is_a?(Transaction) + tx_attrs = {} + + # Merge metadata — always written so the sync engine can skip re-importing. + # Stored as an array so multiple pending entries merged into the same posted + # transaction each preserve their external_id for future sync exclusion. + # Legacy records written as a plain Hash are migrated to a single-element array + # on first append, maintaining backward compatibility. + if external_id.present? + new_record = { + "merged_from_entry_id" => pending_entry_id, + "merged_from_external_id" => external_id, + "merged_at" => Time.current.iso8601, + "source" => pending_entry.source + } + prior = case posted_tx.extra["manual_merge"] + when Array then posted_tx.extra["manual_merge"] + when Hash then [ posted_tx.extra["manual_merge"] ] + else [] + end + tx_attrs[:extra] = posted_tx.extra.merge("manual_merge" => prior + [ new_record ]) + end + + # Attribute inheritance — only when the posted entry is not already user-protected. + unless posted_entry.protected_from_sync? + pending_transaction = pending_entry.entryable + if pending_transaction.is_a?(Transaction) && pending_transaction.category_id.present? + tx_attrs[:category_id] = pending_transaction.category_id + end + end + + posted_tx.update!(tx_attrs) if tx_attrs.any? + end + + # Date inheritance on the Entry row — separate from the Transaction update above. + unless posted_entry.protected_from_sync? + # Date: pending dates reflect actual transaction initiation time + posted_entry.update!(date: pending_entry_date) if posted_entry.date != pending_entry_date + # Name + Merchant intentionally NOT inherited — booked values are canonical + end + + # Lock the posted entry so future syncs cannot overwrite the merged state + posted_entry.mark_user_modified! + + Rails.logger.info("User merged pending entry #{pending_entry_id} (ext: #{external_id}) into posted entry #{posted_entry.id}") + pending_entry.destroy! + merge_succeeded = true + end + + merge_succeeded end # Dismiss the duplicate suggestion - user says these are NOT the same transaction diff --git a/test/models/enable_banking_account/transactions/processor_test.rb b/test/models/enable_banking_account/transactions/processor_test.rb new file mode 100644 index 000000000..f136ab806 --- /dev/null +++ b/test/models/enable_banking_account/transactions/processor_test.rb @@ -0,0 +1,198 @@ +require "test_helper" + +class EnableBankingAccount::Transactions::ProcessorTest < ActiveSupport::TestCase + include EntriesTestHelper + + setup do + @family = families(:dylan_family) + @account = accounts(:depository) + + @enable_banking_item = EnableBankingItem.create!( + family: @family, + name: "Test EB Item", + country_code: "FR", + application_id: "app_id", + client_certificate: "cert" + ) + @enable_banking_account = EnableBankingAccount.create!( + enable_banking_item: @enable_banking_item, + name: "Compte courant", + uid: "uid_txn_proc_test", + currency: "EUR", + current_balance: 1000.00 + ) + AccountProvider.create!(account: @account, provider: @enable_banking_account) + end + + # Minimal raw transaction payload hash matching the shape EnableBankingEntry::Processor expects + def raw_pending_transaction(transaction_id:) + { + transaction_id: transaction_id, + value_date: 3.days.ago.to_date.to_s, + transaction_amount: { amount: "25.00", currency: "EUR" }, + credit_debit_indicator: "DBIT", + _pending: true + } + end + + test "does not re-import a pending transaction whose external_id was manually merged" do + pending_ext_id = "enable_banking_PDNG_MERGED" + + # Simulate a previously-merged state: a posted transaction carries the pending's external_id + # in its manual_merge metadata, which is how merge_with_duplicate! records the merge. + posted_entry = create_transaction( + account: @account, + name: "Coffee Shop", + date: 1.day.ago.to_date, + amount: 25, + currency: "EUR", + external_id: "enable_banking_BOOK_SETTLED", + source: "enable_banking" + ) + posted_entry.transaction.update!( + extra: { + "manual_merge" => { + "merged_from_entry_id" => SecureRandom.uuid, + "merged_from_external_id" => pending_ext_id, + "merged_at" => Time.current.iso8601, + "source" => "enable_banking" + } + } + ) + posted_entry.mark_user_modified! + + # Raw payload contains the pending transaction that was already merged + @enable_banking_account.update!( + raw_transactions_payload: [ + raw_pending_transaction(transaction_id: "PDNG_MERGED") + ] + ) + + assert_no_difference "@account.entries.count" do + EnableBankingAccount::Transactions::Processor.new(@enable_banking_account).process + end + end + + test "imports a pending transaction that has NOT been merged" do + @enable_banking_account.update!( + raw_transactions_payload: [ + raw_pending_transaction(transaction_id: "PDNG_NEW_UNMERGED") + ] + ) + + assert_difference "@account.entries.count", 1 do + EnableBankingAccount::Transactions::Processor.new(@enable_banking_account).process + end + end + + test "imports non-excluded transactions alongside excluded ones in the same batch" do + pending_ext_id = "enable_banking_PDNG_SKIP_ME" + + posted_entry = create_transaction( + account: @account, + name: "Already Merged", + date: 2.days.ago.to_date, + amount: 15, + currency: "EUR", + external_id: "enable_banking_BOOK_ALREADY", + source: "enable_banking" + ) + posted_entry.transaction.update!( + extra: { + "manual_merge" => { + "merged_from_external_id" => pending_ext_id, + "merged_at" => Time.current.iso8601, + "source" => "enable_banking" + } + } + ) + + @enable_banking_account.update!( + raw_transactions_payload: [ + raw_pending_transaction(transaction_id: "PDNG_SKIP_ME"), # excluded + raw_pending_transaction(transaction_id: "PDNG_BRAND_NEW_12345") # should be imported + ] + ) + + assert_difference "@account.entries.count", 1 do + EnableBankingAccount::Transactions::Processor.new(@enable_banking_account).process + end + end + + test "excludes all external_ids when multiple pending entries were merged into the same posted entry" do + pending_ext_id_1 = "enable_banking_PDNG_MULTI_1" + pending_ext_id_2 = "enable_banking_PDNG_MULTI_2" + + posted_entry = create_transaction( + account: @account, + name: "Multi Merge", + date: 2.days.ago.to_date, + amount: 30, + currency: "EUR", + external_id: "enable_banking_BOOK_MULTI", + source: "enable_banking" + ) + posted_entry.transaction.update!( + extra: { + "manual_merge" => [ + { "merged_from_external_id" => pending_ext_id_1, "merged_at" => 2.days.ago.iso8601, "source" => "enable_banking" }, + { "merged_from_external_id" => pending_ext_id_2, "merged_at" => 1.day.ago.iso8601, "source" => "enable_banking" } + ] + } + ) + + @enable_banking_account.update!( + raw_transactions_payload: [ + raw_pending_transaction(transaction_id: "PDNG_MULTI_1"), # excluded + raw_pending_transaction(transaction_id: "PDNG_MULTI_2"), # excluded + raw_pending_transaction(transaction_id: "PDNG_MULTI_NEW") # new — should import + ] + ) + + result = nil + assert_difference "@account.entries.count", 1 do + result = EnableBankingAccount::Transactions::Processor.new(@enable_banking_account).process + end + assert_equal 2, result[:skipped] + assert_equal 1, result[:imported] + end + + test "handles empty raw_transactions_payload gracefully" do + @enable_banking_account.update!(raw_transactions_payload: nil) + + result = EnableBankingAccount::Transactions::Processor.new(@enable_banking_account).process + + assert_equal true, result[:success] + assert_equal 0, result[:total] + end + + test "reports excluded transactions as skipped, not imported or failed" do + pending_ext_id = "enable_banking_PDNG_SKIP_STATS" + + posted_entry = create_transaction( + account: @account, + name: "Stats Test", + date: 2.days.ago.to_date, + amount: 50, + currency: "EUR", + external_id: "enable_banking_BOOK_STATS", + source: "enable_banking" + ) + posted_entry.transaction.update!( + extra: { "manual_merge" => { "merged_from_external_id" => pending_ext_id } } + ) + + @enable_banking_account.update!( + raw_transactions_payload: [ + raw_pending_transaction(transaction_id: "PDNG_SKIP_STATS") + ] + ) + + result = EnableBankingAccount::Transactions::Processor.new(@enable_banking_account).process + + assert_equal true, result[:success] + assert_equal 1, result[:skipped] + assert_equal 0, result[:imported] + assert_equal 0, result[:failed] + end +end diff --git a/test/models/transaction/merge_with_duplicate_test.rb b/test/models/transaction/merge_with_duplicate_test.rb new file mode 100644 index 000000000..8149d0973 --- /dev/null +++ b/test/models/transaction/merge_with_duplicate_test.rb @@ -0,0 +1,308 @@ +require "test_helper" + +class Transaction::MergeWithDuplicateTest < ActiveSupport::TestCase + include EntriesTestHelper + + setup do + @account = accounts(:depository) + @family = @account.family + + @category_a = categories(:food_and_drink) + @category_b = categories(:income) + + # Pending entry — simulates a bank-synced pending transaction + @pending_entry = create_transaction( + account: @account, + name: "Starbucks Pending", + date: 3.days.ago.to_date, + amount: 10, + currency: "USD", + external_id: "enable_banking_PDNG123", + source: "enable_banking", + category: @category_a + ) + @pending_entry.transaction.update!( + extra: { "enable_banking" => { "pending" => true } } + ) + + # Posted (booked) entry — the canonical settled transaction from the bank + @posted_entry = create_transaction( + account: @account, + name: "STARBUCKS CORP", + date: 1.day.ago.to_date, + amount: 10, + currency: "USD", + external_id: "enable_banking_BOOK456", + source: "enable_banking" + ) + + # Wire up the duplicate suggestion on the pending transaction + @pending_entry.transaction.update!( + extra: @pending_entry.transaction.extra.merge( + "potential_posted_match" => { + "entry_id" => @posted_entry.id, + "reason" => "fuzzy_amount_match", + "posted_amount" => "10.0", + "confidence" => "medium", + "detected_at" => Date.current.to_s, + "dismissed" => false + } + ) + ) + end + + test "destroys the pending entry on successful merge" do + pending_id = @pending_entry.id + assert_difference "Entry.count", -1 do + @pending_entry.transaction.merge_with_duplicate! + end + assert_nil Entry.find_by(id: pending_id) + end + + test "records merged_from_external_id on the surviving posted transaction" do + @pending_entry.transaction.merge_with_duplicate! + + posted_tx = @posted_entry.transaction.reload + assert_equal "enable_banking_PDNG123", posted_tx.extra.dig("manual_merge", 0, "merged_from_external_id") + end + + test "records merged_from_entry_id and source in manual_merge metadata" do + pending_id = @pending_entry.id + @pending_entry.transaction.merge_with_duplicate! + + merge_meta = @posted_entry.transaction.reload.extra["manual_merge"].first + assert_equal pending_id, merge_meta["merged_from_entry_id"] + assert_equal "enable_banking", merge_meta["source"] + assert merge_meta["merged_at"].present? + end + + test "appends to existing manual_merge array preserving prior merged IDs" do + # Seed a prior merge record directly so the posted entry already has one ID + prior_ext_id = "enable_banking_PDNG_PRIOR" + @posted_entry.transaction.update!( + extra: { + "manual_merge" => [ + { "merged_from_external_id" => prior_ext_id, "merged_at" => 1.day.ago.iso8601, "source" => "enable_banking" } + ] + } + ) + + @pending_entry.transaction.merge_with_duplicate! + + records = @posted_entry.transaction.reload.extra["manual_merge"] + assert_equal 2, records.size + assert_includes records.map { |r| r["merged_from_external_id"] }, prior_ext_id + assert_includes records.map { |r| r["merged_from_external_id"] }, "enable_banking_PDNG123" + end + + test "migrates legacy single-object manual_merge to array on second merge" do + # Simulate an existing record written in the old single-Hash format + @posted_entry.transaction.update!( + extra: { + "manual_merge" => { + "merged_from_external_id" => "enable_banking_LEGACY", + "merged_at" => 1.day.ago.iso8601, + "source" => "enable_banking" + } + } + ) + + @pending_entry.transaction.merge_with_duplicate! + + records = @posted_entry.transaction.reload.extra["manual_merge"] + assert_kind_of Array, records + assert_equal 2, records.size + assert_includes records.map { |r| r["merged_from_external_id"] }, "enable_banking_LEGACY" + assert_includes records.map { |r| r["merged_from_external_id"] }, "enable_banking_PDNG123" + end + + test "inherits date from pending entry onto posted entry" do + original_posted_date = @posted_entry.date + pending_date = @pending_entry.date + refute_equal original_posted_date, pending_date + + @pending_entry.transaction.merge_with_duplicate! + + assert_equal pending_date, @posted_entry.reload.date + end + + test "inherits category from pending entry onto posted entry" do + assert_nil @posted_entry.transaction.category_id + + @pending_entry.transaction.merge_with_duplicate! + + assert_equal @category_a.id, @posted_entry.transaction.reload.category_id + end + + test "overwrites existing category on posted entry with pending category" do + @posted_entry.transaction.update!(category: @category_b) + + @pending_entry.transaction.merge_with_duplicate! + + assert_equal @category_a.id, @posted_entry.transaction.reload.category_id + end + + test "does not inherit name from pending — booked name is canonical" do + @pending_entry.transaction.merge_with_duplicate! + + assert_equal "STARBUCKS CORP", @posted_entry.reload.name + end + + test "marks the posted entry as user_modified to prevent future sync overwrites" do + refute @posted_entry.user_modified? + + @pending_entry.transaction.merge_with_duplicate! + + assert @posted_entry.reload.user_modified? + end + + test "returns true on success" do + result = @pending_entry.transaction.merge_with_duplicate! + assert_equal true, result + end + + test "returns false when no potential duplicate is set" do + @pending_entry.transaction.update!(extra: {}) + result = @pending_entry.transaction.merge_with_duplicate! + assert_equal false, result + end + + test "returns false when the suggested posted entry no longer exists" do + @posted_entry.destroy! + result = @pending_entry.transaction.merge_with_duplicate! + assert_equal false, result + end + + test "returns false and does not destroy pending entry when posted entry is on a different account" do + other_account = accounts(:credit_card) + @posted_entry.update!(account: other_account) + + result = nil + assert_no_difference "Entry.count" do + result = @pending_entry.transaction.merge_with_duplicate! + end + assert_equal false, result + end + + test "does not update date or category when posted entry is already user_modified" do + # Give posted entry a category so we can assert it was preserved (avoids nil==nil comparison) + @posted_entry.transaction.update!(category: @category_b) + original_date = @posted_entry.reload.date + original_category = @posted_entry.transaction.reload.category_id + @posted_entry.mark_user_modified! + + @pending_entry.transaction.merge_with_duplicate! + + assert_equal original_date, @posted_entry.reload.date + assert_equal original_category, @posted_entry.transaction.reload.category_id + end + + test "still records merge metadata even when posted entry is already user_modified" do + @posted_entry.mark_user_modified! + + @pending_entry.transaction.merge_with_duplicate! + + assert_equal "enable_banking_PDNG123", + @posted_entry.transaction.reload.extra.dig("manual_merge", 0, "merged_from_external_id") + end + + test "is idempotent when pending entry is already destroyed (concurrent merge)" do + @pending_entry.destroy! + + result = nil + assert_no_difference "Entry.count" do + result = @pending_entry.transaction.merge_with_duplicate! + end + assert_equal true, result + end + + test "skips storing merge metadata when pending entry has no external_id" do + @pending_entry.update!(external_id: nil) + + @pending_entry.transaction.merge_with_duplicate! + + merge_meta = @posted_entry.transaction.reload.extra&.dig("manual_merge") + assert_nil merge_meta + end + + # --- C-1: concurrent deletion of posted entry --- + + test "returns false when posted entry is deleted between check and lock" do + # Simulate the race: posted_entry exists at find_by time but is gone at lock! time. + # Use a stub rather than dup so id and account_id are real values — dup gives id: nil. + ghost = stub(account_id: @posted_entry.account_id, id: @posted_entry.id) + ghost.stubs(:lock!).raises(ActiveRecord::RecordNotFound) + @pending_entry.transaction.stubs(:potential_duplicate_entry).returns(ghost) + + result = nil + assert_no_difference "Entry.count" do + result = @pending_entry.transaction.merge_with_duplicate! + end + assert_equal false, result + end + + # --- cascade: delegated_type dependent: :destroy removes the Transaction too --- + + test "destroys the pending Transaction record on successful merge" do + assert_difference "Transaction.count", -1 do + @pending_entry.transaction.merge_with_duplicate! + end + end + + # --- dismiss_duplicate_suggestion! --- + + test "dismiss_duplicate_suggestion! sets dismissed flag on the match" do + @pending_entry.transaction.dismiss_duplicate_suggestion! + assert_equal true, @pending_entry.transaction.reload.extra.dig("potential_posted_match", "dismissed") + end + + test "dismiss_duplicate_suggestion! makes has_potential_duplicate? return false" do + @pending_entry.transaction.dismiss_duplicate_suggestion! + assert_not @pending_entry.transaction.reload.has_potential_duplicate? + end + + test "dismiss_duplicate_suggestion! returns false when no suggestion is present" do + @pending_entry.transaction.update!(extra: {}) + assert_equal false, @pending_entry.transaction.dismiss_duplicate_suggestion! + end + + test "merge_with_duplicate! returns false when suggestion has been dismissed" do + @pending_entry.transaction.dismiss_duplicate_suggestion! + assert_equal false, @pending_entry.transaction.reload.merge_with_duplicate! + end + + # --- clear_duplicate_suggestion! --- + + test "clear_duplicate_suggestion! removes potential_posted_match key entirely" do + @pending_entry.transaction.clear_duplicate_suggestion! + assert_nil @pending_entry.transaction.reload.extra["potential_posted_match"] + end + + test "clear_duplicate_suggestion! returns false when no suggestion is present" do + @pending_entry.transaction.update!(extra: {}) + assert_equal false, @pending_entry.transaction.clear_duplicate_suggestion! + end + + # --- pending_duplicate_candidates --- + + test "pending_duplicate_candidates returns posted transactions from the same account" do + candidates = @pending_entry.transaction.pending_duplicate_candidates + assert_includes candidates, @posted_entry + end + + test "pending_duplicate_candidates excludes the pending entry itself" do + candidates = @pending_entry.transaction.pending_duplicate_candidates + assert_not_includes candidates, @pending_entry + end + + test "pending_duplicate_candidates excludes transactions from other accounts" do + other_entry = create_transaction(account: accounts(:credit_card), amount: 10, currency: "USD") + candidates = @pending_entry.transaction.pending_duplicate_candidates + assert_not_includes candidates, other_entry + end + + test "pending_duplicate_candidates returns Entry.none when transaction is not pending" do + @pending_entry.transaction.update!(extra: {}) + assert_equal [], @pending_entry.transaction.pending_duplicate_candidates.to_a + end +end