Attachment 22329 Details for Bug 11126 – Bug 11126 - Make the holds system optionally give precedence to local holds

[patch] Bug 11126 - Make the holds system optionally give precedence to local holds

Bug-11126---Make-the-holds-system-optionally-give-.patch (text/plain), 14.33 KB, created by Kyle M Hall (khall) on 2013-10-23 16:21:42 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2013-10-23 16:21:42 UTC

Size: 14.33 KB

patch

obsolete

>From 4499f1d46948f0556051271bce22eba0ead46ef5 Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>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
>---
> C4/Reserves.pm                                     |   49 +++++++-
> 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, 191 insertions(+), 7 deletions(-)
> create mode 100755 t/db_dependent/Holds/LocalHoldsPriority.t
>
>diff --git a/C4/Reserves.pm b/C4/Reserves.pm
>index f8666a3..927fc3f 100644
>--- a/C4/Reserves.pm
>+++ b/C4/Reserves.pm
>@@ -847,7 +847,9 @@ sub CheckReserves {
>            items.biblioitemnumber,
>            itemtypes.notforloan,
>            items.notforloan AS itemnotforloan,
>-           items.itemnumber
>+           items.itemnumber,
>+           items.homebranch,
>+           items.holdingbranch
>            FROM   items
>            LEFT JOIN biblioitems ON items.biblioitemnumber = biblioitems.biblioitemnumber
>            LEFT JOIN itemtypes   ON items.itype   = itemtypes.itemtype
>@@ -859,7 +861,9 @@ sub CheckReserves {
>            items.biblioitemnumber,
>            itemtypes.notforloan,
>            items.notforloan AS itemnotforloan,
>-           items.itemnumber
>+           items.itemnumber,
>+           items.homebranch,
>+           items.holdingbranch
>            FROM   items
>            LEFT JOIN biblioitems ON items.biblioitemnumber = biblioitems.biblioitemnumber
>            LEFT JOIN itemtypes   ON biblioitems.itemtype   = itemtypes.itemtype
>@@ -875,12 +879,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 ) = $sth->fetchrow_array;
>+    my ( $biblio, $bibitem, $notforloan_per_itemtype, $notforloan_per_item,
>+        $itemnumber, $item_homebranch, $item_holdingbranch )
>+      = $sth->fetchrow_array;
> 
>     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
>@@ -892,21 +898,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 71513ce..0e7f772 100644
>--- a/installer/data/mysql/sysprefs.sql
>+++ b/installer/data/mysql/sysprefs.sql
>@@ -170,6 +170,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,NULL,'Textarea'),
> ('MARCOrgCode','OSt','','Define MARC Organization Code - http://www.loc.gov/marc/organizations/orgshome.html','free'),
>diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl
>index b031ed8..1925a8b 100755
>--- a/installer/data/mysql/updatedatabase.pl
>+++ b/installer/data/mysql/updatedatabase.pl
>@@ -7330,6 +7330,18 @@ if ( CheckVersion($DBversion) ) {
>     SetVersion($DBversion);
> }
> 
>+$DBversion = "3.13.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 a8f4e43..a2627c6 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
>@@ -489,6 +489,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

Actions: View | Diff | Splinter Review

Attachments on bug 11126: 22328 | 22329 | 22543 | 22778 | 22784 | 24195 | 25589 | 28145 | 28146 | 28181 | 28182 | 30215 | 30216 | 32189 | 32191 | 32192 | 32193 | 32195 | 32196 | 32197