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 bcc8e650f8f0887d3799d95fbc3adaecf5067ae8..a7abcc1c7c9ff22e7cec555bd5bdbb5ac14a403a 100644 (file)
--- 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 f3fa9a01b75ed8bd753d266f5cd57954648456d9..19658cb898848af0e99298b9d3a68334a852af60 100644 (file)
--- 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 a5e3c8c67a249ecd8fb8053590b988caa5625991..92595f83705f4e0a85e38aef5172662754dadba3 100644 (file)
--- 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.
    *