(dev/mail/6) On multilingual mode, choosing mailing group doesn't affect recipient count and list #11906
Conversation
|
@seamuslee001 yes you are right, that's another bug we need to squash :D Will include the fix in this PR shortly. Thanks for pointing that out. |
|
@monishdeb does this need fixing as well Not sure about the from https://github.com/JMAConsulting/civicrm-core/blob/bb47825d98c7407314c72e1bcb5cdcca51e252bb/CRM/Mailing/BAO/Mailing.php#L2892 |
|
@seamuslee001 interesting fact - that function isn't used anywhere, heres what I got on grep only one occurence where this fucntion is declared not get called. This might be when some code dropped it's call. I think we should get rid of this function, what you say? |
|
@monishdeb i think you might be right looks like this was where it was originally called from https://github.com/fuzionnz/civicrm-svn/blob/4e48c95f7026d0a7930397189d4640d29c73bf6a/CRM/Mailing/Form/Group.php#L205 it might have changed when @totten did his angular work on CiviMail |
…ect recipient count and list
|
Done removed the fn |
|
Does this testing fail mean I shouldn't deploy this change in the production environment just yet? |
|
@gljk usually yes that's what the test build tells if the patch is safe to use. But in this case it doesn't seem to be case, as the failing unit-test isn't related and has been the case in other PRs in past, as I call. So let me rebuild the test build on this PR and I am pretty sure that it will pass in next build :) (I believe this patch is safe to deploy in prod.) |
|
Jenkins test this please |
|
Great! Tested and everything works just fine in real life conditions. |
|
I'm merging this based on @gljk testing & because the code seems clear cut & correct. I also did the grep test on the removed function |
| @@ -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")') | |||
There was a problem hiding this comment.
@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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should probably figure out how we can add appropriate testing for these things since otherwise we are always gonna hit more
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@monishdeb I wonder what we could do to see what happens when we run our WHOLE SUITE in another lingua
There was a problem hiding this comment.
@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.
Overview
Steps to reproduce
Mailing => New MailingPlease check https://lab.civicrm.org/dev/mail/issues/6
Before
After