Bug 8063 - Hold print notices do not sort by branch
Summary: Hold print notices do not sort by branch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-09 15:08 UTC by Joy Nelson
Modified: 2013-05-23 06:24 UTC (History)
5 users (show)

See Also:
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 (5.63 KB, patch)
2012-05-09 16:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch (5.69 KB, patch)
2012-05-11 14:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8063 - Hold print notices do not sort by branch (5.76 KB, patch)
2012-05-22 12:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8063 - Followup - Make C4/Letters.pm conform to PBP (5.97 KB, patch)
2012-05-22 12:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8063 - Hold print notices do not sort by branch (5.82 KB, patch)
2012-06-04 13:20 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8063 - Followup - Make C4/Letters.pm conform to PBP (6.03 KB, patch)
2012-06-04 13:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8063 - Followup - Bug fix (1.94 KB, patch)
2012-06-06 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8063 - [SIGNED-OFF] Followup - Bug fix (2.36 KB, patch)
2012-07-12 12:15 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 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