diff --git a/Changelog.md b/Changelog.md index a5ca748b9..81c1b3f18 100644 --- a/Changelog.md +++ b/Changelog.md @@ -29,6 +29,7 @@ Ruby 2.0 is no longer officially supported. * Improved handling of reshares with deleted roots [#5968](https://github.com/diaspora/diaspora/pull/5968) * Remove two unused methods [#5970](https://github.com/diaspora/diaspora/pull/5970) * Refactored the Logger to add basic logrotating and more useful timestamps [#5975](https://github.com/diaspora/diaspora/pull/5975) +* Gracefully handle mailer failures if a like is already deleted again [#5983](https://github.com/diaspora/diaspora/pull/5983) ## Bug fixes * Disable auto follow back on aspect deletion [#5846](https://github.com/diaspora/diaspora/pull/5846) @@ -53,6 +54,7 @@ Ruby 2.0 is no longer officially supported. * Fix a freeze in new post parsing [#5965](https://github.com/diaspora/diaspora/pull/5965) * Add case insensitive unconfirmed email addresses as authentication key [#5967](https://github.com/diaspora/diaspora/pull/5967) * Fix liking on single post views when accessed via GUID [#5978](https://github.com/diaspora/diaspora/pull/5978) +* Gracefully handle mailer failures when a like is already deleted again [#5983](https://github.com/diaspora/diaspora/pull/5983) ## Features * Hide post title of limited post in comment notification email [#5843](https://github.com/diaspora/diaspora/pull/5843) diff --git a/app/mailers/notification_mailers/base.rb b/app/mailers/notification_mailers/base.rb index 086228b06..b3cc8136a 100644 --- a/app/mailers/notification_mailers/base.rb +++ b/app/mailers/notification_mailers/base.rb @@ -8,8 +8,8 @@ module NotificationMailers def initialize(recipient_id, sender_id=nil, *args) @headers = {} - @recipient = User.find_by_id(recipient_id) - @sender = Person.find_by_id(sender_id) if sender_id.present? + @recipient = User.find(recipient_id) + @sender = Person.find(sender_id) if sender_id.present? log_mail(recipient_id, sender_id, self.class.to_s.underscore) @@ -29,11 +29,12 @@ module NotificationMailers end private + def default_headers headers = { - :from => AppConfig.mail.sender_address.get, - :host => "#{AppConfig.pod_uri.host}", - :to => name_and_address(@recipient.name, @recipient.email) + from: AppConfig.mail.sender_address.get, + host: "#{AppConfig.pod_uri.host}", + to: name_and_address(@recipient.name, @recipient.email) } headers[:from] = "\"#{@sender.name} (diaspora*)\" <#{AppConfig.mail.sender_address}>" if @sender.present? @@ -46,14 +47,15 @@ module NotificationMailers end def log_mail(recipient_id, sender_id, type) - log_string = "event=mail mail_type=#{type} recipient_id=#{recipient_id} sender_id=#{sender_id}" - if @recipient && @sender - log_string << "models_found=true sender_handle=#{@sender.diaspora_handle} recipient_handle=#{@recipient.diaspora_handle}" - else - log_string << "models_found=false" - end + log_string = "event=mail mail_type=#{type} recipient_id=#{recipient_id} sender_id=#{sender_id} " \ + " recipient_handle=#{@recipient.diaspora_handle}" + log_string << " sender_handle=#{@sender.diaspora_handle}" if sender_id.present? - Rails.logger.info(log_string) + logger.info(log_string) + end + + def logger + @logger ||= ::Logging::Logger[self] end end end diff --git a/app/workers/mail/liked.rb b/app/workers/mail/liked.rb index a80555859..d2b2748c9 100644 --- a/app/workers/mail/liked.rb +++ b/app/workers/mail/liked.rb @@ -2,9 +2,12 @@ module Workers module Mail class Liked < Base sidekiq_options queue: :mail - + def perform(recipient_id, sender_id, like_id) Notifier.liked(recipient_id, sender_id, like_id).deliver_now + rescue ActiveRecord::RecordNotFound => e + logger.warn("failed to send liked notification mail: #{e.message}") + raise e unless e.message.start_with?("Couldn't find Like with") end end end diff --git a/spec/workers/mail/liked_spec.rb b/spec/workers/mail/liked_spec.rb new file mode 100644 index 000000000..4d91d16e7 --- /dev/null +++ b/spec/workers/mail/liked_spec.rb @@ -0,0 +1,38 @@ +require "spec_helper" + +describe Workers::Mail::Liked do + describe "#perfom" do + it "should call .deliver_now on the notifier object" do + sm = FactoryGirl.build(:status_message, author: bob.person, public: true) + like = FactoryGirl.build(:like, author: alice.person, target: sm) + + mail_double = double + expect(mail_double).to receive(:deliver_now) + expect(Notifier).to receive(:liked).with(bob.id, like.author.id, like.id).and_return(mail_double) + + Workers::Mail::Liked.new.perform(bob.id, like.author.id, like.id) + end + + it "should not fail if the like is not found" do + sm = FactoryGirl.build(:status_message, author: bob.person, public: true) + like = FactoryGirl.build(:like, author: alice.person, target: sm) + + expect(Notifier).to receive(:liked).with(bob.id, like.author.id, like.id) + .and_raise(ActiveRecord::RecordNotFound.new("Couldn't find Like with 'id'=42")) + + Workers::Mail::Liked.new.perform(bob.id, like.author.id, like.id) + end + + it "should fail if the sender is not found" do + sm = FactoryGirl.build(:status_message, author: bob.person, public: true) + like = FactoryGirl.build(:like, author: alice.person, target: sm) + + expect(Notifier).to receive(:liked).with(bob.id, like.author.id, like.id) + .and_raise(ActiveRecord::RecordNotFound.new("Couldn't find Person with 'id'=42")) + + expect { + Workers::Mail::Liked.new.perform(bob.id, like.author.id, like.id) + }.to raise_error(ActiveRecord::RecordNotFound) + end + end +end