Merge pull request #2989 from jmay/group-admin-incremental

API addition: HTTP PATCH support for /groups/xxx: incremental membership changes

app/controllers/admin/groups_controller.rb

@@ -20,18 +20,37 @@ class Admin::GroupsController < Admin::AdminController
     render json: success_json
   end
 
-  def update
-    group = Group.find(params[:id].to_i)
+  def update_patch(group)
+    raise Discourse::InvalidAccess.new("automatic groups do not permit membership changes") if group.automatic
 
-    group.alias_level = params[:group][:alias_level].to_i if params[:group][:alias_level].present?
+    if actions = params[:changes]
+      Array(actions[:add]).each do |username|
+        if user = User.find_by_username(username)
+          group.add(user)
+        end
+      end
+      Array(actions[:delete]).each do |username|
+        if user = User.find_by_username(username)
+          group.remove(user)
+        end
+      end
+    end
+
+    render json: success_json
+  end
+
+  def update_put(group)
+    payload = params[:group]
+
+    group.alias_level = payload[:alias_level].to_i if payload[:alias_level].present?
+    group.visible = payload[:visible] == "true"
 
     if group.automatic
-      # we can only change the alias level on automatic groups
+      # group rename & membership changes are ignored/prohibited for automatic groups
     else
-      group.usernames = params[:group][:usernames]
-      group.name = params[:group][:name] if params[:group][:name]
+      group.usernames = payload[:usernames] if payload[:usernames]
+      group.name = payload[:name] if payload[:name]
     end
-    group.visible = params[:group][:visible] == "true"
 
     if group.save
       render json: success_json
@@ -40,6 +59,16 @@ class Admin::GroupsController < Admin::AdminController
     end
   end
 
+  def update
+    group = Group.find(params[:id].to_i)
+
+    if request.patch?
+      update_patch(group)
+    else
+      update_put(group)
+    end
+  end
+
   def create
     group = Group.new
     group.name = (params[:group][:name] || '').strip

app/models/group.rb

@@ -273,6 +273,10 @@ class Group < ActiveRecord::Base
     self.users.push(user)
   end
 
+  def remove(user)
+    self.group_users.where(user: user).each(&:destroy)
+  end
+
   protected
 
     def name_format_validator

spec/controllers/admin/groups_controller_spec.rb

@@ -79,10 +79,12 @@ describe Admin::GroupsController do
     end
   end
 
+  context '.update' do
+    let (:group) { Fabricate(:group) }
+
     it "is able to update group members" do
       user1 = Fabricate(:user)
       user2 = Fabricate(:user)
-    group = Fabricate(:group)
 
       xhr :put, :update, id: group.id, name: 'fred', group: {
             name: 'fred',
@@ -93,4 +95,50 @@ describe Admin::GroupsController do
       group.users.count.should == 2
       group.name.should == 'fred'
     end
+
+    context 'incremental' do
+      before do
+        @user1 = Fabricate(:user)
+        group.add(@user1)
+        group.reload
+      end
+
+      it "can make incremental adds" do
+        user2 = Fabricate(:user)
+        xhr :patch, :update, id: group.id, changes: {add: user2.username}
+        response.status.should == 200
+        group.reload
+        group.users.count.should eq(2)
+      end
+
+      it "succeeds silently when adding non-existent users" do
+        xhr :patch, :update, id: group.id, changes: {add: "nosuchperson"}
+        response.status.should == 200
+        group.reload
+        group.users.count.should eq(1)
+      end
+
+      it "can make incremental deletes" do
+        xhr :patch, :update, id: group.id, changes: {delete: @user1.username}
+        response.status.should == 200
+        group.reload
+        group.users.count.should eq(0)
+      end
+
+      it "succeeds silently when removing non-members" do
+        user2 = Fabricate(:user)
+        xhr :patch, :update, id: group.id, changes: {delete: user2.username}
+        response.status.should == 200
+        group.reload
+        group.users.count.should eq(1)
+      end
+
+      it "cannot patch automatic groups" do
+        auto_group = Fabricate(:group, name: "auto_group", automatic: true)
+
+        xhr :patch, :update, id: auto_group.id, changes: {add: "bob"}
+        response.status.should == 403
+      end
+    end
+  end
 end