From e888369f51b80191303ea8eb962551d6ee349671 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 12 Jun 2017 16:41:39 +0900 Subject: [PATCH] UX: Don't send emails for discobot notifications. --- app/jobs/regular/post_alert.rb | 3 +- app/models/notification.rb | 9 +++--- app/services/post_alerter.rb | 32 +++++++++++++------ lib/post_creator.rb | 6 +++- lib/post_jobs_enqueuer.rb | 2 +- .../lib/discourse_narrative_bot/actions.rb | 12 +++++-- .../lib/discourse_narrative_bot/base.rb | 2 +- plugins/discourse-narrative-bot/plugin.rb | 1 - .../new_user_narrative_spec.rb | 3 ++ .../track_selector_spec.rb | 18 +++++++++++ .../discourse-narrative-bot/spec/user_spec.rb | 2 ++ spec/components/post_creator_spec.rb | 16 ++++++++++ spec/models/notification_spec.rb | 1 - 13 files changed, 83 insertions(+), 24 deletions(-) diff --git a/app/jobs/regular/post_alert.rb b/app/jobs/regular/post_alert.rb index 30cef2534b1..cc93aad2855 100644 --- a/app/jobs/regular/post_alert.rb +++ b/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 - diff --git a/app/models/notification.rb b/app/models/notification.rb index b64a7d06912..7db7a5b8bd3 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -12,9 +12,11 @@ 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 + # TODO: Revert back to default in Rails 5 after_commit :refresh_notification_count, on: :destroy after_commit :refresh_notification_count, on: [:create, :update] @@ -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 diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index ec3d7e720da..9672512e1b8 100644 --- a/app/services/post_alerter.rb +++ b/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?) diff --git a/lib/post_creator.rb b/lib/post_creator.rb index cfcdc7b1cd9..7f515a7ee45 100644 --- a/lib/post_creator.rb +++ b/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 diff --git a/lib/post_jobs_enqueuer.rb b/lib/post_jobs_enqueuer.rb index 0e05c565f9f..3227f76c3cf 100644 --- a/lib/post_jobs_enqueuer.rb +++ b/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 diff --git a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb index 0b040134a41..d8b6c47b1aa 100644 --- a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb +++ b/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 diff --git a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/base.rb b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/base.rb index 9b2ed63fc9d..3552d1663f3 100644 --- a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/base.rb +++ b/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 diff --git a/plugins/discourse-narrative-bot/plugin.rb b/plugins/discourse-narrative-bot/plugin.rb index 91d0fc246ed..adc514e29c4 100644 --- a/plugins/discourse-narrative-bot/plugin.rb +++ b/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 diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb index 8b82c367d8a..836a715099c 100644 --- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb +++ b/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( diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb index 213aa6917e3..0aa1dde7787 100644 --- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb +++ b/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 diff --git a/plugins/discourse-narrative-bot/spec/user_spec.rb b/plugins/discourse-narrative-bot/spec/user_spec.rb index 326631fc654..86dde81499c 100644 --- a/plugins/discourse-narrative-bot/spec/user_spec.rb +++ b/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', diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 39fe1443fed..6ea5228e5f4 100644 --- a/spec/components/post_creator_spec.rb +++ b/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 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index fa45deb0e74..1c39367f6db 100644 --- a/spec/models/notification_spec.rb +++ b/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)