Bug 333498

tested in kubuntu -  same result

*** This bug has been marked as a duplicate of bug 332793 ***

On 16/04/14 13:01, Chris wrote:
> https://bugs.kde.org/show_bug.cgi?id=333498
>
>              Bug ID: 333498
>             Summary: bulk edit actions results in amount of transaction
>                      being zeroed

I have a potential fix for this, which appears to do the necessary, 
without any apparent harmful effects.

In void StdTransactionEditor::slotUpdateCategory(const QString& id), 
circa line 1543, there is an updateAmount(val), which now I execute only
if (!isMultiSelection().

I don't know if there may be a better way/place to do this.

I notice also, that Buy and Sell activities have a similar problem if a 
fee account is entered.

Allan

Hi Allan

Unfortunately this is not a solution. You should be able to update the 
amount using the bulk edit feature.

The problem seems to be that the val  field is not checked whether it has 
changed or not. It is cast to a Mymoneymoney object and as it is an empty 
string that object returns a 0

The object probably should have a "NaN" test, it does not appear to have 
such a test. Failing that the text field should be checked before the 
MyMoneymoney object is instantiated and used.

What is confusing is for some reason 
StdTransactionEditor::slotUpdateCategory tests for the existence of the 
category label "if (categoryLabel)" and skips updating the value if it 
exists. It seems that that may have been a kludge fix for some other issue. 
The fact that the two types of editing, inline and using the form, have two 
different program paths says to me there are bigger issues.

Lastly when editing multiple transactions if you select a category that has 
a tax auto split on it, it silently fails to add the tax split. That is 
pretty crazy. If you rely on this software to do your annual taxes and it 
silently discards taxes, then you may have problems with your tax office. At 
the very least it should issue a warning. preferably it should create the 
split properly. I don't know if I want a stint in jail due to a software bug.


Chris


On 17/04/2014 8:38 AM, allan wrote:
> https://bugs.kde.org/show_bug.cgi?id=333498
>
> --- Comment #3 from allan <agander93@gmail.com> ---
> On 16/04/14 13:01, Chris wrote:
>> https://bugs.kde.org/show_bug.cgi?id=333498
>>
>>               Bug ID: 333498
>>              Summary: bulk edit actions results in amount of transaction
>>                       being zeroed
> I have a potential fix for this, which appears to do the necessary,
> without any apparent harmful effects.
>
> In void StdTransactionEditor::slotUpdateCategory(const QString& id),
> circa line 1543, there is an updateAmount(val), which now I execute only
> if (!isMultiSelection().
>
> I don't know if there may be a better way/place to do this.
>
> I notice also, that Buy and Sell activities have a similar problem if a
> fee account is entered.
>
> Allan
>

After a little digging I found the correct function to use

I believe the following change resolves the issue without unintended 
consequences. But I may be wrong :)

void StdTransactionEditor::slotUpdateCategory(const QString& id)
{
   QLabel *categoryLabel = dynamic_cast<QLabel*>(haveWidget("category-label"));
   // qDebug("Update category to %s", qPrintable(id));
   kMyMoneyEdit* amount = dynamic_cast<kMyMoneyEdit*>(m_editWidgets["amount"]);

   if (categoryLabel && amount->isValid()) {
     TabBar* tabbar = dynamic_cast<TabBar*>(haveWidget("tabbar"));
     
      MyMoneyMoney val = amount->value();

     if (categoryLabel->text() == i18n("Transfer from")) {
       val = -val;
     } else {
       val = val.abs();
     }


There is still the very serious issue of incorrect tax calculations that 
most definitely needs to be resolved.

Chris

On 20/04/2014 10:14 PM, Chris wrote:
> Hi Allan
>
> Unfortunately this is not a solution. You should be able to update the 
> amount using the bulk edit feature.
>
> The problem seems to be that the val  field is not checked whether it has 
> changed or not. It is cast to a Mymoneymoney object and as it is an empty 
> string that object returns a 0
>
> The object probably should have a "NaN" test, it does not appear to have 
> such a test. Failing that the text field should be checked before the 
> MyMoneymoney object is instantiated and used.
>
> What is confusing is for some reason 
> StdTransactionEditor::slotUpdateCategory tests for the existence of the 
> category label "if (categoryLabel)" and skips updating the value if it 
> exists. It seems that that may have been a kludge fix for some other 
> issue. The fact that the two types of editing, inline and using the form, 
> have two different program paths says to me there are bigger issues.
>
> Lastly when editing multiple transactions if you select a category that 
> has a tax auto split on it, it silently fails to add the tax split. That 
> is pretty crazy. If you rely on this software to do your annual taxes and 
> it silently discards taxes, then you may have problems with your tax 
> office. At the very least it should issue a warning. preferably it should 
> create the split properly. I don't know if I want a stint in jail due to a 
> software bug.
>
>
> Chris
>
>
> On 17/04/2014 8:38 AM, allan wrote:
>> https://bugs.kde.org/show_bug.cgi?id=333498
>>
>> --- Comment #3 from allan<agander93@gmail.com>  ---
>> On 16/04/14 13:01, Chris wrote:
>>> https://bugs.kde.org/show_bug.cgi?id=333498
>>>
>>>               Bug ID: 333498
>>>              Summary: bulk edit actions results in amount of transaction
>>>                       being zeroed
>> I have a potential fix for this, which appears to do the necessary,
>> without any apparent harmful effects.
>>
>> In void StdTransactionEditor::slotUpdateCategory(const QString& id),
>> circa line 1543, there is an updateAmount(val), which now I execute only
>> if (!isMultiSelection().
>>
>> I don't know if there may be a better way/place to do this.
>>
>> I notice also, that Buy and Sell activities have a similar problem if a
>> fee account is entered.
>>
>> Allan
>>
>

(In reply to comment #5)

This below is copied from the list, but with a couple of extra comments added, marked ***.

> After a little digging I found the correct function to use
> 
> I believe the following change resolves the issue without unintended 
> consequences. But I may be wrong :)
> 
> void StdTransactionEditor::slotUpdateCategory(const QString& id)
> {
>    QLabel *categoryLabel =
> dynamic_cast<QLabel*>(haveWidget("category-label"));
>    // qDebug("Update category to %s", qPrintable(id));
>    kMyMoneyEdit* amount =
> dynamic_cast<kMyMoneyEdit*>(m_editWidgets["amount"]);
> 
>    if (categoryLabel && amount->isValid()) {
>      TabBar* tabbar = dynamic_cast<TabBar*>(haveWidget("tabbar"));
>      
>       MyMoneyMoney val = amount->value();
> 
>      if (categoryLabel->text() == i18n("Transfer from")) {
>        val = -val;
>      } else {
>        val = val.abs();
>      }

*** Yes, this looks good to me, and addresses the place where things are starting to go wrong,***

> There is still the very serious issue of incorrect tax calculations that 
> most definitely needs to be resolved.
> 
> Chris

> Unfortunately this is not a solution. You should be able to update the
> amount using the bulk edit feature.

When you say "You should be able to update the amount using the bulk edit feature.", do you mean that it is supposed or stated to, or do you mean "It would be useful if it did"?  My immediate thought the other day was, "Why would anyone want to do that?".  With just a quick look, I can't see any relevant code.

> The problem seems to be that the val  field is not checked whether it
> has changed or not. It is cast to a Mymoneymoney object and as it is an
> empty string that object returns a 0

I would suspect that it is not checked because it is not expected to change,
but the devs really need to pronounce here.

> The object probably should have a "NaN" test, it does not appear to have
> such a test. Failing that the text field should be checked before the
> MyMoneymoney object is instantiated and used.

> What is confusing is for some reason
> StdTransactionEditor::slotUpdateCategory tests for the existence of the
> category label "if (categoryLabel)" and skips updating the value if it
> exists. It seems that that may have been a kludge fix for some other
> issue. The fact that the two types of editing, inline and using the
> form, have two different program paths says to me there are bigger issues.
>
Again, I'm not an expert, but comparing the transaction form with the register fields, one has an Amount field, and the other has Payment and Deposit, so the processing needs are somewhat different.  My deduction was that testing the Category label was a way of determining  which to use.  Possibly the devs can clarify/contradict.

> Lastly when editing multiple transactions if you select a category that
> has a tax auto split on it, it silently fails to add the tax split. That
> is pretty crazy. If you rely on this software to do your annual taxes
> and it silently discards taxes, then you may have problems with your tax
> office. At the very least it should issue a warning. preferably it
> should create the split properly. I don't know if I want a stint in jail
> due to a software bug.
> Chris

This last paragraph needs dev. input.

*** However, I notice that in void StdTransactionEditor::updateVAT(bool amountChanged)
{
<snip>
  // we don't do anything if we have multiple transactions selected
  if (isMultiSelection())
    return;
...***

Allan

> I notice also, that Buy and Sell activities have a similar problem if a 
> fee account is entered.

Please ignore this - I was misleading myself.

Also, assuming no adverse feedback, I'll commit Chris's fix in a week or so.

Please leave this bug closed as a duplicate, it makes no sense having two reports handling the same issue.

*** This bug has been marked as a duplicate of bug 332793 ***