Merge branch 'stable' into develop

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)

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)
+      @recipient = User.find(recipient_id)
-      @sender = Person.find_by_id(sender_id) if sender_id.present?
+      @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,
+        from: AppConfig.mail.sender_address.get,
-        :host => "#{AppConfig.pod_uri.host}",
+        host: "#{AppConfig.pod_uri.host}",
-        :to => name_and_address(@recipient.name, @recipient.email)
+        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}"
+      log_string = "event=mail mail_type=#{type} recipient_id=#{recipient_id} sender_id=#{sender_id} " \
-      if @recipient && @sender
+                   " recipient_handle=#{@recipient.diaspora_handle}"
-        log_string << "models_found=true sender_handle=#{@sender.diaspora_handle} recipient_handle=#{@recipient.diaspora_handle}"
+      log_string << " sender_handle=#{@sender.diaspora_handle}" if sender_id.present?
-      else
-        log_string << "models_found=false"
-      end
 
-      Rails.logger.info(log_string)
+      logger.info(log_string)
+    end
+
+    def logger
+      @logger ||= ::Logging::Logger[self]
     end
   end
 end

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

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