Bug 798093 - Changing the symbol/abbreviation of a security...

after the trading account was created breaks GnuCash.

Revisited. The original changeset looked for a top level trading account
and a namespace account in the transaction currency; if either of those
accounts had been created in a different currency it would duplicate
them.

This commit will accept any such account regardless of commodity. If
more than one exists it will prefer the one in the root currency if
there is one, otherwise it will select the first one found.

libgnucash/engine/Account.cpp

@@ -3120,26 +3120,31 @@ gnc_account_lookup_by_full_name (const Account *any_acc,
     return found;
 }
 
-Account*
+GList*
 gnc_account_lookup_by_type_and_commodity (Account* root,
                                           const char* name,
                                           GNCAccountType acctype,
                                           gnc_commodity* commodity)
 {
+    GList *retval{};
     auto rpriv{GET_PRIVATE(root)};
     for (auto node = rpriv->children; node; node = node->next)
     {
         auto account{static_cast<Account*>(node->data)};
-        if (xaccAccountGetType (account) == acctype &&
-            gnc_commodity_equiv(xaccAccountGetCommodity (account), commodity))
+        if (xaccAccountGetType (account) == acctype)
         {
+            if (commodity &&
+                !gnc_commodity_equiv(xaccAccountGetCommodity (account),
+                                     commodity))
+                continue;
+
             if (name && strcmp(name, xaccAccountGetName(account)))
-                continue; //name doesn't match so skip this one
+                continue;
 
-            return account;
+            retval = g_list_prepend(retval, account);
         }
     }
-    return nullptr;
+    return retval;
 }
 
 void

libgnucash/engine/Account.h

@@ -938,24 +938,24 @@ Account *gnc_account_lookup_by_code (const Account *parent,
  */
 Account *gnc_account_lookup_by_opening_balance (Account *account, gnc_commodity *commodity);
 
-/** Find a direct child account matching name, GNCAccountType, and commodity.
+/** Find a direct child account matching name, GNCAccountType, and/or commodity.
  *
- * Note that commodity matching is by equivalence: If the
- * mnemonic/symbol and namespace are the same, it matches.
+ * Name and commodity may be nullptr in which case the accounts in the
+ * list may have any value for those properties.  Note that commodity
+ * matching is by equivalence: If the mnemonic/symbol and namespace
+ * are the same, it matches.
  *
  *  @param root The account among whose children one expects to find
  *  the account.
- *  @param name The name of the account to look for. If nullptr the
- *  returned account will match only on acctype and commodity.
+ *  @param name The name of the account to look for or nullptr.
  *  @param acctype The GNCAccountType to match.
- *  @param commodity The commodity in which the account should be denominated.
- *  @return The book's trading account for the given commodity if
- *  trading accounts are enabled and one exists; NULL otherwise.
+ *  @param commodity The commodity in which the account should be denominated or nullptr.
+ *  @return A GList of children matching the supplied parameters.
  */
-Account *gnc_account_lookup_by_type_and_commodity (Account* root,
-                                                   const char* name,
-                                                   GNCAccountType acctype,
-                                                   gnc_commodity* commodity);
+GList *gnc_account_lookup_by_type_and_commodity (Account* root,
+                                                 const char* name,
+                                                 GNCAccountType acctype,
+                                                 gnc_commodity* commodity);
 /** @} */
 
 /* ------------------ */

libgnucash/engine/Scrub.c

@@ -51,6 +51,7 @@
 #include "TransactionP.h"
 #include "gnc-commodity.h"
 #include "qofinstance-p.h"
+#include "gnc-session.h"
 
 #undef G_LOG_DOMAIN
 #define G_LOG_DOMAIN "gnc.engine.scrub"
@@ -442,43 +443,46 @@ get_balance_split (Transaction *trans, Account *root, Account *account,
     return balance_split;
 }
 
+static gnc_commodity*
+find_root_currency(void)
+{
+    QofSession *sess = gnc_get_current_session ();
+    Account *root = gnc_book_get_root_account (qof_session_get_book (sess));
+    gnc_commodity *root_currency = xaccAccountGetCommodity (root);
+
+    /* Some older books may not have a currency set on the root
+     * account. In that case find the first top-level INCOME account
+     * and use its currency. */
+    if (!root_currency)
+    {
+         GList *children = gnc_account_get_children (root);
+         for (GList *node = children; node && !root_currency;
+              node = g_list_next (node))
+         {
+              Account *child = GNC_ACCOUNT (node->data);
+              if (xaccAccountGetType (child) == ACCT_TYPE_INCOME)
+                   root_currency = xaccAccountGetCommodity (child);
+         }
+         g_free (children);
+    }
+    return root_currency;
+}
+
 /* Get the trading split for a given commodity, creating it (and the
-   necessary accounts) if it doesn't exist. */
+   necessary parent accounts) if it doesn't exist. */
 static Split *
