Merge pull request #4919 from tgxworld/allow_emails_to_be_skipped_for_a_notification

UX: Don't send emails for discobot notifications.

app/jobs/regular/post_alert.rb

@@ -4,9 +4,8 @@ module Jobs
     def execute(args)
       # maybe it was removed by the time we are making the post
       post = Post.where(id: args[:post_id]).first
-      PostAlerter.post_created(post) if post && post.topic
+      PostAlerter.post_created(post, args[:options] || {}) if post && post.topic
     end
 
   end
 end
-

app/models/notification.rb

@@ -12,7 +12,9 @@ class Notification < ActiveRecord::Base
   scope :visible , lambda { joins('LEFT JOIN topics ON notifications.topic_id = topics.id')
                             .where('topics.id IS NULL OR topics.deleted_at IS NULL') }
 
-  after_commit :send_email
+  attr_accessor :skip_send_email
+
+  after_commit :send_email, on: :create
   # This is super weird because the tests fail if we don't specify `on: :destroy`
   # TODO: Revert back to default in Rails 5
   after_commit :refresh_notification_count, on: :destroy
@@ -206,8 +208,7 @@ class Notification < ActiveRecord::Base
   end
 
   def send_email
-    transaction_includes_action = self.send(:transaction_include_any_action?, [:create])
-    NotificationEmailer.process_notification(self) if transaction_includes_action
+    NotificationEmailer.process_notification(self) if !skip_send_email
   end
 
 end

app/services/post_alerter.rb

@@ -1,13 +1,16 @@
 require_dependency 'distributed_mutex'
 
 class PostAlerter
-
-  def self.post_created(post)
-    alerter = PostAlerter.new
+  def self.post_created(post, opts = {})
+    alerter = PostAlerter.new(opts)
     alerter.after_save_post(post, true)
     post
   end
 
+  def initialize(default_opts = {})
+    @default_opts = default_opts
+  end
+
   def not_allowed?(user, post)
     user.blank? ||
     user.id < 0 ||
@@ -271,14 +274,14 @@ class PostAlerter
     Notification.types[:posted],
   ]
 
-  def create_notification(user, type, post, opts=nil)
+  def create_notification(user, type, post, opts = {})
+    opts = @default_opts.merge(opts)
+
     return if user.blank?
     return if user.id < 0
 
     return if type == Notification.types[:liked] && user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:never]
 
-    opts ||= {}
-
     # Make sure the user can see the post
     return unless Guardian.new(user).can_see?(post)
 
@@ -291,11 +294,19 @@ class PostAlerter
                                       .exists?
 
     # skip if muted on the topic
-    return if TopicUser.get(post.topic, user).try(:notification_level) == TopicUser.notification_levels[:muted]
+    return if TopicUser.where(
+      topic: post.topic,
+      user: user,
+      notification_level: TopicUser.notification_levels[:muted]
+    ).exists?
 
     # skip if muted on the group
     if group = opts[:group]
-      return if GroupUser.find_by(group_id: opts[:group_id], user_id: user.id).try(:notification_level) == TopicUser.notification_levels[:muted]
+      return if GroupUser.where(
+        group_id: opts[:group_id],
+        user_id: user.id,
+        notification_level: TopicUser.notification_levels[:muted]
+      ).exists?
     end
 
     # Don't notify the same user about the same notification on the same post
@@ -375,7 +386,8 @@ class PostAlerter
                               topic_id: post.topic_id,
                               post_number: post.post_number,
                               post_action_id: opts[:post_action_id],
-                              data: notification_data.to_json)
+                              data: notification_data.to_json,
+                              skip_send_email: opts[:skip_send_email])
 
    if !existing_notification && NOTIFIABLE_TYPES.include?(type) && !user.suspended?
      # we may have an invalid post somehow, dont blow up
@@ -458,7 +470,7 @@ class PostAlerter
   end
 
   # Notify a bunch of users
-  def notify_non_pm_users(users, type, post, opts=nil)
+  def notify_non_pm_users(users, type, post, opts = {})
 
     return if post.topic.try(:private_message?)
 

lib/post_creator.rb

@@ -190,7 +190,11 @@ class PostCreator
 
   def enqueue_jobs
     return unless @post && !@post.errors.present?
-    PostJobsEnqueuer.new(@post, @topic, new_topic?, {import_mode: @opts[:import_mode]}).enqueue_jobs
+
+    PostJobsEnqueuer.new(@post, @topic, new_topic?,
+      import_mode: @opts[:import_mode],
+      post_alert_options: @opts[:post_alert_options]
+    ).enqueue_jobs
   end
 
   def self.track_post_stats

lib/post_jobs_enqueuer.rb

@@ -22,7 +22,7 @@ class PostJobsEnqueuer
   private
 
   def enqueue_post_alerts
-    Jobs.enqueue(:post_alert, post_id: @post.id)
+    Jobs.enqueue(:post_alert, post_id: @post.id, options: @opts[:post_alert_options])
   end
 
   def feature_topic_users

plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb

@@ -8,19 +8,25 @@ module DiscourseNarrativeBot
 
     private
 
-    def reply_to(post, raw, opts = {})
+    def reply_to(post, raw, opts = {}, post_alert_options = {})
+      defaut_post_alert_opts = { skip_send_email: true }.merge(post_alert_options)
+
       if post
         default_opts = {
           raw: raw,
           topic_id: post.topic_id,
-          reply_to_post_number: post.post_number
+          reply_to_post_number: post.post_number,
+          post_alert_options: defaut_post_alert_opts
         }
 
         new_post = PostCreator.create!(self.discobot_user, default_opts.merge(opts))
         reset_rate_limits(post) if new_post
         new_post
       else
-        PostCreator.create!(self.discobot_user, { raw: raw }.merge(opts))
+        PostCreator.create!(self.discobot_user, {
+          post_alert_options: defaut_post_alert_opts,
+          raw: raw
+        }.merge(opts))
       end
     end
 

plugins/discourse-narrative-bot/lib/discourse_narrative_bot/base.rb

@@ -94,7 +94,7 @@ module DiscourseNarrativeBot
             skip_trigger: TrackSelector.skip_trigger,
             reset_trigger: "#{TrackSelector.reset_trigger} #{self.class.reset_trigger}"
           )
-        ))
+        ), {}, { skip_send_email: false })
       end
     end
 

plugins/discourse-narrative-bot/plugin.rb

@@ -148,7 +148,6 @@ after_initialize do
     SiteSetting.discourse_narrative_bot_enabled &&
       self.id > 0 &&
       !self.anonymous? &&
-      !self.user_option.mailing_list_mode &&
       !self.staged &&
       !SiteSetting.discourse_narrative_bot_ignored_usernames.split('|'.freeze).include?(self.username)
   end

plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb

@@ -37,6 +37,9 @@ describe DiscourseNarrativeBot::NewUserNarrative do
     end
 
     it 'should create the right message' do
+      NotificationEmailer.enable
+      NotificationEmailer.expects(:process_notification).once
+
       expect { narrative.notify_timeout(user) }.to change { Post.count }.by(1)
 
       expect(Post.last.raw).to eq(I18n.t(

plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb

@@ -102,6 +102,24 @@ describe DiscourseNarrativeBot::TrackSelector do
 
             expect(Post.last.raw).to eq(expected_raw.chomp)
           end
+
+          it 'should not enqueue any user email' do
+            NotificationEmailer.enable
+            user.user_option.update!(email_always: true)
+
+            post.update!(
+              raw: 'show me what you can do',
+              reply_to_post_number: first_post.post_number
+            )
+
+            NotificationEmailer.expects(:process_notification).never
+
+            described_class.new(:reply, user, post_id: post.id).select
+
+            expect(Post.last.raw).to eq(I18n.t(
+              "discourse_narrative_bot.new_user_narrative.formatting.not_found"
+            ))
+          end
         end
 
         context 'when a non regular post is created' do

plugins/discourse-narrative-bot/spec/user_spec.rb

@@ -10,6 +10,8 @@ describe User do
 
   describe 'when a user is created' do
     it 'should initiate the bot' do
+      NotificationEmailer.expects(:process_notification).never
+
       user
 
       expected_raw = I18n.t('discourse_narrative_bot.new_user_narrative.hello.message',

spec/components/post_creator_spec.rb

@@ -265,6 +265,22 @@ describe PostCreator do
         expect(post.valid?).to eq(true)
       end
 
+      it 'allows notification email to be skipped' do
+        user_2 = Fabricate(:user)
+
+        creator = PostCreator.new(user,
+          title: 'hi there welcome to my topic',
+          raw: "this is my awesome message @#{user_2.username_lower}",
+          archetype: Archetype.private_message,
+          target_usernames: [user_2.username],
+          post_alert_options: { skip_send_email: true }
+        )
+
+        NotificationEmailer.expects(:process_notification).never
+
+        creator.create
+      end
+
       describe "topic's auto close" do
         before do
           SiteSetting.queue_jobs = true

spec/models/notification_spec.rb

@@ -44,7 +44,6 @@ describe Notification do
         process_alerts(Fabricate(:post, post_args.merge(raw: "Hello @CodingHorror")))
       }
 
-
       it 'notifies the poster on reply' do
         expect {
           reply = Fabricate(:basic_reply, user: coding_horror, topic: post.topic)