Question: Why is each transaction associated with a currency? For example, in the attached file we have:

<gnc:transaction version="2.0.0">
  <trn:id type="guid">15cea3a11e44f3778f627654f8df1f08</trn:id>
  <trn:currency>
    <cmdty:space>ISO4217</cmdty:space>
    <cmdty:id>GBP</cmdty:id>
  </trn:currency>
  ...
  <trn:description>buy2</trn:description>
  <trn:splits>
    <trn:split>
      <split:id type="guid">7978798b012f0be092aa3f5f27b6ce79</split:id>
      <split:action>Buy</split:action>
      <split:reconciled-state>n</split:reconciled-state>
      <split:value>130000/100</split:value>
      <split:quantity>1000000/10000</split:quantity>
      <split:account type="guid">7cef5fcbed0c51cb21f6cb30f8947fb7</split:account>
    </trn:split>
    <trn:split>
      <split:id type="guid">d60aa303f1e5343e81762ac85e362242</split:id>
      <split:reconciled-state>n</split:reconciled-state>
      <split:value>-130000/100</split:value>
      <split:quantity>0/100</split:quantity>
      <split:account type="guid">2436fe1266b55096efbba950b83d4f21</split:account>
    </trn:split>
  </trn:splits>
</gnc:transaction>




Why do we have
<trn:currency>
    <cmdty:space>ISO4217</cmdty:space>
    <cmdty:id>GBP</cmdty:id>
</trn:currency>?

Does this serve any purpose?  In the example I bought 100 shares of stock ABC at USD 13/share.  In fact, is it not the split parts which require a currency?

Created attachment 347954 [details] [review]
Patch to Warn

Created attachment 347955 [details] [review]
Patch to prevent

I was not really sure which way to go on this so have submitted a patch that would warn the user about changing security/currency but would still allow them to do it and a patch that would prevent them changing the security/currency when the account has transactions.

Only one patch should be applied and thinking more about it I think it should be to prevent them but will see what the developers think.

Bob

Review of attachment 347955 [details] [review]:

Thanks for the patches Bob.

I agree with you gnucash should not allow the user to change the currency of an account once it contains splits.

However your patch is intervening too late in the process IMO.

Instead of catching a click on the currency button and then bringing up a dialog box, you should instead really prevent the user from even attempting to change the currency. I believe the most elegant way would be to replace the commodity picker widget with a GtkLabel which displays the current currency/commodity for this account when it already has splits. As icing on the cake add a tooltip to this label that explains a currency/commodity can't be edited because the account already contains splits.

Review of attachment 347954 [details] [review]:

Rejected because preventing is better than warning in this case.

*** Bug 779864 has been marked as a duplicate of this bug. ***

Created attachment 348165 [details] [review]
Patch to prevent

I was about to submit another patch when I started to think what else you should not change when the account has transactions.

What about the smallest fraction and account type ?

I have noticed if I replace the Security/Currency with a label and you change the account type you get an error asking for a commodity which you can not change as it is a label.

So I may be totally wrong but this patch changes the Security/Currency to a label and tooltip when the account has transactions. Also I have made the Smallest fraction and Account Type widgets insensitive and added the same tooltip to these.

Bob

Review of attachment 348165 [details] [review]:

Bob, your new patch generates an "implicit function declaration" warning, which then gets turned into an error:

