-
Notifications
You must be signed in to change notification settings - Fork 520
EnableBanking: skip CARD-* counterparty in name #685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
EnableBanking: skip CARD-* counterparty in name #685
Conversation
📝 WalkthroughWalkthroughA small logic change in EnableBankingEntry::Processor's name method now treats counterparty values matching the CARD-\d+ pattern as technical and falls back to the bank transaction code description; unit tests were added to verify both the technical-card fallback and normal human-readable counterparty behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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)
77-83: Guard the CARD- regex against non-String counterparties.*
match?will raise ifcounterpartyis present but not a String. A small guard keeps the new behavior while preventing a potential NoMethodError.💡 Proposed fix
- return counterparty if counterparty.present? && !counterparty.match?(/\ACARD-\d+\z/i) + if counterparty.present? + return counterparty unless counterparty.is_a?(String) && counterparty.match?(/\ACARD-\d+\z/i) + end
🧹 Nitpick comments (1)
test/models/enable_banking_entry/processor_test.rb (1)
4-6: PreferOpenStructoverObject.newfor test doubles.Test guidelines recommend
OpenStructfor mocks; it’s clearer and aligns with project conventions.♻️ Proposed tweak
+require "ostruct" def build_name(data) - processor = EnableBankingEntry::Processor.new(data, enable_banking_account: Object.new) + processor = EnableBankingEntry::Processor.new(data, enable_banking_account: OpenStruct.new) processor.send(:name) end
Whitespace added before 'ACME SHOP' in remittance_information. Signed-off-by: Juan José Mata <jjmata@jjmata.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| end | ||
|
|
||
| return counterparty if counterparty.present? | ||
| return counterparty if counterparty.present? && !counterparty.match?(/\ACARD-\d+\z/i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CARD-* filter missing from merchant method creates inconsistency
Medium Severity
The name method now skips CARD-* technical identifiers, but the merchant method uses identical counterparty extraction logic without this filter. When a counterparty is "CARD-1234", the transaction gets a readable name (from fallback) but still creates/links a merchant named "CARD-1234". This inconsistency partially defeats the PR's goal of improving readability since merchant lists will still contain technical identifiers.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on this PR comment @quentinreytinas?
Why:
With Wise (France), the counterparty is often a technical CARD-xxxx token; this hides the human-readable label and makes lists harder to scan.
What
Skip counterparty values matching /\ACARD-\d+\z/i when building the name.
Preserve existing priority order (counterparty → bank_transaction_code → remittance).
Add a test covering CARD-* fallback behavior.
Scope:
EnableBanking only.
Validation:
Added unit test: processor_test.rb
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.