Add GoCardless integration for COMMERZBANK_COBADEFF #537
Conversation
actual-github-bot
bot
changed the title
nsulzer
changed the title
WalkthroughThe pull request introduces a new bank entity, Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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: 2
🧹 Nitpick comments (3)
src/app-gocardless/banks/commerzbank_cobadeff.js (1)
25-50: Keyword fix approach
Appending a comma and space before the found keyword could lead to repeated commas in some edge cases (e.g., if the text already contains partial keywords). Keep track to ensure repeated replacements do not produce unexpected spacing.src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js (2)
5-34: Test coverage looks solid
The first test scenario verifies that unstructured remittance information is correctly formatted, combining multiline data into one string. Ensure you also verify what happens ifremittanceInformationUnstructuredArrayorcreditorNameis missing.Do you want me to open a new issue to discuss adding negative or missing-data tests?
74-108: Removing 'NOTPROVIDED'
The test verifies correct removal of placeholders like'NOTPROVIDED'. This is a good approach to keep the final data clean. Confirm that other placeholders or formatting gotchas are similarly addressed in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/537.mdis excluded by!**/*.md
📒 Files selected for processing (4)
src/app-gocardless/bank-factory.js(2 hunks)src/app-gocardless/banks/commerzbank_cobadeff.js(1 hunks)src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js(1 hunks)src/app-gocardless/gocardless-node.types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Docker image (alpine)
🔇 Additional comments (6)
src/app-gocardless/banks/commerzbank_cobadeff.js (2)
1-2: Import clarity check
These imports look good and appear necessary for the fallback integration and utility functions. No issues identified.
51-61: Careful payee removal
When removing the payee name fromremittanceInformationUnstructured, consider that partial matches of the payee name might also get stripped inadvertently. If the payee includes shorter words, they might be matched incorrectly.src/app-gocardless/bank-factory.js (2)
10-10: New module import
Good job adding the new import statement forCommerzbankCobadeff. This is consistent with the existing pattern for other bank modules.
48-48: Bank array registration
RegisteringCommerzbankCobadeffin thebanksarray ensures the factory will properly identify and use it when given the institution ID. Looks great!src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js (1)
36-72: Split keyword repair
This test effectively checks the partial keyword merging logic. Good coverage. Keep in mind that the logic inkeywords.forEachmay break with unforeseen edge cases or if the user’s data includes unusual bracketed or special characters.src/app-gocardless/gocardless-node.types.ts (1)
359-363: Optional IBAN structure
AllowingcreditorAccountto be a string or an object with a possibleibanis flexible and aligns well with the new Commerzbank integration. If you expect other fields (e.g., BIC) in the future, consider a more generalized type.
| calculateStartingBalance(sortedTransactions = [], balances = []) { | ||
| const currentBalance = balances.find( | ||
| (balance) => 'expected' === balance.balanceType, | ||
| ); | ||
|
|
||
| return sortedTransactions.reduce((total, trans) => { | ||
| return total - amountToInteger(trans.transactionAmount.amount); | ||
| }, amountToInteger(currentBalance.balanceAmount.amount)); | ||
| }, |
There was a problem hiding this comment.
Check for empty balances array
When calling calculateStartingBalance, if balances is empty or lacks an 'expected' entry, currentBalance could be undefined. Consider returning 0 or an error if no suitable balanceType is found.
calculateStartingBalance(sortedTransactions = [], balances = []) {
const currentBalance = balances.find(
(balance) => 'expected' === balance.balanceType,
);
+ if (!currentBalance) {
+ // Decide on fallback or throw error
+ return 0;
+ }
return sortedTransactions.reduce((total, trans) => {
return total - amountToInteger(trans.transactionAmount.amount);
}, amountToInteger(currentBalance.balanceAmount.amount));
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| calculateStartingBalance(sortedTransactions = [], balances = []) { | |
| const currentBalance = balances.find( | |
| (balance) => 'expected' === balance.balanceType, | |
| ); | |
| return sortedTransactions.reduce((total, trans) => { | |
| return total - amountToInteger(trans.transactionAmount.amount); | |
| }, amountToInteger(currentBalance.balanceAmount.amount)); | |
| }, | |
| calculateStartingBalance(sortedTransactions = [], balances = []) { | |
| const currentBalance = balances.find( | |
| (balance) => 'expected' === balance.balanceType, | |
| ); | |
| if (!currentBalance) { | |
| // Decide on fallback or throw error | |
| return 0; | |
| } | |
| return sortedTransactions.reduce((total, trans) => { | |
| return total - amountToInteger(trans.transactionAmount.amount); | |
| }, amountToInteger(currentBalance.balanceAmount.amount)); | |
| }, |
| normalizeAccount(account) { | ||
| return { | ||
| account_id: account.id, | ||
| institution: account.institution, | ||
| mask: account.iban.slice(-4), | ||
| iban: account.iban, | ||
| name: [account.name, printIban(account)].join(' '), | ||
| official_name: account.product, | ||
| type: 'checking', | ||
| }; | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the evidence gathered, I can now provide the final response:
Add safeguard for missing IBAN property in normalizeAccount
The codebase shows that:
- The
printIbanutility already handles missing IBAN gracefully by returning an empty string - There's a test case in
integration-bank.spec.jsthat explicitly tests withiban: undefined - Multiple bank integrations use the same IBAN-dependent pattern
The current implementation needs safeguarding in two places:
mask: account.iban?.slice(-4) ?? ''iban: account.iban ?? ''
🔗 Analysis chain
Ensure robustness for missing account.iban
The normalizeAccount function assumes an iban property is present on the account. If the bank feed or fallback code potentially returns an account object without an iban, this could cause errors. Consider safe-guarding it, for example:
+// Example safeguard, if `account.iban` might be undefined
+iban: account.iban || '',Let me verify if there are any cases where account objects might be missing the iban property by analyzing the codebase.
Let me search for how the account objects are constructed and any test cases that might reveal the behavior.
Let me check the specific test file for Commerzbank and the utility function that handles IBAN printing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for references to account.iban to understand its usage
rg "account\.iban" -B 2 -A 2
# Search for places where account objects are created/returned that might feed into normalizeAccount
ast-grep --pattern 'account = { $$$, iban: $_, $$$}'
# Search for any test files related to this bank integration
fd -g "*commerzbank*test*"
Length of output: 9005
Script:
#!/bin/bash
# Search for test files and fixtures related to bank accounts
fd -e test.js -e spec.js
# Search for account object construction patterns
ast-grep --pattern 'const account = {$$$}'
# Look for any error handling related to IBAN
rg -i "iban.*undefined|missing.*iban" -B 2 -A 2
Length of output: 1977
Script:
#!/bin/bash
# Check the Commerzbank test file
cat src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js
# Check the printIban utility implementation
cat src/app-gocardless/utils.js
Length of output: 5599
Seems fine 👍 |
nsulzer
force-pushed
the
gocardless-commerzbank_cobadeff
branch
from
6807ce6 to
8b4f68c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/app-gocardless/banks/commerzbank_cobadeff.js (1)
19-38: Optimize keyword handling for better performance.Consider these improvements:
- Move keywords array outside function to avoid recreation on each call
- Pre-compile RegExp patterns for better performance
+const TRANSACTION_KEYWORDS = [ + 'End-to-End-Ref.:', + 'Mandatsref:', + 'Gläubiger-ID:', + 'SEPA-BASISLASTSCHRIFT', + 'Kartenzahlung', + 'Dauerauftrag', +]; + +// Pre-compile patterns for better performance +const KEYWORD_PATTERNS = TRANSACTION_KEYWORDS.map( + keyword => new RegExp(keyword.split('').join('\\s*'), 'gi') +); + export default { // ... other code ... normalizeTransaction(transaction, _booked) { // ... other code ... - const keywords = [ - 'End-to-End-Ref.:', - 'Mandatsref:', - 'Gläubiger-ID:', - 'SEPA-BASISLASTSCHRIFT', - 'Kartenzahlung', - 'Dauerauftrag', - ]; - keywords.forEach((keyword) => { + TRANSACTION_KEYWORDS.forEach((keyword, index) => { transaction.remittanceInformationUnstructured = transaction.remittanceInformationUnstructured.replace( - RegExp(keyword.split('').join('\\s*'), 'gi'), + KEYWORD_PATTERNS[index], ', ' + keyword + ' ', ); });src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js (3)
1-34: Enhance test coverage with edge cases.While the basic test case is good, consider adding tests for:
- Empty remittanceInformationUnstructuredArray
- Undefined transaction fields
- Invalid transaction object
it('handles empty remittanceInformationUnstructuredArray', () => { const transaction = { // ... other fields ... remittanceInformationUnstructuredArray: [], }; const result = CommerzbankCobadeff.normalizeTransaction(transaction, false); expect(result.remittanceInformationUnstructured).toEqual(''); }); it('handles undefined transaction fields gracefully', () => { const transaction = {}; const result = CommerzbankCobadeff.normalizeTransaction(transaction, false); expect(result.remittanceInformationUnstructured).toBeDefined(); }); it('throws error for invalid transaction', () => { expect(() => { CommerzbankCobadeff.normalizeTransaction(null, false); }).toThrow('Invalid transaction object'); });
36-72: Improve test assertion specificity.The current test verifies the entire string, making it hard to identify specific issues. Consider breaking down the assertion into multiple specific checks:
- expect(normalizedTransaction.remittanceInformationUnstructured).toEqual( - '901234567890/. Long description tha t gets cut and is very long, did I mention it is long, End-to-End-Ref.: 901234567890, Mandatsref: ABC123DEF456, Gläubiger-ID: AB12CDE0000000000000000012, SEPA-BASISLASTSCHRIFT wiederholend', - ); + const result = normalizedTransaction.remittanceInformationUnstructured; + expect(result).toContain('901234567890/.'); + expect(result).toContain('End-to-End-Ref.: 901234567890'); + expect(result).toContain('Mandatsref: ABC123DEF456'); + expect(result).toContain('Gläubiger-ID: AB12CDE0000000000000000012'); + expect(result).toContain('SEPA-BASISLASTSCHRIFT wiederholend');
74-110: Add assertions for transaction metadata.The test could verify more than just the remittance information:
expect(normalizedTransaction.remittanceInformationUnstructured).toEqual( 'CREDITOR00BIC CREDITOR000IBAN DESCRIPTION, Dauerauftrag', ); + // Verify other normalized transaction fields + expect(normalizedTransaction.payeeName).toBe('CREDITOR NAME'); + expect(normalizedTransaction.date).toBe('2024-12-02'); + expect(normalizedTransaction.creditorAccount).toEqual({ + iban: 'CREDITOR000IBAN', + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/537.mdis excluded by!**/*.md
📒 Files selected for processing (4)
src/app-gocardless/bank-factory.js(2 hunks)src/app-gocardless/banks/commerzbank_cobadeff.js(1 hunks)src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js(1 hunks)src/app-gocardless/gocardless-node.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app-gocardless/gocardless-node.types.ts
- src/app-gocardless/bank-factory.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (1)
src/app-gocardless/banks/commerzbank_cobadeff.js (1)
1-10: LGTM! Well-structured bank integration setup.Good use of the Fallback pattern for code reuse and proper interface implementation.
| normalizeTransaction(transaction, _booked) { | ||
| // remittanceInformationUnstructured is limited to 140 chars thus ... | ||
| // ... missing information form remittanceInformationUnstructuredArray ... | ||
| // ... so we recreate it. | ||
| transaction.remittanceInformationUnstructured = | ||
| transaction.remittanceInformationUnstructuredArray.join(' '); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for remittanceInformationUnstructuredArray.
The array join operation could fail if the array is undefined. Consider adding a defensive check:
- transaction.remittanceInformationUnstructured =
- transaction.remittanceInformationUnstructuredArray.join(' ');
+ transaction.remittanceInformationUnstructured =
+ transaction.remittanceInformationUnstructuredArray?.join(' ') ?? '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| normalizeTransaction(transaction, _booked) { | |
| // remittanceInformationUnstructured is limited to 140 chars thus ... | |
| // ... missing information form remittanceInformationUnstructuredArray ... | |
| // ... so we recreate it. | |
| transaction.remittanceInformationUnstructured = | |
| transaction.remittanceInformationUnstructuredArray.join(' '); | |
| normalizeTransaction(transaction, _booked) { | |
| // remittanceInformationUnstructured is limited to 140 chars thus ... | |
| // ... missing information form remittanceInformationUnstructuredArray ... | |
| // ... so we recreate it. | |
| transaction.remittanceInformationUnstructured = | |
| transaction.remittanceInformationUnstructuredArray?.join(' ') ?? ''; | |
| // Clean up remittanceInformation, deduplicate payee (removing slashes ... | ||
| // ... that are added to the remittanceInformation field), and ... | ||
| // ... remove clutter like "End-to-End-Ref.: NOTPROVIDED" | ||
| const payee = transaction.creditorName || transaction.debtorName || ''; | ||
| transaction.remittanceInformationUnstructured = | ||
| transaction.remittanceInformationUnstructured | ||
| .replace(/\s*(,)?\s+/g, '$1 ') | ||
| .replace(RegExp(payee.split(' ').join('(/*| )'), 'gi'), ' ') | ||
| .replace(', End-to-End-Ref.: NOTPROVIDED', '') | ||
| .trim(); | ||
|
|
||
| return { | ||
| ...transaction, | ||
| payeeName: formatPayeeName(transaction), | ||
| date: transaction.bookingDate, | ||
| }; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of remittance information cleanup.
The current implementation has several areas for improvement:
- Complex regex patterns could be hard to maintain
- No validation of transaction object structure
+ // Validate transaction object structure
+ if (!transaction || typeof transaction !== 'object') {
+ throw new Error('Invalid transaction object');
+ }
+
const payee = transaction.creditorName || transaction.debtorName || '';
+
+ // Break down complex regex into more maintainable steps
transaction.remittanceInformationUnstructured =
transaction.remittanceInformationUnstructured
- .replace(/\s*(,)?\s+/g, '$1 ')
- .replace(RegExp(payee.split(' ').join('(/*| )'), 'gi'), ' ')
+ // Normalize spaces around commas
+ .replace(/\s*,\s*/g, ', ')
+ // Normalize multiple spaces
+ .replace(/\s+/g, ' ')
+ // Remove payee name variations
+ .replace(new RegExp(
+ payee
+ .split(' ')
+ .map(part => `${part}(?:/+|\\s+)?`)
+ .join(''),
+ 'gi'
+ ), ' ')
.replace(', End-to-End-Ref.: NOTPROVIDED', '')
.trim();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Clean up remittanceInformation, deduplicate payee (removing slashes ... | |
| // ... that are added to the remittanceInformation field), and ... | |
| // ... remove clutter like "End-to-End-Ref.: NOTPROVIDED" | |
| const payee = transaction.creditorName || transaction.debtorName || ''; | |
| transaction.remittanceInformationUnstructured = | |
| transaction.remittanceInformationUnstructured | |
| .replace(/\s*(,)?\s+/g, '$1 ') | |
| .replace(RegExp(payee.split(' ').join('(/*| )'), 'gi'), ' ') | |
| .replace(', End-to-End-Ref.: NOTPROVIDED', '') | |
| .trim(); | |
| return { | |
| ...transaction, | |
| payeeName: formatPayeeName(transaction), | |
| date: transaction.bookingDate, | |
| }; | |
| }, | |
| }; | |
| // Clean up remittanceInformation, deduplicate payee (removing slashes ... | |
| // ... that are added to the remittanceInformation field), and ... | |
| // ... remove clutter like "End-to-End-Ref.: NOTPROVIDED" | |
| // Validate transaction object structure | |
| if (!transaction || typeof transaction !== 'object') { | |
| throw new Error('Invalid transaction object'); | |
| } | |
| const payee = transaction.creditorName || transaction.debtorName || ''; | |
| // Break down complex regex into more maintainable steps | |
| transaction.remittanceInformationUnstructured = | |
| transaction.remittanceInformationUnstructured | |
| // Normalize spaces around commas | |
| .replace(/\s*,\s*/g, ', ') | |
| // Normalize multiple spaces | |
| .replace(/\s+/g, ' ') | |
| // Remove payee name variations | |
| .replace(new RegExp( | |
| payee | |
| .split(' ') | |
| .map(part => `${part}(?:/+|\\s+)?`) | |
| .join(''), | |
| 'gi' | |
| ), ' ') | |
| .replace(', End-to-End-Ref.: NOTPROVIDED', '') | |
| .trim(); | |
| return { | |
| ...transaction, | |
| payeeName: formatPayeeName(transaction), | |
| date: transaction.bookingDate, | |
| }; | |
| }, | |
| }; |
nsulzer
force-pushed
the
gocardless-commerzbank_cobadeff
branch
from
8b4f68c to
f66724d
Compare
Add GoCardless integration for COMMERZBANK_COBADEFF, with a bunch of cleanup for remittance information that will become the notes. The source data for this bank is a bit messy to format unfortunately.
Question to the reviewer: Is the implementation of the optional
ibanproperty forcreditorAccountcorrect/clean? Not a dev, sorry.