Skip to content

Commit

Permalink
Remove unnecessary, and possibly incorrect query from email update
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@eileenmcnaughton
eileenmcnaughton committed Dec 11, 2019
1 parent ee3bab5 commit 2ffdaed
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 53 deletions.
12 changes: 8 additions & 4 deletions CRM/Core/BAO/Email.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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);
Expand Down
45 changes: 25 additions & 20 deletions tests/phpunit/CRM/Core/BAO/EmailTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
Expand All @@ -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();
Expand All @@ -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).
Expand All @@ -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).
Expand All @@ -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.'
);
Expand All @@ -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 = [
Expand All @@ -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();
Expand Down
Loading

0 comments on commit 2ffdaed

Please sign in to comment.