returning lost items does not void lost billing consistently

[Ben Shum]

Evergreen version: ~2.0.8
OpenSRF version: 2.0.0
PostgreSQL version: 8.4
Linux distribution: Ubuntu Lucid 10.04

Our system is configured with the library setting for "Circ: Void lost item billing when returned" set to TRUE for our entire consortium as a matter of policy. We also set "Circ: Restore overdues on lost item return" to TRUE for our consortium. There are A/T events set for each library to automatically MarkItemLost.

With all those settings in place, we've noticed that the system is not always respecting the voiding process when lost items are returned. The system definitely is marking the items lost based on the A/T schedule we've set. But then, when a lost item is returned at the library, we've found several cases where lost billings are not voided.

So far I've identified two cases with the examples I've got to work with:

1) The lost item is returned but the library has no overdues to restore and the lost bill is not voided.
2) The lost item is returned but no overdues are restored because the item was backdated on return and the lost bill is not voided, but the overdues stay voided instead of being restored.

My current hypothesis is that something may need to be checked within the circulate logic for checkins to see how it behaves with regards to lost item return and respecting various YAOUS.

[Jason Stephenson (jstephenson)]

Eg version: Master 20111023 + mvlc modifications
Pg version: 9.0
OS Version: Ubuntu 10.04 "Lucid Lynx"

I've seen this, too.

We got a report of a patron returning a copy owned by a different library in the consortium at his home library that had gone to lost. The billing was not voided even though the copy was checked in and the void lost billing on checkin setting is on for all of our members.

I did some experimentation with this patron's other checkouts. He had items from other libraries as well as his own. The billings were only voided when the copy was checked in first at the library that owns the copy (the copy.circ_lib). That is lost billings were voided when I checked in the copies from his home library at his home library. When logged in as his home library, the billings were not voided on copies owned by other libraries.

If I logged out and logged in as a library that owned the copy, and checked it in to complete the transit, the lost billing was still not voided.

If I checked a copy out to the patron from a library other than his home library, the billing was only voided when I logged in as the library that owns the copy and did the first checkin there.

Hope that makes sense.

Jason

[Ben Shum (bshum)]

Since we added a library this week that includes the "lost processing fee" charge, that too is among the lost billings NOT automatically removed from a patron's record upon the item's checkin.

Same general pattern, anything that was checkin and backdated caused the items not to void the lost billings.

[Jason Stephenson (jstephenson)]

What I found tonight is it seems to be an interplay between the void lost billings on checkin and lost items usable at checkin setting. Both of these have to be on at the owning library (or its ancestors) for the billings to be voided when the copy is checked in at a library that doesn't own the copy.--I think this is a bug, and I'll explain my reasons.

In looking at the code in master, the check for the lost usable at checkin setting's value is done in O::A::Circ::Circulate.pm at about line 3304. If that is false, and we're not at the library that owns the copy, we simply log a message to say that we're not changing the status to avoid problems when it goes home.

Following that is an elsif that checks for void lost billings. This only fires if the above check "fails."

Thus, it seems if we lost billings to clear without the copy going home, we set both the void lost billings and lost items usable at checkin ou settings.

