Introduced an IgnoredItem class to avoid false import errors. #5186
Conversation
Background: In addition to bank statements, some banks issue
additional dedicated documents with more information about
buy/sell transactions or dividend payments. We expect to
see those extra documents and will skip such items when
found in the list of a bank statement. Concrete examples:
See the skipTransaction flag used in extractors for Deutsche Bank,
Ebase and Onvista.
Observed behavior: when importing a bank statement from e.g. Deutsche Bank
that has an ignorable entry (like a divident payment) among other
relevant entries, the divident item is ignored silently. In case
the item list happens to consist of nothing but such entries,
the import fails with an error:
"Unknown or unsupported transaction type"
(internal message id: PDFdbMsgCannotDetermineFileType)
This lead to a futile debugging attempt: the file type as well as
the transaction type are known and supported.
Expected behavior: an import should succeed without errors independant
of how many ignorable (but known and valid) items are found in a
document.
The responsible code is
1.) DeutscheBankPDFExtractor.java:
if (type.getCurrentContext().getBoolean("skipTransaction"))
return null;
2.) AbstractPDFExtractor.java:
List<Item> items = parseDocumentTypes(documentTypes, filename, text);
if (items.isEmpty())
{
errors.add(new UnsupportedOperationException(
MessageFormat.format(Messages.PDFdbMsgCannotDetermineFileType, getLabel(), filename)));
}
This patch introduces a IgnoredItem class that will be returned instead
of null. That way, the returned list will not become empty anymore in case
the parsing is successful. This approach is similar to the "null object"
design pattern.
This patch goes way behind just silencing this error: the items
will bubble up to the ReviewExtractedItems wizard page. They will
be shown with a yellow warning sign denoting that the item will be
ignored upon import. This is meant to provide more transparency
to the user. But this advanced behavior is not that important.
A more minimal patch would not show those items to the user.
The items in question could be filtered out much earlier in the
process. By use of a stripped down IgnoredItem class or some
other means that will avoid flagging an empty list as an error.
Todo: patch Ebase and Onvista extractors as well.
|
This pull request is meant to serve as a base for a discussion rather than a fixed implementation. It works for me but "all roads that lead to Rome", i. e. different implementations that have the same net effect (no more import errors) |
buchen
left a comment
buchen
left a comment
There was a problem hiding this comment.
Hi @hporten,
the idea looks good to me. It makes sense to show that the extractor identified something but decided not to import the transaction. I also think it should be visible in the UI - then the user has a chance to understand what to do.
I propose two things:
- add a reason to the Ignored item
- use the code "ERROR" (or introduce "IGNORED")
| } | ||
| else if (entry.getItem().isIgnored()) | ||
| { | ||
| entry.addStatus(new ImportAction.Status(Code.WARNING, "Ignored")); // TODO: translate message |
There was a problem hiding this comment.
With the type WARNING, the transaction is not imported by default.
However, the user can choose to import it nevertheless by choosing "import" from the context menu.
(Background: it is used if a duplicate is detected. But the user can still choose to import).
Either we mark this here as ERROR or we introduce a new category (say IGNORED).
| } | ||
| } | ||
|
|
||
| static class IgnoredItem extends Item |
There was a problem hiding this comment.
Should we add a "reason" label to the item?
The the importer could say why it ignored that item.
And this is what we could display as message in the dialog.
Yes! I already contemplated adding that. The import review UI (as it is now) would not show the reason anywhere but via the tooltip but that would be a good start.
Okay. The reuse of WARNING was indeed a hack. I will work on a new iteration of the patch within the next days. |
2 participants
Background: In addition to bank statements, some banks issue
additional dedicated documents with more information about
buy/sell transactions or dividend payments. We expect to
see those extra documents and will skip such items when
found in the list of a bank statement. Concrete examples:
See the skipTransaction flag used in extractors for Deutsche Bank,
Ebase and Onvista.
Observed behavior: when importing a bank statement from e.g. Deutsche Bank
that has an ignorable entry (like a divident payment) among other
relevant entries, the divident item is ignored silently. In case
the item list happens to consist of nothing but such entries,
the import fails with an error:
This lead to a futile debugging attempt: the file type as well as
the transaction type are known and supported.
Expected behavior: an import should succeed without errors independant
of how many ignorable (but known and valid) items are found in a
document.
The responsible code is
1.) DeutscheBankPDFExtractor.java:
2.) AbstractPDFExtractor.java:
This patch introduces a IgnoredItem class that will be returned instead of null. That way, the returned list will not become empty anymore in case the parsing is successful. This approach is similar to the "null object" design pattern.
This patch goes way behind just silencing this error: the items will bubble up to the ReviewExtractedItems wizard page. They will be shown with a yellow warning sign denoting that the item will be ignored upon import. This is meant to provide more transparency to the user. But this advanced behavior is not that important.
A more minimal patch would not show those items to the user. The items in question could be filtered out much earlier in the process. By use of a stripped down IgnoredItem class or some other means that will avoid flagging an empty list as an error.
Todo: patch Ebase and Onvista extractors as well.