gnucash maint: Bug 752035 - Transaction Report Filter By not Always Working
Geert Janssens
gjanssens at code.gnucash.org
Tue Jul 28 11:13:40 EDT 2015
Updated via https://github.com/Gnucash/gnucash/commit/124a2479 (commit)
from https://github.com/Gnucash/gnucash/commit/3ccaec6e (commit)
commit 124a2479efe8778f1a09cf386b4b9025d1b34049
Author: Geert Janssens <janssens-geert at telenet.be>
Date: Tue Jul 28 17:12:24 2015 +0200
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.
diff --git a/src/engine/Split.c b/src/engine/Split.c
index 64055a3..8d34c23 100644
--- a/src/engine/Split.c
+++ b/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. */
diff --git a/src/engine/Split.h b/src/engine/Split.h
index d240bd8..9b0235b 100644
--- a/src/engine/Split.h
+++ b/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);
diff --git a/src/engine/test/utest-Split.c b/src/engine/test/utest-Split.c
index d3dcc6e..0064a8b 100644
--- a/src/engine/test/utest-Split.c
+++ b/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);
@@ -1311,6 +1325,18 @@ test_get_corr_account_split (Fixture *fixture, gconstpointer pData)
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);
g_assert (!fixture->func->get_corr_account_split(NULL, &result));
diff --git a/src/report/report-system/report-system.scm b/src/report/report-system/report-system.scm
index 9af2efc..9d5a7b9 100644
--- a/src/report/report-system/report-system.scm
+++ b/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)
diff --git a/src/report/report-system/report-utilities.scm b/src/report/report-system/report-utilities.scm
index 61d14ce..7597c82 100644
--- a/src/report/report-system/report-utilities.scm
+++ b/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)
diff --git a/src/report/standard-reports/transaction.scm b/src/report/standard-reports/transaction.scm
index a8b2e60..f010b8b 100644
--- a/src/report/standard-reports/transaction.scm
+++ b/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))
)
)
Summary of changes:
src/engine/Split.c | 36 +++---------
src/engine/Split.h | 6 ++
src/engine/test/utest-Split.c | 26 +++++++++
src/report/report-system/report-system.scm | 1 -
src/report/report-system/report-utilities.scm | 3 -
src/report/standard-reports/transaction.scm | 79 ++++++++++++++-------------
6 files changed, 81 insertions(+), 70 deletions(-)
More information about the gnucash-changes
mailing list