User can select SMS notify without providing a valid address

[Michael Peters]

With the introduction of SMS notifications in TPAC, we've discovered it is possible for users to deslect phone/email notices and select SMS notify, all without providing a valid SMS address.

We think users assume having their cell phone number 'on file' is sufficient, when in reality it isn't. This is resulting in frustrated customers who never get a notification.

For a sys-admin's side of this, you end up with emails attempted with no "to" address, as shown below, and they're all grouped together, making things a little tricky when trying to uncover why a user didn't get a message.

Example mashed template_output:
 From: <email address hidden>
 To:
 Subject: 142 hold(s) ready

 Hello Martha! The item you placed on hold is available for pickup at Kendallville Public Library - Kendallville.
 The item will be held for 1 week.
 Hello Kristin! The item you placed on hold is available for pickup at Kendallville Public Library - Kendallville.
 The item will be held for 1 week.
 Hello BARBARA! The item you placed on hold is available for pickup at Melton Public Library - Melton.
 The item will be held for 1 week.
 Hello MICHAEL! The item you placed on hold is available for pickup at Plainfield-Guilford Township Public Library.
 The item will be held for 1 week.
 Hello Lawrence! The item you placed on hold is available for pickup at Kendallville Public Library - Kendallville.
 The item will be held for 1 week.
 Hello TANYA! The item you placed on hold is available for pickup at Kendallville Public Library - Kendallville.
 The item will be held for 1 week.
 Hello RONALD! The item you placed on hold is available for pickup at Melton Public Library - Melton.
 The item will be held for 1 week.
 Hello Lawrence! The item you placed on hold is available for pickup at Kendallville Public Library - Kendallville.
 The item will be held for 1 week.
 Hello Kristin! The item you placed on hold is available for pickup at Kendallville Public Library - Kendallville.
 The item will be held for 1 week.
 Hello HERITAGE HOUSE! The item you placed on hold is available for pickup at Shelby County Public Library - Shelby County.
 The item will be held for 1 week.
 Hello MIRIAM! The item you placed on hold is available for pickup at Greensburg-Decatur County Public Library - Greensburg.

[Thomas Berezansky (tsbere)]

Note that this is likely due to the "Processing Group Context Field" being sms_notify and not usr...though for text messages I think we shouldn't be grouping at all and thus should probably set that to null.

[Kathy Lussier (klussier)]

Even with the grouping turned off, I think the system should stop the user if they try to select SMS as a notify option and don't provide the SMS address. As Michael mentioned in the original report, it results in frustrated customer, and changing the grouping option doesn't address that core issue.

Since some users must pay for every text message they receive, I think the system should group the messages when they can.

[Thomas Berezansky (tsbere)]

The system doesn't stop you from selecting phone with no phone number either.

Also, I don't see this in MVLC's system. We do have "check_sms_notify" set to 1 in the event def's Event Parameters though. Perhaps that is what is missing elsewhere?

[Galen Charlton (gmc)]

We've also observed cases where the patron has entered their SMS number but not specified their carrier, which also leads to notifications not being sent out.

[Josh Stompro (u-launchpad-stompro-org)]

We are also seeing multiple instances a day when users don't select their carrier even though they enter their cell phone number in the SMS notification area. Right now they are leaving it as the default, which is the first entry alphabetically.

I would like to propose that the carrier dropdown be set to a blank entry or a "Choose Carrier" entry when no user default is set. Along with not allowing the hold to be placed unless both pieces of data are entered.

Is javascript form validation frowned upon for something like this, or would that be a reasonable route?
Josh

[Michele Morgan (mmorgan)]

+1 to a blank value or "Choose Carrier" for the SMS carrier dropdown.

Another problem with selecting SMS notification is that the user can fill in carrier and text number (or they can be filled in from the user's notification preferences), but the SMS notification checkbox can remain unchecked, resulting in no SMS notification stored in the hold.

One approach to addressing this would be to automatically check the checkbox when carrier and text number is filled in.

Another approach would be some styling to draw the user's attention to the fact that, though the carrier and number are filled in, SMS notification is not enabled. For example hiding or greying out the carrier and number values when the checkbox is not checked, or an alert warning the user that the checkbox needs to be checked to activate SMS notification. This approach would allow users to save their carrier and number in their notification preferences to make it easily enabled when placing a hold, but not have it active by default for every hold.

[Michele Morgan (mmorgan)]

Adding a note that bug 1669534 addresses the behavior where the carrier defaults to the first one on the list.

[Cesar V (cesardv)]

For related bug 166953, I've added a blank <option> tag to the SMS carriers dropdown on OPAC holds UI, so that it defaults to that, if none is already saved for that patron.

To address this bug, I've refactored some plain vanilla JS form validation that keeps compatibility with existing code and checks to make sure that if any method of hold alert/notification is checked, that the user cannot proceed unless they enter the needed Phone/SMS number (or "address" as it's been referred to in the report title). It gives user feedback using a yellow background color highlight on the culprit/missing field.

