From 057ecce1023a8a9e1b894b87b1e7622f7fdfddd1 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 5 May 2017 10:58:05 +0200 Subject: [PATCH] - Fixed bug: An error message is shown when entering the LDAP integration admin interface for the first time. Refactoring: - Added documentation and tests to ImportJob. - Added documentation and tests to Import::Ldap. - Introduced Import::Base. - Migrated existing Import::Test modules in tests to new class structure. --- app/models/import_job.rb | 8 +- lib/import/base.rb | 39 ++++ lib/import/ldap.rb | 26 ++- spec/factories/import_job.rb | 7 + spec/lib/import/base_resource_spec.rb | 2 +- spec/lib/import/base_spec.rb | 26 +++ .../lib/import/import_job_backend_examples.rb | 23 ++ spec/lib/import/ldap_spec.rb | 33 +++ spec/lib/import/model_resource_spec.rb | 2 +- spec/lib/import/statistical_factory_spec.rb | 2 +- spec/models/import_job_spec.rb | 200 ++++++++++++++++++ 11 files changed, 360 insertions(+), 8 deletions(-) create mode 100644 lib/import/base.rb create mode 100644 spec/factories/import_job.rb create mode 100644 spec/lib/import/base_spec.rb create mode 100644 spec/lib/import/import_job_backend_examples.rb create mode 100644 spec/lib/import/ldap_spec.rb create mode 100644 spec/models/import_job_spec.rb diff --git a/app/models/import_job.rb b/app/models/import_job.rb index 96edc56cc..c0b6a2d2e 100644 --- a/app/models/import_job.rb +++ b/app/models/import_job.rb @@ -18,7 +18,8 @@ class ImportJob < ApplicationModel def start self.started_at = Time.zone.now save - name.constantize.new(self) + instance = name.constantize.new(self) + instance.start rescue => e Rails.logger.error e @@ -81,7 +82,7 @@ class ImportJob < ApplicationModel end # Queues all configured import backends from Setting 'import_backends' as import jobs - # that are not yet queued. + # that are not yet queued. Backends which are not #queueable? are skipped. # # @example # ImportJob.queue_registered @@ -98,6 +99,9 @@ class ImportJob < ApplicationModel next end + # skip backends that are not "ready" yet + next if !backend.constantize.queueable? + # skip if no entry exists # skip if a not finished entry exists next if ImportJob.exists?(name: backend, finished_at: nil) diff --git a/lib/import/base.rb b/lib/import/base.rb new file mode 100644 index 000000000..3bfe7f41f --- /dev/null +++ b/lib/import/base.rb @@ -0,0 +1,39 @@ +# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ + +module Import + class Base + + # Checks if the able to get queued by the scheduler. + # + # @example + # Import::ExampleBackend.queueable? + # #=> true + # + # return [Boolean] + def self.queueable? + true + end + + # Initializes a new instance with a stored reference to the import job. + # + # @example + # instance = Import::ExampleBackend.new(import_job) + # + # return [Import::ExampleBackend] + def initialize(import_job) + @import_job = import_job + end + + # Starts the life or dry run import of the backend. + # + # @example + # instance = Import::ExampleBackend.new(import_job) + # + # @raise [RuntimeError] Raised if the implementation of this mandatory method is missing + # + # return [nil] + def start + raise "Missing implementation if the 'start' method." + end + end +end diff --git a/lib/import/ldap.rb b/lib/import/ldap.rb index 632aeba2a..4f0834cf9 100644 --- a/lib/import/ldap.rb +++ b/lib/import/ldap.rb @@ -1,12 +1,32 @@ +# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ + require 'ldap' require 'ldap/group' module Import - class Ldap + class Ldap < Import::Base - def initialize(import_job) - @import_job = import_job + # Checks if the integration is activated. Otherwise it won't get queued + # since it will display an error which is confusing and wrong. + # + # @example + # Import::LDAP.queueable? + # #=> true + # + # return [Boolean] + def self.queueable? + Setting.get('ldap_integration') + end + # Starts a live or dry run LDAP import. + # + # @example + # instance = Import::LDAP.new(import_job) + # + # @raise [RuntimeError] Raised if an import should start but the ldap integration is disabled + # + # return [nil] + def start if !Setting.get('ldap_integration') && !@import_job.dry_run raise "LDAP integration deactivated, check Setting 'ldap_integration'." end diff --git a/spec/factories/import_job.rb b/spec/factories/import_job.rb new file mode 100644 index 000000000..ce4ed05e3 --- /dev/null +++ b/spec/factories/import_job.rb @@ -0,0 +1,7 @@ +FactoryGirl.define do + factory :import_job do + name 'Import::Test' + payload {} + dry_run false + end +end diff --git a/spec/lib/import/base_resource_spec.rb b/spec/lib/import/base_resource_spec.rb index ef66bd0f8..f0fb77016 100644 --- a/spec/lib/import/base_resource_spec.rb +++ b/spec/lib/import/base_resource_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Import::BaseResource do before do module Import - module Test + class Test < Import::Base class Group < Import::BaseResource def import_class diff --git a/spec/lib/import/base_spec.rb b/spec/lib/import/base_spec.rb new file mode 100644 index 000000000..d1323ed7a --- /dev/null +++ b/spec/lib/import/base_spec.rb @@ -0,0 +1,26 @@ +require 'rails_helper' +require 'lib/import/import_job_backend_examples' + +RSpec.describe Import::Base do + it_behaves_like 'ImportJob backend' + + describe '#queueable?' do + + it 'returns true by default' do + expect(described_class.queueable?).to be true + end + end + + describe '.start' do + + it 'raises an error if called and not overwritten' do + + import_job = create(:import_job) + instance = described_class.new(import_job) + + expect do + instance.start + end.to raise_error(RuntimeError) + end + end +end diff --git a/spec/lib/import/import_job_backend_examples.rb b/spec/lib/import/import_job_backend_examples.rb new file mode 100644 index 000000000..a72c3e74e --- /dev/null +++ b/spec/lib/import/import_job_backend_examples.rb @@ -0,0 +1,23 @@ +RSpec.shared_examples 'ImportJob backend' do + + it 'responds to #queueable?' do + expect(described_class).to respond_to(:queueable?) + end + + it 'requires an import job instance as parameter' do + + expect do + described_class.new + end.to raise_error(ArgumentError) + + import_job = create(:import_job) + expect do + described_class.new(import_job) + end.not_to raise_error + end + + it 'responds to .start' do + import_job = create(:import_job) + expect(described_class.new(import_job)).to respond_to(:start) + end +end diff --git a/spec/lib/import/ldap_spec.rb b/spec/lib/import/ldap_spec.rb new file mode 100644 index 000000000..507da9b19 --- /dev/null +++ b/spec/lib/import/ldap_spec.rb @@ -0,0 +1,33 @@ +require 'rails_helper' +require 'lib/import/import_job_backend_examples' + +RSpec.describe Import::Ldap do + it_behaves_like 'ImportJob backend' + + describe '#queueable?' do + + it 'is queueable if ldap integration is activated' do + expect(Setting).to receive(:get).with('ldap_integration').and_return(true) + expect(described_class.queueable?).to be true + end + + it "isn't queueable if ldap integration is deactivated" do + expect(Setting).to receive(:get).with('ldap_integration').and_return(false) + expect(described_class.queueable?).to be false + end + end + + describe '.start' do + + it 'starts LDAP import resource factories' do + + import_job = create(:import_job) + instance = described_class.new(import_job) + + expect(Setting).to receive(:get).with('ldap_integration').and_return(true) + expect(Import::Ldap::UserFactory).to receive(:import) + + instance.start + end + end +end diff --git a/spec/lib/import/model_resource_spec.rb b/spec/lib/import/model_resource_spec.rb index 64f2edaef..37c6aa0d1 100644 --- a/spec/lib/import/model_resource_spec.rb +++ b/spec/lib/import/model_resource_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Import::ModelResource do before do module Import - module Test + class Test < Import::Base class Group < Import::ModelResource def source 'RSpec-Test' diff --git a/spec/lib/import/statistical_factory_spec.rb b/spec/lib/import/statistical_factory_spec.rb index 4cc59483a..02bc905a9 100644 --- a/spec/lib/import/statistical_factory_spec.rb +++ b/spec/lib/import/statistical_factory_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Import::StatisticalFactory do before do module Import - module Test + class Test < Import::Base module GroupFactory extend Import::StatisticalFactory diff --git a/spec/models/import_job_spec.rb b/spec/models/import_job_spec.rb new file mode 100644 index 000000000..638277451 --- /dev/null +++ b/spec/models/import_job_spec.rb @@ -0,0 +1,200 @@ +require 'rails_helper' + +RSpec.describe ImportJob do + + before do + module Import + class Test < Import::Base + def start + @import_job.result = { state: 'Done' } + end + end + end + end + + after do + Import.send(:remove_const, :Test) + end + + let(:test_backend_name) { 'Import::Test' } + let(:test_backend_class) { test_backend_name.constantize } + + describe '#dry_run' do + + it 'starts delayed dry run import job' do + expect do + described_class.dry_run( + name: test_backend_name, + payload: {} + ) + end.to change { + Delayed::Job.count + }.by(1) + end + + it 'starts dry run import job immediately' do + expect do + described_class.dry_run( + name: test_backend_name, + payload: {}, + delay: false + ) + + end.not_to change { + Delayed::Job.count + } + end + + it "doesn't start job if one exists" do + + create(:import_job, dry_run: true) + + expect do + described_class.dry_run( + name: test_backend_name, + payload: {}, + ) + + end.not_to change { + Delayed::Job.count + } + end + + end + + describe '#queue_registered' do + + it 'queues registered import jobs' do + allow(Setting).to receive(:get) + expect(Setting).to receive(:get).with('import_backends').and_return([test_backend_name]) + + expect do + described_class.queue_registered + end.to change { + described_class.exists?(name: test_backend_name) + } + end + + it "doesn't queue if backend isn't #queueable?" do + allow(Setting).to receive(:get) + expect(Setting).to receive(:get).with('import_backends').and_return([test_backend_name]) + expect(test_backend_class).to receive(:queueable?).and_return(false) + + expect do + described_class.queue_registered + end.not_to change { + described_class.exists?(name: test_backend_name) + } + end + + it "doesn't queue if unfinished job entries exist" do + + create(:import_job) + + allow(Setting).to receive(:get) + expect(Setting).to receive(:get).with('import_backends').and_return([test_backend_name]) + + expect do + described_class.queue_registered + end.not_to change { + described_class.exists?(name: test_backend_name) + } + end + + 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) + described_class.queue_registered + end + + end + + describe '#start' do + + it 'starts queued import jobs' do + create_list(:import_job, 2) + + expect do + described_class.start + end.to change { + described_class.where(started_at: nil).count + }.by(-2) + end + + it "doesn't start queued dry run import jobs" do + create_list(:import_job, 2) + create(:import_job, dry_run: true) + + expect do + described_class.start + end.to change { + described_class.where(started_at: nil).count + }.by(-2) + end + end + + describe '#start_registered' do + it 'queues and starts registered import backends' do + allow(Setting).to receive(:get) + expect(Setting).to receive(:get).with('import_backends').and_return([test_backend_name]) + + expect do + described_class.start_registered + end.to change { + described_class.where.not(started_at: nil, finished_at: nil).count + }.by(1) + end + end + + describe '#backend_valid?' do + + it 'detects existing backends' do + expect(described_class.backend_valid?(test_backend_name)).to be true + end + + it 'detects not existing backends' do + expect(described_class.backend_valid?('InvalidBackend')).to be false + end + end + + describe '.start' do + + it 'runs import backend and updates started_at and finished_at' do + + instance = create(:import_job) + + expect do + instance.start + end.to change { + instance.started_at + }.and change { + instance.finished_at + }.and change { + instance.result + } + end + + it 'handles exceptions as errors' do + + instance = create(:import_job) + + error_message = 'Some horrible error' + expect_any_instance_of(test_backend_class).to receive(:start).and_raise(error_message) + + expect do + instance.start + instance.reload + end.to change { + instance.started_at + }.and change { + instance.finished_at + }.and change { + instance.result + } + + expect(instance.result[:error]).to eq(error_message) + end + end + +end