FIX: Notify only invited users about mentions in PMs

FIX: Notify only invited users about mentions in PMs

From 5640166b278a0c5f0d084167712a372cafd752fe Mon Sep 17 00:00:00 2001
From: Gerhard Schlager <mail@gerhard-schlager.at>
Date: Fri, 23 Nov 2018 18:25:40 +0100
Subject: [PATCH] FIX: Notify only invited users about mentions in PMs


diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 84166bd..1ebca65 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 6ca008e..922a4a6 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,52 +545,84 @@ 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
-          args = { user: bob, topic: pm_topic, raw: 'Hello @group' }
-          expect { create_post_with_alerts(args) }.to add_notification(carol, :group_mentioned)
-        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' }
-          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
+        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_not add_notification(dave, :group_mentioned)
+          expect { create_post_with_alerts(args) }.to_not add_notification(eve, :group_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 "message to group" do
 
-    context "group message" do
-      let(:some_group) { Fabricate(:group, name: 'some_group') }
+      let(:some_group) { Fabricate(:group, name: 'some_group', mentionable_level: Group::ALIAS_LEVELS[:everyone]) }
       let(:pm_topic)

GitHub

hmm why are we calling only_allowed_users on not messages?

I am really confused about this line, so here we were picking allowed users if it was edited by system … and now we stopped that?

Previously this was used to prevent notifications about mentions by system users. E.g. the admin’s username was mentioned by system in a welcome message. Now, only_allowed_users applies that filter for every message, not just for system generated ones.

1 Like

just a small style thing:

very complicated

unless mentions && mentions.length > 0

Simple

if mentions.blank?

Overall I just stopped using unless altogether a while back, it creeps me out and confuses my brain. I probably wrote that line of code originally, but yeah it should be de-confused.

also indented early returns are very easy to miss, maybe we should just restructure this so it returns only once.

1 Like

No particular reason other than one place for checking if it’s a private message is better than 2 or more? :thinking:

Will this impact discobot?

1 Like

Yes, I can simplify that. All I did was make sure it returns nil instead of empty results, because the rest of the code expected nil as an abort condition. Since that never happened, the rest of the code was always executed.

Nope, discobot won’t be affected. It’s always in a conversation with a particular user. Mentioning that user in the PM will still work.

2 Likes

@gschlager are we done with this one?

Yes, it’s done. REFACTOR: Simplify extraction of mentions

2 Likes

This commit has been mentioned on Discourse Meta. There might be relevant details there: