From 2d70134d7258d3c39541cab7a6591e64165dea40 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 18 Sep 2019 14:20:25 +0200 Subject: [PATCH] Fixed bug: Empty ActiveRecord::AttributeMethods::Dirty#saved_changes in cache invalidation callback (ApplicationModel::HasCache) leads to wrong cache entries if record is a ticket. --- app/models/observer/ticket/escalation_update.rb | 12 +++++++++--- spec/models/ticket_spec.rb | 16 ++++++++-------- test/unit/ticket_sla_test.rb | 16 ++++++++++++++++ 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/app/models/observer/ticket/escalation_update.rb b/app/models/observer/ticket/escalation_update.rb index b8bc039c3..bb9c43a8e 100644 --- a/app/models/observer/ticket/escalation_update.rb +++ b/app/models/observer/ticket/escalation_update.rb @@ -14,9 +14,15 @@ class Observer::Ticket::EscalationUpdate < ActiveRecord::Observer # return if we run import mode return true if Setting.get('import_mode') - return true if !Ticket.exists?(record.id) + # we need to fetch the a new instance of the record + # from the DB instead of using `record.reload` + # because Ticket#reload clears the ActiveMode::Dirty state + # state of the record instance which leads to empty + # Ticket#saved_changes (etc.) results in other callbacks + # later in the chain + updated_ticket = Ticket.find_by(id: record.id) + return true if !updated_ticket - record.reload - record.escalation_calculation + updated_ticket.escalation_calculation end end diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index 0944d79d1..6447f1072 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -450,7 +450,7 @@ RSpec.describe Ticket, type: :model do it 'switches to "open"' do expect { article } - .to change { ticket.state.name }.from('new').to('open') + .to change { ticket.reload.state.name }.from('new').to('open') end end end @@ -462,7 +462,7 @@ RSpec.describe Ticket, type: :model do let(:article) { create(:ticket_article, ticket: ticket, sender_name: 'Agent') } it 'stays "closed"' do - expect { article }.not_to change { ticket.state.name } + expect { article }.not_to change { ticket.reload.state.name } end end end @@ -503,7 +503,7 @@ RSpec.describe Ticket, type: :model do before { sla } # create sla it 'is set based on SLA’s #first_response_time' do - expect(ticket.escalation_at.to_i) + expect(ticket.reload.escalation_at.to_i) .to eq(1.hour.from_now.to_i) end @@ -540,7 +540,7 @@ RSpec.describe Ticket, type: :model do it 'is updated based on the new SLA’s #first_response_time' do expect { ticket.save! } - .to change { ticket.escalation_at.to_i }.from(0).to(1.hour.from_now.to_i) + .to change { ticket.reload.escalation_at.to_i }.from(0).to(1.hour.from_now.to_i) end end @@ -553,7 +553,7 @@ RSpec.describe Ticket, type: :model do it 'is set to nil' do expect { ticket.save! } - .to change(ticket, :escalation_at).to(nil) + .to change { ticket.reload.escalation_at }.to(nil) end end end @@ -574,7 +574,7 @@ RSpec.describe Ticket, type: :model do before { sla } # create sla it 'is set based on SLA’s #first_response_time' do - expect(ticket.first_response_escalation_at.to_i) + expect(ticket.reload.first_response_escalation_at.to_i) .to eq(1.hour.from_now.to_i) end @@ -606,7 +606,7 @@ RSpec.describe Ticket, type: :model do before { sla } # create sla it 'is set based on SLA’s #update_time' do - expect(ticket.update_escalation_at.to_i) + expect(ticket.reload.update_escalation_at.to_i) .to eq(3.hours.from_now.to_i) end @@ -642,7 +642,7 @@ RSpec.describe Ticket, type: :model do before { sla } # create sla it 'is set based on SLA’s #solution_time' do - expect(ticket.close_escalation_at.to_i) + expect(ticket.reload.close_escalation_at.to_i) .to eq(4.hours.from_now.to_i) end diff --git a/test/unit/ticket_sla_test.rb b/test/unit/ticket_sla_test.rb index b372c91b0..b772f5154 100644 --- a/test/unit/ticket_sla_test.rb +++ b/test/unit/ticket_sla_test.rb @@ -165,6 +165,7 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( first_response_at: '2013-03-21 10:00:00 UTC', ) + ticket.reload assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 11:30:00 UTC', 'ticket.escalation_at verify 3') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 3') @@ -184,6 +185,7 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( first_response_at: '2013-03-21 14:00:00 UTC', ) + ticket.reload assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 11:30:00 UTC', 'ticket.escalation_at verify 4') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 4') @@ -203,6 +205,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( last_contact_agent_at: '2013-03-21 11:00:00 UTC', ) + ticket.reload + assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 12:30:00 UTC', 'ticket.escalation_at verify 5') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 5') @@ -222,6 +226,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( last_contact_agent_at: '2013-03-21 12:00:00 UTC', ) + ticket.reload + assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 12:30:00 UTC', 'ticket.escalation_at verify 6') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 6') @@ -241,6 +247,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( last_contact_customer_at: '2013-03-21 12:05:00 UTC', ) + ticket.reload + assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 12:30:00 UTC', 'ticket.escalation_at verify 6') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 6') @@ -260,6 +268,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( last_contact_agent_at: '2013-03-21 12:10:00 UTC', ) + ticket.reload + assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 12:30:00 UTC', 'ticket.escalation_at verify 6') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 6') @@ -279,6 +289,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( close_at: '2013-03-21 11:30:00 UTC', ) + ticket.reload + assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 14:10:00 UTC', 'ticket.escalation_at verify 7') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 7') @@ -298,6 +310,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( close_at: '2013-03-21 13:00:00 UTC', ) + ticket.reload + assert_equal(ticket.escalation_at.gmtime.to_s, '2013-03-21 14:10:00 UTC', 'ticket.escalation_at verify 8') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 8') @@ -317,6 +331,8 @@ class TicketSlaTest < ActiveSupport::TestCase ticket.update!( state: Ticket::State.lookup(name: 'closed') ) + ticket.reload + assert_nil(ticket.escalation_at, 'ticket.escalation_at verify 9') assert_equal(ticket.first_response_escalation_at.gmtime.to_s, '2013-03-21 10:30:00 UTC', 'ticket.first_response_escalation_at verify 9')