- 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

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

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

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

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

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

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

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

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

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'

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

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