FIX: sending emails to mailing list subscribers wasn't working

app/jobs/regular/notify_mailing_list_subscribers.rb

@@ -11,7 +11,7 @@ module Jobs
 
       users =
           User.activated.not_blocked.not_suspended.real
-          .where(mailing_list_mode:  true)
+          .where(mailing_list_mode: true)
           .where('NOT EXISTS(
                       SELECT 1
                       FROM topic_users tu
@@ -29,22 +29,13 @@ module Jobs
                        cu.notification_level = ?
                   )', post.topic.category_id, CategoryUser.notification_levels[:muted])
 
-      error_count = 0
       users.each do |user|
         if Guardian.new(user).can_see?(post)
           begin
             message = UserNotifications.mailing_list_notify(user, post)
-            Email::Sender.new(message, :mailing_list, user).send
+            Email::Sender.new(message, :mailing_list, user).send if message
           rescue => e
-            Discourse.handle_job_exception(e, error_context(
+            Discourse.handle_job_exception(e, error_context(args, "Sending post to mailing list subscribers", { user_id: user.id, user_email: user.email }))
-                args,
-                "Sending post to mailing list subscribers", {
-                user_id: user.id,
-                user_email: user.email
-            }))
-            if (++error_count) >= 4
-              raise RuntimeError, "ABORTING NotifyMailingListSubscribers due to repeated failures"
-            end
           end
         end
       end

app/jobs/regular/user_email.rb

@@ -15,7 +15,7 @@ module Jobs
 
       user = User.find_by(id: args[:user_id])
 
-      set_skip_context(type, args[:user_id], args[:to_address] || user.try(:email) || "no_email_found")
+      set_skip_context(type, args[:user_id], args[:to_address].presence || user.try(:email).presence || "no_email_found")
       return skip(I18n.t("email_log.no_user", user_id: args[:user_id])) unless user
 
       if args[:post_id]
@@ -45,11 +45,18 @@ module Jobs
     end
 
     def set_skip_context(type, user_id, to_address)
-      @skip_context = {type: type, user_id: user_id, to_address: to_address}
+      @skip_context = { type: type, user_id: user_id, to_address: to_address }
     end
 
+    NOTIFICATIONS_SENT_BY_MAILING_LIST ||= Set.new [
+      Notification.types[:posted],
+      Notification.types[:replied],
+      Notification.types[:mentioned],
+      Notification.types[:group_mentioned],
+      Notification.types[:quoted],
+    ]
 
