Merge pull request #16079 from eileenmcnaughton/email

Remove unnecessary, and possibly incorrect query from email update
civicrm · Dec 26, 2019 · ef9684f · ef9684f
2 parents bdec4df + 2ffdaed
commit ef9684f
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 53 deletions.
diff --git a/CRM/Core/BAO/Email.php b/CRM/Core/BAO/Email.php
@@ -54,6 +54,10 @@ public static function add(&$params) {
     $hook = empty($params['id']) ? 'create' : 'edit';
     CRM_Utils_Hook::pre($hook, 'Email', CRM_Utils_Array::value('id', $params), $params);
 
+    if (isset($params['is_bulkmail']) && $params['is_bulkmail'] === 'null') {
+      CRM_Core_Error::deprecatedFunctionWarning('It is not valid to set bulkmail to null, it is boolean');
+      $params['bulkmail'] = 0;
+    }
     $email = new CRM_Core_DAO_Email();
     $email->copyValues($params);
     if (!empty($email->email)) {
@@ -64,17 +68,17 @@ public static function add(&$params) {
 
     /*
      * since we're setting bulkmail for 1 of this contact's emails, first reset all their other emails to is_bulkmail false
-     *  We shouldn't not set the current email to false even though we
-     *  are about to reset it to avoid contaminating the changelog if logging is enabled
+     *  We shouldn't set the current email to false even though we
+     *  are about to reset it to avoid contaminating the changelog if logging is enabled.
      * (only 1 email address can have is_bulkmail = true)
      */
-    if ($email->is_bulkmail != 'null' && !empty($params['contact_id']) && !self::isMultipleBulkMail()) {
+    if ($email->is_bulkmail && !empty($params['contact_id']) && !self::isMultipleBulkMail()) {
       $sql = "
 UPDATE civicrm_email
 SET    is_bulkmail = 0
 WHERE  contact_id = {$params['contact_id']}
 ";
-      if ($hook == 'edit') {
+      if ($hook === 'edit') {
         $sql .= " AND id <> {$params['id']}";
       }
       CRM_Core_DAO::executeQuery($sql);

diff --git a/tests/phpunit/CRM/Core/BAO/EmailTest.php b/tests/phpunit/CRM/Core/BAO/EmailTest.php
@@ -6,43 +6,45 @@
  */
 class CRM_Core_BAO_EmailTest extends CiviUnitTestCase {
 
+  /**
+   * Set up for class.
+   *
+   * @throws \CRM_Core_Exception
+   */
   public function setUp() {
     parent::setUp();
-
     $this->quickCleanup(['civicrm_contact', 'civicrm_email']);
   }
 
   /**
    * Add() method (create and update modes)
+   *
+   * @throws \CRM_Core_Exception
    */
-  public function testAdd() {
+  public function testCreate() {
     $contactId = $this->individualCreate();
-
-    $params = [];
     $params = [
       'email' => 'jane.doe@example.com',
       'is_primary' => 1,
       'location_type_id' => 1,
       'contact_id' => $contactId,
     ];
 
-    CRM_Core_BAO_Email::add($params);
+    CRM_Core_BAO_Email::create($params);
 
     $emailId = $this->assertDBNotNull('CRM_Core_DAO_Email', 'jane.doe@example.com', 'id', 'email',
       'Database check for created email address.'
     );
 
-    // Now call add() to modify an existing email address
-
-    $params = [];
+    // Now call create() to modify an existing email address
     $params = [
       'id' => $emailId,
       'contact_id' => $contactId,
       'is_bulkmail' => 1,
       'on_hold' => 1,
     ];
 
-    CRM_Core_BAO_Email::add($params);
+    CRM_Core_BAO_Email::create($params);
 
     $isBulkMail = $this->assertDBNotNull('CRM_Core_DAO_Email', $emailId, 'is_bulkmail', 'id',
       'Database check on updated email record.'
@@ -53,7 +55,9 @@ public function testAdd() {
   }
 
   /**
-   * HoldEmail() method (set and reset on_hold condition)
+   * HoldEmail() method (set and reset on_hold condition).
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testHoldEmail() {
     $contactId = $this->individualCreate();
@@ -65,21 +69,20 @@ public function testHoldEmail() {
       'contact_id' => $contactId,
     ];
 
-    CRM_Core_BAO_Email::add($params);
+    CRM_Core_BAO_Email::create($params);
 
     $emailId = $this->assertDBNotNull('CRM_Core_DAO_Email', 'jane.doe@example.com', 'id', 'email',
       'Database check for created email address.'
     );
 
-    // Now call add() to update on_hold=1 ("On Hold Bounce") and check record state
-    $params = [];
+    // Now call create() to update on_hold=1 ("On Hold Bounce") and check record state
     $params = [
       'id' => $emailId,
       'contact_id' => $contactId,
       'on_hold' => 1,
     ];
 
-    CRM_Core_BAO_Email::add($params);
+    CRM_Core_BAO_Email::create($params);
 
     // Use assertDBNotNull to get back value of hold_date and check if it's in the current year.
     // NOTE: The assertEquals will fail IF this test is run just as the year is changing (low likelihood).
@@ -95,15 +98,14 @@ public function testHoldEmail() {
       'Check if on_hold=1 in updated email record.'
     );
 
-    // Now call add() to update on_hold=2 ("On Hold Opt-out") and check record state
-    $params = [];
+    // Now call create() to update on_hold=2 ("On Hold Opt-out") and check record state
     $params = [
       'id' => $emailId,
       'contact_id' => $contactId,
       'on_hold' => 2,
     ];
 
-    CRM_Core_BAO_Email::add($params);
+    CRM_Core_BAO_Email::create($params);
 
     // Use assertDBNotNull to get back value of hold_date and check that it's in the current year.
     // NOTE: The assertEquals will fail IF this test is run just as the year is changing (low likelihood).
@@ -119,15 +121,14 @@ public function testHoldEmail() {
       'Check if on_hold=2 in updated email record.'
     );
 
-    // Now call add() with on_hold=null (not on hold) and verify that reset_date is set.
-    $params = [];
+    // Now call create() with on_hold=null (not on hold) and verify that reset_date is set.
     $params = [
       'id' => $emailId,
       'contact_id' => $contactId,
       'on_hold' => 'null',
     ];
 
-    CRM_Core_BAO_Email::add($params);
+    CRM_Core_BAO_Email::create($params);
     $this->assertDBCompareValue('CRM_Core_DAO_Email', $emailId, 'on_hold', 'id', 0,
       'Check if on_hold=0 in updated email record.'
     );
@@ -150,6 +151,8 @@ public function testHoldEmail() {
 
   /**
    * AllEmails() method - get all emails for our contact, with primary email first
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testAllEmails() {
     $contactParams = [
@@ -176,6 +179,8 @@ public function testAllEmails() {
 
   /**
    * Test getting list of Emails for use in Receipts and Single Email sends
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testGetFromEmail() {
     $this->createLoggedInUser();