-get_trading_split (Transaction *trans, Account *root,
+get_trading_split (Transaction *trans, Account *base,
                    gnc_commodity *commodity)
 {
     Split *balance_split;
     Account *trading_account;
     Account *ns_account;
     Account *account;
-    gnc_commodity *default_currency = NULL;
-
-    if (!root)
-    {
-        root = gnc_book_get_root_account (xaccTransGetBook (trans));
-        if (NULL == root)
-        {
-            /* This can't occur, things should be in books */
-            PERR ("Bad data corruption, no root account in book");
-            return NULL;
-        }
-    }
-
-    /* Get the default currency.  This is harder than it seems.  It's not
-       possible to call gnc_default_currency() since it's a UI function.  One
-       might think that the currency of the root account would do, but the root
-       account has no currency.  Instead look for the Income placeholder account
-       and use its currency.  */
-    default_currency = xaccAccountGetCommodity(gnc_account_lookup_by_name(root,
-                                                                          _("Income")));
-    if (! default_currency)
-    {
-        default_currency = commodity;
-    }
+    Account* root = gnc_book_get_root_account (xaccTransGetBook (trans));
+    gnc_commodity *root_currency = find_root_currency ();
 
     trading_account = xaccScrubUtilityGetOrMakeAccount (root,
-                                                        default_currency,
+                                                        NULL,
                                                         _("Trading"),
                                                         ACCT_TYPE_TRADING,
                                                         TRUE, FALSE);
@@ -489,7 +493,7 @@ get_trading_split (Transaction *trans, Account *root,
     }
 
     ns_account = xaccScrubUtilityGetOrMakeAccount (trading_account,
-                                                   default_currency,
+                                                   NULL,
                                                    gnc_commodity_get_namespace(commodity),
                                                    ACCT_TYPE_TRADING,
                                                    TRUE, TRUE);
@@ -1451,42 +1455,91 @@ xaccAccountScrubColorNotSet (QofBook *book)
 
 /* ================================================================ */
 
+static Account*
+construct_account (Account *root, gnc_commodity *currency, const char *accname,
+                   GNCAccountType acctype, gboolean placeholder)
+{
+    gnc_commodity* root_currency = find_root_currency ();
+    Account *acc = xaccMallocAccount(gnc_account_get_book (root));
+    xaccAccountBeginEdit (acc);
+    if (accname && *accname)
+         xaccAccountSetName (acc, accname);
+    if (currency || root_currency)
+         xaccAccountSetCommodity (acc, currency ? currency : root_currency);
+    xaccAccountSetType (acc, acctype);
+    xaccAccountSetPlaceholder (acc, placeholder);
+
+    /* Hang the account off the root. */
+    gnc_account_append_child (root, acc);
+    xaccAccountCommitEdit (acc);
+    return acc;
+}
+
+static Account*
+find_root_currency_account_in_list (GList *acc_list)
+{
+    gnc_commodity* root_currency = find_root_currency();
+    for (GList *node = acc_list; node; node = g_list_next (node))
+    {
+        Account *acc = GNC_ACCOUNT (node->data);
+        gnc_commodity *acc_commodity = NULL;
+        if (G_UNLIKELY (!acc)) continue;
+        acc_commodity = xaccAccountGetCommodity(acc);
+        if (gnc_commodity_equiv (acc_commodity, root_currency))
+            return acc;
+    }
+
+    return NULL;
+}
+
+static Account*
+find_account_matching_name_in_list (GList *acc_list, const char* accname)
+{
+    for (GList* node = acc_list; node; node = g_list_next(node))
+    {
+        Account *acc = GNC_ACCOUNT (node->data);
+        if (G_UNLIKELY (!acc)) continue;
+        if (g_strcmp0 (accname, xaccAccountGetName(acc)))
+        {
+            g_list_free (acc_list);
+            return acc;
+        }
+    }
+    return NULL;
+}
+
 Account *
 xaccScrubUtilityGetOrMakeAccount (Account *root, gnc_commodity * currency,
                                   const char *accname, GNCAccountType acctype,
                                   gboolean placeholder, gboolean checkname)
 {
-    Account * acc;
+    GList* acc_list;
+    Account *acc = NULL;
 
     g_return_val_if_fail (root, NULL);
 
-    /* build the account name */
-    if (!currency)
-    {
-        PERR ("No currency specified!");
-        return NULL;
-    }
+    acc_list =
+        gnc_account_lookup_by_type_and_commodity (root,
+                                                  checkname ? accname : NULL,
+                                                  acctype, currency);
 
-    /* See if we've got one of these going already ... */
-    acc = gnc_account_lookup_by_type_and_commodity (root,
-                                                    checkname ? accname : NULL,
-                                                    acctype, currency);
+    if (!acc_list)
+        return construct_account (root, currency, accname,
+                                  acctype, placeholder);
 
-    if (acc == NULL)
+    if (g_list_next(acc_list))
     {
-        /* Guess not. We'll have to build one. */
-        acc = xaccMallocAccount(gnc_account_get_book (root));
-        xaccAccountBeginEdit (acc);
-        xaccAccountSetName (acc, accname);
-        xaccAccountSetCommodity (acc, currency);
-        xaccAccountSetType (acc, acctype);
-        xaccAccountSetPlaceholder (acc, placeholder);
-
-        /* Hang the account off the root. */
-        gnc_account_append_child (root, acc);
-        xaccAccountCommitEdit (acc);
+        if (!currency)
+            acc = find_root_currency_account_in_list (acc_list);
+
+        if (!acc)
+            acc = find_account_matching_name_in_list (acc_list, accname);
     }
 
+    if (!acc)
+        acc = GNC_ACCOUNT (acc_list->data);
+
+    g_list_free (acc_list);
     return acc;
 }