Bug 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch evasive bugs in production.
Summary: cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch...
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-23 12:06 UTC by Olli-Antti Kivilahti
Modified: 2023-07-09 11:14 UTC (History)
2 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 13135 - cancel_expired_holds.pl/CancelExpiredReserves() logging improvements to catch evasive bugs in production. (9.00 KB, patch)
2014-10-23 12:07 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2014-10-23 12:06:43 UTC
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).
Comment 1 Olli-Antti Kivilahti 2014-10-23 12:07:43 UTC
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).
Comment 2 Olli-Antti Kivilahti 2014-10-23 12:15:21 UTC
Implemented for 3.16.4, not applying to master.
Comment 3 Mark Tompsett 2015-05-09 16:40:57 UTC
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 4 Mark Tompsett 2015-05-09 16:44:47 UTC
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 5 Mark Tompsett 2015-05-09 16:58:05 UTC
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?
Comment 6 Katrin Fischer 2023-07-09 11:14:35 UTC
(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.