FEATURE: disable notifications for small actions that are whispers

Previously we would notify on small actions if they were whispers
this inconsistently lead to all sorts of problems including

- collapsed "N replies" after assign
- empty push notifications

New behavior adds an api to explicitly send push notifications as well
if needed: create_notification_alert

app/models/topic.rb

@@ -731,7 +731,6 @@ class Topic < ActiveRecord::Base
                               post_type: opts[:post_type] || Post.types[:moderator_action],
                               action_code: opts[:action_code],
                               no_bump: opts[:bump].blank?,
-                              skip_notifications: opts[:skip_notifications],
                               topic_id: self.id,
                               skip_validations: true,
                               custom_fields: opts[:custom_fields])

app/services/post_alerter.rb

@@ -43,7 +43,10 @@ class PostAlerter
   end
 
   def notify_about_reply?(post)
-    post.post_type == Post.types[:regular] || post.post_type == Post.types[:whisper]
+    # small actions can be whispers in this case they will have an action code
+    # we never want to notify on this
+    post.post_type == Post.types[:regular] ||
+      (post.post_type == Post.types[:whisper] && post.action_code.nil?)
   end
 
   def after_save_post(post, new_record = false)
@@ -289,9 +292,9 @@ class PostAlerter
 
     # apply muting here
     return if notifier_id && MutedUser.where(user_id: user.id, muted_user_id: notifier_id)
-        .joins(:muted_user)
+      .joins(:muted_user)
-        .where('NOT admin AND NOT moderator')
+      .where('NOT admin AND NOT moderator')
-        .exists?
+      .exists?
 
     # skip if muted on the topic
     return if TopicUser.where(
@@ -396,27 +399,30 @@ class PostAlerter
     )
 
     if created.id && !existing_notification && NOTIFIABLE_TYPES.include?(type) && !user.suspended?
-      post_url = original_post.url
+      create_notification_alert(user: user, post: post, notification_type: type, username: original_username)
-      if post_url
-        payload = {
-         notification_type: type,
-         post_number: original_post.post_number,
-         topic_title: original_post.topic.title,
-         topic_id: original_post.topic.id,
-         excerpt: original_post.excerpt(400, text_entities: true, strip_links: true, remap_emoji: true),
-         username: original_username,
-         post_url: post_url
-        }
-
-        MessageBus.publish("/notification-alert/#{user.id}", payload, user_ids: [user.id])
-        push_notification(user, payload)
-        DiscourseEvent.trigger(:post_notification_alert, user, payload)
-      end
     end
 
     created.id ? created : nil
   end
 
+  def create_notification_alert(user:, post:, notification_type:, excerpt: nil, username: nil)
+    if post_url = post.url
+      payload = {
+       notification_type: notification_type,
+       post_number: post.post_number,
+       topic_title: post.topic.title,
+       topic_id: post.topic.id,
+       excerpt: excerpt || post.excerpt(400, text_entities: true, strip_links: true, remap_emoji: true),
+       username: username || post.username,
+       post_url: post_url
+      }
+
+      MessageBus.publish("/notification-alert/#{user.id}", payload, user_ids: [user.id])
+      push_notification(user, payload)
+      DiscourseEvent.trigger(:post_notification_alert, user, payload)
+    end
+  end
+
   def contains_email_address?(addresses, user)
     return false if addresses.blank?
     addresses.split(";").include?(user.email)

spec/services/post_alerter_spec.rb

@@ -803,7 +803,7 @@ describe PostAlerter do
     it "triggers :before_create_notifications_for_users" do
       user = Fabricate(:user)
       topic = Fabricate(:topic)
-      post = Fabricate(:post, user: user, topic: topic)
+      _post = Fabricate(:post, user: user, topic: topic)
       reply = Fabricate(:post, topic: topic, reply_to_post_number: 1)
       events = DiscourseEvent.track_events do
         PostAlerter.post_created(reply)
@@ -814,7 +814,7 @@ describe PostAlerter do
     it "notifies about regular reply" do
       user = Fabricate(:user)
       topic = Fabricate(:topic)
-      post = Fabricate(:post, user: user, topic: topic)
+      _post = Fabricate(:post, user: user, topic: topic)
 
       reply = Fabricate(:post, topic: topic, reply_to_post_number: 1)
       PostAlerter.post_created(reply)
@@ -827,7 +827,7 @@ describe PostAlerter do
       admin = Fabricate(:admin)
 
       topic = Fabricate(:topic)
-      post = Fabricate(:post, user: user, topic: topic)
+      _post = Fabricate(:post, user: user, topic: topic)
 
       whispered_reply = Fabricate(:post, user: admin, topic: topic, post_type: Post.types[:whisper], reply_to_post_number: 1)
       PostAlerter.post_created(whispered_reply)
@@ -841,7 +841,7 @@ describe PostAlerter do
       admin2 = Fabricate(:admin)
 
       topic = Fabricate(:topic)
-      post = Fabricate(:post, user: user, topic: topic)
+      _post = Fabricate(:post, user: user, topic: topic)
 
       whispered_reply1 = Fabricate(:post, user: admin1, topic: topic, post_type: Post.types[:whisper], reply_to_post_number: 1)
       whispered_reply2 = Fabricate(:post, user: admin2, topic: topic, post_type: Post.types[:whisper], reply_to_post_number: 2)
@@ -849,6 +849,21 @@ describe PostAlerter do
       PostAlerter.post_created(whispered_reply2)
 
       expect(admin1.notifications.where(notification_type: Notification.types[:replied]).count).to eq(1)
+
+      TopicUser.change(admin1.id, topic.id, notification_level: TopicUser.notification_levels[:watching])
+
+      # this should change nothing cause the moderator post has an action code
+      # if we have an action code then we should never have notifications, this is rare but
+      # assign whispers are like this
+      whispered_reply3 = topic.add_moderator_post(admin2, "i am a reply", post_type: Post.types[:whisper], action_code: 'moderator_thing')
+      PostAlerter.post_created(whispered_reply3)
+
+      # if this whisper is not ignored like it should we would see a posted notification and no replied notifications
+      notifications = admin1.notifications.where(topic_id: topic.id).to_a
+
+      expect(notifications.first.notification_type).to eq(Notification.types[:replied])
+      expect(notifications.length).to eq(1)
+      expect(notifications.first.post_number).to eq(whispered_reply2.post_number)
     end
 
     it "sends email notifications only to users not on CC list of incoming email" do