Rearrange mail delivery responsibilities

- each delivery job delivers only a single mail (like in the past)
- a common super-class ensures correct handling of different error conditions (similar to the past)

This new structure should allow for an efficient error handling:
- fail the mail delivery on a per-recipient basis (i.e. redeliveries on error only affect the user that did not receive a mail)
- only fail the delayed job for delivery errors (e.g. network issues, etc)
- do not fail the delayed job for rendering errors (which are likely to re-occur on each rendering)
-> only log those

app/models/watcher_notification_mailer.rb

@@ -29,9 +29,9 @@
 
 class WatcherNotificationMailer
   class << self
-    def handle_watcher(watcher_id, watcher_setter_id)
-      unless other_jobs_queued?(Watcher.find(watcher_id).watchable)
-        job = DeliverWatcherNotificationJob.new(watcher_id, watcher_setter_id)
+    def handle_watcher(watcher, watcher_setter)
+      unless other_jobs_queued?(watcher.watchable)
+        job = DeliverWatcherNotificationJob.new(watcher.id, watcher.user.id, watcher_setter.id)
         Delayed::Job.enqueue job
       end
     end

app/workers/deliver_notification_job.rb

@@ -0,0 +1,73 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+class DeliverNotificationJob
+  include OpenProject::BeforeDelayedJob
+
+  def initialize(recipient_id, sender_id)
+    @recipient_id = recipient_id
+    @sender_id = sender_id
+  end
+
+  def perform
+    # nothing to do if recipient was deleted in the meantime
+    return unless recipient
+
+    mail = User.execute_as(recipient) { build_mail }
+    if mail
+      mail.deliver
+    end
+  end
+
+  private
+
+  # To be implemented by subclasses.
+  # Actual recipient and sender User objects are passed (always non-nil).
+  # Returns a Mail::Message, or nil if no message should be sent.
+  def render_mail(recipient:, sender:)
+    raise 'SubclassResponsibility'
+  end
+
+  def build_mail
+    render_mail(recipient: recipient, sender: sender)
+  rescue StandardError => e
+    Rails.logger.error "#{self.class.name}: Unexpected error rendering a mail: #{e}"
+    # not raising, to avoid re-schedule of DelayedJob; don't expect render errors to fix themselves
+    # by retrying
+    nil
+  end
+
+  def recipient
+    @recipient ||= User.find_by(id: @recipient_id)
+  end
+
+  def sender
+    @sender ||= User.find_by(id: @sender_id) || DeletedUser.first
+  end
+end

app/workers/deliver_watcher_notification_job.rb

@@ -27,26 +27,23 @@
 # See doc/COPYRIGHT.rdoc for more details.
 #++
 
-class DeliverWatcherNotificationJob
-  include OpenProject::BeforeDelayedJob
+class DeliverWatcherNotificationJob < DeliverNotificationJob
 
-  def initialize(watcher_id, watcher_setter_id)
+  def initialize(watcher_id, recipient_id, watcher_setter_id)
     @watcher_id = watcher_id
-    @watcher_setter_id = watcher_setter_id
+
+    super(recipient_id, watcher_setter_id)
   end
 
-  def perform
-    return unless @watcher_id
+  def render_mail(recipient:, sender:)
+    return nil unless watcher
 
-    watcher = Watcher.find(@watcher_id)
-    watcher_setter = User.find(@watcher_setter_id)
+    UserMailer.work_package_watcher_added(watcher.watchable, recipient, sender)
+  end
 
-    return unless watcher && watcher_setter
+  private
 
-    mail = User.execute_as(watcher.user) {
-      UserMailer.work_package_watcher_added(watcher.watchable, watcher.user, watcher_setter)
-    }
-
-    mail.deliver
+  def watcher
+    @watcher ||= Watcher.find_by(id: @watcher_id)
   end
 end

app/workers/deliver_work_package_notification_job.rb

@@ -27,16 +27,15 @@
 # See doc/COPYRIGHT.rdoc for more details.
 #++
 
-class DeliverWorkPackageNotificationJob
-  include OpenProject::BeforeDelayedJob
+class DeliverWorkPackageNotificationJob < DeliverNotificationJob
 
-  def initialize(journal_id, author_id)
+  def initialize(journal_id, recipient_id, author_id)
     @journal_id = journal_id
-    @author_id = author_id
+    super(recipient_id, author_id)
   end
 
-  def perform
-    return unless raw_journal # abort, assuming that the underlying WP was deleted
+  def render_mail(recipient:, sender:)
+    return nil unless raw_journal # abort, assuming that the underlying WP was deleted
 
     journal = find_aggregated_journal
 
@@ -44,39 +43,25 @@ class DeliverWorkPackageNotificationJob
     # before queuing a notification
     raise 'aggregated journal got outdated' unless journal
 
-    notification_receivers(work_package).uniq.each do |recipient|
-      mail = User.execute_as(recipient) {
-        if journal.initial?
-          UserMailer.work_package_added(recipient, journal, author)
-        else
-          UserMailer.work_package_updated(recipient, journal, author)
-        end
-      }
-
-      mail.deliver
+    if journal.initial?
+      UserMailer.work_package_added(recipient, journal, sender)
+    else
+      UserMailer.work_package_updated(recipient, journal, sender)
     end
   end
 
   private
 