If we want copies to go home before voiding lost billings, then we set lost items usable to false (or don't set it) and set void lost billings to true.

However, there is a catch. While the handle_lost routine doesn't alter the copy's status from lost under the latter conditions, the code that puts the copy into transit does change the copy's status from lost to in transit. When the copy arrives home, it is no longer lost, and therefore the code to handle lost and possibly void the billings never runs.

Two options present themselves to me immediately, and i'm sure when I'm more awake tomorrow more will pop up.

1. Is to set the lost items usable at checkin setting and forget about the "bug."

2. Is to make the check for void lost billings independent of the check for lost items becoming usable at checkin.

As for Ben's comment about backdated checkins, I don't really look how that comes into play because I am not aware that our members make heavy use of backdating. I will take a look at that tomorrow and see how it plays a role in this.

[Bill Ott (bott)]

On 11/16/2011 10:14 PM, Jason Stephenson wrote:
> What I found tonight is it seems to be an interplay between the void
> lost billings on checkin and lost items usable at checkin setting. Both
> of these have to be on at the owning library (or its ancestors) for the
> billings to be voided when the copy is checked in at a library that
> doesn't own the copy.--I think this is a bug, and I'll explain my
> reasons.
>
> In looking at the code in master, the check for the lost usable at
> checkin setting's value is done in O::A::Circ::Circulate.pm at about
> line 3304. If that is false, and we're not at the library that owns the
> copy, we simply log a message to say that we're not changing the status
> to avoid problems when it goes home.
>
> Following that is an elsif that checks for void lost billings. This only
> fires if the above check "fails."
>
> Thus, it seems if we lost billings to clear without the copy going home,
> we set both the void lost billings and lost items usable at checkin ou
> settings.

This all sounded very familiar, so I got in the way-back machine...

http://libmail.georgialibraries.org/pipermail/open-ils-dev/2009-March/004380.html

In some very old code, I believe these comments detail what you're
describing, circa line 2124:
    http://svn.open-ils.org/trac/ILS/changeset/12548

When I birthed LOST_IMMEDIATELY_AVAILABLE, I didn't consider the
ramifications, and we were never faced with it.

I'm not sure that this will help remedy the situation, but it may be the
point where the code started causing this issue.

[Jason Stephenson (jstephenson)]

Not to worry, Bill. You added a capability that was completely absent prior to that commit, so for that you are thanked.

I don't think LOST_IMMEDIATELY_AVAILABLE ever did what was intended given how the hold targeter and transit code work. I'm actually inclined to remove it, or at least ignore it my changes to address this bug. LOST copies won't target holds anyway, so the result of a checkin of a lost copy that is not at home would be to generate a transit and transit slip to send the copy home.

Of course, one could always check a copy with transits suppressed, but that's another issue beyond the scope of this bug.

[Jason Stephenson (jstephenson)]

Pushed a branch for master that removes the setting and makes it so that handle_lost always runs on a lost checkin regardless of the checkin lib/copy circ_lib being the same or different.

Branches for rel_2_1 and rel_2_0 to follow.

For sharing lp851000 via the remote repo working:

You need only add the remote once, regardless of how many branches you wish to look at.
To add this repo as working:
 Read-only (git protocol):
  git remote add working git://git.evergreen-ils.org/working/Evergreen
 Read-write (ssh protocol):
  git remote add working <email address hidden>:working/Evergreen

Once you have the remote added you can check out this branch:
git checkout -b lp851000 working/user/dyrcona/lp851000

[Jason Stephenson (jstephenson)]

Branch for rel_2_1:

For sharing lp851000_rel_2_1 via the remote repo working:

You need only add the remote once, regardless of how many branches you wish to look at.
To add this repo as working:
 Read-only (git protocol):
  git remote add working git://git.evergreen-ils.org/working/Evergreen.git
 Read-write (ssh protocol):
  git remote add working <email address hidden>:working/Evergreen.git

Once you have the remote added you can check out this branch:
git checkout -b lp851000_rel_2_1 working/user/dyrcona/lp851000_rel_2_1

[Jason Stephenson (jstephenson)]

Branch for rel_2_0 pushed:

For sharing lp851000_rel_2_0 via the remote repo working:

You need only add the remote once, regardless of how many branches you wish to look at.
To add this repo as working:
 Read-only (git protocol):
  git remote add working git://git.evergreen-ils.org/working/Evergreen.git
 Read-write (ssh protocol):
  git remote add working <email address hidden>:working/Evergreen.git

Once you have the remote added you can check out this branch:
git checkout -b lp851000_rel_2_0 working/user/dyrcona/lp851000_rel_2_0

[Dan Wells (dbw2)]

I have been looking over this branch, and I do not think the present solution does all that is required. LOST items, by default, are supposed to stay lost until they arrive at their home org unit. This is accomplished by storing the lost status in the transit and restoring it when it arrives at its home. The present branch appears to remove this feature.

It seems like what we want to do instead is run the 'checkin_handle_lost' method both where it is now (in master) and also in the 'transit' branch of do_checkin ( branch starting with '} elsif( $self->transit ) {' ), checking for the LOST status after the pre-transit status has been restored (and the subsequent event has been overridden).

Does this make sense? I am happy to work out a branch along these lines if requested, but I am also happy to let the current parties continue owning this bug if they want to keep working on it.

Thanks,
Dan

[Jason Stephenson (jstephenson)]

Dan,

From looking through the code, it doesn't really appear to me that the above ever really worked. It also seems unnecessary, since items that are LOST will not target holds, so even if the status is changed during checkin, the copy will still go home before it can go anywhere else.

However, if you want to start a new branch or take over this one to make it work the way you think it should, that's fine by me.

Jason

[Jason Stephenson (jstephenson)]

How about this:

I'll restore the setting that I deleted and add code to handle_lost so that the status of lost items is not changed unless the setting is set to true.

I'll then push the above to collab branches and anyone who feels like making additional changes can do so.

I think, however, that waiting until a lost copy to return home to void the billing is too much to ask. I know our members' staff don't get it. Of course, that "feature" wasn't working to begin with since the checkin code only looks at the copy's status at the time of checkin, which would be in transit.

[Jason Stephenson (jstephenson)]

Pushed a new collab branch against master:

working/collab/dyrcona/lp851000

Have fun!

[Dan Wells (dbw2)]

Hello Jason,

I am on board with your latest change. At first I wasn't sure about forgiving fines before transiting home, but I now agree that that is probably the most desirable behavior.

I have added one commit to your collab branch. It is mostly code comments in case this gets revisited, but there are also a few tweaks. The most significant is that I have retained the 'missing stays missing' behavior as it is in current releases, as I think that was probably just lost in the shuffle. I have also made the setting of the 'reshelving' status a bit more explicit in a few cases when the lost items are handled. This does not actually affect the behavior as things are, but I think it protects us against future surprises.

If you get a chance to check out the changes and approve, I will get this committed.

Thanks!
Dan

[Jason Stephenson (jstephenson)]

Dan,

Your changes look good and the branch functions how I'd expect given your changes.

Should I amend and sign off on your commit and push to the collab branch, though that requires a force push?

Also, do we have any intention of backporting this to 2.0 and 2.1 as I originally did?

Jason

[Dan Wells (dbw2)]

You are welcome to force push a signed commit if you wish, but given the small size of my commit, I am also fine with just knowing it was tested and pushing it into master as-is. I'll probably push to master tomorrow morning.

I do think this should be backported, but I expect it to be pretty simple in its current state. I will do the backporting when I do the committing and let you know if I hit any snags.

Thanks,
Dan

[Jason Stephenson (jstephenson)]

Thanks, Dan.

Force pushed a signoff on your commit.

[Dan Wells (dbw2)]

Fix committed and backported through rel_2_0. Thanks Jason!