Add support for SWEDBANK_HABALV22 transaction date #490
Conversation
dmednis
force-pushed
the
swedbank-habalv22
branch
from
3816985
to
442901d
Compare
dmednis
force-pushed
the
swedbank-habalv22
branch
from
442901d
to
c8abf56
Compare
dmednis
changed the title
actual-github-bot
bot
changed the title
WalkthroughThis pull request introduces a new bank entity, 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 (3)
src/app-gocardless/banks/swedbank-habalv22.js (2)
1-1: Consider using a more descriptive name for the date-fns import.The single-letter import name 'd' reduces code readability. Consider using a more descriptive name like
dateFnsordateUtils.-import d from 'date-fns'; +import dateFns from 'date-fns';
13-15: Enhance documentation for transaction date normalization.The current comment could be more detailed about:
- The expected format of remittanceInformationUnstructured
- The transformation being performed (dd.MM.yyyy → yyyy-MM-dd)
- Why this normalization is necessary
/** - * The actual transaction date for card transactions is only available in the remittanceInformationUnstructured field when the transaction is booked. + * For card transactions, the actual transaction date is extracted from the remittanceInformationUnstructured + * field when the transaction is booked. The field contains a string like "PIRKUMS 1234 23.04.2024", + * from which we extract and normalize the date (dd.MM.yyyy) to ISO format (yyyy-MM-dd). + * This is necessary because the standard booking date may not reflect the actual transaction date. */src/app-gocardless/banks/tests/swedbank-habalv22.spec.js (1)
4-17: Consider using past dates in test data.The mock transaction data uses future dates (October 2024). While this doesn't affect functionality, it's generally better practice to use past dates in test data to avoid confusion and potential test failures when these dates actually arrive.
- transactionId: '2024102900000000-1', - bookingDate: '2024-10-29', - valueDate: '2024-10-29', + transactionId: '2023102900000000-1', + bookingDate: '2023-10-29', + valueDate: '2023-10-29',And update the remittanceInformationUnstructured accordingly:
- 'PIRKUMS 424242******4242 28.10.2024 22.99 EUR (111111) SOME CREDITOR NAME', + 'PIRKUMS 424242******4242 28.10.2023 22.99 EUR (111111) SOME CREDITOR NAME',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/490.mdis excluded by!**/*.md
📒 Files selected for processing (3)
src/app-gocardless/bank-factory.js(2 hunks)src/app-gocardless/banks/swedbank-habalv22.js(1 hunks)src/app-gocardless/banks/tests/swedbank-habalv22.spec.js(1 hunks)
🔇 Additional comments (6)
src/app-gocardless/banks/swedbank-habalv22.js (3)
5-11: LGTM! Bank configuration is well-structured.
The configuration properly extends the fallback implementation and includes appropriate institution identification.
16-31: Verify the impact on valueDate field.
The PR objectives mention issues with both bookedDate and valueDate, but the implementation only updates bookingDate. Should valueDate also be updated?
Let's check how other bank implementations handle this:
#!/bin/bash
# Search for other bank implementations that modify valueDate
ast-grep --pattern 'normalizeTransaction($transaction, $_) {
$$$
valueDate
$$$
}'17-19: Consider making the date extraction pattern more robust.
The current regex pattern might be too rigid:
- It assumes "PIRKUMS" is always present and spelled exactly
- It doesn't handle potential variations in spacing
- It might miss valid dates in differently formatted strings
Let's check for other possible formats in the codebase:
✅ Verification successful
The previous search might have failed due to regex syntax. Let's try a broader search to understand:
- How remittance information is processed across the codebase
- What other bank-specific transaction patterns exist
- What test cases reveal about actual transaction data formats
Based on the test file output and code search results, I can now generate a final response:
The current date extraction pattern is appropriate for this specific bank's format.
The test file swedbank-habalv22.spec.js shows that:
- The pattern specifically handles Swedbank's card transaction format:
'PIRKUMS 424242******4242 28.10.2024 22.99 EUR (111111) SOME CREDITOR NAME' - The code correctly falls back to default date handling when the pattern doesn't match
- Test cases cover various scenarios including:
- Valid card transactions with "PIRKUMS" prefix
- Non-card transactions with different formats
- Cases with null remittance information
The pattern is intentionally rigid as it's specifically designed for Swedbank's standardized card transaction format, and the code gracefully handles cases where the pattern doesn't match.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar transaction information patterns
rg -i "pirkums|transaction.*\d{2}\.\d{2}\.\d{4}" --type js
Length of output: 89
Script:
#!/bin/bash
# Search for transaction date patterns in the codebase
rg -A 5 "remittanceInformationUnstructured.*match" --type js
# Search for files containing transaction processing logic
fd -e js -e ts "bank|transaction" --exec grep -l "remittanceInformation"
# Search for test files that might contain sample transaction data
fd -e test.js -e spec.js "bank|transaction" --exec cat {}
Length of output: 16241
src/app-gocardless/banks/tests/swedbank-habalv22.spec.js (1)
1-69: Verify the transaction date extraction implementation.
While the tests look good, let's verify the actual implementation of the date extraction logic.
✅ Verification successful
Implementation correctly handles card transaction date extraction
The implementation aligns perfectly with the test cases:
- For card transactions, it extracts the actual transaction date from
remittanceInformationUnstructuredusing regex pattern/PIRKUMS [\d*]+ (\d{2}.\d{2}.\d{4})/ - For non-card transactions (when pattern doesn't match), it falls back to the default behavior
- The date parsing and formatting is handled correctly using date-fns library
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of normalizeTransaction in swedbank-habalv22.js
# Test: Check the date extraction implementation
ast-grep --pattern 'normalizeTransaction(transaction, booked) {
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try to find the implementation file first
fd swedbank-habalv22.js
# Then we'll read its content
cat src/app-gocardless/banks/swedbank-habalv22.js
Length of output: 1014
src/app-gocardless/bank-factory.js (2)
29-29: LGTM! Clean integration of the new bank.
The addition of SwedbankHabaLV22 follows the established patterns and maintains proper alphabetical ordering in both the imports and banks array.
Also applies to: 60-60
29-29: Verify the transaction date handling implementation.
The PR objectives mention fixing inaccuracies in bookedDate and valueDate fields. Let's verify the implementation of the date handling logic.
Would you like me to help review the date handling implementation in detail once you've completed the WIP changes?
Also applies to: 60-60
✅ Verification successful
Transaction date handling implementation has been verified and is working correctly
The implementation correctly handles both card and non-card transactions:
- For card transactions, it extracts the actual transaction date from
remittanceInformationUnstructuredfield using a regex pattern - For non-card transactions, it falls back to using the provided
bookingDate - The implementation is well-tested with comprehensive test cases covering both scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of transaction date handling in SwedbankHabaLV22
# Test 1: Check the normalizeTransaction method implementation
echo "Checking normalizeTransaction implementation..."
ast-grep --pattern 'normalizeTransaction($$$) {
$$$
bookedDate
$$$
}' src/app-gocardless/banks/swedbank-habalv22.js
# Test 2: Check for date-fns usage in the implementation
echo "Checking date handling utilities..."
rg -A 3 "from 'date-fns'" src/app-gocardless/banks/swedbank-habalv22.js
# Test 3: Check test coverage for date handling
echo "Checking test coverage..."
rg -A 5 "test\(.*date" src/app-gocardless/banks/tests/swedbank-habalv22.spec.js
Length of output: 606
Script:
#!/bin/bash
# Let's try a different approach to verify the date handling implementation
# Check the actual implementation of transaction normalization
echo "Checking transaction normalization implementation..."
ast-grep --pattern 'class SwedbankHabaLV22 {
$$$
normalizeTransaction($$$) {
$$$
}
$$$
}'
# Look for any date-related transformations in the file
echo "Checking date-related transformations..."
rg -A 5 "date|Date" src/app-gocardless/banks/swedbank-habalv22.js
# Check test file content for date-related assertions
echo "Checking test assertions..."
cat src/app-gocardless/banks/tests/swedbank-habalv22.spec.js
Length of output: 3358
| if (dateMatch) { | ||
| const extractedDate = d.parse(dateMatch[1], 'dd.MM.yyyy', new Date()); | ||
|
|
||
| return Fallback.normalizeTransaction( | ||
| { ...transaction, bookingDate: d.format(extractedDate, 'yyyy-MM-dd') }, | ||
| booked, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling for date parsing.
The current implementation has several potential issues:
- No validation of the extracted date string
- Silent failures in date parsing
- No handling of invalid dates
Consider implementing this more robust version:
if (dateMatch) {
+ const dateStr = dateMatch[1];
+ if (!dateStr) {
+ return Fallback.normalizeTransaction(transaction, booked);
+ }
- const extractedDate = d.parse(dateMatch[1], 'dd.MM.yyyy', new Date());
+ const extractedDate = dateFns.parse(dateStr, 'dd.MM.yyyy', new Date());
+ if (!dateFns.isValid(extractedDate)) {
+ console.warn(`Invalid date found in transaction: ${dateStr}`);
+ return Fallback.normalizeTransaction(transaction, booked);
+ }
return Fallback.normalizeTransaction(
- { ...transaction, bookingDate: d.format(extractedDate, 'yyyy-MM-dd') },
+ { ...transaction, bookingDate: dateFns.format(extractedDate, 'yyyy-MM-dd') },
booked,
);
}📝 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.
| if (dateMatch) { | |
| const extractedDate = d.parse(dateMatch[1], 'dd.MM.yyyy', new Date()); | |
| return Fallback.normalizeTransaction( | |
| { ...transaction, bookingDate: d.format(extractedDate, 'yyyy-MM-dd') }, | |
| booked, | |
| ); | |
| } | |
| if (dateMatch) { | |
| const dateStr = dateMatch[1]; | |
| if (!dateStr) { | |
| return Fallback.normalizeTransaction(transaction, booked); | |
| } | |
| const extractedDate = dateFns.parse(dateStr, 'dd.MM.yyyy', new Date()); | |
| if (!dateFns.isValid(extractedDate)) { | |
| console.warn(`Invalid date found in transaction: ${dateStr}`); | |
| return Fallback.normalizeTransaction(transaction, booked); | |
| } | |
| return Fallback.normalizeTransaction( | |
| { ...transaction, bookingDate: dateFns.format(extractedDate, 'yyyy-MM-dd') }, | |
| booked, | |
| ); | |
| } |
| it('extracts card transaction date', () => { | ||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(cardTransaction, true).bookingDate, | ||
| ).toEqual('2024-10-28'); | ||
|
|
||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(cardTransaction, true).date, | ||
| ).toEqual('2024-10-28'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and improve test description.
While the test correctly validates the date extraction, consider:
- Making the test description more specific (e.g., "extracts correct transaction date from remittanceInformationUnstructured for card transactions")
- Adding assertions for other normalized fields (amount, creditorName, etc.) to ensure complete transaction normalization
- it('extracts card transaction date', () => {
+ it('extracts correct transaction date from remittanceInformationUnstructured for card transactions', () => {
const normalized = SwedbankHabaLV22.normalizeTransaction(cardTransaction, true);
expect(normalized.bookingDate).toEqual('2024-10-28');
expect(normalized.date).toEqual('2024-10-28');
+ expect(normalized.amount).toEqual('-22.99');
+ expect(normalized.currency).toEqual('EUR');
+ expect(normalized.creditorName).toEqual('SOME CREDITOR NAME');
});Committable suggestion was skipped due to low confidence.
| it('normalizes non-card transactions as usual', () => { | ||
| const nonCardTransaction1 = { | ||
| ...cardTransaction, | ||
| remittanceInformationUnstructured: 'Some info', | ||
| }; | ||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction1, true) | ||
| .bookingDate, | ||
| ).toEqual('2024-10-29'); | ||
|
|
||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction1, true).date, | ||
| ).toEqual('2024-10-29'); | ||
|
|
||
| const nonCardTransaction2 = { | ||
| ...cardTransaction, | ||
| remittanceInformationUnstructured: 'PIRKUMS xxx', | ||
| }; | ||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction2, true) | ||
| .bookingDate, | ||
| ).toEqual('2024-10-29'); | ||
|
|
||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction2, true).date, | ||
| ).toEqual('2024-10-29'); | ||
|
|
||
| const nonCardTransaction3 = { | ||
| ...cardTransaction, | ||
| remittanceInformationUnstructured: null, | ||
| }; | ||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction3, true) | ||
| .bookingDate, | ||
| ).toEqual('2024-10-29'); | ||
|
|
||
| expect( | ||
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction3, true).date, | ||
| ).toEqual('2024-10-29'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor tests to reduce duplication using test.each.
The current implementation has repetitive test code. Consider using test.each to make the tests more maintainable and concise.
- it('normalizes non-card transactions as usual', () => {
- const nonCardTransaction1 = {
- ...cardTransaction,
- remittanceInformationUnstructured: 'Some info',
- };
- expect(
- SwedbankHabaLV22.normalizeTransaction(nonCardTransaction1, true)
- .bookingDate,
- ).toEqual('2024-10-29');
- // ... more repetitive code ...
- });
+ it.each([
+ ['regular text', 'Some info'],
+ ['partial card text', 'PIRKUMS xxx'],
+ ['null value', null]
+ ])('normalizes non-card transaction with %s', (_, remittanceInfo) => {
+ const transaction = {
+ ...cardTransaction,
+ remittanceInformationUnstructured: remittanceInfo,
+ };
+ const normalized = SwedbankHabaLV22.normalizeTransaction(transaction, true);
+
+ expect(normalized.bookingDate).toEqual('2024-10-29');
+ expect(normalized.date).toEqual('2024-10-29');
+ });📝 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.
| it('normalizes non-card transactions as usual', () => { | |
| const nonCardTransaction1 = { | |
| ...cardTransaction, | |
| remittanceInformationUnstructured: 'Some info', | |
| }; | |
| expect( | |
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction1, true) | |
| .bookingDate, | |
| ).toEqual('2024-10-29'); | |
| expect( | |
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction1, true).date, | |
| ).toEqual('2024-10-29'); | |
| const nonCardTransaction2 = { | |
| ...cardTransaction, | |
| remittanceInformationUnstructured: 'PIRKUMS xxx', | |
| }; | |
| expect( | |
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction2, true) | |
| .bookingDate, | |
| ).toEqual('2024-10-29'); | |
| expect( | |
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction2, true).date, | |
| ).toEqual('2024-10-29'); | |
| const nonCardTransaction3 = { | |
| ...cardTransaction, | |
| remittanceInformationUnstructured: null, | |
| }; | |
| expect( | |
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction3, true) | |
| .bookingDate, | |
| ).toEqual('2024-10-29'); | |
| expect( | |
| SwedbankHabaLV22.normalizeTransaction(nonCardTransaction3, true).date, | |
| ).toEqual('2024-10-29'); | |
| }); | |
| it.each([ | |
| ['regular text', 'Some info'], | |
| ['partial card text', 'PIRKUMS xxx'], | |
| ['null value', null] | |
| ])('normalizes non-card transaction with %s', (_, remittanceInfo) => { | |
| const transaction = { | |
| ...cardTransaction, | |
| remittanceInformationUnstructured: remittanceInfo, | |
| }; | |
| const normalized = SwedbankHabaLV22.normalizeTransaction(transaction, true); | |
| expect(normalized.bookingDate).toEqual('2024-10-29'); | |
| expect(normalized.date).toEqual('2024-10-29'); | |
| }); |
Extracts transaction date from
remittanceInformationUnstructuredfor card transactions since it is not correct in the actualbookedDateandvalueDatefields.