Add FinecoBank converter - #314
Open
got3nks wants to merge 3 commits into
Open
Conversation
Add support for importing transaction exports from FinecoBank (Italian broker). Handles buy/sell, dividends, bond coupons (cedole), stock splits (aumento capitale), and bond maturities (rimborso). Supports both comma and semicolon delimited CSVs with Italian number formatting. Bonds are converted with quantity/100 convention for Ghostfolio.
WalkthroughAdds FinecoBank support by introducing a FinecoRecord model, a new FinecoConverter class (with parsing, record filtering, order-type detection, and activity creation), test coverage for many CSV and error scenarios, registration of Fineco headers in the watcher, and registration of the converter in the converter factory. No public API signatures were changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/watcher.ts (1)
118-118: Add a semicolon Fineco header mapping for deterministic auto-detection.Only the comma header is registered right now. Since Fineco semicolon CSV is supported, adding the semicolon header key avoids fuzzy-match edge cases.
♻️ Suggested update
headers.set(`Data operazione,Data valuta,Tipo operazione,Ticker,Isin,Protocollo,Descrizione,Quantità,Importo euro,Importo Divisa,Divisa,Riferimento ordine`, "directa"); headers.set(`Operazione,Data valuta,Descrizione,Titolo,Isin,Segno,Quantita,Divisa,Prezzo,Cambio,Controvalore,Commissioni Fondi Sw/Ingr/Uscita,Commissioni Fondi Banca Corrispondente,Spese Fondi Sgr,Commissioni amministrato`, "fineco"); +headers.set(`Operazione;Data valuta;Descrizione;Titolo;Isin;Segno;Quantita;Divisa;Prezzo;Cambio;Controvalore;Commissioni Fondi Sw/Ingr/Uscita;Commissioni Fondi Banca Corrispondente;Spese Fondi Sgr;Commissioni amministrato`, "fineco"); headers.set(`Date de transaction,Date de règlement,Type de transaction,Classe d'actif,Symbole,Description,Marché,Quantité,Prix,Devise du prix,Commission payée,Montant de l'opération,Devise du compte`, "disnat");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/watcher.ts` at line 118, The Fineco header mapping currently registers only the comma-separated header string via headers.set(...) which breaks deterministic detection for semicolon-delimited CSVs; add an additional headers.set call with the exact same header fields joined by semicolons (i.e., "Operazione;Data valuta;Descrizione;Titolo;Isin;Segno;Quantita;Divisa;Prezzo;Cambio;Controvalore;Commissioni Fondi Sw/Ingr/Uscita;Commissioni Fondi Banca Corrispondente;Spese Fondi Sgr;Commissioni amministrato") next to the existing headers.set(...) in src/watcher.ts so both comma and semicolon variants are registered for deterministic auto-detection.src/converters/finecoConverter.test.ts (2)
94-95: Avoid exact full-string assertion for parser errors.Line 94 is brittle against
csv-parsemessage changes; assert stable fragments instead.🧩 Suggested fix
- expect(err.message).toBe("An error occurred while parsing! Details: Invalid Record Length: columns length is 15, got 16 on line 2"); + expect(err.message).toContain("An error occurred while parsing!"); + expect(err.message).toContain("Invalid Record Length");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/finecoConverter.test.ts` around lines 94 - 95, The test in finecoConverter.test.ts is asserting the full csv-parse error string (expect(err.message).toBe(...)) which is brittle; update the assertion in the failing test (the expect on err.message) to check for stable fragments instead (e.g., use toContain or a regex to assert key substrings such as "Invalid Record Length" and "line 2" or "columns length") so the test verifies the important error characteristics without depending on the exact csv-parse message format.
25-149: Add explicit regression tests for semicolon + bond conversion paths.Current coverage is good, but key Fineco-specific behavior is still unpinned: semicolon-delimited input with Italian numeric formatting and the bond
quantity/100conversion branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/finecoConverter.test.ts` around lines 25 - 149, Add explicit tests to cover semicolon-delimited input with Italian number formatting and the bond quantity/100 conversion path: in src/converters/finecoConverter.test.ts add at least two new it blocks that call FinecoConverter.processFileContents (and/or readAndProcessFile) using semicolon-separated rows (header and data using ';'), numeric fields using Italian format (e.g., "1.234,56" for price/controvalore) and a bond-like record that should trigger the quantity/100 logic; use SecurityService with YahooFinanceServiceMock, assert the produced GhostfolioExport activities contain correctly parsed numeric values and that bond quantities are divided by 100 (check resulting activity.quantity/amount), and also assert no parsing errors are thrown. Ensure tests reference FinecoConverter, processFileContents/readAndProcessFile and YahooFinanceServiceMock so they exercise the intended branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/converters/finecoConverter.test.ts`:
- Around line 12-14: Replace the current test teardown that calls
jest.clearAllMocks() with a teardown that restores mocked implementations so
spies don't leak: update the afterEach block (the one invoking
jest.clearAllMocks()) to call jest.restoreAllMocks() (or call both
jest.clearAllMocks() and jest.restoreAllMocks()) so any spy on console.log and
other mocked implementations created in tests are fully restored after each
test.
In `@src/converters/finecoConverter.ts`:
- Around line 141-144: In the catch block that currently calls
this.logQueryError(record.isin, idx + 2) and returns errorCallback(err), stop
the progress renderer first by invoking this.progress.stop() (or checking for
this.progress and calling stop) before returning; update the catch in the
function containing logQueryError to call this.progress.stop() then
this.logQueryError(...) and finally return errorCallback(err) so the progress is
always cleaned up on early exit.
- Around line 238-239: Guard against zero or falsy bond quantity before dividing
when computing unitPrice and quantity in the Fineco conversion logic: check
record.quantita and if it is 0 (or falsy) set unitPrice and quantity to 0 (or an
appropriate fallback) instead of performing (record.controvalore /
record.quantita) * 100 and record.quantita / 100; update the block that assigns
unitPrice and quantity (referencing the variables unitPrice, quantity and the
fields record.controvalore and record.quantita) to perform the conditional check
and assignment so no division by zero occurs.
- Line 33: The method currently returns the result of calling errorCallback(new
Error("Could not find header row in input file!")), which violates the
void-return rule; update the code to invoke errorCallback(new Error("Could not
find header row in input file!")); on its own line and then follow it with a
bare return; so the function returns void—locate the invocation of errorCallback
in this converter (the line shown) and split it into two statements.
---
Nitpick comments:
In `@src/converters/finecoConverter.test.ts`:
- Around line 94-95: The test in finecoConverter.test.ts is asserting the full
csv-parse error string (expect(err.message).toBe(...)) which is brittle; update
the assertion in the failing test (the expect on err.message) to check for
stable fragments instead (e.g., use toContain or a regex to assert key
substrings such as "Invalid Record Length" and "line 2" or "columns length") so
the test verifies the important error characteristics without depending on the
exact csv-parse message format.
- Around line 25-149: Add explicit tests to cover semicolon-delimited input with
Italian number formatting and the bond quantity/100 conversion path: in
src/converters/finecoConverter.test.ts add at least two new it blocks that call
FinecoConverter.processFileContents (and/or readAndProcessFile) using
semicolon-separated rows (header and data using ';'), numeric fields using
Italian format (e.g., "1.234,56" for price/controvalore) and a bond-like record
that should trigger the quantity/100 logic; use SecurityService with
YahooFinanceServiceMock, assert the produced GhostfolioExport activities contain
correctly parsed numeric values and that bond quantities are divided by 100
(check resulting activity.quantity/amount), and also assert no parsing errors
are thrown. Ensure tests reference FinecoConverter,
processFileContents/readAndProcessFile and YahooFinanceServiceMock so they
exercise the intended branches.
In `@src/watcher.ts`:
- Line 118: The Fineco header mapping currently registers only the
comma-separated header string via headers.set(...) which breaks deterministic
detection for semicolon-delimited CSVs; add an additional headers.set call with
the exact same header fields joined by semicolons (i.e., "Operazione;Data
valuta;Descrizione;Titolo;Isin;Segno;Quantita;Divisa;Prezzo;Cambio;Controvalore;Commissioni
Fondi Sw/Ingr/Uscita;Commissioni Fondi Banca Corrispondente;Spese Fondi
Sgr;Commissioni amministrato") next to the existing headers.set(...) in
src/watcher.ts so both comma and semicolon variants are registered for
deterministic auto-detection.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
samples/fineco-export.csvis excluded by!**/*.csv
📒 Files selected for processing (7)
src/converter.tssrc/converters/finecoConverter.test.tssrc/converters/finecoConverter.tssrc/models/finecoRecord.tssrc/testing/data/yahooFinanceQuoteSummaryResults.jsonsrc/testing/data/yahooFinanceSearchResults.jsonsrc/watcher.ts
- Fix void-return: split return errorCallback() into two statements - Add progress.stop() before early return in catch block - Guard against division by zero for bond quantity - Add JSDoc docstrings to private methods - Use jest.restoreAllMocks() instead of clearAllMocks() - Use toContain for brittle error message assertions - Add semicolon-delimited CSV test with Italian number formatting - Add bond quantity/100 conversion test - Register semicolon header variant in watcher for auto-detection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/converters/finecoConverter.ts (1)
145-145:⚠️ Potential issue | 🟡 MinorSeparate callback invocation from
returnto comply withnoVoidTypeReturn.Same pattern issue as noted above. Split the callback invocation and return statement.
🐛 Proposed fix
- return errorCallback(err); + errorCallback(err); + return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/finecoConverter.ts` at line 145, In the branch that currently does "return errorCallback(err);" in finecoConverter.ts, call the callback and then return separately to avoid returning a void expression: invoke errorCallback(err); on its own line and then use an explicit return (e.g., return;). Update the occurrence using the errorCallback invocation so the callback is executed and the function returns on the next statement rather than returning the callback call value.
🧹 Nitpick comments (1)
src/converters/finecoConverter.ts (1)
251-262: Consider handling zero quantity edge case for bond maturity.For
rimborsobonds (line 254), ifrecord.quantitais 0, the activity will havequantity = 0. While mathematically safe, a zero-quantity activity may not be meaningful for Ghostfolio imports. The guard at line 258 handles the unit price calculation, but you might want to add an early check to skip or log such records.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/converters/finecoConverter.ts` around lines 251 - 262, When handling bond maturity in the block where desc === "rimborso" and isBond, add an explicit guard for record.quantita === 0 to avoid producing a meaningless zero-quantity activity: detect the zero-quantity case and either skip the record (return/continue) or emit a warning log via the existing logger, instead of assigning quantity = 0; ensure this check is placed before assigning quantity and unitPrice so it covers the rimborso branch and keeps the existing isBond fallback logic intact (refer to symbols desc === "rimborso", isBond, record.quantita, quantity, unitPrice).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/converters/finecoConverter.ts`:
- Line 83: In the function in finecoConverter.ts that currently does "return
errorCallback(new Error(errorMsg));", call errorCallback(new Error(errorMsg));
on its own line and then use a plain "return;" immediately after so the method
with void return type doesn't return the callback's value; reference the
existing errorCallback and errorMsg identifiers when making the change.
---
Duplicate comments:
In `@src/converters/finecoConverter.ts`:
- Line 145: In the branch that currently does "return errorCallback(err);" in
finecoConverter.ts, call the callback and then return separately to avoid
returning a void expression: invoke errorCallback(err); on its own line and then
use an explicit return (e.g., return;). Update the occurrence using the
errorCallback invocation so the callback is executed and the function returns on
the next statement rather than returning the callback call value.
---
Nitpick comments:
In `@src/converters/finecoConverter.ts`:
- Around line 251-262: When handling bond maturity in the block where desc ===
"rimborso" and isBond, add an explicit guard for record.quantita === 0 to avoid
producing a meaningless zero-quantity activity: detect the zero-quantity case
and either skip the record (return/continue) or emit a warning log via the
existing logger, instead of assigning quantity = 0; ensure this check is placed
before assigning quantity and unitPrice so it covers the rimborso branch and
keeps the existing isBond fallback logic intact (refer to symbols desc ===
"rimborso", isBond, record.quantita, quantity, unitPrice).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/converters/finecoConverter.test.tssrc/converters/finecoConverter.tssrc/watcher.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/watcher.ts
| errorMsg += ` Details: ${err.message}`; | ||
| } | ||
|
|
||
| return errorCallback(new Error(errorMsg)); |
There was a problem hiding this comment.
Separate callback invocation from return to comply with noVoidTypeReturn.
Similar to the fix applied at lines 33-34, this line returns the result of errorCallback() from a method with return type void. The callback should be invoked separately, then return.
🐛 Proposed fix
- return errorCallback(new Error(errorMsg));
+ errorCallback(new Error(errorMsg));
+ return;📝 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.
Suggested change
| return errorCallback(new Error(errorMsg)); | |
| errorCallback(new Error(errorMsg)); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/converters/finecoConverter.ts` at line 83, In the function in
finecoConverter.ts that currently does "return errorCallback(new
Error(errorMsg));", call errorCallback(new Error(errorMsg)); on its own line and
then use a plain "return;" immediately after so the method with void return type
doesn't return the callback's value; reference the existing errorCallback and
errorMsg identifiers when making the change.
Add broker listing, export instructions, and run command for FinecoBank.
|
Ciao @got3nks ! 👋 Volevo davvero ringraziarti per il lavoro che hai fatto qui per il converter di Fineco! 🚀 Stavo lavorando all'integrazione di Fineco per il mio progetto open-source di tracciamento portafoglio (LibreFolio) e mi sono imbattuto nella tua PR. La tua implementazione e la struttura del parser mi sono state utilissime come spunto per realizzare il nostro parser interno per Fineco, che ora supporta già 2 formati del broker: Ho anche ripreso il file di esempio dell'export Fineco per i test di importazione nel repository: A questo proposito, volevo farti due veloci domande/proposte:
Grazie ancora per l'ottimo lavoro su questo parser! 🙌 Un saluto, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for importing transaction exports from FinecoBank (Italian broker).
Handles buy/sell, dividends, bond coupons (cedole), stock splits (aumento capitale), and bond maturities (rimborso). Supports both comma and semicolon delimited CSVs with Italian number formatting. Bonds are converted with quantity/100 convention for Ghostfolio.
Added
Fixes
Checklist