From 6b0513122b7d5bf9b87e5cecb6216c06a42ecada Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 22 Nov 2019 11:53:02 +1300 Subject: [PATCH] Fix fatal error when sorting by status in activity search --- CRM/Activity/BAO/Activity.php | 5 ++- CRM/Activity/BAO/Query.php | 28 +++++++------- CRM/Contact/BAO/Query.php | 8 +++- CRM/Core/DAO.php | 4 +- .../CRM/Activity/Selector/SearchTest.php | 38 +++++++++++++++++++ tests/phpunit/CiviTest/CiviUnitTestCase.php | 6 ++- 6 files changed, 70 insertions(+), 19 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index a18061ae96..11e8a49c5a 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -2119,6 +2119,9 @@ AND cl.modified_id = c.id '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'), @@ -2189,7 +2192,7 @@ AND cl.modified_id = c.id // 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 index 6cea3e0f90..548d6cd96c 100644 --- a/CRM/Activity/BAO/Query.php +++ b/CRM/Activity/BAO/Query.php @@ -82,13 +82,10 @@ class CRM_Activity_BAO_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'])) { @@ -189,15 +186,24 @@ class CRM_Activity_BAO_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': @@ -215,7 +221,6 @@ class CRM_Activity_BAO_Query { $name = $qillName = str_replace('activity_', '', $name); } if (in_array($name, [ - 'activity_status_id', 'activity_subject', 'activity_priority_id', ])) { @@ -228,7 +233,11 @@ class CRM_Activity_BAO_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'], @@ -242,7 +251,6 @@ class CRM_Activity_BAO_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); @@ -413,12 +421,6 @@ class CRM_Activity_BAO_Query { 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 index 71b5dfbb79..4cfc7b2e84 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -6934,7 +6934,7 @@ AND displayRelType.is_active = 1 * * @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']); @@ -7031,7 +7031,11 @@ AND displayRelType.is_active = 1 */ 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 index c2f024face..4f1bad0a36 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2475,7 +2475,7 @@ SELECT contact_id * @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'])) { @@ -2483,7 +2483,7 @@ SELECT contact_id '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 index 9370cd56c0..c6eec4180d 100644 --- a/tests/phpunit/CRM/Activity/Selector/SearchTest.php +++ b/tests/phpunit/CRM/Activity/Selector/SearchTest.php @@ -47,4 +47,42 @@ class CRM_Activity_Selector_SearchTest extends CiviUnitTestCase { $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 index fae2b981b9..505aa775d9 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -1726,7 +1726,11 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { 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); } } -- 2.25.1