Bug 419975 - When importing transactions, we're matching against the other transactions also being imported
Summary: When importing transactions, we're matching against the other transactions al...
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Unclassified
Component: importer (show other bugs)
Version: git (master)
Platform: Other Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-12 02:19 UTC by Dawid Wróbel
Modified: 2020-06-05 04:07 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In: 5.1.0


Attachments
Sample CSV file (213 bytes, text/csv)
2020-04-12 02:19 UTC, Dawid Wróbel
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dawid Wróbel 2020-04-12 02:19:47 UTC
Created attachment 127457 [details]
Sample CSV file

SUMMARY
While importing investment transactions from CSV file today, I noticed that one of the transactions was missing. Specifically, two of the transactions were similar enough for the TransactionMatcher to drop one of them and reconcile the other. Despite having same quantities, prices and date, the transactions have different memo, so it's understandable that it considers them a loose match.

However, those two shouldn't even be matched against each other, since both are being imported from external source. I can see in the mymoneystatementreader.cpp that this is indeed aimed to be handled properly by checking if a transaction isImported():

    if (result == TransactionMatchFinder::MatchDuplicate
      || !matchedTransaction.isImported()
      || result == TransactionMatchFinder::MatchPrecise) { // don't match with just imported transaction

However, the CSV importer doesn't set it for its transactions, unlike the QIF importer. Note that the latter is the *only* importer that has any usage of setImported() in its code, so I presume other importers may be affected as well.

STEPS TO REPRODUCE
1. Import the CSV file attached


OBSERVED RESULT
Only one of the two transactions was imported.


EXPECTED RESULT
Both should be.
Comment 1 Dawid Wróbel 2020-04-12 03:42:00 UTC
Actually, after reading the code further I stand corrected - isImported() is a member of MyMoneyTransaction, while CSV importer uses the MyMoneyStatement::Transaction.

The MyMoneyStatementReader::processTransactionEntry() actually correctly sets the transactionUnderImport.setImported(). The actual issue is with the conditional expression I previously referred to – the !isImported() condition should not be optional. Proposed resolution:

-      if (result == TransactionMatchFinder::MatchDuplicate
-      || !matchedTransaction.isImported()
-      || result == TransactionMatchFinder::MatchPrecise) { // don't match with just imported transaction
+      if (!matchedTransaction.isImported() &&
+      (result == TransactionMatchFinder::MatchDuplicate || result == TransactionMatchFinder::MatchPrecise)) { // don't match with just imported transaction

Let
Comment 2 Dawid Wróbel 2020-04-12 03:54:18 UTC
Git commit 295d003c977ee67e2af7d76a14a39869c599ac9e by Dawid Wróbel.
Committed on 12/04/2020 at 03:53.
Pushed by wrobelda into branch 'master'.

When importing transactions, make sure not to match against the transactions already imported during the same session.

M  +2    -3    kmymoney/converter/mymoneystatementreader.cpp

https://commits.kde.org/kmymoney/295d003c977ee67e2af7d76a14a39869c599ac9e
Comment 3 Dawid Wróbel 2020-04-12 06:04:21 UTC
Git commit 5bcf3cf9db2f689aefc7d9c3459ac0a496075c85 by Dawid Wróbel.
Committed on 12/04/2020 at 06:00.
Pushed by wrobelda into branch 'master'.

Revert the fix which broke duplicate transaction detection between imports.

This reverts commit 295d003c977ee67e2af7d76a14a39869c599ac9e.

M  +3    -2    kmymoney/converter/mymoneystatementreader.cpp

https://commits.kde.org/kmymoney/5bcf3cf9db2f689aefc7d9c3459ac0a496075c85
Comment 4 Thomas Baumgart 2020-04-12 07:57:05 UTC
Git commit 1655f78fd9bcbb3c23f2a5c8d8d4cf05a61823a0 by Thomas Baumgart.
Committed on 12/04/2020 at 07:55.
Pushed by tbaumgart into branch '5.0'.

Make sure bankID is assigned when importing transactions

The bankID member of a split is the ultimate identifier to detect
duplicate transactions. Yet if empty it is not used and transactions of
the same import session are falsely detected as duplicates.

This change assigns a bankID in case it is empty based on the account
id, the post date, the payee name, the memo text, and the amount which
makes the bankID unique for the import session.
FIXED-IN: 5.0.9

M  +26   -17   kmymoney/converter/mymoneystatementreader.cpp

https://commits.kde.org/kmymoney/1655f78fd9bcbb3c23f2a5c8d8d4cf05a61823a0
Comment 5 Dawid Wróbel 2020-04-15 22:51:22 UTC
Git commit 84d204b26fd449f501d4f7cb360adad7def73627 by Dawid Wróbel.
Committed on 15/04/2020 at 22:49.
Pushed by wrobelda into branch 'bug-420056'.

Add option to enable fixing of the buy/sell signs

Some providers do not follow the OFX spec and do not assign the correct
positive/negative signage to amount (TOTALs) and quantity (UNITs)
values assosciated with BUY/SELL investment transactions (see OFX2.2,
section 13.3.3). This change adds option to OFX import dialog and
account's online settings to force the signage to follow the the spec.
GUI:

M  +1    -0    kmymoney/plugins/ofx/import/dialogs/konlinebankingstatus.cpp
M  +19   -2    kmymoney/plugins/ofx/import/dialogs/konlinebankingstatusdecl.ui
M  +18   -1    kmymoney/plugins/ofx/import/importoption.ui
M  +96   -74   kmymoney/plugins/ofx/import/ofximporter.cpp

https://commits.kde.org/kmymoney/84d204b26fd449f501d4f7cb360adad7def73627