Rickie,

Just to be clear, have you saved your GnuCash data file as uncompressed xml and searched it for the FITID’s that do not get imported?

If so, if you send me (goodchris96@gmail.com) your OFX file, I’ll see if I can duplicate the problem, and if so, debug it.

Hi Rickie,

I tested importing your 80 transaction ofx file in Linux flatpak 4.5 and both the problem transactions showed in the import matcher and all 80 transactions were imported.
I should test in Windows GnuCash 4.5 but before I do that, here are a couple of suggestions:
1. Can you try running "Actions", "Check & Repair", "All transactions",
 then test again. Ideally (to ensure you can go back to 3.11 if needed) you should have upgraded to 3.11, run the Check & Repair, then upgraded to 4.5 and run the Check & Repair again. See https://wiki.gnucash.org/wiki/FAQ#Using_Different_Versions.2C_Up_And_Downgrade

2. I'm not sure if it matters, but could those 2 transactions be somehow automatically matching to accounts in a different currency?

My money is on *existing* splits having online_id already therefore the *incoming* splits are discarded. It would be nice if the importer would identify 'xxx splits imported, yyy splits discarded, zzz splits for matching' somewhere in the import matcher.

I suggest that if (and only if) the import finds transactions that will be dropped, it should come up a window showing at least <DTPOSTED>, <NAME>, <MEMO>, <TRNAMT>, and reason for dropping for each transaction.

Rickie, Have you had a chance to action Comment 2?

Chris - as you requested - on my 4.5 version I performed the Check & Repair (all accounts) nothing reported after it took a while to process 10 years of account data.
I then attempted to reload the ofx file containing the 2 transactions, it reported the same result - 2 transactions in the import - but no matching transactions to process.
The problem is still manifesting the same way.

I do not have any multiple currencies for any accounts, so that is not a factor.

Could you please test in a windows 4.5 version to see what you get

I have since loaded more monthly ofx files and each time the credit card account drops up to two transactions per month, other bank accounts are ok. The transactions that get dropped appear to be random and I dont do any splits or anything special, just simple import and matching via Baysian.

At the very least the importer needs to present a list of what does not get matched and a reason to aid reconciliation and resolution.

I still have 10 months of credit card ofx imports to process, so I will hold for now in case you can identify something to check to help diagnose whats happening during the import. Happy to run any patched version with any extra diags included.

Hi Rickie,
I have tested in GnuCash 4.5 in Windows and again all 80 transactions were imported.
In order to reproduce and debug I will need a copy of your gnucash datafile as at before the above 2 transactions were imported.
If you send to me or allow me to download from somewhere, I promise to ensure no one else will have access to it and will delete as soon as the problem is resolved. If you're not OK with me temporarily having a full copy of your database, perhaps you can create a minimal example of this problem? Failing that, it will be probably not be possible to resolve.

