Suspend Holds at time of placement

Bug #1189989 reported by Dawn Dale on 2013-06-11
28
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

Before our upgrade to EG 2.3.6 patrons were able to "suspend" a hold at the time of placing the hold. This option was available on the place hold screen. The option is no longer available on the place hold screen. We would like to ask that it be added back. The goal is for the patron to place a hold on an item and set the activation date so that the hold will be placed at a later date. Some patrons view this as a way to schedule their holds.

Thanks, Dawn

Ben Shum (bshum) on 2013-07-25
tags: added: holds opac tpac
Changed in evergreen:
importance: Undecided → Wishlist
status: New → Confirmed
Bill Ott (bott) wrote :

I created a basic patch that should do the job of allowing holds to be suspended at the time of placement.

tags: added: pullrequest
Ben Shum (bshum) wrote :

Adding initial review target.

Changed in evergreen:
milestone: none → 2.next
Kathy Lussier (klussier) wrote :

Hi Bill,

Would you also be able to provide a release notes entry for this feature? See http://wiki
.evergreen-ils.org/doku.php?id=dev:meetings:2014-05-19 if your need any guidance on writing the release notes.

Thanks!
Kathy

tags: added: needsreleasenote
Kathy Lussier (klussier) on 2014-11-10
Changed in evergreen:
assignee: nobody → Terran McCanna (tmccanna)
Changed in evergreen:
assignee: Terran McCanna (tmccanna) → nobody
Terran McCanna (tmccanna) wrote :

Mobius set up a sandbox to test this on today's Community Bug Squashing Day. They got merge conflicts when trying to apply it to the current Master (2.7), so they tried applying it to a new instance of 2.5.1 instead. The patch applied successfully, and when I tested placing holds through both the OPAC and the staff client the Suspend Hold options showed up, but when I try to place a hold (regardless of hold options selected) I just get an Internal Server Error.

I'm not sure which version this patch was developed for, but it looks like it might need to be tweaked a little to work with 2.7.

Bill Erickson (berick) wrote :

Removing pullrequest tag, pending release notes and a patch compatible with current Evergreen (preferably posted to the working repository).

tags: removed: pullrequest

user/stompro/LP1189989_Suspend_Hold_At_Placement

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

Working branch added, rebased to master and added release notes.

Josh

tags: added: pullrequest
removed: needsreleasenote

Tested on 8/24/2015 version of master on Debian Jessie, see the same internal server error that Terran mentioned.

Errors in logs:
==> open-ils.circ_stderr.log <==
Caught error from 'run' method: No field by the name in Fieldmapper::action::hold_request! at /usr/local/share/perl/5.20.2/OpenILS/Utils/Fieldmapper.pm line 272.

I was trying to track down the error, by commenting out the changes in Account.pm and adding bits back in... and I also ran autogen.sh again. But now I just un-commented all the changes in Account.pm, and it works fine.

So running autogen.sh again must have fixed it?

I'm trying it again now. And no go, I ran autogen.sh and the error was still there.

It's almost like it only works after one successful hold has already been placed.

I can get it to work by doing the following.
1. Install from master, start everything up run autogen.

2. Try to place a hold, get internal server error. (Not selecting suspend or thaw_date)

3. Edit "/usr/local/share/perl/5.20.2/OpenILS/WWW/EGCatLoader/Account.pm" and comment out line 1254 "thaw_date=> $thaw_date" and adjust the comma.

4. Restart opensrf services and apache. (Restarting with editing Account.pm doesn't change the outcome).

5. Place a hold, it now works.

6. Un-comment change I made to Account.pm and adjust the comma.

7. Restart opensrf services and apache.

8. Place a suspended hold with a thaw date, now it works correctly, no error.

I tried a different user and placed a suspended hold and that account also with no trouble. I'm not sure how to track this down.

Also, as an enhancement to this, if the "Hold Sucessfullly placed" screen showed more info about the state of the hold, I think that would be useful. Like "Hold was successfully placed, in a suspended state to be thawed on 11/24/2015 and picked up at the example branch 1." To give the customer a confirmation that their choices were respected. But that is probably a new bug.

Galen Charlton (gmc) wrote :

From the patch:

+ my $thaw_date;
+ if ($cgi->param('hold_suspend') && $cgi->param('thaw_date') =~ m:^(\d{2})/(\d{2})/(\d{4})$:){
+ $thaw_date = "$3-$1-$2";
+ }

Noting that this is not a great way of parsing dates; it can't be localized, for one thing. I also note that there's at least one other place in Account.pm that does that, but at least it's marked with a TODO.

+ frozen => $cgi->param('hold_suspend'),

Of more import, this construct is problematic: because $cgi->param() is evaluated in list context, an attacker could add multiple instance instances of the hold_suspend URL parameter to inject unwanted keys and values into the parameter hash.

Kathy Lussier (klussier) wrote :

Based on Galen's comments, I'm going to remove the pullrequest tag from this one and add a needsrepatch tag.

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

I pushed a branch based on Bill Ott's patch and fixed the internal server error. My branch is at:

working/user/dyrcona/lp1189989-Add-suspend-option-when-placing-hold

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1189989-Add-suspend-option-when-placing-hold

I did not address Galen's concern about the data parsing in #10 as he notes that almost identical code is used elsewhere when suspending a hold. (I believe it is used by the holds edit functionality of the My Account holds list.)

I believe my change addresses his issue with key injection.

I've tested my branch by placing a suspended hold and a not suspended hold using concerto data. In that limited context it works. I thinks this would make a good base to build off of.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers