renewing lost items fails unintuitively

Bug #638509 reported by James Fournie on 2010-09-14
34
This bug affects 7 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
2.6
Medium
Unassigned
2.7
Medium
Unassigned
2.8
Medium
Unassigned

Bug Description

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) wrote :

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

Changed in evergreen:
status: Triaged → Confirmed
importance: Wishlist → Medium
Jason Stephenson (jstephenson) wrote :

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.

Changed in evergreen:
status: Confirmed → Triaged
importance: Medium → Undecided
Ben Shum (bshum) wrote :

Confirmed to occur in our 2.2 systems.

Changed in evergreen:
status: Triaged → Confirmed
importance: Undecided → Medium
Jason Stephenson (jstephenson) wrote :

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) wrote :

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

I'll take a crack at it.

Changed in evergreen:
milestone: none → 2.4.0-alpha
status: Confirmed → In Progress
assignee: nobody → Jason Stephenson (jstephenson)
Jason Stephenson (jstephenson) wrote :

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) wrote :

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.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
status: In Progress → Confirmed
no longer affects: evergreen/2.0
no longer affects: evergreen/2.1
Dan Scott (denials) wrote :

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

Dan Scott (denials) wrote :

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.

Ben Shum (bshum) on 2013-03-03
Changed in evergreen:
milestone: 2.4.0-alpha1 → 2.4.0-beta
Ben Shum (bshum) on 2013-03-17
Changed in evergreen:
milestone: 2.4.0-beta → 2.4.0-rc
Ben Shum (bshum) on 2013-04-27
Changed in evergreen:
milestone: 2.4.0-rc → none
Ben Shum (bshum) on 2013-08-22
no longer affects: evergreen/2.2
Changed in evergreen:
status: Confirmed → In Progress
assignee: nobody → Jason Stephenson (jstephenson)
Jason Stephenson (jstephenson) wrote :

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

Changed in evergreen:
status: In Progress → Confirmed
assignee: Jason Stephenson (jstephenson) → nobody
tags: added: circulation lost needsreleasenote override renew review
Changed in evergreen:
milestone: none → 2.6.0-rc1
Dan Wells (dbw2) on 2014-03-22
no longer affects: evergreen/2.3
Changed in evergreen:
milestone: 2.6.0-rc1 → 2.next
Kathy Lussier (klussier) wrote :

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) wrote :

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) wrote :

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

Jason Etheridge (phasefx) wrote :

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?

no longer affects: evergreen/2.5
no longer affects: evergreen/2.4
Jason Stephenson (jstephenson) wrote :

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) wrote :

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.

tags: added: pullrequest
removed: review
Jason Stephenson (jstephenson) wrote :

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

The collab branch was deleted.

Kathy Lussier (klussier) on 2015-06-04
Changed in evergreen:
assignee: nobody → Kathy Lussier (klussier)
Kathy Lussier (klussier) wrote :

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!

Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
tags: added: signedoff
Ben Shum (bshum) wrote :

Pushed to master, rel_2_8, and rel_2_7.

Changed in evergreen:
status: Confirmed → Fix Committed
milestone: 2.next → 2.9-alpha
Changed in evergreen:
milestone: 2.9-alpha → 2.9-beta
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers