Skip to content

CRM-21779 - ang\crmMailing - Fix adding empty mailing to recipients field #11724

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

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

lemacarl
Copy link
Contributor

@lemacarl lemacarl commented Feb 26, 2018

Overview

This PR prevents CiviMail from adding mailing lists including draft and unscheduled mailings that do not have any recipients

Before

crm-21779

After

crm-21779 2

Technical Details

Add chain rule to Mailing getlist api call to include MailingRecipients getcount then filter civicrm_mailing options by api.MailingRecipients.getcount

Comments

Anything else you would like the reviewer to note


@totten
Copy link
Member

totten commented Mar 2, 2018

I haven't really reviewed this, but on a policy/usability level -- @lemacarl this seems like a good idea.

if('civicrm_mailing' === rcpAjaxState.entity) {
params["api.MailingRecipients.getcount"] = {};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the above if condition, can this be removed and adjusted in the above "case" section? Something like -

var filterParams = recipients = {}; //Initialize.
.
.
case 'civicrm_mailing':
  filterParams = { is_hidden: 0, is_active: 1 };
  recipients["api.MailingRecipients.getcount"] = {}; //Add chain api param
  break;
}
var params = {
  input: input,
  page_num: rcpAjaxState.page_i,
  params: filterParams,
};
$.extend(params, recipients); //Merge to the main param

@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented Jun 8, 2018

@eileenmcnaughton @totten This is working as expected on the mailing screen. Those having empty recipients are removed from the dropdown list correctly. Though there is a minor scope of improvement from code POV, but does not seem necessary to block the merging of this PR.

@colemanw colemanw merged commit 573259d into civicrm:master Jun 8, 2018
@eileenmcnaughton
Copy link
Contributor

thanks @jitendrapurohit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants