[OPEN-ILS-DEV] Possible Bugs in Circulate.pm (1.6)

[Dan Wells]

Bill,

Thanks again for taking a look at this.  Attached is a patch against trunk to formalize these suggested changes.

Dan


================================================
Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Signed-off-by: Dan Wells <dbw2 at calvin.edu>
================================================

>>> On 1/13/2010 at 10:03 AM, Bill Erickson <erickson at esilibrary.com> wrote:
> On Fri, Jan 8, 2010 at 5:32 PM, Dan Wells <dbw2 at calvin.edu> wrote:
> 
>> Hello all,
>>
>> With the help of Jason and Dan S., it was determined that an error our
>> library was getting when trying to circulate items with a status of
>> 'Reserves' only happens when using in-db circ rules.  After some
>> troubleshooting, I found at least two separate issues are at the root of
>> this.
>>
>> First, it seems that 'run_indb_circ_test()' does both patron checks and
>> copy checks in one swoop, so you end up getting the 'COPY_NOT_AVAILABLE'
>> permit failure when running 'run_patron_permit_scripts()'.  This particular
>> permit failure needs a "copy object" as its payload, but since
>> 'run_patron_permit_scripts()' is not expecting to deal with copy permits, it
>> doesn't fill it.
>>
>> I think we have a number of choices for a fix here.  The simplest to is
>> duplicate the code near the end of 'run_copy_permit_scripts()' and add it to
>> the end of 'run_patron_permit_scripts()':
>>
>>   for (@allevents) {
>>      $_->{payload} = $copy if
>>            ($_->{textcode} eq 'COPY_NOT_AVAILABLE');
>>   }
>>
>>
>> This solution works around the issue but doesn't really address the bigger
>> problem, which is that run_indb_circ_test() is returning non-patron level
>> failures to run_patron_permit_scripts().  In addition, run_indb_circ_test()
>> ends up getting called twice when it doesn't really need to be.  It seems
>> like these function calls in general should be reworked in some minor way,
>> and I am happy to do it, but it might be done more quickly and safely by
>> someone more familiar with the reach of these functions.
>>
> 
> 
> On the contrary, I think this is a fine solution.  One of the nice things
> about in-db circ is that it doesn't differentiate between patron and copy
> permit operations.  It just returns a set of failure reasons.  If handled
> properly, the caller gets more of a full picture of the problem and has a
> better idea of what he/she is overriding if an override is used.  What we
> have in place now for in-db circ is an imperfect blend of old and new and,
> as much as possible, I'd like to move toward the new without breaking the
> old.  If that means adding more objects (in this case, the copy) to the
> failure events for consistency, then I think we should add them.
> 
> run_indb_circ_test() does get called twice as part of this blend of old and
> new, but the routine exits early if it's already been called.  IOW, it's not
> actually going to the DB more than once.
> 
> I'm not saying the code couldn't use some streamlining, but I'm not sure now
> is the time (more below).
> 
> 
>>
>> Aside from this, there is another difference in the code which ends up
>> causing further trouble for the in-db folk.  If you use legacy scripts, your
>> $self-copy is built inside of mk_script_runner(), but if you are using
>> in-db, $self->copy is built by mk_env().  It turns out that these "copy"
>> variables are not being created the same.  More specifically, mk_env()
>> leaves out a number of things, status information included.  This can be
>> fixed by changing (within mk_env()):
>>
>>    unless($self->is_noncat) {
>>        my $copy;
>>            my $flesh = {
>>                    flesh => 2,
>>                    flesh_fields => {acp => ['call_number'], acn =>
>> ['record']}
>>            };
>>
>> to:
>>
>>    unless($self->is_noncat) {
>>        my $copy;
>>            my $flesh = {
>>                    flesh => 2,
>>                    flesh_fields => {acp => ['location', 'status',
>> 'circ_lib', 'age_protect', 'call_number'], acn => ['record']}
>>            };
>>
>> I am not sure about needing all of that information, but this matches the
>> object as it is built in mk_script_runner().
>>
>>
> mk_env() fetches less data because using in-db circ means it doesn't need
> all of that data.  However, if the staff client is expecting those fields to
> be fleshed, then I'm all in favor of adding them.  Also, when we add support
> for script callbacks (in-db circ rule "falls through" to a JS script for the
> final decision), we'll want that data to be available in the copy.
> 
> As far as general reworking goes, I would like to suggest that we get
> support for script callbacks in place then pick a release to drop support
> for legacy circ scripts.  Once that decision is made and the relevant branch
> exists, there is a lot of cleanup we could do to Circulate.pm.
> 
> Hope this helps.  Thanks, Dan!
> 
> -b
> 
> -- 
> Bill Erickson
> | VP, Software Development & Integration
> | Equinox Software, Inc. / The Evergreen Experts
> | phone: 877-OPEN-ILS (673-6457)
> | email: erickson at esilibrary.com 
> | web: http://esilibrary.com 
> 
> Please come by and visit the Equinox team
> and learn more about Evergreen
> ALA MidWinter
> January 15-18, 2010
> booth # 2064
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Circulate.pm.patch
Url: http://libmail.georgialibraries.org/pipermail/open-ils-dev/attachments/20100113/f3080672/attachment.txt