Merge branch 'bug797432bis' into maint.

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,20 +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);
+    AccountOnlineMatch *match = (AccountOnlineMatch*)data;
-    if ( (current_online_id != NULL
+    const char *acct_online_id = gnc_import_get_acc_online_id(acct);
-            && param_online_id != NULL )
+    int acct_len, match_len;
-	 && strncmp( current_online_id, param_online_id,
+
-		     strlen( current_online_id ) ) == 0 )
+    if (acct_online_id == NULL || match->online_id == NULL)
-    {
-        return (gpointer *) acct;
-    }
-    else
-    {
         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;
 }
 
 
@@ -267,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,
+                                                 test_acct_online_id_match,
-                    /* This argument will only be used as a "const char*" */
+                                                 (void*)&match);
-                    (void*)account_online_id_value);
+        if (!retval && match.count == 1 &&
-
+            new_account_default_type == ACCT_TYPE_NONE)
-        /* BEGIN: try again without extra space at the end */
+            retval = match.partial_match;
-        /*
-         * 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 */
     }
     if (retval == NULL && auto_create != 0)
     {

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)

gnucash/import-export/test/gtest-import-account-matcher.cpp

@@ -0,0 +1,172 @@
+/********************************************************************
+ * gtest-import-account-matcher.cpp --                              *
+ *                        unit tests import-account-matcher.        *
+ * Copyright (C) 2020 John Ralls <jralls@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@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);
+    EXPECT_STREQ("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);
+    EXPECT_STREQ("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);
+    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_STOCK,
+                                           nullptr, nullptr);
+    ASSERT_EQ(nullptr, 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);
+    EXPECT_STREQ("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);
+    EXPECT_STREQ("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);
+    EXPECT_STREQ("Cash Management", xaccAccountGetName(found));
+}