From e6b376997ac078b422fee8785923ff1eac73b9c0 Mon Sep 17 00:00:00 2001 From: Jason Stephenson Date: Mon, 10 Dec 2012 10:29:21 -0500 Subject: [PATCH] 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 Signed-off-by: Bill Erickson --- .../perlmods/lib/OpenILS/Application/Circ/Holds.pm | 67 ++++++++++++++++++---- 1 file changed, 55 insertions(+), 12 deletions(-) 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 7b63cfd12b..820e7c6ccc 100644 --- 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') { -- 2.11.0