fix(enable-banking): fix pending→posted auto-claim producing badge, duplicate, and wrong date#1783
Conversation
📝 WalkthroughWalkthroughWhen posted transactions claim matching pending entries, the adapter now preserves the pending external ID and date, clears provider pending flags, and records the old pending ID in ChangesPending Entry Auto-Claim and Re-Import Exclusion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…fter auto-claim When a booked transaction claims a pending entry via the amount/date heuristic (find_pending_transaction), two bugs caused the entry to remain incorrectly pending and the old pending transaction to reappear on subsequent syncs. Bug 1: The extra["enable_banking"]["pending"] flag was never cleared on the claimed entry. For simple booked transactions with nil extra the deep-merge path is skipped entirely, so the pending badge persisted forever. Bug 2: After the claim the old pending external_id (e.g. PDNG_123) stayed in the stored raw_transactions_payload. The importer's C4 filter only removes pending entries whose transaction_id matches a BOOK id — Enable Banking issues completely different ids for pending vs booked transactions — so PDNG_123 was never pruned. On the next sync find_or_initialize_by(PDNG_123) couldn't find the claimed entry (now keyed as BOOK_456) and created a fresh pending duplicate with no category. Fix: on claim, explicitly clear all providers' pending keys from extra in-memory, and store the displaced pending external_id in extra["auto_claimed_pending_ids"]. The Processor now queries this field alongside manual_merge to build the excluded_ids set, so the stale pending data is skipped on every future sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a pending transaction is claimed by a booked transaction, the original pending date is now preserved instead of being overwritten by the booked transaction's date. This ensures historical accuracy for transactions that were originally recorded on a different date.
CrossDrain
force-pushed
the
eb-fix-claiming-pending-transactions
branch
from
cce7cf8 to
76a7e98
Compare
superagent-security
Bot
added
contributor:verified
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/enable_banking_account/transactions/processor.rb (1)
79-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog message no longer matches all exclusion sources.
excluded_idsnow unionsmanual_mergeandauto_claimed_pending_ids, but the skip log always attributes the skip to "manually merged" — making it harder to debug why an auto-claimed pending was skipped vs. one a user merged. Cheap to disambiguate by tracking the source alongside the ID set:♻️ Suggested diff
- manually_merged_ids = Transaction.joins(:entry) + manually_merged_ids = Transaction.joins(:entry) .where(entries: { account_id: 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 @@ - manually_merged_ids | auto_claimed_ids + { + manual_merge: manually_merged_ids, + auto_claimed_pending: auto_claimed_ids + } else - Set.new + { manual_merge: Set.new, auto_claimed_pending: Set.new } end @@ - if ext_id && excluded_ids.include?(ext_id) - Rails.logger.info("EnableBankingAccount::Transactions::Processor - Skipping re-import of manually merged pending transaction: #{ext_id}") + skip_source = if excluded_ids[:manual_merge].include?(ext_id) + "manually merged" + elsif excluded_ids[:auto_claimed_pending].include?(ext_id) + "auto-claimed by booked" + end + + if ext_id && skip_source + Rails.logger.info("EnableBankingAccount::Transactions::Processor - Skipping re-import of #{skip_source} pending transaction: #{ext_id}") skipped_count += 1 next end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/enable_banking_account/transactions/processor.rb` around lines 79 - 83, The skip log in EnableBankingAccount::Transactions::Processor incorrectly always says "manually merged" even though excluded_ids is a union of manual_merge and auto_claimed_pending_ids; change the exclusion logic to track the source per ext_id (e.g., build a mapping/hash or tag each id when combining manual_merge and auto_claimed_pending_ids) and then, inside the check for ext_id inclusion, read the source for that ext_id and log it (use ext_id and its source in the message), increment skipped_count as before, and call next; update any references to excluded_ids, manual_merge, and auto_claimed_pending_ids accordingly so the log accurately reflects whether the skip was due to a manual_merge or auto_claimed_pending_ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/models/enable_banking_account/transactions/processor.rb`:
- Around line 79-83: The skip log in
EnableBankingAccount::Transactions::Processor incorrectly always says "manually
merged" even though excluded_ids is a union of manual_merge and
auto_claimed_pending_ids; change the exclusion logic to track the source per
ext_id (e.g., build a mapping/hash or tag each id when combining manual_merge
and auto_claimed_pending_ids) and then, inside the check for ext_id inclusion,
read the source for that ext_id and log it (use ext_id and its source in the
message), increment skipped_count as before, and call next; update any
references to excluded_ids, manual_merge, and auto_claimed_pending_ids
accordingly so the log accurately reflects whether the skip was due to a
manual_merge or auto_claimed_pending_ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6852ba6-823b-48fd-8a49-c1b2b63de277
📒 Files selected for processing (4)
app/models/account/provider_import_adapter.rbapp/models/enable_banking_account/transactions/processor.rbtest/models/account/provider_import_adapter_test.rbtest/models/enable_banking_account/transactions/processor_test.rb
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
|
@CrossDrain I still see the "pending" badge after my last reimport, is this fix retroactive? |
|
Hey @abeilprincipino! First of all, thank you so much for testing this. This is expected behaviour for transactions that were synced before this fix landed, here's why: The fix only clears the New pending→posted claims going forward will be handled correctly. Hopefully yours too after the steps below. To fix your existing transactions, you can run the following in the Rails console: 1. Open the console From the directory where your docker compose exec web bin/rails consoleWait for the 2. Dry run — see what will be changed Paste this first to inspect affected transactions without changing anything: # Dry run — inspect before applying
stale = Transaction
.joins(:entry)
.where("entries.source = 'enable_banking'")
.where("(transactions.extra -> 'enable_banking' ->> 'pending')::boolean = true")
puts "Found #{stale.count} transaction(s) with stale pending flag"
stale.limit(10).each do |t|
e = t.entry
puts " entry=#{e.id} date=#{e.date} name=#{e.name} amount=#{e.amount}"
endYou should see a count and a list of entries with their date, name, and amount. Verify these look like the transactions with the stale badge. 3. Apply the fix If the output looks right, paste this to clear the flag: fixed = 0
stale.find_each do |t|
ex = t.extra.deep_dup
ex["enable_banking"]&.delete("pending")
ex.delete("enable_banking") if ex["enable_banking"]&.empty?
t.update!(extra: ex)
fixed += 1
end
puts "Done — cleared pending flag on #{fixed} transaction(s)"It will print Once done, you can exit the console with If you do run this, please share your results and let us know whether it helped and whether the overall pending→posted flow is working correctly for you now as it'll be really useful to know! |
|
It worked! Thx a lot again |
You're welcome, glad it's working! Out of curiosity, does the pending → booked transaction claim workflow work for you as well? (i.e. when a pending transaction gets confirmed/posted by the bank, does Sure correctly match it to the existing pending entry instead of creating a duplicate?) |
|
Honestly it doesn’t look like it’s working, I’ve pending transaction since 5 days ago. On the other hand I haven’t experienced the problem you underlined in first place: I never got duplicated transactions, simply they are never becoming “not pending”. |
|
Thanks for testing! "Stuck pending, no duplicates" usually means the bank hasn't returned a booked version yet. The auto-claim only kicks in once a BOOK row actually arrives. A few users in #1470 reported the same thing and it eventually resolved when the bank booked them (sometimes a week+ for EU banks). Two quick things that'd help:
If they're booked on the bank's side but still pending in Sure after the next sync, that's a real bug and I'll dig in. |
|
I really think there’s a bug, usually it takes 3-4 days to get transactions booked from my bank (Revolut in this case). |
|
@abeilprincipino Yes, the test is sensible. If you delete the stuck pendings and the very next sync brings them straight back as booked, that's a strong signal that EB was returning a booked version all along and something in the claim path isn't matching for your setup. But I want to flag upfront: this would be genuinely weird behaviour. I'm also on Enable Banking with Revolut and the pending → booked flow has always worked correctly for me, even before this PR landed (this PR fixed the artefacts of an auto-claim that did happen, not whether the claim happened at all). Two things that would really sharpen the test:
And one extra thing that'd help us narrow it down! Before you run the cleanup, grab the Transaction.joins(:entry)
.where("entries.source = 'enable_banking'")
.where("(transactions.extra -> 'enable_banking' ->> 'pending')::boolean = true")
.limit(5).each do |t|
puts "entry=#{t.entry.id} date=#{t.entry.date} name=#{t.entry.name} amount=#{t.entry.amount} external_id=#{t.entry.external_id}"
endThen after the cleanup + next sync, we can compare. |
|
The ASPS is Revolut (Italy). I've tried what you mentioned, you can find the log before the fix attached to this message. The only thing I can think off is that I've categorized the expenses when they were still "Pending", I know that there's a kind of "override mode" if you manually setup the category that prevents the next sync to override again your choice... but maybe it's unrelated. Btw, thanks in advance for the support and let me know if I can help narrowing down the problem. |
|
@abeilprincipino Thanks for the detailed report and for your patience, this was really helpful to track down! 🙏 This turned out to be outside the scope of this PR, but clearly worth fixing on its own, so we've addressed it separately. What happened: Revolut Italy (unlike most other banks) reuses the exact same transaction ID for both the pending and the booked version of a transaction. Our sync logic normally handles pending→booked by matching on that ID and updating the entry. However, because the ID was already in the database, the entry was found as an existing record and the update path was skipped entirely. The pending badge was stuck with no way to clear itself, even after the bank confirmed the transaction. Categorizing the entry made it even worse, since the protection check (which prevents sync from overwriting user edits) would return early before the flag could ever be cleared. The fix: We added a small bypass so that even when an entry is protected or already persisted, if the bank delivers a booked version, the pending flag gets cleared. We've opened #1982 as a proposed fix. Since we can't reproduce the Revolut Italy behaviour directly, it would mean a lot if you could test it on your end and let us know whether it resolves the issue for you! Thanks again for taking the time to share your setup and reproduce steps! |
4 participants
Problem
When Enable Banking syncs a booked transaction that matches an existing pending entry by amount/date (the auto-claim path in
find_pending_transaction), three things went wrong:1. Pending badge on the booked entry
The
extra["enable_banking"]["pending"] = trueflag was never cleared on the claimed entry. For simple transactions (no FX, no MCC) the incomingextraisnil, so the deep-merge block is skipped entirely — the badge persisted forever.2. The original pending entry reappeared after the next sync
After the claim, the old pending
external_id(e.g.PDNG_123) stayed in the storedraw_transactions_payload. Enable Banking issues completely different IDs for pending vs. booked transactions, so the importer's C4 filter — which only matches bytransaction_idequality — never pruned it. On the next sync,find_or_initialize_by(external_id: "PDNG_123")couldn't find the claimed entry (now keyed asBOOK_456) and created a fresh pending duplicate with no category.3. Wrong date on the claimed entry
The pending date (when the transaction was initiated) was silently overwritten with the booked/settlement date — inconsistent with how the manual
merge_with_duplicate!path already handles this.The result visible to users: a booked transaction showing a pending badge, the user-assigned category, and a later date; plus the original pending transaction reappearing alongside it with no category and the correct earlier date.
Solution
ProviderImportAdapter#import_transactionpending_entry_dateandold_pending_external_idbefore claiming.pending_entry_date || datewhen assigning attributes so the pending date is preserved.pendingkey from all providers inextrain-memory (persisted by the existing save path).old_pending_external_idinextra["auto_claimed_pending_ids"]so the Processor knows to skip it.EnableBankingAccount::Transactions::Processor#processmanual_mergeexclusion to also collect allauto_claimed_pending_ids, then union both intoexcluded_ids. This prevents the old pendingexternal_idfrom being re-imported on every subsequent sync.Testing
provider_import_adapter_test.rb: new test covers nil-extra auto-claim — asserts pending flag cleared, date preserved, andauto_claimed_pending_idspopulated.processor_test.rb: two new tests — one forauto_claimed_pending_idsexclusion alone, one for both exclusion types together in the same batch.Summary by CodeRabbit