Merge pull request #3234 from fullfatthings/add_remove_group_members_by_id_or_name

Allow adding and removing members of groups by username or id

app/controllers/admin/groups_controller.rb

@@ -70,14 +70,19 @@ class Admin::GroupsController < Admin::AdminController
 
   def add_members
     group = Group.find(params.require(:id))
-    usernames = params.require(:usernames)
 
     return can_not_modify_automatic if group.automatic
 
-    usernames.split(",").each do |username|
+    if params[:usernames].present?
-      if user = User.find_by_username(username)
+      users = User.where(username: params[:usernames].split(","))
-        group.add(user)
+    elsif params[:user_ids].present?
-      end
+      users = User.find(params[:user_ids].split(","))
+    else
+      raise Discourse::InvalidParameters.new('user_ids or usernames must be present')
+    end
+
+    users.each do |user|
+      group.add(user)
     end
 
     if group.save
@@ -89,14 +94,20 @@ class Admin::GroupsController < Admin::AdminController
 
   def remove_member
     group = Group.find(params.require(:id))
-    user_id = params.require(:user_id).to_i
 
     return can_not_modify_automatic if group.automatic
 
-    user = User.find(user_id)
+    if params[:user_id].present?
+      user = User.find(params[:user_id])
+    elsif params[:username].present?
+      user = User.find_by_username(params[:username])
+    else
+      raise Discourse::InvalidParameters.new('user_id or username must be present')
+    end
+
     user.primary_group_id = nil if user.primary_group_id == group.id
 
-    group.users.delete(user_id)
+    group.users.delete(user.id)
 
     if group.save && user.save
       render json: success_json

spec/controllers/admin/groups_controller_spec.rb

@@ -110,16 +110,27 @@ describe Admin::GroupsController do
       expect(response.status).to eq(422)
     end
 
-    it "is able to add several members to a group" do
+    context "is able to add several members to a group" do
-      user1 = Fabricate(:user)
-      user2 = Fabricate(:user)
-      group = Fabricate(:group)
 
-      xhr :put, :add_members, id: group.id, usernames: [user1.username, user2.username].join(",")
+      let(:user1) { Fabricate(:user) }
+      let(:user2) { Fabricate(:user) }
+      let(:group) { Fabricate(:group) }
 
-      expect(response).to be_success
+      it "adds by username" do
-      group.reload
+        xhr :put, :add_members, id: group.id, usernames: [user1.username, user2.username].join(",")
-      expect(group.users.count).to eq(2)
+
+        expect(response).to be_success
+        group.reload
+        expect(group.users.count).to eq(2)
+      end
+
+      it "adds by id" do
+        xhr :put, :add_members, id: group.id, user_ids: [user1.id, user2.id].join(",")
+
+        expect(response).to be_success
+        group.reload
+        expect(group.users.count).to eq(2)
+      end
     end
 
   end
@@ -131,23 +142,41 @@ describe Admin::GroupsController do
       expect(response.status).to eq(422)
     end
 
-    it "is able to remove a member" do
+    context "is able to remove a member" do
-      group = Fabricate(:group)
-      user = Fabricate(:user)
-      group.add(user)
-      group.save
 
-      user.primary_group_id = group.id
+      let(:user) { Fabricate(:user) }
-      user.save
+      let(:group) { Fabricate(:group) }
 
-      xhr :delete, :remove_member, id: group.id, user_id: user.id
+      before do
+        group.add(user)
+        group.save
+      end
 
-      expect(response).to be_success
+      it "removes by id" do
-      group.reload
+        xhr :delete, :remove_member, id: group.id, user_id: user.id
-      expect(group.users.count).to eq(0)
 
-      user.reload
+        expect(response).to be_success
-      expect(user.primary_group_id).to eq(nil)
+        group.reload
+        expect(group.users.count).to eq(0)
+      end
+
+      it "removes by username" do
+        xhr :delete, :remove_member, id: group.id, username: user.username
+
+        expect(response).to be_success
+        group.reload
+        expect(group.users.count).to eq(0)
+      end
+
+      it "removes user.primary_group_id when user is removed from group" do
+        user.primary_group_id = group.id
+        user.save
+
+        xhr :delete, :remove_member, id: group.id, username: user.username
+
+        user.reload
+        expect(user.primary_group_id).to eq(nil)
+      end
     end
 
   end