Fixed bug zammad#1070: Existing resources get "save!"ed without chang…

…es. 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

@@ -45,8 +45,13 @@ def changed_attributes
     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 @@ def store_associations(state, source)
     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|

lib/import/ldap/user.rb

@@ -100,19 +100,6 @@ def updated?(resource, *_args)
           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(

spec/lib/import/ldap/user_spec.rb

@@ -220,24 +220,6 @@
         }
       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, '[email protected]')
-
-        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 @@
         }
         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, '[email protected]')
+
+          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 @@
         User.count
       }
       expect(instance.action).to eq(:skipped)
-
     end
 
     it 'skips entries without attributes' do
@@ -288,6 +290,5 @@
       expect(HttpLog.last.status).to eq('success')
       expect(HttpLog.last.url).to start_with('skipped')
     end
-
   end
 end