diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6 index 57a3f4c8bd5..385ad7414e6 100644 --- a/app/assets/javascripts/discourse/controllers/group-index.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group-index.js.es6 @@ -1,6 +1,6 @@ import { popupAjaxError } from 'discourse/lib/ajax-error'; import Group from 'discourse/models/group'; -import { observes } from 'ember-addons/ember-computed-decorators'; +import { default as computed, observes } from 'ember-addons/ember-computed-decorators'; export default Ember.Controller.extend({ queryParams: ['order', 'desc'], @@ -17,6 +17,11 @@ export default Ember.Controller.extend({ this.get('model').findMembers({ order: this.get('order'), desc: this.get('desc') }); }, + @computed("model.public") + canJoinGroup(publicGroup) { + return !!(this.currentUser) && publicGroup; + }, + actions: { removeMember(user) { this.get('model').removeMember(user); @@ -29,6 +34,28 @@ export default Ember.Controller.extend({ } }, + joinGroup() { + this.set('updatingMembership', true); + const model = this.get('model'); + + model.addMembers(this.currentUser.get('username')).then(() => { + model.set('is_group_user', true); + }).catch(popupAjaxError).finally(() => { + this.set('updatingMembership', false); + }); + }, + + leaveGroup() { + this.set('updatingMembership', true); + const model = this.get('model'); + + model.removeMember(this.currentUser).then(() => { + model.set('is_group_user', false); + }).catch(popupAjaxError).finally(() => { + this.set('updatingMembership', false); + }); + }, + loadMore() { if (this.get("loading")) { return; } if (this.get("model.members.length") >= this.get("model.user_count")) { return; } diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 42f9575e1f7..9676cf18100 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -114,7 +114,8 @@ const Group = Discourse.Model.extend({ flair_url: this.get('flair_url'), flair_bg_color: this.get('flairBackgroundHexColor'), flair_color: this.get('flairHexColor'), - bio_raw: this.get('bio_raw') + bio_raw: this.get('bio_raw'), + public: this.get('public') }; }, diff --git a/app/assets/javascripts/discourse/templates/group-index.hbs b/app/assets/javascripts/discourse/templates/group-index.hbs index 7ccbf6ea1c6..8c62198e580 100644 --- a/app/assets/javascripts/discourse/templates/group-index.hbs +++ b/app/assets/javascripts/discourse/templates/group-index.hbs @@ -4,6 +4,20 @@ {{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"}} + {{else if canJoinGroup}} + {{#if model.is_group_user}} + {{d-button action="leaveGroup" + class="btn-danger group-index-leave" + icon="minus" + label="group.leave" + disabled=updatingMembership}} + {{else}} + {{d-button action="joinGroup" + class="group-index-join" + icon="plus" + label="group.join" + disabled=updatingMembership}} + {{/if}} {{/if}} {{#load-more selector=".group-members tr" action="loadMore"}} diff --git a/app/assets/javascripts/discourse/templates/modal/edit-group.hbs b/app/assets/javascripts/discourse/templates/modal/edit-group.hbs index 60a762eb7e9..9ae7f5682c5 100644 --- a/app/assets/javascripts/discourse/templates/modal/edit-group.hbs +++ b/app/assets/javascripts/discourse/templates/modal/edit-group.hbs @@ -1,4 +1,4 @@ -{{#d-modal-body title="group.edit.title" class="edit-group groups"}} +{{#d-modal-body title="group.title" class="edit-group groups"}}
{{/d-modal-body}} diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 08f2bc37e16..6b6de24a4fa 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -105,8 +105,7 @@ table.group-members { .group-flair-inputs { display: inline-block; - margin-top: 30px; - margin-bottom: 30px; + margin: 15px 0px; .group-flair-left { float: left; diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 4e439aec486..346c0ae5dad 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -107,16 +107,29 @@ class GroupsController < ApplicationController def add_members group = Group.find(params[:id]) - guardian.ensure_can_edit!(group) + group.public ? ensure_logged_in : guardian.ensure_can_edit!(group) - if params[:usernames].present? - 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') + users = + if params[:usernames].present? + User.where(username: params[:usernames].split(",")) + elsif params[:user_ids].present? + User.find(params[:user_ids].split(",")) + elsif params[:user_emails].present? + User.where(email: params[:user_emails].split(",")) + else + raise Discourse::InvalidParameters.new( + 'user_ids or usernames or user_emails must be present' + ) + end + + raise Discourse::NotFound if users.blank? + + if group.public + raise Discourse::InvalidAccess unless current_user == users.first + + unless current_user.staff? + RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed! + end end users.each do |user| @@ -146,16 +159,27 @@ class GroupsController < ApplicationController def remove_member group = Group.find(params[:id]) - guardian.ensure_can_edit!(group) + group.public ? ensure_logged_in : guardian.ensure_can_edit!(group) - if params[:user_id].present? - user = User.find(params[:user_id]) - elsif params[:username].present? - user = User.find_by_username(params[:username]) - elsif params[:user_email].present? - user = User.find_by_email(params[:user_email]) - else - raise Discourse::InvalidParameters.new('user_id or username must be present') + user = + if params[:user_id].present? + User.find_by(id: params[:user_id]) + elsif params[:username].present? + User.find_by_username(params[:username]) + elsif params[:user_email].present? + User.find_by_email(params[:user_email]) + else + raise Discourse::InvalidParameters.new('user_id or username must be present') + end + + raise Discourse::NotFound unless user + + if group.public + raise Discourse::InvalidAccess unless current_user == user + + unless current_user.staff? + RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed! + end end user.primary_group_id = nil if user.primary_group_id == group.id @@ -189,7 +213,8 @@ class GroupsController < ApplicationController :flair_bg_color, :flair_color, :bio_raw, - :title + :title, + :public ) end diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb index 401a88b2ff1..a4f1e4a2d0c 100644 --- a/app/serializers/basic_group_serializer.rb +++ b/app/serializers/basic_group_serializer.rb @@ -16,7 +16,8 @@ class BasicGroupSerializer < ApplicationSerializer :flair_bg_color, :flair_color, :bio_raw, - :bio_cooked + :bio_cooked, + :public def include_incoming_email? staff? diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 820734bf5c9..f20e87a3fbd 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1848,9 +1848,12 @@ en: group: edit: title: 'Edit Group' + join: "Join Group" + leave: "Leave Group" title: 'Title' name: "Name" bio: "About Group" + public: "Allow users to join/leave the group freely" name_placeholder: "Group name, no spaces, same as username rule" flair_url: "Avatar Flair Image" flair_url_placeholder: "(Optional) Image URL or Font Awesome class" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e5a78f46506..766a77124dd 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -443,6 +443,7 @@ en: create_topic: "You're creating topics too quickly. Please wait %{time_left} before trying again." create_post: "You're replying too quickly. Please wait %{time_left} before trying again." delete_post: "You're deleting posts too quickly. Please wait %{time_left} before trying again." + public_group_membership: "You're joining/leaving groups too frequently. Please wait %{time_left} before trying again." topics_per_day: "You've reached the maximum number of new topics today. Please wait %{time_left} before trying again." pms_per_day: "You've reached the maximum number of messages today. Please wait %{time_left} before trying again." create_like: "You've reached the maximum number of likes today. Please wait %{time_left} before trying again." diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 53cb83436a4..d3aa8418251 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -39,8 +39,10 @@ describe Admin::GroupsController do "flair_bg_color"=>nil, "flair_color"=>nil, "bio_raw"=>nil, - "bio_cooked"=>nil + "bio_cooked"=>nil, + "public"=>false }]) + end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index de32060f6c2..aeaa1c61f31 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -69,162 +69,6 @@ describe GroupsController do end end - - describe "membership edit permission" do - it "refuses membership changes to unauthorized users" do - Guardian.any_instance.stubs(:can_edit?).with(group).returns(false) - - xhr :put, :add_members, id: group.id, usernames: "bob" - expect(response).to be_forbidden - - 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) - group = Fabricate(:group, name: "auto_group", automatic: true) - - xhr :put, :add_members, id: group.id, usernames: "bob" - expect(response).to be_forbidden - end - end - - describe "membership edits" do - before do - @user1 = Fabricate(:user) - group.add(@user1) - group.reload - - Guardian.any_instance.stubs(:can_edit?).with(group).returns(true) - end - - it "can make incremental adds" do - user2 = Fabricate(:user) - 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 "can make incremental deletes" do - 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 - - end - - context ".add_members" do - - 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 - expect do - xhr :delete, :remove_member, id: group.id, user_id: user.id - - expect(response).to be_success - group.reload - end.to change{group.users.count}.from(1).to(0) - end - - it "removes by username" do - expect do - xhr :delete, :remove_member, id: group.id, username: user.username - - expect(response).to be_success - group.reload - - end.to change{group.users.count}.from(1).to(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 - - it "removes by user_email" do - expect do - xhr :delete, :remove_member, id: group.id, user_email: user.email - expect(response).to be_success - group.reload - end.to change{group.users.count}.from(1).to(0) - end - end - - end - describe '.posts_feed' do it 'renders RSS' do get :posts_feed, group_id: group.name, format: :rss diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb index 6858e1ac5a0..3ca20455f9c 100644 --- a/spec/integration/groups_spec.rb +++ b/spec/integration/groups_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' describe "Groups" do let(:user) { Fabricate(:user) } + let(:group) { Fabricate(:group, users: [user]) } def sign_in(user) password = 'somecomplicatedpassword' @@ -12,11 +13,9 @@ describe "Groups" do end describe "checking if a group can be mentioned" do - let(:group) { Fabricate(:group, name: 'test', users: [user]) } - it "should return the right response" do sign_in(user) - group + group.update_attributes!(name: 'test') get "/groups/test/mentionable.json", { name: group.name } @@ -36,7 +35,11 @@ describe "Groups" do end describe "group can be updated" do - let(:group) { Fabricate(:group, name: 'test', users: [user]) } + let(:group) { Fabricate(:group, name: 'test', users: [user], public: false) } + + before do + sign_in(user) + end context "when user is group owner" do before do @@ -50,7 +53,8 @@ describe "Groups" do flair_color: 'BBB', flair_url: 'fa-adjust', bio_raw: 'testing', - title: 'awesome team' + title: 'awesome team', + public: true } } expect(response).to be_success @@ -62,6 +66,7 @@ describe "Groups" do expect(group.flair_url).to eq('fa-adjust') expect(group.bio_raw).to eq('testing') expect(group.title).to eq('awesome team') + expect(group.public).to eq(true) end end @@ -145,4 +150,220 @@ describe "Groups" do expect(members.map { |m| m["id"] }).to eq([user1.id, user2.id]) end end + + describe "membership edit permissions" do + let(:group) { Fabricate(:group) } + + context 'when user is not signed in' do + it 'should be fobidden' do + xhr :put, "/groups/#{group.id}/members", usernames: "bob" + expect(response).to be_forbidden + + xhr :delete, "/groups/#{group.id}/members", username: "bob" + expect(response).to be_forbidden + end + + context 'public group' do + it 'should be fobidden' do + group.update_attributes!(public: true) + + expect { xhr :put, "/groups/#{group.id}/members", usernames: "bob" } + .to raise_error(Discourse::NotLoggedIn) + + expect { xhr :delete, "/groups/#{group.id}/members", username: "bob" } + .to raise_error(Discourse::NotLoggedIn) + end + end + end + + context 'when user is not an owner of the group' do + before do + sign_in(user) + end + + it "refuses membership changes to unauthorized users" do + xhr :put, "/groups/#{group.id}/members", usernames: "bob" + expect(response).to be_forbidden + + xhr :delete, "/groups/#{group.id}/members", username: "bob" + expect(response).to be_forbidden + end + end + + context 'when user is an admin' do + let(:user) { Fabricate(:admin) } + let(:group) { Fabricate(:group, users: [user], automatic: true) } + + before do + sign_in(user) + end + + it "cannot add members to automatic groups" do + xhr :put, "/groups/#{group.id}/members", usernames: "bob" + expect(response).to be_forbidden + + xhr :delete, "/groups/#{group.id}/members", username: "bob" + expect(response).to be_forbidden + end + end + end + + describe "membership edits" do + let(:admin) { Fabricate(:admin) } + + before do + sign_in(admin) + end + + context 'adding members' do + it "can make incremental adds" do + user2 = Fabricate(:user) + expect(group.users.count).to eq(1) + + xhr :put, "/groups/#{group.id}/members", usernames: user2.username + + expect(response).to be_success + expect(group.reload.users.count).to eq(2) + end + + it "can make incremental deletes" do + expect(group.users.count).to eq(1) + + xhr :delete, "/groups/#{group.id}/members", username: user.username + + expect(response).to be_success + expect(group.reload.users.count).to eq(0) + end + + it "cannot add members to automatic groups" do + group.update!(automatic: true) + + xhr :put, "/groups/#{group.id}/members", 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) } + + it "adds by username" do + expect { xhr :put, "/groups/#{group.id}/members", usernames: [user1.username, user2.username].join(",") } + .to change { group.users.count }.by(2) + + expect(response).to be_success + end + + it "adds by id" do + expect { xhr :put, "/groups/#{group.id}/members", user_ids: [user1.id, user2.id].join(",") } + .to change { group.users.count }.by(2) + + expect(response).to be_success + end + end + + it "returns 422 if member already exists" do + xhr :put, "/groups/#{group.id}/members", usernames: user.username + + expect(response.status).to eq(422) + end + + it "returns 404 if member is not found" do + xhr :put, "/groups/#{group.id}/members", usernames: 'some donkey' + + expect(response.status).to eq(404) + end + + context 'public group' do + let(:other_user) { Fabricate(:user) } + + before do + group.update!(public: true) + end + + it 'should allow a user to join the group' do + sign_in(other_user) + + expect { xhr :put, "/groups/#{group.id}/members", usernames: other_user.username } + .to change { group.users.count }.by(1) + + expect(response).to be_success + end + + it 'should not allow a user to add another user to a group' do + xhr :put, "/groups/#{group.id}/members", usernames: other_user.username + + expect(response).to be_forbidden + end + end + end + + context 'removing members' do + it "cannot remove members from automatic groups" do + group.update!(automatic: true) + + xhr :delete, "/groups/#{group.id}/members", user_id: 42 + expect(response.status).to eq(403) + end + + it "raises an error if user to be removed is not found" do + xhr :delete, "/groups/#{group.id}/members", user_id: -10 + expect(response.status).to eq(404) + end + + context "is able to remove a member" do + it "removes by id" do + expect { xhr :delete, "/groups/#{group.id}/members", user_id: user.id } + .to change { group.users.count }.by(-1) + + expect(response).to be_success + end + + it "removes by username" do + expect { xhr :delete, "/groups/#{group.id}/members", username: user.username } + .to change { group.users.count }.by(-1) + + expect(response).to be_success + end + + it "removes user.primary_group_id when user is removed from group" do + user.update!(primary_group_id: group.id) + + xhr :delete, "/groups/#{group.id}/members", user_id: user.id + + expect(user.reload.primary_group_id).to eq(nil) + end + + it "removes by user_email" do + expect { xhr :delete, "/groups/#{group.id}/members", user_email: user.email } + .to change { group.users.count }.by(-1) + + expect(response).to be_success + end + + context 'public group' do + let(:other_user) { Fabricate(:user) } + let(:group) { Fabricate(:group, users: [other_user]) } + + before do + group.update!(public: true) + end + + it 'should allow a user to leave a group' do + sign_in(other_user) + + expect { xhr :delete, "/groups/#{group.id}/members", username: other_user.username } + .to change { group.users.count }.by(-1) + + expect(response).to be_success + end + + it 'should not allow a user to leave a group for another user' do + xhr :delete, "/groups/#{group.id}/members", username: other_user.username + + expect(response).to be_forbidden + end + end + end + end + end end diff --git a/test/javascripts/acceptance/groups-test.js.es6 b/test/javascripts/acceptance/groups-test.js.es6 index bc6a6ee44a1..9ebe4826943 100644 --- a/test/javascripts/acceptance/groups-test.js.es6 +++ b/test/javascripts/acceptance/groups-test.js.es6 @@ -50,5 +50,6 @@ test("Admin Browsing Groups", () => { ok(find('.group-flair-inputs').length === 1, 'it should display avatar flair inputs'); ok(find('.edit-group-bio').length === 1, 'it should display group bio input'); ok(find('.edit-group-title').length === 1, 'it should display group title input'); + ok(find('.edit-group-public').length === 1, 'it should display group public input'); }); }); diff --git a/test/javascripts/controllers/group-index-test.js.es6 b/test/javascripts/controllers/group-index-test.js.es6 new file mode 100644 index 00000000000..d3ac38a18a0 --- /dev/null +++ b/test/javascripts/controllers/group-index-test.js.es6 @@ -0,0 +1,21 @@ +import { currentUser } from "helpers/qunit-helpers"; + +moduleFor("controller:group-index"); + +test("canJoinGroup", function() { + this.subject().setProperties({ + model: { public: false } + }); + + this.subject().set("currentUser", currentUser()); + + equal(this.subject().get("canJoinGroup"), false, "non public group cannot be joined"); + + this.subject().set("model.public", true); + + equal(this.subject().get("canJoinGroup"), true, "public group can be joined"); + + this.subject().setProperties({ currentUser: null, model: { public: true } }); + + equal(this.subject().get("canJoinGroup"), false, "can't join group when not logged in"); +});