From 7f59431188688d4697bff55738fe17996abe442f Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 2 Mar 2019 19:41:32 +1300 Subject: [PATCH] Fix bug whereby sorting by state province gives an error in search builder --- CRM/Contact/BAO/Query.php | 44 +++++++++++---------- CRM/Contact/Selector.php | 3 +- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 26 ++++++++++++ tests/phpunit/CRM/Contact/SelectorTest.php | 3 +- 4 files changed, 53 insertions(+), 23 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 050beffcff..fc7d9a75e9 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -2680,6 +2680,18 @@ class CRM_Contact_BAO_Query { //CRM-14263 further handling of address joins further down... return " $side JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id {$limitToPrimaryClause} )"; + case 'civicrm_state_province': + // This is encountered when doing an export after having applied a 'sort' - it pretty much implies primary + // but that will have been implied-in by the calling function. + // test cover in testContactIDQuery + return " $side JOIN civicrm_state_province ON ( civicrm_address.state_province_id = civicrm_state_province.id )"; + + case 'civicrm_country': + // This is encountered when doing an export after having applied a 'sort' - it pretty much implies primary + // but that will have been implied-in by the calling function. + // test cover in testContactIDQuery + return " $side JOIN civicrm_country ON ( civicrm_address.country_id = civicrm_country.id )"; + case 'civicrm_phone': return " $side JOIN civicrm_phone ON (contact_a.id = civicrm_phone.contact_id {$limitToPrimaryClause}) "; @@ -2699,8 +2711,9 @@ class CRM_Contact_BAO_Query { return " $side JOIN civicrm_openid ON ( civicrm_openid.contact_id = contact_a.id {$limitToPrimaryClause} )"; case 'civicrm_worldregion': - $from = " $side JOIN civicrm_country ON civicrm_address.country_id = civicrm_country.id "; - return "$from $side JOIN civicrm_worldregion ON civicrm_country.region_id = civicrm_worldregion.id "; + // We can be sure from the calling function that country will already be joined in. + // we really don't need world_region - we could use a pseudoconstant for it. + return "$side JOIN civicrm_worldregion ON civicrm_country.region_id = civicrm_worldregion.id "; case 'civicrm_location_type': return " $side JOIN civicrm_location_type ON civicrm_address.location_type_id = civicrm_location_type.id "; @@ -6320,26 +6333,18 @@ AND displayRelType.is_active = 1 $orderByClauseParts = explode(' ', trim($orderByClause)); $field = $orderByClauseParts[0]; $direction = isset($orderByClauseParts[1]) ? $orderByClauseParts[1] : 'asc'; + $fieldSpec = $this->getMetadataForRealField($field); - switch ($field) { - case 'city': - case 'postal_code': - $this->_tables["civicrm_address"] = $this->_whereTables["civicrm_address"] = 1; - $order = str_replace($field, "civicrm_address.{$field}", $order); - break; + if ($this->_returnProperties === []) { + if (!empty($fieldSpec['table_name']) && !isset($this->_tables[$fieldSpec['table_name']])) { + $this->_tables[$fieldSpec['table_name']] = 1; + $order = $fieldSpec['where'] . ' ' . $direction; + } - case 'country': - case 'state_province': - $this->_tables["civicrm_{$field}"] = $this->_whereTables["civicrm_{$field}"] = 1; - if (is_array($this->_returnProperties) && empty($this->_returnProperties)) { - $additionalFromClause .= " LEFT JOIN civicrm_{$field} ON civicrm_{$field}.id = civicrm_address.{$field}_id"; - } - $order = str_replace($field, "civicrm_{$field}.name", $order); - break; + } + switch ($field) { - case 'email': - $this->_tables["civicrm_email"] = $this->_whereTables["civicrm_email"] = 1; - $order = str_replace($field, "civicrm_email.{$field}", $order); + case 'placeholder-will-remove-next-pr-but-jenkins-will-not-accept-without-and-removing-switch-will-make-hard-to-read': break; default: @@ -6363,7 +6368,6 @@ AND displayRelType.is_active = 1 // 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) ) { diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php index 7f3952a169..fc5bcc3317 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -500,8 +500,7 @@ class CRM_Contact_Selector extends CRM_Core_Selector_Base implements CRM_Core_Se CRM_Core_PseudoConstant::getKey('CRM_Core_DAO_Address', 'location_type_id', $loc) ) ); - // use field name instead of table alias - $prop = $fld; + } elseif (isset($this->_query->_fields[$prop]) && isset($this->_query->_fields[$prop]['title'])) { $title = $this->_query->_fields[$prop]['title']; diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 1f94137f69..a387a60777 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -696,6 +696,32 @@ civicrm_relationship.is_active = 1 AND $this->fail('Test failed for some reason which is not good'); } + /** + * Test the sorting on the contact ID query works. + * + * Checking for lack of fatal. + * + * @param string $sortOrder + * Param reflecting how sort is passed in. + * - 1_d is column 1 descending. + * + * @dataProvider getSortOptions + */ + public function testContactIDQuery($sortOrder) { + $selector = new CRM_Contact_Selector(NULL, ['radio_ts' => 'ts_all'], NULL, ['sort_name' => 1]); + $selector->contactIDQuery([], $sortOrder); + } + + public function getSortOptions() { + return [ + ['1_d'], + ['2_d'], + ['3_d'], + ['4_d'], + ['5_d'], + ['6_d'], + ]; + } /** * Test the summary query does not add an acl clause when acls not enabled.. diff --git a/tests/phpunit/CRM/Contact/SelectorTest.php b/tests/phpunit/CRM/Contact/SelectorTest.php index 2e3bc97fc8..1eb5337323 100644 --- a/tests/phpunit/CRM/Contact/SelectorTest.php +++ b/tests/phpunit/CRM/Contact/SelectorTest.php @@ -550,10 +550,11 @@ class CRM_Contact_SelectorTest extends CiviUnitTestCase { */ public function getDefaultFromString() { return ' FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 )' + . ' LEFT JOIN civicrm_country ON ( civicrm_address.country_id = civicrm_country.id ) ' . ' LEFT JOIN civicrm_email ON (contact_a.id = civicrm_email.contact_id AND civicrm_email.is_primary = 1)' . ' LEFT JOIN civicrm_phone ON (contact_a.id = civicrm_phone.contact_id AND civicrm_phone.is_primary = 1)' . ' LEFT JOIN civicrm_im ON (contact_a.id = civicrm_im.contact_id AND civicrm_im.is_primary = 1) ' - . 'LEFT JOIN civicrm_country ON civicrm_address.country_id = civicrm_country.id LEFT JOIN civicrm_worldregion ON civicrm_country.region_id = civicrm_worldregion.id '; + . 'LEFT JOIN civicrm_worldregion ON civicrm_country.region_id = civicrm_worldregion.id '; } /** -- 2.25.1