Fixed issue #2125 - Auto responder is not set from correct group.

app/models/observer/ticket/article/fillup_from_email.rb

@@ -24,7 +24,7 @@ class Observer::Ticket::Article::FillupFromEmail < ActiveRecord::Observer
     return true if type['name'] != 'email'
 
     # set subject if empty
-    ticket = Ticket.lookup(id: record.ticket_id)
+    ticket = record.ticket
     if !record.subject || record.subject == ''
       record.subject = ticket.title
     end
@@ -44,7 +44,7 @@ class Observer::Ticket::Article::FillupFromEmail < ActiveRecord::Observer
     # set sender
     email_address = ticket.group.email_address
     if !email_address
-      raise "No email address found for group '#{ticket.group.name}'"
+      raise "No email address found for group '#{ticket.group.name}' (#{ticket.group_id})"
     end
 
     # remember email address for background job

app/models/ticket.rb

@@ -824,219 +824,15 @@ perform changes on ticket
       end
     end
 
+    perform_notification = {}
     changed = false
     perform.each do |key, value|
       (object_name, attribute) = key.split('.', 2)
       raise "Unable to update object #{object_name}.#{attribute}, only can update tickets and send notifications!" if object_name != 'ticket' && object_name != 'notification'
 
-      # send notification
+      # send notification (after changes are done)
       if object_name == 'notification'
