SMS functionality generates unnecessary event churn

[Dan Wells]

EG 2.2+

The event definition named "Hold Ready for Pickup SMS Notification" is enabled by default, and this generates a pending event for every readied hold, even if SMS functionality is otherwise disabled. From what I can tell, this is generally harmless, as the end result seems to be an attempt to send an email for every hold with an invalid "To:" field.

My bigger concern is that this practice of generating "black hole" events has the potential to do large amounts of undesirable action, as it is simple to build up an enormous number of pending actions without being aware of it (we have), then manage to set them off in unforeseen ways (we thankfully did not).

Recommendation is to have the definition off by default, then document enabling of SMS notices as a two step process. Or is there more behind this decision to have this event definition enabled all the time? As an alternative to turning it off by default, we could also create a new validator which would honor the global "master" switch" for SMS hold notices.

[Thomas Berezansky (tsbere)]

There should be a validator that checks if SMS is enabled for the hold or not that silently flags those as invalid and moves on. Or, rather, the HoldIsAvailable validator should be doing that.

This requires a parameter for the event def that may not be there on upgrades currently, though....so it may only apply by default to clean installs after the validator changes were introduced.

[Lebbeous Fogle-Weekley (lebbeous)]

I can get behind both recommendations (event def disabled by default and validator improvement) with a +1.

[Thomas Berezansky (tsbere)]

To clarify: the validator improvement should already be there.

[Chris Sharp (chrissharp123)]

I can confirm that adding a new entry in Trigger Event Parameters (Parameter Name = check_sms_notify, Parameter Value = 1) results in exactly what Thomas describes in comment #1. This should have been done during/after an upgrade (per the comment in Open-ILS/src/sql/PG/upgrade/0683.hold_available_email_notify.sql:

-- NOT COVERED: Adding check_sms_notify to the proper trigger. It doesn't have a static id.

So anyone who ran the 2.1-2.2-upgrade-db.sql script will face this same issue. We should really RAISE NOTICEs about this kind of thing in upgrade scripts!

[Jeff Davis (jdavis-sitka)]

It looks like Sitka is affected by this issue.

If I understand correctly, any event definition with SendSMS as the reactor and HoldIsAvailable, HoldIsCancelled, or HoldNotifyCheck as the validator should have a check_sms_notify parameter. I wonder if we should add an upgrade script that adds the param, if it's missing, to all event definitions (including custom ones) that meet those criteria.

[Jeff Davis (jdavis-sitka)]

Working branch user/jeffdavis/lp1096209_check_sms_notify has an upgrade script that adds the check_sms_notify to the appropriate event defs, as suggested in my last comment:

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

[Michele Morgan (mmorgan)]

Jeff, thanks for the patch. I did a quick test of the theory by adding the parameter check_sms_notify with value 1 to an existing trigger with SendSMS reactor and HoldIsAvailable.

I still saw events being created when there were no sms parameters in the hold.

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

Michele, I don't think there is a way to prevent the events from being created, but with check_sms_notify set those events should be marked invalid when processed.

[Jason Etheridge (phasefx)]

Do we need to open a new bug for Jeff's patch? I think it should go in, unless there's a way for the Validator to know what the Reactor is and we can base the logic on that.

As for preventing the events altogether, could an A/T filter do that?

[Chris Sharp (chrissharp123)]

Per Jason's suggestion, created bug 1897146 to accommodate Jeff's patch. Since most sites probably applied it or something similar years ago, it may not affect much, but just to tie a bow on it, let's push it in.

Regarding the original report, I also agree with Jason that the answer would be a better action_trigger_filters.json entry that excludes patrons without an SMS number present. My JSON query chops are not up to that, but for someone with experience, this may actually be a bitesize bug.