DEGIRO: Add Spanish translations for platform fees and money-market fund conversions#328
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDeGiroConverterV2 and DeGiroConverterV3 now recognize additional Spanish transaction descriptions from DEGIRO. Both converters extend ignored-record detection with money-market fund variants. DeGiroConverterV2 adds dividend-income matching for Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nto de capital' - Add 'retención del dividendo' to isTransactionFeeRecord (V2 + V3) so the Spanish dividend-withholding-tax row pairs with the dividend record as a fee instead of being imported as a duplicate dividend activity. - Add 'rendimiento de capital' to isDividendRecord (V2) so Spanish capital return records are recognized as dividends, mirroring the existing English 'capital return' keyword. V3 routes non-buy/sell records through mapDividendRecord by default, so no change is required there.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/converters/degiroConverterV2.ts (1)
260-261: ⚡ Quick winAdd table-driven regression tests for the new Spanish classifier keywords.
These string-match rules are easy to regress; a compact fixture-based test for each new phrase (
fondos del mercado monetario,rendimiento de capital,y/o,retención del dividendo,comisión de conectividad) would lock behavior and prevent locale drift between V2/V3.Also applies to: 432-432, 446-446, 453-453
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/converters/degiroConverterV2.ts` around lines 260 - 261, Add compact table-driven Jest tests for the new Spanish classifier keywords in src/converters/degiroConverterV2.ts: create a data-driven fixture in degiroConverterV2.test.ts that asserts the converter's classification function (use the module's exported function, e.g., degiroConverterV2.classifyInstrumentType or the specific validator used for keyword matching) returns the expected category for each phrase ("fondos del mercado monetario", "rendimiento de capital", "y/o", "retención del dividendo", "comisión de conectividad") and include entries for the other occurrences referenced (lines with the same phrases around 432, 446, 453) so future changes fail the test; iterate over the table with meaningful case/whitespace variants and assert exact string-match behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/converters/degiroConverterV2.ts`:
- Around line 260-261: Add compact table-driven Jest tests for the new Spanish
classifier keywords in src/converters/degiroConverterV2.ts: create a data-driven
fixture in degiroConverterV2.test.ts that asserts the converter's classification
function (use the module's exported function, e.g.,
degiroConverterV2.classifyInstrumentType or the specific validator used for
keyword matching) returns the expected category for each phrase ("fondos del
mercado monetario", "rendimiento de capital", "y/o", "retención del dividendo",
"comisión de conectividad") and include entries for the other occurrences
referenced (lines with the same phrases around 432, 446, 453) so future changes
fail the test; iterate over the table with meaningful case/whitespace variants
and assert exact string-match behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e7578f9-ec12-4b7b-b49f-c3a454546cdb
📒 Files selected for processing (2)
src/converters/degiroConverterV2.tssrc/converters/degiroConverterV3.ts
Compact it.each table per converter, asserting each of the new Spanish
keywords ('fondos del mercado monetario', 'comisión de conectividad',
'y/o', 'retención del dividendo', and 'rendimiento de capital' in V2)
is recognized by its classifier in both lowercase and uppercase form,
locking in the lowercase normalization behavior of the existing
substring-match logic.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/converters/degiroConverterV2.test.ts (1)
151-199: 💤 Low valueSolid coverage of the new Spanish classifiers; consider locking in negative cases.
The factory correctly populates
date/time/product/isinand a non-emptydescription, so theisIgnoredRecordearly-return guards are bypassed and the keyword path is actually exercised. The buy/sell ([true]) vs dividend ([false]) contexts forisTransactionFeeRecordare also handled correctly given the empty defaultorderId.One gap: every row asserts
expected === true, so the suite only proves these keywords match — it can't catch future over-matching (the keywords are short substrings, e.g.y/o). A couple offalserows (e.g. an unrelated description, or a dividend-context record that has anorderId) would harden the suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/converters/degiroConverterV2.test.ts` around lines 151 - 199, Tests only assert positive matches, so add negative cases to prevent over-matching: extend the table in the Spanish keyword classification test (the it.each block using record(...) and newSut()) with rows where expected === false — e.g., a description that contains the short substrings in an unrelated context (to test isIgnoredRecord/isPlatformFees/isDividendRecord do not false-positive) and a transaction-fee check with the opposite context (call isTransactionFeeRecord with [true] when it should be false because orderId is present, and with [false] when orderId is non-empty) — reference the classifier methods isIgnoredRecord, isPlatformFees, isTransactionFeeRecord, isDividendRecord and the helpers record() and newSut() so you add a couple of rows asserting false to harden the suite.src/converters/degiroConverterV3.test.ts (1)
182-226: 💤 Low valueTest block mirrors V2 correctly.
Factory state bypasses the
isIgnoredRecordguards, and the buy/sell vs dividendisTransactionFeeRecordcontexts are exercised correctly. Note the V3 suite intentionally omits theisDividendRecordcase (V2-only per PR), which is consistent since V3 routes non-buy/sell records throughmapDividendRecord.Same optional observation as V2: all rows assert
true, so adding a few negative cases would guard against future over-matching on the short keywords.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/converters/degiroConverterV3.test.ts` around lines 182 - 226, Tests only assert true for Spanish keyword classifiers, leaving no negative cases to prevent over-matching; add a few negative test rows to the it.each matrix that assert false for each classifier (isIgnoredRecord, isPlatformFees, isTransactionFeeRecord) using descriptions that are similar but lack the keyword (and include mixed case variants) so the calls to sut[method](record(description), ...extraArgs) return false; update the dataset in src/converters/degiroConverterV3.test.ts (the it.each array) to include these negative rows alongside the existing positives to guard against future over-matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/converters/degiroConverterV2.test.ts`:
- Around line 151-199: Tests only assert positive matches, so add negative cases
to prevent over-matching: extend the table in the Spanish keyword classification
test (the it.each block using record(...) and newSut()) with rows where expected
=== false — e.g., a description that contains the short substrings in an
unrelated context (to test isIgnoredRecord/isPlatformFees/isDividendRecord do
not false-positive) and a transaction-fee check with the opposite context (call
isTransactionFeeRecord with [true] when it should be false because orderId is
present, and with [false] when orderId is non-empty) — reference the classifier
methods isIgnoredRecord, isPlatformFees, isTransactionFeeRecord,
isDividendRecord and the helpers record() and newSut() so you add a couple of
rows asserting false to harden the suite.
In `@src/converters/degiroConverterV3.test.ts`:
- Around line 182-226: Tests only assert true for Spanish keyword classifiers,
leaving no negative cases to prevent over-matching; add a few negative test rows
to the it.each matrix that assert false for each classifier (isIgnoredRecord,
isPlatformFees, isTransactionFeeRecord) using descriptions that are similar but
lack the keyword (and include mixed case variants) so the calls to
sut[method](record(description), ...extraArgs) return false; update the dataset
in src/converters/degiroConverterV3.test.ts (the it.each array) to include these
negative rows alongside the existing positives to guard against future
over-matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b8a8aac-3127-42ea-8cbf-4721cc245670
📒 Files selected for processing (2)
src/converters/degiroConverterV2.test.tssrc/converters/degiroConverterV3.test.ts
…tests Extend the it.each tables in both degiroConverterV2.test.ts and degiroConverterV3.test.ts with negative rows that assert each classifier returns false for unrelated phrases (real Compra/Venta/Dividendo descriptions) and for cross-classifier overlap (the Spanish trade-fee phrase does not match isPlatformFees or isDividendRecord). Also add a dedicated test for the isTransactionFeeRecord dividend-context orderId guard, asserting the early-return path returns false even for a real 'Retención del dividendo' phrase when an orderId is present.
1 participant
Added
DEGIRO Spanish-locale recognition keywords across V2 and V3 converters:
comisión de conectividad→isPlatformFees. Spanish-language exchange connectivity fees (Comisión de conectividad con el mercado YYYY (...)) are now classified as platform fees instead of failing the security lookup.fondos del mercado monetario→isIgnoredRecord. DEGIRO's internal money-market fund cash-sweep pseudo-transactions (Conversión fondos del mercado monetario: Compra/Venta ... @ ... EUR) are skipped, matching the existing behaviour for the Frenchfonds monétaires.y/o→isTransactionFeeRecord. Spanish trade-fee rows (Costes de transacción y/o externos de DEGIRO) now pair with their buy/sell record so the fee ends up on the trade instead of being dropped, matching the existingen/of,and/or,und/oder,e/okeywords.retención del dividendo→isTransactionFeeRecord. Spanish dividend-withholding-tax rows (Retención del dividendo) now pair with the dividend record as a fee instead of being imported as a duplicateDIVIDENDactivity (the description containeddividendand was matchingisDividendRecord).rendimiento de capital→isDividendRecord(V2 only). Spanish capital-return rows are recognised as dividends, mirroring the existing Englishcapital returnkeyword. V3 routes non-buy/sell records throughmapDividendRecordby default, so no change is needed there.Fixes
An error occurred trying to retrieve an empty symbolfor market connectivity fee rows.quantity: 0,unitPrice: 0,currency: "") for money-market fund internal conversion rows, which then failed Ghostfolio import validation withcurrency must be a valid ISO4217 currency code.Record <ISIN> with currency EUR was skipped because it could not be matched to a valid transaction!for every Spanish trade fee row, with the fee silently dropped from the corresponding buy/sell activity.DIVIDENDactivities per dividend payment — one forDividendo, one forRetención del dividendo— instead of a single dividend with the withholding tax recorded as the fee.Rendimiento de capitalrows.Checklist
it.eachtable per converter indegiroConverterV2.test.tsanddegiroConverterV3.test.tsasserting each new Spanish keyword is recognized by its classifier, with lowercase and uppercase variants to lock in the lowercase-normalization behaviour.Related issue (if applicable)
Fixes #..
Notes
This follows the existing pattern in both converters where the platform-fee, ignored-record, transaction-fee, and dividend-keyword lists already contain entries in Dutch, English, Italian, German, Portuguese, and French. Spanish was the only major DEGIRO-supported locale missing from these lists, and these were the specific terms encountered when running a real Spanish
Account.csvexport end-to-end.Reproduced and verified locally on a 1398-row Spanish DEGIRO
Account.csv:FEEactivitiescurrency: "") that broke Ghostfolio importfonds monétaires)was skipped because it could not be matchedwarningDIVIDENDactivities per payment (one for the gross, one for the withholding)DIVIDENDactivity with the withholding as the feeRendimiento de capitalrowsDIVIDENDactivityEnd-to-end the converter now produces a valid Ghostfolio import file with no skip warnings, no empty currencies, and all activities passing ISO4217 validation.