From 22f87c8826fd4455af68c4839d638a3f2a3fdea8 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 12 May 2017 17:24:15 +0200 Subject: [PATCH] Fixed bug #1070: Existing resources get "save!"ed without changes. This triggers Rails model callbacks and executes unneeded actions. This was caused by an error in the detection of changed associations. Unchanged associations were interpreted as a reset of the association. Therefore all associations without a new value won't get tracked anymore. This is the right behavior since they won't change the current value of the resource without a value. --- lib/import/base_resource.rb | 26 +++++++++++++------- lib/import/ldap/user.rb | 13 ---------- spec/lib/import/ldap/user_spec.rb | 41 ++++++++++++++++--------------- 3 files changed, 38 insertions(+), 42 deletions(-) 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