-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Improve abn amro sync #534
Conversation
WalkthroughThe pull request introduces modifications to the ABNAMRO bank transaction normalization logic in two files: In the main implementation file, the code has been updated to directly assign the joined The test file complements these changes by adding a new test case for Google Pay transactions and updating an existing test to reflect the new comma-separated remittance information handling. These modifications aim to improve the robustness and flexibility of transaction normalization for ABNAMRO bank transactions. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (2)
src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js (1)
28-28
: Double-check spacing and punctuation in joined stringIn some transaction entries, commas appear without a trailing space (e.g., "...Name,PAS123..."). This is consistent with the final joined result here, but ensure that the consumer of these notes doesn't require consistent spacing after commas. If any consumer or downstream parser expects normalized spacing, consider adjusting the join logic or test data for readability.
src/app-gocardless/banks/abnamro_abnanl2a.js (1)
27-28
: Potential edge cases with comma-separated fieldsBy opting to join the unstructured array with commas, if any of those elements already contain embedded commas, it may result in ambiguous or confusing entries. If these entries are guaranteed to be well-formatted or always comma-free, then this approach is fine. Otherwise, consider a safer or more robust delimiter—or escaping the existing commas—to avoid conflated data fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/534.md
is excluded by!**/*.md
📒 Files selected for processing (2)
src/app-gocardless/banks/abnamro_abnanl2a.js
(1 hunks)src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js
(1 hunks)
🔇 Additional comments (3)
src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js (2)
32-32
: Empty line: no action needed
33-59
: Good addition of a Google Pay testThis new test cleanly verifies that when the
remittanceInformationUnstructuredArray
contains a Google Pay entry, the payee is extracted correctly as "Other Payee Name," and the joined string contains all fragments. The test coverage looks good for this scenario.src/app-gocardless/banks/abnamro_abnanl2a.js (1)
32-34
: Regex approach for payee extraction is concise, but handle unexpected formatsThe current pattern matches strings like “CCV*Other payee name,PAS123” by capturing the substring before
,PAS
. Ensure that this pattern won’t accidentally miss payee names if they include special characters or if “PAS” appears out of place. If there is any risk, consider adding fallback logic or additional validation for unrecognized lines.
Hiya @UnderKoen, after tinkering with the rule action templating a bit, I have no objections. I agree that it's convinent to keep all information and let people decide for themselves what to keep. Also, thanks for cleaning up the payeeName extraction 😅. |
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.
LGTM
The way the approach in the PR works is that it relies on particular payment service providers that inject their own name into the transaction description; e.g., in the provided test case, this is CCV and the regex relies on This is not a very solid approach; many payment terminals do not have the same treatment. E.g., looking at my own transactions, Albert Heijn is listed as something like |
Or were you intending to just remove the payment service provider from the vendor name? (because AFAIK that's not related to Google Pay at all). |
Never mind, I misunderstood how the parsing changed: it now only matches on line 2 of the input and that automatically removes the Apple Pay indicator as well. |
Hey @thomwiggers , So for your example in #540
It would look for the line with |
Yeah I clearly wasn't awake enough last night, all good! |
For payment made with google pay this also cleanes the payee_name.
Removes the removal of certain terms in the notes. As for me it really helpful to have BEA and GEA in the notes. (GEA is for atm withdraws)
The cleaning should be achieved with actualbudget/actual#3606 I think. @nsulzer would you be opposed by this?