From: eileen <emcnaughton@wikimedia.org>
Date: Thu, 29 Aug 2019 04:15:53 +0000 (+1200)
Subject: Fix inconsistencies in duplicate retrieval
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=22912befa31fbdb3343ea407c56c468956da3724;p=civicrm-core.git
Fix inconsistencies in duplicate retrieval
Alternative to
https://github.com/civicrm/civicrm-core/pull/15158
and
https://github.com/civicrm/civicrm-core/pull/15153
fixing both the inconsistency & performance & making code more legible
---
diff --git a/CRM/Core/BAO/PrevNextCache.php b/CRM/Core/BAO/PrevNextCache.php
index bcc8e650f8..a7abcc1c7c 100644
--- a/CRM/Core/BAO/PrevNextCache.php
+++ b/CRM/Core/BAO/PrevNextCache.php
@@ -450,15 +450,24 @@ WHERE (pn.cachekey $op %1 OR pn.cachekey $op %2)
// would chain to a delete. Limiting to getfields for 'get' limits us to declared fields,
// although we might wish to revisit later to allow joins.
$validFieldsForRetrieval = civicrm_api3('Contact', 'getfields', ['action' => 'get'])['values'];
- if (!empty($criteria)) {
+ $filteredCriteria = isset($criteria['contact']) ? array_intersect_key($criteria['contact'], $validFieldsForRetrieval) : [];
+
+ if (!empty($criteria) || !empty($searchLimit)) {
$contacts = civicrm_api3('Contact', 'get', array_merge([
- 'options' => ['limit' => 0],
+ 'options' => ['limit' => $searchLimit],
'return' => 'id',
'check_permissions' => TRUE,
- ], array_intersect_key($criteria['contact'], $validFieldsForRetrieval)));
+ 'contact_type' => civicrm_api3('RuleGroup', 'getvalue', ['id' => $rgid, 'return' => 'contact_type']),
+ ], $filteredCriteria));
$contactIDs = array_keys($contacts['values']);
+
+ if (empty($contactIDs)) {
+ // If there is criteria but no contacts were found then we should return now
+ // since we have no contacts to match.
+ return [];
+ }
}
- $foundDupes = CRM_Dedupe_Finder::dupes($rgid, $contactIDs, $checkPermissions, $searchLimit);
+ $foundDupes = CRM_Dedupe_Finder::dupes($rgid, $contactIDs, $checkPermissions);
}
if (!empty($foundDupes)) {
diff --git a/CRM/Dedupe/Finder.php b/CRM/Dedupe/Finder.php
index f3fa9a01b7..19658cb898 100644
--- a/CRM/Dedupe/Finder.php
+++ b/CRM/Dedupe/Finder.php
@@ -51,32 +51,18 @@ class CRM_Dedupe_Finder {
* @param bool $checkPermissions
* Respect logged in user permissions.
*
- * @param int $searchLimit
- * Limit for the number of contacts to be used for comparison.
- * The search methodology finds all matches for the searchedContacts so this limits
- * the number of searched contacts, not the matches found.
- *
* @return array
* Array of (cid1, cid2, weight) dupe triples
*
- * @throws CiviCRM_API3_Exception
* @throws Exception
*/
- public static function dupes($rgid, $cids = [], $checkPermissions = TRUE, $searchLimit = 0) {
+ public static function dupes($rgid, $cids = [], $checkPermissions = TRUE) {
$rgBao = new CRM_Dedupe_BAO_RuleGroup();
$rgBao->id = $rgid;
$rgBao->contactIds = $cids;
if (!$rgBao->find(TRUE)) {
CRM_Core_Error::fatal("Dedupe rule not found for selected contacts");
}
- if (empty($rgBao->contactIds) && !empty($searchLimit)) {
- $limitedContacts = civicrm_api3('Contact', 'get', [
- 'return' => 'id',
- 'contact_type' => $rgBao->contact_type,
- 'options' => ['limit' => $searchLimit],
- ]);
- $rgBao->contactIds = array_keys($limitedContacts['values']);
- }
$rgBao->fillTable();
$dao = new CRM_Core_DAO();
diff --git a/tests/phpunit/CRM/Dedupe/MergerTest.php b/tests/phpunit/CRM/Dedupe/MergerTest.php
index a5e3c8c67a..92595f8370 100644
--- a/tests/phpunit/CRM/Dedupe/MergerTest.php
+++ b/tests/phpunit/CRM/Dedupe/MergerTest.php
@@ -11,6 +11,15 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
protected $_contactIds = [];
+ /**
+ * Contacts created for the test.
+ *
+ * Overlaps contactIds....
+ *
+ * @var array
+ */
+ protected $contacts = [];
+
/**
* Tear down.
*
@@ -21,6 +30,7 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
'civicrm_contact',
'civicrm_group_contact',
'civicrm_group',
+ 'civicrm_prevnext_cache',
]);
parent::tearDown();
}
@@ -321,6 +331,79 @@ class CRM_Dedupe_MergerTest extends CiviUnitTestCase {
], $pairs);
}
+ /**
+ * Test results are returned when criteria are passed in.
+ */
+ public function testGetMatchesCriteriaMatched() {
+ $this->setupMatchData();
+ $pairs = $this->callAPISuccess('Dedupe', 'getduplicates', [
+ 'rule_group_id' => 1,
+ 'criteria' => ['contact' => ['id' => ['>' => 1]]],
+ ])['values'];
+ $this->assertCount(2, $pairs);
+ }
+
+ /**
+ * Test results are returned when criteria are passed in & limit is respected.
+ */
+ public function testGetMatchesCriteriaMatchedWithLimit() {
+ $this->setupMatchData();
+ $pairs = $this->callAPISuccess('Dedupe', 'getduplicates', [
+ 'rule_group_id' => 1,
+ 'criteria' => ['contact' => ['id' => ['>' => 1]]],
+ 'options' => ['limit' => 1],
+ ])['values'];
+ $this->assertCount(1, $pairs);
+ }
+
+ /**
+ * Test results are returned when criteria are passed in & limit is respected.
+ */
+ public function testGetMatchesCriteriaMatchedWithSearchLimit() {
+ $this->setupMatchData();
+ $pairs = $this->callAPISuccess('Dedupe', 'getduplicates', [
+ 'rule_group_id' => 1,
+ 'criteria' => ['contact' => ['id' => ['>' => 1]]],
+ 'search_limit' => 1,
+ ])['values'];
+ $this->assertCount(1, $pairs);
+ }
+
+ /**
+ * Test getting matches where there are no criteria.
+ */
+ public function testGetMatchesNoCriteria() {
+ $this->setupMatchData();
+ $pairs = $this->callAPISuccess('Dedupe', 'getduplicates', [
+ 'rule_group_id' => 1,
+ ])['values'];
+ $this->assertCount(2, $pairs);
+ }
+
+ /**
+ * Test getting matches with a limit in play.
+ */
+ public function testGetMatchesNoCriteriaButLimit() {
+ $this->setupMatchData();
+ $pairs = $this->callAPISuccess('Dedupe', 'getduplicates', [
+ 'rule_group_id' => 1,
+ 'options' => ['limit' => 1],
+ ])['values'];
+ $this->assertCount(1, $pairs);
+ }
+
+ /**
+ * Test that if criteria are passed and there are no matching contacts no matches are returned.
+ */
+ public function testGetMatchesCriteriaNotMatched() {
+ $this->setupMatchData();
+ $pairs = $this->callAPISuccess('Dedupe', 'getduplicates', [
+ 'rule_group_id' => 1,
+ 'criteria' => ['contact' => ['id' => ['>' => 100000]]],
+ ])['values'];
+ $this->assertCount(0, $pairs);
+ }
+
/**
* Test function that gets organization pairs.
*