Add & verify balance assertions

[christopherlam]

Add account API to store reconciliation pairs of statement_date and ending_balance
Upon reconciliation complete, add into account's kvp. Overwrite any previous reconciliation pair.
There's off-by-one error somewhere. Buggy. No g_free at all yet.
Assistance would be nice.
I'll follow on with tools to actually use it and possibly augment the reconciliation report to find errant transactions.

[jralls]

ISTM the first commit is separate from the latter two. Since KVP is ignored unless used there's no backwards-compatibility motivation to putting the latter two commits in maint. It's a new feature so it belongs in master.

[christopherlam]

This is obviously a very early draft. I'm somewhat more familiar with maint and will rebase onto master eventually.

For now my immediate dilemma is how to store these date/balance pairs. As above I'm using a counter, reconcile-info\num-dates and a list of pairs reconcile-info\N\statement-date and reconcile-info\N\ending-balance where N is std::tostring(index). I wonder if there is a better approach.

[jralls]

Yes, there is, and I suggested it in the comment about architecture: reconcile-data/statements/{date-string}={statement balance}. That will among other things get rid of all of the list gyrations, you just make a new key for a new date; if you need to change a statement balance you can overwrite the key, and if you need to blow them all away you can delete the reconcile-data/statements frame.

[christopherlam]

Yes, there is, and I suggested it in the comment about architecture: reconcile-data/statements/{date-string}={statement balance}. That will among other things get rid of all of the list gyrations, you just make a new key for a new date; if you need to change a statement balance you can overwrite the key, and if you need to blow them all away you can delete the reconcile-data/statements frame.

