From 5b4b95094616530759d8558ff31477afedf37d5c Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 13 Apr 2020 16:07:30 +1200 Subject: [PATCH] Convert bcc field to use an entity reference. Note I've restricted to the bcc field for now as having it different makes it easier to do comparitive testing. The other fields will follow once this is merged --- CRM/Contact/Form/Task/EmailTrait.php | 87 +++++++++++++------ CRM/Core/BAO/Email.php | 20 +++++ api/v3/Email.php | 5 +- templates/CRM/Contact/Form/Task/Email.tpl | 4 +- .../CRM/Contact/Form/Task/EmailCommonTest.php | 43 +++++++-- 5 files changed, 122 insertions(+), 37 deletions(-) diff --git a/CRM/Contact/Form/Task/EmailTrait.php b/CRM/Contact/Form/Task/EmailTrait.php index fd24e0423c..0db21dbdf6 100644 --- a/CRM/Contact/Form/Task/EmailTrait.php +++ b/CRM/Contact/Form/Task/EmailTrait.php @@ -15,6 +15,8 @@ * @copyright CiviCRM LLC https://civicrm.org/licensing */ +use Civi\Api4\Email; + /** * This class provides the common functionality for tasks that send emails. */ @@ -75,6 +77,8 @@ trait CRM_Contact_Form_Task_EmailTrait { */ public $isSearchContext = TRUE; + public $contactEmails = []; + /** * Getter for isSearchContext. * @@ -135,7 +139,7 @@ trait CRM_Contact_Form_Task_EmailTrait { $this->assign('suppressForm', FALSE); $this->assign('emailTask', TRUE); - $toArray = $ccArray = $bccArray = []; + $toArray = $ccArray = []; $suppressedEmails = 0; //here we are getting logged in user id as array but we need target contact id. CRM-5988 $cid = $this->get('cid'); @@ -152,7 +156,11 @@ trait CRM_Contact_Form_Task_EmailTrait { ]; $to = $this->add('text', 'to', ts('To'), $emailAttributes, TRUE); $cc = $this->add('text', 'cc_id', ts('CC'), $emailAttributes); - $bcc = $this->add('text', 'bcc_id', ts('BCC'), $emailAttributes); + + $this->addEntityRef('bcc_id', ts('BCC'), [ + 'entity' => 'Email', + 'multiple' => TRUE, + ]); if ($to->getValue()) { $this->_toContactIds = $this->_contactIds = []; @@ -162,7 +170,7 @@ trait CRM_Contact_Form_Task_EmailTrait { $setDefaults = FALSE; } - $elements = ['to', 'cc', 'bcc']; + $elements = ['to', 'cc']; $this->_allContactIds = $this->_toContactIds = $this->_contactIds; foreach ($elements as $element) { if ($$element->getValue()) { @@ -180,10 +188,6 @@ trait CRM_Contact_Form_Task_EmailTrait { case 'cc': $this->_ccContactIds[] = $contactId; break; - - case 'bcc': - $this->_bccContactIds[] = $contactId; - break; } $this->_allContactIds[] = $contactId; @@ -253,12 +257,6 @@ trait CRM_Contact_Form_Task_EmailTrait { 'id' => "$contactId::{$email}", ]; } - elseif (in_array($contactId, $this->_bccContactIds)) { - $bccArray[] = [ - 'text' => '"' . $value['sort_name'] . '" <' . $email . '>', - 'id' => "$contactId::{$email}", - ]; - } } } @@ -269,7 +267,6 @@ trait CRM_Contact_Form_Task_EmailTrait { $this->assign('toContact', json_encode($toArray)); $this->assign('ccContact', json_encode($ccArray)); - $this->assign('bccContact', json_encode($bccArray)); $this->assign('suppressedEmails', $suppressedEmails); @@ -403,6 +400,7 @@ trait CRM_Contact_Form_Task_EmailTrait { * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception * @throws \Civi\API\Exception\UnauthorizedException + * @throws \API_Exception */ public function submit($formValues) { $this->saveMessageTemplate($formValues); @@ -415,9 +413,9 @@ trait CRM_Contact_Form_Task_EmailTrait { $subject = $formValues['subject']; // CRM-13378: Append CC and BCC information at the end of Activity Details and format cc and bcc fields - $elements = ['cc_id', 'bcc_id']; + $elements = ['cc_id']; $additionalDetails = NULL; - $ccValues = $bccValues = []; + $ccValues = []; foreach ($elements as $element) { if (!empty($formValues[$element])) { $allEmails = explode(',', $formValues[$element]); @@ -430,24 +428,19 @@ trait CRM_Contact_Form_Task_EmailTrait { $ccValues['details'][] = "" . $this->_contactDetails[$contactId]['display_name'] . ""; break; - case 'bcc_id': - $bccValues['email'][] = '"' . $this->_contactDetails[$contactId]['sort_name'] . '" <' . $email . '>'; - $bccValues['details'][] = "" . $this->_contactDetails[$contactId]['display_name'] . ""; - break; } } } } + $cc = ''; - $cc = $bcc = ''; if (!empty($ccValues)) { $cc = implode(',', $ccValues['email']); $additionalDetails .= "\ncc : " . implode(", ", $ccValues['details']); } - if (!empty($bccValues)) { - $bcc = implode(',', $bccValues['email']); - $additionalDetails .= "\nbcc : " . implode(", ", $bccValues['details']); - } + $bccArray = explode(',', $formValues['bcc_id'] ?? ''); + $bcc = $this->getEmailString($bccArray); + $additionalDetails .= empty($bccArray) ? '' : "\nbcc : " . $this->getEmailUrlString($bccArray); // CRM-5916: prepend case id hash to CiviCase-originating emails’ subjects if (isset($this->_caseId) && is_numeric($this->_caseId)) { @@ -635,4 +628,48 @@ trait CRM_Contact_Form_Task_EmailTrait { return $return; } + /** + * Get the string for the email IDs. + * + * @param array $emailIDs + * Array of email IDs. + * + * @return string + * e.g. "Smith, Bob". + * + * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException + */ + protected function getEmailString(array $emailIDs): string { + $emails = Email::get() + ->addWhere('id', 'IN', $emailIDs) + ->setCheckPermissions(FALSE) + ->setSelect(['contact_id', 'email', 'contact.sort_name', 'contact.display_name'])->execute(); + $emailStrings = []; + foreach ($emails as $email) { + $this->contactEmails[$email['id']] = $email; + $emailStrings[] = '"' . $email['contact.sort_name'] . '" <' . $email['email'] . '>'; + } + return implode(',', $emailStrings); + } + + /** + * Get the url string. + * + * This is called after the contacts have been retrieved so we don't need to re-retrieve. + * + * @param array $emailIDs + * + * @return string + * e.g. Bob Smith' + */ + protected function getEmailUrlString(array $emailIDs): string { + $urlString = ''; + foreach ($emailIDs as $email) { + $contactURL = CRM_Utils_System::url('civicrm/contact/view', ['reset' => 1, 'force' => 1, 'cid' => $this->contactEmails[$email]['contact_id']], TRUE); + $urlString .= "" . $this->contactEmails[$email]['contact.display_name'] . ''; + } + return $urlString; + } + } diff --git a/CRM/Core/BAO/Email.php b/CRM/Core/BAO/Email.php index c8bcb650cf..3ec6dea97f 100644 --- a/CRM/Core/BAO/Email.php +++ b/CRM/Core/BAO/Email.php @@ -345,4 +345,24 @@ AND reset_date IS NULL return CRM_Contact_BAO_Contact::deleteObjectWithPrimary('Email', $id); } + /** + * Get filters for entity reference fields. + * + * @return array + */ + public static function getEntityRefFilters() { + $contactFields = CRM_Contact_BAO_Contact::getEntityRefFilters(); + foreach ($contactFields as $index => &$contactField) { + if (!empty($contactField['entity'])) { + // For now email_getlist can't parse state, country etc. + unset($contactFields[$index]); + } + elseif ($contactField['key'] !== 'contact_id') { + $contactField['entity'] = 'Contact'; + $contactField['key'] = 'contact_id.' . $contactField['key']; + } + } + return $contactFields; + } + } diff --git a/api/v3/Email.php b/api/v3/Email.php index d4c08250d8..a20bd0dd68 100644 --- a/api/v3/Email.php +++ b/api/v3/Email.php @@ -91,7 +91,6 @@ function civicrm_api3_email_get($params) { function _civicrm_api3_email_getlist_defaults(&$request) { return [ 'description_field' => [ - 'contact_id.sort_name', 'email', ], 'params' => [ @@ -100,6 +99,10 @@ function _civicrm_api3_email_getlist_defaults(&$request) { 'contact_id.is_deceased' => 0, 'contact_id.do_not_email' => 0, ], + // Note that changing this to display name affects query performance. The label field is used + // for sorting & mysql will prefer to use the index on the ORDER BY field. So if this is changed + // to display name then the filtering will bypass the index. In testing this took around 30 times + // as long. 'label_field' => 'contact_id.sort_name', // If no results from sort_name try email. 'search_field' => 'contact_id.sort_name', diff --git a/templates/CRM/Contact/Form/Task/Email.tpl b/templates/CRM/Contact/Form/Task/Email.tpl index 825dfaf8bc..9f0517df2b 100644 --- a/templates/CRM/Contact/Form/Task/Email.tpl +++ b/templates/CRM/Contact/Form/Task/Email.tpl @@ -125,12 +125,10 @@ CRM.$(function($) { {/literal} var toContact = {if $toContact}{$toContact}{else}''{/if}, - ccContact = {if $ccContact}{$ccContact}{else}''{/if}, - bccContact = {if $bccContact}{$bccContact}{else}''{/if}; + ccContact = {if $ccContact}{$ccContact}{else}''{/if}; {literal} emailSelect('#to', toContact); emailSelect('#cc_id', ccContact); - emailSelect('#bcc_id', bccContact); }); diff --git a/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php b/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php index 10939b6235..5671eb154b 100644 --- a/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php +++ b/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php @@ -8,10 +8,13 @@ | and copyright information, see https://civicrm.org/licensing | +--------------------------------------------------------------------+ */ - /** - * Test class for CRM_Contact_Form_Task_EmailCommon. - * @group headless - */ + +use Civi\Api4\Activity; + +/** + * Test class for CRM_Contact_Form_Task_EmailCommon. + * @group headless + */ class CRM_Contact_Form_Task_EmailCommonTest extends CiviUnitTestCase { /** @@ -31,6 +34,16 @@ class CRM_Contact_Form_Task_EmailCommonTest extends CiviUnitTestCase { ]); } + /** + * Cleanup after test class. + * + * Make sure the setting is returned to 'stock'. + */ + public function tearDown() { + Civi::settings()->set('allow_mail_from_logged_in_contact', 0); + parent::tearDown(); + } + /** * Test generating domain emails * @@ -49,17 +62,27 @@ class CRM_Contact_Form_Task_EmailCommonTest extends CiviUnitTestCase { /** * Test email uses signature. * + * @throws \API_Exception * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception * @throws \Civi\API\Exception\UnauthorizedException */ public function testPostProcessWithSignature() { $mut = new CiviMailUtils($this, TRUE); + $bcc1 = $this->individualCreate(['email' => 'bcc1@example.com']); + $bcc2 = $this->individualCreate(['email' => 'bcc2@example.com']); + $emails = $this->callAPISuccess('Email', 'getlist', ['input' => 'bcc'])['values']; + $bcc = []; + foreach ($emails as $email) { + $bcc[] = $email['id']; + } + $bcc = implode(',', $bcc); + Civi::settings()->set('allow_mail_from_logged_in_contact', 1); $loggedInContactID = $this->createLoggedInUser(); - $form = new CRM_Contact_Form_Task_Email(); - $_SERVER['REQUEST_METHOD'] = 'GET'; - $form->controller = new CRM_Core_Controller(); + /* @var CRM_Contact_Form_Task_Email $form*/ + $form = $this->getFormObject('CRM_Contact_Form_Task_Email'); + for ($i = 0; $i < 27; $i++) { $email = 'spy' . $i . '@secretsquirrels.com'; $contactID = $this->individualCreate(['email' => $email]); @@ -85,12 +108,16 @@ class CRM_Contact_Form_Task_EmailCommonTest extends CiviUnitTestCase { $form->submit(array_merge($form->_defaultValues, [ 'from_email_address' => $loggedInEmail['id'], 'subject' => 'Really interesting stuff', + 'bcc_id' => $bcc, ])); $mut->checkMailLog([ 'This is a test Signature', ]); $mut->stop(); - Civi::settings()->set('allow_mail_from_logged_in_contact', 0); + $activity = Activity::get()->setCheckPermissions(FALSE)->setSelect(['details'])->execute()->first(); + $bccUrl1 = CRM_Utils_System::url('civicrm/contact/view', ['reset' => 1, 'force' => 1, 'cid' => $bcc1], TRUE); + $bccUrl2 = CRM_Utils_System::url('civicrm/contact/view', ['reset' => 1, 'force' => 1, 'cid' => $bcc2], TRUE); + $this->assertContains("bcc : Mr. Anthony Anderson IIMr. Anthony Anderson II", $activity['details']); } } -- 2.25.1