Bug 797631 - Superfluous account selection dialog on first online transaction retrieval #651

pkzw · 2020-02-27T11:32:45Z

See Bug 797631 for more details.

jralls · 2020-02-27T16:05:34Z

This doesn't even compile, see the Travis CI logs.
Do not include merge commits in PRs.
Notice that the '#' character has a special meaning on Github and that #1 points entirely to the wrong place. As it happens that's how a normal person would read that commit message away from Github. The changes merged are for Bug 797540 - Mapping of aqbanking accounts to gnucash accounts doesn'… #649 and that's already merged into maint. Instead of doing your own merge you should pull maint after the merge and then rebase this feature branch on that.
The function gnc_ab_create_acct_online_id doesn't belong in gnc-ab-kvp.c, it has nothing to do with KVP.
When making a function public as you do with create_online_id you should add the prefix gnc; in this case since it's part of the AQBanking model use gnc_aq for the prefix, i.e. gnc_aq_create_online_id.
How does creating a test fix the bug?

pkzw · 2020-02-27T19:14:48Z

Yes, meanwhile I have seen this too and I can reproduce it if I build the check target. I does not occur on my side if I build the default (all) target. I think the reason is that function test_acct_online_id_match tries to call a function from the AqBanking plugin, but it's not clear to me why it fails only in building the check target.
This has happened by accident I will remove it.
Thanks for the advice.
Could you suggest a better place? The function needs to read slots/KVPs 'hbci/bank-code' and 'hbci/account-id' for the GnuCash account because they are needed for the (already existing) function create_online_id to derive the Online-ID for the account. Maybe also the function create_online_id should be put to another place, but for my understanding it still belongs to the AqBanking plugin.
This should be no problem to fix.
Sorry, but I haven't understood this question.

gjanssens · 2020-02-27T20:39:10Z

test_acct_online_id_match is a unit test function that is meant to ensure a part of the code continues to work as intended. unit tests are only run as part of the check target. It's in fact the whole point of this check target. The fact this test now fails means you have broken an assumption this test relies on. You'll now have to figure out whether that change is justified or not. If so, you also have to update the test to work with the new code. If not, you need to rethink your code.
Kvp is an implementation detail and should almost never be used directly. And your new function is indeed not interacting with kvp directly. Instead it uses higher level functions that do the kvp interaction for you. That's good, but also the reason why your new function does not belong next to the kvp code directly. Unfortunately I don't know the aqbanking module well enough to suggest a better place.

jralls · 2020-02-28T03:53:01Z

Were gnc_ab_create_online_id useful it would belong in gnc-ab-utils.cpp with create_online_id, meaning that the latter could remain a static function local to that file.

As to question 6 I misunderstood the last patch. On a second look I understand what you're doing and why the unit test is broken: You've taken a general purpose function used for all of the importers and made it AQBanking-only. That's not going to work.

pkzw · 2020-02-28T07:53:42Z

If test_acct_online_id_match is just a unit test function, why is it then called by the standard code? For getting transactions from my bank account I have observed the following call stack:

function	location
test_acct_online_id_match()	import-account-matcher.c:92
gnc_account_foreach_descendant_until()	Account.cpp:3,060
gnc_import_select_account()	import-account-matcher.c:289
gnc_ab_accinfo_to_gnc_acc()	gnc-ab-utils.c:672
txn_accountinfo_cb()	gnc-ab-utils.c:917
AB_ImExporterAccountInfo_List_ForEach()	imexporter_accountinfo.c:20
gnc_ab_import_context()	gnc-ab-utils.c:1,238
gnc_ab_gettrans()	gnc-ab-gettrans.c:242
...	...

I think that this issue is difficult to solve from the layering perspective.
What about fixing this issue completely in the AqBanking plugin layer. I could try to provide a patch which already does the assignment of the Online-ID to the GnuCash account within the account matching part of the Onlinebanking Setup tool. Would this be a better approach?

gjanssens · 2020-02-28T08:12:10Z

I was clearly talking outside of my area of knowledge. Please ignore that comment and sorry for adding confusion.

jralls · 2020-02-28T15:30:15Z

Yeah, I got misled by that function name too.

@pkzw let's work on understanding (or maybe just helping me to understand) the underlying problem a bit in the bug report before doing any more coding.

pkzw · 2020-02-28T16:57:16Z

@jralls that's a good idea. I will add a comment with my (current) understanding to the bug report.