+  def raw_journal
+    @raw_journal ||= Journal.find_by(id: @journal_id)
+  end
+
   def find_aggregated_journal
     wp_journals = Journal::AggregatedJournal.aggregated_journals(journable: work_package)
     wp_journals.detect { |journal| journal.version == raw_journal.version }
   end
 
-  def notification_receivers(work_package)
-    work_package.recipients + work_package.watcher_recipients
-  end
-
-  def raw_journal
-    @raw_journal ||= Journal.find_by(id: @journal_id)
-  end
-
   def work_package
     @work_package ||= raw_journal.journable
   end
-
-  def author
-    @author ||= User.find_by(id: @author_id) || DeletedUser.first
-  end
 end

app/workers/enqueue_work_package_notification_job.rb

@@ -63,8 +63,10 @@ class EnqueueWorkPackageNotificationJob
   end
 
   def deliver_notifications_for(journal)
-    job = DeliverWorkPackageNotificationJob.new(journal.id, @author_id)
-    Delayed::Job.enqueue job
+    notification_receivers(work_package).each do |recipient|
+      job = DeliverWorkPackageNotificationJob.new(journal.id, recipient.id, @author_id)
+      Delayed::Job.enqueue job
+    end
   end
 
   def raw_journal
@@ -74,4 +76,8 @@ class EnqueueWorkPackageNotificationJob
   def work_package
     @work_package ||= raw_journal.journable
   end
+
+  def notification_receivers(work_package)
+    work_package.recipients + work_package.watcher_recipients
+  end
 end

config/initializers/subscribe_listeners.rb

@@ -32,5 +32,5 @@ OpenProject::Notifications.subscribe('journal_created') do |payload|
 end
 
 OpenProject::Notifications.subscribe('watcher_added') do |payload|
-  WatcherNotificationMailer.handle_watcher(payload[:watcher_id], payload[:watcher_setter_id])
+  WatcherNotificationMailer.handle_watcher(payload[:watcher], payload[:watcher_setter])
 end

lib/services/create_watcher.rb

@@ -42,8 +42,8 @@ class Services::CreateWatcher
         @work_package.watchers << @watcher
         success.(created: true)
         OpenProject::Notifications.send('watcher_added',
-                                        watcher_id: @watcher.id,
-                                        watcher_setter_id: User.current.id)
+                                        watcher: @watcher,
+                                        watcher_setter: User.current)
       else
         failure.(@watcher)
       end

spec/workers/mail_notification_jobs/deliver_watcher_notification_job_spec.rb

@@ -38,10 +38,8 @@ describe DeliverWatcherNotificationJob, type: :model do
   end
   let(:work_package) { FactoryGirl.build(:work_package, project: project) }
   let(:watcher) { FactoryGirl.create(:watcher, watchable: work_package, user: watcher_user) }
-  let(:watcher_id) { watcher.id }
-  let(:watcher_setter_id) { watcher_setter.id }
 
-  subject { described_class.new(watcher_id, watcher_setter_id) }
+  subject { described_class.new(watcher.id, watcher_user.id, watcher_setter.id) }
 
   before do
     # make sure no actual calls make it into the UserMailer

spec/workers/mail_notification_jobs/deliver_work_package_notification_job_spec.rb

@@ -39,11 +39,10 @@ describe DeliverWorkPackageNotificationJob, type: :model do
   let(:work_package) {
     FactoryGirl.create(:work_package,
                        project: project,
-                       author: author,
-                       assigned_to: recipient)
+                       author: author)
   }
   let(:journal) { work_package.journals.first }
-  subject { described_class.new(journal.id, author.id) }
+  subject { described_class.new(journal.id, recipient.id, author.id) }
 
   before do
     # make sure no actual calls make it into the UserMailer
@@ -95,8 +94,8 @@ describe DeliverWorkPackageNotificationJob, type: :model do
       work_package.save!
     end
 
-    it 'raises an error' do
-      expect { subject.perform }.to raise_error('aggregated journal got outdated')
+    it 'raises no observable error' do
+      expect { subject.perform }.not_to raise_error
     end
   end
 
@@ -148,17 +147,25 @@ describe DeliverWorkPackageNotificationJob, type: :model do
     end
   end
 
-  describe 'exceptions' do
-    describe 'exceptions should be raised' do
-      before do
-        mail = double('mail')
-        allow(mail).to receive(:deliver).and_raise(SocketError)
-        expect(UserMailer).to receive(:work_package_added).and_return(mail)
-      end
+  describe 'exceptions during delivery' do
+    before do
+      mail = double('mail')
+      allow(mail).to receive(:deliver).and_raise(SocketError)
+      expect(UserMailer).to receive(:work_package_added).and_return(mail)
+    end
 
-      it 'raises the error' do
-        expect { subject.perform }.to raise_error(SocketError)
-      end
+    it 'raises the error' do
+      expect { subject.perform }.to raise_error(SocketError)
+    end
+  end
+
+  describe 'exceptions during rendering' do
+    before do
+      expect(UserMailer).to receive(:work_package_added).and_raise('not today!')
+    end
+
+    it 'swallows the error' do
+      expect { subject.perform }.not_to raise_error
     end
   end
 end