diff --git a/app/models/import_job.rb b/app/models/import_job.rb index 16bae41c4..4623aba5c 100644 --- a/app/models/import_job.rb +++ b/app/models/import_job.rb @@ -5,6 +5,8 @@ class ImportJob < ApplicationModel store :payload store :result + scope :running, -> { where(finished_at: nil, dry_run: false).where.not(started_at: nil) } + # Starts the import backend class based on the name attribute. # Import backend class is initialized with the current instance. # Logs the start and end time (if ended successfully) and logs @@ -109,15 +111,7 @@ class ImportJob < ApplicationModel # # return [nil] def self.queue_registered - import_backends = Setting.get('import_backends') - return if import_backends.blank? - - import_backends.each do |backend| - - if !backend_valid?(backend) - Rails.logger.error "Invalid import backend '#{backend}'" - next - end + backends.each do |backend| # skip backends that are not "ready" yet next if !backend.constantize.queueable? diff --git a/app/models/scheduler.rb b/app/models/scheduler.rb index e3b1baa60..5aa54e7ca 100644 --- a/app/models/scheduler.rb +++ b/app/models/scheduler.rb @@ -121,9 +121,24 @@ class Scheduler < ApplicationModel raise 'This method should only get called when Scheduler.threads are initialized. Use `force: true` to start anyway.' end - log_start_finish(:info, 'Cleanup of left over locked delayed jobs') do + start_time = Time.zone.now - Delayed::Job.all.each do |job| + cleanup_delayed_jobs(start_time) + cleanup_import_jobs(start_time) + end + + # Checks for locked delayed jobs and tries to reschedule or destroy each of them. + # + # @param [ActiveSupport::TimeWithZone] after the time the cleanup was started + # + # @example + # Scheduler.cleanup_delayed_jobs(TimeZone.now) + # + # return [nil] + def self.cleanup_delayed_jobs(after) + log_start_finish(:info, "Cleanup of left over locked delayed jobs #{after}") do + + Delayed::Job.where('updated_at < ?', after).where.not(locked_at: nil).each do |job| log_start_finish(:info, "Checking left over delayed job #{job.inspect}") do cleanup_delayed(job) end @@ -131,13 +146,13 @@ class Scheduler < ApplicationModel end end - # Checks if the given job can be rescheduled or destroys it. Logs the action as warn. - # Works only for locked jobs. Jobs that are not locked are ignored and + # Checks if the given delayed job can be rescheduled or destroys it. Logs the action as warn. + # Works only for locked delayed jobs. Delayed jobs that are not locked are ignored and # should get destroyed directly. - # Checks the delayed job object for a method called .reschedule?. The memthod is called - # with the delayed job as a parameter. The result value is expected as a Boolean. If the - # result is true the lock gets removed and the delayed job gets rescheduled. If the return - # value is false it will get destroyed which is the default behaviour. + # Checks the Delayed::Job instance for a method called .reschedule?. The method is called + # with the Delayed::Job instance as a parameter. The result value is expected to be a Boolean. + # If the result is true the lock gets removed and the delayed job gets rescheduled. + # If the return value is false it will get destroyed which is the default behaviour. # # @param [Delayed::Job] job the job that should get checked for destroying/rescheduling. # @@ -181,6 +196,33 @@ class Scheduler < ApplicationModel logger.warn "#{action} locked delayed job: #{job_name}" end + # Checks for killed import jobs and marks them as finished and adds a note. + # + # @param [ActiveSupport::TimeWithZone] after the time the cleanup was started + # + # @example + # Scheduler.cleanup_import_jobs(TimeZone.now) + # + # return [nil] + def self.cleanup_import_jobs(after) + log_start_finish(:info, "Cleanup of left over import jobs #{after}") do + error = 'Interrupted by scheduler restart. Please restart manually or wait till next execution time.'.freeze + + # we need to exclude jobs that were updated at or since we started + # cleaning up (via the #reschedule? call) because they might + # were started `.delay`-ed and are flagged for restart + ImportJob.running.where('updated_at < ?', after).each do |job| + + job.update!( + finished_at: after, + result: { + error: error + } + ) + end + end + end + def self.start_job(job) # start job and return thread handle diff --git a/config/application.rb b/config/application.rb index 4fe500369..7b59d86f1 100644 --- a/config/application.rb +++ b/config/application.rb @@ -2,6 +2,9 @@ require File.expand_path('boot', __dir__) require 'rails/all' +# DO NOT REMOVE THIS LINE - see issue #2037 +Bundler.setup + # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. Bundler.require(*Rails.groups) diff --git a/lib/html_sanitizer.rb b/lib/html_sanitizer.rb index 8c6308a2b..f8b7a09ba 100644 --- a/lib/html_sanitizer.rb +++ b/lib/html_sanitizer.rb @@ -380,7 +380,28 @@ cleanup html string: else /[[:space:]]|\t|\n|\r/ end - string.strip.gsub(blank_regex, '').gsub(%r{/\*.*?\*/}, '').gsub(//, '').gsub(/\[.+?\]/, '').delete("\u0000") + cleaned_string = string.strip.gsub(blank_regex, '').gsub(%r{/\*.*?\*/}, '').gsub(//, '').gsub(/\[.+?\]/, '').delete("\u0000") + sanitize_attachment_disposition(cleaned_string) + end + + def self.sanitize_attachment_disposition(url) + uri = URI(url) + return url if uri.host != Setting.get('fqdn') + + params = CGI.parse(uri.query || '') + if params.key?('disposition') + params['disposition'] = 'attachment' + end + + uri.query = if params.blank? + nil + else + URI.encode_www_form(params) + end + + uri.to_s + rescue URI::InvalidURIError + url end def self.url_same?(url_new, url_old) diff --git a/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb b/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb index 4afd75537..9096c001a 100644 --- a/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb +++ b/lib/sequencer/unit/import/common/object_attribute/sanitized_name.rb @@ -12,6 +12,7 @@ class Sequencer # model_nos # model_name # model_name + # model_name without_double_underscores.gsub(/_id(s?)$/, '_no\1') end @@ -20,6 +21,7 @@ class Sequencer # model_ids # model_name # model_name + # model_name without_spaces_and_slashes.gsub(/_{2,}/, '_') end @@ -28,7 +30,17 @@ class Sequencer # model_ids # model___name # model_name - unsanitized_name.gsub(%r{[\s\/]}, '_').underscore + # model_name + transliterated.gsub(%r{[\s\/]}, '_').underscore + end + + def transliterated + # Model ID + # Model IDs + # Model / Name + # Model Name + # Model Name + ::ActiveSupport::Inflector.transliterate(unsanitized_name, '_'.freeze) end def unsanitized_name @@ -36,6 +48,9 @@ class Sequencer # Model IDs # Model / Name # Model Name + # rubocop:disable Style/AsciiComments + # Mödel Nâmé + # rubocop:enable Style/AsciiComments raise 'Missing implementation for unsanitized_name method' end end diff --git a/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb b/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb index ee130de05..5932ff3a8 100644 --- a/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb +++ b/lib/sequencer/unit/import/zendesk/ticket/comment/source_based.rb @@ -9,9 +9,12 @@ class Sequencer uses :resource def value - method_name = resource.via.channel.to_sym - return if !respond_to?(method_name, true) - send(method_name) + return if private_methods(false).exclude?(value_method_name) + send(value_method_name) + end + + def value_method_name + @value_method_name ||= resource.via.channel.to_sym end end end diff --git a/lib/tasks/bootstrap.rake b/lib/tasks/bootstrap.rake index 99771ad14..9605bce75 100644 --- a/lib/tasks/bootstrap.rake +++ b/lib/tasks/bootstrap.rake @@ -21,7 +21,7 @@ module BootstrapRakeHelper end def add_database_config - raise Errno::ENOENT, 'contrib/database.yml not found' unless File.exist?(DB_CONFIG[:source]) + raise Errno::ENOENT, 'config/database.yml not found' unless File.exist?(DB_CONFIG[:source]) if File.exist?(DB_CONFIG[:dest]) return if FileUtils.identical?(DB_CONFIG[:source], DB_CONFIG[:dest]) diff --git a/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb b/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb index ea4d43c40..d471fb53d 100644 --- a/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb +++ b/spec/lib/sequencer/unit/import/common/object_attribute/sanitized_name_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, context 'sanitizes' do - it 'whitespaces' do + it 'replaces whitespaces' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('model name') end @@ -19,7 +19,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, expect(provided[:sanitized_name]).to eq('model_name') end - it 'dashes' do + it 'replaces dashes' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('model-name') end @@ -27,7 +27,7 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, expect(provided[:sanitized_name]).to eq('model_name') end - it 'ids suffix' do + it 'replaces ids suffix' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('Model Ids') end @@ -35,12 +35,20 @@ RSpec.describe Sequencer::Unit::Import::Common::ObjectAttribute::SanitizedName, expect(provided[:sanitized_name]).to eq('model_nos') end - it 'id suffix' do + it 'replaces id suffix' do provided = process do |instance| expect(instance).to receive(:unsanitized_name).and_return('Model Id') end expect(provided[:sanitized_name]).to eq('model_no') end + + it 'replaces non-ASCII characters' do + provided = process do |instance| + expect(instance).to receive(:unsanitized_name).and_return('Ærøskøbing Ät Mödél') + end + + expect(provided[:sanitized_name]).to eq('a_eroskobing_at_model') + end end end diff --git a/spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb b/spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb new file mode 100644 index 000000000..bb09d5659 --- /dev/null +++ b/spec/lib/sequencer/unit/import/zendesk/ticket/comment/source_based_spec.rb @@ -0,0 +1,39 @@ +require 'rails_helper' + +RSpec.describe Sequencer::Unit::Import::Zendesk::Ticket::Comment::SourceBased, sequencer: :unit do + + before(:all) do + + described_class.class_eval do + + private + + def email + 'test@example.com' + end + end + end + + def parameters_for_channel(channel) + { + resource: double( + via: double( + channel: channel + ) + ) + } + end + + context 'for resource.via.channel attribute' do + + it 'provides from existing method' do + provided = process(parameters_for_channel('email')) + expect(provided[:source_based]).to eq('test@example.com') + end + + it 'provides nil for non existing method' do + provided = process(parameters_for_channel('system')) + expect(provided[:source_based]).to be_nil + end + end +end diff --git a/spec/models/import_job_spec.rb b/spec/models/import_job_spec.rb index 0f5f88f34..a99d029a4 100644 --- a/spec/models/import_job_spec.rb +++ b/spec/models/import_job_spec.rb @@ -120,7 +120,7 @@ RSpec.describe ImportJob do it 'logs errors for invalid registered backends' do allow(Setting).to receive(:get) expect(Setting).to receive(:get).with('import_backends').and_return(['InvalidBackend']) - expect(Rails.logger).to receive(:error) + expect(described_class.logger).to receive(:error) described_class.queue_registered end diff --git a/spec/models/scheduler_spec.rb b/spec/models/scheduler_spec.rb index aef9511cc..0f11afc74 100644 --- a/spec/models/scheduler_spec.rb +++ b/spec/models/scheduler_spec.rb @@ -99,56 +99,24 @@ RSpec.describe Scheduler do described_class.cleanup end - it 'keeps unlocked Delayed::Job-s' do - # meta :) - described_class.delay.cleanup + context 'Delayed::Job' do - expect do - simulate_threads_call - end.not_to change { - Delayed::Job.count - } - end - - context 'locked Delayed::Job' do - - it 'gets destroyed' do + it 'keeps unlocked' do # meta :) described_class.delay.cleanup - # lock job (simluates interrupted scheduler task) - locked_job = Delayed::Job.last - locked_job.update!(locked_at: Time.zone.now) - expect do simulate_threads_call - end.to change { + end.not_to change { Delayed::Job.count - }.by(-1) + } end - context 'respond to reschedule?' do + context 'locked' do - it 'gets rescheduled for positive responses' do - SpecSpace::DelayedJobBackend.reschedule = true - SpecSpace::DelayedJobBackend.delay.start - - # lock job (simluates interrupted scheduler task) - locked_job = Delayed::Job.last - locked_job.update!(locked_at: Time.zone.now) - - expect do - simulate_threads_call - end.to not_change { - Delayed::Job.count - }.and change { - Delayed::Job.last.locked_at - } - end - - it 'gets destroyed for negative responses' do - SpecSpace::DelayedJobBackend.reschedule = false - SpecSpace::DelayedJobBackend.delay.start + it 'gets destroyed' do + # meta :) + described_class.delay.cleanup # lock job (simluates interrupted scheduler task) locked_job = Delayed::Job.last @@ -160,6 +128,78 @@ RSpec.describe Scheduler do Delayed::Job.count }.by(-1) end + + context 'respond to reschedule?' do + + it 'gets rescheduled for positive responses' do + SpecSpace::DelayedJobBackend.reschedule = true + SpecSpace::DelayedJobBackend.delay.start + + # lock job (simluates interrupted scheduler task) + locked_job = Delayed::Job.last + locked_job.update!(locked_at: Time.zone.now) + + expect do + simulate_threads_call + end.to not_change { + Delayed::Job.count + }.and change { + Delayed::Job.last.locked_at + } + end + + it 'gets destroyed for negative responses' do + SpecSpace::DelayedJobBackend.reschedule = false + SpecSpace::DelayedJobBackend.delay.start + + # lock job (simluates interrupted scheduler task) + locked_job = Delayed::Job.last + locked_job.update!(locked_at: Time.zone.now) + + expect do + simulate_threads_call + end.to change { + Delayed::Job.count + }.by(-1) + end + end + end + end + + context 'ImportJob' do + + context 'affected job' do + + let(:job) { create(:import_job, started_at: 5.minutes.ago) } + + it 'finishes stuck jobs' do + + expect do + simulate_threads_call + end.to change { + job.reload.finished_at + } + end + + it 'adds an error message to the result' do + + expect do + simulate_threads_call + end.to change { + job.reload.result[:error] + } + end + end + + it "doesn't change jobs added after stop" do + + job = create(:import_job) + + expect do + simulate_threads_call + end.not_to change { + job.reload + } end end end diff --git a/test/browser/agent_ticket_overview_level0_test.rb b/test/browser/agent_ticket_overview_level0_test.rb index 0f2f9b038..848cd17ff 100644 --- a/test/browser/agent_ticket_overview_level0_test.rb +++ b/test/browser/agent_ticket_overview_level0_test.rb @@ -304,7 +304,7 @@ class AgentTicketOverviewLevel0Test < TestCase set( css: '.content.active .bulkAction [data-item="date"]', - value: '05/23/2088', + value: '05/23/2037', ) select( diff --git a/test/unit/html_sanitizer_test.rb b/test/unit/html_sanitizer_test.rb index e084a63cc..9c95a0dad 100644 --- a/test/unit/html_sanitizer_test.rb +++ b/test/unit/html_sanitizer_test.rb @@ -113,9 +113,26 @@ test 123 assert_equal(HtmlSanitizer.strict('
123
'), '
123
') assert_equal(HtmlSanitizer.strict('
123
'), '
123
') assert_equal(HtmlSanitizer.strict('
123
'), '
123
') + assert_equal(HtmlSanitizer.strict('
123
'), '
123
') assert_equal(HtmlSanitizer.strict('test'), 'test') assert_equal(HtmlSanitizer.strict('test'), 'test') assert_equal(HtmlSanitizer.strict('test'), 'test') - end + api_path = Rails.configuration.api_path + http_type = Setting.get('http_type') + fqdn = Setting.get('fqdn') + attachment_url = "#{http_type}://#{fqdn}#{api_path}/ticket_attachment/239/986/1653" + attachment_url_good = "#{attachment_url}?disposition=attachment" + attachment_url_evil = "#{attachment_url}?disposition=inline" + assert_equal(HtmlSanitizer.strict("Evil link"), "Evil link") + + assert_equal(HtmlSanitizer.strict("Good link"), "Good link") + assert_equal(HtmlSanitizer.strict("No disposition"), "No disposition") + + different_fqdn_url = attachment_url_evil.gsub(fqdn, 'some.other.tld') + assert_equal(HtmlSanitizer.strict("Different FQDN"), "Different FQDN") + + attachment_url_evil_other = "#{attachment_url}?disposition=some_other" + assert_equal(HtmlSanitizer.strict("Evil link"), "Evil link") + end end