Merge pull request #17250 from eileenmcnaughton/act_search

Remove slow join from activity search

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'),

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'])) {

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,

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)

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

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

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

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