Bug 394124 - Investment Sell activity is automatically set to Buy when Total ammount is rounded to zero
Summary: Investment Sell activity is automatically set to Buy when Total ammount is ro...
Status: CONFIRMED
Alias: None
Product: kmymoney
Classification: Applications
Component: general (show other bugs)
Version: 4.8.0
Platform: Debian stable Linux
: NOR normal
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-11 09:45 UTC by guiloma
Modified: 2018-05-30 07:53 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
test xmi file (27.33 KB, text/xml)
2018-05-11 21:13 UTC, Ralf Habacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description guiloma 2018-05-11 09:45:41 UTC
When creating the entry for selling a small ammount of an investment (price*ammount ~=0.00) after clikcing "Enter" the ledger shows the entry as "Buy" (Both in the Investment legder and the charge account ledger).
The asset balance is calculated correctly as a Sell and the operation has no impact on the charge account, since the ammount is 0.00, but it is also shown as a Buy operation on the account ledger.

Even though the issue has no impact on the balance computer, the operation should be recorded properly for tracking purposes.
Comment 1 Ralf Habacker 2018-05-11 21:13:40 UTC
Created attachment 112585 [details]
test xmi file

With the appended kmymoney test file the bug can be reproduced:
1. open the appended file with kmymoney
2. select account 'invest' and open ledger view
3. There is a transaction with activity type "buy shares" although it has been created with activity "sell shares".
4. edit the transaction from step 3
5. set activity to "sell shares" and save
The transaction shows "buy shares" as activity.
Comment 2 Ralf Habacker 2018-05-11 21:46:44 UTC
The problem is caused in void InvestTransactionEditor::dissectTransaction() where the transaction type is determined from the split value:

186: } else if (split.action() == MyMoneySplit::ActionBuyShares) {
187:    transactionType = (!split.value().isNegative()) ? MyMoneySplit::BuyShares : MyMoneySplit::SellShares;

In case the split value is zero, it is determined as BuyShares because it is not negative.
Comment 3 Jack 2018-05-11 23:22:30 UTC
I believe the same issue happens with other account types - a cash/checking account transaction of $0 always ends up labelled as a deposit (I think) even if you entered it as a withdrawal.  Should this be turned into a wishlist item that transactions retain the transaction type with which they were originally created?
Comment 4 Ralf Habacker 2018-05-14 10:46:00 UTC
An investment related split uses 'action="Buy"' in the saved file and uses the sign to indicate a sell.

buy:

    <SPLIT payee="" reconcileflag="0" shares="1/1" reconciledate="" action="Buy" bankid="" account="A000002" number="" value="1/1" memo="" id="S0002"/>

sell: 
    <SPLIT payee="" reconcileflag="0" shares="-1/1" reconciledate="" action="Buy" bankid="" account="A000002" number="" value="-1/1" memo="" id="S0002"/>

This could be changed into 

sell: 
    <SPLIT payee="" reconcileflag="0" shares="1/1" reconciledate="" action="Sell" bankid="" account="A000002" number="" value="1/1" memo="" id="S0002"/>

to give the transaction type a higher priority.

> a cash/checking account transaction of $0 always ends up labeled as a deposit 

This is a different case and need to be fixed seperatly, because the action attribute is currently only used for investment transactions. All other transaction types detect whether the transaction is a deposit or a withdrawal from the sign:

deposit: 
  <TRANSACTION postdate="2018-05-14" commodity="EUR" memo="" id="T000000000000000002" entrydate="2018-05-14">
   <SPLITS>
    <SPLIT payee="" reconcileflag="0" shares="1/1" reconciledate="" action="" bankid="" account="A000001" number="" value="1/1" memo="" id="S0001"/>
    <SPLIT payee="" reconcileflag="0" shares="-1/1" reconciledate="" action="" bankid="" account="A000002" number="" value="-1/1" memo="" id="S0002"/>
   </SPLITS>
  </TRANSACTION>

withdrawal:
  <TRANSACTION postdate="2018-05-14" commodity="EUR" memo="" id="T000000000000000003" entrydate="2018-05-14">
   <SPLITS>
    <SPLIT payee="" reconcileflag="0" shares="-1/1" reconciledate="" action="" bankid="" account="A000001" number="" value="-1/1" memo="" id="S0001"/>
    <SPLIT payee="" reconcileflag="0" shares="1/1" reconciledate="" action="" bankid="" account="A000002" number="" value="1/1" memo="" id="S0002"/>
   </SPLITS>
  </TRANSACTION>

To fix this, an action needs to saved into and loaded from the file. In mymoneysplit.h there are already related constants defined 

  static const char ActionDeposit[];
  static const char ActionTransfer[];
  static const char ActionWithdrawal[];

and are already used for in the gnu cash reader and in KMyMoneyView::fixTransactions_0(), called from KMyMoneyView::initializeStorage().
Comment 5 Ralf Habacker 2018-05-16 06:57:43 UTC
see https://phabricator.kde.org/D12917 for a preparation patch
Comment 6 Thomas Baumgart 2018-05-18 06:15:30 UTC
In comment #4 you write:

> This could be changed into 
> 
> sell: 
>     <SPLIT payee="" reconcileflag="0" shares="1/1" reconciledate="" action="Sell" bankid="" account="A000002" number="" value="1/1" memo="" id="S0002"/>
> 
> to give the transaction type a higher priority.

Please be aware, that you can change the text of the action but you must keep the sign as it is. Changing the sign here will break other areas of the code.
Comment 7 Ralf Habacker 2018-05-30 07:53:37 UTC
Git commit 98a9ff8ed371c2a549e3c03ec1a23678434bcdf3 by Ralf Habacker.
Committed on 30/05/2018 at 07:53.
Pushed by habacker into branch '4.8'.

Make sure that all code uses MyMoneySplit::Action... variables instead of duplicated strings

Summary:
This patch is a preparation for adding MyMoneySplit::ActionSellShares and
helps to avoid regressions by not patching all related locations.

Test Plan: compiled on linux

Reviewers: tbaumgart

Reviewed By: tbaumgart

Differential Revision: https://phabricator.kde.org/D12917

M  +6    -6    kmymoney/converter/mymoneyqifwriter.cpp
M  +2    -2    kmymoney/dialogs/transactioneditor.cpp
M  +5    -5    kmymoney/plugins/csvexport/csvwriter.cpp

https://commits.kde.org/kmymoney/98a9ff8ed371c2a549e3c03ec1a23678434bcdf3