From fb65cebad414efdd341975c9527053e71b02c159 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 29 Jan 2014 10:52:08 -0500 Subject: [PATCH] [PASSED QA] Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds The current holds behavior in Koha allows a situation like this: - Patron A has an item currently checked out. - Patron B places a hold on the next available copy of that title. - Then Patron A will not be able to renew his item, even if there are other available copies of that title that could potentially fill Patron B's hold. Since this seems unfair to Patron A, we should allow renewal of items even if there are unfilled holds, but those holds could all be filled with currently available items. Test Plan: 1) Apply this patch 2) Create a record with two items 3) Check out the item to a patron 4) Place a hold on the record 5) Note you cannot renew the item for the patron 6) Enable the new system preference AllowRenewalIfOtherItemsAvailable 7) Note you can now renew the item, as all the holds can be satisfied by available items. 8) Place a second hold on the record 9) Note you can no longer renew the item, as all the holds *cannot* be filled by currently available items Signed-off-by: Holger Meissner Signed-off-by: Chris Rohde Signed-off-by: Katrin Fischer --- C4/Circulation.pm | 54 ++++++++++++++++++++++ C4/Reserves.pm | 19 ++++---- installer/data/mysql/sysprefs.sql | 1 + installer/data/mysql/updatedatabase.pl | 10 ++++ .../en/modules/admin/preferences/circulation.pref | 6 +++ 5 files changed, 82 insertions(+), 8 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 76b6652..f786a13 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -48,7 +48,9 @@ use Data::Dumper; use Koha::DateUtils; use Koha::Calendar; use Koha::Borrower::Debarments; +use Koha::Database; use Carp; +use List::MoreUtils qw( uniq ); use Date::Calc qw( Today Today_and_Now @@ -2637,6 +2639,58 @@ sub CanBookBeRenewed { my ( $resfound, $resrec, undef ) = C4::Reserves::CheckReserves($itemnumber); + # This item can fill one or more unfilled reserve, can those unfilled reserves + # all be filled by other available items? + if ( $resfound + && C4::Context->preference('AllowRenewalIfOtherItemsAvailable') ) + { + my $schema = Koha::Database->new()->schema(); + + # Get all other items that could possibly fill reserves + my @itemnumbers = $schema->resultset('Item')->search( + { + biblionumber => $resrec->{biblionumber}, + onloan => undef, + -not => { itemnumber => $itemnumber } + }, + { columns => 'itemnumber' } + )->get_column('itemnumber')->all(); + + # Get all other reserves that could have been filled by this item + my @borrowernumbers; + while (1) { + my ( $reserve_found, $reserve, undef ) = + C4::Reserves::CheckReserves( $itemnumber, undef, undef, + \@borrowernumbers ); + + if ($reserve_found) { + push( @borrowernumbers, $reserve->{borrowernumber} ); + } + else { + last; + } + } + + # If the count of the union of the lists of reservable items for each borrower + # is equal or greater than the number of borrowers, we know that all reserves + # can be filled with available items. We can get the union of the sets simply + # by pushing all the elements onto an array and removing the duplicates. + my @reservable; + foreach my $b (@borrowernumbers) { + foreach my $i (@itemnumbers) { + if ( CanItemBeReserved( $b, $i ) ) { + push( @reservable, $i ); + } + } + } + + @reservable = uniq(@reservable); + + if ( @reservable >= @borrowernumbers ) { + $resfound = 0; + } + } + return ( 0, "on_reserve" ) if $resfound; # '' when no hold was found return ( 1, undef ) if $override_limit; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 7a075a4..a33d309 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -41,7 +41,7 @@ use Koha::DateUtils; use Koha::Calendar; use Koha::Database; -use List::MoreUtils qw( firstidx ); +use List::MoreUtils qw( firstidx any ); use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -904,7 +904,7 @@ table in the Koha database. =cut sub CheckReserves { - my ( $item, $barcode, $lookahead_days) = @_; + my ( $item, $barcode, $lookahead_days, $ignore_borrowers) = @_; my $dbh = C4::Context->dbh; my $sth; my $select; @@ -955,7 +955,7 @@ sub CheckReserves { return if ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype; # Find this item in the reserves - my @reserves = _Findgroupreserve( $bibitem, $biblio, $itemnumber, $lookahead_days); + my @reserves = _Findgroupreserve( $bibitem, $biblio, $itemnumber, $lookahead_days, $ignore_borrowers); # $priority and $highest are used to find the most important item # in the list returned by &_Findgroupreserve. (The lower $priority, @@ -1813,7 +1813,7 @@ sub _FixPriority { =head2 _Findgroupreserve - @results = &_Findgroupreserve($biblioitemnumber, $biblionumber, $itemnumber, $lookahead); + @results = &_Findgroupreserve($biblioitemnumber, $biblionumber, $itemnumber, $lookahead, $ignore_borrowers); Looks for an item-specific match first, then for a title-level match, returning the first match found. If neither, then we look for a 3rd kind of match based on @@ -1830,7 +1830,7 @@ C. =cut sub _Findgroupreserve { - my ( $bibitem, $biblio, $itemnumber, $lookahead) = @_; + my ( $bibitem, $biblio, $itemnumber, $lookahead, $ignore_borrowers) = @_; my $dbh = C4::Context->dbh; # TODO: consolidate at least the SELECT portion of the first 2 queries to a common $select var. @@ -1862,7 +1862,8 @@ sub _Findgroupreserve { $sth->execute($itemnumber, $lookahead||0); my @results; if ( my $data = $sth->fetchrow_hashref ) { - push( @results, $data ); + push( @results, $data ) + unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; } return @results if @results; @@ -1894,7 +1895,8 @@ sub _Findgroupreserve { $sth->execute($itemnumber, $lookahead||0); @results = (); if ( my $data = $sth->fetchrow_hashref ) { - push( @results, $data ); + push( @results, $data ) + unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; } return @results if @results; @@ -1927,7 +1929,8 @@ sub _Findgroupreserve { $sth->execute( $biblio, $bibitem, $itemnumber, $lookahead||0); @results = (); while ( my $data = $sth->fetchrow_hashref ) { - push( @results, $data ); + push( @results, $data ) + unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; } return @results; } diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index e881086..597852e 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -28,6 +28,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('AllowOnShelfHolds','0','','Allow hold requests to be placed on items that are not on loan','YesNo'), ('AllowPKIAuth','None','None|Common Name|emailAddress','Use the field from a client-side SSL certificate to look a user in the Koha database','Choice'), ('AllowPurchaseSuggestionBranchChoice','0','1','Allow user to choose branch when making a purchase suggestion','YesNo'), +('AllowRenewalIfOtherItemsAvailable','0',NULL,'If enabled, allow a patron to renew an item with unfilled holds if other available items can fill that hold.','YesNo'), ('AllowRenewalLimitOverride','0',NULL,'if ON, allows renewal limits to be overridden on the circulation screen','YesNo'), ('AllowReturnToBranch','anywhere','anywhere|homebranch|holdingbranch|homeorholdingbranch','Where an item may be returned','Choice'), ('AllowSelfCheckReturns','0','','If enabled, patrons may return items through the Web-based Self Checkout','YesNo'), diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 5064c40..62c334d 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -8891,6 +8891,16 @@ if ( CheckVersion($DBversion) ) { SetVersion($DBversion); } +$DBversion = "3.17.00.XXX"; +if (CheckVersion($DBversion)) { + $dbh->do(q{ + INSERT INTO systempreferences ( variable, value, options, explanation, type ) VALUES + ('AllowRenewalIfOtherItemsAvailable','0',NULL,'If enabled, allow a patron to renew an item with unfilled holds if other available items can fill that hold.','YesNo') + }); + print "Upgrade to $DBversion done (Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds)\n"; + SetVersion($DBversion); +} + =head1 FUNCTIONS =head2 TableExists($table) 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 2190004..f1dec0e 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 @@ -385,6 +385,12 @@ Circulation: - Each pair of values should be on a separate line. Holds Policy: - + - pref: AllowRenewalIfOtherItemsAvailable + choices: + yes: Allow + no: "Don't allow" + - a patron to renew an item with unfilled holds if other available items can fill that hold. + - - pref: AllowHoldPolicyOverride choices: yes: Allow -- 1.9.1