Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CRM-21380: Add setting to block activity types from sending assignee … #11222

Merged
merged 7 commits into from
Dec 8, 2017
Merged

CRM-21380: Add setting to block activity types from sending assignee … #11222

merged 7 commits into from
Dec 8, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Nov 1, 2017 •
edited
Loading

…notification

Overview

"Notify Activity Assignee" setting manages whether to send emails to the assignees or not.
This PR provides extra configuration to block emails for certain activity types even if the main setting is checked.

Before

No way to block some activity types from sending email to assignees.

After

New setting available on Display Preference Page to disable notification for some activity types.

image

Comments

As it adds a new setting, a menu rebuild is needed to test this.

@MegaphoneJon
Copy link
Contributor

Just to flag that a change to the Settings screen should be accompanied by a change to the documentation.

@jitendrapurohit jitendrapurohit changed the title CRM-21380: Add setting to block activity types from sending assignee … wip - CRM-21380: Add setting to block activity types from sending assignee … Nov 11, 2017
@jitendrapurohit
Copy link
Contributor Author

tagging as wip until a doc and a unit test is added.

@jitendrapurohit jitendrapurohit mentioned this pull request Nov 20, 2017
Merged
@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Nov 20, 2017 •
edited
Loading

Docs is being added here - civicrm/civicrm-user-guide#240. Also added a unit test to check for this setting. I've removed wip from the title now.

@jitendrapurohit jitendrapurohit changed the title wip - CRM-21380: Add setting to block activity types from sending assignee … CRM-21380: Add setting to block activity types from sending assignee … Nov 20, 2017
colemanw
colemanw reviewed Dec 2, 2017
View reviewed changes
templates/CRM/Admin/Form/Preferences/Display.tpl Outdated
$('#filter_activity_type_notification').toggle();
$('#filter_activity_type_notification-desc').toggle();
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This javascript is overly complicated and prone to failure, since the .toggle() function is being used without any arguments passed in, which means it could get out-of-sync with the checkbox. I'm also not a fan of how it hides the element then immediately shows it again, causing a potential annoying flash on page load.

Why is it necessary to target two elements filter_activity_type_notification and filter_activity_type_notification-desc instead of using a wrapper around both?

@colemanw
Copy link
Member

colemanw commented Dec 2, 2017

The UI looks confusing in the screenshot. It's not clear what "Blocked Activity Types" means or that it's related to the checkbox above. It's also not clear whether the activity types listed in the left box or the right box are "blocked."

I suggest the following changes:

  • Change label to "Do not notify assignees for"
  • Change widget to select2
  • Add help text saying "These activity types will be excluded from automated email notifications to assignees."

@colemanw colemanw added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Dec 2, 2017
@jitendrapurohit
Copy link
Contributor Author

Done all the suggested changes. The first commit of this PR actually contained the select2 widget as the element. It was changed to advmultiselect after we received a non-convinced message referring the UI interface on the Display Preference with the below screenshot.

image

I've updated the element to select2 with the huge class. Hope it looks good now :-).

@colemanw
Copy link
Member

colemanw commented Dec 6, 2017

That screenshot is a bit extreme. I think the intention is to only block a few activity types.

colemanw
colemanw requested changes Dec 6, 2017
View reviewed changes
CRM/Admin/Form/Preferences/Display.php Outdated
@@ -37,6 +37,7 @@
class CRM_Admin_Form_Preferences_Display extends CRM_Admin_Form_Preferences {
public function preProcess() {
CRM_Utils_System::setTitle(ts('Settings - Display Preferences'));
$optionValues = CRM_Activity_BAO_Activity::buildOptions('activity_type_id', 'validate');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you want to pass the 'validate' context here because then labels won't be shown properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the changes as per the above comment. Also handled the false display of notification description text(both activity and case activity forms) for activity types selected in do not notify setting. Thanks for the review @colemanw

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed to rebase before pushing the update which removed your commit. I've now included the js cleanup you did on this PR.

colemanw
colemanw requested changes Dec 7, 2017
View reviewed changes
templates/CRM/Case/Form/Activity.tpl Outdated
CRM.buildCustomData( '{$customDataType}' );
{/if}
{literal}
var doNotNotifyAssigneeFor = ["{/literal}{'","'|implode:$doNotNotifyAssigneeFor}{literal}"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think best practice is to use json_encode whenever assigning smarty variables to js vars. E.g.

var doNotNotifyAssigneeFor = {/literal}{$doNotNotifyAssigneeFor|@json_encode}{literal};

I know there's lots of examples in tpl files where we don't do that but I've fixed quite a few bugs caused by improper escaping and edge cases that were solved by using json_encode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated and tested the above json_encode assignment and it works fine 👍

@colemanw
Copy link
Member

colemanw commented Dec 8, 2017

Tested it out and all the JS is working as designed. The mail functionality comes with a unit test so I think we're good to go.

@colemanw colemanw merged commit 0cef192 into civicrm:master Dec 8, 2017
@jitendrapurohit jitendrapurohit deleted the CRM-21380 branch December 18, 2017 02:53
@petednz
Copy link

petednz commented Dec 18, 2017

@colemanw For the client who funded this, their requirement was that notifications are limited to a very few Activity Types - so the example may see 'extreme' but it is the actual use case this has been developed for

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
@colemanw @sluc23
Merge pull request civicrm#11222 from jitendrapurohit/CRM-21380
8dc3fb6 
CRM-21380: Add setting to block activity types from sending assignee …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colemanw colemanw colemanw approved these changes

Assignees
No one assigned
Labels
master needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jitendrapurohit @MegaphoneJon @colemanw @petednz @eileenmcnaughton