diff --git a/app/models/ticket.rb b/app/models/ticket.rb index f71bc5300..2dd8db5d9 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -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], diff --git a/spec/factories/email_address.rb b/spec/factories/email_address.rb new file mode 100644 index 000000000..951917a68 --- /dev/null +++ b/spec/factories/email_address.rb @@ -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 diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index bdb317e4f..057c7118a 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -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