xaccTransSetCurrency and Bug 763146

[John Ralls]

> On Mar 17, 2016, at 3:27 PM, Mike Alexander <mta at umich.edu> wrote:
> 
>> On Mar 16, 2016, at 10:54 PM, John Ralls <jralls at ceridwen.us> wrote:
>> 
>> In the course of working on and testing bug 763146 [1] the reporter discovered an interesting auto-completion bug: If the transaction that's being copied is multi-currency and was created in the other register (so the transaction currency is for a different currency from the present register's) then attempts to edit the debit/credit amount will produce incorrect results. The problem was made more obvious because the reporter's currency, BYR, trades at around 21000 to the USD, so the incorrectly calculated values and prices were 0 after rounding, causing GnuCash to blank the cells.
>> 
>> On investigating I found that the most obvious problem was that the calculations were being run in the wrong direction because of the transaction currency to account currency mismatch. Adding a line to gnc_split_register_autocompletion() to change xaccTransSetCurrency() didn't fix the problem, though, because the split values weren't changed by that call. Adding code to recalculate the split values does correct the problem.
>> 
>> While it seems pretty obvious to me that recalculating the split values after re-setting a transaction's currency is The Right Thing To Do, I'm worried anyway. 
>> 
>> Can anyone think of use cases where it's not the right thing to do?
> 
> Do you have any specific reason you are concerned, or just the general feeling that something so obvious can't have been missed for so long?  It seems clear that the values must be changed somehow if the currency changes, since they are supposed to be expressed in the currency that just changed.  I guess my only question is whether that's the right place to do it.

Mike,

Neither. I'm used to finding bugs like that because the early authors weren't thinking about multi-currency transactions and by the time you started to the codebase had become far too bloated and convoluted to track down all of the problems.

In fact I even wrote a test that specifically checked that this worked wrong because I didn't recognize it as a bug and was documenting with the tests the existing behavior so that when we got to rewriting the test would show that the behavior had been preserved through the rewrite. That was 4 1/2 years ago.

What I'm worried about is that there's code in GnuCash that depends on that bug. I'm less worried today because I went through every call of xaccTransSetCurrency and almost all of them operate only on new transactions. The few that don't (cap-gains.c, Scrub.c) should be OK with the change. With that done I've pushed the commit.

Regards,
John Ralls