From 49d104a11c06aeebba86c76294ec4e0fc6ca51b5 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Tue, 6 Oct 2020 15:03:30 +0200 Subject: [PATCH] Refactoring: Migrate ticket_trigger_extended_recursive_disabled_test to RSpec. --- spec/models/ticket_spec.rb | 23 + spec/models/trigger_spec.rb | 185 +++++ ...rigger_extended_recursive_disabled_test.rb | 677 ------------------ 3 files changed, 208 insertions(+), 677 deletions(-) delete mode 100644 test/unit/ticket_trigger_extended_recursive_disabled_test.rb diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index 14f921130..6c34d74b6 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -1194,5 +1194,28 @@ RSpec.describe Ticket, type: :model do end end end + + describe 'Ticket lifecycle order-of-operations:' do + subject!(:ticket) { create(:ticket) } + + let!(:agent) { create(:agent, groups: [group]) } + let(:group) { create(:group) } + + before do + create( + :trigger, + condition: { 'ticket.action' => { 'operator' => 'is', 'value' => 'create' } }, + perform: { 'ticket.group_id' => { 'value' => group.id } } + ) + end + + it 'fires triggers before new ticket notifications are sent' do + expect { Observer::Transaction.commit } + .to change { ticket.reload.group }.to(group) + + expect { Scheduler.worker(true) } + .to change { NotificationFactory::Mailer.already_sent?(ticket, agent, 'email') }.to(1) + end + end end end diff --git a/spec/models/trigger_spec.rb b/spec/models/trigger_spec.rb index 9a4d822c5..38a85c797 100644 --- a/spec/models/trigger_spec.rb +++ b/spec/models/trigger_spec.rb @@ -581,4 +581,189 @@ RSpec.describe Trigger, type: :model do it_behaves_like 'successful trigger', attribute: 'ticket.updated_by_id' it_behaves_like 'successful trigger', attribute: 'ticket.owner_id' end + + describe 'Multi-trigger interactions:' do + let(:ticket) { create(:ticket) } + + context 'cascading (i.e., trigger A satisfies trigger B satisfies trigger C)' do + subject!(:triggers) do + [ + create(:trigger, condition: initial_state, perform: first_change, name: 'A'), + create(:trigger, condition: first_change, perform: second_change, name: 'B'), + create(:trigger, condition: second_change, perform: third_change, name: 'C') + ] + end + + context 'in a chain' do + let(:initial_state) do + { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + } + end + + let(:first_change) do + { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'open').id.to_s, + } + } + end + + let(:second_change) do + { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'closed').id.to_s, + } + } + end + + let(:third_change) do + { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'merged').id.to_s, + } + } + end + + context 'in alphabetical order (by name)' do + it 'fires all triggers in sequence' do + expect { Observer::Transaction.commit } + .to change { ticket.reload.state.name }.to('merged') + end + end + + context 'out of alphabetical order (by name)' do + before do + triggers.first.update(name: 'E') + triggers.second.update(name: 'F') + triggers.third.update(name: 'D') + end + + context 'with Setting ticket_trigger_recursive: true' do + before { Setting.set('ticket_trigger_recursive', true) } + + it 'evaluates triggers in sequence, then loops back to the start and re-evalutes skipped triggers' do + expect { Observer::Transaction.commit } + .to change { ticket.reload.state.name }.to('merged') + end + end + + context 'with Setting ticket_trigger_recursive: false' do + before { Setting.set('ticket_trigger_recursive', false) } + + it 'evaluates triggers in sequence, firing only the ones that match' do + expect { Observer::Transaction.commit } + .to change { ticket.reload.state.name }.to('closed') + end + end + end + end + + context 'in circular reference (i.e., trigger A satisfies trigger B satisfies trigger C satisfies trigger A...)' do + let(:initial_state) do + { + 'ticket.priority_id' => { + 'operator' => 'is', + 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, + } + } + end + + let(:first_change) do + { + 'ticket.priority_id' => { + 'operator' => 'is', + 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, + } + } + end + + let(:second_change) do + { + 'ticket.priority_id' => { + 'operator' => 'is', + 'value' => Ticket::Priority.lookup(name: '1 low').id.to_s, + } + } + end + + let(:third_change) do + { + 'ticket.priority_id' => { + 'operator' => 'is', + 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, + } + } + end + + context 'with Setting ticket_trigger_recursive: true' do + before { Setting.set('ticket_trigger_recursive', true) } + + it 'fires each trigger once, without being caught in an endless loop' do + expect { Timeout.timeout(2) { Observer::Transaction.commit } } + .to not_change { ticket.reload.priority.name } + .and not_raise_error + end + end + + context 'with Setting ticket_trigger_recursive: false' do + before { Setting.set('ticket_trigger_recursive', false) } + + it 'fires each trigger once, without being caught in an endless loop' do + expect { Timeout.timeout(2) { Observer::Transaction.commit } } + .to not_change { ticket.reload.priority.name } + .and not_raise_error + end + end + end + end + + context 'competing (i.e., trigger A un-satisfies trigger B)' do + subject!(:triggers) do + [ + create(:trigger, condition: initial_state, perform: change_a, name: 'A'), + create(:trigger, condition: initial_state, perform: change_b, name: 'B') + ] + end + + let(:initial_state) do + { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'new').id.to_s, + } + } + end + + let(:change_a) do + { + 'ticket.state_id' => { + 'operator' => 'is', + 'value' => Ticket::State.lookup(name: 'open').id.to_s, + } + } + end + + let(:change_b) do + { + 'ticket.priority_id' => { + 'operator' => 'is', + 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, + } + } + end + + it 'evaluates triggers in sequence, firing only the ones that match' do + expect { Observer::Transaction.commit } + .to change { ticket.reload.state.name }.to('open') + .and not_change { ticket.reload.priority.name } + end + end + end end diff --git a/test/unit/ticket_trigger_extended_recursive_disabled_test.rb b/test/unit/ticket_trigger_extended_recursive_disabled_test.rb deleted file mode 100644 index a70ae2f00..000000000 --- a/test/unit/ticket_trigger_extended_recursive_disabled_test.rb +++ /dev/null @@ -1,677 +0,0 @@ -require 'test_helper' - -class TicketTriggerExtendedRecursiveDisabledTest < ActiveSupport::TestCase - - setup do - Setting.set('ticket_trigger_recursive', false) - end - - test 'recursive trigger' do - Trigger.create!( - name: '1) set prio to 3 high', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - '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, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '2) set state to closed', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - email_raw_string = 'From: me@example.com -To: customer@example.com -Subject: some new subject - -Some Text' - - ticket_p, _article_p, _user_p, _mail = Channel::EmailParser.new.process({}, email_raw_string) - assert_equal('some new subject', ticket_p.title) - assert_equal('Users', ticket_p.group.name) - assert_equal('3 high', ticket_p.priority.name) - assert_equal('closed', ticket_p.state.name) - - assert_equal(1, ticket_p.articles.count, 'ticket1.articles verify') - end - - test 'recursive trigger - loop test' do - Trigger.create!( - name: '1) set prio to 3 high', - condition: { - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '2) set prio to 1 low', - condition: { - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '1 low').id.to_s, - }, - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '3) set prio to 3 high', - condition: { - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '1 low').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - email_raw_string = 'From: me@example.com -To: customer@example.com -Subject: some new subject - -Some Text' - - ticket_p, _article_p, _user_p, _mail = Channel::EmailParser.new.process({}, email_raw_string) - assert_equal('some new subject', ticket_p.title) - assert_equal('Users', ticket_p.group.name) - assert_equal('2 normal', ticket_p.priority.name) - assert_equal('open', ticket_p.state.name) - - assert_equal(1, ticket_p.articles.count, 'ticket1.articles verify') - end - - test 'recursive trigger - 2 trigger will not trigger next trigger' do - Trigger.create!( - name: '1) set prio to 3 high', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '2) set state to open', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '3) set state to closed', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - 'ticket.state_id' => { - 'operator' => 'is', - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - email_raw_string = 'From: me@example.com -To: customer@example.com -Subject: some new subject - -Some Text' - - ticket_p, _article_p, _user_p, _mail = Channel::EmailParser.new.process({}, email_raw_string) - assert_equal('some new subject', ticket_p.title) - assert_equal('Users', ticket_p.group.name) - assert_equal('3 high', ticket_p.priority.name) - assert_equal('new', ticket_p.state.name) - - assert_equal(1, ticket_p.articles.count, 'ticket1.articles verify') - - end - - test 'recursive trigger - 2 trigger will trigger next trigger - case 1' do - Trigger.create!( - name: '1) set state to closed', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - 'ticket.state_id' => { - 'operator' => 'is', - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '2) set prio to 3 high', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '3) set state to open', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - email_raw_string = 'From: me@example.com -To: customer@example.com -Subject: some new subject - -Some Text' - - ticket_p, _article_p, _user_p, _mail = Channel::EmailParser.new.process({}, email_raw_string) - - assert_equal('some new subject', ticket_p.title) - assert_equal('Users', ticket_p.group.name) - assert_equal('3 high', ticket_p.priority.name) - assert_equal('open', ticket_p.state.name) - assert_equal(1, ticket_p.articles.count, 'ticket1.articles verify') - - end - - test 'recursive trigger - 2 trigger will trigger next trigger - case 2' do - Trigger.create!( - name: '1) set prio to 3 high', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - 'ticket.state_id' => { - 'operator' => 'is', - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '2) set state to closed', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - 'ticket.state_id' => { - 'operator' => 'is', - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - Trigger.create!( - name: '3) set state to open', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'open').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - email_raw_string = 'From: me@example.com -To: customer@example.com -Subject: some new subject - -Some Text' - - ticket_p, _article_p, _user_p, _mail = Channel::EmailParser.new.process({}, email_raw_string) - - assert_equal('some new subject', ticket_p.title) - assert_equal('Users', ticket_p.group.name) - assert_equal('2 normal', ticket_p.priority.name) - assert_equal('open', ticket_p.state.name) - - assert_equal(1, ticket_p.articles.count, 'ticket1.articles verify') - - end - - test 'trigger based move and verify correct agent notifications' do - - group1 = Group.create!( - name: 'Group 1', - active: true, - email_address: EmailAddress.first, - created_by_id: 1, - updated_by_id: 1, - ) - group2 = Group.create!( - name: 'Group 2', - active: true, - email_address: EmailAddress.first, - created_by_id: 1, - updated_by_id: 1, - ) - group3 = Group.create!( - name: 'Group 3', - active: true, - email_address: EmailAddress.first, - created_by_id: 1, - updated_by_id: 1, - ) - roles = Role.where(name: 'Agent') - user1 = User.create!( - login: 'trigger1@example.org', - firstname: 'trigger1', - lastname: 'trigger1', - email: 'trigger1@example.org', - password: 'some_pass', - active: true, - groups: [group1], - roles: roles, - created_by_id: 1, - updated_by_id: 1, - ) - user2 = User.create!( - login: 'trigger2@example.org', - firstname: 'trigger2', - lastname: 'trigger2', - email: 'trigger2@example.org', - password: 'some_pass', - active: true, - groups: [group2], - roles: roles, - created_by_id: 1, - updated_by_id: 1, - ) - - # trigger, move ticket created in group1 into group3 and then into group2 - Trigger.create_or_update( - name: '1 dispatch', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.group_id' => { - 'operator' => 'is', - 'value' => group3.id.to_s, - }, - 'ticket.state_id' => { - 'operator' => 'is', - 'value' => Ticket::State.lookup(name: 'new').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, - ) - Trigger.create_or_update( - name: '2 dispatch', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.state_id' => { - 'operator' => 'is', - 'value' => Ticket::State.lookup(name: 'new').id.to_s, - }, - }, - perform: { - 'ticket.group_id' => { - 'value' => group3.id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - - ticket1 = Ticket.create!( - title: '123', - group: group1, - customer_id: 2, - state: Ticket::State.lookup(name: 'new'), - priority: Ticket::Priority.lookup(name: '2 normal'), - updated_by_id: 1, - created_by_id: 1, - ) - assert(ticket1) - - assert_equal(ticket1.title, '123') - assert_equal(ticket1.group.name, group1.name) - assert_equal(ticket1.state.name, 'new') - - 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', - 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, - ) - - # verfiy if agent1 got no notifcation - assert_equal(0, NotificationFactory::Mailer.already_sent?(ticket1, user1, 'email'), ticket1.id) - - # verfiy if agent2 got no notifcation - assert_equal(0, NotificationFactory::Mailer.already_sent?(ticket1, user2, 'email'), ticket1.id) - - Observer::Transaction.commit - Scheduler.worker(true) - - ticket1.reload - assert_equal('123', ticket1.title) - assert_equal(group3.name, ticket1.group.name) - assert_equal('new', ticket1.state.name) - assert_equal('2 normal', ticket1.priority.name) - assert_equal(1, ticket1.articles.count) - - # verfiy if agent1 got no notifcation - assert_equal(0, NotificationFactory::Mailer.already_sent?(ticket1, user1, 'email'), ticket1.id) - - # verfiy if agent2 got notifcation - assert_equal(0, NotificationFactory::Mailer.already_sent?(ticket1, user2, 'email'), ticket1.id) - - end - - test 'recursive trigger loop check' do - Trigger.create!( - name: '000', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '1 low').id.to_s, - }, - }, - perform: { - 'ticket.state_id' => { - 'value' => Ticket::State.lookup(name: 'closed').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - Trigger.create!( - name: '001', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '1 low').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - Trigger.create!( - name: '002', - condition: { - 'ticket.action' => { - 'operator' => 'is', - 'value' => 'create', - }, - 'ticket.priority_id' => { - 'operator' => 'is', - 'value' => Ticket::Priority.lookup(name: '2 normal').id.to_s, - }, - }, - perform: { - 'ticket.priority_id' => { - 'value' => Ticket::Priority.lookup(name: '3 high').id.to_s, - }, - }, - disable_notification: true, - active: true, - created_by_id: 1, - updated_by_id: 1, - ) - group1 = Group.find_by(name: 'Users') - ticket1 = Ticket.create!( - title: '123', - group: group1, - customer_id: 2, - state: Ticket::State.lookup(name: 'new'), - priority: Ticket::Priority.lookup(name: '2 normal'), - updated_by_id: 1, - created_by_id: 1, - ) - assert(ticket1) - - assert_equal(ticket1.title, '123') - assert_equal(ticket1.group.name, group1.name) - assert_equal(ticket1.state.name, 'new') - - 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', - 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 - Scheduler.worker(true) - - ticket1.reload - assert_equal('123', ticket1.title) - assert_equal('new', ticket1.state.name) - assert_equal('3 high', ticket1.priority.name) - assert_equal(1, ticket1.articles.count) - - end - -end