Bug 351874 - QIF import of investment buys and sells mishandles commissions
Summary: QIF import of investment buys and sells mishandles commissions
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Applications
Component: general (show other bugs)
Version: 4.7.2
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-27 21:20 UTC by Jeff
Modified: 2017-07-01 21:21 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.8.0


Attachments
Simple QIF file to demonstrate the problems (677 bytes, text/qif)
2015-08-31 22:13 UTC, Jeff
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff 2015-08-27 21:20:58 UTC
When importing a QIF file from Quicken 2013, investment buys and sells that have a commission are imported with the commission applied three times.  The KMM ledger knows something is wrong because it marks every transaction in both the "brokerage" and the investment accounts with warning triangles about missing assignment for the commission times 2. The price and value in the ledger are correct until you edit the transaction, at which time the extra commissions are added, making the overall transaction wrong (not matching the QIF file numbers), but the warning triangle goes away.

I have a fix that I will be posting to the review board.  I developed the fix using KMM 4.7.2 because I could not get the 'gitHEAD' version to build on Windows. But the patch file will be against gitHEAD.

Reproducible: Always

Steps to Reproduce:
1. Import a QIF file with investment trades with a commission
2.
3.

Actual Results:  
The commission is added 3 times to the transaction. KMM shows warnings on every transaction.

Expected Results:  
The commission should only count one time and KMM should not have warnings.

Here's a simple QIF example that exhibits the problem (notice the transaction date is really old):

!Option:AutoSwitch
!Account
NFidelity Brokerage
TPort
^
NFidelity Brokerage (Brokerage)
TChecking
^
!Type:Security
NApple Computer
SAAPL
TStock
^
!Account
NFidelity Brokerage
TInvst
^
!Type:Invst
D7/27/81
NBuy
YApple Computer
I25.375
Q100
C
U2,567.50
T2,567.50
O30.00
^
Comment 1 Jeff 2015-08-31 22:13:48 UTC
Created attachment 94312 [details]
Simple QIF file to demonstrate the problems

Adding a simple QIF file that demonstrates the problem and includes investment buys, a sell, and a sell where the commission is greater than the proceeds of the sale. The data is cut from my Quicken QIF file.
Comment 2 NSLW 2016-05-15 18:44:21 UTC
Hi Jeff,

could you please check with KMM from git master branch to see if the problem persists.
I import your QIF file and there are no warning triangles. Commissions seems to me correct either.

Earlier I've got the same problem with CSV importer but I managed to patch KMM to solve that problem and to me it seems that inadvertently it solved this bug too.

Regards
Łukasz
Comment 3 Jeff 2016-05-16 00:40:01 UTC
Hi Łukasz,

The git master branch does import my test file without any warnings.

I had patched my copy of mymoneyqifreader.cpp to fix my problem. I undid that patch to test your fix.

I'm a little concerned about the change to mymoneystatementreader.cpp. I didn't change it to fix my problem because the statement reader also has to handle csv and OFX imports. You have verified the csv import. Have you verified OFX imports?

