Test notification method

[Kathy Lussier]

It would be useful to have a way to easily test a patron's e-mail or SMS notification method to make sure it works.

We would like an enhancement that adds buttons in the patron editor and in the "My Account" section of the public catalog that allows staff and patrons to test their email and SMS notification.

Full requirements are available at http://masslnc.org/node/3154.

The MassLNC Evergreen Development Initiative is working with Catalyte on this enhancement.

[Kyle Huckins (khuckins)]

Looking for some community input here, is there any reason we don't open up the SendEmail method(and other related methods) to be usable without action triggers? I understand there may be some benefits to using action triggers when it comes to logging messages that aren't sending, but are there any other benefits to this outside of having a configurable delay(which may be useful in some cases)? My thought process here is that we could just call the API for an instant or near-instant notification, but any pushback would be welcome.

[Jeff Davis (jdavis-sitka)]

In theory, by using action triggers, staff can review all past or pending notifications in a single, consistent place in the client, without having to ask a system administrator.

You can also disable notifications, if necessary, by disabling the action_trigger_runner.pl --run-pending action as appropriate. This can be useful if you need to take your mail system offline temporarily, or if you have a test environment where you don't want notifications to go out. This is harder to control when EG sends emails without going through the a/t system.

At Sitka we have literally hundreds of SendEmail a/t's and we run pending events on them every 2 minutes without any trouble. That's close enough to instant for our purposes.

[Bill Erickson (berick)]

It's also possible to get real time A/T by calling the A/T API directly. What's more, you can tell A/T to process specific events by ID. The new code could create the test email event, then immediately tell A/T to process the event by ID. See the 'open-ils.trigger.event.fire' API.

[Kyle Huckins (khuckins)]

Bill,

If I'm understanding that right, we'd need to create the event, presumably using 'open-ils.trigger.event.autocreate.by_definition', and follow up by retrieving the new event's ID and calling 'open-ils.trigger.event.fire'. I'm running into an issue where calling the event.autocreate series of APIs returns a payload without a statuscode, which makes opensrf.js cranky. The APIs seem to return undef in the code. The payload does have a hash of the event's information when it goes through opensrf.js, but the event isn't created, I'm guessing because of the lack of a statuscode.

[Kyle Huckins (khuckins)]

I've pushed up a WIP branch here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1777677-test-notification-method

Ideally, I'd like to leverage the fire_object_event utility method, as I'm basically replicating the core aspects of it right now. The fire_test_notification method is returning as a successful event, but the osrferror log shows a severe query error when checking for the org.bounced_emails setting, where it's not finding what I'm assuming is the org id. This error also appears when I utilize fire_object_event, and I'm not entirely sure why it's not picking up the org id. I'm also unsure as to where or why this check is being made. As of now, the SQL file to create the new entries in event_hooks and event_definition are nonexistent, but the entries are present on my environment:
Hook: key: au.email.test, core_type: au, description: "A test email has been requested for a user.", passive: false
Definition: active: true, owner: 1, name: "Email Test Notification", Hook: au.email.test, Validator: NOOP_True, reactor: SendEmail, delay_interval: 00:01:00, template: https://gist.github.com/khuckins/4f8994064a3669e4d0bce8853ed675ba

The particular error I'm running into is:
2018-09-17 16:38:10 kcls-greenlandic open-ils.cstore: [ERR :32145:oils_sql.c:5898:15372022753212029] open-ils.cstore: Error with query [SELECT * FROM actor.org_unit_ancestor_setting( 'org.bounced_emails', '' ) AS "actor.org_unit_ancestor_setting" ;]: 3484946 3484946: ERROR: invalid input syntax for integer: ""
LINE 1: ....org_unit_ancestor_setting( 'org.bounced_emails', '' ) AS "a...
                                                             ^

[Kyle Huckins (khuckins)]

I've determined the above error was due to issues with the email template. I've since resolved those, should have some sql files pushed up soon. I've added a new commit to the branch that properly leverages $U->fire_object_event, and seems to be almost working. The issue now, I'm suspecting, is that it's not properly grabbing the org unit's email address. The patron.home_ou field doesn't seem to be fleshing, although I've attempted to manually flesh it. The event state returns as "error," though no errors appear in the logs.

[Kyle Huckins (khuckins)]

Additional commit made with the sql upgrade file, and my latest attempt at fleshing the target patron's home org unit. It does flesh, as evidenced by the logger lines I've placed just after fleshing, however it seems to unflesh prior to the event firing, resulting in an error related to the bounced_emails library setting(attempting to pass in target.home_ou.id when target.home_ou is the unfleshed id). If "helpers.get_org_setting(target.home_ou.id, 'org.bounced_emails')" is removed from the from lines of the templates, the event will fire with an error state, though the output's is_error field will be false. None of the information relating to the library will be visible, and editing the template to show the value of [% lib %] will display the id.

