Attachment 119029 Details for Bug 10744 – Bug 10744 - ExpireReservesMaxPickUpDelay works with hold(s) over report

[patch] Bug 10744 - ExpireReservesMaxPickUpDelay works with hold(s) over report

0001-Bug-10744-ExpireReservesMaxPickUpDelay-works-with-ho.patch (text/plain), 18.62 KB, created by Emmi Takkinen on 2021-03-31 07:32:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Emmi Takkinen

Created: 2021-03-31 07:32:32 UTC

Size: 18.62 KB

patch

obsolete

>From 430d50210d45558d1e30405f518c2f95a77585c8 Mon Sep 17 00:00:00 2001
>From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
>Date: Wed, 28 Oct 2015 18:37:26 +0200
>Subject: [PATCH 1/2] Bug 10744 - ExpireReservesMaxPickUpDelay works with
> hold(s) over report
>
>Hold(s) over -report at circ/waitingreserves.pl doesn't work because
>ExpireReservesMaxPickup removes reserves from the koha.reserves-table to the
>koha.old_reserves-table.
>
>This patch adds a new column koha.reserves.pickupexpired telling when/if the
>reserve has got it's pickup duration expired.
>
>This date is used to pull old reserves to the waitingreserves-view.
>
>syspref 'PickupExpiredHoldsOverReportDuration' controls how many days after
>expiration old reserves are displayed on the report, and the values respect
>Koha::Calendar holidays
>
>To test:
>1) Apply patch.
>2) Enable 'ExpireReservesMaxPickUpDelay'.
>3) Set some value to 'ReservesMaxPickUpDelay' and 'PickupExpiredHoldsOverReportDuration' (e.g. 2 and 7).
>4) Find or create hold(s) waiting pickup and set their expiration date so it exceeds 'ReservesMaxPickUpDelay'
>but not 'PickupExpiredHoldsOverReportDuration'.
>5) Confirm hold(s) are moved to 'Holds waiting over ...' list.
>6) Run misc/cronjobs/holds/cancel_expired_holds.pl.
>7) Confirm hold(s) are still displayed on the list.
>8) Confirm from db that hold(s) have 'pickupexpired' set same as 'expirationdate'.
>9) Set new value to 'PickupExpiredHoldsOverReportDuration' so it no longer covers hold(s) expiration date(s).
>10) Confirm that hold(s) are no longer displayed in waiting list
>
>Also prove t/db_depentent/Koha/Old.t
>---
> C4/Reserves.pm                                |   3 +
> Koha/Hold.pm                                  |  11 ++
> Koha/Old/Holds.pm                             |  59 ++++++++++
> circ/waitingreserves.pl                       |   9 +-
> ...re_reserves_conflict_hold_over_report.perl |  16 +++
> installer/data/mysql/kohastructure.sql        |   4 +
> installer/data/mysql/mandatory/sysprefs.sql   |   1 +
> .../admin/preferences/circulation.pref        |   4 +
> t/db_dependent/Koha/Old.t                     | 103 +++++++++++++++++-
> 9 files changed, 208 insertions(+), 2 deletions(-)
> create mode 100644 installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl
>
>diff --git a/C4/Reserves.pm b/C4/Reserves.pm
>index 54ccc7457d..cb1c7502c2 100644
>--- a/C4/Reserves.pm
>+++ b/C4/Reserves.pm
>@@ -948,6 +948,9 @@ sub CancelExpiredReserves {
>         if ( defined($hold->found) && $hold->found eq 'W' ) {
>             $cancel_params->{charge_cancel_fee} = 1;
>         }
>+
>+        $cancel_params->{pickupexpired} = dt_from_string($hold->expirationdate);
>+
>         $hold->cancel( $cancel_params );
>     }
> }
>diff --git a/Koha/Hold.pm b/Koha/Hold.pm
>index 892fb20cd5..ed194164fd 100644
>--- a/Koha/Hold.pm
>+++ b/Koha/Hold.pm
>@@ -38,6 +38,7 @@ use Koha::Old::Holds;
> use Koha::Calendar;
> 
> use Koha::Exceptions::Hold;
>+use Koha::Exceptions::BadParameter;
> 
> use base qw(Koha::Object);
> 
>@@ -499,6 +500,16 @@ sub cancel {
>     my ( $self, $params ) = @_;
>     $self->_result->result_source->schema->txn_do(
>         sub {
>+
>+            my $pickupexpired = $params->{pickupexpired};
>+            if ($pickupexpired) {
>+                unless ($pickupexpired && $pickupexpired->isa('DateTime')) {
>+                    Koha::Exceptions::BadParameter->throw(error => "cancel():> Parameter 'pickupexpired' is not a DateTime-object or undef!");
>+                } else {
>+                    $self->pickupexpired($pickupexpired->ymd());
>+                }
>+            }
>+
>             $self->cancellationdate( dt_from_string->strftime( '%Y-%m-%d %H:%M:%S' ) );
>             $self->priority(0);
>             $self->cancellation_reason( $params->{cancellation_reason} );
>diff --git a/Koha/Old/Holds.pm b/Koha/Old/Holds.pm
>index e12e1446b2..8c2b84d45e 100644
>--- a/Koha/Old/Holds.pm
>+++ b/Koha/Old/Holds.pm
>@@ -20,8 +20,10 @@ package Koha::Old::Holds;
> use Modern::Perl;
> 
> use Carp;
>+use Scalar::Util qw(blessed);
> 
> use Koha::Database;
>+use Koha::Exceptions::BadParameter;
> 
> use Koha::Old::Hold;
> 
>@@ -39,6 +41,63 @@ This object represents a set of holds that have been filled or canceled
> 
> =cut
> 
>+=head3
>+
>+my $expired_holds = Koha::Old::Holds->expired();
>+
>+Returns all holds that have already expired and moved to old_reserves during period
>+of time given in 'PickupExpiredHoldsOverReportDuration'.
>+
>+If branchcode is given returns expired holds for that branch and take into account
>+its holidays.
>+
>+=cut
>+
>+sub expired {
>+    my ($self, $params) = @_;
>+
>+    my $pickupExpiredHoldsOverReportDuration = C4::Context->preference('PickupExpiredHoldsOverReportDuration');
>+    return undef unless $pickupExpiredHoldsOverReportDuration;
>+
>+    my $branchcode = $params->{branchcode};
>+    if ($params->{from}) {
>+        unless (blessed($params->{from}) && $params->{from}->isa('DateTime')) {
>+            Koha::Exceptions::BadParameter->throw(error => "expired():> Parameter 'from' is not a DateTime-object or undef!");
>+        }
>+    }
>+    if ($params->{to}) {
>+        unless (blessed($params->{from}) && $params->{from}->isa('DateTime')) {
>+            Koha::Exceptions::BadParameter->throw(error => "expired():> Parameter 'from' is not a DateTime-object or undef!");
>+        }
>+    }
>+
>+    #Calculate the days for which we get the expired reserves.
>+    my $fromDate   = $params->{from};
>+    my $toDate     = $params->{to}   || DateTime->now(time_zone => C4::Context->tz());
>+
>+    unless ($fromDate) {
>+        $fromDate = DateTime->now( time_zone => C4::Context->tz() );
>+        #Look for previous open days
>+        if ($branchcode) {
>+            my $calendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Default');
>+            foreach my $i (1..$pickupExpiredHoldsOverReportDuration) {
>+                $fromDate = $calendar->prev_open_days($fromDate, 1);
>+            }
>+        }
>+        #If no branch has been specified we cannot use a calendar, so simply just go back in time.
>+        else {
>+            $fromDate = DateTime->now(time_zone => C4::Context->tz())->subtract(days => $pickupExpiredHoldsOverReportDuration);
>+        }
>+    }
>+
>+    my $search_params;
>+    $search_params->{priority} = 0;
>+    $search_params->{pickupexpired} = { -between => [ $fromDate->ymd(), $toDate->ymd() ] };
>+    $search_params->{branchcode} = $branchcode if defined $branchcode;
>+
>+    return $self->search( $search_params, { order_by => 'waitingdate' } );
>+}
>+
> =head3 type
> 
> =cut
>diff --git a/circ/waitingreserves.pl b/circ/waitingreserves.pl
>index 1a8d74ade4..596e10ebfd 100755
>--- a/circ/waitingreserves.pl
>+++ b/circ/waitingreserves.pl
>@@ -39,6 +39,7 @@ use Koha::BiblioFrameworks;
> use Koha::Items;
> use Koha::ItemTypes;
> use Koha::Patrons;
>+use Koha::Old::Holds;
> 
> my $input = CGI->new;
> 
>@@ -83,7 +84,7 @@ $template->param( all_branches => 1 ) if $all_branches;
> my (@reserve_loop, @over_loop);
> # FIXME - Is priority => 0 useful? If yes it must be moved to waiting, otherwise we need to remove it from here.
> my $holds = Koha::Holds->waiting->search({ priority => 0, ( $all_branches ? () : ( branchcode => $default ) ) }, { order_by => ['waitingdate'] });
>-
>+my $expired_holds = $all_branches ? Koha::Old::Holds->expired() : Koha::Old::Holds->expired({branchcode => $default});
> # get reserves for the branch we are logged into, or for all branches
> 
> my $today = Date_to_Days(&Today);
>@@ -108,6 +109,12 @@ while ( my $hold = $holds->next ) {
>     
> }
> 
>+if($expired_holds){
>+    while (my $expired_hold = $expired_holds->next){
>+        push @over_loop, $expired_hold;
>+    }
>+}
>+
> $template->param(cancel_result => \@cancel_result) if @cancel_result;
> 
> $template->param(
>diff --git a/installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl b/installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl
>new file mode 100644
>index 0000000000..71753eec9f
>--- /dev/null
>+++ b/installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl
>@@ -0,0 +1,16 @@
>+$DBversion = 'XXX'; # will be replaced by the RM
>+if( CheckVersion( $DBversion ) ) {
>+    # you can use $dbh here like:
>+    # $dbh->do( "ALTER TABLE biblio ADD COLUMN badtaste int" );
>+
>+    $dbh->do("ALTER TABLE reserves ADD `pickupexpired` DATE DEFAULT NULL AFTER `expirationdate`");
>+    $dbh->do("ALTER TABLE reserves ADD KEY `reserves_pickupexpired` (`pickupexpired`)");
>+    $dbh->do("ALTER TABLE old_reserves ADD `pickupexpired` DATE DEFAULT NULL AFTER `expirationdate`");
>+    $dbh->do("ALTER TABLE old_reserves ADD KEY `old_reserves_pickupexpired` (`pickupexpired`)");
>+
>+    $dbh->do("INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('PickupExpiredHoldsOverReportDuration','1',NULL,\"For how many days holds expired by the 'ExpireReservesMaxPickUpDelay'-syspref are visible in the 'Hold Over'-tab in /circ/waitingreserves.pl ?\",'Integer')");
>+
>+    # Always end with this (adjust the bug info)
>+    SetVersion( $DBversion );
>+    print "Upgrade to $DBversion done (Bug 10744 - ExpireReservesMaxPickUpDelay works with hold(s) over report)\n";
>+}
>diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
>index ae247e67c8..6aeee74e58 100644
>--- a/installer/data/mysql/kohastructure.sql
>+++ b/installer/data/mysql/kohastructure.sql
>@@ -3872,6 +3872,7 @@ CREATE TABLE `old_reserves` (
>   `itemnumber` int(11) DEFAULT NULL COMMENT 'foreign key from the items table defining the specific item the patron has placed on hold or the item this hold was filled with',
>   `waitingdate` date DEFAULT NULL COMMENT 'the date the item was marked as waiting for the patron at the library',
>   `expirationdate` date DEFAULT NULL COMMENT 'the date the hold expires (usually the date entered by the patron to say they don''t need the hold after a certain date)',
>+  `pickupexpired` DATE DEFAULT NULL COMMENT 'if hold has been waiting but it expired before it was picked up, the expiration date is set here',
>   `lowestPriority` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'has this hold been pinned to the lowest priority in the holds queue (1 for yes, 0 for no)',
>   `suspend` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'in this hold suspended (1 for yes, 0 for no)',
>   `suspend_until` datetime DEFAULT NULL COMMENT 'the date this hold is suspended until (NULL for infinitely)',
>@@ -3883,6 +3884,7 @@ CREATE TABLE `old_reserves` (
>   KEY `old_reserves_biblionumber` (`biblionumber`),
>   KEY `old_reserves_itemnumber` (`itemnumber`),
>   KEY `old_reserves_branchcode` (`branchcode`),
>+  KEY `old_reserves_pickupexpired` (`pickupexpired`),
>   KEY `old_reserves_itemtype` (`itemtype`),
>   CONSTRAINT `old_reserves_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL,
>   CONSTRAINT `old_reserves_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE SET NULL ON UPDATE SET NULL,
>@@ -4311,6 +4313,7 @@ CREATE TABLE `reserves` (
>   `itemnumber` int(11) DEFAULT NULL COMMENT 'foreign key from the items table defining the specific item the patron has placed on hold or the item this hold was filled with',
>   `waitingdate` date DEFAULT NULL COMMENT 'the date the item was marked as waiting for the patron at the library',
>   `expirationdate` date DEFAULT NULL COMMENT 'the date the hold expires (usually the date entered by the patron to say they don''t need the hold after a certain date)',
>+  `pickupexpired` DATE DEFAULT NULL COMMENT 'if hold has been waiting but it expired before it was picked up, the expiration date is set here',
>   `lowestPriority` tinyint(1) NOT NULL DEFAULT 0,
>   `suspend` tinyint(1) NOT NULL DEFAULT 0,
>   `suspend_until` datetime DEFAULT NULL,
>@@ -4323,6 +4326,7 @@ CREATE TABLE `reserves` (
>   KEY `biblionumber` (`biblionumber`),
>   KEY `itemnumber` (`itemnumber`),
>   KEY `branchcode` (`branchcode`),
>+  KEY `reserves_pickupexpired` (`pickupexpired`),
>   KEY `desk_id` (`desk_id`),
>   KEY `itemtype` (`itemtype`),
>   CONSTRAINT `reserves_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE,
>diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql
>index 1034313061..f8354df879 100644
>--- a/installer/data/mysql/mandatory/sysprefs.sql
>+++ b/installer/data/mysql/mandatory/sysprefs.sql
>@@ -195,6 +195,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
> ('EnhancedMessagingPreferencesOPAC', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo'),
> ('expandedSearchOption','0',NULL,'If ON, set advanced search to be expanded by default','YesNo'),
> ('ExpireReservesMaxPickUpDelay','0','','Enabling this allows holds to expire automatically if they have not been picked by within the time period specified in ReservesMaxPickUpDelay','YesNo'),
>+('PickupExpiredHoldsOverReportDuration','1',NULL,"For how many days holds expired by the 'ExpireReservesMaxPickUpDelay'-syspref are visible in the 'Hold Over'-tab in /circ/waitingreserves.pl ?",'Integer'),
> ('ExpireReservesMaxPickUpDelayCharge','0',NULL,'If ExpireReservesMaxPickUpDelay is enabled, and this field has a non-zero value, than a borrower whose waiting hold has expired will be charged this amount.','free'),
> ('ExpireReservesOnHolidays', '1', NULL, 'If false, reserves at a library will not be canceled on days the library is not open.', 'YesNo'),
> ('ExcludeHolidaysFromMaxPickUpDelay', '0', NULL, 'If ON, reserves max pickup delay takes into accountthe closed days.', 'YesNo'),
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>index 9acc59f50e..2bbcb148d5 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>@@ -738,6 +738,10 @@ Circulation:
>             - pref: ExpireReservesMaxPickUpDelayCharge
>               class: currency
>             - "."
>+        -
>+            - pref: PickupExpiredHoldsOverReportDuration
>+              class: integer
>+            - "For how many days holds expired by the 'ExpireReservesMaxPickUpDelay'-syspref are visible in the 'Hold Over'-tab in /circ/waitingreserves.pl ?"
>         -
>             - Satisfy holds using items from the libraries
>             - pref: StaticHoldsQueueWeight
>diff --git a/t/db_dependent/Koha/Old.t b/t/db_dependent/Koha/Old.t
>index 42bdbe4d65..0a3aa534c8 100755
>--- a/t/db_dependent/Koha/Old.t
>+++ b/t/db_dependent/Koha/Old.t
>@@ -19,17 +19,28 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 3;
>+use Test::More tests => 4;
> 
>+use DateTime;
>+use DateTime::TimeZone;
>+
>+use C4::Reserves;
> use Koha::Database;
> use Koha::Old::Patrons;
> use Koha::Old::Biblios;
> use Koha::Old::Checkouts;
> use Koha::Old::Items;
>+use Koha::Old::Holds;
>+use Koha::Calendar;
> 
> use t::lib::TestBuilder;
>+use t::lib::Mocks;
> 
> my $schema  = Koha::Database->new->schema;
>+
>+#since we use calendar here, flush caches
>+Koha::Caches->get_instance()->flush_all();
>+
> my $builder = t::lib::TestBuilder->new;
> 
> subtest 'Koha::Old::Patrons' => sub {
>@@ -85,3 +96,93 @@ subtest 'Koha::Old::Checkout->library() tests' => sub {
> 
>     $schema->storage->txn_rollback;
> };
>+
>+subtest 'Koha::Old::Holds' => sub {
>+
>+    $schema->storage->txn_begin;
>+
>+    plan tests => 5;
>+
>+    my $branch = $builder->build_object({ class => 'Koha::Libraries' });
>+    my $calendar = Koha::Calendar->new( branchcode => $branch->branchcode );
>+    my $holiday_date = DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2);
>+    my $holiday = $builder->build({
>+        source => 'SpecialHoliday',
>+        value  => {
>+            branchcode  => $branch->branchcode,
>+            day         => $holiday_date->day,
>+            month       => $holiday_date->month,
>+            year        => $holiday_date->year,
>+        },
>+    });
>+
>+    t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelay', 1);
>+
>+    t::lib::Mocks::mock_preference('ReservesMaxPickUpDelay', 6);
>+    t::lib::Mocks::mock_preference('PickupExpiredHoldsOverReportDuration', 2);
>+
>+    my $hold_1 = $builder->build_object({
>+        class => 'Koha::Holds',
>+        value => {
>+            branchcode => $branch->branchcode,
>+            expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 3)->iso8601(),
>+        }
>+    });
>+    my $hold_2 = $builder->build_object({
>+        class => 'Koha::Holds',
>+        value => {
>+            branchcode => $branch->branchcode,
>+            expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2)->iso8601(),
>+        }
>+    });
>+    my $hold_3 = $builder->build_object({
>+        class => 'Koha::Holds',
>+        value => {
>+            branchcode => $branch->branchcode,
>+            expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->iso8601(),
>+        }
>+    });
>+    my $hold_4 = $builder->build_object({
>+        class => 'Koha::Holds',
>+        value => {
>+            branchcode => $branch->branchcode,
>+            expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->iso8601(),
>+        }
>+    });
>+    my $hold_5 = $builder->build_object({
>+        class => 'Koha::Holds',
>+        value => {
>+            branchcode => $branch->branchcode,
>+            expirationdate => DateTime->now(time_zone => C4::Context->tz()),
>+        }
>+    });
>+    my $hold_6 = $builder->build_object({
>+        class => 'Koha::Holds',
>+        value => {
>+            branchcode => $branch->branchcode,
>+            expirationdate => DateTime->now(time_zone => C4::Context->tz())->add(days => 1)->iso8601(),
>+        }
>+    });
>+
>+    C4::Reserves::CancelExpiredReserves();
>+
>+    my $expired_holds = Koha::Old::Holds->expired({ branchcode => $branch->branchcode });
>+    is($expired_holds->count(), 3, 'expired() returns 3 holds');
>+
>+    my $expired_hold_1 = $expired_holds->find($hold_1->id);
>+    my $expired_hold_2 = $expired_holds->find($hold_2->id);
>+    my $expired_hold_3 = $expired_holds->find($hold_3->id);
>+    my $expired_hold_4 = $expired_holds->find($hold_4->id);
>+    my $expired_hold_5 = $expired_holds->find($hold_5->id);
>+    my $expired_hold_6 = $expired_holds->find($hold_6->id);
>+
>+    is($expired_hold_1, undef, "Hold 1 not found with expired().");
>+    ok($expired_hold_2->pickupexpired eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2)->ymd()
>+    , "Pick up for hold 2 expired 2 days ago.");
>+    ok($expired_hold_3->pickupexpired && $expired_hold_4->pickupexpired eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->ymd()
>+    , "Pick up for holds 3 and 4 expired yesterday.");
>+    is($expired_hold_5 && $expired_hold_6, undef, "Holds 5 and 6 not expired.");
>+
>+    $schema->storage->txn_rollback;
>+
>+};
>-- 
>2.25.1
>

Actions: View | Diff | Splinter Review

Attachments on bug 10744: 44134 | 44294 | 44414 | 44464 | 106446 | 110030 | 110031 | 119029 | 119030