From 1bc227ea44fd3aba2af8e46d39815daace903cc1 Mon Sep 17 00:00:00 2001 From: CrossDrain <32982516+CrossDrain@users.noreply.github.com> Date: Wed, 27 May 2026 21:36:33 +0000 Subject: [PATCH] fix(enable_banking): clear stuck pending flag when ASPSP reuses same transaction_id (#1982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(enable_banking): clear stuck pending flag when ASPSP reuses same transaction_id for booked version * fix: scope pending→booked bypass to user_modified entries only * refactor: extract clear_pending_flags_from_extra helper to deduplicate pending-flag removal logic * refactor: use clear_pending_flags_from_extra in user_modified bypass path * fix(provider_import_adapter): add type check in clear_pending_flags_from_extra Add a check to ensure that the value associated with a provider key in the `extra` hash is a Hash before attempting to call `delete` on it. This prevents a `NoMethodError` when encountering malformed data where the provider key exists but does not map to a Hash. * fix(provider_import_adapter): fix indentation and ensure proper return in clear_pending_flags_from_extra * fix(provider_import_adapter): make clear_pending_flags_from_extra private * fix: guard clear_pending_flags_from_extra against non-Hash extra values --- app/models/account/provider_import_adapter.rb | 69 +++++++++++++----- .../account/provider_import_adapter_test.rb | 73 +++++++++++++++++++ 2 files changed, 125 insertions(+), 17 deletions(-) diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index 549423f10..b4f8e17dc 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -44,12 +44,37 @@ class Account::ProviderImportAdapter raise ArgumentError, "Entry with external_id '#{external_id}' already exists with different entryable type: #{entry.entryable_type}" end + # Determine early whether the incoming transaction is pending — needed by both + # the protection check (pending→booked bypass) and the auto-claim path below. + incoming_pending = false + if extra.is_a?(Hash) + pending_extra = extra.with_indifferent_access + incoming_pending = + ActiveModel::Type::Boolean.new.cast(pending_extra.dig("simplefin", "pending")) || + ActiveModel::Type::Boolean.new.cast(pending_extra.dig("plaid", "pending")) || + ActiveModel::Type::Boolean.new.cast(pending_extra.dig("lunchflow", "pending")) || + ActiveModel::Type::Boolean.new.cast(pending_extra.dig("enable_banking", "pending")) + end + # === PROTECTION CHECK: Skip entries that should not be overwritten === # Check persisted Transaction entries for protection flags before making changes. # This prevents sync from overwriting user edits, CSV imports, or excluded entries. if entry.persisted? skip_reason = determine_skip_reason(entry) if skip_reason + # Pending→booked bypass for user_modified entries: clear the stale pending flag + # when the provider delivers a booked version of the same transaction. + # Some ASPSPs (e.g. Revolut Italy via Enable Banking) reuse the same transaction_id + # for pending and booked, so the entry is found by external_id rather than going + # through the auto-claim path. Without this, a user who categorised a pending entry + # (setting user_modified=true) would see the pending badge stuck forever. + # Excluded and import_locked entries are intentionally left untouched. + if skip_reason == "user_modified" && !incoming_pending && entry.entryable.is_a?(Transaction) + entry_is_pending = Transaction::PENDING_PROVIDERS.any? { |p| entry.transaction.extra&.dig(p, "pending") } + if entry_is_pending + entry.transaction.update!(extra: clear_pending_flags_from_extra(entry.transaction.extra)) + end + end record_skip(entry, skip_reason) return entry end @@ -76,17 +101,6 @@ class Account::ProviderImportAdapter end end - # If still a new entry and this is a POSTED transaction, check for matching pending transactions - incoming_pending = false - if extra.is_a?(Hash) - pending_extra = extra.with_indifferent_access - incoming_pending = - ActiveModel::Type::Boolean.new.cast(pending_extra.dig("simplefin", "pending")) || - ActiveModel::Type::Boolean.new.cast(pending_extra.dig("plaid", "pending")) || - ActiveModel::Type::Boolean.new.cast(pending_extra.dig("lunchflow", "pending")) || - ActiveModel::Type::Boolean.new.cast(pending_extra.dig("enable_banking", "pending")) - end - if entry.new_record? && !incoming_pending pending_match = nil @@ -119,12 +133,7 @@ class Account::ProviderImportAdapter # exclude it from re-import (preventing the old pending from being recreated on the # next sync when the stored raw payload still contains the pending transaction data). if entry.entryable.is_a?(Transaction) - ex = (entry.transaction.extra || {}).deep_dup - Transaction::PENDING_PROVIDERS.each do |provider| - next unless ex.key?(provider) - ex[provider].delete("pending") - ex.delete(provider) if ex[provider].empty? - end + ex = clear_pending_flags_from_extra(entry.transaction.extra) if old_pending_external_id.present? existing_claims = Array.wrap(ex["auto_claimed_pending_ids"]) ex["auto_claimed_pending_ids"] = (existing_claims + [ old_pending_external_id ]).uniq @@ -134,6 +143,18 @@ class Account::ProviderImportAdapter end end + # Pending→booked for same-external-id providers (non-protected path). + # For ASPSPs like Revolut Italy that reuse the same transaction_id for pending and + # booked, the auto-claim path above is skipped (entry.persisted? from the start). + # If extra is nil (no FX, no MCC) the deep-merge block later is skipped too, so we + # must clear the stale pending flag here before the final save. + # (The auto-claim path already clears it in-memory, so this is a no-op there.) + if !incoming_pending && entry.entryable.is_a?(Transaction) + if Transaction::PENDING_PROVIDERS.any? { |p| entry.transaction.extra&.dig(p, "pending") } + entry.transaction.extra = clear_pending_flags_from_extra(entry.transaction.extra) + end + end + # Track if this is a new posted transaction (for fuzzy suggestion after save) is_new_posted = entry.new_record? && !incoming_pending @@ -963,4 +984,18 @@ class Account::ProviderImportAdapter account_name: entry.account.name } end + + private + + def clear_pending_flags_from_extra(extra) + ex = (extra || {}).deep_dup + ex = {} unless ex.is_a?(Hash) + Transaction::PENDING_PROVIDERS.each do |provider| + next unless ex.key?(provider) + next unless ex[provider].is_a?(Hash) + ex[provider].delete("pending") + ex.delete(provider) if ex[provider].empty? + end + ex + end end diff --git a/test/models/account/provider_import_adapter_test.rb b/test/models/account/provider_import_adapter_test.rb index a62c30209..29ab62fb8 100644 --- a/test/models/account/provider_import_adapter_test.rb +++ b/test/models/account/provider_import_adapter_test.rb @@ -1445,4 +1445,77 @@ class Account::ProviderImportAdapterTest < ActiveSupport::TestCase pending1.reload assert_equal "plaid_pending_1", pending1.external_id end + + # ========================================================================= + # Same-external-id pending → booked (e.g. Revolut Italy via Enable Banking) + # Some ASPSPs reuse the same transaction_id for pending and booked, so the + # entry is found by find_or_initialize_by (persisted), bypassing auto-claim. + # ========================================================================= + + test "clears pending flag when same external_id is reused for booked version (not user-modified)" do + pending_entry = @adapter.import_transaction( + external_id: "eb_same_id_123", + amount: 30.0, + currency: "EUR", + date: Date.today - 2.days, + name: "Piero Fiorista", + source: "enable_banking", + extra: { "enable_banking" => { "pending" => true } } + ) + assert pending_entry.transaction.pending?, "entry should start as pending" + + # Booked version arrives with the SAME external_id (Revolut Italy behaviour). + # extra is nil (no FX, no MCC) so the deep-merge block would normally be skipped. + assert_no_difference "@account.entries.count" do + booked_entry = @adapter.import_transaction( + external_id: "eb_same_id_123", + amount: 30.0, + currency: "EUR", + date: Date.today, + name: "Piero Fiorista", + source: "enable_banking", + extra: nil + ) + + assert_equal pending_entry.id, booked_entry.id, "should update the same entry" + assert_not booked_entry.transaction.pending?, + "pending flag should be cleared even when extra is nil" + end + end + + test "clears pending flag when same external_id reused and entry is user-modified (Revolut Italy)" do + pending_entry = @adapter.import_transaction( + external_id: "eb_same_id_user_mod", + amount: 50.0, + currency: "EUR", + date: Date.today - 3.days, + name: "Clean Center", + source: "enable_banking", + extra: { "enable_banking" => { "pending" => true } } + ) + assert pending_entry.transaction.pending?, "entry should start as pending" + + # User categorises the pending entry — sets user_modified = true + pending_entry.mark_user_modified! + assert pending_entry.reload.user_modified?, "entry should be marked user-modified" + + # Booked version with the same external_id arrives — protection check would normally + # return early and leave the pending badge intact. + assert_no_difference "@account.entries.count" do + booked_entry = @adapter.import_transaction( + external_id: "eb_same_id_user_mod", + amount: 50.0, + currency: "EUR", + date: Date.today, + name: "Clean Center", + source: "enable_banking", + extra: nil + ) + + assert_equal pending_entry.id, booked_entry.id, "should reference the same entry" + booked_entry.reload + assert_not booked_entry.transaction.pending?, + "pending flag must be cleared even for user-modified entries" + end + end end