From 86ab13b72d6bcf45d1f663ecfb29d04320ed66f1 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 23 Feb 2017 17:56:17 +1300 Subject: [PATCH] Towards CRM-19815 make pseudoconstant handling more generic. Using metadata pseudoconstants we can remove joins on the option_value table, which hur performance. This patch only opens it up within the context of the Contact table. It is intended to open the code up to allow us to extend the performance advantage of dropping the bad join to other entities. The following fields have metadata links to the option_value table: gender_id prefix_id suffix_id preferred_communication_method communication_style_id preferred_language The general pattern for the api is to return (eg) communication_style_id = 1, communication_style = 'Formal', where communication_style is the db name of the option group for communication_style_id. preferred_communication_method is the exception. For api calls it returns an array of ids like all other api calls. However, it returns a string of names for non-api calls on that field, allowing us to avoid handling it in a number of other places. I added tests to ensure no change on the api inputs & outputs and searched these fields through search builder & advanced search + export. I also checked profile search listings. They are not using the convert routine & have some e-notices that pre-existed, but no regression I could see. --- CRM/Contact/BAO/Query.php | 149 +++++++++++++------- CRM/Contact/Selector.php | 14 -- CRM/Export/BAO/Export.php | 25 ---- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 2 +- tests/phpunit/api/v3/ContactTest.php | 34 +++++ 5 files changed, 136 insertions(+), 88 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index e4c08ed9b7..5874379f8a 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -698,15 +698,12 @@ class CRM_Contact_BAO_Query { } } - if (in_array($name, array('prefix_id', 'suffix_id', 'gender_id', 'communication_style_id'))) { - if (CRM_Utils_Array::value($field['pseudoconstant']['optionGroupName'], $this->_returnProperties)) { - $makeException = TRUE; - } - } - $cfID = CRM_Core_BAO_CustomField::getKeyID($name); - if (!empty($this->_paramLookup[$name]) || !empty($this->_returnProperties[$name]) || - $makeException + if ( + !empty($this->_paramLookup[$name]) + || !empty($this->_returnProperties[$name]) + || $this->pseudoConstantNameIsInReturnProperties($field) + || $makeException ) { if ($cfID) { // add to cfIDs array if not present @@ -826,42 +823,12 @@ class CRM_Contact_BAO_Query { $this->_element['provider_id'] = 1; } - if ($tName == 'contact') { + if ($tName == 'contact' && $fieldName == 'organization_name') { // special case, when current employer is set for Individual contact - if ($fieldName == 'organization_name') { - $this->_select[$name] = "IF ( contact_a.contact_type = 'Individual', NULL, contact_a.organization_name ) as organization_name"; - } - elseif ($fieldName != 'id') { - if ($fieldName == 'prefix_id') { - $this->_pseudoConstantsSelect['individual_prefix'] = array( - 'pseudoField' => 'prefix_id', - 'idCol' => "prefix_id", - 'bao' => 'CRM_Contact_BAO_Contact', - ); - } - if ($fieldName == 'suffix_id') { - $this->_pseudoConstantsSelect['individual_suffix'] = array( - 'pseudoField' => 'suffix_id', - 'idCol' => "suffix_id", - 'bao' => 'CRM_Contact_BAO_Contact', - ); - } - if ($fieldName == 'gender_id') { - $this->_pseudoConstantsSelect['gender'] = array( - 'pseudoField' => 'gender_id', - 'idCol' => "gender_id", - 'bao' => 'CRM_Contact_BAO_Contact', - ); - } - if ($name == 'communication_style_id') { - $this->_pseudoConstantsSelect['communication_style'] = array( - 'pseudoField' => 'communication_style_id', - 'idCol' => "communication_style_id", - 'bao' => 'CRM_Contact_BAO_Contact', - ); - } - $this->_select[$name] = "contact_a.{$fieldName} as `$name`"; - } + $this->_select[$name] = "IF ( contact_a.contact_type = 'Individual', NULL, contact_a.organization_name ) as organization_name"; + } + elseif ($tName == 'contact' && $fieldName === 'id') { + // Handled elsewhere, explicitly ignore. Possibly for all tables... } elseif (in_array($tName, array('country', 'county'))) { $this->_pseudoConstantsSelect[$name]['select'] = "{$field['where']} as `$name`"; @@ -877,7 +844,22 @@ class CRM_Contact_BAO_Query { } } else { - $this->_select[$name] = "{$field['where']} as `$name`"; + // If we have an option group defined then rather than joining the option value table in + // (which is an unindexed join) we render the option value on output. + // @todo - extend this to other pseudoconstants. + if ($this->pseudoConstantNameIsInReturnProperties($field)) { + $pseudoFieldName = $field['pseudoconstant']['optionGroupName']; + $this->_pseudoConstantsSelect[$pseudoFieldName] = array( + 'pseudoField' => $field['name'], + 'idCol' => $field['name'], + 'field_name' => $field['name'], + 'bao' => $field['bao'], + 'pseudoconstant' => $field['pseudoconstant'], + ); + $this->_tables[$tableName] = 1; + $this->_element[$pseudoFieldName] = 1; + } + $this->_select[$name] = str_replace('civicrm_contact.', 'contact_a.', "{$field['where']} as `$name`"); } if (!in_array($tName, array('state_province', 'country', 'county'))) { $this->_element[$name] = 1; @@ -4400,7 +4382,7 @@ civicrm_relationship.is_permission_a_b = 0 return array($noRows, NULL); } $val = $query->store($dao); - $convertedVals = $query->convertToPseudoNames($dao, TRUE); + $convertedVals = $query->convertToPseudoNames($dao, TRUE, TRUE); if (!empty($convertedVals)) { $val = array_replace_recursive($val, $convertedVals); @@ -5829,12 +5811,40 @@ AND displayRelType.is_active = 1 $val = $dao->{$value['idCol']}; if ($key == 'groups') { $dao->groups = $this->convertGroupIDStringToLabelString($dao, $val); - return; + continue; } if (CRM_Utils_System::isNull($val)) { $dao->$key = NULL; } + elseif (!empty($value['pseudoconstant'])) { + // If pseudoconstant is set that is kind of defacto for 'we have a bit more info about this' + // and we can use the metadata to figure it out. + // ideally this bit of IF will absorb & replace all the rest in time as we move to + // more metadata based choices. + if (strpos($val, CRM_Core_DAO::VALUE_SEPARATOR) !== FALSE) { + $dbValues = explode(CRM_Core_DAO::VALUE_SEPARATOR, trim($val, CRM_Core_DAO::VALUE_SEPARATOR)); + foreach ($dbValues as $pseudoValue) { + $convertedValues[] = CRM_Core_PseudoConstant::getLabel($value['bao'], $value['idCol'], $pseudoValue); + } + + $dao->$key = ($usedForAPI) ? $convertedValues : implode(', ', $convertedValues); + $realFieldName = CRM_Utils_Array::value('field_name', $this->_pseudoConstantsSelect[$key]); + if ($usedForAPI && $realFieldName) { + // normally we would see 2 fields returned for pseudoConstants. An exception is + // preferred_communication_method where there is no id-variant. + // For the api we prioritise getting the real data returned. + // over the resolved version + $dao->$realFieldName = $dbValues; + } + + } + else { + // This is basically the same as the default but since we have the bao we can use + // a cached function. + $dao->$key = CRM_Core_PseudoConstant::getLabel($value['bao'], $value['idCol'], $val); + } + } elseif ($baoName = CRM_Utils_Array::value('bao', $value, NULL)) { //preserve id value $idColumn = "{$key}_id"; @@ -5850,8 +5860,7 @@ AND displayRelType.is_active = 1 elseif ($value['pseudoField'] == 'state_province_abbreviation') { $dao->$key = CRM_Core_PseudoConstant::stateProvinceAbbreviation($val); } - // FIX ME: we should potentially move this to component Query and write a wrapper function that - // handles pseudoconstant fixes for all component + // @todo handle this in the section above for pseudoconstants. elseif (in_array($value['pseudoField'], array('participant_role_id', 'participant_role'))) { // @todo define bao on this & merge into the above condition. $viewValues = explode(CRM_Core_DAO::VALUE_SEPARATOR, $val); @@ -5889,6 +5898,21 @@ AND displayRelType.is_active = 1 } } } + if (!$usedForAPI) { + foreach (array( + 'gender_id' => 'gender', + 'prefix_id' => 'individual_prefix', + 'suffix_id' => 'individual_suffix', + 'communication_style_id' => 'communication_style', + ) as $realField => $labelField) { + // This is a temporary routine for handling these fields while + // we figure out how to handled them based on metadata in + /// export and search builder. CRM-19815, CRM-19830. + if (isset($dao->$realField) && is_numeric($dao->$realField)) { + $dao->$realField = $dao->$labelField; + } + } + } return $values; } @@ -6273,4 +6297,33 @@ AND displayRelType.is_active = 1 )); } + /** + * Has the pseudoconstant of the field been requested. + * + * For example if the field is payment_instrument_id then it + * has been requested if either payment_instrument_id or payment_instrument + * have been requested. Payment_instrument is the option groun name field value. + * + * @param array $field + * + * @return bool + */ + private function pseudoConstantNameIsInReturnProperties($field) { + if (!isset($field['pseudoconstant']['optionGroupName'])) { + return FALSE; + } + if (empty($field['bao']) || $field['bao'] != 'CRM_Contact_BAO_Contact') { + // For now.... + return FALSE; + } + + if (CRM_Utils_Array::value($field['pseudoconstant']['optionGroupName'], $this->_returnProperties)) { + return TRUE; + } + if (CRM_Utils_Array::value($field['name'], $this->_returnProperties)) { + return TRUE; + } + return FALSE; + } + } diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php index 8a92ce1a09..8cb3ec84e6 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -637,8 +637,6 @@ class CRM_Contact_Selector extends CRM_Core_Selector_Base implements CRM_Core_Se $names = self::$_properties; } - $multipleSelectFields = array('preferred_communication_method' => 1); - $links = self::links($this->_context, $this->_contextMenu, $this->_key); //check explicitly added contact to a Smart Group. @@ -656,7 +654,6 @@ class CRM_Contact_Selector extends CRM_Core_Selector_Base implements CRM_Core_Se while ($result->fetch()) { $row = array(); $this->_query->convertToPseudoNames($result); - // the columns we are interested in foreach ($names as $property) { if ($property == 'status') { @@ -669,17 +666,6 @@ class CRM_Contact_Selector extends CRM_Core_Selector_Base implements CRM_Core_Se $result->contact_id ); } - elseif ( - $multipleSelectFields && - array_key_exists($property, $multipleSelectFields) - ) { - $key = $property; - $paramsNew = array($key => $result->$property); - $name = array($key => array('newName' => $key, 'groupName' => $key)); - - CRM_Core_OptionGroup::lookupValues($paramsNew, $name, FALSE); - $row[$key] = $paramsNew[$key]; - } elseif (strpos($property, '-im')) { $row[$property] = $result->$property; if (!empty($result->$property)) { diff --git a/CRM/Export/BAO/Export.php b/CRM/Export/BAO/Export.php index cb0c0c762c..22fa793331 100644 --- a/CRM/Export/BAO/Export.php +++ b/CRM/Export/BAO/Export.php @@ -721,8 +721,6 @@ INSERT INTO {$componentTable} SELECT distinct gc.contact_id FROM civicrm_group_c } } - $multipleSelectFields = array('preferred_communication_method' => 1); - $addPaymentHeader = FALSE; $paymentDetails = array(); @@ -790,7 +788,6 @@ INSERT INTO {$componentTable} SELECT distinct gc.contact_id FROM civicrm_group_c $query->convertToPseudoNames($iterationDAO); //first loop through output columns so that we return what is required, and in same order. - $relationshipField = 0; foreach ($outputColumns as $field => $value) { // add im_provider to $dao object @@ -868,12 +865,6 @@ INSERT INTO {$componentTable} SELECT distinct gc.contact_id FROM civicrm_group_c } $field = $field . '_'; - if (array_key_exists($relationField, $multipleSelectFields)) { - $param = array($relationField => $fieldValue); - $names = array($relationField => array('newName' => $relationField, 'groupName' => $relationField)); - CRM_Core_OptionGroup::lookupValues($param, $names, FALSE); - $fieldValue = $param[$relationField]; - } if (is_object($relDAO) && $relationField == 'id') { $row[$field . $relationField] = $relDAO->contact_id; } @@ -949,22 +940,6 @@ INSERT INTO {$componentTable} SELECT distinct gc.contact_id FROM civicrm_group_c if ($cfID = CRM_Core_BAO_CustomField::getKeyID($field)) { $row[$field] = CRM_Core_BAO_CustomField::displayValue($fieldValue, $cfID); } - elseif (array_key_exists($field, $multipleSelectFields)) { - //option group fixes - $paramsNew = array($field => $fieldValue); - if ($field == 'test_tutoring') { - $name = array($field => array('newName' => $field, 'groupName' => 'test')); - // for readers group - } - elseif (substr($field, 0, 4) == 'cmr_') { - $name = array($field => array('newName' => $field, 'groupName' => substr($field, 0, -3))); - } - else { - $name = array($field => array('newName' => $field, 'groupName' => $field)); - } - CRM_Core_OptionGroup::lookupValues($paramsNew, $name, FALSE); - $row[$field] = $paramsNew[$field]; - } elseif (in_array($field, array( 'email_greeting', diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 8111339be4..8ed611f1cb 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -178,7 +178,7 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { 'contact_sub_type' => 1, 'sort_name' => 1, ); - $expectedSQL = "SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, civicrm_address.city as `city` FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) WHERE ( ( LOWER(civicrm_address.city) = 'cool city' ) ) AND (contact_a.is_deleted = 0) ORDER BY `contact_a`.`sort_name` asc, `contact_a`.`id` "; + $expectedSQL = "SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, civicrm_address.city as `city` FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) WHERE ( ( LOWER(civicrm_address.city) = 'cool city' ) ) AND (contact_a.is_deleted = 0) ORDER BY `contact_a`.`sort_name` asc, `contact_a`.`id` "; $queryObj = new CRM_Contact_BAO_Query($params, $returnProperties); try { $this->assertEquals($expectedSQL, $queryObj->searchQuery(0, 0, NULL, diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 15e3be87db..8e9c020395 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -1646,6 +1646,40 @@ class api_v3_ContactTest extends CiviUnitTestCase { $this->callAPISuccess('contact', 'delete', $contact); } + /** + * Ensure consistent return format for option group fields. + */ + public function testPseudoFields() { + $params = array( + 'preferred_communication_method' => array('Phone', 'SMS'), + 'preferred_language' => 'en_US', + 'gender_id' => 'Female', + 'prefix_id' => 'Mrs.', + 'suffix_id' => 'II', + 'communication_style_id' => 'Formal', + ); + + $contact = $this->callAPISuccess('contact', 'create', array_merge($this->_params, $params)); + + $result = $this->callAPISuccess('contact', 'getsingle', array('id' => $contact['id'])); + $this->assertEquals('Both', $result['preferred_mail_format']); + + $this->assertEquals('en_US', $result['preferred_language']); + $this->assertEquals(1, $result['communication_style_id']); + $this->assertEquals(1, $result['gender_id']); + $this->assertEquals('Female', $result['gender']); + $this->assertEquals('Mrs.', $result['individual_prefix']); + $this->assertEquals(1, $result['prefix_id']); + $this->assertEquals('II', $result['individual_suffix']); + $this->assertEquals(CRM_Core_PseudoConstant::getKey("CRM_Contact_BAO_Contact", 'suffix_id', 'II'), $result['suffix_id']); + $this->callAPISuccess('contact', 'delete', $contact); + $this->assertEquals(array( + CRM_Core_PseudoConstant::getKey("CRM_Contact_BAO_Contact", 'preferred_communication_method', 'Phone'), + CRM_Core_PseudoConstant::getKey("CRM_Contact_BAO_Contact", 'preferred_communication_method', 'SMS'), + ), $result['preferred_communication_method']); + } + + /** * Test birth date parameters. * -- 2.25.1