From 6fa80db27e6885080006fb2f184bbfee70040cd8 Mon Sep 17 00:00:00 2001 From: Christian Stimming Date: Tue, 10 Feb 2009 21:16:22 +0000 Subject: [PATCH] Bug#139310: Store online_id in split instead of transaction to avoid import conflicts Affects all users of the generic import: OFX, HBCI and AqBanking. Suggested by . " Assume you have a transaction that transfers money from account 1 to account 2. Import an OFX statement from account 1 that matches the transaction. Then import an OFX statement from account 2 that matches the same transaction. Because each transaction can only store one online_id, when you import from account 1 again it doesn't remember that it's already been imported. If online_id was stored in a split slot instead of a transaction slot, it would allow tracking the online_id of both accounts without being overwritten. " For backwards compatibility, we still check for online_id in transactions. This is more of a convenience than a critical feature, however. The compatibility code could be removed in future versions if desired. Patch by Alan Jenkins git-svn-id: svn+ssh://svn.gnucash.org/repo/gnucash/trunk@17889 57a11ea4-9604-0410-9ed3-97b8803252fd --- src/engine/kvp_doc.txt | 13 +++ src/import-export/aqbanking/gnc-ab-utils.c | 10 +-- src/import-export/hbci/gnc-hbci-gettrans.c | 15 ++-- src/import-export/import-backend.c | 99 ++++++++++------------ src/import-export/import-utilities.c | 31 ++++++- src/import-export/import-utilities.h | 14 +++ src/import-export/ofx/gnc-ofx-import.c | 11 ++- 7 files changed, 121 insertions(+), 72 deletions(-) diff --git a/src/engine/kvp_doc.txt b/src/engine/kvp_doc.txt index a4dcad0f88..3ce0bff876 100644 --- a/src/engine/kvp_doc.txt +++ b/src/engine/kvp_doc.txt @@ -426,6 +426,19 @@ Use: This holds the old security scu. This may be deleted at some point in the future. \endverbatim +\verbatim +Name: online-id +Type: string +Entities: All +Use: This holds a unique identifier for accounts, transactions and splits + created by the generic import layer. This allows imports of + successive statements to automatically match accounts and transactions + which have already been imported. + + This used to be set on transactions but not splits. It is now set on + splits but not transactions. It is still checked on transactions, but + this may change at some point in the future. + \subsection kvpP P \subsection kvpQ Q diff --git a/src/import-export/aqbanking/gnc-ab-utils.c b/src/import-export/aqbanking/gnc-ab-utils.c index 0133c8adc1..348079c18d 100644 --- a/src/import-export/aqbanking/gnc-ab-utils.c +++ b/src/import-export/aqbanking/gnc-ab-utils.c @@ -357,11 +357,6 @@ gnc_ab_trans_to_gnc(const AB_TRANSACTION *ab_trans, Account *gnc_acc) gnc_trans = xaccMallocTransaction(book); xaccTransBeginEdit(gnc_trans); - /* Set OFX unique transaction ID */ - fitid = AB_Transaction_GetFiId(ab_trans); - if (fitid && *fitid) - gnc_import_set_trans_online_id(gnc_trans, fitid); - /* Date / Time */ valuta_date = AB_Transaction_GetValutaDate(ab_trans); if (!valuta_date) { @@ -400,6 +395,11 @@ gnc_ab_trans_to_gnc(const AB_TRANSACTION *ab_trans, Account *gnc_acc) xaccSplitSetParent(split, gnc_trans); xaccSplitSetAccount(split, gnc_acc); + /* Set OFX unique transaction ID */ + fitid = AB_Transaction_GetFiId(ab_trans); + if (fitid && *fitid) + gnc_import_set_split_online_id(split, fitid); + /* Amount into the split */ ab_value = AB_Transaction_GetValue(ab_trans); gnc_amount = double_to_gnc_numeric( diff --git a/src/import-export/hbci/gnc-hbci-gettrans.c b/src/import-export/hbci/gnc-hbci-gettrans.c index ea2eb01e3a..5ea3bc8414 100644 --- a/src/import-export/hbci/gnc-hbci-gettrans.c +++ b/src/import-export/hbci/gnc-hbci-gettrans.c @@ -262,13 +262,6 @@ AB_TRANSACTION *gnc_hbci_trans_list_cb(AB_TRANSACTION *h_trans, void *user_data) /* Create new gnucash transaction for the given hbci one */ gnc_trans = xaccMallocTransaction(book); xaccTransBeginEdit(gnc_trans); - - { - /* OFX unique transaction ID */ - const char *fitid = AB_Transaction_GetFiId(h_trans); - if (fitid && (strlen (fitid) > 0)) - gnc_import_set_trans_online_id(gnc_trans, fitid); - } normalDate = AB_Transaction_GetDate(h_trans); valutaDate = AB_Transaction_GetValutaDate(h_trans); @@ -317,6 +310,14 @@ AB_TRANSACTION *gnc_hbci_trans_list_cb(AB_TRANSACTION *h_trans, void *user_data) xaccTransAppendSplit(gnc_trans, split); xaccAccountInsertSplit(gnc_acc, split); + + { + /* OFX unique transaction ID */ + const char *fitid = AB_Transaction_GetFiId(h_trans); + if (fitid && (strlen (fitid) > 0)) + gnc_import_set_split_online_id(split, fitid); + } + { /* Amount into the split */ const AB_VALUE *h_value = AB_Transaction_GetValue (h_trans); diff --git a/src/import-export/import-backend.c b/src/import-export/import-backend.c index a662c3d8b8..675e4039b6 100644 --- a/src/import-export/import-backend.c +++ b/src/import-export/import-backend.c @@ -58,12 +58,6 @@ static QofLogModule log_module = GNC_MOD_IMPORT; static const int MATCH_DATE_THRESHOLD=4; /*within 4 days*/ static const int MATCH_DATE_NOT_THRESHOLD = 14; -/**Transaction's who have an online_id kvp frame have been downloaded - online can probably be skipped in the match list, since it is very - unlikely that they would match a transaction downloaded at a later - date and yet not have the same online_id. This also increases - performance of the matcher. */ -static const int SHOW_TRANSACTIONS_WITH_UNIQUE_ID = TRUE; /* DISABLE once account transfer bug is fixed! */ /********************************************************************\ * Forward declared prototypes * @@ -570,13 +564,8 @@ static void split_find_match (GNCImportTransInfo * trans_info, /* DEBUG("Begin"); */ /*Ignore the split if the transaction is open for edit, meaning it - was just downloaded. Ignore the split if the transaction has an - online ID , unless overriden in prefs (i.e. do not ignore the - split if the online_id kvp is NULL or if it has zero length). */ - if ((xaccTransIsOpen(xaccSplitGetParent(split)) == FALSE) && - ((gnc_import_get_trans_online_id(xaccSplitGetParent(split))==NULL) || - (strlen(gnc_import_get_trans_online_id(xaccSplitGetParent(split))) == 0) || - SHOW_TRANSACTIONS_WITH_UNIQUE_ID==TRUE)) + was just downloaded. */ + if (xaccTransIsOpen(xaccSplitGetParent(split)) == FALSE) { GNCImportMatchInfo * match_info; gint prob = 0; @@ -745,17 +734,6 @@ static void split_find_match (GNCImportTransInfo * trans_info, } } - /*Online id punishment*/ -/* if ((gnc_import_get_trans_online_id(xaccSplitGetParent(split))!=NULL) && */ -/* (strlen(gnc_import_get_trans_online_id(xaccSplitGetParent(split)))>0)) */ -/* { */ - /* If the pref is to show match even with online ID's, - puninsh the transaction with online id */ - - /* DISABLED, it's the wrong solution to the problem. benoitg, 24/2/2003 */ - /*prob = prob-3;*/ -/* } */ - /* Is the probability high enough? Otherwise do nothing and return. */ if(prob < display_threshold) { @@ -933,13 +911,10 @@ gnc_import_process_trans_item (GncImportMatchMap *matchmap, /* Copy the online id to the reconciled transaction, so the match will be remembered */ - if ((gnc_import_get_trans_online_id(trans_info->trans) - != NULL) && - (strlen (gnc_import_get_trans_online_id(trans_info->trans)) - > 0)) - gnc_import_set_trans_online_id - (selected_match->trans, - gnc_import_get_trans_online_id(trans_info->trans)); + if (gnc_import_split_has_online_id(trans_info->first_split)) + gnc_import_set_split_online_id + (selected_match->split, + gnc_import_get_split_online_id(trans_info->first_split)); /* Done editing. */ /*DEBUG("CommitEdit selected_match")*/ @@ -969,19 +944,38 @@ gnc_import_process_trans_item (GncImportMatchMap *matchmap, } /********************************************************************\ - * check_trans_online_id() Callback function to be used by - * gnc_import_exists_online_id. Takes pointers to two transaction and - * returns 0 if their online_id kvp_frame do NOT match, or if both - * pointers point to the same transaction. - * \********************************************************************/ + * check_trans_online_id() Callback function used by + * gnc_import_exists_online_id. Takes pointers to transaction and split, + * returns 0 if their online_id kvp_frames do NOT match, or if the split + * belongs to the transaction +\********************************************************************/ static gint check_trans_online_id(Transaction *trans1, void *user_data) { - Transaction *trans2 = user_data; - const gchar *online_id1 = gnc_import_get_trans_online_id(trans1); - const gchar *online_id2 = gnc_import_get_trans_online_id(trans2); + Account *account; + Split *split1; + Split *split2 = user_data; + const gchar *online_id1; + const gchar *online_id2; - if ((trans1 == trans2) || (online_id1 == NULL) || - (online_id2 == NULL) || (strcmp(online_id1, online_id2) != 0)) + account = xaccSplitGetAccount(split2); + split1 = xaccTransFindSplitByAccount(trans1, account); + if (split1 == split2) + return 0; + + /* hack - we really want to iterate over the _splits_ of the account + instead of the transactions */ + g_assert(split1 != NULL); + + if (gnc_import_split_has_online_id(split1)) + online_id1 = gnc_import_get_split_online_id(split1); + else + online_id1 = gnc_import_get_trans_online_id(trans1); + + online_id2 = gnc_import_get_split_online_id(split2); + + if ((online_id1 == NULL) || + (online_id2 == NULL) || + (strcmp(online_id1, online_id2) != 0)) { return 0; } @@ -1000,20 +994,15 @@ gboolean gnc_import_exists_online_id (Transaction *trans) gboolean online_id_exists = FALSE; Account *dest_acct; Split *source_split; - - /* For each split in the transaction, check whether the parent account - contains a transaction with the same online id. */ - for (i=0; - ((source_split = xaccTransGetSplit(trans, i)) != NULL) && - (online_id_exists == FALSE); - i++) - { - /* DEBUG("%s%d%s","Checking split ",i," for duplicates"); */ - dest_acct = xaccSplitGetAccount(source_split); - online_id_exists = xaccAccountForEachTransaction(dest_acct, - check_trans_online_id, - trans); - } + + /* Look for an online_id in the first split */ + source_split = xaccTransGetSplit(trans, 0); + + /* DEBUG("%s%d%s","Checking split ",i," for duplicates"); */ + dest_acct = xaccSplitGetAccount(source_split); + online_id_exists = xaccAccountForEachTransaction(dest_acct, + check_trans_online_id, + source_split); /* If it does, abort the process for this transaction, since it is already in the system. */ diff --git a/src/import-export/import-utilities.c b/src/import-export/import-utilities.c index 3fbb202d92..bb1352f823 100644 --- a/src/import-export/import-utilities.c +++ b/src/import-export/import-utilities.c @@ -37,7 +37,7 @@ /********************************************************************\ * Setter and getter functions for the online_id kvp frame in - * Account and Transaction + * Account, Transaction and Split \********************************************************************/ const gchar * gnc_import_get_acc_online_id(Account * account) @@ -70,4 +70,33 @@ void gnc_import_set_trans_online_id(Transaction * transaction, kvp_frame_set_str (frame, "online_id", string_value); } +gboolean gnc_import_trans_has_online_id(Transaction * transaction) +{ + const gchar * online_id; + online_id = gnc_import_get_trans_online_id(transaction); + return (online_id != NULL && strlen(online_id) > 0); +} + +const gchar * gnc_import_get_split_online_id(Split * split) +{ + kvp_frame * frame; + frame = xaccSplitGetSlots(split); + return kvp_frame_get_string(frame, "online_id"); +} + +void gnc_import_set_split_online_id(Split * split, + const gchar * string_value) +{ + kvp_frame * frame; + frame = xaccSplitGetSlots(split); + kvp_frame_set_str (frame, "online_id", string_value); +} + +gboolean gnc_import_split_has_online_id(Split * split) +{ + const gchar * online_id; + online_id = gnc_import_get_split_online_id(split); + return (online_id != NULL && strlen(online_id) > 0); +} + /* @} */ diff --git a/src/import-export/import-utilities.h b/src/import-export/import-utilities.h index 24eb3b7d92..f2d34452f1 100644 --- a/src/import-export/import-utilities.h +++ b/src/import-export/import-utilities.h @@ -46,6 +46,20 @@ void gnc_import_set_trans_online_id(Transaction * transaction, const gchar * string_value); /** @} */ +gboolean gnc_import_trans_has_online_id(Transaction * transaction); + +/** @name Setter-getters + Setter and getter functions for the online_id kvp_frame for + Splits. + @{ +*/ +const gchar * gnc_import_get_split_online_id(Split * split); +void gnc_import_set_split_online_id(Split * split, + const gchar * string_value); +/** @} */ + +gboolean gnc_import_split_has_online_id(Split * split); + #endif /** @} */ diff --git a/src/import-export/ofx/gnc-ofx-import.c b/src/import-export/ofx/gnc-ofx-import.c index d50f6a84b6..a14cd9d9ae 100644 --- a/src/import-export/ofx/gnc-ofx-import.c +++ b/src/import-export/ofx/gnc-ofx-import.c @@ -139,10 +139,6 @@ int ofx_proc_transaction_cb(struct OfxTransactionData data, void * transaction_u transaction = xaccMallocTransaction(book); xaccTransBeginEdit(transaction); - if(data.fi_id_valid==true){ - gnc_import_set_trans_online_id(transaction, data.fi_id); - } - if(data.date_initiated_valid==true){ xaccTransSetDateSecs(transaction, data.date_initiated); } @@ -339,6 +335,9 @@ int ofx_proc_transaction_cb(struct OfxTransactionData data, void * transaction_u if(data.memo_valid==true){ xaccSplitSetMemo(split, data.memo); } + if(data.fi_id_valid==true){ + gnc_import_set_split_online_id(split, data.fi_id); + } } else if(data.unique_id_valid == true && data.security_data_valid @@ -393,6 +392,10 @@ int ofx_proc_transaction_cb(struct OfxTransactionData data, void * transaction_u { xaccSplitSetMemo(split, data.security_data_ptr->memo); } + if(data.fi_id_valid==true) + { + gnc_import_set_split_online_id(split, data.fi_id); + } } else {