From 160b5211161d7616e57cbb8c6230ca46f579dbfb Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Thu, 7 Mar 2019 10:00:56 +0100 Subject: [PATCH] Fixes issue #2497 - Missing business hours validation for calendars creates infinitive loop (also for background jobs). --- Gemfile.lock | 4 +- .../_application_controller_form.coffee | 4 +- app/models/calendar.rb | 86 ++++++++++++++++++- app/models/ticket/escalation.rb | 27 +----- spec/models/calendar_spec.rb | 48 +++++++++++ test/unit/ticket_sla_test.rb | 6 +- 6 files changed, 142 insertions(+), 33 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index c829a8886..c011db8eb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -105,7 +105,7 @@ GEM execjs binding_of_caller (0.8.0) debug_inspector (>= 0.0.1) - biz (1.7.0) + biz (1.8.2) clavius (~> 1.0) tzinfo browser (2.5.2) @@ -121,7 +121,7 @@ GEM xpath (>= 2.0, < 4.0) childprocess (0.9.0) ffi (~> 1.0, >= 1.0.11) - clavius (1.0.3) + clavius (1.0.4) clearbit (0.2.8) nestful (~> 1.1.0) coderay (1.1.2) diff --git a/app/assets/javascripts/app/controllers/_application_controller_form.coffee b/app/assets/javascripts/app/controllers/_application_controller_form.coffee index 3f5429b7b..af92b3deb 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_form.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_form.coffee @@ -53,10 +53,10 @@ class App.ControllerForm extends App.Controller @form showAlert: (message) => - @form.find('.alert').first().removeClass('hide').html(App.i18n.translateInline(message)) + @form.find('.alert--danger').first().removeClass('hide').html(App.i18n.translateInline(message)) hideAlert: => - @form.find('.alert').addClass('hide').html() + @form.find('.alert--danger').addClass('hide').html() html: => @form.html() diff --git a/app/models/calendar.rb b/app/models/calendar.rb index 366fe4cdf..7b2ce7f7d 100644 --- a/app/models/calendar.rb +++ b/app/models/calendar.rb @@ -7,8 +7,8 @@ class Calendar < ApplicationModel store :business_hours store :public_holidays - before_create :validate_public_holidays, :fetch_ical - before_update :validate_public_holidays, :fetch_ical + before_create :validate_public_holidays, :validate_hours, :fetch_ical + before_update :validate_public_holidays, :validate_hours, :fetch_ical after_create :sync_default, :min_one_check after_update :sync_default, :min_one_check after_destroy :min_one_check @@ -265,6 +265,67 @@ returns [day, comment] end +=begin + + calendar = Calendar.find(123) + calendar.business_hours_to_hash + +returns + + { + mon: {'09:00' => '18:00'}, + tue: {'09:00' => '18:00'}, + wed: {'09:00' => '18:00'}, + thu: {'09:00' => '18:00'}, + sat: {'09:00' => '18:00'} + } + +=end + + def business_hours_to_hash + hours = {} + business_hours.each do |day, meta| + next if !meta[:active] + next if !meta[:timeframes] + + hours[day.to_sym] = {} + meta[:timeframes].each do |frame| + next if !frame[0] + next if !frame[1] + + hours[day.to_sym][frame[0]] = frame[1] + end + end + hours + end + +=begin + + calendar = Calendar.find(123) + calendar.public_holidays_to_array + +returns + + [ + Thu, 08 Mar 2020, + Sun, 25 Mar 2020, + Thu, 29 Mar 2020, + ] + +=end + + def public_holidays_to_array + holidays = [] + public_holidays&.each do |day, meta| + next if !meta + next if !meta['active'] + next if meta['removed'] + + holidays.push Date.parse(day) + end + holidays + end + private # if changed calendar is default, set all others default to false @@ -330,4 +391,25 @@ returns end true end + + # validate business hours + def validate_hours + + # get business hours + hours = business_hours_to_hash + raise Exceptions::UnprocessableEntity, 'No configured business hours found!' if hours.blank? + + # validate if business hours are usable by execute a try calculation + begin + Biz.configure do |config| + config.hours = hours + end + Biz.time(10, :minutes).after(Time.zone.parse('Tue, 05 Feb 2019 21:40:28 UTC +00:00')) + rescue => e + raise Exceptions::UnprocessableEntity, e.message + end + + true + end + end diff --git a/app/models/ticket/escalation.rb b/app/models/ticket/escalation.rb index 6b769c170..f172943f2 100644 --- a/app/models/ticket/escalation.rb +++ b/app/models/ticket/escalation.rb @@ -157,34 +157,13 @@ returns biz = Biz::Schedule.new do |config| # get business hours - hours = {} - calendar.business_hours.each do |day, meta| - next if !meta[:active] - next if !meta[:timeframes] + hours = calendar.business_hours_to_hash + raise "No configured hours found in calendar #{calendar.inspect}" if hours.blank? - hours[day.to_sym] = {} - meta[:timeframes].each do |frame| - next if !frame[0] - next if !frame[1] - - hours[day.to_sym][frame[0]] = frame[1] - end - end config.hours = hours - if hours.blank? - raise "No configured hours found in calendar #{calendar.inspect}" - end # get holidays - holidays = [] - calendar.public_holidays&.each do |day, meta| - next if !meta - next if !meta['active'] - next if meta['removed'] - - holidays.push Date.parse(day) - end - config.holidays = holidays + config.holidays = calendar.public_holidays_to_array config.time_zone = calendar.timezone end diff --git a/spec/models/calendar_spec.rb b/spec/models/calendar_spec.rb index a1ff8134b..e22016904 100644 --- a/spec/models/calendar_spec.rb +++ b/spec/models/calendar_spec.rb @@ -143,4 +143,52 @@ RSpec.describe Calendar, type: :model do end end end + + describe '#validate_hours' do + context 'when business_hours are invalid' do + + it 'fails for hours ending at 00:00' do + expect do + create(:calendar, + business_hours: { + mon: { + active: true, + timeframes: [['09:00', '00:00']] + }, + tue: { + active: true, + timeframes: [['09:00', '00:00']] + }, + wed: { + active: true, + timeframes: [['09:00', '00:00']] + }, + thu: { + active: true, + timeframes: [['09:00', '00:00']] + }, + fri: { + active: true, + timeframes: [['09:00', '00:00']] + }, + sat: { + active: false, + timeframes: [['09:00', '00:00']] + }, + sun: { + active: false, + timeframes: [['09:00', '00:00']] + } + }) + end.to raise_error(Exceptions::UnprocessableEntity, 'nonsensical hours provided') + end + + it 'fails for blank structure' do + expect do + create(:calendar, + business_hours: {}) + end.to raise_error(Exceptions::UnprocessableEntity, 'No configured business hours found!') + end + end + end end diff --git a/test/unit/ticket_sla_test.rb b/test/unit/ticket_sla_test.rb index 2e012960b..ff3fa76f6 100644 --- a/test/unit/ticket_sla_test.rb +++ b/test/unit/ticket_sla_test.rb @@ -1017,8 +1017,8 @@ class TicketSlaTest < ActiveSupport::TestCase ) ticket2.escalation_calculation(true) ticket2 = Ticket.find(ticket2.id) - assert_equal(ticket2.escalation_at.gmtime.to_s, '2013-06-04 16:00:00 UTC', 'ticket2.escalation_at verify 1') - assert_equal(ticket2.first_response_escalation_at.gmtime.to_s, '2013-06-04 16:00:00 UTC', 'ticket2.first_response_escalation_at verify 1') + assert_equal(ticket2.escalation_at.gmtime.to_s, '2013-06-05 07:00:00 UTC', 'ticket2.escalation_at verify 1') + assert_equal(ticket2.first_response_escalation_at.gmtime.to_s, '2013-06-05 07:00:00 UTC', 'ticket2.first_response_escalation_at verify 1') assert_nil(ticket2.first_response_in_min, 'ticket2.first_response_in_min verify 3') assert_nil(ticket2.first_response_diff_in_min, 'ticket2.first_response_diff_in_min verify 3') @@ -1259,7 +1259,7 @@ class TicketSlaTest < ActiveSupport::TestCase assert_nil(ticket.first_response_in_min, 'ticket.first_response_in_min verify 3') assert_nil(ticket.first_response_diff_in_min, 'ticket.first_response_diff_in_min verify 3') assert_equal(ticket.update_escalation_at.gmtime.to_s, '2013-06-04 15:00:00 UTC', 'ticket.update_escalation_at verify 1') - assert_equal(ticket.close_escalation_at.gmtime.to_s, '2013-06-04 16:00:00 UTC', 'ticket.close_escalation_at verify 1') + assert_equal(ticket.close_escalation_at.gmtime.to_s, '2013-06-05 07:00:00 UTC', 'ticket.close_escalation_at verify 1') assert_equal(ticket.close_in_min, 0, 'ticket.close_in_min verify 3') assert_equal(ticket.close_diff_in_min, 240, 'ticket.close_diff_in_min# verify 3')