We have issues with the Calendar giving bad expiration dates for waiting holds. So far unable to isolate the bug, which appears to appear only in prodcution. This logging improvement gives a lot more information about the cronjob internals in varying levels. (L1) Logs expired holds which have expirationdate overdue. (L1) Logs holds waiting too long, those that did get cancelled and (L2) those that didn't. (L3) Logs the used Calendar-objects content ** * * * ** * TEST PLAN * ** * * * ** 0a. Set the ReservesMaxPickUpDelay-syspref to 6 days. 0b. Create reservations, with expirydate in the past, and in the future, and now. Catch some of those to a borrower. 0c. Create reservations without an expirationdate and catch them to borrowers. In the DB, set the waitingdate 2 days to the past for some of them, 4 days to the past for a portion, 6 days to past for some and 8 days past for the rest. 1. Run the cancel_expired_holds.pl-script with --verbose 3 Observe log entries of all the handled reservations and of all the used Calendars (not all Calendars, simply those that were needed).
Created attachment 32619 [details] [review] Bug 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch evasive bugs in production. We have issues with the Calendar giving bad expiration dates for waiting holds. So far unable to isolate the bug, which appears to appear only in prodcution. This logging improvement gives a lot more information about the cronjob internals in varying levels. (L1) Logs expired holds which have expirationdate overdue. (L1) Logs holds waiting too long, those that did get cancelled and (L2) those that didn't. (L3) Logs the used Calendar-objects content ** * * * ** * TEST PLAN * ** * * * ** 0a. Set the ReservesMaxPickUpDelay-syspref to 6 days. 0b. Create reservations, with expirydate in the past, and in the future, and now. Catch some of those to a borrower. 0c. Create reservations without an expirationdate and catch them to borrowers. In the DB, set the waitingdate 2 days to the past for some of them, 4 days to the past for a portion, 6 days to past for some and 8 days past for the rest. 1. Run the cancel_expired_holds.pl-script with --verbose 3 Observe log entries of all the handled reservations and of all the used Calendars (not all Calendars, simply those that were needed).
Implemented for 3.16.4, not applying to master.
Comment on attachment 32619 [details] [review] Bug 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch evasive bugs in production. Review of attachment 32619 [details] [review]: ----------------------------------------------------------------- ::: C4/Reserves.pm @@ +949,4 @@ > =cut > > sub CancelExpiredReserves { > + my $verbose = shift; || 0 would be a good thing to add, so $verbose is always defined.
Comment on attachment 32619 [details] [review] Bug 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch evasive bugs in production. Review of attachment 32619 [details] [review]: ----------------------------------------------------------------- ::: Koha/Calendar.pm @@ +365,5 @@ > > +sub printMe { > + my $self = shift; > + my @sb; #String buffer to collect self output. > + Do you have your git configured properly? White space like this is bad. git config --global core.whitespace trailing-space,space-before-tab git config --global apply.whitespace fix ::: misc/cronjobs/holds/cancel_expired_holds.pl @@ +17,4 @@ > # with Koha; if not, write to the Free Software Foundation, Inc., > # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > > +use Modern::Perl; Don't just change to Modern::Perl when both strict and warnings are commented out. Are you sure this doesn't break anything else?
Comment on attachment 32619 [details] [review] Bug 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch evasive bugs in production. Review of attachment 32619 [details] [review]: ----------------------------------------------------------------- Given that 3.16.x is much higher than 3.16.4, is there merit reworking this? Or should it be closed RESOLVED WON'T FIX? ::: C4/Reserves.pm @@ +981,4 @@ > > my $today = DateTime->now( time_zone => C4::Context->tz() ); > > + push @sb, "##Removing holds waiting too long\n##using today=$today, ReservesMaxPickUpDelay=$max_pickup_delay, ExpireReservesMaxPickUpDelayCharge=$charge\n". The whole today logic was ripped out and replaced with SQL logic in 3.16.x -- not required. @@ +992,5 @@ > } > CancelReserve({ reserve_id => $res->{'reserve_id'} }); > + push @sb, printReserve($res,'tab',['reserve_id','borrowernumber','branchcode','waitingdate']).sprintf("% 14s",substr($expiration,0,10))."| past lastpickupdate.\n" if $verbose; > + } > + elsif($verbose > 1) { Because of SQL logic, there is no if statement to elsif off of. @@ +998,2 @@ > } > + $usedCalendars{ $res->{branchcode} } = Koha::Calendar->new( branchcode => $res->{branchcode} ) if ( $verbose > 2 && not(exists($usedCalendars{$res->{branchcode}})) ); Is this required?
(In reply to Mark Tompsett from comment #5) > Comment on attachment 32619 [details] [review] [review] > Bug 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging > improvements to catch evasive bugs in production. > > Review of attachment 32619 [details] [review] [review]: > ----------------------------------------------------------------- > > Given that 3.16.x is much higher than 3.16.4, is there merit reworking this? > Or should it be closed RESOLVED WON'T FIX? Another 8 years later... I am marking WONTFIX.