Skip to content
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

Add GoCardless integration for COMMERZBANK_COBADEFF #537

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/app-gocardless/bank-factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Belfius from './banks/belfius_gkccbebb.js';
import Berliner_Sparkasse_beladebexxx from './banks/berliner_sparkasse_beladebexxx.js';
import BnpBeGebabebb from './banks/bnp-be-gebabebb.js';
import CBCcregbebb from './banks/cbc_cregbebb.js';
import CommerzbankCobadeff from './banks/commerzbank_cobadeff.js';
import DanskeBankDabNO22 from './banks/danskebank-dabno22.js';
import EasybankBawaatww from './banks/easybank-bawaatww.js';
import EntercardSwednokk from './banks/entercard-swednokk.js';
Expand Down Expand Up @@ -44,6 +45,7 @@ export const banks = [
Berliner_Sparkasse_beladebexxx,
BnpBeGebabebb,
CBCcregbebb,
CommerzbankCobadeff,
DanskeBankDabNO22,
EasybankBawaatww,
EntercardSwednokk,
Expand Down
87 changes: 87 additions & 0 deletions src/app-gocardless/banks/commerzbank_cobadeff.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import Fallback from './integration-bank.js';
import { amountToInteger, printIban } from '../utils.js';
import { formatPayeeName } from '../../util/payee-name.js';

/** @type {import('./bank.interface.js').IBank} */
export default {
...Fallback,

institutionIds: ['COMMERZBANK_COBADEFF'],

accessValidForDays: 179,

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',
};
},
Comment on lines +13 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. The printIban utility already handles missing IBAN gracefully by returning an empty string
  2. There's a test case in integration-bank.spec.js that explicitly tests with iban: undefined
  3. 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

Comment on lines +13 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is the same as in the fallback you could remove this and rely on the fallback

Suggested change
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',
};
},


normalizeTransaction(transaction, _booked) {
// remittanceInformationUnstructured is limited to 140 chars thus ...
// ... missing information form remittanceInformationUnstructuredArray ...
// ... so we recreate it.
transaction.remittanceInformationUnstructured =
transaction.remittanceInformationUnstructuredArray.join(' ');

// The limitations of remittanceInformationUnstructuredArray ...
// ... can result in split keywords. We fix these. Other ...
// ... splits will need to be fixed by user with rules.
const keywords = [
'End-to-End-Ref.:',
'Mandatsref:',
'Gläubiger-ID:',
'SEPA-BASISLASTSCHRIFT',
'Kartenzahlung',
'Dauerauftrag',
];
keywords.forEach((keyword) => {
transaction.remittanceInformationUnstructured =
transaction.remittanceInformationUnstructured.replace(
RegExp(keyword.split('').join('\\s*'), 'gi'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RegExp(keyword.split('').join('\\s*'), 'gi'),
// There can be spaces inbetween keywords
RegExp(keyword.split('').join('\\s*'), 'gi'),

', ' + keyword + ' ',
);
});

// 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,
};
},

/**
* For COMMERZBANK_COBADEFF we don't know what balance was
* after each transaction so we have to calculate it by getting
* current balance from the account and subtract all the transactions
*
* As a current balance we use `expected` balance type because it
* corresponds to the current running balance, whereas `interimAvailable`
* holds the remaining credit limit.
*/
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));
},
Comment on lines +78 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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));
},

Comment on lines +69 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could also just use the fallback?

};
110 changes: 110 additions & 0 deletions src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import CommerzbankCobadeff from '../commerzbank_cobadeff.js';

