CRM-17123 further removal of damaging OR query.

Note that in testing this I checked
1) searching in search builder with groups as a criteria
(checked that correct groups show & only 'Added' ones)
2) Exporting  - groups show per above when selected as an export field
3) Groups tab on a contact correctly shows added & removed groups
4) Api calls per tests
5) Manage groups - clicking through shows correct status for all members and it is possible to alter the group criteria to include 'Removed' and they show with the correct status

Author: eileenmcnaughton

CRM/Contact/BAO/GroupContact.php

@@ -331,7 +331,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,
@@ -386,10 +386,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) {

CRM/Contact/BAO/Query.php

@@ -872,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
@@ -1370,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)
@@ -2517,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(
@@ -2641,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':
@@ -4259,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";
   }
 
   /**
@@ -5687,6 +5690,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;
@@ -6069,4 +6076,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);
+  }
+
 }

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;
   }
 

tests/phpunit/CRM/Contact/BAO/QueryTest.php

@@ -292,16 +292,27 @@ public function testGroupClause() {
 
     // 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(
+
+    $sql = CRM_Contact_BAO_Query::getQuery(
       array(array('group', 'IN', array($groupID), 0, 0)),
       array('contact_id')
     );
 
-    $sql = $query->query();
-    $queryString = implode(' ', $sql);
-    $dao = CRM_Core_DAO::executeQuery($queryString);
+    $dao = CRM_Core_DAO::executeQuery($sql);
     $this->assertEquals(3, $dao->N);
-    $this->assertFalse(strstr(implode(' ', $sql), ' OR '));
+    $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 . ',' . $groupID), $dao->groups . ' includes ' . $groupID);
+    }
   }
 
 }