Fix fatal error when sorting by status in activity search

civicrm · Nov 25, 2019 · 26896d8 · 26896d8
1 parent 6aa2665
commit 26896d8
Show file tree

Hide file tree

Showing 6 changed files with 70 additions and 19 deletions.
diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
@@ -2097,6 +2097,9 @@ public static function exportableFields($name = 'Activity') {
         'type' => CRM_Utils_Type::T_STRING,
       ];
 
+      // @todo - remove these - they are added by CRM_Core_DAO::appendPseudoConstantsToFields
+      // below. That search label stuff is referenced in search builder but is likely just
+      // a hack that duplicates, maybe differently, other functionality.
       $Activityfields = [
         'activity_type' => [
           'title' => ts('Activity Type'),
@@ -2168,7 +2171,7 @@ public static function exportableFields($name = 'Activity') {
 
     // add custom data for case activities
     $fields = array_merge($fields, CRM_Core_BAO_CustomField::getFieldsForImport('Activity'));
-
+    CRM_Core_DAO::appendPseudoConstantsToFields($fields);
     self::$_exportableFields[$name] = $fields;
     return self::$_exportableFields[$name];
   }

diff --git a/CRM/Activity/BAO/Query.php b/CRM/Activity/BAO/Query.php
@@ -66,13 +66,10 @@ public static function select(&$query) {
     }
 
     if (!empty($query->_returnProperties['activity_status'])) {
-      $query->_select['activity_status'] = 'activity_status.label as activity_status,
-      civicrm_activity.status_id as status_id';
+      $query->_select['activity_status_id'] = 1;
       $query->_element['activity_status'] = 1;
       $query->_tables['civicrm_activity'] = 1;
-      $query->_tables['activity_status'] = 1;
       $query->_whereTables['civicrm_activity'] = 1;
-      $query->_whereTables['activity_status'] = 1;
     }
 
     if (!empty($query->_returnProperties['activity_duration'])) {
@@ -173,15 +170,24 @@ public static function where(&$query) {
    *
    * @param array $values
    * @param CRM_Contact_BAO_Query $query
+   *
+   * @throws \CRM_Core_Exception
    */
   public static function whereClauseSingle(&$values, &$query) {
     list($name, $op, $value, $grouping) = $values;
 
     $fields = CRM_Activity_BAO_Activity::exportableFields();
+    $fieldSpec = $query->getFieldSpec($name);
     $query->_tables['civicrm_activity'] = $query->_whereTables['civicrm_activity'] = 1;
     if ($query->_mode & CRM_Contact_BAO_Query::MODE_ACTIVITY) {
       $query->_skipDeleteClause = TRUE;
     }
+    // @todo we want to do this in a more metadata driven way, and also in contribute.
+    // But for the rc...
+    $namesToConvert = [
+      'activity_status' => 'activity_status_id',
+    ];
+    $name = $namesToConvert[$name] ?? $name;
 
     switch ($name) {
       case 'activity_type_id':
@@ -199,7 +205,6 @@ public static function whereClauseSingle(&$values, &$query) {
           $name = $qillName = str_replace('activity_', '', $name);
         }
         if (in_array($name, [
-          'activity_status_id',
           'activity_subject',
           'activity_priority_id',
         ])) {
@@ -212,7 +217,11 @@ public static function whereClauseSingle(&$values, &$query) {
 
         $dataType = !empty($fields[$qillName]['type']) ? CRM_Utils_Type::typeToString($fields[$qillName]['type']) : 'String';
 
-        $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause("civicrm_activity.$name", $op, $value, $dataType);
+        $where = $fieldSpec['where'];
+        if (!$where) {
+          $where = 'civicrm_activity.' . $name;
+        }
+        $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause($where, $op, $value, $dataType);
         list($op, $value) = CRM_Contact_BAO_Query::buildQillForFieldValue('CRM_Activity_DAO_Activity', $name, $value, $op);
         $query->_qill[$grouping][] = ts('%1 %2 %3', [
           1 => $fields[$qillName]['title'],
@@ -226,7 +235,6 @@ public static function whereClauseSingle(&$values, &$query) {
         break;
 
       case 'activity_type':
-      case 'activity_status':
       case 'activity_priority':
         $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause("$name.label", $op, $value, 'String');
         list($op, $value) = CRM_Contact_BAO_Query::buildQillForFieldValue('CRM_Activity_DAO_Activity', $name, $value, $op);
@@ -397,12 +405,6 @@ public static function from($name, $mode, $side) {
                       ON ( civicrm_activity_contact.contact_id = civicrm_contact.id and civicrm_contact.is_deleted != 1 )";
         break;
 
-      case 'activity_status':
-        $from .= " $side JOIN civicrm_option_group option_group_activity_status ON (option_group_activity_status.name = 'activity_status')";
-        $from .= " $side JOIN civicrm_option_value activity_status ON (civicrm_activity.status_id = activity_status.value
-                               AND option_group_activity_status.id = activity_status.option_group_id ) ";
-        break;
-
       case 'activity_type':
         $from .= " $side JOIN civicrm_option_group option_group_activity_type ON (option_group_activity_type.name = 'activity_type')";
         $from .= " $side JOIN civicrm_option_value activity_type ON (civicrm_activity.activity_type_id = activity_type.value

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
@@ -6914,7 +6914,7 @@ protected function getMetadataForField($fieldName) {
    *
    * @return array
    */
-  protected function getMetadataForRealField($fieldName) {
+  public function getMetadataForRealField($fieldName) {
     $field = $this->getMetadataForField($fieldName);
     if (!empty($field['is_pseudofield_for'])) {
       $field = $this->getMetadataForField($field['is_pseudofield_for']);
@@ -7011,7 +7011,11 @@ protected function isARelativeDateField($fieldName) {
    */
   public function getFieldSpec($fieldName) {
     if (isset($this->_fields[$fieldName])) {
-      return $this->_fields[$fieldName];
+      $fieldSpec = $this->_fields[$fieldName];
+      if (!empty($fieldSpec['is_pseudofield_for'])) {
+        $fieldSpec = array_merge($this->_fields[$fieldSpec['is_pseudofield_for']], $this->_fields[$fieldName]);
+      }
+      return $fieldSpec;
     }
     $lowFieldName = str_replace('_low', '', $fieldName);
     if (isset($this->_fields[$lowFieldName])) {

diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
@@ -2467,15 +2467,15 @@ public static function getGlobalSetting($name, $default = NULL) {
    * @param array $fields
    */
   public static function appendPseudoConstantsToFields(&$fields) {
-    foreach ($fields as $field) {
+    foreach ($fields as $fieldUniqueName => $field) {
       if (!empty($field['pseudoconstant'])) {
         $pseudoConstant = $field['pseudoconstant'];
         if (!empty($pseudoConstant['optionGroupName'])) {
           $fields[$pseudoConstant['optionGroupName']] = [
             'title' => CRM_Core_BAO_OptionGroup::getTitleByName($pseudoConstant['optionGroupName']),
             'name' => $pseudoConstant['optionGroupName'],
             'data_type' => CRM_Utils_Type::T_STRING,
-            'is_pseudofield_for' => $field['name'],
+            'is_pseudofield_for' => $fieldUniqueName,
           ];
         }
         // We restrict to id + name + FK as we are extending this a bit, but cautiously.

diff --git a/tests/phpunit/CRM/Activity/Selector/SearchTest.php b/tests/phpunit/CRM/Activity/Selector/SearchTest.php
@@ -31,4 +31,42 @@ public function testActivitySearchComponentPermission() {
     $this->assertEquals("civicrm_activity.location = 'Baker Street'", $queryObject->_where[''][0]);
   }
 
+  public function testActivityOrderBy() {
+    $sortVars = [
+      1 => [
+        'name' => 'activity_date_time',
+        'sort' => 'activity_date_time',
+        'direction' => 2,
+        'title' => 'Date',
+      ],
+      2 => [
+        'name' => 'activity_type_id',
+        'sort' => 'activity_type_id',
+        'direction' => 4,
+        'title' => 'Type',
+      ],
+      3 => [
+        'name' => 'activity_subject',
+        'sort' => 'activity_subject',
+        'direction' => 4,
+        'title' => 'Subject',
+      ],
+      4 => [
+        'name' => 'source_contact',
+        'sort' => 'source_contact',
+        'direction' => 4,
+        'title' => 'Added By',
+      ],
+      5 => [
+        'name' => 'activity_status',
+        'sort' => 'activity_status',
+        'direction' => 1,
+        'title' => 'Status',
+      ],
+    ];
+    $sort = new CRM_Utils_Sort($sortVars, '5_u');
+    $searchSelector = new CRM_Activity_Selector_Search($queryParams, CRM_Core_Action::VIEW);
+    $searchSelector->getRows(4, 0, 50, $sort);
+  }
+
 }
diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php
@@ -1765,7 +1765,11 @@ public function quickCleanup($tablesToTruncate, $dropCustomValueTables = FALSE)
     if ($dropCustomValueTables) {
       $optionGroupResult = CRM_Core_DAO::executeQuery('SELECT option_group_id FROM civicrm_custom_field');
       while ($optionGroupResult->fetch()) {
-        if (!empty($optionGroupResult->option_group_id)) {
+        // We have a test that sets the option_group_id for a custom group to that of 'activity_type'.
+        // Then test tearDown deletes it. This is all mildly terrifying but for the context here we can be pretty
+        // sure the low-numbered (50 is arbitrary) option groups are not ones to 'just delete' in a
+        // generic cleanup routine.
+        if (!empty($optionGroupResult->option_group_id) && $optionGroupResult->option_group_id > 50) {
           CRM_Core_DAO::executeQuery('DELETE FROM civicrm_option_group WHERE id = ' . $optionGroupResult->option_group_id);
         }
       }