FIX: when a user got multiple replies to a topic, emails were missing

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

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]
+    unless String === notification_type
+      if Numeric === notification_type
+        notification_type = Notification.types[notification_type]
+      end
+      notification_type = notification_type.to_s
+    end
 
-    if @post && SiteSetting.enable_names && SiteSetting.display_name_on_email_from
-      name = User.where(id: @post.user_id).pluck(:name).first
+    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,

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

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)

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

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

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
 
       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
     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,17 +213,12 @@ 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 = UserNotifications.send(mail_type, user,
+                                    notification_type: Notification.types[notification.notification_type],
+                                    notification_data_hash: notification.data_hash,
+                                    post: notification.post)
     mailer.message
   end
-  end
 
   shared_examples "supports reply by email" do
     context "reply_by_email" do

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