Ok looks good! I do hope it is equally easy to enumerate all reconcile-data/statements/* into a GList or something.

[jralls]

For listing the children of reconcile-data/statements there's std::vector<std::string> KvpFrame::get_keys(). KvpFrame also provides four templated variants of for_each_slot so that you can easily perform an operation on all of the statements, including stuffing the values into a container.

[christopherlam]

a running commentary will be appreciated :)
eventually we'll be able to rewrite xaccAccountGetReconcileLastInterval and friends.

[christopherlam]

Ok completed. I had to hack kvp-frame.hpp. However when the GList is returned by xaccAccountGetReconcileStatementDates into scheme it magically becomes a GncInvoiceList. See the last debugging commit.

* 07:36:07  WARN <gnc.account> [xaccAccountGetReconcileStatementDates()] date 31/01/20
* 07:36:07  WARN <gnc.account> [xaccAccountGetReconcileStatementDates()] date 31/03/20
d6.9982 t6.998: ('acc #<swig-pointer GncInvoiceList * 562beefd7660>)

[christopherlam]

With the latest commit to return a DatesList instead of GList, the list elements are now accessible, however the elements are still a pointer rather a time64 numeric. Assistance appreciated!

* 16:24:52  WARN <gnc.account> [xaccAccountGetReconcileStatementDates()] date 31/01/20
* 16:24:52  WARN <gnc.account> [xaccAccountGetReconcileStatementDates()] date 31/03/20
d9.9613 t9.961: ('acc (list #<swig-pointer Time64 * 5e344eff> #<swig-pointer Time64 * 5e8368ff>))

[christopherlam]

Checkpoint - basic getter/setter complete. I can now retrieve old statement dates and ending balances. Debugging macros removed. Haven't connected signals yet.

* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good
* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good. olddate = 1580486399;
* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good. old_bal 12000/100
* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good. olddate = 1582991999;
* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good. old_bal 10200/100
* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good. olddate = 1585670399;
* 01:03:08  WARN <gnc.gui> [startRecnWindow()] good. old_bal 11300/100

[jralls]

Where are you headed with the added line on the reconcile information window? Or is that just a test to make sure that the old statement information is saved and you can retrieve it?

[christopherlam]

The final ui is still TBD but the basics are now complete. I don't quite know what control will be the most appropriate one.

[christopherlam]

Works and complete. A few issues left:

1. Haven't done the https://developer.gnome.org/glib/stable/glib-Memory-Slices.html as suggested 3 days ago
2. UI works but is not exactly idiomatic. I'll leave that to someone more clever.
3. A bit ugly in gnc_start_recn_load_previous_cb to re-retrieve all old dates, find correct one, and find its balance.

[gjanssens]

The user enters the statement balance as usual and ticks off splits until the ending balance matches. To make things even easier for the user, display as already ticked off all splits that are already reconciled or cleared (YREC or CREC) with the same date_reconciled.

Remains to be seen whether this can work. I can't follow your reasoning myself now.

I think our reliance on split.date_reconciled is actually the weak spot in the current code. We use it to roughly define statement boundaries. That is, all splits with the same date_reconciled are assumed to belong to the same statement. That breaks however if a user inadvertently unreconciles a split and isn't extremely cautious to re-reconcile on the original statement date (I don't even know if that's possible if later reconciles already happened). That way the split can end up in a different statement, simply because it gets a new date_reconciled. That may even be the date of a different reconcile, which now makes that split appear as part of that statement.

This all allows for too much uncertainty, not a good quality for reconciliation IMO.

I'll stipulate that there's a small usability advantage to limiting the available splits. That advantage is offset by the fact that not showing splits past a certain date hides splits with an erroneous date_posted, making it harder to recognize and fix the error.

If there's really demand for this, it could be made optional via a toggle button. Either show only splits before the statement date or show all splits. I can't judge very well how much benefit would be in the shorter list.

How about instead drawing two lines across the listviews, one representing the previous reconcile and the other the statement date?

That could be useful as well IMO.

Are you referring to the reconciliation window? They currently show NREC CREC only, hiding YREC. Do you mean we show all splits instead, sort by reconcile status Y->C->N, with dividing line inbetween?

On re-reconciliation one should obviously also show already reconciled splits in the statement that is being re-reconciled. That would be a change from the current behaviour of the reconcile dialog.

[christopherlam]

Agreed. The saved data is IMO a reconciliation point. And I suggested before it would be very useful to consider introducing a new reconcile object or (bank)statement object to gnucash. To solve the whole past-reconcile issue it would need 3 parameters:

(snip specifics of a reconcile object)

While a split->statement_data (many:1 mapping) can work, there's also the "use case" of reconciling at random times outside formal bank-statement dates. e.g. periodically I'd log onto bank statement, update the recent transactions, double check the ending balances match, and hit Reconcile to set all recent splits to YREC thereby verifying I've added data accurately. This means the reconcile_date is effectively "today" and I'm applying a stamp onto the account split-list as of today, without formally following the bank's formal statement. Therefore the "reconcile date, starting balance, ending balance" would match the random reconciles. This is still workable IMHO.

[gjanssens]

Agreed. The saved data is IMO a reconciliation point. And I suggested before it would be very useful to consider introducing a new reconcile object or (bank)statement object to gnucash. To solve the whole past-reconcile issue it would need 3 parameters:

(snip specifics of a reconcile object)

While a split->statement_data (many:1 mapping) can work, there's also the "use case" of reconciling at random times outside formal bank-statement dates. e.g. periodically I'd log onto bank statement, update the recent transactions, double check the ending balances match, and hit Reconcile to set all recent splits to YREC thereby verifying I've added data accurately. This means the reconcile_date is effectively "today" and I'm applying a stamp onto the account split-list as of today, without formally following the bank's formal statement. Therefore the "reconcile date, starting balance, ending balance" would match the random reconciles. This is still workable IMHO.

Not sure how to read this. Is this meant as a counterargument to the split->statement_data proposal ?
Regardless, I think if you want to not follow the bank's formal statement you could slightly alter your workflow and instead of adding new reconciles each day, have one "not finished" reconcile that you re-reconcile over and over until you have the bank's formal statement to reconcile against. Also isn't the clear flag meant for these more informal interim verifications ?

[christopherlam]

Not sure how to read this. Is this meant as a counterargument to the split->statement_data proposal ?
Regardless, I think if you want to not follow the bank's formal statement you could slightly alter your workflow and instead of adding new reconciles each day, have one "not finished" reconcile that you re-reconcile over and over until you have the bank's formal statement to reconcile against. Also isn't the clear flag meant for these more informal interim verifications ?

No, This is not a formal counter argument. These adhoc reconciles still have date/startbal/endbal, just don't need to match the bank formal statement boundaries. Insofar as the CREC status, from my understanding their purpose is to hold/memorise the 'ticked' status during reconciliation, and either converted to YREC when finalised, or left when postponed.

[gjanssens]

Op donderdag 10 september 2020 15:44:51 CEST schreef Christopher Lam:

No, This is not a formal counter argument. These adhoc reconciles still have date/startbal/endbal, just don't need to match the bank formal statement boundaries.

Are you saying these would be additional statements then ? That is for one account and a given date period you could have one formal bank statement and several informal adhoc statements ? That would kill the possibility to detect missing statements as outlined above unless we would add a flag to distinguish between formal and informal statements. However I feel that would start adding fuzz again to what is supposed to be a formal and tight procedure. If you want daily statements, can't you ask your bank to generate those ? In Belgium all banks offer that option, but of course that's not necessarily representative for the rest of the world.

Insofar as the `CREC` status, from my understanding their purpose is to hold/memorise the 'ticked' status during reconciliation, and either converted to YREC when finalised, or left when postponed.

That may be one use case. You can manually set those directly in the register windows as well and the generic import matcher sets those for all matched transactions as well. The latter is what I'm interpreting as "verified transactions (as they come from the bank) waiting for formal reconciliation".

[christopherlam]

Are you saying these would be additional statements then ? That is for one account and a given date period you could have one formal bank statement and several informal adhoc statements ? That would kill the possibility to detect missing statements as outlined above unless we would add a flag to distinguish between formal and informal statements. However I feel that would start adding fuzz again to what is supposed to be a formal and tight procedure. If you want daily statements, can't you ask your bank to generate those ? In Belgium all banks offer that option, but of course that's not necessarily representative for the rest of the world.

Ok let's ignore my use-case then. The reconcile statement object is fine as you suggested. I will happily adapt my workflow :)

[gjanssens]

Ok let's ignore my use-case then. The reconcile statement object is fine as you suggested. I will happily adapt my workflow :)

Well if it's your use case, there are very likely other users doing the same. 😄
Do you think the adapted workflow would work for all of those ? If not this would require more thought.

In your use case do you want to do perform both informal and formal reconciliation on the same account ?
Would you want the formal reconciliation to take optional informal reconciliation as input ?

If we want to have both, I think informal reconciliation should get a different name. For me reconciling is really tied to bank statements...

Also mind you that while I am proposing this idea, there's little chance I'll find time and energy implementing it, let alone selling/defending it to our user base.

[christopherlam]

Ok let's ignore my use-case then. The reconcile statement object is fine as you suggested. I will happily adapt my workflow :)

Well if it's your use case, there are very likely other users doing the same. 😄
Do you think the adapted workflow would work for all of those ? If not this would require more thought.

It works fine. No difference between formal bank-led and informal ad-hoc reconciliation. As long as there's no obligatory "statement number" things will work out just fine!

EDIT: the small difference is that combining formal and informal reconciliation means that the formal reconciles will have shorter list of splits because of the preceding informal reconcile. But as far as the reconciliation process is concerned I can't see an issue.

[jralls]

I agree that a proper reconcile object is a better solution than the current date+enum overload. It's obviously a pretty substantial design change that would need a scrub to convert existing reconciles, gnc_feature protection, and two-phase introduction (4.x can read files with reconcile objects, 5.x can write them). We also overload the date_reconciled and reconciled fields for void. Since a voided transaction by definition can't clear or reconcile that's OK. There's an additional flag for "Frozen into accounting period" that's not yet used, intended to flag transactions as immutable because it's part of a closed book. That's a bit confused since it's marking splits not transactions. ISTM we should remove it.

For storage we have long held that all new data goes into KVP to protect backwards compatibility, but we recognized some time ago that we sometimes change the way data in the book are interpreted and that's breaking too so @gjanssens wrote the feature system. I think with that in place we should consider creating top level elements and tables instead of stuffing KVP. (That also means we can start taking stuff out of KVP, but that's for a separate discussion.)

[gjanssens]

I agree that a proper reconcile object is a better solution than the current date+enum overload. It's obviously a pretty substantial design change that would need a scrub to convert existing reconciles, gnc_feature protection, and two-phase introduction (4.x can read files with reconcile objects, 5.x can write them).

\o/

We also overload the date_reconciled and reconciled fields for void. Since a voided transaction by definition can't clear or reconcile that's OK.

Agreed

There's an additional flag for "Frozen into accounting period" that's not yet used, intended to flag transactions as immutable because it's part of a closed book. That's a bit confused since it's marking splits not transactions. ISTM we should remove it.

Agreed as well. Immutable transactions are something I have seen in odoo, Expert/M, Vero,... (all commercial accounting packages targeting businesses). Odoo's implementation is based on transaction post dates which makes more sense than a separate split property. I'll not go into details here as a re-implementation of this is not the point of this discussion. I agree to remove the current implementation to simplify the code.

For storage we have long held that all new data goes into KVP to protect backwards compatibility, but we recognized some time ago that we sometimes change the way data in the book are interpreted and that's breaking too so @gjanssens wrote the feature system.

Credits go to @derekatkins ... He wrote the feature system.

I think with that in place we should consider creating top level elements and tables instead of stuffing KVP. (That also means we can start taking stuff out of KVP, but that's for a separate discussion.)

Ok

[jralls]

Odoo's implementation [of immutable transactions] is based on transaction post dates

As is GnuCash's.

[jralls]

Is all of this relevant for the current PR ?

Depends. Do you want to pursue this PR as an interim measure or drop it and start working on the Reconciliation class? IMO that should be C++.

[jralls]

though the QIF importer can set split.reconciled without setting split.date_reconciled. I consider that a bug, both because reconcile should be set to YREC only by the reconcile dialog and because split.reconciled shouldn't be set without also setting split.date_reconciled for any value other than NREC.

I agree that's a bug.

Note, however, that it supports a valid use-case: A Quicken user switching to GnuCash likely has legitimately reconciled transactions to import. I don't know what Quicken puts in its QIF exports but @derekatkins is pretty sure that it only marks transactions as reconciled or not, no dates or balances.

@gjanssens proposed on IRC that we take all of the transactions marked reconciled in an import and synthesize monthly reconciliation objects so that if a user later needs to re-reconcile for some reason it would be possible to localize the damage and present a digestible reconciliation dialog to fix it. I suggest that we could facilitate that by including a flag on the reconciliation class differentiating reconciles from imports and from the reconciliation dialog as the former wouldn't necessarily match the user's bank statements.

[jralls]

One more thing: I think the reconciliation class needs a start date as well as a starting balance because there's no guarantee that a balance value is unique.

I disagree with @christopherlam's "informal reconciliation". Mark those splits cleared in the register and check that the cleared balance in the status line matches what the bank's website says. When you get your statement all of the cleared splits will show up in the reconcile window already checked off and the ending balance line will reflect that. The starting balance on the statement should match the starting balance in the reconcile window and you just check off the splits that haven't already been marked cleared to complete the reconcile.

[gjanssens]

One more thing: I think the reconciliation class needs a start date as well as a starting balance because there's no guarantee that a balance value is unique.

Can you elaborate on this ? I don't understand what you're trying to say.

[jralls]

You said:

if you sort all statements on statement date then the ending balance of a given statement should always match the starting balance the next statement.

That's true, but it could also be true in the case of a missing statement if the credits and debits were equal for the missing statement's period. In order to be sure that there are no missing statements we need a start and end date as well as a balance.

[gjanssens]

Ah, now I understand what you mean with balances not guaranteed to be unique. My 'reference implementation' (odoo) doesn't take starting dates, so that issue didn't occur to me.

[christopherlam]

I have no objection to the reconcile object. As long as the statement dates can be arbitrary (rather than a Recurrence type offshoot) and there's no obligatory "statement number" string field, I'm happy :-)

[jralls]

Why, since you don't use it?

[christopherlam]

It would be used in 2f6c893#diff-a2fe08453182373caa62b6e813e7050bd5944a14592a1e1f0b9b9607be8e7605R4737 -- but this branch may be mothballed -- not sure how useful it'll be if a reconciliation data structure is introduced in the future.

[jralls]

Mothballed or not, don't use KVP directly, use qof_instance_get instead.