From 2ffdaed1d83574812251e3b8a5c23672b7b2b38d Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 11 Dec 2019 12:42:47 +1300 Subject: [PATCH] Remove unnecessary, and possibly incorrect query from email update When updating an email the comments pretty clearly say to update bulkmail if it is 1 but the current if clause picks up anything other than 'null' - which is invalid for a boolean field. This creates an additional query which has some locking risk being in a high volume place. In addition it seems like it could be a bug in some cases --- CRM/Core/BAO/Email.php | 12 ++- tests/phpunit/CRM/Core/BAO/EmailTest.php | 45 +++++----- tests/phpunit/api/v3/EmailTest.php | 102 ++++++++++++++++------- 3 files changed, 106 insertions(+), 53 deletions(-) diff --git a/CRM/Core/BAO/Email.php b/CRM/Core/BAO/Email.php index f2441a740f47..c8bcb650cf88 100644 --- 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 index 765150d615e8..3c0f4f61829a 100644 --- a/tests/phpunit/CRM/Core/BAO/EmailTest.php +++ b/tests/phpunit/CRM/Core/BAO/EmailTest.php @@ -6,19 +6,23 @@ */ 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, @@ -26,15 +30,13 @@ public function testAdd() { '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, @@ -42,7 +44,7 @@ public function testAdd() { '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(); diff --git a/tests/phpunit/api/v3/EmailTest.php b/tests/phpunit/api/v3/EmailTest.php index 85c24952c4cf..7f2e413936aa 100644 --- a/tests/phpunit/api/v3/EmailTest.php +++ b/tests/phpunit/api/v3/EmailTest.php @@ -11,11 +11,13 @@ /** * Class api_v3_EmailTest + * * @group headless */ class api_v3_EmailTest extends CiviUnitTestCase { protected $_contactID; protected $_locationType; + protected $locationType2; protected $_entity; protected $_params; @@ -26,7 +28,7 @@ public function setUp() { $this->_contactID = $this->organizationCreate(NULL); $this->_locationType = $this->locationTypeCreate(NULL); - $this->_locationType2 = $this->locationTypeCreate([ + $this->locationType2 = $this->locationTypeCreate([ 'name' => 'New Location Type 2', 'vcard_name' => 'New Location Type 2', 'description' => 'Another Location Type', @@ -43,8 +45,12 @@ public function setUp() { } /** + * Test create email. + * * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreateEmail($version) { $this->_apiversion = $version; @@ -53,13 +59,13 @@ public function testCreateEmail($version) { $get = $this->callAPISuccess('email', 'get', [ 'location_type_id' => $this->_locationType->id, ]); - $this->assertEquals(0, $get['count'], 'Contact not successfully deleted In line ' . __LINE__); + $this->assertEquals(0, $get['count'], 'Contact not successfully deleted.'); $result = $this->callAPIAndDocument('email', 'create', $params, __FUNCTION__, __FILE__); $this->assertEquals(1, $result['count']); $this->assertNotNull($result['id']); $this->assertNotNull($result['values'][$result['id']]['id']); - $delresult = $this->callAPISuccess('email', 'delete', ['id' => $result['id']]); + $this->callAPISuccess('email', 'delete', ['id' => $result['id']]); } /** @@ -67,7 +73,9 @@ public function testCreateEmail($version) { * the LocationType default * * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreateEmailDefaultLocation($version) { $this->_apiversion = $version; @@ -82,8 +90,11 @@ public function testCreateEmailDefaultLocation($version) { * If a new email is set to is_primary the prev should no longer be. * * If is_primary is not set then it should become is_primary is no others exist + * * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreateEmailPrimaryHandlingChangeToPrimary($version) { $this->_apiversion = $version; @@ -92,7 +103,7 @@ public function testCreateEmailPrimaryHandlingChangeToPrimary($version) { $email1 = $this->callAPISuccess('email', 'create', $params); //now we check & make sure it has been set to primary $expected = 1; - $check = $this->callAPISuccess('email', 'getcount', [ + $this->callAPISuccess('email', 'getcount', [ 'is_primary' => 1, 'id' => $email1['id'], ], @@ -102,12 +113,14 @@ public function testCreateEmailPrimaryHandlingChangeToPrimary($version) { /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreateEmailPrimaryHandlingChangeExisting($version) { $this->_apiversion = $version; - $email1 = $this->callAPISuccess('email', 'create', $this->_params); - $email2 = $this->callAPISuccess('email', 'create', $this->_params); + $this->callAPISuccess('email', 'create', $this->_params); + $this->callAPISuccess('email', 'create', $this->_params); $check = $this->callAPISuccess('email', 'getcount', [ 'is_primary' => 1, 'contact_id' => $this->_contactID, @@ -128,7 +141,9 @@ public function testCreateEmailWithoutEmail($version) { /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testGetEmail($version) { $this->_apiversion = $version; @@ -139,12 +154,14 @@ public function testGetEmail($version) { $this->assertEquals($get['count'], 1); $get = $this->callAPISuccess('email', 'create', $this->_params + ['debug' => 1, 'action' => 'get']); $this->assertEquals($get['count'], 1); - $delresult = $this->callAPISuccess('email', 'delete', ['id' => $result['id']]); + $this->callAPISuccess('email', 'delete', ['id' => $result['id']]); } /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testDeleteEmail($version) { $this->_apiversion = $version; @@ -160,7 +177,7 @@ public function testDeleteEmail($version) { $get = $this->callAPISuccess('email', 'get', [ 'location_type_id' => $this->_locationType->id, ]); - $this->assertEquals(0, $get['count'], 'email already exists ' . __LINE__); + $this->assertEquals(0, $get['count'], 'email already exists'); //create one $create = $this->callAPISuccess('email', 'create', $params); @@ -170,12 +187,14 @@ public function testDeleteEmail($version) { $get = $this->callAPISuccess('email', 'get', [ 'location_type_id' => $this->_locationType->id, ]); - $this->assertEquals(0, $get['count'], 'Contact not successfully deleted In line ' . __LINE__); + $this->assertEquals(0, $get['count'], 'Contact not successfully deleted'); } /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testReplaceEmail($version) { $this->_apiversion = $version; @@ -183,7 +202,7 @@ public function testReplaceEmail($version) { $get = $this->callAPISuccess('email', 'get', [ 'contact_id' => $this->_contactID, ]); - $this->assertEquals(0, $get['count'], 'email already exists ' . __LINE__); + $this->assertEquals(0, $get['count'], 'email already exists'); // initialize email list with three emails at loc #1 and two emails at loc #2 $replace1Params = [ @@ -205,12 +224,12 @@ public function testReplaceEmail($version) { 'is_primary' => 0, ], [ - 'location_type_id' => $this->_locationType2->id, + 'location_type_id' => $this->locationType2->id, 'email' => '2-1@example.com', 'is_primary' => 0, ], [ - 'location_type_id' => $this->_locationType2->id, + 'location_type_id' => $this->locationType2->id, 'email' => '2-2@example.com', 'is_primary' => 0, ], @@ -223,7 +242,7 @@ public function testReplaceEmail($version) { $get = $this->callAPISuccess('email', 'get', [ 'contact_id' => $this->_contactID, ]); - $this->assertEquals(5, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(5, $get['count'], 'Incorrect email count'); // replace the subset of emails in location #1, but preserve location #2 $replace2Params = [ @@ -244,15 +263,15 @@ public function testReplaceEmail($version) { 'contact_id' => $this->_contactID, 'location_type_id' => $this->_locationType->id, ]); - $this->assertEquals(1, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(1, $get['count'], 'Incorrect email count'); // check emails at location #2 -- preserve the original two $get = $this->callAPISuccess('email', 'get', [ 'contact_id' => $this->_contactID, - 'location_type_id' => $this->_locationType2->id, + 'location_type_id' => $this->locationType2->id, ]); - $this->assertEquals(2, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(2, $get['count'], 'Incorrect email count'); // replace the set of emails with an empty set $replace3Params = [ @@ -268,12 +287,14 @@ public function testReplaceEmail($version) { 'contact_id' => $this->_contactID, ]); $this->assertAPISuccess($get); - $this->assertEquals(0, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(0, $get['count'], 'Incorrect email count'); } /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testReplaceEmailsInChain($version) { $this->_apiversion = $version; @@ -283,9 +304,9 @@ public function testReplaceEmailsInChain($version) { 'contact_id' => $this->_contactID, ]); $this->assertAPISuccess($get); - $this->assertEquals(0, $get['count'], 'email already exists ' . __LINE__); - $description = "Demonstrates use of Replace in a nested API call."; - $subfile = "NestedReplaceEmail"; + $this->assertEquals(0, $get['count'], 'email already exists'); + $description = 'Demonstrates use of Replace in a nested API call.'; + $subfile = 'NestedReplaceEmail'; // initialize email list with three emails at loc #1 and two emails at loc #2 $getReplace1Params = [ @@ -308,12 +329,12 @@ public function testReplaceEmailsInChain($version) { 'is_primary' => 0, ], [ - 'location_type_id' => $this->_locationType2->id, + 'location_type_id' => $this->locationType2->id, 'email' => '2-1@example.com', 'is_primary' => 0, ], [ - 'location_type_id' => $this->_locationType2->id, + 'location_type_id' => $this->locationType2->id, 'email' => '2-2@example.com', 'is_primary' => 0, ], @@ -327,7 +348,7 @@ public function testReplaceEmailsInChain($version) { $get = $this->callAPISuccess('email', 'get', [ 'contact_id' => $this->_contactID, ]); - $this->assertEquals(5, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(5, $get['count'], 'Incorrect email count'); // replace the subset of emails in location #1, but preserve location #2 $getReplace2Params = [ @@ -352,19 +373,21 @@ public function testReplaceEmailsInChain($version) { 'location_type_id' => $this->_locationType->id, ]); - $this->assertEquals(1, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(1, $get['count'], 'Incorrect email count'); // check emails at location #2 -- preserve the original two $get = $this->callAPISuccess('email', 'get', [ 'contact_id' => $this->_contactID, - 'location_type_id' => $this->_locationType2->id, + 'location_type_id' => $this->locationType2->id, ]); - $this->assertEquals(2, $get['count'], 'Incorrect email count at ' . __LINE__); + $this->assertEquals(2, $get['count'], 'Incorrect email count'); } /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testReplaceEmailWithId($version) { $this->_apiversion = $version; @@ -417,9 +440,12 @@ public function testReplaceEmailWithId($version) { $this->assertEquals('1-2@example.com', $get['values'][$emailID]['email']); } + /** + * Test updates affecting on hold emails. + * + * @throws \CRM_Core_Exception + */ public function testEmailOnHold() { - $params = []; - $params_change = []; $params = [ 'contact_id' => $this->_contactID, 'email' => 'api@a-team.com', @@ -448,7 +474,25 @@ public function testEmailOnHold() { $this->assertEquals(date('Y-m-d H:i'), date('Y-m-d H:i', strtotime($result_change['values'][$result_change['id']]['reset_date']))); $this->assertEmpty($result_change['values'][$result_change['id']]['hold_date']); - $delresult = $this->callAPISuccess('email', 'delete', ['id' => $result['id']]); + $this->callAPISuccess('email', 'delete', ['id' => $result['id']]); + } + + /** + * Test setting a bulk email unsets others on the contact. + * + * @throws \CRM_Core_Exception + */ + public function testSetBulkEmail() { + $individualID = $this->individualCreate([]); + $email = $this->callAPISuccessGetSingle('Email', ['contact_id' => $individualID]); + $this->assertEquals(0, $email['is_bulkmail']); + $this->callAPISuccess('Email', 'create', ['id' => $email['id'], 'is_bulkmail' => 1]); + $email = $this->callAPISuccessGetSingle('Email', ['contact_id' => $individualID]); + $this->assertEquals(1, $email['is_bulkmail']); + $email2 = $this->callAPISuccess('Email', 'create', ['contact_id' => $individualID, 'email' => 'mail@Example.com', 'is_bulkmail' => 1]); + $emails = $this->callAPISuccess('Email', 'get', ['contact_id' => $individualID])['values']; + $this->assertEquals(0, $emails[$email['id']]['is_bulkmail']); + $this->assertEquals(1, $emails[$email2['id']]['is_bulkmail']); } }