fix: Sabadell Bank regression, missing date field during normalization #474
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces changes to the Possibly related PRs
Suggested labels
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 (
|
davidmartos96
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/app-gocardless/banks/bancsabadell-bsabesbbb.js (1)
Line range hint
1-37: Consider reviewing date handling across all bank integrations.While this change effectively addresses the Sabadell Bank regression, it might be beneficial to:
- Review date handling in other bank integrations to ensure consistency and prevent similar issues.
- Assess any potential impacts on other parts of the system that consume normalized transaction data, especially those that rely on transaction dates.
To help with this review, you can use the following script to check date-related fields in other bank integration files:
#!/bin/bash # Description: Check date-related fields in bank integration files echo "Date-related fields in bank integration files:" rg --type js '\b(date|Date)\b' src/app-gocardless/banks/ -C 2 echo "\nNormalization methods in bank integration files:" rg --type js 'normalizeTransaction' src/app-gocardless/banks/ -C 5This will help identify any inconsistencies in date handling across different bank integrations and highlight areas that might need similar updates.
src/app-gocardless/banks/tests/bancsabadell-bsabesbbb.spec.js (3)
12-12: LGTM! Consider adding an assertion for thedatefield.The addition of the
bookingDateproperty is in line with the PR objectives. However, to ensure complete coverage, consider adding an assertion to verify that thedatefield is correctly set in the normalized transaction.You could add the following assertion:
expect(normalizedTransaction.date).toEqual('2022-05-01');
30-30: LGTM! Consider adding an assertion for thedatefield.The addition of the
bookingDateproperty is consistent with the changes in the first test case. To ensure complete coverage, consider adding an assertion to verify that thedatefield is correctly set in the normalized transaction.You could add the following assertion:
expect(normalizedTransaction.date).toEqual('2022-05-01');
41-55: Great addition! Consider adding a test for fallback behavior.This new test case effectively verifies the correct extraction of the
datefield from the transaction, addressing the PR objectives. It's a valuable addition to the test suite.To make the test suite more robust, consider adding another test case that verifies the fallback behavior when
bookingDateis not present. This would ensure that thevalueDateis used correctly in such scenarios. For example:it('extract date fallback to valueDate', () => { const transaction = { transactionAmount: { amount: '-100', currency: 'EUR' }, remittanceInformationUnstructuredArray: ['some-creditor-name'], internalTransactionId: 'd7dca139cf31d9', transactionId: '04704109322', valueDate: '2024-10-05', }; const normalizedTransaction = Sabadell.normalizeTransaction( transaction, true, ); expect(normalizedTransaction.date).toEqual('2024-10-05'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/474.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- src/app-gocardless/banks/bancsabadell-bsabesbbb.js (1 hunks)
- src/app-gocardless/banks/tests/bancsabadell-bsabesbbb.spec.js (3 hunks)
🔇 Additional comments (2)
src/app-gocardless/banks/bancsabadell-bsabesbbb.js (1)
34-34: LGTM. Please verify date field usage across all transaction types.The addition of the
datefield with a fallback mechanism (transaction.bookingDate || transaction.valueDate) addresses the regression issue mentioned in the PR objectives. This implementation looks correct and should resolve the missing date field during normalization.However, to ensure robustness:
- Please verify that this date extraction works correctly for all transaction types supported by Sabadell Bank.
- Consider adding test cases that cover scenarios where:
a) OnlybookingDateis present
b) OnlyvalueDateis present
c) BothbookingDateandvalueDateare present
d) NeitherbookingDatenorvalueDateis present (edge case)To assist in verifying the usage of
bookingDateandvalueDateacross the codebase, you can run the following script:This will help ensure that the date field usage is consistent across the codebase and that we haven't missed any other relevant date fields.
✅ Verification successful
Verified:
datefield usage is consistent across all transaction types.The addition of the
datefield with the fallback mechanism (transaction.bookingDate || transaction.valueDate) aligns with existing implementations throughout the codebase. This change effectively addresses the regression issue and maintains uniformity across all transaction handling modules.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of bookingDate and valueDate in the codebase # Search for bookingDate usage echo "Usage of bookingDate:" rg --type js 'bookingDate' -C 2 echo "\nUsage of valueDate:" rg --type js 'valueDate' -C 2 echo "\nCheck for any other date fields that might be relevant:" rg --type js '\b\w+Date\b' -C 2Length of output: 157218
src/app-gocardless/banks/tests/bancsabadell-bsabesbbb.spec.js (1)
Line range hint
1-55: Overall, the changes effectively address the regression issue.The updates to the existing test cases and the addition of the new "extract date" test case align well with the PR objectives. These changes provide good coverage for the date field normalization functionality.
To further enhance the test suite:
- Consider adding assertions for the
datefield in the existing test cases.- Add a test case to verify the fallback behavior when
bookingDateis not present.These additions would ensure comprehensive coverage of the date normalization logic.
Fixes a regression from a PR of mine from some weeks ago #445. I forgot to include the
datefield in the normalized transaction, which fails when syncing.