gnucash maint: Multiple changes pushed
John Ralls
jralls at code.gnucash.org
Sat Mar 14 14:46:41 EDT 2020
Updated via https://github.com/Gnucash/gnucash/commit/d188bca0 (commit)
via https://github.com/Gnucash/gnucash/commit/0d5bfd79 (commit)
via https://github.com/Gnucash/gnucash/commit/d7ccea59 (commit)
via https://github.com/Gnucash/gnucash/commit/0cd52ec5 (commit)
from https://github.com/Gnucash/gnucash/commit/d744b79b (commit)
commit d188bca06a91b1047d610613053ab1d42e73bd9c
Merge: d744b79be 0d5bfd79a
Author: John Ralls <jralls at ceridwen.us>
Date: Sat Mar 14 11:23:20 2020 -0700
Merge branch 'bug797432bis' into maint.
commit 0d5bfd79a6be58668eaeec5a33b719e6d085cda1
Author: John Ralls <jralls at ceridwen.us>
Date: Mon Jan 6 09:40:35 2020 -0800
Account matching: Test for full and partial online-id matches.
An OFX file import use-case includes a master account with sub accounts
having additional qualifiers appended. 7853f5a2 broke that because the
trailing characters aren't noise.
Instead return only perfect matches (less a trailing space if present,
replacing a second traversal) and cache partial matches and continue the
search. If we find a longer partial match replace the shorter one. If we
find a second account with the same online-id raise an error. If at the
end there's a unambiguous partial match but no full match then it's
either trailing noise from AQBanking or it's a new account. We use the
passed-in new_account_default_type to decide which: If it's
ACCT_TYPE_NONE then we use the partially-matched account, otherwise we
continue on to handle the remaining arguments.
diff --git a/gnucash/import-export/import-account-matcher.c b/gnucash/import-export/import-account-matcher.c
index c0ae5584e..60c8fffea 100644
--- a/gnucash/import-export/import-account-matcher.c
+++ b/gnucash/import-export/import-account-matcher.c
@@ -50,6 +50,23 @@ static QofLogModule log_module = GNC_MOD_IMPORT;
#define GNC_PREFS_GROUP "dialogs.import.generic.account-picker"
+typedef struct
+{
+ Account* partial_match;
+ int count;
+ const char* online_id;
+} AccountOnlineMatch;
+
+static Account*
+partial_match_if_valid (AccountOnlineMatch *match)
+{
+ if (match->partial_match && match->count == 1)
+ return match->partial_match;
+ else
+ PERR("Online ID %s partially matches %d accounts and fully matches none",
+ match->online_id, match->count);
+ return NULL;
+}
/*-******************************************************************\
* Functions needed by gnc_import_select_account
@@ -81,19 +98,66 @@ static AccountPickerDialog* gnc_import_new_account_picker(void)
*
* test for match of account online_ids.
**************************************************/
-static gpointer test_acct_online_id_match(Account *acct, gpointer param_online_id)
+static gpointer test_acct_online_id_match(Account *acct, gpointer data)
{
- const gchar * current_online_id = gnc_import_get_acc_online_id(acct);
- if ((current_online_id != NULL && param_online_id != NULL)
- && strncmp (current_online_id, param_online_id,
- strlen (current_online_id)) == 0)
- {
- return (gpointer *) acct;
- }
- else
- {
+ AccountOnlineMatch *match = (AccountOnlineMatch*)data;
+ const char *acct_online_id = gnc_import_get_acc_online_id(acct);
+ int acct_len, match_len;
+
+ if (acct_online_id == NULL || match->online_id == NULL)
return NULL;
+
+ acct_len = strlen(acct_online_id);
+ match_len = strlen(match->online_id);
+
+ if (acct_online_id[acct_len - 1] == ' ')
+ --acct_len;
+ if (match->online_id[match_len - 1] == ' ')
+ --match_len;
+
+ if (strncmp (acct_online_id, match->online_id, acct_len) == 0)
+ {
+ if (strncmp(acct_online_id, match->online_id, match_len) == 0)
+ return (gpointer *) acct;
+ if (match->partial_match == NULL)
+ {
+ match->partial_match = acct;
+ ++match->count;
+ }
+ else
+ {
+ const char *partial_online_id =
+ gnc_import_get_acc_online_id(match->partial_match);
+ int partial_len = strlen(partial_online_id);
+ if (partial_online_id[partial_len - 1] == ' ')
+ --partial_len;
+ /* Both partial_online_id and acct_online_id are substrings of
+ * match->online_id, but whichever is longer is the better match.
+ * Reset match->count to 1 just in case there was ambiguity on the
+ * shorter partial match.
+ */
+ if (partial_len < acct_len)
+ {
+ match->partial_match = acct;
+ match->count = 1;
+ }
+ /* If they're the same size then there are two accounts with the
+ * same online id and we don't know which one to select. Increment
+ * match->count to dissuade gnc_import_find_account from using
+ * match->online_id and log an error.
+ */
+ else if (partial_len == acct_len)
+ {
+ ++match->count;
+ PERR("Accounts %s and %s have the same online-id %s",
+ gnc_account_get_full_name(match->partial_match),
+ gnc_account_get_full_name(acct),
+ partial_online_id);
+ }
+ }
}
+
+ return NULL;
}
@@ -266,39 +330,14 @@ Account * gnc_import_select_account(GtkWidget *parent,
/*DEBUG("Looking for account with online_id: \"%s\"", account_online_id_value);*/
if (account_online_id_value != NULL)
{
+ AccountOnlineMatch match = {NULL, 0, account_online_id_value};
retval =
gnc_account_foreach_descendant_until(gnc_get_current_root_account (),
- test_acct_online_id_match,
- /* This argument will only be used as a "const char*" */
- (void*)account_online_id_value);
-
- /* BEGIN: try again without extra space at the end */
- /*
- * libofx, used for file import, generates online_id as
- * ACCTID + space + ACCTKEY which differs from the online_id
- * generated by aqbanking for online ofx transfer as ACCTID.
- *
- * If a gnucash account has been associated with an online_id
- * via aqbanking data, it is not possible to construct an OFX
- * file for gnucash import that matches the same online_id
- * because even with no ACCTKEY in the file, there will be a
- * trailing space.
- *
- * This is a hack to overcome that problem.
- */
- if ((retval == NULL) && g_str_has_suffix(account_online_id_value, " "))
- {
- gchar *trimmed = g_strndup(account_online_id_value, strlen(account_online_id_value) - 1);
- if (trimmed)
- {
- retval = gnc_account_foreach_descendant_until(
- gnc_get_current_root_account (),
- test_acct_online_id_match,
- (void *)trimmed);
- }
- g_free(trimmed);
- }
- /* END: try again without extra space at the end */
+ test_acct_online_id_match,
+ (void*)&match);
+ if (!retval && match.count == 1 &&
+ new_account_default_type == ACCT_TYPE_NONE)
+ retval = match.partial_match;
}
if (retval == NULL && auto_create != 0)
{
diff --git a/gnucash/import-export/test/gtest-import-account-matcher.cpp b/gnucash/import-export/test/gtest-import-account-matcher.cpp
index 468b935c8..004c564f7 100644
--- a/gnucash/import-export/test/gtest-import-account-matcher.cpp
+++ b/gnucash/import-export/test/gtest-import-account-matcher.cpp
@@ -76,7 +76,7 @@ protected:
create_account(stocks, ACCT_TYPE_STOCK, "HPE", "BrokerStocksHPE");
create_account(broker, ACCT_TYPE_BANK, "Cash Management",
"BrokerCash Management");
- create_account(expenses, ACCT_TYPE_EXPENSE, "Food", nullptr);
+ create_account(expenses, ACCT_TYPE_EXPENSE, "Food", nullptr);
create_account(expenses, ACCT_TYPE_EXPENSE, "Gas", nullptr);
create_account(expenses, ACCT_TYPE_EXPENSE, "Rent", nullptr);
}
@@ -115,8 +115,7 @@ TEST_F(ImportMatcherTest, test_match_with_subaccounts)
nullptr, nullptr, ACCT_TYPE_NONE,
nullptr, nullptr);
ASSERT_NE(nullptr, found);
- // Should be equal
- EXPECT_STRNE("Stocks", xaccAccountGetName(found));
+ EXPECT_STREQ("Stocks", xaccAccountGetName(found));
}
TEST_F(ImportMatcherTest, test_subaccount_match)
@@ -125,8 +124,7 @@ TEST_F(ImportMatcherTest, test_subaccount_match)
nullptr, nullptr, ACCT_TYPE_NONE,
nullptr, nullptr);
ASSERT_NE(nullptr, found);
- //should be equal
- EXPECT_STRNE("HPE", xaccAccountGetName(found));
+ EXPECT_STREQ("HPE", xaccAccountGetName(found));
}
TEST_F(ImportMatcherTest, test_subaccount_match_trailing_noise)
@@ -134,21 +132,16 @@ TEST_F(ImportMatcherTest, test_subaccount_match_trailing_noise)
auto found = gnc_import_select_account(nullptr, "BrokerStocksHPEUSD", FALSE,
nullptr, nullptr, ACCT_TYPE_NONE,
nullptr, nullptr);
- // Should be equal
- EXPECT_STRNE("HPE", xaccAccountGetName(found));
+ ASSERT_NE(nullptr, found);
+ EXPECT_STREQ("HPE", xaccAccountGetName(found));
}
TEST_F(ImportMatcherTest, test_subaccount_no_match)
{
auto found = gnc_import_select_account(nullptr, "BrokerStocksINTC", FALSE,
- nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr, ACCT_TYPE_STOCK,
nullptr, nullptr);
-/* We really want nullptr in this case, but the algorithm can't tell the
- * difference between trailing noise and a new security that needs a new
- * account.
- */
- ASSERT_NE(nullptr, found);
- EXPECT_STREQ("Stocks", xaccAccountGetName(found));
+ ASSERT_EQ(nullptr, found);
}
TEST_F(ImportMatcherTest, test_subaccount_match_trailing_space)
@@ -157,8 +150,7 @@ TEST_F(ImportMatcherTest, test_subaccount_match_trailing_space)
nullptr, nullptr, ACCT_TYPE_NONE,
nullptr, nullptr);
ASSERT_NE(nullptr, found);
- //Should be equal
- EXPECT_STRNE("MSFT", xaccAccountGetName(found));
+ EXPECT_STREQ("MSFT", xaccAccountGetName(found));
}
TEST_F(ImportMatcherTest, test_subaccount_match_trim_trailing_space)
@@ -167,8 +159,7 @@ TEST_F(ImportMatcherTest, test_subaccount_match_trim_trailing_space)
nullptr, nullptr, ACCT_TYPE_NONE,
nullptr, nullptr);
ASSERT_NE(nullptr, found);
- //Should be equal
- EXPECT_STRNE("MSFT", xaccAccountGetName(found));
+ EXPECT_STREQ("MSFT", xaccAccountGetName(found));
}
TEST_F(ImportMatcherTest, test_subaccount_match_internal_space)
@@ -177,6 +168,5 @@ TEST_F(ImportMatcherTest, test_subaccount_match_internal_space)
FALSE, nullptr, nullptr,
ACCT_TYPE_NONE, nullptr, nullptr);
ASSERT_NE(nullptr, found);
- // Should be equal, of course
- EXPECT_STRNE("Cash Management", xaccAccountGetName(found));
+ EXPECT_STREQ("Cash Management", xaccAccountGetName(found));
}
commit d7ccea592a74e78906f770e2943be15c6775cd56
Author: John Ralls <jralls at ceridwen.us>
Date: Mon Jan 6 15:16:30 2020 -0800
Test the online-id matching of gnc_import_select_account.
diff --git a/gnucash/import-export/test/CMakeLists.txt b/gnucash/import-export/test/CMakeLists.txt
index 11e92a624..8d197e57c 100644
--- a/gnucash/import-export/test/CMakeLists.txt
+++ b/gnucash/import-export/test/CMakeLists.txt
@@ -22,5 +22,22 @@ gnc_add_test(test-link-generic-import test-link.c
gnc_add_test(test-import-pending-matches test-import-pending-matches.cpp
GENERIC_IMPORT_TEST_INCLUDE_DIRS GENERIC_IMPORT_TEST_LIBS
)
+
+set(IMPORT_ACCOUNT_MATCHER_TEST_INCLUDE_DIRS
+ ${CMAKE_BINARY_DIR}/common # for config.h
+ ${CMAKE_SOURCE_DIR}/gnucash/import-export
+ ${CMAKE_SOURCE_DIR}/libgnucash/engine
+ ${CMAKE_SOURCE_DIR}/libgnucash/app-utils
+ ${CMAKE_SOURCE_DIR}/gnucash/gnome-utils
+ ${GLIB2_INCLUDE_DIRS}
+ ${GTK3_INCLUDE_DIRS}
+ ${GTEST_INCLUDE_DIR}
+ )
+
+set(IMPORT_ACCOUNT_MATCHER_TEST_LIBS gncmod-generic-import gncmod-engine test-core ${GTEST_LIB})
+gnc_add_test(test-import-account-matcher gtest-import-account-matcher.cpp
+ IMPORT_ACCOUNT_MATCHER_TEST_INCLUDE_DIRS IMPORT_ACCOUNT_MATCHER_TEST_LIBS)
+
set_dist_list(test_generic_import_DIST CMakeLists.txt
- test-link.c test-import-parse.c test-import-pending-matches.cpp)
+ test-link.c test-import-parse.c test-import-pending-matches.cpp
+ gtest-import-account-matcher.cpp)
diff --git a/gnucash/import-export/test/gtest-import-account-matcher.cpp b/gnucash/import-export/test/gtest-import-account-matcher.cpp
new file mode 100644
index 000000000..468b935c8
--- /dev/null
+++ b/gnucash/import-export/test/gtest-import-account-matcher.cpp
@@ -0,0 +1,182 @@
+/********************************************************************
+ * gtest-import-account-matcher.cpp -- *
+ * unit tests import-account-matcher. *
+ * Copyright (C) 2020 John Ralls <jralls at ceridwen.us> *
+ * *
+ * This program is free software; you can redistribute it and/or *
+ * modify it under the terms of the GNU General Public License as *
+ * published by the Free Software Foundation; either version 2 of *
+ * the License, or (at your option) any later version. *
+ * *
+ * This program is distributed in the hope that it will be useful, *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
+ * GNU General Public License for more details. *
+ * *
+ * You should have received a copy of the GNU General Public License*
+ * along with this program; if not, contact: *
+ * *
+ * Free Software Foundation Voice: +1-617-542-5942 *
+ * 51 Franklin Street, Fifth Floor Fax: +1-617-542-2652 *
+ * Boston, MA 02110-1301, USA gnu at gnu.org *
+ * *
+ *******************************************************************/
+
+#include <gtest/gtest.h>
+extern "C"
+{
+#include <config.h>
+#include <import-account-matcher.h>
+#include <gnc-session.h>
+#include <qofbook.h>
+#include <Account.h>
+#include <gtk/gtk.h>
+}
+#include <vector>
+
+using AccountV = std::vector<const Account*>;
+using AccountTypeV = std::vector<GNCAccountType>;
+using AccountPair = std::pair<AccountV&,
+ const AccountTypeV&>;
+
+class ImportMatcherTest : public ::testing::Test
+{
+protected:
+ ImportMatcherTest() :
+ m_book{gnc_get_current_book()}, m_root{gnc_account_create_root(m_book)}
+ {
+ auto create_account = [this](Account* parent, GNCAccountType type,
+ const char* name,
+ const char* online)->Account* {
+ auto account = xaccMallocAccount(this->m_book);
+ xaccAccountBeginEdit(account);
+ xaccAccountSetType(account, type);
+ xaccAccountSetName(account, name);
+ xaccAccountBeginEdit(parent);
+ gnc_account_append_child(parent, account);
+ if (online)
+ qof_instance_set(QOF_INSTANCE(account), "online-id", online, NULL);
+ xaccAccountCommitEdit(parent);
+ xaccAccountCommitEdit(account);
+ return account;
+ };
+ auto assets = create_account(m_root, ACCT_TYPE_ASSET,
+ "Assets", nullptr);
+ auto liabilities = create_account(m_root, ACCT_TYPE_LIABILITY,
+ "Liabilities", nullptr);
+ auto expenses = create_account(m_root, ACCT_TYPE_EXPENSE,
+ "Expenses", nullptr);
+ create_account(assets, ACCT_TYPE_BANK, "Bank", "Bank");
+ auto broker = create_account(assets, ACCT_TYPE_ASSET,
+ "Broker", "Broker");
+ auto stocks = create_account(broker, ACCT_TYPE_STOCK,
+ "Stocks", "BrokerStocks");
+ create_account(stocks, ACCT_TYPE_STOCK, "AAPL", "BrokerStocksAAPL");
+ create_account(stocks, ACCT_TYPE_STOCK, "MSFT", "BrokerStocksMSFT ");
+ create_account(stocks, ACCT_TYPE_STOCK, "HPE", "BrokerStocksHPE");
+ create_account(broker, ACCT_TYPE_BANK, "Cash Management",
+ "BrokerCash Management");
+ create_account(expenses, ACCT_TYPE_EXPENSE, "Food", nullptr);
+ create_account(expenses, ACCT_TYPE_EXPENSE, "Gas", nullptr);
+ create_account(expenses, ACCT_TYPE_EXPENSE, "Rent", nullptr);
+ }
+ ~ImportMatcherTest()
+ {
+ xaccAccountBeginEdit(m_root);
+ xaccAccountDestroy(m_root); //It does the commit
+ gnc_clear_current_session();
+ }
+
+ QofBook* m_book;
+ Account* m_root;
+};
+
+TEST_F(ImportMatcherTest, test_simple_match)
+{
+ auto found = gnc_import_select_account(nullptr, "Bank", FALSE, nullptr,
+ nullptr, ACCT_TYPE_NONE, nullptr,
+ nullptr);
+ ASSERT_NE(nullptr, found);
+ EXPECT_STREQ("Bank", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_noisy_match)
+{
+ auto found = gnc_import_select_account(nullptr, "BankUSD", FALSE, nullptr,
+ nullptr, ACCT_TYPE_NONE, nullptr,
+ nullptr);
+ ASSERT_NE(nullptr, found);
+ EXPECT_STREQ("Bank", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_match_with_subaccounts)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerStocks", FALSE,
+ nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr);
+ ASSERT_NE(nullptr, found);
+ // Should be equal
+ EXPECT_STRNE("Stocks", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_subaccount_match)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerStocksHPE", FALSE,
+ nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr);
+ ASSERT_NE(nullptr, found);
+ //should be equal
+ EXPECT_STRNE("HPE", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_subaccount_match_trailing_noise)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerStocksHPEUSD", FALSE,
+ nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr);
+ // Should be equal
+ EXPECT_STRNE("HPE", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_subaccount_no_match)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerStocksINTC", FALSE,
+ nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr);
+/* We really want nullptr in this case, but the algorithm can't tell the
+ * difference between trailing noise and a new security that needs a new
+ * account.
+ */
+ ASSERT_NE(nullptr, found);
+ EXPECT_STREQ("Stocks", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_subaccount_match_trailing_space)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerStocksMSFT ", FALSE,
+ nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr);
+ ASSERT_NE(nullptr, found);
+ //Should be equal
+ EXPECT_STRNE("MSFT", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_subaccount_match_trim_trailing_space)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerStocksMSFT", FALSE,
+ nullptr, nullptr, ACCT_TYPE_NONE,
+ nullptr, nullptr);
+ ASSERT_NE(nullptr, found);
+ //Should be equal
+ EXPECT_STRNE("MSFT", xaccAccountGetName(found));
+}
+
+TEST_F(ImportMatcherTest, test_subaccount_match_internal_space)
+{
+ auto found = gnc_import_select_account(nullptr, "BrokerCash Management",
+ FALSE, nullptr, nullptr,
+ ACCT_TYPE_NONE, nullptr, nullptr);
+ ASSERT_NE(nullptr, found);
+ // Should be equal, of course
+ EXPECT_STRNE("Cash Management", xaccAccountGetName(found));
+}
commit 0cd52ec5fe6d4d35b9a22dc309933898010aaf92
Author: John Ralls <jralls at ceridwen.us>
Date: Sun Jan 5 16:18:12 2020 -0800
Small whitespace fixup.
diff --git a/gnucash/import-export/import-account-matcher.c b/gnucash/import-export/import-account-matcher.c
index 7cbbcd7bb..c0ae5584e 100644
--- a/gnucash/import-export/import-account-matcher.c
+++ b/gnucash/import-export/import-account-matcher.c
@@ -84,10 +84,9 @@ static AccountPickerDialog* gnc_import_new_account_picker(void)
static gpointer test_acct_online_id_match(Account *acct, gpointer param_online_id)
{
const gchar * current_online_id = gnc_import_get_acc_online_id(acct);
- if ( (current_online_id != NULL
- && param_online_id != NULL )
- && strncmp( current_online_id, param_online_id,
- strlen( current_online_id ) ) == 0 )
+ if ((current_online_id != NULL && param_online_id != NULL)
+ && strncmp (current_online_id, param_online_id,
+ strlen (current_online_id)) == 0)
{
return (gpointer *) acct;
}
Summary of changes:
gnucash/import-export/import-account-matcher.c | 122 ++++++++++-----
gnucash/import-export/test/CMakeLists.txt | 19 ++-
.../test/gtest-import-account-matcher.cpp | 172 +++++++++++++++++++++
3 files changed, 270 insertions(+), 43 deletions(-)
create mode 100644 gnucash/import-export/test/gtest-import-account-matcher.cpp
More information about the gnucash-changes
mailing list