From: eileen Date: Fri, 22 Feb 2019 20:49:12 +0000 (+1300) Subject: Fix & test searchQuery order by to be less dependent on what is selected for search X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=a0090e6bf5bd0d91e22642d0a36f124d6f1877ea;p=civicrm-core.git Fix & test searchQuery order by to be less dependent on what is selected for search The pseudoconstant array is built when doing a select - if a field is in order by but not select it will fail - as in the test. This is a bit hard to hit outside the test for contribution but makes it more robust` allowing us to address other bugs & performance issues. --- diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 7a236281d7..fc18ed8e20 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -6315,24 +6315,25 @@ AND displayRelType.is_active = 1 $this->_select = array_merge($this->_select, $this->_customQuery->_select); $this->_tables = array_merge($this->_tables, $this->_customQuery->_tables); } - foreach ($this->_pseudoConstantsSelect as $key => $pseudoConstantMetadata) { - // By replacing the join to the option value table with the mysql construct - // ORDER BY field('contribution_status_id', 2,1,4) - // we can remove a join. In the case of the option value join it is - /// a join known to cause slow queries. - // @todo cover other pseudoconstant types. Limited to option group ones in the - // first instance for scope reasons. They require slightly different handling as the column (label) - // is not declared for them. - // @todo so far only integer fields are being handled. If we add string fields we need to look at - // escaping. - if (isset($pseudoConstantMetadata['pseudoconstant']) - && isset($pseudoConstantMetadata['pseudoconstant']['optionGroupName']) - && $field === CRM_Utils_Array::value('optionGroupName', $pseudoConstantMetadata['pseudoconstant']) - ) { - $sortedOptions = $pseudoConstantMetadata['bao']::buildOptions($pseudoConstantMetadata['pseudoField'], NULL, array( + + // By replacing the join to the option value table with the mysql construct + // ORDER BY field('contribution_status_id', 2,1,4) + // we can remove a join. In the case of the option value join it is + /// a join known to cause slow queries. + // @todo cover other pseudoconstant types. Limited to option group ones in the + // first instance for scope reasons. They require slightly different handling as the column (label) + // is not declared for them. + // @todo so far only integer fields are being handled. If we add string fields we need to look at + // escaping. + $fieldSpec = $this->getMetadataForRealField($field); + $pseudoConstantMetadata = CRM_Utils_Array::value('pseudoconstant', $fieldSpec, FALSE); + if (!empty($pseudoConstantMetadata) + ) { + if (!empty($pseudoConstantMetadata['optionGroupName'])) { + $sortedOptions = $fieldSpec['bao']::buildOptions($fieldSpec['name'], NULL, [ 'orderColumn' => 'label', - )); - $order = str_replace("$field $direction", "field({$pseudoConstantMetadata['pseudoField']}," . implode(',', array_keys($sortedOptions)) . ") $direction", $order); + ]); + $order = str_replace("$field", "field({$fieldSpec['name']}," . implode(',', array_keys($sortedOptions)) . ")", $order); } //CRM-12565 add "`" around $field if it is a pseudo constant // This appears to be for 'special' fields like locations with appended numbers or hyphens .. maybe. @@ -6572,7 +6573,6 @@ AND displayRelType.is_active = 1 } /** - * /** * Create the sql query for an contact search. * * @param int $offset @@ -6702,4 +6702,39 @@ AND displayRelType.is_active = 1 return $query; } + /** + * Get the metadata for a given field. + * + * @param string $fieldName + * + * @return array + */ + protected function getMetadataForField($fieldName) { + if ($fieldName === 'contact_a.id') { + // This seems to be the only anomaly. + $fieldName = 'id'; + } + $pseudoField = isset($this->_pseudoConstantsSelect[$fieldName]) ? $this->_pseudoConstantsSelect[$fieldName] : []; + $field = isset($this->_fields[$fieldName]) ? $this->_fields[$fieldName] : $pseudoField; + $field = array_merge($field, $pseudoField); + if (!empty($field) && empty($field['name'])) { + // standardising field formatting here - over time we can phase out variants. + // all paths using this currently unit tested + $field['name'] = CRM_Utils_Array::value('field_name', $field, $field['idCol']); + } + return $field; + } + + /** + * Get the metadata for a given field, returning the 'real field' if it is a pseudofield. + * + * @param string $fieldName + * + * @return array + */ + protected function getMetadataForRealField($fieldName) { + $field = $this->getMetadataForField($fieldName); + return empty($field['is_pseudofield_for']) ? $field : $this->getMetadataForField($field['is_pseudofield_for']); + } + } diff --git a/CRM/Contribute/BAO/Query.php b/CRM/Contribute/BAO/Query.php index 6b2ff0e93c..0cd7e158fe 100644 --- a/CRM/Contribute/BAO/Query.php +++ b/CRM/Contribute/BAO/Query.php @@ -48,7 +48,8 @@ class CRM_Contribute_BAO_Query extends CRM_Core_BAO_Query { */ public static function getFields($checkPermission = TRUE) { if (!isset(\Civi::$statics[__CLASS__]) || !isset(\Civi::$statics[__CLASS__]['fields']) || !isset(\Civi::$statics[__CLASS__]['contribution'])) { - $fields = CRM_Contribute_BAO_Contribution::exportableFields($checkPermission); + $fields = CRM_Contribute_BAO_Contribution::exportableFields($checkPermission); + CRM_Contribute_BAO_Contribution::appendPseudoConstantsToFields($fields); unset($fields['contribution_contact_id']); \Civi::$statics[__CLASS__]['fields']['contribution'] = $fields; } diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index eb24e0ed28..54ac1e4be7 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2367,13 +2367,14 @@ SELECT contact_id * * @param array $fields */ - protected static function appendPseudoConstantsToFields(&$fields) { + public static function appendPseudoConstantsToFields(&$fields) { foreach ($fields as $field) { if (!empty($field['pseudoconstant']) && !empty($field['pseudoconstant']['optionGroupName'])) { $fields[$field['pseudoconstant']['optionGroupName']] = array( 'title' => CRM_Core_BAO_OptionGroup::getTitleByName($field['pseudoconstant']['optionGroupName']), 'name' => $field['pseudoconstant']['optionGroupName'], 'data_type' => CRM_Utils_Type::T_STRING, + 'is_pseudofield_for' => $field['name'], ); } } diff --git a/tests/phpunit/CRM/Contribute/BAO/QueryTest.php b/tests/phpunit/CRM/Contribute/BAO/QueryTest.php new file mode 100644 index 0000000000..928ac2bd17 --- /dev/null +++ b/tests/phpunit/CRM/Contribute/BAO/QueryTest.php @@ -0,0 +1,57 @@ +quickCleanUpFinancialEntities(); + } + + /** + * Check that we get a successful trying to return by pseudo-fields + * - financial_type. + * + * @param string $sort + * + * @dataProvider getSortFields + */ + public function testSearchPseudoReturnProperties($sort) { + $contactID = $this->individualCreate(); + $this->contributionCreate(['contact_id' => $contactID, 'financial_type_id' => 'Campaign Contribution']); + $this->contributionCreate(['contact_id' => $contactID, 'financial_type_id' => 'Donation']); + $donationTypeID = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 'Donation'); + + $params = [ + ['financial_type_id', '=', $donationTypeID , 1, 0], + ]; + + $queryObj = new CRM_Contact_BAO_Query($params); + try { + $resultDAO = $queryObj->searchQuery(0, 0, $sort . ' asc'); + $this->assertTrue($resultDAO->fetch()); + $this->assertEquals(1, $resultDAO->N); + } + catch (PEAR_Exception $e) { + $err = $e->getCause(); + $this->fail('invalid SQL created' . $e->getMessage() . " " . $err->userinfo); + + } + } + + /** + * Data provider for sort fields + */ + public function getSortFields() { + return [ + ['payment_instrument'], + ['individual_prefix'], + ['communication_style'], + ['gender'], + ['state_province'], + ['country'], + ]; + } + +} diff --git a/tests/phpunit/CRM/Export/BAO/ExportTest.php b/tests/phpunit/CRM/Export/BAO/ExportTest.php index e698fc550b..e8d25568a0 100644 --- a/tests/phpunit/CRM/Export/BAO/ExportTest.php +++ b/tests/phpunit/CRM/Export/BAO/ExportTest.php @@ -2192,7 +2192,7 @@ class CRM_Export_BAO_ExportTest extends CiviUnitTestCase { 85 => 'Cancel Date', 86 => 'Total Amount', 87 => 'Accounting Code', - 88 => 'payment_instrument', + 88 => 'Payment Methods', 89 => 'Payment Method ID', 90 => 'Check Number', 91 => 'Non-deductible Amount', @@ -2212,7 +2212,7 @@ class CRM_Export_BAO_ExportTest extends CiviUnitTestCase { 105 => 'End date for premium', 106 => 'Test', 107 => 'Is Pay Later', - 108 => 'contribution_status', + 108 => 'Contribution Status', 109 => 'Recurring Contribution ID', 110 => 'Amount Label', 111 => 'Contribution Note', @@ -2600,7 +2600,7 @@ class CRM_Export_BAO_ExportTest extends CiviUnitTestCase { 'cancel_date' => 'cancel_date varchar(32)', 'total_amount' => 'total_amount varchar(32)', 'accounting_code' => 'accounting_code varchar(64)', - 'payment_instrument' => 'payment_instrument text', + 'payment_instrument' => 'payment_instrument varchar(255)', 'payment_instrument_id' => 'payment_instrument_id varchar(16)', 'contribution_check_number' => 'contribution_check_number varchar(255)', 'non_deductible_amount' => 'non_deductible_amount varchar(32)', @@ -2620,7 +2620,7 @@ class CRM_Export_BAO_ExportTest extends CiviUnitTestCase { 'contribution_end_date' => 'contribution_end_date varchar(32)', 'is_test' => 'is_test varchar(16)', 'is_pay_later' => 'is_pay_later varchar(16)', - 'contribution_status' => 'contribution_status text', + 'contribution_status' => 'contribution_status varchar(255)', 'contribution_recur_id' => 'contribution_recur_id varchar(16)', 'amount_level' => 'amount_level longtext', 'contribution_note' => 'contribution_note text',