Description
Galen Charlton
2012-05-30 17:35:56 UTC
*** 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 |