Fix sequential execution of multiple email notification triggers

This commit is contained in:
Ryan Lue 2018-05-23 18:25:11 +08:00
parent 3a645d1c6e
commit 5dd699ce6f
3 changed files with 64 additions and 11 deletions

View file

@ -806,6 +806,12 @@ perform changes on ticket
def perform_changes(perform, perform_origin, item = nil, current_user_id = nil)
logger.debug { "Perform #{perform_origin} #{perform.inspect} on Ticket.find(#{id})" }
article = begin
Ticket::Article.lookup(id: item.try(:dig, :article_id))
rescue ArgumentError
nil
end
# if the configuration contains the deletion of the ticket then
# we skip all other ticket changes because they does not matter
if perform['ticket.action'].present? && perform['ticket.action']['value'] == 'delete'
@ -835,8 +841,7 @@ perform changes on ticket
recipients_raw = []
value_recipient.each do |recipient|
if recipient == 'article_last_sender'
if item && item[:article_id]
article = Ticket::Article.lookup(id: item[:article_id])
if article.present?
if article.reply_to.present?
recipients_raw.push(article.reply_to)
elsif article.from.present?
@ -914,12 +919,9 @@ perform changes on ticket
end
# check if notification should be send because of customer emails
if item && item[:article_id]
article = Ticket::Article.lookup(id: item[:article_id])
if article&.preferences&.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i
logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email"
next
end
if article.present? && article.preferences.fetch('is-auto-response', false) == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i
logger.info "Send no trigger based notification to #{recipient_email} because of auto response tagged incoming email"
next
end
# loop protection / check if maximal count of trigger mail has reached
@ -984,9 +986,12 @@ perform changes on ticket
next
end
# articles.last breaks (returns the wrong article)
# if another email notification trigger preceded this one
# (see https://github.com/zammad/zammad/issues/1543)
objects = {
ticket: self,
article: articles.last,
article: article || articles.last
}
# get subject
@ -1007,7 +1012,7 @@ perform changes on ticket
(body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id)
article = Ticket::Article.create(
message = Ticket::Article.create(
ticket_id: id,
to: recipient_string,
subject: subject,
@ -1026,7 +1031,7 @@ perform changes on ticket
attachments_inline.each do |attachment|
Store.add(
object: 'Ticket::Article',
o_id: article.id,
o_id: message.id,
data: attachment[:data],
filename: attachment[:filename],
preferences: attachment[:preferences],

View file

@ -0,0 +1,9 @@
FactoryBot.define do
factory :email_address do
email 'zammad@localhost'
realname 'zammad'
channel_id 1
created_by_id 1
updated_by_id 1
end
end

View file

@ -218,6 +218,45 @@ RSpec.describe Ticket do
.and not_change { trigger.perform['notification.email'][:subject] }
end
# Regression test for https://github.com/zammad/zammad/issues/1543
#
# If a new article fires an email notification trigger,
# and then another article is added to the same ticket
# before that trigger is performed,
# the email template's 'article' var should refer to the originating article,
# not the newest one.
#
# (This occurs whenever one action fires multiple email notification triggers.)
it 'passes the correct article to NotificationFactory::Mailer' do
# required by Ticket#perform_changes for email notifications
Group.first.update(email_address: create(:email_address))
ticket = Ticket.first
orig_article = Ticket::Article.where(ticket_id: ticket.id).first
newer_article = create(:ticket_article, ticket_id: ticket.id)
trigger = Trigger.new(
perform: {
'notification.email' => {
body: '',
recipient: 'ticket_customer',
subject: ''
}
}
)
allow(NotificationFactory::Mailer).to receive(:template).and_return('')
ticket.perform_changes(trigger.perform, 'trigger', { article_id: orig_article.id }, 1)
expect(NotificationFactory::Mailer)
.to have_received(:template)
.with(hash_including(objects: { ticket: ticket, article: orig_article }))
.at_least(:once)
expect(NotificationFactory::Mailer)
.not_to have_received(:template)
.with(hash_including(objects: { ticket: ticket, article: newer_article }))
end
end
describe '#selectors' do