Jeff.
Comment 4 NSLW 2016-05-16 18:28:38 UTC
Statement reader did it wrong also for CSV imports (see bug #361021) so I made corrections to the code which seemed to help also QIF reader. Honestly at that time I didn't know that investment statements are imported by anything else than CSV and thanks to your bug and code review I see bigger picture now. I proposed two next patches to statement reader which now I must review myself because I already see mixed logic between QIF and CSV imports, which somehow worked by now.
It would be nice if you could send me for testing an anonymized QIF file with less frequent transaction types such as "dividends" etc.

I don't know about OFX imports because valid  ACCTTYPEs are only: CHECKING, SAVINGS, MONEYMRKT, and CREDITLINE, and there are no investment ACCTTYPE, so I assume OFX doesn't support that. Correct me if I'm wrong.

Is this bug solved for you then?
Comment 5 Jack 2016-05-16 21:39:49 UTC
I import to investment accounts from OFX frequently, and it works just fine.  (Well, mostly, but my problems are mainly in what my broker provides, not how KMM handles it, and I've complained about it in the past on the mailing list.)  I can probably provide some example files, but I'd have to choose carefully for ones that  don't show any of the problems.
Comment 6 Jeff 2016-05-16 22:29:10 UTC
Hi Łukasz,

There is still a problem with the QIF import with your change.  My test file also tested the case where the commission was greater than the proceeds from the sale (which can happen when trading options.)  Your fix changed a "sell" trade that actually cost money into one that brought in money. The example in my test file was the "sell" of the "NFLX Aug 18 2012 110.0 Call". The price is 0.02, times 100 shares = 2.00. The commission is 10.77.  So income of 2.00, outgo of 10.77 makes the total -8.77 (as shown in the U and T values in the QIF file).  The cash account should decrease by 8.77. Your change turned that into a positive 8.77, and increased the cash account.  This is admittedly a corner case, and I think I am the only KMM user that trades options because I have made a bunch of other changes to the KMM code to support that.

So, to support my particular needs, I am going to undo your change in my local copy and restore my QIF importer change (which I submitted to the review board on Aug 15, 2015 but was never accepted). And still claim that the master branch does not fix my problem.  (Though I hardly use QIF import anymore. I was using it to transfer my data from Quicken to KMM. I do have one investment account that doesn't support OFX and I use QIF for that one.)

And I know that without your change, my OFX imports have been working. I don't have a quick and easy test for OFX import. But my review board submittal for the QIF importer change says I made the QIF importer data look just like the OFX importer data. Thus my concern in comment #3 about OFX testing with your change.  I suspect OFX will work except for the case I explained in my first paragraph. Which probably nobody but me cares about :-)

Jeff.
Comment 7 allan 2016-05-16 22:53:11 UTC
(In reply to NSLW from comment #4)
> Statement reader did it wrong also for CSV imports (see bug #361021) so I
> made corrections to the code which seemed to help also QIF reader. Honestly
> at that time I didn't know that investment statements are imported by
> anything else than CSV and thanks to your bug and code review I see bigger
> picture now. I proposed two next patches to statement reader which now I
> must review myself because I already see mixed logic between QIF and CSV
> imports, which somehow worked by now.
> It would be nice if you could send me for testing an anonymized QIF file
> with less frequent transaction types such as "dividends" etc.
> 
> I don't know about OFX imports because valid  ACCTTYPEs are only: CHECKING,
> SAVINGS, MONEYMRKT, and CREDITLINE, and there are no investment ACCTTYPE, so
> I assume OFX doesn't support that. Correct me if I'm wrong.

The OFX specification 2.0.3 includes -
"CHAPTER 13 INVESTMENTS
OFX supports download of security information and detailed investment account statements including
transactions, open orders, balances, and positions.
" plus a lot more in detail.
Allan
Comment 8 allan 2016-05-17 11:23:56 UTC
(In reply to Jeff from comment #6)
> Hi Łukasz,
> 
> There is still a problem with the QIF import with your change.  My test file
> also tested the case where the commission was greater than the proceeds from
> the sale (which can happen when trading options.)  Your fix changed a "sell"
> trade that actually cost money into one that brought in money. The example
> in my test file was the "sell" of the "NFLX Aug 18 2012 110.0 Call". The
> price is 0.02, times 100 shares = 2.00. The commission is 10.77.  So income
> of 2.00, outgo of 10.77 makes the total -8.77 (as shown in the U and T
> values in the QIF file).  The cash account should decrease by 8.77. Your
> change turned that into a positive 8.77, and increased the cash account. 
> This is admittedly a corner case, and I think I am the only KMM user that
> trades options because I have made a bunch of other changes to the KMM code
> to support that.
> 
It would not have surprised me if your problem was caused by the same issue I reported in https://bugs.kde.org/show_bug.cgi?id=362139 comment #7, with the negative sign getting dropped.
However, the sign appears to be negative throughout the import process until -
Line 1564 in mymoneyqifreader.cpp()
"else if (action == "sell") {
    d->st.m_listPrices += price;
    tr.m_shares = -quantity;
    tr.m_amount = -amount;
    tr.m_eAction = (MyMoneyStatement::Transaction::eaSell);"

I don't profess to know anything about a sell where the commission is greater than the proceeds of the sale.  However, I think what happens in general is that a sell is expected to be input with no sign, and therefore the amount needs to be set to a negative value.  So, in your case, as you have a negative sign on input, that sign gets removed.
That seems to imply that the commission needs to negative in your case, and reversing the signs appears to produce your expected result.
If this is all rubbish, just ignore it.
Comment 9 Jeff 2016-05-17 13:30:50 UTC
That line:

    tr.m_amount = -amount;

is the one I changed in my patch to fix my problem for "sells".  Need remove the negative sign.
Comment 10 NSLW 2016-05-17 18:04:16 UTC
(In reply to Jack from comment #5)
> I import to investment accounts from OFX frequently, and it works just fine.
> (Well, mostly, but my problems are mainly in what my broker provides, not
> how KMM handles it, and I've complained about it in the past on the mailing
> list.)  I can probably provide some example files, but I'd have to choose
> carefully for ones that  don't show any of the problems.

I wonder how you test if OFX imports are correct? Is your test based only on fact that KMM doesn't show you warning signs in ledger?

My problem with CSV was that buy and sell amounts with commissions were wrong but warning sing was shown only for sell transactions. I patched CSV code in such way that no warning sign was shown, but both buy and sell amounts still were wrong. In my opinion, I tried every combination in CSV code to make amounts right but I failed, so statement reader was to blame. 

I'm going to code on weekend, so your OFX file would help me embrace it all at one time.


(In reply to Jeff from comment #6)
> There is still a problem with the QIF import with your change.  My test file
> also tested the case where the commission was greater than the proceeds from
> the sale (which can happen when trading options.)  Your fix changed a "sell"
> trade that actually cost money into one that brought in money. The example
> in my test file was the "sell" of the "NFLX Aug 18 2012 110.0 Call". The
> price is 0.02, times 100 shares = 2.00. The commission is 10.77.  So income
> of 2.00, outgo of 10.77 makes the total -8.77 (as shown in the U and T
> values in the QIF file).  The cash account should decrease by 8.77. Your
> change turned that into a positive 8.77, and increased the cash account. 
> This is admittedly a corner case, and I think I am the only KMM user that
> trades options because I have made a bunch of other changes to the KMM code
> to support that.

Now I see that too and it needs to be fixed. As you've said it's corner case and I didn't take it into account.
Summarizing it: It didn't work for you in general case and corner case. Now it works for you in general case but still not in corner case :)


(In reply to allan from comment #7)
> The OFX specification 2.0.3 includes -
> "CHAPTER 13 INVESTMENTS
> OFX supports download of security information and detailed investment
> account statements including
> transactions, open orders, balances, and positions.
> " plus a lot more in detail.
> Allan

Thanks for the info. It looks like I was searching in the wrong area.

Regards
Łukasz
Comment 11 Jeff 2016-05-17 19:45:48 UTC
> I wonder how you test if OFX imports are correct? Is your test based only on fact that KMM doesn't show you warning signs in ledger?

This wasn't addressed to me, but I will chime in. I know OFX imports work because I have 2 investment accounts that I trade in quite frequently (daily to weekly), 2 more that I trade in fairly often (monthly), and 4 more that get updates every few months. I know the imports are working because my stock and cash balances in KMM match the balances at the brokers.

> Summarizing it: It didn't work for you in general case and corner case. Now it works for you in general case but still not in corner case :)

I believe that is correct. But I didn't do extensive testing before I backed your change out of my copy.
Comment 12 allan 2016-05-17 21:59:17 UTC
(In reply to Jeff from comment #9)
> That line:
> 
>     tr.m_amount = -amount;
> 
> is the one I changed in my patch to fix my problem for "sells".  Need remove
> the negative sign.

Sorry, but I'm a bit puzzled here.  With your suggested change, to remove the sign reversal, when the transaction imports, the result shows as a deposit of $8.77 (positive).  Throughout the import, the negative sign is maintained, as far as I can see.  So, I don't see "The cash account should decrease by 8.77."  Why don't I see the result you are expecting?
Comment 13 Jeff 2016-05-17 23:38:11 UTC
>  With your suggested change, to remove the sign reversal, when the transaction imports, the result shows as a deposit of $8.77 (positive).  

In order for that 8.77 to properly become a payment instead of a deposit, I have to restore the statement reader to its 6/28/2015 version as well as remove the negative sign on the line in the QIF reader (and to make it all work, place a negative sign on the "buy" case in the QIF importer). It then imports with a warning triangle, but simply hitting "Edit" followed by the "Enter" fixes the math and the warning goes away.  To remove the warning and have the whole thing work would probably take some different change to the statement reader that I have not pursued. (I'm guessing the 6/28/2015 statement reader has some extraneous .abs() when it sets up the split and the transaction editor does not, which is why the editor fixes it.)  

As I recall from my quick look, the current master branch version of the statement reader makes assumptions about buys and sells and basically ignores the signs on the incoming data. So you could change all the signs in the QIF importer and the ledger results would not change.
Comment 14 NSLW 2016-05-21 13:26:23 UTC
Jeff please test your patch from review #124957 together with my patch from review #127983 atop git master branch to see if you can import your QIF and OFX files properly. I hope we will be able to close this bug soon.
Comment 15 Jeff 2016-05-21 23:22:57 UTC
With my testing, the combination described in comment #14 seems to work.
Comment 16 NSLW 2016-05-28 18:12:42 UTC
Git commit 8b3eac05eaa8437215232dfd4f7ddb3da5c49ad8 by Łukasz Wojniłowicz, on behalf of Jeff Lundblad.
Committed on 28/05/2016 at 17:48.
Pushed by wojnilowicz into branch 'master'.

Set signs properly for buy/sell transactions in QIF importer

Send to statement reader negative buy transactions and positive or negative sell transactions, which is dependend on fees and proceeds relationship.
REVIEW: 124957

Signed-off-by: Łukasz Wojniłowicz <lukasz.wojnilowicz@gmail.com>

M  +4    -1    kmymoney/converter/mymoneyqifreader.cpp

http://commits.kde.org/kmymoney/8b3eac05eaa8437215232dfd4f7ddb3da5c49ad8
Comment 17 NSLW 2016-05-28 18:12:42 UTC
Git commit d9a042a0322a7d76633f0c9bc53a367036ed684a by Łukasz Wojniłowicz.
Committed on 28/05/2016 at 17:53.
Pushed by wojnilowicz into branch 'master'.

Allow importing negative sell transaction

Buy transactions should be negative and sell transactions
are generally positive but can be negative if fees are
higher than proceeds. Let OFX, QIF, and CSV importer
and not statement reader decide about transaction amount signs.
REVIEW: 127983

Signed-off-by: Łukasz Wojniłowicz <lukasz.wojnilowicz@gmail.com>

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

http://commits.kde.org/kmymoney/d9a042a0322a7d76633f0c9bc53a367036ed684a
Comment 18 NSLW 2016-05-28 18:29:38 UTC
Git commit d6ac96c17fe5d4af18e34bf63635a810cafb4ca0 by Łukasz Wojniłowicz.
Committed on 28/05/2016 at 18:27.
Pushed by wojnilowicz into branch 'frameworks'.

Allow importing negative sell transaction

Buy transactions should be negative and sell transactions
are generally positive but can be negative if fees are
higher than proceeds. Let OFX, QIF, and CSV importer
and not statement reader decide about transaction amount signs.
REVIEW: 127983

Signed-off-by: Łukasz Wojniłowicz <lukasz.wojnilowicz@gmail.com>

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

http://commits.kde.org/kmymoney/d6ac96c17fe5d4af18e34bf63635a810cafb4ca0
Comment 19 NSLW 2016-05-28 18:29:38 UTC
Git commit 6605675815914296c5ebc9b7d8e90658de1708a2 by Łukasz Wojniłowicz, on behalf of Jeff Lundblad.
Committed on 28/05/2016 at 18:26.
Pushed by wojnilowicz into branch 'frameworks'.

Set signs properly for buy/sell transactions in QIF importer

Send to statement reader negative buy transactions and positive or negative sell transactions, which is dependend on fees and proceeds relationship.
REVIEW: 124957

Signed-off-by: Łukasz Wojniłowicz <lukasz.wojnilowicz@gmail.com>

M  +4    -1    kmymoney/converter/mymoneyqifreader.cpp

http://commits.kde.org/kmymoney/6605675815914296c5ebc9b7d8e90658de1708a2