Fixed #2399 - Attached images are broken on trigger reply with #{article.body_as_html}

app/controllers/concerns/clones_ticket_article_attachments.rb

@@ -6,41 +6,7 @@ module ClonesTicketArticleAttachments
   def article_attachments_clone(article)
     raise Exceptions::UnprocessableEntity, 'Need form_id to attach attachments to new form.' if params[:form_id].blank?
 
-    existing_attachments = Store.list(
-      object: 'UploadCache',
-      o_id:   params[:form_id],
-    )
-    attachments = []
-    article.attachments.each do |new_attachment|
-      next if new_attachment.preferences['content-alternative'] == true
-
-      if article.content_type.present? && article.content_type =~ %r{text/html}i
-        next if new_attachment.preferences['content_disposition'].present? && new_attachment.preferences['content_disposition'] !~ /inline/
-
-        if new_attachment.preferences['Content-ID'].present? && article.body.present?
-          next if article.body.match?(/#{Regexp.quote(new_attachment.preferences['Content-ID'])}/i)
-        end
-      end
-      already_added = false
-      existing_attachments.each do |existing_attachment|
-        next if existing_attachment.filename != new_attachment.filename || existing_attachment.size != new_attachment.size
-
-        already_added = true
-        break
-      end
-      next if already_added == true
-
-      file = Store.add(
-        object:      'UploadCache',
-        o_id:        params[:form_id],
-        data:        new_attachment.content,
-        filename:    new_attachment.filename,
-        preferences: new_attachment.preferences,
-      )
-      attachments.push file
-    end
-
-    attachments
+    article.clone_attachments('UploadCache', params[:form_id], only_attached_attachments: true)
   end
 
 end

app/models/ticket.rb

@@ -1486,6 +1486,12 @@ result
         preferences: attachment[:preferences],
       )
     end
+
+    original_article = objects[:article]
+    if original_article&.should_clone_inline_attachments? # rubocop:disable Style/GuardClause
+      original_article.clone_attachments('Ticket::Article', message.id, only_inline_attachments: true)
+      original_article.should_clone_inline_attachments = false # cancel the temporary flag after cloning
+    end
   end
 
   def sms_recipients_by_type(recipient_type, article)

app/models/ticket/article.rb

@@ -44,6 +44,9 @@ class Ticket::Article < ApplicationModel
                              :to,
                              :cc
 
+  attr_accessor :should_clone_inline_attachments
+  alias should_clone_inline_attachments? should_clone_inline_attachments
+
   # fillup md5 of message id to search easier on very long message ids
   def check_message_id_md5
     return true if message_id.blank?
@@ -130,6 +133,77 @@ returns
     new_attachments
   end
 