-    def message_for_email(user, post, type, notification,
+   def message_for_email(user, post, type, notification,
                           notification_type=nil, notification_data_hash=nil,
                           email_token=nil, to_address=nil)
 
@@ -77,6 +84,13 @@ module Jobs
         email_args[:notification_type] ||= notification_type || notification.try(:notification_type)
         email_args[:notification_data_hash] ||= notification_data_hash || notification.try(:data_hash)
 
+        if user.mailing_list_mode? &&
+           !post.topic.private_message? &&
+           NOTIFICATIONS_SENT_BY_MAILING_LIST.include?(email_args[:notification_type])
+           # no need to log a reason when the mail was already sent via the mailing list job
+           return [nil, nil]
+        end
+
         unless user.email_always?
           if (notification && notification.read?) || (post && post.seen?(user))
             return skip_message(I18n.t('email_log.notification_already_read'))
@@ -97,7 +111,7 @@ module Jobs
       message = UserNotifications.send(type, user, email_args)
 
       # Update the to address if we have a custom one
-      if to_address.present?
+      if message && to_address.present?
         message.to = [to_address]
       end
 

app/mailers/user_notifications.rb

@@ -22,23 +22,27 @@ class UserNotifications < ActionMailer::Base
   end
 
   def authorize_email(user, opts={})
-    build_email(user.email, template: "user_notifications.authorize_email", email_token: opts[:email_token])
+    build_email(user.email,
+                template: "user_notifications.authorize_email",
+                email_token: opts[:email_token])
   end
 
   def forgot_password(user, opts={})
-    build_email( user.email,
+    build_email(user.email,
-                 template: user.has_password? ? "user_notifications.forgot_password" : "user_notifications.set_password",
+                template: user.has_password? ? "user_notifications.forgot_password" : "user_notifications.set_password",
-                 email_token: opts[:email_token])
+                email_token: opts[:email_token])
   end
 
   def admin_login(user, opts={})
-    build_email( user.email,
+    build_email(user.email,
-                 template: "user_notifications.admin_login",
+                template: "user_notifications.admin_login",
-                 email_token: opts[:email_token])
+                email_token: opts[:email_token])
   end
 
   def account_created(user, opts={})
-    build_email( user.email, template: "user_notifications.account_created", email_token: opts[:email_token])
+    build_email(user.email,
+                template: "user_notifications.account_created",
+                email_token: opts[:email_token])
   end
 
   def short_date(dt)
@@ -152,20 +156,19 @@ class UserNotifications < ActionMailer::Base
   end
 
   def mailing_list_notify(user, post)
-    send_notification_email(
+    opts = {
-      title: post.topic.title,
       post: post,
-      username: post.user.username,
-      from_alias: (SiteSetting.enable_names &&
-                   SiteSetting.display_name_on_email_from &&
-                   post.user.name.present?) ? post.user.name : post.user.username,
       allow_reply_by_email: true,
       use_site_subject: true,
       add_re_to_subject: true,
       show_category_in_subject: true,
       notification_type: "posted",
-      user: user
+      notification_data_hash: {
-    )
+        original_username: post.user.username,
+        topic_title: post.topic.title,
+      },
+    }
+    notification_email(user, opts)
   end
 
   protected
@@ -183,7 +186,6 @@ class UserNotifications < ActionMailer::Base
   end
 
   def self.get_context_posts(post, topic_user)
-
     context_posts = Post.where(topic_id: post.topic_id)
                         .where("post_number < ?", post.post_number)
                         .where(user_deleted: false)
@@ -200,9 +202,9 @@ class UserNotifications < ActionMailer::Base
   end
 
   def notification_email(user, opts)
-    return unless notification_type = opts[:notification_type]
+    notification_type = opts[:notification_type]
-    return unless notification_data = opts[:notification_data_hash]
+    notification_data = opts[:notification_data_hash]
-    return unless post = opts[:post]
+    post = opts[:post]
 
     unless String === notification_type
       if Numeric === notification_type
@@ -218,10 +220,6 @@ class UserNotifications < ActionMailer::Base
       user_name = name unless name.blank?
     end
 
-
-    return if user.mailing_list_mode && !post.topic.private_message? &&
-       ["replied", "mentioned", "quoted", "posted", "group_mentioned"].include?(notification_type)
-
     title = notification_data[:topic_title]
     allow_reply_by_email = opts[:allow_reply_by_email] unless user.suspended?
     use_site_subject = opts[:use_site_subject]
@@ -243,7 +241,6 @@ class UserNotifications < ActionMailer::Base
       use_template_html: use_template_html,
       user: user
     )
-
   end
 
   def send_notification_email(opts)

lib/email/message_builder.rb

@@ -7,7 +7,7 @@ module Email
       builder = Email::MessageBuilder.new(*builder_args)
       headers(builder.header_args) if builder.header_args.present?
       mail(builder.build_args).tap { |message|
-        if message and h = builder.html_part
+        if message && h = builder.html_part
           message.html_part = h
         end
       }

spec/jobs/notify_mailing_list_subscribers_spec.rb

@@ -4,53 +4,49 @@ describe Jobs::NotifyMailingListSubscribers do
 
   context "with mailing list on" do
     before { SiteSetting.stubs(:default_email_mailing_list_mode).returns(true) }
+    let(:user) { Fabricate(:user) }
 
