[OPEN-ILS-DEV] Holds Problems and a Proposed Solution

[Jason Stephenson]

While looking into Launchpad Bug 1076062
(https://bugs.launchpad.net/evergreen/+bug/1076062), we have
identified several potential issues with hold failure events and how
they are handled.  In order to resolve this bug, we believe that
extensive changes need to be made to the plumbing in
OpenILS/Application/Circ/Holds.pm and OpenILS/Event.pm.
Openils/WWW/EGCatLoader/Account.pm may require some minor
modifications, but we hope to implement the fix without requiring
changes in the client code of Holds.pm.

The primary problem appears to be the override logic in the
verify_copy_for_hold helper function in Holds.pm.  It receives a list
of permissions in the oargs parameter that the user has asked to
override and has the permission to override.  The permit_copy_hold
function in Utils::PermitHold is called and returns a list of events,
if any, that prevent the copy from filling this hold.  This list is
checked against the oargs parameter, and at the first match, the event
list is cleared and success is returned.

The trouble with that should be obvious, but in case it isn't, I'll
explain.  The list from the permit_copy_hold function may be a
superset of the oargs argument, i.e. it could contain events not in
oargs.  If it does contain events not in oargs as well as at least 1
event in oargs, success is returned, even though failure should be
returned since there are failures beyond the one (or more) that we
wish to or are allowed to override.

Additionally, if oargs is equal to "{ all => 1 }" as is the case
whenever open-ils.circ.title_hold.is_possible is called without the
oargs parameter, then the checks in verify_copy_for_hold always fail.
Using the override option will not help in this case as the "{ all =>
1 }" is never fleshed out to a list of the requestor's override
permissions before being handed to verify_copy_for_hold.  Since there
is no list of events in the oargs parameter in this case, there is no
way for verify_copy_for_hold to know what events it can override, so
we get an automatic failure if any events are returned, regardless of
whether the requestor can override them all or not.

In short, verify_copy_for_hold will sometimes allow a copy to fill a
hold that shouldn't and will sometimes disallow a copy for a hold when
it should allow it.

As verify_copy_for_hold is used by all of the hold verification
functions, the above problem affects all holds, except for FORCE and
RECALL holds.  (FORCE and RECALL holds don't bother to do any
verification.  If the requestor has the permission, the copy hold is
placed.)

Being a logic error at the heart of hold verification, the above
problem is bad enough by itself.  However, it is not the only issue
with the hold test and placement chain.

The other tests for whether or not a hold can be placed are split
between open-ils.circ.title_hold.is_possible and
open-ils.circ.hold.create.  The apparent problem with this is that
title_hold.is_possible can return success when hold.create would
return failure.  There are tests in hold.create that do not exist in
title_hold.is_possible.  In fact, the intersection of the tests from
both functions is the empty set, so this is good in the sense that it
avoids duplication.  It is bad in the sense that the (misnamed)
title_hold.is_possible function does not check for every failure
possiblity when it probably should.  There are no comments or obvious
indications in the code for the checks that hold.create runs being in
that function and not in title_hold.is_possible's implementation.
Admittedly, they do use objects that are retrieved/created in
hold.create and not in title_hold.is_possible, but that same
information is available in title_hold.is_possible, albeit in a
slightly different form.  This rules out implementation quirks as a
requirement for those checks being in hold.create and not in
title_hold.is_possible.--If anyone knows a good reason that the tests
were split in the way that they were, please speak up.

Another minor issue arises out of the above.  The
open-ils.circ.title_hold.is_possible method is misnamed.  It actually
takes a parameter for the hold type and will check any type of hold,
not just title holds.  Again, this is a minor point, but it should
probably be renamed open-ils.circ.hold.is_possible to better reflect
what it actually does.

There are some other details of various functions that could use some
attention, but for the most part they don't impinge on the holds
functionality, so we'll leave them for another time.

If you've bothered to read this far, you are probably thinking, "It's
great that you've identified these problems, but what do you propose
as a fix?"

The verify_copy_for_hold function is going to need to have its logic
repaired.  It should also take into account when given the oargs with
the value of {all => 1}.

The proposal for fixing the logic is to simply remove those events
from the set returned by OpenILS::Utils::PemitHold::permit_copy that
are in the oargs events list.  This will then return an empty list
when the user can override all of the events, and will otherewise
return the list of those events that they cannot override.

To handle the {all => 1} situation, verify_copy_for_hold should look
up the uesr's override permissions for each event that is returned by
OpenILS::Utils::PemitHold::permit_copy. If the requestor has the
override permission, then the event is removed from the array ref
returned by OpenILS::Utils::PemitHold::permit_copy and added to the
events array ref in oargs.  In the opposite case, the event is left in
the arrary ref returned by OpenILS::Utils::PemitHold::permit_copy and
added to a list of "failed" events in oargs.  Subsequent calls to
verify_copy_for_hold will check the events and failed array refs in
oargs before looking up the requestor's permissions in the database.
This is of course, an optimization to spare unnecessary permission
lookups.

In order to make the above work properly for cases when the override
argument is not being used, all functions that create oargs and
subsequently call verify_copy_for_hold or another function that in
turn calls verify_copy_for_hold need to be checked so that the oargs
hash ref is only defined and/or used when the override option is
present.  The verify_copy_for_hold function has no other way to
determine if we're overriding at the moment, and I'd rather not change
its signature at this time.  It is easy enough to pass in an empty
hashref or an undef value.

To remedy the way that the permission checks are split between the
is_possible and create methods, I propose combining the functionality
of the two methods into a single subroutine.  The open-ils method
descriptions will be modified to point at the new subroutine.  This
function can determine if it is being run as the is_possible or the
create method.  If run as the is_possible method, it will simply do
all of the combined checks from the two separate methods and return
whatever value the existing is_possible method returns.  If called as
the create method, it will do the same checks as it would as the
is_possible method, and then place the hold if the checks pass or fail
to place the hold.  In this case, of course, the return values of the
existing create method will be maintained.

Along the way, I'd like to add open-ils.circ.hold.is_possible as a
method to replace open-ils.circ.title_hold.is_possible.  I think the
latter should be deprecated and scheduled for removal at some point in
the future.  It should not, of course, be removed right away because
too much client code at the moment uses it.

Given the merging of functionality between the is_possible and create
methods, it would logically make little sense to call is_possible
before the create method if your goal is to actually place a hold.
However, I think there maybe enough cases where you simply wish to see
if a hold placement would succeed and then not actually place the hold
that the is_possible method should stick around on the public API.

The above is my plan for addressing some holds issues that have lately
been a problem for our membership.  As always, comments and
suggestions are welcome!


-- 
Jason Stephenson
Assistant Director for Technology Services
Merrimack Valley Library Consortium
Chief Bug Wrangler, Evergreen ILS