Created attachment 373460 [details]
A vendor payment with an extra split in an AR account

The first transaction is a payment transaction for a vendor (which typically goes into an AP account). For an experiment unrelated to this bug I once manually added an AR split (normally for customer payments) to this transaction.

The experiment tried to model a business relation that's both a Vendor and a Customer (like for your electrician also buys one of your products). Gnucash has no support for this, so it's really an extremely oddball transaction. Yet there's currently no code to prevent a user from making such a transaction and the backtrace from the report is rather daunting for an ordinary user. So at least the report should at least not crash, and ideally it would point out to the user it encountered transactions it doesn't know how to process.

Below is the backtrace for this particular issue:

In ice-9/eval-string.scm:
     38:6 19 (read-and-eval #<input: string 112b150> #:lang _)
In report-core.scm:
    735:4 18 (gnc:report-run _)
In unknown file:
          17 (gnc-set-busy-cursor () #t)
In c-interface.scm:
     22:4 16 (gnc:call-with-error-handling _ _)
In ice-9/boot-9.scm:
    829:9 15 (catch #t #<procedure 130e180 at c-interface.scm:23:6 ()> #<procedure 15cf4c0 at c-interface.scm:28:6 (key . parameters)> _)
In c-interface.scm:
    27:37 14 (_)
In unknown file:
          13 (eval-string "(gnc:report-run 37)" #<undefined>)
In ice-9/boot-9.scm:
   2312:4 12 (save-module-excursion #<procedure 15cf460 at ice-9/eval-string.scm:66:9 ()>)
In ice-9/eval-string.scm:
     38:6 11 (read-and-eval #<input: string 112b070> #:lang _)
In report-core.scm:
    736:4 10 (gnc:report-run _)
In c-interface.scm:
    64:23  9 (gnc:backtrace-if-exception _ . _)
     22:4  8 (gnc:call-with-error-handling _ _)
In ice-9/boot-9.scm:
    829:9  7 (catch #t #<procedure 130e0f0 at c-interface.scm:23:6 ()> #<procedure 15cf2a0 at c-interface.scm:28:6 (key . parameters)> _)
In c-interface.scm:
    26:40  6 (_)
In report-core.scm:
   740:24  5 (_)
   716:25  4 (gnc:report-render-html #<<report> type: 9cf76bed17f14401b8e3e22d0079cb98-new id: 37 options: #<procedure dispatch (key)> dirty?: #t needs-save?: #t editor-widget: #f ctext: #f custom-template: > #t)
In standard/new-aging.scm:
   335:37  3 (aging-renderer _ #t)
In utilities.scm:
   209:26  2 (sort-and-delete-duplicates (#<swig-pointer GncOwner * 3538f60> #<swig-pointer GncOwner * 3538fa0> #<swig-pointer GncOwner * 3538fc0> #<swig-pointer GncOwner * 3538fe0> #<swig-pointer GncOwner * 3539000> #<swig-point…> …) …)
In unknown file:
           1 (sort (#<swig-pointer GncOwner * 3538f60> #<swig-pointer GncOwner * 3538fa0> #<swig-pointer GncOwner * 3538fc0> #<swig-pointer GncOwner * 3538fe0> #<swig-pointer GncOwner * 3539000> #<swig-pointer GncOwner * 3539020> # …) …)
           0 (string<? "e829579babdeca24d2b041105f81f8fb" #f)

The second issue happens when running the new Aging Payable report. Below is the backtrace. It looks similar, but in this case both the first and the second parameter to the sort are #f.

          17 (apply-smob/1 #<catch-closure 11ecac0>)
In c-interface.scm:
     22:4 16 (gnc:call-with-error-handling _ _)
In ice-9/boot-9.scm:
    829:9 15 (catch _ _ #<procedure 1ccdea0 at c-interface.scm:28:6 (key . parameters)> _)
In c-interface.scm:
    27:37 14 (_)
In unknown file:
          13 (eval-string "(gnc:report-run 38)" #<undefined>)
In ice-9/boot-9.scm:
   2312:4 12 (save-module-excursion _)
In ice-9/eval-string.scm:
     38:6 11 (read-and-eval #<input: string 1e3de00> #:lang _)
In report-core.scm:
    737:4 10 (gnc:report-run _)
In c-interface.scm:
    64:23  9 (gnc:backtrace-if-exception _ . _)
     22:4  8 (gnc:call-with-error-handling _ _)
In ice-9/boot-9.scm:
    829:9  7 (catch _ _ #<procedure 1ccdbe0 at c-interface.scm:28:6 (key . parameters)> _)
In c-interface.scm:
    26:40  6 (_)
In report-core.scm:
   741:24  5 (_)
   717:25  4 (gnc:report-render-html #<<report> type: e57770f2dbca46619d6dac4ac5469b50-new id: 38 options: #<procedure dispatch (key)> dirty?: #t needs-save?: #t editor-widget: #f ctext: #f custom-template: > #t)
In standard/new-aging.scm:
   343:38  3 (aging-renderer _ #f)
In utilities.scm:
   206:26  2 (sort-and-delete-duplicates (#<swig-pointer GncOwner * 57203b0> #<swig-pointer GncOwner * 57203d0> #<swig-pointer GncOwner * 57203f0> #<swig-pointer GncOwner * 5720410> #<swig-pointer GncOwner * 5720430> #<swig-point…> …) …)
In unknown file:
           1 (sort (#<swig-pointer GncOwner * 57203b0> #<swig-pointer GncOwner * 57203d0> #<swig-pointer GncOwner * 57203f0> #<swig-pointer GncOwner * 5720410> #<swig-pointer GncOwner * 5720430> #<swig-pointer GncOwner * 5720450> # …) …)
           0 (string<? #f #f)


At this point I don't know yet which transaction would be causing this. I have five candidates. 4 of them display are invoice transactions in the AP register, but are not marked as type invoice in the lot viewer. One suggests to be a payment but is of type "?" in the AP register. I'll need some more time to evaluate this. I'll come back to this bit later.

For the record - Christopher Lam provided me with this patch to debug which splits may be faulty:



modified   gnucash/report/reports/standard/new-aging.scm
@@ -332,6 +332,13 @@ exist but have no suitable transactions."))
                    (acc-splits (car splits-acc-others))
                    (other-acc-splits (cdr splits-acc-others))
                    (split-owners (map split->owner acc-splits))
+                   (dummy
+                    (begin
+                      (for-each (lambda (split owner)
+                                  (gnc:pk split '-> owner))
+                                acc-splits
+                                split-owners)
+                      #f))
                    (acc-owners (sort (sort-and-delete-duplicates
                                       split-owners ownerGUID<? gnc-owner-equal?)
                                      owner<?)))

I've just created a Vendor prepayment, and modify the txn manually to include a split to AR.

Then I run the receivable aging (beta) report. It crashes.

Let's consider this weird payment txn:

AP splits: s1 - pays 1 vendor bill - this split is part of a vendor-bill lot
AR split: s2 - no lot assigned to this split
non-APAR: split: s3 - bank txn

For the Receivable-Aging(beta) new-aging report, s2 *is* included because the parent transaction is of txn-type-payment type, and it calls (split->owner s3) to find owner... this is bound to fail.

2 options to fix crash:

1) silently exclude s2: it cannot find split->owner. exclude it from calculations

@@ -331,7 +332,7 @@ exist but have no suitable transactions."))
            (splits-acc-others (list-split splits split-from-acct? account))
            (acc-splits (car splits-acc-others))
            (other-acc-splits (cdr splits-acc-others))
-           (split-owners (map split->owner acc-splits))
+           (split-owners (filter gncOwnerIsValid (map split->owner acc-splits)))
            (acc-owners (sort (sort-and-delete-duplicates
                               split-owners ownerGUID<? gnc-owner-equal?)
                               owner<?)))

2) include s2 but unpredictable output:

@@ -216,7 +216,8 @@ exist but have no suitable transactions.")) 
     ;; for sorting and delete-duplicates. compare GUIDs
     (define (ownerGUID<? a b)
-      (string<? (gncOwnerGetGUID a) (gncOwnerGetGUID b)))
+      (string<? (or (gncOwnerGetGUID a) (gnc:owner-get-name-and-address-dep a))
+                (or (gncOwnerGetGUID b) (gnc:owner-get-name-and-address-dep b))))
 
     ;; for presentation. compare names.
     (define (owner<? a b)


This is a corner case that will currently make the aging-list different between new-aging and new-owner-report.

How should we interpret this type of txn?

P.S. a test datafile is always welcome!

Hmm reviewing the code I don't know what's best.

The owner fails the gncOwnerIsValid test. Excluding it is an easy strategy. Otherwise the following can happen:

We can create damaged transactions as follows:
- process $20 payment to vendor, not attached to bill
- modify split manually reversing the signs
- add a $5 split manually to AR account, fixing others to prevent imbalance
- run receivable aging (beta) report whereby this $5 is now classed as an overpayment and will appear in my report, separate from any other customer.

No matter how foolproof I make it, I feel there will always be more foolish transactions.

Here's a better fix.

It only processes valid owners, and invalid owners are properly freed ;) I think it's a much better fix.

modified   gnucash/report/business-reports/new-aging.scm
@@ -332,7 +332,14 @@ exist but have no suitable transactions."))
                    (splits-acc-others (list-split splits split-from-acct? account))
                    (acc-splits (car splits-acc-others))
                    (other-acc-splits (cdr splits-acc-others))
-                   (split-owners (map split->owner acc-splits))
+                   (split-owners
+                    (fold
+                     (lambda (a b)
+                       (let ((owner (split->owner a)))
+                         (cond
+                          ((gncOwnerIsValid owner) (cons owner b))
+                          (else (gncOwnerFree owner) b))))
+                     '() acc-splits))
                    (acc-owners (sort (sort-and-delete-duplicates
                                       split-owners ownerGUID<? gnc-owner-equal?)
                                      owner<?)))

In other words we can fix this crash, which ignores the errant payment split in AR account.

I suspect someone can still craft a similar payment transaction whereby the AP split belongs to a bill lot, and AR split belongs to a invoice lot. I suspect the aging report will then process the bill/invoice owner aging list properly.

I didn't pepper the report with too many invalid-object handlers, partly for code clarity, and partly because I could not claim to understand every combination of weird transactions in existence.

For now though the above will fix it satisfactorily.

Last word before applying fix:

do we want to add something like

(set! invalid-splits (cons a invalid-splits))

to display a list of unprocessed/weird transactions, either as gnc-warning-dialog or as a <li> list at the end of the report?

That's what I wanted to suggest, but apparently my comment got lost in a mid-air collision.

While The original reports don't do this, I prefer the user to know when something irregular happened, rather than just quietly omitting the results.

fixed in maint, I'll merge to master soon.

I don't think it matters for new-aging (which tracks amounts in APAR currency) but old-aging (which tracks amounts in transaction currency :-/) will loudly complain via gnc-warning-dialog when transaction-currency != owner-currency. The complaint was never actually triggered because that code path was very buggy (and I'd fixed in https://github.com/Gnucash/gnucash/commit/e83938fdc2ae8b26070d1dd08f91f0240af75bd8 -- see the invalid call '(gnc-ommodity-get-mnemonic(company-get-currency company-info)))))'). I think gnc-error-dialog is too intrusive and I'd rather clutter the report html instead with the invalid split list.