From 9369ea25d489141e60f70a4928ec259587f1f6eb Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 23 Oct 2013 11:58:34 -0400 Subject: [PATCH] Bug 11126 - Make the holds system optionally give precedence to local holds This feature will allow libraries to specify that, when an item is returned, a local hold may be given priority for fulfillment even though it is of lower priority in the list of unfilled holds. This feature has three settings: * LocalHoldsPriority, which enables the feature * LocalHoldsPriorityPatronControl, which selects for either tha patron's home library, or the patron's pickup library for the hold * LocalHoldsPriorityItemControl, which selects for either the item's holding library, or home library. So, this feature can "give priority for filling holds to patrons whose (home library|pickup library) matches the item's (home library|holding library)" Test Plan: 1) Apply this patch 2) Run t/db_dependent/Holds/LocalHoldsPriority.t Signed-off-by: Joel Sasse --- C4/Reserves.pm | 47 +++++++- installer/data/mysql/sysprefs.sql | 3 + installer/data/mysql/updatedatabase.pl | 12 ++ .../en/modules/admin/preferences/circulation.pref | 15 +++ t/db_dependent/Holds/LocalHoldsPriority.t | 119 ++++++++++++++++++++ 5 files changed, 189 insertions(+), 7 deletions(-) create mode 100755 t/db_dependent/Holds/LocalHoldsPriority.t diff --git a/C4/Reserves.pm b/C4/Reserves.pm index a98eed4..426eab9 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -874,7 +874,9 @@ sub CheckReserves { itemtypes.notforloan, items.notforloan AS itemnotforloan, items.itemnumber, - items.damaged + items.damaged, + items.homebranch, + items.holdingbranch FROM items LEFT JOIN biblioitems ON items.biblioitemnumber = biblioitems.biblioitemnumber LEFT JOIN itemtypes ON items.itype = itemtypes.itemtype @@ -887,7 +889,9 @@ sub CheckReserves { itemtypes.notforloan, items.notforloan AS itemnotforloan, items.itemnumber, - items.damaged + items.damaged, + items.homebranch, + items.holdingbranch FROM items LEFT JOIN biblioitems ON items.biblioitemnumber = biblioitems.biblioitemnumber LEFT JOIN itemtypes ON biblioitems.itemtype = itemtypes.itemtype @@ -903,14 +907,14 @@ sub CheckReserves { $sth->execute($barcode); } # note: we get the itemnumber because we might have started w/ just the barcode. Now we know for sure we have it. - my ( $biblio, $bibitem, $notforloan_per_itemtype, $notforloan_per_item, $itemnumber, $damaged ) = $sth->fetchrow_array; + my ( $biblio, $bibitem, $notforloan_per_itemtype, $notforloan_per_item, $itemnumber, $damaged, $item_homebranch, $item_holdingbranch ) = $sth->fetchrow_array; return if ( $damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); return unless $itemnumber; # bail if we got nothing. # if item is not for loan it cannot be reserved either..... - # execpt where items.notforloan < 0 : This indicates the item is holdable. + # except where items.notforloan < 0 : This indicates the item is holdable. return if ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype; # Find this item in the reserves @@ -922,21 +926,50 @@ sub CheckReserves { # $highest is the most important item we've seen so far. my $highest; if (scalar @reserves) { + my $LocalHoldsPriority = C4::Context->preference('LocalHoldsPriority'); + my $LocalHoldsPriorityPatronControl = C4::Context->preference('LocalHoldsPriorityPatronControl'); + my $LocalHoldsPriorityItemControl = C4::Context->preference('LocalHoldsPriorityItemControl'); + my $priority = 10000000; foreach my $res (@reserves) { if ( $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) { return ( "Waiting", $res, \@reserves ); # Found it } else { + # Lazy fetch for borrower and item. We only need to know about the patron and item + # each and every time if we are using LocalHoldsPriority. This is a great place to + # leverage the inherent lazy fetching of DBIx::Class. + my $borrowerinfo; + my $iteminfo; + + my $local_hold_match; + if ($LocalHoldsPriority) { + $borrowerinfo = C4::Members::GetMember( borrowernumber => $res->{'borrowernumber'} ); + $iteminfo = C4::Items::GetItem($itemnumber); + + my $local_holds_priority_item_branchcode = + $iteminfo->{$LocalHoldsPriorityItemControl}; + my $local_holds_priority_patron_branchcode = + ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) + ? $res->{branchcode} + : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) + ? $borrowerinfo->{branchcode} + : undef; + $local_hold_match = + $local_holds_priority_item_branchcode eq + $local_holds_priority_patron_branchcode; + } + # See if this item is more important than what we've got so far - if ( $res->{'priority'} && $res->{'priority'} < $priority ) { - my $borrowerinfo=C4::Members::GetMember(borrowernumber => $res->{'borrowernumber'}); - my $iteminfo=C4::Items::GetItem($itemnumber); + if ( ( $res->{'priority'} && $res->{'priority'} < $priority ) || $local_hold_match ) { + $borrowerinfo ||= C4::Members::GetMember( borrowernumber => $res->{'borrowernumber'} ); + $iteminfo ||= C4::Items::GetItem($itemnumber); my $branch = GetReservesControlBranch( $iteminfo, $borrowerinfo ); my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$iteminfo->{'itype'}); next if ($branchitemrule->{'holdallowed'} == 0); next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $borrowerinfo->{'branchcode'})); $priority = $res->{'priority'}; $highest = $res; + last if $local_hold_match; } } } diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index d820b94..3ee5bcb 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -180,6 +180,9 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('LinkerOptions','','','A pipe-separated list of options for the linker.','free'), ('LinkerRelink','1',NULL,'If ON the authority linker will relink headings that have previously been linked every time it runs.','YesNo'), ('LocalCoverImages','0','1','Display local cover images on intranet details pages.','YesNo'), +('LocalHoldsPriority', '0', NULL, 'Enables the LocalHoldsPriority feature', 'YesNo'), +('LocalHoldsPriorityItemControl', 'holdingbranch', 'holdingbranch|homebranch', 'decides if the feature operates using the item''s home or holding library.', 'Choice'), +('LocalHoldsPriorityPatronControl', 'PickupLibrary', 'HomeLibrary|PickupLibrary', 'decides if the feature operates using the library set as the patron''s home library, or the library set as the pickup library for the given hold.', 'Choice'), ('ManInvInNoissuesCharge','1',NULL,'MANUAL_INV charges block checkouts (added to noissuescharge).','YesNo'), ('MARCAuthorityControlField008','|| aca||aabn | a|a d',NULL,'Define the contents of MARC21 authority control field 008 position 06-39','Textarea'), ('MarcFieldsToOrder','',NULL,'Set the mapping values for a new order line created from a MARC record in a staged file. In a YAML format.','textarea'), diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 94bb4ff..bf21037 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -8577,6 +8577,18 @@ 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 + ('LocalHoldsPriority', '0', NULL, 'Enables the LocalHoldsPriority feature', 'YesNo'), + ('LocalHoldsPriorityItemControl', 'holdingbranch', 'holdingbranch|homebranch', 'decides if the feature operates using the item''s home or holding library.', 'Choice'), + ('LocalHoldsPriorityPatronControl', 'PickupLibrary', 'HomeLibrary|PickupLibrary', 'decides if the feature operates using the library set as the patron''s home library, or the library set as the pickup library for the given hold.', 'Choice') + }); + print "Upgrade to $DBversion done (Bug 11126 - Make the holds system optionally give precedence to local 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 55d8a78..ec68eab 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 @@ -509,6 +509,21 @@ Circulation: yes: Allow no: "Don't allow" - a patron to place a hold on a record where the patron already has one or more items attached to that record checked out. + - + - pref: LocalHoldsPriority + choices: + yes: Give + no: "Don't give" + - priority for filling holds to patrons whose + - pref: LocalHoldsPriorityPatronControl + choices: + PickupLibrary: "pickup library" + HomeLibrary: "home library" + - matches the item's + - pref: LocalHoldsPriorityItemControl + choices: + homebranch: "home library" + holdingbranch: "holding library" Fines Policy: - - Calculate fines based on days overdue diff --git a/t/db_dependent/Holds/LocalHoldsPriority.t b/t/db_dependent/Holds/LocalHoldsPriority.t new file mode 100755 index 0000000..2e092b2 --- /dev/null +++ b/t/db_dependent/Holds/LocalHoldsPriority.t @@ -0,0 +1,119 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +use t::lib::Mocks; +use C4::Context; +use C4::Branch; + +use Test::More tests => 6; +use MARC::Record; +use C4::Biblio; +use C4::Items; +use C4::Members; + +BEGIN { + use FindBin; + use lib $FindBin::Bin; + use_ok('C4::Reserves'); +} + +my $dbh = C4::Context->dbh; + +# Start transaction +$dbh->{AutoCommit} = 0; +$dbh->{RaiseError} = 1; + +my $borrowers_count = 5; + +# Setup Test------------------------ +# Helper biblio. +diag("Creating biblio instance for testing."); +my ( $bibnum, $title, $bibitemnum ) = create_helper_biblio(); + +# Helper item for that biblio. +diag("Creating item instance for testing."); +my ( $item_bibnum, $item_bibitemnum, $itemnumber ) = + AddItem( { homebranch => 'MPL', holdingbranch => 'CPL' }, $bibnum ); + +my @branchcodes = qw{XXX RPL CPL MPL CPL MPL}; + +# Create some borrowers +diag("Creating borrowers."); +my @borrowernumbers; +foreach ( 1 .. $borrowers_count ) { + my $borrowernumber = AddMember( + firstname => 'my firstname', + surname => 'my surname ' . $_, + categorycode => 'S', + branchcode => $branchcodes[$_], + ); + push @borrowernumbers, $borrowernumber; +} + +my $biblionumber = $bibnum; + +my @branches = GetBranchesLoop(); +my $branch = $branches[0][0]{value}; + +# Create five item level holds +diag("Creating holds."); +my $i = 1; +foreach my $borrowernumber (@borrowernumbers) { + AddReserve( + $branchcodes[$i], + $borrowernumber, + $biblionumber, + my $constraint = 'a', + my $bibitems = q{}, + my $priority = $i, + my $resdate, + my $expdate, + my $notes = q{}, + $title, + my $checkitem, + my $found, + ); + + $i++; +} + +my ($status, $reserve, $all_reserves); + +t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 0 ); +($status, $reserve, $all_reserves) = CheckReserves($itemnumber); +ok( $reserve->{borrowernumber} eq $borrowernumbers[0], "Recieved expected results with LocalHoldsPriority disabled" ); + +t::lib::Mocks::mock_preference( 'LocalHoldsPriority', 1 ); + +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); +($status, $reserve, $all_reserves) = CheckReserves($itemnumber); +ok( $reserve->{borrowernumber} eq $borrowernumbers[2], "Received expected results with PickupLibrary/homebranch" ); + +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'PickupLibrary' ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); +($status, $reserve, $all_reserves) = CheckReserves($itemnumber); +ok( $reserve->{borrowernumber} eq $borrowernumbers[1], "Received expected results with PickupLibrary/holdingbranch" ); + +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'holdingbranch' ); +($status, $reserve, $all_reserves) = CheckReserves($itemnumber); +ok( $reserve->{borrowernumber} eq $borrowernumbers[1], "Received expected results with HomeLibrary/holdingbranch" ); + +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityPatronControl', 'HomeLibrary' ); +t::lib::Mocks::mock_preference( 'LocalHoldsPriorityItemControl', 'homebranch' ); +($status, $reserve, $all_reserves) = CheckReserves($itemnumber); +ok( $reserve->{borrowernumber} eq $borrowernumbers[2], "Received expected results with HomeLibrary/homebranch" ); + +# Helper method to set up a Biblio. +sub create_helper_biblio { + my $bib = MARC::Record->new(); + my $title = 'Silence in the library'; + $bib->append_fields( + MARC::Field->new( '100', ' ', ' ', a => 'Moffat, Steven' ), + MARC::Field->new( '245', ' ', ' ', a => $title ), + ); + return ( $bibnum, $title, $bibitemnum ) = AddBiblio( $bib, '' ); +} -- 1.7.2.5