From 67fea9b44a55b4e9a5b2221be2a7b6423c71fba3 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 29 Apr 2021 11:32:39 -0400 Subject: [PATCH] APIv4 - Setting api misc fixes & tests Fixes the setting api to correctly handle pseudoconstants & field suffixes --- CRM/Core/BAO/Setting.php | 6 ++- CRM/Core/SelectValues.php | 2 +- .../Action/Setting/AbstractSettingAction.php | 46 ++++++++++++++-- Civi/Api4/Action/Setting/Get.php | 20 ++++--- Civi/Api4/Action/Setting/GetFields.php | 32 ++++++------ Civi/Core/SettingsMetadata.php | 45 ++++++++++++---- api/api.php | 2 +- tests/phpunit/api/v3/SettingTest.php | 10 ++-- tests/phpunit/api/v4/Entity/SettingTest.php | 52 +++++++++++++++++-- 9 files changed, 165 insertions(+), 50 deletions(-) diff --git a/CRM/Core/BAO/Setting.php b/CRM/Core/BAO/Setting.php index 02ec91a4ac..f075a24957 100644 --- a/CRM/Core/BAO/Setting.php +++ b/CRM/Core/BAO/Setting.php @@ -272,13 +272,15 @@ class CRM_Core_BAO_Setting extends CRM_Core_DAO_Setting { * value of the setting to be set * @param array $fieldSpec * Metadata for given field (drawn from the xml) + * @param bool $convertToSerializedString + * Deprecated mode * * @return bool * @throws \API_Exception */ - public static function validateSetting(&$value, array $fieldSpec) { + public static function validateSetting(&$value, array $fieldSpec, $convertToSerializedString = TRUE) { // Deprecated guesswork - should use $fieldSpec['serialize'] - if ($fieldSpec['type'] == 'String' && is_array($value)) { + if ($convertToSerializedString && $fieldSpec['type'] == 'String' && is_array($value)) { $value = CRM_Core_DAO::VALUE_SEPARATOR . implode(CRM_Core_DAO::VALUE_SEPARATOR, $value) . CRM_Core_DAO::VALUE_SEPARATOR; } if (empty($fieldSpec['validate_callback'])) { diff --git a/CRM/Core/SelectValues.php b/CRM/Core/SelectValues.php index 4ebec39596..69866466fb 100644 --- a/CRM/Core/SelectValues.php +++ b/CRM/Core/SelectValues.php @@ -437,7 +437,7 @@ class CRM_Core_SelectValues { * * @throws \CRM_Core_Exception */ - public function taxDisplayOptions() { + public static function taxDisplayOptions() { return [ 'Do_not_show' => ts('Do not show breakdown, only show total - i.e %1', [ 1 => CRM_Utils_Money::format(120), diff --git a/Civi/Api4/Action/Setting/AbstractSettingAction.php b/Civi/Api4/Action/Setting/AbstractSettingAction.php index abbd07ae9a..3f362dc404 100644 --- a/Civi/Api4/Action/Setting/AbstractSettingAction.php +++ b/Civi/Api4/Action/Setting/AbstractSettingAction.php @@ -58,14 +58,24 @@ abstract class AbstractSettingAction extends \Civi\Api4\Generic\AbstractAction { */ protected function validateSettings($domain) { $meta = \Civi\Core\SettingsMetadata::getMetadata([], $domain); - $names = isset($this->values) ? array_keys($this->values) : $this->select; + $names = array_map(function($name) { + return explode(':', $name)[0]; + }, isset($this->values) ? array_keys($this->values) : $this->select); $invalid = array_diff($names, array_keys($meta)); if ($invalid) { throw new \API_Exception("Unknown settings for domain $domain: " . implode(', ', $invalid)); } if (isset($this->values)) { - foreach ($this->values as $name => &$value) { - \CRM_Core_BAO_Setting::validateSetting($value, $meta[$name]); + foreach ($this->values as $name => $value) { + [$name, $suffix] = array_pad(explode(':', $name), 2, NULL); + // Replace pseudoconstants in values array + if ($suffix) { + $value = $this->matchPseudoconstant($name, $value, $suffix, 'id', $domain); + unset($this->values["$name:$suffix"]); + $this->values[$name] = $value; + } + \CRM_Core_BAO_Setting::validateSetting($this->values[$name], $meta[$name], FALSE); + } } return $meta; @@ -88,4 +98,34 @@ abstract class AbstractSettingAction extends \Civi\Api4\Generic\AbstractAction { } } + /** + * @param string $name + * @param mixed $value + * @param string $from + * @param string $to + * @param int $domain + * @return mixed + */ + protected function matchPseudoconstant(string $name, $value, $from, $to, $domain) { + if ($value === NULL) { + return NULL; + } + if ($from === $to) { + return $value; + } + $meta = \Civi\Core\SettingsMetadata::getMetadata(['name' => [$name]], $domain, [$from, $to]); + $options = $meta[$name]['options'] ?? []; + $map = array_column($options, $to, $from); + $translated = []; + foreach ((array) $value as $key) { + if (isset($map[$key])) { + $translated[] = $map[$key]; + } + } + if (!is_array($value)) { + return \CRM_Utils_Array::first($translated); + } + return $translated; + } + } diff --git a/Civi/Api4/Action/Setting/Get.php b/Civi/Api4/Action/Setting/Get.php index 6ce7b39b8e..e048c79561 100644 --- a/Civi/Api4/Action/Setting/Get.php +++ b/Civi/Api4/Action/Setting/Get.php @@ -39,15 +39,26 @@ class Get extends AbstractSettingAction { protected function processSettings(Result $result, $settingsBag, $meta, $domain) { if ($this->select) { foreach ($this->select as $name) { + [$name, $suffix] = array_pad(explode(':', $name), 2, NULL); + $value = $settingsBag->get($name); + if (isset($value) && !empty($meta[$name]['serialize'])) { + $value = \CRM_Core_DAO::unSerializeField($value, $meta[$name]['serialize']); + } + if ($suffix) { + $value = $this->matchPseudoconstant($name, $value, 'id', $suffix, $domain); + } $result[] = [ - 'name' => $name, - 'value' => $settingsBag->get($name), + 'name' => $suffix ? "$name:$suffix" : $name, + 'value' => $value, 'domain_id' => $domain, ]; } } else { foreach ($settingsBag->all() as $name => $value) { + if (isset($value) && !empty($meta[$name]['serialize'])) { + $value = \CRM_Core_DAO::unSerializeField($value, $meta[$name]['serialize']); + } $result[] = [ 'name' => $name, 'value' => $value, @@ -55,11 +66,6 @@ class Get extends AbstractSettingAction { ]; } } - foreach ($result as &$setting) { - if (isset($setting['value']) && !empty($meta[$setting['name']]['serialize'])) { - $setting['value'] = \CRM_Core_DAO::unSerializeField($setting['value'], $meta[$setting['name']]['serialize']); - } - } } /** diff --git a/Civi/Api4/Action/Setting/GetFields.php b/Civi/Api4/Action/Setting/GetFields.php index 44711ef4c8..bcd9a04821 100644 --- a/Civi/Api4/Action/Setting/GetFields.php +++ b/Civi/Api4/Action/Setting/GetFields.php @@ -28,11 +28,21 @@ class GetFields extends \Civi\Api4\Generic\BasicGetFieldsAction { protected $domainId; protected function getRecords() { - // TODO: Waiting for filter handling to get fixed in core - // $names = $this->_itemsToGet('name'); - // $filter = $names ? ['name' => $names] : []; - $filter = []; - return \Civi\Core\SettingsMetadata::getMetadata($filter, $this->domainId, $this->loadOptions); + $names = $this->_itemsToGet('name'); + $filter = $names ? ['name' => $names] : []; + $settings = \Civi\Core\SettingsMetadata::getMetadata($filter, $this->domainId, $this->loadOptions); + foreach ($settings as $index => $setting) { + // Unserialize default value + if (!empty($setting['serialize']) && !empty($setting['default']) && is_string($setting['default'])) { + $setting['default'] = \CRM_Core_DAO::unSerializeField($setting['default'], $setting['serialize']); + } + if (!isset($setting['options'])) { + $setting['options'] = !empty($setting['pseudoconstant']); + } + // Filter out deprecated properties + $settings[$index] = array_intersect_key($setting, array_column($this->fields(), NULL, 'name')); + } + return $settings; } public function fields() { @@ -57,22 +67,10 @@ class GetFields extends \Civi\Api4\Generic\BasicGetFieldsAction { 'name' => 'default', 'data_type' => 'String', ], - [ - 'name' => 'pseudoconstant', - 'data_type' => 'String', - ], [ 'name' => 'options', 'data_type' => 'Array', ], - [ - 'name' => 'group_name', - 'data_type' => 'String', - ], - [ - 'name' => 'group', - 'data_type' => 'String', - ], [ 'name' => 'html_type', 'data_type' => 'String', diff --git a/Civi/Core/SettingsMetadata.php b/Civi/Core/SettingsMetadata.php index fb5c05aed8..c91e5b616b 100644 --- a/Civi/Core/SettingsMetadata.php +++ b/Civi/Core/SettingsMetadata.php @@ -35,7 +35,7 @@ class SettingsMetadata { * * @param array $filters * @param int $domainID - * @param bool $loadOptions + * @param bool|array $loadOptions * * @return array * the following information as appropriate for each setting @@ -70,7 +70,7 @@ class SettingsMetadata { self::_filterSettingsSpecification($filters, $settingsMetadata); if ($loadOptions) { - self::loadOptions($settingsMetadata); + self::fillOptions($settingsMetadata, $loadOptions); } return $settingsMetadata; @@ -98,7 +98,7 @@ class SettingsMetadata { /** * Load up settings metadata from files. * - * @param array $metaDataFolder + * @param string $metaDataFolder * * @return array */ @@ -141,22 +141,35 @@ class SettingsMetadata { * Retrieve options from settings metadata * * @param array $settingSpec + * @param bool|array $optionsFormat + * TRUE for a flat array; otherwise an array of keys to return */ - protected static function loadOptions(&$settingSpec) { + protected static function fillOptions(&$settingSpec, $optionsFormat) { foreach ($settingSpec as &$spec) { if (empty($spec['pseudoconstant'])) { continue; } $pseudoconstant = $spec['pseudoconstant']; + $spec['options'] = []; // It would be nice if we could leverage CRM_Core_PseudoConstant::get() somehow, // but it's tightly coupled to DAO/field. However, if you really need to support // more pseudoconstant types, then probably best to refactor it. For now, KISS. - if (!empty($pseudoconstant['callback'])) { - $spec['options'] = Resolver::singleton()->call($pseudoconstant['callback'], []); - } - elseif (!empty($pseudoconstant['optionGroupName'])) { + if (!empty($pseudoconstant['optionGroupName'])) { $keyColumn = \CRM_Utils_Array::value('keyColumn', $pseudoconstant, 'value'); - $spec['options'] = \CRM_Core_OptionGroup::values($pseudoconstant['optionGroupName'], FALSE, FALSE, TRUE, NULL, 'label', TRUE, FALSE, $keyColumn); + if (is_array($optionsFormat)) { + $optionValues = \CRM_Core_OptionValue::getValues(['name' => $pseudoconstant['optionGroupName']]); + foreach ($optionValues as $option) { + $option['id'] = $option['value']; + $spec['options'][] = $option; + } + } + else { + $spec['options'] = \CRM_Core_OptionGroup::values($pseudoconstant['optionGroupName'], FALSE, FALSE, TRUE, NULL, 'label', TRUE, FALSE, $keyColumn); + } + continue; + } + if (!empty($pseudoconstant['callback'])) { + $options = Resolver::singleton()->call($pseudoconstant['callback'], []); } if (!empty($pseudoconstant['table'])) { $params = [ @@ -164,7 +177,19 @@ class SettingsMetadata { 'keyColumn' => $pseudoconstant['keyColumn'] ?? NULL, 'labelColumn' => $pseudoconstant['labelColumn'] ?? NULL, ]; - $spec['options'] = \CRM_Core_PseudoConstant::renderOptionsFromTablePseudoconstant($pseudoconstant, $params, ($spec['localize_context'] ?? NULL), 'get'); + $options = \CRM_Core_PseudoConstant::renderOptionsFromTablePseudoconstant($pseudoconstant, $params, ($spec['localize_context'] ?? NULL), 'get'); + } + if (is_array($optionsFormat)) { + foreach ($options as $key => $value) { + $spec['options'][] = [ + 'id' => $key, + 'name' => $value, + 'label' => $value, + ]; + } + } + else { + $spec['options'] = $options; } } } diff --git a/api/api.php b/api/api.php index 7e45ab2ae6..9ed9747dd0 100644 --- a/api/api.php +++ b/api/api.php @@ -64,7 +64,7 @@ function civicrm_api4(string $entity, string $action, array $params = [], $index $removeIndexField = FALSE; // If index field is not part of the select query, we add it here and remove it below (except for oddball "Setting" api) - if ($indexField && !empty($params['select']) && is_array($params['select']) && $entity !== 'Setting' && !\Civi\Api4\Utils\SelectUtil::isFieldSelected($indexField, $params['select'])) { + if ($indexField && !empty($params['select']) && is_array($params['select']) && !($entity === 'Setting' && $action === 'get') && !\Civi\Api4\Utils\SelectUtil::isFieldSelected($indexField, $params['select'])) { $params['select'][] = $indexField; $removeIndexField = TRUE; } diff --git a/tests/phpunit/api/v3/SettingTest.php b/tests/phpunit/api/v3/SettingTest.php index 1bc407ee82..36474d33d0 100644 --- a/tests/phpunit/api/v3/SettingTest.php +++ b/tests/phpunit/api/v3/SettingTest.php @@ -111,11 +111,9 @@ class api_v3_SettingTest extends CiviUnitTestCase { /** * Test that getfields will filter on group. - * @param int $version - * @dataProvider versionThreeAndFour */ - public function testGetFieldsGroupFilters($version) { - $this->_apiversion = $version; + public function testGetFieldsGroupFilters() { + $this->_apiversion = 3; $params = ['filters' => ['group' => 'multisite']]; $result = $this->callAPISuccess('setting', 'getfields', $params); $this->assertArrayNotHasKey('customCSSURL', $result['values']); @@ -363,11 +361,11 @@ class api_v3_SettingTest extends CiviUnitTestCase { $this->hookClass->setHook('civicrm_alterSettingsFolders', [$this, 'setExtensionMetadata']); $data = NULL; Civi::cache('settings')->flush(); - $fields = $this->callAPISuccess('setting', 'getfields', ['filters' => ['group_name' => 'Test Settings']]); + $fields = $this->callAPISuccess('setting', 'getfields'); $this->assertArrayHasKey('test_key', $fields['values']); $this->callAPISuccess('setting', 'create', ['test_key' => 'keyset']); $this->assertEquals('keyset', Civi::settings()->get('test_key')); - $result = $this->callAPISuccess('setting', 'getvalue', ['name' => 'test_key', 'group' => 'Test Settings']); + $result = $this->callAPISuccess('setting', 'getvalue', ['name' => 'test_key']); $this->assertEquals('keyset', $result); } diff --git a/tests/phpunit/api/v4/Entity/SettingTest.php b/tests/phpunit/api/v4/Entity/SettingTest.php index a50c00605c..9db05160a8 100644 --- a/tests/phpunit/api/v4/Entity/SettingTest.php +++ b/tests/phpunit/api/v4/Entity/SettingTest.php @@ -51,10 +51,24 @@ class SettingTest extends UnitTestCase { } public function testSerailizedSetting() { - $settings = \Civi\Api4\Setting::get(FALSE) - ->addSelect('contact_edit_options') + $set = \Civi\Api4\Setting::set(FALSE) + ->addValue('contact_edit_options:name', [ + 'CommunicationPreferences', + 'CustomData', + ]) ->execute(); - $this->assertTrue(is_array($settings[0]['value'])); + + $setting = \Civi\Api4\Setting::get(FALSE) + ->addSelect('contact_edit_options') + ->execute()->first(); + $this->assertTrue(is_array($setting['value'])); + $this->assertTrue(is_numeric($setting['value'][0])); + + $setting = \Civi\Api4\Setting::get(FALSE) + ->addSelect('contact_edit_options:label') + ->execute()->first(); + $this->assertEquals(['Communication Preferences', 'Custom Data'], $setting['value']); + $this->assertEquals('contact_edit_options:label', $setting['name']); } /** @@ -78,4 +92,36 @@ class SettingTest extends UnitTestCase { $this->assertTrue($arrayValues); } + /** + * Make sure options load from getFields. + */ + public function testSettingGetFieldsOptions() { + $setting = civicrm_api4('Setting', 'getFields', [ + 'select' => ['options'], + 'loadOptions' => FALSE, + ], 'name'); + $this->assertTrue($setting['contact_edit_options']['options']); + + $setting = civicrm_api4('Setting', 'getFields', [ + 'select' => ['options'], + 'where' => [['name', '=', 'contact_edit_options']], + 'loadOptions' => TRUE, + ], 'name'); + $this->assertContains('Custom Data', $setting['contact_edit_options']['options']); + + $setting = civicrm_api4('Setting', 'getFields', [ + 'select' => ['options'], + 'loadOptions' => ['id', 'name', 'label'], + ], 'name'); + $this->assertTrue(is_array($setting['contact_edit_options']['options'][0])); + } + + /** + * Ensure settings default values unserialize. + */ + public function testSettingUnserializeDefaults() { + $setting = civicrm_api4('Setting', 'getFields', ['where' => [['name', '=', 'contact_view_options']]], 0); + $this->assertTrue(is_array($setting['default'])); + } + } -- 2.25.1