Bug 765920 - Running balance on report is not accurate when report is sorted in in different ways.

[dawansv]

Summary:

• Renamed option to "Account Balance at Transaction" to avoid confusion with running total.
• Added helper function to ensure running balance and balance forward are only shown when transaction are grouped by account and sorted as in register. In that case column heading remains "Running Balance" and balance forward is shown. Otherwwise column heading is renamed "Account Balance" and balance forward is not shown.
• Also added missing code for Common Currency conversion.

More details:

This bug relates to the running account balance option being available and rendered even when the transactions selected and sort order do not allow for a coherent running account balance pulled from the underlying account, including a balance forward amount shown at the top, before the first transaction on the account. The resulting “running balance” is very confusing as the numbers do not actually add up from one line to the next.

A running balance pulled from the underlying account should only be shown when most of the report defaults are maintained. A new helper function in this PR ensures this is the case based on the following criteria:

• The primary sort is set to account name (as per default) or account code (which only works if unique codes are used for each account, otherwise the result is still nonsensical and the current fix does not check that)
• The primary subtotals are displayed (as per default) so the accounts are listed separately with a break between them.
• The secondary sort is set to register order (as per default) or date ascending to match the transaction order on the underlying account. Date ascending is allowed to offer a secondary date grouping subtotal which is compatible with the running balance.
• Detail level is set to “one transaction per line” (as per default). If “one split per line” is selected, the account balance is pulled from the corresponding account every 2 lines, hence breaking up the running balance.
• Date filter is set to date posted (as per default). The code that fetches the balance forward needs a transaction start date for the report which the other date filter options cannot provide.
• Filtering on transactions is kept as per default. Filtering on account is fine. Because closing transactions are excluded by default, we are not excluding that option. However, if closing transactions are present (and not shown as per default) then the running balance will jump back to zero as it crosses the (hidden) closing transaction threshold.

In an earlier PR #1460 , it was discussed to possibly ONLY make the running balance option available in the options when relevant as per above. However one challenge is the disabling (graying) of the option in the option dialog. Because so many underlying options trigger the unavailability of the running balance, and because the code disabling options has to be triggered from each of the other options causing it to be unavailable, it creates a lot of additional code to write and maintain, including converting several simple options that currently have no triggering functions to more complex ones. It seems like a lot of code to maintain for such a side issue.

An alternative proposed at the time avoid that problem. The option (in the dialog) is renamed from “Running Balance” to “Account Balance at Transaction”. However, when the account balance is in fact running (as per the criteria above), the column heading is kept as “Running Balance” and the balance forward is shown. When the balance is not running (as per the criteria above) then the column heading is shown as “Account Balance”.

The renaming also clarifies a related issue. Some users that have commented on the bug report appear think this feature is meant to creates a running total based on the report transactions. The renaming to “Account Balance at Transaction” makes it less likely this will be confused with a running total. A running total feature would be useful however, and I will be submitting a PR later on with a running total enhancement that will implement that feature, separately from this PR.

Finally this PR also adds missing code related to Common Currency conversion for the Account Balance.

[dawansv]

Screenshots
Option renamed in dialog

Report with running balance and balance forward (sorted by account/register ascending)

Report with transactions sorted by description without balance forward showing account balance (note column is renamed)

[christopherlam]

IMHO the trep-engine should be modified more conservatively -- I'm aware of custom reports using, e.g. 'bal-bf to print the account balance at report start date.

[dawansv]

IMHO the trep-engine should be modified more conservatively -- I'm aware of custom reports using, e.g. 'bal-bf to print the account balance at report start date.

Thanks Christopher for the pointer. Ooops you are right. Not only do I need to keep 'bal-bf as is instead of renaming it (that part is easy) but I need to avoid messing with the "add-subheading" function. Will fix and further test...

[dawansv]

IMHO the trep-engine should be modified more conservatively -- I'm aware of custom reports using, e.g. 'bal-bf to print the account balance at report start date.

Ok so I have revised my code and brought back 'bal-bf (instead of renaming it) and also I am no longer changing the "add-subheading" function or any of the generic code in the second part EXCEPT for for the currency conversion code for 'bal-bf.

