[OPEN-ILS-DEV] Possible Bugs in Circulate.pm (1.6)
Bill Erickson
erickson at esilibrary.com
Fri Jan 15 16:08:37 EST 2010
Committed to trunk, rel_1_6, and rel_1_6_0.
Thanks, Dan.
-b
On Wed, Jan 13, 2010 at 3:05 PM, Dan Wells <dbw2 at calvin.edu> wrote:
> 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
>
--
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/20100115/00cf9744/attachment.htm
More information about the Open-ils-dev
mailing list