GnuCash
Contact   Instructions
Bug 798149 - CSV transaction Import setting loses account name if it is changed after setting is memorized
Summary: CSV transaction Import setting loses account name if it is changed after sett...
Status: RESOLVED FIXED
Alias: None
Product: GnuCash
Classification: Unclassified
Component: Import - CSV (show other bugs)
Version: 3.8
Hardware: PC Linux
: Normal normal
Target Milestone: ---
Assignee: import
QA Contact: import
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-16 11:05 EDT by David Carlson
Modified: 2021-09-24 18:53 EDT (History)
6 users (show)

See Also:


Attachments

Description David Carlson 2021-03-16 11:05:41 EDT
First, prepare a CSV transaction import file for an existing account such as a credit card account.

Then commence importing that file, configuring the CSV importer with valid settings including the target account name, date format, and desired import column information.  Save the settings under an appropriate name for later use. Complete the import to verify that it works properly.

Then edit the name of the account by changing a few characters to represent some change that the financial institute made such as a name change or credit limit change.

Then prepare a new CSV transaction import file for that account.

Commence importing the file.  Note that when the named settings configuration is loaded, the account name is missing.  I would expect the changed account name to be updated as it would be in the main database.
Comment 1 Christopher Lam 2021-03-16 11:20:44 EDT
There's been some big changes to the CSV transaction importer. Does 4.4 exhibit such behaviour?
Comment 2 David Carlson 2021-03-16 16:02:40 EDT
The behavior is the same in release 4.4 in Windows 10.  The account name is lost from the saved import configuration if the target account name has been edited by changing a few characters.
Comment 3 Bob 2021-03-17 11:40:09 EDT
I know what is wrong with this, we are saving the account full name instead of the account guid. Will try and change later.
Comment 4 Bob 2021-03-18 06:58:08 EDT
I have pushed a change to maint and will be in version 4.5 which saves the base account setting by its Guid. When the base account is recalled from the saved settings, the account will first be found by Guid lookup, if unsuccessful the full path will be used as before and if found the saved base account setting will immediately be updated with the Guid for future use.


Please test and confirm bug is fixed.
Comment 5 Geert Janssens 2021-03-18 08:43:09 EDT
Thanks for working on this Bob.

A note: while this is certainly an improvement it will break import matching in gnucash 4.4 and older when opening a file that had imports done in 4.5 or more recent. The imports in 4.5 will overwrite the full account names stored in the maps with guids. When you then try to import again in an older gnucash version, it will search for full account name but find a guid, so no match.

Personally I think that's ok. There's no important data loss, just the inconvenience of having to retrain the importer when going back and forth between gnucash versions. Because of that I believe it should come with an explanation or warning in the release notes.
Comment 6 David Carlson 2021-03-18 10:33:18 EDT
Thank you gentlemen for working on this bug.  

Would it be possible to either provide alternative code to leave the setting generated by previous versions while saving the new settings to be backwards compatible at least until the next major release, or if some settings have been found to include a warning when release 4.5 or later is first run that they will no longer be found if the file is opened by an earlier release, as Geert has suggested?

Also, do you need any additional information from me?
Comment 7 Christopher Lam 2021-03-18 10:43:12 EDT
This new strategy would fail for an obtuse user who has a top-level account whose name is the guid of a different top-level account <evil-grin>

I'd be possible to be fwd+backwd compatible by adding another field; so, CSV import settings have:
- account name
- account guid (maybe)

The account search would first query GUID; 
- if missing then retrieve name and store GUID.
- if present then retrieve both, find account using GUID and test if name matches
  if they match then all good; return account
  if they mismatch then name has priority; find using name and store GUID.

I think this works?
Comment 8 Geert Janssens 2021-03-18 10:57:32 EDT
(In reply to Christopher Lam from comment #7)
>   if they mismatch then name has priority; find using name and store GUID.
> 
This part would mean this very bug would continue to exist. If the name mismatch happens because an account name has been changed, the old name wouldn't match the new name and hence the existing map would be skipped, even though the guid would have pointed to the proper account.
Comment 9 John Ralls 2021-03-18 12:48:22 EDT
We set a feature when we did this conversion for Bayesian maps so ISTM we should do the same here, and while we're at it we should check for and fix anywhere else that we're using account names instead of guids as references so we don't have to do this again.
Comment 10 John Ralls 2021-03-18 12:55:39 EDT
(In reply to Christopher Lam from comment #7)
> This new strategy would fail for an obtuse user who has a top-level account
> whose name is the guid of a different top-level account <evil-grin>

That user deserves the outcome. Actually, that user deserves GnuCash refusing to load the data file, but it's too much work to implement and would slow down loading, something that already takes too long.

> 
> I'd be possible to be fwd+backwd compatible by adding another field; so, CSV
> import settings have:
> - account name
> - account guid (maybe)
> 
> The account search would first query GUID; 
> - if missing then retrieve name and store GUID.
> - if present then retrieve both, find account using GUID and test if name
> matches
>   if they match then all good; return account
>   if they mismatch then name has priority; find using name and store GUID.
> 
> I think this works?

Nope. It would work better if it gives priority to GUID, best if it raises an error and refuses the import.
Comment 11 Bob 2021-03-18 12:57:58 EDT
(In reply to Geert Janssens from comment #5)
> Thanks for working on this Bob.
> 
> A note: while this is certainly an improvement it will break import matching
> in gnucash 4.4 and older when opening a file that had imports done in 4.5 or
> more recent. The imports in 4.5 will overwrite the full account names stored
> in the maps with guids. When you then try to import again in an older
> gnucash version, it will search for full account name but find a guid, so no
> match.
> 

I forgot about backward compatibility and I am inclined not to make it more complicated but maybe Chris has a point and do the following...

For 4.5 going forward, 
Save BaseAccount, full path saved, as existing
Save BaseAccountGuid, guid of account, new setting

Load from BaseAccountGuid first, if it fails, could be missing
Load from BaseAccount, if successful immediately save BaseAccountGuid
                       if unsuccessful then that's it, account selection blank.


For 4.4 and below they will try and use existing full path, the BaseAccountGuid will survive until the CSV save button is pressed.

I think that may work.
Comment 12 Bob 2021-03-19 07:15:21 EDT
I have pushed my suggestion to maint.

So going forward the BaseAccountGuid is searched first, if successful a check is made on the saved full path and updated if required.

If no Guid found, the full path is used and if found the account Guid is immediately saved for future use, if not the account combo is blank with a warning message as before.

Previous version can still use the BaseAccount full path as before and on testing the BaseAccountGuid will stay until the CSV save settings button is used.

I think that should be OK for now, I will add to my list to try and check to see if there any more instances like this and maybe do a bulk change with a feature flag.
Comment 13 Christopher Lam 2021-03-19 10:38:06 EDT
I had the same thought for bug 737006 and think the same strategy can be used.
Comment 14 Christopher Lam 2021-03-24 12:23:22 EDT
The new Guid code: I wonder if the account name option should be deprecated in master to simplify code, and ensure future hackers don't wonder why maintain two account descriptors.

The disadvantage is that 5.x CSV data won't be usable in 4.x.

Note You need to log in before you can comment on or make changes to this bug.