SMS functionality generates unnecessary event churn

Bug #1096209 reported by Dan Wells on 2013-01-04
16
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
3.3
Undecided
Unassigned
3.4
Medium
Unassigned
3.5
Medium
Unassigned

Bug Description

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.

Revision history for this message
Thomas Berezansky (tsbere) wrote :

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.

Revision history for this message
Lebbeous Fogle-Weekley (lebbeous) wrote :

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

Revision history for this message
Thomas Berezansky (tsbere) wrote :

To clarify: the validator improvement should already be there.

Changed in evergreen:
status: New → Triaged
Ben Shum (bshum) on 2013-03-03
Changed in evergreen:
milestone: 2.4.0-alpha1 → 2.4.0-beta
Ben Shum (bshum) on 2013-03-17
Changed in evergreen:
milestone: 2.4.0-beta → 2.4.0-rc
Ben Shum (bshum) on 2013-04-27
Changed in evergreen:
milestone: 2.4.0-rc → none
Ben Shum (bshum) on 2013-08-22
no longer affects: evergreen/2.2
Revision history for this message
Chris Sharp (chrissharp123) wrote :

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!

Revision history for this message
Jeff Davis (jdavis-sitka) wrote :

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.

no longer affects: evergreen/2.3
no longer affects: evergreen/2.4
Jane Sandberg (sandbej) on 2019-08-21
tags: added: actiontrigger holds
Michele Morgan (mmorgan) on 2020-05-27
Changed in evergreen:
status: Triaged → Confirmed
Revision history for this message
Jeff Davis (jdavis-sitka) wrote :

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

Changed in evergreen:
milestone: none → 3.5.0
tags: added: pullrequest
Revision history for this message
Michele Morgan (mmorgan) wrote :

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.

Revision history for this message
Josh Stompro (u-launchpad-stompro-org) wrote :

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.

Changed in evergreen:
milestone: 3.5.0 → 3.5.1
Changed in evergreen:
importance: Undecided → Medium
Changed in evergreen:
milestone: 3.5.1 → 3.5.2
Revision history for this message
Jason Etheridge (phasefx) wrote :

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?

Revision history for this message
Chris Sharp (chrissharp123) wrote :

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.

tags: removed: pullrequest
tags: added: bitesize
Changed in evergreen:
milestone: 3.5.2 → 3.6.1
Changed in evergreen:
milestone: 3.6.1 → 3.6.2
Changed in evergreen:
milestone: 3.6.2 → 3.6.3
Changed in evergreen:
milestone: 3.6.3 → none
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers