Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add / make fit for purpose email.getlist api call #16993

Merged
merged 1 commit into from
Apr 8, 2020
Merged

Add / make fit for purpose email.getlist api call #16993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion api/v3/Email.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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',
];

}
28 changes: 27 additions & 1 deletion api/v3/Generic/Getlist.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand All @@ -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']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add something to the params here that excludes the original results.
NOT IN(array_keys($results)) would work partially, but it would only exclude the current page.
What about a "NOT LIKE" clause to exclude any results that would have been returned by searching on the original field.
The reason I bring this up is because we are dealing with a paging situation so duplicate results can mess up the pager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw - that makes sense. In my testing it had no negative performance impact adding that - however my testing did demonstrate that the way it was doing the ordering WAS having a performance impact and that for this to perform well the sort_field needs to be the same as the filter field. I'm on the fence about doing a post-sort in php - as of now its

  1. sort_name matches, ordered by sort_name, labeled by sort_name padded with
  2. email matches, ordered by email, labeled by sort_name

Copy link
Member

@colemanw colemanw Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton this is looking really good. My one hesitation is that the 'NOT IN' approach will possibly return redundant results because of the pager. Ex:

  • Type a search query
  • It returns 10 results.
  • Scroll to the end and another 10 results appears (page 2)
  • Scroll down again and 5 results are fetched by the main $request. It then fires the fallback request, using 'NOT IN' to exclude the 5 current results.
  • Since it did not exclude results from pages 1 & 2 (only the five contacts from page 3 were excluded) you'll get duplicates on page 3+.
  • On page 4 you can get even more duplicates since main request will return 0 results, nothing will be excluded.

If it's not a big performance hit, the 'NOT LIKE' method could fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it was OK in the performance tests below so trying it

$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";
Expand Down Expand Up @@ -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
Expand Down
16 changes: 15 additions & 1 deletion tests/phpunit/CRM/Contact/Form/Task/EmailCommonTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,27 @@
*/
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" <[email protected]>',
'option_group_id' => 'from_email_address',
]);
}

/**
* Test generating domain emails
*
* @throws \CRM_Core_Exception
*/
public function testDomainEmailGeneration() {
$emails = CRM_Core_BAO_Email::domainEmails();
Expand All @@ -39,6 +46,13 @@ public function testDomainEmailGeneration() {
$this->assertEquals('"Seamus Lee" <[email protected]>', $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);
Expand Down
2 changes: 2 additions & 0 deletions tests/phpunit/api/v3/ContactTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
22 changes: 22 additions & 0 deletions tests/phpunit/api/v3/EmailTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' => '[email protected]']);
$emailMatchEmailID = $this->callAPISuccessGetValue('Email', ['return' => 'id', 'contact_id' => $emailMatchContactID]);
$this->individualCreate(['last_name' => $name, 'email' => '[email protected]', 'is_deceased' => 1]);
$this->individualCreate(['last_name' => $name, 'email' => '[email protected]', 'is_deleted' => 1]);
$this->individualCreate(['last_name' => $name, 'api.email.create' => ['email' => '[email protected]', 'on_hold' => 1]]);
$this->individualCreate(['last_name' => $name, 'do_not_email' => 1, 'api.email.create' => ['email' => '[email protected]']]);
$nameMatchContactID = $this->individualCreate(['last_name' => 'bob', 'email' => '[email protected]']);
$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']);
}

}