From d1d108ee227039e6eee69dd037c626cd9b301c20 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 7 May 2020 16:41:14 +1200 Subject: [PATCH] Remove slow join from activity search This removes the unindexed join from activity search. The join is used to apply component permission filtering and to retrieve the activity_type - however, this latter is still rendered by the pseudoconstant handling --- CRM/Activity/BAO/Activity.php | 8 +++-- CRM/Activity/BAO/Query.php | 17 +++------- CRM/Activity/Selector/Search.php | 31 ------------------- CRM/Contact/BAO/Query.php | 22 +++++++++++++ CRM/Core/DAO.php | 4 +-- CRM/Export/BAO/ExportProcessor.php | 2 +- .../CRM/Activity/Selector/SearchTest.php | 3 ++ tests/phpunit/CRM/Contact/SelectorTest.php | 12 +++---- 8 files changed, 43 insertions(+), 56 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 68282aee31c1..cea9b5335563 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -905,13 +905,15 @@ public static function filterActivityTypes($params) { */ public function addSelectWhereClause() { $clauses = []; - // @todo - check if $permissedActivityTYpes === all activity types and do not add critieria if so. $permittedActivityTypeIDs = self::getPermittedActivityTypes(); + $allActivityTypes = self::buildOptions('activity_type_id'); if (empty($permittedActivityTypeIDs)) { // This just prevents a mysql fail if they have no access - should be extremely edge case. $permittedActivityTypeIDs = [0]; } - $clauses['activity_type_id'] = ('IN (' . implode(', ', $permittedActivityTypeIDs) . ')'); + if (array_keys($allActivityTypes) !== array_keys($permittedActivityTypeIDs)) { + $clauses['activity_type_id'] = ('IN (' . implode(', ', $permittedActivityTypeIDs) . ')'); + } $contactClause = CRM_Utils_SQL::mergeSubquery('Contact'); if ($contactClause) { @@ -2131,7 +2133,7 @@ public static function exportableFields($name = 'Activity') { // TODO: ideally we should retrieve all fields from xml, in this case since activity processing is done // my case hence we have defined fields as case_* - if ($name == 'Activity') { + if ($name === 'Activity') { $exportableFields = CRM_Activity_DAO_Activity::export(); $exportableFields['source_contact_id'] = [ 'title' => ts('Source Contact ID'), diff --git a/CRM/Activity/BAO/Query.php b/CRM/Activity/BAO/Query.php index 808b672e451a..c36e6829a7d1 100644 --- a/CRM/Activity/BAO/Query.php +++ b/CRM/Activity/BAO/Query.php @@ -28,22 +28,13 @@ public static function select(&$query) { $query->_tables['civicrm_activity'] = $query->_whereTables['civicrm_activity'] = 1; } - if (!empty($query->_returnProperties['activity_type_id'])) { - $query->_select['activity_type_id'] = 'activity_type.value as activity_type_id'; + if (!empty($query->_returnProperties['activity_type_id']) + || !empty($query->_returnProperties['activity_type']) + ) { + $query->_select['activity_type_id'] = 'civicrm_activity.activity_type_id'; $query->_element['activity_type_id'] = 1; $query->_tables['civicrm_activity'] = 1; - $query->_tables['activity_type'] = 1; - $query->_whereTables['civicrm_activity'] = 1; - $query->_whereTables['activity_type'] = 1; - } - - if (!empty($query->_returnProperties['activity_type'])) { - $query->_select['activity_type'] = 'activity_type.label as activity_type'; - $query->_element['activity_type'] = 1; - $query->_tables['civicrm_activity'] = 1; - $query->_tables['activity_type'] = 1; $query->_whereTables['civicrm_activity'] = 1; - $query->_whereTables['activity_type'] = 1; } if (!empty($query->_returnProperties['activity_subject'])) { diff --git a/CRM/Activity/Selector/Search.php b/CRM/Activity/Selector/Search.php index d0367750cb8b..10c76703c287 100644 --- a/CRM/Activity/Selector/Search.php +++ b/CRM/Activity/Selector/Search.php @@ -154,37 +154,6 @@ public function __construct( $this->_activityClause = $activityClause; - // CRM-12675 - $components = CRM_Core_Component::getNames(); - $componentClause = []; - foreach ($components as $componentID => $componentName) { - // CRM-19201: Add support for searching CiviCampaign and CiviCase - // activities. For CiviCase, "access all cases and activities" is - // required here rather than "access my cases and activities" to - // prevent those with only the later permission from seeing a list - // of all cases which might present a privacy issue. - // @todo this is the cause of the current devastatingly bad performance on - // activity search - it involves a bad join. - // The correct fix is to use the permission infrastrucutre - ie. add in the - // clause generated by CRM_Activity_BAO_Query::addSelectWhere - // but some testing needs to check that before making the change - // see https://github.com/civicrm/civicrm-core/blob/be2fb01f90f5f299dd07402a41fed7c7c7567f00/CRM/Utils/SQL.php#L48 - // for how it's done in the api kernel. - if (!CRM_Core_Permission::access($componentName, TRUE, TRUE)) { - $componentClause[] = " (activity_type.component_id IS NULL OR activity_type.component_id <> {$componentID}) "; - } - } - - if (!empty($componentClause)) { - $componentRestriction = implode(' AND ', $componentClause); - if (empty($this->_activityClause)) { - $this->_activityClause = $componentRestriction; - } - else { - $this->_activityClause .= ' AND ' . $componentRestriction; - } - } - // type of selector $this->_action = $action; $this->_query = new CRM_Contact_BAO_Query($this->_queryParams, diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index cda4ff52bec5..3b1340a0d09c 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -5047,6 +5047,28 @@ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) { $this->_skipDeleteClause ); + if (isset($this->_tables['civicrm_activity'])) { + $bao = new CRM_Activity_BAO_Activity(); + $clauses = $subclauses = []; + foreach ((array) $bao->addSelectWhereClause() as $field => $vals) { + if ($vals && $field !== 'id') { + $clauses[] = $bao->tableName() . ".$field " . $vals; + } + elseif ($vals) { + $subclauses[] = "$field " . implode(" AND $field ", (array) $vals); + } + } + if ($subclauses) { + $clauses[] = $bao->tableName() . '.`id` IN (SELECT `id` FROM `' . $bao->tableName() . '` WHERE ' . implode(' AND ', $subclauses) . ')'; + } + if (!empty($clauses) && $this->_permissionWhereClause) { + $this->_permissionWhereClause .= ' AND (' . implode(' AND ', $clauses) . ')'; + } + elseif (!empty($clauses)) { + $this->_permissionWhereClause .= '(' . implode(' AND ', $clauses) . ')'; + } + } + // regenerate fromClause since permission might have added tables if ($this->_permissionWhereClause) { //fix for row count in qill (in contribute/membership find) diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index ce62eaba1c3a..056c3a9a33cd 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2640,7 +2640,7 @@ public function getOptionLabels() { * * @param string $context * - * @throws Exception + * @throws CRM_Core_Exception * @return array */ public static function buildOptionsContext($context = NULL) { @@ -2654,7 +2654,7 @@ public static function buildOptionsContext($context = NULL) { ]; // Validation: enforce uniformity of this param if ($context !== NULL && !isset($contexts[$context])) { - throw new Exception("'$context' is not a valid context for buildOptions."); + throw new CRM_Core_Exception("'$context' is not a valid context for buildOptions."); } return $contexts; } diff --git a/CRM/Export/BAO/ExportProcessor.php b/CRM/Export/BAO/ExportProcessor.php index 9402772dce17..03e64b31b2a0 100644 --- a/CRM/Export/BAO/ExportProcessor.php +++ b/CRM/Export/BAO/ExportProcessor.php @@ -749,7 +749,7 @@ public function getExportFileName() { * @return string */ public function getHeaderForRow($field) { - if (substr($field, -11) == 'campaign_id') { + if (substr($field, -11) === 'campaign_id') { // @todo - set this correctly in the xml rather than here. // This will require a generalised handling cleanup return ts('Campaign ID'); diff --git a/tests/phpunit/CRM/Activity/Selector/SearchTest.php b/tests/phpunit/CRM/Activity/Selector/SearchTest.php index ab28f9bd7ed2..584d6184b646 100644 --- a/tests/phpunit/CRM/Activity/Selector/SearchTest.php +++ b/tests/phpunit/CRM/Activity/Selector/SearchTest.php @@ -18,6 +18,9 @@ class CRM_Activity_Selector_SearchTest extends CiviUnitTestCase { /** * Test activity search applies a permission based component filter. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ public function testActivitySearchComponentPermission() { $this->activityCreate(['activity_type_id' => 'Contribution']); diff --git a/tests/phpunit/CRM/Contact/SelectorTest.php b/tests/phpunit/CRM/Contact/SelectorTest.php index 8597a60f1f83..e86c633c12bc 100644 --- a/tests/phpunit/CRM/Contact/SelectorTest.php +++ b/tests/phpunit/CRM/Contact/SelectorTest.php @@ -55,24 +55,24 @@ public function testSelectorQuery($dataSet) { $this->assertLike($this->strWrangle($queryString), $this->strWrangle($sql[$index])); } // Ensure that search builder return individual contact as per criteria - if ($dataSet['context'] == 'builder') { + if ($dataSet['context'] === 'builder') { $contactID = $this->individualCreate(['first_name' => 'James', 'last_name' => 'Bond']); - if ('Search builder behaviour for Activity' == $dataSet['description']) { + if ('Search builder behaviour for Activity' === $dataSet['description']) { $this->callAPISuccess('Activity', 'create', [ 'activity_type_id' => 'Meeting', - 'subject' => "Test", + 'subject' => 'Test', 'source_contact_id' => $contactID, ]); $rows = CRM_Core_DAO::executeQuery(implode(' ', $sql))->fetchAll(); - $this->assertEquals(1, count($rows)); + $this->assertCount(1, $rows); $this->assertEquals($contactID, $rows[0]['source_contact_id']); } else { $this->callAPISuccess('Address', 'create', [ 'contact_id' => $contactID, - 'location_type_id' => "Home", + 'location_type_id' => 'Home', 'is_primary' => 1, - 'country_id' => "IN", + 'country_id' => 'IN', ]); $rows = $selector->getRows(CRM_Core_Action::VIEW, 0, 50, ''); $this->assertEquals(1, count($rows));