From 3bec4854eedca7ff3218ae76b263fb68a35d9552 Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 26 Sep 2020 08:49:18 +1200 Subject: [PATCH] Fix bug inn primary handling where TRUE rather than 1 used --- CRM/Core/BAO/Address.php | 5 +---- CRM/Core/BAO/Block.php | 4 ++++ CRM/Core/BAO/Email.php | 5 +---- CRM/Core/BAO/IM.php | 4 +--- CRM/Core/BAO/OpenID.php | 4 +--- CRM/Core/BAO/Phone.php | 13 +++++-------- tests/phpunit/api/v3/PhoneTest.php | 20 +++++++++++++++++--- 7 files changed, 30 insertions(+), 25 deletions(-) diff --git a/CRM/Core/BAO/Address.php b/CRM/Core/BAO/Address.php index ea7da9440c..771c6cc95f 100644 --- a/CRM/Core/BAO/Address.php +++ b/CRM/Core/BAO/Address.php @@ -131,10 +131,7 @@ class CRM_Core_BAO_Address extends CRM_Core_DAO_Address { $hook = empty($params['id']) ? 'create' : 'edit'; CRM_Utils_Hook::pre($hook, 'Address', CRM_Utils_Array::value('id', $params), $params); - // if id is set & is_primary isn't we can assume no change - if (is_numeric(CRM_Utils_Array::value('is_primary', $params)) || empty($params['id'])) { - CRM_Core_BAO_Block::handlePrimary($params, get_class()); - } + CRM_Core_BAO_Block::handlePrimary($params, get_class()); // (prevent chaining 1 and 3) CRM-21214 if (isset($params['master_id']) && !CRM_Utils_System::isNull($params['master_id'])) { diff --git a/CRM/Core/BAO/Block.php b/CRM/Core/BAO/Block.php index 4f17f5b57c..07167d621c 100644 --- a/CRM/Core/BAO/Block.php +++ b/CRM/Core/BAO/Block.php @@ -391,6 +391,10 @@ class CRM_Core_BAO_Block { * @throws API_Exception */ public static function handlePrimary(&$params, $class) { + if (isset($params['id']) && CRM_Utils_System::isNull($params['is_primary'] ?? NULL)) { + // if id is set & is_primary isn't we can assume no change) + return; + } $table = CRM_Core_DAO_AllCoreTables::getTableForClass($class); if (!$table) { throw new API_Exception("Failed to locate table for class [$class]"); diff --git a/CRM/Core/BAO/Email.php b/CRM/Core/BAO/Email.php index d92b6e94a6..aa0fdc956a 100644 --- a/CRM/Core/BAO/Email.php +++ b/CRM/Core/BAO/Email.php @@ -31,10 +31,7 @@ class CRM_Core_BAO_Email extends CRM_Core_DAO_Email { * @return object */ public static function create($params) { - // if id is set & is_primary isn't we can assume no change - if (is_numeric(CRM_Utils_Array::value('is_primary', $params)) || empty($params['id'])) { - CRM_Core_BAO_Block::handlePrimary($params, get_class()); - } + CRM_Core_BAO_Block::handlePrimary($params, get_class()); $hook = empty($params['id']) ? 'create' : 'edit'; CRM_Utils_Hook::pre($hook, 'Email', CRM_Utils_Array::value('id', $params), $params); diff --git a/CRM/Core/BAO/IM.php b/CRM/Core/BAO/IM.php index cdab960f65..c7e11c450c 100644 --- a/CRM/Core/BAO/IM.php +++ b/CRM/Core/BAO/IM.php @@ -30,9 +30,7 @@ class CRM_Core_BAO_IM extends CRM_Core_DAO_IM { * @throws \API_Exception */ public static function add($params) { - if (empty($params['id']) || is_numeric($params['is_primary'] ?? NULL)) { - CRM_Core_BAO_Block::handlePrimary($params, __CLASS__); - } + CRM_Core_BAO_Block::handlePrimary($params, __CLASS__); return self::writeRecord($params); } diff --git a/CRM/Core/BAO/OpenID.php b/CRM/Core/BAO/OpenID.php index c603155a9f..4382097e67 100644 --- a/CRM/Core/BAO/OpenID.php +++ b/CRM/Core/BAO/OpenID.php @@ -31,9 +31,7 @@ class CRM_Core_BAO_OpenID extends CRM_Core_DAO_OpenID { * @throws \CRM_Core_Exception */ public static function add($params) { - if (empty($params['id']) || is_numeric($params['is_primary'] ?? NULL)) { - CRM_Core_BAO_Block::handlePrimary($params, __CLASS__); - } + CRM_Core_BAO_Block::handlePrimary($params, __CLASS__); return self::writeRecord($params); } diff --git a/CRM/Core/BAO/Phone.php b/CRM/Core/BAO/Phone.php index dac8c9fa5f..7e5a7eede5 100644 --- a/CRM/Core/BAO/Phone.php +++ b/CRM/Core/BAO/Phone.php @@ -26,19 +26,15 @@ class CRM_Core_BAO_Phone extends CRM_Core_DAO_Phone { * * @param array $params * - * @return object + * @return \CRM_Core_DAO_Phone + * * @throws API_Exception + * @throws \CRM_Core_Exception */ public static function create($params) { // Ensure mysql phone function exists CRM_Core_DAO::checkSqlFunctionsExist(); - - if (is_numeric(CRM_Utils_Array::value('is_primary', $params)) || - // if id is set & is_primary isn't we can assume no change - empty($params['id']) - ) { - CRM_Core_BAO_Block::handlePrimary($params, get_class()); - } + CRM_Core_BAO_Block::handlePrimary($params, get_class()); return self::writeRecord($params); } @@ -69,6 +65,7 @@ class CRM_Core_BAO_Phone extends CRM_Core_DAO_Phone { * * @return array * array of phone objects + * @throws \CRM_Core_Exception */ public static function &getValues($entityBlock) { $getValues = CRM_Core_BAO_Block::getValues('phone', $entityBlock); diff --git a/tests/phpunit/api/v3/PhoneTest.php b/tests/phpunit/api/v3/PhoneTest.php index 6b13d392dc..a051b92656 100644 --- a/tests/phpunit/api/v3/PhoneTest.php +++ b/tests/phpunit/api/v3/PhoneTest.php @@ -22,6 +22,13 @@ class api_v3_PhoneTest extends CiviUnitTestCase { protected $_params; protected $_entity; + /** + * Should location types be checked to ensure primary addresses are correctly assigned after each test. + * + * @var bool + */ + protected $isLocationTypesOnPostAssert = TRUE; + public function setUp() { $this->_entity = 'Phone'; parent::setUp(); @@ -35,19 +42,21 @@ class api_v3_PhoneTest extends CiviUnitTestCase { 'contact_id' => $this->_contactID, 'location_type_id' => $this->_locationType, 'phone' => '(123) 456-7890', - 'is_primary' => 1, + 'is_primary' => TRUE, 'phone_type_id' => 1, ]; } /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreatePhone($version) { $this->_apiversion = $version; - $result = $this->callAPIAndDocument('phone', 'create', $this->_params, __FUNCTION__, __FILE__); + $result = $this->callAPIAndDocument('Phone', 'create', $this->_params, __FUNCTION__, __FILE__); $this->assertEquals(1, $result['count']); $this->assertNotNull($result['values'][$result['id']]['id']); @@ -59,7 +68,9 @@ class api_v3_PhoneTest extends CiviUnitTestCase { * the LocationType default * * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreatePhoneDefaultLocation($version) { $this->_apiversion = $version; @@ -164,17 +175,20 @@ class api_v3_PhoneTest extends CiviUnitTestCase { /** * @param int $version + * * @dataProvider versionThreeAndFour + * @throws \CRM_Core_Exception */ public function testCreatePhonePrimaryHandlingChangeExisting($version) { $this->_apiversion = $version; $phone1 = $this->callAPISuccess('phone', 'create', $this->_params); - $phone2 = $this->callAPISuccess('phone', 'create', $this->_params); + $this->callAPISuccess('phone', 'create', $this->_params); $check = $this->callAPISuccess('phone', 'getcount', [ 'is_primary' => 1, 'contact_id' => $this->_contactID, ]); $this->assertEquals(1, $check); + $this->callAPISuccess('Phone', 'create', ['id' => $phone1['id'], 'is_primary' => TRUE]); } } -- 2.25.1