From 6a5f12fae78b0b491c66e0fab3d0878c632d6ee6 Mon Sep 17 00:00:00 2001
From: Benn Oshrin <boshrin@users.noreply.github.com>
Date: Sun, 9 Oct 2022 15:53:45 -0400
Subject: [PATCH] Miscellaneous fixes for Group Nestings (CFM-41)

---
 app/src/Model/Table/GroupMembersTable.php  | 5 ++++-
 app/src/Model/Table/GroupNestingsTable.php | 8 ++++----
 app/src/Model/Table/GroupsTable.php        | 2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/app/src/Model/Table/GroupMembersTable.php b/app/src/Model/Table/GroupMembersTable.php
index 8bd398855..93076a0ce 100644
--- a/app/src/Model/Table/GroupMembersTable.php
+++ b/app/src/Model/Table/GroupMembersTable.php
@@ -386,7 +386,10 @@ public function syncNestedMembership(int  $personId,
                           ->all();
     
     // We convert $groupNestings to an array to avoid any confusion with the
-    // nested foreach() loops
+    // nested foreach() loops. Note that (unlike v4) we do not need to check for
+    // suspended Group status here since Groups cannot be suspended if they are
+    // nested (AR-Group-2), and cannot be nested if they are suspended (AR-GroupNesting-1).
+    // (This prevents admins from inadvertantly messing things up.)
     foreach($groupNestings->toArray() as $groupNesting) {
       $shouldBe = false;      // Should $person be a member of $targetGroup?
       $negated = false;       // $person is ineligible for $targetGroup due to any negative membership
diff --git a/app/src/Model/Table/GroupNestingsTable.php b/app/src/Model/Table/GroupNestingsTable.php
index ebd30ab8e..d3a36e791 100644
--- a/app/src/Model/Table/GroupNestingsTable.php
+++ b/app/src/Model/Table/GroupNestingsTable.php
@@ -103,13 +103,13 @@ public function initialize(array $config): void {
    * specified source.
    *
    * @since  COmanage Registry v5.0.0
-   * @param  int   $id Group ID (of source Group)
-   * @return array     Array of available target Groups, as returned by find('list')
+   * @param  int   $groupId Group ID (of source Group)
+   * @return array          Array of available target Groups, as returned by find('list')
    */
   
   public function availableGroups(int $groupId): array {
     // Find the CO for $id. This will throw an exception if not found.
-    $sourceGroup = $this->Groups->get($id);
+    $sourceGroup = $this->Groups->get($groupId);
     
     // We don't remove groups that are already nested from the list -- we'll
     // catch those in rule validation.
@@ -120,7 +120,7 @@ public function availableGroups(int $groupId): array {
                           // AR-Group-Nesting-1 Only Active groups may be nested
                           'Groups.status'     => SuspendableStatusEnum::Active,
                           // AR-Group-Nesting-2 A group may not nest into itself
-                          'Groups.id IS NOT'  => $id,
+                          'Groups.id IS NOT'  => $groupId,
                           // AR-Group-Nesting-3 Automatic groups cannot be targets
                           'OR' => [
                             'Groups.group_type NOT IN' => [GroupTypeEnum::ActiveMembers, GroupTypeEnum::AllMembers],
diff --git a/app/src/Model/Table/GroupsTable.php b/app/src/Model/Table/GroupsTable.php
index 8dc71b91e..d0046913b 100644
--- a/app/src/Model/Table/GroupsTable.php
+++ b/app/src/Model/Table/GroupsTable.php
@@ -252,7 +252,7 @@ public function buildRules(RulesChecker $rules): RulesChecker {
     
     // AR-Group-2 A Group cannot be set to Suspended if it is nested into a
     // Target Group or is a Target Group for a nesting. This and AR-Group-3
-    // are to avoid unexpected consequences from implicitly undoing anesting...
+    // are to avoid unexpected consequences from implicitly undoing a nesting...
     // the administrator must do that first.
     $rules->addUpdate([$this, 'ruleIsNested'],
                        'isNestedUpdate',