After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771667 - Different warnings when changing reconciled splits vs. splits linked to reconciled splits
Different warnings when changing reconciled splits vs. splits linked to recon...
Status: RESOLVED OBSOLETE
Product: GnuCash
Classification: Other
Component: Register
2.6.14
Other Mac OS
: Normal normal
: ---
Assigned To: gnucash-ui-maint
gnucash-ui-maint
Depends on:
Blocks:
 
 
Reported: 2016-09-19 18:14 UTC by Christoph Rohland
Modified: 2018-06-29 23:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to allow the two warnings to be saved separatly (3.39 KB, patch)
2017-03-13 16:57 UTC, Bob
none Details | Review
Patch to allow the two warnings based on protected fields (10.83 KB, patch)
2017-04-20 14:26 UTC, Bob
committed Details | Review

Description Christoph Rohland 2016-09-19 18:14:24 UTC
When trying to change a reconciled split of a transaction there is the following warning:

"Change reconciled split?

You are about to change a reconciled split. Doing so might make future reconciliation difficult! Continue with this change?"


On the other hand when trying to change a non-reconciled split which is linked to a reconciled split there is another warning:

"Change split linked to a reconciled split?

You are about to change a split that is linked to a reconciled split. Doing so might make future reconciliation difficult! Continue with this change?"


But when storing the answer to any of these warnings both warnings will be answered. This does not allow to change unreconciled splits and still get a warning for reconciled splits. 


Background info: Changing unreconciled splits is absolutely valid to do - and was possible in 2.4. My preferred workflow in 2.4 was: Reconcile the current account to the bank statement. Then split of the bank provisions and taxes where applicable. When erroneously changing the current account split it would correctly warn me.
Comment 1 Bob 2017-03-13 16:57:09 UTC
Created attachment 347853 [details] [review]
Patch to allow the two warnings to be saved separatly

Currently the warnings for changing a reconciled split and the linked reconciled split share the same saved value, with these changes they have there own saved values.

Bob
Comment 2 Geert Janssens 2017-03-16 20:10:10 UTC
Review of attachment 347853 [details] [review]:

Thanks for the patch although I'm having a deja vu feeling here...

The other split warning was implemented to resolve bug 344869 in 2012. While just like then your patch is technically correct. However I had some issues with the patch then and this current bug report and your new patch remind me of those. 5 years later I still believe the solution for the old bug was not optimal.

Here are my reservations:
1. as back then, I don't think the user should be prevented from altering non-reconciled splits in any way. There is no reason to it.
2. if you insist on keeping this option, the message is misleading. Altering anything on an unreconciled split, including the amount is not making future reconciliation more difficult. It has no effect whatsoever on future reconciliation.
3. This is different for some transaction properties though, for which no message is emitted. If the transaction date, num or description are changed this alters data that should have been verified by reconciliation so future reconciliations may be confusing. I think this should be handled separately from altering a reconciled split.
4. Lastly, I believe any change that alters verified data (being date, num, description, reconciled_split account, reconciled_split amount) and that is accepted by the user should unreconcile the split (or splits). This should force the user to formally redo the reconciliation for any reconciled split in that transaction.

I'm holding off applying this patch until we have come to a consensus on this. Because if the outcome is that unreconciled splits no longer merit a warning there should be no key added to the setting schema for it (which I only want to do if we're sure that's where we want to go. Dropping schema is much harder than adding it).
Comment 3 Christoph Rohland 2017-03-16 20:31:37 UTC
Hi Geert,

I fully agree with you. But the current behavior is really bad. You have the choice to have no warnings at all or to be warned all the time you want to change a split - even if it is not reconciled.

I would be happy to go back to the old behavior without any warning on unreconciled splits.

Christoph
Comment 4 Bob 2017-03-17 12:28:53 UTC
Geert,

I had totally forgot that I had poked around in this area before, this obviously means as you say it was not correct. Personally I do not reconcile any thing but verify accounts against statements so I have no preference to what is best, just saw the bug and it look like a simple fix, how wrong was I.

I have tested the current situation with the following results...

Changing Transaction
date, num, description - reconciled split warning
notes - no warnings

Changing Non reconciled Split
action, memo - no warning
account, debit, credit - linked reconciled split warning.

Changing Reconciled Split
action, memo - no warning
account, debit, credit - reconciled split warning.

So if I understand you correctly...
1) un-protect non reconcile split fields account,debit,credit and get rid of associated warning.
2) If any of the protected transaction/reconciled split fields change, then un-reconcile any reconciled splits

Bob
Comment 5 Geert Janssens 2017-03-17 13:00:46 UTC
(In reply to Bob from comment #4)
> Geert,
> 
> I had totally forgot that I had poked around in this area before, this
> obviously means as you say it was not correct. Personally I do not reconcile
> any thing but verify accounts against statements so I have no preference to
> what is best, just saw the bug and it look like a simple fix, how wrong was
> I.
> 

:-)

