fix: Update inline image processing logic to fix missing images when multiple inline images present (#7861)

app/mailboxes/mailbox_helper.rb

@@ -23,24 +23,47 @@ module MailboxHelper
   def add_attachments_to_message
     return if @message.blank?
 
-    processed_mail.attachments.last(Message::NUMBER_OF_PERMITTED_ATTACHMENTS).each do |mail_attachment|
-      if inline_attachment?(mail_attachment)
-        embed_inline_image_source(mail_attachment)
-      else
-        attachment = @message.attachments.new(
-          account_id: @conversation.account_id,
-          file_type: 'file'
-        )
-        attachment.file.attach(mail_attachment[:blob])
-      end
-    end
+    # ensure we don't add more than the permitted number of attachments
+    all_attachments = processed_mail.attachments.last(Message::NUMBER_OF_PERMITTED_ATTACHMENTS)
+
+    inline_attachments = all_attachments.select { |attachment| attachment[:original].inline? }
+    regular_attachments = all_attachments - inline_attachments
+
+    process_inline_attachments(inline_attachments) if inline_attachments.present?
+    process_regular_attachments(regular_attachments) if regular_attachments.present?
+
     @message.save!
   end
 
+  def process_regular_attachments(attachments)
+    attachments.each do |mail_attachment|
+      attachment = @message.attachments.new(
+        account_id: @conversation.account_id,
+        file_type: 'file'
+      )
+      attachment.file.attach(mail_attachment[:blob])
+    end
+  end
+
+  def process_inline_attachments(attachments)
+    # create an instance variable here, the `embed_inline_image_source`
+    # updates them directly. And then the value is eventaully used to update the message content
+    @html_content = processed_mail.serialized_data[:html_content][:full]
+    @text_content = processed_mail.serialized_data[:text_content][:reply]
+
+    attachments.each do |mail_attachment|
+      embed_inline_image_source(mail_attachment)
+    end
+
+    # update the message content with the updated html and text content
+    @message.content_attributes[:email][:html_content][:full] = @html_content
+    @message.content_attributes[:email][:text_content][:full] = @text_content
+  end
+
   def embed_inline_image_source(mail_attachment)
-    if processed_mail.serialized_data[:html_content].present?
+    if @html_content.present?
       upload_inline_image(mail_attachment)
-    elsif processed_mail.serialized_data[:text_content].present?
+    elsif @text_content.present?
       embed_plain_text_email_with_inline_image(mail_attachment)
     end
   end
@@ -48,23 +71,15 @@ module MailboxHelper
   def upload_inline_image(mail_attachment)
     content_id = mail_attachment[:original].cid
 
-    @message.content_attributes[:email][:html_content][:full] = processed_mail.serialized_data[:html_content][:full].gsub(
-      "cid:#{content_id}", inline_image_url(mail_attachment[:blob]).to_s
-    )
-    @message.save!
-  end
-
-  def inline_attachment?(mail_attachment)
-    mail_attachment[:original].inline?
+    @html_content = @html_content.gsub("cid:#{content_id}", inline_image_url(mail_attachment[:blob]).to_s)
   end
 
   def embed_plain_text_email_with_inline_image(mail_attachment)
     attachment_name = mail_attachment[:original].filename
 
-    @message.content_attributes[:email][:text_content][:full] = processed_mail.serialized_data[:text_content][:reply].gsub(
-      "[image: #{attachment_name}]", "<img src=\"#{inline_image_url(mail_attachment[:blob])}\" alt=\"#{attachment_name}>\""
+    @text_content = @text_content.gsub(
+      "[image: #{attachment_name}]", "<img src=\"#{inline_image_url(mail_attachment[:blob])}\" alt=\"#{attachment_name}\">"
     )
-    @message.save!
   end
 
   def inline_image_url(blob)

spec/fixtures/files/mail_with_inline_images.eml

@@ -4,44 +4,50 @@ Message-ID: <CAFkiBVzURp=z2g5nAevAtdQvFfxkLrpvTeDFcwFaGQhdo_-Mkg@mail.gmail.com>
 Subject: New inline image test email conversation
 From: Sony Mathew <sony@chatwoot.com>
 To: "Replies" <reply+6bdc3f4d-0bed-4515-a284-5d916fdde489@example.com>
-Content-Type: multipart/related; boundary="0000000000007a538405fcfa5967"
+Content-Type: multipart/related; boundary="000000000000cade250604abfbe2"
 
---0000000000007a538405fcfa5967
-Content-Type: multipart/alternative; boundary="0000000000007a538205fcfa5966"
+--000000000000cade250604abfbe2
+Content-Type: multipart/alternative; boundary="000000000000cade230604abfbe1"
 
---0000000000007a538205fcfa5966
+--000000000000cade230604abfbe1
 Content-Type: text/plain; charset="UTF-8"
 
-[image: ptrol.jpeg]
-
 HTML content and inline images
+[image: poster (8).jpg][image: poster (7).jpg][image: poster (1).jpg]
 
---
-*Tejaswini Chile.*
-Software developer
-*Mob:8485827731 | *@tejaswini_chile <https://twitter.com/tejaswini_chile>
-
---0000000000007a538205fcfa5966
+--000000000000cade230604abfbe1
 Content-Type: text/html; charset="UTF-8"
 Content-Transfer-Encoding: quoted-printable
 
-<div dir=3D"ltr"><img src=3D"cid:ii_libj9tsr0" alt=3D"ptrol.jpeg" width=3D"=
-286" height=3D"508"><div><br></div><div>Let&#39;s add no HTML content here,=
- just plain text and images<br><div><br></div><span class=3D"gmail_signatur=
-e_prefix">-- </span><br><div dir=3D"ltr" class=3D"gmail_signature" data-sma=
-rtmail=3D"gmail_signature"><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div=
-><b>Tejaswini Chile.</b><br></div><span style=3D"font-family:times new roma=
-n,serif"><span></span><span></span>Software developer</span><br></div><b>Mo=
-b:8485827731 |=C2=A0</b><a href=3D"https://twitter.com/tejaswini_chile" tar=
-get=3D"_blank">@tejaswini_chile</a></div></div></div></div></div></div>
+<div dir=3D"ltr"><div>HTML content and inline images</div><img src=3D"=
+cid:ii_lm7fuura0" alt=3D"poster (8).jpg" width=3D"542" height=3D"285"><img =
+src=3D"cid:ii_lm7fuuvm1" alt=3D"poster (7).jpg" width=3D"542" height=3D"285=
+"><img src=3D"cid:ii_lm7fuuwn2" alt=3D"poster (1).jpg" width=3D"542" height=
+=3D"285"><br></div>
 
---0000000000007a538205fcfa5966--
---0000000000007a538405fcfa5967
-Content-Type: image/jpeg; name="ptrol.jpeg"
-Content-Disposition: inline; filename="ptrol.jpeg"
+--000000000000cade230604abfbe1--
+--000000000000cade250604abfbe2
+Content-Type: image/jpeg; name="poster (8).jpg"
+Content-Disposition: inline; filename="poster (8).jpg"
 Content-Transfer-Encoding: base64
-X-Attachment-Id: ii_libj9tsr0
-Content-ID: <ii_libj9tsr0>
+X-Attachment-Id: ii_lm7fuura0
+Content-ID: <ii_lm7fuura0>
 
 
---0000000000007a538405fcfa5967--
+--000000000000cade250604abfbe2
+Content-Type: image/jpeg; name="poster (7).jpg"
+Content-Disposition: inline; filename="poster (7).jpg"
+Content-Transfer-Encoding: base64
+X-Attachment-Id: ii_lm7fuuvm1
+Content-ID: <ii_lm7fuuvm1>
+
+
+--000000000000cade250604abfbe2
+Content-Type: image/jpeg; name="poster (1).jpg"
+Content-Disposition: inline; filename="poster (1).jpg"
+Content-Transfer-Encoding: base64
+X-Attachment-Id: ii_lm7fuuwn2
+Content-ID: <ii_lm7fuuwn2>
+
+
+--000000000000cade250604abfbe2--

spec/fixtures/files/mail_with_plain_text_and_inline_image.eml

@@ -12,7 +12,7 @@ Content-Type: multipart/alternative; boundary="0000000000007a538205fcfa5966"
 --0000000000007a538205fcfa5966
 Content-Type: text/plain; charset="UTF-8"
 
-[image: ptrol.jpeg]
+[image: poster (8).jpg][image: poster (7).jpg][image: poster (1).jpg]
 
 Let's add no HTML content here, just plain text and images
 
@@ -23,11 +23,27 @@ Software developer
 
 
 --0000000000007a538405fcfa5967
-Content-Type: image/jpeg; name="ptrol.jpeg"
-Content-Disposition: inline; filename="ptrol.jpeg"
+Content-Type: image/jpeg; name="poster (8).jpg"
+Content-Disposition: inline; filename="poster (8).jpg"
 Content-Transfer-Encoding: base64
-X-Attachment-Id: ii_libj9tsr0
-Content-ID: <ii_libj9tsr0>
+X-Attachment-Id: ii_lm7fuura0
+Content-ID: <ii_lm7fuura0>
+
+
+--0000000000007a538405fcfa5967
+Content-Type: image/jpeg; name="poster (7).jpg"
+Content-Disposition: inline; filename="poster (7).jpg"
+Content-Transfer-Encoding: base64
+X-Attachment-Id: ii_lm7fuuvm1
+Content-ID: <ii_lm7fuuvm1>
+
+
+--0000000000007a538405fcfa5967
+Content-Type: image/jpeg; name="poster (1).jpg"
+Content-Disposition: inline; filename="poster (1).jpg"
+Content-Transfer-Encoding: base64
+X-Attachment-Id: ii_lm7fuuwn2
+Content-ID: <ii_lm7fuuwn2>
 
 
 --0000000000007a538405fcfa5967--

spec/mailboxes/mailbox_helper_spec.rb

@@ -0,0 +1,51 @@
+require 'rails_helper'
+
+RSpec.describe MailboxHelper do
+  include ActionMailbox::TestHelper
+
+  # Setup anonymous class
+  let(:mailbox_helper_obj) do
+    Class.new do
+      include MailboxHelper
+      attr_accessor :conversation, :processed_mail
+
+      def initialize(conversation, processed_mail)
+        @conversation = conversation
+        @processed_mail = processed_mail
+      end
+    end
+  end
+
+  let(:mail) { create_inbound_email_from_fixture('welcome.eml').mail }
+  let(:processed_mail) { MailPresenter.new(mail) }
+  let(:conversation) { create(:conversation) }
+  let(:dummy_message) { create(:message) }
+
+  describe '#create_message' do
+    before do
+      create_list(:message, 5, conversation: conversation)
+    end
+
+    context 'when message already exist' do
+      it 'creates a new message' do
+        helper_instance = mailbox_helper_obj.new(conversation, processed_mail)
+
+        expect(conversation.messages).to receive(:find_by).with(source_id: processed_mail.message_id).and_return(dummy_message)
+        expect(conversation.messages).not_to receive(:create!)
+
+        helper_instance.send(:create_message)
+      end
+    end
+
+    context 'when message does not exist' do
+      it 'creates a new message' do
+        helper_instance = mailbox_helper_obj.new(conversation, processed_mail)
+
+        expect(conversation.messages).to receive(:find_by).with(source_id: processed_mail.message_id).and_return(nil)
+        expect(conversation.messages).to receive(:create!)
+
+        helper_instance.send(:create_message)
+      end
+    end
+  end
+end

spec/mailboxes/reply_mailbox_spec.rb

@@ -86,6 +86,9 @@ RSpec.describe ReplyMailbox do
 
         html_full_content = conversation.messages.last.content_attributes[:email][:html_content][:full]
         expect(html_full_content).to include('img')
+        expect(html_full_content).not_to include('cid:ii_lm7fuura0')
+        expect(html_full_content).not_to include('cid:ii_lm7fuuvm1')
+        expect(html_full_content).not_to include('cid:ii_lm7fuuwn2')
       end
     end
 
@@ -105,6 +108,9 @@ RSpec.describe ReplyMailbox do
         text_full_content = conversation.messages.last.content_attributes[:email][:text_content][:full]
 
         expect(text_full_content).to include('img')
+        expect(text_full_content).not_to include('[image: poster (8).jpg]')
+        expect(text_full_content).not_to include('[image: poster (7).jpg]')
+        expect(text_full_content).not_to include('[image: poster (1).jpg]')
         expect(conversation.messages.last.content).to include("Let's add no HTML content here, just plain text and images")
         expect(conversation.messages.last.attachments.count).to eq(0)
       end