Moved escalation calculation from after_create/after_update to before_create/before_update to have only one ticket save action.

app/models/observer/ticket/escalation_calculation.rb

@@ -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

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_create   :check_generate, :check_defaults, :check_title, :check_escalation_update
-  before_update   :check_defaults, :check_title, :reset_pending_time
+  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

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)
+    ticket_ids = Ticket.where(state_id: state_list_open).pluck(:id)
-    tickets.each(&:escalation_calculation)
+    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
 
+    # 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,16 +168,17 @@ 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 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
@@ -115,34 +186,22 @@ returns
       # 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
       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
 
-    # update time
+    # calculate update time escalation
-    # calculate escalation
+    if force == true || last_update_changed == true
-    if !last_contact_customer && !last_contact_agent
+      if !history_data
-      last_update = created_at
+        history_data = history_get
-    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
@@ -153,9 +212,13 @@ returns
       if sla.update_time && update_time_in_min
         self.update_time_diff_in_min = sla.update_time - update_time_in_min
       end
+    end
 
-    # close time
     # calculate close time escalation
+    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
@@ -163,24 +226,41 @@ returns
       # 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
       end
-
-    if escalation_disabled
-      self.escalation_time = nil
     end
 
-    return if !changed?
+    # 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
 
-    self.callback_loop = true
+    # remember already counted time to do on next update only the diff
-    self.updated_by_id = 1
+    preferences[:escalation_calculation] = {
-    save
+      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

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',

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)

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)

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,