Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify Swissquote Bank PDF-Importer to support new transaction #3789

Conversation

Nirus2000
Copy link
Member

@Nirus2000 Nirus2000 changed the title Modify eBase PDF-Importer to support new transaction Modify Swissquote Bank PDF-Importer to support new transaction Feb 11, 2024
@Nirus2000 Nirus2000 added the pdf label Feb 11, 2024
@Nirus2000 Nirus2000 removed the request for review from buchen February 11, 2024 16:15
@Nirus2000
Copy link
Member Author

Nirus2000 commented Feb 11, 2024

Hello @buchen
I think we need a function analogous to the check by documentContext("currency") to move the PeriodicHelper or similar into the extractutils.
For example, baseCurrency and year should be found. (see Postbank PDF Importer)

The other point is that I can only check one currency via check in the tests, but not EUR, USD, GPB, CHF
new AssertImportActions().check(results, CurrencyUnit.EUR);
However, if I only check one currency, it is always displayed incorrectly. We need a list of currencies.

What did you think?

@buchen
Copy link
Member

buchen commented Feb 14, 2024

I think we need a function analogous to the check by documentContext("currency") to move the PeriodicHelper or similar into the extractutils.

If there is significant re-use, then why not.
Do you have an idea how you want to do it?

The current code around the PeriodicHelper is short and easily understandable. So one could make the PeriodicItem an abstract class with only the line number and each importer is extending it with whatever fields it needs (currency, year, date, whatever).

I looked at the change for the documentContext("currency"). Supporting it here is much more tricky. Maybe we could allow each importer to define their own record class (containing date, currency, whatever is needed) and parse a pseudo block that collects those information in the range of the block (start - end line number). That sounds like a couple hours...

The other point is that I can only check one currency via check in the tests, but not EUR, USD, GPB, CHF

Can't we just add the other currencies (of course, where needed, I don't think it makes sense to add this to all tests).

new AssertImportActions().check(results, CurrencyUnit.EUR);
new AssertImportActions().check(results, "USD");

@Nirus2000
Copy link
Member Author

Hello @buchen
I have opened an issue or extension for this.
Let's bring this PR together and see how we can implement the idea.
Alex

buchen added a commit that referenced this pull request Feb 19, 2024
Additionally, the check makes sure that only the given currencies are
available for import.

Issue: #3789
@buchen buchen merged commit 276d442 into portfolio-performance:master Feb 27, 2024
2 checks passed
@Nirus2000 Nirus2000 deleted the Modify-Swissquote-Bank-PDF-Importer-to-support-new-transaction branch March 21, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants