Add GoCardless integration for ENTERCARD_SWEDNOKK #506
Conversation
Signed-off-by: Johannes Löthberg <[email protected]>
actual-github-bot
bot
changed the title
kyrias
changed the title
WalkthroughThe pull request introduces changes to the bank integration system within the application. Specifically, it adds a new bank, Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (1)
src/app-gocardless/banks/entercard-swednokk.js (1)
36-41: Avoid mutating the input 'transaction' objectModifying the
transactionobject directly can lead to unintended side effects. Instead of mutatingtransaction.transactionAmount, consider creating a new object with the updatedtransactionAmount.Apply this diff to avoid mutating the input object:
const remittanceInformationUnstructured = transaction.remittanceInformationUnstructured; if ( remittanceInformationUnstructured && remittanceInformationUnstructured.startsWith('billingAmount: ') ) { - transaction.transactionAmount = { + const updatedTransactionAmount = { amount: parseFloat(remittanceInformationUnstructured.substring(15)), currency: 'SEK', }; + return { + ...transaction, + transactionAmount: updatedTransactionAmount, + payeeName: formatPayeeName(transaction), + date: transaction.valueDate + ? d.format(d.parseISO(transaction.valueDate), 'yyyy-MM-dd') + : null, + }; }This ensures that the original
transactionobject remains unmodified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/506.mdis excluded by!**/*.md
📒 Files selected for processing (2)
src/app-gocardless/bank-factory.js(4 hunks)src/app-gocardless/banks/entercard-swednokk.js(1 hunks)
🔇 Additional comments (5)
src/app-gocardless/bank-factory.js (3)
25-25: Verify if sandbox bank activation is intentional
The sandbox bank SandboxfinanceSfin0000 has been uncommented alongside the new Entercard integration. This seems unusual for a production bank addition.
Let's check if this was discussed in the PR or if there are related test changes:
✅ Verification successful
Sandbox bank is used for integration testing
The sandbox bank SandboxfinanceSfin0000 is intentionally included in the codebase as it's used for integration testing:
- It's referenced in test files (
gocardless-service.spec.js,integration-bank.spec.js) - The README explicitly mentions it as a template: "Create new a bank class based on
app-gocardless/banks/sandboxfinance-sfin0000.js" - It's consistently used across multiple test fixtures and integration tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any test files or documentation mentioning the sandbox bank
rg -i "sandboxfinance|sfin0000" --type md --type js
Length of output: 3151
Line range hint 69-116: Verify bank configuration requirements
Please verify:
- Whether ENTERCARD_SWEDNOKK should be added to BANKS_WITH_LIMITED_HISTORY
- If the institutionId in the implementation matches "ENTERCARD_SWEDNOKK"
Let's check the configuration:
✅ Verification successful
Bank configuration verification completed
Based on the code analysis:
- The institutionId "ENTERCARD_SWEDNOKK" is correctly implemented in the bank file.
- The bank should not be added to BANKS_WITH_LIMITED_HISTORY as:
- The implementation shows no specific history limitations
- The bank follows the standard transaction fetching pattern
- BANKS_WITH_LIMITED_HISTORY is used to cap transaction history at 89 days for specific banks, which isn't a requirement for this integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the institution ID in the implementation
rg -A 2 "institutionIds.*ENTERCARD" ./banks/entercard-swednokk.js
# Look for any documentation about history limitations
rg -i "entercard.*history" --type md
Length of output: 176
Script:
#!/bin/bash
# Let's first find the correct path for the bank implementation
fd -t f "entercard" --full-path
# Search for institution ID pattern in all files
rg -i "entercard.*swednokk"
# Check if there's any documentation about this bank
rg -i "entercard" --type md
Length of output: 539
Script:
#!/bin/bash
# Let's check the implementation of entercard-swednokk.js to see if there are any history limitations
cat src/app-gocardless/banks/entercard-swednokk.js
# Let's also check if there are any patterns or documentation about banks with limited history
rg -i "limited.?history" --type js
Length of output: 2310
Script:
#!/bin/bash
# Let's check how maxHistoricalDays is used for banks with limited history
rg -A 5 "maxHistoricalDays:" src/app-gocardless/services/gocardless-service.js
# Let's also check if there are any comments or documentation about the criteria for limited history banks
rg -B 2 "BANKS_WITH_LIMITED_HISTORY" src/app-gocardless/
Length of output: 1034
44-44: Verify EntercardSwednokk implementation
The bank is correctly added to the array, but let's verify its implementation matches the interface requirements.
Let's check the implementation:
✅ Verification successful
EntercardSwednokk implementation matches interface requirements
The implementation correctly implements all required methods from the IBank interface:
institutionIds: Correctly defined with 'ENTERCARD_SWEDNOKK'accessValidForDays: Set to 180normalizeAccount: Properly formats account details with required fieldsnormalizeTransaction: Handles transaction normalization with special forex handlingsortTransactions: Uses common utility for sortingcalculateStartingBalance: Correctly implements balance calculation
The implementation follows similar patterns to other bank implementations like American Express, with appropriate customizations for Entercard's specific needs, particularly in forex transaction handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new bank implements required interface methods
ast-grep --pattern 'export default {
$$$
institutionIds: $_,
normalizeAccount: $_,
normalizeTransaction: $_,
$$$
}' ./banks/entercard-swednokk.js
# Check for test coverage
rg -l "entercard.*\.test\."
Length of output: 264
Script:
#!/bin/bash
# Let's first find the correct path for the bank implementation
fd -t f -i "entercard"
# After finding the file, we'll check its content
fd -t f -i "entercard" --exec cat {}
Length of output: 1994
Script:
#!/bin/bash
# Let's verify the test coverage
fd -t f -i "entercard.*test"
# Let's also check the bank interface requirements
cat src/app-gocardless/banks/bank.interface.js
Length of output: 154
Script:
#!/bin/bash
# Let's find the interface file first
fd -t f "bank.interface"
# Let's also check other bank implementations for comparison
fd -t f -d 1 . src/app-gocardless/banks/ --exec ls -la {}
Length of output: 3306
Script:
#!/bin/bash
# Let's check the interface requirements
cat src/app-gocardless/banks/bank.interface.ts
# Let's also check a similar bank implementation for comparison
cat src/app-gocardless/banks/american-express-aesudef1.js
Length of output: 3217
src/app-gocardless/banks/entercard-swednokk.js (2)
55-57: Verify the correctness of starting balance calculation
The calculateStartingBalance function subtracts each transaction amount from the initial balance. Please verify that this logic accurately reflects the intended calculation, considering the sign of transaction amounts and whether they should be added or subtracted to compute the starting balance.
24-24: Ensure 'account.institution_id' is defined
In the official_name property, you are using account.institution_id. Please verify that institution_id exists on the account object to avoid undefined values.
| return { | ||
| ...transaction, | ||
| payeeName: formatPayeeName(transaction), | ||
| date: d.format(d.parseISO(transaction.valueDate), 'yyyy-MM-dd'), |
There was a problem hiding this comment.
Ensure 'transaction.valueDate' is defined before parsing
If transaction.valueDate is undefined or invalid, d.parseISO may result in an invalid date, and d.format could produce unexpected results or throw an error. Consider checking that transaction.valueDate is a valid ISO string before parsing.
Apply this diff to handle undefined transaction.valueDate:
return {
...transaction,
payeeName: formatPayeeName(transaction),
- date: d.format(d.parseISO(transaction.valueDate), 'yyyy-MM-dd'),
+ date: transaction.valueDate
+ ? d.format(d.parseISO(transaction.valueDate), 'yyyy-MM-dd')
+ : null,
};📝 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.
| date: d.format(d.parseISO(transaction.valueDate), 'yyyy-MM-dd'), | |
| date: transaction.valueDate | |
| ? d.format(d.parseISO(transaction.valueDate), 'yyyy-MM-dd') | |
| : null, |
| amount: remittanceInformationUnstructured.substring(15), | ||
| currency: 'SEK', |
There was a problem hiding this comment.
Parse and validate 'amount' to ensure it's a valid number
When extracting the amount from remittanceInformationUnstructured, it's important to parse and validate it to ensure it's a valid number. This prevents potential errors or injection attacks due to unexpected input formats.
Apply this diff to parse and validate the amount:
transaction.transactionAmount = {
- amount: remittanceInformationUnstructured.substring(15),
+ amount: parseFloat(remittanceInformationUnstructured.substring(15)),
currency: 'SEK',
};
+if (isNaN(transaction.transactionAmount.amount)) {
+ // Handle the error appropriately, e.g., throw an error or set a default value
+ throw new Error('Invalid transaction amount extracted from remittance information.');
+}📝 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.
| amount: remittanceInformationUnstructured.substring(15), | |
| currency: 'SEK', | |
| amount: parseFloat(remittanceInformationUnstructured.substring(15)), | |
| currency: 'SEK', | |
| if (isNaN(transaction.transactionAmount.amount)) { | |
| // Handle the error appropriately, e.g., throw an error or set a default value | |
| throw new Error('Invalid transaction amount extracted from remittance information.'); | |
| } |
| transaction.remittanceInformationUnstructured; | ||
| if (remittanceInformationUnstructured.startsWith('billingAmount: ')) { | ||
| transaction.transactionAmount = { | ||
| amount: remittanceInformationUnstructured.substring(15), | ||
| currency: 'SEK', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Handle undefined 'remittanceInformationUnstructured' before calling 'startsWith'
If transaction.remittanceInformationUnstructured is undefined or null, calling startsWith on it will result in a runtime error. Consider adding a check to ensure it is defined before calling startsWith.
Apply this diff to fix the issue:
const remittanceInformationUnstructured =
transaction.remittanceInformationUnstructured;
+if (
+ remittanceInformationUnstructured &&
+ remittanceInformationUnstructured.startsWith('billingAmount: ')
+) {
transaction.transactionAmount = {
amount: remittanceInformationUnstructured.substring(15),
currency: 'SEK',
};
}📝 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.
| transaction.remittanceInformationUnstructured; | |
| if (remittanceInformationUnstructured.startsWith('billingAmount: ')) { | |
| transaction.transactionAmount = { | |
| amount: remittanceInformationUnstructured.substring(15), | |
| currency: 'SEK', | |
| }; | |
| } | |
| const remittanceInformationUnstructured = | |
| transaction.remittanceInformationUnstructured; | |
| if ( | |
| remittanceInformationUnstructured && | |
| remittanceInformationUnstructured.startsWith('billingAmount: ') | |
| ) { | |
| transaction.transactionAmount = { | |
| amount: remittanceInformationUnstructured.substring(15), | |
| currency: 'SEK', | |
| }; | |
| } |
No description provided.