QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-05-29 07:24:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(enable_banking): clear stuck pending flag when ASPSP reuses same transaction_id (#1982)

Browse Source 
* 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
This commit is contained in:
CrossDrain
2026-05-27 21:36:33 +00:00
committed by GitHub
parent b3fce37424
commit a3609b81d3
2 changed files with 125 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
app/models/account/provider_import_adapter.rb
View File

@@ -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

73
test/models/account/provider_import_adapter_test.rb
View File

@@ -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