Merge pull request #8645 from eileenmcnaughton/group_query

CRM-17213 add tests for group search
civicrm · Jul 14, 2016 · 178cf31 · 178cf31
2 parents c2ae5aa + 3875e6b
commit 178cf31
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 44 deletions.
diff --git a/CRM/Contact/BAO/Group.php b/CRM/Contact/BAO/Group.php
@@ -346,7 +346,7 @@ public static function checkPermission($id, $excludeHidden = FALSE) {
    * @return CRM_Contact_BAO_Group|NULL
    *   The new group BAO (if created)
    */
-  public static function &create(&$params) {
+  public static function create(&$params) {
 
     if (!empty($params['id'])) {
       CRM_Utils_Hook::pre('edit', 'Group', $params['id'], $params);
@@ -460,7 +460,6 @@ public static function &create(&$params) {
     }
 
     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/GroupContact.php b/CRM/Contact/BAO/GroupContact.php
@@ -332,7 +332,7 @@ public static function getGroupList($contactId = 0, $visibility = FALSE) {
    * @return array|int $values
    *   the relevant data object values for the contact or the total count when $count is TRUE
    */
-  public static function &getContactGroup(
+  public static function getContactGroup(
     $contactId,
     $status = NULL,
     $numGroupContact = NULL,
@@ -357,7 +357,7 @@ public static function &getContactGroup(
                     civicrm_subscription_history.method as method';
     }
 
-    $where = " WHERE contact_a.id = %1 AND civicrm_group.is_active = 1";
+    $where = " WHERE contact_a.id = %1 AND civicrm_group.is_active = 1 AND saved_search_id IS NULL";
 
     if ($excludeHidden) {
       $where .= " AND civicrm_group.is_hidden = 0 ";
@@ -387,10 +387,6 @@ public static function &getContactGroup(
 
     $from = CRM_Contact_BAO_Query::fromClause($tables);
 
-    //CRM-16945: seems hackish but as per CRM-16483 of using group criteria for Search Builder it is mandatory
-    //to include group_contact_cache clause when group table is present, so following code remove duplicacy
-    $from = str_replace("OR civicrm_group.id = civicrm_group_contact_cache.group_id", 'AND civicrm_group.saved_search_id IS NULL', $from);
-
     $where .= " AND $permission ";
 
     if ($onlyPublicGroups) {

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
@@ -586,7 +586,10 @@ public function buildParamsLookup() {
       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;
+      }
     }
   }
 
@@ -869,9 +872,20 @@ public function selectClause($apiEntity = NULL) {
         }
         elseif ($name === 'groups') {
           $this->_useGroupBy = TRUE;
-          $this->_select[$name] = "GROUP_CONCAT(DISTINCT(civicrm_group.title)) as groups";
+          // Duplicates will be created here but better to sort them out in php land.
+          $this->_select[$name] = "
+            CONCAT_WS(',',
+            GROUP_CONCAT(DISTINCT IF(civicrm_group_contact.status = 'Added', civicrm_group_contact.group_id, '')),
+            GROUP_CONCAT(DISTINCT civicrm_group_contact_cache.group_id)
+          )
+          as groups";
           $this->_element[$name] = 1;
-          $this->_tables['civicrm_group'] = 1;
+          $this->_tables['civicrm_group_contact'] = 1;
+          $this->_tables['civicrm_group_contact_cache'] = 1;
+          $this->_pseudoConstantsSelect["{$name}"] = array(
+            'pseudoField' => "groups",
+            'idCol' => "groups",
+          );
         }
         elseif ($name === 'notes') {
           // if note field is subject then return subject else body of the note
@@ -1367,6 +1381,7 @@ public function query($count = FALSE, $sortByChar = FALSE, $groupContacts = FALS
           $this->_paramLookup['group'][0][1] = key($value);
         }
 
+        // Presumably the lines below come into manage groups screen.
         // make sure there is only one element
         // this is used when we are running under smog and need to know
         // how the contact was added (CRM-1203)
@@ -2514,16 +2529,6 @@ public static function fromClause(&$tables, $inner = NULL, $right = NULL, $prima
       );
     }
 
-    // add group_contact and group_contact_cache table if group table is present
-    if (!empty($tables['civicrm_group'])) {
-      if (empty($tables['civicrm_group_contact'])) {
-        $tables['civicrm_group_contact'] = " LEFT JOIN civicrm_group_contact ON civicrm_group_contact.contact_id = contact_a.id AND civicrm_group_contact.status = 'Added' ";
-      }
-      if (empty($tables['civicrm_group_contact_cache'])) {
-        $tables['civicrm_group_contact_cache'] = " LEFT JOIN civicrm_group_contact_cache ON civicrm_group_contact_cache.contact_id = contact_a.id ";
-      }
-    }
-
     // add group_contact and group table is subscription history is present
     if (!empty($tables['civicrm_subscription_history']) && empty($tables['civicrm_group'])) {
       $tables = array_merge(array(
@@ -2638,7 +2643,7 @@ public static function fromClause(&$tables, $inner = NULL, $right = NULL, $prima
           continue;
 
         case 'civicrm_group':
-          $from .= " $side JOIN civicrm_group ON (civicrm_group.id = civicrm_group_contact.group_id OR civicrm_group.id = civicrm_group_contact_cache.group_id) ";
+          $from .= " $side JOIN civicrm_group ON civicrm_group.id = civicrm_group_contact.group_id ";
           continue;
 
         case 'civicrm_group_contact':
@@ -2878,11 +2883,11 @@ public function includeContactSubTypes($value, $grouping, $op = 'LIKE') {
   }
 
   /**
-   * Where / qill clause for groups
+   * Where / qill clause for groups.
    *
    * @param $values
    */
-  public function group(&$values) {
+  public function group($values) {
     list($name, $op, $value, $grouping, $wildcard) = $values;
 
     // If the $value is in OK (operator as key) array format we need to extract the key as operator and value first
@@ -2910,8 +2915,7 @@ public function group(&$values) {
     }
 
     $groupIds = NULL;
-    $names = array();
-    $isSmart = FALSE;
+
     $isNotOp = ($op == 'NOT IN' || $op == '!=');
 
     $statii = array();
@@ -2929,7 +2933,8 @@ public function group(&$values) {
       $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 +2944,6 @@ public function group(&$values) {
         $isSmart = TRUE;
       }
     }
-
-    $ssClause = $this->addGroupContactCache($value, NULL, "contact_a", $op);
-    $isSmart = (!$ssClause) ? FALSE : $isSmart;
     $groupClause = NULL;
 
     if (!$isSmart) {
@@ -3038,6 +3040,9 @@ public function addGroupContactCache($groups, $tableAlias = NULL, $joinTable = "
         CRM_Contact_BAO_GroupContactCache::load($group);
       }
     }
+    if ($group->N == 0) {
+      return NULL;
+    }
 
     if (!$tableAlias) {
       $tableAlias = "`civicrm_group_contact_cache_";
@@ -4256,8 +4261,9 @@ public static function getPrimaryCondition($value) {
   public static function getQuery($params = NULL, $returnProperties = NULL, $count = FALSE) {
     $query = new CRM_Contact_BAO_Query($params, $returnProperties);
     list($select, $from, $where, $having) = $query->query();
+    $groupBy = ($query->_useGroupBy) ? 'GROUP BY contact_a.id' : '';
 
-    return "$select $from $where $having";
+    return "$select $from $where $groupBy $having";
   }
 
   /**
@@ -5748,6 +5754,10 @@ public function convertToPseudoNames(&$dao, $return = FALSE, $usedForAPI = FALSE
 
       if (is_object($dao) && property_exists($dao, $value['idCol'])) {
         $val = $dao->$value['idCol'];
+        if ($key == 'groups') {
+          $dao->groups = $this->convertGroupIDStringToLabelString($dao, $val);
+          return;
+        }
 
         if (CRM_Utils_System::isNull($val)) {
           $dao->$key = NULL;
@@ -6130,4 +6140,28 @@ protected function prepareOrderBy($sort, $sortByChar, $sortOrder, $additionalFro
     return array($order, $additionalFromClause);
   }
 
+  /**
+   * Convert a string of group IDs to a string of group labels.
+   *
+   * The original string may include duplicates and groups the user does not have
+   * permission to see.
+   *
+   * @param CRM_Core_DAO $dao
+   * @param string $val
+   *
+   * @return string
+   */
+  public function convertGroupIDStringToLabelString(&$dao, $val) {
+    $groupIDs = explode(',', $val);
+
+    // The pseudoConstant function does not actually cache.
+    static $allGroups;
+    if (!$allGroups) {
+      $allGroups = CRM_Core_PseudoConstant::group();
+    }
+    // Note that groups that the user does not have permission to will be excluded (good).
+    $groups = array_intersect_key($allGroups, array_flip($groupIDs));
+    return implode(', ', $groups);
+  }
+
 }
diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php
@@ -1189,7 +1189,7 @@ private static function &_getColumnHeaders() {
   /**
    * @return CRM_Contact_BAO_Query
    */
-  public function &getQuery() {
+  public function getQuery() {
     return $this->_query;
   }
 

diff --git a/tests/phpunit/CRM/Contact/BAO/GroupContactTest.php b/tests/phpunit/CRM/Contact/BAO/GroupContactTest.php
@@ -94,48 +94,46 @@ public function testGetGroupId() {
    */
   public function testContactSearchByParentGroup() {
     // create a parent group
-    // TODO: This is not an API test!!
     $groupParams1 = array(
       'title' => 'Parent Group',
       'description' => 'Parent Group',
       'visibility' => 'User and User Admin Only',
       'parents' => '',
       'is_active' => 1,
     );
-    $parentGroup = CRM_Contact_BAO_Group::create($groupParams1);
+    $parentGroup = $this->callAPISuccess('Group', 'create', $groupParams1);
 
     // create a child group
     $groupParams2 = array(
       'title' => 'Child Group',
       'description' => 'Child Group',
       'visibility' => 'User and User Admin Only',
-      'parents' => $parentGroup->id,
+      'parents' => $parentGroup['id'],
       'is_active' => 1,
     );
-    $childGroup = CRM_Contact_BAO_Group::create($groupParams2);
+    $childGroup = $this->callAPISuccess('Group', 'create', $groupParams2);
 
     // Create a contact within parent group
     $parentContactParams = array(
       'first_name' => 'Parent1 Fname',
       'last_name' => 'Parent1 Lname',
-      'group' => array($parentGroup->id => 1),
+      'group' => array($parentGroup['id'] => 1),
     );
     $parentContact = $this->individualCreate($parentContactParams);
 
     // create a contact within child dgroup
     $childContactParams = array(
       'first_name' => 'Child1 Fname',
       'last_name' => 'Child2 Lname',
-      'group' => array($childGroup->id => 1),
+      'group' => array($childGroup['id'] => 1),
     );
     $childContact = $this->individualCreate($childContactParams);
 
     // Check if searching by parent group  returns both parent and child group contacts
     $searchParams = array(
-      'group' => $parentGroup->id,
-      'version' => 3,
+      'group' => $parentGroup['id'],
     );
-    $result = civicrm_api('contact', 'get', $searchParams);
+    $result = $this->callAPISuccess('contact', 'get', $searchParams);
     $validContactIds = array($parentContact, $childContact);
     $resultContactIds = array();
     foreach ($result['values'] as $k => $v) {
@@ -146,10 +144,9 @@ public function testContactSearchByParentGroup() {
 
     // Check if searching by child group returns just child group contacts
     $searchParams = array(
-      'group' => $childGroup->id,
-      'version' => 3,
+      'group' => $childGroup['id'],
     );
-    $result = civicrm_api('contact', 'get', $searchParams);
+    $result = $this->callAPISuccess('contact', 'get', $searchParams);
     $validChildContactIds = array($childContact);
     $resultChildContactIds = array();
     foreach ($result['values'] as $k => $v) {

diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php
@@ -275,4 +275,44 @@ public function testNonNumericEqualsPostal() {
 
   }
 
+  /**
+   * Test the group contact clause does not contain an OR.
+   *
+   * The search should return 3 contacts - 2 households in the smart group of
+   * Contact Type = Household and one Individual hard-added to it. The
+   * Household that meets both criteria should be returned once.
+   */
+  public function testGroupClause() {
+    $this->householdCreate();
+    $householdID = $this->householdCreate();
+    $individualID = $this->individualCreate();
+    $groupID = $this->smartGroupCreate();
+    $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);
+
+    $sql = CRM_Contact_BAO_Query::getQuery(
+      array(array('group', 'IN', array($groupID), 0, 0)),
+      array('contact_id')
+    );
+
+    $dao = CRM_Core_DAO::executeQuery($sql);
+    $this->assertEquals(3, $dao->N);
+    $this->assertFalse(strstr($sql, ' OR '));
+
+    $sql = CRM_Contact_BAO_Query::getQuery(
+      array(array('group', 'IN', array($groupID), 0, 0)),
+      array('contact_id' => 1, 'group' => 1)
+    );
+
+    $dao = CRM_Core_DAO::executeQuery($sql);
+    $this->assertEquals(3, $dao->N);
+    $this->assertFalse(strstr($sql, ' OR '), 'Query does not include or');
+    while ($dao->fetch()) {
+      $this->assertTrue(($dao->groups == $groupID || $dao->groups == ',' . $groupID), $dao->groups . ' includes ' . $groupID);
+    }
+  }
+
 }
diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php
@@ -1786,6 +1786,25 @@ public function groupCreate($params = array()) {
     return $result['id'];
   }
 
+  /**
+   * Create a smart group.
+   *
+   * By default it will be a group of households.
+   *
+   * @param array $smartGroupParams
+   * @param array $groupParams
+   * @return int
+   */
+  public function smartGroupCreate($smartGroupParams = array(), $groupParams = array()) {
+    $smartGroupParams = array_merge(array(
+      'formValues' => array('contact_type' => array('IN' => array('Household'))),
+    ),
+    $smartGroupParams);
+    $savedSearch = CRM_Contact_BAO_SavedSearch::create($smartGroupParams);
+
+    $groupParams['saved_search_id'] = $savedSearch->id;
+    return $this->groupCreate($groupParams);
+  }
 
   /**
    * Function to add a Group.