From 1ce347af618fd015bc1e5cbe76afe718b2a430f3 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Wed, 9 Nov 2022 12:41:34 +1300 Subject: [PATCH] Fix api handling of OptionValue defaults Overview ---------------------------------------- Fix api handling of OptionValue defaults Before ---------------------------------------- Some option values (greetings, from_email_address) have more than one default. For from_email_address it is per domain and for the others it is per 'filter' (aka contact_type). The special handling for greetings relies on the form layer - updating a greeting for 'Individual' and passing 'is_default' will unset 'is_default' for Household greetings. For from_email_address the form layer magic-param is set but ultimately has no bearing as there is duplicate handling in the bao layer After ---------------------------------------- The default handling is done in the BAO layer & removed from the form layer Technical Details ---------------------------------------- I moved the handling to after-update because values are more guarantee there. I did this by excluding the id from the update query (also prevents flappy updates). I didn't move all the way to the post hook because I wasn't sure about the interaction with cache flushing etc (that already happen after save in create) but that could be considered. --- CRM/Admin/Form/Options.php | 40 ++++++----- CRM/Core/BAO/OptionValue.php | 64 +++++++++-------- .../phpunit/CRM/Core/BAO/OptionValueTest.php | 72 +++++++++++++++++-- 3 files changed, 124 insertions(+), 52 deletions(-) diff --git a/CRM/Admin/Form/Options.php b/CRM/Admin/Form/Options.php index 23753bf351..5a36fc49ba 100644 --- a/CRM/Admin/Form/Options.php +++ b/CRM/Admin/Form/Options.php @@ -26,7 +26,7 @@ class CRM_Admin_Form_Options extends CRM_Admin_Form { /** * The option group name. * - * @var array + * @var string */ protected $_gName; @@ -139,6 +139,13 @@ class CRM_Admin_Form_Options extends CRM_Admin_Form { return $defaults; } + /** + * @return string + */ + public function getOptionGroupName() : string { + return $this->_gName; + } + /** * Build the form object. * @@ -466,24 +473,14 @@ class CRM_Admin_Form_Options extends CRM_Admin_Form { } else { $params = $this->exportValues(); - - // allow multiple defaults within group. - $allowMultiDefaults = ['email_greeting', 'postal_greeting', 'addressee', 'from_email_address']; - if (in_array($this->_gName, $allowMultiDefaults)) { - if ($this->_gName == 'from_email_address') { - $params['reset_default_for'] = ['domain_id' => CRM_Core_Config::domainID()]; - } - elseif ($filter = CRM_Utils_Array::value('contact_type_id', $params)) { - $params['filter'] = $filter; - $params['reset_default_for'] = ['filter' => "0, " . $params['filter']]; - } - - //make sure we only have a single space, CRM-6977 and dev/mail/15 - if ($this->_gName == 'from_email_address') { - $params['label'] = $this->sanitizeFromEmailAddress($params['label']); - } + if ($this->isGreetingOptionGroup()) { + $params['filter'] = $params['contact_type_id']; } + //make sure we only have a single space, CRM-6977 and dev/mail/15 + if ($this->_gName == 'from_email_address') { + $params['label'] = $this->sanitizeFromEmailAddress($params['label']); + } // set value of filter if not present in params if ($this->_id && !array_key_exists('filter', $params)) { if ($this->_gName == 'participant_role') { @@ -514,4 +511,13 @@ class CRM_Admin_Form_Options extends CRM_Admin_Form { return "\"{$parts[1]}\" <$parts[2]>"; } + /** + * Is the option group one of our greetings. + * + * @return bool + */ + protected function isGreetingOptionGroup(): bool { + return in_array($this->getOptionGroupName(), ['email_greeting', 'postal_greeting', 'addressee'], TRUE); + } + } diff --git a/CRM/Core/BAO/OptionValue.php b/CRM/Core/BAO/OptionValue.php index f7abe94b3a..a5977d8c7d 100644 --- a/CRM/Core/BAO/OptionValue.php +++ b/CRM/Core/BAO/OptionValue.php @@ -167,36 +167,6 @@ class CRM_Core_BAO_OptionValue extends CRM_Core_DAO_OptionValue { $optionValue->domain_id = CRM_Core_Config::domainID(); } - // When setting a default option, unset other options in this group as default - // FIXME: The extra CRM_Utils_System::isNull is because the API will pass the string 'null' - // FIXME: It would help to make this column NOT NULL DEFAULT 0 - if (!empty($params['is_default']) && !CRM_Utils_System::isNull($params['is_default'])) { - $query = 'UPDATE civicrm_option_value SET is_default = 0 WHERE option_group_id = %1'; - - // tweak default reset, and allow multiple default within group. - if ($resetDefaultFor = CRM_Utils_Array::value('reset_default_for', $params)) { - if (is_array($resetDefaultFor)) { - $colName = key($resetDefaultFor); - $colVal = $resetDefaultFor[$colName]; - $query .= " AND ( $colName IN ( $colVal ) )"; - } - } - - $p = [1 => [$params['option_group_id'], 'Integer']]; - - // Limit update by domain of option - $domain = CRM_Utils_System::isNull($optionValue->domain_id) ? NULL : $optionValue->domain_id; - if (!$domain && $id && $isDomainOptionGroup) { - $domain = CRM_Core_DAO::getFieldValue(__CLASS__, $id, 'domain_id'); - } - if ($domain) { - $query .= ' AND domain_id = %2'; - $p[2] = [$domain, 'Integer']; - } - - CRM_Core_DAO::executeQuery($query, $p); - } - $groupsSupportingDuplicateValues = ['languages']; if (!$id && !empty($params['value'])) { $dao = new CRM_Core_DAO_OptionValue(); @@ -218,6 +188,14 @@ class CRM_Core_BAO_OptionValue extends CRM_Core_DAO_OptionValue { $optionValue->id = $id; $optionValue->save(); + $id = $optionValue->id; + // When setting a default option, unset other options in this group as default + // FIXME: The extra CRM_Utils_System::isNull is because the API will pass the string 'null' + // FIXME: It would help to make this column NOT NULL DEFAULT 0 + if (!CRM_Utils_System::isNull($params['is_default'] ?? NULL)) { + $optionValue->find(TRUE); + self::updateOptionDefaults($params['option_group_id'], $optionValue->id, $optionValue, $groupName); + } Civi::cache('metadata')->flush(); CRM_Core_PseudoConstant::flush(); @@ -571,4 +549,30 @@ class CRM_Core_BAO_OptionValue extends CRM_Core_DAO_OptionValue { return CRM_Utils_Array::first($result['values']); } + /** + * Update the default values of other options in the group when the new value is set to is_default. + * + * @param int $optionGroupID + * @param int $id + * @param \CRM_Core_DAO_OptionValue $optionValue + * @param string $groupName + */ + private static function updateOptionDefaults(int $optionGroupID, int $id, CRM_Core_DAO_OptionValue $optionValue, string $groupName): void { + $query = 'UPDATE civicrm_option_value SET is_default = 0 WHERE option_group_id = %1 AND id <> %2'; + $queryParams = [1 => [$optionGroupID, 'Integer'], 2 => [$id, 'Integer']]; + + // Limit update by domain of option. This is loaded if it is a domain option group. + if (!empty($optionValue->domain_id)) { + $query .= ' AND domain_id = %3'; + $queryParams[3] = [(int) $optionValue->domain_id, 'Integer']; + } + if (in_array($groupName, ['email_greeting', 'postal_greeting', 'addressee'], TRUE)) { + $variableNumber = count($queryParams) + 1; + $query .= ' AND filter = %' . $variableNumber; + $queryParams[$variableNumber] = [(int) $optionValue->filter, 'Integer']; + } + + CRM_Core_DAO::executeQuery($query, $queryParams); + } + } diff --git a/tests/phpunit/CRM/Core/BAO/OptionValueTest.php b/tests/phpunit/CRM/Core/BAO/OptionValueTest.php index 2123167af9..3f332a9964 100644 --- a/tests/phpunit/CRM/Core/BAO/OptionValueTest.php +++ b/tests/phpunit/CRM/Core/BAO/OptionValueTest.php @@ -9,6 +9,8 @@ +--------------------------------------------------------------------+ */ +use Civi\Api4\OptionValue; + /** * Class CRM_Core_BAO_SchemaHandlerTest. * @@ -23,13 +25,58 @@ class CRM_Core_BAO_OptionValueTest extends CiviUnitTestCase { */ public function setUp(): void { parent::setUp(); - $this->useTransaction(TRUE); + $this->useTransaction(); + } + + /** + * Test that option values that support more than one default maintain them. + * + * There is code to support updating default = 0 for other options + * when an option value is set to 1. However, some option groups (eg. + * email_greeting) support multiple option values. + * + * @throws \CRM_Core_Exception + */ + public function testHandlingForMultiDefaultOptions(): void { + foreach (['email_greeting', 'postal_greeting', 'addressee'] as $optionGroupName) { + $options = $this->getDefaultOptions($optionGroupName); + $existing = count($options); + OptionValue::update()->addWhere('id', 'IN', array_keys($options)) + ->setValues(['description', '=', 'updated', 'is_default' => TRUE]) + ->execute(); + $options = $this->getDefaultOptions($optionGroupName); + $this->assertCount($existing, $options, 'There should be no change in the number of default options'); + } + } + + /** + * Test that option values that support more than one default maintain them. + * + * The from_email_address supports a single default per domain. + * + * @throws \CRM_Core_Exception + */ + public function testDefaultHandlingForFromEmailAddress(): void { + $options = $this->getDefaultOptions('from_email_address'); + $this->assertCount(2, $options); + OptionValue::create() + ->setValues([ + 'option_group_id:name' => 'from_email_address', + 'is_default' => TRUE, + 'label' => 'email@example.com', + 'name' => 'email@example.com', + 'value' => 3, + ]) + ->execute(); + + $options = $this->getDefaultOptions('from_email_address'); + $this->assertCount(2, $options, 'There should be one default per domain'); } /** * Ensure only one option value exists after calling ensureOptionValueExists. */ - public function testEnsureOptionValueExistsExistingValue() { + public function testEnsureOptionValueExistsExistingValue(): void { CRM_Core_BAO_OptionValue::ensureOptionValueExists(['name' => 'Completed', 'option_group_id' => 'contribution_status']); $this->callAPISuccessGetSingle('OptionValue', ['name' => 'Completed', 'option_group_id' => 'contribution_status']); } @@ -37,11 +84,11 @@ class CRM_Core_BAO_OptionValueTest extends CiviUnitTestCase { /** * Ensure only one option value exists adds a new value. */ - public function testEnsureOptionValueExistsNewValue() { + public function testEnsureOptionValueExistsNewValue(): void { CRM_Core_BAO_OptionValue::ensureOptionValueExists(['name' => 'Bombed', 'option_group_id' => 'contribution_status']); $optionValues = $this->callAPISuccess('OptionValue', 'get', ['option_group_id' => 'contribution_status']); foreach ($optionValues['values'] as $value) { - if ($value['name'] == 'Bombed') { + if ($value['name'] === 'Bombed') { return; } } @@ -54,7 +101,7 @@ class CRM_Core_BAO_OptionValueTest extends CiviUnitTestCase { * (Our expectation is no change - ie. currently we are respecting 'someone's * decision to disable it & leaving it in that state. */ - public function testEnsureOptionValueExistsDisabled() { + public function testEnsureOptionValueExistsDisabled(): void { $optionValue = CRM_Core_BAO_OptionValue::ensureOptionValueExists(['name' => 'Crashed', 'option_group_id' => 'contribution_status', 'is_active' => 0]); $value = $this->callAPISuccessGetSingle('OptionValue', ['name' => 'Crashed', 'option_group_id' => 'contribution_status']); $this->assertEquals(0, $value['is_active']); @@ -66,4 +113,19 @@ class CRM_Core_BAO_OptionValueTest extends CiviUnitTestCase { $this->assertEquals($value['id'], $optionValue['id']); } + /** + * @param string $optionGroupName + * + * @return array + * @throws \CRM_Core_Exception + * @throws \Civi\API\Exception\UnauthorizedException + */ + protected function getDefaultOptions(string $optionGroupName): array { + $options = (array) OptionValue::get() + ->addWhere('option_group_id:name', '=', $optionGroupName) + ->addWhere('is_default', '=', TRUE) + ->execute()->indexBy('id'); + return $options; + } + } -- 2.25.1