FEATURE: disable notifications for small actions that are whispers

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

From aa97f6fdba76c363ac407c85713ac16c4444d33d Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Tue, 4 Dec 2018 17:54:27 +1100
Subject: [PATCH] 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

diff --git a/app/models/topic.rb b/app/models/topic.rb
index ccabbfc..734fb24 100644
--- a/app/models/topic.rb
+++ b/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])
diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 8cda53c..a1a1c93 100644
--- a/app/services/post_alerter.rb
+++ b/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)
-        .where('NOT admin AND NOT moderator')
-        .exists?
+      .joins(:muted_user)
+      .where('NOT admin AND NOT moderator')
+      .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
-      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
+      create_notification_alert(user: user, post: post, notification_type: type, username: original_username)
     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)
diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb
index 922a4a6..90c948c 100644
--- a/spec/services/post_alerter_spec.rb
+++ b/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

GitHub

1 Like

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

Notifications from the discourse-assigned plugin result in translation missing: en.discourse_push_notifications.popup.custom as notification title on Android.

I guess there should be a generic translation for the custom type, but it would be great if plugins could provide a custom title as well.

3 Likes

Something weird is going on here - these are notifications I received via the DiscourseHub app for a couple of assignments from @ZogStriP . I have no idea why @tophee and @vespura are included in the text (they are the OPs of the topics):

1 Like

Fixed in

1 Like

Assigning this to you @ZogStriP for testing… :slight_smile: assign it back to me

1 Like

I think we need to stop with all this “custom” type and just reserve types in core for various plugins … then it is very easy to translate. I will have a play with it later today

1 Like