GNOME Bugzilla – Bug 754533
Add the option to do a CSV export of a Register view
Last modified: 2018-06-29 23:42:53 UTC
With this patch set you can select an option to export the current register view in normal or simple form to a CSV file, if it is a search or general register the output is only in normal form. This bug relies on the patches of bug 754530 being implemented.
Created attachment 310599 [details] [review] Split PluginRegister .h file For me to use the PluginRegister properties / functions, I needed to split the .h file so the procedures I can not see from import-export are in a separate file. I am not sure if this is the proper way of doing this or file names but this allows the following patch to work as expected.
Created attachment 310600 [details] [review] Add CSV Register export This patch adds the option to do a CSV export of the currently selected register.
Created attachment 310601 [details] [review] No Account Templates patch Whilst working on this and when using the search or general register I was getting the following error messages... * 13:59:55 WARN <gnc.engine> Got a NULL guid_list but the QofGuidMatch is not MATCH_NULL (but instead 2). In other words, the list of GUID matches is empty but it must contain something non-empty. This seems to stem from the test file I was using has no scheduled transactions and as such has no template accounts. This patch adds a test for a zero length account list.
Created attachment 310602 [details] [review] Split PluginRegister2 .h file For completeness this patch makes the same PluginRegister changes to PluginRegister2.
Created attachment 312069 [details] [review] Add ability to do a CSV Export from a Register View This patch replaces previous one as it had some text added by mistake. I am still not sure if what I have done in splitting the .h files is OK but will wait for feedback.
Review of attachment 310599 [details] [review]: There are two ways to resolve the issue of hidden functions: 1. split them into a separate file which will be included as needed. This is what you did. 2. Make the symbols available to your module by adding the proper include paths in CPP_FLAGS and LDFLAGS (found in Makefile.am) I discussed this yesterday on irc, because I was not 100% sure which approach would be the more appropriate one. We agreed in the end that splitting the functions into a separate file in this case is not the best approach. Particularly one of the two functions you are splitting off is the constructor for a GncPluginPageRegister. That should be kept in the main interface file. Even if this means you need to add libgncmod-gnome's dependencies to the csv-export module. According to John Ralls that is a best practise anyway, because you are introducing a dependency on that module and some platforms will fail to link if you don't explicitly define the dependencies. And he thinks our tests will fail if the dependencies aren't added. So, please go for option 2 - add the necessary dependencies to your Makefile.am. Thanks!
Review of attachment 310602 [details] [review]: For some reason this patch didn't apply. It will probably be no longer necessary if you switch to adding the dependencies to the Makefile.
Comment on attachment 310601 [details] [review] No Account Templates patch Thanks for this patch. It looks technically correct. I can't however reproduce the warnings you are experiencing. Just to keep the information on this bug complete, which steps do you take exactly to reproduce this ?
Review of attachment 312069 [details] [review]: As a general remark to these and your other patches, please keep cosmetic changes (indentation, return value on separate line,...) separate from the real changes. Especially for larger patches this makes it needlessly difficult to follow what the real changes are. Your last patch looks ok in principle. You can add the required dependency changes to this patch. It would make sense to keep those together as they are part of the same logical change. I'll review again afterwards
(In reply to Geert Janssens from comment #8) > Comment on attachment 310601 [details] [review] [review] > No Account Templates patch > > Thanks for this patch. It looks technically correct. > > I can't however reproduce the warnings you are experiencing. > Just to keep the information on this bug complete, which > steps do you take exactly to reproduce this ? To observe this do the following... Start gnucash and create a new file taking any defaults. close gnucash and then start it up again which should open previous new file. open the general ledger or use the find option and you should see errors mentioned. This can be observed on the current release version.
Created attachment 313611 [details] [review] Add ability to do a CSV Export from a Register View This patch replaces the previous three patches. I have added three lines to the makefile.am and included the changes for the Register2 files.
(In reply to Geert Janssens from comment #9) > Review of attachment 312069 [details] [review] [review]: > > As a general remark to these and your other patches, please keep cosmetic > changes (indentation, return value on separate line,...) separate from the > real changes. > > Especially for larger patches this makes it needlessly difficult to follow > what the real changes are. > > Your last patch looks ok in principle. You can add the required dependency > changes to this patch. It would make sense to keep those together as they > are part of the same logical change. I'll review again afterwards Sorry for the clutter, I have a tendency to make those cosmetic changes so I understand what is going on and so it is clearer to me. I will try and curb that in the future.
No need to curb it, the cosmetic changes are in general good. Just commit them separately so that patches with material changes are easier to parse. You might consider Github pull requests instead of attaching patches to bug reports for complex changes, it's a bit less work.
Comment on attachment 313611 [details] [review] Add ability to do a CSV Export from a Register View I'm a bit behind on patch reviewing due to non-gnucash related stuff. Thanks for you patience... The revised patch is good as far as I'm concerned. I added a few minor tweaks in a couple of follow-up commits. Nothing serious. There's maybe one more thing to consider for this feature: the finish page displays the number of accounts exports. This works for general transaction exports and for singe account registers. For General Journal and search registers it shows 0, which may be confusing for the user. Can you come up with something more elegant ? Perhaps we should drop this account counting completely for register type exports. It's not adding much information in that context.
Created attachment 314834 [details] [review] Change finish page text for Search and General Journal Register This patch changes the finish page text to remove the number of accounts when exporting a Search or General Journal Register. It also fixes an error with the add_account_name function when the account is NULL.
Thanks for the fast response. I have pushed your patch to master. That should do it for this bug report. Closing now.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=754533. Please update any external references or bookmarks.