From 5ef85fd60696ca78c4086659a22000c62f495684 Mon Sep 17 00:00:00 2001 From: Jamie McClelland Date: Tue, 2 Feb 2021 16:30:57 -0500 Subject: [PATCH] refactor to improve readability and expand test coverage. --- CRM/Contact/Form/Task/SMSCommon.php | 38 +++-- .../CRM/Contact/Form/Task/SMSCommonTest.php | 142 ++++++++++++------ 2 files changed, 111 insertions(+), 69 deletions(-) diff --git a/CRM/Contact/Form/Task/SMSCommon.php b/CRM/Contact/Form/Task/SMSCommon.php index 8c689f14ad..84a657f64a 100644 --- a/CRM/Contact/Form/Task/SMSCommon.php +++ b/CRM/Contact/Form/Task/SMSCommon.php @@ -175,7 +175,7 @@ class CRM_Contact_Form_Task_SMSCommon { foreach ($form->_contactIds as $key => $contactId) { $mobilePhone = NULL; - $value = $form->_contactDetails[$contactId]; + $contactDetails = $form->_contactDetails[$contactId]; //to check if the phone type is "Mobile" $phoneTypes = CRM_Core_OptionGroup::values('phone_type', TRUE, FALSE, FALSE, NULL, 'name'); @@ -193,25 +193,21 @@ class CRM_Contact_Form_Task_SMSCommon { } } - if ((isset($value['phone_type_id']) && $value['phone_type_id'] != CRM_Utils_Array::value('Mobile', $phoneTypes)) || $value['do_not_sms'] || empty($value['phone']) || !empty($value['is_deceased'])) { - + // No phone, No SMS or Deceased: then we suppress it. + if (empty($contactDetails['phone']) || $contactDetails['do_not_sms'] || !empty($contactDetails['is_deceased'])) { + $suppressedSms++; + unset($form->_contactDetails[$contactId]); + continue; + } + elseif ($contactDetails['phone_type_id'] != CRM_Utils_Array::value('Mobile', $phoneTypes)) { //if phone is not primary check if non-primary phone is "Mobile" - if (!empty($value['phone']) - && $value['phone_type_id'] != CRM_Utils_Array::value('Mobile', $phoneTypes) && empty($value['is_deceased']) - ) { - $filter = ['do_not_sms' => 0]; - $contactPhones = CRM_Core_BAO_Phone::allPhones($contactId, FALSE, 'Mobile', $filter); - if (count($contactPhones) > 0) { - $mobilePhone = CRM_Utils_Array::retrieveValueRecursive($contactPhones, 'phone'); - $form->_contactDetails[$contactId]['phone_id'] = CRM_Utils_Array::retrieveValueRecursive($contactPhones, 'id'); - $form->_contactDetails[$contactId]['phone'] = $mobilePhone; - $form->_contactDetails[$contactId]['phone_type_id'] = $phoneTypes['Mobile'] ?? NULL; - } - else { - $suppressedSms++; - unset($form->_contactDetails[$contactId]); - continue; - } + $filter = ['do_not_sms' => 0]; + $contactPhones = CRM_Core_BAO_Phone::allPhones($contactId, FALSE, 'Mobile', $filter); + if (count($contactPhones) > 0) { + $mobilePhone = CRM_Utils_Array::retrieveValueRecursive($contactPhones, 'phone'); + $form->_contactDetails[$contactId]['phone_id'] = CRM_Utils_Array::retrieveValueRecursive($contactPhones, 'id'); + $form->_contactDetails[$contactId]['phone'] = $mobilePhone; + $form->_contactDetails[$contactId]['phone_type_id'] = $phoneTypes['Mobile'] ?? NULL; } else { $suppressedSms++; @@ -224,7 +220,7 @@ class CRM_Contact_Form_Task_SMSCommon { $phone = $mobilePhone; } elseif (empty($form->_toContactPhone)) { - $phone = $value['phone']; + $phone = $contactDetails['phone']; } else { $phone = $form->_toContactPhone[$key] ?? NULL; @@ -232,7 +228,7 @@ class CRM_Contact_Form_Task_SMSCommon { if ($phone) { $toArray[] = [ - 'text' => '"' . $value['sort_name'] . '" (' . $phone . ')', + 'text' => '"' . $contactDetails['sort_name'] . '" (' . $phone . ')', 'id' => "$contactId::{$phone}", ]; } diff --git a/tests/phpunit/CRM/Contact/Form/Task/SMSCommonTest.php b/tests/phpunit/CRM/Contact/Form/Task/SMSCommonTest.php index 694125265d..edf9058b05 100644 --- a/tests/phpunit/CRM/Contact/Form/Task/SMSCommonTest.php +++ b/tests/phpunit/CRM/Contact/Form/Task/SMSCommonTest.php @@ -15,7 +15,7 @@ */ class CRM_Contact_Form_Task_SMSCommonTest extends CiviUnitTestCase { - protected $_contactMobileNumbers = []; + protected $_smsRecipients = []; /** * Set up for tests. @@ -26,60 +26,90 @@ class CRM_Contact_Form_Task_SMSCommonTest extends CiviUnitTestCase { parent::setUp(); $mobile_type_id = CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_Phone', 'phone_type_id', 'Mobile'); $phone_type_id = CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_Phone', 'phone_type_id', 'Phone'); - + $contact1 = $this->individualCreate([ - 'first_name' => 'First', - 'last_name' => 'Person', - 'do_not_sms' => 0, - 'phone' => [ - 1 => [ - 'phone_type_id' => $mobile_type_id, - 'location_type_id' => 1, - 'phone' => '1111111111' - ] - ] + 'first_name' => 'First', + 'last_name' => 'Person', + 'do_not_sms' => 0, + 'phone' => [ + 1 => [ + 'phone_type_id' => $mobile_type_id, + 'location_type_id' => 1, + 'phone' => '1111111111', + ], + ], ]); $contact2 = $this->individualCreate([ - 'first_name' => 'Second', - 'last_name' => 'Person', - 'do_not_sms' => 0, - 'phone' => [ - 1 => [ - 'phone_type_id' => $phone_type_id, - 'location_type_id' => 1, - 'phone' => '9999999999', - 'is_primary' => 1, - ], - 2 => [ - 'phone_type_id' => $mobile_type_id, - 'location_type_id' => 1, - 'phone' => '2222222222' - ] - ] + 'first_name' => 'Second', + 'last_name' => 'Person', + 'do_not_sms' => 0, + 'phone' => [ + 1 => [ + 'phone_type_id' => $phone_type_id, + 'location_type_id' => 1, + 'phone' => '9999999999', + 'is_primary' => 1, + ], + 2 => [ + 'phone_type_id' => $mobile_type_id, + 'location_type_id' => 1, + 'phone' => '2222222222', + ], + ], ]); $contact3 = $this->individualCreate([ - 'first_name' => 'Third', - 'last_name' => 'Person', - 'do_not_sms' => 0, - 'phone' => [ - 1 => [ - 'phone_type_id' => $mobile_type_id, - 'location_type_id' => 1, - 'phone' => '3333333333', - 'is_primary' => 0, - ] - ] + 'first_name' => 'Third', + 'last_name' => 'Person', + 'do_not_sms' => 0, + 'phone' => [ + 1 => [ + 'phone_type_id' => $mobile_type_id, + 'location_type_id' => 1, + 'phone' => '3333333333', + 'is_primary' => 0, + ], + ], ]); - - // Track the contact id to correct mobile phone number. - $this->_contactMobileNumbers = [ + $contact4 = $this->individualCreate([ + 'first_name' => 'Fourth', + 'last_name' => 'Person', + 'do_not_sms' => 1, + 'phone' => [ + 1 => [ + 'phone_type_id' => $mobile_type_id, + 'location_type_id' => 1, + 'phone' => '4444444444', + ], + ], + ]); + $contact5 = $this->individualCreate([ + 'first_name' => 'Fifth', + 'last_name' => 'Person', + 'do_not_sms' => 0, + 'is_deceased' => 1, + 'phone' => [ + 1 => [ + 'phone_type_id' => $mobile_type_id, + 'location_type_id' => 1, + 'phone' => '5555555555', + ], + ], + ]); + // Track the contacts that should get an SMS and which + // number they should receive it. + $this->_smsRecipients = [ $contact1 => "1111111111", $contact2 => "2222222222", - $contact3 => "3333333333" + $contact3 => "3333333333", ]; - $this->_contactIds = array_keys($this->_contactMobileNumbers); - + $this->_contactIds = [ + $contact1, + $contact2, + $contact3, + $contact4, + $contact5, + ]; } /** @@ -96,15 +126,31 @@ class CRM_Contact_Form_Task_SMSCommonTest extends CiviUnitTestCase { $form->_single = FALSE; CRM_Contact_Form_Task_SMSCommon::buildQuickForm($form); $contacts = json_decode($form->get_template_vars('toContact')); + $smsRecipientsActual = []; foreach ($contacts as $contact) { $id = $contact->id; - // id is in the format: contact_id::phone_number, e.g.: 5::2222222222 $ret = preg_match('/^([0-9]+)::([0-9]+)/', $id, $matches); - $this->assertEquals(1, $ret, "Failed to find mobile number."); + // id is in the format: contact_id::phone_number, e.g.: 5::2222222222 + $this->assertEquals(1, $ret, "Failed to extract the mobile number and contact id."); $contact_id = $matches[1]; $phone_number = $matches[2]; - $this->assertEquals($phone_number, $this->_contactMobileNumbers[$contact_id], "Returned incorrect mobile number in SMS send quick form."); + // Check if we are supposed to send an SMS to this contact. + if (array_key_exists($contact_id, $this->_smsRecipients)) { + // We are supposed to send an SMS to this contact, now make sure we have the right phone number. + $this->assertEquals($phone_number, $this->_smsRecipients[$contact_id], "Returned incorrect mobile number in SMS send quick form."); + $smsRecipientsActual[] = $contact_id; + } + else { + // We are not supposed to send this contact an email. + $this->assertTrue(FALSE, "We should not be sending an SMS to contact_id: $contact_id."); + } } + + // Make sure we sent to all the contacts. + sort($smsRecipientsActual); + $smsRecipientsIntended = array_keys($this->_smsRecipients); + sort($smsRecipientsIntended); + $this->assertEquals($smsRecipientsActual, $smsRecipientsIntended, "We did not send an SMS to all the contacts."); } } -- 2.25.1