FIX: missing edit notifications in some rare cases

FIX: missing edit notifications in some rare cases

Due to a refactor in e90f9e5cc47 we stopped notifying on edits if a user liked a post and then edited.

The like could have happened a long time ago so this gets extra confusing.

This change makes the suppression more deliberate. We only want to suppress quite/link/mention if the user already got a reply notification.

We can expand this suppression if it is not enough.

diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index ae560ec..7b1965b 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -8,6 +8,11 @@ class PostAlerter
     post
   end
 
+  def self.post_edited(post, opts = {})
+    PostAlerter.new(opts).after_save_post(post, false)
+    post
+  end
+
   def initialize(default_opts = {})
     @default_opts = default_opts
   end
@@ -261,7 +266,6 @@ 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)
@@ -331,7 +335,14 @@ 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_notifications.present? && !should_notify_previous?(user, post, existing_notification_of_same_type, opts)
+    if existing_notification_of_same_type && !should_notify_previous?(user, post, existing_notification_of_same_type, opts)
+      return
+    end
+
+    # linked, quoted, mentioned may be suppressed if you already have a reply notification
+    if type == Notification.types[:quoted] || type == Notification.types[:linked] || type == Notification.types[:mentioned]
+      return if existing_notifications.find { |n| n.notification_type == Notification.types[:replied] }
+    end
 
     notification_data = {}
 
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index 973f31c..2b3734f 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/spec/services/post_alerter_spec.rb
@@ -110,7 +110,15 @@ describe PostAlerter do
       admin = Fabricate(:admin)
       post.revise(admin, raw: 'I made a revision')
 
-      # skip this notification cause we already notified on a similar edit
+      # lets also like this post which should trigger a notification
+      PostActionCreator.new(
+        admin,
+        post,
+        PostActionType.types[:like]
+      ).perform
+
+      # skip this notification cause we already notified on an edit by the same user
+      # in the previous edit
       freeze_time 2.hours.from_now
       post.revise(admin, raw: 'I made another revision')
 
@@ -119,7 +127,7 @@ describe PostAlerter do
       freeze_time 2.hours.from_now
       post.revise(admin, raw: 'I made another revision')
 
-      expect(Notification.where(post_number: 1, topic_id: post.topic_id).count).to eq(3)
+      expect(Notification.where(post_number: 1, topic_id: post.topic_id).count).to eq(4)
     end
 
     it 'notifies flaggers when flagged post gets unhidden by edit' do
@@ -195,14 +203,13 @@ describe PostAlerter do
                                           user: evil_trout,
                                           data: { topic_title: "test topic" }.to_json
                                          )
-
       expect {
-        PostAlerter.post_created(post)
+        PostAlerter.post_edited(post)
       }.to change(evil_trout.notifications, :count).by(0)
 
       notification.destroy
       expect {
-        PostAlerter.post_created(post)
+        PostAlerter.post_edited(post)
       }.to change(evil_trout.notifications, :count).by(1)
     end
 

GitHub sha: 3d0ccf86

1 Like

@lis2 be sure to review, very close to your proposed change. I made the change a tiny bit more surgical and reused and existing test to check for the fail condition.

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

https://meta.discourse.org/t/edits-not-being-stored-in-user-actions-table/145025/22

@lis2 have you had a chance to check this out yet? Iā€™d like to close it out.

I removed original PR in github, but forgot about review page. In the end, it was fixed by Sam, so we can close this topic