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

[Bill Erickson]

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 HTML attachment was scrubbed...
URL: http://libmail.georgialibraries.org/pipermail/open-ils-dev/attachments/20100113/94330cc5/attachment.htm