This branch hasn't been updated to the latest master just yet, though Intend to do so before pushing the completed branch.

[Bill Erickson (berick)]

Kyle, I believe you need to add some environment variables to your new event definitions. See for example: select * from action_trigger.environment where event_def = 1;

Those define the in-template fleshing behavior. The API itself does not need to flesh the user object.

Also, the open-ils.actor.event.test_notification should check a permission (e.g. UPDATE_USER at patron->home_ou) to prevent abuse. And I suggest it exit early with $e->event if the $$args{target} does not refer to a valid user to avoid unnecessary processing.

[Bill Erickson (berick)]

... sanity check on $$args{event_def_type} needed as well.

[Kyle Huckins (khuckins)]

Thanks Bill, that helped quite a bit! As a general update on the issue's progress, I've updated the branch, and rebased to the latest master. It's still WIP, but the SMS and Email notification events are firing successfully from the patron edit screen. Next steps are to fire the action trigger event from the OPAC, and to determine where to implement the message customization.

[Kyle Huckins (khuckins)]

A couple updates added to the branch. I'm having some issues on the OPAC side of things, specifically with the login session being either timed out or non-existent. My guess is that it's because I'm not passing through an authtoken, but I'm not 100% on that.

This is the commit for the OPAC additions: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=6b30221874caf7168613b6acfee75c3003a9c732;hp=53a0e8567f960b30a53c108ec9156e72b2c510dc

I'm wondering if anyone can take a look at the code and let me know if I'm missing anything obvious here.

[Kyle Huckins (khuckins)]

I was able to figure out the problem, and have since squashed and pushed my commits. So it doesn't get lost, the branch can be found here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1777677-test-notification-method

On the Patron Edit side of things, as long as the patron has an email address or default sms number, the notification event should created and fired when the requisite button is pressed, with a toast appearing if successful.

On the opac side, if the patron has an email address or default sms number, pressing the link next to the respective field will, on success, display a new row underneath the default mobile number/email address row, displaying a message that the notification has been sent, and containing the branch name, phone number, and email address of the patron's home org unit.

[Kathy Lussier (klussier)]

Thanks Kyle! I like the feature! I found a few issues in testing.

1. When in the patron editor, the test button immediately appears after adding an email address or SMS number for the patron. However, if the record has not yet been saved with those changes, the notification will be sent with nothing in the "To" line. Could that "Test" button only appear after the email address or SMS number has been saved to the user's record?

2. When testing the notification method in the public catalog, I was only get it to get it to work when the logged-in user was a staff user. When I was logged in with a public user Concerto account, which has fewer permissions, the action trigger event is never created. Is there a permission to this feature that would prevent it from working for public users?

3. In the public catalog, the test notification action for the SMS number is using the email test event, not the SMS test event.

4. When clicking the action to send an email or SMS test message, the screen shifts in a way that makes it difficult to see the resulting confirmation message. See https://drive.google.com/file/d/1vuNg6TX37vhNtHl8dUVYoWVvOH-fb5ri/view I think users may click the link multiple times before they realize the message was sent. I'm not sure of the best way to handle this. Maybe the confirmation could appear to the right of the email address instead of below it? Or even above the email address would make it more visible. Using red font for the confirmation message may also help.

5. I just have some suggested language changes. For the SMS message, I wouldn't use the term 'SMS' since most users don't know what this means. Maybe we could change it to say "Test text message / This is a test confirming your mobile number for [library name] is correct." For the confirmation message in the public catalog, I would prefer to see "Message Sent" instead of "Notification Sent." "If you do not receive and email message" or "If you do not receive a text message."

I could make the changes in #5 when signing off on the branch, but I'm putting the suggestions here in case the other issues aren't fixed until after I leave.

Thanks again!

[Kyle Huckins (khuckins)]

Thanks for the feedback Kathy! I've pushed an update to resolve issues 3 and 4, and rebased the code to the latest master. I'm currently looking into a solution for issue #1.

To address #2, currently the UPDATE_USER permission is being used. I'll take a look at the permissions currently available to a patron to see if there's a more preferred permission to use, unless there's one that's more ideal off the top of anyone's head.

[Kyle Huckins (khuckins)]

Another commit has been pushed addressing issue #1.

Re: Issue #2, I identified two permissions a regular user would have that initially seemed workable, OPAC_LOGIN and user_request.create. After some research it looks like user_request.create has to do with user requested acquisitions, but I'm not 100% sure on that. Does anyone in the community have any objections to using OPAC_LOGIN as a permission for this?

[Jane Sandberg (sandbergja)]

