Suspend Holds at time of placement

Bug #1189989 reported by Dawn Dale on 2013-06-11
This bug affects 7 people
Affects Status Importance Assigned to Milestone
Jason Stephenson

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 →
Kathy Lussier (klussier) wrote :

Hi Bill,

Would you also be able to provide a release notes entry for this feature? See http://wiki if your need any guidance on writing the release notes.


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


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


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/ line 272.

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

So running again must have fixed it?

I'm trying it again now. And no go, I ran 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/" and comment out line 1254 "thaw_date=> $thaw_date" and adjust the comma.

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

5. Place a hold, it now works.

6. Un-comment change I made to 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 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:


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.

Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
status: Confirmed → In Progress
Jason Stephenson (jstephenson) wrote :

I am taking Bill's original code and my fix for the internal server errors and using it to finish development of this feature as specified by MassLNC. The main additions to what has been done so far include a "Set activation date" link to toggle the appearance of the text box to be used for entering the date, the addition of a help button, and a text string to appear to the right of the text box to specify how to enter the date. All strings will be translatable. I am attaching two clips from a MassLNC mock up showing roughly what these changes will look like.

Jason Stephenson (jstephenson) wrote :

Here's the second attachment to show what it should look like with the text box activated. The "Set activation date" text in this layout may change to indicate that it will make the activation date box go away, perhaps "Do not set activation date." I am open to suggestions on the exact wording.

I am using a text box like the other date boxes in the OPAC for now. I think in the long run we should use the new HTML5 date entry boxes, supported by most browsers except for Firefox. However, that would mean changing all of our templates to HTML5 from XHTML 1.0. That is a bigger task than can be undertaken in this bug, and should be a bug (or bugs) of its own.

Jason Stephenson (jstephenson) wrote :

Galen pointed out to me in IRC ( that I was looking at the wrong base.tt2 file when I made the remarks about XHTML 1.0 and not being able to use the HTML5 date element in the OPAC in comment #14. It turns out that we do, indeed, use HTML5 in the OPAC, so I'm going to alter the plans a bit and try this with a date input type.

Firefox does not, yet, have full support for this widget but recent versions do appear to work rather well with it. At least, Firefox 53 recently showed a format hint inside the text box that a date was expected in the locale format. (Yes, I meant locale for the locale of the browser.)

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

Other bug subscribers