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

(dev/mail/6) On multilingual mode, choosing mailing group doesn't affect recipient count and list #11906

Merged
merged 1 commit into from Apr 1, 2018
Merged

(dev/mail/6) On multilingual mode, choosing mailing group doesn't affect recipient count and list #11906

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 2 additions & 20 deletions CRM/Mailing/BAO/Mailing.php
View file
Open in desktop
Expand Up @@ -128,8 +128,9 @@ public static function getRecipients($mailingID) {
$recipientsGroup = $excludeSmartGroupIDs = $includeSmartGroupIDs = $priorMailingIDs = array();
$dao = CRM_Utils_SQL_Select::from('civicrm_mailing_group')
->select('GROUP_CONCAT(entity_id SEPARATOR ",") as group_ids, group_type, entity_table')
->where('mailing_id = #mailing_id AND entity_table IN ("civicrm_group", "civicrm_mailing")')
->where('mailing_id = #mailing_id AND entity_table IN ("!groupTableName", "civicrm_mailing")')
Copy link
Member

@totten totten Apr 2, 2018 •
edited

Choose a reason for hiding this comment

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

@monishdeb @seamuslee001 I have no mechanism for demonstrating the frequency/distribution in real-world scenarios, but the list of valid values for civicrm_mailing_group.entity_table allows any mix of civicrm_group, civicrm_group_en_US, civicrm_group_fr_FR, civicrm_mailing -- e.g. one might get a mix by:

  • Interleaving steps of (a) composing/reusing mailings and (b) toggling multilingual support.
  • Composing/previewing/delivering messages with different users (and different default locales).
  • Enabling integrations/extensions that work with mailing APIs (but aren't clever about multilingual)

Shouldn't this really be a pattern-match?

->where('mailing_id = #mailing_id AND entity_table RLIKE "^civicrm_(group.*|mailing)$"')

(Aside: The data model is evil.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that could work. I also wonder do we need the AND entity_table IN etc if we can only have 2 types of values groups and mailings and we are including both here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably figure out how we can add appropriate testing for these things since otherwise we are always gonna hit more

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh right, so using RLIKE pattern will be flexible enough to tackle mix of group table name values (i.e. w/o locale part the case on toggling multilingual mode). Will create a separate PR with this fix also with UT to use getRecipients() on multilingual test env.

Copy link
Contributor

Choose a reason for hiding this comment

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

@monishdeb I wonder what we could do to see what happens when we run our WHOLE SUITE in another lingua

Copy link
Member Author

Choose a reason for hiding this comment

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

@eileenmcnaughton I have submitted a PR #11924 in this regard. Extended a UT to ensure that switching to multilingual mode in and then reusing the existing mail draft won't affect recipient list building.

->groupBy(array('group_type', 'entity_table'))
->param('!groupTableName', CRM_Contact_BAO_Group::getTableName())
->param('#mailing_id', $mailingID)
->execute();
while ($dao->fetch()) {
Expand Down Expand Up @@ -2878,25 +2879,6 @@ public static function getMailingsList($isSMS = FALSE) {
return $list;
}

/**
* @param int $mid
*
* @return null|string
*/
public static function hiddenMailingGroup($mid) {
$sql = "
SELECT g.id
FROM civicrm_mailing m
INNER JOIN civicrm_mailing_group mg ON mg.mailing_id = m.id
INNER JOIN civicrm_group g ON mg.entity_id = g.id AND mg.entity_table = 'civicrm_group'
WHERE g.is_hidden = 1
AND mg.group_type = 'Include'
AND m.id = %1
";
$params = array(1 => array($mid, 'Integer'));
return CRM_Core_DAO::singleValueQuery($sql, $params);
}

/**
* wrapper for ajax activity selector.
*
Expand Down