So with the exception of the currency conversion, code change is either in the option definitions (just for the name change) or in the default-calculated-cells (column definition to decide if bal-bf is shown or not). It is only the default-calculated-cell heading vector definition that is adjusted to decide whether a bal-bf is displayed or not in the transaction report. So a custom report with its own calculated cells can still decide that on its own by either adding the 'bal-bf or not as before.

PS: Also note that the number of lines changed is a bit inflated because I had to realign all lines in the option list with a couple spaces to keep the list lined up with the slightly longer option name "Account Balance at Transaction"

[christopherlam]

I have tried and I'm not sure this change will be warranted. Here's a test book of mine with multiple AUD GBP USD JPY accounts. The report currency is GBP. I've selected all accounts, including the Bank-AUD block which is of interest.

Current report; note the Running Balance (-$2 -$202 -$232) remains in AUD.

Changed report; note the Running Balance is converted to GBP, and the amounts are inexplicably (to the user) different from the Debit(GBP)/Credit(GBP) columns.

In addition, I made a few other editorialising changes to be even more conservative with changes, and won't require changing the columns of the option list. (It's really not necessary for the columns to align!)

modified   gnucash/report/trep-engine.scm
@@ -918,7 +918,7 @@ be excluded from periodic reporting.")
       (list (N_ "Price")                           "l"  (G_ "Display the shares price?") #f)
       ;; note the "Amount" multichoice option in between here
       (list optname-grid                           "m5" (G_ "Display a subtotal summary table.") #f)
-      (list (N_ "Account Balance at Transaction")  "n"  (G_ "Display the balance of the underlying account on each line?") #f)
+      (list (N_ "Account Balance")  "n"  (G_ "Display the balance of the underlying account on each line?") #f)
       (list (N_ "Totals")                          "o"  (G_ "Display the totals?") #t)))
 
     (when BOOK-SPLIT-ACTION
@@ -1033,7 +1033,7 @@ be excluded from periodic reporting.")
                 (and (opt-val pagename-sorting optname-show-subtotals-only)
                      (or (primary-get-info 'renderer-fn)
                          (secondary-get-info 'renderer-fn))))
-          (cons 'account-balance (opt-val gnc:pagename-display (N_ "Account Balance at Transaction")))
+          (cons 'running-balance (opt-val gnc:pagename-display (N_ "Account Balance")))
           (cons 'account-full-name
                 (opt-val gnc:pagename-display (N_ "Use Full Account Name")))
           (cons 'memo (opt-val gnc:pagename-display (N_ "Memo")))
@@ -1100,7 +1100,7 @@ be excluded from periodic reporting.")
     ; - The primary sort is set to account name (or code)
     ; - The primary subtotals are displayed (to separate accounts)
     ; - The secondary sort is set to register order or date ascending.
-    (define (show-bal-bf?)
+    (define show-bal-bf?
       (and (eq? (opt-val gnc:pagename-display optname-detail-level) 'single)
            (eq? (opt-val gnc:pagename-general optname-date-source) 'posted)
            (string-null? (opt-val pagename-filter optname-transaction-matcher))
@@ -1325,8 +1325,8 @@ be excluded from periodic reporting.")
          ;;                             column must be the credit side
          ;;         friendly-heading-fn (friendly-heading-fn account) to retrieve
          ;;                             friendly name for account debit/credit
-         ;;                             or 'bal-bf for balance-brought-forward
-         ;;                             or 'original-bal-bf for bal-bf in original currency
+         ;;                             or 'bal-bf-converted for balance-brought-forward
+         ;;                             or 'bal-bf for bal-bf in original currency
          ;;                             when currency conversion is used
          ;;         start-dual-column?  #t: merge with next cell for subtotal table.
 
@@ -1345,11 +1345,11 @@ be excluded from periodic reporting.")
                            friendly-credit #f))
              '())
 
-         (if (column-uses? 'account-balance)
-             (if (show-bal-bf?)
+         (if (column-uses? 'running-balance)
+             (if show-bal-bf?
                  (list (vector (header-commodity (G_ "Running Balance"))
                            converted-account-balance #t #f #f
-                           'bal-bf #f))
+                           'bal-bf-converted #f))
                  (list (vector (header-commodity (G_ "Account Balance"))
                            converted-account-balance #t #f #f
                            #f #f)))
@@ -1373,11 +1373,11 @@ be excluded from periodic reporting.")
              '())
 
          (if (and (column-uses? 'amount-original-currency)
-                  (column-uses? 'account-balance))
-             (if (show-bal-bf?)
+                  (column-uses? 'running-balance))
+             (if show-bal-bf?
                  (list (vector (G_ "Running Balance")
                            original-account-balance #t #f #f
-                           'original-bal-bf #f))
+                           'bal-bf #f))
                  (list (vector (G_ "Account Balance")
                            original-account-balance #t #f #f
                            #f #f)))
@@ -1422,7 +1422,7 @@ be excluded from periodic reporting.")
                                (case level
                                  ((primary) optname-prime-sortkey)
                                  ((secondary) optname-sec-sortkey))))
-             (data (if (and (any (lambda (c) (eq? 'bal-bf (vector-ref c 5)))
+             (data (if (and (any (lambda (c) (eq? 'bal-bf-converted (vector-ref c 5)))
                                  calculated-cells)
                             (memq sortkey ACCOUNT-SORTING-TYPES))
                        ;; Translators: Balance b/f stands for "Balance
@@ -1454,7 +1454,7 @@ be excluded from periodic reporting.")
              (lambda (cell)
                (match (vector-ref cell 5)
                  (#f #f)
-                 ('bal-bf
+                 ('bal-bf-converted
                   (let* ((acc (xaccSplitGetAccount split))
                          (bal (exchange-fn
                                 (gnc:make-gnc-monetary 
@@ -1469,7 +1469,7 @@ be excluded from periodic reporting.")
                          (gnc:make-html-table-cell/markup
                           "number-cell"
                           (if (acc-reverse? acc) (gnc:monetary-neg bal) bal)))))
-                 ('original-bal-bf
+                 ('bal-bf
                   (let* ((acc (xaccSplitGetAccount split))
                          (bal (xaccAccountGetBalanceAsOfDate acc begindate)))
                     (and (memq sortkey ACCOUNT-SORTING-TYPES)

I'm guessing that you wish to print a genuine running balance as a column, which properly adds/subtracts the Dr/Cr column of the current line? This can be done; however there are 3 subtotal collectors - the global, primary and secondary ones; and it's not obvious (to me) which one must be rendered.

[dawansv]

Thanks Christopher for the feedback. I am currently travelling so will look at it in more details in a few days when I am back.

That said on a cursory look I am not able to see the problem with your second example, the one with the currency conversion. The Account Balance/Running Balance column just pulls the balance from the account at each transaction (that's the existing code) and I just fixed the fact that it didn't follow the currency conversion. In your example I see the balances being converted to the common currency in the last 3 transactions. The Amounts match for each separate account as far as I can see. Each amount is added to the previous balance, but it restarts at each account as it should based on what that feature does. There is also a balance forward for each account that will be different than zero if you start your report at a later date. In any case none of that code has changed nor did I write it. The only changed code is the currency conversion. Possibly I am missing something in your example and will see it when I check later in more details. The running balance works on each separate account by the nature of what this option does. It simply fetches the account balance of the underlying account at each transaction. This is also why it is not "running" when transactions are not sorted by account and register order ascending. In that case my fix renames the column "Account Balance" to at least highlight the fact it is not running.

As for your last comment, I'm trying to clarify what the current implementation of the running balance does to the end user because I don't want it to be confused with the running total that will also be proposing. These will just create running totals based on transactions in the report, no matter the order presented. This is probably what most people think the running balance option does, but it doesn't. In fact that's why I go involved in fixing this, because I was confused why it didn't to that. That feature will work on all 3 total levels (main total, primary and secondary). Unlike the Running Total feature I want to implement, the account balance feature doesn't calculates or totals anything. It simply pulls the account balance as already calculated in the underlying account for each transaction. It just happens to look like a running balance when the report is sorted in the exact same way as the underlying accounts.

So I am not sure which of the changes are not warranted. The renaming of the option to Account Balance at Transaction or the missing currency conversion?

[christopherlam]

That said on a cursory look I am not able to see the problem with your second example, the one with the currency conversion.

You're right, I was missing the fact it was a "Running Balance". My apologies! I'll continue testing and see if it breaks the report in other ways.

[dawansv]

Hi Christopher.

Now that I had a chance to look at your changes, here is another comment related to the switch from 'bal-bf / 'original-bal-bf (in my version) to 'converted-bal-bf / 'bal-bf (in your version).

In my version, 'bal-bf is the main one because as for the columns themselves (debit. credit and account balance) the converted amount is actually the default column as I will show below. When common conversion is not requested as an option, the default columns displayed are still the one with the converted-amount variable as in the example below with the debit/credit columns.

         (if (column-uses? 'amount-double)
             (list (vector (header-commodity (G_ "Debit"))
                           converted-debit-amount #f #t #t
                           friendly-debit #t)
                   (vector (header-commodity (G_ "Credit"))
                           converted-credit-amount #f #t #f
                           friendly-credit #f))
             '())

If you follow the code underlying converted-debit-amount for instance, you'll see it eventually calls a generic converted-amount function that does the currency exchange to the common curency.

       (converted-amount (lambda (s)
                           (exchange-fn
                            (gnc:make-gnc-monetary (split-currency s)
                                                   (split-amount s))
                            (row-currency s)
                            (xaccTransGetDate (xaccSplitGetParent s)))))

The currency is retrieved by calling another generic function, row-currency.

           (row-currency (lambda (s) (if (column-uses? 'common-currency)
                                         (opt-val pagename-currency optname-currency)
                                         (split-currency s))))

But here we see that this function EITHER picks the common currency if the option is enabled, or if it is not, it simply returns the currency of the split, which means that the function exchange-fn ends up converting the amount into its own currency, hence not converting it. So we can see that regular and converted amounts follow the same path, it's just the currency chosen by function row-currency that makes the difference between the 2.

Unlike all of the above, the ORIGINAL-amount usage is different. This ORIGINAL columns are ONLY created when common currency is requested AND the option to also display the original amount is requested as well. So these columns are NOT always presented. They are not the default columns. You can see the extra check on (column-uses? 'amount-original-currency) in this code sample:

         (if (and (column-uses? 'amount-original-currency)
                  (column-uses? 'amount-double))
             (list (vector (G_ "Debit")
                           original-debit-amount #f #t #t
                           friendly-debit #t)
                   (vector (G_ "Credit")
                           original-credit-amount #f #t #f
                           friendly-credit #f))
             '())

You can see the extra check for (column-uses? 'amount-original-currency). These columns are only displayed when that option is selected.

To be clear, the above works exactly the same for the account balance column. I used the debit/credit column in my example to show this applies equally to all of the number columns.

Hence to me the logical treatment of 'bal-bf is that it should apply to the default colomn, which is the converted amount column. Of course 'bal-bf will not be converted when (column-uses? 'common-currency) returns false, following the same logic as the converted columns themselves. See the same logic in the 'bal-bf code below in defining the currency for the exchange function, using the (if (column-uses? 'common-currency) that either picks the common currency selected, or if that option is not enabled, simply the currency of the account (in lieu of the split since here we are at the account level). Hence this should be the default 'bal-bf for custom reports.

                 ('bal-bf
                  (let* ((acc (xaccSplitGetAccount split))
                         (bal (exchange-fn
                                (gnc:make-gnc-monetary 
                                      (xaccAccountGetCommodity acc)
                                      (xaccAccountGetBalanceAsOfDate acc begindate))
                                (if (column-uses? 'common-currency)
                                      (opt-val pagename-currency optname-currency)
                                      (xaccAccountGetCommodity acc))
                                (time64CanonicalDayTime
                                 (xaccTransGetDate (xaccSplitGetParent split))))))
                    (and (memq sortkey ACCOUNT-SORTING-TYPES)
                         (gnc:make-html-table-cell/markup
                          "number-cell"
                          (if (acc-reverse? acc) (gnc:monetary-neg bal) bal)))))

The 'original-bal-bf on the other side is only to be used on the column created when (column-uses? 'amount-original-currency) is true. It's also only used in that case. It should not be the default 'bal-bf.

[dawansv]

I have changed my commits as per some of Christopher's suggestions:

• Changed option name to "Account Balance" instead of "Account Balance at Transaction"
• reverted back to existing 'running-balance cons to reduce code change
• removed unneeded parenthesis around show-bal-bf?

I have not implemented Christopher's suggestions to change 'bal-bf / 'original-bal-bf to 'baf-bf-converted / 'bal-bf. See my previous comment as to why I don't think that is what we want to retain compatibility with existing custom reports

[christopherlam]

Thanks for reducing the change to minimise changed lines. I haven't quite been able to understand 100% fully the criteria for selecting Account vs Running balance. In any case there are test failures -- the headings test must be amended in test-transaction.scm.

If you are able to add a test for the new running balance, it will be appreciated!

[dawansv]

Thanks christopher for the pointer to test-transaction.scm. Will look at it later. I assume I should include whatever changes I made there to the same commit.

As for the criteria selecting Account vs Running balance, essentially the running balance is shown only when all transactions in an account are shown and in the same order as in the register since it's the only time the balance pulled from the register (which is what this option does) will actually be running as in the register. So anything with filters or any sort of ordering other than by account and register order is excluded.

The subtotals are needed because otherwise several accounts would be mixed together with no place to pull the balance forward for each account.

An extra criteria is the date filter because the report engine needs a start date for the transactions themselves to pull the balance forward at that date. The other date filters are not on the transaction date itselft, hence the BF would be pulled at the wrong date.

Finally displaying the full split also doesn't work for a running balance since it pulls balances back and forth from 2 different accounts (the corresponding account) every 2 lines.

Luckily all of these are the default options.

[dawansv]

OK got "Running Balance" > "Account Balance" done in test-transaction.scm

[christopherlam]

I'd think this is nearly ready to merge.

Do you think you can add necessary code into test-transaction.scm to test the conditions to trigger the original 'Running Balance'?

[dawansv]

I'd think this is nearly ready to merge.

Do you think you can add necessary code into test-transaction.scm to test the conditions to trigger the original 'Running Balance'?

Will work on this and get back to you.

[dawansv]

OK so I added a couple tests. Arguably they are relatively basic as they focus mostly on the column heading going from Account Balance to Running Balance and the Balance b/f being displayed.

The first test simply runs on the default options because that's the main scenario when running balance is displayed. The second test runs on the 2 only alternatives to default, which are primary sort by account code and secondary sort by date (I test them together to reduce the number of tests, I could test them separately if better).

I could of course do more tests triggering back to "Account Balance" but then that could be almost an unlimited amount of tests. Hopefully that is enough test for this feature, but if you feel more is needed I am happy to add more.

[christopherlam]

Thanks!

[christopherlam]

More tests are always welcome, especially testing the conditions for triggering running vs account balance header and amount.

[dawansv]

Hi Christopher:

As I was working on my new running total code, I noticed in your commit of this PR you removed the N_ modifier that was still there in my version on line 1036:

        - (cons 'running-balance (opt-val gnc:pagename-display (N_ "Running Balance")))
        + (cons 'running-balance (opt-val gnc:pagename-display "Account Balance"))

I assume that's because we don't need these N_ internationaliztion modifiers here and you are starting to remove them as we update the code? They seem superfluous indeed since these option labels are already marked for translation earlier in the option list at line 901. Just making sure I understand this so I can follow the same pattern for future updates.

[christopherlam]

I assume that's because we don't need these N_ internationaliztion modifiers here and you are starting to remove them as we update the code

That's right

[jralls]

It's safer to leave the markers. Extra ones are harmless and make sure that some future delete of the one that's marked doesn't break translations. If it really bugs you, define a symbol e.g. (define account-bal-label (N_ "Account Balance"))
and then use account-bal-label instead of the literal.