Add Message-ID Tracking on receipts

Refactor MailDispatcher to consume receipts
Extract RecipentFilter into own class.

app/models/mailboxer/message.rb

@@ -34,8 +34,8 @@ def deliver(reply = false, should_clean = true)
     temp_receipts << sender_receipt
 
     if temp_receipts.all?(&:valid?)
+      Mailboxer::MailDispatcher.new(self, temp_receipts).call
       temp_receipts.each(&:save!)
-      Mailboxer::MailDispatcher.new(self, recipients).call
 
       conversation.touch if reply
 

app/models/mailboxer/notification.rb

@@ -84,8 +84,8 @@ def deliver(should_clean = true, send_mail = true)
     temp_receipts = recipients.map { |r| build_receipt(r, nil, false) }
 
     if temp_receipts.all?(&:valid?)
+      Mailboxer::MailDispatcher.new(self, temp_receipts).call if send_mail
       temp_receipts.each(&:save!)   #Save receipts
-      Mailboxer::MailDispatcher.new(self, recipients).call if send_mail
       self.recipients = nil
     end
 
@@ -96,7 +96,8 @@ def deliver(should_clean = true, send_mail = true)
   #Returns the recipients of the Notification
   def recipients
     return Array.wrap(@recipients) unless @recipients.blank?
-    @recipients = receipts.includes(:receiver).map { |receipt| receipt.receiver }
+    recipients  = receipts.includes(:receiver).map(&:receiver)
+    @recipients = Mailboxer::RecipientFilter.new(self, recipients).call
   end
 
   #Returns the receipt for the participant

db/migrate/20151103080417_add_delivery_tracking_info_to_mailboxer_receipts.rb

@@ -0,0 +1,7 @@
+class AddDeliveryTrackingInfoToMailboxerReceipts < ActiveRecord::Migration
+  def change
+    add_column :mailboxer_receipts, :is_delivered, :boolean, default: false
+    add_column :mailboxer_receipts, :delivery_method, :string
+    add_column :mailboxer_receipts, :message_id, :string
+  end
+end

lib/mailboxer.rb

@@ -7,8 +7,6 @@ module Models
   @@default_from = "[email protected]"
   mattr_accessor :uses_emails
   @@uses_emails = true
-  mattr_accessor :mailer_wants_array
-  @@mailer_wants_array = false
   mattr_accessor :search_enabled
   @@search_enabled = false
   mattr_accessor :search_engine
@@ -41,3 +39,4 @@ def protected_attributes?
 require 'mailboxer/engine'
 require 'mailboxer/cleaner'
 require 'mailboxer/mail_dispatcher'
+require 'mailboxer/recipient_filter'

lib/mailboxer/mail_dispatcher.rb

@@ -1,21 +1,18 @@
 module Mailboxer
   class MailDispatcher
+    attr_reader :mailable, :receipts
 
-    attr_reader :mailable, :recipients
-
-    def initialize(mailable, recipients)
-      @mailable, @recipients = mailable, recipients
+    def initialize(mailable, receipts)
+      @mailable, @receipts = mailable, receipts
     end
 
     def call
+      # Why is this here, the dispatch should dispatch email not decide to dispatch
       return false unless Mailboxer.uses_emails
-      if Mailboxer.mailer_wants_array
-        send_email(filtered_recipients)
-      else
-        filtered_recipients.each do |recipient|
-          email_to = recipient.send(Mailboxer.email_method, mailable)
-          send_email(recipient) if email_to.present?
-        end
+
+      receipts.map do |receipt|
+        email_to = receipt.receiver.send(Mailboxer.email_method, mailable)
+        send_email(receipt) if email_to.present?
       end
     end
 
@@ -27,22 +24,22 @@ def mailer
       Mailboxer.send(method) || "#{mailable.class}Mailer".constantize
     end
 
-    # recipients can be filtered on a conversation basis
-    def filtered_recipients
-      return recipients unless mailable.respond_to?(:conversation)
-
-      recipients.each_with_object([]) do |recipient, array|
-        array << recipient if mailable.conversation.has_subscriber?(recipient)
-      end
-    end
-
-    def send_email(recipient)
+    def send_email(receipt)
       if Mailboxer.custom_deliver_proc
-        Mailboxer.custom_deliver_proc.call(mailer, mailable, recipient)
+        Mailboxer.custom_deliver_proc.call(mailer, mailable, receipt.receiver)
       else
