From 1bb485fca50c7c235ea307f5b51bb0366f80317b Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 27 Jan 2016 12:19:49 +1100 Subject: [PATCH] FIX: when a user got multiple replies to a topic, emails were missing --- app/jobs/regular/user_email.rb | 129 ++++++++++++++++-------- app/mailers/user_notifications.rb | 29 ++++-- app/models/notification.rb | 4 + app/models/post.rb | 4 + app/models/user_email_observer.rb | 51 +++++----- spec/jobs/user_email_spec.rb | 59 +++++++---- spec/mailers/user_notifications_spec.rb | 57 ++++++----- spec/models/user_email_observer_spec.rb | 23 +++-- 8 files changed, 226 insertions(+), 130 deletions(-) diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 817b0701a..ea4a5b931 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -7,70 +7,117 @@ module Jobs def execute(args) - @args = args + notification,post = nil - # Required parameters raise Discourse::InvalidParameters.new(:user_id) unless args[:user_id].present? raise Discourse::InvalidParameters.new(:type) unless args[:type].present? - # Find the user - @user = User.find_by(id: args[:user_id]) - return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless @user - return skip(I18n.t("email_log.anonymous_user")) if @user.anonymous? - return skip(I18n.t("email_log.suspended_not_pm")) if @user.suspended? && args[:type] != :user_private_message + type = args[:type] - # ensure we *never* send a digest to a staged user - return if @user.staged && args[:type] == :digest + user = User.find_by(id: args[:user_id]) - seen_recently = (@user.last_seen_at.present? && @user.last_seen_at > SiteSetting.email_time_window_mins.minutes.ago) - seen_recently = false if @user.email_always || @user.staged + set_skip_context(type, args[:user_id], args[:to_address] || user.try(:email) || "no_email_found") + return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless user + + if args[:post_id] + post = Post.find_by(id: args[:post_id]) + return skip(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present? + end + + if args[:notification_id].present? + notification = Notification.find_by(id: args[:notification_id]) + end + + message, skip_reason = message_for_email( user, + post, + type, + notification, + args[:notification_type], + args[:notification_data_hash], + args[:email_token], + args[:to_address] ) + + + if message + Email::Sender.new(message, args[:type], @user).send + else + skip_reason + end + end + + def set_skip_context(type, user_id, to_address) + @skip_context = {type: type, user_id: user_id, to_address: to_address} + end + + + def message_for_email( user, + post, + type, + notification, + notification_type=nil, + notification_data_hash=nil, + email_token=nil, + to_address=nil) + + set_skip_context(type, user.id, to_address || user.email) + + return skip_message(I18n.t("email_log.anonymous_user")) if user.anonymous? + return skip_message(I18n.t("email_log.suspended_not_pm")) if user.suspended? && type != :user_private_message + + return if user.staged && type == :digest + + seen_recently = (user.last_seen_at.present? && user.last_seen_at > SiteSetting.email_time_window_mins.minutes.ago) + seen_recently = false if user.email_always || user.staged email_args = {} - if args[:post_id] - # Don't email a user about a post when we've seen them recently. - return skip(I18n.t('email_log.seen_recently')) if seen_recently - - post = Post.find_by(id: args[:post_id]) - return skip(I18n.t('email_log.post_not_found', post_id: args[:post_id])) unless post.present? + if post || notification || notification_type + return skip_message(I18n.t('email_log.seen_recently')) if seen_recently && !user.suspended? + end + if post email_args[:post] = post end - email_args[:email_token] = args[:email_token] if args[:email_token].present? + if notification || notification_type - notification = nil - notification = Notification.find_by(id: args[:notification_id]) if args[:notification_id].present? - if notification.present? - # Don't email a user about a post when we've seen them recently. - return skip(I18n.t('email_log.seen_recently')) if seen_recently && !@user.suspended? + email_args[:notification_type] ||= notification_type || notification.try(:notification_type) + email_args[:notification_data_hash] ||= notification_data_hash || notification.try(:data_hash) - # Load the post if present - email_args[:post] ||= Post.find_by(id: notification.data_hash[:original_post_id].to_i) - email_args[:post] ||= notification.post - email_args[:notification] = notification - - return skip(I18n.t('email_log.notification_already_read')) if notification.read? && !@user.email_always + unless user.email_always? + if (notification && notification.read?) || (post && post.seen?(user)) + return skip_message(I18n.t('email_log.notification_already_read')) + end + end end - skip_reason = skip_email_for_post(email_args[:post], @user) - return skip(skip_reason) if skip_reason + skip_reason = skip_email_for_post(post, user) + return skip_message(skip_reason) if skip_reason # Make sure that mailer exists - raise Discourse::InvalidParameters.new("type=#{args[:type]}") unless UserNotifications.respond_to?(args[:type]) + raise Discourse::InvalidParameters.new("type=#{type}") unless UserNotifications.respond_to?(type) - message = UserNotifications.send(args[:type], @user, email_args) - - # Update the to address if we have a custom one - if args[:to_address].present? - message.to = [args[:to_address]] + if email_token.present? + email_args[:email_token] = email_token end - Email::Sender.new(message, args[:type], @user).send + message = UserNotifications.send(type, user, email_args) + + # Update the to address if we have a custom one + if to_address.present? + message.to = [to_address] + end + + [message,nil] + end private + def skip_message(reason) + [nil, skip(reason)] + end + # If this email has a related post, don't send an email if it's been deleted or seen recently. def skip_email_for_post(post, user) if post @@ -84,9 +131,9 @@ module Jobs end def skip(reason) - EmailLog.create( email_type: @args[:type], - to_address: @args[:to_address] || @user.try(:email) || "no_email_found", - user_id: @user.try(:id), + EmailLog.create( email_type: @skip_context[:type], + to_address: @skip_context[:to_address], + user_id: @skip_context.try(:user_id), skipped: true, skipped_reason: reason) end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 947735eaa..d2ff9cc51 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -200,32 +200,39 @@ class UserNotifications < ActionMailer::Base end def notification_email(user, opts) - return unless @notification = opts[:notification] - return unless @post = opts[:post] + return unless notification_type = opts[:notification_type] + return unless notification_data = opts[:notification_data_hash] + return unless post = opts[:post] - user_name = @notification.data_hash[:original_username] - - if @post && SiteSetting.enable_names && SiteSetting.display_name_on_email_from - name = User.where(id: @post.user_id).pluck(:name).first + unless String === notification_type + if Numeric === notification_type + notification_type = Notification.types[notification_type] + end + notification_type = notification_type.to_s + end + + user_name = notification_data[:original_username] + + if post && SiteSetting.enable_names && SiteSetting.display_name_on_email_from + name = User.where(id: post.user_id).pluck(:name).first user_name = name unless name.blank? end - notification_type = opts[:notification_type] || Notification.types[@notification.notification_type].to_s - return if user.mailing_list_mode && !@post.topic.private_message? && + return if user.mailing_list_mode && !post.topic.private_message? && ["replied", "mentioned", "quoted", "posted", "group_mentioned"].include?(notification_type) - title = @notification.data_hash[:topic_title] + title = notification_data[:topic_title] allow_reply_by_email = opts[:allow_reply_by_email] unless user.suspended? use_site_subject = opts[:use_site_subject] add_re_to_subject = opts[:add_re_to_subject] show_category_in_subject = opts[:show_category_in_subject] use_template_html = opts[:use_template_html] - original_username = @notification.data_hash[:original_username] || @notification.data_hash[:display_username] + original_username = notification_data[:original_username] || notification_data[:display_username] send_notification_email( title: title, - post: @post, + post: post, username: original_username, from_alias: user_name, allow_reply_by_email: allow_reply_by_email, diff --git a/app/models/notification.rb b/app/models/notification.rb index eababfdc8..e2f057530 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -158,6 +158,10 @@ class Notification < ActiveRecord::Base Notification.types[:private_message] == self.notification_type && !read end + def post_id + Post.where(topic: topic_id, post_number: post_number).pluck(:id).first + end + protected def refresh_notification_count diff --git a/app/models/post.rb b/app/models/post.rb index 746a7a33b..3f93899ff 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -580,6 +580,10 @@ class Post < ActiveRecord::Base SQL end + def seen?(user) + PostTiming.where(topic_id: topic_id, post_number: post_number, user_id: user.id).exists? + end + private def parse_quote_into_arguments(quote) diff --git a/app/models/user_email_observer.rb b/app/models/user_email_observer.rb index 67bcc5d6f..8143fef4f 100644 --- a/app/models/user_email_observer.rb +++ b/app/models/user_email_observer.rb @@ -40,55 +40,58 @@ class UserEmailObserver < ActiveRecord::Observer enqueue(:user_invited_to_topic, 0) end + def self.notification_params(notification, type) + post_id = (notification.data_hash[:original_post_id] || notification.post_id).to_i + + hash = { + type: type, + user_id: notification.user_id, + notification_id: notification.id, + notification_data_hash: notification.data_hash, + notification_type: Notification.types[notification.notification_type], + } + + hash[:post_id] = post_id if post_id > 0 + hash + end + private EMAILABLE_POST_TYPES ||= Set.new [Post.types[:regular], Post.types[:whisper]] def enqueue(type, delay=default_delay) return unless notification.user.email_direct? - return unless notification.user.active? || notification.user.staged? - return unless EMAILABLE_POST_TYPES.include? notification.post.try(:post_type) - - Jobs.enqueue_in(delay, - :user_email, - type: type, - user_id: notification.user_id, - notification_id: notification.id) + perform_enqueue(type,delay) end def enqueue_private(type, delay=default_delay) return unless notification.user.email_private_messages? + perform_enqueue(type,delay) + end + + def perform_enqueue(type, delay) return unless notification.user.active? || notification.user.staged? return unless EMAILABLE_POST_TYPES.include? notification.post.try(:post_type) - Jobs.enqueue_in(delay, - :user_email, - type: type, - user_id: notification.user_id, - notification_id: notification.id) + Jobs.enqueue_in(delay, :user_email, self.class.notification_params(notification, type)) end + def default_delay SiteSetting.email_time_window_mins.minutes end + end def after_commit(notification) transaction_includes_action = notification.send(:transaction_include_any_action?, [:create]) - delegate_to_email_user(notification) if transaction_includes_action + self.class.process_notification(notification) if transaction_includes_action end - private - - - def extract_notification_type(notification) - Notification.types[notification.notification_type] - end - - def delegate_to_email_user(notification) + def self.process_notification(notification) email_user = EmailUser.new(notification) - email_method = extract_notification_type(notification) - + email_method = Notification.types[notification.notification_type] email_user.send(email_method) if email_user.respond_to? email_method end + end diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 0bb58c955..bfc2ae53b 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -4,7 +4,7 @@ require_dependency 'jobs/base' describe Jobs::UserEmail do before do - SiteSetting.stubs(:email_time_window_mins).returns(10) + SiteSetting.email_time_window_mins = 10 end let(:user) { Fabricate(:user, last_seen_at: 11.minutes.ago ) } @@ -135,17 +135,22 @@ describe Jobs::UserEmail do ) } - it 'passes a notification as an argument when a notification_id is present' do - Email::Sender.any_instance.expects(:send) - UserNotifications.expects(:user_mentioned).with(user, notification: notification, post: post).returns(mailer) - Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id) + it "doesn't send the email if the notification has been seen" do + notification.update_column(:read, true) + message, err = Jobs::UserEmail.new.message_for_email( + user, + post, + :user_mentioned, + notification, + notification.notification_type, + notification.data_hash, + nil, + nil) + + expect(message).to eq nil + expect(err.skipped_reason).to match(/notification.*already/) end - it "doesn't send the email if the notification has been seen" do - Email::Sender.any_instance.expects(:send).never - notification.update_column(:read, true) - Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id) - end it "does send the email if the notification has been seen but the user is set for email_always" do Email::Sender.any_instance.expects(:send) @@ -157,13 +162,21 @@ describe Jobs::UserEmail do it "doesn't send the email if the post has been user deleted" do Email::Sender.any_instance.expects(:send).never post.update_column(:user_deleted, true) - Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id) + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, + notification_id: notification.id, post_id: post.id) end context 'user is suspended' do it "doesn't send email for a pm from a regular user" do - Email::Sender.any_instance.expects(:send).never - Jobs::UserEmail.new.execute(type: :user_private_message, user_id: suspended.id, notification_id: notification.id) + msg,err = Jobs::UserEmail.new.message_for_email( + suspended, + Fabricate.build(:post), + :user_private_message, + notification + ) + + expect(msg).to eq(nil) + expect(err).not_to eq(nil) end context 'pm from staff' do @@ -176,19 +189,29 @@ describe Jobs::UserEmail do post_number: @pm_from_staff.post_number, data: { original_post_id: @pm_from_staff.id }.to_json ) - UserNotifications.expects(:user_private_message).with(suspended, notification: @pm_notification, post: @pm_from_staff).returns(mailer) end - subject(:execute_user_email_job) { - Jobs::UserEmail.new.execute(type: :user_private_message, user_id: suspended.id, notification_id: @pm_notification.id) } + let :sent_message do + Jobs::UserEmail.new.message_for_email( + suspended, + @pm_from_staff, + :user_private_message, + @pm_notification + ) + end it "sends an email" do - execute_user_email_job + msg,err = sent_message + expect(msg).not_to be(nil) + expect(err).to be(nil) end it "sends an email even if user was last seen recently" do suspended.update_column(:last_seen_at, 1.minute.ago) - execute_user_email_job + + msg,err = sent_message + expect(msg).not_to be(nil) + expect(err).to be(nil) end end end diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index fafe7b8ee..4c250f18e 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -99,7 +99,11 @@ describe UserNotifications do it 'generates a correct email' do SiteSetting.enable_names = true SiteSetting.display_name_on_posts = true - mail = UserNotifications.user_replied(response.user, post: response, notification: notification) + mail = UserNotifications.user_replied(response.user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) # from should include full user name expect(mail[:from].display_names).to eql(['John Doe']) @@ -119,22 +123,21 @@ describe UserNotifications do # in mailing list mode user_replies is not sent through response.user.mailing_list_mode = true - mail = UserNotifications.user_replied(response.user, post: response, notification: notification) + mail = UserNotifications.user_replied(response.user, post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) - if Rails.version >= "4.2.0" - expect(mail.message.class).to eq(ActionMailer::Base::NullMail) - else - expect(mail.class).to eq(ActionMailer::Base::NullMail) - end + expect(mail.message.class).to eq(ActionMailer::Base::NullMail) response.user.mailing_list_mode = nil - mail = UserNotifications.user_replied(response.user, post: response, notification: notification) + mail = UserNotifications.user_replied(response.user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) - if Rails.version >= "4.2.0" - expect(mail.message.class).not_to eq(ActionMailer::Base::NullMail) - else - expect(mail.class).not_to eq(ActionMailer::Base::NullMail) - end + expect(mail.message.class).not_to eq(ActionMailer::Base::NullMail) end end @@ -147,7 +150,11 @@ describe UserNotifications do it 'generates a correct email' do SiteSetting.enable_names = false - mail = UserNotifications.user_posted(response.user, post: response, notification: notification) + mail = UserNotifications.user_posted(response.user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) # from should not include full user name if "show user full names" is disabled expect(mail[:from].display_names).to_not eql(['John Doe']) @@ -179,7 +186,12 @@ describe UserNotifications do it 'generates a correct email' do SiteSetting.enable_names = true - mail = UserNotifications.user_private_message(response.user, post: response, notification: notification) + mail = UserNotifications.user_private_message( + response.user, + post: response, + notification_type: notification.notification_type, + notification_data_hash: notification.data_hash + ) # from should include username if full user name is not provided expect(mail[:from].display_names).to eql(['john']) @@ -201,16 +213,11 @@ describe UserNotifications do def expects_build_with(condition) UserNotifications.any_instance.expects(:build_email).with(user.email, condition) - mailer = UserNotifications.send(mail_type, user, notification: notification, post: notification.post) - - if Rails.version >= "4.2.0" - # Starting from Rails 4.2, calling MyMailer.some_method no longer result - # in an immediate call to MyMailer#some_method. Instead, a "lazy proxy" is - # returned (this is changed to support #deliver_later). As a quick hack to - # fix the test, calling #message (or anything, really) would force the - # Mailer object to be created and the method invoked. - mailer.message - end + mailer = UserNotifications.send(mail_type, user, + notification_type: Notification.types[notification.notification_type], + notification_data_hash: notification.data_hash, + post: notification.post) + mailer.message end shared_examples "supports reply by email" do diff --git a/spec/models/user_email_observer_spec.rb b/spec/models/user_email_observer_spec.rb index 9dfe1cdae..0824f1949 100644 --- a/spec/models/user_email_observer_spec.rb +++ b/spec/models/user_email_observer_spec.rb @@ -8,14 +8,15 @@ describe UserEmailObserver do # something is off with fabricator def create_notification(type, user=nil) user ||= Fabricate(:user) - Notification.create(data: '', user: user, notification_type: type, topic: topic, post_number: post.post_number) + Notification.create(data: "{\"a\": 1}", user: user, notification_type: type, topic: topic, post_number: post.post_number) end shared_examples "enqueue" do + it "enqueues a job for the email" do - Jobs.expects(:enqueue_in).with(delay, :user_email, type: type, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).after_commit(notification) + Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) + UserEmailObserver.process_notification(notification) end context "inactive user" do @@ -24,13 +25,13 @@ describe UserEmailObserver do it "doesn't enqueue a job" do Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.send(:new).after_commit(notification) + UserEmailObserver.process_notification(notification) end it "enqueues a job if the user is staged" do notification.user.staged = true - Jobs.expects(:enqueue_in).with(delay, :user_email, type: type, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).after_commit(notification) + Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) + UserEmailObserver.process_notification(notification) end end @@ -40,7 +41,7 @@ describe UserEmailObserver do it "doesn't enqueue a job" do Post.any_instance.expects(:post_type).returns(Post.types[:small_action]) Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.send(:new).after_commit(notification) + UserEmailObserver.process_notification(notification) end end @@ -53,7 +54,7 @@ describe UserEmailObserver do it "doesn't enqueue a job if the user has mention emails disabled" do notification.user.expects(:email_direct?).returns(false) Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.send(:new).after_commit(notification) + UserEmailObserver.process_notification(notification) end end @@ -63,7 +64,7 @@ describe UserEmailObserver do it "doesn't enqueue a job if the user has private message emails disabled" do notification.user.expects(:email_private_messages?).returns(false) Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never - UserEmailObserver.send(:new).after_commit(notification) + UserEmailObserver.process_notification(notification) end end @@ -76,8 +77,8 @@ describe UserEmailObserver do it "enqueue a delayed job for users that are online" do notification.user.last_seen_at = 1.minute.ago - Jobs.expects(:enqueue_in).with(delay, :user_email, type: type, user_id: notification.user_id, notification_id: notification.id) - UserEmailObserver.send(:new).after_commit(notification) + Jobs.expects(:enqueue_in).with(delay, :user_email, UserEmailObserver::EmailUser.notification_params(notification,type)) + UserEmailObserver.process_notification(notification) end end