Add support for LHV_LHVBEE22 #542
Conversation
lnagel
force-pushed
the
add-lhv-lhvbee22
branch
from
cf8bbdd to
2779640
Compare
WalkthroughThe pull request introduces a new bank module for LHV (LhvLhvbee22) to the GoCardless application. Changes include adding a new bank import in the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app-gocardless/banks/lhv-lhvbee22.js (1)
16-17: Consider making the regex pattern more specific for merchant info.The current pattern
(.+)$is too permissive and might capture unexpected data. Consider restricting the merchant info format.- /^\(\.\.(\d{4})\) (\d{4}-\d{2}-\d{2}) (\d{2}:\d{2}) (.+)$/g; + /^\(\.\.(\d{4})\) (\d{4}-\d{2}-\d{2}) (\d{2}:\d{2}) ([^\\]+)(?:\\.*)?$/g;src/app-gocardless/banks/tests/lhv-lhvbee22.spec.js (1)
36-49: Add more edge cases to the test suite.Consider adding test cases for:
- Merchant names containing backslashes
- Invalid date formats
- Special characters in merchant names
it.each([ ['regular text', 'Some info'], ['partial card text', 'PIRKUMS xxx'], ['null value', null], + ['merchant with backslash', '(..1234) 2025-01-02 09:32 Merchant\\Name\\City'], + ['special characters', '(..1234) 2025-01-02 09:32 Café & Bar\\Address'], + ['invalid date', '(..1234) 2025-13-45 09:32 Merchant\\Address'], ])('normalizes non-card transaction with %s', (_, remittanceInfo) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/542.mdis excluded by!**/*.md
📒 Files selected for processing (3)
src/app-gocardless/bank-factory.js(2 hunks)src/app-gocardless/banks/lhv-lhvbee22.js(1 hunks)src/app-gocardless/banks/tests/lhv-lhvbee22.spec.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app-gocardless/banks/lhv-lhvbee22.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual-server#490
File: src/app-gocardless/banks/swedbank-habalv22.js:21-28
Timestamp: 2024-11-10T16:45:31.225Z
Learning: `Fallback.normalizeTransaction` handles error handling for date parsing.
🔇 Additional comments (2)
src/app-gocardless/banks/lhv-lhvbee22.js (1)
23-26: Handle potential edge cases with backslashes in merchant names.The current implementation assumes the first part before a backslash is the merchant name. This might not always be true.
src/app-gocardless/bank-factory.js (1)
23-23: LGTM!The integration of LhvLhvbee22 follows the established pattern for bank imports and registration.
Also applies to: 62-62
Add GoCardless formatter for LHV Estonia (
LHV_LHVBEE22).This extracts bookingDate and creditorName for card transactions where creditorName is not provided originally (null) and bookingDate delayed until the card transaction clears a few days later. The current mainline version simply falls back to setting the payeeName to remittanceInformationUnstructured and this is not good, since the values are long and unique every time.
For example:
will now yield these values
I tried to follow the pattern used by other bank integrations here and add a few test cases as well. I have been running this code on my server for a bit and seems to work correctly.
I also noticed #541 and tried to follow the pattern suggested there. If it gets merged before, I can do a rebase.