-        email = mailer.send_email(mailable, recipient)
-        email.respond_to?(:deliver_now) ? email.deliver_now : email.deliver
+        default_send_email(receipt)
       end
     end
+
+    def default_send_email(receipt)
+      mail = mailer.send_email(mailable, receipt.receiver)
+      mail.respond_to?(:deliver_now) ? mail.deliver_now : mail.deliver
+      receipt.assign_attributes(
+        :delivery_method => :email,
+        :message_id => mail.message_id
+      )
+      mail
+    end
   end
 end

lib/mailboxer/recipient_filter.rb

@@ -0,0 +1,17 @@
+module Mailboxer
+  class RecipientFilter
+    attr_reader :mailable, :recipients
+    def initialize(mailable, recipients)
+      @mailable, @recipients = mailable, recipients
+    end
+
+    # recipients can be filtered on a conversation basis
+    def call
+      return recipients unless mailable.respond_to?(:conversation)
+
+      recipients.each_with_object([]) do |recipient, array|
+        array << recipient if mailable.conversation.has_subscriber?(recipient)
+      end
+    end
+  end
+end

spec/dummy/db/migrate/20151103202534_add_missing_indices.mailboxer_engine.rb

@@ -0,0 +1,20 @@
+# This migration comes from mailboxer_engine (originally 20131206080417)
+class AddMissingIndices < ActiveRecord::Migration
+  def change
+    # We'll explicitly specify its name, as the auto-generated name is too long and exceeds 63
+    # characters limitation.
+    add_index :mailboxer_conversation_opt_outs, [:unsubscriber_id, :unsubscriber_type],
+      name: 'index_mailboxer_conversation_opt_outs_on_unsubscriber_id_type'
+    add_index :mailboxer_conversation_opt_outs, :conversation_id
+
+    add_index :mailboxer_notifications, :type
+    add_index :mailboxer_notifications, [:sender_id, :sender_type]
+
+    # We'll explicitly specify its name, as the auto-generated name is too long and exceeds 63
+    # characters limitation.
+    add_index :mailboxer_notifications, [:notified_object_id, :notified_object_type],
+      name: 'index_mailboxer_notifications_on_notified_object_id_and_type'
+
+    add_index :mailboxer_receipts, [:receiver_id, :receiver_type]
+  end
+end

spec/dummy/db/migrate/20151103202535_add_delivery_tracking_info_to_mailboxer_receipts.mailboxer_engine.rb

@@ -0,0 +1,8 @@
+# This migration comes from mailboxer_engine (originally 20151103080417)
+class AddDeliveryTrackingInfoToMailboxerReceipts < ActiveRecord::Migration
+  def change
+    add_column :mailboxer_receipts, :is_delivered, :boolean, default: false
+    add_column :mailboxer_receipts, :delivery_method, :string
+    add_column :mailboxer_receipts, :message_id, :string
+  end
+end

spec/mailboxer/mail_dispatcher_spec.rb

@@ -2,12 +2,15 @@
 
 describe Mailboxer::MailDispatcher do
 
-  subject(:instance) { described_class.new(mailable, recipients) }
+  subject(:instance) { described_class.new(mailable, receipts) }
 
   let(:mailable)   { Mailboxer::Notification.new }
-  let(:recipient1) { double 'recipient1', mailboxer_email: ''  }
-  let(:recipient2) { double 'recipient2', mailboxer_email: '[email protected]'  }
-  let(:recipients) { [ recipient1, recipient2 ] }
+  let(:recipient1) { double 'recipient1', id: 1, mailboxer_email: '' }
+  let(:recipient2) { double 'recipient2', id: 2, mailboxer_email: '[email protected]'}
+  let(:receipt1) { double 'receipt1', id: 1, receiver: recipient1 }
+  let(:receipt2) { double 'receipt2', id: 2, receiver: recipient2  }
+
+  let(:receipts) { [ receipt1, receipt2 ] }
 
   describe "call" do
     context "no emails" do
@@ -16,19 +19,10 @@
       its(:call) { should be false }
     end
 
-    context "mailer wants array" do
-      before { Mailboxer.mailer_wants_array = true  }
-      after  { Mailboxer.mailer_wants_array = false }
+    context "mailer doesn't want array" do
       it 'sends collection' do
