From 613643e0be5f3460160d72a5e61b35fc8f5967b3 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 20 Oct 2016 18:35:51 +1300 Subject: [PATCH] CRM-19547 rationalise expensive use of exactMatch search on quicksearch - do not do exact match when the string is just a wildcard (it will never be an exact match) - do not do exact match when the search is an email & there is no @ (it will never be an exact match) - do not do exact match when the search is for sort_name and prepending wildcards is disabled for the site (it is functionally equivalent Change-Id: I17506264eb24f88b6818b014ce248580df1d962c --- api/v3/Contact.php | 74 +++++++++++++--- tests/phpunit/api/v3/ContactTest.php | 125 ++++++++++++++++++++++----- 2 files changed, 164 insertions(+), 35 deletions(-) diff --git a/api/v3/Contact.php b/api/v3/Contact.php index 0a2f686d700a..1805ed204908 100644 --- a/api/v3/Contact.php +++ b/api/v3/Contact.php @@ -806,6 +806,7 @@ function civicrm_api3_contact_getquick($params) { if ($aclWhere) { $where .= " AND $aclWhere "; } + $isPrependWildcard = \Civi::settings()->get('includeWildCardInName'); if (!empty($params['org'])) { $where .= " AND contact_type = \"Organization\""; @@ -818,7 +819,7 @@ function civicrm_api3_contact_getquick($params) { (int) $params['employee_id'], 'employer_id' )) { - if ($config->includeWildCardInName) { + if ($isPrependWildcard) { $strSearch = "%$name%"; } else { @@ -865,7 +866,7 @@ function civicrm_api3_contact_getquick($params) { $rel = CRM_Utils_Type::escape($relation[2], 'String'); } - if ($config->includeWildCardInName) { + if ($isPrependWildcard) { $strSearch = "%$name%"; } else { @@ -880,15 +881,13 @@ function civicrm_api3_contact_getquick($params) { //CRM-10687 if (!empty($params['field_name']) && !empty($params['table_name'])) { $whereClause = " WHERE ( $table_name.$field_name LIKE '$strSearch') {$where}"; - $exactWhereClause = " WHERE ( $table_name.$field_name = '$name') {$where}"; // Search by id should be exact if ($field_name == 'id' || $field_name == 'external_identifier') { - $whereClause = $exactWhereClause; + $whereClause = " WHERE ( $table_name.$field_name = '$name') {$where}"; } } else { $whereClause = " WHERE ( sort_name LIKE '$strSearch' $includeNickName ) {$where} "; - $exactWhereClause = " WHERE ( sort_name LIKE '$name' $exactIncludeNickName ) {$where} "; if ($config->includeEmailInName) { if (!in_array('email', $list)) { $includeEmailFrom = "LEFT JOIN civicrm_email eml ON ( cc.id = eml.contact_id AND eml.is_primary = 1 )"; @@ -913,12 +912,7 @@ function civicrm_api3_contact_getquick($params) { INNER JOIN civicrm_uf_match um ON (um.contact_id=cc.id) "; } - - $orderByInner = $orderByOuter = "ORDER BY exactFirst"; - if ($config->includeOrderByClause) { - $orderByInner = "ORDER BY exactFirst, sort_name"; - $orderByOuter .= ", sort_name"; - } + $orderBy = _civicrm_api3_quicksearch_get_order_by($name, $isPrependWildcard, $field_name); //CRM-5954 $query = " @@ -932,7 +926,7 @@ function civicrm_api3_contact_getquick($params) { {$aclFrom} {$additionalFrom} {$whereClause} - {$orderByInner} + {$orderBy} LIMIT 0, {$limit} ) "; @@ -947,13 +941,13 @@ function civicrm_api3_contact_getquick($params) { {$aclFrom} {$additionalFrom} {$includeEmailFrom} {$emailWhere} AND cc.is_deleted = 0 " . ($aclWhere ? " AND $aclWhere " : '') . " - {$orderByInner} + {$orderBy} LIMIT 0, {$limit} ) "; } $query .= ") t - {$orderByOuter} + {$orderBy} LIMIT 0, {$limit} "; @@ -1008,6 +1002,58 @@ function civicrm_api3_contact_getquick($params) { return civicrm_api3_create_success($contactList, $params, 'Contact', 'getquick'); } +/** + * Get the order by string for the quicksearch query. + * + * Get the order by string. The string might be + * - sort name if there is no search value provided and the site is configured + * to search by sort name + * - empty if there is no search value provided and the site is not configured + * to search by sort name + * - exactFirst and then sort name if a search value is provided and the site is configured + * to search by sort name + * - exactFirst if a search value is provided and the site is not configured + * to search by sort name + * + * exactFirst means 'yes if the search value exactly matches the searched field. else no'. + * It is intended to prioritise exact matches for the entered string so on a first name search + * for 'kath' contacts with a first name of exactly Kath rise to the top. + * + * On short strings it is expensive. Per CRM-19547 there is still an open question + * as to whether we should only do exactMatch on a minimum length or on certain fields. + * + * However, we have mitigated this somewhat by not doing an exact match search on + * empty strings, non-wildcard sort-name searches and email searches where there is + * no @ after the first character. + * + * For the user it is further mitigated by the fact they just don't know the + * slower queries are firing. If they type 'smit' slowly enough 4 queries will trigger + * but if the first 3 are slow the first result they see may be off the 4th query. + * + * @param string $name + * @param bool $isPrependWildcard + * @param string $field_name + * + * @return string + */ +function _civicrm_api3_quicksearch_get_order_by($name, $isPrependWildcard, $field_name) { + $skipExactMatch = ($name === '%'); + if ($field_name === 'email' && !strpos('@', $name)) { + $skipExactMatch = TRUE; + } + + if (!\Civi::settings()->get('includeOrderByClause')) { + return $skipExactMatch ? '' : "ORDER BY exactFirst"; + } + if ($skipExactMatch || (!$isPrependWildcard && $field_name === 'sort_name')) { + // If there is no wildcard then sorting by exactFirst would have the same + // effect as just a sort_name search, but slower. + return "ORDER BY sort_name"; + } + + return "ORDER BY exactFirst, sort_name"; +} + /** * Declare deprecated api functions. * diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 480cc9355a6b..cfe690a31cd4 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -2407,19 +2407,100 @@ public function testGetquickPermissionCRM13744() { * * The search string 'b' & 'bob' both return ordered by sort_name if includeOrderByClause * is true (default) but if it is false then matches are returned in ID order. + * + * @dataProvider getSearchSortOptions */ - public function testGetQuickExactFirst() { + public function testGetQuickExactFirst($searchParameters, $settings, $firstContact, $secondContact = NULL) { $this->getQuickSearchSampleData(); - $result = $this->callAPISuccess('contact', 'getquick', array('name' => 'b')); - $this->assertEquals('A Bobby, Bobby', $result['values'][0]['sort_name']); - $this->assertEquals('B Bobby, Bobby', $result['values'][1]['sort_name']); - $result = $this->callAPISuccess('contact', 'getquick', array('name' => 'bob')); - $this->assertEquals('A Bobby, Bobby', $result['values'][0]['sort_name']); - $this->assertEquals('B Bobby, Bobby', $result['values'][1]['sort_name']); - $this->callAPISuccess('Setting', 'create', array('includeOrderByClause' => FALSE)); - $result = $this->callAPISuccess('contact', 'getquick', array('name' => 'bob')); - $this->assertEquals('Bob, Bob', $result['values'][0]['sort_name']); - $this->assertEquals('A Bobby, Bobby', $result['values'][1]['sort_name']); + $this->callAPISuccess('Setting', 'create', $settings); + $result = $this->callAPISuccess('contact', 'getquick', $searchParameters); + $this->assertEquals($firstContact, $result['values'][0]['sort_name']); + $this->assertEquals($secondContact, $result['values'][1]['sort_name']); + $this->callAPISuccess('Setting', 'create', array('includeWildCardInName' => TRUE, 'includeOrderByClause' => TRUE)); + } + + public function getSearchSortOptions() { + $firstAlphabeticalContactBySortName = 'A Bobby, Bobby'; + $secondAlphabeticalContactBySortName = 'Aadvark, Bob'; + $secondAlphabeticalContactWithEmailBySortName = 'Bob, Bob'; + $firstAlphabeticalContactFirstNameBob = 'Aadvark, Bob'; + $secondAlphabeticalContactFirstNameBob = 'Bob, Bob'; + $firstByIDContactFirstNameBob = 'Bob, Bob'; + $secondByIDContactFirstNameBob = 'K Bobby, Bob'; + $firstContactByID = 'Bob, Bob'; + $secondContactByID = 'E Bobby, Bobby'; + $bobLikeEmail = 'A Bobby, Bobby'; + + return array( + 'empty_search_basic' => array( + 'search_parameters' => array('name' => '%'), + 'settings' => array('includeWildCardInName' => TRUE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactBySortName, + 'second_contact' => $secondAlphabeticalContactBySortName, + ), + 'empty_search_basic_no_wildcard' => array( + 'search_parameters' => array('name' => '%'), + 'settings' => array('includeWildCardInName' => FALSE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactBySortName, + 'second_contact' => $secondAlphabeticalContactBySortName, + ), + 'single_letter_search_basic' => array( + 'search_parameters' => array('name' => 'b'), + 'settings' => array('includeWildCardInName' => TRUE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactBySortName, + 'second_contact' => $secondAlphabeticalContactBySortName, + ), + 'bob_search_basic' => array( + 'search_parameters' => array('name' => 'bob'), + 'settings' => array('includeWildCardInName' => TRUE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactBySortName, + 'second_contact' => $secondAlphabeticalContactBySortName, + ), + 'bob_search_no_orderby' => array( + 'search_parameters' => array('name' => 'bob'), + 'settings' => array('includeWildCardInName' => TRUE, 'includeOrderByClause' => FALSE), + 'first_contact' => $firstContactByID, + 'second_contact' => $secondContactByID, + ), + 'bob_search_no_wildcard' => array( + 'search_parameters' => array('name' => 'bob'), + 'settings' => array('includeWildCardInName' => FALSE, 'includeOrderByClause' => TRUE), + 'second_contact' => $bobLikeEmail, + 'first_contact' => $secondAlphabeticalContactFirstNameBob, + ), + // This should be the same as just no wildcard as if we had an exactMatch while searching by + // sort name it would rise to the top CRM-19547 + 'bob_search_no_wildcard_no_orderby' => array( + 'search_parameters' => array('name' => 'bob'), + 'settings' => array('includeWildCardInName' => FALSE, 'includeOrderByClause' => TRUE), + 'second_contact' => $bobLikeEmail, + 'first_contact' => $secondAlphabeticalContactFirstNameBob, + ), + 'first_name_search_basic' => array( + 'search_parameters' => array('name' => 'bob', 'field_name' => 'first_name'), + 'settings' => array('includeWildCardInName' => TRUE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactFirstNameBob, + 'second_contact' => $secondAlphabeticalContactFirstNameBob, + ), + 'first_name_search_no_wildcard' => array( + 'search_parameters' => array('name' => 'bob', 'field_name' => 'first_name'), + 'settings' => array('includeWildCardInName' => FALSE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactFirstNameBob, + 'second_contact' => $secondAlphabeticalContactFirstNameBob, + ), + 'first_name_search_no_orderby' => array( + 'search_parameters' => array('name' => 'bob', 'field_name' => 'first_name'), + 'settings' => array('includeWildCardInName' => TRUE, 'includeOrderByClause' => FALSE), + 'first_contact' => $firstByIDContactFirstNameBob, + 'second_contact' => $secondByIDContactFirstNameBob, + ), + 'email_search_basic' => array( + 'search_parameters' => array('name' => 'bob', 'field_name' => 'email', 'table_name' => 'eml'), + 'settings' => array('includeWildCardInName' => FALSE, 'includeOrderByClause' => TRUE), + 'first_contact' => $firstAlphabeticalContactBySortName, + 'second_contact' => $secondAlphabeticalContactWithEmailBySortName, + ), + ); } /** @@ -2432,9 +2513,9 @@ public function testGetQuickEmail() { 'name' => 'c', )); $expectedData = array( + 'A Bobby, Bobby :: bob@bobby.com', 'Bob, Bob :: bob@bob.com', 'C Bobby, Bobby', - 'E Bobby, Bobby :: bob@bobby.com', 'H Bobby, Bobby :: bob@h.com', 'Second Domain', $this->callAPISuccessGetValue('Contact', array('id' => $loggedInContactID, 'return' => 'last_name')) . ', Logged In :: anthony_anderson@civicrm.org', @@ -2481,9 +2562,9 @@ public function testGetQuickEmailACL() { // Without the acl it would be 6 like the previous email getquick test. $this->assertEquals(5, $result['count']); $expectedData = array( + 'A Bobby, Bobby :: bob@bobby.com', 'Bob, Bob :: bob@bob.com', 'C Bobby, Bobby', - 'E Bobby, Bobby :: bob@bobby.com', 'Second Domain', $this->callAPISuccessGetValue('Contact', array('id' => $loggedInContactID, 'return' => 'last_name')) . ', Logged In :: anthony_anderson@civicrm.org', ); @@ -2524,14 +2605,14 @@ public function testGetQuickID() { 'table_name' => 'cc', )); $this->assertEquals(1, $result['count']); - $this->assertEquals('A Bobby, Bobby', $result['values'][0]['sort_name']); + $this->assertEquals('E Bobby, Bobby', $result['values'][0]['sort_name']); $result = $this->callAPISuccess('contact', 'getquick', array( 'name' => $max + 2, 'field_name' => 'contact_id', 'table_name' => 'cc', )); $this->assertEquals(1, $result['count']); - $this->assertEquals('A Bobby, Bobby', $result['values'][0]['sort_name']); + $this->assertEquals('E Bobby, Bobby', $result['values'][0]['sort_name']); } /** @@ -2548,6 +2629,7 @@ public function testGetQuickFirstName() { 'table_name' => 'cc', )); $expected = array( + 'Aadvark, Bob', 'Bob, Bob', 'K Bobby, Bob', 'A Bobby, Bobby', @@ -2559,7 +2641,7 @@ public function testGetQuickFirstName() { $this->callAPISuccess('Setting', 'create', array('includeOrderByClause' => FALSE)); $result = $this->callAPISuccess('contact', 'getquick', array('name' => 'bob')); $this->assertEquals('Bob, Bob', $result['values'][0]['sort_name']); - $this->assertEquals('A Bobby, Bobby', $result['values'][1]['sort_name']); + $this->assertEquals('E Bobby, Bobby', $result['values'][1]['sort_name']); } /** @@ -2568,7 +2650,7 @@ public function testGetQuickFirstName() { public function testGetQuickFirstNameACLs() { $this->getQuickSearchSampleData(); $userID = $this->createLoggedInUser(); - $this->callAPISuccess('Setting', 'create', array('includeOrderByClause' => TRUE)); + $this->callAPISuccess('Setting', 'create', array('includeOrderByClause' => TRUE, 'search_autocomplete_count' => 15)); CRM_Core_Config::singleton()->userPermissionClass->permissions = array(); $result = $this->callAPISuccess('contact', 'getquick', array( 'name' => 'Bob', @@ -2584,9 +2666,9 @@ public function testGetQuickFirstNameACLs() { 'field_name' => 'first_name', 'table_name' => 'cc', )); - $this->assertEquals('K Bobby, Bob', $result['values'][1]['sort_name']); + $this->assertEquals('K Bobby, Bob', $result['values'][2]['sort_name']); // Without the ACL 9 would be bob@h.com. - $this->assertEquals('I Bobby, Bobby', $result['values'][9]['sort_name']); + $this->assertEquals('I Bobby, Bobby', $result['values'][10]['sort_name']); } /** @@ -2655,7 +2737,7 @@ public function testGetQuickCity() { public function getQuickSearchSampleData() { $contacts = array( array('first_name' => 'Bob', 'last_name' => 'Bob', 'external_identifier' => 'abc', 'email' => 'bob@bob.com'), - array('first_name' => 'Bobby', 'last_name' => 'A Bobby', 'external_identifier' => 'abcd'), + array('first_name' => 'Bobby', 'last_name' => 'E Bobby', 'external_identifier' => 'abcd'), array( 'first_name' => 'Bobby', 'last_name' => 'B Bobby', @@ -2677,13 +2759,14 @@ public function getQuickSearchSampleData() { ), ), array('first_name' => 'Bobby', 'last_name' => 'D Bobby', 'external_identifier' => 'efg'), - array('first_name' => 'Bobby', 'last_name' => 'E Bobby', 'external_identifier' => 'hij', 'email' => 'bob@bobby.com'), + array('first_name' => 'Bobby', 'last_name' => 'A Bobby', 'external_identifier' => 'hij', 'email' => 'bob@bobby.com'), array('first_name' => 'Bobby', 'last_name' => 'F Bobby', 'external_identifier' => 'klm'), array('first_name' => 'Bobby', 'last_name' => 'G Bobby', 'external_identifier' => 'nop'), array('first_name' => 'Bobby', 'last_name' => 'H Bobby', 'external_identifier' => 'qrs', 'email' => 'bob@h.com'), array('first_name' => 'Bobby', 'last_name' => 'I Bobby'), array('first_name' => 'Bobby', 'last_name' => 'J Bobby'), array('first_name' => 'Bob', 'last_name' => 'K Bobby', 'external_identifier' => 'bcdef'), + array('first_name' => 'Bob', 'last_name' => 'Aadvark'), ); foreach ($contacts as $type => $contact) { $contact['contact_type'] = 'Individual';