/bin/sh ../../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I/home/janssege/Development/gnucash/gnucash/src/gnome-utils -I../..  -I/home/janssege/Development/gnucash/gnucash/src/core-utils -I/home/janssege/Development/gnucash/gnucash/src/gnc-module -I/home/janssege/Development/gnucash/gnucash/src/engine -I/home/janssege/Development/gnucash/gnucash/src/backend/xml -I/home/janssege/Development/gnucash/gnucash/src/app-utils -I/home/janssege/Development/gnucash/gnucash/src -I../../src -I/home/janssege/Development/gnucash/gnucash/lib/libc -I/home/janssege/Development/gnucash/gnucash/src/libqof/qof -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/gnome-keyring-1 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/libsecret-1 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/guile/2.0 -I../../src/libqof/qof -I/home/janssege/Development/gnucash/gnucash/src/libqof/qof  -I/usr/include/libxml2  -DG_LOG_DOMAIN=\"gnc.gui\"   -Werror -Wdeclaration-after-statement -Wno-pointer-sign -g  -std=gnu99 -g  -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations  -Wno-unused -MT dialog-account.lo -MD -MP -MF .deps/dialog-account.Tpo -c -o dialog-account.lo /home/janssege/Development/gnucash/gnucash/src/gnome-utils/dialog-account.c
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I/home/janssege/Development/gnucash/gnucash/src/gnome-utils -I../.. -I/home/janssege/Development/gnucash/gnucash/src/core-utils -I/home/janssege/Development/gnucash/gnucash/src/gnc-module -I/home/janssege/Development/gnucash/gnucash/src/engine -I/home/janssege/Development/gnucash/gnucash/src/backend/xml -I/home/janssege/Development/gnucash/gnucash/src/app-utils -I/home/janssege/Development/gnucash/gnucash/src -I../../src -I/home/janssege/Development/gnucash/gnucash/lib/libc -I/home/janssege/Development/gnucash/gnucash/src/libqof/qof -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/gnome-keyring-1 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/libsecret-1 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/guile/2.0 -I../../src/libqof/qof -I/home/janssege/Development/gnucash/gnucash/src/libqof/qof -I/usr/include/libxml2 -DG_LOG_DOMAIN=\"gnc.gui\" -Werror -Wdeclaration-after-statement -Wno-pointer-sign -g -std=gnu99 -g -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -MT dialog-account.lo -MD -MP -MF .deps/dialog-account.Tpo -c /home/janssege/Development/gnucash/gnucash/src/gnome-utils/dialog-account.c  -fPIC -DPIC -o .libs/dialog-account.o
/home/janssege/Development/gnucash/gnucash/src/gnome-utils/dialog-account.c: In function ‘gnc_account_window_create’:
/home/janssege/Development/gnucash/gnucash/src/gnome-utils/dialog-account.c:1346:9: error: implicit declaration of function ‘xaccAccountCountSplits’ [-Werror=implicit-function-declaration]
     if (xaccAccountCountSplits (aw_get_account (aw), FALSE) > 0)
         ^~~~~~~~~~~~~~~~~~~~~~

The idea is good. Strictly speaking the account type can change as long as the existing currency/commodity is a valid one for the new account type or the type is not changing to/from a business account type. That's a lot of complex logic to implement though, so I'm fine with preventing type changes once there are transactions/splits. As for the implementation, I'd do the same as with the currency. If you can't change it, make it a label.

The smallest fraction makes me hesitate. I wouldn't expect a change in fraction to cause trouble, but unfortunately it does if you change to a smaller fraction (meaning from for example 1/100 to 1/10). Testing this some more however this looks like a completely unrelated bug: if you create a new account with a fraction of 1/10 and try to make a transfer of say 10.05 from that account to an account with at 1/100 fraction, you'll end up in an endless unbalanced warning loop. The only way out would be to close the account register. So it seems this is a use case gnucash doesn't currently cope well with at all.

I guess the way forward is for me to file a new bug for this. IMO there's no need/use for you to prevent changes to the fraction in the patch for this bug. It wouldn't prevent the user from running into the same issue when creating a new account with a different fraction.

Changing the control to a label is unnecessary, just call gtk_widget_set_sensitive(widget, FALSE) on it. Do the same with the type list box. 

The fraction problem is indeed a separate bug. I suspect the only clean way to fix it will be to remove the option to set the fraction on the account and have it only on the commodity and to make it immutable after the commodity is created.

