Add custom handler for the German bank Rheinhessen Sparkasse to the GoCardless app #454
Conversation
…oCardless app. Following the guide and using the template added the code, similar to other handlers, that should match the transaction as seen in the logs when syncing my bank through Actual. The only thing that had to be added was the notes about the transaction are stored under remittanceInformationStructured and not under remittanceInformationUnstructured. Testing still pending.
actual-github-bot
bot
changed the title
Replaced the indentation from 4 spaces to 2, to fix linting error as recommended by the yarn test. Added the release notes file 454.md.
DirgoSalga
changed the title
WalkthroughThe pull request introduces a new bank module, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Co-authored-by: Matt Fiddaman <[email protected]>
Co-authored-by: Matt Fiddaman <[email protected]>
Co-authored-by: Matt Fiddaman <[email protected]>
Co-authored-by: Matt Fiddaman <[email protected]>
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
upcoming-release-notes/454.mdis excluded by!**/*.md
Files selected for processing (2)
- src/app-gocardless/bank-factory.js (2 hunks)
- src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js
Additional context used
GitHub Check: build
src/app-gocardless/bank-factory.js
[failure] 26-26:
Cannot find module './banks/spk-worms-alzey-ried-malade51wor.js' or its corresponding type declarations.
Additional comments not posted (2)
src/app-gocardless/bank-factory.js (2)
26-26: The past review comment suggesting to rename the import to match the naming convention is still applicable.Tools
GitHub Check: build
[failure] 26-26:
Cannot find module './banks/spk-worms-alzey-ried-malade51wor.js' or its corresponding type declarations.
55-55: LGTM!The new bank module is correctly added to the exported
banksarray.
Since no changes were made to the default functions, they should be deleted from this specific bank (SPK_WORMS_ALZEY_RIED_MALADE51WOR).
Remove import of unsed variables.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/app-gocardless/banks/spk-worms-alzey-ried-malade51wor.js (1)
1-30: Overall implementation looks good, next steps for completionThe implementation of the custom handler for the Rheinhessen Sparkasse bank is well-structured and aligns with the PR objectives. Key points:
- The handler extends the fallback integration correctly.
- Bank-specific details (institutionIds, accessValidForDays) are properly set.
- The
normalizeTransactionfunction processes transactions as expected, including the use ofremittanceInformationStructuredas mentioned in the PR description.To complete this PR:
- Address the suggested changes in the previous comments.
- Implement the pending tests mentioned in the PR description.
- Write the release notes as reminded in the PR description.
- Consider adding comments to explain any bank-specific logic, especially in the
normalizeTransactionfunction.For future improvements, consider:
- Implementing error handling for edge cases in transaction processing.
- Adding logging for important operations to aid in debugging and monitoring.
- Creating a test suite that covers various transaction scenarios specific to this bank.
🧰 Tools
GitHub Check: lint
[failure] 3-3:
'printIban' is defined but never used
[failure] 3-3:
'amountToInteger' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/app-gocardless/banks/spk-worms-alzey-ried-malade51wor.js (1 hunks)
🧰 Additional context used
GitHub Check: lint
src/app-gocardless/banks/spk-worms-alzey-ried-malade51wor.js
[failure] 3-3:
'printIban' is defined but never used
[failure] 3-3:
'amountToInteger' is defined but never used
🔇 Additional comments not posted (1)
src/app-gocardless/banks/spk-worms-alzey-ried-malade51wor.js (1)
6-12: LGTM: Exported object correctly configuredThe exported object is properly set up:
- It extends the Fallback integration as expected.
- The
institutionIdscorrectly identifies the Rheinhessen Sparkasse bank (SPK_WORMS_ALZEY_RIED_MALADE51WOR).- The
accessValidForDaysis set to 90, which is a reasonable duration for access validity.This implementation aligns well with the PR objectives.
| import Fallback from './integration-bank.js'; | ||
|
|
||
| import { printIban, amountToInteger } from '../utils.js'; | ||
| import { formatPayeeName } from '../../util/payee-name.js'; |
There was a problem hiding this comment.
Remove unused imports to improve code cleanliness
The static analysis tool correctly identified that printIban and amountToInteger are imported but not used in this file. To keep the code clean and prevent potential confusion, it's recommended to remove unused imports.
Apply this diff to remove the unused imports:
import Fallback from './integration-bank.js';
-import { printIban, amountToInteger } from '../utils.js';
import { formatPayeeName } from '../../util/payee-name.js';📝 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.
| import Fallback from './integration-bank.js'; | |
| import { printIban, amountToInteger } from '../utils.js'; | |
| import { formatPayeeName } from '../../util/payee-name.js'; | |
| import Fallback from './integration-bank.js'; | |
| import { formatPayeeName } from '../../util/payee-name.js'; |
🧰 Tools
GitHub Check: lint
[failure] 3-3:
'printIban' is defined but never used
[failure] 3-3:
'amountToInteger' is defined but never used
| normalizeTransaction(transaction, _booked) { | ||
| const date = transaction.bookingDate || transaction.valueDate; | ||
| if (!date) { | ||
| return null; | ||
| } | ||
|
|
||
| transaction.remittanceInformationUnstructured = | ||
| transaction.remittanceInformationUnstructured ?? | ||
| transaction.remittanceInformationStructured ?? | ||
| transaction.remittanceInformationStructuredArray?.join(' '); | ||
| return { | ||
| ...transaction, | ||
| payeeName: formatPayeeName(transaction), | ||
| date: transaction.bookingDate || transaction.valueDate, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Refactor to avoid mutating the transaction object
The normalizeTransaction function is mostly well-implemented, but there's an issue with directly mutating the transaction object. This was correctly identified in a previous review comment. Let's address this to improve the function's immutability.
Apply this diff to refactor the function:
normalizeTransaction(transaction, _booked) {
const date = transaction.bookingDate || transaction.valueDate;
if (!date) {
return null;
}
- transaction.remittanceInformationUnstructured =
- transaction.remittanceInformationUnstructured ??
- transaction.remittanceInformationStructured ??
- transaction.remittanceInformationStructuredArray?.join(' ');
return {
...transaction,
+ remittanceInformationUnstructured:
+ transaction.remittanceInformationUnstructured ??
+ transaction.remittanceInformationStructured ??
+ transaction.remittanceInformationStructuredArray?.join(' '),
payeeName: formatPayeeName(transaction),
date: transaction.bookingDate || transaction.valueDate,
};
},This change ensures that we're not modifying the original transaction object, which aligns better with immutability principles and prevents potential side effects.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| normalizeTransaction(transaction, _booked) { | |
| const date = transaction.bookingDate || transaction.valueDate; | |
| if (!date) { | |
| return null; | |
| } | |
| transaction.remittanceInformationUnstructured = | |
| transaction.remittanceInformationUnstructured ?? | |
| transaction.remittanceInformationStructured ?? | |
| transaction.remittanceInformationStructuredArray?.join(' '); | |
| return { | |
| ...transaction, | |
| payeeName: formatPayeeName(transaction), | |
| date: transaction.bookingDate || transaction.valueDate, | |
| }; | |
| }, | |
| normalizeTransaction(transaction, _booked) { | |
| const date = transaction.bookingDate || transaction.valueDate; | |
| if (!date) { | |
| return null; | |
| } | |
| return { | |
| ...transaction, | |
| remittanceInformationUnstructured: | |
| transaction.remittanceInformationUnstructured ?? | |
| transaction.remittanceInformationStructured ?? | |
| transaction.remittanceInformationStructuredArray?.join(' '), | |
| payeeName: formatPayeeName(transaction), | |
| date: transaction.bookingDate || transaction.valueDate, | |
| }; | |
| }, |
Following the guide and using the template, I added the code, similar to other handlers, that should match the transaction as seen in the logs when syncing my bank (Rheinhessen Sparkasse) through Actual.
The only thing that had to be modified was that the notes about the transaction are stored under remittanceInformationStructured and not under remittanceInformationUnstructured.
Testing still pending.