r19744 - gnucash/trunk/src/engine - Bugs 632346 & 632166: Fixup transaction currency scrubbing.

[John Ralls]

Author: jralls
Date: 2010-11-02 17:14:38 -0400 (Tue, 02 Nov 2010)
New Revision: 19744
Trac: http://svn.gnucash.org/trac/changeset/19744

Modified:
   gnucash/trunk/src/engine/Scrub.c
Log:
Bugs 632346 & 632166: Fixup transaction currency scrubbing.

Biggest problem was that almost well-formed transactions missing a currency 
element wouldn't import correctly because the FindCommonCurrencies function 
only used the old currency values, ignoring the commodity values in the 
splits' accounts. A new function, xaccFindCommonCurrencies(), looks at those 
first and then calls xaccFindOldCommonCurrencies only if it fails. That 
addresses the cause of the import failure reported in 632166 and a big chunk 
of the warning messages complained about in 632346.

A secondary problem, also addressed in this change, was that the last block 
in xaccTransScrubCurrencyFromSplits always replaced the "amount" with the 
"value". This is the right thing to do if the commodity for the split account 
is a currency, but if it isn't, the replacement should be reversed. 



Modified: gnucash/trunk/src/engine/Scrub.c
===================================================================
--- gnucash/trunk/src/engine/Scrub.c	2010-11-02 18:21:50 UTC (rev 19743)
+++ gnucash/trunk/src/engine/Scrub.c	2010-11-02 21:14:38 UTC (rev 19744)
@@ -883,6 +883,12 @@
             int ab = !gnc_commodity_equiv(ra, sb);
             if ( aa && ab ) ra = NULL;
         }
+        else if (!ra && rb)
+        {
+            int aa = !gnc_commodity_equiv(rb, sa);
+            int ab = !gnc_commodity_equiv(rb, sb);
+            ra = ( aa && ab ) ? NULL : rb;
+        }
 
         if ((!ra) && (!rb)) return NULL;
     }
@@ -948,6 +954,103 @@
     return retval;
 }
 
+/* Test the currency of the splits and find the most common and return
+ * it, or NULL if there is no currency more common than the
+ * others -- or none at all.
+ */
+typedef struct {
+    gnc_commodity *commodity;
+    unsigned int count;
+} CommodityCount;
+
+static gint
+commodity_equal (gconstpointer a, gconstpointer b)
+{
+    CommodityCount *cc = (CommodityCount*)a;
+    gnc_commodity *com = (gnc_commodity*)b;
+    if ( cc == NULL || cc->commodity == NULL || 
+	 !GNC_IS_COMMODITY( cc->commodity ) ) return -1;
+    if ( com == NULL || !GNC_IS_COMMODITY( com ) ) return 1;
+    if ( gnc_commodity_equal(cc->commodity, com) )
+	return 0;
+    return 1;
+}
+
+static gint
+commodity_compare( gconstpointer a, gconstpointer b)
+{
+    CommodityCount *ca = (CommodityCount*)a, *cb = (CommodityCount*)b;
+    if (ca == NULL || ca->commodity == NULL ||
+	!GNC_IS_COMMODITY( ca->commodity ) )
+    {
+	if (cb == NULL || cb->commodity == NULL ||
+	    !GNC_IS_COMMODITY( cb->commodity ) )
+	    return 0;
+	return -1;
+    }
+    if (cb == NULL || cb->commodity == NULL ||
+	    !GNC_IS_COMMODITY( cb->commodity ) )
+	return 1;
+    if (ca->count == cb->count)
+	return 0;
+    return ca->count > cb->count ? 1 : -1;
+}
+
+/* Find the commodities in the account of each of the splits of a
+ * transaction, and rank them by how many splits in which they
+ * occur. Commodities which are currencies count more than those which
+ * aren't, because for simple buy and sell transactions it makes
+ * slightly more sense for the transaction commodity to be the
+ * currency -- to the extent that it makes sense for a transaction to
+ * have a currency at all. jralls, 2010-11-02 */ 
+
+static gnc_commodity *
+xaccTransFindCommonCurrency (Transaction *trans, QofBook *book)
+{
+    gnc_commodity *com_first, *com_scratch;
+    GList *node = NULL;
+    GSList *comlist = NULL, *found = NULL;
+    int score = 0;
+
+    if (!trans) return NULL;
+
+    if (trans->splits == NULL) return NULL;
+
+    g_return_val_if_fail (book, NULL);
+
+    for (node = trans->splits; node; node = node->next)
+    {
+        Split *s = node->data;
+	if (s == NULL || s->acc == NULL) continue;
+	com_scratch = xaccAccountGetCommodity(s->acc);
+	if ( comlist )
+	{
+	    found = g_slist_find_custom(comlist, com_scratch, commodity_equal);
+	}
+	if (comlist == NULL || found == NULL) 
+	{
+	    CommodityCount *count = g_slice_new0(CommodityCount);
+	    count->commodity = com_scratch;
+	    count->count = ( gnc_commodity_is_currency( com_scratch ) ? 3 : 2 );
+	    comlist = g_slist_append(comlist, count);
+	}
+	else
+	{
+	  CommodityCount *count = (CommodityCount*)(found->data);
+	  count->count += ( gnc_commodity_is_currency( com_scratch ) ? 3 : 2 );
+	}
+    }
+    found = g_slist_sort( comlist, commodity_compare);
+
+    if ( ((CommodityCount*)(found->data))->commodity != NULL)
+    {
+	return ((CommodityCount*)(found->data))->commodity;
+    }
+/* We didn't find a currency in the current account structure, so try
+ * an old one. */
+    return xaccTransFindOldCommonCurrency( trans, book );
+}
+
 /* ================================================================ */
 
 void
@@ -967,7 +1070,7 @@
     currency = xaccTransGetCurrency (trans);
     if (currency) return;
 
-    currency = xaccTransFindOldCommonCurrency (trans, qof_instance_get_book(trans));
+    currency = xaccTransFindCommonCurrency (trans, qof_instance_get_book(trans));
     if (currency)
     {
         xaccTransBeginEdit (trans);
@@ -1033,6 +1136,9 @@
                  * 'other' transaction, which is going to keep that
                  * information. So I don't bother with that here. -- cstim,
                  * 2002/11/20. */
+		/* But if the commodity *isn't* a currency, then it's
+		 * the value that should be changed to the
+		 * amount. jralls, 2010-11-02 */
 
                 PWARN ("Adjusted split with mismatched values, desc=\"%s\" memo=\"%s\""
                        " old amount %s %s, new amount %s",
@@ -1041,7 +1147,14 @@
                        gnc_commodity_get_mnemonic (currency),
                        gnc_num_dbg_to_string (xaccSplitGetValue(sp)));
                 xaccTransBeginEdit (trans);
-                xaccSplitSetAmount (sp, xaccSplitGetValue(sp));
+		if ( gnc_commodity_is_currency( currency))
+		{
+		    xaccSplitSetAmount (sp, xaccSplitGetValue(sp));
+		}
+		else 
+		{
+		    xaccSplitSetValue(sp, xaccSplitGetAmount(sp));
+		}
                 xaccTransCommitEdit (trans);
             }
             /*else
@@ -1055,6 +1168,7 @@
             }*/
         }
     }
+
 }
 
 /* ================================================================ */