dev/core/#273 Fix issue where sending an SMS with the To Field in the… #12634
Conversation
|
(Standard links)
|
seamuslee001
force-pushed
the
dev_core_273
branch
2 times, most recently
from
b587006 to
5dab823
Compare
| // Collect all of the PEAR_Error objects | ||
| $errMsgs[] = $sendResult; | ||
| if ($doNotSms) { | ||
| $errMsgs[] = PEAR::raiseError('Contact Does not accept SMS', NULL, PEAR_ERROR_RETURN); |
There was a problem hiding this comment.
Keeps it consistent with all the other errors
There was a problem hiding this comment.
Well it's not worse than before - I'll give you that
|
Thanks, @seamuslee001 |
|
Nice work! |
|
@mlutfy is this something you can review? Or @jmcclelland |
|
Changes look sane and I'm really happy to see the unit tests. Ping @JKingsnorth @michaelmcandrew in case they have time to run/review. |
| @@ -231,6 +231,7 @@ public static function getRecipients($mailingID) { | |||
| $criteria = array( | |||
| 'is_opt_out' => CRM_Utils_SQL_Select::fragment()->where("$contact.is_opt_out = 0"), | |||
| 'is_deceased' => CRM_Utils_SQL_Select::fragment()->where("$contact.is_deceased <> 1"), | |||
| 'do_not_sms' => CRM_Utils_SQL_Select::fragment()->where("$contact.do_not_sms = 0"), | |||
| 'location_filter' => CRM_Utils_SQL_Select::fragment()->where("$entityTable.phone_type_id = " . CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_Phone', 'phone_type_id', 'Mobile')), | |||
There was a problem hiding this comment.
Nice work! I don't want to hold this PR back (and this could be a separate issue) but since it's related to this change I'll mention it: If someone opts out of email (is_opt_out), should they really opt out of SMS too? The UI very clearly says no bulk emails, so I would be in favor of removing that query item for sending mass SMS so people do get an SMS message even if they have opted out of bulk email.
There was a problem hiding this comment.
Rather than slow down this PR, I just opened a separate issue:
https://lab.civicrm.org/dev/core/issues/318
Let's merge this PR :).
|
@jmcclelland have you |
|
Yes, I have done an |
|
@seamuslee001 seems good to merge but against the rc |
… in the provider params set did not work and fix handling of do_not_sms Add test of error message
seamuslee001
force-pushed
the
dev_core_273
branch
from
5dab823 to
a9b7ee4
Compare
|
@eileenmcnaughton this is against the RC now should we put it as merge on pass? |
|
Merging per the tag |
… provider params set did not work and fix handling of do_not_sms
Overview
Picking up the work done in these 2 PRs https://github.com/civicrm/civicrm-core/pull/12614/files https://github.com/civicrm/civicrm-core/pull/12554/files it was obvious that when the phone number was set in the To param for the sms provider params that SMSes were failing. This was shown to be happening when you selected outbound sms from the list of actions on a contact screen.
When investigating this i found that in that case where the phone number was set in the SMS provider Params, the do_not_sms flag was potentially not properly being respected and would have been worsened under PR 12614. This now has tests that a) prove the original bug and b) lock in the handling on do not SMS
Before
SMS sending fails when phone number included in the To param of smsProviderParams
After
SMSes fail to send and potential mis handling in terms of do_not_sms
Technical Details
SMSes send correctly have better handling of do_not_sms and more tests
ping @mattwire @eileenmcnaughton @vinuvarshith @jyothi-shree21