Bug 11331 - CSV export for viewlog.pl is missing newlines
Summary: CSV export for viewlog.pl is missing newlines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P4 normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL: /cgi-bin/koha/tools/viewlog.pl
Keywords:
Depends on:
Blocks: 13522
  Show dependency treegraph
 
Reported: 2013-12-03 14:00 UTC by Sophie MEYNIEUX
Modified: 2015-12-03 22:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11331 : corrects csv export for viewlog.pl (add newlines, string separator and same column as screen output) (1.04 KB, patch)
2013-12-03 14:05 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 11331 - CSV export for viewlog.pl is missing newlines (5.84 KB, patch)
2014-07-29 07:20 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 11331 - CSV export for viewlog.pl is missing newlines - followup (15.13 KB, patch)
2014-07-29 07:20 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11331 - CSV export for viewlog.pl is missing newlines (6.12 KB, patch)
2014-08-01 16:32 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11331 - CSV export for viewlog.pl is missing newlines - followup (15.24 KB, patch)
2014-08-01 16:34 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 11331 - CSV export for viewlog.pl is missing newlines (6.25 KB, patch)
2014-08-02 20:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11331 - CSV export for viewlog.pl is missing newlines - followup (15.25 KB, patch)
2014-08-02 20:02 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sophie MEYNIEUX 2013-12-03 14:00:04 UTC
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
Comment 1 Sophie MEYNIEUX 2013-12-03 14:05:39 UTC Comment hidden (obsolete)
Comment 2 Christopher Brannon 2013-12-10 23:19:17 UTC
Can you please provide a test plan?

Christopher
Comment 3 David Cook 2014-01-07 00:59:57 UTC
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.
Comment 4 Sophie MEYNIEUX 2014-03-13 16:44:02 UTC
Is there a hope that logs could be exported one day ?
What could be done to make this happen ?
Comment 5 Fridolin SOMERS 2014-07-25 15:04:43 UTC
I'm on a patch developpement
Comment 6 Fridolin SOMERS 2014-07-29 07:20:05 UTC Comment hidden (obsolete)
Comment 7 Fridolin SOMERS 2014-07-29 07:20:21 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2014-08-01 16:32:02 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2014-08-01 16:34:21 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2014-08-02 20:02:40 UTC
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.
Comment 11 Katrin Fischer 2014-08-02 20:02:49 UTC
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>
Comment 12 Tomás Cohen Arazi 2014-08-05 23:27:54 UTC
Patches pushed to master.

Thanks Fridolin!
Comment 13 Jonathan Druart 2015-01-08 12:14:17 UTC
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 "];"
Comment 14 Mason James 2015-03-10 02:02:54 UTC
> 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
Comment 15 Mason James 2015-03-10 02:34:03 UTC
(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]};
Comment 16 Fridolin SOMERS 2015-03-24 08:36:30 UTC
Bug 13522 is pushed to master and stable so I close discussion on this bug and set it back to pushed to master
Comment 17 Mason James 2015-04-23 04:11:21 UTC
(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