Use transaction texts if they are available

[uniederer]

While importing a transaction list in mt940 format, I found that not all information supplied by the bank has been used to create a proper transaction description. Finally I found that the purpose supplied by aqbanking does not contain the whole non-swift-section (:NS:) as lines with a number >14 are treated specailly. The one of interest for me (line 17) is treated as transaction text.

This pull request add some additional logic to the aqbanking adapter of GNUcash that uses transaction text if available as a description and adds the purpose as a note. In case there is no transaction text, the purpose is used for the description.

As the transaction text contains vital information, I also changed the gnc_ab_description_to_gnc such that it concatenates transaction text and purpose, separated by ";" to give a complete overview.

I'd be happy if this change would find it's way into one of the next maintenance releases. However being new to this project I'm pretty sure I did not comply with all the rules and concepts. So feel free to review and comment on these changes.

Attached you'll find some examples of the different SWIFT transactions created by my bank (of course with fake data :-) ). Just rename Examples.mt940.txt to Examples.mt940 and import it into GNUcash.

[jralls]

You're right that you didn't follow our coding guidelines for formatting. Please fix that up.

But if you want this to go into a maintenance release instead of waiting for the major release at the end of the year then your feature branch must be based on maint rather than master. Don't try to use git rebase to fix that. If you need instructions, just ask.

[uniederer]

@jralls Thanks for your comment. I guess you were referring to indentiation and brace positions. I tried to correct this. Did I miss something else?

As for the shift towards the maintenance branch I'd be happy for guidance how to approach this without using rebase. Shall I just create a feature branch from the maint branch and create a pull request for that one too?

Thanks in advance for your help.

[jralls]

Correct, indentation and brace position. There's one other item, a space between function names and the open-paren, e.g.

    g_free (foo);

but that's so raggedly applied that I wouldn't require you to fix it.

I don't think that there are any changes between maint and master in the files you've changed, so you should be able to do the following with no conflicts:

    git checkout maint
    git checkout -b new-swift-translationtxt
    git cherry-pick aa1e88a
    git cherry-pick 45f5380
    git cherry-pick 92cfb28
    git checkout maint
    git branch -D swift-translationtxt
    git branch -m new-swift-translationtxt swift-translationtxt
    git push -f <mygithubremote> swift-translationtxt

with the obvious substitution for . BTW, it might have worked to use

    git rebase --onto maint 92cfb28 0a1e88a

if you hadn't stuck that merge commit in the middle. As a general rule you should never merge into a PR branch: Instead, rebase the PR branch onto the base branch, i.e.

    git rebase master swift-translationtxt

to move the whole PR branch to the HEAD of the base branch. If you merge the base branch into the PR branch then it would create duplicate history when it's merged back into the base branch. Of course when switching base branches the main concern is to avoid pulling extraneous commits from one to the other.

[jralls]

Oops, didn't notice this before. One statement per line, please.

[uniederer]

Ah, yeah, sorry, that's how I do this usually as I consider this as "one transaction". Hence it escaped my attention. But of course you're right. I'll fix that.

[jralls]

I don't see where the purpose is added as a note; it looks to me like the purpose is added to the transaction text and inserted in the description. That might be better for most users as the note field is hidden by default.

[uniederer]

The addition as note is done in the changes from Line 554. I checked it using my example above and the text appears in the note field. However there is a piece of code using gnc_ab_description_to_gnc in gnc-ab-transfer.c. That code is used in gnc-plugin-aqbanking.c where I lost track of possible the entry points.

So I wanted to preserve that function and just join the transaction text in front of the purpose to have it available too. If I understood you correctly, you'd prefer to have the transaction text always stuck in front of the rest of the transaction? In that case I can simply revert my changes around line 554 and the split of the interface into transaction text and purpose I introduced.

The reason I decided to not just join the two strings was, that the description might get a bit "bulky" depending on what the bank put into transaction text and the purpose fields (mind you, that there are up to 12 or so lines concatenated worst case). But you're right. The downside is, that the extra text in purpose is hidden by default if placed in the note and not accessible during the import phase...

Although to my experience, transaction text (if available) is strong enough to decide what transaction we are talking about. I don't have a too strong opinion on that. So shall I change it as outlined in the second paragraph?

[jralls]

Those calls in gnc-plugin-aqbanking.c are menu callbacks for issuing a transaction via HBCI. It's not at all clear to me why you'd want to do that with a transaction in the process of being imported, if one does one would probably prefer that the text is the same as the transaction one creates in GnuCash.

It occurs to me that you're changing the default behavior for everyone else who uses this import and that might be thought rude. Perhaps it would be better to leave it so that the Description field gets the "purpose" content and the Notes field gets the "transaction text". You might discuss this on gnucash-user-de to see if there's some consensus about it.

BTW, every commit you add to the swift-transactiontxt branch before you change the base to maint is another commit you have to cherry-pick...

[uniederer]

for issuing a transaction via HBCI

Issuing wouldn't make sense as the direction of data flow is the other way round, but now that you've pointed out HBCI, it struck me, that this must be the use of HBCI for fetching transactions instead of importing them from a mt940 file. And you're right, the behaviour should be symmetrical there....

Putting the transactiontxt in the notes field would not have a nice enough effect as my bank puts there the information more valuable to assign the transaction during the import phase. However, before working further on this I'll launch a thread in gnucash-user-de for discussion. Thanks for your advice.

As for the cherry-picking: I'm aware of that. The first attempt I made led to some conflict there. However as the change is minimal I think I'll just re-introduce it by a short copy-paste session. :-)

[fellen]

The discussion continues in this thread: http://lists.gnucash.org/pipermail/gnucash-de/2017-March/009879.html

[uniederer]

Based on the discussion in pull request #139 and the discussion in the german user group I decided to revise my pull request as follows:

1. I based my change on maint, hoping to have it probably within the next maintenance release
2. The changed behaviour is now limited to gnc_ab_get_purpose, guaranteeing unified behaviour for every use of the transaction’s purpose.
3. In order to hand those feeling disturbed by the change a chance to revert without having to wait for a new release, I added a preference in the online banking section that allows to turn off the behaviour. As I personally consider the current behaviour of GNUcash as a bug, I decided to turn the change on by default.

I hope by this, I made the proposed change more acceptable. Thank you in advance for your feedback.

[cstim]

Sounds good for me, and especially with the option to switch to the old behaviour this should now be ok with all users who are concerned with this. In my opinion even the option wouldn't be needed because the change is only for a very small set of banks, but maybe the option is on the safe side.

[uniederer]

In my opinion even the option wouldn't be needed because the change is only for a very small set of banks, but maybe the option is on the safe side.

Being on the safe side (and a Little bit curiosity how hard it would be to introduce a new setting) was exactly my motivation to introduce the preference setting. I think depending on the reaction of the users, we probably can remove the setting in future releases anyway.

[jralls]

Very nice and much more elegant.

[jralls]

Merged after a rebase.

[jralls]

This change turns out to affect OFX imports in a way that's extremely annoying, as the description of the OFX type code (that nobody really cares about) is prepended to the transaction description.

To be consistent with the OFX importer (which uses libofx instead of libaqbanking) it should put the transaction text in the notes field, but since no-one has ever complained about the inconsistency ISTM simply disabling it for OFX would suffice.

[jralls]

Done.