group.update Parent omitted in params should do nothing + avoid recursion

diff --git a/CRM/Contact/BAO/Group.php b/CRM/Contact/BAO/Group.php
index 6ff6d55533db8b35f20d473b96b17f87cfb3c35b..5c16c159418881189f34c0d4577c1cb0368f2ee4 100644 (file)
--- a/CRM/Contact/BAO/Group.php
+++ b/CRM/Contact/BAO/Group.php
@@ -334,6 +334,10 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group {
    *   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 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group {
       $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 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group {
       }
 
       // 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 @@ class CRM_Contact_BAO_Group extends CRM_Contact_DAO_Group {
         }
       }
 
-      // 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) {
+        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 bfe4ab1c49b8dfd551b7d608f8cfc92ce8bdb37e..4bb70048dd6128a56d778eb39458004650928b53 100644 (file)
--- a/tests/phpunit/api/v4/Entity/GroupTest.php
+++ b/tests/phpunit/api/v4/Entity/GroupTest.php
@@ -109,4 +109,59 @@ class GroupTest extends Api4TestBase {
     $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']);
+  }
+
 }