Bug 752035 - Transaction Report Filter By not Always Working

Make sure the internal split function get_corr_account_split
behaves consistently on multi-split transactions. The transaction
report depends on this.

Add test case to catch potential regressions

Simplify filter test function in transaction report.

src/engine/Split.c

@@ -1383,40 +1383,18 @@ get_corr_account_split(const Split *sa, const Split **retval)
 {
 
     const Split *current_split;
-    GList *node;
-    gnc_numeric sa_value, current_value;
-    gboolean sa_value_positive, current_value_positive, seen_one = FALSE;
 
     *retval = NULL;
     g_return_val_if_fail(sa, FALSE);
 
-    sa_value = xaccSplitGetValue (sa);
-    sa_value_positive = gnc_numeric_positive_p(sa_value);
+    if (xaccTransCountSplits (sa->parent) > 2)
+        return FALSE;
 
-    for (node = sa->parent->splits; node; node = node->next)
-    {
-        current_split = node->data;
-        if (current_split == sa) continue;
-
-        if (!xaccTransStillHasSplit(sa->parent, current_split)) continue;
-        current_value = xaccSplitGetValue (current_split);
-        current_value_positive = gnc_numeric_positive_p(current_value);
-        if ((sa_value_positive && !current_value_positive) ||
-                (!sa_value_positive && current_value_positive))
-        {
-            if (seen_one)
-            {
-                *retval = NULL;
-                return FALSE;
-            }
-            else
-            {
-                *retval = current_split;
-                seen_one = TRUE;
-            }
-        }
-    }
-    return seen_one;
+    *retval = xaccSplitGetOtherSplit (sa);
+    if (*retval)
+        return TRUE;
+    else
+        return FALSE;
 }
 
 /* TODO: these static consts can be shared. */

src/engine/Split.h

@@ -441,6 +441,12 @@ int xaccSplitCompareOtherAccountCodes(const Split *sa, const Split *sb);
  * were added for the transaction report, and is in C because the code
  * was already written in C for the above functions and duplication
  * is silly.
+ *
+ * Note that this will only return a real value in case of a
+ * two-split transaction as that is the only situation in which
+ * a reliable value can be returned. In other situations
+ * "-- Split Transaction --" will be returned as Account Name
+ * or "Split" for Account Code.
  */
 
 char * xaccSplitGetCorrAccountFullName(const Split *sa);

src/engine/test/utest-Split.c

@@ -1265,10 +1265,15 @@ test_get_corr_account_split (Fixture *fixture, gconstpointer pData)
     Split *split1 = xaccMallocSplit (book);
     Split *split2 = xaccMallocSplit (book);
     Split *split3 = xaccMallocSplit (book);
+    Split *split4 = xaccMallocSplit (book);
+    Split *split5 = xaccMallocSplit (book);
     const Split *result = NULL;
+    const gnc_numeric factor = gnc_numeric_create (2, 1);
     Account *acc1 = xaccMallocAccount (book);
     Account *acc2 = xaccMallocAccount (book);
     Account *acc3 = xaccMallocAccount (book);
+    Account *acc4 = xaccMallocAccount (book);
+    Account *acc5 = xaccMallocAccount (book);
 #if defined(__clang__) && __clang_major__ < 6
 #define _func "gboolean get_corr_account_split(const Split *, const Split **)"
 #else
@@ -1285,14 +1290,23 @@ test_get_corr_account_split (Fixture *fixture, gconstpointer pData)
     xaccAccountSetCommodity (acc1, fixture->curr);
     xaccAccountSetCommodity (acc2, fixture->curr);
     xaccAccountSetCommodity (acc3, fixture->curr);
+    xaccAccountSetCommodity (acc4, fixture->curr);
+    xaccAccountSetCommodity (acc5, fixture->curr);
 
     split1->acc = acc1;
     split2->acc = acc2;
     split3->acc = acc3;
+    split4->acc = acc4;
+    split5->acc = acc5;
 
     split1->value = gnc_numeric_create (456, 240);
     split2->value = gnc_numeric_neg (fixture->split->value);
     split3->value = gnc_numeric_neg (split1->value);
+    split4->value = gnc_numeric_neg (gnc_numeric_mul (fixture->split->value,
+                                                      factor,
+                                                      GNC_DENOM_AUTO,
+                                                      GNC_HOW_RND_NEVER));
+    split5->value = fixture->split->value;
 
     g_assert (!fixture->func->get_corr_account_split(fixture->split, &result));
     g_assert (result == NULL);
@@ -1309,6 +1323,18 @@ test_get_corr_account_split (Fixture *fixture, gconstpointer pData)
     xaccSplitSetParent (split3, txn);
     xaccTransCommitEdit (txn);
 
+    g_assert (!fixture->func->get_corr_account_split(fixture->split, &result));
+    g_assert (result == NULL);
+
+    /* Test for bug 752035 */
+    xaccTransBeginEdit (txn);
+    xaccSplitSetParent (split1, NULL);
+    xaccSplitSetParent (split2, NULL);
+    xaccSplitSetParent (split3, NULL);
+    xaccSplitSetParent (split4, txn);
+    xaccSplitSetParent (split5, txn);
+    xaccTransCommitEdit (txn);
+
     g_assert (!fixture->func->get_corr_account_split(fixture->split, &result));
     g_assert (result == NULL);
     g_assert_cmpint (check->hits, ==, 0);