> I have tested the current situation with the following results...
> 
> Changing Transaction
> date, num, description - reconciled split warning
> notes - no warnings
> 
> Changing Non reconciled Split
> action, memo - no warning
> account, debit, credit - linked reconciled split warning.
> 
> Changing Reconciled Split
> action, memo - no warning
> account, debit, credit - reconciled split warning.
> 
Still assuming a transaction with at least one reconciled split, one scenario is missing:
From a register of a non-reconciled split's account, changing
Transaction date, num, description - linked reconciled split warning

> So if I understand you correctly...
> 1) un-protect non reconcile split fields account,debit,credit and get rid of
> associated warning.

Yes

> 2) If any of the protected transaction/reconciled split fields change, then
> un-reconcile any reconciled splits

Close
2a) if a protected reconciled split field changes, unreconcile that split
2b) if a protected transaction field changes, unreconcile all reconciled splits

I'm also trying to imagine what would be the cleanest way to inform the user of the fact's s/he's about to change a field being frozen by reconciliation. If we can manage to convey this clearly in one warning message I'd prefer this. The message should clearly indicate the field is protected because either
- it's part of a split that's been reconciled
- it's part of a transaction with a reconciled split
And it should also inform the user that continuing will undo reconciliation of the affected split or all reconciled splits in this transaction.

That's a lot to put in a single warning that works in all entry points and use cases I'm afraid.

So probably you should tailor the given message a bit based on what the user is doing.
- One warning while changing a split field. This warning can then inform the user this particular split will be unreconciled
- One warning while changing a transaction field. In addition to informing the user all reconciled splits will be unreconciled, it could also indicate in which account(s) this transaction has reconciled splits.

I believe this combination of information will give the user the best basis to decide how to proceed.

Finally, I believe we can stick to one warning preference because in essence we're still only warning for fields that are effectively protected by reconciliation, even though some of these fields are transaction fields and others are split fields. The idea behind the warning is the user is about the break reconciliation. It doesn't matter which field is triggering this so it wouldn't make sense to warn for one and not for the other. This was different with the linked split warning this bug report was filed for initially.
Comment 6 Bob 2017-04-20 14:26:20 UTC
Created attachment 350135 [details] [review]
Patch to allow the two warnings based on protected fields

This patch displays two distinct warnings when changing protected fields of a transaction that contains reconciled splits. If the fields date, num and description are changed, then the warning list the accounts that have reconciled splits and also advises that they will be unreconciled after editing the transaction. If the fields account, transfer, debit or credit are changed then the warning advises that the split will be unreconciled after editing the transaction.

There is still just one warning preference as it is all to do with fields protected by reconciliation.

I think this covers what is required but may of missed something.

Bob
Comment 7 Geert Janssens 2017-12-30 14:02:31 UTC
My apologies for not reviewing your work earlier. I somehow missed you had attached a patch here.

Meanwhile the PR you created based on this patch has been merged into our unstable branch, so this will appear in the upcoming gnucash 3.0.

The trouble with fine-tuning like this is your tester pays much more attention to the details. And while doing so I have found another small issue to solve: there are ways to get hit by both messages in one transaction editing session.

Here's how to reproduce:

1. find a transaction with reconciled splits
2. start editing the description

=> A warning will pop up because you're about to change a transaction with reconciled splits.

3. Assume this is what you want so click continue
4. Now before leaving the transaction, enter in split view mode (you could also do this before starting these steps)
5. Change the amount of a reconciled split

=> A warning will pop up because you're about to change a reconciled split.

I believe that's one too much. In the first warning you're informed of the consequences of continuing the changes (namely all splits will become unreconciled). There's no need any more to get the second reconcile warning for a particular single split.

This is tied to how the unreconciling is handled: it's only done when the transaction editing completes or gets committed. I think this is counter-intuitive as well. I would change this to do the unreconcile actions as soon as the user decides to continue. If s/he changes its mind before completing the transaction editing, s/he can always choose to "Cancel" the transaction, which will undo all changes including unreconciling.

Note I have already pushed a followup commit changing the warning message text because we're about to enter string freeze, but I didn't (and won't) have time soon to also alter the behaviour as described above.

Note 2: if the scenario runs the other way around, I would expect two warnings. That is, if the user starts changing the amount of a reconciled split (warning 1) and then proceeds to alter the transaction description AND that transaction still has other reconciled splits the user should be warned again (unless s/he disabled warnings on the first one) because the action is about to unreconcile even more splits than s/he agreed to in the first warning. If splits get unreconciled immediately after continuing on the first warning it will automatically behave this way. No furthers checks required.
Comment 8 Geert Janssens 2018-03-02 14:32:16 UTC
Comment on attachment 350135 [details] [review]
Patch to allow the two warnings based on protected fields

This patch has been applied a while back.

I'll keep the bug open for now as reminder of the cleanup in comment 7.
Comment 9 John Ralls 2018-06-29 23:51:13 UTC
GnuCash bug tracking has moved to a new Bugzilla host. The new URL for this bug is https://bugs.gnucash.org/show_bug.cgi?id=771667. Please continue processing the bug there and please update any external references or bookmarks.