Code is in user/cesardv/hold_notification_bugs

[Cesar V (cesardv)]

Above comment should read: For related bug 1669534 ...

[Cesar V (cesardv)]

Added a better fix for this that uses HTML5 input required attributes, while also falling back to IE 8-compatible plain JS validation.

That fix lives at /user/cesardv/hold_notify_html5_formval

[Jason Etheridge (phasefx)]

Cesar, the form validation appears for a given notice type after you manually click the corresponding checkbox, but does not work if the page is simply loaded with a users notification preferences. For example, under My Account -> Account Preferences -> Notification Preferences, if the user enables "Notify by Text by default when a hold is ready for pickup?" but does not select a carrier and number at that time, then the Place Hold screen will have "Yes, by text messaging" checked by default, but no validation occurs.

My other comment is about the fallback Javascript for IE8. The string "Please complete hold notification method info." is not internationalized. For IE8, I'm not too worried about that, but it is a wart.

Thanks!

[Cesar V (cesardv)]

Jason, my bad... I was rather confusion/ambiguous as to what branch I was intending to actually submit, and I think you might have tested this using the incorrect branch...

So the code for the alternative html5 version of the fix is not quite kosher, and I guess is still experimental since not all browsers support the "required" input attribute. (http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/cesardv/hold_notify_html5_formval)
Don't test/merge it...

The code I do what to do a PR on is the user/cesardv/hold_notification_bugs (http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/cesardv/hold_notification_bugs) branch, which fixed the issue using plain vanilla JS form validation.

I just retested that branch and it seems to be working correctly for me... at this point though the branch itself is a little stale so I'm going to rebase with master and force push, so make it easier to test. Can you delete your local version of branch and repull and retest when you get the chance? Thanks!

[Chris Sharp (chrissharp123)]

I tested the fix for this bug in the omnibus branch (http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/cesardv/hold_notification_bugs) but it wasn't working for me. After rebasing the branch to master and installing it on my test box, I'm still able to select the "Notify by Text by default when a hold is ready for pickup?" checkbox and save without any sort of prompt to enter the number. Let me know if I'm doing something wrong :-).

I did sign off on the other fix in this branch that has not already been pushed to master (see bug 1669534), FYI.

[Galen Charlton (gmc)]

The patch had fallen prey to an unexpected effect of commit 9619a1b and its dropping want_dojo being on all the time.

I've adjusted and rebased it (and added the cache key stuff); new branch is user/gmcharlt/lp1098685_rebase.

[Chris Sharp (chrissharp123)]

Okay, I wasn't testing the right screen. I can confirm that the patch works when setting notification preferences when placing a hold, but it doesn't affect the Notification Preferences screen within the OPAC view of the patron account. Since setting preferences there results in the same problem, shouldn't that be included in the scope of this? Or should we consider the hold placement notification preferences a separate bug?

[Galen Charlton (gmc)]

I think it should be considered a separate bug. I agree that it would be good for the notification preferences page to, if the user sets an SMS number, ensure that the user also selects their carrier, but if they fail to do that, the current patch will still catch their mistake.

[Chris Sharp (chrissharp123)]

Okay, here's the signoff branch:

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

I'll update here when I open the other bug.

[Galen Charlton (gmc)]

Pushed to master and backported to rel_2_12. Thanks, Cesar and Chris!

[Cesar V (cesardv)]

Chris, I've pushed a branch to address the other bug, in the myOPAC user notification prefs... per your comment above (#17)... There was a html typo prevent the dropdown from working right there.

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

Wasn't sure you if you had created a new LP bug/report for it or not... so I'm holding off from writing one up myself, in an effort to avoid duplicates...

But if you haven't created one yet, let me know I can take care of it!

[Cesar V (cesardv)]

Went ahead and created https://bugs.launchpad.net/evergreen/+bug/1710972 to address issues in my last comment.

[Chris Sharp (chrissharp123)]

Cesar - thank you for taking care of that - I had completely forgotten! - Chris

[Cesar V (cesardv)]

Submitted branch user/cesardv/lp1098685_2_11_backport to working/evergree repo, to be reviewed and backported to 2.11. I tested 2.11 on my dev machine and did not see any issues. I did have to find a cherry-pick merge conflict on Open-ILS/src/templates/opac/parts/js.tt2 but it was expected. Everything else was pretty smooth.

[Jason Etheridge (phasefx)]

Works for me, pushed to rel_2_11. Thanks!