-    context "with mailing list on" do
+    context "with a valid post" do
-      let(:user) { Fabricate(:user) }
+      let!(:post) { Fabricate(:post, user: user) }
 
-      context "with a valid post" do
+      it "sends the email to the user" do
-        let!(:post) { Fabricate(:post, user: user) }
+        UserNotifications.expects(:mailing_list_notify).with(user, post).once
-
+        Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
-        it "sends the email to the user" do
-          UserNotifications.expects(:mailing_list_notify).with(user, post).once
-          Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
-        end
       end
-
-      context "with a deleted post" do
-        let!(:post) { Fabricate(:post, user: user, deleted_at: Time.now) }
-
-        it "doesn't send the email to the user" do
-          UserNotifications.expects(:mailing_list_notify).with(user, post).never
-          Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
-        end
-      end
-
-      context "with a user_deleted post" do
-        let!(:post) { Fabricate(:post, user: user, user_deleted: true) }
-
-        it "doesn't send the email to the user" do
-          UserNotifications.expects(:mailing_list_notify).with(user, post).never
-          Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
-        end
-      end
-
-      context "with a deleted topic" do
-        let!(:post) { Fabricate(:post, user: user) }
-
-        before do
-          post.topic.update_column(:deleted_at, Time.now)
-        end
-
-        it "doesn't send the email to the user" do
-          UserNotifications.expects(:mailing_list_notify).with(user, post).never
-          Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
-        end
-      end
-
     end
 
-    context "to an anonymous user with mailing list on" do
+    context "with a deleted post" do
+      let!(:post) { Fabricate(:post, user: user, deleted_at: Time.now) }
+
+      it "doesn't send the email to the user" do
+        UserNotifications.expects(:mailing_list_notify).with(user, post).never
+        Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
+      end
+    end
+
+    context "with a user_deleted post" do
+      let!(:post) { Fabricate(:post, user: user, user_deleted: true) }
+
+      it "doesn't send the email to the user" do
+        UserNotifications.expects(:mailing_list_notify).with(user, post).never
+        Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
+      end
+    end
+
+    context "with a deleted topic" do
+      let!(:post) { Fabricate(:post, user: user) }
+
+      before do
+        post.topic.update_column(:deleted_at, Time.now)
+      end
+
+      it "doesn't send the email to the user" do
+        UserNotifications.expects(:mailing_list_notify).with(user, post).never
+        Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id)
+      end
+    end
+
+    context "to an anonymous user" do
       let(:user) { Fabricate(:anonymous) }
       let!(:post) { Fabricate(:post, user: user) }
 

spec/jobs/user_email_spec.rb

@@ -192,11 +192,16 @@ describe Jobs::UserEmail do
         Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id)
       end
 
+      it "doesn't send the mail if the user is using mailing list mode" do
+        Email::Sender.any_instance.expects(:send).never
+        user.update_column(:mailing_list_mode, true)
+        Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id, post_id: post.id)
+      end
+
       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,
+        Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id, post_id: post.id)
-                                      notification_id: notification.id, post_id: post.id)
       end
 
       context 'user is suspended' do

spec/mailers/user_notifications_spec.rb

@@ -120,24 +120,6 @@ describe UserNotifications do
       # side effect, topic user is updated with post number
       tu = TopicUser.get(post.topic_id, response.user)
       expect(tu.last_emailed_post_number).to eq(response.post_number)
-
-      # 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_type: notification.notification_type,
-                                              notification_data_hash: notification.data_hash
-                                           )
-
-      expect(mail.message.class).to eq(ActionMailer::Base::NullMail)
-
-      response.user.mailing_list_mode = nil
-      mail = UserNotifications.user_replied(response.user,
-                                              post: response,
-                                              notification_type: notification.notification_type,
-                                              notification_data_hash: notification.data_hash
-                                           )
-
-      expect(mail.message.class).not_to eq(ActionMailer::Base::NullMail)
     end
   end