-        expect(subject).to receive(:send_email).with(recipients)
-        subject.call
-      end
-    end
-
-    context "mailer doesnt want array" do
-      it 'sends collection' do
-        expect(subject).not_to receive(:send_email).with(recipient1) #email is blank
-        expect(subject).to receive(:send_email).with(recipient2)
+        expect(subject).not_to receive(:send_email).with(receipt1) #email is blank
+        expect(subject).to receive(:send_email).with(receipt2)
         subject.call
       end
     end
@@ -49,24 +43,25 @@
       after  { Mailboxer.custom_deliver_proc = nil     }
       it "triggers proc" do
         expect(my_proc).to receive(:call).with(mailer, mailable, recipient1)
-        subject.send :send_email, recipient1
+        subject.send :send_email, receipt1
       end
     end
 
     context "without custom_deliver_proc" do
-      let(:email) { double :email }
+      let(:email) { double :email, message_id: '[email protected]' }
 
       it "triggers standard deliver chain" do
         expect(mailer).to receive(:send_email).with(mailable, recipient1).and_return email
+        expect(receipt1).to receive(:assign_attributes).with({:delivery_method=>:email, :message_id=>"[email protected]"}).and_return email
         expect(email).to receive :deliver
 
-        subject.send :send_email, recipient1
+        subject.send :send_email, receipt1
       end
     end
   end
 
   describe "mailer" do
-    let(:recipients) { [] }
+    let(:receipts) { [] }
 
     context "mailable is a Message" do
       let(:mailable) { Mailboxer::Notification.new }
@@ -93,22 +88,4 @@
       end
     end
   end
-
-  describe "filtered_recipients" do
-    context "responds to conversation" do
-      let(:conversation) { double 'conversation' }
-      let(:mailable)     { double 'mailable', :conversation => conversation }
-      before(:each) do
-        expect(conversation).to receive(:has_subscriber?).with(recipient1).and_return false
-        expect(conversation).to receive(:has_subscriber?).with(recipient2).and_return true
-      end
-
-      its(:filtered_recipients){ should eq [recipient2] }
-    end
-
-    context 'doesnt respond to conversation' do
-      let(:mailable) { double 'mailable' }
-      its(:filtered_recipients){ should eq recipients }
-    end
-  end
 end

spec/mailboxer/recipient_filter_spec.rb

@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe Mailboxer::RecipientFilter do
+  subject(:instance) { described_class.new(mailable, recipients) }
+  let(:recipient1) { double 'recipient1', id: 1, mailboxer_email: '' }
+  let(:recipient2) { double 'recipient2', id: 2, mailboxer_email: '[email protected]'}
+  let(:recipients) { [ recipient1, recipient2 ] }
+
+  describe "call" do
+    context "responds to conversation" do
+      let(:conversation) { double 'conversation' }
+      let(:mailable)     { double 'mailable', :conversation => conversation }
+      before(:each) do
+        expect(conversation).to receive(:has_subscriber?).with(recipient1).and_return false
+        expect(conversation).to receive(:has_subscriber?).with(recipient2).and_return true
+      end
+
+      its(:call){ should eq [recipient2] }
+    end
+
+    context 'doesnt respond to conversation' do
+      let(:mailable) { double 'mailable' }
+      its(:call){ should eq recipients }
+    end
+  end
+end

spec/mailers/message_mailer_spec.rb

@@ -60,36 +60,6 @@ def sent_to?(entity)
       end
     end
   end
-
-  context "when mailer_wants_array is false" do
-    it_behaves_like 'message_mailer'
-  end
-
-  context "mailer_wants_array is true" do
-    class ArrayMailer < Mailboxer::MessageMailer
-      default template_path: 'mailboxer/message_mailer'
-
-      def new_message_email(message, receivers)
-        receivers.each { |receiver| super(message, receiver) if receiver.mailboxer_email(message).present? }
-      end
-
-      def reply_message_email(message, receivers)
-        receivers.each { |receiver| super(message, receiver) if receiver.mailboxer_email(message).present? }
-      end
-    end
-
-    before :all do
-      Mailboxer.mailer_wants_array = true
-      Mailboxer.message_mailer = ArrayMailer
-    end
-
-    after :all do
-      Mailboxer.mailer_wants_array = false
-      Mailboxer.message_mailer = Mailboxer::MessageMailer
-    end
-
-    it_behaves_like 'message_mailer'
-  end
 end
 
 def print_emails