FIX: when unread reply notification exists don't create new (#8921)

FIX: when unread reply notification exists don’t create new (#8921)

  • FIX: when unread reply notification exists don’t create new

From time to time, the user is creating a reply post and then they want to add additional details. They edit an existing post and for example, add a quote from a previous one.

In that situation, if the user to whom reply was directed to already have the unread notification, we should not create the new one.

That behaviour was mentioned here: Reply then edit to add quote notification redundancy - feature - Discourse Meta

  • FIX: dont create new notification if already exists
diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 7a4580ad18..ea1b810590 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -261,6 +261,7 @@ class PostAlerter
   end
 
   def should_notify_previous?(user, post, notification, opts)
+    return false unless notification
     case notification.notification_type
     when Notification.types[:edited] then should_notify_edit?(notification, post, opts)
     when Notification.types[:liked]  then should_notify_like?(user, notification)
@@ -330,7 +331,7 @@ class PostAlerter
     # 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, post, existing_notification_of_same_type, opts)
+    return if existing_notifications.present? && !should_notify_previous?(user, post, existing_notification_of_same_type, opts)
 
     notification_data = {}
 
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index 301f16fadd..4a73313c7b 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -165,9 +165,11 @@ describe PostAlerter do
   end
 
   context 'quotes' do
+    let(:category) { Fabricate(:category) }
+    let(:topic) { Fabricate(:topic, category: category) }
 
     it 'does not notify for muted users' do
-      post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]')
+      post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
       MutedUser.create!(user_id: evil_trout.id, muted_user_id: post.user_id)
 
       expect {
@@ -176,7 +178,7 @@ describe PostAlerter do
     end
 
     it 'does not notify for ignored users' do
-      post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]')
+      post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
       IgnoredUser.create!(user_id: evil_trout.id, ignored_user_id: post.user_id)
 
       expect {
@@ -184,9 +186,27 @@ describe PostAlerter do
       }.to change(evil_trout.notifications, :count).by(0)
     end
 
-    it 'does not collapse quote notifications' do
-      topic = Fabricate(:topic)
+    it 'does not notify for users with new reply notification' do
+      post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
+      notification = Notification.create!(topic: post.topic,
+                                          post_number: post.post_number,
+                                          read: false,
+                                          notification_type: Notification.types[:replied],
+                                          user: evil_trout,
+                                          data: { topic_title: "test topic" }.to_json
+                                         )
+
+      expect {
+        PostAlerter.post_created(post)
+      }.to change(evil_trout.notifications, :count).by(0)
 
+      notification.destroy
+      expect {
+        PostAlerter.post_created(post)
+      }.to change(evil_trout.notifications, :count).by(1)
+    end
+
+    it 'does not collapse quote notifications' do
       expect {
         2.times do
           create_post_with_alerts(

GitHub sha: e90f9e5c

This commit appears in #8921 which was approved by @eviltrout and @ZogStriP. It was merged by @SamSaffron.