From: eileen <emcnaughton@wikimedia.org>
Date: Thu, 30 Jun 2016 08:53:04 +0000 (+1200)
Subject: CRM-17123 remove damaging OR from smart group query
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=485a3a1f02a668f7945b45bb40e163ca50e6d3a9;p=civicrm-core.git

CRM-17123 remove damaging OR from smart group query

Note that I added a group refresh to pick up the 'hard-adds'. I think the correct place to deal with this is in fact to alter the GroupContact Add functionality - ie.

IF (CRM_Core_DAO::executeQuery('SELECT id FROM civicrm_group_contact_cache WHERE group_id = 1 LIMIT 1)) {
  // Add the contact just added to the group to the group_contact_cache table.
}

I started that a bit but I think it should be dealt with separately after this is resolved
---

diff --git a/CRM/Contact/BAO/Group.php b/CRM/Contact/BAO/Group.php
index 7125c2a139..9679a216bd 100644
--- a/CRM/Contact/BAO/Group.php
+++ b/CRM/Contact/BAO/Group.php
@@ -460,7 +460,6 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group {
     }
 
     if (!empty($params['organization_id'])) {
-      $groupOrg = array();
       $groupOrg = $params;
       $groupOrg['group_id'] = $group->id;
       CRM_Contact_BAO_GroupOrganization::add($groupOrg);
diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index 59ec38668d..cce169d2ee 100644
--- a/CRM/Contact/BAO/Query.php
+++ b/CRM/Contact/BAO/Query.php
@@ -586,7 +586,10 @@ class CRM_Contact_BAO_Query {
       if (!array_key_exists($value[0], $this->_paramLookup)) {
         $this->_paramLookup[$value[0]] = array();
       }
-      $this->_paramLookup[$value[0]][] = $value;
+      if ($value[0] !== 'group') {
+        // Just trying to unravel how group interacts here! This whole function is wieid.
+        $this->_paramLookup[$value[0]][] = $value;
+      }
     }
   }
 
@@ -2911,7 +2914,6 @@ class CRM_Contact_BAO_Query {
 
     $groupIds = NULL;
 
-    $isSmart = FALSE;
     $isNotOp = ($op == 'NOT IN' || $op == '!=');
 
     $statii = array();
@@ -2929,7 +2931,8 @@ class CRM_Contact_BAO_Query {
       $statii[] = '"Added"';
     }
 
-    $skipGroup = FALSE;
+    $ssClause = $this->addGroupContactCache($value, NULL, "contact_a", $op);
+    $isSmart = (!$ssClause) ? FALSE : TRUE;
     if (!is_array($value) &&
       count($statii) == 1 &&
       $statii[0] == '"Added"' &&
@@ -2939,9 +2942,6 @@ class CRM_Contact_BAO_Query {
         $isSmart = TRUE;
       }
     }
-
-    $ssClause = $this->addGroupContactCache($value, NULL, "contact_a", $op);
-    $isSmart = (!$ssClause) ? FALSE : $isSmart;
     $groupClause = NULL;
 
     if (!$isSmart) {
@@ -3038,6 +3038,9 @@ WHERE  $smartGroupClause
         CRM_Contact_BAO_GroupContactCache::load($group);
       }
     }
+    if ($group->N == 0) {
+      return NULL;
+    }
 
     if (!$tableAlias) {
       $tableAlias = "`civicrm_group_contact_cache_";
diff --git a/tests/phpunit/CRM/Contact/BAO/GroupContactTest.php b/tests/phpunit/CRM/Contact/BAO/GroupContactTest.php
index 01a90fad3a..83bb495cfe 100644
--- a/tests/phpunit/CRM/Contact/BAO/GroupContactTest.php
+++ b/tests/phpunit/CRM/Contact/BAO/GroupContactTest.php
@@ -110,7 +110,7 @@ class CRM_Contact_BAO_GroupContactTest extends CiviUnitTestCase {
       'title' => 'Child Group',
       'description' => 'Child Group',
       'visibility' => 'User and User Admin Only',
-      'parents' => $parentGroup->id,
+      'parents' => $parentGroup['id'],
       'is_active' => 1,
     );
     $childGroup = $this->callAPISuccess('Group', 'create', $groupParams2);
@@ -119,7 +119,7 @@ class CRM_Contact_BAO_GroupContactTest extends CiviUnitTestCase {
     $parentContactParams = array(
       'first_name' => 'Parent1 Fname',
       'last_name' => 'Parent1 Lname',
-      'group' => array($parentGroup->id => 1),
+      'group' => array($parentGroup['id'] => 1),
     );
     $parentContact = Contact::createIndividual($parentContactParams);
 
@@ -127,13 +127,13 @@ class CRM_Contact_BAO_GroupContactTest extends CiviUnitTestCase {
     $childContactParams = array(
       'first_name' => 'Child1 Fname',
       'last_name' => 'Child2 Lname',
-      'group' => array($childGroup->id => 1),
+      'group' => array($childGroup['id'] => 1),
     );
     $childContact = Contact::createIndividual($childContactParams);
 
     // Check if searching by parent group  returns both parent and child group contacts
     $searchParams = array(
-      'group' => $parentGroup->id,
+      'group' => $parentGroup['id'],
     );
     $result = $this->callAPISuccess('contact', 'get', $searchParams);
     $validContactIds = array($parentContact, $childContact);
@@ -146,7 +146,7 @@ class CRM_Contact_BAO_GroupContactTest extends CiviUnitTestCase {
 
     // Check if searching by child group returns just child group contacts
     $searchParams = array(
-      'group' => $childGroup->id,
+      'group' => $childGroup['id'],
     );
     $result = $this->callAPISuccess('contact', 'get', $searchParams);
     $validChildContactIds = array($childContact);
diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php
index b0233c1c5b..5b91b22a7b 100644
--- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php
+++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php
@@ -290,6 +290,8 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase {
     $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $individualID, 'status' => 'Added'));
     $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $householdID, 'status' => 'Added'));
 
+    // Refresh the cache for test purposes. It would be better to alter to alter the GroupContact add function to add contacts to the cache.
+    CRM_Contact_BAO_GroupContactCache::remove($groupID, FALSE);
     $query = new CRM_Contact_BAO_Query(
       array(array('group', 'IN', array($groupID), 0, 0)),
       array('contact_id')
@@ -299,6 +301,7 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase {
     $queryString = implode(' ', $sql);
     $dao = CRM_Core_DAO::executeQuery($queryString);
     $this->assertEquals(3, $dao->N);
+    $this->assertFalse(strstr(implode(' ', $sql), ' OR '));
   }
 
 }