From: eileen Date: Fri, 1 Jul 2016 01:51:38 +0000 (+1200) Subject: CRM-17123 further removal of damaging OR query. X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=3875e6b655e6ee13b11ccee31cd3a3c0421a0724;p=civicrm-core.git 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 --- diff --git a/CRM/Contact/BAO/GroupContact.php b/CRM/Contact/BAO/GroupContact.php index 78722e001b..e400cdc103 100644 --- a/CRM/Contact/BAO/GroupContact.php +++ b/CRM/Contact/BAO/GroupContact.php @@ -331,7 +331,7 @@ class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact { * @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 @@ class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact { 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 @@ class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact { $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 index cce169d2ee..b3031054e8 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -872,9 +872,20 @@ class CRM_Contact_BAO_Query { } 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 @@ class CRM_Contact_BAO_Query { $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 @@ class CRM_Contact_BAO_Query { ); } - // 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 @@ class CRM_Contact_BAO_Query { 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 @@ civicrm_relationship.is_permission_a_b = 0 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 @@ AND displayRelType.is_active = 1 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 @@ AND displayRelType.is_active = 1 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 index 295c569fc8..0bad88963b 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -1189,7 +1189,7 @@ SELECT DISTINCT 'civicrm_contact', contact_a.id, contact_a.id, '$cacheKey', cont /** * @return CRM_Contact_BAO_Query */ - public function &getQuery() { + public function getQuery() { return $this->_query; } diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 5b91b22a7b..d3479ea1d0 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -292,16 +292,27 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { // 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), $dao->groups . ' includes ' . $groupID); + } } }