Skip to content

Commit

Permalink
Fixed #2399 - Attached images are broken on trigger reply with #{arti…
Browse files Browse the repository at this point in the history 
…cle.body_as_html}
  • Loading branch information
@znuny-robo
Billy Zhou authored and znuny-robo committed Feb 10, 2019
1 parent f7a59c6 commit 487f36a
Show file tree
Hide file tree
Showing 6 changed files with 264 additions and 35 deletions.
36 changes: 1 addition & 35 deletions app/controllers/concerns/clones_ticket_article_attachments.rb
View file
Expand Up @@ -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
6 changes: 6 additions & 0 deletions app/models/ticket.rb
View file
Expand Up @@ -1486,6 +1486,12 @@ def send_email_notification(value, article, perform_origin)
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)
Expand Down
74 changes: 74 additions & 0 deletions app/models/ticket/article.rb
View file
Expand Up @@ -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?
Expand Down Expand Up @@ -130,6 +133,77 @@ def attachments_inline
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
Expand Down
5 changes: 5 additions & 0 deletions lib/notification_factory/renderer.rb
View file
Expand Up @@ -126,6 +126,11 @@ def d(key, escape = nil)
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
Expand Down
91 changes: 91 additions & 0 deletions spec/models/ticket/article_spec.rb
View file
Expand Up @@ -124,4 +124,95 @@
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
87 changes: 87 additions & 0 deletions test/unit/ticket_trigger_test.rb
View file
Expand Up @@ -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

0 comments on commit 487f36a

Please sign in to comment.