Fixes #18325: Ensure existing permission group users are maintained when editing a group

[dylang3]

Fixes #18325

The existing associated users for a permission group are being passed to the view as a Collection in app/Http/Controllers/GroupsController.php:

public function edit(Group $group): View|RedirectResponse
    {
        $permissions = config("permissions");
        $groupPermissions = $group->decodePermissions();

        if (!is_array($groupPermissions) || !$groupPermissions) {
            $groupPermissions = [];
        }

        $selected_array = Helper::selectedPermissionsArray(
            $permissions,
            $groupPermissions,
        );

        $users_query = User::where("show_in_list", 1)->whereNull("deleted_at");
        $users_count = $users_query->count();

        $associated_users = collect();
        $unselected_users = collect();

        if ($users_count <= config("app.max_unpaginated_records")) {
            $associated_users = $group
                ->users()
                ->where("show_in_list", 1)
                ->orderBy("first_name", "asc")
                ->orderBy("last_name", "asc")
                ->get();
            // Get the unselected users
            $unselected_users = User::where("show_in_list", 1)
                ->whereNotIn("id", $associated_users->pluck("id")->toArray())
                ->orderBy("first_name", "asc")
                ->orderBy("last_name", "asc")
                ->get();
        }

        return view(
            "groups.edit",
            compact(
                "group",
                "permissions",
                "selected_array",
                "groupPermissions",
            ),
        )
            ->with("associated_users", $associated_users)
            ->with("unselected_users", $unselected_users)
            ->with("all_users_count", $users_count);
    }

However, in the groups.edit view, the value of the users_to_sync hidden field is checking if the $associated_users prop is an array. Since that prop is actually a Collection, this check fails, and the value defaults to an empty string even if there are users already associated with this group.

<input type="hidden" name="users_to_sync" id="hidden_ids_box" class="form-control" value="{{ ($associated_users && is_array($associated_users)) ? implode(",", $associated_users->pluck('id')->toArray()) :  '' }}"/>

Once the form is submitted, unless the users have been updated (i.e. users were added or removed to the group as part of the update -- I assume this is thanks to select2 pulling the existing members back in), all existing pivot records for the group are dropped from the users_groups table in the database when $group->users()->sync($associated_users) is called in the controller's update method since $associated_users consists solely of an empty string.

The fix removes the conditional and uses Collection methods directly: pluck('id')->implode(','). This works for both groups with no existing members (returning an empty string) and groups with existing members (returning a comma-separated list of user IDs), ensuring user-group associations are maintained when the group is updated.

[snipe]

I guess this seems fine, but all of our tests, we weren't dropping users in the first place. (See the piles of screencasts associated with those changes)

[snipe]

Looks like we have some failing tests here.

[dylang3]

Ah, it looks like the associated_users prop was being sent as an array in the create method. I've updated that to be consistent in sending the data as a collection in both methods now.

And apologies -- it completely slipped my mind to run the test suite before submitting the original PR -- we are all green now, though 🙂

[marcusmoore]

I tested this locally and it works 👍🏾