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

Revision ComDirect PDF-Importer #3555

Conversation

Nirus2000
Copy link
Member

@Nirus2000 Nirus2000 commented Sep 14, 2023

Closes #3553
Closes #3313
Closes #1131

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/229
https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/236
https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/242
https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/254
https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/270
https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/276
https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/283

ComDirect provides two documents for the transaction. The security transaction and the taxes treatment.
Both documents are provided as one PDF or as two PDFs.

The security transaction includes the fees, but not the correct taxes and the taxes treatment includes all taxes (including withholding tax), but not all fees.

Therefore, we use the documents based on their function and merge both documents, if possible, as one transaction.

Always import the securities transaction and the taxes treatment for a correct transaction. Due to rounding differences, the correct gross amount is not always shown in the securities transaction.

@buchen
Many work... so plz check the postProcessing if you find any problems.

Maybe it's possible to leave a note about this in the next changelog.


A special thanks goes to those who sent me more than 400 PDF debugs.

Forum user:

  • jensinho
  • DonQuijote
  • Manuel ... i dont know the user name in PP-Forum
  • Max ... i dont know the user name in PP-Forum
    Maybe you find by email adress the user in the PP-Forum userlist

@Nirus2000 Nirus2000 added pdf blocked Blocked due to external dependencies labels Sep 14, 2023
@Nirus2000 Nirus2000 requested a review from buchen September 14, 2023 08:36
@Nirus2000 Nirus2000 removed the blocked Blocked due to external dependencies label Sep 14, 2023
@Nirus2000 Nirus2000 force-pushed the Revision-ComDirect-PDF-Importer branch from ed14620 to 9967060 Compare September 14, 2023 08:41
@buchen buchen self-assigned this Sep 14, 2023
@Nirus2000
Copy link
Member Author

Nirus2000 commented Sep 15, 2023

Hello @buchen
One point occurred to me today, where I still have no solution in mind.

Scenario:
When importing on the same day with same security...

  • 1x dividend transaction without taxes treatment
  • 1x taxes treatment of sale

or

  • 1x sale transaction without taxes treatment
  • 1x taxes treatment of dividend

...is imported.

I think when creating the maps (concat() -> taxes treatment with sell or dividend) in postProcessing,
it still needs to be identified somehow if the taxes treatment is a purchase/sale or a taxes treatment of a dividend.

Copy link
Member

@buchen buchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Nirus2000,

what a change. 😄 Great to see the Comdirect Importer getting up to its task. I started digging through it - here are my comments on the postProcessing method.

The main point: we should not use attributes of the security. First, there can be one security for many transactions (which one wins?). Second, it pollutes the XML document.

Bildschirmfoto 2023-09-23 um 08 26 10

I have tried to make suggestions how to store the data differently.
The Item has a generic setData and getData which is currently not used at all.
If needed, we could extend it to (typed) map.

Finally, maybe it could enhance the code if the post processing of sale and dividend transactions are two different methods. The way I read the post processing, sale are dividend processing is completely separated - but the code is kind of mixed: prepare list S, prepare list D, process S, process D.

Comment on lines 1150 to 1116
t.getSecurity().getAttributes().put( //
new AttributeType(ATTRIBUTE_GROSS_TAXES_TREATMENT), grossTaxesBaseBeforeLossOffset);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use the attributes of the security here.

Why? First: there can be multiple transactions for one security - the attributes override each other. Second: it pollutes into the XML file. (The same is true for other data that you store into attributes).

You can use the "Item" to store the grossTaxesBaseBeforeLossOffset:

