From 3caaad2f7fea406dbca8360fb4b48f8436455866 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 2 Sep 2016 12:54:13 +0200 Subject: [PATCH 1/3] Moved escalation calculation from after_create/after_update to before_create/before_update to have only one ticket save action. --- .../observer/ticket/escalation_calculation.rb | 41 --- app/models/ticket.rb | 9 +- app/models/ticket/escalation.rb | 242 ++++++++++++------ config/application.rb | 1 - test/unit/calendar_subscription_test.rb | 35 +-- test/unit/ticket_selector_test.rb | 8 +- test/unit/ticket_sla_test.rb | 37 +-- 7 files changed, 210 insertions(+), 163 deletions(-) delete mode 100644 app/models/observer/ticket/escalation_calculation.rb diff --git a/app/models/observer/ticket/escalation_calculation.rb b/app/models/observer/ticket/escalation_calculation.rb deleted file mode 100644 index a089c8fa2..000000000 --- a/app/models/observer/ticket/escalation_calculation.rb +++ /dev/null @@ -1,41 +0,0 @@ -# Copyright (C) 2012-2014 Zammad Foundation, http://zammad-foundation.org/ - -class Observer::Ticket::EscalationCalculation < ActiveRecord::Observer - observe 'ticket', 'ticket::_article' - - def after_create(record) - - # return if we run import mode - return if Setting.get('import_mode') && !Setting.get('import_ignore_sla') - - # do not recalculation if first respons is already out - if record.class.name == 'Ticket::Article' - record.ticket.escalation_calculation - return true - end - - # update escalation - return if record.callback_loop - record.callback_loop = true - record.escalation_calculation - record.callback_loop = false - end - - def after_update(record) - - # return if we run import mode - return if Setting.get('import_mode') && !Setting.get('import_ignore_sla') - - # do not recalculation if first respons is already out - if record.class.name == 'Ticket::Article' - record.ticket.escalation_calculation - return true - end - - # update escalation - return if record.callback_loop - record.callback_loop = true - record.escalation_calculation - record.callback_loop = false - end -end diff --git a/app/models/ticket.rb b/app/models/ticket.rb index c456fa36f..2d5519422 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -16,8 +16,8 @@ class Ticket < ApplicationModel extend Ticket::Search store :preferences - before_create :check_generate, :check_defaults, :check_title - before_update :check_defaults, :check_title, :reset_pending_time + before_create :check_generate, :check_defaults, :check_title, :check_escalation_update + before_update :check_defaults, :check_title, :reset_pending_time, :check_escalation_update before_destroy :destroy_dependencies notify_clients_support @@ -876,6 +876,11 @@ result self.pending_time = nil end + def check_escalation_update + escalation_calculation_int + true + end + def destroy_dependencies # delete articles diff --git a/app/models/ticket/escalation.rb b/app/models/ticket/escalation.rb index e02746eb3..8853e4c45 100644 --- a/app/models/ticket/escalation.rb +++ b/app/models/ticket/escalation.rb @@ -16,8 +16,10 @@ returns def self.rebuild_all state_list_open = Ticket::State.by_category('open') - tickets = Ticket.where(state_id: state_list_open) - tickets.each(&:escalation_calculation) + ticket_ids = Ticket.where(state_id: state_list_open).pluck(:id) + ticket_ids.each { |ticket_id| + Ticket.find(ticket_id).escalation_calculation(true) + } end =begin @@ -29,17 +31,37 @@ rebuild escalation for ticket returns - result = true + result = true # true = ticket has been updated | false = no changes on ticket =end - def escalation_calculation + def escalation_calculation(force = false) + return if !escalation_calculation_int(force) + self.callback_loop = true + save! + self.callback_loop = false + true + end - # set escalation off if ticket is already closed + def escalation_calculation_int(force = false) + return if callback_loop == true + + # return if we run import mode + return if Setting.get('import_mode') && !Setting.get('import_ignore_sla') + + # set escalation off if current state is not escalation relativ (e. g. ticket is closed) + return if !state_id state = Ticket::State.lookup(id: state_id) escalation_disabled = false if state.ignore_escalation? escalation_disabled = true + + # early exit if nothing current state is not escalation relativ + if !force + return false if escalation_time.nil? + self.escalation_time = nil + return true + end end # get sla for ticket @@ -51,23 +73,71 @@ returns # if no escalation is enabled if !sla || !calendar + preferences[:escalation_calculation] = {} # nothing to change - return true if !escalation_time - + return false if !escalation_time self.escalation_time = nil - self.updated_by_id = 1 - self.callback_loop = true - save return true end - # reset escalation attributes - self.escalation_time = nil - self.first_response_escal_date = nil - self.update_time_escal_date = nil - self.close_time_escal_date = nil + # get last_update + if !last_contact_customer && !last_contact_agent + last_update = created_at + elsif !last_contact_customer && last_contact_agent + last_update = last_contact_agent + elsif last_contact_customer && !last_contact_agent + last_update = last_contact_customer + elsif last_contact_agent > last_contact_customer + last_update = last_contact_agent + elsif last_contact_agent < last_contact_customer + last_update = last_contact_customer + end + # check if calculation need be done + escalation_calculation = preferences[:escalation_calculation] || {} + sla_changed = true + if escalation_calculation['sla_id'] == sla.id && escalation_calculation['sla_updated_at'] == sla.updated_at + sla_changed = false + end + calendar_changed = true + if escalation_calculation['calendar_id'] == calendar.id && escalation_calculation['calendar_updated_at'] == calendar.updated_at + calendar_changed = false + end + if sla_changed == true || calendar_changed == true + force = true + end + first_response_changed = true + if escalation_calculation['first_response'] == first_response + first_response_changed = false + end + last_update_changed = true + if escalation_calculation['last_update'] == last_update + last_update_changed = false + end + close_time_changed = true + if escalation_calculation['close_time'] == close_time + close_time_changed = false + end + + if !force && preferences[:escalation_calculation] + if first_response_changed == false && + last_update_changed == false && + close_time_changed == false && + sla_changed == false && + calendar_changed == false && + escalation_calculation['escalation_disabled'] == escalation_disabled + return false + end + end + + # reset escalation attributes + self.escalation_time = nil + if force == true + self.first_response_escal_date = nil + self.update_time_escal_date = nil + self.close_time_escal_date = nil + end biz = Biz::Schedule.new do |config| # get business hours @@ -98,89 +168,99 @@ returns } end config.holidays = holidays - - # get timezone config.time_zone = calendar.timezone end # get history data - history_data = history_get + history_data = nil - # fist response # calculate first response escalation - if sla.first_response_time - self.first_response_escal_date = destination_time(created_at, sla.first_response_time, biz, history_data) + if force == true || first_response_changed == true + if !history_data + history_data = history_get + end + if sla.first_response_time + self.first_response_escal_date = destination_time(created_at, sla.first_response_time, biz, history_data) + end + + # get response time in min + if first_response + self.first_response_in_min = pending_minutes(created_at, first_response, biz, history_data, 'business_minutes') + end + + # set time to show if sla is raised or not + if sla.first_response_time && first_response_in_min + self.first_response_diff_in_min = sla.first_response_time - first_response_in_min + end end - # get response time in min - if first_response - self.first_response_in_min = pending_minutes(created_at, first_response, biz, history_data, 'business_minutes') - else - self.escalation_time = first_response_escal_date + # calculate update time escalation + if force == true || last_update_changed == true + if !history_data + history_data = history_get + end + if sla.update_time && last_update + self.update_time_escal_date = destination_time(last_update, sla.update_time, biz, history_data) + end + + # get update time in min + if last_update && last_update != created_at + self.update_time_in_min = pending_minutes(created_at, last_update, biz, history_data, 'business_minutes') + end + + # set sla time + if sla.update_time && update_time_in_min + self.update_time_diff_in_min = sla.update_time - update_time_in_min + end end - # set time to show if sla is raised or not - if sla.first_response_time && first_response_in_min - self.first_response_diff_in_min = sla.first_response_time - first_response_in_min - end - - # update time - # calculate escalation - if !last_contact_customer && !last_contact_agent - last_update = created_at - elsif !last_contact_customer && last_contact_agent - last_update = last_contact_agent - elsif last_contact_customer && !last_contact_agent - last_update = last_contact_customer - elsif last_contact_agent > last_contact_customer - last_update = last_contact_agent - elsif last_contact_agent < last_contact_customer - last_update = last_contact_customer - end - if sla.update_time && last_update - self.update_time_escal_date = destination_time(last_update, sla.update_time, biz, history_data) - end - if update_time_escal_date && ((!escalation_time && update_time_escal_date) || update_time_escal_date < escalation_time) - self.escalation_time = update_time_escal_date - end - - # get update time in min - if last_update && last_update != created_at - self.update_time_in_min = pending_minutes(created_at, last_update, biz, history_data, 'business_minutes') - end - - # set sla time - if sla.update_time && update_time_in_min - self.update_time_diff_in_min = sla.update_time - update_time_in_min - end - - # close time # calculate close time escalation - if sla.solution_time - self.close_time_escal_date = destination_time(created_at, sla.solution_time, biz, history_data) - end - - # get close time in min - if close_time - self.close_time_in_min = pending_minutes(created_at, close_time, biz, history_data, 'business_minutes') - elsif close_time_escal_date && ((!escalation_time && close_time_escal_date) || close_time_escal_date < escalation_time) - self.escalation_time = close_time_escal_date - end - - # set time to show if sla is raised or not - if sla.solution_time && close_time_in_min - self.close_time_diff_in_min = sla.solution_time - close_time_in_min + if force == true || close_time_changed == true + if !history_data + history_data = history_get + end + if sla.solution_time + self.close_time_escal_date = destination_time(created_at, sla.solution_time, biz, history_data) + end + + # get close time in min + if close_time + self.close_time_in_min = pending_minutes(created_at, close_time, biz, history_data, 'business_minutes') + end + + # set time to show if sla is raised or not + if sla.solution_time && close_time_in_min + self.close_time_diff_in_min = sla.solution_time - close_time_in_min + end end + # set closest escalation time if escalation_disabled self.escalation_time = nil + else + if !first_response && first_response_escal_date + self.escalation_time = first_response_escal_date + end + if update_time_escal_date && ((!escalation_time && update_time_escal_date) || update_time_escal_date < escalation_time) + self.escalation_time = update_time_escal_date + end + if !close_time && close_time_escal_date && ((!escalation_time && close_time_escal_date) || close_time_escal_date < escalation_time) + self.escalation_time = close_time_escal_date + end end - return if !changed? - - self.callback_loop = true - self.updated_by_id = 1 - save + # remember already counted time to do on next update only the diff + preferences[:escalation_calculation] = { + first_response: first_response, + last_update: last_update, + close_time: close_time, + sla_id: sla.id, + sla_updated_at: sla.updated_at, + calendar_id: calendar.id, + calendar_updated_at: calendar.updated_at, + escalation_disabled: escalation_disabled, + } + true end =begin diff --git a/config/application.rb b/config/application.rb index e783fae9a..05dc172b3 100644 --- a/config/application.rb +++ b/config/application.rb @@ -31,7 +31,6 @@ module Zammad 'observer::_ticket::_article::_communicate_facebook', 'observer::_ticket::_article::_communicate_twitter', 'observer::_ticket::_reset_new_state', - 'observer::_ticket::_escalation_calculation', 'observer::_ticket::_ref_object_touch', 'observer::_ticket::_online_notification_seen', 'observer::_ticket::_stats_reopen', diff --git a/test/unit/calendar_subscription_test.rb b/test/unit/calendar_subscription_test.rb index f8451c2e9..580dc6abf 100644 --- a/test/unit/calendar_subscription_test.rb +++ b/test/unit/calendar_subscription_test.rb @@ -77,7 +77,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase Ticket.destroy_all - ticket1 = Ticket.create( + ticket1 = Ticket.create!( title: 'some title1 - new - group_calendar', group: group_calendar, customer_id: customer1.id, @@ -88,7 +88,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket2 = Ticket.create( + ticket2 = Ticket.create!( title: 'some title1 - new - group_default', group: group_default, customer_id: customer1.id, @@ -99,7 +99,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket3 = Ticket.create( + ticket3 = Ticket.create!( title: 'some title1 - pending - group_calendar', group: group_calendar, customer_id: customer1.id, @@ -111,7 +111,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket4 = Ticket.create( + ticket4 = Ticket.create!( title: 'some title1 - pending - group_default', group: group_default, customer_id: customer1.id, @@ -123,32 +123,33 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket5 = Ticket.create( + ticket5 = Ticket.create!( title: 'some title1 - escalation - group_calendar', group: group_calendar, customer_id: customer1.id, owner_id: agent1.id, state: Ticket::State.lookup(name: 'new'), - escalation_time: '2016-02-07 17:39:00', priority: Ticket::Priority.lookup(name: '2 normal'), created_at: '2016-02-05 16:41:00', updated_by_id: 1, created_by_id: 1, ) - ticket6 = Ticket.create( + ticket5.update_columns(escalation_time: '2016-02-07 17:39:00') + + ticket6 = Ticket.create!( title: 'some title1 - escalation - group_default', group: group_default, customer_id: customer1.id, owner_id: agent2.id, state: Ticket::State.lookup(name: 'new'), - escalation_time: '2016-02-07 16:37:00', priority: Ticket::Priority.lookup(name: '2 normal'), created_at: '2016-02-05 16:42:00', updated_by_id: 1, created_by_id: 1, ) + ticket6.update_columns(escalation_time: '2016-02-07 16:37:00') - ticket7 = Ticket.create( + ticket7 = Ticket.create!( title: 'some title2 - new - group_calendar', group: group_calendar, customer_id: customer1.id, @@ -159,7 +160,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket8 = Ticket.create( + ticket8 = Ticket.create!( title: 'some title2 - new - group_default', group: group_default, customer_id: customer1.id, @@ -170,7 +171,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket9 = Ticket.create( + ticket9 = Ticket.create!( title: 'some title2 - pending - group_calendar', group: group_calendar, customer_id: customer1.id, @@ -182,7 +183,7 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket10 = Ticket.create( + ticket10 = Ticket.create!( title: 'some title2 - pending - group_default', group: group_default, customer_id: customer1.id, @@ -194,30 +195,32 @@ class CalendarSubscriptionTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - ticket11 = Ticket.create( + ticket11 = Ticket.create!( title: 'some title2 - escalation - group_calendar', group: group_calendar, customer_id: customer1.id, owner_id: 1, state: Ticket::State.lookup(name: 'new'), - escalation_time: '2016-02-08 18:37:00', priority: Ticket::Priority.lookup(name: '2 normal'), created_at: '2016-02-05 17:41:00', updated_by_id: 1, created_by_id: 1, ) - ticket12 = Ticket.create( + ticket11.update_columns(escalation_time: '2016-02-08 18:37:00') + + ticket12 = Ticket.create!( title: 'some title2 - escalation - group_default', group: group_default, customer_id: customer1.id, owner_id: 1, state: Ticket::State.lookup(name: 'new'), - escalation_time: '2016-02-08 18:38:00', priority: Ticket::Priority.lookup(name: '2 normal'), created_at: '2016-02-05 17:42:00', updated_by_id: 1, created_by_id: 1, ) + ticket12.update_columns(escalation_time: '2016-02-08 18:38:00') + Cache.clear # set escalation_time manually, to clear cache to have correct content later # check agent 1 calendar_subscriptions = CalendarSubscriptions.new(agent1) diff --git a/test/unit/ticket_selector_test.rb b/test/unit/ticket_selector_test.rb index 0e89cff26..2d602b7f2 100644 --- a/test/unit/ticket_selector_test.rb +++ b/test/unit/ticket_selector_test.rb @@ -85,7 +85,7 @@ class TicketSelectorTest < ActiveSupport::TestCase Ticket.destroy_all - ticket1 = Ticket.create( + ticket1 = Ticket.create!( title: 'some title1', group: group, customer_id: customer1.id, @@ -102,7 +102,7 @@ class TicketSelectorTest < ActiveSupport::TestCase assert_equal(ticket1.organization.id, organization1.id) sleep 1 - ticket2 = Ticket.create( + ticket2 = Ticket.create!( title: 'some title2', group: group, customer_id: customer2.id, @@ -118,18 +118,18 @@ class TicketSelectorTest < ActiveSupport::TestCase assert_equal(ticket2.organization_id, nil) sleep 1 - ticket3 = Ticket.create( + ticket3 = Ticket.create!( title: 'some title3', group: group, customer_id: customer2.id, state: Ticket::State.lookup(name: 'open'), priority: Ticket::Priority.lookup(name: '2 normal'), - escalation_time: '2015-02-06 10:00:00', created_at: '2015-02-05 16:37:00', #updated_at: '2015-02-05 17:37:00', updated_by_id: 1, created_by_id: 1, ) + ticket3.update_columns(escalation_time: '2015-02-06 10:00:00') assert(ticket3, 'ticket created') assert_equal(ticket3.customer.id, customer2.id) assert_equal(ticket3.organization_id, nil) diff --git a/test/unit/ticket_sla_test.rb b/test/unit/ticket_sla_test.rb index 3c9f528ef..d81318700 100644 --- a/test/unit/ticket_sla_test.rb +++ b/test/unit/ticket_sla_test.rb @@ -11,7 +11,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = Ticket.destroy_all assert(delete, 'ticket destroy_all') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -343,7 +343,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = ticket.destroy assert(delete, 'ticket destroy') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -361,7 +361,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_equal(ticket.state.name, 'new', 'ticket.state verify') # create inbound article - article_inbound = Ticket::Article.create( + article_inbound = Ticket::Article.create!( ticket_id: ticket.id, from: 'some_sender@example.com', to: 'some_recipient@example.com', @@ -385,7 +385,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_equal(ticket.close_time, nil, 'ticket.close_time verify - inbound') # create outbound article - article_outbound = Ticket::Article.create( + article_outbound = Ticket::Article.create!( ticket_id: ticket.id, from: 'some_recipient@example.com', to: 'some_sender@example.com', @@ -414,7 +414,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = ticket.destroy assert(delete, 'ticket destroy') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -432,7 +432,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_equal(ticket.state.name, 'new', 'ticket.state verify') # create inbound article - article_inbound = Ticket::Article.create( + article_inbound = Ticket::Article.create!( ticket_id: ticket.id, from: 'some_sender@example.com', subject: 'some subject', @@ -455,7 +455,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_equal(ticket.close_time, nil, 'ticket.close_time verify - inbound') # create note article - article_note = Ticket::Article.create( + article_note = Ticket::Article.create!( ticket_id: ticket.id, from: 'some_sender@example.com', subject: 'some subject', @@ -478,7 +478,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_equal(ticket.close_time, nil, 'ticket.close_time verify - inbound') # create outbound article - article_outbound = Ticket::Article.create( + article_outbound = Ticket::Article.create!( ticket_id: ticket.id, from: 'some_sender@example.com', subject: 'some subject', @@ -515,7 +515,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = Ticket.destroy_all assert(delete, 'ticket destroy_all') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -610,7 +610,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = ticket.destroy assert(delete, 'ticket destroy') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -700,7 +700,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = sla.destroy assert(delete, 'sla destroy') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -732,7 +732,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_equal(ticket.update_time_escal_date.gmtime.to_s, '2013-10-21 09:00:00 UTC', 'ticket.update_time_escal_date verify 1') assert_equal(ticket.close_time_escal_date.gmtime.to_s, '2013-10-21 10:00:00 UTC', 'ticket.close_time_escal_date verify 1') - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title holiday test', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -765,7 +765,7 @@ class TicketSlaTest < ActiveSupport::TestCase delete = Ticket.destroy_all assert(delete, 'ticket destroy_all') - ticket1 = Ticket.create( + ticket1 = Ticket.create!( title: 'some title äöüß3', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -936,7 +936,7 @@ class TicketSlaTest < ActiveSupport::TestCase created_at: '2013-06-04 15:00:00 UTC', updated_at: '2013-06-04 15:00:00 UTC', ) - ticket2.escalation_calculation + ticket2.escalation_calculation(true) ticket2 = Ticket.find(ticket2.id) assert_equal(ticket2.escalation_time.gmtime.to_s, '2013-06-04 16:00:00 UTC', 'ticket2.escalation_time verify 1') assert_equal(ticket2.first_response_escal_date.gmtime.to_s, '2013-06-04 16:00:00 UTC', 'ticket2.first_response_escal_date verify 1') @@ -953,7 +953,7 @@ class TicketSlaTest < ActiveSupport::TestCase end test 'ticket escalation suspend' do - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß3', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -1091,7 +1091,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert(delete, 'ticket destroy') # test Ticket created in state pending and closed without reopen or state change - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß3', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -1121,6 +1121,7 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update_attributes( close_time: '2013-06-04 12:00:00 UTC', ) + ticket.escalation_calculation(true) calendar = Calendar.create_or_update( name: 'EU 5', @@ -1188,7 +1189,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert(delete, 'ticket destroy') # test Ticket created in state pending, changed state to openen, back to pending and closed - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß3', group: Group.lookup(name: 'Users'), customer_id: 2, @@ -1316,7 +1317,7 @@ class TicketSlaTest < ActiveSupport::TestCase ### Test Ticket created in state pending, changed state to openen, back to pending and back to open then ### close ticket - ticket = Ticket.create( + ticket = Ticket.create!( title: 'some title äöüß3', group: Group.lookup(name: 'Users'), customer_id: 2, From 2e1ebf0c6f399fa9dd8881d5372ad99a4af29069 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 2 Sep 2016 13:07:20 +0200 Subject: [PATCH 2/3] Fixed new article creation on customer change. --- .../javascripts/app/controllers/ticket_customer.coffee | 6 ++++-- .../javascripts/app/controllers/ticket_zoom/sidebar.coffee | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/app/controllers/ticket_customer.coffee b/app/assets/javascripts/app/controllers/ticket_customer.coffee index d4a843455..e23ea954c 100644 --- a/app/assets/javascripts/app/controllers/ticket_customer.coffee +++ b/app/assets/javascripts/app/controllers/ticket_customer.coffee @@ -26,9 +26,11 @@ class App.TicketCustomer extends App.ControllerModal @close() # update ticket - @ticket.updateAttributes( + ticket = App.Ticket.find(@ticket_id) + ticket.article = undefined + ticket.updateAttributes( customer_id: @customer_id ) # load user if not already exists - App.User.full( @customer_id, callback ) + App.User.full(@customer_id, callback) diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee index 745f95859..cb5af2292 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee @@ -80,7 +80,7 @@ class App.TicketZoomSidebar extends App.ObserverController ) changeCustomer = (e, el) => new App.TicketCustomer( - ticket: ticket + ticket_id: ticket.id container: @el.closest('.content') ) @sidebarItems = [ From 068e445fb717334d778f5769c720a9444adecb6b Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 2 Sep 2016 13:07:57 +0200 Subject: [PATCH 3/3] Introduced fetchMayBe to avoid race conditions. --- .../app/controllers/ticket_zoom.coffee | 77 +++++++++++++------ .../agent_ticket_actions_level3_test.rb | 3 +- 2 files changed, 54 insertions(+), 26 deletions(-) diff --git a/app/assets/javascripts/app/controllers/ticket_zoom.coffee b/app/assets/javascripts/app/controllers/ticket_zoom.coffee index ec61ad2b2..75f462d62 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom.coffee @@ -14,7 +14,7 @@ class App.TicketZoom extends App.Controller super # check authentication - @authenticateCheckRedirect(true) + @authenticateCheckRedirect() @formMeta = undefined @ticket_id = params.ticket_id @@ -44,11 +44,8 @@ class App.TicketZoom extends App.Controller return if data.id.toString() isnt @ticket_id.toString() # check if we already have the request queued - #@log 'notice', 'TRY', @ticket_id, new Date(data.updated_at), new Date(@ticketUpdatedAtLastCall) - update = => - @fetch() - if !@ticketUpdatedAtLastCall || ( new Date(data.updated_at).toString() isnt new Date(@ticketUpdatedAtLastCall).toString() ) - @delay(update, 1200, "ticket-zoom-#{@ticket_id}") + #@log 'notice', 'TRY', @ticket_id, new Date(data.updated_at), new Date(@ticketUpdatedAtLastCallRequested) + @fetchMayBe(data, true) ) fetchStart: => @@ -64,6 +61,24 @@ class App.TicketZoom extends App.Controller @fetchIsRunningAgain = false @fetch() + fetchMayBe: (data, delay = true) => + if @ticketUpdatedAtLastCallRequested + if new Date(data.updated_at).getTime() is new Date(@ticketUpdatedAtLastCallRequested).getTime() + @log 'debug', 'no fetch, current ticket already there or requested' + return + if new Date(data.updated_at).getTime() < new Date(@ticketUpdatedAtLastCallRequested).getTime() + @log 'debug', 'no fetch, current ticket already newser or requested' + return + + @ticketUpdatedAtLastCallRequested = data.updated_at + if delay isnt true + @fetch() + return + + fetchDelayed = => + @fetch() + @delay(fetchDelayed, 1200, "ticket-zoom-#{@ticket_id}") + fetch: => return if !@Session.get() return if !@fetchStart() @@ -79,17 +94,18 @@ class App.TicketZoom extends App.Controller # check if ticket has changed newTicketRaw = data.assets.Ticket[@ticket_id] - if @ticketUpdatedAtLastCall + if @ticketUpdatedAtLastCallDone # return if ticket hasnt changed - return if @ticketUpdatedAtLastCall is newTicketRaw.updated_at + return if @ticketUpdatedAtLastCallDone is newTicketRaw.updated_at # notify if ticket changed not by my self if newTicketRaw.updated_by_id isnt @Session.get('id') App.TaskManager.notify(@task_key) # remember current data - @ticketUpdatedAtLastCall = newTicketRaw.updated_at + @ticketUpdatedAtLastCallRequested = newTicketRaw.updated_at + @ticketUpdatedAtLastCallDone = newTicketRaw.updated_at @load(data) App.SessionStorage.set(@key, data) @@ -111,7 +127,7 @@ class App.TicketZoom extends App.Controller @renderDone = false # if ticket is already loaded, ignore status "0" - network issues e. g. temp. not connection - if @ticketUpdatedAtLastCall && status is 0 + if @ticketUpdatedAtLastCallRequested && status is 0 console.log('network issues e. g. temp. not connection', status, statusText, detail) return @@ -698,27 +714,28 @@ class App.TicketZoom extends App.Controller ticket.article = article # submit changes + ui = @ ticket.save( - done: (r) => + done: (r) -> # reset article - should not be resubmited on next ticket update ticket.article = undefined # reset form after save - @reset() + ui.reset() if taskAction is 'closeNextInOverview' - if @overview_id + if ui.overview_id current_position = 0 - overview = App.Overview.find(@overview_id) + overview = App.Overview.find(ui.overview_id) list = App.OverviewListCollection.get(overview.link) for ticket in list.tickets current_position += 1 - if ticket.id is @ticket_id + if ticket.id is ui.ticket_id next = list.tickets[current_position] if next # close task - App.TaskManager.remove(@task_key) + App.TaskManager.remove(ui.task_key) # open task via task manager to get overview information App.TaskManager.execute( @@ -726,33 +743,43 @@ class App.TicketZoom extends App.Controller controller: 'TicketZoom' params: ticket_id: next.id - overview_id: @overview_id + overview_id: ui.overview_id show: true ) - @navigate "ticket/zoom/#{next.id}" + ui.navigate "ticket/zoom/#{next.id}" return # fallback, close task taskAction = 'closeTab' if taskAction is 'closeTab' - App.TaskManager.remove(@task_key) + App.TaskManager.remove(ui.task_key) nextTaskUrl = App.TaskManager.nextTaskUrl() if nextTaskUrl - @navigate nextTaskUrl + ui.navigate nextTaskUrl return - @navigate '#' + ui.navigate '#' return + ui.autosaveStart() + ui.muteTask() + ui.fetchMayBe({ id: @id, updated_at: @updated_at }, true) + + # enable form + ui.formEnable(e) + + App.Event.trigger('overview:fetch') + fail: (settings, details) => + App.Event.trigger 'notify', { + type: 'error' + msg: App.i18n.translateContent(details.error_human || details.error || 'Unable to update!') + timeout: 2000 + } @autosaveStart() @muteTask() @fetch() - - # enable form @formEnable(e) - - App.Event.trigger('overview:fetch') ) bookmark: (e) -> diff --git a/test/browser/agent_ticket_actions_level3_test.rb b/test/browser/agent_ticket_actions_level3_test.rb index 15c8a4c7f..ae6402a34 100644 --- a/test/browser/agent_ticket_actions_level3_test.rb +++ b/test/browser/agent_ticket_actions_level3_test.rb @@ -145,10 +145,11 @@ class AgentTicketActionsLevel3Test < TestCase ) # check content and edit screen in instance 1 - match( + watch_for( browser: browser2, css: '.active div.ticket-article', value: 'some level 3 body in instance 2', + timeout: 1, ) ticket_verify(