From fc5bb39096bd91d65038ebf1deb594ed15e6ee9c Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Wed, 15 May 2019 12:47:36 -0400
Subject: [PATCH] FIX: Do not send duplicate alerts for the same post (#7476)

---
 app/services/post_alerter.rb       | 25 ++++++++---------
 spec/services/post_alerter_spec.rb | 43 +++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index f9d67620fb5..11ebd97faf5 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -321,32 +321,33 @@ class PostAlerter
       ).exists?
     end
 
-    # Don't notify the same user about the same notification on the same post
-    existing_notification = user.notifications
+    existing_notifications = user.notifications
       .order("notifications.id DESC")
-      .find_by(
+      .where(
         topic_id: post.topic_id,
-        post_number: post.post_number,
-        notification_type: type
-      )
+        post_number: post.post_number
+      ).limit(10)
 
-    return if existing_notification && !should_notify_previous?(user, existing_notification, opts)
+    # Don't notify the same user about the same type of notification on the same post
+    existing_notification_of_same_type = existing_notifications.find { |n| n.notification_type == type }
+
+    return if existing_notification_of_same_type && !should_notify_previous?(user, existing_notification_of_same_type, opts)
 
     notification_data = {}
 
     if is_liked
-      if existing_notification &&
-        existing_notification.created_at > 1.day.ago &&
+      if existing_notification_of_same_type &&
+        existing_notification_of_same_type.created_at > 1.day.ago &&
         (
           user.user_option.like_notification_frequency ==
           UserOption.like_notification_frequency_type[:always]
         )
 
-        data = existing_notification.data_hash
+        data = existing_notification_of_same_type.data_hash
         notification_data["username2"] = data["display_username"]
         notification_data["count"] = (data["count"] || 1).to_i + 1
         # don't use destroy so we don't trigger a notification count refresh
-        Notification.where(id: existing_notification.id).destroy_all
+        Notification.where(id: existing_notification_of_same_type.id).destroy_all
       elsif !SiteSetting.likes_notification_consolidation_threshold.zero?
         notification = consolidate_liked_notifications(
           user,
@@ -417,7 +418,7 @@ class PostAlerter
       skip_send_email: skip_send_email
     )
 
-    if created.id && !existing_notification && NOTIFIABLE_TYPES.include?(type) && !user.suspended?
+    if created.id && existing_notifications.empty? && NOTIFIABLE_TYPES.include?(type) && !user.suspended?
       create_notification_alert(user: user, post: original_post, notification_type: type, username: original_username)
     end
 
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index c91379d992d..203c472d10d 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -545,12 +545,13 @@ describe PostAlerter do
   describe ".create_notification" do
     fab!(:topic) { Fabricate(:private_message_topic, user: user, created_at: 1.hour.ago) }
     fab!(:post) { Fabricate(:post, topic: topic, created_at: 1.hour.ago) }
+    let(:type) { Notification.types[:private_message] }
 
     it "creates a notification for PMs" do
       post.revise(user, { raw: 'This is the revised post' }, revised_at: Time.zone.now)
 
       expect {
-        PostAlerter.new.create_notification(user, Notification.types[:private_message], post)
+        PostAlerter.new.create_notification(user, type, post)
       }.to change { user.notifications.count }.by(1)
 
       expect(user.notifications.last.data_hash["topic_title"]).to eq(topic.title)
@@ -562,14 +563,50 @@ describe PostAlerter do
       post.revise(user, { title: "This is the revised title" }, revised_at: Time.now)
 
       expect {
-        PostAlerter.new.create_notification(user, Notification.types[:private_message], post)
+        PostAlerter.new.create_notification(user, type, post)
       }.to change { user.notifications.count }.by(1)
 
       expect(user.notifications.last.data_hash["topic_title"]).to eq(original_title)
     end
 
-    it "triggers :post_notification_alert" do
+    it "triggers :pre_notification_alert" do
+      events = DiscourseEvent.track_events do
+        PostAlerter.new.create_notification(user, type, post)
+      end
 
+      payload = {
+       notification_type: type,
+       post_number: post.post_number,
+       topic_title: post.topic.title,
+       topic_id: post.topic.id,
+       excerpt: post.excerpt(400, text_entities: true, strip_links: true, remap_emoji: true),
+       username: post.username,
+       post_url: post.url
+      }
+
+      expect(events).to include(event_name: :pre_notification_alert, params: [user, payload])
+    end
+
+    it "does not alert when revising and changing notification type" do
+      PostAlerter.new.create_notification(user, type, post)
+
+      post.revise(user, { raw: "Editing post to fake include a mention of @eviltrout" }, revised_at: Time.now)
+
+      events = DiscourseEvent.track_events do
+        PostAlerter.new.create_notification(user, Notification.types[:mentioned], post)
+      end
+
+      payload = {
+       notification_type: type,
+       post_number: post.post_number,
+       topic_title: post.topic.title,
+       topic_id: post.topic.id,
+       excerpt: post.excerpt(400, text_entities: true, strip_links: true, remap_emoji: true),
+       username: post.username,
+       post_url: post.url
+      }
+
+      expect(events).not_to include(event_name: :pre_notification_alert, params: [user, payload])
     end
 
     it "triggers :before_create_notification" do