From a7bcb32d4b9eda2a5f2358c423c5c17abdbfe7b1 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Sat, 5 Sep 2020 15:22:32 -0400 Subject: [PATCH] Remove redundant custom field types The html_type 'Select Country' and 'Select State/Province' serve no purpose. They are the same as Select, just with a different data_type. This adds an APIv3 shim to display the old types. --- CRM/Core/BAO/CustomField.php | 7 +- CRM/Core/SelectValues.php | 2 - CRM/Custom/Form/Field.php | 8 +- .../Page/MultipleRecordFieldsListing.php | 4 +- .../Incremental/sql/5.31.alpha1.mysql.tpl | 4 + Civi/Api4/Service/Spec/SpecFormatter.php | 2 - api/v3/CustomField.php | 34 ++++++-- .../phpunit/CRM/Core/BAO/CustomFieldTest.php | 19 +++-- .../CRM/Core/BAO/CustomValueTableTest.php | 2 +- tests/phpunit/api/v3/CustomFieldTest.php | 83 +++++++++++++++++++ 10 files changed, 131 insertions(+), 34 deletions(-) diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php index 66871a7d5d..107242a515 100644 --- a/CRM/Core/BAO/CustomField.php +++ b/CRM/Core/BAO/CustomField.php @@ -250,7 +250,9 @@ class CRM_Core_BAO_CustomField extends CRM_Core_DAO_CustomField { // This provides legacy support for APIv3, allowing no-longer-existent html types if ($fieldName == 'html_type' && isset($props['version']) && $props['version'] == 3) { $options['Multi-Select'] = 'Multi-Select'; + $options['Select Country'] = 'Select Country'; $options['Multi-Select Country'] = 'Multi-Select Country'; + $options['Select State/Province'] = 'Select State/Province'; $options['Multi-Select State/Province'] = 'Multi-Select State/Province'; } return $options; @@ -678,11 +680,8 @@ class CRM_Core_BAO_CustomField extends CRM_Core_DAO_CustomField { $placeholder = $search ? ts('- any -') : ($useRequired ? ts('- select -') : ts('- none -')); - // FIXME: Why are select state/country separate widget types? $isSelect = (in_array($widget, [ 'Select', - 'Select State/Province', - 'Select Country', 'CheckBox', 'Radio', ])); @@ -1057,8 +1056,6 @@ class CRM_Core_BAO_CustomField extends CRM_Core_DAO_CustomField { case 'Select': case 'Autocomplete-Select': case 'Radio': - case 'Select Country': - case 'Select State/Province': case 'CheckBox': if ($field['data_type'] == 'ContactReference' && $value) { if (is_numeric($value)) { diff --git a/CRM/Core/SelectValues.php b/CRM/Core/SelectValues.php index f7d22c826a..7adcd178c5 100644 --- a/CRM/Core/SelectValues.php +++ b/CRM/Core/SelectValues.php @@ -174,8 +174,6 @@ class CRM_Core_SelectValues { 'CheckBox' => ts('Checkbox(es)'), 'Select Date' => ts('Select Date'), 'File' => ts('File'), - 'Select State/Province' => ts('Select State/Province'), - 'Select Country' => ts('Select Country'), 'RichTextEditor' => ts('Rich Text Editor'), 'Autocomplete-Select' => ts('Autocomplete-Select'), 'Link' => ts('Link'), diff --git a/CRM/Custom/Form/Field.php b/CRM/Custom/Form/Field.php index f5e6096c0e..840650e7d9 100644 --- a/CRM/Custom/Form/Field.php +++ b/CRM/Custom/Form/Field.php @@ -82,8 +82,8 @@ class CRM_Custom_Form_Field extends CRM_Core_Form { ['TextArea' => 'TextArea', 'RichTextEditor' => 'RichTextEditor'], ['Date' => 'Select Date'], ['Radio' => 'Radio'], - ['StateProvince' => 'Select State/Province'], - ['Country' => 'Select Country'], + ['StateProvince' => 'Select'], + ['Country' => 'Select'], ['File' => 'File'], ['Link' => 'Link'], ['ContactReference' => 'Autocomplete-Select'], @@ -158,8 +158,8 @@ class CRM_Custom_Form_Field extends CRM_Core_Form { ['TextArea' => ts('TextArea'), 'RichTextEditor' => ts('Rich Text Editor')], ['Date' => ts('Select Date')], ['Radio' => ts('Radio')], - ['StateProvince' => ts('Select State/Province')], - ['Country' => ts('Select Country')], + ['StateProvince' => ts('Select')], + ['Country' => ts('Select')], ['File' => ts('Select File')], ['Link' => ts('Link')], ['ContactReference' => ts('Autocomplete-Select')], diff --git a/CRM/Profile/Page/MultipleRecordFieldsListing.php b/CRM/Profile/Page/MultipleRecordFieldsListing.php index 1311d85c3e..a6c92afc62 100644 --- a/CRM/Profile/Page/MultipleRecordFieldsListing.php +++ b/CRM/Profile/Page/MultipleRecordFieldsListing.php @@ -341,8 +341,6 @@ class CRM_Profile_Page_MultipleRecordFieldsListing extends CRM_Core_Page_Basic { case 'Radio': case 'Select': - case 'Select Country': - case 'Select State/Province': $editable = TRUE; $fieldAttributes['data-type'] = $spec['data_type'] == 'Boolean' ? 'boolean' : 'select'; if (!$spec['is_required']) { @@ -408,7 +406,7 @@ class CRM_Profile_Page_MultipleRecordFieldsListing extends CRM_Core_Page_Basic { $headers = []; if (!empty($fieldIDs)) { - $fields = ['Radio', 'Select', 'Select Country', 'Select State/Province']; + $fields = ['Radio', 'Select']; foreach ($fieldIDs as $fieldID) { if ($this->_pageViewType == 'profileDataView') { $headers[$fieldID] = $customGroupInfo[$fieldID]['fieldLabel']; diff --git a/CRM/Upgrade/Incremental/sql/5.31.alpha1.mysql.tpl b/CRM/Upgrade/Incremental/sql/5.31.alpha1.mysql.tpl index aaf47e53be..663ae49a26 100644 --- a/CRM/Upgrade/Incremental/sql/5.31.alpha1.mysql.tpl +++ b/CRM/Upgrade/Incremental/sql/5.31.alpha1.mysql.tpl @@ -1 +1,5 @@ {* file to handle db changes in 5.31.alpha1 during upgrade *} + +{* Remove Country & State special select fields *} +UPDATE civicrm_custom_field SET html_type = 'Select' +WHERE html_type IN ('Select Country', 'Select State/Province'); diff --git a/Civi/Api4/Service/Spec/SpecFormatter.php b/Civi/Api4/Service/Spec/SpecFormatter.php index 23209a4e24..9e00ba09f5 100644 --- a/Civi/Api4/Service/Spec/SpecFormatter.php +++ b/Civi/Api4/Service/Spec/SpecFormatter.php @@ -143,8 +143,6 @@ class SpecFormatter { unset($inputAttrs['type']); $map = [ - 'Select State/Province' => 'Select', - 'Select Country' => 'Select', 'Select Date' => 'Date', 'Link' => 'Url', ]; diff --git a/api/v3/CustomField.php b/api/v3/CustomField.php index a9b664b0b5..ecfe665fdd 100644 --- a/api/v3/CustomField.php +++ b/api/v3/CustomField.php @@ -31,9 +31,11 @@ function civicrm_api3_custom_field_create($params) { // Legacy handling for old way of naming serialized fields - if (!empty($params['html_type']) && ($params['html_type'] == 'CheckBox' || strpos($params['html_type'], 'Multi-') === 0)) { - $params['serialize'] = 1; - $params['html_type'] = str_replace('Multi-', '', $params['html_type']); + if (!empty($params['html_type'])) { + if ($params['html_type'] == 'CheckBox' || strpos($params['html_type'], 'Multi-') === 0) { + $params['serialize'] = 1; + } + $params['html_type'] = str_replace(['Multi-Select', 'Select Country', 'Select State/Province'], 'Select', $params['html_type']); } // Array created for passing options in params. @@ -131,7 +133,14 @@ function civicrm_api3_custom_field_get($params) { if (!in_array('serialize', $params['return'])) { $params['return'][] = 'serialize'; } + if (!in_array('data_type', $params['return'])) { + $params['return'][] = 'data_type'; + } } + $legacyDataTypes = [ + 'Select State/Province' => 'StateProvince', + 'Select Country' => 'Country', + ]; if ($handleLegacy && !empty($params['html_type'])) { $serializedTypes = ['CheckBox', 'Multi-Select', 'Multi-Select Country', 'Multi-Select State/Province']; if (is_string($params['html_type'])) { @@ -142,12 +151,16 @@ function civicrm_api3_custom_field_get($params) { elseif (!in_array($params['html_type'], $serializedTypes)) { $params['serialize'] = 0; } + if (isset($legacyDataTypes[$params['html_type']])) { + $params['data_type'] = $legacyDataTypes[$params['html_type']]; + unset($params['html_type']); + } } elseif (is_array($params['html_type']) && !empty($params['html_type']['IN'])) { $excludeNonSerialized = !array_diff($params['html_type']['IN'], $serializedTypes); $onlyNonSerialized = !array_intersect($params['html_type']['IN'], $serializedTypes); $params['html_type']['IN'] = array_map(function($val) { - return str_replace('Multi-Select', 'Select', $val); + return str_replace(['Multi-Select', 'Select Country', 'Select State/Province'], 'Select', $val); }, $params['html_type']['IN']); if ($excludeNonSerialized) { $params['serialize'] = 1; @@ -160,10 +173,15 @@ function civicrm_api3_custom_field_get($params) { $results = _civicrm_api3_basic_get(_civicrm_api3_get_BAO(__FUNCTION__), $params); - if ($handleLegacy && !empty($results['values']) && is_array($results['values']) && !isset($params['serialize'])) { - foreach ($results['values'] as $id => $result) { - if (!empty($result['serialize']) && !empty($result['html_type'])) { - $results['values'][$id]['html_type'] = str_replace('Select', 'Multi-Select', $result['html_type']); + if ($handleLegacy && !empty($results['values']) && is_array($results['values'])) { + foreach ($results['values'] as $id => &$result) { + if (!empty($result['html_type'])) { + if (in_array($result['data_type'], $legacyDataTypes)) { + $result['html_type'] = array_search($result['data_type'], $legacyDataTypes); + } + if (!empty($result['serialize'])) { + $result['html_type'] = str_replace('Select', 'Multi-Select', $result['html_type']); + } } } } diff --git a/tests/phpunit/CRM/Core/BAO/CustomFieldTest.php b/tests/phpunit/CRM/Core/BAO/CustomFieldTest.php index bf3cdf38c8..6642d75c77 100644 --- a/tests/phpunit/CRM/Core/BAO/CustomFieldTest.php +++ b/tests/phpunit/CRM/Core/BAO/CustomFieldTest.php @@ -135,7 +135,7 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { $fieldsToCreate = [ [ 'data_type' => 'Country', - 'html_type' => 'Select Country', + 'html_type' => 'Select', 'tests' => [ 'United States' => 1228, '' => NULL, @@ -143,7 +143,8 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { ], [ 'data_type' => 'StateProvince', - 'html_type' => 'Multi-Select State/Province', + 'html_type' => 'Select', + 'serialize' => 1, 'tests' => [ '' => 0, 'Alabama' => 1000, @@ -296,21 +297,21 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { 'custom_group_id' => $groups['A']['id'], 'label' => 'Country A', 'dataType' => 'Country', - 'htmlType' => 'Select Country', + 'htmlType' => 'Select', 'default_value' => NULL, ]); $countryB = $this->customFieldCreate([ 'custom_group_id' => $groups['A']['id'], 'label' => 'Country B', 'dataType' => 'Country', - 'htmlType' => 'Select Country', + 'htmlType' => 'Select', 'default_value' => NULL, ]); $countryC = $this->customFieldCreate([ 'custom_group_id' => $groups['B']['id'], 'label' => 'Country C', 'dataType' => 'Country', - 'htmlType' => 'Select Country', + 'htmlType' => 'Select', 'default_value' => NULL, ]); @@ -460,7 +461,7 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { 'options_per_line' => NULL, 'text_length' => NULL, 'data_type' => 'Country', - 'html_type' => 'Select Country', + 'html_type' => 'Select', 'is_search_range' => '0', 'id' => $this->getCustomFieldID('country'), 'label' => 'Country', @@ -499,7 +500,7 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { 'options_per_line' => NULL, 'text_length' => NULL, 'data_type' => 'Country', - 'html_type' => 'Select Country', + 'html_type' => 'Select', 'is_search_range' => '0', 'id' => $this->getCustomFieldID('multi_country'), 'label' => 'Country-multi', @@ -797,7 +798,7 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { 'import' => 1, 'data_type' => 'StateProvince', 'type' => 1, - 'html_type' => 'Select State/Province', + 'html_type' => 'Select', 'text_length' => NULL, 'options_per_line' => NULL, 'is_search_range' => '0', @@ -835,7 +836,7 @@ class CRM_Core_BAO_CustomFieldTest extends CiviUnitTestCase { 'data_type' => 'StateProvince', 'name' => $this->getCustomFieldName('multi_state'), 'type' => 1, - 'html_type' => 'Select State/Province', + 'html_type' => 'Select', 'text_length' => NULL, 'options_per_line' => NULL, 'is_search_range' => '0', diff --git a/tests/phpunit/CRM/Core/BAO/CustomValueTableTest.php b/tests/phpunit/CRM/Core/BAO/CustomValueTableTest.php index 580df3134d..a4d51bdc0f 100644 --- a/tests/phpunit/CRM/Core/BAO/CustomValueTableTest.php +++ b/tests/phpunit/CRM/Core/BAO/CustomValueTableTest.php @@ -89,7 +89,7 @@ class CRM_Core_BAO_CustomValueTableTest extends CiviUnitTestCase { $fields = [ 'custom_group_id' => $customGroup['id'], 'data_type' => 'StateProvince', - 'html_type' => 'Select State/Province', + 'html_type' => 'Select', 'default_value' => '', ]; diff --git a/tests/phpunit/api/v3/CustomFieldTest.php b/tests/phpunit/api/v3/CustomFieldTest.php index 8fc16468a1..e5c900dd24 100644 --- a/tests/phpunit/api/v3/CustomFieldTest.php +++ b/tests/phpunit/api/v3/CustomFieldTest.php @@ -107,6 +107,16 @@ class api_v3_CustomFieldTest extends CiviUnitTestCase { $dtype = CRM_Core_BAO_CustomField::dataType(); $htype = CRM_Custom_Form_Field::$_dataToHTML; + // Legacy html types returned by v3 + foreach ($htype as &$item) { + if (isset($item['StateProvince'])) { + $item['StateProvince'] = 'Select State/Province'; + } + if (isset($item['Country'])) { + $item['Country'] = 'Select Country'; + } + } + $n = 0; foreach ($dtype as $dkey => $dvalue) { foreach ($htype[$n] as $hkey => $hvalue) { @@ -678,4 +688,77 @@ class api_v3_CustomFieldTest extends CiviUnitTestCase { $this->assertEquals('SingleSelect', $result['values'][0]['label']); } + public function testLegacyStateCountryTypes() { + $customGroup = $this->customGroupCreate([ + 'name' => 'testCustomGroup', + 'title' => 'testCustomGroup', + 'extends' => 'Individual', + ]); + $f1 = $this->callAPISuccess('CustomField', 'create', [ + 'label' => 'CountrySelect', + 'custom_group_id' => 'testCustomGroup', + 'data_type' => 'Country', + 'html_type' => 'Select Country', + ]); + $f2 = $this->callAPISuccess('CustomField', 'create', [ + 'label' => 'StateSelect', + 'custom_group_id' => 'testCustomGroup', + 'data_type' => 'StateProvince', + 'html_type' => 'Select State/Province', + ]); + $f3 = $this->callAPISuccess('CustomField', 'create', [ + 'label' => 'MultiSelectSP', + 'custom_group_id' => 'testCustomGroup', + 'data_type' => 'StateProvince', + 'html_type' => 'Multi-Select State/Province', + ]); + $f4 = $this->callAPISuccess('CustomField', 'create', [ + 'label' => 'MultiSelectCountry', + 'custom_group_id' => 'testCustomGroup', + 'data_type' => 'Country', + 'html_type' => 'Select Country', + 'serialize' => 1, + ]); + + $result = $this->callAPISuccess('CustomField', 'get', [ + 'custom_group_id' => 'testCustomGroup', + 'html_type' => 'Multi-Select State/Province', + 'sequential' => 1, + ]); + $this->assertCount(1, $result['values']); + $this->assertEquals('MultiSelectSP', $result['values'][0]['label']); + $this->assertEquals('Multi-Select State/Province', $result['values'][0]['html_type']); + $this->assertEquals('1', $result['values'][0]['serialize']); + + $result = $this->callAPISuccess('CustomField', 'get', [ + 'custom_group_id' => 'testCustomGroup', + 'html_type' => 'Multi-Select Country', + 'sequential' => 1, + ]); + $this->assertCount(1, $result['values']); + $this->assertEquals('MultiSelectCountry', $result['values'][0]['label']); + $this->assertEquals('Multi-Select Country', $result['values'][0]['html_type']); + $this->assertEquals('1', $result['values'][0]['serialize']); + + $result = $this->callAPISuccess('CustomField', 'get', [ + 'custom_group_id' => 'testCustomGroup', + 'html_type' => 'Select Country', + 'sequential' => 1, + ]); + $this->assertCount(1, $result['values']); + $this->assertEquals('CountrySelect', $result['values'][0]['label']); + $this->assertEquals('Select Country', $result['values'][0]['html_type']); + $this->assertEquals('0', $result['values'][0]['serialize']); + + $result = $this->callAPISuccess('CustomField', 'get', [ + 'custom_group_id' => 'testCustomGroup', + 'html_type' => 'Select State/Province', + 'sequential' => 1, + ]); + $this->assertCount(1, $result['values']); + $this->assertEquals('StateSelect', $result['values'][0]['label']); + $this->assertEquals('Select State/Province', $result['values'][0]['html_type']); + $this->assertEquals('0', $result['values'][0]['serialize']); + } + } -- 2.25.1