GNOME Bugzilla – Bug 739571
Matching imported transactions doesn't indicate previously matched entries
Last modified: 2018-06-29 23:35:51 UTC
Created attachment 289901 [details] SMET window showing cleared and not cleared transactions This isn't strictly OFX Import related, but it was the closest category I could find to file under. The problem is in the "Generic Import Transaction Matcher" (hereafter GITM for short). When importing transactions (I am importing from OFX/QFX files downloaded from my bank), and attempting to match these transactions with transactions already in the register, I have no way to identify which transactions are "reconciled", "cleared" or "not cleared". Example: I make three transactions at Walmart, two on 10/16, one on 10/17, all in the amount of $1.61. When I get home each day, I empty the receipts from my pockets and enter those receipts into my transaction register. On the weekend, I download transactions from my bank. Included in the downloaded transactions are the three referenced above. When I import the QFX file, the transactions are shown in the GITM, with a default action chosen of "U+R". Now, before I allow GITM to match these transactions, I want to verify the matches for the transactions. So I double-click on one of the transactions. It opens a "Select Matching Existing Transaction" window (hereafter "SMET") with the three transactions that possibly match listed, and one transaction selected. So I accept that match (returning to the GITM window) and change the action from "U+R" to "R" (as I don't want the date or the payee modified). At this point, I remove the default action from the other two transactions referenced above, and choose "OK" in the GITM window. Visiting the register, I can see that the transaction now shows as a "cleared" transaction (and the other two are still listed as "not cleared"). So, I import the QFX file again (to match additional transactions). All the transactions that have not been matched again appear in the GITM window. At this point I choose the second Walmart transaction made on 10/16, and double-click it to verify the matching transaction from the register. In the SMET window, it still lists both transactions made on 10/16, with no way to differentiate them.... This is a problem, as there is no way to be sure that the second transaction from 10/16 in the QFX file is not being matched to the transaction already "cleared" in the previous import. I have attached an image of the SMET window showing the difficulty. Possible solutions: 1) Exclude "cleared" transactions (perhaps by offering an "option", such as a checkbox, to do this) from the SMET window. This is not optimal, because there may be situations where a single register transaction may need to be matched against multiple import transactions. 2) Indicate (either by foreground or background color, or by including a new column with a "cleared" indicator) in the SMET window that a transaction has already been cleared by matching during a previous import process. (Potentially this could be expanded to indicate that the transaction has already been chosen for matching during the current import process as well.) This seems to me like the better solution, as this indicates that transactions presented are already "cleared", so the choice between transactions becomes much easier, in both the case where individual import transactions match individual register transactions, and in the case where multiple import transactions match individual register transactions. I can't think of a reasonable third "possible solution" at this time. I have not looked at the code behind the GITM and SMET windows, so I don't know how technically difficult this change might be - but assuming that these windows are simply accessing the register transactions when they are being displayed, it seems like it would be fairly simple to do - since the indicator for "reconciled", "cleared", and "not cleared" is already in the transaction record - this would be simply a case of displaying that information visually to the user....
There was some discussion on the gnucash-devel list about how best to identify which transactions to omit or otherwise identify from the SMET. The suggestion was that since the system uses the FITID internally to track these associations that it would be better to use this than the reconcile state of the potential transactions. I think that while this is the primary association made internally it is not a user-facing concept (or at least not a primary one) in the application and so I feel that Tracy's suggestion of displaying the reconcile state of items is a more familiar solution. Regarding the suggestion to provide indication that a potential match is already a match for other entries in the same import, is it enough to know that such an association exists, or would some kind of identifier be necessary to take action on it? Would you want to know if the match was manually selected or the default based on the heuristic? I feel like this may be the more valuable part of this feature, but also has the possibility to balloon the amount of information displayed in the window for this (edge?) case.
Speaking strictly for myself, I think perhaps the easiest thing to do would be to add a column to the SMET window which conveys the information that the transaction has been matched. To use the register window as a comparison, the column there that displays the reconciled state of a transaction displays "r" for reconciled, "c" for cleared, and "n" for not cleared or reconciled (there are possibly other values I haven't encountered). Displaying that data in the SMET window, along with, perhaps, an "s" for "selected as match in this import", would satisfy me in all cases. I don't know if it's necessary (or desirable) to provide additional detail (such as showing the transactions that it is currently selected to match), as that would decidedly increase the complexity of the "fix"....
Created attachment 321098 [details] [review] Display reconcile state in match picker and allow users to show/hide reconciled matches
Created attachment 321099 [details] [review] Display pending matches for current import in match picker
Review of attachment 321098 [details] [review]: Thank you for your patch and my apologies I didn't get to reviewing it earlier. The general idea is good and implementation looks ok. On the other hand I have one conceptual question: how do you see the new "show reconciled" option being used ? Is it expected the user only sets this once as a general preference and then forgets about it or rather something to use dynamically (toggle on/off depending on taste for each individual import or even multiple times in one import) ? The former use case would suggest either a book level option or a global user option. The later use case is rather a book option or even a volatile one with no saved state between import runs. I can't really estimate this as I hardly use the importer. So I'm asking both (original reporter/Tracy and patch submitter/Jesse) your feedback on this. FTIW, the current implementation is as a global user option, which is normally only done for options that rarely change and are even valid acros different books.
Review of attachment 321099 [details] [review]: I'm quite pleased you even added a unit test to this :) Unfortunately the build fails with this error: Making all in test make[4]: Entering directory '/kobaltnet/janssege/Development/builds/gnucash/master/src/import-export/test' gcc -DHAVE_CONFIG_H -I. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export/test -I../../.. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/test-core -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/app-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/test-core -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -pthread -I/usr/include/guile/2.0 -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/test-core -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/app-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/test-core -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -pthread -I/usr/include/guile/2.0 -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -g -std=gnu99 -g -MT test_import_pending_matches-test-import-pending-matches.o -MD -MP -MF .deps/test_import_pending_matches-test-import-pending-matches.Tpo -c -o test_import_pending_matches-test-import-pending-matches.o `test -f 'test-import-pending-matches.c' || echo '/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export/test/'`test-import-pending-matches.c gcc -DHAVE_CONFIG_H -I. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export/test -I../../.. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/test-core -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/app-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/test-core -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -pthread -I/usr/include/guile/2.0 -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -MT test-link.o -MD -MP -MF .deps/test-link.Tpo -c -o test-link.o /kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export/test/test-link.c gcc -DHAVE_CONFIG_H -I. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export/test -I../../.. -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/test-core -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/app-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/test-core -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -pthread -I/usr/include/guile/2.0 -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -MT test-import-parse.o -MD -MP -MF .deps/test-import-parse.Tpo -c -o test-import-parse.o /kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export/test/test-import-parse.c mv -f .deps/test-link.Tpo .deps/test-link.Po /bin/sh ../../../libtool --tag=CC --mode=link gcc -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -g -o test-link test-link.o ../../../src/libqof/qof/libgnc-qof.la ../../../src/core-utils/libgnc-core-utils.la ../../../src/gnc-module/libgnc-module.la ../../../src/test-core/libtest-core.la ../libgncmod-generic-import.la ../../../src/app-utils/libgncmod-app-utils.la ../../../src/gnome-utils/libgncmod-gnome-utils.la ../../../src/engine/libgncmod-engine.la -lgio-2.0 -lgthread-2.0 -pthread -lgobject-2.0 -Wl,--export-dynamic -lgmodule-2.0 -pthread -lglib-2.0 -lguile-2.0 -lgc -lm mv -f .deps/test-import-parse.Tpo .deps/test-import-parse.Po /bin/sh ../../../libtool --tag=CC --mode=link gcc -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -g -o test-import-parse test-import-parse.o ../../../src/libqof/qof/libgnc-qof.la ../../../src/core-utils/libgnc-core-utils.la ../../../src/gnc-module/libgnc-module.la ../../../src/test-core/libtest-core.la ../libgncmod-generic-import.la ../../../src/app-utils/libgncmod-app-utils.la ../../../src/gnome-utils/libgncmod-gnome-utils.la ../../../src/engine/libgncmod-engine.la -lgio-2.0 -lgthread-2.0 -pthread -lgobject-2.0 -Wl,--export-dynamic -lgmodule-2.0 -pthread -lglib-2.0 -lguile-2.0 -lgc -lm mv -f .deps/test_import_pending_matches-test-import-pending-matches.Tpo .deps/test_import_pending_matches-test-import-pending-matches.Po /bin/sh ../../../libtool --tag=CC --mode=link gcc -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/test-core -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/app-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/test-core -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -pthread -I/usr/include/guile/2.0 -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -g -std=gnu99 -g -g -o test-import-pending-matches test_import_pending_matches-test-import-pending-matches.o ../../../src/libqof/qof/libgnc-qof.la ../../../src/engine/libgncmod-engine.la ../libgncmod-generic-import.la ../../../src/test-core/libtest-core.la ../../../src/engine/test-core/libgncmod-test-engine.la -lgio-2.0 -lgthread-2.0 -pthread -lgobject-2.0 -Wl,--export-dynamic -lgmodule-2.0 -pthread -lglib-2.0 -lm libtool: link: gcc -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -g -o .libs/test-link test-link.o -pthread -Wl,--export-dynamic -pthread ../../../src/libqof/qof/.libs/libgnc-qof.so ../../../src/core-utils/.libs/libgnc-core-utils.so ../../../src/gnc-module/.libs/libgnc-module.so ../../../src/test-core/.libs/libtest-core.so ../.libs/libgncmod-generic-import.so /kobaltnet/janssege/Development/builds/gnucash/master/src/gnome-utils/.libs/libgncmod-gnome-utils.so ../../../src/app-utils/.libs/libgncmod-app-utils.so ../../../src/gnome-utils/.libs/libgncmod-gnome-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/backend/xml/.libs/libgnc-backend-xml-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/app-utils/.libs/libgncmod-app-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/engine/.libs/libgncmod-engine.so -lxslt -lz -ldl -lgtk-x11-2.0 -lgdk-x11-2.0 -lpangocairo-1.0 -latk-1.0 -lcairo -lgdk_pixbuf-2.0 -lpangoft2-1.0 -lpango-1.0 -lfontconfig -lfreetype -lgnome-keyring -lsecret-1 -lxml2 -lX11 ../../../src/engine/.libs/libgncmod-engine.so /kobaltnet/janssege/Development/builds/gnucash/master/src/gnc-module/.libs/libgnc-module.so /kobaltnet/janssege/Development/builds/gnucash/master/src/core-utils/.libs/libgnc-core-utils.so -lpthread /kobaltnet/janssege/Development/builds/gnucash/master/src/libqof/qof/.libs/libgnc-qof.so -lgio-2.0 -lgthread-2.0 -lgobject-2.0 -lgmodule-2.0 -lglib-2.0 -lguile-2.0 -lgc -lm -pthread -Wl,-rpath -Wl,/kobaltnet/janssege/Development/installs/gnucash/master/lib -Wl,-rpath -Wl,/kobaltnet/janssege/Development/installs/gnucash/master/lib/gnucash libtool: link: gcc -Werror -Wdeclaration-after-statement -Wno-pointer-sign -Wall -Wunused -Wmissing-prototypes -Wmissing-declarations -Wno-unused -g -std=gnu99 -g -g -o .libs/test-import-parse test-import-parse.o -pthread -Wl,--export-dynamic -pthread ../../../src/libqof/qof/.libs/libgnc-qof.so ../../../src/core-utils/.libs/libgnc-core-utils.so ../../../src/gnc-module/.libs/libgnc-module.so ../../../src/test-core/.libs/libtest-core.so ../.libs/libgncmod-generic-import.so /kobaltnet/janssege/Development/builds/gnucash/master/src/gnome-utils/.libs/libgncmod-gnome-utils.so ../../../src/app-utils/.libs/libgncmod-app-utils.so ../../../src/gnome-utils/.libs/libgncmod-gnome-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/backend/xml/.libs/libgnc-backend-xml-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/app-utils/.libs/libgncmod-app-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/engine/.libs/libgncmod-engine.so -lxslt -lz -ldl -lgtk-x11-2.0 -lgdk-x11-2.0 -lpangocairo-1.0 -latk-1.0 -lcairo -lgdk_pixbuf-2.0 -lpangoft2-1.0 -lpango-1.0 -lfontconfig -lfreetype -lgnome-keyring -lsecret-1 -lxml2 -lX11 ../../../src/engine/.libs/libgncmod-engine.so /kobaltnet/janssege/Development/builds/gnucash/master/src/gnc-module/.libs/libgnc-module.so /kobaltnet/janssege/Development/builds/gnucash/master/src/core-utils/.libs/libgnc-core-utils.so -lpthread /kobaltnet/janssege/Development/builds/gnucash/master/src/libqof/qof/.libs/libgnc-qof.so -lgio-2.0 -lgthread-2.0 -lgobject-2.0 -lgmodule-2.0 -lglib-2.0 -lguile-2.0 -lgc -lm -pthread -Wl,-rpath -Wl,/kobaltnet/janssege/Development/installs/gnucash/master/lib -Wl,-rpath -Wl,/kobaltnet/janssege/Development/installs/gnucash/master/lib/gnucash libtool: link: gcc -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/gnc-module -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/test-core -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/app-utils -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/import-export -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/libqof/qof -I/kobaltnet/janssege/Development/gnucash/gnucash-master/src/engine/test-core -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -pthread -I/usr/include/guile/2.0 -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -g -std=gnu99 -g -g -o .libs/test-import-pending-matches test_import_pending_matches-test-import-pending-matches.o -pthread -Wl,--export-dynamic -pthread ../../../src/libqof/qof/.libs/libgnc-qof.so ../../../src/engine/.libs/libgncmod-engine.so ../.libs/libgncmod-generic-import.so /kobaltnet/janssege/Development/builds/gnucash/master/src/gnome-utils/.libs/libgncmod-gnome-utils.so /kobaltnet/janssege/Development/builds/gnucash/master/src/backend/xml/.libs/libgnc-backend-xml-utils.so -lgnome-keyring -lsecret-1 -lX11 /kobaltnet/janssege/Development/builds/gnucash/master/src/app-utils/.libs/libgncmod-app-utils.so -lgtk-x11-2.0 -lgdk-x11-2.0 -lpangocairo-1.0 -latk-1.0 -lcairo -lgdk_pixbuf-2.0 -lpangoft2-1.0 -lpango-1.0 -lfontconfig -lfreetype -lxslt -lz -ldl -lxml2 ../../../src/test-core/.libs/libtest-core.so ../../../src/engine/test-core/.libs/libgncmod-test-engine.a /kobaltnet/janssege/Development/builds/gnucash/master/src/engine/.libs/libgncmod-engine.so /kobaltnet/janssege/Development/builds/gnucash/master/src/gnc-module/.libs/libgnc-module.so /kobaltnet/janssege/Development/builds/gnucash/master/src/core-utils/.libs/libgnc-core-utils.so -lpthread -lguile-2.0 -lgc /kobaltnet/janssege/Development/builds/gnucash/master/src/libqof/qof/.libs/libgnc-qof.so -lgio-2.0 -lgthread-2.0 -lgobject-2.0 -lgmodule-2.0 -lglib-2.0 -lm -pthread -Wl,-rpath -Wl,/kobaltnet/janssege/Development/installs/gnucash/master/lib -Wl,-rpath -Wl,/kobaltnet/janssege/Development/installs/gnucash/master/lib/gnucash /usr/bin/ld: ../../../src/engine/test-core/.libs/libgncmod-test-engine.a(test-engine-stuff.o): undefined reference to symbol '__gxx_personality_v0@@CXXABI_1.3' /usr/lib64/libstdc++.so.6: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status Makefile:837: recipe for target 'test-import-pending-matches' failed make[4]: *** [test-import-pending-matches] Error 1 make[4]: *** Waiting for unfinished jobs.... make[4]: Leaving directory '/kobaltnet/janssege/Development/builds/gnucash/master/src/import-export/test' Makefile:775: recipe for target 'all-recursive' failed make[3]: *** [all-recursive] Error 1 make[3]: Leaving directory '/kobaltnet/janssege/Development/builds/gnucash/master/src/import-export' Makefile:581: recipe for target 'all-recursive' failed make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory '/kobaltnet/janssege/Development/builds/gnucash/master/src' Makefile:814: recipe for target 'all-recursive' failed make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory '/kobaltnet/janssege/Development/builds/gnucash/master' Makefile:668: recipe for target 'all' failed make: *** [all] Error 2 Please look into this. Thank you.
http://stackoverflow.com/questions/19901934/strange-linking-error-dso-missing-from-command-line
(In reply to Geert Janssens from comment #5) > The general idea is good and implementation looks ok. On the other hand I > have one conceptual question: how do you see the new "show reconciled" > option being used ? Is it expected the user only sets this once as a general > preference and then forgets about it or rather something to use dynamically > (toggle on/off depending on taste for each individual import or even > multiple times in one import) ? My own thought is that it is something that would probably be a "set and forget" thing. Someone either wants to see this kind of information, or they don't. However, I can see where an argument could be made for having it available to toggle on and off during a session, although multiple times per session seems a bit overkill to me. Personally, I would definitely like the option to be stateful - I don't want to have to set it every time I do an import.
Regarding how often I think this would typically be changed (showing or hiding reconciled transaction matches): Probably not more than once a session, however I do see use for it to be set both ways in the same book depending on what you're doing and/or what quirks the transactions you're importing have. e.g. generally, I'd want to hide reconciled transactions during an import, however if I have an updated/corrected import file, or if I have a number of similar transactions (some cleared from a previous import and some not) I may want to enable display of the extra transactions to help me identify the best matches or potentially correct any previous mistakes reconciling. Perhaps with this use case and Tracy's it would be best to always hide by default and not persist any preference. I went with showing by default so as to not change behavior from previous versions, and persisting to allow those who desire it to change the behavior manually once they understand it. Regarding the compile error for the patches, it looks like you're applying this to master? I wrote this against maint after discussion with John Ralls, and the issue does not repro for me there. In fact, I can't even apply the patch to master without changes at this point. I'd appreciate guidance on the appropriate process here if I've done something wrong (or perhaps next time I just need to be clear about what branch my submissions are for).
Ok, regarding change frequency, I think we can conclude everything is possible. Some users will never change after first time it's set, others will depending on which transactions they intend to import. So persistence is ok. And though my gut feeling tells me a book level option would be better in this particular case, the difference in this use case is subtle. So for now I'm fine with making it a global user option. That means if a user has multiple books, the last saved state will be seen in all books. (In reply to Jesse Olmer from comment #9) > I went with showing by default so as > to not change behavior from previous versions, and persisting to allow those > who desire it to change the behavior manually once they understand it. > That's the correct decision. Keep it that way. > > Regarding the compile error for the patches, it looks like you're applying > this to master? I wrote this against maint after discussion with John Ralls, > and the issue does not repro for me there. In fact, I can't even apply the > patch to master without changes at this point. I'd appreciate guidance on > the appropriate process here if I've done something wrong (or perhaps next > time I just need to be clear about what branch my submissions are for). I was indeed applying to master as already mentioned on irc. I have now applied to maint. The patches build fine and make check works as well. Unfortunately the program crashes on the first attempt to import some csv file I had lying around with this error: sys:1: Warning: invalid unclassed pointer in cast to 'GObject' sys:1: Warning: g_object_notify: assertion 'G_IS_OBJECT (object)' failed ** gnc.i-e:ERROR:/kobaltnet/janssege/Development/gnucash/gnucash/src/import-export/import-pending-matches.c:100:gnc_import_PendingMatches_add_match: assertion failed: (map) Aborted (core dumped) It looks one of your asserts gets triggered. Note that gnucash rarely uses asserts to verify function parameter validity. Instead we use g_return_if_fail and g_return_val_if_fail. These will print warnings to the log but allow the program to continue (more or less). They can be made fatal by setting G_DEBUG to fatal-warnings. However either way it appears your code makes an assumption that's not satisfied in my environment. Can you investigate further ? Thank you.
Created attachment 324076 [details] Sample csv file to import For your convenience I have added the csv test file I was using. Note it's in Dutch locale, which means the date format is "d-m-y" and number format is € 1.000,00. To import I selected the following columns: date, num, description, account and deposit The column with the full other account name is not used in maint. (It can be selected in master as well). The crash happened when clicking forward on the intermediate information screen, the one after account selection.
Apologies, I missed your response on IRC (we had a power outage). Thanks for the sample file - my bank didn't have any options other than QFX. The assert you hit was because I didn't realize there were two "constructors" for gnc_trans_list. I put the pending match initialization in gnc_gen_trans_list_new, which OFX/QFX import uses but CSV import does not. Before I submit an updated patch is there a set of sample files I could leverage for testing? Specifically, I'd love to validate the QIF code path as well if possible. Regarding the use of asserts, I'm coming from a C++ & C# background and so the pattern I've typically used there is that the "base" libraries (e.g. the pending match system in this case) use asserts or throw exceptions, and the callers either guard or handle the exceptions (which also appears to be the pattern in parts of the import-export code). Given the ongoing work to port to C++ would you rather I conform to the current g_return_if_fail pattern or attempt to fix the call guards?
(In reply to Jesse Olmer from comment #12) > Apologies, I missed your response on IRC (we had a power outage). Thanks for > the sample file - my bank didn't have any options other than QFX. > No problem. > The assert you hit was because I didn't realize there were two > "constructors" for gnc_trans_list. I put the pending match initialization in > gnc_gen_trans_list_new, which OFX/QFX import uses but CSV import does not. > Before I submit an updated patch is there a set of sample files I could > leverage for testing? Specifically, I'd love to validate the QIF code path > as well if possible. > You can find several samples in the source tree under doc/examples Will these be sufficient ? > Regarding the use of asserts, I'm coming from a C++ & C# background and so > the > pattern I've typically used there is that the "base" libraries (e.g. the > pending match system in this case) use asserts or throw exceptions, and the > callers either guard or handle the exceptions (which also appears to be the > pattern in parts of the import-export code). Given the ongoing work to port > to > C++ would you rather I conform to the current g_return_if_fail pattern or > attempt to fix the call guards? I realize g_return_if_fail is highly glib dependent and we want to move away from it. I have no strong affection with them. I do care that the program won't just suddenly exit due to an unhandled assert. Not all of the asserts you have set result in fatal situations IMO. For example in your code to free a data structure you assert whether this data structure is effectively allocated. If it isn't, the code could likely happily continue after emitting a warning only. So if you have other ways to fix that, I'm fine with this as well.
> Regarding the use of asserts, I'm coming from a C++ & C# background and so the > pattern I've typically used there is that the "base" libraries (e.g. the > pending match system in this case) use asserts or throw exceptions, and the > callers either guard or handle the exceptions (which also appears to be the > pattern in parts of the import-export code). Given the ongoing work to port to > C++ would you rather I conform to the current g_return_if_fail pattern or > attempt to fix the call guards? For C code, please continue to use g_return_if_fail. We can have a discussion about exceptions on the dev list if you decide you'd like to help with the C++ conversion. > I do care that the program won't just suddenly exit due to an unhandled > assert. Not all of the asserts you have set result in fatal situations IMO. > For example in your code to free a data structure you assert whether this > data structure is effectively allocated. If it isn't, the code could likely > happily continue after emitting a warning only. There's no such thing as a "handled assert". The assert macro either calls exit(3) or does nothing depending on whether NDEBUG (G_DISABLE_ASSERT in the case of g_assert) is defined. It's expected that one disables asserts when making a release build; they're intended for debugging during development, not for surprising users. Since GnuCash hasn't historically used assert much we don't have disabling them set up in our build system so they should be used sparingly and only when they'll signal an error in a more useful place than the crash that they're short-stopping. Turning a non-crash into a crash is bad, so they shouldn't be used for non-fatal errors.
This is more along the lines of a thread bump, intending to see where this falls in the implementation/incorporation journey. Has this patch gotten far enough to be considered for inclusion in a release version? I haven't seen any status updates since March, so I'm not sure if all code issues have been addressed - but as the original submission author, I can assure you that I am anxiously awaiting a release build which incorporates this patch. Along those lines, if there is a maintenance / test build which is "reasonably" stable (ie. could potentially be used for day to day operations without significant risk for data loss) which incorporates the patch, I would be willing to install it and "beat on it" a bit. (I am not "gnu" and "glib" savvy enough to be able to attempt a custom build for testing.)
Incorporating the feedback/best practices has fallen down my todo list a bit as a number of "real life" things have gotten in the way recently. I should have time to button up the last few pieces in the next couple of weeks, though.
Just checking in to see how things are progressing. Wasn't sure whether the lack of activity here means things are still pending, or if there's activity somewhere that I don't see here on the bug report. Anyone? Anyone? Bueller? Bueller? O:-)
Time flies and I lost track of this bug. I have fixed the remaining issues where Jesse left off and pushed the result to maint. This will appear in the next release due this weekend. Thank you for your patches and I hope you find time again in the future to contribute.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=739571. Please update any external references or bookmarks.