+=begin
+
+clone existing attachments of article to the target object
+
+  article_parent = Ticket::Article.find(123)
+  article_new = Ticket::Article.find(456)
+
+  attached_attachments = article_parent.clone_attachments(article_new.class.name, article_new.id, only_attached_attachments: true)
+
+  inline_attachments = article_parent.clone_attachments(article_new.class.name, article_new.id, only_inline_attachments: true)
+
+returns
+
+  [attachment1, attachment2, ...]
+
+=end
+
+  def clone_attachments(object_type, object_id, options = {})
+    existing_attachments = Store.list(
+      object: object_type,
+      o_id:   object_id,
+    )
+
+    is_html_content = false
+    if content_type.present? && content_type =~ %r{text/html}i
+      is_html_content = true
+    end
+
+    new_attachments = []
+    attachments.each do |new_attachment|
+      next if new_attachment.preferences['content-alternative'] == true
+
+      # only_attached_attachments mode is used by apply attached attachments to forwared article
+      if options[:only_attached_attachments] == true
+        if is_html_content
+          next if new_attachment.preferences['content_disposition'].present? && new_attachment.preferences['content_disposition'] =~ /inline/
+          next if new_attachment.preferences['Content-ID'].blank?
+          next if body.present? && body.match?(/#{Regexp.quote(new_attachment.preferences['Content-ID'])}/i)
+        end
+      end
+
+      # only_inline_attachments mode is used when quoting HTML mail with #{article.body_as_html}
+      if options[:only_inline_attachments] == true
+        next if is_html_content == false
+        next if body.blank?
+        next if new_attachment.preferences['content_disposition'].present? && new_attachment.preferences['content_disposition'] !~ /inline/
+        next if new_attachment.preferences['Content-ID'].present? && !body.match?(/#{Regexp.quote(new_attachment.preferences['Content-ID'])}/i)
+      end
+
+      already_added = false
+      existing_attachments.each do |existing_attachment|
+        next if existing_attachment.filename != new_attachment.filename || existing_attachment.size != new_attachment.size
+
+        already_added = true
+        break
+      end
+      next if already_added == true
+
+      file = Store.add(
+        object:      object_type,
+        o_id:        object_id,
+        data:        new_attachment.content,
+        filename:    new_attachment.filename,
+        preferences: new_attachment.preferences,
+      )
+      new_attachments.push file
+    end
+
+    new_attachments
+  end
+
   def self.last_customer_agent_article(ticket_id)
     sender = Ticket::Article::Sender.lookup(name: 'System')
     Ticket::Article.where('ticket_id = ? AND sender_id NOT IN (?)', ticket_id, sender.id).order('created_at DESC').first

lib/notification_factory/renderer.rb

@@ -126,6 +126,11 @@ examples how to use
       begin
         previous_object_refs = object_refs
         object_refs = object_refs.send(method.to_sym, *arguments)
+
+        # body_as_html should trigger the cloning of all inline attachments from the parent article (issue #2399)
+        if method.to_sym == :body_as_html && previous_object_refs.respond_to?(:should_clone_inline_attachments)
+          previous_object_refs.should_clone_inline_attachments = true
+        end
       rescue => e
         value = "\#{#{object_name}.#{object_methods_s} / #{e.message}}"
         break

spec/models/ticket/article_spec.rb

@@ -124,4 +124,95 @@ RSpec.describe Ticket::Article, type: :model do
       end
     end
   end
+
+  describe 'clone attachments' do
+    context 'of forwarded article' do
+      context 'via email' do
+
+        it 'only need to clone attached attachments' do
+          article_parent = create(:ticket_article,
+                                  type:         Ticket::Article::Type.find_by(name: 'email'),
+                                  content_type: 'text/html',
+                                  body:         '<img src="cid:15.274327094.140938@zammad.example.com"> some text',)
+          Store.add(
+            object:        'Ticket::Article',
+            o_id:          article_parent.id,
+            data:          'content_file1_normally_should_be_an_image',
+            filename:      'some_file1.jpg',
+            preferences:   {
+              'Content-Type'        => 'image/jpeg',
+              'Mime-Type'           => 'image/jpeg',
+              'Content-ID'          => '15.274327094.140938@zammad.example.com',
+              'Content-Disposition' => 'inline',
+            },
+            created_by_id: 1,
+          )
+          Store.add(
+            object:        'Ticket::Article',
+            o_id:          article_parent.id,
+            data:          'content_file2_normally_should_be_an_image',
+            filename:      'some_file2.jpg',
+            preferences:   {
+              'Content-Type'        => 'image/jpeg',
+              'Mime-Type'           => 'image/jpeg',
+              'Content-ID'          => '15.274327094.140938_not_reffered@zammad.example.com',
+              'Content-Disposition' => 'inline',
+            },
+            created_by_id: 1,
+          )
+          article_new = create(:ticket_article)
+          UserInfo.current_user_id = 1
+
+          attachments = article_parent.clone_attachments(article_new.class.name, article_new.id, only_attached_attachments: true)
+
+          expect(attachments.count).to eq(1)
+          expect(attachments[0].filename).to eq('some_file2.jpg')
+        end
+      end
+    end
+
+    context 'of trigger' do
+      context 'via email notifications' do
+        it 'only need to clone inline attachments used in body' do
+          article_parent = create(:ticket_article,
+                                  type:         Ticket::Article::Type.find_by(name: 'email'),
+                                  content_type: 'text/html',
+                                  body:         '<img src="cid:15.274327094.140938@zammad.example.com"> some text',)
+          Store.add(
+            object:        'Ticket::Article',
+            o_id:          article_parent.id,
+            data:          'content_file1_normally_should_be_an_image',
+            filename:      'some_file1.jpg',
+            preferences:   {
+              'Content-Type'        => 'image/jpeg',
+              'Mime-Type'           => 'image/jpeg',
+              'Content-ID'          => '15.274327094.140938@zammad.example.com',
+              'Content-Disposition' => 'inline',
+            },
+            created_by_id: 1,
+          )
+          Store.add(
+            object:        'Ticket::Article',
+            o_id:          article_parent.id,
+            data:          'content_file2_normally_should_be_an_image',
+            filename:      'some_file2.jpg',
+            preferences:   {
+              'Content-Type'        => 'image/jpeg',
+              'Mime-Type'           => 'image/jpeg',
+              'Content-ID'          => '15.274327094.140938_not_reffered@zammad.example.com',
+              'Content-Disposition' => 'inline',
+            },
+            created_by_id: 1,
+          )
+          article_new = create(:ticket_article)
+          UserInfo.current_user_id = 1
+
+          attachments = article_parent.clone_attachments(article_new.class.name, article_new.id, only_inline_attachments: true )
+
+          expect(attachments.count).to eq(1)
+          expect(attachments[0].filename).to eq('some_file1.jpg')
+        end
+      end
+    end
+  end
 end

test/unit/ticket_trigger_test.rb

@@ -4596,4 +4596,91 @@ class TicketTriggerTest < ActiveSupport::TestCase
     end
   end
 
+  #2399 - Attached images are broken on trigger reply with #{article.body_as_html}
+  test 'make sure auto reply using #{article.body_as_html} copies all articles image attachments as well' do
+    # make sure that this auto reply trigger only reacts to this particular test in order not to interfer with other auto reply tests
+    trigger1 = Trigger.create!(
+      name:                 'auto reply with HTML quote',
+      condition:            {
+        'ticket.action'   => {
+          'operator' => 'is',
+          'value'    => 'create',
+        },
+        'ticket.state_id' => {
+          'operator' => 'is',
+          'value'    => Ticket::State.lookup(name: 'new').id.to_s,
+        },
+        'ticket.title'    => {
+          'operator' => 'contains',
+          'value'    => 'AW: OTRS / Anfrage OTRS Einführung/Präsentation [Ticket#11545]',
+        },
+      },
+      perform:              {
+        'notification.email' => {
+          'body'      => '#{article.body_as_html}',
+          'recipient' => 'article_last_sender',
+          'subject'   => 'Thanks for your inquiry (#{ticket.title})!',
+        },
+      },
+      disable_notification: true,
+      active:               true,
+      created_by_id:        1,
+      updated_by_id:        1,
+    )
+
+    ticket1, article1, user, mail = Channel::EmailParser.new.process({}, File.read(Rails.root.join('test', 'data', 'mail', 'mail048.box')))
+
+    assert_equal('AW: OTRS / Anfrage OTRS Einführung/Präsentation [Ticket#11545]', ticket1.title, 'ticket1.title verify')
+    assert_equal(2, ticket1.articles.count, 'ticket1.articles verify')
+    assert_equal(2, ticket1.articles.first.attachments.count)
+
+    article1 = ticket1.articles.last
+    assert_match('Thanks for your inquiry (AW: OTRS / Anfrage OTRS Einführung/Präsentation [Ticket#11545])!', article1.subject)
+    assert_equal(1, article1.attachments.count)
+    assert_equal('50606', article1.attachments[0].size)
+    assert_equal('CPG-Reklamationsmitteilung bezügl.01234567895 an Voda-28.03.2017.jpg', article1.attachments[0].filename)
+  end
+
+  #2399 - Attached images are broken on trigger reply with #{article.body_as_html}
+  test 'make sure auto reply using #{article.body_as_html} does not copy any non-image attachments' do
+    # make sure that this auto reply trigger only reacts to this particular test in order not to interfer with other auto reply tests
+    trigger1 = Trigger.create!(
+      name:                 'auto reply with HTML quote',
+      condition:            {
+        'ticket.action'   => {
+          'operator' => 'is',
+          'value'    => 'create',
+        },
+        'ticket.state_id' => {
+          'operator' => 'is',
+          'value'    => Ticket::State.lookup(name: 'new').id.to_s,
+        },
+        'ticket.title'    => {
+          'operator' => 'contains',
+          'value'    => 'Online-apotheke. Günstigster Preis. Ohne Rezepte',
+        },
+      },
+      perform:              {
+        'notification.email' => {
+          'body'      => '#{article.body_as_html}',
+          'recipient' => 'article_last_sender',
+          'subject'   => 'Thanks for your inquiry (#{ticket.title})!',
+        },
+      },
+      disable_notification: true,
+      active:               true,
+      created_by_id:        1,
+      updated_by_id:        1,
+    )
+
+    ticket1, article1, user, mail = Channel::EmailParser.new.process({}, File.read(Rails.root.join('test', 'data', 'mail', 'mail069.box')))
+
+    assert_equal('Online-apotheke. Günstigster Preis. Ohne Rezepte', ticket1.title, 'ticket1.title verify')
+    assert_equal(2, ticket1.articles.count, 'ticket1.articles verify')
+    assert_equal(1, ticket1.articles.first.attachments.count)
+
+    article1 = ticket1.articles.last
+    assert_match('Thanks for your inquiry (Online-apotheke. Günstigster Preis. Ohne Rezepte)!', article1.subject)
+    assert_equal(0, article1.attachments.count)
+  end
 end