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.

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

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

Merged
merged 7 commits into from Dec 8, 2017

Conversation

Projects
None yet
5 participants
@jitendrapurohit
Contributor

jitendrapurohit commented Nov 1, 2017

…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

This comment has been minimized.

Contributor

MegaphoneJon commented Nov 1, 2017

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

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

@jitendrapurohit

This comment has been minimized.

Contributor

jitendrapurohit commented Nov 11, 2017

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

@jitendrapurohit

This comment has been minimized.

Contributor

jitendrapurohit commented Nov 20, 2017

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 from wip - CRM-21380: Add setting to block activity types from sending assignee … to CRM-21380: Add setting to block activity types from sending assignee … Nov 20, 2017

$('#filter_activity_type_notification').toggle();
$('#filter_activity_type_notification-desc').toggle();
});

This comment has been minimized.

@colemanw

colemanw Dec 2, 2017

Member

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

This comment has been minimized.

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 wip label Dec 2, 2017

@jitendrapurohit

This comment has been minimized.

Contributor

jitendrapurohit commented Dec 6, 2017

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

This comment has been minimized.

Member

colemanw commented Dec 6, 2017

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

@@ -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');

This comment has been minimized.

@colemanw

colemanw Dec 6, 2017

Member

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

This comment has been minimized.

@jitendrapurohit

jitendrapurohit Dec 7, 2017

Contributor

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

This comment has been minimized.

@jitendrapurohit

jitendrapurohit Dec 7, 2017

Contributor

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

CRM.buildCustomData( '{$customDataType}' );
{/if}
{literal}
var doNotNotifyAssigneeFor = ["{/literal}{'","'|implode:$doNotNotifyAssigneeFor}{literal}"];

This comment has been minimized.

@colemanw

colemanw Dec 7, 2017

Member

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.

This comment has been minimized.

@jitendrapurohit

jitendrapurohit Dec 8, 2017

Contributor

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

@colemanw

This comment has been minimized.

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

1 check passed

default Build finished.
Details

@jitendrapurohit jitendrapurohit deleted the jitendrapurohit:CRM-21380 branch Dec 18, 2017

@petednz

This comment has been minimized.

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 added a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018

Merge pull request civicrm#11222 from jitendrapurohit/CRM-21380
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