CRM-21405 Allow "Outbound SMS" when Mobile is not primary phone number

[mattwire]

Overview

Allow "Outbound SMS" when Mobile is not primary phone number

Before

When a contact has a mobile phone number but it is not the primary number the "Outbound SMS" action is removed from the contact actions.

After

When a contact has a mobile phone number but it is not the primary number the "Outbound SMS" action is allowed, and will use the first available mobile number from the contact.

---

• CRM-21405: Allow "Outbound SMS" when Mobile is not primary phone number

[mattwire]

@JKingsnorth Are you able to review this please?

[mattwire]

Jenkins test this please

[mattwire]

@JKingsnorth @monishdeb @michaelmcandrew Be great if we could get this simple SMS PR reviewed/merged :-)

[michaelmcandrew]

Hi @mattwire

I did some manual UI testing on this. Here are the results in the format "Conditions -> Could I see the outbound SMS action?"

• No mobile number - No
• Primary mobile number - Yes
• Secondary mobile number - Yes
• Primary mobile number + DO NOT SMS - No
• Secondary mobile number + DO NOT SMS - No

So it looks like it is working nicely.

I presume that the following section is API chaining but I am slightly baffled as to how it works. I would have thought that if you want to get a field from the contact entity, you need to reference the contact entity somewhere.

$phone = civicrm_api3('Phone', 'getsingle', array(
  'return' => array("contact_id.do_not_sms", "phone"),
  'phone_type_id' => $mobileTypeID,
  'contact_id' => $contactId,
  'options' => array('limit' => 1, 'sort' => "is_primary DESC"),

Maybe the phone API is randomly returning some fields from the contact. Or maybe it is a documented feature that I am not aware of.

On the one hand, I am hesitant to suggest modifying code that works / passes the manual testing I did.

On the other hand, if this is an undocumented way of using the phone API that is not guaranteed to work in the future, it would be good to change it.

Do you have any ideas about that?

[michaelmcandrew]

@mattwire - I rewrote the api call to use explicit chaining - let me know what you think.

[mattwire]

@michaelmcandrew That's fine - happy with your changes. I think both work :-) So, do you think this is good to merge?

[monishdeb]

Please use

-  'phone_type_id' => $mobileTypeID,
+ 'phone_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_Phone', 'phone_type_id', 'Mobile'),

as I can see $mobileTypeID isn't used anywhere else

[michaelmcandrew]

looks good

[monishdeb]

I hope this doesn't throw notice errors as it will hit continue before that, at line 94 if $phone is empty

[michaelmcandrew]

If the contact does not have a phone, then getsingle will indeed throw an exception which will get caught, so no notice will be shown. I ran the code to check that this actually happened.

[monishdeb]

Cool 👍

[monishdeb]

Apart from the minor change mentioned in the patch and based on @michaelmcandrew feedback, this PR good to merge. Will it be possible to replicate this behavior in a unit test?

[michaelmcandrew]

Yup, I vote merge

…

On 14 Feb 2018 21:07, "Matthew Wire" ***@***.***> wrote: @michaelmcandrew <https://github.com/michaelmcandrew> That's fine - happy with your changes. I think both work :-) So, do you think this is good to merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11252 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAM0L5lZ6K-Pg04S-eGEfgQFm1MZFmY4ks5tU0r6gaJpZM4QWPB9> .

[michaelmcandrew]

Sorry @monishdeb, I sent that previous message from my email and had not seen your message - have replied now.

[monishdeb]

@michaelmcandrew no problem and thanks for your quick response. @mattwire can you please make the minor change mentioned in the patch?

[mattwire]

@monishdeb I've made the changes as requested. Merge on pass?

[monishdeb]

Absolutely 👍