feat(enable-banking): safe pending transaction merge with sync re-import prevention#1709
Conversation
brin-security-scanner
Bot
added
the
contributor:verified
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a locked atomic merge flow for pending duplicates with metadata tracking, prevents re-importing manually-merged transactions by external_id, centralizes Enable Banking external_id computation, makes pending detection provider-agnostic via dynamic SQL, adds controller exception handling, and provides comprehensive test coverage for processor and merge behavior. ChangesPending Duplicate Merge with Re-import Prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f702e924
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
CrossDrain
force-pushed
the
eb-merge-pending-transactions
branch
from
f6f702e to
cf4d2f0
Compare
brin-security-scanner
Bot
added
pr:verified
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/models/enable_banking_account/transactions/processor.rb (1)
9-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the processor result shape consistent on the empty-payload path.
This early return omits
:skipped, while the normal path always includes it. Any caller consuming the new API now has to special-case the zero-transaction case. Returnskipped: 0here too.Suggested fix
- return { success: true, total: 0, imported: 0, failed: 0, errors: [] } + return { success: true, total: 0, imported: 0, skipped: 0, failed: 0, errors: [] }🤖 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 9 - 11, The early return in EnableBankingAccount::Transactions::Processor inside the unless enable_banking_account.raw_transactions_payload.present? branch returns a result hash missing the :skipped key; update that return to include skipped: 0 so the processor result shape matches the normal path (i.e., return { success: true, total: 0, imported: 0, failed: 0, skipped: 0, errors: [] }).app/controllers/pending_duplicate_merges_controller.rb (1)
22-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the new flash messages.
These new notices/alerts are hard-coded English strings, so they bypass the repo's i18n convention and won't be translatable. Please move them to locale keys and use
t(...)here.As per coding guidelines
**/*.{erb,rb}:Always use t() helper for user-facing strings.Also applies to: 66-68
🤖 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/controllers/pending_duplicate_merges_controller.rb` around lines 22 - 58, Replace the hard-coded English flash strings in the manual merge flow with i18n keys and use the t(...) helper: update the redirect_back_or_to calls that pass "Please select a posted transaction to merge with", "Invalid transaction selected for merge", "Pending transaction merged with posted transaction", and "Could not merge transactions" to use t(...) (e.g. t("transactions.merge_duplicate.select_posted"), t("transactions.merge_duplicate.invalid_selection"), t("transactions.merge_duplicate.success"), t("transactions.merge_duplicate.failure_manual") respectively), keep the control flow around merge_params, find_eligible_posted_entry, `@transaction.update`!(...), and `@transaction.merge_with_duplicate`! unchanged, and add corresponding locale entries in the locale yml so the strings are translatable (also update the similar messages mentioned at lines 66-68 to use t()).
🤖 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.
Inline comments:
In `@app/models/enable_banking_entry/processor.rb`:
- Around line 13-17: compute_external_id currently raises when both
transaction_id and entry_reference are missing, and the process method re-logs
external_id inside rescue blocks causing exceptions during error handling;
change compute_external_id (or add a new safe_external_id helper) to return a
non-raising diagnostic string (e.g. "enable_banking_unknown" or "unknown")
instead of raising/nil, and update process to cache that safe identifier at the
start (use the cached safe id in all rescue/logging paths) so logging never
calls the potentially-raising external_id method; update references mentioned
around compute_external_id and process (and the rescue lines ~72-75) to use the
cached safe id.
In `@app/models/transaction.rb`:
- Around line 200-249: The merge currently only locks entries but then
reads/modifies the associated Transaction (posted_tx) which can be concurrently
updated; after assigning posted_tx = posted_entry.entryable (and before reading
posted_tx.extra or changing category_id), acquire a row lock on the transaction
(e.g. call posted_tx.lock! and similarly lock pending_transaction if you read
its DB state) and handle ActiveRecord::RecordNotFound the same way you did for
entries; this ensures the posted_tx row is DB-fresh and prevents lost updates
before building tx_attrs and calling posted_tx.update!.
In `@db/migrate/20260508120000_add_manual_merge_ext_id_index.rb`:
- Around line 2-6: The migration currently uses add_index on the transactions
table for Arel.sql("(extra->'manual_merge'->>'merged_from_external_id')") with
name "idx_transactions_manual_merge_ext_id" and a where clause, which can block
writes; update the migration to run the index build concurrently by adding
disable_ddl_transaction! at the top of the migration and passing algorithm:
:concurrently to add_index (keep the same Arel.sql expression, name and where
clause), so the final change method disables the DDL transaction and calls
add_index(..., algorithm: :concurrently, name:
"idx_transactions_manual_merge_ext_id", where: "(extra ? 'manual_merge')").
In `@test/models/transaction/merge_with_duplicate_test.rb`:
- Around line 190-202: The test uses `@posted_entry.dup` which creates a
non-persisted record (id: nil) and can trigger persisted?/id guards before lock!
is called; replace the dup-based ghost with an object that preserves the real
id/account_id and any other attributes read by merge_with_duplicate! (e.g., use
an OpenStruct or a mocha stub) and stub its lock! to raise
ActiveRecord::RecordNotFound so execution reaches the lock! rescue path; ensure
you stub the transaction.potential_duplicate_entry to return this constructed
ghost (referencing merge_with_duplicate!, potential_duplicate_entry, lock!, and
`@posted_entry`) and include any attributes the method inspects so the guard
checks pass.
---
Outside diff comments:
In `@app/controllers/pending_duplicate_merges_controller.rb`:
- Around line 22-58: Replace the hard-coded English flash strings in the manual
merge flow with i18n keys and use the t(...) helper: update the
redirect_back_or_to calls that pass "Please select a posted transaction to merge
with", "Invalid transaction selected for merge", "Pending transaction merged
with posted transaction", and "Could not merge transactions" to use t(...) (e.g.
t("transactions.merge_duplicate.select_posted"),
t("transactions.merge_duplicate.invalid_selection"),
t("transactions.merge_duplicate.success"),
t("transactions.merge_duplicate.failure_manual") respectively), keep the control
flow around merge_params, find_eligible_posted_entry, `@transaction.update`!(...),
and `@transaction.merge_with_duplicate`! unchanged, and add corresponding locale
entries in the locale yml so the strings are translatable (also update the
similar messages mentioned at lines 66-68 to use t()).
In `@app/models/enable_banking_account/transactions/processor.rb`:
- Around line 9-11: The early return in
EnableBankingAccount::Transactions::Processor inside the unless
enable_banking_account.raw_transactions_payload.present? branch returns a result
hash missing the :skipped key; update that return to include skipped: 0 so the
processor result shape matches the normal path (i.e., return { success: true,
total: 0, imported: 0, failed: 0, skipped: 0, errors: [] }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2da8667-090d-4a92-aa39-3cb598a85f1e
📒 Files selected for processing (10)
app/controllers/pending_duplicate_merges_controller.rbapp/controllers/transactions_controller.rbapp/models/enable_banking_account/transactions/processor.rbapp/models/enable_banking_entry/processor.rbapp/models/entry.rbapp/models/transaction.rbdb/migrate/20260508120000_add_manual_merge_ext_id_index.rbdb/schema.rbtest/models/enable_banking_account/transactions/processor_test.rbtest/models/transaction/merge_with_duplicate_test.rb
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/models/transaction/merge_with_duplicate_test.rb (1)
230-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a persisted-like mock instead of
dupfor the lock-race test path.Line 234 uses
@posted_entry.dup, which can produce a non-persisted object (id=nil) and short-circuit guards beforelock!is reached, so this test may not actually exercise the lock-failure rescue path.Proposed fix
- ghost = `@posted_entry.dup.tap` { |e| e.stubs(:lock!).raises(ActiveRecord::RecordNotFound) } + ghost = OpenStruct.new( + id: `@posted_entry.id`, + account_id: `@posted_entry.account_id`, + date: `@posted_entry.date`, + name: `@posted_entry.name` + ) + ghost.stubs(:persisted?).returns(true) + ghost.stubs(:lock!).raises(ActiveRecord::RecordNotFound) `@pending_entry.transaction.stubs`(:potential_duplicate_entry).returns(ghost)#!/bin/bash # Verify this file no longer uses dup-based ghost objects in the concurrency test. rg -nP --type=rb -C2 '@posted_entry\.dup\.tap|OpenStruct\.new' test/models/transaction/merge_with_duplicate_test.rbAs per coding guidelines, "Always prefer
OpenStructwhen creating mock instances, or in complex cases, a mock class."🤖 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 `@test/models/transaction/merge_with_duplicate_test.rb` around lines 230 - 236, The test uses `@posted_entry.dup` to build ghost which can be non-persisted (id=nil) and cause early exits instead of exercising the lock-failure path; replace the dup-based ghost with a persisted-like mock (e.g., an OpenStruct or a stubbed object with an id and any required attributes) that responds to lock! and whose lock! raises ActiveRecord::RecordNotFound, then stub `@pending_entry.transaction.potential_duplicate_entry` to return that mock so merge_with_duplicate! goes through the same code path as a real persisted posted_entry and triggers the rescue handling.
🤖 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.
Duplicate comments:
In `@test/models/transaction/merge_with_duplicate_test.rb`:
- Around line 230-236: The test uses `@posted_entry.dup` to build ghost which can
be non-persisted (id=nil) and cause early exits instead of exercising the
lock-failure path; replace the dup-based ghost with a persisted-like mock (e.g.,
an OpenStruct or a stubbed object with an id and any required attributes) that
responds to lock! and whose lock! raises ActiveRecord::RecordNotFound, then stub
`@pending_entry.transaction.potential_duplicate_entry` to return that mock so
merge_with_duplicate! goes through the same code path as a real persisted
posted_entry and triggers the rescue handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40b3520f-b24c-4983-b8e9-4274a1851a69
📒 Files selected for processing (4)
app/models/enable_banking_account/transactions/processor.rbapp/models/transaction.rbtest/models/enable_banking_account/transactions/processor_test.rbtest/models/transaction/merge_with_duplicate_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- test/models/enable_banking_account/transactions/processor_test.rb
- app/models/enable_banking_account/transactions/processor.rb
- app/models/transaction.rb
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_entry/processor.rb (1)
232-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
log_invalid_currencystill calls the potentially-raisingexternal_id— the fix inprocesswas incomplete.This is the same class of defect the PR's
safe_idintroduction was designed to eliminate.log_invalid_currencyis called viaparse_currency→currency→ insideprocess'sbegin/rescue. When bothtransaction_idandentry_referenceare absent, the call toexternal_idon line 233 raisesArgumentErrorbefore the warning is logged, meaning the diagnostic message is silently swallowed and the raise propagates as anArgumentError(which the outer rescue re-raises as aStandardError). The fix is the same pattern already used inprocess: usesafe_idor compute a non-raising identifier inline.Because
safe_idis scoped toprocess, the cleanest fix is to move the safe fallback into a memoized private helper:🐛 Proposed fix
+ def safe_external_id + self.class.compute_external_id(data) || "unknown" + 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") + Rails.logger.warn("Invalid currency code '#{currency_value}' in Enable Banking transaction #{safe_external_id}, falling back to account currency") endAnd in
process, replace the inline computation with the shared helper:- safe_id = self.class.compute_external_id(`@enable_banking_transaction`) || "unknown" + safe_id = safe_external_idBased on learnings: the project standard is to override
log_invalid_currencyin each provider processor to include provider-specific context — this override correctly does so, but it must not call a method that can raise.🤖 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_entry/processor.rb` around lines 232 - 234, log_invalid_currency currently calls external_id which can raise when both transaction_id and entry_reference are missing; create a private memoized helper (e.g., safe_identifier or safe_id_helper) that returns transaction_id || entry_reference || "(no id)" without raising, use that helper inside log_invalid_currency (and in parse_currency/currency if they still call external_id), and remove any direct external_id calls from log_invalid_currency so the warning never triggers an ArgumentError before logging.
🤖 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_entry/processor.rb`:
- Around line 232-234: log_invalid_currency currently calls external_id which
can raise when both transaction_id and entry_reference are missing; create a
private memoized helper (e.g., safe_identifier or safe_id_helper) that returns
transaction_id || entry_reference || "(no id)" without raising, use that helper
inside log_invalid_currency (and in parse_currency/currency if they still call
external_id), and remove any direct external_id calls from log_invalid_currency
so the warning never triggers an ArgumentError before logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a53d2d83-594f-4a9e-a2b5-78d29838f66f
📒 Files selected for processing (4)
app/models/enable_banking_entry/processor.rbapp/models/transaction.rbdb/migrate/20260508120000_add_manual_merge_ext_id_index.rbtest/models/transaction/merge_with_duplicate_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- db/migrate/20260508120000_add_manual_merge_ext_id_index.rb
- test/models/transaction/merge_with_duplicate_test.rb
- app/models/transaction.rb
|
Three follow-up fixes after review:
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Move the SQL fragment used to identify pending transactions from the `Entry` model to a constant in the `Transaction` model. This improves maintainability and ensures that the logic for determining if a transaction is pending is defined in a single location.
|
Really solid work on the atomic merge and lock ordering. Two things on the index and the excluded-IDs query: 1. The functional index doesn't cover the new Array format The migration adds: add_index :transactions,
Arel.sql("(extra->'manual_merge'->>'merged_from_external_id')"),
where: "(extra ? 'manual_merge')"The Additionally, the sync pre-fetch in .where("transactions.extra ? 'manual_merge'")
.pluck(:extra)That If you want to index for the array case, a GIN index with 2. Transaction.joins(:entry)
.where(entries: { account_id: ... })
.where("transactions.extra ? 'manual_merge'")
.pluck(:extra)
.flat_map { |extra| ... }This pulls the full Not a blocker, but something to revisit if this becomes a performance concern in production. Generated by Claude Code Generated by Claude Code |
|
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
2 participants
Overview
Enable Banking syncs pending transactions before they are settled by the bank. Once the bank posts the settled version, users are left with two entries for the same transaction. This PR gives users the ability to manually merge a pending entry into its posted counterpart and ensures subsequent syncs never re-import the already-resolved pending entry. It also corrects a systemic gap where
enable_bankingwas silently excluded from every provider-aware pending scope inEntry, making stale-entry cleanup and duplicate-suggestion detection inoperative for this provider.The Problem
Context:
Enable Banking accounts accumulate duplicate entry pairs — one pending, one posted. The merge UI (added in #1088) exists, but the backend it calls was a thin placeholder that was neither atomic nor durable. Stale and re-imported pending entries continued to bloat the transaction list and corrupt balance calculations even after a user performed a merge.
Root Cause / Requirement:
entry.destroy!— non-atomic, no locking, no sync suppression. The next sync immediately re-imported the destroyed pending entry, undoing the user's action.Entry.pending,Entry.excluding_pending, andreconcile_pending_duplicateshardcoded onlysimplefin,plaid, andlunchflow—enable_bankingentries were invisible to stale cleanup and the fuzzy-match suggestion engine.external_idconstruction formula was duplicated across two processors, creating a silent divergence risk.Linked Issues:
Key Changes
Features
app/models/transaction.rb—merge_with_duplicate!ApplicationRecord.transaction(requires_new: true)with row-levelSELECT FOR UPDATElocks on both entriesposted_entry.account_id != pending_entry.account_id"manual_merge"metadata ontoposted_tx.extra(keys:merged_from_entry_id,merged_from_external_id,merged_at,source) so the sync engine can detect and skip re-import on subsequent runsuser_modified; name and merchant are intentionally preserved from the canonical posted recordposted_entry.mark_user_modified!after merge to protect the posted entry from future sync overwritesapp/models/transaction.rb— new model API backing the pre-existing merge UIhas_potential_duplicate?— returnstruewhen a non-dismissedpotential_posted_matchis presentpotential_duplicate_entry— resolves the suggested postedEntryfrom match metadatapotential_duplicate_confidence— returns match confidence ("low"/"medium"/"high")potential_duplicate_posted_amount— exposes the posted amount for UI comparisonlow_confidence_duplicate?— convenience predicatedismiss_duplicate_suggestion!— marksdismissed: trueon the stored suggestion, blocking further merge attemptsclear_duplicate_suggestion!— removes thepotential_posted_matchkey entirelypending_duplicate_candidates(limit:, offset:)— returns paginated posted entries from the same account and currency eligible for manual mergeapp/models/enable_banking_account/transactions/processor.rbmerged_from_external_idvalues into an in-memorySet(one query per sync, O(1) per transaction lookup) before the main processing loopexternal_idappears in the merged set; records the skip under a dedicatedskipped_countcounter:skippedkey to the result hash so callers can distinguish new imports from no-op skipsRefactors
app/models/enable_banking_entry/processor.rbself.compute_external_id(raw_transaction_data)as a public class method — single authoritative source for the"enable_banking_#{id}"formulaexternal_iddelegates to the class method; callers outside the class use the class method directly, eliminating duplicationapp/models/entry.rb—scope :pending,scope :excluding_pendingTransaction::PENDING_PROVIDERS, makingenable_banking(and any future provider) automatically includedapp/models/entry.rb—reconcile_pending_duplicatesnot_pending_sqlvariable built fromTransaction::PENDING_PROVIDERS; replaces two identical hardcodedWHEREblocks for exact-match and fuzzy-match candidate queriespotential_posted_matchhash now includes"confidence": "medium"and"dismissed": falsefor structural consistency with the merge readerapp/models/transaction.rb—merge_with_duplicate!UPDATEstatements onposted_tx(metadata write + category write) into a singleupdate!(tx_attrs)call, halvingafter_savecallback fires and DB round-trips|| {}guard onposted_tx.extra(transactions.extraisnull: false, default: {})Bug Fixes
app/models/transaction.rb—merge_with_duplicate!return false unless pending?as first guard — prevents non-pending transactions with stalepotential_posted_matchdata from entering the merge flowpending_entry.lock!andposted_entry.lock!are now individually guarded:pending_entrygone → idempotentreturn true;posted_entrygone →raise ActiveRecord::Rollback→ returnsfalse(previously: unhandled 500)merge_succeededflag replaces the hardcodedtruereturn so rollback paths correctly returnfalseapp/models/transaction.rb—pending?rescuetorescue StandardErrorto avoid swallowingSignalException,Interrupt, andNoMemoryErrorapp/controllers/transactions_controller.rb—merge_duplicaterescueto includeActiveRecord::RecordNotFound,ActiveRecord::Deadlocked,ActiveRecord::LockWaitTimeoutin addition to pre-existingRecordInvalid/RecordNotDestroyedapp/controllers/pending_duplicate_merges_controller.rb—createrescueclause forRecordInvalid,RecordNotDestroyed,Deadlocked,LockWaitTimeout— previously any database exception frommerge_with_duplicate!produced an unhandled 500Expected Impact
enable_bankingpending entries are fully visible — stale cleanup and fuzzy-match suggestions now work for this providerdestroy!; both race paths return gracefully instead of 500ingexcluded_idspre-fetch uses a partial functional index instead of a sequential scan; per-merge DB writes reduced from 2 → 1Technical Implementation
Architectural Decisions
Savepoint-based atomic merge (
requires_new: true)merge_with_duplicate!wraps all writes insideApplicationRecord.transaction(requires_new: true), creating a PostgreSQL savepoint. If the outer request is already inside a transaction (e.g., a controller callback), the savepoint rolls back independently without aborting the outer transaction. This is critical for correctness in theDeadlocked/LockWaitTimeoutpaths.Consistent lock ordering to prevent deadlocks
Entries are always locked in the same order:
pending_entryfirst,posted_entrysecond. Any concurrent merge of the same pair will block on the first lock and re-evaluate state once released, rather than deadlocking. This is the standard two-phase locking protocol applied to polymorphic AR records.Durable merge marker as the sync skip signal
Rather than relying on the pending entry's absence (which is fragile — a bug or re-sync could recreate it), the merge writes
merged_from_external_idonto the surviving posted transaction'sextraJSONB field. TheProcessorreads this at sync time and skips re-import. This is a write-once audit trail, not a soft-delete flag.compute_external_idas single source of truthThe ID formula (
transaction_id || entry_reference→"enable_banking_#{id}") must be identical in both the sync skip check and the entry processor. Extracting it toEnableBankingEntry::Processor.compute_external_ideliminates the class of bug where one site is updated and the other isn't — silent double-imports being the failure mode.Dynamic
PENDING_PROVIDERSSQL generationEntry.pendingpreviously hardcoded provider names. Any new provider added toTransaction::PENDING_PROVIDERSnow propagates automatically to all scopes — zero-touch extensibility for future integrations.Data Flow (Merge Path)
Testing & Validation
Test Coverage
Unit Tests —
test/models/transaction/merge_with_duplicate_test.rb— 33 casesTransaction.countdiff), metadata written, date/category inherited, name preserved,user_modifiedsetpending_entryalready destroyed (idempotenttrue);posted_entrydeleted between check and lock (returnsfalse, no entries destroyed)user_modifiedprotection: date and category not overwritten when posted entry is already protected; metadata still writtenexternal_idpath: metadata block skipped cleanlydismiss_duplicate_suggestion!: dismissed flag set,has_potential_duplicate?returnsfalse, merge blockedclear_duplicate_suggestion!: key removed, returnsfalsewhen already absentpending_duplicate_candidates: same-account posted entries returned; self excluded; other-account entries excluded; returns empty when not pendingUnit Tests —
test/models/enable_banking_account/transactions/processor_test.rb— 5 cases{ success: true, total: 0, … }skipped: 1,imported: 0,failed: 0for a skipped-only batchIntegration — Full suite (
bin/rails test, 3,471 tests) — all pre-existing failures confirmed pre-existing and in unrelated areas (subscriptions, market data, AI chat)Test Results / Logs
Summary by CodeRabbit
Bug Fixes
New Features