From e529e1206e85ad4b30e118b431759c21484a32a2 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Wed, 6 Sep 2023 14:35:19 +0530 Subject: [PATCH] fix: Update inline image processing logic to fix missing images when multiple inline images present (#7861) --- app/mailboxes/mailbox_helper.rb | 63 ++++++++++++------- .../files/mail_with_inline_images.eml | 62 +++++++++--------- .../mail_with_plain_text_and_inline_image.eml | 26 ++++++-- spec/mailboxes/mailbox_helper_spec.rb | 51 +++++++++++++++ spec/mailboxes/reply_mailbox_spec.rb | 6 ++ 5 files changed, 151 insertions(+), 57 deletions(-) create mode 100644 spec/mailboxes/mailbox_helper_spec.rb diff --git a/app/mailboxes/mailbox_helper.rb b/app/mailboxes/mailbox_helper.rb index ec3685cc3..5d1033d20 100644 --- a/app/mailboxes/mailbox_helper.rb +++ b/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}]", "\"#{attachment_name}\"" + @text_content = @text_content.gsub( + "[image: #{attachment_name}]", "\"#{attachment_name}\"" ) - @message.save! end def inline_image_url(blob) diff --git a/spec/fixtures/files/mail_with_inline_images.eml b/spec/fixtures/files/mail_with_inline_images.eml index 2a7792ed1..138e4a4b9 100644 --- a/spec/fixtures/files/mail_with_inline_images.eml +++ b/spec/fixtures/files/mail_with_inline_images.eml @@ -4,44 +4,50 @@ Message-ID: Subject: New inline image test email conversation From: Sony Mathew To: "Replies" -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 - ---0000000000007a538205fcfa5966 +--000000000000cade230604abfbe1 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable -
3D"ptrol.jpeg"

Let's add no HTML content here,= - just plain text and images

--
Tejaswini Chile.
Software developer
Mo= -b:8485827731 |=C2=A0@tejaswini_chile
+
HTML content and inline images
3D"poster3D"poster3D"poster
---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: +X-Attachment-Id: ii_lm7fuura0 +Content-ID: ---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: + + +--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: + + +--000000000000cade250604abfbe2-- diff --git a/spec/fixtures/files/mail_with_plain_text_and_inline_image.eml b/spec/fixtures/files/mail_with_plain_text_and_inline_image.eml index b4ea01175..63b0caebe 100644 --- a/spec/fixtures/files/mail_with_plain_text_and_inline_image.eml +++ b/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: +X-Attachment-Id: ii_lm7fuura0 +Content-ID: + + +--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: + + +--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: --0000000000007a538405fcfa5967-- diff --git a/spec/mailboxes/mailbox_helper_spec.rb b/spec/mailboxes/mailbox_helper_spec.rb new file mode 100644 index 000000000..fbcf1423e --- /dev/null +++ b/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 diff --git a/spec/mailboxes/reply_mailbox_spec.rb b/spec/mailboxes/reply_mailbox_spec.rb index 0bee15fa9..a9a802bb9 100644 --- a/spec/mailboxes/reply_mailbox_spec.rb +++ b/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