describe('CommerzbankCobadeff', () => {
describe('#normalizeTransaction', () => {
it('correctly formats remittanceInformationUnstructured', () => {
const transaction = {
endToEndId: '1234567890',
mandateId: '321654',
bookingDate: '2024-12-20',
valueDate: '2024-12-20',
transactionAmount: {
amount: '-12.34',
currency: 'EUR',
},
creditorName: 'SHOP NAME CITY DE',
remittanceInformationUnstructured:
'SHOP NAME//CITY/DE\n2024-12-19T15:34:31 KFN 1 AB 1234\nKartenzahlung',
remittanceInformationUnstructuredArray: [
'SHOP NAME//CITY/DE',
'2024-12-19T15:34:31 KFN 1 AB 1234',
'Kartenzahlung',
],
remittanceInformationStructured:
'SHOP NAME//CITY/DE 2024-12-19T15:34:31 KFN 1 AB 1234 Kartenzahlung',
internalTransactionId: '3815213adb654baeadfb231c853',
};
const normalizedTransaction = CommerzbankCobadeff.normalizeTransaction(
transaction,
false,
);
expect(normalizedTransaction.remittanceInformationUnstructured).toEqual(
'2024-12-19T15:34:31 KFN 1 AB 1234, Kartenzahlung',
);
});

it('correctly formats remittanceInformationUnstructured; repair split keyword', () => {
const transaction = {
endToEndId: '901234567890',
mandateId: 'ABC123DEF456',
bookingDate: '2024-10-11',
valueDate: '2024-10-11',
transactionAmount: {
amount: '-56.78',
currency: 'EUR',
},
creditorName: 'Long payee name that is eaxtly 35ch',
remittanceInformationUnstructured:
'Long payee name that is eaxtly 35ch\n901234567890/. Long description tha\nt gets cut and is very long, did I\nmention it is long\nEnd-to-En',
remittanceInformationUnstructuredArray: [
'Long payee name that is eaxtly 35ch',
'901234567890/. Long description tha',
't gets cut and is very long, did I',
'mention it is long',
'End-to-En',
'd-Ref.: 901234567890',
'Mandatsref: ABC123DEF456',
'Gläubiger-ID:',
'AB12CDE0000000000000000012',
'SEPA-BASISLASTSCHRIFT wiederholend',
],
remittanceInformationStructured:
'Long payee name that is eaxtly 35ch 901234567890/. Long description tha t gets cut and is very long, did I mention it is long End-to-En',
internalTransactionId: '812354cfdea36465asdfe',
};
const normalizedTransaction = CommerzbankCobadeff.normalizeTransaction(
transaction,
false,
);
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',
);
});

it('correctly formats remittanceInformationUnstructured; removing NOTPROVIDED', () => {
const transaction = {
endToEndId: 'NOTPROVIDED',
bookingDate: '2024-12-02',
valueDate: '2024-12-02',
transactionAmount: {
amount: '-9',
currency: 'EUR',
},
creditorName: 'CREDITOR NAME',
creditorAccount: {
iban: 'CREDITOR000IBAN',
},
remittanceInformationUnstructured:
'CREDITOR NAME\nCREDITOR00BIC\nCREDITOR000IBAN\nDESCRIPTION\nEnd-to-End-Ref.: NOTPROVIDED\nDauerauftrag',
remittanceInformationUnstructuredArray: [
'CREDITOR NAME',
'CREDITOR00BIC',
'CREDITOR000IBAN',
'DESCRIPTION',
'End-to-End-Ref.: NOTPROVIDED',
'Dauerauftrag',
],
remittanceInformationStructured:
'CREDITOR NAME CREDITOR00BIC CREDITOR000IBAN DESCRIPTION End-to-End-Ref.: NOTPROVIDED Dauerauftrag',
internalTransactionId: 'f617dc31ab77622bf13d6c95d6dd8b4a',
};
const normalizedTransaction = CommerzbankCobadeff.normalizeTransaction(
transaction,
false,
);
expect(normalizedTransaction.remittanceInformationUnstructured).toEqual(
'CREDITOR00BIC CREDITOR000IBAN DESCRIPTION, Dauerauftrag',
);
});
});
});
6 changes: 5 additions & 1 deletion src/app-gocardless/gocardless-node.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,11 @@ export type Transaction = {
/**
* Account reference, conditional
*/
creditorAccount?: string;
creditorAccount?:
| string
| {
iban?: string;
};
Comment on lines +359 to +363
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it happens, this can just be an object. It was wrongly typed before.

Suggested change
creditorAccount?:
| string
| {
iban?: string;
};
creditorAccount?: {
iban: string;
};


/**
* BICFI
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/537.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Enhancements
authors: [nsulzer]
---

Add GoCardless integration for COMMERZBANK_COBADEFF
Loading