Merge pull request #26260 from samuelsov/baogroup

Fix problem with group.update api
civicrm · May 29, 2023 · 85b3d15 · 85b3d15
2 parents 617cea2 + fcba67d
commit 85b3d15
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 5 deletions.
diff --git 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
@@ -146,4 +146,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']);
+  }
+
 }