[Bug Fix] resolve email threads

Addresses: #1234

app/mailboxes/e_mailbox.rb

@@ -1,18 +1,23 @@
 class EMailbox < ApplicationMailbox
 
   def process
-    subject = mail.subject
+    subject = sanitize_email_subject_prefixes(mail.subject)
     recipients = mail.to.map{|email| Mail::Address.new(email)}
     recipients.each do |address|
       schema_domain = address.local == Subdomain::ROOT_DOMAIN_EMAIL_NAME ? 'public' : address.local
       next if !Subdomain.find_by(name: schema_domain)
       Apartment::Tenant.switch schema_domain do
         mailbox = Mailbox.first_or_create
         if mailbox
-          message_thread = MessageThread.find_or_create_by(
-            current_email_message_id: mail.in_reply_to
-          )
-          message_thread.update(recipients: mail.from, subject: subject)
+          if mail.in_reply_to && in_reply_to_message = Message.find_by(email_message_id: mail.in_reply_to)
+            message_thread = in_reply_to_message.message_thread
+          else
+            message_thread = MessageThread.find_or_create_by(
+              recipients: mail.from,
+              subject: subject
+            )
+          end
+
           message = Message.create!(
             email_message_id: mail.message_id,
             message_thread: message_thread,
@@ -54,7 +59,6 @@ def multipart_attached
     return blobs
   end
 
-
   def body
     if mail.multipart? && mail.html_part
       document = Nokogiri::HTML(mail.html_part.body.decoded)
@@ -77,4 +81,13 @@ def body
       mail.decoded
     end
   end
+
+  private
+  def sanitize_email_subject_prefixes(subject)
+    return subject if subject.blank?
+
+    # There are some standard email subject prefixes which gets prepended in the email's subject.
+    # examples: 'Re: ', 're: ', 'FWD: ', 'Fwd: ', 'Fw: '
+    subject.gsub(/^((re|fw(d)?): )/i, '')
+  end
 end

app/mailers/e_mailer.rb

@@ -8,8 +8,6 @@ def ship
             else
               "#{@subdomain.name}@#{ENV["APP_HOST"]}"
             end
-    message_id = "#{Digest::SHA2.hexdigest(Time.now.to_i.to_s)}@#{Apartment::Tenant.current}@#{ENV['APP_HOST']}"
-    @message_thread.update(current_email_message_id: message_id)
 
     # if additional attachment params are passed, they are included in the email
     unless params[:attachments].nil?
@@ -21,12 +19,42 @@ def ship
       end
     end
 
-    mail(
+    mail_settings = {
       # This will make the mail addresses visible to all (no Blank Carbon Copy)
       to: @message_thread.recipients, 
       subject: @message_thread.subject,
       from: @from,
-      message_id: message_id
-    )
+      message_id: email_message_id(@message)
+    }.merge(email_headers)
+
+    mail(mail_settings)
+  end
+
+  private
+  def email_message_id(message)
+    return '' if message.blank?
+
+    "<#{message.email_message_id}>"
+  end
+
+  def in_reply_to_message_id
+    last_message = messages_in_thread.last
+
+    email_message_id(last_message)
+  end
+
+  def thread_references
+    messages_in_thread.map { |message| email_message_id(message) }.join(" ")
+  end
+
+  def messages_in_thread
+    @message_thread.messages.where.not(id: @message.id).reorder(created_at: :asc)
+  end
+
+  def email_headers
+    {
+      in_reply_to: in_reply_to_message_id,
+      references: thread_references
+    }.reject { |_, v| v.blank? }
   end
 end

app/models/message.rb

@@ -10,10 +10,26 @@ class Message < ApplicationRecord
 
   default_scope { order(created_at: 'DESC') }
 
+  before_save :generate_message_id
+  after_create :update_message_thread_current_email_message_id
   after_create_commit :deliver
 
   private
 
+  def generate_message_id
+    if self.email_message_id.blank?
+      # RFC format for message-id: local-part@domain
+      # '@' is a identifier to separate local part and the domain part. So, it cannot be used multiple times.
+      self.email_message_id = "#{Digest::SHA2.hexdigest(Time.now.to_i.to_s)}.#{Apartment::Tenant.current}@#{ENV['APP_HOST']}"
+    end
+  end
+
+  def update_message_thread_current_email_message_id
+    if self.message_thread
+      self.message_thread.update(current_email_message_id: self.email_message_id)
+    end
+  end
+
   def deliver
     if !self.from
       # if there is no from attribute, we can assume that its an outgoing message

test/mailboxes/e_mailbox_test.rb

@@ -126,40 +126,249 @@ class EMailboxTest < ActionMailbox::TestCase
     end
   end
 
-  test 'message threads' do
+  test 'emails are mapped into same threads if the subjects are same, recipients are same and the in-reply-to header is not present' do
     Apartment::Tenant.switch @restarone_subdomain do      
       perform_enqueued_jobs do
         assert_difference "MessageThread.all.reload.size" , +1 do        
           assert_difference "Message.all.reload.size", +2 do          
             subject_line = "Hello world!"
             receive_inbound_email_from_mail \
-            to: '"Don Restarone" <[email protected]>',
-            from: '"else" <[email protected]>',
-            subject: subject_line,
-            body: "Hello?"
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+
             assert MessageThread.all.last.subject
             assert Message.last.from
+
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+          end
+        end
+      end
+    end
+  end
+
+  test 'emails are mapped into same threads if the subjects differ by standard prefixes for replies and email forwarding, recipients are same and the in-reply-to header is not present' do
+    Apartment::Tenant.switch @restarone_subdomain do      
+      perform_enqueued_jobs do
+        assert_difference "MessageThread.all.reload.size" , +1 do        
+          assert_difference "Message.all.reload.size", +2 do          
+            subject_line = "Hello world!"
             receive_inbound_email_from_mail \
               to: '"Don Restarone" <[email protected]>',
               from: '"else" <[email protected]>',
               subject: subject_line,
               body: "Hello?"
+
+            assert MessageThread.all.last.subject
+            assert Message.last.from
+
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: 'Fw: ' + subject_line,
+              body: "Hello?"
+          end
+        end
+      end
+    end
+  end
+
+  test 'emails are mapped into different threads if the subjects differ by standard prefixed for replies and email forwarding but recipients are different and the in-reply-to header is not present' do
+    Apartment::Tenant.switch @restarone_subdomain do      
+      perform_enqueued_jobs do
+        assert_difference "MessageThread.all.reload.size" , +2 do        
+          assert_difference "Message.all.reload.size", +2 do          
+            subject_line = "Hello world!"
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+
+            assert MessageThread.all.last.subject
+            assert Message.last.from
+
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"if.else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+          end
+        end
+      end
+    end
+  end
+
+  test 'emails are mapped into different threads if the subjects are same but recipients are different and the in-reply-to header is not present' do
+    Apartment::Tenant.switch @restarone_subdomain do      
+      perform_enqueued_jobs do
+        assert_difference "MessageThread.all.reload.size" , +2 do        
+          assert_difference "Message.all.reload.size", +2 do          
+            subject_line = "Hello world!"
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+
+            assert MessageThread.all.last.subject
+            assert Message.last.from
+
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"if.else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+          end
+        end
+      end
+    end
+  end
+
+  test 'emails are mapped into same threads when the in-reply-to header refers to one of the email from the same thread' do
+    Apartment::Tenant.switch @restarone_subdomain do      
+      perform_enqueued_jobs do
+        assert_difference "MessageThread.all.reload.size" , +1 do        
+          assert_difference "Message.all.reload.size", +2 do          
+            subject_line = "Hello world!"
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?"
+            assert MessageThread.all.last.subject
+            assert Message.last.from
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?",
+              'in-reply-to': "<#{Message.last.email_message_id}>"
           end
         end
 
         assert_no_difference "MessageThread.all.reload.size" do        
           assert_difference "Message.all.reload.size", +2 do
             receive_inbound_email_from_mail \
-            to: '"Don Restarone" <[email protected]>',
-            from: '"else" <[email protected]>',
-            subject: 'subject_line',
-            body: "Hello?"
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: 'subject_line',
+              body: "Hello?",
+              'in-reply-to': "<#{Message.last.email_message_id}>"
 
             receive_inbound_email_from_mail \
               to: '"Don Restarone" <[email protected]>',
               from: '"else" <[email protected]>',
               subject: 'subject_line 22',
+              body: "Hello?",
+              'in-reply-to': "<#{Message.last.email_message_id}>"
+          end
+        end
+      end
+    end
+  end
+
+  test 'emails are mapped into same threads when the in-reply-to header refers to one of the email from the same thread even if the subject and recipients are different' do
+    Apartment::Tenant.switch @restarone_subdomain do      
+      perform_enqueued_jobs do
+        assert_difference "MessageThread.all.reload.size" , +1 do        
+          assert_difference "Message.all.reload.size", +2 do          
+            subject_line = "Hello world!"
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: subject_line,
               body: "Hello?"
+            assert MessageThread.all.last.subject
+            assert Message.last.from
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"if.else" <[email protected]>',
+              subject: 'Test: ' + subject_line,
+              body: "Hello?",
+              'in-reply-to': "<#{Message.last.email_message_id}>"
+          end
+        end
+
+        assert_no_difference "MessageThread.all.reload.size" do        
+          assert_difference "Message.all.reload.size", +2 do
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"else" <[email protected]>',
+              subject: 'subject_line',
+              body: "Hello?",
+              'in-reply-to': "<#{Message.last.email_message_id}>"
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"when.else" <[email protected]>',
+              subject: 'subject_line 22',
+              body: "Hello?",
+              'in-reply-to': "<#{Message.last.email_message_id}>"
+          end
+        end
+      end
+    end
+  end
+
+  # This is how github sends email notifications.
+  test 'emails are mapped into same threads when the in-reply-to header refers to a different non-existent email message-id but the subject and recipients are same' do
+    in_reply_to = "<restarone/violet_rails/test_email/[email protected]>"
+    subject_line = "Hello world!"
+
+    Apartment::Tenant.switch @restarone_subdomain do      
+      perform_enqueued_jobs do
+        assert_difference "MessageThread.all.reload.size" , +1 do        
+          assert_difference "Message.all.reload.size", +2 do          
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"Violet Rails" <[email protected]>',
+              subject: subject_line,
+              body: "Hello?",
+              'in-reply-to': in_reply_to,
+              references: in_reply_to
+
+            assert MessageThread.all.last.subject
+            assert Message.last.from
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"Violet Rails" <[email protected]>',
+              subject: 'Re: ' + subject_line,
+              body: "Hello?",
+              'in-reply-to': in_reply_to,
+              references: in_reply_to
+          end
+        end
+
+        assert_no_difference "MessageThread.all.reload.size" do        
+          assert_difference "Message.all.reload.size", +2 do
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"Violet Rails" <[email protected]>',
+              subject: 'FWD: ' + subject_line,
+              body: "Hello?",
+              'in-reply-to': in_reply_to,
+              references: in_reply_to
+
+            receive_inbound_email_from_mail \
+              to: '"Don Restarone" <[email protected]>',
+              from: '"Violet Rails" <[email protected]>',
+              subject: 're: ' + subject_line,
+              body: "Hello?",
+              'in-reply-to': in_reply_to,
+              references: in_reply_to
           end
         end
       end