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
- erroneousFormRuleError.PNG
- 80 kB
- David Greenberg
- LabelChangeMockup.PNG
- 65 kB
- David Greenberg
- RecipientsFieldMissing.PNG
- 55 kB
- David Greenberg
- RemoveOR_ifAbsoluteDate.PNG
- 18 kB
- David Greenberg
- Screen Shot 2014-11-13 at 5.23.14 PM.PNG
- 41 kB
- David Greenberg
- Screen Shot 2014-11-13 at 5.37.15 PM.PNG
- 140 kB
- David Greenberg
- wrongHelpIcon.PNG
- 81 kB
- David Greenberg
Issue Links
- supplements
-
CRM-15500 Scheduled reminders: if 'Also include' and 'Choose recipients' is selected, and no recipients are entered - reminder is sent to all contacts
-
- Done/Fixed
-
Activity
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.
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 ?
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.
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.
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).
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.
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).
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.
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
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.
Thanks Joanne for highlighting this Submitted the PR https://github.com/civicrm/civicrm-core/pull/4997
Additional form validation for 'Also Include' working as expected. Thanks Joanne for catching this!
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.
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.
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.
A tangential issue - https://issues.civicrm.org/jira/browse/CRM-16094
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?
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.
Submitted the PR https://github.com/civicrm/civicrm-core/pull/5421
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).
Submitted the PR https://github.com/civicrm/civicrm-core/pull/5477
https://github.com/civicrm/civicrm-core/pull/4508