(In reply to Chris Good from comment #6)
> Hi Rickie,
> I have tested in GnuCash 4.5 in Windows and again all 80 transactions were
> imported.
> In order to reproduce and debug I will need a copy of your gnucash datafile
> as at before the above 2 transactions were imported.
> If you send to me or allow me to download from somewhere, I promise to
> ensure no one else will have access to it and will delete as soon as the
> problem is resolved. If you're not OK with me temporarily having a full copy
> of your database, perhaps you can create a minimal example of this problem?
> Failing that, it will be probably not be possible to resolve.

Ok thanks - I will send the relevant files to you by email.

Hi Rickie,

I have duplicated your problem in GnuCash 4.4 for Windows 10.
I have found that in the example you sent me 21/6/2021 where a transaction with <FITID>202009300002 (online_id) was missing from the Generic transaction window, there are no other splits for the account being imported (say CREDITCARDA) with the same online_id.
But there IS a transaction already in your GC data file which is a transfer from the account being imported (CREDITCARDA) to another bank account (BANKACCTB) and  the BANKACCTB split has the same online_id.

To prove it is the duplicate online_id on the BANKACCTB split, I editted your GC datafile to change the online_id of the BANKACCTB split, and then the missing transaction DOES appear in the Generic transaction matcher window.

It seems to me this is the bug. Could a developer please confirm the import should only match online_id for splits for the account being imported?

Chris, yes, it should be per account. This check is performed by gnc_import_exists_online_id, https://github.com/Gnucash/gnucash/blob/maint/gnucash/import-export/import-backend.c#L1114.

Hi Rickie,

I'm keen to have a try at fixing this problem in GnuCash but I suspect it is unlikely I will have it fixed before 4.6 is released 2021-06-27.

To enable you to continue your imports asap, you might like to try preprocessing your OFX files using my IngAusOfxFix utility https://github.com/goodvibes2/IngAusOfxFixWin which will append "." + DTPOSTED + "." + TRNAMT to the input file FITID. This will make it much more unlikely that an imported transaction FITID will match an existing online_id on a split from a different account in your GnuCash datafile.

The reason you have found this problem but no one else has yet I think is because of your banks FITID of yyyymmddnnnn in both accounts where yyyymmdd is the date posted and nnnn is a sequence no starting from 0001 for each day.

If you are good with some other language, you may prefer to write some other program to make the FITID's unique per account, for example appending the AccountName to the existing FITID.

Rickie, PS my IngAusOfxFix utility was created for Savings accounts, not credit cards, but I have just tested it with your ofx file and it worked fine.

(In reply to Chris Good from comment #11)
> Rickie, PS my IngAusOfxFix utility was created for Savings accounts, not
> credit cards, but I have just tested it with your ofx file and it worked
> fine.

Hi Chris 
Well done on finding the cause of the issue, I really appreciate your time and effort to get this resolved.

Your diagnosis is spot on as I deliberately import the Credit Card account last each month after the other bank accounts. So that is why its only impacting on transactions in the credit card import.

Thanks for the utility, that will do the trick as a work around.
I don't have any real urgency as I just need it fixed before I prepare my next  tax return (before Oct usually), so if the fix doesn't make the 4.6 release what might be the timing there after?

Hi Rickie, According to https://wiki.gnucash.org/wiki/Release_Schedule
4.7 is due 2021-09-26.

Hi John Ralls,
I have confirmed the code is incorrect.
gnc_import_exists_online_id() does
  xaccAccountForEachTransaction (dest_acct, collect_trans_online_id, new_hash);
to build new_hash (a hash of all the online_id's for the account being imported.
collect_trans_online_id(), also in import-backend.c, is run for each transaction which includes a split for the account being imported.

The problem is that collect_trans_online_id() looks at all the splits in the transaction, not just the ones for the account being imported.

I cannot see a nice way to pass the account being imported to collect_trans_online_id().

Could you please suggest how I should ensure new_hash only contains online_id's for the account being imported?

https://github.com/Gnucash/gnucash/pull/721#discussion_r457118918

I think my comment is relevant?

Chris Good,

Create a struct {Account*, GHashTable*} and pass that to xaccAccountForEachTransaction instead of just the GHashTable*. Modify collect_trans_online_id accordingly.

Alternatively fake xaccAccountForEachSplit (note https://github.com/Gnucash/gnucash/blob/maint/libgnucash/engine/Account.cpp#L3520) by looping over the split list directly (you can get it with xaccAccountGetSplitList). There's no point in getting the txn from each split and iterating over the txn's split list only to find the split you've already got.

static GHashTable*
collect_acct_online_ids (Account* acct)
{
    GHashTable* hash = g_hash_table_new (g_str_hash, g_str_equal);
    for (GList* node = xaccAccountGetSplitList(dest_acct); node; node = g_list_next (node))
    {
         Split* split = node->data;
         const gchar* split_id = gnc_import_get_split_online_id (split);
         const gchar* trans_id = gnc_import_get_trans_online_id (split->parent);
         if (trans_id)
            g_hash_table_add (hash, (void*)trans_id);
         else if (split_id)
            g_hash_table_add (hash, (void*)split_id);
     }
     return hash;
}

Note that this is still vulnerable to cross-account online-ids because the transaction's online-id might have come from importing a different account.

But let's think this through a little more. How is it that the CC account has a transaction with a split from another account with the same FITID? That can only be a transfer to or from another asset account, either an overdraft or a payment. If the user imports the other account first then the transaction is created and we don't want to duplicate it during the second import. If the reporter's bank is screwing up and using the same FITIDs for different transactions then modifying GnuCash to accommodate it will result in making duplicate transactions for all of the users whose banks are doing it right.

Christopher Lam,
Yes you raised this previously. It seems it was not discussed enough.

John,
Thanks for the guidance. I actually thought of that in the middle of the night!

The latest (2.3) OFX spec https://www.ofx.org/downloads.html says:
FITIDs must be unique within the scope of an account but need not be sequential or even increasing.
Clients should be aware that FITIDs are not unique across FIs. If a client performs the same type of request
within the same scope at two different FIs, clients will need to use FI + ACCTID + FITID as a globally unique key in a client database. That is, the <FITID> value must be unique within the account and
Financial Institution (independent of the service provider).

That seems to be pretty clear that the scope should only be within an account.
GnuCash stores the online_id linked to the split for the account being imported, not the transaction.

My bank DOES use the same fitid on a transfer transaction between my accounts when you look at the files exported from both accounts. That saves me having to delete a duplicate transaction now but I believe, previous to the changes in 4.2, I would have had to delete a duplicate.

However, I see that for the OP's bank accounts (and I believe all 3 in question are from the same financial institution, a major Australian Bank), FITID's for the same transaction are NOT the same in different accounts and it is only a coincidence that some have the same FITID because of the poor method of using yyyymmddnnnn as FITID.

I think there should be option, ideally set for each pair of bank accounts you may transfer between, but maybe system wide would do, where you specify if FITID matching should be limited to only the account being imported.

Failing that, IMHO I think FITID's should only be matched against splits for the account being imported as it much easier to find and delete a duplicate transaction, as opposed to find and enter a missing transaction.

I still think it would be a good idea to be shown a list of transactions that were dropped if any are found as per my comment 4.

Thanks for the action on this issue so far - some guidance please - would you like me to engage the Bank if they are indeed the root cause?

I am happy to encourage and request them to fix their export if there is a clear breach of the OFX standard. If you recommend me doing that, please provide a statement of the standard breach and any recommended resolution so I can engage them.

Hi Rickie,
There is no breach of the OFX standard that I know of.
It's just a poor choice of method your bank uses to create the FITID.
Ideally a transfer between 2 accounts from the same financial institution should have the same FITID when you export the transactions from both accounts. This is so you can easily tell if the transaction has already been imported.
Failing that, the FITID's should be different enough that there is no possiblity of 2 unrelated transactions having the same FITID.

Everybody,
If my idea of having an option to specify only to look in splits for the imported accounts for an existing FITID is acceptable, I'd like to start on that. It seems too hard to have an option for each pair of accounts, so I'd go for a system wide preference. There doesn't seem to be a screen where this option could be chosen each import. Comments please?

How about displaying the FITID-matched transactions in the match window just like text-matched ones? 

That way nothing is silently discarded and users like Rickie whose banks aren't careful about such FITIDs can simply click the A column to tell the matcher that they're actually new transactions. The FITID match shouldn't preclude the text matcher from also running so that the user can double-click the match and see the text-matcher possibilities as well as the FITID one.

John,
I think I know generally what you mean and that sounds good.
By the text matcher, do you mean the 'Select matching existing transaction' window?

I assume you mean display the FITID-matched transactions in the Generic Import Matcher instead of silently dropping them?
And by 'text-matched' transactions, I assume you mean those matched by non FITID means (date, amount, description & memo).

As an aside,
I haven't looked into the code deeply yet, but I don't seem to be able to get the Generic Import Matcher to show more than 1 possible match when you click on the Right arrow to the left of a transaction with a possible match. Although if you go into 'Select matching existing transaction', multiple possible matches can show...

Also, I've found a crash in the Gen Imp Trans Matcher which I'll try to fix first. I'll raise a bug tomorrow.

Chris,

Yes, I think you've got the idea.

You don't need to write a bug for the new crasher, just write a PR and describe the crash and how you fixed it there.

Pull Request https://github.com/Gnucash/gnucash/pull/1069 is my fix for 2 crashes in the Gen Imp Trans Matcher currently waiting to be checked and merged.

Pull Request https://github.com/Gnucash/gnucash/pull/1098 is my fix for this problem.

Hi Rickie,
The fix for this problem (PR 1098) has been merged into maint and will be part of the 4.7 release scheduled for 26 Sep 2021.
If you wish, you can test before then using the Windows Daily maint build which should be available for download tomorrow in https://code.gnucash.org/builds/win32/maint/

Note: Bug 798298 is a request to have this new behavior optionally.

Chris, could you send me by email the database and ofx file to repro the issue? I have reverted the code change and added code to make sure only the right splits are considered when ignoring an imported transaction based on its FITID but it would help me greatly to have a test case. ripngo at gmail dot com.

Hi Jean,
I'm have already deleted Rickie's data, so I have created a test database (Bug798205Demo.gnucash) and Bug798205DemoBankAcct2.ofx file that demonstrates the problem which I will attach to this bug.
Bug798205Demo.gnucash contains 1 transaction:
Date 		Desc			Acct		Debit	Credit	FITID
02 Sep 2021	Tfr Acct1 to Acct2	BankAcct1		1.00	1
					BankAcct2	1.00		2

Bug798205DemoBankAcct2.OFX contains 1 transaction:
Date		Name 		Memo		Amt		FITID
04 Sep 2021	YeOld Book Shop Buy Books	-2.00		1

When this ofx file is imported (Bank 2222 Acct 22222222 matches to BankAcct2), the transaction is silently dropped - the "Generic import transaction matcher window" does not appear, only the dialog that shows "1 transactions processed, no transactions to match".

Created attachment 374172 [details]
Demo Database for Comment 28 uncompressed xml

Created attachment 374173 [details]
Demo .ofx file for Comment 28

OK great! Thanks a bunch.

Chris, thanks a lot for the attachments. They allowed me to verify that my (very small) change indeed fixes the issue at hand. 
I created a PR for this
https://github.com/Gnucash/gnucash/pull/1129
I wish we had a unit test for this type of problem, in which case the account and ofx file you created would be very useful.

> I wish we had a unit test for this type of problem, in which case the account and ofx file you created would be very useful.

So write a test. Tests should be fast so it would be better to hard-code the splits in the test code than to do a file access.

(In reply to John Ralls from comment #33)
> > I wish we had a unit test for this type of problem, in which case the account and ofx file you created would be very useful.
> 
> So write a test. Tests should be fast so it would be better to hard-code the
> splits in the test code than to do a file access.

Don't think I'm going to do that. The unit test framework is another nightmare thing mixing C++ and C, doing "mock" this and that. I don't want to touch it.

Doesn't have to be fully featured unit test, a basic book and ofx string is all that's required initially. There are lots of test-*.c to copy from.

(In reply to Christopher Lam from comment #35)
> Doesn't have to be fully featured unit test, a basic book and ofx string is
> all that's required initially. There are lots of test-*.c to copy from.

Please don't emulate any of the test-*.c files. Even the GLib test ones (which are mostly named utest-*.c) use the deprecated GLib testing framework and the older tests that Josh Sled wrote were a valiant effort when he wrote them because there weren't any good testing frameworks in C in the early aughts, but that's not the way to write tests in 2021.

Use Google Test. There's plenty of gtest-*.cpp to template from, it's well documented at https://github.com/google/googletest/tree/master/docs and it's really easy to use--much easier than GLib test never mind Sled's framework.

(In reply to Christopher Lam from comment #35)
> Doesn't have to be fully featured unit test, a basic book and ofx string is
> all that's required initially. There are lots of test-*.c to copy from.

Not even that. Jean's change loops through an account's splits and creates a hash table of the splits' online-ids, so a unit test just needs an account--you'll need to create a book to pass to xaccMallocAccount--and populate it with a few splits, some having online ids that demonstrate the various aspects of this bug and the others that are related. The EXPECTS will test the hash table's contents for each the various online ids.

Sorry guys, don't have time for this.