Move notes normalisation to the server and expose new bank sync fields #558
Conversation
actual-github-bot
bot
changed the title
matt-fidd
force-pushed
the
gocardless-handlers
branch
2 times, most recently
from
dc21c38 to
6009ce9
Compare
matt-fidd
changed the title
WalkthroughThis pull request introduces widespread changes to the transaction normalization process across multiple bank integration files. The primary modifications involve updating the Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 5
🔭 Outside diff range comments (3)
src/app-gocardless/banks/tests/abanca_caglesmm.spec.js (1)
Line range hint
6-19: Update test description to match the new assertion.The test description states it checks "creditorName and debtorName" but the assertion now verifies "payeeName". This mismatch makes the test description misleading.
Apply this diff to align the test description with its assertion:
- it('returns the creditorName and debtorName as remittanceInformationStructured', () => { + it('normalizes remittanceInformationStructured as payeeName', () => {src/app-gocardless/banks/hype_hyeeit22.js (1)
Add error handling for malformed Unicode sequences in Hype bank P2P transactions
The code assumes all Unicode sequences are valid 4-byte patterns without any validation or error handling. This could lead to crashes when processing malformed input. The suggested error handling should be implemented to improve robustness.
🔗 Analysis chain
Line range hint
44-66: Add error handling for malformed Unicode sequences.The Unicode handling code assumes 4-byte sequences and lacks error handling for malformed input.
Consider refactoring to:
let codepoints = []; while (idx !== -1) { + try { codepoints.push(parseInt(str.slice(idx + 2, idx + 6), 16)); + } catch (e) { + console.warn(`Malformed Unicode sequence at index ${idx}`); + break; + } let next_idx = str.indexOf('\\U', idx + 6); if (next_idx == idx + 6) { idx = next_idx; continue; } str = str.slice(0, start_idx) + + (codepoints.every(c => c <= 0x10FFFF) ? + String.fromCodePoint(...codepoints) : + '[Invalid Unicode]') + str.slice(idx + 6);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Unicode handling patterns in the codebase rg -A 5 '\\U[0-9a-fA-F]{4}'Length of output: 29
Script:
#!/bin/bash # Search for String.fromCodePoint usage and surrounding context rg -A 5 "String\.fromCodePoint" # Search for P2P transaction handling rg -A 5 "proprietaryBankTransactionCode.*['\"]p2p['\"]" # Search for various Unicode escape patterns rg -A 5 '\\[uU]' # Search for remittanceInformationUnstructured rg -A 5 "remittanceInformationUnstructured"Length of output: 87108
src/app-gocardless/banks/ing_ingbrobu.js (1)
Line range hint
28-42: Avoid modifying the original transaction object.The code directly modifies
transaction.transactionIdwhich breaks immutability.- transaction.transactionId = transaction.internalTransactionId; + editedTrans.transactionId = transaction.internalTransactionId;
🧹 Nitpick comments (37)
src/app-gocardless/banks/cbc_cregbebb.js (1)
17-29: Validate Payee Name Logic for Credit/Debit CasesThe conditional logic for setting
payeeNamebased on transaction amounts is sensible. Consider ensuring all relevant fields (debtorName,creditorName, or structured info) are consistently populated so that fallback logic is applied uniformly throughout the app.src/app-gocardless/banks/spk_karlsruhe_karsde66.js (1)
11-12: Consider using a deep clone if nested fields are modified later.
Creating a shallow copy for immutability is a good practice. However, iftransactioncontains nested objects that might be mutated later, a shallow copy won’t fully protect the original.src/app-gocardless/banks/direkt_heladef1822.js (1)
10-11: Check consistency of shallow copy.
Similar to other files, this shallow copy is fine if you only modify top-level properties. For deeper structures, you may need a deep clone.src/app-gocardless/banks/isybank_itbbitmm.js (1)
12-12: Revisit shallow copy usage for future-proofing.
Creating a separateeditedTransobject is good for immutability, but iftransactionacquires complex structures in the future, you may need a deeper clone strategy.src/app-gocardless/banks/belfius_gkccbebb.js (1)
12-15: Consider using shallow copy pattern for consistency.The implementation directly mutates the transaction object, while other bank integrations create a shallow copy using
editedTrans. Consider following the same pattern for consistency:normalizeTransaction(transaction, booked) { - transaction.transactionId = transaction.internalTransactionId; + const editedTrans = { ...transaction }; + editedTrans.transactionId = transaction.internalTransactionId; - return Fallback.normalizeTransaction(transaction, booked); + return Fallback.normalizeTransaction(transaction, booked, editedTrans); }src/app-gocardless/banks/abanca_caglesmm.js (1)
14-20: Consider adding null check for remittanceInformationStructured.The implementation might throw if
remittanceInformationStructuredis undefined.normalizeTransaction(transaction, booked) { const editedTrans = { ...transaction }; - editedTrans.creditorName = transaction.remittanceInformationStructured; - editedTrans.debtorName = transaction.remittanceInformationStructured; + if (transaction.remittanceInformationStructured) { + editedTrans.creditorName = transaction.remittanceInformationStructured; + editedTrans.debtorName = transaction.remittanceInformationStructured; + } return Fallback.normalizeTransaction(transaction, booked, editedTrans); },src/app-gocardless/banks/sparnord_spnodk22.js (1)
19-20: Consider adding null check for additionalInformation.The implementation might assign undefined to
remittanceInformationUnstructuredifadditionalInformationis not present.- editedTrans.remittanceInformationUnstructured = - transaction.additionalInformation; + if (transaction.additionalInformation) { + editedTrans.remittanceInformationUnstructured = + transaction.additionalInformation; + }src/app-gocardless/banks/bank_of_ireland_b365_bofiie2d.js (1)
10-16: LGTM! Consider combining regex operations for better performance.The implementation correctly follows the new transaction normalization pattern. The fixupPayee function could potentially be optimized by combining some of the regex operations, but this is a minor optimization.
Optional optimization for fixupPayee:
function fixupPayee(/** @type {string} */ payee) { let fixedPayee = payee; - // remove all duplicate whitespace - fixedPayee = fixedPayee.replace(/\s+/g, ' ').trim(); - - // remove date prefix - fixedPayee = fixedPayee.replace(/^(POS)?(C)?[0-9]{1,2}\w{3}/, '').trim(); - - // remove direct debit postfix - fixedPayee = fixedPayee.replace(/sepa dd$/i, '').trim(); - - // remove bank transfer prefix - fixedPayee = fixedPayee.replace(/^365 online/i, '').trim(); - - // remove curve card prefix - fixedPayee = fixedPayee.replace(/^CRV\*/, '').trim(); + // combine all cleanups into a single pass + fixedPayee = fixedPayee + .replace(/\s+/g, ' ') + .replace(/^(POS)?(C)?[0-9]{1,2}\w{3}/, '') + .replace(/sepa dd$/i, '') + .replace(/^365 online/i, '') + .replace(/^CRV\*/, '') + .trim(); return fixedPayee; }src/app-gocardless/banks/kbc_kredbebb.js (1)
17-21: Consider a more descriptive fallback value for payeeName.Instead of using 'undefined' as the fallback value, consider using a more descriptive value like 'Unknown Debtor' that better indicates the nature of the missing data.
- 'undefined'; + 'Unknown Debtor';src/app-gocardless/banks/bancsabadell_bsabesbbb.js (1)
Line range hint
19-21: Consider caching the joined payee name.The payeeName is computed but used multiple times. Consider storing it in a variable to avoid repeated array operations.
- const payeeName = transaction.remittanceInformationUnstructuredArray - .join(' ') - .trim(); + const payeeName = transaction.remittanceInformationUnstructuredArray?.join(' ')?.trim() || '';src/app-gocardless/banks/lhv-lhvbee22.js (1)
Line range hint
17-20: Consider using a more robust regex pattern.The current regex pattern could be made more secure and maintainable:
- Use RegExp constructor with proper escaping
- Consider adding input validation
- const cardTxRegex = - /^\(\.\.(\d{4})\) (\d{4}-\d{2}-\d{2}) (\d{2}:\d{2}) (.+)$/g; + const cardTxRegex = new RegExp( + '^\\(\\.\\.(\\d{4})\\) (\\d{4}-\\d{2}-\\d{2}) (\\d{2}:\\d{2}) (.+)$', + 'g' + );src/app-gocardless/banks/danskebank_dabno22.js (1)
21-25: Consider adding null check for remittanceInformationUnstructured.The string replacement could fail if remittanceInformationUnstructured is undefined.
- editedTrans.remittanceInformationUnstructured = - transaction.remittanceInformationUnstructured.replace( - '\nEndToEndID: NOTPROVIDED', - '', - ); + editedTrans.remittanceInformationUnstructured = + transaction.remittanceInformationUnstructured?.replace( + '\nEndToEndID: NOTPROVIDED', + '', + ) || '';src/app-gocardless/banks/virgin_nrnbgb22.js (1)
22-24: Consider simplifying redundant assignments.In all code paths,
creditorNameanddebtorNameare set to the same value. This could be simplified to a single assignment.- editedTrans.creditorName = parts[1]; - editedTrans.debtorName = parts[1]; + editedTrans.payeeName = parts[1];Also applies to: 28-29, 33-34
src/app-gocardless/banks/nationwide_naiagb21.js (1)
Line range hint
35-40: Consider moving transaction ID validation to editedTrans.The implementation directly mutates
transaction.transactionId. Consider usingeditedTransfor consistency with the immutable pattern:- transaction.transactionId = null; + editedTrans.transactionId = null;src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
Line range hint
16-21: Consider enhancing debug logging for unbooked transactions.The debug log could include more context about why the transaction was skipped:
- 'Skipping unbooked transaction:', - transaction.transactionId, + 'Skipping unbooked transaction (missing payee/notes information):', + { id: transaction.transactionId, date: transaction.bookingDate },src/app-gocardless/banks/easybank_bawaatww.js (2)
22-29: Consider using a deep copy for nested transaction objects.The shallow copy using object spread (
{ ...transaction }) might not be sufficient if the transaction object contains nested objects that need to be modified.Consider using a deep copy to ensure all nested objects are properly copied:
-const editedTrans = { ...transaction }; +const editedTrans = JSON.parse(JSON.stringify(transaction));
Line range hint
36-45: Improve regex pattern readability and maintainability.The current regex pattern for extracting payee name could be made more maintainable by:
- Using a named capture group for the payee name
- Making the pattern more explicit with comments
Consider this improvement:
- const regex = /\d{2}\.\d{2}\. \d{2}:\d{2}(.*)\\\\/; + // Format: DD.MM. HH:mm PAYEE_NAME\\LOCATION + const regex = /\d{2}\.\d{2}\. \d{2}:\d{2}(?<payeeName>.*?)\\\\/; const matches = structured.match(regex); - if (matches && matches.length > 1 && matches[1]) { - return title(matches[1]); + if (matches?.groups?.payeeName) { + return title(matches.groups.payeeName); }src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (1)
Line range hint
9-42: Enhance regex pattern maintainability and transaction copy.Several improvements could be made to the code:
- The regex patterns could be more maintainable using constants
- The shallow copy might not be sufficient for nested objects
- The amount parsing could be made more robust
Consider these improvements:
+const TRANSACTION_KEYWORDS = { + BANK_TRANSFER: ['VIR INST', 'VIR'], + CARD_PAYMENT: ['CARTE \\d{2}\\/\\d{2}', 'ANN CARTE'], + DIRECT_DEBIT: ['PRLV'] +}; normalizeTransaction(transaction, booked) { - const editedTrans = { ...transaction }; + const editedTrans = JSON.parse(JSON.stringify(transaction)); - const keywordsToRemove = [ - 'VIR INST', - 'VIR', - 'PRLV', - 'ANN CARTE', - 'CARTE \\d{2}\\/\\d{2}', - ]; + const keywordsToRemove = Object.values(TRANSACTION_KEYWORDS).flat(); const details = transaction.remittanceInformationUnstructuredArray.join(' '); - const amount = transaction.transactionAmount.amount; + const amount = Number(transaction.transactionAmount.amount); const regex = new RegExp(keywordsToRemove.join('|'), 'g'); const payeeName = details.replace(regex, '').trim(); - const isCreditorPayee = parseFloat(amount) < 0; + const isCreditorPayee = amount < 0;src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js (1)
Line range hint
6-51: Enhance test coverage with edge cases and error scenarios.While the current tests cover the basic scenarios, consider adding tests for:
- Empty or malformed transaction data
- Special characters in payee names
- Edge cases for different transaction types
Add these test cases:
it('handles empty transaction data gracefully', () => { const transaction = { bookingDate: '2024-01-01T00:00:00Z', remittanceInformationUnstructured: '', transactionAmount: mockTransactionAmount, }; const normalizedTransaction = Virgin.normalizeTransaction(transaction, true); expect(normalizedTransaction.payeeName).toBeFalsy(); }); it('handles special characters in payee names', () => { const transaction = { bookingDate: '2024-01-01T00:00:00Z', remittanceInformationUnstructured: 'FPS, Joe & Mary's Shop, Food', transactionAmount: mockTransactionAmount, }; const normalizedTransaction = Virgin.normalizeTransaction(transaction, true); expect(normalizedTransaction.payeeName).toEqual('Joe & Mary's Shop'); });src/app-gocardless/banks/nbg_ethngraaxxx.js (1)
27-28: Improve string manipulation readability.The magic number
6in the substring operation makes the code less maintainable. Consider using a constant to improve readability.Apply this change:
+const PURCHASE_PREFIX = 'ΑΓΟΡΑ '; normalizeTransaction(transaction, booked) { const editedTrans = JSON.parse(JSON.stringify(transaction)); if ( !transaction.transactionId && - transaction.remittanceInformationUnstructured.startsWith('ΑΓΟΡΑ ') + transaction.remittanceInformationUnstructured.startsWith(PURCHASE_PREFIX) ) { editedTrans.transactionAmount = { amount: '-' + transaction.transactionAmount.amount, currency: transaction.transactionAmount.currency, }; editedTrans.remittanceInformationUnstructured = - transaction.remittanceInformationUnstructured.substring(6); + transaction.remittanceInformationUnstructured.substring(PURCHASE_PREFIX.length); }src/app-gocardless/banks/seb_kort_bank_ab.js (1)
18-20: Consider usingeditedTransfor all modifications.While the changes follow the new pattern of using
editedTrans, the transaction amount modification is still being done on the originaltransactionobject. This is inconsistent with the pattern of not modifying the original transaction.normalizeTransaction(transaction, booked) { const editedTrans = { ...transaction }; // Creditor name is stored in additionInformation for SEB editedTrans.creditorName = transaction.additionalInformation; - transaction.transactionAmount = { + editedTrans.transactionAmount = { // Flip transaction amount sign amount: (-parseFloat(transaction.transactionAmount.amount)).toString(), currency: transaction.transactionAmount.currency, }; - return Fallback.normalizeTransaction(transaction, booked, editedTrans); + return Fallback.normalizeTransaction(transaction, booked, editedTrans);Also applies to: 29-29
src/app-gocardless/banks/commerzbank_cobadeff.js (1)
15-17: Consider optimizing string manipulations.The current implementation performs multiple string operations that could be optimized:
- Multiple string joins and replacements could be combined
- Regular expressions could be pre-compiled for better performance
- The payee name replacement could be simplified
+ const KEYWORDS_REGEX = new RegExp( + keywords.map(keyword => `(${keyword.split('').join('\\s*')})`).join('|'), + 'gi' + ); - keywords.forEach((keyword) => { - editedTrans.remittanceInformationUnstructured = - editedTrans.remittanceInformationUnstructured.replace( - RegExp(keyword.split('').join('\\s*'), 'gi'), - ', ' + keyword + ' ', - ); - }); + editedTrans.remittanceInformationUnstructured = + editedTrans.remittanceInformationUnstructured.replace( + KEYWORDS_REGEX, + (match) => `, ${match} ` + );Also applies to: 30-34, 42-47
src/app-gocardless/banks/abnamro_abnanl2a.js (1)
15-16: Consider using optional chaining for safer property access.The code assumes these properties exist on the transaction object. While they might be guaranteed by the interface, using optional chaining would make the code more resilient to potential API changes.
- editedTrans.remittanceInformationUnstructured = - transaction.remittanceInformationUnstructuredArray.join(', '); + editedTrans.remittanceInformationUnstructured = + transaction.remittanceInformationUnstructuredArray?.join(', '); - editedTrans.debtorName = transaction.debtorName || payeeName; - editedTrans.creditorName = transaction.creditorName || payeeName; + editedTrans.debtorName = transaction.debtorName ?? payeeName; + editedTrans.creditorName = transaction.creditorName ?? payeeName;Also applies to: 23-24
src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js (2)
Line range hint
14-27: Simplify the remittanceInformation logic using nullish coalescing.The current implementation can be made more concise and maintainable.
- if (transaction.remittanceInformationUnstructured) { - remittanceInformationUnstructured = - transaction.remittanceInformationUnstructured; - } else if (transaction.remittanceInformationStructured) { - remittanceInformationUnstructured = - transaction.remittanceInformationStructured; - } else if (transaction.remittanceInformationStructuredArray?.length > 0) { - remittanceInformationUnstructured = - transaction.remittanceInformationStructuredArray?.join(' '); - } + remittanceInformationUnstructured = + transaction.remittanceInformationUnstructured ?? + transaction.remittanceInformationStructured ?? + (transaction.remittanceInformationStructuredArray?.length > 0 + ? transaction.remittanceInformationStructuredArray.join(' ') + : undefined); - if (transaction.additionalInformation) - remittanceInformationUnstructured += - ' ' + transaction.additionalInformation; + if (transaction.additionalInformation) { + remittanceInformationUnstructured = [ + remittanceInformationUnstructured, + transaction.additionalInformation + ].filter(Boolean).join(' '); + }
Line range hint
29-36: Consider using nullish coalescing for creditorName assignment.The current implementation can be simplified.
- const usefulCreditorName = - transaction.ultimateCreditor || - transaction.creditorName || - transaction.debtorName; - - editedTrans.creditorName = usefulCreditorName; + editedTrans.creditorName = + transaction.ultimateCreditor ?? + transaction.creditorName ?? + transaction.debtorName;src/app-gocardless/banks/norwegian_xx_norwnok1.js (1)
39-40: Consider consolidating date assignment logic.The date assignment logic is repeated in multiple places. Consider extracting it into a helper function.
+ function assignDate(date) { + editedTrans.date = date; + return Fallback.normalizeTransaction(transaction, booked, editedTrans); + } + if (transaction.valueDate !== undefined) { - editedTrans.date = transaction.valueDate; - return Fallback.normalizeTransaction(transaction, booked, editedTrans); + return assignDate(transaction.valueDate); } if (transaction.remittanceInformationStructured) { const remittanceInfoRegex = / (\d{4}-\d{2}-\d{2}) /; const matches = transaction.remittanceInformationStructured.match(remittanceInfoRegex); if (matches) { - editedTrans.date = matches[1]; - return Fallback.normalizeTransaction(transaction, booked, editedTrans); + return assignDate(matches[1]); } }Also applies to: 48-49
src/app-gocardless/banks/hype_hyeeit22.js (1)
10-10: Consider using deep copy for nested transaction objects.The shallow copy might not be sufficient if the transaction object contains nested objects that need to be modified.
- const editedTrans = { ...transaction }; + const editedTrans = JSON.parse(JSON.stringify(transaction));src/app-gocardless/banks/ing_ingbrobu.js (1)
52-55: Consider extracting card number masking logic to a utility function.The card number masking logic could be reused across different bank integrations.
+ function maskCardNumber(str) { + return str.replace(/x{4}/g, 'Xxxx '); + } + - editedTrans.creditorName = - transaction.remittanceInformationUnstructured.replace( - /x{4}/g, - 'Xxxx ', - ); + editedTrans.creditorName = maskCardNumber( + transaction.remittanceInformationUnstructured + );src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js (1)
48-57: Add test cases for edge cases.The tests should cover scenarios with empty or malformed transaction data.
Add these test cases:
it('handles empty transaction data gracefully', () => { const emptyTransaction = { transactionId: '2024102900000000-3', transactionAmount: { amount: '0', currency: 'EUR' } }; expect( SskDusseldorfDussdeddxxx.normalizeTransaction( emptyTransaction, true ).notes ).toEqual(''); }); it('handles malformed transaction data', () => { const malformedTransaction = { transactionId: '2024102900000000-4', transactionAmount: { amount: 'invalid', currency: 'EUR' }, remittanceInformationUnstructured: null }; expect( SskDusseldorfDussdeddxxx.normalizeTransaction( malformedTransaction, true ) ).not.toBeNull(); });src/app-gocardless/banks/bnp_be_gebabebb.js (2)
Line range hint
33-35: Consider precompiling the regex pattern.Moving the regex pattern outside the function would improve performance by avoiding recompilation on each call.
+const ADDITIONAL_INFO_REGEX = /(, )?([^:]+): ((\[.*?\])|([^,]*))/g; + if (transaction.additionalInformation) { let additionalInformationObject = {}; - const additionalInfoRegex = /(, )?([^:]+): ((\[.*?\])|([^,]*))/g; let matches = - transaction.additionalInformation.matchAll(additionalInfoRegex); + transaction.additionalInformation.matchAll(ADDITIONAL_INFO_REGEX);
Line range hint
52-57: Simplify array filtering logic.The current array filtering could be simplified using array methods.
- editedTrans.remittanceInformationUnstructuredArray = [ - transaction.remittanceInformationUnstructuredArray ?? '', - additionalInformationObject?.atmPosName ?? '', - additionalInformationObject?.narrative ?? '', - ].filter(Boolean); + editedTrans.remittanceInformationUnstructuredArray = [ + transaction.remittanceInformationUnstructuredArray, + additionalInformationObject?.atmPosName, + additionalInformationObject?.narrative + ].filter(value => value?.trim());src/app-gocardless/banks/integration-bank.js (2)
63-67: Consider handling empty strings in notes concatenation.The notes concatenation logic looks good, but consider handling empty strings to avoid potential
" "results when some array elements are empty.- trans.remittanceInformationUnstructuredArray?.join(' '); + trans.remittanceInformationUnstructuredArray?.filter(Boolean).join(' ');
68-71: Consider handling null arrays in string conversion.Good addition of string conversion for arrays, but consider handling null arrays to avoid potential undefined behavior.
- transaction.remittanceInformationUnstructuredArray?.join(','); + transaction.remittanceInformationUnstructuredArray?.filter(Boolean).join(',') ?? ''; - transaction.remittanceInformationStructuredArray?.join(','); + transaction.remittanceInformationStructuredArray?.filter(Boolean).join(',') ?? '';src/app-gocardless/services/tests/fixtures.js (1)
38-39: Consider diversifying test dates.While using a consistent date ('2000-01-01') makes tests predictable, consider adding test cases with different dates to ensure proper handling of various date scenarios (e.g., different months, leap years, etc.).
Also applies to: 48-49, 58-58
src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js (1)
161-202: Consider adding edge case tests for payee name extraction.While the current tests cover various payee name formats, consider adding tests for edge cases such as:
- Empty remittance information
- Special characters in payee names
- Very long payee names
src/app-simplefin/app-simplefin.js (1)
221-233: LGTM! Consider adding date validation.The changes enhance the transaction object with more detailed date information and align with the standardized property naming. The conditional checks for timestamps before setting the date fields are a good practice.
Consider adding date validation to ensure the timestamps are valid before converting them to dates:
if (trans.transacted_at) { + const transactedTimestamp = trans.transacted_at * 1000; + if (!isNaN(transactedTimestamp) && isFinite(transactedTimestamp)) { newTrans.transactedDate = getDate(new Date(trans.transacted_at * 1000)); + } } if (trans.posted) { + const postedTimestamp = trans.posted * 1000; + if (!isNaN(postedTimestamp) && isFinite(postedTimestamp)) { newTrans.postedDate = getDate(new Date(trans.posted * 1000)); + } }src/app-gocardless/banks/ing_ingddeff.js (1)
11-22: LGTM! Consider using a deep clone for better immutability.The changes improve the code by:
- Using better parameter naming.
- Not modifying the original transaction object.
- Standardizing transaction normalization through the fallback implementation.
Consider using a deep clone to ensure complete immutability, especially for nested objects:
- const editedTrans = { ...transaction }; + const editedTrans = JSON.parse(JSON.stringify(transaction));Note: While
JSON.parse(JSON.stringify())is a simple way to deep clone, it has limitations with certain types (e.g.,Date,undefined, functions). If you need to handle these types, consider using a library likelodash.cloneDeep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/558.mdis excluded by!**/*.md
📒 Files selected for processing (57)
src/app-gocardless/banks/abanca_caglesmm.js(1 hunks)src/app-gocardless/banks/abnamro_abnanl2a.js(1 hunks)src/app-gocardless/banks/american_express_aesudef1.js(0 hunks)src/app-gocardless/banks/bancsabadell_bsabesbbb.js(2 hunks)src/app-gocardless/banks/bank.interface.ts(2 hunks)src/app-gocardless/banks/bank_of_ireland_b365_bofiie2d.js(1 hunks)src/app-gocardless/banks/bankinter_bkbkesmm.js(1 hunks)src/app-gocardless/banks/belfius_gkccbebb.js(1 hunks)src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js(2 hunks)src/app-gocardless/banks/bnp_be_gebabebb.js(3 hunks)src/app-gocardless/banks/cbc_cregbebb.js(1 hunks)src/app-gocardless/banks/commerzbank_cobadeff.js(3 hunks)src/app-gocardless/banks/danskebank_dabno22.js(1 hunks)src/app-gocardless/banks/direkt_heladef1822.js(1 hunks)src/app-gocardless/banks/easybank_bawaatww.js(1 hunks)src/app-gocardless/banks/entercard_swednokk.js(2 hunks)src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js(2 hunks)src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)src/app-gocardless/banks/ing_ingbrobu.js(4 hunks)src/app-gocardless/banks/ing_ingddeff.js(1 hunks)src/app-gocardless/banks/ing_pl_ingbplpw.js(1 hunks)src/app-gocardless/banks/integration-bank.js(1 hunks)src/app-gocardless/banks/isybank_itbbitmm.js(1 hunks)src/app-gocardless/banks/kbc_kredbebb.js(1 hunks)src/app-gocardless/banks/lhv-lhvbee22.js(2 hunks)src/app-gocardless/banks/mbank_retail_brexplpw.js(0 hunks)src/app-gocardless/banks/nationwide_naiagb21.js(3 hunks)src/app-gocardless/banks/nbg_ethngraaxxx.js(1 hunks)src/app-gocardless/banks/norwegian_xx_norwnok1.js(2 hunks)src/app-gocardless/banks/revolut_revolt21.js(1 hunks)src/app-gocardless/banks/sandboxfinance_sfin0000.js(0 hunks)src/app-gocardless/banks/seb_kort_bank_ab.js(1 hunks)src/app-gocardless/banks/seb_privat.js(1 hunks)src/app-gocardless/banks/sparnord_spnodk22.js(1 hunks)src/app-gocardless/banks/spk_karlsruhe_karsde66.js(2 hunks)src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js(2 hunks)src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js(1 hunks)src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(2 hunks)src/app-gocardless/banks/swedbank_habalv22.js(3 hunks)src/app-gocardless/banks/tests/abanca_caglesmm.spec.js(1 hunks)src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js(2 hunks)src/app-gocardless/banks/tests/bancsabadell_bsabesbbb.spec.js(2 hunks)src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js(1 hunks)src/app-gocardless/banks/tests/cbc_cregbebb.spec.js(2 hunks)src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js(3 hunks)src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js(2 hunks)src/app-gocardless/banks/tests/kbc_kredbebb.spec.js(2 hunks)src/app-gocardless/banks/tests/lhv-lhvbee22.spec.js(2 hunks)src/app-gocardless/banks/tests/revolut_revolt21.spec.js(2 hunks)src/app-gocardless/banks/tests/spk_marburg_biedenkopf_heladef1mar.spec.js(2 hunks)src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)src/app-gocardless/banks/tests/swedbank_habalv22.spec.js(1 hunks)src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js(3 hunks)src/app-gocardless/banks/virgin_nrnbgb22.js(2 hunks)src/app-gocardless/services/tests/fixtures.js(3 hunks)src/app-gocardless/services/tests/gocardless-service.spec.js(1 hunks)src/app-simplefin/app-simplefin.js(1 hunks)
💤 Files with no reviewable changes (3)
- src/app-gocardless/banks/american_express_aesudef1.js
- src/app-gocardless/banks/mbank_retail_brexplpw.js
- src/app-gocardless/banks/sandboxfinance_sfin0000.js
✅ Files skipped from review due to trivial changes (3)
- src/app-gocardless/banks/tests/cbc_cregbebb.spec.js
- src/app-gocardless/banks/tests/kbc_kredbebb.spec.js
- src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js
🧰 Additional context used
📓 Learnings (7)
src/app-gocardless/banks/entercard_swednokk.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.
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.
src/app-gocardless/banks/seb_privat.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.
src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.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.
src/app-gocardless/banks/berliner_sparkasse_beladebexxx.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.
src/app-gocardless/banks/spk_karlsruhe_karsde66.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.
src/app-gocardless/banks/tests/swedbank_habalv22.spec.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual-server#490
File: src/app-gocardless/banks/tests/swedbank-habalv22.spec.js:19-27
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In tests scoped to a specific institution (like `SwedbankHabaLV22` in `swedbank-habalv22.spec.js`), it's acceptable to keep test descriptions general since they already indicate the context. If the changes to a function do not affect other parameters, it's unnecessary to add assertions for those parameters in the tests.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (46)
src/app-gocardless/banks/tests/swedbank_habalv22.spec.js (1)
54-55: LGTM! Verify the consistency ofpayeeNameusage.The change from
creditorNametopayeeNamealigns with the broader refactoring effort across the codebase.Let's verify that this naming change is consistent across other bank integrations:
✅ Verification successful
✓ Change verified -
payeeNameassertion is correctThe change from
creditorNametopayeeNamein the test assertion is correct. The codebase shows a clear pattern wherecreditorNameis used internally during transaction processing, whilepayeeNameis the standardized property name in the normalized output.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of creditorName and consistency of payeeName usage # Check for any remaining creditorName assertions in tests echo "Checking for remaining creditorName assertions in tests:" rg -l "\.creditorName\)" "src/app-gocardless/banks/tests" # Check for consistent usage of payeeName in tests echo "Checking payeeName assertions in tests:" rg -l "\.payeeName\)" "src/app-gocardless/banks/tests" # Check implementation files for any remaining creditorName references echo "Checking for remaining creditorName usage in implementation:" rg -l "creditorName" "src/app-gocardless/banks"Length of output: 2428
Script:
#!/bin/bash # Check the context of creditorName usage in implementation and test files echo "Checking context of creditorName usage in swedbank_habalv22.spec.js:" rg "creditorName" "src/app-gocardless/banks/tests/swedbank_habalv22.spec.js" -C 2 echo -e "\nChecking context of creditorName usage in implementation files:" rg "creditorName" "src/app-gocardless/banks/swedbank_habalv22.js" -C 2 echo -e "\nChecking for variable assignments and property access patterns:" ast-grep --pattern 'creditorName = $_' ast-grep --pattern '$_.creditorName'Length of output: 4507
src/app-gocardless/banks/seb_privat.js (2)
11-12: Adopted Immutability Approach Looks GoodRenaming
_bookedtobookedwhile introducing theeditedTransobject ensures immutability and clarity regarding the transaction's original data. This is a positive shift that keeps the code more maintainable.
15-17: Confirm Consistency ofadditionalInformationUsageAssigning
creditorNamefromtransaction.additionalInformationis correct if the server consistently populatesadditionalInformation. However, verify thatFallback.normalizeTransactiondoes not override or conflict withcreditorName.src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js (2)
9-10: Parameter Rename & Shallow Copy Improve ReadabilitySwitching
_bookedtobookedclarifies usage, and copyingtransactionintoeditedTranshelps preserve the original data, aligning with best practices for immutability.
25-28: Ensure Robust Handling ofremittanceInformationUnstructuredStoring
remittanceInformationUnstructuredineditedTranscorrectly defers final formatting to the fallback. Double-check that empty or null values don't cause downstream issues.src/app-gocardless/banks/cbc_cregbebb.js (1)
14-15: Effective Use ofeditedTransCreating a shallow copy
editedTransis a maintainable approach, preventing inadvertent modifications to the original transaction. The parameter rename tobookedalso enhances clarity.src/app-gocardless/banks/spk_karlsruhe_karsde66.js (2)
36-37: Clarify fields to avoid confusion with fallback logic.
By assigningcreditorNameandremittanceInformationUnstructuredtoeditedTrans, changes will pass through to the fallback. Ensure these field names align with the fallback’s expectations (e.g., payeeName vs. creditorName) to prevent overwriting or duplication.
40-40: Ensure the fallback normalization handles combined fields correctly.
You are passing both the originaltransactionand the updatededitedTrans. Confirm thatFallback.normalizeTransactionmerges or overrides the relevant fields as intended, especially any date or name fields that might conflict.src/app-gocardless/banks/direkt_heladef1822.js (2)
12-12: Validate use of the nullish coalescing operator.
Using??is correct if you only want to fallback when the primary value isnullorundefined. If you also need to handle empty strings, consider using||or a more robust check.
16-16: Confirm fallback behavior for remittance fields.
PassingeditedTransto the fallback function is consistent with the new pattern. However, ensure that any fallback logic forremittanceInformationUnstructuredor additional fields doesn’t unintentionally override the ones you’ve just set.src/app-gocardless/banks/isybank_itbbitmm.js (2)
14-14: Confirm date priority logic.
You’re prioritizingvalueDateoverbookingDate. IfvalueDatecan sometimes be an empty string or an invalid value, consider a more explicit fallback check or validation.
16-16: Keep fallback date handling in sync.
SinceFallback.normalizeTransactionmay parse dates internally, ensure that the newly assignededitedTrans.datefield won’t cause confusion or duplication with any date logic in the fallback.src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js (1)
9-17: LGTM! Well-structured implementation.The implementation follows best practices:
- Creates a shallow copy to avoid mutations
- Uses nullish coalescing for fallbacks
- Properly handles structured array joining
src/app-gocardless/banks/bankinter_bkbkesmm.js (1)
9-22: LGTM! Clean implementation of the new transaction normalization pattern.The changes follow the consistent pattern seen across other bank integrations:
- Creates a shallow copy of the transaction
- Safely modifies the copy using optional chaining
- Delegates to Fallback.normalizeTransaction
src/app-gocardless/banks/kbc_kredbebb.js (1)
14-31: LGTM! Clean implementation of transaction normalization.The implementation correctly:
- Handles both positive and negative transaction amounts
- Uses appropriate fallback patterns for payee name extraction
- Preserves the original transaction object using editedTrans
src/app-gocardless/banks/bancsabadell_bsabesbbb.js (1)
Line range hint
10-29: LGTM! Well-structured transaction normalization logic.The implementation correctly:
- Determines creditor/debtor based on transaction amount
- Handles the payee name extraction
- Preserves the original transaction object
src/app-gocardless/banks/lhv-lhvbee22.js (1)
27-31: LGTM! Proper date validation and assignment.The implementation correctly:
- Validates the date before assignment
- Only updates the date for booked transactions
- Safely extracts the payee name
src/app-gocardless/banks/danskebank_dabno22.js (1)
11-27: LGTM! Clean implementation of transaction normalization.The implementation correctly:
- Cleans up unnecessary EndToEndID information
- Preserves the original transaction object
- Delegates to the fallback normalizer
src/app-gocardless/banks/entercard_swednokk.js (2)
11-13: LGTM! Good practice using shallow copy.Creating a shallow copy prevents unintended mutations of the original transaction object.
27-29: Consider handling potential date parsing errors.While the date assignment looks correct, consider adding error handling for invalid dates.
Based on previous learnings from PR #490,
Fallback.normalizeTransactionhandles date parsing errors, so this implementation is safe.src/app-gocardless/banks/virgin_nrnbgb22.js (2)
10-11: LGTM! Consistent with immutable pattern.Creating a shallow copy aligns with the pattern of preventing mutations to the original transaction.
37-37: LGTM! Proper delegation to Fallback.Correctly delegates to Fallback implementation with all required parameters.
src/app-gocardless/banks/ing_pl_ingbplpw.js (1)
11-16: LGTM! Clean and focused implementation.The implementation follows the established pattern and correctly handles only what's specific to this bank integration.
src/app-gocardless/banks/tests/revolut_revolt21.spec.js (1)
20-20: LGTM! Tests updated to reflect new property names.Test assertions correctly updated to use
notesinstead ofremittanceInformationUnstructured, aligning with the server-side normalization changes.Also applies to: 40-40
src/app-gocardless/banks/bank.interface.ts (2)
7-13: LGTM! Well-structured type definition for normalized transaction data.The
TransactionExtendedtype properly captures all the fields needed for server-side normalization.
34-37: LGTM! Interface changes support immutable transaction handling.The updated signature with optional
editedTransactionparameter enables implementations to handle transaction modifications immutably.src/app-gocardless/banks/swedbank_habalv22.js (2)
45-45: LGTM! Proper use of Fallback normalization.The implementation correctly passes both original and edited transactions to the fallback normalizer.
15-16: Consider adding date validation before parsing.While the implementation correctly uses immutable updates, the date parsing could be more robust:
- No validation of
dateMatch[1]format before parsing- No error handling for invalid dates
Also applies to: 28-28, 37-41
src/app-gocardless/banks/nationwide_naiagb21.js (1)
10-11: LGTM! Proper use of immutable updates for date handling.The implementation correctly uses
editedTransfor date modifications.Also applies to: 24-24
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
9-11: LGTM! Clean implementation of immutable updates.The implementation correctly uses
editedTransfor all modifications.Also applies to: 43-44
src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js (2)
27-29: LGTM! Property name update aligns with the new transaction structure.The assertion now correctly uses
notesinstead ofremittanceInformationUnstructured, maintaining consistency with the server-side notes normalization changes.
55-57: LGTM! Consistent property naming in Google Pay test case.The assertion follows the same pattern of using
notesfor the normalized transaction information.src/app-gocardless/banks/tests/bancsabadell_bsabesbbb.spec.js (1)
18-18: LGTM! Property name updates align with the new transaction structure.The assertions now correctly use
payeeNameinstead ofcreditorName/debtorName, maintaining consistency with the server-side normalization changes.Also applies to: 33-33
src/app-gocardless/banks/commerzbank_cobadeff.js (1)
9-11: LGTM! Changes align with the new transaction normalization pattern.The method correctly uses
editedTransfor modifications and follows the standardized parameter naming.Also applies to: 49-49
src/app-gocardless/banks/abnamro_abnanl2a.js (1)
11-12: LGTM! Good practice using object spread for immutability.Creating a shallow copy of the transaction object before modification prevents unintended side effects.
src/app-gocardless/banks/tests/lhv-lhvbee22.spec.js (1)
21-21: LGTM! Tests correctly updated to reflect the new property name.The test cases have been properly updated to use
payeeNameinstead ofcreditorName, maintaining consistency with the changes in the implementation.Also applies to: 59-59
src/app-gocardless/banks/bnp_be_gebabebb.js (1)
Line range hint
31-51: Add validation for additionalInformation format.The code assumes a specific format for additionalInformation without validation.
Add validation:
if (transaction.additionalInformation) { + if (typeof transaction.additionalInformation !== 'string') { + console.warn('Invalid additionalInformation format'); + return Fallback.normalizeTransaction(transaction, booked, editedTrans); + } let additionalInformationObject = {};✅ Verification successful
The suggested validation is necessary and correct
The validation is particularly important for this bank parser as it performs complex string operations with
matchAll, while other banks useadditionalInformationin simpler ways. The TypeScript types confirm thatadditionalInformationshould be a string when present.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar additionalInformation parsing patterns ast-grep --pattern 'transaction.additionalInformation.matchAll($_)'Length of output: 189
Script:
#!/bin/bash # Search for additionalInformation usage rg "additionalInformation" -B 2 -A 2 # Search for similar validation patterns ast-grep --pattern 'typeof $_ !== "string"'Length of output: 8281
src/app-gocardless/banks/integration-bank.js (3)
46-47: LGTM! Good use of nullish coalescing for fallback.The addition of the
editedTransactionparameter with a default value ofnulland the use of the nullish coalescing operator for fallback is a clean implementation.
75-77: LGTM! Clean implementation of the return statement.The return statement correctly spreads the transaction object and overrides specific fields. The use of nullish coalescing for
payeeNameprovides a good fallback mechanism.
50-55: Consider adding type validation for date fields.While the date handling logic is correct, consider adding type validation to ensure the date fields contain valid ISO strings before parsing.
Run this script to check date field types across transactions:
src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js (3)
31-33: LGTM! Test assertion updated correctly.The assertion has been properly updated to check the
notesproperty instead ofremittanceInformationUnstructured.
69-72: LGTM! Complex string handling test case.Good test coverage for complex string concatenation with multiple fields.
105-107: LGTM! Edge case handling test.Good test coverage for the edge case of removing 'NOTPROVIDED' values.
src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js (1)
143-150: LGTM! Proper validation of payee name based on transaction type.Good test coverage ensuring
payeeNameis set correctly for both creditor and debtor transactions.src/app-gocardless/banks/tests/spk_marburg_biedenkopf_heladef1mar.spec.js (1)
151-151: LGTM!The test assertions have been updated to use the new standardized property name
notesinstead ofremittanceInformationUnstructured, aligning with the broader refactoring effort.Also applies to: 175-175
src/app-gocardless/services/tests/gocardless-service.spec.js (1)
420-421: LGTM!The test data has been improved with:
- More realistic date strings instead of placeholder values.
- New standardized fields for transaction notes and remittance information.
Also applies to: 426-429, 435-435, 439-441, 443-444, 450-450, 455-456, 458-459, 464-464
matt-fidd
force-pushed
the
gocardless-handlers
branch
from
6009ce9 to
d80325c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app-gocardless/banks/entercard_swednokk.js (1)
Line range hint
19-26: Maintain immutability by updating editedTrans instead of transaction.The code currently modifies the original transaction object when handling forex transactions. To maintain immutability, these changes should be made to
editedTransinstead.if (remittanceInformationUnstructured.startsWith('billingAmount: ')) { - transaction.transactionAmount = { + editedTrans.transactionAmount = { amount: remittanceInformationUnstructured.substring(15), currency: 'SEK', }; }
🧹 Nitpick comments (9)
src/app-gocardless/banks/commerzbank_cobadeff.js (1)
42-43: Improve clarity in replace logic.
Chaining multiple.replace()calls is functionally correct. If performance or readability becomes a concern for large strings, consider building a combined pattern or refactoring to a more readable approach.src/app-gocardless/banks/cbc_cregbebb.js (1)
17-27: Consider simplifying the payee name logic.The current implementation could be more concise by using a ternary operator and extracting the positive amount case to a separate function.
- if (Number(transaction.transactionAmount.amount) > 0) { - editedTrans.payeeName = - transaction.debtorName || transaction.remittanceInformationUnstructured; - } else { - editedTrans.payeeName = - transaction.creditorName || - extractPayeeNameFromRemittanceInfo( - transaction.remittanceInformationUnstructured, - ['Paiement', 'Domiciliation', 'Transfert', 'Ordre permanent'], - ); - } + const isPositiveAmount = Number(transaction.transactionAmount.amount) > 0; + editedTrans.payeeName = isPositiveAmount + ? (transaction.debtorName || transaction.remittanceInformationUnstructured) + : (transaction.creditorName || + extractPayeeNameFromRemittanceInfo( + transaction.remittanceInformationUnstructured, + ['Paiement', 'Domiciliation', 'Transfert', 'Ordre permanent'] + ));src/app-gocardless/banks/ing_ingddeff.js (1)
11-22: Consider optimizing regex pattern compilation.The implementation correctly uses
editedTransfor immutability. However, the regex pattern could be optimized by moving it outside the function as a constant.+const REMITTANCE_INFO_REGEX = /remittanceinformation:(.*)$/; + export default { ...Fallback, institutionIds: ['ING_INGDDEFF'], normalizeTransaction(transaction, booked) { const editedTrans = { ...transaction }; - const remittanceInformationMatch = /remittanceinformation:(.*)$/.exec( + const remittanceInformationMatch = REMITTANCE_INFO_REGEX.exec( transaction.remittanceInformationUnstructured, );src/app-gocardless/banks/easybank_bawaatww.js (1)
22-29: Optimize payee name assignment.The implementation correctly uses
editedTransfor immutability. However, the payee name assignment could be optimized.normalizeTransaction(transaction, booked) { const editedTrans = { ...transaction }; - let payeeName = formatPayeeName(transaction); - if (!payeeName) payeeName = extractPayeeName(transaction); - editedTrans.payeeName = payeeName; + editedTrans.payeeName = formatPayeeName(transaction) || extractPayeeName(transaction); return Fallback.normalizeTransaction(transaction, booked, editedTrans); },src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (1)
Line range hint
9-42: Optimize regex pattern and keywords compilation.The implementation correctly uses
editedTransfor immutability. However, the regex pattern and keywords could be optimized by moving them outside the function.+const KEYWORDS_TO_REMOVE = [ + 'VIR INST', + 'VIR', + 'PRLV', + 'ANN CARTE', + 'CARTE \\d{2}\\/\\d{2}', +]; +const KEYWORDS_REGEX = new RegExp(KEYWORDS_TO_REMOVE.join('|'), 'g'); + export default { ...Fallback, institutionIds: ['FORTUNEO_FTNOFRP1XXX'], normalizeTransaction(transaction, booked) { const editedTrans = { ...transaction }; - const keywordsToRemove = [ - 'VIR INST', - 'VIR', - 'PRLV', - 'ANN CARTE', - 'CARTE \\d{2}\\/\\d{2}', - ]; - const details = transaction.remittanceInformationUnstructuredArray.join(' '); const amount = transaction.transactionAmount.amount; - const regex = new RegExp(keywordsToRemove.join('|'), 'g'); - const payeeName = details.replace(regex, '').trim(); + const payeeName = details.replace(KEYWORDS_REGEX, '').trim();src/app-gocardless/banks/integration-bank.js (1)
63-72: Consider using a different delimiter for array joins.Using commas as delimiters could cause issues if the array elements themselves contain commas. Consider using a less common character or sequence.
- transaction.remittanceInformationUnstructuredArrayString = - transaction.remittanceInformationUnstructuredArray?.join(','); - transaction.remittanceInformationStructuredArrayString = - transaction.remittanceInformationStructuredArray?.join(','); + transaction.remittanceInformationUnstructuredArrayString = + transaction.remittanceInformationUnstructuredArray?.join(' | '); + transaction.remittanceInformationStructuredArrayString = + transaction.remittanceInformationStructuredArray?.join(' | ');src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js (1)
Line range hint
11-40: Simplify the remittance information logic.The current implementation can be made more concise using array methods.
- let remittanceInformationUnstructured; - - if (transaction.remittanceInformationUnstructured) { - remittanceInformationUnstructured = - transaction.remittanceInformationUnstructured; - } else if (transaction.remittanceInformationStructured) { - remittanceInformationUnstructured = - transaction.remittanceInformationStructured; - } else if (transaction.remittanceInformationStructuredArray?.length > 0) { - remittanceInformationUnstructured = - transaction.remittanceInformationStructuredArray?.join(' '); - } - - if (transaction.additionalInformation) - remittanceInformationUnstructured += - ' ' + transaction.additionalInformation; + const remittanceInformationUnstructured = [ + transaction.remittanceInformationUnstructured, + transaction.remittanceInformationStructured, + transaction.remittanceInformationStructuredArray?.join(' '), + transaction.additionalInformation + ].find(Boolean) || '';src/app-gocardless/banks/abnamro_abnanl2a.js (1)
19-24: Extract payee name parsing logic to a utility function.The payee name extraction logic is specific to ABNAMRO's format and would be clearer as a separate utility function.
+ const extractPayeeFromRemittance = (remittanceArray) => { + return remittanceArray + .map((el) => el.match(/^(?:.*\*)?(.+),PAS\d+$/)) + .find((match) => match)?.[1]; + }; + - const payeeName = transaction.remittanceInformationUnstructuredArray - .map((el) => el.match(/^(?:.*\*)?(.+),PAS\d+$/)) - .find((match) => match)?.[1]; + const payeeName = extractPayeeFromRemittance( + transaction.remittanceInformationUnstructuredArray + );src/app-gocardless/banks/tests/lhv-lhvbee22.spec.js (1)
21-23: Add more edge cases to the tests.Consider adding test cases for:
- Empty or malformed payee names
- Special characters in payee names
- Missing transaction dates
it('handles empty payee name', () => { const transaction = { ...bookedCardTransaction, creditorName: '', remittanceInformationUnstructured: '' }; expect( LhvLhvbee22.normalizeTransaction(transaction, true).payeeName ).toBeNull(); });Also applies to: 59-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/558.mdis excluded by!**/*.md
📒 Files selected for processing (57)
src/app-gocardless/banks/abanca_caglesmm.js(1 hunks)src/app-gocardless/banks/abnamro_abnanl2a.js(1 hunks)src/app-gocardless/banks/american_express_aesudef1.js(0 hunks)src/app-gocardless/banks/bancsabadell_bsabesbbb.js(2 hunks)src/app-gocardless/banks/bank.interface.ts(2 hunks)src/app-gocardless/banks/bank_of_ireland_b365_bofiie2d.js(1 hunks)src/app-gocardless/banks/bankinter_bkbkesmm.js(1 hunks)src/app-gocardless/banks/belfius_gkccbebb.js(1 hunks)src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js(2 hunks)src/app-gocardless/banks/bnp_be_gebabebb.js(3 hunks)src/app-gocardless/banks/cbc_cregbebb.js(1 hunks)src/app-gocardless/banks/commerzbank_cobadeff.js(3 hunks)src/app-gocardless/banks/danskebank_dabno22.js(1 hunks)src/app-gocardless/banks/direkt_heladef1822.js(1 hunks)src/app-gocardless/banks/easybank_bawaatww.js(1 hunks)src/app-gocardless/banks/entercard_swednokk.js(2 hunks)src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js(2 hunks)src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)src/app-gocardless/banks/ing_ingbrobu.js(4 hunks)src/app-gocardless/banks/ing_ingddeff.js(1 hunks)src/app-gocardless/banks/ing_pl_ingbplpw.js(1 hunks)src/app-gocardless/banks/integration-bank.js(1 hunks)src/app-gocardless/banks/isybank_itbbitmm.js(1 hunks)src/app-gocardless/banks/kbc_kredbebb.js(1 hunks)src/app-gocardless/banks/lhv-lhvbee22.js(2 hunks)src/app-gocardless/banks/mbank_retail_brexplpw.js(0 hunks)src/app-gocardless/banks/nationwide_naiagb21.js(3 hunks)src/app-gocardless/banks/nbg_ethngraaxxx.js(1 hunks)src/app-gocardless/banks/norwegian_xx_norwnok1.js(2 hunks)src/app-gocardless/banks/revolut_revolt21.js(1 hunks)src/app-gocardless/banks/sandboxfinance_sfin0000.js(0 hunks)src/app-gocardless/banks/seb_kort_bank_ab.js(1 hunks)src/app-gocardless/banks/seb_privat.js(1 hunks)src/app-gocardless/banks/sparnord_spnodk22.js(1 hunks)src/app-gocardless/banks/spk_karlsruhe_karsde66.js(2 hunks)src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js(2 hunks)src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js(1 hunks)src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(2 hunks)src/app-gocardless/banks/swedbank_habalv22.js(3 hunks)src/app-gocardless/banks/tests/abanca_caglesmm.spec.js(1 hunks)src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js(2 hunks)src/app-gocardless/banks/tests/bancsabadell_bsabesbbb.spec.js(2 hunks)src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js(1 hunks)src/app-gocardless/banks/tests/cbc_cregbebb.spec.js(2 hunks)src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js(3 hunks)src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js(2 hunks)src/app-gocardless/banks/tests/kbc_kredbebb.spec.js(2 hunks)src/app-gocardless/banks/tests/lhv-lhvbee22.spec.js(2 hunks)src/app-gocardless/banks/tests/revolut_revolt21.spec.js(2 hunks)src/app-gocardless/banks/tests/spk_marburg_biedenkopf_heladef1mar.spec.js(2 hunks)src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)src/app-gocardless/banks/tests/swedbank_habalv22.spec.js(1 hunks)src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js(3 hunks)src/app-gocardless/banks/virgin_nrnbgb22.js(2 hunks)src/app-gocardless/services/tests/fixtures.js(3 hunks)src/app-gocardless/services/tests/gocardless-service.spec.js(1 hunks)src/app-simplefin/app-simplefin.js(1 hunks)
💤 Files with no reviewable changes (3)
- src/app-gocardless/banks/american_express_aesudef1.js
- src/app-gocardless/banks/mbank_retail_brexplpw.js
- src/app-gocardless/banks/sandboxfinance_sfin0000.js
🚧 Files skipped from review as they are similar to previous changes (37)
- src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js
- src/app-gocardless/banks/tests/cbc_cregbebb.spec.js
- src/app-gocardless/banks/tests/revolut_revolt21.spec.js
- src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.js
- src/app-gocardless/banks/tests/commerzbank_cobadeff.spec.js
- src/app-gocardless/services/tests/gocardless-service.spec.js
- src/app-gocardless/banks/tests/abanca_caglesmm.spec.js
- src/app-gocardless/banks/tests/kbc_kredbebb.spec.js
- src/app-simplefin/app-simplefin.js
- src/app-gocardless/banks/isybank_itbbitmm.js
- src/app-gocardless/banks/direkt_heladef1822.js
- src/app-gocardless/banks/virgin_nrnbgb22.js
- src/app-gocardless/banks/tests/spk_marburg_biedenkopf_heladef1mar.spec.js
- src/app-gocardless/services/tests/fixtures.js
- src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js
- src/app-gocardless/banks/lhv-lhvbee22.js
- src/app-gocardless/banks/nationwide_naiagb21.js
- src/app-gocardless/banks/seb_kort_bank_ab.js
- src/app-gocardless/banks/ing_pl_ingbplpw.js
- src/app-gocardless/banks/bank.interface.ts
- src/app-gocardless/banks/bancsabadell_bsabesbbb.js
- src/app-gocardless/banks/bnp_be_gebabebb.js
- src/app-gocardless/banks/tests/swedbank_habalv22.spec.js
- src/app-gocardless/banks/tests/bancsabadell_bsabesbbb.spec.js
- src/app-gocardless/banks/bankinter_bkbkesmm.js
- src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js
- src/app-gocardless/banks/sparnord_spnodk22.js
- src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js
- src/app-gocardless/banks/ing_ingbrobu.js
- src/app-gocardless/banks/abanca_caglesmm.js
- src/app-gocardless/banks/danskebank_dabno22.js
- src/app-gocardless/banks/hype_hyeeit22.js
- src/app-gocardless/banks/bank_of_ireland_b365_bofiie2d.js
- src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js
- src/app-gocardless/banks/norwegian_xx_norwnok1.js
- src/app-gocardless/banks/spk_karlsruhe_karsde66.js
- src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js
🧰 Additional context used
📓 Learnings (6)
src/app-gocardless/banks/swedbank_habalv22.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.
src/app-gocardless/banks/seb_privat.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.
src/app-gocardless/banks/revolut_revolt21.js (2)
Learnt from: matt-fidd
PR: actualbudget/actual-server#558
File: src/app-gocardless/banks/revolut_revolt21.js:13-24
Timestamp: 2025-01-29T19:42:31.559Z
Learning: For Revolut institution (REVOLUT_REVOLT21), remittanceInformationUnstructuredArray is guaranteed to exist and contain the expected elements, so bounds checking is not necessary.
Learnt from: matt-fidd
PR: actualbudget/actual-server#558
File: src/app-gocardless/banks/revolut_revolt21.js:26-33
Timestamp: 2025-01-29T19:42:22.973Z
Learning: For the Revolut bank integration (REVOLUT_REVOLT21), remittanceInformationUnstructuredArray is guaranteed to exist and contain the required elements for Bizum payments, making array bounds checking unnecessary.
src/app-gocardless/banks/nbg_ethngraaxxx.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual-server#558
File: src/app-gocardless/banks/nbg_ethngraaxxx.js:16-31
Timestamp: 2025-01-29T19:40:22.266Z
Learning: In the GoCardless NBG bank integration, the transaction amount field must be modified directly on the original transaction object since it's not processed by the fallback handler.
src/app-gocardless/banks/berliner_sparkasse_beladebexxx.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.
src/app-gocardless/banks/entercard_swednokk.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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (16)
src/app-gocardless/banks/commerzbank_cobadeff.js (4)
9-10: Function signature renamed for clarity.
Good job standardizing the parameter name and creating a shallow copy of the transaction. This approach helps maintain immutability and better readability.
30-31: Insight on complex regex usage.
Replacing spaces within keywords using a dynamic RegExp is powerful but can be tricky. Ensure that these replacements won’t inadvertently match unintended substrings containing these keywords in different contexts.
49-49: Delegating to Fallback for final processing.
This is consistent with your new design pattern. Good work on ensuring all relevant parameters (transaction,booked,editedTrans) are passed.
15-15: Check for undefined array before joining.
Iftransaction.remittanceInformationUnstructuredArrayis ever undefined or null, calling.join(' ')will throw an error. Consider using optional chaining or a default value.- editedTrans.remittanceInformationUnstructured = - transaction.remittanceInformationUnstructuredArray.join(' '); + editedTrans.remittanceInformationUnstructured = + transaction.remittanceInformationUnstructuredArray?.join(' ') ?? '';✅ Verification successful
Optional chaining needed for array access
TheremittanceInformationUnstructuredArrayis marked as optional in the type definitions and other bank implementations use defensive coding. The suggested change to use optional chaining with nullish coalescing is correct to prevent runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for remittanceInformationUnstructuredArray usage echo "=== Searching for remittanceInformationUnstructuredArray usage ===" rg "remittanceInformationUnstructuredArray" -B 2 -A 2 echo -e "\n=== Checking other bank implementations for similar patterns ===" fd ".*\.js$" "src/app-gocardless/banks" --exec grep -l "remittanceInformationUnstructured" echo -e "\n=== Looking for transaction object structure ===" ast-grep --pattern 'transaction.$_'Length of output: 39883
src/app-gocardless/banks/belfius_gkccbebb.js (3)
12-12: Parameter rename aligns with consistency.
Updating_bookedtobookedprovides uniformity across the codebase.
13-13: Assigning transactionId from internalTransactionId.
This is a straightforward fix for duplicated transaction IDs. Just keep in mind that ifinternalTransactionIdis ever missing or incorrect, it may cause reconciliation issues downstream.
15-15: Fallback normalization delegated properly.
Clean and concise. This is in line with the new architecture for normalization.src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js (3)
9-10: Preserving immutability via shallow copy.
Good move usingeditedTrans. This prevents accidental mutations of the originaltransaction.
12-12: Handle potential null or undefined.
Using the nullish coalescing operators is great. Ensure no confusion arises whenremittanceInformationUnstructuredvs.remittanceInformationStructuredvs. their array forms all exist simultaneously.
17-17: Consistent fallback usage.
Properly passing both the originaltransactionand theeditedTransobject is aligned with your new normalization pattern.src/app-gocardless/banks/seb_privat.js (1)
11-17: LGTM! Clean implementation of the server-side normalization.The changes follow the new pattern correctly:
- Parameter renamed for consistency
- Uses
editedTransto avoid mutating the original transaction- Properly delegates to
Fallback.normalizeTransactionsrc/app-gocardless/banks/revolut_revolt21.js (1)
9-35: LGTM! Implementation correctly handles Bizum payments.Based on the provided learnings, the implementation is correct as
remittanceInformationUnstructuredArrayis guaranteed to exist and contain the expected elements for this institution.src/app-gocardless/banks/swedbank_habalv22.js (1)
Line range hint
15-45: LGTM! Clean implementation of immutable transaction handling.The changes correctly implement immutability by using
editedTransfor modifications while preserving the original transaction object. The date parsing is appropriately handled using date-fns, and error cases are covered by the fallback handler.src/app-gocardless/banks/nbg_ethngraaxxx.js (1)
16-31: LGTM! Correctly handles transaction amount modification.The implementation correctly modifies the transaction amount directly on the original transaction object (as required since it's not processed by the fallback handler) while maintaining immutability for other fields using
editedTrans.src/app-gocardless/banks/integration-bank.js (2)
46-48: LGTM! Good improvement to transaction immutability.The addition of the
editedTransactionparameter allows for modifications without mutating the original transaction object, which is a good practice.
49-62: LGTM! Improved date handling logic.The date prioritization and early return for invalid dates enhance the robustness of the transaction normalization process.
matt-fidd
force-pushed
the
gocardless-handlers
branch
from
d0468d5 to
3418e06
Compare
matt-fidd
force-pushed
the
gocardless-handlers
branch
from
3418e06 to
4441caa
Compare
Complimentary to actualbudget/actual#4253
This changes the pattern for GoCardless handlers too: