refactor to improve readability and expand test coverage.

diff --git a/CRM/Contact/Form/Task/SMSCommon.php b/CRM/Contact/Form/Task/SMSCommon.php
index 8c689f14ad4f4a129b55a13bcb0a77c6f97a7be8..84a657f64ab29b73e28f41dd80eae773cc2628fb 100644 (file)
--- 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 694125265dafe8c761a29b648a12bc5ba549c735..edf9058b0527ffda659eea06cc833818b9721e71 100644 (file)
--- 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.");
   }
 
 }