Evergreen

hold override fails in the TPAC

Bug #1076062 reported by Jason Etheridge
24
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
High
Unassigned
Evergreen 2.4.0-alpha1
2.3
Fix Released
High
Unassigned
Evergreen 2.3.3

Bug Description

Example: Create a bib, add an item, make the item not holdable via the holdable attribute. Attempt a staff-placed hold on the title. On the hold failure/override screen, when you click Submit, activity happens on the server, but nothing on screen. The interface does not change. My contrived examples end up checking for the PLACE_UNFILLABLE_HOLD permission, which I have, but not for stuff like STAFF_CHR.override. There are Javascript console errors, that may be red herrings:

Timestamp: 11/7/2012 12:51:16 PM
Error: TypeError: document.getElementById("staff_barcode") is null
Source File: oils://remote/js/ui/default/opac/staff.js
Line: 33

Tags: pullrequest
Revision history for this message
Jason Etheridge (phasefx) wrote :

oh, meant to say that this is on master and 2.3.0

Thomas Berezansky (tsbere)
Changed in evergreen:
assignee: nobody → Thomas Berezansky (tsbere)
Revision history for this message
Ben Shum (bshum) wrote :

Confirmed as not working.

Changed in evergreen:
milestone: none → 2.3.2
status: New → Confirmed
importance: Undecided → Medium
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Taking this over from tsbere. It's our #1 priority and we want to have it fixed before we update again.

Changed in evergreen:
milestone: 2.3.2 → 2.4.0-alpha
importance: Medium → High
assignee: Thomas Berezansky (tsbere) → Jason Stephenson (jstephenson)
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Wow! There is no easy fix for this. It looks like event handling for holds and circ are going to need some changes, then major methods in Holds.pm rewritten, and eventually Circ.pm, too.

I'm going to drop the priority to Medium, since this shouldn't be a show stopper. (Did we agree High was a show stopper or not? I don't remember.)

Changed in evergreen:
importance: High → Medium
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Do we care if the fix breaks JSPAC? I suppose if it does, we can't backport to 2.3, which means 2.3 would stay broken.

I can try to fix it without breaking JSPAC, but that will make the fix take longer.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

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

Fixes it for me.

tags: added: pullrequest
Revision history for this message
Jason Stephenson (jstephenson) wrote :

And a backport branch:

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

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Set the importance against 2.4.0 at high. That release should not ship with this bug.

Changed in evergreen:
importance: Medium → High
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Keeping it assigned to me for now. My whitespace cleanup (Ctrl-Alt-\) added some lines because of the "electric" settings on braces. Some of these we might to keep, but some should be snugged up.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Cleaned up the white space clean ups and rebased.

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

and backported in

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

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Revision history for this message
Jason Stephenson (jstephenson) wrote :

I did it all without changing external API and without moving functionality around.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Rebased and forced pushed the branches above to the working repository.

Revision history for this message
Bill Erickson (berick) wrote :

I'm testing this in master with 'admin' using the methodology described (a record with one non-holdable copy). The initial hold placement fails with HIGH_LEVEL_HOLD_HAS_NO_COPIES as expected. The override attempt then fails with the same event, which puts me in an override loop, where each attempt fails the same way, consistent with the bug description.

Looking closer, it's unclear to me how this test would ever touch the new code to begin with, since the bulk of the new code lives in verify_copy_for_hold(), which is never called when _check_title_hold_is_possible() finds zero holdable copies.

I think maybe I'm testing the wrong thing and/or exposing a different bug entirely.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

If the asset.copy.holdable flag is false, then the fix does not come into play. You can't actually override that AFAICT.

The case we are mainly trying to address is when we have given age hold protection override to staff and/or patrons and that is the reason that the hold cannot be placed. There are a few other conditions that this bug fix would address, but age hold protection is the main one.

Revision history for this message
Bill Erickson (berick) wrote :

Ah, that makes more sense. I have confirmed the initial bug and the fix and I agree with the approach / code. Merging shortly..

Revision history for this message
Bill Erickson (berick) wrote :

Merged to master and rel_2_3. Thanks!

Changed in evergreen:
status: Confirmed → Fix Committed
Ben Shum (bshum)
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.