From 30420aa3c548149281d437cda1cb39bca81e2ef9 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 6 Apr 2020 15:48:00 +1200 Subject: [PATCH] Add / make fit for purpose email.getlist api call The function CRM_Contact_Page_AJAX::getContactEmail is one of our earlier ajax attempts & this approach has been largely replaced with entity Reference fields. In order to switch over we need to bring Email.getlist api to parity which means 1) searching on sortname first, if less than 10 results on emails include emails 2) appropriate respect for includeWildCardInName (this should already be in the generic getlist) 3) filter out on_hold, is_deceased, do_not_email 4) acl support (should already be part of the api). The trickiest of these to support is the first - because we need to avoid using a non-performant OR My current solution is the idea of a fallback field to search if the search results are less than the limit. in most cases this won't require a second query but when it does it should be fairly quick. --- api/v3/Email.php | 37 ++++++++++++++++++- api/v3/Generic/Getlist.php | 28 +++++++++++++- .../CRM/Contact/Form/Task/EmailCommonTest.php | 16 +++++++- tests/phpunit/api/v3/ContactTest.php | 2 + tests/phpunit/api/v3/EmailTest.php | 22 +++++++++++ 5 files changed, 102 insertions(+), 3 deletions(-) diff --git a/api/v3/Email.php b/api/v3/Email.php index ad4f61995608..d4c08250d8be 100644 --- a/api/v3/Email.php +++ b/api/v3/Email.php @@ -23,6 +23,9 @@ * * @return array * API result array + * + * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException */ function civicrm_api3_email_create($params) { return _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params, 'Email'); @@ -37,7 +40,6 @@ function civicrm_api3_email_create($params) { * Array of parameters determined by getfields. */ function _civicrm_api3_email_create_spec(&$params) { - // TODO a 'clever' default should be introduced $params['is_primary']['api.default'] = 0; $params['email']['api.required'] = 1; $params['contact_id']['api.required'] = 1; @@ -55,6 +57,10 @@ function _civicrm_api3_email_create_spec(&$params) { * * @return array * API result array. + * + * @throws \API_Exception + * @throws \CiviCRM_API3_Exception + * @throws \Civi\API\Exception\UnauthorizedException */ function civicrm_api3_email_delete($params) { return _civicrm_api3_basic_delete(_civicrm_api3_get_BAO(__FUNCTION__), $params); @@ -72,3 +78,32 @@ function civicrm_api3_email_delete($params) { function civicrm_api3_email_get($params) { return _civicrm_api3_basic_get(_civicrm_api3_get_BAO(__FUNCTION__), $params); } + +/** + * Set default getlist parameters. + * + * @see _civicrm_api3_generic_getlist_defaults + * + * @param array $request + * + * @return array + */ +function _civicrm_api3_email_getlist_defaults(&$request) { + return [ + 'description_field' => [ + 'contact_id.sort_name', + 'email', + ], + 'params' => [ + 'on_hold' => 0, + 'contact_id.is_deleted' => 0, + 'contact_id.is_deceased' => 0, + 'contact_id.do_not_email' => 0, + ], + 'label_field' => 'contact_id.sort_name', + // If no results from sort_name try email. + 'search_field' => 'contact_id.sort_name', + 'search_field_fallback' => 'email', + ]; + +} diff --git a/api/v3/Generic/Getlist.php b/api/v3/Generic/Getlist.php index 464bd09be028..2fecc84c4fcf 100644 --- a/api/v3/Generic/Getlist.php +++ b/api/v3/Generic/Getlist.php @@ -19,6 +19,7 @@ * @param array $apiRequest * * @return mixed + * @throws \CiviCRM_API3_Exception */ function civicrm_api3_generic_getList($apiRequest) { $entity = _civicrm_api_get_entity_name_from_camel($apiRequest['entity']); @@ -37,6 +38,31 @@ function civicrm_api3_generic_getList($apiRequest) { $request['params']['check_permissions'] = !empty($apiRequest['params']['check_permissions']); $result = civicrm_api3($entity, 'get', $request['params']); + if (!empty($request['input']) && !empty($defaults['search_field_fallback']) && $result['count'] < $request['params']['options']['limit']) { + // We support a field fallback. Note we don't do this as an OR query because that could easily + // bypass an index & kill the server. We just 'pad' the results if needed with the second + // query - this is effectively the same as what the old Ajax::getContactEmail function did. + // Since these queries should be quick & often only one should be needed this is a simpler alternative + // to constructing a UNION via the api. + $request['params'][$defaults['search_field_fallback']] = $request['params'][$defaults['search_field']]; + if ($request['params']['options']['sort'] === $defaults['search_field']) { + // The way indexing works here is that the order by field will be chosen in preference to the + // filter field. This can result in really bad performance so use the filter field for the sort. + // See https://github.com/civicrm/civicrm-core/pull/16993 for performance test results. + $request['params']['options']['sort'] = $defaults['search_field_fallback']; + } + // Exclude anything returned from the previous query since we are looking for additional rows in this + // second query. + $request['params'][$defaults['search_field']] = ['NOT LIKE' => $request['params'][$defaults['search_field_fallback']]['LIKE']]; + $request['params']['options']['limit'] -= $result['count']; + $result2 = civicrm_api3($entity, 'get', $request['params']); + $result['values'] = array_merge($result['values'], $result2['values']); + $result['count'] = count($result['values']); + } + else { + // Re-index to sequential = 0. + $result['values'] = array_merge($result['values']); + } // Hey api, would you like to format the output? $fnName = "_civicrm_api3_{$entity}_getlist_output"; @@ -98,7 +124,7 @@ function _civicrm_api3_generic_getList_defaults($entity, &$request, $apiDefaults $request += $apiDefaults + $defaults; // Default api params $params = [ - 'sequential' => 1, + 'sequential' => 0, 'options' => [], ]; // When searching e.g. autocomplete diff --git a/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php b/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php index 0326787d18ef..8fe8462266a2 100644 --- a/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php +++ b/tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php @@ -14,13 +14,18 @@ */ class CRM_Contact_Form_Task_EmailCommonTest extends CiviUnitTestCase { + /** + * Set up for tests. + * + * @throws \CRM_Core_Exception + */ protected function setUp() { parent::setUp(); $this->_contactIds = [ $this->individualCreate(['first_name' => 'Antonia', 'last_name' => 'D`souza']), $this->individualCreate(['first_name' => 'Anthony', 'last_name' => 'Collins']), ]; - $this->_optionValue = $this->callApiSuccess('optionValue', 'create', [ + $this->_optionValue = $this->callAPISuccess('optionValue', 'create', [ 'label' => '"Seamus Lee" ', 'option_group_id' => 'from_email_address', ]); @@ -28,6 +33,8 @@ protected function setUp() { /** * Test generating domain emails + * + * @throws \CRM_Core_Exception */ public function testDomainEmailGeneration() { $emails = CRM_Core_BAO_Email::domainEmails(); @@ -39,6 +46,13 @@ public function testDomainEmailGeneration() { $this->assertEquals('"Seamus Lee" ', $optionValue['values'][$this->_optionValue['id']]['label']); } + /** + * Test email uses signature. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception + * @throws \Civi\API\Exception\UnauthorizedException + */ public function testPostProcessWithSignature() { $mut = new CiviMailUtils($this, TRUE); Civi::settings()->set('allow_mail_from_logged_in_contact', 1); diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 90f0e22e82f6..db3fd1efb170 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -3683,6 +3683,8 @@ public function testReturnCityProfile() { /** * CRM-15443 - ensure getlist api does not return deleted contacts. + * + * @throws \CRM_Core_Exception */ public function testGetlistExcludeConditions() { $name = 'Scarabée'; diff --git a/tests/phpunit/api/v3/EmailTest.php b/tests/phpunit/api/v3/EmailTest.php index 7f2e413936aa..a54183e71c6f 100644 --- a/tests/phpunit/api/v3/EmailTest.php +++ b/tests/phpunit/api/v3/EmailTest.php @@ -495,4 +495,26 @@ public function testSetBulkEmail() { $this->assertEquals(1, $emails[$email2['id']]['is_bulkmail']); } + /** + * Test getlist. + * + * @throws \CRM_Core_Exception + */ + public function testGetlist() { + $name = 'Scarabée'; + $emailMatchContactID = $this->individualCreate(['last_name' => $name, 'email' => 'bob@bob.com']); + $emailMatchEmailID = $this->callAPISuccessGetValue('Email', ['return' => 'id', 'contact_id' => $emailMatchContactID]); + $this->individualCreate(['last_name' => $name, 'email' => 'bob@bob.com', 'is_deceased' => 1]); + $this->individualCreate(['last_name' => $name, 'email' => 'bob@bob.com', 'is_deleted' => 1]); + $this->individualCreate(['last_name' => $name, 'api.email.create' => ['email' => 'bob@bob.com', 'on_hold' => 1]]); + $this->individualCreate(['last_name' => $name, 'do_not_email' => 1, 'api.email.create' => ['email' => 'bob@bob.com']]); + $nameMatchContactID = $this->individualCreate(['last_name' => 'bob', 'email' => 'blah@example.com']); + $nameMatchEmailID = $this->callAPISuccessGetValue('Email', ['return' => 'id', 'contact_id' => $nameMatchContactID]); + // We should get only the active live email-able contact. + $result = $this->callAPISuccess('Email', 'getlist', ['input' => 'bob'])['values']; + $this->assertCount(2, $result); + $this->assertEquals($nameMatchEmailID, $result[0]['id']); + $this->assertEquals($emailMatchEmailID, $result[1]['id']); + } + }