Thanks for your work on this, Kyle. Just wanted to let you know that this feature will need some release notes as well. Here's some how-to information about release notes: https://wiki.evergreen-ils.org/doku.php?id=contributing:release_notes

[Terran McCanna (tmccanna)]

Kyle - OPAC_LOGIN seems reasonable to me. I can't think of anyone that should be able to use this feature that wouldn't have that permission (at least, not in PINES).

[Kyle Huckins (khuckins)]

Thanks Jane and Terran. I've pushed an update resolving the 2nd and 5th issues Kathy brought up, as well as the addition of release notes, and with that, this feature should be ready for review.

[Lucien Kress (lkress)]

MassLNC testing complete, all OK from a functionality perspective.

[Terran McCanna (tmccanna)]

Thank you, Kyle! I was one of the testers as part of the MassLNC development initiative, so I will sign off:

I have tested this code and consent to signing off on it with my name, Terran McCanna and my email address, <email address hidden>.

[Chris Sharp (chrissharp123)]

I just removed the signedoff tag. As the "admin" user, I am able to send test SMS and Email messages as expected, but as a non-"admin" user with the required OPAC_LOGIN permission, I am prompted with a permission override box saying that I need OPAC_LOGIN to proceed.

[Chris Sharp (chrissharp123)]

See also some IRC discussion about some recommended security improvements for this feature:

http://irc.evergreen-ils.org/evergreen/2019-01-14#i_390612

[Kyle Huckins (khuckins)]

Thanks for the feedback! I've pushed a new commit taking into account the recommended security improvements. It appears to work on my end when testing with the admin user and a patron user.

[Terran McCanna (tmccanna)]

I've tested this on 3.2.3 and unless there are any other security improvements that need resolving, I am happy to sign off:

I have tested this code and consent to signing off on it with my name, Terran McCanna and my email address, <email address hidden>.

[Terran McCanna (tmccanna)]

Removing signoff because permission referenced in Actor.pm should be VIEW_USER and not USER_VIEW.

After updating that, works as expected.

[Terran McCanna (tmccanna)]

Also noticed that when sending the test email message from the OPAC, the on-screen confirmation message is very confusing to an end user. See screenshot.

[Terran McCanna (tmccanna)]

Looks like there is a typo in templates/opac/myopac/test_notification.tt2 - [% l(a'n email') %] should be [% l('an email') %].

[Terran McCanna (tmccanna)]

I consent to signing off on Kyle's work along with two typo fixes included here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mccanna/lp1777677_test_notification_method

[Chris Sharp (chrissharp123)]

Fixed-up, rebased branch with all the appropriate signoffs (including mine) located here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1777677-test-notification-method

[Jeff Godin (jgodin)]

I think the latest branch still has at least one of the issues raised in the Jan 14 IRC conversation linked above in comment 22.

The most recent branch still appears to permit passing an arbitrary hook to OpenILS::Application::AppUtils::fire_object_event

I'll try to take a look this week.

[Jeff Godin (jgodin)]

Confirmed with Terran that she did not intend to remain the assignee, assigned myself.

[Galen Charlton (gmc)]

Because the pending issue from comment 22 is a potential security issue, setting to needsrepatch.

Jeff, do you expect to have cycles soon to finish your review?

[Jeff Davis (jdavis-sitka)]

Working branch user/jeffdavis/lp1777677-test-notification-method-restrict-hooks builds on csharp's last branch by restricting open-ils.actor.event.test_notification to the appropriate hooks, thereby hopefully resolving the outstanding issue from comments 22 and 30:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jeffdavis/lp1777677-test-notification-method-restrict-hooks

This ought to be rebased to master and re-tested.

[Galen Charlton (gmc)]

I've pushed a rebase to

user/gmcharlt/lp1777677_test_notification /
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/gmcharlt/lp1777677_test_notification

The branch also contains a follow-up tweaking the test notification buttons on the patron registration form:

    * moves the 'Send Test Email' button in the patron registration form
      to cuddle next to the 'Invalidate' button
    * disables the 'Send Test Email' button if the email address is
      changed on the form
    * disables the 'Send Test Text' button if the SMS carrier or
      SMS text number is changed on the form

    The point of the last two changes is to prevent staff members
    from sending a test message prior to saving changes to the email
    address or SMS number, as otherwise the test methods would not
    have access to the new values.

I would appreciate review of that follow-up, but once that's done, I am otherwise prepared to merge this.

[Ruth Frasur Davis (redavis)]

Galen, has there been your requested review of this yet?

[Galen Charlton (gmc)]

Ruth: not yet.

[Galen Charlton (gmc)]

Given the relatively small size of my follow-up, I've gone ahead and merged this to master for inclusion in 3.6. Thanks, Kyle, Terran, Chris, and Jeff!