From da695015317fcce5d92eb069e86691a8eda40287 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Tue, 2 May 2017 13:34:18 +0200 Subject: [PATCH] Fixed issue #1006 - Reply-To is not considered for automatic responses (triggers). --- .../_ui_element/ticket_perform_action.coffee | 23 +- app/models/ticket.rb | 90 +++-- test/unit/ticket_trigger_test.rb | 374 ++++++++++++++++++ 3 files changed, 453 insertions(+), 34 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee b/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee index bd7b04dc5..fa8a00d79 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee @@ -304,18 +304,31 @@ class App.UiElement.ticket_perform_action return if elementRow.find('.js-setNotification .js-body').get(0) options = + 'article_last_sender': 'Article Last Sender' 'ticket_owner': 'Owner' 'ticket_customer': 'Customer' 'ticket_agents': 'All Agents' name = "#{attribute.name}::notification.email" - selection = $("") + # meta.recipient was a string in the past (single-select) so we convert it to array if needed + if !_.isArray(meta.recipient) + meta.recipient = [meta.recipient] + + column_select_options = [] for key, value of options - selected = '' - if key is meta.recipient - selected = 'selected="selected"' - selection.append("") + selected = undefined + for recipient in meta.recipient + if key is recipient + selected = true + column_select_options.push({ value: key, name: App.i18n.translateInline(value), selected: selected }) + + column_select = new App.ColumnSelect + attribute: + name: "#{name}::recipient" + options: column_select_options + + selection = column_select.element() notificationElement = $( App.view('generic/ticket_perform_action/notification_email')( attribute: attribute diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 863338ca8..f529c3c93 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -745,53 +745,87 @@ perform changes on ticket # send notification if object_name == 'notification' - recipients = [] - if value['recipient'] == 'ticket_customer' - recipients.push User.lookup(id: customer_id) - elsif value['recipient'] == 'ticket_owner' - recipients.push User.lookup(id: owner_id) - elsif value['recipient'] == 'ticket_agents' - recipients = recipients.concat(agent_of_group) - else - logger.error "Unknown email notification recipient '#{value['recipient']}'" - next + + # value['recipient'] was a string in the past (single-select) so we convert it to array if needed + value_recipient = value['recipient'] + if !value_recipient.is_a?(Array) + value_recipient = [value_recipient] end - recipient_string = '' - recipient_already = {} - recipients.each { |user| + + recipients_raw = [] + value_recipient.each { |recipient| + if recipient == 'article_last_sender' + if item && item[:article_id] + article = Ticket::Article.lookup(id: item[:article_id]) + if article.reply_to.present? + recipients_raw.push(article.reply_to) + elsif article.from.present? + recipients_raw.push(article.from) + elsif article.origin_by_id + email = User.lookup(id: article.origin_by_id).email + recipients_raw.push(email) + elsif article.created_by_id + email = User.lookup(id: article.created_by_id).email + recipients_raw.push(email) + end + end + elsif recipient == 'ticket_customer' + email = User.lookup(id: customer_id).email + recipients_raw.push(email) + elsif recipient == 'ticket_owner' + email = User.lookup(id: owner_id).email + recipients_raw.push(email) + elsif recipient == 'ticket_agents' + agent_of_group.each { |user| + recipients_raw.push(user.email) + } + else + logger.error "Unknown email notification recipient '#{recipient}'" + next + end + } + + recipients_checked = [] + recipients_raw.each { |recipient_email| # send notifications only to email adresses - next if !user.email - next if user.email !~ /@/ + next if !recipient_email + next if recipient_email !~ /@/ + + # check if address is valid + begin + recipient_email = Mail::Address.new(recipient_email).address + rescue + next # because unable to parse + end # do not sent notifications to this recipients send_no_auto_response_reg_exp = Setting.get('send_no_auto_response_reg_exp') begin - next if user.email =~ /#{send_no_auto_response_reg_exp}/i + next if recipient_email =~ /#{send_no_auto_response_reg_exp}/i rescue => e logger.error "ERROR: Invalid regex '#{send_no_auto_response_reg_exp}' in setting send_no_auto_response_reg_exp" logger.error 'ERROR: ' + e.inspect - next if user.email =~ /(mailer-daemon|postmaster|abuse|root)@.+?\..+?/i + next if recipient_email =~ /(mailer-daemon|postmaster|abuse|root)@.+?\..+?/i 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 && article.preferences['is-auto-response'] == true && article.from && article.from =~ /#{Regexp.quote(user.email)}/i - logger.info "Send not trigger based notification to #{user.email} because of auto response tagged incoming email" + if article && article.preferences['is-auto-response'] == true && article.from && article.from =~ /#{Regexp.quote(recipient_email)}/i + logger.info "Send not trigger based notification to #{recipient_email} because of auto response tagged incoming email" next end end - email = user.email.downcase.strip - next if recipient_already[email] - recipient_already[email] = true - if recipient_string != '' - recipient_string += ', ' - end - recipient_string += email + email = recipient_email.downcase.strip + next if recipients_checked.include?(email) + recipients_checked.push(email) } - next if recipient_string == '' + + next if recipients_checked.blank? + recipient_string = recipients_checked.join(', ') + group = self.group next if !group email_address = group.email_address @@ -808,8 +842,6 @@ perform changes on ticket objects = { ticket: self, article: articles.last, - #recipient: user, - #changes: changes, } # get subject diff --git a/test/unit/ticket_trigger_test.rb b/test/unit/ticket_trigger_test.rb index 3bf16da4e..05343af10 100644 --- a/test/unit/ticket_trigger_test.rb +++ b/test/unit/ticket_trigger_test.rb @@ -2471,4 +2471,378 @@ class TicketTriggerTest < ActiveSupport::TestCase } end + test 'article_last_sender trigger -> reply_to' do + trigger = Trigger.create_or_update( + name: 'auto reply', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'create', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'notification.email' => { + 'body' => 'some text
#{ticket.customer.lastname}
#{ticket.title}
#{article.body}', + '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 = Ticket.create( + title: "some title\n äöüß", + group: Group.lookup(name: 'Users'), + customer: User.lookup(email: 'nicole.braun@zammad.org'), + updated_by_id: 1, + created_by_id: 1, + ) + Ticket::Article.create( + ticket_id: ticket1.id, + from: 'some_sender@example.com', + to: 'some_recipient+from@example.com', + reply_to: 'some_recipient+reply_to@example.com', + subject: 'some subject', + message_id: 'some@id', + body: "some message note\nnew line", + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Agent'), + type: Ticket::Article::Type.find_by(name: 'note'), + updated_by_id: 1, + created_by_id: 1, + ) + + Observer::Transaction.commit + + ticket1.reload + assert_equal('new', ticket1.state.name, 'ticket1.state new') + assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') + auto_response = ticket1.articles.last + assert_match('Zammad ', auto_response.from) + assert_match('some_recipient+reply_to@example.com', auto_response.to) + end + + test 'article_last_sender trigger -> from' do + trigger = Trigger.create_or_update( + name: 'auto reply', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'create', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'notification.email' => { + 'body' => 'some text
#{ticket.customer.lastname}
#{ticket.title}
#{article.body}', + '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 = Ticket.create( + title: "some title\n äöüß", + group: Group.lookup(name: 'Users'), + customer: User.lookup(email: 'nicole.braun@zammad.org'), + updated_by_id: 1, + created_by_id: 1, + ) + Ticket::Article.create( + ticket_id: ticket1.id, + from: 'some_sender+from@example.com', + to: 'some_recipient@example.com', + subject: 'some subject', + message_id: 'some@id', + body: "some message note\nnew line", + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Customer'), + type: Ticket::Article::Type.find_by(name: 'email'), + updated_by_id: 1, + created_by_id: 1, + ) + + Observer::Transaction.commit + + ticket1.reload + assert_equal('new', ticket1.state.name, 'ticket1.state new') + assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') + auto_response = ticket1.articles.last + assert_match('Zammad ', auto_response.from) + assert_match('some_sender+from@example.com', auto_response.to) + end + + test 'article_last_sender trigger -> origin_by_id' do + trigger = Trigger.create_or_update( + name: 'auto reply', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'create', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'notification.email' => { + 'body' => 'some text
#{ticket.customer.lastname}
#{ticket.title}
#{article.body}', + 'recipient' => 'article_last_sender', + 'subject' => 'Thanks for your inquiry (#{ticket.title})!', + }, + }, + disable_notification: true, + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + roles = Role.where(name: 'Customer') + customer1 = User.create_or_update( + login: 'customer+origin_by_id@example.com', + firstname: 'Trigger', + lastname: 'Customer1', + email: 'customer+origin_by_id@example.com', + password: 'customerpw', + active: true, + roles: roles, + updated_at: '2015-02-05 16:37:00', + updated_by_id: 1, + created_by_id: 1, + ) + ticket1 = Ticket.create( + title: "some title\n äöüß", + group: Group.lookup(name: 'Users'), + customer: User.lookup(email: 'nicole.braun@zammad.org'), + updated_by_id: 1, + created_by_id: 1, + ) + Ticket::Article.create( + ticket_id: ticket1.id, + to: 'some_recipient@example.com', + subject: 'some subject', + message_id: 'some@id', + body: "some message note\nnew line", + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Customer'), + type: Ticket::Article::Type.find_by(name: 'email'), + origin_by_id: customer1.id, + updated_by_id: 1, + created_by_id: 1, + ) + + Observer::Transaction.commit + + ticket1.reload + assert_equal('new', ticket1.state.name, 'ticket1.state new') + assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') + auto_response = ticket1.articles.last + assert_match('Zammad ', auto_response.from) + assert_match('customer+origin_by_id@example.com', auto_response.to) + end + + test 'article_last_sender trigger -> created_by_id' do + trigger = Trigger.create_or_update( + name: 'auto reply', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'create', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'notification.email' => { + 'body' => 'some text
#{ticket.customer.lastname}
#{ticket.title}
#{article.body}', + 'recipient' => 'article_last_sender', + 'subject' => 'Thanks for your inquiry (#{ticket.title})!', + }, + }, + disable_notification: true, + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + roles = Role.where(name: 'Customer') + customer1 = User.create_or_update( + login: 'customer+created_by_id@example.com', + firstname: 'Trigger', + lastname: 'Customer1', + email: 'customer+created_by_id@example.com', + password: 'customerpw', + active: true, + roles: roles, + updated_at: '2015-02-05 16:37:00', + updated_by_id: 1, + created_by_id: 1, + ) + ticket1 = Ticket.create( + title: "some title\n äöüß", + group: Group.lookup(name: 'Users'), + customer: User.lookup(email: 'nicole.braun@zammad.org'), + updated_by_id: 1, + created_by_id: 1, + ) + Ticket::Article.create( + ticket_id: ticket1.id, + to: 'some_recipient@example.com', + subject: 'some subject', + message_id: 'some@id', + body: "some message note\nnew line", + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Customer'), + type: Ticket::Article::Type.find_by(name: 'email'), + updated_by_id: customer1.id, + created_by_id: customer1.id, + ) + + Observer::Transaction.commit + + ticket1.reload + assert_equal('new', ticket1.state.name, 'ticket1.state new') + assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') + auto_response = ticket1.articles.last + assert_match('Zammad ', auto_response.from) + assert_match('customer+created_by_id@example.com', auto_response.to) + end + + test 'multiple recipients owner_id, article_last_sender(reply_to) trigger' do + trigger = Trigger.create_or_update( + name: 'auto reply', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'create', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'notification.email' => { + 'body' => 'some text
#{ticket.customer.lastname}
#{ticket.title}
#{article.body}', + 'recipient' => %w(ticket_owner article_last_sender), + 'subject' => 'Thanks for your inquiry (#{ticket.title})!', + }, + }, + disable_notification: true, + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + admin = User.create_or_update( + login: 'admin+owner_recipient@example.com', + firstname: 'Role', + lastname: "Admin#{name}", + email: 'admin+owner_recipient@example.com', + password: 'adminpw', + active: true, + roles: Role.where(name: %w(Admin Agent)), + updated_by_id: 1, + created_by_id: 1, + ) + ticket1 = Ticket.create( + title: "some title\n äöüß", + group: Group.lookup(name: 'Users'), + customer: User.lookup(email: 'nicole.braun@zammad.org'), + owner_id: admin.id, + updated_by_id: 1, + created_by_id: 1, + ) + Ticket::Article.create( + ticket_id: ticket1.id, + from: 'some_sender@example.com', + to: 'some_recipient+from@example.com', + reply_to: 'some_recipient+reply_to@example.com', + subject: 'some subject', + message_id: 'some@id', + body: "some message note\nnew line", + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Agent'), + type: Ticket::Article::Type.find_by(name: 'note'), + updated_by_id: 1, + created_by_id: 1, + ) + + Observer::Transaction.commit + + ticket1.reload + assert_equal('new', ticket1.state.name, 'ticket1.state new') + assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') + auto_response = ticket1.articles.last + assert_match('Zammad ', auto_response.from) + assert_match('some_recipient+reply_to@example.com', auto_response.to) + assert_match('admin+owner_recipient@example.com', auto_response.to) + end + + test 'article_last_sender trigger -> invalid reply_to' do + trigger = Trigger.create_or_update( + name: 'auto reply', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'create', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'notification.email' => { + 'body' => 'some text
#{ticket.customer.lastname}
#{ticket.title}
#{article.body}', + '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 = Ticket.create( + title: "some title\n äöüß", + group: Group.lookup(name: 'Users'), + customer: User.lookup(email: 'nicole.braun@zammad.org'), + updated_by_id: 1, + created_by_id: 1, + ) + Ticket::Article.create( + ticket_id: ticket1.id, + from: 'some_sender@example.com', + to: 'some_recipient+from@example.com', + reply_to: 'Blub blub blub some_recipient+reply_to@example', + subject: 'some subject', + message_id: 'some@id', + body: "some message note\nnew line", + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Agent'), + type: Ticket::Article::Type.find_by(name: 'note'), + updated_by_id: 1, + created_by_id: 1, + ) + + Observer::Transaction.commit + + ticket1.reload + assert_equal('new', ticket1.state.name, 'ticket1.state new') + assert_equal(1, ticket1.articles.count, 'ticket1.articles verify') + end + end