The feature is not yet available, but it would be an improvement. Thanks for reporting this.

Created attachment 310594 [details] [review]
Allow saving of account mappings

This patch allows the saving of account mappings used in the CSV Transaction Import. These mappings are saved to a text file and when recalled, the retrieved Guid is used to select the proper account match.

Created attachment 310596 [details] [review]
Add Other account

This patch adds the option to specify the other account and other memo field for the CSV Transaction import.

To achieve this I have used the assistant page forward function to do the page navigation and it is here where I do all the requirement testing. Using this function allows the back button to work better when jumping over pages. The down side to this is that this function gets called multiple times in page navigation so I have used a test so that it only runs once per page navigation.

Using a keyfile in GNC_DOT_DIR is bad, it separates data from the book it belongs to so that if a user works on the book on two systems the map won't be synced between them. Using a hard-coded name is *very* bad, it means that there's only one key file for all of a user's books.

Isn't this code for CSV import only? Shouldn't it be in csv-imp? What are the QIF and OFX types for? Are you contemplating preloading matches somehow? 

Style note: If you have a block which looks like a function then it should be a separate static function. If you have a bunch of state that needs to be passed around to static functions put it all in a struct and pass a pointer.

I was in two minds about the directory / file name when I did this. The reason I chose this one is that there is a use case when you start with no current file and use a CSV file for input with you creating matching accounts on the fly. In this case there is no Gnucash file name to base the map file name on, maybe I will only save the account mappings when we have a valid file name as this should only happen once.

The reason for the location and extra types was that maybe it could be used for QIF and OFX account mappings also. I seem to recall there was a similar page in those assistants.

