From ed92673b1ca8974c6dbd5af8d178281a7030dfe1 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 21 Jun 2016 18:42:20 +1200 Subject: [PATCH] CRM-18577 fix dedupe search by postcode Note there is a separate issue to add regex to this, but I opted to restrict this to refactoring + the bug fix, rather than include the improvement. Let's add a test when we improve! (CRM-18694) --- CRM/Contact/Page/AJAX.php | 125 +++++++++----- CRM/Core/BAO/PrevNextCache.php | 14 +- tests/phpunit/CRM/Contact/Page/AjaxTest.php | 178 ++++++++++++++++++++ 3 files changed, 269 insertions(+), 48 deletions(-) diff --git a/CRM/Contact/Page/AJAX.php b/CRM/Contact/Page/AJAX.php index 84052e751f..2da17695ea 100644 --- a/CRM/Contact/Page/AJAX.php +++ b/CRM/Contact/Page/AJAX.php @@ -665,51 +665,36 @@ LIMIT {$offset}, {$rowCount} $whereClause = $orderByClause = ''; $cacheKeyString = CRM_Dedupe_Merger::getMergeCacheKeyString($rgid, $gid); $searchRows = array(); - $selectorElements = array('is_selected', 'is_selected_input', 'src_image', 'src', 'src_email', 'src_street', 'src_postcode', 'dst_image', 'dst', 'dst_email', 'dst_street', 'dst_postcode', 'conflicts', 'weight', 'actions'); - foreach ($_REQUEST['columns'] as $columnInfo) { - if (!empty($columnInfo['search']['value'])) { - ${$columnInfo['data']} = CRM_Utils_Type::escape($columnInfo['search']['value'], 'String'); - } - } + $searchParams = self::getSearchOptionsFromRequest(); + $queryParams = array(); + $join = ''; $where = array(); - $searchData = CRM_Utils_Array::value('search', $_REQUEST); - $searchData['value'] = CRM_Utils_Type::escape($searchData['value'], 'String'); - if (!empty($src) || !empty($searchData['value'])) { - $src = $src ? $src : $searchData['value']; - $where[] = " cc1.display_name LIKE '%{$src}%'"; - } - if (!empty($dst) || !empty($searchData['value'])) { - $dst = $dst ? $dst : $searchData['value']; - $where[] = " cc2.display_name LIKE '%{$dst}%'"; - } - if (!empty($src_email) || !empty($searchData['value'])) { - $src_email = $src_email ? $src_email : $searchData['value']; - $where[] = " (ce1.is_primary = 1 AND ce1.email LIKE '%{$src_email}%')"; - } - if (!empty($dst_email) || !empty($searchData['value'])) { - $dst_email = $dst_email ? $dst_email : $searchData['value']; - $where[] = " (ce2.is_primary = 1 AND ce2.email LIKE '%{$dst_email}%')"; - } - if (!empty($src_postcode) || !empty($searchData['value'])) { - $src_postcode = $src_postcode ? $src_postcode : $searchData['value']; - $where[] = " (ca1.is_primary = 1 AND ca1.postal_code LIKE '%{$src_postcode}%')"; - } - if (!empty($dst_postcode) || !empty($searchData['value'])) { - $dst_postcode = $dst_postcode ? $dst_postcode : $searchData['value']; - $where[] = " (ca2.is_primary = 1 AND ca2.postal_code LIKE '%{$dst_postcode}%')"; - } - if (!empty($src_street) || !empty($searchData['value'])) { - $src_street = $src_street ? $src_street : $searchData['value']; - $where[] = " (ca1.is_primary = 1 AND ca1.street_address LIKE '%{$src_street}%')"; - } - if (!empty($dst_street) || !empty($searchData['value'])) { - $dst_street = $dst_street ? $dst_street : $searchData['value']; - $where[] = " (ca2.is_primary = 1 AND ca2.street_address LIKE '%{$dst_street}%')"; + $isOrQuery = self::isOrQuery(); + + $nextParamKey = 3; + $mappings = array( + 'src' => 'cc1.display_name', + 'dst' => 'cc2.display_name', + 'src_email' => 'ce1.email', + 'dst_email' => 'ce2.email', + 'src_postcode' => 'ca1.postal_code', + 'dst_postcode' => 'ca2.postal_code', + 'src_street' => 'ca1.street', + 'dst_street' => 'ca2.street', + ); + + foreach ($mappings as $key => $dbName) { + if (!empty($searchParams[$key])) { + $queryParams[$nextParamKey] = array('%' . $searchParams[$key] . '%', 'String'); + $where[] = $dbName . " LIKE %{$nextParamKey} "; + $nextParamKey++; + } } - if (!empty($searchData['value'])) { + + if ($isOrQuery) { $whereClause = ' ( ' . implode(' OR ', $where) . ' ) '; } else { @@ -747,7 +732,7 @@ LIMIT {$offset}, {$rowCount} $join .= " LEFT JOIN civicrm_address ca1 ON (ca1.contact_id = pn.entity_id1 AND ca1.is_primary = 1 )"; $join .= " LEFT JOIN civicrm_address ca2 ON (ca2.contact_id = pn.entity_id2 AND ca2.is_primary = 1 )"; } - $iTotal = CRM_Core_BAO_PrevNextCache::getCount($cacheKeyString, $join, $whereClause); + $iTotal = CRM_Core_BAO_PrevNextCache::getCount($cacheKeyString, $join, $whereClause, '=', $queryParams); if (!empty($_REQUEST['order'])) { foreach ($_REQUEST['order'] as $orderInfo) { if (!empty($orderInfo['column'])) { @@ -796,7 +781,7 @@ LIMIT {$offset}, {$rowCount} } } - $dupePairs = CRM_Core_BAO_PrevNextCache::retrieve($cacheKeyString, $join, $whereClause, $offset, $rowCount, $select, $orderByClause); + $dupePairs = CRM_Core_BAO_PrevNextCache::retrieve($cacheKeyString, $join, $whereClause, $offset, $rowCount, $select, $orderByClause, TRUE, $queryParams); $iFilteredTotal = CRM_Core_DAO::singleValueQuery("SELECT FOUND_ROWS()"); $count = 0; @@ -857,6 +842,62 @@ LIMIT {$offset}, {$rowCount} CRM_Utils_JSON::output($dupePairs); } + /** + * Get the searchable options from the request. + * + * @return array + */ + public static function getSearchOptionsFromRequest() { + $searchParams = array(); + $searchData = CRM_Utils_Array::value('search', $_REQUEST); + $searchData['value'] = CRM_Utils_Type::escape($searchData['value'], 'String'); + $selectorElements = array( + 'is_selected', + 'is_selected_input', + 'src_image', + 'src', + 'src_email', + 'src_street', + 'src_postcode', + 'dst_image', + 'dst', + 'dst_email', + 'dst_street', + 'dst_postcode', + 'conflicts', + 'weight', + 'actions', + ); + $columns = $_REQUEST['columns']; + + foreach ($columns as $column) { + if (!empty($column['search']['value']) && in_array($column['data'], $selectorElements)) { + $searchParams[$column['data']] = CRM_Utils_Type::escape($column['search']['value'], 'String'); + } + elseif (!empty($searchData['value'])) { + $searchParams[$column['data']] = $searchData['value']; + } + } + return $searchParams; + } + + /** + * Is the query an OR query. + * + * If a generic search value is passed in - ie. $_REQUEST['search']['value'] = 'abc' + * then all fields are searched for this. + * + * It is unclear if there is any code that still passes this in or whether is is just legacy. It + * could cause a server-killing query on a large site so it probably is NOT in use if we haven't + * had complaints. + * + * @return bool + */ + public static function isOrQuery() { + $searchData = CRM_Utils_Array::value('search', $_REQUEST); + return !empty($searchData['value']); + } + /** * Retrieve a PDF Page Format for the PDF Letter form. */ diff --git a/CRM/Core/BAO/PrevNextCache.php b/CRM/Core/BAO/PrevNextCache.php index 649d997e70..3bfdd77385 100644 --- a/CRM/Core/BAO/PrevNextCache.php +++ b/CRM/Core/BAO/PrevNextCache.php @@ -217,7 +217,7 @@ WHERE cacheKey = %3 AND * * @return array */ - public static function retrieve($cacheKey, $join = NULL, $whereClause = NULL, $offset = 0, $rowCount = 0, $select = array(), $orderByClause = '', $includeConflicts = TRUE) { + public static function retrieve($cacheKey, $join = NULL, $whereClause = NULL, $offset = 0, $rowCount = 0, $select = array(), $orderByClause = '', $includeConflicts = TRUE, $params = array()) { $selectString = 'pn.*'; if (!empty($select)) { @@ -230,7 +230,7 @@ WHERE cacheKey = %3 AND $params = array( 1 => array($cacheKey, 'String'), - ); + ) + $params; if (!empty($whereClause)) { $whereClause = " AND " . $whereClause; @@ -312,13 +312,15 @@ FROM civicrm_prevnext_cache pn /** * @param $cacheKey - * @param NULL $join - * @param NULL $where + * @param string $join + * @param string $where * @param string $op + * @param array $params + * Extra query params to parse into the query. * * @return int */ - public static function getCount($cacheKey, $join = NULL, $where = NULL, $op = "=") { + public static function getCount($cacheKey, $join = NULL, $where = NULL, $op = "=", $params = array()) { $query = " SELECT COUNT(*) FROM civicrm_prevnext_cache pn {$join} @@ -331,7 +333,7 @@ WHERE (pn.cacheKey $op %1 OR pn.cacheKey $op %2) $params = array( 1 => array($cacheKey, 'String'), 2 => array("{$cacheKey}_conflicts", 'String'), - ); + ) + $params; return (int) CRM_Core_DAO::singleValueQuery($query, $params, TRUE, FALSE); } diff --git a/tests/phpunit/CRM/Contact/Page/AjaxTest.php b/tests/phpunit/CRM/Contact/Page/AjaxTest.php index cd040c1012..6b98734914 100644 --- a/tests/phpunit/CRM/Contact/Page/AjaxTest.php +++ b/tests/phpunit/CRM/Contact/Page/AjaxTest.php @@ -32,4 +32,182 @@ class CRM_Contact_Page_AjaxTest extends CiviUnitTestCase { $this->assertEquals(array('data' => array(), 'recordsTotal' => 0, 'recordsFiltered' => 0), $result); } + public function testGetDedupesPostCode() { + $_REQUEST['gid'] = 1; + $_REQUEST['rgid'] = 1; + $_REQUEST['snippet'] = 4; + $_REQUEST['draw'] = 3; + $_REQUEST['columns'] = array( + 0 => array( + 'data' => 'is_selected_input', + 'name' => '', + 'searchable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 1 => array( + 'data' => 'src_image', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => FALSE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 2 => array( + 'data' => 'src', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 3 => array( + 'data' => 'dst_image', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => FALSE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 4 => array( + 'data' => 'dst', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 5 => array( + 'data' => 'src_email', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 6 => array( + 'data' => 'dst_email', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 7 => array( + 'data' => 'src_street', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 8 => array( + 'data' => 'dst_street', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 9 => array( + 'data' => 'src_postcode', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => 123, + 'regex' => FALSE, + ), + ), + + 10 => array( + 'data' => 'dst_postcode', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 11 => array( + 'data' => 'conflicts', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 12 => array( + 'data' => 'weight', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => TRUE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + + 13 => array( + 'data' => 'actions', + 'name' => '', + 'searchable' => TRUE, + 'orderable' => FALSE, + 'search' => array( + 'value' => '', + 'regex' => FALSE, + ), + ), + ); + + $_REQUEST['start'] = 0; + $_REQUEST['length'] = 10; + $_REQUEST['search'] = array( + 'value' => '', + 'regex' => FALSE, + ); + + $_REQUEST['_'] = 1466478641007; + $_REQUEST['Drupal_toolbar_collapsed'] = 0; + $_REQUEST['has_js'] = 1; + $_REQUEST['SESSa06550b3043ecca303761d968e3c846a'] = 'qxSxw0F_UmBITMM0JaVwTRcHV1bQqBSHNmBMY9AA8Wk'; + + $_REQUEST['is_unit_test'] = TRUE; + + $result = CRM_Contact_Page_AJAX::getDedupes(); + $this->assertEquals(array('data' => array(), 'recordsTotal' => 0, 'recordsFiltered' => 0), $result); + } + } -- 2.25.1