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 7125c2a13947692e73f749431e6b69c9bbebdae6..9679a216bd82ab5a017becb123b72908139af2a4 100644 (file)
--- 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 59ec38668dbf3a497538d91f37bb640a16f04d4e..cce169d2ee4fa758a1775e5a78ab8fdb8ad0c49d 100644 (file)
--- 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 01a90fad3a6864afc2a40cdc9bd25d8949c69160..83bb495cfe3541155a1ddc12a7d941650db39fe9 100644 (file)
--- 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 b0233c1c5b9650d7128196c387529423118fdaee..5b91b22a7b574d4635ca03ba8feaf4fb760d8150 100644 (file)
--- 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 '));
   }
 
 }