CRM-15536 Scheduled reminders: Clarify "Limit to" and "Also include" UI to prevent user errors

    Details

    • Type: Sub-task
    • Status: Done/Fixed
    • Priority: Minor
    • Resolution: Fixed/Completed
    • Affects Version/s: 4.5
    • Fix Version/s: 4.6
    • Component/s: Core CiviCRM
    • Labels:
    • Documentation Required?:
      User and Admin Doc

      Description

      1. Add an empty option to the limit_to form field - and have that be the default for new reminders for all entities except Activities. The field is not a required field for Events, Memberships, Contacts - so in those cases the default should be <option value="">- neither -</option>

      2. For all entities except Activities, change the label for that field to 'Limit or Add Recipients'. Hide #recipient select field if limit_to value is empty.

      3. Add form_rule to validate that user has selected > 0 recipients (manual or Group ID), or participant roles if "Limit to" or "Also include" are selected. Form rule error message is:

      "If "Also include" or "Limit to" are selected, you must specify at least one recipient."

      4. Change DB default for limit_to from 1 to NULL (schema and upgrade)

        Attachments

        1. erroneousFormRuleError.PNG
          80 kB
          David Greenberg
        2. LabelChangeMockup.PNG
          65 kB
          David Greenberg
        3. RecipientsFieldMissing.PNG
          55 kB
          David Greenberg
        4. RemoveOR_ifAbsoluteDate.PNG
          18 kB
          David Greenberg
        5. Screen Shot 2014-11-13 at 5.23.14 PM.PNG
          41 kB
          David Greenberg
        6. Screen Shot 2014-11-13 at 5.37.15 PM.PNG
          140 kB
          David Greenberg
        7. wrongHelpIcon.PNG
          81 kB
          David Greenberg

          Issue Links

            Activity

            [CRM-15536] Scheduled reminders: Clarify "Limit to" and "Also include" UI to prevent user errors
            David Greenberg added a comment -

            PR is stale (only due to conflict in 4.6.alpha1.mysql.tpl) - so you'll need to rebase.

            I found the following issues when testing:
            1. Limit to / Also include => Participant Role is not working. Limit to = throws a form rule error even though I have a role in the select2 box. Also include = doesn't throw a form rule error, but doesn't save the selected Participant Role(s).

            2. Typo (missing space) in the form rule error: "you must specify atleast one participant role"

            3. Create new scheduled reminder. Switch entity from Activity to Membership. Switch back to Activity.

            • the Recipients label is present but the input field is not restored (still hidden).

            4. We need a separate inline help tag for 'Recipients' field since it's only used for Activities and does not use the 'Limit to'/'Also include' widget. (I can write the help for this once I have a new PR to apply.)

            5. The 'OR' text should be hidden (along with the relative date fields) when user enters Absolute Date.

            Monish Deb added a comment - - edited

            Dave - I have updated the PR and covered 4 of the issues, except #4.

            However I found a strange behavior on formRule validation/edit of schedule reminder. Go to Add Schedule Reminder(SR) page and choose any entity Activity or etc. Simply submit to get validation error and after the page loads you will see the entity dropdown(id=#entity_0) has only one option that you last selected. Same on existing SR edit page. So is this a expected behaivour or need to fix that making other entity options available all the time ?

            David Greenberg added a comment - - edited

            I'm not seeing that formRule entity_id issue you've described. Screenshot attached showing formRule error message and entity dropdown still has all the options ???

            I merged the PR so it doesn't get stale.

            One small issue to clear up, the 'OR' string is still visible when EDITING a Scheduled Reminder with absolute date. (screenshot attached as well).

            Regarding #5, looking at ScheduleReminders.tpl I don't see a way to have 2 different

            {help ...}

            tags for the recipient field. If you know of a way, let me know. Else I'll 'expand' the help text to cover both cases.

            Monish Deb added a comment -

            Covered all the issue, including entity dropdown break and this is the PR https://github.com/civicrm/civicrm-core/pull/4557

            #4 - For now I have left the help text ('...') for recipients, to be filled.

            In addition through the changes I have added a flexibility to provide class parameter to helpicon as very useful in certain case where helpicons can be enabled/disabled based on class.

            David Greenberg added a comment -

            Nice improvement adding class to help icons!
            I merge your last PR and also merged a PR w/ additions to .hlp for recipient help.
            https://github.com/civicrm/civicrm-core/pull/4561

            Still 2 problems in my testing:
            1. When entity = Event *, and using Limit to OR Also include => Participant Role - the roles are NOT being saved. This is a recurrence of a problem from as few days back.

            2. When entity != Activity, the wrong help icon is shown next to 'limit_to' field initially. It should be the 'Limit of Add Recipients' help, but it's the 'Recipients' help. That help should show only after limit_to is changed to non-null value - and should follow the recipients dropdown. (The proper help icons are shown in the correct spot AFTER I select Limit to OR Also include).

            Monish Deb added a comment - - edited

            Submitted the PR https://github.com/civicrm/civicrm-core/pull/4563

            #1 was a regression due to deleting those code, to set $mappingID. Fixed at BAO level and ensured its safety.

            David Greenberg added a comment -

            Latest version of PR fixes previously reported issues as well as problem with sending reminders to event participants when they have > 1 participant role (fatal SQL error was occurring).

            Joanne Chester added a comment - - edited

            This only seems to be half fixed.

            If you set Limit or Add Recipients to "LImit to" and don't actually choose any recipients (group or manually entered) then you can't save the scheduled reminder and get the error message "If "Also include" or "Limit to" are selected, you must specify at least one recipient."

            However if you set Limit or Add Recipients to "Also Include" and don't actually choose any recipients (group or manually entered) then you CAN save the scheduled reminder and perhaps the error of sending to everyone in the database will still occur.

            Monish Deb added a comment - - edited

            Joanne,

            1) We enforced the formrule validation on choosing 'Limit To' >> null manual recipient/group because we cannot limit the recipient to null/reminder to no one. So the choice has been made mandatory. You can found the changes in https://github.com/civicrm/civicrm-core/pull/4508/files#diff-e179caac63281aad4b5df3aab2e370e2R289 against master. So thats expected behaivour, don't ya think so?

            2) Ya I agree the problem still persist in master/4.6 as the changes were made in 4.5 CRM-15500 and not present in master. Will investigate and send a new PR for this against master.

            Thanks,
            Monish

            Joanne Chester added a comment -

            Yes, I think the form rule validation and message on choosing "Limit to" is correct.

            I reopened it because I thought the same was meant to happen on choosing "Also include" and it doesn't.

            Monish Deb added a comment -

            Thanks Joanne for highlighting this Submitted the PR https://github.com/civicrm/civicrm-core/pull/4997

            David Greenberg added a comment -

            Additional form validation for 'Also Include' working as expected. Thanks Joanne for catching this!

            Joe Murray added a comment -

            The Description doesn't seem to capture a use case one of my clients just experienced. I think there was a misunderstanding of what 'Addition to' versus 'Limited to' means.

            One of their staff configured a scheduled reminder to be sent to people associated with a specific event and selected Recipients = 'Addition to', 'Participant Role' with Recipient Listing of

            {Attendee, Host, Speaker}

            . Should have been Recipients Limited To. The result was thousands of reminder emails sent, including to people who have never registered for an event, probably to the whole database.

            I don't even really understand what a good use case would be to sending a reminder email to an unlimited set of those not in a particular role. It's obviously very dangerous.

            It seems to me that anytime a Scheduled Reminder is about an event, only participants in any status should be eligible to receive an email, no matter the combination of 'Addition to' and 'Participant Role'. It is too dangerous to provide possibility of sending to all contacts in db on a form that is somewhat confusing.

            Joe Murray added a comment -

            Dave has asked me to confirm behaviour on a 4.6 install. Still relevant that this be backported to LTS - I see one of Eileen's clients was bitten on a 4.4 install.

            David Greenberg added a comment -

            My understanding is that the fix in https://issues.civicrm.org/jira/browse/CRM-15500 prevents the 'send to all contacts' behavior regardless of options selected, and it has been backported to LTS.

            Joanne Chester added a comment -
            Joanne Chester added a comment - - edited

            I think the only options that make sense for "Also include" are Choose Recipient(s) or Select Group.

            I am not sure what Also include - Participant role would do, but it sounds like either it would send to every person with that role for the event template/Name/type even if their participant status doesn't match the one you have specified or, more alarmingly, it might send to every person who had ever held the role you specified for any event in civicrm.

            I agree with Joe in that I can't actually see when being able to also include a participant role would be useful and the potential for near disaster is too great.

            Would it make sense to remove the option Participant Role whenever 'Also include" is chosen?

            David Greenberg added a comment -

            I've verified that Also Include => Participant Role DOES NOT have any affect on the recipient list since it does NOT send to participants whose status doesn't match the status selected above.

            Let's modify the dynamically populated 'recipient' dropdown so that it only offers the 'Participant Role' option when the 'limit_to' value is "Limit to".

            ====

            NOTE (for other folks watching this issue): Selecting Also Include => Participant Role does NOT cause participants in that role in other events or any other wider set of contacts to receive notifications based on my testing in 4.6. I think the original idea was that some participant roles might be assigned a special status (e.g. 'Speaker' role / 'Complementary' status) and this would be a way of including them. However, I think this is an unlikely use case and it doesn't work currently in any case.

            Monish Deb added a comment -
            David Greenberg added a comment -

            The fix to remove Participant Role works great and I've merged it. However I'm getting Notices after edit + save of a scheduled reminder IF I open the edit form in a new tab (rather than the default 'edit as pop-up form' mode). Can you investigate please:

            • Notice: Array to string conversion in HTML_QuickForm_Rule_Required->validate() (line 63 of/Users/dgg/git/crm_v4.6/packages/HTML/QuickForm/Rule/Required.php).

            Monish Deb added a comment -

              People

              • Assignee:
                David Greenberg
                Reporter:
                David Greenberg

                Dates

                • Created:
                  Updated:
                  Resolved: