diff --git a/app/models/topic.rb b/app/models/topic.rb index ccabbfc4a46..734fb240bd4 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 8cda53c5261..a1a1c9388c1 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 922a4a6b89b..90c948ce065 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