FIX: Create notifications for mentions in PMs

app/services/post_alerter.rb

@@ -57,13 +57,11 @@ class PostAlerter
       end
 
       expand_group_mentions(mentioned_groups, post) do |group, users|
-        notify_non_pm_users(users - notified, :group_mentioned, post, mentioned_opts.merge(group: group))
-        notified += users
+        notified += notify_users(users - notified, :group_mentioned, post, mentioned_opts.merge(group: group))
       end
 
       if mentioned_users
-        notify_non_pm_users(mentioned_users - notified, :mentioned, post, mentioned_opts)
-        notified += mentioned_users
+        notified += notify_users(mentioned_users - notified, :mentioned, post, mentioned_opts)
       end
     end
 
@@ -71,34 +69,31 @@ class PostAlerter
     reply_to_user = post.reply_notification_target
 
     if new_record && reply_to_user && !notified.include?(reply_to_user) && notify_about_reply?(post)
-      notify_non_pm_users(reply_to_user, :replied, post)
-      notified += [reply_to_user]
+      notified += notify_non_pm_users(reply_to_user, :replied, post)
     end
 
     # quotes
     quoted_users = extract_quoted_users(post)
-    notify_non_pm_users(quoted_users - notified, :quoted, post)
-    notified += quoted_users
+    notified += notify_non_pm_users(quoted_users - notified, :quoted, post)
 
     # linked
     linked_users = extract_linked_users(post)
-    notify_non_pm_users(linked_users - notified, :linked, post)
-    notified += linked_users
+    notified += notify_non_pm_users(linked_users - notified, :linked, post)
 
     # private messages
     if new_record
       if post.topic.private_message?
         # users that aren't part of any mentioned groups
-        users = directly_targeted_users(post)
+        users = directly_targeted_users(post).reject { |u| notified.include?(u) }
         DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
         users.each do |user|
           notification_level = TopicUser.get(post.topic, user).try(:notification_level)
-          if notified.include?(user) || notification_level == TopicUser.notification_levels[:watching] || user.staged?
+          if reply_to_user == user || notification_level == TopicUser.notification_levels[:watching] || user.staged?
             create_notification(user, Notification.types[:private_message], post)
           end
         end
         # users that are part of all mentionned groups
-        users = indirectly_targeted_users(post)
+        users = indirectly_targeted_users(post).reject { |u| notified.include?(u) }
         DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
         users.each do |user|
           # only create a notification when watching the group
@@ -107,11 +102,7 @@ class PostAlerter
           if notification_level == TopicUser.notification_levels[:watching]
             create_notification(user, Notification.types[:private_message], post)
           elsif notification_level == TopicUser.notification_levels[:tracking]
-            if notified.include?(user)
-              create_notification(user, Notification.types[:private_message], post)
-            else
-              notify_group_summary(user, post)
-            end
+            notify_group_summary(user, post)
           end
         end
       elsif post.post_type == Post.types[:regular]
@@ -499,15 +490,21 @@ class PostAlerter
 
   # Notify a bunch of users
   def notify_non_pm_users(users, type, post, opts = {})
+    return [] if post.topic&.private_message?
 
-    return if post.topic.try(:private_message?)
+    notify_users(users, type, post, opts)
+  end
 
+  def notify_users(users, type, post, opts = {})
     users = [users] unless users.is_a?(Array)
+    users = users.reject { |u| u.staged? } if post.topic&.private_message?
 
     DiscourseEvent.trigger(:before_create_notifications_for_users, users, post)
     users.each do |u|
       create_notification(u, Notification.types[type], post, opts)
     end
+
+    users
   end
 
   def notify_post_users(post, notified)

spec/services/post_alerter_spec.rb

@@ -1,5 +1,26 @@
 require 'rails_helper'
 
