From 79de10b212f1f3d63d39d52a2f330eb957e3d943 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 6 Oct 2017 15:56:58 +0800 Subject: [PATCH 1/2] FEATURE: Allow users to disable new PMs. https://meta.discourse.org/t/is-it-possible-to-disable-private-messaging-for-a-specific-user/46391 --- .../controllers/preferences/interface.js.es6 | 3 ++- .../javascripts/discourse/models/user.js.es6 | 3 ++- .../templates/preferences/interface.hbs | 10 ++++++++ app/models/user_option.rb | 13 ++++++---- app/serializers/user_option_serializer.rb | 3 ++- app/services/user_updater.rb | 3 ++- config/locales/client.en.yml | 1 + ..._allow_private_messages_to_user_options.rb | 5 ++++ lib/guardian.rb | 9 +++++-- lib/post_creator.rb | 14 +++++++++-- lib/topic_creator.rb | 2 +- spec/components/guardian_spec.rb | 21 ++++++++++++++++ spec/components/post_creator_spec.rb | 24 +++++++++++++++++++ spec/services/user_updater_spec.rb | 6 +++-- 14 files changed, 101 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20171006030028_add_allow_private_messages_to_user_options.rb diff --git a/app/assets/javascripts/discourse/controllers/preferences/interface.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/interface.js.es6 index d49a6ffd6c0..6a7094886bf 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/interface.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences/interface.js.es6 @@ -13,7 +13,8 @@ export default Ember.Controller.extend(PreferencesTabController, { 'dynamic_favicon', 'enable_quoting', 'disable_jump_reply', - 'automatically_unpin_topics' + 'automatically_unpin_topics', + 'allow_private_messages', ]; if (makeDefault) { diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index b8adf073d89..3296c5fbe7c 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -247,7 +247,8 @@ const User = RestModel.extend({ 'notification_level_when_replying', 'like_notification_frequency', 'include_tl0_in_digests', - 'theme_key' + 'theme_key', + 'allow_private_messages', ]; if (fields) { diff --git a/app/assets/javascripts/discourse/templates/preferences/interface.hbs b/app/assets/javascripts/discourse/templates/preferences/interface.hbs index 99ad7891fee..4096ef0ad6a 100644 --- a/app/assets/javascripts/discourse/templates/preferences/interface.hbs +++ b/app/assets/javascripts/discourse/templates/preferences/interface.hbs @@ -10,6 +10,16 @@ {{/if}} +
+ + +
+ {{preference-checkbox + labelKey="user.allow_private_messages" + checked=model.user_option.allow_private_messages}} +
+
+ {{#if siteSettings.allow_user_locale}}
diff --git a/app/models/user_option.rb b/app/models/user_option.rb index 6ce2f12ef5e..17eed6baa7f 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -59,11 +59,6 @@ class UserOption < ActiveRecord::Base super end - def update_tracked_topics - return unless saved_change_to_auto_track_topics_after_msecs? - TrackedTopicsUpdater.new(id, auto_track_topics_after_msecs).call - end - def redirected_to_top_yet? last_redirected_to_top_at.present? end @@ -133,6 +128,13 @@ class UserOption < ActiveRecord::Base times.max end + private + + def update_tracked_topics + return unless saved_change_to_auto_track_topics_after_msecs? + TrackedTopicsUpdater.new(id, auto_track_topics_after_msecs).call + end + end # == Schema Information @@ -162,6 +164,7 @@ end # notification_level_when_replying :integer # theme_key :string # theme_key_seq :integer default(0), not null +# allow_private_messages :boolean default(TRUE), not null # # Indexes # diff --git a/app/serializers/user_option_serializer.rb b/app/serializers/user_option_serializer.rb index b8253b40d78..844f6c06b5c 100644 --- a/app/serializers/user_option_serializer.rb +++ b/app/serializers/user_option_serializer.rb @@ -20,7 +20,8 @@ class UserOptionSerializer < ApplicationSerializer :like_notification_frequency, :include_tl0_in_digests, :theme_key, - :theme_key_seq + :theme_key_seq, + :allow_private_messages, def auto_track_topics_after_msecs object.auto_track_topics_after_msecs || SiteSetting.default_other_auto_track_topics_after_msecs diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 8b36ce94252..d7f764e7b04 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -34,7 +34,8 @@ class UserUpdater :email_in_reply_to, :like_notification_frequency, :include_tl0_in_digests, - :theme_key + :theme_key, + :allow_private_messages, ] def initialize(actor, user) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index db8bf8bd1b0..5c78efd4f02 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -595,6 +595,7 @@ en: disable_jump_reply: "Don't jump to my post after I reply" dynamic_favicon: "Show new / updated topic count on browser icon" theme_default_on_all_devices: "Make this my default theme on all my devices" + allow_private_messages: "Allow other users to send me private messages" external_links_in_new_tab: "Open all external links in a new tab" enable_quoting: "Enable quote reply for highlighted text" change: "change" diff --git a/db/migrate/20171006030028_add_allow_private_messages_to_user_options.rb b/db/migrate/20171006030028_add_allow_private_messages_to_user_options.rb new file mode 100644 index 00000000000..c74f68fff84 --- /dev/null +++ b/db/migrate/20171006030028_add_allow_private_messages_to_user_options.rb @@ -0,0 +1,5 @@ +class AddAllowPrivateMessagesToUserOptions < ActiveRecord::Migration[5.1] + def change + add_column :user_options, :allow_private_messages, :boolean, default: true, null: false + end +end diff --git a/lib/guardian.rb b/lib/guardian.rb index 2bc13f9e683..00bad7075cb 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -286,17 +286,22 @@ class Guardian end def can_send_private_message?(target) - (target.is_a?(Group) || target.is_a?(User)) && + is_user = target.is_a?(User) + is_group = target.is_a?(Group) + + (is_group || is_user) && # User is authenticated authenticated? && # Have to be a basic level at least, or are contacting moderators (@user.has_trust_level?(SiteSetting.min_trust_to_send_messages) || (target.is_a?(User) && target.moderator?) || (target.name == Group[:moderators].name)) && + # User disabled private message + (is_staff? || is_group || target.user_option.allow_private_messages) && # PMs are enabled (is_staff? || SiteSetting.enable_private_messages) && # Can't send PMs to suspended users - (is_staff? || target.is_a?(Group) || !target.suspended?) && + (is_staff? || is_group || !target.suspended?) && # Blocked users can only send PM to staff (!is_blocked? || target.staff?) end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index dc28b66bce0..be0e6a80260 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -109,10 +109,20 @@ class PostCreator # Make sure none of the users have muted the creator users = User.where(username: names).pluck(:id, :username).to_h - MutedUser.where(user_id: users.keys, muted_user_id: @user.id).pluck(:user_id).each do |m| + User + .joins("LEFT JOIN user_options ON user_options.user_id = users.id") + .joins("LEFT JOIN muted_users ON muted_users.muted_user_id = #{@user.id.to_i}") + .where("user_options.user_id IS NOT NULL") + .where(" + (user_options.user_id IN (:user_ids) AND NOT user_options.allow_private_messages) OR + muted_users.user_id IN (:user_ids) + ", user_ids: users.keys) + .pluck(:id).each do |m| + errors[:base] << I18n.t(:not_accepting_pms, username: users[m]) - return false end + + return false if errors[:base].present? end if new_topic? diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 36c20247c02..69ca76d73cd 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -190,7 +190,7 @@ class TopicCreator names = usernames.split(',').flatten len = 0 - User.where(username: names).each do |user| + User.includes(:user_option).where(username: names).find_each do |user| check_can_send_permission!(topic, user) @added_users << user topic.topic_allowed_users.build(user_id: user.id) diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index c2674e38ea7..04496841318 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -233,6 +233,27 @@ describe Guardian do end end end + + context 'target user has private message disabled' do + before do + another_user.user_option.update!(allow_private_messages: false) + end + + context 'for a normal user' do + it 'should return false' do + expect(Guardian.new(user).can_send_private_message?(another_user)).to eq(false) + end + end + + context 'for a staff user' do + it 'should return true' do + [admin, moderator].each do |staff_user| + expect(Guardian.new(staff_user).can_send_private_message?(another_user)) + .to eq(true) + end + end + end + end end describe 'can_reply_as_new_topic' do diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index a2e4908dbd8..f3d2e8350ae 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -954,6 +954,30 @@ describe PostCreator do end end + context 'private message to a user that has disabled private messages' do + let(:another_user) { Fabricate(:user) } + + before do + another_user.user_option.update!(allow_private_messages: false) + end + + it 'should not be valid' do + post_creator = PostCreator.new( + user, + title: 'this message is to someone who muted me!', + raw: "you will have to see this even if you muted me!", + archetype: Archetype.private_message, + target_usernames: "#{another_user.username}" + ) + + expect(post_creator).to_not be_valid + + expect(post_creator.errors.full_messages).to include(I18n.t( + "not_accepting_pms", username: another_user.username + )) + end + end + context "private message to a muted user" do let(:muted_me) { Fabricate(:evil_trout) } diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index 91fde1605a6..b7c595c7bee 100644 --- a/spec/services/user_updater_spec.rb +++ b/spec/services/user_updater_spec.rb @@ -76,8 +76,9 @@ describe UserUpdater do notification_level_when_replying: 3, email_in_reply_to: false, date_of_birth: date_of_birth, - theme_key: theme.key - ) + theme_key: theme.key, + allow_private_messages: false) + expect(val).to be_truthy user.reload @@ -92,6 +93,7 @@ describe UserUpdater do expect(user.user_option.email_in_reply_to).to eq false expect(user.user_option.theme_key).to eq theme.key expect(user.user_option.theme_key_seq).to eq(seq + 1) + expect(user.user_option.allow_private_messages).to eq(false) expect(user.date_of_birth).to eq(date_of_birth.to_date) end From 25c25ae423859ae5a1abea3d2230677d058caa0d Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 10 Oct 2017 16:26:56 +0800 Subject: [PATCH 2/2] FEATURE: Allow user to leave a PM. --- .../discourse/controllers/topic.js.es6 | 6 +- .../discourse/lib/transform-post.js.es6 | 1 + .../widgets/post-small-action.js.es6 | 1 + .../widgets/private-message-map.js.es6 | 32 ++++++++--- app/controllers/topics_controller.rb | 5 +- app/models/topic.rb | 17 ++++-- app/serializers/topic_view_serializer.rb | 1 + config/locales/client.en.yml | 2 + lib/guardian/topic_guardian.rb | 9 ++- spec/components/guardian_spec.rb | 56 +++++++++++++++++++ spec/models/topic_spec.rb | 20 +++++++ 11 files changed, 132 insertions(+), 18 deletions(-) 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