FEATURE: support group owner, capable of controlling group membership

Group owners are regular users that can add or remove users to a group
The Admin UX allows admins to appoint group owners
The public group UX will display group owners first and unlock UI to
add and remove members

Group owners can only be appointed on non automatic groups
Group owners may not appoint another group owner

app/assets/javascripts/admin/controllers/admin-group.js.es6

@@ -67,6 +67,22 @@ export default Ember.Controller.extend({
       });
     },
 
+    removeOwner(member) {
+      const self = this,
+            message = I18n.t("admin.groups.delete_owner_confirm", { username: member.get("username"), group: this.get("model.name") });
+      return bootbox.confirm(message, I18n.t("no_value"), I18n.t("yes_value"), function(confirm) {
+        if (confirm) {
+          self.get("model").removeOwner(member);
+        }
+      });
+    },
+
+    addOwners() {
+      if (Em.isEmpty(this.get("model.ownerUsernames"))) { return; }
+      this.get("model").addOwners(this.get("model.ownerUsernames")).catch(popupAjaxError);
+      this.set("model.ownerUsernames", null);
+    },
+
     addMembers() {
       if (Em.isEmpty(this.get("model.usernames"))) { return; }
       this.get("model").addMembers(this.get("model.usernames")).catch(popupAjaxError);

app/assets/javascripts/admin/templates/group.hbs

@@ -10,6 +10,23 @@
   </div>
 
   {{#if model.id}}
+    {{#unless model.automatic}}
+      {{#if model.hasOwners}}
+      <div>
+        <label for='owner-list'>{{i18n 'admin.groups.group_owners'}}</label>
+        <div class="ac-wrap clearfix" id='owner-list'>
+          {{#each model.owners as |member|}}
+            {{group-member member=member removeAction="removeOwner"}}
+          {{/each}}
+        </div>
+      </div>
+      {{/if}}
+      <div>
+        <label for="owner-selector">{{i18n 'admin.groups.add_owners'}}</label>
+        {{user-selector usernames=model.ownerUsernames placeholderKey="admin.groups.selector_placeholder" id="owner-selector"}}
+        {{d-button action="addOwners" class="add" icon="plus" label="admin.groups.add"}}
+      </div>
+    {{/unless}}
     <div>
       <label>{{i18n 'admin.groups.group_members'}} ({{model.user_count}})</label>
       <div>

app/assets/javascripts/discourse/controllers/group/members.js.es6

@@ -3,7 +3,29 @@ export default Ember.Controller.extend({
   limit: null,
   offset: null,
 
+  isOwner: function() {
+    if (this.get('currentUser.admin')) {
+      return true;
+    }
+    const owners = this.get('model.owners');
+    const currentUserId = this.get('currentUser.id');
+    if (currentUserId) {
+      return !!owners.findBy('id', currentUserId);
+    }
+  }.property('model.owners.@each'),
+
   actions: {
+    removeMember(user) {
+      this.get('model').removeMember(user);
+    },
+
+    addMembers() {
+      const usernames = this.get('usernames');
+      if (usernames && usernames.length > 0) {
+        this.get('model').addMembers(usernames).then(() => this.set('usernames', []));
+      }
+    },
+
     loadMore() {
       if (this.get("loading")) { return; }
       // we've reached the end

app/assets/javascripts/discourse/models/group.js.es6

@@ -1,12 +1,17 @@
+import computed from 'ember-addons/ember-computed-decorators';
+
 const Group = Discourse.Model.extend({
   limit: 50,
   offset: 0,
   user_count: 0,
+  owners: [],
 
-  emailDomains: function() {
+  hasOwners: Ember.computed.notEmpty('owners'),
-    var value = this.get("automatic_membership_email_domains");
+
+  @computed("automatic_membership_email_domains")
+  emailDomains(value) {
     return Em.isEmpty(value) ? "" : value;
-  }.property("automatic_membership_email_domains"),
+  },
 
   type: function() {
     return this.get("automatic") ? "automatic" : "custom";
@@ -24,18 +29,41 @@ const Group = Discourse.Model.extend({
     const self = this, offset = Math.min(this.get("user_count"), Math.max(this.get("offset"), 0));
 
     return Discourse.Group.loadMembers(this.get("name"), offset, this.get("limit")).then(function (result) {
+      var ownerIds = {};
+      result.owners.forEach(function(owner){
+        ownerIds[owner.id] = true;
+      });
+      const owners = result.owners.map(owner => Discourse.User.create(owner));
+
       self.setProperties({
         user_count: result.meta.total,
         limit: result.meta.limit,
         offset: result.meta.offset,
-        members: result.members.map(member => Discourse.User.create(member))
+        members: result.members.map(member => {
+          if (ownerIds[member.id]) {
+            member.owner = true;
+          }
+          return Discourse.User.create(member);
+        }),
+        owners: result.owners.map(owner => Discourse.User.create(owner))
       });
     });
   },
 
+  removeOwner(member) {
+    var self = this;
+    return Discourse.ajax('/admin/groups/' + this.get('id') + '/owners.json', {
+      type: "DELETE",
+      data: { user_id: member.get("id") }
+    }).then(function() {
+      // reload member list
+      self.findMembers();
+    });
+  },
+
   removeMember(member) {
     var self = this;
-    return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
+    return Discourse.ajax('/groups/' + this.get('id') + '/members.json', {
       type: "DELETE",
       data: { user_id: member.get("id") }
     }).then(function() {
@@ -46,7 +74,17 @@ const Group = Discourse.Model.extend({
 
   addMembers(usernames) {
     var self = this;
-    return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
+    return Discourse.ajax('/groups/' + this.get('id') + '/members.json', {
+      type: "PUT",
+      data: { usernames: usernames }
+    }).then(function() {
+      self.findMembers();
+    });
+  },
+
+  addOwners(usernames) {
+    var self = this;
+    return Discourse.ajax('/admin/groups/' + this.get('id') + '/owners.json', {
       type: "PUT",
       data: { usernames: usernames }
     }).then(function() {

app/assets/javascripts/discourse/templates/group/members.hbs

@@ -1,18 +1,40 @@
 {{#if model}}
+  {{#if isOwner}}
+    <div class='clearfix'>
+      <form id='add-user-to-group' autocomplete="off">
+        {{user-selector usernames=usernames placeholderKey="groups.selector_placeholder" id="user-search-selector" name="usernames"}}
+        {{d-button action="addMembers" class="add" icon="plus" label="groups.add"}}
+      </form>
+    </div>
+  {{/if}}
   <table class='group-members'>
     <tr>
       <th colspan="2">{{i18n 'last_post'}}</th>
       <th>{{i18n 'last_seen'}}</th>
+      {{#if isOwner}}
+      <th></th>
+      {{/if}}
     </tr>
   {{#each model.members as |m|}}
     <tr>
-      <td class='avatar'>{{user-small user=m}}</td>
+      <td class='avatar'>{{user-small user=m}}
+        {{#if m.owner}}
+        <span class='is-owner'>{{i18n "groups.owner"}}</span>
+        {{/if}} 
+      </td>
       <td>
         <span class="text">{{bound-date m.last_posted_at}}</span>
       </td>
       <td>
         <span class="text">{{bound-date m.last_seen_at}}</span>
       </td>
+      {{#if isOwner}}
+        <td class='remove-user'>
+        {{#unless m.owner}}
+        <a class="remove-link" {{action "removeMember" m}}><i class="fa fa-times"></i></a>
+        {{/unless}}
+        </td>
+      {{/if}}
     </tr>
   {{/each}}
 </table>

app/assets/stylesheets/common/base/compose.scss

@@ -75,6 +75,23 @@ div.ac-wrap.disabled {
   }
 }
 
+div.ac-wrap div.item a.remove, .remove-link {
+  margin-left: 4px;
+  font-size: 11px;
+  line-height: 10px;
+  padding: 1.5px 1.5px 1.5px 2.5px;
+  border-radius: 12px;
+  width: 10px;
+  display: inline-block;
+  border: 1px solid dark-light-diff($primary, $secondary, 90%, -60%);
+  &:hover {
+    background-color: scale-color($danger, $lightness: 75%);
+    border: 1px solid scale-color($danger, $lightness: 30%);
+    text-decoration: none;
+    color: $danger;
+  }
+}
+
 div.ac-wrap {
   background-color: $secondary;
   border: 1px solid dark-light-diff($primary, $secondary, 90%, -60%);
@@ -88,22 +105,6 @@ div.ac-wrap {
       display: inline-block;
       line-height: 20px;
     }
-    a.remove {
-      margin-left: 4px;
-      font-size: 11px;
-      line-height: 10px;
-      padding: 1.5px 1.5px 1.5px 2.5px;
-      border-radius: 12px;
-      width: 10px;
-      display: inline-block;
-      border: 1px solid dark-light-diff($primary, $secondary, 90%, -60%);
-      &:hover {
-        background-color: scale-color($danger, $lightness: 75%);
-        border: 1px solid scale-color($danger, $lightness: 30%);
-        text-decoration: none;
-        color: $danger;
-      }
-    }
   }
   input[type="text"] {
     float: left;

app/assets/stylesheets/desktop/user.scss

@@ -101,6 +101,21 @@
 .user-main {
   margin-bottom: 50px;
 
+  // name hacky so lastpass does not freak out
+  // -search- means it is bypassed
+  #add-user-to-group {
+      button, .ac-wrap {
+        float: left;
+      }
+      button {
+        margin-top: 3px;
+        margin-left: 10px;
+      }
+      #user-search-selector {
+        width: 400px;
+      }
+   }
+
   table.group-members {
     width: 100%;
     p {
@@ -116,6 +131,16 @@
     }
     td.avatar {
       width: 60px;
+      position: relative;
+      .is-owner {
+        position: absolute;
+        right: 0;
+        top: 20px;
+        color: dark-light-diff($primary, $secondary, 50%, -50%);
+      }
+    }
+    td.remove-user {
+      text-align: right;
     }
     td {
       padding: 0.5em;

app/controllers/admin/groups_controller.rb

@@ -107,58 +107,30 @@ class Admin::GroupsController < Admin::AdminController
     render json: success_json
   end
 
-  def add_members
+  def add_owners
     group = Group.find(params.require(:id))
-
     return can_not_modify_automatic if group.automatic
 
-    if params[:usernames].present?
+    users = User.where(username: params[:usernames].split(","))
-      users = User.where(username: params[:usernames].split(","))
-    elsif params[:user_ids].present?
-      users = User.find(params[:user_ids].split(","))
-    elsif params[:user_emails].present?
-      users = User.where(email: params[:user_emails].split(","))
-    else
-      raise Discourse::InvalidParameters.new('user_ids or usernames or user_emails must be present')
-    end
 
     users.each do |user|
       if !group.users.include?(user)
         group.add(user)
-      else
-        return render_json_error I18n.t('groups.errors.member_already_exist', username: user.username)
       end
+      group.group_users.where(user_id: user.id).update_all(owner: true)
     end
 
-    if group.save
+    render json: success_json
-      render json: success_json
-    else
-      render_json_error(group)
-    end
   end
 
-  def remove_member
+  def remove_owner
     group = Group.find(params.require(:id))
-
     return can_not_modify_automatic if group.automatic
 
-    if params[:user_id].present?
+    user = User.find(params[:user_id].to_i)
-      user = User.find(params[:user_id])
+    group.group_users.where(user_id: user.id).update_all(owner: false)
-    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
+    render json: success_json
-
-    group.users.delete(user.id)
-
-    if group.save && user.save
-      render json: success_json
-    else
-      render_json_error(group)
-    end
   end
 
   protected

app/controllers/groups_controller.rb

@@ -23,10 +23,12 @@ class GroupsController < ApplicationController
     offset = params[:offset].to_i
 
     total = group.users.count
-    members = group.users.order(:username_lower).limit(limit).offset(offset)
+    members = group.users.order('NOT group_users.owner').order(:username_lower).limit(limit).offset(offset)
+    owners = group.users.order(:username_lower).where('group_users.owner')
 
     render json: {
       members: serialize_data(members, GroupUserSerializer),
+      owners: serialize_data(owners, GroupUserSerializer),
       meta: {
         total: total,
         limit: limit,
@@ -36,35 +38,56 @@ class GroupsController < ApplicationController
   end
 
   def add_members
-    guardian.ensure_can_edit!(the_group)
+    group = Group.find(params[:id])
+    guardian.ensure_can_edit!(group)
 
-    added_users = []
+    if params[:usernames].present?
-    usernames = params.require(:usernames)
+      users = User.where(username: params[:usernames].split(","))
-    usernames.split(",").each do |username|
+    elsif params[:user_ids].present?
-      if user = User.find_by_username(username)
+      users = User.find(params[:user_ids].split(","))
-        unless the_group.users.include?(user)
+    elsif params[:user_emails].present?
-          the_group.add(user)
+      users = User.where(email: params[:user_emails].split(","))
-          added_users << user
+    else
-        end
+      raise Discourse::InvalidParameters.new('user_ids or usernames or user_emails must be present')
+    end
+
+    users.each do |user|
+      if !group.users.include?(user)
+        group.add(user)
+      else
+        return render_json_error I18n.t('groups.errors.member_already_exist', username: user.username)
       end
     end
 
-    # always succeeds, even if bogus usernames were provided
+    if group.save
-    render_serialized(added_users, GroupUserSerializer)
+      render json: success_json
+    else
+      render_json_error(group)
+    end
   end
 
   def remove_member
-    guardian.ensure_can_edit!(the_group)
+    group = Group.find(params[:id])
+    guardian.ensure_can_edit!(group)
 
-    removed_users = []
+    if params[:user_id].present?
-    username = params.require(:username)
+      user = User.find(params[:user_id])
-    if user = User.find_by_username(username)
+    elsif params[:username].present?
-      the_group.remove(user)
+      user = User.find_by_username(params[:username])
-      removed_users << user
+    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)
+
+    if group.save && user.save
+      render json: success_json
+    else
+      render_json_error(group)
     end
 
-    # always succeeds, even if user was not a member
-    render_serialized(removed_users, GroupUserSerializer)
   end
 
   private

app/models/group.rb

@@ -7,9 +7,6 @@ class Group < ActiveRecord::Base
   has_many :categories, through: :category_groups
   has_many :users, through: :group_users
 
-  has_many :group_managers, dependent: :destroy
-  has_many :managers, through: :group_managers
-
   after_save :destroy_deletions
   after_save :automatic_group_membership
   after_save :update_primary_group
@@ -283,8 +280,8 @@ class Group < ActiveRecord::Base
     user.update_columns(primary_group_id: nil) if user.primary_group_id == self.id
   end
 
-  def appoint_manager(user)
+  def add_owner(user)
-    managers << user
+    self.group_users.create(user_id: user.id, owner: true)
   end
 
   protected

app/models/group_user.rb

@@ -66,6 +66,7 @@ end
 #  user_id    :integer          not null
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
+#  owner      :boolean          default(FALSE), not null
 #
 # Indexes
 #

app/models/topic.rb

@@ -1062,6 +1062,7 @@ end
 #  auto_close_based_on_last_post :boolean          default(FALSE)
 #  auto_close_hours              :float
 #  pinned_until                  :datetime
+#  fancy_title                   :string(400)
 #
 # Indexes
 #

app/models/user.rb

@@ -52,9 +52,6 @@ class User < ActiveRecord::Base
   has_many :groups, through: :group_users
   has_many :secure_categories, through: :groups, source: :categories
 
-  has_many :group_managers, dependent: :destroy
-  has_many :managed_groups, through: :group_managers, source: :group
-
   has_many :muted_user_records, class_name: 'MutedUser'
   has_many :muted_users, through: :muted_user_records
 
@@ -1081,13 +1078,14 @@ end
 #  uploaded_avatar_id            :integer
 #  email_always                  :boolean          default(FALSE), not null
 #  mailing_list_mode             :boolean          default(FALSE), not null
-#  locale                        :string(10)
 #  primary_group_id              :integer
+#  locale                        :string(10)
 #  registration_ip_address       :inet
 #  last_redirected_to_top_at     :datetime
 #  disable_jump_reply            :boolean          default(FALSE), not null
 #  edit_history_public           :boolean          default(FALSE), not null
 #  trust_level_locked            :boolean          default(FALSE), not null
+#  staged                        :boolean          default(FALSE), not null
 #
 # Indexes
 #

app/models/user_profile_view.rb

@@ -45,3 +45,21 @@ class UserProfileView < ActiveRecord::Base
     profile_views.group("date(viewed_at)").order("date(viewed_at)").count
   end
 end
+
+# == Schema Information
+#
+# Table name: user_profile_views
+#
+#  id              :integer          not null, primary key
+#  user_profile_id :integer          not null
+#  viewed_at       :datetime         not null
+#  ip_address      :inet             not null
+#  user_id         :integer
+#
+# Indexes
+#
+#  index_user_profile_views_on_user_id          (user_id)
+#  index_user_profile_views_on_user_profile_id  (user_profile_id)
+#  unique_profile_view_ip                       (viewed_at,ip_address,user_profile_id) UNIQUE
+#  unique_profile_view_user                     (viewed_at,user_id,user_profile_id) UNIQUE
+#

config/locales/client.en.yml

@@ -330,6 +330,9 @@ en:
         other: "%{count} users"
 
     groups:
+      add: "Add"
+      selector_placeholder: "Add members"
+      owner: "owner"
       visible: "Group is visible to all users"
       title:
         one: "group"
@@ -1937,6 +1940,7 @@ en:
         delete_confirm: "Delete this group?"
         delete_failed: "Unable to delete group. If this is an automatic group, it cannot be destroyed."
         delete_member_confirm: "Remove '%{username}' from the '%{group}' group?"
+        delete_owner_confirm: "Remove owner privilege for '%{username}'?"
         name: "Name"
         add: "Add"
         add_members: "Add members"
@@ -1950,6 +1954,9 @@ en:
         automatic_membership_retroactive: "Apply the same email domain rule to add existing registered users"
         default_title: "Default title for all users in this group"
         primary_group: "Automatically set as primary group"
+        group_owners: Owners
+        add_owners: Add owners
+
 
       api:
         generate_master: "Generate Master API Key"

config/routes.rb

@@ -65,8 +65,8 @@ Discourse::Application.routes.draw do
         put 'bulk' => 'groups#bulk_perform'
       end
       member do
-        put "members" => "groups#add_members"
+        put "owners" => "groups#add_owners"
-        delete "members" => "groups#remove_member"
+        delete "owners" => "groups#remove_owner"
       end
     end
 
@@ -332,10 +332,16 @@ Discourse::Application.routes.draw do
     get 'posts'
     get 'counts'
 
-    put "members" => "groups#add_members"
+    member do
-    delete "members/:username" => "groups#remove_member"
+      put "members" => "groups#add_members"
+      delete "members" => "groups#remove_member"
+    end
   end
 
+  # aliases so old API code works
+  delete "admin/groups/:id/members" => "groups#remove_member", constraints: AdminConstraint.new
+  put "admin/groups/:id/members" => "groups#add_members", constraints: AdminConstraint.new
+
   # In case people try the wrong URL
   get '/group/:id', to: redirect('/groups/%{id}')
   get '/group/:id/members', to: redirect('/groups/%{id}/members')

db/migrate/20151107042241_add_owner_to_group_users.rb

@@ -0,0 +1,5 @@
+class AddOwnerToGroupUsers < ActiveRecord::Migration
+  def change
+    add_column :group_users, :owner, :boolean, null: false, default: false
+  end
+end

db/migrate/20151109124147_drop_group_managers.rb

@@ -0,0 +1,15 @@
+class DropGroupManagers < ActiveRecord::Migration
+  def up
+    # old data under old structure
+    execute "UPDATE group_users SET owner = true
+      WHERE exists (SELECT 1 FROM group_managers m
+                    WHERE m.group_id = group_users.group_id AND
+                          m.user_id = group_users.user_id)"
+
+    drop_table "group_managers"
+  end
+
+  def down
+    raise ActiveRecord::IrriversableMigration
+  end
+end

lib/guardian/group_guardian.rb

@@ -5,7 +5,7 @@ module GroupGuardian
   # Automatic groups are not represented in the GROUP_USERS
   # table and thus do not allow membership changes.
   def can_edit_group?(group)
-    (group.managers.include?(user) || is_admin?) && !group.automatic
+    (group.users.where('group_users.owner').include?(user) || is_admin?) && !group.automatic
   end
 
 end

spec/components/guardian_spec.rb

@@ -291,7 +291,7 @@ describe Guardian do
     let(:admin) { Fabricate(:admin) }
     let(:private_category)  { Fabricate(:private_category, group: group) }
     let(:group_private_topic) { Fabricate(:topic, category: private_category) }
-    let(:group_manager) { group_private_topic.user.tap { |u| group.add(u); group.appoint_manager(u) } }
+    let(:group_owner) { group_private_topic.user.tap { |u| group.add_owner(u) } }
 
     it 'handles invitation correctly' do
       expect(Guardian.new(nil).can_invite_to?(topic)).to be_falsey
@@ -324,8 +324,8 @@ describe Guardian do
       expect(Guardian.new(admin).can_invite_to?(private_topic)).to be_truthy
     end
 
-    it 'returns true for a group manager' do
+    it 'returns true for a group owner' do
-      expect(Guardian.new(group_manager).can_invite_to?(group_private_topic)).to be_truthy
+      expect(Guardian.new(group_owner).can_invite_to?(group_private_topic)).to be_truthy
     end
   end
 

spec/controllers/admin/groups_controller_spec.rb

@@ -127,92 +127,6 @@ describe Admin::GroupsController do
 
   end
 
-  context ".add_members" do
 
-    it "cannot add members to automatic groups" do
-      xhr :put, :add_members, id: 1, usernames: "l77t"
-      expect(response.status).to eq(422)
-    end
-
-    context "is able to add several members to a group" do
-
-      let(:user1) { Fabricate(:user) }
-      let(:user2) { Fabricate(:user) }
-      let(:group) { Fabricate(:group) }
-
-      it "adds by username" do
-        xhr :put, :add_members, id: group.id, usernames: [user1.username, user2.username].join(",")
-
-        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
-
-    it "returns 422 if member already exists" do
-      group = Fabricate(:group)
-      existing_member = Fabricate(:user)
-      group.add(existing_member)
-      group.save
-
-      xhr :put, :add_members, id: group.id, usernames: existing_member.username
-      expect(response.status).to eq(422)
-    end
-
-  end
-
-  context ".remove_member" do
-
-    it "cannot remove members from automatic groups" do
-      xhr :put, :remove_member, id: 1, user_id: 42
-      expect(response.status).to eq(422)
-    end
-
-    context "is able to remove a member" do
-
-      let(:user) { Fabricate(:user) }
-      let(:group) { Fabricate(:group) }
-
-      before do
-        group.add(user)
-        group.save
-      end
-
-      it "removes by id" do
-        xhr :delete, :remove_member, id: group.id, user_id: user.id
-
-        expect(response).to be_success
-        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
 
 end

spec/controllers/groups_controller_spec.rb

@@ -90,18 +90,18 @@ describe GroupsController do
     it "refuses membership changes to unauthorized users" do
       Guardian.any_instance.stubs(:can_edit?).with(group).returns(false)
 
-      xhr :put, :add_members, group_id: group.name, usernames: "bob"
+      xhr :put, :add_members, id: group.id, usernames: "bob"
       expect(response).to be_forbidden
 
-      xhr :delete, :remove_member, group_id: group.name, username: "bob"
+      xhr :delete, :remove_member, id: group.id, username: "bob"
       expect(response).to be_forbidden
     end
 
     it "cannot add members to automatic groups" do
       Guardian.any_instance.stubs(:is_admin?).returns(true)
-      auto_group = Fabricate(:group, name: "auto_group", automatic: true)
+      group = Fabricate(:group, name: "auto_group", automatic: true)
 
-      xhr :put, :add_members, group_id: group.name, usernames: "bob"
+      xhr :put, :add_members, id: group.id, usernames: "bob"
       expect(response).to be_forbidden
     end
   end
@@ -117,45 +117,117 @@ describe GroupsController do
 
     it "can make incremental adds" do
       user2 = Fabricate(:user)
-      xhr :put, :add_members, group_id: group.name, usernames: user2.username
+      xhr :put, :add_members, id: group.id, usernames: user2.username
 
       expect(response).to be_success
       group.reload
       expect(group.users.count).to eq(2)
     end
 
-    it "succeeds silently when adding non-existent users" do
-      xhr :put, :add_members, group_id: group.name, usernames: "nosuchperson"
-
-      expect(response).to be_success
-      group.reload
-      expect(group.users.count).to eq(1)
-    end
-
-    it "succeeds silently when adding duplicate users" do
-      xhr :put, :add_members, group_id: group.name, usernames: @user1.username
-
-      expect(response).to be_success
-      group.reload
-      expect(group.users).to eq([@user1])
-    end
-
     it "can make incremental deletes" do
-      xhr :delete, :remove_member, group_id: group.name, username: @user1.username
+      xhr :delete, :remove_member, id: group.id, username: @user1.username
 
       expect(response).to be_success
       group.reload
       expect(group.users.count).to eq(0)
     end
 
-    it "succeeds silently when removing non-members" do
+  end
-      user2 = Fabricate(:user)
-      xhr :delete, :remove_member, group_id: group.name, username: user2.username
 
-      expect(response).to be_success
+  context ".add_members" do
-      group.reload
+
-      expect(group.users.count).to eq(1)
+    before do
+      @admin = log_in(:admin)
     end
+
+    it "cannot add members to automatic groups" do
+      xhr :put, :add_members, id: 1, usernames: "l77t"
+      expect(response.status).to eq(403)
+    end
+
+    context "is able to add several members to a group" do
+
+      let(:user1) { Fabricate(:user) }
+      let(:user2) { Fabricate(:user) }
+      let(:group) { Fabricate(:group) }
+
+      it "adds by username" do
+        xhr :put, :add_members, id: group.id, usernames: [user1.username, user2.username].join(",")
+
+        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
+
+    it "returns 422 if member already exists" do
+      group = Fabricate(:group)
+      existing_member = Fabricate(:user)
+      group.add(existing_member)
+      group.save
+
+      xhr :put, :add_members, id: group.id, usernames: existing_member.username
+      expect(response.status).to eq(422)
+    end
+
+  end
+
+  context ".remove_member" do
+
+    before do
+      @admin = log_in(:admin)
+    end
+
+    it "cannot remove members from automatic groups" do
+      xhr :put, :remove_member, id: 1, user_id: 42
+      expect(response.status).to eq(403)
+    end
+
+    context "is able to remove a member" do
+
+      let(:user) { Fabricate(:user) }
+      let(:group) { Fabricate(:group) }
+
+      before do
+        group.add(user)
+        group.save
+      end
+
+      it "removes by id" do
+        xhr :delete, :remove_member, id: group.id, user_id: user.id
+
+        expect(response).to be_success
+        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
 
 end

spec/controllers/posts_controller_spec.rb

@@ -861,8 +861,7 @@ describe PostsController do
         Fabricate(:moderator)
 
         group = Fabricate(:group)
-        group.add(user)
+        group.add_owner(user)
-        group.appoint_manager(user)
 
         secured_category = Fabricate(:private_category, group: group)
         secured_post = create_post(user: user, category: secured_category)

spec/controllers/topics_controller_spec.rb

@@ -939,7 +939,7 @@ describe TopicsController do
 
     describe 'when logged in as group manager' do
       let(:group_manager) { log_in }
-      let(:group) { Fabricate(:group).tap { |g| g.add(group_manager); g.appoint_manager(group_manager) } }
+      let(:group) { Fabricate(:group).tap { |g| g.add_owner(group_manager) } }
       let(:private_category)  { Fabricate(:private_category, group: group) }
       let(:group_private_topic) { Fabricate(:topic, category: private_category, user: group_manager) }
       let(:recipient) { 'jake@adventuretime.ooo' }

spec/models/topic_spec.rb

@@ -1538,7 +1538,7 @@ describe Topic do
 
   context 'invite by group manager' do
     let(:group_manager) { Fabricate(:user) }
-    let(:group) { Fabricate(:group).tap { |g| g.add(group_manager); g.appoint_manager(group_manager) } }
+    let(:group) { Fabricate(:group).tap { |g| g.add_owner(group_manager) } }
     let(:private_category)  { Fabricate(:private_category, group: group) }
     let(:group_private_topic) { Fabricate(:topic, category: private_category, user: group_manager) }
 
@@ -1564,8 +1564,5 @@ describe Topic do
       end
     end
 
-    context 'to a previously-invited user' do
-
-    end
   end
 end

spec/models/user_spec.rb

@@ -986,22 +986,6 @@ describe User do
     end
   end
 
-  context "group management" do
-    let!(:user) { Fabricate(:user) }
-
-    it "by default has no managed groups" do
-      expect(user.managed_groups).to be_empty
-    end
-
-    it "can manage multiple groups" do
-      3.times do |i|
-        g = Fabricate(:group, name: "group_#{i}")
-        g.appoint_manager(user)
-      end
-      expect(user.managed_groups.count).to eq(3)
-    end
-  end
-
   describe "should_be_redirected_to_top" do
     let!(:user) { Fabricate(:user) }