Created attachment 373506 [details] Multicolumn IS report with zero 2018 column, derived from attached GC xml file The new multicolumn reports are great - much appreciated! The multicolumn income statement does NOT ignore closing bookings within the reporting period. The attached example provides bookings in the years 2018 and 2019 with closing bookings as at YE2018 and YE2019. The multicolumn IS shows an empty 2018 column, an expected 2019 column and an expected total column. In particular the columns do not add up to the displayed total. See the attached jpg of the report. However, the report looks fine when all closing bookings are removed. Please note that there might be a time (or time zone?) issue if closing bookings are posted at the end of the reporting period. Thanks for looking into this! Attached in separate files: - GC sample file - jpeg with resulting report (with closing bookings)
Created attachment 373507 [details] sample GC file with closing bookings
I agree there bug is that some closing entries are not accounted for at date boundaries. Note the *purpose* of the report is to ignore closing entries, so, the Income-Statement Columns should not show "$0.00" revenue/expense. A simple fix is to normalise dates prior to comparing with the <= operator. Try this fix: diff --git a/gnucash/report/standard-reports/balsheet-pnl.scm b/gnucash/report/standard-reports/balsheet-pnl.scm --- a/gnucash/report/standard-reports/balsheet-pnl.scm +++ b/gnucash/report/standard-reports/balsheet-pnl.scm @@ -1181,11 +1181,13 @@ also show overall period profit & loss.")) (closing-adjustment (lambda (account col-idx) (define datepair (col-idx->datepair col-idx)) + (define (date<= . dates) + (apply <= (map time64CanonicalDayTime dates))) (define (include-split? split) (and (equal? (xaccSplitGetAccount split) account) - (<= (car datepair) - (xaccTransGetDate (xaccSplitGetParent split)) - (cdr datepair)))) + (date<= (car datepair) + (xaccTransGetDate (xaccSplitGetParent split)) + (cdr datepair)))) (let ((account-closing-splits (filter include-split? closing-entries))) (gnc:make-gnc-monetary (xaccAccountGetCommodity account)
A more radical fix will be to change the balance accumulator to query the split->closing property, which means we'd need to remove the Closing tab in options. The Closing tab uses a different mechanism to identify them, and would be incompatible with the split->closing property because it'd need C regex instead of guile regex. The Closing tab would only be useful for books created more than 10 years ago, so, it shouldn't be a great loss.
(In reply to Christopher Lam from comment #2) > I agree there bug is that some closing entries are not accounted for at date > boundaries. > > Note the *purpose* of the report is to ignore closing entries, so, the > Income-Statement Columns should not show "$0.00" revenue/expense. Yes, this is why this bug report was issued. :) > A simple fix is to normalise dates prior to comparing with the <= operator. > Try this fix: > > diff --git a/gnucash/report/standard-reports/balsheet-pnl.scm > b/gnucash/report/standard-reports/balsheet-pnl.scm > --- a/gnucash/report/standard-reports/balsheet-pnl.scm > +++ b/gnucash/report/standard-reports/balsheet-pnl.scm > @@ -1181,11 +1181,13 @@ also show overall period profit & loss.")) > (closing-adjustment > (lambda (account col-idx) > (define datepair (col-idx->datepair col-idx)) > + (define (date<= . dates) > + (apply <= (map time64CanonicalDayTime dates))) > (define (include-split? split) > (and (equal? (xaccSplitGetAccount split) account) > - (<= (car datepair) > - (xaccTransGetDate (xaccSplitGetParent split)) > - (cdr datepair)))) > + (date<= (car datepair) > + (xaccTransGetDate (xaccSplitGetParent split)) > + (cdr datepair)))) > (let ((account-closing-splits (filter include-split? > closing-entries))) > (gnc:make-gnc-monetary > (xaccAccountGetCommodity account) Thanks - that works well! Please note the forced line break in the diff above, so probably better to attach a patch file to the ticket next time (as far as I know attachments are removable, but not 100% sure). Fortunately, this diff was not too complicated for me to change it to make it work. Separately, another issue: If the field of the closing booking pattern is empty, the report is empty, i.e. no records at all go into the report. Undesired behaviour. (In reply to Christopher Lam from comment #3) > A more radical fix will be to change the balance accumulator to query the > split->closing property, which means we'd need to remove the Closing tab in > options. The Closing tab uses a different mechanism to identify them, and > would be incompatible with the split->closing property because it'd need C > regex instead of guile regex. > > The Closing tab would only be useful for books created more than 10 years > ago, so, it shouldn't be a great loss. So, in the light of the empty regex pattern bug mentioned above, why not removing the 'Closing' tab from *this* report or at least ignoring the pattern (required to follow the guile regex syntax)? All that given that - if I understand you corretly - for a decade or so closing transactions are internally earmarked as such in the GC file.
Yes there is a dedicated closing flag which applies to every transaction, and is cached for fast access. Nowadays the engine has a dedicated running-balance calculator (xaccSplitGetNoclosingBalance) which ignores closing entries. In balsheet-pnl.scm I'd probably use a similar approach in the balances-list accumulator. The old "Income Statement" has "Entries" tab to identify closing-entries via C regex; but this is now mostly superceded by using the closing-flag. Other income-type reports such as "Income Chart" *should* have the closing-entries options but they don't offer it, and the charts query only the closing-flag. And nobody has complained. Conclusion: closing-entries regex search is now mostly obsolete. All/most income-type reports handle them already via closing-flags set for every closing transactions created in the last 10 years. I think it would be safe, and future-orientated to remove the closing-entries options tab. I'd await further opinion from Geert about this.
(In reply to Christopher Lam from comment #5) > Yes there is a dedicated closing flag which applies to every transaction, > and is cached for fast access. Great - many thanks for the confirmation. I beieve the flag is set in the SLOTS entity. > Conclusion: closing-entries regex search is now mostly obsolete. All/most > income-type reports handle them already via closing-flags set for every > closing transactions created in the last 10 years. I think it would be safe, > and future-orientated to remove the closing-entries options tab. I'd await > further opinion from Geert about this. Understood. Would that mean that the empty closing booking pattern issue (see comment #4) disappears with these changes, too? Thanks.
(In reply to Jannick from comment #6) > Great - many thanks for the confirmation. I beieve the flag is set in the > SLOTS entity. It is. See how it's accessed and cached via https://github.com/Gnucash/gnucash/blob/d409d009fb6bd15fc5d5c2b50e9d3510b373d529/libgnucash/engine/Transaction.c#L2370 > Understood. Would that mean that the empty closing booking pattern issue > (see comment #4) disappears with these changes, too? It should do.
(In reply to Christopher Lam from comment #7) > (In reply to Jannick from comment #6) > > Understood. Would that mean that the empty closing booking pattern issue > > (see comment #4) disappears with these changes, too? > > It should do. Great - I hope that the changes covering the comment #4 issue, too, will find their way into the repo soon. Let's leave this ticket open until then. Many thanks.
Created attachment 373515 [details] Sample patch on current maint to remove Closing tab and rely on txn->closing flag only.
Looks good to me: closing tab removed, closing bookings removed from evaluated record sets. Great - many thanks!
(In reply to Christopher Lam from comment #5) > Conclusion: closing-entries regex search is now mostly obsolete. All/most > income-type reports handle them already via closing-flags set for every > closing transactions created in the last 10 years. I think it would be safe, > and future-orientated to remove the closing-entries options tab. I'd await > further opinion from Geert about this. As it's a behavioural change in a stable report I propose to make this change in master. And be sure to add a small release note for it. @jralls on the topic of release notes, do you want to pursue the idea of using git commit notes for this ?
Geert, yes, but we should discuss it in gnucash-devel rather than a random bug report.
(In reply to Geert Janssens from comment #11) > As it's a behavioural change in a stable report I propose to make this > change in master. And be sure to add a small release note for it. It's still in Experimental :) but I think it's ready to graduate to Asset & Income submenus instead.
(In reply to Christopher Lam from comment #13) > (In reply to Geert Janssens from comment #11) > > As it's a behavioural change in a stable report I propose to make this > > change in master. And be sure to add a small release note for it. > > It's still in Experimental :) So it is... I missed that bit. A smartphone screen is rather small to keep track of a full bug thread :( No objections to making this change on maint then. > but I think it's ready to graduate to Asset & > Income submenus instead. Assuming they are intended to replace the same report under that menu, this on the other hand is something for master. I would make a big announcement on gnucash-user and gnucash-devel about your intention to replace the old report and ask users to test and report any issues they still have to allow for a smooth transition.
(In reply to Geert Janssens from comment #14) > So it is... I missed that bit. A smartphone screen is rather small to keep > track of a full bug thread :( > > No objections to making this change on maint then. So... are we happy to remove the multicolumn balsheet-pnl's Closing tab? i.e. No more closing-entries regex/freetext search? I'd be fine with it too. > > but I think it's ready to graduate to Asset & > > Income submenus instead. > > Assuming they are intended to replace the same report under that menu, this > on the other hand is something for master. I would make a big announcement > on gnucash-user and gnucash-devel about your intention to replace the old > report and ask users to test and report any issues they still have to allow > for a smooth transition. I was not intending to completely replace the old balance-sheet and income-statement. However I (obviously) prefer to use the new ones and degrade old-ones to legacy. My opinion would still be to eradicate eguile <evil grin>. There's no 100% compatible option upgrade path. e.g old balsheet asks 'show accounting-style-rules' (IMHO useless), 'parent-account=text-book style' (IMHO broken), 'include asset/liability/equity section/total'. Moreover old balsheet/income-statement still have the weird 'flatten list to depth limit' which I'm not bothered to emulate. *IF* old regex closing-entries could be made obsolete, there would be some good cleanup of report-utilities.scm.
(In reply to Christopher Lam from comment #15) > (In reply to Geert Janssens from comment #14) > > So it is... I missed that bit. A smartphone screen is rather small to keep > > track of a full bug thread :( > > > > No objections to making this change on maint then. > > So... are we happy to remove the multicolumn balsheet-pnl's Closing tab? > i.e. No more closing-entries regex/freetext search? I'd be fine with it too. > Yes. And make sure to solicit feedback from users after this change has been released. It may be we have to inform users to redo their ancient closing transactions, if they are in a book that has closing transactions from before the closing slot was implemented. If so, it would be nice if we can add this as an actionable note in the 4.0 release notes. > > > but I think it's ready to graduate to Asset & > > > Income submenus instead. > > > > Assuming they are intended to replace the same report under that menu, this > > on the other hand is something for master. I would make a big announcement > > on gnucash-user and gnucash-devel about your intention to replace the old > > report and ask users to test and report any issues they still have to allow > > for a smooth transition. > > I was not intending to completely replace the old balance-sheet and > income-statement. However I (obviously) prefer to use the new ones and > degrade old-ones to legacy. My opinion would still be to eradicate eguile > <evil grin>. > > There's no 100% compatible option upgrade path. e.g old balsheet asks 'show > accounting-style-rules' (IMHO useless), 'parent-account=text-book style' > (IMHO broken), 'include asset/liability/equity section/total'. > > Moreover old balsheet/income-statement still have the weird 'flatten list to > depth limit' which I'm not bothered to emulate. > I strongly prefer *not* to keep "legacy" reports around. They will either bitrot or become an additional maintenance burden. Just look at bug 797468 in which you state you won't fix that problem in the old report. So let's assume we do want to replace reports and let's consider them revamped versions. That means not all old options *have* to be migrated. It is however important to ensure we're not accidentally breaking valid use cases we didn't think of in the new report. So it's worth asking users about this. Which is why I suggested announcing the pending replacement on the mailing lists. Evaluate user feedback and adjust if necessary. Note not all user requests have to be implemented as the user sees it either. Extremely uncommon or unreasonable use cases can always be evaluated against development (and maintenance!) effort. > *IF* old regex closing-entries could be made obsolete, there would be some > good cleanup of report-utilities.scm. I just found it's also used in the current (not your new) Profit & Loss report and perhaps others. As far as I'm concerned they can be removed there as well, but only for master. That's what my comment 11 was really about. And for the best possible user experience this should be well documented in the release notes (including how to possibly fix older closing transactions).
(In reply to Christopher Lam from comment #15) > My opinion would still be to eradicate eguile <evil grin>. > I think with the introduction of the generic css stylesheet and your much improved balance sheet we're much closer to that objective. Functionally, your report covers most of what the eguile report does (except maybe the ability to add a note to the report). The flexible css styling (which really was the only end-user benefit of the eguile reports) is now covered by a generic css stylesheet. What we may still have to evaluate is whether the generated report is semantic enough, that is, whether it adds enough class properties to all html nodes such that they can all be uniquely identified in css. Historically we have never cared about that much due to our very limited stylesheet functionality. However if we want to replace the eguile reports, this is the bit to catch up on.
Removal of 'Closing' tab is pushed to maint.
