Skip to content

Commit

Permalink
CRM-17123 further removal of damaging OR query.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@eileenmcnaughton
eileenmcnaughton committed Jul 1, 2016
1 parent 485a3a1 commit 3875e6b
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 26 deletions.
8 changes: 2 additions & 6 deletions CRM/Contact/BAO/GroupContact.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 ";
Expand Down Expand Up @@ -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) {
Expand Down
59 changes: 45 additions & 14 deletions CRM/Contact/BAO/Query.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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':
Expand Down Expand Up @@ -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";
}

/**
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

}
2 changes: 1 addition & 1 deletion CRM/Contact/Selector.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,7 @@ private static function &_getColumnHeaders() {
/**
* @return CRM_Contact_BAO_Query
*/
public function &getQuery() {
public function getQuery() {
return $this->_query;
}

Expand Down
21 changes: 16 additions & 5 deletions tests/phpunit/CRM/Contact/BAO/QueryTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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), $dao->groups . ' includes ' . $groupID);
}
}

}

0 comments on commit 3875e6b

Please sign in to comment.