When you try to export the result of tools/viewlog.pl in csv, file cannot be correctly loaded : - Newline is missing after each record, - strings should be enclosed in "" - columns are not the same as for screen output This patch corrects this 3 points
Created attachment 23265 [details] [review] Bug 11331 : corrects csv export for viewlog.pl (add newlines, string separator and same column as screen output)
Can you please provide a test plan? Christopher
1) I agree that a newline should be added. 2) I'm not certain about using double quotes as a text qualifier. While this will work for many of the logs, it won't work for any log that contains double quotes within it (such as a cataloguing (biblio or item) modification, authority modification, system preference modification). I think most programs typically use single quotes, double quotes, or spaces as text qualifiers, and all three of these won't work for us, since we have all three of those within our text. We also have commas in our text...and could possibly have tabs...so those two delimiters aren't very useful either without using text qualifiers. Of course, I don't see any easy solution to this problem. I think bibliographic data is too difficult to express in a regular delimited text format, and we're showing entire records in many of the logs. Perhaps we could clean problematic characters (tabs, commas, quotes, etc) from the logs, but then we also compromise the integrity of those logs. Perhaps another idea would be to export using XML formats (ODS,XLSX) instead. (We do this elsewhere in Koha.) Perhaps we should leave the simpler formats for simpler data. 3) "Title" and "author" should be removed from the list. You could probably safely do this without leaving the comment behind. -- Anyway, those are my thoughts. Since this patch doesn't completely fix the log export, I'm tempted to mark it as Failed QA. However, while it isn't a perfect fix, it is better than what we're currently doing, so maybe it's good enough for now. I'm marking it "In Discussion" to see what others think.
Is there a hope that logs could be exported one day ? What could be done to make this happen ?
I'm on a patch developpement
Created attachment 30264 [details] [review] Bug 11331 - CSV export for viewlog.pl is missing newlines When you try to export the result of tools/viewlog.pl in csv, file cannot be correctly loaded : - newline is missing after each record, - strings should be enclosed in "" - columns are not the same as for screen output This patch corrects this by using like other export Text::CSV. Adds a header line made with the keys of first data. For that, all data values are initialiszed with empty string. Test plan : - Use a database with some logs, see sysprefs /cgi-bin/koha/admin/preferences.pl?tab=logs - Go to export page /cgi-bin/koha/tools/viewlog.pl - Select a module - Click on "To a file" and choose a file name - Click on "Submit" - Open file => Without this patch : newline is missing, multi-lines cells are not enclosed in "", there are no column headings => Without this patch : each line is a data line, complexe cells are enclosed in "", there are column headings - Test the export of all modules to see that all headings are necessary - Check the output to screen in the browser
Created attachment 30265 [details] [review] Bug 11331 - CSV export for viewlog.pl is missing newlines - followup Perl formatting and cleaning. Also corrects the HTML of restults table in viewlog.tt.
Created attachment 30466 [details] [review] [SIGNED-OFF] Bug 11331 - CSV export for viewlog.pl is missing newlines When you try to export the result of tools/viewlog.pl in csv, file cannot be correctly loaded : - newline is missing after each record, - strings should be enclosed in "" - columns are not the same as for screen output This patch corrects this by using like other export Text::CSV. Adds a header line made with the keys of first data. For that, all data values are initialiszed with empty string. Test plan : - Use a database with some logs, see sysprefs /cgi-bin/koha/admin/preferences.pl?tab=logs - Go to export page /cgi-bin/koha/tools/viewlog.pl - Select a module - Click on "To a file" and choose a file name - Click on "Submit" - Open file => Without this patch : newline is missing, multi-lines cells are not enclosed in "", there are no column headings => Without this patch : each line is a data line, complexe cells are enclosed in "", there are column headings - Test the export of all modules to see that all headings are necessary - Check the output to screen in the browser Signed-off-by: Owen Leonard <oleonard@myacpl.org> The CSV export is significantly improved. I question the usefulness of including biblioitemnumber in the output. A better inclusion would be itemnumber.
Created attachment 30467 [details] [review] [SIGNED-OFF] Bug 11331 - CSV export for viewlog.pl is missing newlines - followup Perl formatting and cleaning. Also corrects the HTML of restults table in viewlog.tt. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30493 [details] [review] [PASSED QA] Bug 11331 - CSV export for viewlog.pl is missing newlines When you try to export the result of tools/viewlog.pl in csv, file cannot be correctly loaded : - newline is missing after each record, - strings should be enclosed in "" - columns are not the same as for screen output This patch corrects this by using like other export Text::CSV. Adds a header line made with the keys of first data. For that, all data values are initialiszed with empty string. Test plan : - Use a database with some logs, see sysprefs /cgi-bin/koha/admin/preferences.pl?tab=logs - Go to export page /cgi-bin/koha/tools/viewlog.pl - Select a module - Click on "To a file" and choose a file name - Click on "Submit" - Open file => Without this patch : newline is missing, multi-lines cells are not enclosed in "", there are no column headings => Without this patch : each line is a data line, complexe cells are enclosed in "", there are column headings - Test the export of all modules to see that all headings are necessary - Check the output to screen in the browser Signed-off-by: Owen Leonard <oleonard@myacpl.org> The CSV export is significantly improved. I question the usefulness of including biblioitemnumber in the output. A better inclusion would be itemnumber. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> While this feature is still not perfect, this is a big improvement. Passes tests and QA script, restores basic functionality.
Created attachment 30494 [details] [review] [PASSED QA] Bug 11331 - CSV export for viewlog.pl is missing newlines - followup Perl formatting and cleaning. Also corrects the HTML of results table in viewlog.tt. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patches pushed to master. Thanks Fridolin!
Comment on attachment 30493 [details] [review] [PASSED QA] Bug 11331 - CSV export for viewlog.pl is missing newlines Review of attachment 30493 [details] [review]: ----------------------------------------------------------------- ::: tools/viewlog.pl @@ +201,5 @@ > + my $csv = Text::CSV::Encoded->new( { encoding_out => 'utf8', sep_char => $delimiter } ); > + $csv or die "Text::CSV::Encoded->new FAILED: " . Text::CSV::Encoded->error_diag(); > + # First line with heading > + # Exporting bd id seems useless > + my @headings = grep { $_ ne 'action_id' } sort keys $data[0]; Raises an error with perl <= 5.10 Type of arg 1 to keys must be hash (not array element) at /home/koha/src/tools/viewlog.pl line 214, near "];"
> Raises an error with perl <= 5.10 > > Type of arg 1 to keys must be hash (not array element) at > /home/koha/src/tools/viewlog.pl line 214, near "];" reopening bug to fix this new error
(In reply to Mason James from comment #14) > > Raises an error with perl <= 5.10 > > > > Type of arg 1 to keys must be hash (not array element) at > > /home/koha/src/tools/viewlog.pl line 214, near "];" > > > reopening bug to fix this new error fyi: i think the following mod might fix the above fatal - my @headings = grep { $_ ne 'action_id' } sort keys $data[0]; + my @headings = grep { $_ ne 'action_id' } sort keys %{$data[0]};
Bug 13522 is pushed to master and stable so I close discussion on this bug and set it back to pushed to master
(In reply to Tomás Cohen Arazi from comment #12) > Patches pushed to master. Pushed to 3.16.x, will be in 3.16.10