From f601553f6d8a2c33c232be20504bfaf868338871 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 18 Nov 2016 08:25:07 +0100 Subject: [PATCH] Fixed issue#441 - Unable to execute more then one trigger (e. g. adding 2 tags with 2 triggers) at one ticket at same update cycle. --- app/models/observer/transaction.rb | 1 + app/models/ticket.rb | 16 ++ app/models/transaction.rb | 5 +- app/models/transaction/background_job.rb | 4 +- app/models/transaction/trigger.rb | 150 ++++++++++--------- test/unit/ticket_trigger_test.rb | 183 +++++++++++++++++++++-- 6 files changed, 270 insertions(+), 89 deletions(-) diff --git a/app/models/observer/transaction.rb b/app/models/observer/transaction.rb index 385263676..e62b7e788 100644 --- a/app/models/observer/transaction.rb +++ b/app/models/observer/transaction.rb @@ -29,6 +29,7 @@ class Observer::Transaction < ActiveRecord::Observer sync_backends = [] Setting.where(area: 'Transaction::Backend::Sync').order(:name).each { |setting| backend = Setting.get(setting.name) + next if params[:disable] && params[:disable].include?(backend) sync_backends.push Kernel.const_get(backend) } diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 220935ae2..5ad23f19c 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -844,6 +844,22 @@ result references end +=begin + +get all articles of a ticket in correct order (overwrite active record default method) + + artilces = ticket.articles + +result + + [article1, articl2] + +=end + + def articles + Ticket::Article.where(ticket_id: id).order(:created_at, :id) + end + private def check_generate diff --git a/app/models/transaction.rb b/app/models/transaction.rb index 23e1641d1..25b94fa55 100644 --- a/app/models/transaction.rb +++ b/app/models/transaction.rb @@ -13,7 +13,10 @@ class Transaction if options[:interface_handle] ApplicationHandleInfo.current = original_interface_handle end - Observer::Transaction.commit(disable_notification: options[:disable_notification]) + Observer::Transaction.commit( + disable_notification: options[:disable_notification], + disable: options[:disable], + ) PushMessages.finish end end diff --git a/app/models/transaction/background_job.rb b/app/models/transaction/background_job.rb index e8a71ef2b..57a649b1a 100644 --- a/app/models/transaction/background_job.rb +++ b/app/models/transaction/background_job.rb @@ -24,7 +24,9 @@ class Transaction::BackgroundJob def perform Setting.where(area: 'Transaction::Backend::Async').order(:name).each { |setting| - backend = Kernel.const_get(Setting.get(setting.name)) + backend = Setting.get(setting.name) + next if @params[:disable] && @params[:disable].include?(backend) + backend = Kernel.const_get(backend) Observer::Transaction.execute_singel_backend(backend, @item, @params) } end diff --git a/app/models/transaction/trigger.rb b/app/models/transaction/trigger.rb index 90fff8c26..7b2b6ad5d 100644 --- a/app/models/transaction/trigger.rb +++ b/app/models/transaction/trigger.rb @@ -29,7 +29,7 @@ class Transaction::Trigger return if @item[:object] != 'Ticket' - triggers = Trigger.where(active: true) + triggers = Trigger.where(active: true).order('LOWER(name)') return if triggers.empty? ticket = Ticket.lookup(id: @item[:object_id]) @@ -39,86 +39,94 @@ class Transaction::Trigger end original_user_id = UserInfo.current_user_id - UserInfo.current_user_id = 1 - triggers.each { |trigger| - condition = trigger.condition + Transaction.execute(reset_user_id: true, disable: ['Transaction::Trigger']) do + triggers.each { |trigger| + condition = trigger.condition - # check action - if condition['ticket.action'] - next if condition['ticket.action']['operator'] == 'is' && condition['ticket.action']['value'] != @item[:type] - next if condition['ticket.action']['operator'] != 'is' && condition['ticket.action']['value'] == @item[:type] - condition.delete('ticket.action') - end - - # check "has changed" options - has_changed_condition_exists = false - has_changed = false - condition.each do |key, value| - next if !value - next if !value['operator'] - next if !value['operator']['has changed'] - has_changed_condition_exists = true - - # next if has changed? && !@item[:changes][attribute] - (object_name, attribute) = key.split('.', 2) - - # remove condition item, because it has changed - if @item[:changes][attribute] - has_changed = true - condition.delete(key) - next + # check action + if condition['ticket.action'] + next if condition['ticket.action']['operator'] == 'is' && condition['ticket.action']['value'] != @item[:type] + next if condition['ticket.action']['operator'] != 'is' && condition['ticket.action']['value'] == @item[:type] + condition.delete('ticket.action') end - break - end - next if has_changed_condition_exists && !has_changed + # check action + if condition['article.action'] + next if !article + condition.delete('article.action') + end - # check if selector is matching - condition['ticket.id'] = { - operator: 'is', - value: ticket.id, - } - if article - condition['article.id'] = { - operator: 'is', - value: article.id, - } - end + # check "has changed" options + has_changed_condition_exists = false + has_changed = false + condition.each do |key, value| + next if !value + next if !value['operator'] + next if !value['operator']['has changed'] + has_changed_condition_exists = true - ticket_count, tickets = Ticket.selectors(condition, 1) - next if ticket_count.zero? - next if tickets.first.id != ticket.id + # next if has changed? && !@item[:changes][attribute] + (object_name, attribute) = key.split('.', 2) - # check if min one article attribute is used - article_selector = false - trigger.condition.each do |key, _value| - (object_name, attribute) = key.split('.', 2) - next if object_name != 'article' - next if attribute == 'id' - article_selector = true - end - - # check in min one attribute has changed - if @item[:type] == 'update' && !article_selector - match = false - if has_changed_condition_exists && has_changed - match = true - else - trigger.condition.each do |key, _value| - (object_name, attribute) = key.split('.', 2) - next if object_name != 'ticket' - next if !@item[:changes][attribute] - match = true - break + # remove condition item, because it has changed + if @item[:changes][attribute] + has_changed = true + condition.delete(key) + next end + break end - next if !match - end - Transaction.execute do + + next if has_changed_condition_exists && !has_changed + + # check if selector is matching + condition['ticket.id'] = { + operator: 'is', + value: ticket.id, + } + + # check if min one article attribute is used + article_selector = false + trigger.condition.each do |key, _value| + (object_name, attribute) = key.split('.', 2) + next if object_name != 'article' + next if attribute == 'id' + article_selector = true + end + + next if article_selector && !article + if article_selector + condition['article.id'] = { + operator: 'is', + value: article.id, + } + end + + ticket_count, tickets = Ticket.selectors(condition, 1) + next if ticket_count.zero? + next if tickets.first.id != ticket.id + + # check in min one attribute has changed + if @item[:type] == 'update' && !article_selector + match = false + if has_changed_condition_exists && has_changed + match = true + else + trigger.condition.each do |key, _value| + (object_name, attribute) = key.split('.', 2) + next if object_name != 'ticket' + next if !@item[:changes][attribute] + match = true + break + end + end + next if !match + end + ticket.perform_changes(trigger.perform, 'trigger', @item) - end - } + } + end UserInfo.current_user_id = original_user_id end diff --git a/test/unit/ticket_trigger_test.rb b/test/unit/ticket_trigger_test.rb index 5d86c2659..91ac530d3 100644 --- a/test/unit/ticket_trigger_test.rb +++ b/test/unit/ticket_trigger_test.rb @@ -4,8 +4,37 @@ require 'test_helper' class TicketTriggerTest < ActiveSupport::TestCase test '1 basic' do trigger1 = Trigger.create_or_update( + name: 'aaa loop check', + condition: { + 'article.subject' => { + 'operator' => 'contains', + 'value' => 'Thanks for your inquiry', + }, + }, + perform: { + 'ticket.tags' => { + 'operator' => 'add', + 'value' => 'should_not_loop', + }, + 'notification.email' => { + 'body' => 'some lala', + 'recipient' => 'ticket_customer', + 'subject' => 'Thanks for your inquiry - loop check (#{ticket.title})!', + }, + }, + disable_notification: true, + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + trigger2 = 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, @@ -31,7 +60,54 @@ class TicketTriggerTest < ActiveSupport::TestCase updated_by_id: 1, ) - trigger2 = Trigger.create_or_update( + trigger3 = Trigger.create_or_update( + name: 'auto tag 1', + condition: { + 'ticket.action' => { + 'operator' => 'is', + 'value' => 'update', + }, + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'ticket.priority_id' => { + 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, + }, + 'ticket.tags' => { + 'operator' => 'remove', + 'value' => 'kk', + }, + }, + disable_notification: true, + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + trigger4 = Trigger.create_or_update( + name: 'auto tag 2', + condition: { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + }, + perform: { + 'ticket.tags' => { + 'operator' => 'add', + 'value' => 'abc', + }, + }, + disable_notification: true, + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + trigger5 = Trigger.create_or_update( name: 'not matching', condition: { 'ticket.state_id' => { @@ -50,6 +126,31 @@ class TicketTriggerTest < ActiveSupport::TestCase updated_by_id: 1, ) + trigger6 = Trigger.create_or_update( + name: 'zzz last', + condition: { + 'article.subject' => { + 'operator' => 'contains', + 'value' => 'some subject 1234', + }, + }, + perform: { + 'ticket.tags' => { + 'operator' => 'add', + 'value' => 'article_create_trigger', + }, + 'notification.email' => { + 'body' => 'some lala', + 'recipient' => 'ticket_customer', + 'subject' => 'Thanks for your inquiry - 1234 check (#{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'), @@ -89,7 +190,7 @@ class TicketTriggerTest < ActiveSupport::TestCase 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), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) + assert_equal(%w(aa kk abc), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) article1 = ticket1.articles.last assert_match('Zammad ', article1.from) assert_match('nicole.braun@zammad.org', article1.to) @@ -108,7 +209,7 @@ class TicketTriggerTest < ActiveSupport::TestCase assert_equal('new', ticket1.state.name, 'ticket1.state verify') assert_equal('2 normal', ticket1.priority.name, 'ticket1.priority verify') assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') - assert_equal(%w(aa kk), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) + assert_equal(%w(aa kk abc), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) ticket1.state = Ticket::State.lookup(name: 'open') ticket1.save @@ -120,7 +221,7 @@ class TicketTriggerTest < ActiveSupport::TestCase assert_equal('open', ticket1.state.name, 'ticket1.state verify') assert_equal('2 normal', ticket1.priority.name, 'ticket1.priority verify') assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') - assert_equal(%w(aa kk), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) + assert_equal(%w(aa kk abc), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) ticket1.state = Ticket::State.lookup(name: 'new') ticket1.save @@ -132,14 +233,8 @@ class TicketTriggerTest < ActiveSupport::TestCase assert_equal('Users', 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(3, ticket1.articles.count, 'ticket1.articles verify') - assert_equal(%w(aa kk), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) - article1 = ticket1.articles.last - assert_match('Zammad ', article1.from) - assert_match('nicole.braun@zammad.org', article1.to) - assert_match('Thanks for your inquiry (some title äöüß)!', article1.subject) - assert_match('Braun
some <b>title</b>', article1.body) - assert_equal('text/html', article1.content_type) + assert_equal(2, ticket1.articles.count, 'ticket1.articles verify') + assert_equal(%w(aa abc), Tag.tag_list(object: 'Ticket', o_id: ticket1.id)) ticket2 = Ticket.create( title: "some title\n äöüß", @@ -179,11 +274,12 @@ class TicketTriggerTest < ActiveSupport::TestCase created_by_id: 1, ) assert(ticket3, 'ticket3 created') + Ticket::Article.create( ticket_id: ticket3.id, from: 'some_sender@example.com', to: 'some_recipient@example.com', - subject: 'some subject', + subject: 'some subject 1234', message_id: 'some@id', content_type: 'text/html', body: 'some message note
new line', @@ -208,9 +304,9 @@ class TicketTriggerTest < ActiveSupport::TestCase assert_equal('Users', ticket3.group.name, 'ticket3.group verify') assert_equal('new', ticket3.state.name, 'ticket3.state verify') assert_equal('3 high', ticket3.priority.name, 'ticket3.priority verify') - assert_equal(2, ticket3.articles.count, 'ticket3.articles verify') - assert_equal(%w(aa kk), Tag.tag_list(object: 'Ticket', o_id: ticket3.id)) - article3 = ticket3.articles.last + assert_equal(3, ticket3.articles.count, 'ticket3.articles verify') + assert_equal(%w(aa kk abc article_create_trigger), Tag.tag_list(object: 'Ticket', o_id: ticket3.id)) + article3 = ticket3.articles[1] assert_match('Zammad ', article3.from) assert_match('nicole.braun@zammad.org', article3.to) assert_match('Thanks for your inquiry (some title äöüß3)!', article3.subject) @@ -218,6 +314,61 @@ class TicketTriggerTest < ActiveSupport::TestCase assert_match('> some message note
> new line', article3.body) assert_no_match('> some message <b>note</b>
> new line', article3.body) assert_equal('text/html', article3.content_type) + article3 = ticket3.articles[2] + assert_match('Zammad ', article3.from) + assert_match('nicole.braun@zammad.org', article3.to) + assert_match('Thanks for your inquiry - 1234 check (some title äöüß3)!', article3.subject) + assert_equal('text/html', article3.content_type) + + Ticket::Article.create( + ticket_id: ticket3.id, + from: 'some_sender@example.com', + to: 'some_recipient@example.com', + subject: 'some subject - not 1234', + message_id: 'some@id', + content_type: 'text/html', + body: 'some message note
new 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 + + ticket3 = Ticket.lookup(id: ticket3.id) + assert_equal('some title äöüß3', ticket3.title, 'ticket3.title verify') + assert_equal('Users', ticket3.group.name, 'ticket3.group verify') + assert_equal('new', ticket3.state.name, 'ticket3.state verify') + assert_equal('3 high', ticket3.priority.name, 'ticket3.priority verify') + assert_equal(4, ticket3.articles.count, 'ticket3.articles verify') + assert_equal(%w(aa kk abc article_create_trigger), Tag.tag_list(object: 'Ticket', o_id: ticket3.id)) + + Ticket::Article.create( + ticket_id: ticket3.id, + from: 'some_sender@example.com', + to: 'some_recipient@example.com', + subject: 'some subject 1234', + message_id: 'some@id', + content_type: 'text/html', + body: 'some message note
new 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 + + ticket3 = Ticket.lookup(id: ticket3.id) + assert_equal('some title äöüß3', ticket3.title, 'ticket3.title verify') + assert_equal('Users', ticket3.group.name, 'ticket3.group verify') + assert_equal('new', ticket3.state.name, 'ticket3.state verify') + assert_equal('3 high', ticket3.priority.name, 'ticket3.priority verify') + assert_equal(5, ticket3.articles.count, 'ticket3.articles verify') + assert_equal(%w(aa kk abc article_create_trigger), Tag.tag_list(object: 'Ticket', o_id: ticket3.id)) Trigger.destroy_all end