FEATURE: Allow group owners promote more owners (#19768)

This change allows group owners (in addition to admins) to promote other members to owners.

app/assets/javascripts/discourse/app/components/group-member-dropdown.js

@@ -43,6 +43,15 @@ export default DropdownSelectBoxComponent.extend({
           icon: "shield-alt",
         });
       }
+    } else if (this.canEditGroup && !this.member.owner) {
+      items.push({
+        id: "makeOwner",
+        name: I18n.t("groups.members.make_owner"),
+        description: I18n.t("groups.members.make_owner_description", {
+          username: this.get("member.username"),
+        }),
+        icon: "shield-alt",
+      });
     }
 
     if (this.currentUser.staff) {

app/assets/javascripts/discourse/app/controllers/group-index.js

@@ -134,17 +134,17 @@ export default Controller.extend({
       case "removeMembers":
         return ajax(`/groups/${this.model.id}/members.json`, {
           type: "DELETE",
-          data: { user_ids: selection.map((u) => u.id).join(",") },
+          data: { user_ids: selection.mapBy("id").join(",") },
         }).then(() => {
           this.model.reloadMembers(this.memberParams, true);
           this.set("isBulk", false);
         });
 
       case "makeOwners":
-        return ajax(`/admin/groups/${this.model.id}/owners.json`, {
+        return ajax(`/groups/${this.model.id}/owners.json`, {
           type: "PUT",
           data: {
-            group: { usernames: selection.map((u) => u.username).join(",") },
+            usernames: selection.mapBy("username").join(","),
           },
         }).then(() => {
           selection.forEach((s) => s.set("owner", true));

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

@@ -148,9 +148,9 @@ const Group = RestModel.extend({
   },
 
   async addOwners(usernames, filter, notifyUsers) {
-    const response = await ajax(`/admin/groups/${this.id}/owners.json`, {
+    const response = await ajax(`/groups/${this.id}/owners.json`, {
       type: "PUT",
-      data: { group: { usernames, notify_users: notifyUsers } },
+      data: { usernames, notify_users: notifyUsers },
     });
 
     if (filter) {

app/assets/javascripts/discourse/app/templates/group-index.hbs

@@ -54,6 +54,7 @@
                     <BulkGroupMemberDropdown
                       @bulkSelection={{this.bulkSelection}}
                       @canAdminGroup={{this.model.can_admin_group}}
+                      @canEditGroup={{this.model.can_edit_group}}
                       @onChange={{action "actOnSelection" this.bulkSelection}}
                     />
                   {{/if}}
@@ -148,6 +149,7 @@
                   <GroupMemberDropdown
                     @member={{m}}
                     @canAdminGroup={{this.model.can_admin_group}}
+                    @canEditGroup={{this.model.can_edit_group}}
                     @onChange={{action "actOnGroup" m}}
                   />
                 {{/if}}

app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs

@@ -44,6 +44,7 @@
                 <GroupMemberDropdown
                   @member={{user}}
                   @canAdminGroup={{this.model.can_admin_group}}
+                  @canEditGroup={{this.model.can_edit_group}}
                   @onChange={{action "actOnGroup" user}}
                 />
               {{/if}}

app/assets/javascripts/discourse/tests/acceptance/group-index-test.js

@@ -39,7 +39,7 @@ acceptance("Group Members", function (needs) {
   needs.user();
 
   needs.pretender((server, helper) => {
-    server.put("/admin/groups/47/owners.json", () => {
+    server.put("/groups/47/owners.json", () => {
       return helper.response({ success: true });
     });
   });

app/controllers/admin/groups_controller.rb

@@ -44,35 +44,6 @@ def destroy
     end
   end
 
-  def add_owners
-    group = Group.find_by(id: params.require(:id))
-    raise Discourse::NotFound unless group
-
-    return can_not_modify_automatic if group.automatic
-    guardian.ensure_can_edit_group!(group)
-
-    users = User.where(username: group_params[:usernames].split(","))
-
-    users.each do |user|
-      group_action_logger = GroupActionLogger.new(current_user, group)
-
-      if !group.users.include?(user)
-        group.add(user)
-        group_action_logger.log_add_user_to_group(user)
-      end
-      group.group_users.where(user_id: user.id).update_all(owner: true)
-      group_action_logger.log_make_user_group_owner(user)
-
-      if group_params[:notify_users] == "true" || group_params[:notify_users] == true
-        group.notify_added_to_group(user, owner: true)
-      end
-    end
-
-    group.restore_user_count!
-
-    render json: success_json.merge!(usernames: users.pluck(:username))
-  end
-
   def remove_owner
     group = Group.find_by(id: params.require(:id))
     raise Discourse::NotFound unless group

app/controllers/groups_controller.rb

@@ -385,6 +385,32 @@ def add_members
     end
   end
 
+  def add_owners
+    group = Group.find_by(id: params.require(:id))
+    raise Discourse::NotFound unless group
+
+    return can_not_modify_automatic if group.automatic
+    guardian.ensure_can_edit_group!(group)
+
+    users = users_from_params
+    group_action_logger = GroupActionLogger.new(current_user, group)
+
+    users.each do |user|
+      if !group.users.include?(user)
+        group.add(user)
+        group_action_logger.log_add_user_to_group(user)
+      end
+      group.group_users.where(user_id: user.id).update_all(owner: true)
+      group_action_logger.log_make_user_group_owner(user)
+
+      group.notify_added_to_group(user, owner: true) if params[:notify_users].to_s == "true"
+    end
+
+    group.restore_user_count!
+
+    render json: success_json.merge!(usernames: users.pluck(:username))
+  end
+
   def join
     ensure_logged_in
     unless current_user.staff?
@@ -667,6 +693,12 @@ def test_email_settings
     end
   end
 
+  protected
+
+  def can_not_modify_automatic
+    render_json_error(I18n.t("groups.errors.can_not_modify_automatic"))
+  end
+
   private
 
   def add_user_to_group(group, user, notify = false)

app/serializers/basic_group_serializer.rb

@@ -31,6 +31,7 @@ class BasicGroupSerializer < ApplicationSerializer
              :members_visibility_level,
              :can_see_members,
              :can_admin_group,
+             :can_edit_group,
              :publish_read_state
 
   def include_display_name?
@@ -73,6 +74,14 @@ def include_is_group_owner?
     owner_group_ids.present?
   end
 
+  def can_edit_group
+    scope.can_edit_group?(object)
+  end
+
+  def include_can_edit_group?
+    scope.can_edit_group?(object)
+  end
+
   def can_admin_group
     scope.can_admin_group?(object)
   end

config/routes.rb

@@ -112,7 +112,6 @@ def patch(*)
 
       resources :groups, only: [:create] do
         member do
-          put "owners" => "groups#add_owners"
           delete "owners" => "groups#remove_owner"
           put "primary" => "groups#set_primary"
         end
@@ -1052,6 +1051,7 @@ def patch(*)
 
           get "permissions" => "groups#permissions"
           put "members" => "groups#add_members"
+          put "owners" => "groups#add_owners"
           put "join" => "groups#join"
           delete "members" => "groups#remove_member"
           delete "leave" => "groups#leave"

spec/requests/admin/groups_controller_spec.rb

@@ -146,157 +146,6 @@
     end
   end
 
-  describe "#add_owners" do
-    context "when logged in as an admin" do
-      before { sign_in(admin) }
-
-      it "should work" do
-        put "/admin/groups/#{group.id}/owners.json",
-            params: {
-              group: {
-                usernames: [user.username, admin.username].join(","),
-              },
-            }
-
-        expect(response.status).to eq(200)
-
-        response_body = response.parsed_body
-
-        expect(response_body["usernames"]).to contain_exactly(user.username, admin.username)
-
-        expect(group.group_users.where(owner: true).map(&:user)).to contain_exactly(user, admin)
-      end
-
-      it "returns not-found error when there is no group" do
-        group.destroy!
-
-        put "/admin/groups/#{group.id}/owners.json", params: { group: { usernames: user.username } }
-
-        expect(response.status).to eq(404)
-      end
-
-      it "does not allow adding owners to an automatic group" do
-        group.update!(automatic: true)
-
-        expect do
-          put "/admin/groups/#{group.id}/owners.json",
-              params: {
-                group: {
-                  usernames: user.username,
-                },
-              }
-        end.to_not change { group.group_users.count }
-
-        expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"]).to eq(["You cannot modify an automatic group"])
-      end
-
-      it "does not notify users when the param is not present" do
-        put "/admin/groups/#{group.id}/owners.json", params: { group: { usernames: user.username } }
-        expect(response.status).to eq(200)
-
-        topic =
-          Topic.find_by(
-            title:
-              I18n.t(
-                "system_messages.user_added_to_group_as_owner.subject_template",
-                group_name: group.name,
-              ),
-            archetype: "private_message",
-          )
-        expect(topic.nil?).to eq(true)
-      end
-
-      it "notifies users when the param is present" do
-        put "/admin/groups/#{group.id}/owners.json",
-            params: {
-              group: {
-                usernames: user.username,
-                notify_users: true,
-              },
-            }
-        expect(response.status).to eq(200)
-
-        topic =
-          Topic.find_by(
-            title:
-              I18n.t(
-                "system_messages.user_added_to_group_as_owner.subject_template",
-                group_name: group.name,
-              ),
-            archetype: "private_message",
-          )
-        expect(topic.nil?).to eq(false)
-        expect(topic.topic_users.map(&:user_id)).to include(-1, user.id)
-      end
-    end
-
-    context "when logged in as a moderator" do
-      before { sign_in(moderator) }
-
-      context "with moderators_manage_categories_and_groups enabled" do
-        before { SiteSetting.moderators_manage_categories_and_groups = true }
-
-        it "adds owners" do
-          put "/admin/groups/#{group.id}/owners.json",
-              params: {
-                group: {
-                  usernames: [user.username, admin.username, moderator.username].join(","),
-                },
-              }
-
-          response_body = response.parsed_body
-
-          expect(response.status).to eq(200)
-          expect(response_body["usernames"]).to contain_exactly(
-            user.username,
-            admin.username,
-            moderator.username,
-          )
-          expect(group.group_users.where(owner: true).map(&:user)).to contain_exactly(
-            user,
-            admin,
-            moderator,
-          )
-        end
-      end
-
-      context "with moderators_manage_categories_and_groups disabled" do
-        before { SiteSetting.moderators_manage_categories_and_groups = false }
-
-        it "prevents adding of owners with a 403 response" do
-          put "/admin/groups/#{group.id}/owners.json",
-              params: {
-                group: {
-                  usernames: [user.username, admin.username, moderator.username].join(","),
-                },
-              }
-
-          expect(response.status).to eq(403)
-          expect(response.parsed_body["errors"]).to include(I18n.t("invalid_access"))
-          expect(group.group_users.where(owner: true).map(&:user)).to be_empty
-        end
-      end
-    end
-
-    context "when logged in as a non-staff user" do
-      before { sign_in(user) }
-
-      it "prevents adding of owners with a 404 response" do
-        put "/admin/groups/#{group.id}/owners.json",
-            params: {
-              group: {
-                usernames: [user.username, admin.username].join(","),
-              },
-            }
-
-        expect(response.status).to eq(404)
-        expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
-        expect(group.group_users.where(owner: true).map(&:user)).to be_empty
-      end
-    end
-  end
-
   describe "#remove_owner" do
     let(:user2) { Fabricate(:user) }
     let(:user3) { Fabricate(:user) }

spec/requests/api/schemas/json/group_create_response.json

@@ -119,6 +119,9 @@
         "can_admin_group": {
           "type": "boolean"
         },
+        "can_edit_group": {
+          "type": "boolean"
+        },
         "publish_read_state": {
           "type": "boolean"
         }

spec/requests/api/schemas/json/group_response.json

@@ -122,6 +122,9 @@
         "can_admin_group": {
           "type": "boolean"
         },
+        "can_edit_group": {
+          "type": "boolean"
+        },
         "publish_read_state": {
           "type": "boolean"
         },

spec/requests/api/schemas/json/groups_list_response.json

@@ -131,6 +131,9 @@
             "can_admin_group": {
               "type": "boolean"
             },
+            "can_edit_group": {
+              "type": "boolean"
+            },
             "publish_read_state": {
               "type": "boolean"
             }