From 50de22801f93aa29486b6f50eed005511fba2ba8 Mon Sep 17 00:00:00 2001
From: "Jason W. May" <jmay@pobox.com>
Date: Thu, 20 Nov 2014 09:29:56 -0800
Subject: [PATCH] API addition: HTTP PATCH support for /groups/xxx: incremental
 membership changes

---
 app/controllers/admin/groups_controller.rb    | 42 +++++++++--
 app/models/group.rb                           |  4 ++
 .../admin/groups_controller_spec.rb           | 70 ++++++++++++++++---
 3 files changed, 98 insertions(+), 18 deletions(-)

diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 21920d8a587..df21a4328a3 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -20,18 +20,36 @@ 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?
+    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
+
+    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 +58,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
diff --git a/app/models/group.rb b/app/models/group.rb
index ff143a9c1ff..2a451f192e5 100644
--- a/app/models/group.rb
+++ b/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
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index 7c5192ea332..5979523a1ea 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -79,18 +79,66 @@ describe Admin::GroupsController do
     end
   end
 
-  it "is able to update group members" do
-    user1 = Fabricate(:user)
-    user2 = Fabricate(:user)
-    group = Fabricate(:group)
+  context '.update' do
+    let (:group) { Fabricate(:group) }
 
-    xhr :put, :update, id: group.id, name: 'fred', group: {
-        name: 'fred',
-        usernames: "#{user1.username},#{user2.username}"
-    }
+    it "is able to update group members" do
+      user1 = Fabricate(:user)
+      user2 = Fabricate(:user)
 
-    group.reload
-    group.users.count.should == 2
-    group.name.should == 'fred'
+      xhr :put, :update, id: group.id, name: 'fred', group: {
+            name: 'fred',
+            usernames: "#{user1.username},#{user2.username}"
+          }
+
+      group.reload
+      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