+RSpec::Matchers.define :add_notification do |user, notification_type|
+  match(notify_expectation_failures: true) do |actual|
+    notifications = user.notifications
+    before = notifications.count
+
+    actual.call
+
+    expect(notifications.count).to eq(before + 1), "expected 1 new notification, got #{notifications.count - before}"
+
+    last_notification_type = notifications.last.notification_type
+    expect(last_notification_type).to eq(Notification.types[notification_type]),
+                                      "expected notification type to be '#{notification_type}', got '#{Notification.types.key(last_notification_type)}'"
+  end
+
+  match_when_negated do |actual|
+    expect { actual.call }.to_not change { user.notifications.where(notification_type: Notification.types[notification_type]).count }
+  end
+
+  supports_block_expectations
+end
+
 describe PostAlerter do
 
   let!(:evil_trout) { Fabricate(:evil_trout) }
@@ -337,15 +358,122 @@ describe PostAlerter do
       expect(events).to include(event_name: :before_create_notifications_for_users, params: [[evil_trout], mention_post])
     end
 
-  it "notification comes from editor is mention is added later" do
-    admin = Fabricate(:admin)
-      post = create_post_with_alerts(user: user, raw: 'No mention here.')
-      expect {
-        post.revise(admin, raw: "Mention @eviltrout in this edit.")
-      }.to change(evil_trout.notifications, :count)
-      n = evil_trout.notifications.last
-      expect(n.data_hash["original_username"]).to eq(admin.username)
-  end
+    it "notification comes from editor if mention is added later" do
+      admin = Fabricate(:admin)
+        post = create_post_with_alerts(user: user, raw: 'No mention here.')
+        expect {
+          post.revise(admin, raw: "Mention @eviltrout in this edit.")
+        }.to change(evil_trout.notifications, :count)
+        n = evil_trout.notifications.last
+        expect(n.data_hash["original_username"]).to eq(admin.username)
+    end
+
+    let(:alice) { Fabricate(:user, username: 'alice') }
+    let(:bob) { Fabricate(:user, username: 'bob') }
+
+    def create_post_with_alerts(args = {})
+      post = Fabricate(:post, args)
+      PostAlerter.post_created(post)
+    end
+
+    def set_topic_notification_level(user, topic, level_name)
+      TopicUser.change(user.id, topic.id, notification_level: TopicUser.notification_levels[level_name])
+    end
+
+    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
+          before do
+            set_topic_notification_level(alice, topic, notification_level)
+          end
+
+          it "notifies about @username mention" do
+            args = { user: bob, topic: topic, raw: 'Hello @alice' }
+            expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned)
+          end
+        end
+      end
+
+      context "when notification level is 'muted'" do
+        before do
+          set_topic_notification_level(alice, topic, :muted)
+        end
+
+        it "does not notify about @username mention" do
+          args = { user: bob, topic: topic, raw: 'Hello @alice' }
+          expect { create_post_with_alerts(args) }.to_not add_notification(alice, :mentioned)
+        end
+      end
+    end
+
+    shared_context "message" do
+      context "when mentioned user 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 @username mention" do
+              args = { user: bob, topic: pm_topic, raw: 'Hello @alice' }
+              expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned)
+            end
+          end
+        end
+
+        context "when notification level is 'muted'" do
+          before do
+            set_topic_notification_level(alice, pm_topic, :muted)
+          end
+
+          it "does not notify about @username mention" do
+            args = { user: bob, topic: pm_topic, raw: 'Hello @alice' }
+            expect { create_post_with_alerts(args) }.to_not add_notification(alice, :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
+          carol = Fabricate(:admin, username: 'carol')
+          args = { user: bob, topic: pm_topic, raw: 'Hello @carol' }
+          expect { create_post_with_alerts(args) }.to add_notification(carol, :mentioned)
+        end
+
+        it "does not notify about @username mention when mentioned user is not allowed to see message" do
+          dave = Fabricate(:user, username: 'dave')
+          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)
+        ])
+      end
+      let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) }
+
+      include_context "message"
+    end
+
+    context "group message" do
+      let(:group) { Fabricate(:group) }
+      let(:pm_topic) do
+        Fabricate(:private_message_topic, user: alice, topic_allowed_groups: [
+          Fabricate.build(:topic_allowed_group, group: group)
+        ])
+      end
+      let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) }
+
+      include_context "message"
+    end
   end
 
   describe ".create_notification" do