From 725f21337d7974445dcab6bfed818a6f0a40de85 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 1 Nov 2017 15:23:44 +0100 Subject: [PATCH] Fixed issue #1375 - Improved handling assignment_timeout. --- .../observer/ticket/last_owner_update.rb | 33 +++-- app/models/ticket.rb | 2 +- test/unit/ticket_last_owner_update_test.rb | 114 ++++++++++++++++-- 3 files changed, 128 insertions(+), 21 deletions(-) diff --git a/app/models/observer/ticket/last_owner_update.rb b/app/models/observer/ticket/last_owner_update.rb index e96b5712d..4d081b05b 100644 --- a/app/models/observer/ticket/last_owner_update.rb +++ b/app/models/observer/ticket/last_owner_update.rb @@ -4,31 +4,48 @@ class Observer::Ticket::LastOwnerUpdate < ActiveRecord::Observer observe 'ticket' def before_create(record) - _check('create', record) + _check(record) end def before_update(record) - _check('update', record) + _check(record) end private - def _check(type, record) + def _check(record) # return if we run import mode return true if Setting.get('import_mode') - # check if owner has changed - if type == 'update' - return true if record.changes_to_save['owner_id'].blank? - end + # check if owner, state or group has changed + return true if record.changes_to_save['owner_id'].blank? && record.changes_to_save['state_id'].blank? && record.changes_to_save['group_id'].blank? && record.changes_to_save['last_contact_agent_at'].blank? # check if owner is nobody - if record.owner_id.blank? || record.owner_id == 1 + if record.changes_to_save['owner_id'].present? && record.changes_to_save['owner_id'][1] == 1 record.last_owner_update_at = nil return true end + # check if group is change + if record.changes_to_save['group_id'].present? + group = Group.lookup(id: record.changes_to_save['group_id'][1]) + return true if !group + if group.assignment_timeout.blank? || group.assignment_timeout.zero? + record.last_owner_update_at = nil + return true + end + end + + # check if state is not new/open + if record.changes_to_save['state_id'].present? + state_ids = Ticket::State.by_category(:work_on).pluck(:id) + if !state_ids.include?(record.changes_to_save['state_id'][1]) + record.last_owner_update_at = nil + return true + end + end + record.last_owner_update_at = Time.zone.now end end diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 37a341ea9..8568dc75c 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -249,7 +249,7 @@ returns return [] if groups.blank? groups.each do |group| next if group.assignment_timeout.blank? - ticket_ids = Ticket.where('state_id IN (?) AND owner_id != 1 AND group_id = ?', state_ids, group.id).limit(600).pluck(:id) + ticket_ids = Ticket.where('state_id IN (?) AND owner_id != 1 AND group_id = ? AND last_owner_update_at IS NOT NULL', state_ids, group.id).limit(600).pluck(:id) ticket_ids.each do |ticket_id| ticket = Ticket.find_by(id: ticket_id) next if !ticket diff --git a/test/unit/ticket_last_owner_update_test.rb b/test/unit/ticket_last_owner_update_test.rb index 33492783c..74aa16c7e 100644 --- a/test/unit/ticket_last_owner_update_test.rb +++ b/test/unit/ticket_last_owner_update_test.rb @@ -6,6 +6,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase setup do group = Group.create_or_update( name: 'LastOwnerUpdate', + email_address: EmailAddress.first, assignment_timeout: 60, updated_by_id: 1, created_by_id: 1, @@ -25,6 +26,95 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase ) end + test 'last_owner_update_at check by state' do + + ticket = Ticket.create!( + title: 'assignment_timeout test by state 1', + group: Group.lookup(name: 'LastOwnerUpdate'), + owner: @agent1, + customer_id: 2, + state: Ticket::State.lookup(name: 'new'), + updated_by_id: 1, + created_by_id: 1, + ) + assert_equal(ticket.last_owner_update_at.to_s, ticket.updated_at.to_s) + + ticket.state = Ticket::State.lookup(name: 'closed') + ticket.save! + assert_nil(ticket.last_owner_update_at) + + ticket = Ticket.create!( + title: 'assignment_timeout test by state 1', + group: Group.lookup(name: 'LastOwnerUpdate'), + owner: @agent1, + customer_id: 2, + state: Ticket::State.lookup(name: 'pending reminder'), + updated_by_id: 1, + created_by_id: 1, + ) + assert_nil(ticket.last_owner_update_at) + + ticket.state = Ticket::State.lookup(name: 'open') + ticket.save! + assert_equal(ticket.last_owner_update_at.to_s, ticket.updated_at.to_s) + + end + + test 'last_owner_update_at check with agent reply' do + + ticket = Ticket.create!( + title: 'assignment_timeout test by state 1', + group: Group.lookup(name: 'LastOwnerUpdate'), + owner: @agent1, + customer_id: 2, + state: Ticket::State.lookup(name: 'open'), + updated_by_id: 1, + created_by_id: 1, + ) + assert_equal(ticket.last_owner_update_at.to_s, ticket.updated_at.to_s) + + travel 1.hour + + article1 = Ticket::Article.create( + ticket_id: ticket.id, + from: 'some_sender@example.com', + to: 'some_recipient@example.com', + subject: 'some subject', + message_id: 'some@id', + body: 'some message reply by customer email', + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Customer'), + type: Ticket::Article::Type.find_by(name: 'email'), + updated_by_id: 2, + created_by_id: 2, + ) + + ticket_last_owner_update_at = ticket.last_owner_update_at + ticket.reload + assert_equal(ticket_last_owner_update_at.to_s, ticket.last_owner_update_at.to_s) + + travel 1.hour + + article2 = Ticket::Article.create( + ticket_id: ticket.id, + from: 'some_sender@example.com', + to: 'some_recipient@example.com', + subject: 'some subject', + message_id: 'some@id', + body: 'some message reply by agent email', + internal: false, + sender: Ticket::Article::Sender.find_by(name: 'Agent'), + type: Ticket::Article::Type.find_by(name: 'email'), + updated_by_id: @agent1.id, + created_by_id: @agent1.id, + ) + + ticket_last_owner_update_at = Time.zone.now + ticket.reload + assert_equal(ticket_last_owner_update_at.to_s, ticket.last_owner_update_at.to_s) + + end + test 'last_owner_update_at check' do ticket = Ticket.create!( @@ -77,7 +167,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase group: Group.lookup(name: 'LastOwnerUpdate'), owner: @agent1, customer_id: 2, - state: Ticket::State.lookup(name: 'closed'), + state: Ticket::State.lookup(name: 'open'), updated_by_id: 1, created_by_id: 1, ) @@ -119,7 +209,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase ticket = Ticket.create!( title: 'assignment_timeout test 2', - group: Group.lookup(name: 'Users'), + group: Group.lookup(name: 'LastOwnerUpdate'), owner: @agent1, customer_id: 2, state: Ticket::State.lookup(name: 'new'), @@ -141,7 +231,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - assert_equal(ticket.last_owner_update_at.to_s, ticket.updated_at.to_s) + assert_nil(ticket.last_owner_update_at) ticket.owner_id = 1 ticket.save! @@ -177,7 +267,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase group: Group.lookup(name: 'LastOwnerUpdate'), owner: @agent1, customer_id: 2, - state: Ticket::State.lookup(name: 'closed'), + state: Ticket::State.lookup(name: 'open'), updated_by_id: 1, created_by_id: 1, ) @@ -195,7 +285,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase ticket5 = Ticket.create!( title: 'assignment_timeout test 5', - group: Group.lookup(name: 'Users'), + group: Group.lookup(name: 'LastOwnerUpdate'), owner: @agent1, customer_id: 2, state: Ticket::State.lookup(name: 'new'), @@ -229,7 +319,7 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase travel 15.minutes Ticket.process_auto_unassign - ticket2_updated_at = Time.current + ticket_updated_at = Time.current ticket1after = Ticket.find(ticket1.id) assert_nil(ticket1.last_owner_update_at) @@ -238,20 +328,20 @@ class TicketLastOwnerUpdateTest < ActiveSupport::TestCase ticket2after = Ticket.find(ticket2.id) assert_nil(ticket2after.last_owner_update_at) assert_equal(ticket2after.owner_id, 1) - assert_equal(ticket2_updated_at.to_s, ticket2after.updated_at.to_s) + assert_equal(ticket_updated_at.to_s, ticket2after.updated_at.to_s) ticket3after = Ticket.find(ticket3.id) - assert_equal(ticket3after.owner_id, @agent1.id) - assert_equal(ticket3.last_owner_update_at.to_s, ticket3after.last_owner_update_at.to_s) - assert_equal(ticket3.updated_at.to_s, ticket3after.updated_at.to_s) + assert_nil(ticket3after.last_owner_update_at) + assert_equal(ticket3after.owner_id, 1) + assert_equal(ticket_updated_at.to_s, ticket3after.updated_at.to_s) ticket4after = Ticket.find(ticket4.id) assert_nil(ticket4.last_owner_update_at) assert_equal(ticket4.updated_at.to_s, ticket4after.updated_at.to_s) ticket5after = Ticket.find(ticket5.id) - assert_equal(ticket5after.owner_id, @agent1.id) - assert_equal(ticket5.updated_at.to_s, ticket5after.updated_at.to_s) + assert_equal(ticket5after.owner_id, 1) + assert_equal(ticket_updated_at.to_s, ticket5after.updated_at.to_s) end