-
-        # 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
-
-        recipients_raw = []
-        value_recipient.each do |recipient|
-          if recipient == 'article_last_sender'
-            if article.present?
-              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.find_by(id: article.origin_by_id).email
-                recipients_raw.push(email)
-              elsif article.created_by_id
-                email = User.find_by(id: article.created_by_id).email
-                recipients_raw.push(email)
-              end
-            end
-          elsif recipient == 'ticket_customer'
-            email = User.find_by(id: customer_id).email
-            recipients_raw.push(email)
-          elsif recipient == 'ticket_owner'
-            email = User.find_by(id: owner_id).email
-            recipients_raw.push(email)
-          elsif recipient == 'ticket_agents'
-            User.group_access(group_id, 'full').sort_by(&:login).each do |user|
-              recipients_raw.push(user.email)
-            end
-          else
-            logger.error "Unknown email notification recipient '#{recipient}'"
-            next
-          end
-        end
-
-        recipients_checked = []
-        recipients_raw.each do |recipient_email|
-
-          skip_user = false
-          users = User.where(email: recipient_email)
-          users.each do |user|
-            next if user.preferences[:mail_delivery_failed] != true
-            next if !user.preferences[:mail_delivery_failed_data]
-            till_blocked = ((user.preferences[:mail_delivery_failed_data] - Time.zone.now - 60.days) / 60 / 60 / 24).round
-            next if till_blocked.positive?
-            logger.info "Send no trigger based notification to #{recipient_email} because email is marked as mail_delivery_failed for #{till_blocked} days"
-            skip_user = true
-            break
-          end
-          next if skip_user
-
-          # send notifications only to email adresses
-          next if recipient_email.blank?
-          next if recipient_email !~ /@/
-
-          # check if address is valid
-          begin
-            Mail::AddressList.new(recipient_email).addresses.each do |address|
-              recipient_email = address.address
-              break if recipient_email.present? && recipient_email =~ /@/ && !recipient_email.match?(/\s/)
-            end
-          rescue
-            if recipient_email.present?
-              if recipient_email !~ /^(.+?)<(.+?)@(.+?)>$/
-                next # no usable format found
-              end
-              recipient_email = "#{$2}@#{$3}"
-            end
-            next if recipient_email.blank?
-            next if recipient_email !~ /@/
-            next if recipient_email.match?(/\s/)
-          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 recipient_email.match?(/#{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 recipient_email.match?(/(mailer-daemon|postmaster|abuse|root|noreply|noreply.+?|no-reply|no-reply.+?)@.+?/i)
-          end
-
-          # check if notification should be send because of customer emails
-          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
-          map = {
-            10 => 10,
-            30 => 15,
-            60 => 25,
-            180 => 50,
-            600 => 100,
-          }
-          skip = false
-          map.each do |minutes, count|
-            already_sent = Ticket::Article.where(
-              ticket_id: id,
-              sender: Ticket::Article::Sender.find_by(name: 'System'),
-              type: Ticket::Article::Type.find_by(name: 'email'),
-            ).where('ticket_articles.created_at > ? AND ticket_articles.to LIKE ?', Time.zone.now - minutes.minutes, "%#{recipient_email.strip}%").count
-            next if already_sent < count
-            logger.info "Send no trigger based notification to #{recipient_email} because already sent #{count} for this ticket within last #{minutes} minutes (loop protection)"
-            skip = true
-            break
-          end
-          next if skip
-          map = {
-            10 => 30,
-            30 => 60,
-            60 => 120,
-            180 => 240,
-            600 => 360,
-          }
-          skip = false
-          map.each do |minutes, count|
-            already_sent = Ticket::Article.where(
-              sender: Ticket::Article::Sender.find_by(name: 'System'),
-              type: Ticket::Article::Type.find_by(name: 'email'),
-            ).where('ticket_articles.created_at > ? AND ticket_articles.to LIKE ?', Time.zone.now - minutes.minutes, "%#{recipient_email.strip}%").count
-            next if already_sent < count
-            logger.info "Send no trigger based notification to #{recipient_email} because already sent #{count} in total within last #{minutes} minutes (loop protection)"
-            skip = true
-            break
-          end
-          next if skip
-
-          email = recipient_email.downcase.strip
-          next if recipients_checked.include?(email)
-          recipients_checked.push(email)
-        end
-
-        next if recipients_checked.blank?
-        recipient_string = recipients_checked.join(', ')
-
-        group = self.group
-        next if !group
-        email_address = group.email_address
-        if !email_address
-          logger.info "Unable to send trigger based notification to #{recipient_string} because no email address is set for group '#{group.name}'"
-          next
-        end
-
-        if !email_address.channel_id
-          logger.info "Unable to send trigger based notification to #{recipient_string} because no channel is set for email address '#{email_address.email}' (id: #{email_address.id})"
-          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: article || articles.last
-        }
-
-        # get subject
-        subject = NotificationFactory::Mailer.template(
-          templateInline: value['subject'],
-          locale: 'en-en',
-          objects: objects,
-          quote: false,
-        )
-        subject = subject_build(subject)
-
-        body = NotificationFactory::Mailer.template(
-          templateInline: value['body'],
-          locale: 'en-en',
-          objects: objects,
-          quote: true,
-        )
-
-        (body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id)
-
-        message = Ticket::Article.create(
-          ticket_id: id,
-          to: recipient_string,
-          subject: subject,
-          content_type: 'text/html',
-          body: body,
-          internal: false,
-          sender: Ticket::Article::Sender.find_by(name: 'System'),
-          type: Ticket::Article::Type.find_by(name: 'email'),
-          preferences: {
-            perform_origin: perform_origin,
-          },
-          updated_by_id: 1,
-          created_by_id: 1,
-        )
-
-        attachments_inline.each do |attachment|
-          Store.add(
-            object: 'Ticket::Article',
-            o_id: message.id,
-            data: attachment[:data],
-            filename: attachment[:filename],
-            preferences: attachment[:preferences],
-          )
-        end
+        perform_notification[key] = value
         next
       end
 
@@ -1081,10 +877,227 @@ perform changes on ticket
       changed = true
 
       self[attribute] = value['value']