(In reply to John Ralls from comment #10)
> Changing the control to a label is unnecessary, just call
> gtk_widget_set_sensitive(widget, FALSE) on it. Do the same with the type
> list box.

While disabling the current widget will effectively prevent the user from making the changes it's not always the nicest user experience IMO. For example if a treeview gets disabled the currently selected line is no longer visible. So in case of the Account Type this information would no longer be visible.
And I don't know how the currency selector widget would behave when it gets a gtk_widget_set_sensitive(widget, FALSE) call. It's a composite widget and I never tested whether it would disable all children or not. Ideally it should but that has to be verified. For the SCU it may be ok.

In addition I think a tool tip explaining why the widget was disabled is helpful to the user. Although on second thoughts a tool tip should really explain what the widget is for. The explanation as to why it's disabled makes a nice extra.

Just a heads up: disallowing the user to change account types is actually the solution for bug 548021.

Created attachment 348228 [details] [review]
Patch to prevent

Reworked patch to replace the Security/Currency with a label and also the account type when the account has transactions.

Bob

Created attachment 348229 [details] [review]
Count splits in accounts

This patch adds the xaccAccountCountSplits that is available in master. I assumed that with the inclusion of new text that the fix would be on master.

Bob

Review of attachment 348228 [details] [review]:

Oh, you are right it should go to master because of the new string. I overlooked that.

However while thinking about this I am willing to make an exception in this case due to the severity of the bug. I think an untranslated tool tip is a minor issue compared to the potential data loss/corruption a user could create by changing commodity or account type once an account has splits already.

Having said that I applied your patches to maint, but it's not working the same way there as on master, at least not for me.

When I apply the patch to master it works as advertised: if there are no transactions yet the commodity and type selectors are enabled. Once there are transactions both are changed to a label.

On maint however, the commodity selector is always a label and the type treeview is always a disabled treeview. Regardless of whether there are transactions or not. Can you reproduce this ?

ISTM requiring a change to go to master because of a single new string, or even a bunch of new strings, is too strict. I also don't think that we've ever applied that standard; if we did we wouldn't need to update the Translation Project with every release and translators would need only create new translations at major releases.

If there's a reason to hold this off to master it's that it's a significant change to the UI, but as with my price db changes a year ago it's clearly an important fix and I agree that it should go into maint if possible.

(In reply to John Ralls from comment #16)
> ISTM requiring a change to go to master because of a single new string, or
> even a bunch of new strings, is too strict. I also don't think that we've
> ever applied that standard; if we did we wouldn't need to update the
> Translation Project with every release and translators would need only
> create new translations at major releases.
> 
It's odd I seem to remember this used to be a guideline. Perhaps I've mixed it up with string freeze during late beta phase. Regardless I agree it's too strict anyway. So I'll no longer apply that restriction from now on. 

> If there's a reason to hold this off to master it's that it's a significant
> change to the UI, but as with my price db changes a year ago it's clearly an
> important fix and I agree that it should go into maint if possible.

(In reply to Geert Janssens from comment #15)
> 
> On maint however, the commodity selector is always a label and the type
> treeview is always a disabled treeview. Regardless of whether there are
> transactions or not. Can you reproduce this ?

Built with the maint version, 283cdba and observed that both options are editable. Applied both patches and rebuilt using cmake, and now BOTH options get labels when there are transactions present. I can not see why yours should be different. Only been using cmake recently as it seems much quicker, I can try to rebuild with autotools tomorrow if required.

Bob

Erh oops...

Turns out I was mixing up two local repository clones while reviewing your patches. Got that sorted out and now your changes do what you say.

Good work and sorry for the distraction.

And I should have added this has been applied to the maint branch so it will appear in the next stable software release to be expected next weekend.

(In reply to Geert Janssens from comment #9)
> The idea is good. Strictly speaking the account type can change as long as
> the existing currency/commodity is a valid one for the new account type or
> the type is not changing to/from a business account type. That's a lot of
> complex logic to implement though, so I'm fine with preventing type changes
> once there are transactions/splits. As for the implementation, I'd do the
> same as with the currency. If you can't change it, make it a label.
> 
> The smallest fraction makes me hesitate. I wouldn't expect a change in
> fraction to cause trouble, but unfortunately it does if you change to a
> smaller fraction (meaning from for example 1/100 to 1/10). Testing this some
> more however this looks like a completely unrelated bug: if you create a new
> account with a fraction of 1/10 and try to make a transfer of say 10.05 from
> that account to an account with at 1/100 fraction, you'll end up in an
> endless unbalanced warning loop. The only way out would be to close the
> account register. So it seems this is a use case gnucash doesn't currently
> cope well with at all.
> 
> I guess the way forward is for me to file a new bug for this. IMO there's no
> need/use for you to prevent changes to the fraction in the patch for this
> bug. It wouldn't prevent the user from running into the same issue when
> creating a new account with a different fraction.

This fix has had retrograde consequences, see gnucash-devel for my thoughts.  I don't think it is a new report, I think it is this one and the other being over-fixed that is the problem now.

Wm, you're mixing up two separate issues here. There is an independent issue with the smallest fraction setting that can cause infinite rounding error warnings. This was discovered by chance while testing for this bug.

The other is how account type changes are handled if the account already has splits. I agree discussion on this belongs to this bug.

Thanks for bringing this up by the way. It has led me to fine tune my thoughts. I think it can indeed be fixed more elegantly: if an account already has splits, the list of eligible account types should be filtered based on the criteria mentioned earlier. This can be implemented relatively easily internally using a couple of lists grouping acceptable types and only displaying the list in which the current type exists.

I do agree with Wm that the current change is way too strict.

I think that people should be allowed to do whatever they want, but in cases where something dangerous could happen (which, by the way, is ONLY when changing currency/commodity of an account that has splits in it -- or has children accounts of stock/mutual that have splits it in) -- we should pop up a warning box (that cannot be turned off) about it.  I don't believe we should outright prevent it.

I see no reason to prevent changing any account type, even between Asset and Expense, if the commodity doesn't change.

(In reply to Derek Atkins from comment #23)
> I do agree with Wm that the current change is way too strict.
> 
> I think that people should be allowed to do whatever they want, but in cases
> where something dangerous could happen (which, by the way, is ONLY when
> changing currency/commodity of an account that has splits in it -- or has
> children accounts of stock/mutual that have splits it in) -- we should pop
> up a warning box (that cannot be turned off) about it.  I don't believe we
> should outright prevent it.

I think we should if it's an unreasonable thing to do or if it causes the program logic to break.

I agree preventing account type changes in general was a mistake. What should be prevented is changing currency/commodities (either manually or induced by changing the account type to/form stock/mutual).

I'm trying to get a clearer picture on your second use case: if an account has children accounts of stock/mutual that have splits in it. How are these affected if the account itself doesn't have any splits (the precondition to allow changing commodity in the first place) ?

> 
> I see no reason to prevent changing any account type, even between Asset and
> Expense, if the commodity doesn't change.

How about changing from A/(RP) to any other account ? I doubt our business logic will handle this elegantly.

And I also have no idea what the side effects would be if one changes a trading account to a different type. Unless you have good reasons not to I'm inclined to treat it the same way as A/(RP) accounts and make it immutable. It's an automatically generated account type so I think it's fair to consider it under program control.

Also note that allowing users to change between Asset and Expense for example will be even less restrictive than it was before. In 2.6.15 and before one could change freely between Income and Expense, but not between Asset and Income/Expense.

I have relaxed the account type changing as follows. When an account
already has splits, it can only change to another account type that
won't force a commodity change (typically when switching between
a stock related account and a non-stock related one).
In addition, to be on the safe side the code will prevent you from
switching between AR/AP/Trading accounts and any other account type
if the account already has splits for the simple reason I'm not
sufficiently confident the internals of gnucash won't get confused
by such a change.

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=603379. Please update any external references or bookmarks.