(In reply to Bob from comment #5)
> I was in two minds about the directory / file name when I did this. The
> reason I chose this one is that there is a use case when you start with no
> current file and use a CSV file for input with you creating matching
> accounts on the fly. In this case there is no Gnucash file name to base the
> map file name on, maybe I will only save the account mappings when we have a
> valid file name as this should only happen once.
> 
> The reason for the location and extra types was that maybe it could be used
> for QIF and OFX account mappings also. I seem to recall there was a similar
> page in those assistants.

There is no "start with no current file". You might start with an empty book, but GnuCash won't do anything without a book. Try running `gnucash --nofile`: You'll see that the title bar says "Unsaved Book".
So lose the file and save the association data to the book's KVP, ideally using the same mechanism as the non-Bayesian matcher for OFX. 

I don't think that an OFX or QIF import is likely to contain GnuCash accounts since GnuCash has no facility for exporting either format. Early over-generalization is a bug, just like premature optimization.

Created attachment 312066 [details] [review]
Add Saving CSV Import Account mapping

This patch save the mappings using KVP data via the existing non-bayes procedures with the addition of a delete procedure in Account.c and replaces the extra file.

Created attachment 312067 [details] [review]
Add Other Account to CSV Import

This patch replaces the first attempt and now uses the new save CSV account mapping patch.

Review of attachment 312066 [details] [review]:

Thank you for your patches.

I applied them to master. They won't build however. This is the build error:

make[5]: Entering directory '/kobaltnet/janssege/Development/builds/gnucash/master/src/engine'
/bin/sh ../../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I../..  -I/kobaltnet/janssege/Development/gnucash/gnucash-master/lib/libc -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/core-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I../../src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -pthread -I/usr/include/guile/2.0  -I/usr/include -DG_LOG_DOMAIN=\"gnc.engine\"  -Werror -Wdeclaration-after-statement -Wno-pointer-sign  -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations  -Wno-unused -g  -std=gnu99 -g  -MT libgncmod_engine_la-Account.lo -MD -MP -MF .deps/libgncmod_engine_la-Account.Tpo -c -o libgncmod_engine_la-Account.lo `test -f 'Account.c' || echo '/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/'`Account.c
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I../.. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/lib/libc -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/core-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I../../src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/guile/2.0 -I/usr/include -DG_LOG_DOMAIN=\"gnc.engine\" -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -MT libgncmod_engine_la-Account.lo -MD -MP -MF .deps/libgncmod_engine_la-Account.Tpo -c /kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/Account.c  -fPIC -DPIC -o .libs/libgncmod_engine_la-Account.o
/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/Account.c:5116:1: error: no previous prototype for 'gnc_imap_delete_account' [-Werror=missing-prototypes]
 gnc_imap_delete_account (GncImportMatchMap *imap,
 ^
cc1: all warnings being treated as errors
Makefile:942: recipe for target 'libgncmod_engine_la-Account.lo' failed



You probably forgot to add a function prototype for gnc_imap_delete_account. For the other gnc_imap_* functions these prototypes are defined around line 5040 in the same file.

Having said that however, I'm not too fond of this construct. Because the prototypes are in the source file and not in some (private) header file you are forced to use extern declarations to use these functions. I'd prefer to move the prototypes to AccountP.h and include that header file instead. Or perhaps even to Account.h and consider these as public interface to the Account object.

I realize you took the import-backend.c code as example, which also uses the extern definitions. I think they can be avoided in this case. So if no one with more experience than me objects, I propose you fix import-backend.c in a separate commit as well.

The contortion of not using the header to pass the gnc_imap_foo function declarations is my fault. They used to be static functions in import-backend.c, but they rely on intimate access to the Account's KVP in a way that simple get/set functions wouldn't accommodate. I didn't want to expose them to the world but I also didn't want to spend a lot of time rewriting them into a clean API when I knew that I'd be rewriting the whole class in C++ later.

Since we now need those functions in two places instead of one, yes, go ahead and move them to Account.h. Let's rename them from gnc_imap_foo to gnc_account_imap_foo to make it clear what the owning class is.

Created attachment 313608 [details] [review]
Rename Imap functions

This patch renames the Imap functions in Account.c and defines them in Account.h so that you do need to use extern to use them. I have also included a delete function for the non Bayesian functions and added a test case for it.

Created attachment 313609 [details] [review]
Add Saving CSV Import Account mapping

This patch replaces previous one and no uses the renamed Imap functions to save the account mappings as kvp data.

Created attachment 313610 [details] [review]
Add Other Account to CSV Import

This patch replaces previous one which adds the ability to specify counter accounts along with the memo field.

Hi Bob, your first two patches are fine now and I have committed them to master.

While testing the last one, I ran into a couple of inconsistencies that need some cleanup first:

My first test was with a csv file exported from gnucash some time ago. It does contain relevant information such as the other account to run this test. After marking the columns and clicking next I come on the Account Mapping page. For some transactions I can simply select an account from the list. For other accounts I need to create a new account.

1. First issue (minor):
My Other Account names in the csv file are hierarchical as in Gnucash (they have ':' in them). The new account dialog uses the first component of the Other Account Name as new account name. That is however not the best option as that's usually the most generic part like Assets in Assets:Current Assets:BankX. The obvious solution would be to strip anything up until the last ':'. That may however also be unwanted if the import file does not come from gnucash originally. So maybe you should parse the Account Names and replace each occurrence of the Account Separator character as it is defined in the preferences.

2. Second issue:
After entering all the details for a new account, I click Ok. The new account is added to the account hierarchy. The New Account window doesn't close however, which is confusing. Trying to click Ok again (thinking I mis-clicked) adds a second account with the same name as a subaccount of the one I just created. Hitting Ok here should close the new account dialog.

The remainder of the first run worked fine. I saved my file and restarted gnucash for a second test run. This time I'm using the same csv file to import, but with a few extra transactions added. I specifically want to test how the importer behaves if some transactions are already imported.

So after setting the columns, I arrive again at the Account Matching page.

3. Third issue:
This time however it only offers me one account to match (which I also matched already in my previous run and the importer correctly remembered as it proposes the same match). The account it asks for is the account I have in the column I marked with "Account". It Account matcher page doesn't ask for any account to match from my "Other Account" column, although the new transactions have all values in Other Account that aren't mapped yet.
So I'd have expected the matcher to ask at least for the accounts it hadn't encountered yet. And if it asks again for one existing account it should ask for all already known accounts. I'm not sure if it should ask for already known accounts again, perhaps it should. It depends on whether or not it's a reasonable assumption that the same name in the "Other Account" column will always map to the same gnucash account across imports. And there's also the consideration of how to correct mappings if the user made a mistake ?

Anyway, I went on to the next page to see what would happen. On this page the previously imported transactions are properly recognized and marked as matched already.

4. Fourth issue:
My new transactions however are all highlighted in yellow, indicating they should be updated. On this page however it's no longer visible what the Other Account value was in the original csv file, so it's difficult to remember which account these transactions should be completed to. Double-clicking one of the transactions gives the account hierarchy. Trying to add another account here will propose a generic name for the account.
I suppose this would be fixed if in the 3rd issue each unmapped account is properly asked for.

That's where I stopped testing for now. Feel free to work with a follow-up patch rather than replacing the current patch if that's more comfortable for you.

Well this is disappointing as I thought I tested it quite well. I will look at comments above and do more testing and report back. Sorry for the inconvenience.

Heh, I know the feeling. I guess we have all been in such a situation before (I still remember my lot link story from earlier this year...). Also more eyeballs simply catch more bugs, or try more use cases. No need to apologize :)

Created attachment 314928 [details]
Archive of screen images

Still pondering but thought I would add a sample test run I did with screen images so I could narrow down your observations...

ScreenShot1 just shows my test file account structure.
ScreenShot2 Shows the Preview page of my test csv file with two rows excluded.
ScreenShot3 Shows the Account match page with all needed matches.
ScreenShot4 Shows all matches populated.
ScreenShot5 Shows all Transaction matches and I then go on to Apply.

Second run of same test file...
ScreenShot7 Shows the Preview page of my test csv file with all rows selected.
ScreenShot8 Shows all retrieved matches, no changes made.
ScreenShot9 Shows all Transaction matches and I then go on to Apply.

This second run completes as expected with new transactions being added.

Issue1, I do not see this unless I am looking at the wrong thing. When I click on row 9 and choose to create a new account I have "New Account" in the Account Name field and "Assets:Current Assets" as the parent. But when I click on row 10 and choose to create a new account I have nothing in the Account Name field and "Assets:Current Assets:Savings Account" as the parent.

Issue2, I see what you mean, you click OK on the "New account Dialogue" and it takes you back to "Select Account Dialogue".

Issue3 and 4 Not sure what is going on there, would it be possible to take some screen shots like mine so I can see what is happening.

Created attachment 316663 [details]
Screencast illustrating issue 1 & 2

My apologies for taking so long to reply. Daily life has been taking most of my time lately.

Anyway, I set out to create screen casts of the issues I experienced. I think they will convey more detail than words ever can.

This first screencast illustrates the issues 1 and 2.

While doing the screencast I realized my interpretation of what happens was not accurate.

I now believe this is what happens:
1. The other account name is hierarchical: Activa:Transfer
2. Neither account exists in the current hierarchy.
3. However "Activa" is Dutch for "Assets" so I would want to create a "Transfer" account under "Assets".
4. I click on new account with "Assets" selected as base account.
5. The importer detects there's no subaccount named "Activa" under "Assets" and proposes to create one.
6. In my previous attempt I remember I changed the name of this new account to "Transfer", believing it poorly offered a default name. In the screencast however I have just accepted to create the new account "Activa".
7. Immediately a new account creation dialog pops up, this time for the sub account "Transfer" (from our original Activa:Transfer). As this goes really fast, it looks like the dialog never closed. In my previous attempt it looked like the same account was asked again, because I renamed the first account creation. It is however a second account that's being created.

Net conclusion: there's currently no easy way to create "Activa:Transfer" as "Assets":"Transfer" where "Assets" is an existing parent account and "Activa" is not created as part of the process.

Does this clarify the first 2 issues ?

I'm currently not able to reproduce the other 2 issues. I guess they are the result of my trying to make sense of what was happening in issue 1-2. You can ignore number 3 and 4 for now. I'll create a new bug should I figure out how to reproduce them consistently.

As a matter of fact, I even found a workaround for the 1-2 issue: to only create the transfer account, one can:

1. Click on New account
2. Change the account details to "Transfer" (and other correct settings like "Assets" parent account)
3. Click ok
4. Cancel the second dialog window that offers (again) to create the "Transfer" account
5. Because you cancelled, the importer won't automatically link the newly created "Transfer" account, so you now have to select it yourself before closing the account matching dialog.

That's good enough to go into master.

I won't close the bug yet however as a reminder there's still some confusing situation to improve upon. The workaround is only just that - a workaround.

Created attachment 316836 [details] [review]
Allow the Account Matcher dialog to close automatically.

This patch should fix problem 2, when you have created a new account the account matcher dialog will close and update the account list in the assistant.

Created attachment 316838 [details] [review]
Test the match text for valid account path

This patch should fix problem 1. The match text is split with the current separator and this is used to create two strings, a path and the last part which possibly could be the account name. Using the path a test is made to see if it is valid, if so the last part is added and this new string is returned. If the path is not valid, the original text is returned with the separator changed to an alternative. This allows the user to decide the account name and where the new account should be in the tree and limits the calls to create a new account to one.

Thanks. That is a nice way to deal with it. Committed with a small followup commit to slightly clean up the case construct.

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=706021. Please update any external references or bookmarks.