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

[James Fournie]

Hey Jason,

I've have some feedback on part of your proposal.

Looking at Launchpad bug 1064651, I discovered that
test_and_create_hold_batch() calls both title_hold.is_possible and
open-ils.circ.hold.create.  It calls the former to check if a hold is
possible and the latter to create the hold, as one would predict.
title_hold.is_possible returns a depth value in some cases, which I don't
think is useful for a method of that name.  I've made a small fix for that
but it's not ideal and based on this issue, I'd support combining the
circ.hold.create and title_hold.is_possible into a single more concise
method and keeping around the is_possible for legacy purposes.

~James

On Fri, Dec 7, 2012 at 7:18 AM, Jason Stephenson <jstephenson at mvlc.org>wrote:

> While looking into Launchpad Bug 1076062
> (https://bugs.launchpad.net/**evergreen/+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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://libmail.georgialibraries.org/pipermail/open-ils-dev/attachments/20121218/bb2e1708/attachment-0001.htm>