SSK_DUSSELDORF_DUSSDEDDXXX: remove non-booked transactions from import #553
Conversation
actual-github-bot
bot
changed the title
DennaGherlyn
changed the title
|
Warning Rate limit exceeded@DennaGherlyn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces changes to 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 (2)
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (2)
10-16: LGTM! Consider adding debug logging.The implementation effectively addresses the issue with unbooked transactions containing placeholder text. The comments clearly explain the rationale.
Consider adding debug logging when skipping unbooked transactions to aid troubleshooting:
if (!_booked) { + console.debug('Skipping unbooked transaction:', transaction.id); return null; }
Line range hint
26-29: Improve string concatenation logic.When concatenating
additionalInformation, the current code adds a leading space even whenremittanceInformationUnstructuredis null. Consider using string trimming to avoid unnecessary spaces.if (transaction.additionalInformation) remittanceInformationUnstructured = - (remittanceInformationUnstructured ?? '') + - ' ' + - transaction.additionalInformation; + [remittanceInformationUnstructured, transaction.additionalInformation] + .filter(Boolean) + .join(' ');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/553.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js (1)
15-15: Fix typo in test data.There's a typo in
aditionalwhich should beadditional.- additionalInformation: 'some aditional information', + additionalInformation: 'some additional information',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(1 hunks)src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js
[warning] 24-37: Code coverage warning: Branch coverage is only 70%. Consider adding more test cases to cover all code branches.
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js
[error] 48-48: Test failure: Expected payeeName to be 'a useful creditor name' but received 'A Useful Creditor Name'. Case sensitivity mismatch in creditor name normalization.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (2)
src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (2)
10-20: LGTM! Well-documented unbooked transaction handling.The implementation aligns perfectly with the PR objectives. The comments clearly explain why unbooked transactions are skipped, which helps future maintainers understand the rationale.
29-34: Consider adding test cases for empty additionalInformation.The
.filter(Boolean)handles falsy values well, but the branch coverage warning suggests we need more test cases.Add test cases for:
- When
additionalInformationis an empty string- When
additionalInformationis undefined- When both
remittanceInformationUnstructuredandadditionalInformationare emptyit('handles empty additional information', () => { const emptyAdditionalInfo = { ...bookedTransactionOne, additionalInformation: '' }; const undefinedAdditionalInfo = { ...bookedTransactionOne, additionalInformation: undefined }; const allEmpty = { ...bookedTransactionOne, remittanceInformationUnstructured: '', additionalInformation: '' }; // Add assertions for each case });✅ Verification successful
The review comment is valid - test coverage can be improved
- Examined test file
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js- Confirmed no existing test cases for empty/undefined additionalInformation scenarios
- The suggested test cases would improve edge case coverage while matching the existing test style
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find test files related to ssk_dusseldorf_dussdeddxxx fd "ssk_dusseldorf_dussdeddxxx\.(test|spec)\.js$" || fd "__tests__.*ssk_dusseldorf_dussdeddxxx\.js$" # If test file exists, check its content for existing test cases rg -l "ssk_dusseldorf_dussdeddxxx" --type js | xargs rg -A 10 "it\(.*additionalInformation.*\)" || trueLength of output: 220
Script:
#!/bin/bash # Get the content of the test file cat src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.jsLength of output: 2379
| it('returns null for unbooked transactions', () => { | ||
| expect( | ||
| SskDusseldorfDussdeddxxx.normalizeTransaction(unbookedTransaction, false), | ||
| ).toBeNull; | ||
| }); |
There was a problem hiding this comment.
Fix incorrect assertion method.
The test for unbooked transactions has two issues:
.toBeNullis a matcher function but it's being referenced instead of called- The test doesn't verify the debug log message
- ).toBeNull;
+ ).toBeNull();
+ expect(console.debug).toHaveBeenCalledWith(
+ 'Skipping unbooked transaction:',
+ unbookedTransaction.transactionId
+ );Add this setup at the beginning of the test:
beforeEach(() => {
jest.spyOn(console, 'debug').mockImplementation();
});
afterEach(() => {
jest.restoreAllMocks();
});
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js (2)
15-15: Fix typo in test data.There's a typo in the
additionalInformationfield: "aditional" should be "additional".- additionalInformation: 'some aditional information', + additionalInformation: 'some additional information',Also applies to: 29-29
56-71: Add test cases for edge scenarios.To improve test coverage, consider adding the following test cases:
- Test with undefined
_bookedparameter- Test with transaction missing
transactionIdit('returns null when _booked parameter is undefined', () => { expect( SskDusseldorfDussdeddxxx.normalizeTransaction(unbookedTransaction) ).toBeNull(); }); it('handles transaction without transactionId gracefully', () => { const transactionWithoutId = { ...unbookedTransaction }; delete transactionWithoutId.transactionId; expect( SskDusseldorfDussdeddxxx.normalizeTransaction(transactionWithoutId, false) ).toBeNull(); expect(console.debug).toHaveBeenCalledWith( 'Skipping unbooked transaction:', undefined ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (1)
src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js (1)
67-71: Fix incorrect assertion and add debug log verification.The test for unbooked transactions needs two fixes:
.toBeNullis a matcher function but it's being referenced instead of called- The test should verify the debug log message
+ beforeEach(() => { + jest.spyOn(console, 'debug').mockImplementation(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + it('returns null for unbooked transactions', () => { expect( SskDusseldorfDussdeddxxx.normalizeTransaction(unbookedTransaction, false), - ).toBeNull; + ).toBeNull(); + expect(console.debug).toHaveBeenCalledWith( + 'Skipping unbooked transaction:', + unbookedTransaction.transactionId + ); });
The "Stadtsparkasse Düsseldorf" fills uncleared transactions with placeholder text. This text is not useful for human eyes or rule mapping. When the transaction is cleared the bank replaces the text with the correct payee and updates the notes. These don't get picked up on the next import, because the fields are already filled, therefore I removed the import of uncleared transactions.