♻️ standardise GoCardless handlers #541
Conversation
actual-github-bot
bot
changed the title
matt-fidd
force-pushed
the
gcl-handlers
branch
from
d1f7cf3 to
fcdc432
Compare
matt-fidd
force-pushed
the
gcl-handlers
branch
2 times, most recently
from
48e9ccd to
1fba049
Compare
matt-fidd
force-pushed
the
gcl-handlers
branch
from
1fba049 to
a97a832
Compare
WalkthroughThis pull request encompasses a comprehensive set of modifications across multiple bank integration modules within the 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: 1
🧹 Nitpick comments (6)
src/app-gocardless/banks/swedbank_habalv22.js (1)
Line range hint
13-49: Consider enhancing documentation for card transaction parsing.While the code is functional, the complex regex patterns and date parsing logic could benefit from more detailed documentation explaining:
- The format of
remittanceInformationUnstructuredfield- Examples of valid card transaction strings
- The significance of extracted date formats
Add documentation like this:
/** * The actual transaction date for card transactions is only available in the remittanceInformationUnstructured field when the transaction is booked. + * + * Example remittanceInformationUnstructured formats: + * - Card purchase: "PIRKUMS 1234 01.02.24 10:30 123.45 EUR (1234) STORE NAME" + * - Date format: "dd.MM.yyyy" */src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js (1)
Line range hint
16-41: Consider standardizing date handling in Fallback.The date handling logic in
normalizeTransactionis complex and appears in multiple bank handlers. Consider moving this common pattern to the Fallback implementation for better maintainability.- const date = - transaction.bookingDate || - transaction.bookingDateTime || - transaction.valueDate || - transaction.valueDateTime; - // If we couldn't find a valid date field we filter out this transaction - // and hope that we will import it again once the bank has processed the - // transaction further. - if (!date) { - return null; - } + const date = Fallback.getTransactionDate(transaction);src/app-gocardless/banks/ing_ingddeff.js (1)
Line range hint
31-41: Consider moving sorting logic to Fallback.The transaction sorting logic appears to be a common pattern that could be standardized in the Fallback implementation, while allowing banks to override when needed.
src/app-gocardless/banks/seb_kort_bank_ab.js (1)
1-1: Well-structured standardization approach.The approach of centralizing common account normalization logic in the integration-bank while preserving bank-specific transaction and balance calculation logic is a good architectural decision. This improves maintainability while respecting the unique requirements of each bank integration.
src/app-gocardless/banks/hype_hyeeit22.js (1)
Line range hint
1-1: Excellent architectural approach to standardization!The changes demonstrate a well-thought-out approach to standardizing bank handlers:
- Consistent use of integration-bank.js as a fallback
- Standardized import patterns
- Removal of redundant account normalization code
- Unified access validity periods
- Preservation of bank-specific transaction handling logic
This approach reduces code duplication while maintaining the necessary flexibility for bank-specific requirements.
src/app-gocardless/banks/tests/ing_ingddeff.spec.js (1)
Line range hint
69-249: Consider enhancing transaction test coverage.While the transaction test data is comprehensive, consider adding test cases for:
- Transactions with different currencies
- Edge cases like very large amounts
- Special characters in transaction descriptions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/541.mdis excluded by!**/*.md
📒 Files selected for processing (42)
src/app-gocardless/bank-factory.js(2 hunks)src/app-gocardless/banks/abnamro_abnanl2a.js(1 hunks)src/app-gocardless/banks/american_express_aesudef1.js(1 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/berliner_sparkasse_beladebexxx.js(2 hunks)src/app-gocardless/banks/cbc_cregbebb.js(1 hunks)src/app-gocardless/banks/danskebank_dabno22.js(1 hunks)src/app-gocardless/banks/entercard_swednokk.js(1 hunks)src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js(1 hunks)src/app-gocardless/banks/hype_hyeeit22.js(1 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/mbank_retail_brexplpw.js(1 hunks)src/app-gocardless/banks/nationwide_naiagb21.js(0 hunks)src/app-gocardless/banks/nbg_ethngraaxxx.js(2 hunks)src/app-gocardless/banks/norwegian_xx_norwnok1.js(1 hunks)src/app-gocardless/banks/revolut_revolt21.js(1 hunks)src/app-gocardless/banks/sandboxfinance_sfin0000.js(2 hunks)src/app-gocardless/banks/seb_kort_bank_ab.js(1 hunks)src/app-gocardless/banks/spk_karlsruhe_karsde66.js(2 hunks)src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js(1 hunks)src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js(1 hunks)src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(1 hunks)src/app-gocardless/banks/swedbank_habalv22.js(1 hunks)src/app-gocardless/banks/tests/abanca_caglesmm.spec.js(1 hunks)src/app-gocardless/banks/tests/bancsabadell_bsabesbbb.spec.js(1 hunks)src/app-gocardless/banks/tests/easybank_bawaatww.spec.js(1 hunks)src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js(1 hunks)src/app-gocardless/banks/tests/ing_ingddeff.spec.js(2 hunks)src/app-gocardless/banks/tests/ing_pl_ingbplpw.spec.js(2 hunks)src/app-gocardless/banks/tests/integration_bank.spec.js(2 hunks)src/app-gocardless/banks/tests/mbank_retail_brexplpw.spec.js(2 hunks)src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js(1 hunks)src/app-gocardless/banks/tests/sandboxfinance_sfin0000.spec.js(2 hunks)src/app-gocardless/banks/tests/spk_marburg_biedenkopf_heladef1mar.spec.js(1 hunks)src/app-gocardless/banks/tests/swedbank_habalv22.spec.js(1 hunks)src/app-gocardless/banks/virgin_nrnbgb22.js(0 hunks)src/app-gocardless/services/tests/gocardless-service.spec.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/app-gocardless/banks/nationwide_naiagb21.js
- src/app-gocardless/banks/virgin_nrnbgb22.js
✅ Files skipped from review due to trivial changes (7)
- src/app-gocardless/banks/tests/abanca_caglesmm.spec.js
- src/app-gocardless/banks/tests/fortuneo_ftnofrp1xxx.spec.js
- src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js
- src/app-gocardless/banks/tests/swedbank_habalv22.spec.js
- src/app-gocardless/banks/tests/bancsabadell_bsabesbbb.spec.js
- src/app-gocardless/banks/tests/easybank_bawaatww.spec.js
- src/app-gocardless/banks/tests/spk_marburg_biedenkopf_heladef1mar.spec.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Docker image (alpine)
🔇 Additional comments (36)
src/app-gocardless/services/tests/gocardless-service.spec.js (1)
133-133: LGTM! Consistent standardization of account naming.The change from pattern matching to a static string aligns with the standardization effort and matches the changes in
integration_bank.spec.js.Let's verify the implementation of this standardization:
Also applies to: 138-138
✅ Verification successful
Standardization verified - correctly implemented for test scenarios
The standardization of "Savings Account for Individuals (Retail)" is consistently applied in test files while production code correctly maintains bank-specific naming conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of official_name assignment ast-grep --pattern 'official_name = $value'Length of output: 45
Script:
#!/bin/bash # Search for official_name assignments with ast-grep ast-grep --pattern 'official_name: $value' # Backup search with ripgrep for all occurrences rg "official_name.*" -A 1 -B 1Length of output: 3623
src/app-gocardless/banks/entercard_swednokk.js (1)
3-4: EnsureFallbackintegration maintains bank-specific functionalityBy importing
Fallbackand spreading its properties into the exported object, please verify that any bank-specific logic previously handled bynormalizeAccountandsortTransactionsis preserved or no longer required. The removal of these methods could impact how account data is processed and how transactions are sorted.Also applies to: 10-11
src/app-gocardless/banks/isybank_itbbitmm.js (1)
9-10: ConfirmaccessValidForDaysvalue for ISYBANK_ITBBITMMThe
accessValidForDaysproperty has been set to180. Please ensure that this duration accurately reflects the access validity period provided by GoCardless for ISYBANK_ITBBITMM.src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js (1)
11-11: Verify updatedaccessValidForDaysfrom 90 to 180The
accessValidForDaysvalue has been updated from90to180. Please confirm that this change aligns with the latest data from GoCardless for theSPK_WORMS_ALZEY_RIED_MALADE51WORinstitution and that it does not affect any existing access logic.src/app-gocardless/banks/bankinter_bkbkesmm.js (1)
11-11: LGTM! The accessValidForDays update aligns with standardization goals.The update from 90 to 180 days matches the data provided by GoCardless as mentioned in the PR objectives.
src/app-gocardless/banks/bank_of_ireland_b365_bofiie2d.js (1)
9-10: LGTM! Consistent with standardization efforts.The addition of accessValidForDays: 180 aligns with the standardization goals and matches the GoCardless data.
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
9-10: LGTM! Follows the standardization pattern.The addition of accessValidForDays: 180 is consistent with other handlers and aligns with GoCardless data.
src/app-gocardless/banks/kbc_kredbebb.js (1)
10-11: LGTM! Maintains consistency with other handlers.The addition of accessValidForDays: 180 follows the standardization pattern and aligns with GoCardless data.
src/app-gocardless/banks/cbc_cregbebb.js (1)
10-10: LGTM! Changes align with standardization goals.The addition of
accessValidForDays: 180aligns with the PR's objective to standardize access validity periods across GoCardless handlers.src/app-gocardless/banks/ing_pl_ingbplpw.js (1)
3-4: LGTM! Import changes align with standardization.The imports have been simplified and standardized, removing the unused
printIbanfunction and relying on the integration-bank fallback for account normalization.src/app-gocardless/banks/mbank_retail_brexplpw.js (1)
Line range hint
11-11: Verify if accessValidForDays: 179 is intentional.While other bank handlers use 180 days, this file uses 179 days. This might be a typo or could be specific to this bank's requirements.
Run this script to check the consistency across all bank handlers:
src/app-gocardless/banks/swedbank_habalv22.js (1)
11-11: LGTM! Standardized access validity period.The update from 90 to 180 days aligns with the standardization across other bank handlers.
src/app-gocardless/banks/danskebank_dabno22.js (1)
Line range hint
1-8: LGTM! Changes align with standardization goals.The implementation follows the standardization pattern by:
- Using the integration-bank fallback mechanism
- Following consistent import naming conventions
src/app-gocardless/banks/revolut_revolt21.js (2)
11-11: LGTM! Standardized access validity period.The update to
accessValidForDays: 180aligns with the standardization goals across all bank handlers.
Line range hint
13-57: Well-structured bank-specific logic with fallback.Good implementation pattern:
- Handles bank-specific Bizum payments
- Falls back to standard implementation when needed
src/app-gocardless/banks/american_express_aesudef1.js (1)
16-23: LGTM! Good use of the fallback mechanism while preserving AMEX-specific logic.The refactored
normalizeAccountmethod appropriately leverages the standard implementation while maintaining the necessary AMEX-specific handling of masked PAN and name formatting.src/app-gocardless/banks/sandboxfinance_sfin0000.js (2)
3-3: LGTM! Clean import statement.Import statements are now standardized and unnecessary imports have been removed.
12-12: Verify the impact of extending access validity.The
accessValidForDayshas been updated from 90 to 180 days. Please ensure this aligns with GoCardless's documentation for sandbox environments.✅ Verification successful
The change to 180 days access validity is consistent with production configurations.
The majority of GoCardless bank integrations use 180 days access validity, and this change aligns the sandbox environment with production behavior. This is supported by the EEA 180-day access standard referenced in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any documentation or comments about access validity periods rg -i "access.*valid.*days|validity.*period" --type mdLength of output: 56
Script:
#!/bin/bash # Search for accessValidForDays in all files rg "accessValidForDays" -B 2 -A 2 # Search for any mentions of access validity or days in comments rg -i "//.*access.*valid|/\*.*access.*valid" # Search specifically for sandbox configuration files fd -e js -e json sandboxLength of output: 14548
src/app-gocardless/banks/abnamro_abnanl2a.js (2)
2-5: LGTM! Clean import organization.Imports are now properly organized and unnecessary imports have been removed.
Line range hint
15-29: Verify the transaction normalization logic remains effective.The complex transaction normalization logic, especially the payee name extraction from
remittanceInformationUnstructuredArray, should be tested to ensure it still works as expected after the standardization changes.✅ Verification successful
Transaction normalization logic is properly tested
The complex transaction normalization logic, including the payee name extraction from
remittanceInformationUnstructuredArray, is thoroughly covered by unit tests. The test suite validates both standard transactions and edge cases (like Google Pay), ensuring the standardization changes work as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files covering this specific bank's transaction normalization rg -l "remittanceInformationUnstructuredArray.*PAS\d+" --type jsLength of output: 66
Script:
#!/bin/bash # Search for ABNAMRO/ABNANL2A related test files rg -l "ABNANL2A|ABN.?AMRO" --type js | grep -i "test" # Search for remittanceInformation related test files rg -l "remittanceInformation.*test" --type js # Search for transaction normalization test patterns rg -l "transaction.*normalize" --type js | grep -i "test"Length of output: 214
Script:
#!/bin/bash # View NBG test file content for test structure cat src/app-gocardless/banks/tests/nbg_ethngraaxxx.spec.js # Search for bank test files using different patterns fd -e js -e spec.js . src/app-gocardless/banks/tests # Search for transaction tests in spec files rg "describe.*transaction" -A 5 --type jsLength of output: 2692
Script:
#!/bin/bash # View ABN AMRO test file content cat src/app-gocardless/banks/tests/abnamro_abnanl2a.spec.jsLength of output: 1977
src/app-gocardless/banks/seb_kort_bank_ab.js (2)
3-5: LGTM! Clean import organization.Imports are now properly organized and unnecessary imports have been removed.
Line range hint
45-58: Verify the complex balance calculation logic.The balance calculation for SEB cards involves both
expectedandnonInvoicedbalance types, with sign flipping. Please ensure this logic remains correct after the standardization changes.src/app-gocardless/banks/nbg_ethngraaxxx.js (2)
3-4: LGTM! Import statements are now standardized.The imports have been simplified and follow a consistent pattern across the codebase.
12-12: Verify the impact of extending access validity.The access validity period has been extended from 90 to 180 days. While this aligns with the standardization effort, we should verify this change with GoCardless documentation.
✅ Verification successful
Access validity extension to 180 days is consistent with standard configuration
The change aligns with the established pattern across the GoCardless integration, where the vast majority of banks (25+) use a 180-day access validity period, including major institutions like Revolut, ING, and BNP Paribas.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any documentation or comments about access validity periods rg -g '!*.{js,jsx,ts,tsx}' -i "access.*valid.*days|token.*expir|valid.*period" .Length of output: 82
Script:
#!/bin/bash # Search for bank configuration files and access validity settings echo "=== Bank Configuration Files ===" fd -e js -e ts banks/ echo -e "\n=== Access Validity Configurations ===" rg -t js -t ts "accessValidForDays|validityPeriod|tokenExpiry" -A 2 echo -e "\n=== GoCardless Related Files ===" fd -e js -e ts gocardlessLength of output: 9167
src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js (1)
11-11: LGTM! Access validity period standardized.The change to 180 days is consistent with other bank handlers.
src/app-gocardless/banks/norwegian_xx_norwnok1.js (1)
3-4: LGTM! Import statements follow the new standard.The imports have been simplified and standardized.
src/app-gocardless/banks/hype_hyeeit22.js (1)
11-11: LGTM! Access validity period standardized.The change to 180 days is consistent with other bank handlers.
src/app-gocardless/banks/spk_karlsruhe_karsde66.js (1)
12-12: LGTM! Update toaccessValidForDaysaligns with EEA requirements.The change from 90 to 180 days correctly implements the new EEA requirements for extended access duration.
src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js (1)
12-12: LGTM! Consistent with standardization pattern.The update to
accessValidForDaysmatches the standardization across other bank handlers.src/app-gocardless/banks/integration-bank.js (1)
41-48: LGTM! Enhanced account name generation with robust fallbacks.The improved implementation provides better fallback mechanisms and maintains consistency across all bank handlers.
A few key improvements:
- Fallback chain for account names:
name→displayName→product- Consistent inclusion of currency information
- More meaningful
official_nameusing product informationsrc/app-gocardless/banks/tests/sandboxfinance_sfin0000.spec.js (1)
1-1: LGTM! Test updates reflect standardization changes.The changes correctly update:
- Import path to use underscore naming convention
- Test expectations to include currency in the account name
However, let's verify the test coverage for the new fallback mechanisms.
Also applies to: 51-51
✅ Verification successful
Test coverage for account normalization is sufficient
The test case comprehensively verifies the account normalization, including:
- Account name with currency suffix
- IBAN mask extraction
- Type mapping
- Institution details preservation
Since the implementation inherits normalization from the Fallback class, core fallback mechanisms are covered in the parent class tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for account name fallbacks ast-grep --pattern 'expect($_.normalizeAccount($_)).toMatchInlineSnapshot'Length of output: 531
Script:
#!/bin/bash # Get test file content echo "=== Test File Content ===" cat src/app-gocardless/banks/tests/sandboxfinance_sfin0000.spec.js # Get implementation file content echo -e "\n=== Implementation File Content ===" cat src/app-gocardless/banks/sandboxfinance_sfin0000.jsLength of output: 6088
src/app-gocardless/bank-factory.js (2)
1-19: LGTM! Consistent naming convention applied.The import statements follow a standardized underscore-based naming pattern, improving code consistency.
Line range hint
78-81: Verify the fallback mechanism.The fallback to
IntegrationBankis correctly implemented. However, let's verify that all bank handlers properly extend or implement the required interface.src/app-gocardless/banks/tests/mbank_retail_brexplpw.spec.js (1)
1-1: LGTM! Test expectations updated correctly.The import path and account name format have been standardized to include the currency code, maintaining consistency with other bank handlers.
Also applies to: 68-68
src/app-gocardless/banks/tests/ing_pl_ingbplpw.spec.js (1)
1-1: LGTM! Consistent with standardization.The changes align with the standardization effort across bank handlers.
Also applies to: 69-69
src/app-gocardless/banks/tests/ing_ingddeff.spec.js (1)
1-1: LGTM! Standardization applied consistently.Import path and account name format updated to match the project-wide standardization.
Also applies to: 64-64
New fields added to integration-bank as defaults are documented as:
