CRM-17123 remove damaging OR from smart group query

Please add review comments to civicrm#8645

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

Change-Id: I1304eaced73262f66030010bfcce05bc7d70d0b5

Author: eileenmcnaughton

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

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,
@@ -356,7 +356,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 ";
@@ -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

@@ -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)
@@ -2511,16 +2526,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(
@@ -2635,7 +2640,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':
@@ -2875,11 +2880,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
@@ -2907,8 +2912,7 @@ public function group(&$values) {
     }
 
     $groupIds = NULL;
-    $names = array();
-    $isSmart = FALSE;
+
     $isNotOp = ($op == 'NOT IN' || $op == '!=');
 
     $statii = array();
@@ -2926,7 +2930,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"' &&
@@ -2936,9 +2941,6 @@ public function group(&$values) {
         $isSmart = TRUE;
       }
     }
-
-    $ssClause = $this->addGroupContactCache($value, NULL, "contact_a", $op);
-    $isSmart = (!$ssClause) ? FALSE : $isSmart;
     $groupClause = NULL;
 
     if (!$isSmart) {
@@ -3035,6 +3037,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_";
@@ -4253,8 +4258,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";
   }
 
   /**
@@ -5681,6 +5687,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;
@@ -6085,4 +6095,28 @@ public function setQillAndWhere($name, $op, $value, $grouping, $field) {
     ));
   }
 
+  /**
+   * 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;
   }