From 0cd52ec5fe6d4d35b9a22dc309933898010aaf92 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Sun, 5 Jan 2020 16:18:12 -0800 Subject: [PATCH 1/3] Small whitespace fixup. --- gnucash/import-export/import-account-matcher.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/gnucash/import-export/import-account-matcher.c b/gnucash/import-export/import-account-matcher.c index 7cbbcd7bb4..c0ae5584e6 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; } From d7ccea592a74e78906f770e2943be15c6775cd56 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Mon, 6 Jan 2020 15:16:30 -0800 Subject: [PATCH 2/3] Test the online-id matching of gnc_import_select_account. --- gnucash/import-export/test/CMakeLists.txt | 19 +- .../test/gtest-import-account-matcher.cpp | 182 ++++++++++++++++++ 2 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 gnucash/import-export/test/gtest-import-account-matcher.cpp diff --git a/gnucash/import-export/test/CMakeLists.txt b/gnucash/import-export/test/CMakeLists.txt index 11e92a624f..8d197e57c7 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 0000000000..468b935c8b --- /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 * + * * + * 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@gnu.org * + * * + *******************************************************************/ + +#include +extern "C" +{ +#include +#include +#include +#include +#include +#include +} +#include + +using AccountV = std::vector; +using AccountTypeV = std::vector; +using AccountPair = std::pair; + +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)); +} From 0d5bfd79a6be58668eaeec5a33b719e6d085cda1 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Mon, 6 Jan 2020 09:40:35 -0800 Subject: [PATCH 3/3] 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. --- .../import-export/import-account-matcher.c | 121 ++++++++++++------ .../test/gtest-import-account-matcher.cpp | 30 ++--- 2 files changed, 90 insertions(+), 61 deletions(-) diff --git a/gnucash/import-export/import-account-matcher.c b/gnucash/import-export/import-account-matcher.c index c0ae5584e6..60c8fffeac 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 468b935c8b..004c564f71 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)); }