-      logger.debug { "set #{object_name}.#{attribute} = #{value['value'].inspect}" }
+      logger.debug { "set #{object_name}.#{attribute} = #{value['value'].inspect} for ticket_id #{id}" }
+    end
+
+    if changed
+      save!
+    end
+
+    perform_notification.each do |key, value|
+      perform_changes_notification(key, value, perform_origin, article)
+    end
+
+    true
+  end
+
+  def perform_changes_notification(_key, value, perform_origin, article)
+
+    # 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
+
+    recipients_raw = []
+    value_recipient.each do |recipient|
+      if recipient == 'article_last_sender'
+        if article.present?
+          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.find_by(id: article.origin_by_id).email
+            recipients_raw.push(email)
+          elsif article.created_by_id
+            email = User.find_by(id: article.created_by_id).email
+            recipients_raw.push(email)
+          end
+        end
+      elsif recipient == 'ticket_customer'
+        email = User.find_by(id: customer_id).email
+        recipients_raw.push(email)
+      elsif recipient == 'ticket_owner'
+        email = User.find_by(id: owner_id).email
+        recipients_raw.push(email)
+      elsif recipient == 'ticket_agents'
+        User.group_access(group_id, 'full').sort_by(&:login).each do |user|
+          recipients_raw.push(user.email)
+        end
+      else
+        logger.error "Unknown email notification recipient '#{recipient}'"
+        next
+      end
+    end
+
+    recipients_checked = []
+    recipients_raw.each do |recipient_email|
+
+      skip_user = false
+      users = User.where(email: recipient_email)
+      users.each do |user|
+        next if user.preferences[:mail_delivery_failed] != true
+        next if !user.preferences[:mail_delivery_failed_data]
+        till_blocked = ((user.preferences[:mail_delivery_failed_data] - Time.zone.now - 60.days) / 60 / 60 / 24).round
+        next if till_blocked.positive?
+        logger.info "Send no trigger based notification to #{recipient_email} because email is marked as mail_delivery_failed for #{till_blocked} days"
+        skip_user = true
+        break
+      end
+      next if skip_user
+
+      # send notifications only to email adresses
+      next if recipient_email.blank?
+      next if recipient_email !~ /@/
+
+      # check if address is valid
+      begin
+        Mail::AddressList.new(recipient_email).addresses.each do |address|
+          recipient_email = address.address
+          break if recipient_email.present? && recipient_email =~ /@/ && !recipient_email.match?(/\s/)
+        end
+      rescue
+        if recipient_email.present?
+          if recipient_email !~ /^(.+?)<(.+?)@(.+?)>$/
+            next # no usable format found
+          end
+          recipient_email = "#{$2}@#{$3}"
+        end
+        next if recipient_email.blank?
+        next if recipient_email !~ /@/
+        next if recipient_email.match?(/\s/)
+      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 recipient_email.match?(/#{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 recipient_email.match?(/(mailer-daemon|postmaster|abuse|root|noreply|noreply.+?|no-reply|no-reply.+?)@.+?/i)
+      end
+
+      # check if notification should be send because of customer emails
+      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
+      map = {
+        10 => 10,
+        30 => 15,
+        60 => 25,
+        180 => 50,
+        600 => 100,
+      }
+      skip = false
+      map.each do |minutes, count|
+        already_sent = Ticket::Article.where(
+          ticket_id: id,
+          sender: Ticket::Article::Sender.find_by(name: 'System'),
+          type: Ticket::Article::Type.find_by(name: 'email'),
+        ).where('ticket_articles.created_at > ? AND ticket_articles.to LIKE ?', Time.zone.now - minutes.minutes, "%#{recipient_email.strip}%").count
+        next if already_sent < count
+        logger.info "Send no trigger based notification to #{recipient_email} because already sent #{count} for this ticket within last #{minutes} minutes (loop protection)"
+        skip = true
+        break
+      end
+      next if skip
+      map = {
+        10 => 30,
+        30 => 60,
+        60 => 120,
+        180 => 240,
+        600 => 360,
+      }
+      skip = false
+      map.each do |minutes, count|
+        already_sent = Ticket::Article.where(
+          sender: Ticket::Article::Sender.find_by(name: 'System'),
+          type: Ticket::Article::Type.find_by(name: 'email'),
+        ).where('ticket_articles.created_at > ? AND ticket_articles.to LIKE ?', Time.zone.now - minutes.minutes, "%#{recipient_email.strip}%").count
+        next if already_sent < count
+        logger.info "Send no trigger based notification to #{recipient_email} because already sent #{count} in total within last #{minutes} minutes (loop protection)"
+        skip = true
+        break
+      end
+      next if skip
+
+      email = recipient_email.downcase.strip
+      next if recipients_checked.include?(email)
+      recipients_checked.push(email)
+    end
+
+    return if recipients_checked.blank?
+    recipient_string = recipients_checked.join(', ')
+
+    group_id = self.group_id
+    return if !group_id
+    email_address = Group.find(group_id).email_address
+    if !email_address
+      logger.info "Unable to send trigger based notification to #{recipient_string} because no email address is set for group '#{group.name}'"
+      return
+    end
+
+    if !email_address.channel_id
+      logger.info "Unable to send trigger based notification to #{recipient_string} because no channel is set for email address '#{email_address.email}' (id: #{email_address.id})"
+      return
+    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: article || articles.last
+    }
+
+    # get subject
+    subject = NotificationFactory::Mailer.template(
+      templateInline: value['subject'],
+      locale: 'en-en',
+      objects: objects,
+      quote: false,
+    )
+    subject = subject_build(subject)
+
+    body = NotificationFactory::Mailer.template(
+      templateInline: value['body'],
+      locale: 'en-en',
+      objects: objects,
+      quote: true,
+    )
+
+    (body, attachments_inline) = HtmlSanitizer.replace_inline_images(body, id)
+
+    message = Ticket::Article.create(
+      ticket_id: id,
+      to: recipient_string,
+      subject: subject,
+      content_type: 'text/html',
+      body: body,
+      internal: false,
+      sender: Ticket::Article::Sender.find_by(name: 'System'),
+      type: Ticket::Article::Type.find_by(name: 'email'),
+      preferences: {
+        perform_origin: perform_origin,
+      },
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    attachments_inline.each do |attachment|
+      Store.add(
+        object: 'Ticket::Article',
+        o_id: message.id,
+        data: attachment[:data],
+        filename: attachment[:filename],
+        preferences: attachment[:preferences],
+      )
     end
-    return if !changed
-    save
 
     true
   end
@@ -1136,6 +1149,8 @@ perform active triggers on ticket
 
     Transaction.execute(local_options) do
       triggers.each do |trigger|
+        logger.debug { "Probe trigger (#{trigger.name}/#{trigger.id}) for this object (Ticket:#{ticket.id}/Loop:#{local_options[:loop_count]})" }
+
         condition = trigger.condition
 
         # check if one article attribute is used

test/unit/ticket_trigger_extended_test.rb

@@ -715,4 +715,141 @@ Some Text'
 
   end
 
+  test 'recursive trigger with auto responder' do
+
+    group1 = Group.create!(
+      name: 'Group dispatch',
+      active: true,
+      created_by_id: 1,
+      updated_by_id: 1,
+    )
+    group2 = Group.create!(
+      name: 'Group with auto responder',
+      active: true,
+      email_address: EmailAddress.first,
+      created_by_id: 1,
+      updated_by_id: 1,
+    )
+
+    trigger1 = Trigger.create!(
+      name: "002 - move ticket to #{group2.name}",
+      condition: {
+        'ticket.action' => {
+          'operator' => 'is',
+          'value' => 'create',
+        },
+        'ticket.group_id' => {
+          'operator' => 'is',
+          'value' => group1.id.to_s,
+        },
+        'ticket.organization_id' => {
+          'operator' => 'is',
+          'pre_condition' => 'specific',
+          'value' => User.lookup(email: 'nicole.braun@zammad.org').organization_id.to_s,
+        }
+      },
+      perform: {
+        'ticket.group_id' => {
+          'value' => group2.id.to_s,
+        },
+      },
+      disable_notification: true,
+      active: true,
+      created_by_id: 1,
+      updated_by_id: 1,
+    )
+
+    trigger2 = Trigger.create_or_update(
+      name: "001 auto reply for tickets in group #{group1.name}",
+      condition: {
+        'ticket.action' => {
+          'operator' => 'is',
+          'value' => 'create',
+        },
+        'ticket.state_id' => {
+          'operator' => 'is',
+          'value' => Ticket::State.lookup(name: 'new').id.to_s,
+        },
+        'ticket.group_id' => {
+          'operator' => 'is not',
+          'value' => group1.id.to_s,
+        },
+      },
+      perform: {
+        'notification.email' => {
+          'body' => "some text<br>\#{ticket.customer.lastname}<br>\#{ticket.title}<br>\#{article.body}",
+          'recipient' => 'ticket_customer',
+          'subject' => "Thanks for your inquiry (\#{ticket.title})!",
+        },
+        'ticket.priority_id' => {
+          'value' => Ticket::Priority.lookup(name: '3 high').id.to_s,
+        },
+        'ticket.tags' => {
+          'operator' => 'add',
+          'value' => 'aa, kk',
+        },
+      },
+      disable_notification: true,
+      active: true,
+      created_by_id: 1,
+      updated_by_id: 1,
+    )
+
+    ticket1 = Ticket.create!(
+      title: "some <b>title</b>\n äöüß",
+      group: group1,
+      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@example.com',
+      subject: 'some subject',
+      message_id: 'some@id',
+      body: "some message <b>note</b>\nnew line",
+      internal: false,
+      sender: Ticket::Article::Sender.find_by(name: 'Customer'),
+      type: Ticket::Article::Type.find_by(name: 'web'),
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    ticket1.reload
+    assert_equal('some <b>title</b>  äöüß', ticket1.title, 'ticket1.title verify')
+    assert_equal('Group dispatch', ticket1.group.name, 'ticket1.group verify')
+    assert_equal('new', ticket1.state.name, 'ticket1.state verify')
+    assert_equal('2 normal', ticket1.priority.name, 'ticket1.priority verify')
+    assert_equal(1, ticket1.articles.count, 'ticket1.articles verify')
+    assert_equal([], ticket1.tag_list)
+
+    Observer::Transaction.commit
+
+    ticket1.reload
+    assert_equal('some <b>title</b>  äöüß', ticket1.title, 'ticket1.title verify')
+    assert_equal('Group with auto responder', ticket1.group.name, 'ticket1.group verify')
+    assert_equal('new', ticket1.state.name, 'ticket1.state verify')
+    assert_equal('3 high', ticket1.priority.name, 'ticket1.priority verify')
+    assert_equal(2, ticket1.articles.count, 'ticket1.articles verify')
+    assert_equal(%w[aa kk], ticket1.tag_list)
+
+    email_raw = "From: nicole.braun@zammad.org
+To: zammad@example.com
+Subject: test 1
+X-Zammad-Ticket-Group: #{group1.name}
+
+test 1"
+
+    ticket2, article2, user2 = Channel::EmailParser.new.process({ trusted: true }, email_raw)
+
+    assert_equal('test 1', ticket2.title, 'ticket2.title verify')
+    assert_equal('Group with auto responder', ticket2.group.name, 'ticket2.group verify')
+    assert_equal('new', ticket2.state.name, 'ticket2.state verify')
+    assert_equal('3 high', ticket2.priority.name, 'ticket2.priority verify')
+    assert_equal(2, ticket2.articles.count, 'ticket2.articles verify')
+    assert_equal(%w[aa kk], ticket2.tag_list)
+
+  end
+
 end