Suspend Holds at time of placement

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

Adding initial review target.

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

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.

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

I have a patch for this based on master here:

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

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

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.

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.

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

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.

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.

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.

Galen pointed out to me in IRC (http://irc.evergreen-ils.org/evergreen/2017-05-24#i_306319) 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.)

So, I've been working on this and testing some options today. I decided not to use a HTML5 date input because there's not hint in Firefox that you're supposed to enter a date. It would be necessary for current Firefox users to give them a hint and specify the date format. This would be useless for Chrome, Edge, and Safari users. I don't want to get into the position of guessing what features are available based on the user's reported browser and version.

I understand that Firefox nightly builds recently had this feature turned on, but Firefox 55 has not been released, yet. Even after Firefox 55 is released it will be a while before it is in general use. At this point, I think we should wait before using the HTML5 date input element.

I am willing to be persuaded otherwise, if anyone feels strongly about it.

Thanks Jason!

I have a couple of questions about the HTML5 date input to make sure I'm understanding the issue correctly.

My understanding that in the browsers that support this element, the date format that is entered matches the locale of the user's device. However, the actual date sent to the server is in a standard format, no matter what format is displayed/entered in the browser. Is that correct?

In the case of Firefox, then, where we don't have a date picker, in what format is the date sent to the server? Does it send the date in one standard format no matter how the date is entered in the form? Or does it send the date to the server in the format in which it was entered?

The answer to that question gives me a better idea of how I feel about using the HTML5 date input before full support is available in Firefox.

Kathy, the HTML5 date input element sends YYYY-MM-DD to the server in browsers that support the element.

The text element, which Firefox < 55.0 (for "released" versions of Firefox) treats the date element as, sends whatever the user puts in with no validation. Thus, we'd have to do all kinds of validation on the backend to figure out if it is a date and what format it might be in, which would be impossible for some input dates. Ex. 12/07/17 could Dec. 7, 2017, or July 12, 2017, or July 17, 2012. Even a four digit year leaves you guessing between the first two.

We have at least one other place in the OPAC where we use similar language with a text input element for a date: in the My Account holds list when suspending them. My suggestion is that we wait until Firefox 55.0 is in general release and has been available for a couple of months before we start using the HTML5 date input element. That would likely put that back to Evergreen 3.1 release in the spring of 2018.

My WIP branch (mentioned in a previous comment) does the same thing at the My Account hold suspension code with the date provided by the user. It assumes the MM/DD/YYYY format, and if the data doesn't match that format, then the thaw date is not entered.

A note about Firefox, if you use nightly Firefox builds or the Firefox Developer Edition, then the date input element has been available since release 51.0, but has to be activated from about:config. AFAIK, it has not been available in a general release build of Firefox. The fact that it is enabled by default in the Firefox 55.0 nightly build leads me to believe that it will be enabled in the final Firefox 55.0 general release.

As I mentioned in my previous comment, I also don't want to add a lot of code to check for the user's browser and alter the features that are available or how they are presented. We haven't done that anywhere else in tpac that I can find, and I think it would be a bad precedent.

One final note, we will never have a date input element in the XUL staff client unless we add a JQuery date element.

OK, thanks. I just wasn't sure if Firefox had some minimal support for the date input that would make it workable, but if it's falling back to a text input element, then I agree it's better to hold off.

I would love to see it in 3.1, but looking at your final comment regarding the XUL client, I'm wondering if we should wait until XUL support is entirely removed, which would be 3.2.

I've reviewed this code, and it works well for us. I have added my signoff to:

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

I also added a release notes entry.

I'm holding off on merging the code so that others uninvolved in this development have an opportunity to review it.

Pushed to master, along with two follow-ups:

* avoid crashing if an invalid thaw date (e.g., 13/02/2017) is specified (though we really do need to stop kicking that l10n can down the road; I wish Firefox hadn't been so intransigent, and would be open to proposals to add a decent polyfill datepicker for 3.0)
* normalize the capitalization of the onclick attribute

Thanks, Bill, Jason, and Kathy!