From 4ee163393f00a48ebf4ffb1919b308159818f0ef Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 7 Sep 2015 15:20:33 +0200 Subject: [PATCH] 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 | 6 +- app/workers/deliver_notification_job.rb | 73 +++++++++++++++++++ .../deliver_watcher_notification_job.rb | 25 +++---- .../deliver_work_package_notification_job.rb | 41 ++++------- .../enqueue_work_package_notification_job.rb | 10 ++- config/initializers/subscribe_listeners.rb | 2 +- lib/services/create_watcher.rb | 4 +- .../deliver_watcher_notification_job_spec.rb | 4 +- ...iver_work_package_notification_job_spec.rb | 37 ++++++---- 9 files changed, 134 insertions(+), 68 deletions(-) create mode 100644 app/workers/deliver_notification_job.rb diff --git a/app/models/watcher_notification_mailer.rb b/app/models/watcher_notification_mailer.rb index 163af6c9b3d..0080c9b0b8f 100644 --- a/app/models/watcher_notification_mailer.rb +++ b/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 diff --git a/app/workers/deliver_notification_job.rb b/app/workers/deliver_notification_job.rb new file mode 100644 index 00000000000..4fa75873568 --- /dev/null +++ b/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 diff --git a/app/workers/deliver_watcher_notification_job.rb b/app/workers/deliver_watcher_notification_job.rb index 2bbea8c31af..f63cacc0023 100644 --- a/app/workers/deliver_watcher_notification_job.rb +++ b/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 diff --git a/app/workers/deliver_work_package_notification_job.rb b/app/workers/deliver_work_package_notification_job.rb index 11792196575..d8d7a34b1fd 100644 --- a/app/workers/deliver_work_package_notification_job.rb +++ b/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 diff --git a/app/workers/enqueue_work_package_notification_job.rb b/app/workers/enqueue_work_package_notification_job.rb index 2fc3d404058..8d5ded7d697 100644 --- a/app/workers/enqueue_work_package_notification_job.rb +++ b/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 diff --git a/config/initializers/subscribe_listeners.rb b/config/initializers/subscribe_listeners.rb index 86b12e2b36f..ae36fe561bb 100644 --- a/config/initializers/subscribe_listeners.rb +++ b/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 diff --git a/lib/services/create_watcher.rb b/lib/services/create_watcher.rb index 36150ed2435..32fa2b98f9a 100644 --- a/lib/services/create_watcher.rb +++ b/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 diff --git a/spec/workers/mail_notification_jobs/deliver_watcher_notification_job_spec.rb b/spec/workers/mail_notification_jobs/deliver_watcher_notification_job_spec.rb index ce66df1373f..0c05a665656 100644 --- a/spec/workers/mail_notification_jobs/deliver_watcher_notification_job_spec.rb +++ b/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 diff --git a/spec/workers/mail_notification_jobs/deliver_work_package_notification_job_spec.rb b/spec/workers/mail_notification_jobs/deliver_work_package_notification_job_spec.rb index dd8ba58c742..b6662fd4e57 100644 --- a/spec/workers/mail_notification_jobs/deliver_work_package_notification_job_spec.rb +++ b/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