CRM-21316: Add missing code to handle smart group on sms mode #11558

monishdeb · 2018-01-19T17:51:55Z

Overview

This is a break on CRM_Mailing_BAO_Mailing::getRecipients() where during refactoring, the code responsible to fetch contacts of selected smart group IDs, on SMS mode was missed. And this PR is about retaining that code and some additional changes. Thanks to @JKingsnorth for notifying this break.

CRM-21316: Refactor getRecipients fn by creating a new fn getMailRecipients, that will be used ONLY to fetch and store mail recipients

monishdeb · 2018-01-19T17:52:21Z

@JKingsnorth can you please review my patch?

eileenmcnaughton · 2018-01-21T22:40:57Z

CRM/Mailing/BAO/Mailing.php

+        "$contact.do_not_sms",
+        "$contact.is_opt_out = 0",
+        "$contact.is_deceased <> 1",
+        "$phone.phone_type_id = " . CRM_Utils_Array::value('Mobile', CRM_Core_OptionGroup::values('phone_type', TRUE, FALSE, FALSE, NULL, 'name')),


does CRM_Core_PseudoConstant::getKey not work here? It's certainly shorter & that OptionGroup::values function is a bit evil

eileenmcnaughton · 2018-01-21T22:41:34Z

UT would be great here.

JKingsnorth · 2018-01-22T09:06:10Z

CRM/Mailing/BAO/Mailing.php

+          ->param('#groups', $includeSmartGroupIDs)
+          ->param('#mailingID', $mailingID)
+          ->execute();
+      }


This is almost all duplicated - could we just store the label of the ID field in a variable to use? This should help avoid SMS being accidentally omitted if anything else were to change. Or do you think that would make it unnecessarily hard to read?

JKingsnorth · 2018-01-22T12:21:25Z

CRM/Mailing/BAO/Mailing.php

+        "$phone.phone != ''",
+        "mg.mailing_id = #mailingID",
+        'temp.contact_id IS null',
+      );
      CRM_Utils_SQL_Select::from($phone)
        ->select(" DISTINCT $phone.id as phone_id ")


@monishdeb This needs to also select contact id, otherwise it causes a fatal error because the column count doesn't match up with replace into.

JKingsnorth · 2018-01-22T12:27:33Z

@monishdeb Additionally, we can't rely on this here: https://github.com/JMAConsulting/civicrm-core/blob/038b55fd5b9407b4f10eb9666c0106a24173cc9b/CRM/Mailing/BAO/Mailing.php#L126

Because the SMS provider is set on the second step of creating an SMS. So the first time this function is called, the SMS provider ID is not set, and it tries to calculate email recipients for the SMS.

We could fix this by passing a 'dummy' SMS provider ID from the first page of the 'New SMS' form perhaps?

JKingsnorth · 2018-01-22T12:31:51Z

I have started on some tests for the getRecipients function as part of #11162 . I'd like to set aside some time to write some further tests for this.

monishdeb · 2018-01-26T15:16:42Z

@JKingsnorth sorry for the delay. I am not getting enough time (working on an internal project, need to be deployed next week) to make the additional changes as required in this PR. Can you please submit a patch on top my PR changes. And I will include that patch as commit of yours, in this PR. Or if it's not urgent can I submit the final changes by next week itself?

@michaelmcandrew I saw your PR #11589, can I include your fix in this PR?

michaelmcandrew · 2018-01-26T17:24:38Z

@monishdeb - sure :)

michaelmcandrew · 2018-01-27T22:28:23Z

@JKingsnorth - since you are interested in SMS tests, I wanted to point you in the direction of @mattwire's open pr with some sms tests in case you hadn't seen it. (I had forgotten about it until today).

monishdeb · 2018-02-02T16:22:32Z

