From 72a2eeabc8d602b2ae6e456737aae83736889b43 Mon Sep 17 00:00:00 2001 From: Allen Shaw Date: Sat, 17 Mar 2018 11:11:23 -0500 Subject: [PATCH] Fix CRM-21811: Optimize advanced search by relationship with target group for reciprocal relationship types. --- CRM/Contact/BAO/Query.php | 38 +++--- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 122 ++++++++++++++++++++ 2 files changed, 143 insertions(+), 17 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index ac156fdf3afa..22e2681eda32 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -3073,14 +3073,18 @@ public function getGroupsFromTypeCriteria($value) { } /** - * @param array $groups - * @param string $tableAlias - * @param string $joinTable - * @param string $op + * Prime smart group cache for smart groups in the search, and join + * civicrm_group_contact_cache table into the query. + * + * @param array $groups IDs of groups specified in search criteria. + * @param string $tableAlias Alias to use for civicrm_group_contact_cache table. + * @param string $joinTable Table on which to join civicrm_group_contact_cache + * @param string $op SQL comparison operator (NULL, IN, !=, IS NULL, etc.) + * @param string $joinColumn Column in $joinTable on which to join civicrm_group_contact_cache.contact_id * - * @return null|string + * @return string WHERE clause component for smart group criteria. */ - public function addGroupContactCache($groups, $tableAlias = NULL, $joinTable = "contact_a", $op) { + public function addGroupContactCache($groups, $tableAlias = NULL, $joinTable = "contact_a", $op, $joinColumn = 'id') { $isNullOp = (strpos($op, 'NULL') !== FALSE); $groupsIds = $groups; if (!$isNullOp && !$groups) { @@ -3121,7 +3125,7 @@ public function addGroupContactCache($groups, $tableAlias = NULL, $joinTable = " $tableAlias .= ($isNullOp) ? "a`" : implode(',', (array) $groupsIds) . "`"; } - $this->_tables[$tableAlias] = $this->_whereTables[$tableAlias] = " LEFT JOIN civicrm_group_contact_cache {$tableAlias} ON {$joinTable}.id = {$tableAlias}.contact_id "; + $this->_tables[$tableAlias] = $this->_whereTables[$tableAlias] = " LEFT JOIN civicrm_group_contact_cache {$tableAlias} ON {$joinTable}.{$joinColumn} = {$tableAlias}.contact_id "; return self::buildClause("{$tableAlias}.group_id", $op, $groups, 'Int'); } @@ -4073,25 +4077,28 @@ public function relationship(&$values) { $rTypeValue = (array) $rTypeValue; if ($rTypeValue['name_a_b'] == $rTypeValue['name_b_a']) { // if we don't know which end of the relationship we are dealing with we'll create a temp table - //@todo unless we are dealing with a target group self::$_relType = 'reciprocal'; } } } // if we are creating a temp table we build our own where for the relationship table $relationshipTempTable = NULL; - if (self::$_relType == 'reciprocal' && empty($targetGroup)) { + if (self::$_relType == 'reciprocal') { $where = array(); self::$_relationshipTempTable = $relationshipTempTable = CRM_Core_DAO::createTempTableName('civicrm_rel'); if ($nameClause) { $where[$grouping][] = " sort_name $nameClause "; } + $groupJoinTable = "civicrm_relationship"; + $groupJoinColumn = "contact_id_alt"; } else { $where = &$this->_where; if ($nameClause) { $where[$grouping][] = "( contact_b.sort_name $nameClause AND contact_b.id != contact_a.id )"; } + $groupJoinTable = "contact_b"; + $groupJoinColumn = "id"; } $allRelationshipType = CRM_Contact_BAO_Relationship::getContactRelationshipType(NULL, 'null', NULL, NULL, TRUE); if ($nameClause || !$targetGroup) { @@ -4114,12 +4121,12 @@ public function relationship(&$values) { if ($targetGroup) { //add contacts from static groups $this->_tables['civicrm_relationship_group_contact'] = $this->_whereTables['civicrm_relationship_group_contact'] - = " LEFT JOIN civicrm_group_contact civicrm_relationship_group_contact ON civicrm_relationship_group_contact.contact_id = contact_b.id AND civicrm_relationship_group_contact.status = 'Added'"; + = " LEFT JOIN civicrm_group_contact civicrm_relationship_group_contact ON civicrm_relationship_group_contact.contact_id = {$groupJoinTable}.{$groupJoinColumn} AND civicrm_relationship_group_contact.status = 'Added'"; $groupWhere[] = "( civicrm_relationship_group_contact.group_id IN (" . implode(",", $targetGroup[2]) . ") ) "; //add contacts from saved searches - $ssWhere = $this->addGroupContactCache($targetGroup[2], "civicrm_relationship_group_contact_cache", "contact_b", $op); + $ssWhere = $this->addGroupContactCache($targetGroup[2], "civicrm_relationship_group_contact_cache", $groupJoinTable, $op, $groupJoinColumn); //set the group where clause if ($ssWhere) { @@ -4141,7 +4148,7 @@ public function relationship(&$values) { if (!empty($relQill)) { $relQill .= ' OR '; } - $relQill .= $allRelationshipType[$rel]; + $relQill .= CRM_Utils_Array::value($rel, $allRelationshipType); } $this->_qill[$grouping][] = 'Relationship Type(s) ' . $relQill . " ( " . implode(", ", $qillNames) . " )"; } @@ -4201,9 +4208,6 @@ public function relationship(&$values) { $this->_relationshipValuesAdded = TRUE; // it could be a or b, using an OR creates an unindexed join - better to create a temp table & // join on that, - // @todo creating a temp table could be expanded to group filter - // as even creating a temp table of all relationships is much much more efficient than - // an OR in the join if ($relationshipTempTable) { $whereClause = ''; if (!empty($where[$grouping])) { @@ -4212,12 +4216,12 @@ public function relationship(&$values) { } $sql = " CREATE TEMPORARY TABLE {$relationshipTempTable} - (SELECT contact_id_b as contact_id, civicrm_relationship.id + (SELECT contact_id_b as contact_id, contact_id_a as contact_id_alt, civicrm_relationship.id FROM civicrm_relationship INNER JOIN civicrm_contact c ON civicrm_relationship.contact_id_a = c.id $whereClause ) UNION - (SELECT contact_id_a as contact_id, civicrm_relationship.id + (SELECT contact_id_a as contact_id, contact_id_b as contact_id_alt, civicrm_relationship.id FROM civicrm_relationship INNER JOIN civicrm_contact c ON civicrm_relationship.contact_id_b = c.id $whereClause ) diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 7ffcd046c25f..df1990fceebb 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -370,6 +370,128 @@ public function testNonNumericEqualsPostal() { } + public function testNonReciprocalRelationshipTargetGroupIsCorrectResults() { + $contactID_a = $this->individualCreate(); + $contactID_b = $this->individualCreate(); + $this->callAPISuccess('Relationship', 'create', array( + 'contact_id_a' => $contactID_a, + 'contact_id_b' => $contactID_b, + 'relationship_type_id' => 1, + 'is_active' => 1, + )); + // Create a group and add contact A to it. + $groupID = $this->groupCreate(); + $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $contactID_a, 'status' => 'Added')); + + // Add another (sans-relationship) contact to the group, + $contactID_c = $this->individualCreate(); + $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $contactID_c, 'status' => 'Added')); + + $params = array( + array( + 0 => 'relation_type_id', + 1 => 'IN', + 2 => + array( + 0 => '1_b_a', + ), + 3 => 0, + 4 => 0, + ), + array( + 0 => 'relation_target_group', + 1 => 'IN', + 2 => + array( + 0 => $groupID, + ), + 3 => 0, + 4 => 0, + ), + ); + + $query = new CRM_Contact_BAO_Query($params); + $dao = $query->searchQuery(); + $this->assertEquals('1', $dao->N, "Search query returns exactly 1 result?"); + $this->assertTrue($dao->fetch(), "Search query returns success?"); + $this->assertEquals($contactID_b, $dao->contact_id, "Search query returns parent of contact A?"); + } + + public function testReciprocalRelationshipTargetGroupIsCorrectResults() { + $contactID_a = $this->individualCreate(); + $contactID_b = $this->individualCreate(); + $this->callAPISuccess('Relationship', 'create', array( + 'contact_id_a' => $contactID_a, + 'contact_id_b' => $contactID_b, + 'relationship_type_id' => 2, + 'is_active' => 1, + )); + // Create a group and add contact A to it. + $groupID = $this->groupCreate(); + $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $contactID_a, 'status' => 'Added')); + + // Add another (sans-relationship) contact to the group, + $contactID_c = $this->individualCreate(); + $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $contactID_c, 'status' => 'Added')); + + $params = array( + array( + 0 => 'relation_type_id', + 1 => 'IN', + 2 => + array( + 0 => '2_a_b', + ), + 3 => 0, + 4 => 0, + ), + array( + 0 => 'relation_target_group', + 1 => 'IN', + 2 => + array( + 0 => $groupID, + ), + 3 => 0, + 4 => 0, + ), + ); + + $query = new CRM_Contact_BAO_Query($params); + $dao = $query->searchQuery(); + $this->assertEquals('1', $dao->N, "Search query returns exactly 1 result?"); + $this->assertTrue($dao->fetch(), "Search query returns success?"); + $this->assertEquals($contactID_b, $dao->contact_id, "Search query returns spouse of contact A?"); + } + + public function testReciprocalRelationshipTargetGroupUsesTempTable() { + $groupID = $this->groupCreate(); + $params = array( + array( + 0 => 'relation_type_id', + 1 => 'IN', + 2 => + array( + 0 => '2_a_b', + ), + 3 => 0, + 4 => 0, + ), + array( + 0 => 'relation_target_group', + 1 => 'IN', + 2 => + array( + 0 => $groupID, + ), + 3 => 0, + 4 => 0, + ), + ); + $sql = CRM_Contact_BAO_Query::getQuery($params); + $this->assertContains('INNER JOIN civicrm_rel_temp_', $sql, "Query appears to use temporary table of compiled relationships?", TRUE); + } + /** * Test Relationship Clause */