LP1076062: Hold overrides not working properly.

Edit the verify_copy_for_hold helper function to actually work.  There
were cases where it would permit a copy when it shouldn't and also
disallow a copy when it should have allowed it.  It now makes better
use of the oargs override argument.

If oargs has an events member, the method now removes the matching from
the list of failure events returned by the permit_copy_hold check.  It
was previously bugged, stopping on the first matching event, clearing
the list of failed and events, and returning a value to indicate that
the copy is permitted for the hold, even when it may not be.

If oargs has the "all" member and there are failure events remaining,
the function loops through those events checking if the requestor has
the override permission for the event.  If the requestor does have the
override permission, then the event is added to oargs->{events} to be
saved for future checks on future copies.  It the requestor does not
have the override permission, then the event is pushed onto a new array
of failed events and also onto oargs->{failed}.  The latter is kept
to avoid looking up events repeatedly on future copy checks.

The oargs->{failed} member is added as a shortcut to avoid repeatedly
looking up override permissions when the requestor does not have them.
It is actually checked, when present, before the user permission is
checked with a costly database lookup.

Since verify_copy_for_hold has no other way to determine that an override
was requested, it checks for the events and/or all members of oargs being
set and having a value that would evaluate to true.  If oargs is undefined,
a hashref with no members, or lacks a "true" events or all member, then
verify_copy_for_hold functions as though no overrides are requested.

Additionally, all functions that call verify_copy_for_hold, either directly
or indirectly, have had their intro logic modified to only set oargs when
an override is requested and oargs is not already set.  We make the assump-
tion that if oargs is set, it contains the events member.  (Perhaps that is
a poor assumption, but all of the code so far looks like it will work.)

Fix test_and_create_hold_batch so that it passes the oargs parameter into
open-ils.circ.title_hold.is_possible in the way that check_title_hold (the
implementation function) actually expects it to be passed.  This also means
that we need to delete oargs from the params hashref before creating the
hold objects, as that would blow up if it were present.

Signed-off-by: Jason Stephenson <jstephenson@mvlc.org>
Signed-off-by: Bill Erickson <berick@esilibrary.com>

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm
index 7b63cfd..820e7c6 100644 (file)
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm
@@ -69,8 +69,12 @@ __PACKAGE__->register_method(
 sub test_and_create_hold_batch {
     my( $self, $conn, $auth, $params, $target_list, $oargs ) = @_;
 
-    my $override = 1 if $self->api_name =~ /override/;
-    $oargs = { all => 1 } unless defined $oargs;
+    my $override = 0;
+    if ($self->api_name =~ /override/) {
+        $override = 1;
+        $oargs = { all => 1 } unless defined $oargs;
+        $$params{oargs} = $oargs; # for is_possible checking.
+    }
 
     my $e = new_editor(authtoken=>$auth);
     return $e->die_event unless $e->checkauth;
@@ -96,6 +100,11 @@ sub test_and_create_hold_batch {
 
             $params->{'depth'} = $res->{'depth'} if $res->{'depth'};
 
+            # Remove oargs from params so holds can be created.
+            if ($$params{oargs}) {
+                delete $$params{oargs};
+            }
+
             my $ahr = construct_hold_request_object($params);
             my ($res2) = $self->method_lookup(
                 $override
@@ -230,8 +239,11 @@ sub create_hold {
     my $e = new_editor(authtoken=>$auth, xact=>1);
     return $e->die_event unless $e->checkauth;
 
-    my $override = 1 if $self->api_name =~ /override/;
-    $oargs = { all => 1 } unless defined $oargs;
+    my $override = 0;
+    if ($self->api_name =~ /override/) {
+        $override = 1;
+        $oargs = { all => 1 } unless defined $oargs;
+    }
 
     my @events;
 
@@ -2885,7 +2897,7 @@ sub _check_volume_hold_is_possible {
 
 sub verify_copy_for_hold {
     my( $patron, $requestor, $title, $copy, $pickup_lib, $request_lib, $oargs ) = @_;
-    $oargs = {} unless defined $oargs;
+    # $oargs should be undef unless we're overriding.
     $logger->info("checking possibility of copy in hold request for copy ".$copy->id);
     my $permitted = OpenILS::Utils::PermitHold::permit_copy_hold(
         {
@@ -2901,15 +2913,46 @@ sub verify_copy_for_hold {
         }
     );
 
-    # All overridden?
-    my $permit_anyway = 0;
-    foreach my $permit_event (@$permitted) {
-        if (grep { $_ eq $permit_event->{textcode} } @{$oargs->{events}}) {
-            $permit_anyway = 1;
-            last;
+    # Check for override permissions on events.
+    if ($oargs && $permitted && scalar @$permitted) {
+        # Remove the events from permitted that we can override.
+        if ($oargs->{events}) {
+            foreach my $evt (@{$oargs->{events}}) {
+                $permitted = [grep {$_->{textcode} ne $evt} @{$permitted}];
+            }
+        }
+        # Now, we handle the override all case by checking remaining
+        # events against override permissions.
+        if (scalar @$permitted && $oargs->{all}) {
+            # Pre-set events and failed members of oargs to empty
+            # arrays, if they are not set, yet.
+            $oargs->{events} = [] unless ($oargs->{events});
+            $oargs->{failed} = [] unless ($oargs->{failed});
+            # When we're done with these checks, we swap permitted
+            # with a reference to @disallowed.
+            my @disallowed = ();
+            foreach my $evt (@{$permitted}) {
+                # Check if we've already seen the event in this
+                # session and it failed.
+                if (grep {$_ eq $evt->{textcode}} @{$oargs->{failed}}) {
+                    push(@disallowed, $evt);
+                } else {
+                    # We have to check if the requestor has the
+                    # override permission.
+
+                    # AppUtils::check_user_perms returns the perm if
+                    # the user doesn't have it, undef if they do.
+                    if ($apputils->check_user_perms($requestor->id, $requestor->ws_ou, $evt->{textcode} . '.override')) {
+                        push(@disallowed, $evt);
+                        push(@{$oargs->{failed}}, $evt->{textcode});
+                    } else {
+                        push(@{$oargs->{events}}, $evt->{textcode});
+                    }
+                }
+            }
+            $permitted = \@disallowed;
         }
     }
-    $permitted = [] if $permit_anyway;
 
     my $age_protect_only = 0;
     if (@$permitted == 1 && @$permitted[0]->{textcode} eq 'ITEM_AGE_PROTECTED') {