The gather_print_notices.pl cronjob for printing hold notices mails all notices as one file and does not separate out the notices by branch. For libraries with multiple branches this results in large files being received by individual branches that need to be read/sorted/printed or ignored.
Created attachment 9489 [details] [review] Bug 8063 - Hold print notices do not sort by branch Adds the option -s/--split to enable notices to be separated into different files by borrower home library.
Created attachment 9514 [details] [review] [SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch Works as expected. Signed off.
QA comments: * a lot of code cleaning that make the visibility of the real changes hard to spot ! * perlcritic is failing. Before the patch there is one error, after the patch there are 2: Bareword file handle opened at line 82, column 9. See pages 202,204 of PBP. (Severity: 5) Bareword file handle opened at line 107, column 5. See pages 202,204 of PBP. (Severity: 5) * tiny SQL comment: + LEFT JOIN borrowers b on b.borrowernumber = mq.borrowernumber should be written with UC for on: + LEFT JOIN borrowers b ON b.borrowernumber = mq.borrowernumber (could also be written + LEFT JOIN borrowers b USING(borrowernumber) ) Please fix both (they are the same error) and resubmit. If you submit a follow-up, directly switch to signed-off
Created attachment 9696 [details] [review] Bug 8063 - Hold print notices do not sort by branch Adds the option -s/--split to enable notices to be separated into different files by borrower home library.
Created attachment 9697 [details] [review] Bug 8063 - Followup - Make C4/Letters.pm conform to PBP Perlcritic reported the following errors: Subroutine prototypes used at line 96, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 120, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 152, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 173, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 190, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 227, column 1. See page 194 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 228, column 31. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 229, column 31. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 236, column 9. See page 199 of PBP. (Severity: 5) Subroutine prototypes used at line 668, column 1. See page 194 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 669, column 27. See page 199 of PBP. (Severity: 5) Subroutine prototypes used at line 719, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 865, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 899, column 1. See page 194 of PBP. (Severity: 5) Subroutine prototypes used at line 981, column 1. See page 194 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 982, column 28. See page 199 of PBP. (Severity: 5) Subroutine prototypes used at line 1000, column 1. See page 194 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 1001, column 27. See page 199 of PBP. (Severity: 5) "return" statement with explicit "undef" at line 1004, column 9. See page 199 of PBP. (Severity: 5)
I do apologize for the code cleanup blocking the visibility of changes. I figured this was a good file to tidy as it is rarely modified so the likelihood of the cleaning breaking another patch is extremely low. If I continue this practice, I will submit a the cleanup as a separate followup patch. I had not know that we should be updating code to conform to PBP, but it's a very good idea. In that spirit, I have submitted a followup to conform C4/Letters.pm to PBP.
Created attachment 9912 [details] [review] [SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch Still works !
Created attachment 9913 [details] [review] [SIGNED-OFF] Bug 8063 - Followup - Make C4/Letters.pm conform to PBP I don't understand why making things explicit is a high severity violation of best practices... but the patch is ok and perlcritic doesn't complain anymore.
First patch: Moves some code around, as well as adding a new loop and hash for splitting by branchcode. Marking Passed QA. Second patch: Just PBP cleanup, safe and sane, passed QA.
mmm... Kyle/Julian, When trying to run this script I gat a nasty: 15:42 ~/koha.dev/koha-community (new/bug_8063 $%)$ ./misc/cronjobs/gather_print_notices.pl Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 81. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 95. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 102. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 106. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 120. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 127. Execution of ./misc/cronjobs/gather_print_notices.pl aborted due to compilation errors. WTF ?
changing severity, it's an ENH, as well as assignee
Created attachment 9953 [details] [review] Bug 8063 - Followup - Bug fix $OUTPUT being used but not being declared. When trying to run this script I gat a nasty: 15:42 ~/koha.dev/koha-community (new/bug_8063 $%)$ ./misc/cronjobs/gather_print_notices.pl Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 81. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 95. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 102. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 106. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 120. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 127. Execution of ./misc/cronjobs/gather_print_notices.pl aborted due to compilation errors.
Created attachment 10786 [details] [review] Bug 8063 - [SIGNED-OFF] Followup - Bug fix $OUTPUT being used but not being declared. When trying to run this script I gat a nasty: 15:42 ~/koha.dev/koha-community (new/bug_8063 $%)$ ./misc/cronjobs/gather_print_notices.pl Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 81. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 95. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 102. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 106. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 120. Global symbol "$OUTPUT" requires explicit package name at ./misc/cronjobs/gather_print_notices.pl line 127. Execution of ./misc/cronjobs/gather_print_notices.pl aborted due to compilation errors. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Before the patch: $perl -wc ./misc/cronjobs/gather_print_notices.pl Global symbol "$OUTPUT" requires explicit package name at [...]./misc/cronjobs/gather_print_notices.pl line 81. ./misc/cronjobs/gather_print_notices.pl had compilation errors. With this patch: $perl -wc ./misc/cronjobs/gather_print_notices.pl ./misc/cronjobs/gather_print_notices.pl syntax OK
Only the last patch is waiting for QA
QA comment: No more comments, passed QA !
Tested before pushing: * UPDATE message_queue SET status='pending' WHERE status=sent AND message_transport_type='print'; to set some message to pending status * run misc/cronjobs/gather_print_notices.pl tmp => VERIFIED that the print is in tmp/holdnotices-2012-07-16.html, and contains notices * UPDATE message_queue SET status='pending' WHERE status=sent AND message_transport_type='print'; to set some message to pending status * run misc/cronjobs/gather_print_notices.pl -s tmp => VERIFIED that the print is in tmp/holdnotices-2012-07-16-MAURES.html (MAURES being my branchcode), and contains notices