pkzw · 2020-02-28T18:06:42Z

I have created a new version of the patch within a new local branch pkzw_bug797631a based on commit feee495 from gnucash/maint. What is the easiest way to replace the changes of the current PR by the changes of that branch?
Locally, I can now also build the check target without errors and the superfluous account selection dialog does no longer occur.

jralls · 2020-02-29T01:57:47Z

Just git reset --hard maint to nuke your old commit, commit the new one, and force-push the branch. Assuming origin points to your github repository, git push -f origin pkzw_bug797631.

jralls

Much better and cleaner than the predecessor.

jralls · 2020-03-14T18:29:13Z

gnucash/import-export/aqb/assistant-ab-initial.c

@@ -694,6 +696,10 @@ save_kvp_acc_cb(gpointer key, gpointer value, gpointer user_data)
    guint32 ab_account_uid;
    const gchar *ab_accountid, *gnc_accountid;
    const gchar *ab_bankcode, *gnc_bankcode;
+#ifdef AQBANKING6
+    gchar *ab_online_id;
+       const gchar *gnc_online_id;


Please fix the indentation.

gnucash/import-export/aqb/gnc-ab-utils.h

jralls · 2020-03-14T18:47:58Z

gnucash/import-export/aqb/assistant-ab-initial.c

@@ -682,7 +683,8 @@ static void
 clear_kvp_acc_cb(gpointer gnc_acc, gpointer ab_acc, gpointer user_data)
 {
    g_return_if_fail(gnc_acc);
-    /* Delete complete "hbci..." KVPs for GnuCash account */
+    /* Delete "online-id" and complete "hbci..." KVPs for GnuCash account */
+    gnc_import_delete_acc_online_id((Account *) gnc_acc);


I don't see the point of the indirection. Why not just call gnc_account_delete_map_entry(account, "online_id", NULL, NULL, FALSE); here?

Done. I have removed the whole commit.

The function is renamed to gnc_ab_create_online_id. It shall be callable from the Online Banking Setup tool in order to (re-)calculate the online id for changed account matches

…rieval The online id, which is needed to find a GnuCash account for a transaction or the balance retrieved from an online account, is already assigned to each matched GnuCash account within the Online Banking Setup tool. The online id is removed from the GnuCash account if it is no longer matched with an AqBanking account.

jralls requested changes Mar 14, 2020

View reviewed changes

pkzw added 2 commits March 15, 2020 18:23

Rename function create_online_id and make it externally callable

64874df

The function is renamed to gnc_ab_create_online_id. It shall be callable from the Online Banking Setup tool in order to (re-)calculate the online id for changed account matches

jralls approved these changes Mar 15, 2020

View reviewed changes

code-gnucash-org merged commit d825d74 into Gnucash:maint Mar 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 797631 - Superfluous account selection dialog on first online transaction retrieval #651

Bug 797631 - Superfluous account selection dialog on first online transaction retrieval #651

pkzw commented Feb 27, 2020

jralls commented Feb 27, 2020

pkzw commented Feb 27, 2020

gjanssens commented Feb 27, 2020

jralls commented Feb 28, 2020

pkzw commented Feb 28, 2020

gjanssens commented Feb 28, 2020

jralls commented Feb 28, 2020

pkzw commented Feb 28, 2020

pkzw commented Feb 28, 2020

jralls commented Feb 29, 2020

jralls left a comment

jralls Mar 14, 2020

pkzw Mar 15, 2020

jralls Mar 14, 2020

pkzw Mar 15, 2020

Bug 797631 - Superfluous account selection dialog on first online transaction retrieval #651

Bug 797631 - Superfluous account selection dialog on first online transaction retrieval #651

Conversation

pkzw commented Feb 27, 2020

jralls commented Feb 27, 2020

pkzw commented Feb 27, 2020

gjanssens commented Feb 27, 2020

jralls commented Feb 28, 2020

pkzw commented Feb 28, 2020

gjanssens commented Feb 28, 2020

jralls commented Feb 28, 2020

pkzw commented Feb 28, 2020

pkzw commented Feb 28, 2020

jralls commented Feb 29, 2020

jralls left a comment

Choose a reason for hiding this comment

jralls Mar 14, 2020

Choose a reason for hiding this comment

pkzw Mar 15, 2020

Choose a reason for hiding this comment

jralls Mar 14, 2020

Choose a reason for hiding this comment

pkzw Mar 15, 2020

Choose a reason for hiding this comment