diff --git a/app/assets/javascripts/discourse/app/components/group-member-dropdown.js b/app/assets/javascripts/discourse/app/components/group-member-dropdown.js index bd6fa1006a3..f8bff73391b 100644 --- a/app/assets/javascripts/discourse/app/components/group-member-dropdown.js +++ b/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) { diff --git a/app/assets/javascripts/discourse/app/controllers/group-index.js b/app/assets/javascripts/discourse/app/controllers/group-index.js index fd6826cd958..a1fa8632995 100644 --- a/app/assets/javascripts/discourse/app/controllers/group-index.js +++ b/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)); diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index fc1b4725fb8..f43812a8d3b 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/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) { diff --git a/app/assets/javascripts/discourse/app/templates/group-index.hbs b/app/assets/javascripts/discourse/app/templates/group-index.hbs index 926d895daa7..dc0dfa4809b 100644 --- a/app/assets/javascripts/discourse/app/templates/group-index.hbs +++ b/app/assets/javascripts/discourse/app/templates/group-index.hbs @@ -54,6 +54,7 @@ {{/if}} @@ -148,6 +149,7 @@ {{/if}} diff --git a/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs b/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs index 1986e243741..831ec8e81fd 100644 --- a/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs +++ b/app/assets/javascripts/discourse/app/templates/mobile/group-index.hbs @@ -44,6 +44,7 @@ {{/if}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js index 020a5a4463f..226775a6805 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/group-index-test.js +++ b/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 }); }); }); diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 4d9e581fd3c..1af73960fe5 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -44,35 +44,6 @@ class Admin::GroupsController < Admin::StaffController 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 diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 99f3634ee6c..4a772039f6f 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -385,6 +385,32 @@ class GroupsController < ApplicationController 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 @@ class GroupsController < ApplicationController 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) diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb index ab64df800a1..f5d4c80d365 100644 --- a/app/serializers/basic_group_serializer.rb +++ b/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 @@ class BasicGroupSerializer < ApplicationSerializer 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 diff --git a/config/routes.rb b/config/routes.rb index abdf514ef8d..8531638a997 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -112,7 +112,6 @@ Discourse::Application.routes.draw do 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 @@ Discourse::Application.routes.draw do 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" diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb index c09142449e2..5f101bf10bc 100644 --- a/spec/requests/admin/groups_controller_spec.rb +++ b/spec/requests/admin/groups_controller_spec.rb @@ -146,157 +146,6 @@ RSpec.describe Admin::GroupsController do 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) } diff --git a/spec/requests/api/schemas/json/group_create_response.json b/spec/requests/api/schemas/json/group_create_response.json index 2d77910bbeb..6c50cb851b9 100644 --- a/spec/requests/api/schemas/json/group_create_response.json +++ b/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" } diff --git a/spec/requests/api/schemas/json/group_response.json b/spec/requests/api/schemas/json/group_response.json index a663ba0e1b4..9163e33f9dc 100644 --- a/spec/requests/api/schemas/json/group_response.json +++ b/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" }, diff --git a/spec/requests/api/schemas/json/groups_list_response.json b/spec/requests/api/schemas/json/groups_list_response.json index cea82af0f2a..25fc1ac007b 100644 --- a/spec/requests/api/schemas/json/groups_list_response.json +++ b/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" } diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 0e108963aee..6b432c1b2a7 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -1638,6 +1638,164 @@ RSpec.describe GroupsController do end end + describe "#add_owners" do + context "when logged in as an admin" do + before { sign_in(admin) } + + it "should work" do + put "/groups/#{group.id}/owners.json", + params: { + 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 "/groups/#{group.id}/owners.json", params: { 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 "/groups/#{group.id}/owners.json", params: { usernames: user.username } + end.to_not change { group.group_users.count } + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to eq( + [I18n.t("groups.errors.can_not_modify_automatic")], + ) + end + + it "does not notify users when the param is not present" do + put "/groups/#{group.id}/owners.json", params: { 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 "/groups/#{group.id}/owners.json", + params: { + 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 "/groups/#{group.id}/owners.json", + params: { + 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 "/groups/#{group.id}/owners.json", + params: { + 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-owner" do + before { sign_in(user) } + + it "prevents adding of owners with a 403 response" do + put "/groups/#{group.id}/owners.json", + params: { + usernames: [user.username, admin.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 + + context "when logged in as an owner" do + before { sign_in(user) } + + it "allows adding new owners" do + group.add_owner(user) + + put "/groups/#{group.id}/owners.json", + params: { + usernames: [user.username, admin.username].join(","), + } + + expect(response.status).to eq(200) + expect(response.parsed_body["usernames"]).to contain_exactly( + user.username, + admin.username, + ) + expect(group.group_users.where(owner: true).map(&:user)).to contain_exactly(user, admin) + end + end + end + describe "#join" do let(:public_group) { Fabricate(:public_group) }