renewing lost items fails unintuitively

[James Fournie]

when attempting to renew a lost item, you get the error message "ACTION_CIRCULATION_NOT_FOUND" -- this is very unintuitive. Reviewing the code, this same error would be produced on attempting to renew a LONGOVERDUE or CLAIMSRETURNED item.

[Ben Shum (bshum)]

Confirmed to affect our 2.0 systems (someone finally tried to renew a LOST transaction today).

[Jason Stephenson (jstephenson)]

2.0 is definitely not seeing a fix for this.

If no one confirms it in 2.1, 2.2 or master, I vote for Won't Fix.

[Ben Shum (bshum)]

Confirmed to occur in our 2.2 systems.

[Jason Stephenson (jstephenson)]

I've targeted the bug at 2.2 and 2.1. The target at master more or less covers 2.3 at this point.

I've left it confirmed in master and set to confirmed in 2.2. Since no one has confirmed it in 2.1, I'm leaving it at New there. However, I am confident it is a bug in 2.1 since it was confirmed in 2.0.

I'm not targeting this at any milestones yet because I don't expect it to be fixed before the next milestone releases.

[Jason Stephenson (jstephenson)]

I can confirm this in master as of yesterday, so it affects 2.3 also.

I'll take a crack at it.

[Jason Stephenson (jstephenson)]

Looks like it fails to find the circulation because the renewal code is looking for a circ with xact_finish of NULL and a stop fines reason of NULL or max fines.

I just wonder if there should be an arbitrary list of stop fines reasons that we look for in the code or if it should be configurable somehow?

[Thomas Berezansky (tsbere)]

Creating a table of stop fines reasons with booleans for things like "Renewable" might not be a bad thing.

As for the code, perhaps it should look for non-finished circs, but complain (rather than erroring) when the stop fines reason isn't in the approved list? Then the error can say "This was LOST/CLAIMSRETURNED/whatever" and possibly allow an override.

[Dan Scott (denials)]

Happens when the stop_fines reason is MAXFINES too (commenting to increase retrievability).

[Dan Scott (denials)]

Also of note: as a workaround, staff can just check in the item, then check it back out again. Not perfect, but hey, it's a workaround.

[Jason Stephenson (jstephenson)]

I am working on this bug under the auspices of:

https://blueprints.launchpad.net/evergreen/+spec/billing-enhancements

For more details see

http://www.sigio.com/evergreen/billing2013.html#renewal

[Jason Stephenson (jstephenson)]

In looking at/fixing this I see there are two basic issues.

#1 is that the backend excluded circulations with a reason other than MAXFINES in its search for a circulation to renew. This caused claims returned, lost, etc. circulation to give the circulation not found error. The first commit in the branch below addresses this issue.

#2 is that the renewal code does not currently do an override. When looking at the code in circ/util.js in the staff client, it looks like the intention was to have the renewal code use open-ils.circ.renew.override instead of just open-ils.circ.renew, because a long of override paramenters are passed in with the call to renew. The second commit on the branch below addresses this by making all renewal calls use the .override variant. It also adds two more overridable event (COPY_STATUS_LOST and COPY_STATUS_LONG_OVERDUE) to the list of requested overrides.

I am not adding the pullrequest tag just yet, I think the people who contracted this work ought to have a chance to review it before it goes in.

I also think this is more of a bug than a wishlist item after seeing what I see in the code.

Anyway, here is the code:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp638509

[Kathy Lussier (klussier)]

There were too remaining issues I found when testing Jason's code:

- If a library had enabled the "Lost Checkin Generates New Overdue" setting, the renewal wasn't generating those new settings. I had created two parallel transactions that were set to Lost. One transaction was checked in a couple of days after setting it to lost and the overdues were generated. The other transaction was renewed on the same day, but the overdues were not generated.

- When renewing an item that had been set to Claims Returned, the system was not restoring fines.

[Jason Etheridge (phasefx)]

Just a comment, the intention in the staff client was not to automatically call the .override variant of the renewal method. It should offer an override prompt if one of the listed events is returned.

[Jason Stephenson (jstephenson)]

Well, it doesn't (even without this patch), and even if it did, the override version is never called.

[Jason Etheridge (phasefx)]

I do get an override prompt for MAX_RENEWALS_REACHED, and it makes the override call when confirmed.

PATRON_BARRED works as well.

Are there specific events not working? Should we separate that out into its own bug?

[Jason Stephenson (jstephenson)]

All right. I'm not sure if any in particular don't work, so I rebased out the addition of .override to CHECKOUT_RENEW. There has also been some slight drift from master in circ/utils.js, so I resolved that, too

I pushed to a new branch under collab: collab/dyrcona/lp638509.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dyrcona/lp638509

If no one objects, I'd like to remove the original branch under user to avoid confusion.

I also wonder if the last two commits on this branch really belong here. They were part of MassLNC specs for fixing this bug, but I now think they should be treated as a separate issue. I have no objection if the consensus is to rebase them out as well.

[Jason Stephenson (jstephenson)]

In response to Kathy's concerns about fines, I think that should be orthogonal to the fix for this bug. It was part of MassLNC requirements, but not a requirement to fixing this bug.

I'm going to rebase on master, drop the code that messes with fines from this branch, and throw it over the wall for testing again.

[Jason Stephenson (jstephenson)]

Absolute latest code force pushed to the original branch: working/user/dyrcona/lp638509.

The collab branch was deleted.

[Kathy Lussier (klussier)]

Testing the new code, I find that we can now renew lost items without error. With the new code, the fines also seem to be handled the same way as if this transaction were a checkin, which addresses the issue I discovered above with new overdue fines not being applied when the lost item is renewed.

I can file a wishlist bug for the claims returned behavior we would like to see.

Signoff branch is available at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp638509

Thanks Jason!

[Ben Shum (bshum)]

Pushed to master, rel_2_8, and rel_2_7.