First of all, sorry for delay :( I have updated the PR and here are new changes:

Cherry-picked @michaelmcandrew commit for CRM-21316: Add missing code to handle smart group on sms mode #11558
Cherry-picked @JKingsnorth commit to include SMS unit test 31a5c38
Refactored code to use same SQL queries for both mailing and sms
Modified unit-test to use smart group and other style fixes.

@eileenmcnaughton @seamuslee001 @JKingsnorth @michaelmcandrew thankyou all for your feedback and fixes made for this regression. Can you please review my latest patch so that we can merge it against 4.7.31-rc ?

seamuslee001 · 2018-02-02T19:55:11Z

Jenkins re test this please

seamuslee001 · 2018-02-02T19:58:48Z

CRM/Mailing/BAO/Mailing.php

        ->where('gc.group_id IN (#groups)')
        ->where($includeFilters)
-        ->orderBy($order_by)


we don't need the orderby anymore? @monishdeb

eileenmcnaughton · 2018-02-02T21:38:58Z

Just a note that I have not been following this closely but I know others have so I'm happy to merge if @seamuslee001 @JKingsnorth or @michaelmcandrew approve it

JKingsnorth · 2018-02-02T21:42:55Z

I'll try to take a look at this early on Monday. Thanks for merging in the test and fix for the SMS ordering.

eileenmcnaughton · 2018-02-02T21:45:11Z

test this please

JKingsnorth · 2018-02-05T09:53:14Z

Looking at this one now. I'm running through some manual tests, but once this issue is merged I'm going to work on some more unit tests for getRecipients.

JKingsnorth · 2018-02-05T11:27:57Z

@monishdeb @michaelmcandrew This still isn't working at all for SMS mailings. The count is incorrect on the second step of sending a mass SMS. The code:
$isSMSmode = (!CRM_Utils_System::isNull($mailingObj->sms_provider_id));

at the beginning of getRecipients doesn't work because an sms_provider_id is not set during the first step of creating an SMS mailing. This means that getRecipients treats it as an 'email', and bases the recipient list off of whether they have an email address.

This is currently not covered by the test because the test creates the mailing with the SMS provider ID.

We can resolve this by setting the SMS provider ID to the default SMS provider on this step of the form - like: #11628

(As a side note - you can have TWO default SMS providers (separate issue), hence the way I've structured the API call...)

This seems to get things working; but I'm going to keep testing the SMS inclusion/exclusion for a few minutes.

JKingsnorth · 2018-02-05T12:00:10Z

@monishdeb Additionally, the functionality to include/exclude recipients of previous mailings does not appear to work for email or SMS mailings?

monishdeb · 2018-02-05T13:59:35Z

Thanks @JKingsnorth for your feedback let me cover each of your points:

This still isn't working at all for SMS mailings. The count is incorrect on the second step of sending a mass SMS. The code:
$isSMSmode = (!CRM_Utils_System::isNull($mailingObj->sms_provider_id));

This would be a separate issue as the current patch is only about retaining the earlier code which got missed during refactoring.

Additionally, the functionality to include/exclude recipients of previous mailings does not appear to work for email or SMS mailings?

Yes, it should because the same table is used to fetch the exclude and include group ids here and eventually same SQL queries is used filter out contacts which are present in exclude groups.

JKingsnorth · 2018-02-05T16:57:18Z

I think these issues are within scope for this, since they were all introduced as a side-effect of #11142. Reverting that PR fixes all these issues as far as I can tell.

Cannot include 'previous recipients' in mailing
There doesn't seem to be anything in the new SQL that includes the groups used in previous mailings? If you try to include a 'previous mailing' then the estimated recipients does not change. I am not sure whether this is due to a UI issue, or a functional issue (see below). Reverting 11142 makes this work again.

UI is buggy when adding/removing recipient groups

Add a group to a mailing
Click the x to remove it - it is not removed from the UI (https://issues.civicrm.org/jira/browse/CRM-21736)

and

Add a 'previous mailing' recipient list to a mailing
Note that it does not appear
Add a normal group
Now they both appear

SMS recipient estimation doesn't work on second step of creating SMS
This is actually caused by the original PR too. In the original code we passed in a $mode to the getRecipients function (see https://github.com/civicrm/civicrm-core/pull/11142/files#diff-d751a392a437280fae775ae39f59760bL273), so the 'isSMS' detection worked correctly. That parameter was removed as part of the original PR, so SMS mode detection stopped working. I've suggested a workaround, as per #11628

If we have time to fix these in this release cycle then great, I can help with the PRs. Otherwise I think we should revert #11142 until we have unit tests in place to ensure continuity.

There have undoubtedly been performance improvements thanks to @monishdeb 's hard work on this area. But I don't think we can introduce this many regressions in such a sensitive area of the system.

eileenmcnaughton · 2018-02-05T20:52:40Z

@totten @colemanw @mlutfy adding you in on this as we may need to make a decision to revert & try again next release if this can't be sorted out in time. Current status is that a patch merged to master (in time for 4.7.31) is causing regression/s that Monish & John are working on. Monish's time has opened up a bit so we may be fine but you should be monitoring this

monishdeb · 2018-02-05T22:46:41Z

I have fixed the 'UI is buggy when adding/removing recipient groups' issue. Going to fix the 'previous mailing' issue shortly and will update the PR.

monishdeb · 2018-02-06T05:11:01Z

@eileenmcnaughton @JKingsnorth I have fixed the two regressions where earlier:

If we select any prior mailing in recipient field then it doesn't fetch the prior recipients from civicrm_mailing_recipients table.
As per refactoring, I have retained the query that handles this scenario.
We cannot unselect the groups/prior mailing from recipients field.
This was a regression due to a bug fix where earlier there was a one-line change made in Recipients.js to fix an API error when no group/mailing was selected.

Here's the screencast, after the fix:

eileenmcnaughton · 2018-02-06T06:01:08Z

Well done @monishdeb - @JKingsnorth await your review on this to merge

JKingsnorth · 2018-02-06T08:45:16Z

Cool, thanks @monishdeb, testing this now.

JKingsnorth · 2018-02-06T09:38:23Z

Yes, thanks @monishdeb, confirmed this version fixes:

The UI issues
Including/excluding previous mailing recipients

I'm working on the PR to fix SMS recipient estimation now. Just a couple of things I want to test/investigate there; but we can do that separately.

monishdeb · 2018-02-06T09:50:16Z

Great. Thanks a lot for your persistent response and getting all the regression fixed :)

@JKingsnorth can we merge this PR ?

JKingsnorth · 2018-02-06T10:00:11Z

Thumbs up from me to merge.

eileenmcnaughton · 2018-02-06T10:01:21Z

merged based on @JKingsnorth review

JKingsnorth · 2018-02-06T10:02:29Z

Fastest merging finger in the Southern Hemisphere @eileenmcnaughton .

eileenmcnaughton · 2018-02-06T10:02:34Z

@JKingsnorth @monishdeb @michaelmcandrew is this closable now? https://issues.civicrm.org/jira/browse/CRM-21713

eileenmcnaughton · 2018-02-06T10:02:55Z

quickdraw-mcGraw

JKingsnorth · 2018-02-06T10:03:00Z

@eileenmcnaughton yes indeed - it can be closed.

monishdeb · 2018-02-06T10:04:12Z

Yup can be closed.

JKingsnorth · 2018-02-06T10:57:28Z

I'm starting work on a suite of tests for getRecipients, https://issues.civicrm.org/jira/browse/CRM-21739

JKingsnorth · 2018-02-06T22:14:20Z

And the last fix in the other PR has passed the test suite: #11628

civicrm-builder added the master label Jan 19, 2018

eileenmcnaughton reviewed Jan 21, 2018

View reviewed changes

JKingsnorth reviewed Jan 22, 2018

View reviewed changes

seamuslee001 mentioned this pull request Jan 26, 2018

fix select SMS recipients query #11589

Closed

monishdeb force-pushed the CRM-21260 branch 2 times, most recently from e8a785a to 2be0395 Compare February 2, 2018 15:59

monishdeb force-pushed the CRM-21260 branch from 2be0395 to b40b96d Compare February 2, 2018 17:54

seamuslee001 reviewed Feb 2, 2018

View reviewed changes

monishdeb force-pushed the CRM-21260 branch from b40b96d to 72c882e Compare February 5, 2018 07:39

monishdeb and others added 2 commits February 6, 2018 03:51

CRM-21316: Add missing code to handle smart group on sms mode

f7ff8a7

fix select SMS recipients query

e624bc9

CRM-21320 Test for mass SMS recipients

9f0a25d

monishdeb added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Feb 5, 2018

additional fixes and optimisation

6f3a35e

monishdeb force-pushed the CRM-21260 branch from 72c882e to 6f3a35e Compare February 6, 2018 04:53

eileenmcnaughton merged commit 1a34d42 into civicrm:master Feb 6, 2018

monishdeb deleted the CRM-21260 branch February 6, 2018 10:02

monishdeb removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Feb 6, 2018

JKingsnorth mentioned this pull request Feb 6, 2018

CRM-21316 Additional fix. Set default provider when creating SMS. #11628

Closed

mlutfy added this to the 4.7.31 milestone Feb 9, 2018

JKingsnorth mentioned this pull request Mar 23, 2018

[ON HOLD] CRM-21320 Fix selection of mobile number in mass SMS #11162

Closed

jmcclelland mentioned this pull request Aug 8, 2018

Issue273 #12638

Closed

CRM-21316: Add missing code to handle smart group on sms mode #11558

CRM-21316: Add missing code to handle smart group on sms mode #11558

Conversation

monishdeb commented Jan 19, 2018 • edited Loading

Overview

monishdeb commented Jan 19, 2018

eileenmcnaughton Jan 21, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 21, 2018

JKingsnorth Jan 22, 2018

Choose a reason for hiding this comment

JKingsnorth Jan 22, 2018

Choose a reason for hiding this comment

JKingsnorth commented Jan 22, 2018

JKingsnorth commented Jan 22, 2018

monishdeb commented Jan 26, 2018 • edited Loading

michaelmcandrew commented Jan 26, 2018

michaelmcandrew commented Jan 27, 2018

monishdeb commented Feb 2, 2018 • edited Loading

seamuslee001 commented Feb 2, 2018

seamuslee001 Feb 2, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 2, 2018

JKingsnorth commented Feb 2, 2018

eileenmcnaughton commented Feb 2, 2018

JKingsnorth commented Feb 5, 2018

JKingsnorth commented Feb 5, 2018

JKingsnorth commented Feb 5, 2018

monishdeb commented Feb 5, 2018 • edited Loading

JKingsnorth commented Feb 5, 2018 • edited Loading

eileenmcnaughton commented Feb 5, 2018

monishdeb commented Feb 5, 2018

monishdeb commented Feb 6, 2018 • edited Loading

eileenmcnaughton commented Feb 6, 2018

JKingsnorth commented Feb 6, 2018

JKingsnorth commented Feb 6, 2018

monishdeb commented Feb 6, 2018 • edited Loading

JKingsnorth commented Feb 6, 2018

eileenmcnaughton commented Feb 6, 2018

JKingsnorth commented Feb 6, 2018

eileenmcnaughton commented Feb 6, 2018

eileenmcnaughton commented Feb 6, 2018

JKingsnorth commented Feb 6, 2018 • edited Loading

monishdeb commented Feb 6, 2018

JKingsnorth commented Feb 6, 2018

JKingsnorth commented Feb 6, 2018

monishdeb commented Jan 19, 2018 •

edited

Loading

monishdeb commented Jan 26, 2018 •

edited

Loading

monishdeb commented Feb 2, 2018 •

edited

Loading

monishdeb commented Feb 5, 2018 •

edited

Loading

JKingsnorth commented Feb 5, 2018 •

edited

Loading

monishdeb commented Feb 6, 2018 •

edited

Loading

monishdeb commented Feb 6, 2018 •

edited

Loading

JKingsnorth commented Feb 6, 2018 •

edited

Loading