Bug 8063

Summary: Hold print notices do not sort by branch
Product: Koha Reporter: Joy Nelson <1joynelson>
Component: Command-line UtilitiesAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Paul Poulain <paul.poulain>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, julian.maurice, koha.sekjal, kyle.m.hall, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5544
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 8063 - Hold print notices do not sort by branch
[SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch
Bug 8063 - Hold print notices do not sort by branch
Bug 8063 - Followup - Make C4/Letters.pm conform to PBP
[SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch
[SIGNED-OFF] Bug 8063 - Followup - Make C4/Letters.pm conform to PBP
Bug 8063 - Followup - Bug fix
Bug 8063 - [SIGNED-OFF] Followup - Bug fix

Description Joy Nelson 2012-05-09 15:08:43 UTC
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.
Comment 1 Kyle M Hall 2012-05-09 16:27:11 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-05-11 14:17:55 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2012-05-22 10:33:37 UTC
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
Comment 4 Kyle M Hall 2012-05-22 12:07:11 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-05-22 12:07:22 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-05-22 12:11:55 UTC
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.
Comment 7 Julian Maurice 2012-06-04 13:20:38 UTC
Created attachment 9912 [details] [review]
[SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch

Still works !
Comment 8 Julian Maurice 2012-06-04 13:22:53 UTC
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.
Comment 9 Ian Walls 2012-06-04 15:50:39 UTC
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.
Comment 10 Paul Poulain 2012-06-05 13:44:57 UTC
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 ?
Comment 11 Paul Poulain 2012-06-05 13:45:46 UTC
changing severity, it's an ENH, as well as assignee
Comment 12 Kyle M Hall 2012-06-06 13:42:27 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2012-07-12 12:15:49 UTC
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
Comment 14 Jonathan Druart 2012-07-12 12:18:08 UTC
Only the last patch is waiting for QA
Comment 15 Paul Poulain 2012-07-16 12:46:06 UTC
QA comment:

No more comments, passed QA !
Comment 16 Paul Poulain 2012-07-16 12:53:58 UTC
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