// store in transaction context when parsing section
v.getTransactionContext().put(ATTRIBUTE_GROSS_TAXES_TREATMENT, grossTaxesBaseBeforeLossOffset);
// save in transaction item when wrapping the transaction
.wrap((t, ctx) -> {
    TransactionItem item = new TransactionItem(t);
    item.setData(ctx.get(ATTRIBUTE_GROSS_TAXES_TREATMENT));

...
}

and then use in postProcessing method.

Comment on lines 1908 to 1910
if (transactions.get(0).getSubject() instanceof BuySellEntry portfolioTransaction)
{
a2 = (AccountTransaction) similarTransactions.get(1).getSubject();
BuySellEntry sellTransaction = (BuySellEntry) transactions.get(0).getSubject();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assign the value to portfolioTransaction and then immediately again to sellTransaction.
The second line is unnecessary because the value is already casted and assigned.

Comment on lines 1913 to 1914
if (sellTransaction.getPortfolioTransaction().getSecurity().getAttributes()
.get(new AttributeType(ATTRIBUTE_DATE_TIME)) != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the reasoning: the sale document contains a time, the tax document does not.

However, storing the date time as attribute does not work for multiple reasons: first: what happens if there are multiple sale transactions for one security - which date/time is winning? Second: the attributes are polluting the XML.

Maybe a better way is to create the grouping not on LocalDateTime but on LocalDate. In the process, it is "just" stripping the time. But the transaction keeps the original time. Something along these lines:

// Group dividend and taxes transactions together and group by date and security
Map<LocalDate, Map<Security, List<Item>>> dividendTaxesTransactions;
[...]
dividendTaxesTransactions = Stream.concat(dividendTransactionList.stream(), taxesTransactionList.stream())
   .collect(Collectors.groupingBy(item -> item.getDate().toLocalDate(), Collectors.groupingBy(Item::getSecurity)));

{
return AccountTransaction.Type.DIVIDENDS.equals(a.getType());
if (ATTRIBUTE_TO_BE_DELETED.equals(a.getNote()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the deletion logic is not wrong, I wonder if we can make it easier by using a Set:

Set<AccountTransaction> toBeDeleted = new HashSet<>();

...

toBeDeleted.add(taxesTransaction)

...

Iterator<Item> iter = items.iterator();
while (iter.hasNext())
{
    Object o = iter.next().getSubject();
    if (toBeDeleted.contains(o))
        iter.remove();
}

(or, if you kept the item around, then items.remove(taxesTransactionItem))

@buchen
Copy link
Member

buchen commented Sep 23, 2023

I think when creating the maps (concat() -> taxes treatment with sell or dividend) in postProcessing,
it still needs to be identified somehow if the taxes treatment is a purchase/sale or a taxes treatment of a dividend.

I guess it depends on the "somehow".
If the document contains some kind of transaction identifier, then this could be stored in the Item and used to match.
But if the security + the date is the only identifier, then I do not see an way how we should know for which one the tax statement is.

@Nirus2000 Nirus2000 force-pushed the Revision-ComDirect-PDF-Importer branch 2 times, most recently from afdf45e to 7863e19 Compare September 24, 2023 09:31
@Nirus2000
Copy link
Member Author

Nirus2000 commented Sep 24, 2023

Hallo @buchen

Thank you for the review.

I have implemented these points.

I need ATTRIBUTE_GROSS_TAXES_TREATMENT and ATTRIBUTE_EXCHANGE_RATE as attribute to read them from transaction A and process them in transaction B if necessary.
If you have time, let's do a TeamViewer meeting on this so we could work out a solution together here.

#3555 (comment)
I guess it depends on the "somehow".
If the document contains some kind of transaction identifier, then this could be stored in the Item and used to match.
But if the security + the date is the only identifier, then I do not see an way how we should know for which one the tax statement is.

This is a scenario for which I have no PDF debugs. It was just a mental game, but both documents have a taxes reference number that could be compared.

Regards
Alex

@Nirus2000 Nirus2000 force-pushed the Revision-ComDirect-PDF-Importer branch from 7863e19 to 36ce3d3 Compare September 24, 2023 09:57
@Nirus2000 Nirus2000 requested a review from buchen September 24, 2023 10:02
@buchen
Copy link
Member

buchen commented Sep 24, 2023

I need ATTRIBUTE_GROSS_TAXES_TREATMENT and ATTRIBUTE_EXCHANGE_RATE as attribute to read them from transaction A and process them in transaction B if necessary.

My understanding is you need both in the postProcessing method. In that method, you also have the Item at hand and therefore also the Item#getData method (you'd need to create the maps wither with the item - or maybe create a record (item, transaction) - we can do team viewer later)

@Nirus2000 Nirus2000 force-pushed the Revision-ComDirect-PDF-Importer branch 3 times, most recently from 754256d to dc9da0a Compare September 30, 2023 06:26
buchen added a commit that referenced this pull request Oct 2, 2023
@buchen buchen mentioned this pull request Oct 2, 2023
Closes #3553
Closes #3313
Closes #1131

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/229

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/236

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/242

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/254

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/270

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/276

https://forum.portfolio-performance.info/t/pdf-import-von-comdirect/1647/283

ComDirect provides two documents for the transaction.
The security transaction and the taxes treatment.
Both documents are provided as one PDF or as two PDFs.

The security transaction includes the fees, but not the correct taxes
and the taxes treatment includes all taxes (including withholding tax),
but not all fees.

Therefore, we use the documents based on their function and merge both documents,
if possible, as one transaction.

Always import the securities transaction and the taxes treatment for a correct transaction.
Due to rounding differences, the correct gross amount is not always shown in the securities transaction.

Co-Authored-By: buchen <[email protected]>
@Nirus2000 Nirus2000 force-pushed the Revision-ComDirect-PDF-Importer branch from 698d6f3 to 1378f39 Compare October 3, 2023 07:28
@buchen
Copy link
Member

buchen commented Oct 4, 2023

merged

@buchen buchen closed this Oct 4, 2023
buchen added a commit that referenced this pull request Oct 4, 2023
buchen added a commit that referenced this pull request Oct 7, 2023
@Nirus2000 Nirus2000 deleted the Revision-ComDirect-PDF-Importer branch October 7, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants