From 5640166b278a0c5f0d084167712a372cafd752fe Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Fri, 23 Nov 2018 18:25:40 +0100 Subject: [PATCH] FIX: Notify only invited users about mentions in PMs --- app/services/post_alerter.rb | 16 +++- spec/services/post_alerter_spec.rb | 146 ++++++++++++++++++----------- 2 files changed, 100 insertions(+), 62 deletions(-) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 84166bdd4c6..1ebca651156 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -38,7 +38,8 @@ class PostAlerter end def only_allowed_users(users, post) - users.select { |u| allowed_users(post).include?(u) || allowed_group_users(post).include?(u) } + return users unless post.topic.private_message? + users.select { |u| all_allowed_users(post).include?(u) } end def notify_about_reply?(post) @@ -61,12 +62,12 @@ class PostAlerter end expand_group_mentions(mentioned_groups, post) do |group, users| - users = only_allowed_users(users, post) if editor.id < 0 + users = only_allowed_users(users, post) notified += notify_users(users - notified, :group_mentioned, post, mentioned_opts.merge(group: group)) end if mentioned_users - mentioned_users = only_allowed_users(mentioned_users, post) if editor.id < 0 + mentioned_users = only_allowed_users(mentioned_users, post) notified += notify_users(mentioned_users - notified, :mentioned, post, mentioned_opts) end end @@ -457,11 +458,16 @@ class PostAlerter return unless mentions && mentions.length > 0 groups = Group.where('LOWER(name) IN (?)', mentions) - mentions -= groups.map(&:name).map(&:downcase) - return [groups, nil] unless mentions && mentions.length > 0 + if groups.empty? + groups = nil + else + mentions -= groups.map(&:name).map(&:downcase) + return [groups, nil] unless mentions && mentions.length > 0 + end users = User.where(username_lower: mentions).where.not(id: post.user_id) + users = nil if users.empty? [groups, users] end diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 6ca008ea8c3..922a4a6b89b 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -441,7 +441,7 @@ describe PostAlerter do let(:group) { Fabricate(:group, name: 'group', mentionable_level: Group::ALIAS_LEVELS[:everyone]) } before do - group.bulk_add([alice.id, carol.id]) + group.bulk_add([alice.id, eve.id]) end def create_post_with_alerts(args = {}) @@ -455,7 +455,6 @@ describe PostAlerter do context "topic" do let(:topic) { Fabricate(:topic, user: alice) } - let(:first_post) { Fabricate(:post, user: topic.user) } [:watching, :tracking, :regular].each do |notification_level| context "when notification level is '#{notification_level}'" do @@ -482,8 +481,19 @@ describe PostAlerter do end end - shared_context "message" do - context "when mentioned user is part of conversation" do + context "message to users" do + let(:pm_topic) do + Fabricate(:private_message_topic, + user: alice, + topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: alice), + Fabricate.build(:topic_allowed_user, user: bob), + Fabricate.build(:topic_allowed_user, user: Discourse.system_user) + ] + ) + end + + context "when user is part of conversation" do [:watching, :tracking, :regular].each do |notification_level| context "when notification level is '#{notification_level}'" do before do @@ -500,15 +510,10 @@ describe PostAlerter do expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned) end - it "notifies about @group mention" do + it "notifies about @group mention when allowed user is part of group" do args = { user: bob, topic: pm_topic, raw: 'Hello @group' } expect { create_post_with_alerts(args) }.to add_notification(alice, :group_mentioned) end - - it "notifies about @group mentions by non-human users" do - args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @group' } - expect { create_post_with_alerts(args) }.to add_notification(alice, :group_mentioned) - end end end @@ -521,21 +526,16 @@ describe PostAlerter do args = { user: bob, topic: pm_topic, raw: 'Hello @alice' } expect { create_post_with_alerts(args) }.to_not add_notification(alice, :mentioned) end - - it "does not notify about @group mention" do - args = { user: bob, topic: pm_topic, raw: 'Hello @group' } - expect { create_post_with_alerts(args) }.to_not add_notification(alice, :group_mentioned) - end end end - context "when mentioned user is not part of conversation" do - it "notifies about @username mention when mentioned user is allowed to see message" do + context "when user is not part of conversation" do + it "does not notify about @username mention even though mentioned user is an admin" do args = { user: bob, topic: pm_topic, raw: 'Hello @carol' } - expect { create_post_with_alerts(args) }.to add_notification(carol, :mentioned) + expect { create_post_with_alerts(args) }.to_not add_notification(carol, :mentioned) end - it "does not notify about @username mention by non-human user even though mentioned user is allowed to see message" do + it "does not notify about @username mention by non-human user even though mentioned user is an admin" do args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @carol' } expect { create_post_with_alerts(args) }.to_not add_notification(carol, :mentioned) end @@ -545,53 +545,85 @@ describe PostAlerter do expect { create_post_with_alerts(args) }.to_not add_notification(dave, :mentioned) end - it "notifies about @group mention when mentioned user is allowed to see message" do + it "does not notify about @group mention when user is not an allowed user" do args = { user: bob, topic: pm_topic, raw: 'Hello @group' } - expect { create_post_with_alerts(args) }.to add_notification(carol, :group_mentioned) + expect { create_post_with_alerts(args) }.to_not add_notification(eve, :group_mentioned) + end + end + end + + context "message to group" do + + let(:some_group) { Fabricate(:group, name: 'some_group', mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + let(:pm_topic) do + Fabricate(:private_message_topic, + user: alice, + topic_allowed_groups: [ + Fabricate.build(:topic_allowed_group, group: group) + ], + topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: Discourse.system_user) + ] + ) + end + + before do + some_group.bulk_add([alice.id, carol.id]) + end + + context "when group is part of conversation" do + [:watching, :tracking, :regular].each do |notification_level| + context "when notification level is '#{notification_level}'" do + before do + set_topic_notification_level(alice, pm_topic, notification_level) + end + + it "notifies about @group mention" do + args = { user: bob, topic: pm_topic, raw: 'Hello @group' } + expect { create_post_with_alerts(args) }.to add_notification(alice, :group_mentioned) + end + + it "notifies about @group mentions by non-human users" do + args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @group' } + expect { create_post_with_alerts(args) }.to add_notification(alice, :group_mentioned) + end + + it "notifies about @username mention when user belongs to allowed group" do + args = { user: bob, topic: pm_topic, raw: 'Hello @alice' } + expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned) + end + end end - it "does not notify about @group mention by non-human user even though mentioned user is allowed to see message" do - args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @group' } + context "when notification level is 'muted'" do + before do + set_topic_notification_level(alice, pm_topic, :muted) + end + + it "does not notify about @group mention" do + args = { user: bob, topic: pm_topic, raw: 'Hello @group' } + expect { create_post_with_alerts(args) }.to_not add_notification(alice, :group_mentioned) + end + end + end + + context "when group is not part of conversation" do + it "does not notify about @group mention even though mentioned user is an admin" do + args = { user: bob, topic: pm_topic, raw: 'Hello @some_group' } expect { create_post_with_alerts(args) }.to_not add_notification(carol, :group_mentioned) end - it "does not notify about @group mention when mentioned user is not allowed to see message" do - args = { user: bob, topic: pm_topic, raw: 'Hello @group' } - expect { create_post_with_alerts(args) }.to_not add_notification(dave, :group_mentioned) + it "does not notify about @group mention by non-human user even though mentioned user is an admin" do + args = { user: Discourse.system_user, topic: pm_topic, raw: 'Hello @some_group' } + expect { create_post_with_alerts(args) }.to_not add_notification(carol, :group_mentioned) + end + + it "does not notify about @username mention when user doesn't belong to allowed group" do + args = { user: bob, topic: pm_topic, raw: 'Hello @dave' } + expect { create_post_with_alerts(args) }.to_not add_notification(dave, :mentioned) end end end - - context "personal message" do - let(:pm_topic) do - Fabricate(:private_message_topic, user: alice, topic_allowed_users: [ - Fabricate.build(:topic_allowed_user, user: alice), - Fabricate.build(:topic_allowed_user, user: bob), - Fabricate.build(:topic_allowed_user, user: eve) - ]) - end - let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) } - - include_context "message" - end - - context "group message" do - let(:some_group) { Fabricate(:group, name: 'some_group') } - let(:pm_topic) do - Fabricate(:private_message_topic, user: alice, topic_allowed_groups: [ - Fabricate.build(:topic_allowed_group, group: some_group) - ], topic_allowed_users: [ - Fabricate.build(:topic_allowed_user, user: eve) - ]) - end - let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) } - - before do - some_group.add(alice) - end - - include_context "message" - end end describe ".create_notification" do