src/report/report-system/report-system.scm

@@ -648,7 +648,6 @@
 (export gnc:account-get-type-string-plural)
 (export gnc:accounts-get-commodities)
 (export gnc:get-current-account-tree-depth)
-(export gnc:split-get-corr-account-full-name)
 (export gnc:acccounts-get-all-subaccounts)
 (export gnc:make-stats-collector)
 (export gnc:make-drcr-collector)

src/report/report-system/report-utilities.scm

@@ -143,9 +143,6 @@
   (let ((root (gnc-get-current-root-account)))
     (gnc-account-get-tree-depth root)))
 
-(define (gnc:split-get-corr-account-full-name split)
-  (xaccSplitGetCorrAccountFullName split))
-
 
 ;; Get all children of this list of accounts.
 (define (gnc:acccounts-get-all-subaccounts accountlist)

src/report/standard-reports/transaction.scm

@@ -1378,40 +1378,45 @@ Credit Card, and Income accounts.")))))
      name-sortkey name-subtotal name-date-subtotal
      3 2))
 
-  (define (get-other-account-names account-list)
-    ( map (lambda (acct)  (gnc-account-get-full-name acct)) account-list))
-
-  (define (is-filter-member split account-list splits-ok?)
-    (let ((fullname (gnc:split-get-corr-account-full-name split)))
-
-      (if (string=? fullname (_ "-- Split Transaction --"))
-	  ;; Yep, this is a split transaction.
-
-	  (if splits-ok?
-	      (let* ((txn (xaccSplitGetParent split))
-		     (splits (xaccTransGetSplitList txn)))
-
-		;; Walk through the list of splits.
-		;; if we reach the end, return #f
-		;; if the 'this' != 'split' and the split->account is a member
-		;; of the account-list, then return #t, else recurse
-		(define (is-member splits)
-		  (if (null? splits)
-		      #f
-		      (let* ((this (car splits))
-			     (rest (cdr splits))
-			     (acct (xaccSplitGetAccount this)))
-			(if (and (not (eq? this split))
-				 (member acct account-list))
-			    #t
-			    (is-member rest)))))
-
-		(is-member splits))
-	      #f)
-
-	  ;; Nope, this is a regular transaction
-	  (member fullname (get-other-account-names account-list))
-	  )))
+  ;;(define (get-other-account-names account-list)
+  ;;  ( map (lambda (acct)  (gnc-account-get-full-name acct)) account-list))
+
+  (define (is-filter-member split account-list)
+    (let* ((txn (xaccSplitGetParent split))
+           (splitcount (xaccTransCountSplits txn)))
+
+      (cond
+        ;; A 2-split transaction - test separately so it can be optimized
+        ;; to significantly reduce the number of splits to traverse
+        ;; in guile code
+        ((= splitcount 2)
+         (let* ((other      (xaccSplitGetOtherSplit split))
+                (other-acct (xaccSplitGetAccount other)))
+               (member other-acct account-list)))
+
+        ;; A multi-split transaction - run over all splits
+        ((> splitcount 2)
+         (let ((splits (xaccTransGetSplitList txn)))
+
+                ;; Walk through the list of splits.
+                ;; if we reach the end, return #f
+                ;; if the 'this' != 'split' and the split->account is a member
+                ;; of the account-list, then return #t, else recurse
+                (define (is-member splits)
+                  (if (null? splits)
+                      #f
+                      (let* ((this (car splits))
+                             (rest (cdr splits))
+                             (acct (xaccSplitGetAccount this)))
+                        (if (and (not (eq? this split))
+                                 (member acct account-list))
+                            #t
+                            (is-member rest)))))
+
+                (is-member splits)))
+
+        ;; Single transaction splits
+        (else #f))))
 
 
   (gnc:report-starting reportname)
@@ -1467,7 +1472,7 @@ Credit Card, and Income accounts.")))))
 
           (set! splits (qof-query-run query))
 
-          ;;(gnc:warn "Splits in trep-renderer:" splits)
+          (gnc:warn "Splits in trep-renderer:" splits)
 
 	  ;;(gnc:warn "Filter account names:" (get-other-account-names c_account_2))
 
@@ -1477,7 +1482,7 @@ Credit Card, and Income accounts.")))))
 	      (begin
 		;;(gnc:warn "Including Filter Accounts")
 		(set! splits (filter (lambda (split) 
-				       (is-filter-member split c_account_2 #t))
+				       (is-filter-member split c_account_2))
 				     splits))
 		)
 	      )
@@ -1486,7 +1491,7 @@ Credit Card, and Income accounts.")))))
 	      (begin
 		;;(gnc:warn "Excluding Filter Accounts")
 		(set! splits (filter (lambda (split) 
-				       (not (is-filter-member split c_account_2 #t)))
+				       (not (is-filter-member split c_account_2)))
 				     splits))
 		)
 	      )