Bug 8168 - ersatz CSV header in attachment of overdue notices sent to administrator
Summary: ersatz CSV header in attachment of overdue notices sent to administrator
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Daniel Barker
QA Contact:
URL:
Keywords:
: 8690 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-05-30 17:35 UTC by Galen Charlton
Modified: 2015-06-04 23:30 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug:8168 Fixing header of non csv files (1.47 KB, patch)
2014-01-14 22:37 UTC, Daniel Barker
Details | Diff | Splinter Review
Bug 8168 - Fixing header of non csv files (1.80 KB, patch)
2014-01-23 22:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8168 - Fixing header of non csv files (1.85 KB, patch)
2014-01-24 07:37 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8168 - Use comma as default delimiter (997 bytes, patch)
2014-01-24 11:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8168 - Use semicolon as default delimiter (1.09 KB, patch)
2014-01-24 12:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8168 - Use semicolon as default delimiter (1.56 KB, patch)
2014-01-24 12:41 UTC, Mark Tompsett
Details | Diff | Splinter Review
SIGNED-OFF Bug-8168-Fixing-header-of-non-csv-files (1.90 KB, patch)
2014-03-05 18:55 UTC, wajasu
Details | Diff | Splinter Review
SIGNED-OFF-Bug-8168-Use-semicolon-as-default-delimit (1.70 KB, patch)
2014-03-05 18:59 UTC, wajasu
Details | Diff | Splinter Review
Bug 8168 - Fixing header of non csv files (1.96 KB, patch)
2014-03-17 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8168 - Use semicolon as default delimiter (1.74 KB, patch)
2014-03-17 16:12 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2012-05-30 17:35:56 UTC
overdue_notices.pl sends an email to the Koha admin with any notices that should be printed as an attachment.  However, if the attachment contains printable notices, rather than CSV, the first line of the attachment still includes a CSV header:

title;name;surname;address1;address2;zipcode;city;email;itemcount;itemsinfo;due_date;issue_date

There are two problems with this:

[1] This header shouldn't be included unless CSV output was specifically requested (by use of the -csv) switch.
[2] The header does not respect the delimiter and tabulation system preferences -- it always uses a semicolon.
Comment 1 Owen Leonard 2012-08-26 19:42:19 UTC
*** Bug 8690 has been marked as a duplicate of this bug. ***
Comment 2 Sophie MEYNIEUX 2013-09-13 15:50:22 UTC
This problem was introduced by http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5098
Comment 3 Daniel Barker 2014-01-14 22:37:51 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2014-01-23 22:12:36 UTC
Comment on attachment 24321 [details] [review]
Bug:8168 Fixing header of non csv files

Review of attachment 24321 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/overdue_notices.pl
@@ +628,5 @@
>          }
>          # Generate the content of the csv with headers
> +		my $content;
> +		if ( defined $csvfilename ) {
> +			 $content = join(";", qw(title name surname address1 address2 zipcode city country email 				itemcount itemsinfo due_date issue_date)) . "\n";        	

You missed Galen's second comment. The ";" should refer to C4::Context->preference('delimiter') I believe. You also have tab issues.

@@ +634,5 @@
> +		else {
> +			 $content = "";
> +		}
> +		$content .= join( "\n", @output_chunks );
> +        

This should be a blank line, not something with a tab or spaces.

mtompset@ubuntu:~/kohaclone$ koha-qa.pl -v 2 -c 1
testing 1 commit(s) (applied to 1cc0d7b 'Bug 10907: (follow-up) fix table sort')

 FAIL   misc/cronjobs/overdue_notices.pl
   OK     pod
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 637)
                forbidden pattern: tab char (line 633)
                forbidden pattern: tab char (line 630)
                forbidden pattern: tab char (line 635)
                forbidden pattern: tab char (line 631)
                forbidden pattern: tab char (line 636)
                forbidden pattern: tab char (line 634)
                forbidden pattern: tab char (line 632)
   OK     valid
   OK     critic
Comment 5 Mark Tompsett 2014-01-23 22:28:07 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2014-01-23 22:28:59 UTC
Since I fixed what I reported wrong, some one else will need to sign off on this. :)
Comment 7 Chris Cormack 2014-01-24 07:37:25 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-01-24 11:48:46 UTC
There is already a call to the syspref delimiter in this script:
    my $sep_char = C4::Context->preference('delimiter') || ',';
vs yours:
    my $delimiter = C4::Context->preference('delimiter') // ';';

I think we should use the same default delimiter.

Marked as Failed QA.
Comment 9 Jonathan Druart 2014-01-24 11:50:47 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2014-01-24 12:36:42 UTC
installer/data/mysql/sysprefs.sql has a default value of semicolon.
Comment 11 Mark Tompsett 2014-01-24 12:39:37 UTC Comment hidden (obsolete)
Comment 12 Mark Tompsett 2014-01-24 12:41:09 UTC Comment hidden (obsolete)
Comment 13 wajasu 2014-03-05 18:55:52 UTC Comment hidden (obsolete)
Comment 14 wajasu 2014-03-05 18:59:46 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-03-17 16:11:50 UTC
Created attachment 26413 [details] [review]
Bug 8168 - Fixing header of non csv files

This determines if the CSV header should be included or not and
then generates it as needed using the delimiter specified in the
delimiter system preference.

TEST PLAN
---------
1. make some overdues books
2. run the overdue notices script without the -csv
3. check emails notice csv header is in the email
4. apply the patch
5. run the overdue notice again
6. check email notice csv header is absent

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: wajasu <matted-34813@mypacks.net>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 16 Jonathan Druart 2014-03-17 16:12:38 UTC
Created attachment 26414 [details] [review]
Bug 8168 - Use semicolon as default delimiter

installer/data/mysql/sysprefs.sql has semicolon as default.
This fixes both instances to use the same fallback value.

It also prevents csv header info from being included in non-CSV messages.

Signed-off-by: wajasu <matted-34813@mypacks.net>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 17 Galen Charlton 2014-03-18 12:17:27 UTC
Pushed to master.  This appears to be the first patch of yours accepted into Koha master -- congratulations, Daniel!
Comment 18 Fridolin Somers 2014-06-12 09:15:14 UTC
Pushed to 3.14.x, will be in 3.14.08

a ; nice ; correction ; thanks ;)
Comment 19 Kyle M Hall 2014-06-13 13:02:19 UTC
Pushed to 3.12.x, will be in 3.12.13