From ea926d1095201da4c318522270b6990c30670c2c Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 10 May 2017 16:43:44 +0200 Subject: [PATCH] Working on issue #981 - Error message about insufficient LDAP configuration is confusing and should be an information instead. --- .../app/controllers/_integration/ldap.coffee | 4 +- .../integration/ldap_last_import.jst.eco | 4 ++ lib/import/ldap.rb | 23 ++++++-- spec/lib/import/ldap_spec.rb | 55 ++++++++++++++++++- 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_integration/ldap.coffee b/app/assets/javascripts/app/controllers/_integration/ldap.coffee index d861005ca..c6ca0e2b1 100644 --- a/app/assets/javascripts/app/controllers/_integration/ldap.coffee +++ b/app/assets/javascripts/app/controllers/_integration/ldap.coffee @@ -498,9 +498,9 @@ class ConnectionWizard extends App.WizardModal finished: true processData: true success: (job, status, xhr) => - if job.result && job.result.error + if job.result && (job.result.error || job.result.info) @showSlide('js-error') - @showAlert('js-error', job.result.error) + @showAlert('js-error', (job.result.error || job.result.info)) return if job.result && job.result.sum diff --git a/app/assets/javascripts/app/views/integration/ldap_last_import.jst.eco b/app/assets/javascripts/app/views/integration/ldap_last_import.jst.eco index 9388cd23d..163f59277 100644 --- a/app/assets/javascripts/app/views/integration/ldap_last_import.jst.eco +++ b/app/assets/javascripts/app/views/integration/ldap_last_import.jst.eco @@ -3,6 +3,8 @@ <% if _.isEmpty(@job.started_at): %> <% if @job.result && @job.result.error: %> + <% else if @job.result && @job.result.info: %> + <% else: %>

<%- @T('Job is waiting to get started...') %>

<% end %> @@ -11,6 +13,8 @@

<%- @Ttimestamp(@job.started_at) %> - <%- @Ttimestamp(@job.finished_at) %>

<% if @job.result && @job.result.error: %> + <% else if @job.result && @job.result.info: %> + <% end %> <% else: %> <% if @job.result && @job.result.error: %> diff --git a/lib/import/ldap.rb b/lib/import/ldap.rb index ea643781d..e817762e3 100644 --- a/lib/import/ldap.rb +++ b/lib/import/ldap.rb @@ -28,10 +28,7 @@ module Import # # return [nil] def start - if !Setting.get('ldap_integration') && !@import_job.dry_run - raise "LDAP integration deactivated, check Setting 'ldap_integration'." - end - + return if !requirements_completed? start_import end @@ -48,5 +45,23 @@ module Import @import_job.result = Import::Ldap::UserFactory.statistics end + + def requirements_completed? + return true if @import_job.dry_run + + if !Setting.get('ldap_integration') + message = 'Sync cancelled. LDAP integration deactivated. Activate via the switch.' + elsif Setting.get('ldap_config').blank? && @import_job.payload.blank? + message = 'Sync cancelled. LDAP configration or ImportJob payload missing.' + end + + return true if !message + + @import_job.update_attribute(:result, { + info: message + }) + + false + end end end diff --git a/spec/lib/import/ldap_spec.rb b/spec/lib/import/ldap_spec.rb index ecb58fbdb..645c45a2c 100644 --- a/spec/lib/import/ldap_spec.rb +++ b/spec/lib/import/ldap_spec.rb @@ -4,7 +4,7 @@ require 'lib/import/import_job_backend_examples' RSpec.describe Import::Ldap do it_behaves_like 'ImportJob backend' - describe '#queueable?' do + describe '::queueable?' do it 'is queueable if LDAP integration is activated and configured' do allow(Setting).to receive(:get).with('ldap_integration').and_return(true) @@ -25,16 +25,65 @@ RSpec.describe Import::Ldap do end end - describe '.start' do - + describe '#start' do it 'starts LDAP import resource factories' do import_job = create(:import_job) instance = described_class.new(import_job) allow(Setting).to receive(:get).with('ldap_integration').and_return(true) + allow(Setting).to receive(:get).with('ldap_config').and_return(true) expect(Import::Ldap::UserFactory).to receive(:import) instance.start end + + context 'requirements' do + + it 'lets dry runs always start' do + import_job = create(:import_job, dry_run: true) + instance = described_class.new(import_job) + + expect(Import::Ldap::UserFactory).to receive(:import) + + instance.start + end + + it 'informs about deactivated ldap_integration' do + import_job = create(:import_job) + instance = described_class.new(import_job) + + allow(Setting).to receive(:get).with('ldap_integration').and_return(false) + + expect(Import::Ldap::UserFactory).not_to receive(:import) + + expect do + instance.start + import_job.reload + end.to change { + import_job.result + } + + expect(import_job.result.key?(:info)).to be true + end + + it 'informs about blank ldap_config' do + import_job = create(:import_job) + instance = described_class.new(import_job) + + allow(Setting).to receive(:get).with('ldap_integration').and_return(true) + allow(Setting).to receive(:get).with('ldap_config').and_return({}) + + expect(Import::Ldap::UserFactory).not_to receive(:import) + + expect do + instance.start + import_job.reload + end.to change { + import_job.result + } + + expect(import_job.result.key?(:info)).to be true + end + end end end