After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 754533 - Add the option to do a CSV export of a Register view
Add the option to do a CSV export of a Register view
Status: RESOLVED FIXED
Product: GnuCash
Classification: Other
Component: General
git-master
Other Linux
: Normal normal
: ---
Assigned To: gnucash-general-maint
gnucash-general-maint
Depends on: 754530
Blocks:
 
 
Reported: 2015-09-03 14:36 UTC by Bob
Modified: 2018-06-29 23:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Split PluginRegister .h file (10.77 KB, patch)
2015-09-03 14:47 UTC, Bob
none Details | Review
Add CSV Register export (35.45 KB, patch)
2015-09-03 14:49 UTC, Bob
none Details | Review
No Account Templates patch (3.37 KB, patch)
2015-09-03 14:53 UTC, Bob
committed Details | Review
Split PluginRegister2 .h file (11.50 KB, patch)
2015-09-03 14:55 UTC, Bob
none Details | Review
Add ability to do a CSV Export from a Register View (15.03 KB, patch)
2015-09-24 15:57 UTC, Bob
none Details | Review
Add ability to do a CSV Export from a Register View (19.36 KB, patch)
2015-10-18 10:03 UTC, Bob
committed Details | Review
Change finish page text for Search and General Journal Register (3.54 KB, patch)
2015-11-04 15:58 UTC, Bob
committed Details | Review

Description Bob 2015-09-03 14:36:46 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.
Comment 1 Bob 2015-09-03 14:47:39 UTC
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.
Comment 2 Bob 2015-09-03 14:49:22 UTC
Created attachment 310600 [details] [review]
Add CSV Register export

This patch adds the option to do a CSV export of the currently selected register.
Comment 3 Bob 2015-09-03 14:53:17 UTC
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.
Comment 4 Bob 2015-09-03 14:55:03 UTC
Created attachment 310602 [details] [review]
Split PluginRegister2 .h file

For completeness this patch makes the same PluginRegister changes to PluginRegister2.
Comment 5 Bob 2015-09-24 15:57:59 UTC
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.
Comment 6 Geert Janssens 2015-10-09 08:44:33 UTC
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!
Comment 7 Geert Janssens 2015-10-09 08:45:46 UTC
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 8 Geert Janssens 2015-10-09 10:19:41 UTC
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 ?
Comment 9 Geert Janssens 2015-10-09 11:07:48 UTC
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
Comment 10 Bob 2015-10-18 09:58:16 UTC
(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.
Comment 11 Bob 2015-10-18 10:03:24 UTC
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.
Comment 12 Bob 2015-10-18 10:06:03 UTC
(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.
Comment 13 John Ralls 2015-10-18 17:35:11 UTC
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 14 Geert Janssens 2015-11-02 11:37:49 UTC
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.
Comment 15 Bob 2015-11-04 15:58:36 UTC
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.
Comment 16 Geert Janssens 2015-11-04 19:52:24 UTC
Thanks for the fast response. I have pushed your patch to master.
That should do it for this bug report. Closing now.
Comment 17 John Ralls 2018-06-29 23:42:53 UTC
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.