diff --git a/lib/import/base_resource.rb b/lib/import/base_resource.rb index d2b5dc0a2..7b70d305a 100644 --- a/lib/import/base_resource.rb +++ b/lib/import/base_resource.rb @@ -45,8 +45,13 @@ module Import def changed_associations changes = {} tracked_associations.each do |association| + # skip if no new value will get assigned (no change is performed) + next if !@associations[:after].key?(association) + # skip if both values are equal next if @associations[:before][association] == @associations[:after][association] + # skip if both values are blank next if @associations[:before][association].blank? && @associations[:after][association].blank? + # store changes changes[association] = [@associations[:before][association], @associations[:after][association]] end changes @@ -127,19 +132,22 @@ module Import def associations_state(source) state = {} tracked_associations.each do |association| - state[association] = association_value(source, association) + # we have to support instances and (resource) hashes + # here since in case of an update we only have the + # hash as a source but on create we have an instance + if source.is_a?(Hash) + # ignore if there is no key for the association + # of the Hash (update) + # otherwise wrong changes may get detected + next if !source.key?(association) + state[association] = source[association] + else + state[association] = source.send(association) + end end state end - # we have to support instances and (resource) hashes - # here since in case of an update we only have the - # hash as a source but on create we have an instance - def association_value(source, association) - return source[association] if source.is_a?(Hash) - source.send(association) - end - def tracked_associations # loop over all reflections import_class.reflect_on_all_associations.collect do |reflection| diff --git a/lib/import/ldap/user.rb b/lib/import/ldap/user.rb index bcfa52100..a70f85c50 100644 --- a/lib/import/ldap/user.rb +++ b/lib/import/ldap/user.rb @@ -100,19 +100,6 @@ module Import user_found = super end - # in case an User was found and we had no roles - # to set/update we have to note the currently locally - # assigned roles so that our action check won't - # falsly detect changes to the User (from nil to current) - if user_found && @update_role_ids.blank? - resource[:role_ids] = @resource.role_ids - - # we have to re-store/overwrite the stored - # associations since we've added the current - # state of the roles just yet - store_associations(:after, resource) - end - user_found rescue => e ldap_log( diff --git a/spec/lib/import/ldap/user_spec.rb b/spec/lib/import/ldap/user_spec.rb index 6e5e8d50a..823218edc 100644 --- a/spec/lib/import/ldap/user_spec.rb +++ b/spec/lib/import/ldap/user_spec.rb @@ -220,24 +220,6 @@ RSpec.describe Import::Ldap::User do } end - it 'keeps local roles if signup roles are configured' do - - ldap_config[:unassigned_users] = 'sigup_roles' - expect do - described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) - end.not_to change { - User.last.role_ids - } - end - - it "doesn't detect false changes if signup roles are configured" do - # make sure that the nothing has changed - User.find_by(login: uid).update_attribute(:email, 'example@example.com') - - instance = described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) - expect(instance.action).to eq(:unchanged) - end - it 'skips user if configured' do ldap_config[:unassigned_users] = 'skip_sync' @@ -250,6 +232,27 @@ RSpec.describe Import::Ldap::User do } expect(instance.action).to eq(:skipped) end + + context 'signup roles configuration' do + it 'keeps local roles' do + + ldap_config[:unassigned_users] = 'sigup_roles' + expect do + described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + end.not_to change { + User.last.role_ids + } + end + + it "doesn't detect false changes" do + # make sure that the nothing has changed + User.find_by(login: uid).update_attribute(:email, 'example@example.com') + + expect_any_instance_of(User).not_to receive(:save!) + instance = described_class.new(user_entry, ldap_config, user_roles, signup_role_ids) + expect(instance.action).to eq(:unchanged) + end + end end end @@ -265,7 +268,6 @@ RSpec.describe Import::Ldap::User do User.count } expect(instance.action).to eq(:skipped) - end it 'skips entries without attributes' do @@ -288,6 +290,5 @@ RSpec.describe Import::Ldap::User do expect(HttpLog.last.status).to eq('success') expect(HttpLog.last.url).to start_with('skipped') end - end end