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.
*** Bug 8690 has been marked as a duplicate of this bug. ***
This problem was introduced by http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5098
Created attachment 24321 [details] [review] Bug:8168 Fixing header of non csv files 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
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
Created attachment 24656 [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>
Since I fixed what I reported wrong, some one else will need to sign off on this. :)
Created attachment 24665 [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>
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.
Created attachment 24675 [details] [review] Bug 8168 - Use comma as default delimiter
installer/data/mysql/sysprefs.sql has a default value of semicolon.
Created attachment 24680 [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.
Created attachment 24681 [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.
Created attachment 25874 [details] [review] SIGNED-OFF Bug-8168-Fixing-header-of-non-csv-files
Created attachment 25875 [details] [review] SIGNED-OFF-Bug-8168-Use-semicolon-as-default-delimit Tested both patches. Observed behavior of what was written to message_queue after running overdue_notices.pl with some overdue checked out books. Default delimeter of semicolon is aligned with sysprefs value;
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>
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>
Pushed to master. This appears to be the first patch of yours accepted into Koha master -- congratulations, Daniel!
Pushed to 3.14.x, will be in 3.14.08 a ; nice ; correction ; thanks ;)
Pushed to 3.12.x, will be in 3.12.13