fix(enable_banking): clear stuck pending flag when ASPSP reuses same transaction_id#1982
fix(enable_banking): clear stuck pending flag when ASPSP reuses same transaction_id#1982CrossDrain wants to merge 9 commits into
Conversation
…transaction_id for booked version
superagent-security
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 (1)
📝 WalkthroughWalkthroughAccount::ProviderImportAdapter#import_transaction computes incoming_pending earlier, adds a private helper to clear provider ChangesPending-to-booked reconciliation with stale flag clearing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: d4227d6b13
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/account/provider_import_adapter.rb (1)
157-173: 💤 Low valueConsider extracting the duplicate pending-flag clearing logic.
The clearing logic at lines 164-172 is nearly identical to lines 75-80. Extracting this into a private helper would improve maintainability.
♻️ Suggested refactor
# Add private helper method at the end of the class private def clear_pending_flags_from_extra(extra_hash) return nil if extra_hash.blank? ex = extra_hash.deep_dup Transaction::PENDING_PROVIDERS.each do |p| next unless ex.key?(p) ex[p].delete("pending") ex.delete(p) if ex[p].empty? end ex endThen replace the duplicate blocks:
# In protection check (lines 75-80): - ex = (entry.transaction.extra || {}).deep_dup - Transaction::PENDING_PROVIDERS.each do |p| - next unless ex.key?(p) - ex[p].delete("pending") - ex.delete(p) if ex[p].empty? - end + ex = clear_pending_flags_from_extra(entry.transaction.extra) entry.transaction.update!(extra: ex) # In post-protection reconciliation (lines 165-170): - ex = (entry.transaction.extra || {}).deep_dup - Transaction::PENDING_PROVIDERS.each do |p| - next unless ex.key?(p) - ex[p].delete("pending") - ex.delete(p) if ex[p].empty? - end + ex = clear_pending_flags_from_extra(entry.transaction.extra) entry.transaction.extra = ex🤖 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/account/provider_import_adapter.rb` around lines 157 - 173, Duplicate logic that removes "pending" flags from entry.transaction.extra (iterating Transaction::PENDING_PROVIDERS, deleting "pending" keys and removing empty provider hashes) should be extracted into a private helper (e.g., clear_pending_flags_from_extra(extra_hash)). Implement that helper to deep_dup the incoming extra, perform the same deletion loop, and return the modified hash (or nil/unchanged value consistent with current behavior). Replace both duplicated blocks that operate on entry.transaction.extra with a call to clear_pending_flags_from_extra(entry.transaction.extra) and assign the result back to entry.transaction.extra when non-nil.
🤖 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.
Nitpick comments:
In `@app/models/account/provider_import_adapter.rb`:
- Around line 157-173: Duplicate logic that removes "pending" flags from
entry.transaction.extra (iterating Transaction::PENDING_PROVIDERS, deleting
"pending" keys and removing empty provider hashes) should be extracted into a
private helper (e.g., clear_pending_flags_from_extra(extra_hash)). Implement
that helper to deep_dup the incoming extra, perform the same deletion loop, and
return the modified hash (or nil/unchanged value consistent with current
behavior). Replace both duplicated blocks that operate on
entry.transaction.extra with a call to
clear_pending_flags_from_extra(entry.transaction.extra) and assign the result
back to entry.transaction.extra when non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00c84dd7-9523-4853-95b8-81219362b11b
📒 Files selected for processing (1)
app/models/account/provider_import_adapter.rb
…e pending-flag removal logic
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/account/provider_import_adapter.rb (1)
75-81: 💤 Low valueConsider using the new helper here for consistency.
Lines 75–80 duplicate the logic now extracted into
clear_pending_flags_from_extra. Reusing the helper would reduce duplication.Suggested change
if entry_is_pending - ex = (entry.transaction.extra || {}).deep_dup - Transaction::PENDING_PROVIDERS.each do |p| - next unless ex.key?(p) - ex[p].delete("pending") - ex.delete(p) if ex[p].empty? - end - entry.transaction.update!(extra: ex) + entry.transaction.update!(extra: clear_pending_flags_from_extra(entry.transaction.extra)) 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/account/provider_import_adapter.rb` around lines 75 - 81, This block duplicates logic now in clear_pending_flags_from_extra: remove the manual deep_dup/each/delete/update! block and call clear_pending_flags_from_extra with the transaction instead (e.g. clear_pending_flags_from_extra(entry.transaction)); if the helper returns a new extra hash, assign it and call entry.transaction.update!(extra: returned_extra), otherwise rely on the helper to persist the change—use the clear_pending_flags_from_extra helper to centralize the behavior and eliminate the duplicated logic that currently iterates Transaction::PENDING_PROVIDERS and mutates extra.
🤖 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.
Nitpick comments:
In `@app/models/account/provider_import_adapter.rb`:
- Around line 75-81: This block duplicates logic now in
clear_pending_flags_from_extra: remove the manual deep_dup/each/delete/update!
block and call clear_pending_flags_from_extra with the transaction instead (e.g.
clear_pending_flags_from_extra(entry.transaction)); if the helper returns a new
extra hash, assign it and call entry.transaction.update!(extra: returned_extra),
otherwise rely on the helper to persist the change—use the
clear_pending_flags_from_extra helper to centralize the behavior and eliminate
the duplicated logic that currently iterates Transaction::PENDING_PROVIDERS and
mutates extra.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c435db3-16a2-4d9c-82e1-354c9718e098
📒 Files selected for processing (1)
app/models/account/provider_import_adapter.rb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/account/provider_import_adapter.rb (1)
988-996: 💤 Low valueConsider adding a type guard for defensive safety.
If
ex[provider]is ever not a Hash (e.g.,nilor malformed data from a migration/manual edit), line 992 would raiseNoMethodError. While the current data model makes this unlikely, a small guard adds resilience.🛡️ Optional defensive improvement
def clear_pending_flags_from_extra(extra) ex = (extra || {}).deep_dup 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🤖 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/account/provider_import_adapter.rb` around lines 988 - 996, The method clear_pending_flags_from_extra should defensively ensure ex[provider] is a Hash before calling Hash methods: when iterating Transaction::PENDING_PROVIDERS inside clear_pending_flags_from_extra, add a type guard (e.g., skip or coerce) so you only call delete("pending") and check empty? on ex[provider] if ex[provider].is_a?(Hash); otherwise skip that provider (or replace non-Hash values with an empty Hash) to avoid NoMethodError on malformed data.
🤖 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.
Nitpick comments:
In `@app/models/account/provider_import_adapter.rb`:
- Around line 988-996: The method clear_pending_flags_from_extra should
defensively ensure ex[provider] is a Hash before calling Hash methods: when
iterating Transaction::PENDING_PROVIDERS inside clear_pending_flags_from_extra,
add a type guard (e.g., skip or coerce) so you only call delete("pending") and
check empty? on ex[provider] if ex[provider].is_a?(Hash); otherwise skip that
provider (or replace non-Hash values with an empty Hash) to avoid NoMethodError
on malformed data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00d46aae-5ea5-4a20-ac9e-60da731e5d37
📒 Files selected for processing (1)
app/models/account/provider_import_adapter.rb
…rom_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.
…n in clear_pending_flags_from_extra
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/account/provider_import_adapter.rb (1)
988-1000: ⚡ Quick winMake
clear_pending_flags_from_extraprivate.In
app/models/account/provider_import_adapter.rb(around the method at ~988), the indentation is already correct; the method is only referenced within this class, so mark itprivateto reflect internal-only usage.🤖 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/account/provider_import_adapter.rb` around lines 988 - 1000, Move the clear_pending_flags_from_extra method to the class's private scope (or add a private keyword immediately above it) so it is not part of the public API; locate the clear_pending_flags_from_extra method in provider_import_adapter.rb (which uses Transaction::PENDING_PROVIDERS) and ensure it remains callable by other methods in the same class after being made private.
🤖 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.
Nitpick comments:
In `@app/models/account/provider_import_adapter.rb`:
- Around line 988-1000: Move the clear_pending_flags_from_extra method to the
class's private scope (or add a private keyword immediately above it) so it is
not part of the public API; locate the clear_pending_flags_from_extra method in
provider_import_adapter.rb (which uses Transaction::PENDING_PROVIDERS) and
ensure it remains callable by other methods in the same class after being made
private.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f22e9105-39cb-47e4-939f-463b6bc853bc
📒 Files selected for processing (1)
app/models/account/provider_import_adapter.rb
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/account/provider_import_adapter.rb`:
- Around line 991-994: The code assumes extra is a Hash when building ex and
calling ex.key?, which will raise if extra is an Array/scalar; guard the
top-level type by ensuring ex is a Hash before using key?/indexing (e.g., after
ex = (extra || {}).deep_dup, coerce to an empty Hash unless ex.is_a?(Hash) or
respond_to?(:key?) so the Transaction::PENDING_PROVIDERS loop (and checks
ex.key?(provider) / ex[provider].is_a?(Hash)) always operate on a Hash and the
helper reliably returns a Hash.
🪄 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: 4feac145-b6f6-4607-8aa5-0e3e3f0b2fa4
📒 Files selected for processing (1)
app/models/account/provider_import_adapter.rb
|
Good fix for a subtle edge case. A few observations:
Narrow bypass: The
Second clearing block (non-protected same-external-id path): The comment explains why this block is needed: when
Generated by Claude Code |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
2 participants
Problem
Some ASPSPs reuse the same
transaction_idfor both the pending and booked versions of a transaction. Becausefind_or_initialize_byfinds the existing entry as already persisted, the auto-claim path is bypassed entirely — and the pending badge gets permanently stuck, even after the bank confirms the transaction.If the user had categorised the pending entry (setting
user_modified=true), the protection check returned early before any clearing could happen, making it impossible for the flag to ever clear.Reported by @abeilprincipino in #1783.
Fix
user_modified=true): clear the pending flag viaupdate!before the protection early-return.extrais blank).incoming_pendingcomputation before the protection check so both paths can use it.Test plan
bin/rails test test/models/account/provider_import_adapter_test.rb— two new tests cover both pathsSummary by CodeRabbit
Bug Fixes
Tests