From eb6dad86e3dc48ceaebf586b280b1fc09d8087b7 Mon Sep 17 00:00:00 2001 From: lmat Date: Fri, 27 Oct 2017 11:09:42 -0400 Subject: [PATCH] Fixed conversion problem The conversion assumed there were only three levels to bayes import map kvp: IMAP token, user-supplied token, GUID/account name. In actuality, since user-supplied tokens could have the delimiter in them, there could be several. This fix takes that into account like so: IMAP token, potentially several user-supplied tokens, GUID/account name. The import map is undergoing two conversions at the same time: account names to guids and an hierarchical representation to a flat representation in KVP. --- gnucash/gnome-utils/gnc-file.c | 1 - libgnucash/engine/Account.cpp | 170 +++++--------------- libgnucash/engine/Account.h | 4 - libgnucash/engine/kvp-frame.cpp | 12 +- libgnucash/engine/kvp-frame.hpp | 8 +- libgnucash/engine/test/gtest-import-map.cpp | 56 +------ 6 files changed, 57 insertions(+), 194 deletions(-) diff --git a/gnucash/gnome-utils/gnc-file.c b/gnucash/gnome-utils/gnc-file.c index 96821e5144..12d5e59764 100644 --- a/gnucash/gnome-utils/gnc-file.c +++ b/gnucash/gnome-utils/gnc-file.c @@ -1026,7 +1026,6 @@ RESTART: // Convert imap mappings from account full name to guid strings qof_event_suspend(); gnc_account_imap_convert_bayes (gnc_get_current_book()); - gnc_account_imap_convert_flat (gnc_get_current_book()); qof_event_resume(); return TRUE; diff --git a/libgnucash/engine/Account.cpp b/libgnucash/engine/Account.cpp index a3603e9223..8ce126d513 100644 --- a/libgnucash/engine/Account.cpp +++ b/libgnucash/engine/Account.cpp @@ -43,6 +43,8 @@ #include "gnc-features.h" #include "guid.hpp" +#include + static QofLogModule log_module = GNC_MOD_ACCOUNT; /* The Canonical Account Separator. Pre-Initialized. */ @@ -5269,8 +5271,8 @@ get_first_pass_probabilities(GncImportMatchMap * imap, GList * tokens) * in the input tokens list. */ for (auto current_token = tokens; current_token; current_token = current_token->next) { - auto translated_token = std::string{static_cast(current_token->data)}; - std::replace(translated_token.begin(), translated_token.end(), '/', '-'); + auto translated_token = std::string {static_cast (current_token->data)}; + std::replace (translated_token.begin (), translated_token.end (), '/', '-'); token_accounts_info tokenInfo{}; auto path = std::string{IMAP_FRAME_BAYES "-"} + translated_token; qof_instance_foreach_slot_prefix (QOF_INSTANCE (imap->acc), path, &build_token_info, tokenInfo); @@ -5331,7 +5333,7 @@ gnc_account_imap_find_account_bayes (GncImportMatchMap *imap, GList *tokens) } static void -change_imap_entry (GncImportMatchMap *imap, gchar *kvp_path, int64_t token_count) +change_imap_entry (GncImportMatchMap *imap, gchar const * kvp_path, int64_t token_count) { GValue value = G_VALUE_INIT; @@ -5377,7 +5379,7 @@ gnc_account_imap_add_account_bayes (GncImportMatchMap *imap, { GList *current_token; gint64 token_count; - char *account_fullname, *kvp_path; + char *account_fullname; char *guid_string; ENTER(" "); @@ -5405,28 +5407,20 @@ gnc_account_imap_add_account_bayes (GncImportMatchMap *imap, skip this case here. */ if (!current_token->data || (*((char*)current_token->data) == '\0')) continue; - /* start off with one token for this account */ token_count = 1; - PINFO("adding token '%s'", (char*)current_token->data); - - std::string translated_token {static_cast(current_token->data)}; - std::replace(translated_token.begin(), translated_token.end(), '/', '-'); - kvp_path = g_strdup_printf (IMAP_FRAME_BAYES "-%s-%s", - translated_token.c_str(), - guid_string); + std::string translated_token {static_cast (current_token->data)}; + std::replace (translated_token.begin (), translated_token.end (), '/', '-'); + auto path = std::string {IMAP_FRAME_BAYES} + '-' + translated_token + '-' + guid_string; /* change the imap entry for the account */ - change_imap_entry (imap, kvp_path, token_count); - g_free (kvp_path); + change_imap_entry (imap, path.c_str (), token_count); } - /* free up the account fullname and guid string */ qof_instance_set_dirty (QOF_INSTANCE (imap->acc)); xaccAccountCommitEdit (imap->acc); g_free (account_fullname); g_free (guid_string); - LEAVE(" "); } @@ -5655,68 +5649,48 @@ look_for_old_separator_descendants (Account *root, gchar const *full_name, const return new_name; } -static Account * -look_for_old_mapping (GncImapInfo *imapInfo) +static std::string +get_guid_from_account_name (Account * root, std::string const & name) { - Account *root, *map_account = NULL; - const gchar *sep = gnc_get_account_separator_string (); - gchar *full_name; - - PINFO("Category Head is '%s', Full Category is '%s'", imapInfo->category_head, imapInfo->full_category); - - // do we have a map_account all ready, implying a guid string - if (imapInfo->map_account != NULL) - return NULL; - - root = gnc_account_get_root (imapInfo->source_account); - - full_name = g_strdup (imapInfo->full_category + strlen (imapInfo->category_head) + 1); - - // may be top level or match with existing separator - map_account = gnc_account_lookup_by_full_name (root, full_name); - - // do we have a valid account, if not, look for old separator - if (map_account == NULL) + auto map_account = gnc_account_lookup_by_full_name (root, name.c_str ()); + if (!map_account) { - gchar * temp_name = look_for_old_separator_descendants (root, full_name, sep); - g_free(full_name); - full_name = temp_name; - map_account = gnc_account_lookup_by_full_name (root, full_name); // lets try again + auto temp_account_name = look_for_old_separator_descendants (root, name.c_str (), + gnc_get_account_separator_string ()); + map_account = gnc_account_lookup_by_full_name (root, temp_account_name); + g_free (temp_account_name); } + auto temp_guid = gnc::GUID {*xaccAccountGetGUID (map_account)}; + return temp_guid.to_string (); +} - PINFO("Full account name is '%s'", full_name); - - g_free (full_name); - - return map_account; +static std::pair +convert_entry (std::pair , KvpValue*> entry, Account* root) +{ + auto const & account_name = entry.first.back(); + entry.first.pop_back(); + auto guid_str = get_guid_from_account_name (root, account_name); + entry.first.emplace_back ("/"); + entry.first.emplace_back (guid_str); + std::string new_key {std::accumulate (entry.first.begin(), entry.first.end(), std::string {})}; + new_key = IMAP_FRAME_BAYES + new_key; + std::replace (new_key.begin(), new_key.end(), '/', '-'); + return {new_key, entry.second}; } static std::vector> get_new_guid_imap (Account * acc) { auto frame = qof_instance_get_slots (QOF_INSTANCE (acc)); - auto slot = frame->get_slot(IMAP_FRAME_BAYES); + auto slot = frame->get_slot (IMAP_FRAME_BAYES); if (!slot) return {}; - std::string const imap_frame_str {IMAP_FRAME_BAYES}; - std::vector> ret; + auto imap_frame = slot->get (); + auto flat_kvp = imap_frame->flatten_kvp (); auto root = gnc_account_get_root (acc); - auto imap_frame = slot->get(); - imap_frame->for_each_slot_temp ([&ret, root, &imap_frame_str] (char const * token, KvpValue* val) { - auto token_frame = val->get(); - token_frame->for_each_slot_temp ([&ret, root, &imap_frame_str, token] (char const * account_name, KvpValue* val) { - auto map_account = gnc_account_lookup_by_full_name (root, account_name); - if (!map_account) - { - auto temp_account_name = look_for_old_separator_descendants (root, account_name, gnc_get_account_separator_string()); - map_account = gnc_account_lookup_by_full_name (root, temp_account_name); - g_free (temp_account_name); - } - auto temp_guid = gnc::GUID{*xaccAccountGetGUID (map_account)}; - auto guid_str = temp_guid.to_string(); - ret.push_back({imap_frame_str + "-" + token + "-" + guid_str, val}); - }); - }); + std::vector > ret; + for (auto const & flat_entry : flat_kvp) + ret.emplace_back (convert_entry (flat_entry, root)); return ret; } @@ -5737,7 +5711,6 @@ convert_imap_account_bayes_to_guid (Account *acc) } char const * run_once_key_to_guid {"changed-bayesian-to-guid"}; -char const * run_once_key_to_flat {"changed-bayesian-to-flat"}; static void imap_convert_bayes_to_guid (QofBook * book) @@ -5752,55 +5725,6 @@ imap_convert_bayes_to_guid (QofBook * book) g_list_free (accts); } -static std::vector> -get_new_flat_imap (Account * acc) -{ - auto frame = qof_instance_get_slots (QOF_INSTANCE (acc)); - auto slot = frame->get_slot(IMAP_FRAME_BAYES); - if (!slot) - return {}; - std::string const imap_frame_str {IMAP_FRAME_BAYES}; - std::vector> ret; - auto root = gnc_account_get_root (acc); - auto imap_frame = slot->get(); - imap_frame->for_each_slot_temp ([&ret, &imap_frame_str] (char const * token, KvpValue* val) { - auto token_frame = val->get(); - token_frame->for_each_slot_temp ([&ret, &imap_frame_str, token] (char const * account_guid, KvpValue* val) { - ret.push_back({imap_frame_str + "-" + token + "-" + account_guid, val}); - }); - }); - return ret; -} - -static void -convert_imap_account_bayes_to_flat (Account * acc) -{ - auto flat_imap = get_new_flat_imap (acc); - if (!flat_imap.size()) - return; - auto frame = qof_instance_get_slots (QOF_INSTANCE (acc)); - xaccAccountBeginEdit(acc); - frame->set(IMAP_FRAME_BAYES, nullptr); - std::for_each(flat_imap.begin(), flat_imap.end(), [&frame] (std::pair const & entry) { - frame->set(entry.first.c_str(), entry.second); - }); - qof_instance_set_dirty (QOF_INSTANCE (acc)); - xaccAccountCommitEdit(acc); -} - -static void -imap_convert_bayes_to_flat (QofBook * book) -{ - auto root = gnc_book_get_root_account (book); - auto accts = gnc_account_get_descendants_sorted (root); - for (auto ptr = accts; ptr; ptr = g_list_next (ptr)) - { - Account *acc = static_cast (ptr->data); - convert_imap_account_bayes_to_flat (acc); - } - g_list_free (accts); -} - static bool run_once_key_set (char const * key, QofBook * book) { @@ -5818,22 +5742,13 @@ set_run_once_key (char const * key, QofBook * book) qof_instance_set_kvp(QOF_INSTANCE (book), key, &value); } -void -gnc_account_imap_convert_flat (QofBook *book) -{ - if (run_once_key_set(run_once_key_to_flat, book)) - return; - imap_convert_bayes_to_flat (book); - set_run_once_key(run_once_key_to_flat, book); -} - void gnc_account_imap_convert_bayes (QofBook *book) { - if (run_once_key_set(run_once_key_to_guid, book)) + if (run_once_key_set (run_once_key_to_guid, book)) return; - imap_convert_bayes_to_guid(book); - set_run_once_key(run_once_key_to_guid, book); + imap_convert_bayes_to_guid (book); + set_run_once_key (run_once_key_to_guid, book); } /* ================================================================ */ @@ -5843,7 +5758,6 @@ static void gnc_account_book_end(QofBook* book) { Account *root_account = gnc_book_get_root_account(book); - xaccAccountBeginEdit(root_account); xaccAccountDestroy(root_account); } diff --git a/libgnucash/engine/Account.h b/libgnucash/engine/Account.h index 7c93259115..2f80632f87 100644 --- a/libgnucash/engine/Account.h +++ b/libgnucash/engine/Account.h @@ -1454,10 +1454,6 @@ void gnc_account_delete_map_entry (Account *acc, char *full_category, gboolean e */ void gnc_account_imap_convert_bayes (QofBook *book); -/** Change the bayes imap entries from a nested representation to a flat representation. - */ -void gnc_account_imap_convert_flat (QofBook *); - /** @} */ diff --git a/libgnucash/engine/kvp-frame.cpp b/libgnucash/engine/kvp-frame.cpp index 9461d010c6..29fe927c16 100644 --- a/libgnucash/engine/kvp-frame.cpp +++ b/libgnucash/engine/kvp-frame.cpp @@ -38,6 +38,7 @@ extern "C" #include #include #include +#include /* This static indicates the debugging module that this .o belongs to. */ static QofLogModule log_module = "qof.kvp"; @@ -473,7 +474,7 @@ gnc_value_list_get_type (void) } void -KvpFrame::flatten_kvp_impl(std::vector path, std::vector> & entries) const +KvpFrame::flatten_kvp_impl(std::vector path, std::vector , KvpValue*>> & entries) const { for (auto const & entry : m_valuemap) { @@ -486,16 +487,17 @@ KvpFrame::flatten_kvp_impl(std::vector path, std::vector new_path {path}; + new_path.emplace_back (entry.first); + entries.emplace_back (new_path, entry.second); } } } -std::vector> +std::vector , KvpValue*>> KvpFrame::flatten_kvp(void) const { - std::vector> ret; + std::vector , KvpValue*>> ret; flatten_kvp_impl({}, ret); return ret; } diff --git a/libgnucash/engine/kvp-frame.hpp b/libgnucash/engine/kvp-frame.hpp index 0cc206ec63..49572b8962 100644 --- a/libgnucash/engine/kvp-frame.hpp +++ b/libgnucash/engine/kvp-frame.hpp @@ -213,6 +213,10 @@ struct KvpFrameImpl */ KvpValue* get_slot(Path keys) const noexcept; + /** + * proc is called with each of the immediate contents of this frame, passing it the key, + * value, and specified data. + */ void for_each_slot(void (*proc)(const char *key, KvpValue *, void *data), void* data) const noexcept; /** The function should be of the form: @@ -239,7 +243,7 @@ struct KvpFrameImpl * Returns all keys and values of this frame recursively, flattening * the frame-containing values. */ - std::vector> + std::vector , KvpValue*>> flatten_kvp(void) const; /** Test for emptiness @@ -251,7 +255,7 @@ struct KvpFrameImpl private: map_type m_valuemap; - void flatten_kvp_impl(std::vector, std::vector> &) const; + void flatten_kvp_impl(std::vector , std::vector , KvpValue*>> &) const; }; template diff --git a/libgnucash/engine/test/gtest-import-map.cpp b/libgnucash/engine/test/gtest-import-map.cpp index 5938f40cff..117fe59cda 100644 --- a/libgnucash/engine/test/gtest-import-map.cpp +++ b/libgnucash/engine/test/gtest-import-map.cpp @@ -318,7 +318,6 @@ TEST_F(ImapBayesTest, AddAccountBayes) EXPECT_EQ(2, value->get()); } - TEST_F(ImapBayesTest, ConvertAccountBayes) { // prevent the embedded beginedit/committedit from doing anything @@ -326,18 +325,15 @@ TEST_F(ImapBayesTest, ConvertAccountBayes) qof_instance_mark_clean(QOF_INSTANCE(t_bank_account)); gnc_account_imap_add_account_bayes(t_imap, t_list1, t_expense_account1); //Food gnc_account_imap_add_account_bayes(t_imap, t_list2, t_expense_account2); //Drink - auto root = qof_instance_get_slots(QOF_INSTANCE(t_bank_account)); auto book = qof_instance_get_slots(QOF_INSTANCE(t_imap->book)); auto acct1_guid = guid_to_string (xaccAccountGetGUID(t_expense_account1)); //Food auto acct2_guid = guid_to_string (xaccAccountGetGUID(t_expense_account2)); //Drink auto acct3_guid = guid_to_string (xaccAccountGetGUID(t_asset_account2)); //Asset-Bank auto acct4_guid = guid_to_string (xaccAccountGetGUID(t_sav_account)); //Sav Bank - auto val1 = new KvpValue(static_cast(10)); auto val2 = new KvpValue(static_cast(5)); auto val3 = new KvpValue(static_cast(2)); - // Test for existing entries, all will be 1 auto value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + foo + "-" + acct1_guid).c_str()); EXPECT_EQ(1, value->get()); @@ -347,84 +343,36 @@ TEST_F(ImapBayesTest, ConvertAccountBayes) EXPECT_EQ(1, value->get()); value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + waldo + "-" + acct2_guid).c_str()); EXPECT_EQ(1, value->get()); - // Set up some old entries - root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + pepper + "/" + "Asset-Bank").c_str(), val1); + root->set_path((std::string{IMAP_FRAME_BAYES} + "/token/with/slashes/" + "Asset-Bank").c_str(), val1); root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + salt + "/" + "Asset-Bank#Bank").c_str(), new KvpValue{*val1}); root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + salt + "/" + "Asset>Bank#Bank").c_str(), val2); root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + pork + "/" + "Expense#Food").c_str(), new KvpValue{*val2}); root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + sausage + "/" + "Expense#Drink").c_str(), val3); root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + foo + "/" + "Expense#Food").c_str(), new KvpValue{*val2}); - EXPECT_EQ(1, qof_instance_get_editlevel(QOF_INSTANCE(t_bank_account))); EXPECT_TRUE(qof_instance_get_dirty_flag(QOF_INSTANCE(t_bank_account))); qof_instance_mark_clean(QOF_INSTANCE(t_bank_account)); - // Start Convert gnc_account_imap_convert_bayes (t_imap->book); - // convert from 'Asset-Bank' to 'Asset-Bank' guid - value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + pepper + "-" + acct3_guid).c_str()); + value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-token-with-slashes-" + acct3_guid).c_str()); EXPECT_EQ(10, value->get()); - // convert from 'Asset-Bank#Bank' to 'Sav Bank' guid value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + salt + "-" + acct4_guid).c_str()); EXPECT_EQ(10, value->get()); - // convert from 'Expense#Food' to 'Food' guid value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + pork + "-" + acct1_guid).c_str()); EXPECT_EQ(5, value->get()); - // convert from 'Expense#Drink' to 'Drink' guid value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + sausage + "-" + acct2_guid).c_str()); EXPECT_EQ(2, value->get()); - // convert from 'Expense#Food' to 'Food' guid but add to original value value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + foo + "-" + acct1_guid).c_str()); EXPECT_EQ(5, value->get()); - // Check for run once flag auto vals = book->get_slot("changed-bayesian-to-guid"); EXPECT_STREQ("true", vals->get()); - - EXPECT_EQ(1, qof_instance_get_editlevel(QOF_INSTANCE(t_bank_account))); - EXPECT_TRUE(qof_instance_get_dirty_flag(QOF_INSTANCE(t_bank_account))); -} - -TEST_F (ImapBayesTest, convert_map_flat) -{ - // prevent the embedded beginedit/committedit from doing anything - qof_instance_increase_editlevel(QOF_INSTANCE(t_bank_account)); - qof_instance_mark_clean(QOF_INSTANCE(t_bank_account)); - //gnc_account_imap_add_account_bayes(t_imap, t_list1, t_expense_account1); //Food - //gnc_account_imap_add_account_bayes(t_imap, t_list2, t_expense_account2); //Drink - auto root = qof_instance_get_slots(QOF_INSTANCE(t_bank_account)); - auto acct1_guid = guid_to_string (xaccAccountGetGUID(t_expense_account1)); //Food - auto acct2_guid = guid_to_string (xaccAccountGetGUID(t_expense_account2)); //Drink - auto acct3_guid = guid_to_string (xaccAccountGetGUID(t_asset_account2)); //Asset-Bank - auto acct4_guid = guid_to_string (xaccAccountGetGUID(t_sav_account)); //Sav Bank - auto val1 = new KvpValue(static_cast(10)); - auto val2 = new KvpValue(static_cast(5)); - auto val3 = new KvpValue(static_cast(2)); - // Set up some old entries - root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + pepper + "/" + acct1_guid).c_str(), val1); - root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + salt + "/" + acct2_guid).c_str(), new KvpValue{*val1}); - root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + foo + "/" + acct3_guid).c_str(), val2); - root->set_path((std::string{IMAP_FRAME_BAYES} + "/" + pork + "/" + acct4_guid).c_str(), val3); - EXPECT_EQ(1, qof_instance_get_editlevel(QOF_INSTANCE(t_bank_account))); - qof_instance_mark_clean(QOF_INSTANCE(t_bank_account)); - gnc_account_imap_convert_flat (t_imap->book); - auto value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + pepper + "-" + acct1_guid).c_str()); - EXPECT_EQ(10, value->get()); - value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + salt + "-" + acct2_guid).c_str()); - EXPECT_EQ(10, value->get()); - value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + foo + "-" + acct3_guid).c_str()); - EXPECT_EQ(5, value->get()); - value = root->get_slot((std::string{IMAP_FRAME_BAYES} + "-" + pork + "-" + acct4_guid).c_str()); - EXPECT_EQ(2, value->get()); - auto book = qof_instance_get_slots(QOF_INSTANCE(t_imap->book)); - auto vals = book->get_slot("changed-bayesian-to-flat"); - EXPECT_STREQ("true", vals->get()); EXPECT_EQ(1, qof_instance_get_editlevel(QOF_INSTANCE(t_bank_account))); EXPECT_TRUE(qof_instance_get_dirty_flag(QOF_INSTANCE(t_bank_account))); }