diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index bb8500f08d9..3966e69905a 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -231,7 +231,11 @@ export default Ember.Controller.extend(SelectedPostsCount, BufferedContent, { }, removeAllowedUser(user) { - return this.get('model.details').removeAllowedUser(user); + return this.get('model.details').removeAllowedUser(user).then(() => { + if (this.currentUser.id === user.id) { + this.transitionToRoute("userPrivateMessages", user); + } + }); }, removeAllowedGroup(group) { diff --git a/app/assets/javascripts/discourse/lib/transform-post.js.es6 b/app/assets/javascripts/discourse/lib/transform-post.js.es6 index 5faac346ece..ac81e64e682 100644 --- a/app/assets/javascripts/discourse/lib/transform-post.js.es6 +++ b/app/assets/javascripts/discourse/lib/transform-post.js.es6 @@ -127,6 +127,7 @@ export default function transformPost(currentUser, site, post, prevPost, nextPos postAtts.allowedGroups = details.allowed_groups; postAtts.allowedUsers = details.allowed_users; postAtts.canRemoveAllowedUsers = details.can_remove_allowed_users; + postAtts.canRemoveSelfId = details.can_remove_self_id; postAtts.canInvite = details.can_invite_to; } diff --git a/app/assets/javascripts/discourse/widgets/post-small-action.js.es6 b/app/assets/javascripts/discourse/widgets/post-small-action.js.es6 index 2f26fac0986..8f7113727df 100644 --- a/app/assets/javascripts/discourse/widgets/post-small-action.js.es6 +++ b/app/assets/javascripts/discourse/widgets/post-small-action.js.es6 @@ -48,6 +48,7 @@ const icons = { 'split_topic': 'sign-out', 'invited_user': 'plus-circle', 'invited_group': 'plus-circle', + 'user_left': 'minus-circle', 'removed_user': 'minus-circle', 'removed_group': 'minus-circle', 'public_topic': 'comment', diff --git a/app/assets/javascripts/discourse/widgets/private-message-map.js.es6 b/app/assets/javascripts/discourse/widgets/private-message-map.js.es6 index fc8b505c28f..3e3eba852b0 100644 --- a/app/assets/javascripts/discourse/widgets/private-message-map.js.es6 +++ b/app/assets/javascripts/discourse/widgets/private-message-map.js.es6 @@ -36,8 +36,12 @@ createWidget('pm-remove-link', { template: hbs`{{d-icon "times"}}`, click() { - bootbox.confirm(I18n.t("private_message_info.remove_allowed_user", {name: this.attrs.username}), (confirmed) => { - if (confirmed) { this.sendWidgetAction('removeAllowedUser', this.attrs); } + const messageKey = this.attrs.isCurrentUser ? 'leave_message' : 'remove_allowed_user'; + + bootbox.confirm(I18n.t(`private_message_info.${messageKey}`, { name: this.attrs.user.username }), confirmed => { + if (confirmed) { + this.sendWidgetAction('removeAllowedUser', this.attrs.user); + } }); } }); @@ -49,11 +53,12 @@ createWidget('pm-map-user', { const user = attrs.user; const avatar = avatarFor('small', { template: user.avatar_template, username: user.username }); const link = h('a', { attributes: { href: user.get('path') } }, [ avatar, ' ', user.username ]); - const result = [link]; - if (attrs.canRemoveAllowedUsers) { + const isCurrentUser = attrs.canRemoveSelfId === user.get('id'); + + if (attrs.canRemoveAllowedUsers || isCurrentUser) { result.push(' '); - result.push(this.attach('pm-remove-link', user)); + result.push(this.attach('pm-remove-link', { user, isCurrentUser } )); } return result; @@ -67,12 +72,23 @@ export default createWidget('private-message-map', { const participants = []; if (attrs.allowedGroups.length) { - participants.push(attrs.allowedGroups.map(ag => this.attach('pm-map-user-group', {group: ag, canRemoveAllowedUsers: attrs.canRemoveAllowedUsers}))); + participants.push(attrs.allowedGroups.map(ag => { + this.attach('pm-map-user-group', { + group: ag, + canRemoveAllowedUsers: attrs.canRemoveAllowedUsers + }); + })); } - if (attrs.allowedUsers.length) { + const allowedUsersLength = attrs.allowedUsers.length; + + if (allowedUsersLength) { participants.push(attrs.allowedUsers.map(au => { - return this.attach('pm-map-user', { user: au, canRemoveAllowedUsers: attrs.canRemoveAllowedUsers }); + return this.attach('pm-map-user', { + user: au, + canRemoveAllowedUsers: attrs.canRemoveAllowedUsers, + canRemoveSelfId: attrs.canRemoveSelfId + }); })); } diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 631a6158240..69717e3587c 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -439,9 +439,10 @@ class TopicsController < ApplicationController def remove_allowed_user params.require(:username) topic = Topic.find_by(id: params[:topic_id]) - guardian.ensure_can_remove_allowed_users!(topic) + user = User.find_by(username: params[:username]) + guardian.ensure_can_remove_allowed_users!(topic, user) - if topic.remove_allowed_user(current_user, params[:username]) + if topic.remove_allowed_user(current_user, user) render json: success_json else render json: failed_json, status: 422 diff --git a/app/models/topic.rb b/app/models/topic.rb index f4093cb8f5c..9cbaed9ee73 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -709,14 +709,21 @@ SQL end def remove_allowed_user(removed_by, username) - if user = User.find_by(username: username) + user = username.is_a?(User) ? username : User.find_by(username: username) + + if user topic_user = topic_allowed_users.find_by(user_id: user.id) + if topic_user topic_user.destroy - # we can not remove ourselves cause then we will end up adding - # ourselves in add_small_action - removed_by = Discourse.system_user if user.id == removed_by&.id - add_small_action(removed_by, "removed_user", user.username) + + if user.id == removed_by&.id + removed_by = Discourse.system_user + add_small_action(removed_by, "user_left", user.username) + else + add_small_action(removed_by, "removed_user", user.username) + end + return true end end diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index df70f3592bd..552aeeb25f7 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -114,6 +114,7 @@ class TopicViewSerializer < ApplicationSerializer result[:can_delete] = true if scope.can_delete?(object.topic) result[:can_recover] = true if scope.can_recover_topic?(object.topic) result[:can_remove_allowed_users] = true if scope.can_remove_allowed_users?(object.topic) + result[:can_remove_self_id] = scope.user.id if scope.can_remove_allowed_users?(object.topic, scope.user) result[:can_invite_to] = true if scope.can_invite_to?(object.topic) result[:can_invite_via_email] = true if scope.can_invite_via_email?(object.topic) result[:can_create_post] = true if scope.can_create?(Post, object.topic) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5c78efd4f02..d5e17d752d7 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -143,6 +143,7 @@ en: split_topic: "split this topic %{when}" invited_user: "invited %{who} %{when}" invited_group: "invited %{who} %{when}" + user_left: "%{who} left this message %{when}" removed_user: "removed %{who} %{when}" removed_group: "removed %{who} %{when}" autoclosed: @@ -1030,6 +1031,7 @@ en: private_message_info: title: "Message" invite: "Invite Others..." + leave_message: "Do you really want to leave this message?" remove_allowed_user: "Do you really want to remove {{name}} from this message?" remove_allowed_group: "Do you really want to remove {{name}} from this message?" diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index b8db72d3158..1bf07ef82ca 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -1,8 +1,13 @@ #mixin for all guardian methods dealing with topic permisions module TopicGuardian - def can_remove_allowed_users?(topic) - is_staff? + def can_remove_allowed_users?(topic, target_user = nil) + is_staff? || + ( + topic.allowed_users.count > 1 && + topic.user != target_user && + !!(target_user && user == target_user) + ) end # Creating Methods diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 04496841318..d0af49efb97 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -2613,4 +2613,60 @@ describe Guardian do end end end + + describe '#can_remove_allowed_users?' do + context 'staff users' do + it 'should be true' do + expect(Guardian.new(moderator).can_remove_allowed_users?(topic)) + .to eq(true) + end + end + + context 'normal user' do + let(:topic) { Fabricate(:topic, user: Fabricate(:user)) } + let(:another_user) { Fabricate(:user) } + + before do + topic.allowed_users << user + topic.allowed_users << another_user + end + + it 'should be false' do + expect(Guardian.new(user).can_remove_allowed_users?(topic)) + .to eq(false) + end + + describe 'target_user is the user' do + describe 'when user is in a pm with another user' do + it 'should return true' do + expect(Guardian.new(user).can_remove_allowed_users?(topic, user)) + .to eq(true) + end + end + + describe 'when user is the creator of the topic' do + it 'should return false' do + expect(Guardian.new(topic.user).can_remove_allowed_users?(topic, topic.user)) + .to eq(false) + end + end + + describe 'when user is the only user in the topic' do + it 'should return false' do + topic.remove_allowed_user(Discourse.system_user, another_user.username) + + expect(Guardian.new(user).can_remove_allowed_users?(topic, user)) + .to eq(false) + end + end + end + + describe 'target_user is not the user' do + it 'should return false' do + expect(Guardian.new(user).can_remove_allowed_users?(topic, moderator)) + .to eq(false) + end + end + end + end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 706b2ee4970..6bb619fbce3 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -6,6 +6,7 @@ require_dependency 'post_destroyer' describe Topic do let(:now) { Time.zone.local(2013, 11, 20, 8, 0) } let(:user) { Fabricate(:user) } + let(:topic) { Fabricate(:topic) } context 'validations' do let(:topic) { Fabricate.build(:topic) } @@ -2040,4 +2041,23 @@ describe Topic do end end end + + describe '#remove_allowed_user' do + let(:another_user) { Fabricate(:user) } + + describe 'removing oneself' do + it 'should remove onself' do + topic.allowed_users << another_user + + expect(topic.remove_allowed_user(another_user, another_user)).to eq(true) + expect(topic.allowed_users.include?(another_user)).to eq(false) + + post = Post.last + + expect(post.user).to eq(Discourse.system_user) + expect(post.post_type).to eq(Post.types[:small_action]) + expect(post.action_code).to eq('user_left') + end + end + end end