diff --git a/CRM/Contact/BAO/Group.php b/CRM/Contact/BAO/Group.php index 6ff6d55533db..6563b61d57c2 100644 --- a/CRM/Contact/BAO/Group.php +++ b/CRM/Contact/BAO/Group.php @@ -334,6 +334,10 @@ public function addSelectWhereClause() { * The new group BAO (if created) */ public static function create(&$params) { + + // only check if we need to remove parents if a params was provided + $parentsParamProvided = array_key_exists('parents', $params); + $params += [ 'group_type' => NULL, 'parents' => NULL, @@ -382,8 +386,12 @@ public static function create(&$params) { $params['name'] = CRM_Utils_String::titleToVar($params['title']); } - if (!CRM_Utils_System::isNull($params['parents'])) { + if ($parentsParamProvided) { $params['parents'] = CRM_Utils_Array::convertCheckboxFormatToArray((array) $params['parents']); + // failsafe: forbid adding itself as parent + if (!empty($params['id']) && ($key = array_search($params['id'], $params['parents'])) !== FALSE) { + unset($params['parents'][$key]); + } } // convert params if array type @@ -448,7 +456,7 @@ public static function create(&$params) { } // first deal with removed parents - if (array_key_exists('parents', $params) && !empty($currentParentGroupIDs)) { + if ($parentsParamProvided && !empty($currentParentGroupIDs)) { foreach ($currentParentGroupIDs as $parentGroupId) { // no more parents or not in the new list, let's remove if (empty($params['parents']) || !in_array($parentGroupId, $params['parents'])) { @@ -466,9 +474,10 @@ public static function create(&$params) { } } - // this is always required, since we don't know when a - // parent group is removed - CRM_Contact_BAO_GroupNestingCache::update(); + // refresh cache if parents param was provided + if ($parentsParamProvided || !empty($params['parents'])) { + CRM_Contact_BAO_GroupNestingCache::update(); + } // update group contact cache for all parent groups $parentIds = CRM_Contact_BAO_GroupNesting::getParentGroupIds($group->id); diff --git a/tests/phpunit/api/v4/Entity/GroupTest.php b/tests/phpunit/api/v4/Entity/GroupTest.php index bfe4ab1c49b8..4bb70048dd61 100644 --- a/tests/phpunit/api/v4/Entity/GroupTest.php +++ b/tests/phpunit/api/v4/Entity/GroupTest.php @@ -109,4 +109,59 @@ public function testGetParents() { $this->assertEquals([$parent1['title'], $parent2['title']], $get['parents:label']); } + public function testAddRemoveParents() { + $group1 = Group::create(FALSE) + ->addValue('title', uniqid()) + ->execute()->single(); + $parent1 = Group::create(FALSE) + ->addValue('title', uniqid()) + ->execute()->single(); + $parent2 = Group::create(FALSE) + ->addValue('title', uniqid()) + ->execute()->single(); + + // ensure self is not added as parent + Group::update(FALSE) + ->addValue('parents', [$group1['id']]) + ->addWhere('id', '=', $group1['id']) + ->execute(); + $get = Group::get(FALSE) + ->addWhere('id', '=', $group1['id']) + ->addSelect('parents') + ->execute()->single(); + $this->assertEmpty($get['parents']); + + Group::update(FALSE) + ->addValue('parents', [$parent1['id'], $parent2['id'], $group1['id']]) + ->addWhere('id', '=', $group1['id']) + ->execute(); + $get = Group::get(FALSE) + ->addWhere('id', '=', $group1['id']) + ->addSelect('parents') + ->execute()->single(); + $this->assertEquals([$parent1['id'], $parent2['id']], $get['parents']); + + // ensure adding something else doesn't impact parents + Group::update(FALSE) + ->addValue('title', uniqid()) + ->addWhere('id', '=', $group1['id']) + ->execute(); + $get = Group::get(FALSE) + ->addWhere('id', '=', $group1['id']) + ->addSelect('parents') + ->execute()->single(); + $this->assertEquals([$parent1['id'], $parent2['id']], $get['parents']); + + // ensure removing parent is working + Group::update(FALSE) + ->addValue('parents', [$parent2['id']]) + ->addWhere('id', '=', $group1['id']) + ->execute(); + $get = Group::get(FALSE) + ->addWhere('id', '=', $group1['id']) + ->addSelect('parents') + ->execute()->single(); + $this->assertEquals([$parent2['id']], $get['parents']); + } + }