Fixed bug: Dry run imports for resources with has_many associations store the new association value.

- Reworked action and change detection since association changes are not possible to track via ActiveRecord API.

Tests:
- Added import base resource test to ensure associations won't get updated in dry runs.
- Added dry run tests to statistical import factory.
- Fixed broken tests due to switch from .save to .save!

lib/import/base_resource.rb

@@ -5,11 +5,10 @@ module Import
     attr_reader :resource, :remote_id, :errors
 
     def initialize(resource, *args)
+      @action = :unknown
       handle_args(resource, *args)
       initialize_associations_states
       import(resource, *args)
-      return if @resource.blank?
-      store_associations(:after, @resource)
     end
 
     def import_class
@@ -26,10 +25,9 @@ module Import
 
     def action
       return :failed if errors.present?
-      return :skipped if !@resource
+      return :skipped if @resource.blank?
       return :unchanged if !attributes_changed?
-      return :created if created?
+      @action
-      :updated
     end
 
     def attributes_changed?
@@ -48,19 +46,12 @@ module Import
       changes = {}
       tracked_associations.each do |association|
         next if @associations[:before][association] == @associations[:after][association]
+        next if @associations[:before][association].blank? && @associations[:after][association].blank?
         changes[association] = [@associations[:before][association], @associations[:after][association]]
       end
       changes
     end
 
-    def created?
-      return false if @resource.blank?
-      # dry run
-      return @resource.created_at.nil? if @resource.changed?
-      # live run
-      @resource.created_at == @resource.updated_at
-    end
-
     private
 
     def initialize_associations_states
@@ -91,7 +82,14 @@ module Import
       # the record is already created
       resource.delete(:created_by_id)
 
-      @resource.assign_attributes(resource)
+      # store the current state of the associations
+      # from the resource hash because if we assign
+      # them to the instance some (e.g. has_many)
+      # will get stored even in the dry run :/
+      store_associations(:after, resource)
+
+      associations = tracked_associations
+      @resource.assign_attributes(resource.except(*associations))
 
       # the return value here is kind of misleading
       # and should not be trusted to indicate if a
@@ -99,7 +97,10 @@ module Import
       # Use .action instead
       return true if !attributes_changed?
 
+      @action = :updated
+
       return true if @dry_run
+      @resource.assign_attributes(resource.slice(*associations))
       @resource.save!
       true
     end
@@ -119,18 +120,26 @@ module Import
       instance
     end
 
-    def store_associations(state, instance)
+    def store_associations(state, source)
-      @associations[state] = associations_state(instance)
+      @associations[state] = associations_state(source)
     end
 
-    def associations_state(instance)
+    def associations_state(source)
       state = {}
       tracked_associations.each do |association|
-        state[association] = instance.send(association)
+        state[association] = association_value(source, association)
       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|
@@ -150,6 +159,8 @@ module Import
 
     def create(resource, *_args)
       @resource = import_class.new(resource)
+      store_associations(:after, @resource)
+      @action = :created
       return if @dry_run
       @resource.save!
       external_sync_create(

spec/factories/signature.rb

@@ -0,0 +1,14 @@
+FactoryGirl.define do
+  sequence :test_signature_name do |n|
+    "Test signature #{n}"
+  end
+end
+
+FactoryGirl.define do
+  factory :signature do
+    name { generate(:test_signature_name) }
+    body '#{user.firstname} #{user.lastname}'.text2html
+    created_by_id 1
+    updated_by_id 1
+  end
+end

spec/lib/import/base_resource_spec.rb

@@ -113,6 +113,38 @@ RSpec.describe Import::BaseResource do
             ExternalSync.count
           }
       end
+
+      it "doesn't update associations of existing resources" do
+
+        # initial import
+        Import::Test::Group.new(attributes)
+        group = Group.last
+
+        old_signature = create(:signature)
+        old_users     = create_list(:user, 2)
+
+        group.update_attribute(:signature_id, old_signature.id)
+        group.update_attribute(:user_ids, old_users.collect(&:id))
+
+        # simulate next import run
+        travel 20.minutes
+
+        new_signature             = create(:signature)
+        new_users                 = create_list(:user, 2)
+        attributes[:signature_id] = new_signature.id
+        attributes[:user_ids]     = new_users.collect(&:id)
+
+        expect do
+          Import::Test::Group.new(attributes, dry_run: true)
+          group.reload
+        end
+          .to not_change {
+            group.signature_id
+          }
+          .and not_change {
+            group.user_ids
+          }
+      end
     end
   end
 

spec/lib/import/ldap/user_spec.rb

@@ -96,7 +96,7 @@ RSpec.describe Import::Ldap::User do
     end
 
     it 'logs failures to HTTP Log' do
-      expect_any_instance_of(User).to receive(:save).and_raise('SOME ERROR')
+      expect_any_instance_of(User).to receive(:save!).and_raise('SOME ERROR')
       described_class.new(user_entry, ldap_config, user_roles, signup_role_ids)
 
       expect(HttpLog.last.status).to eq('failed')
@@ -173,7 +173,7 @@ RSpec.describe Import::Ldap::User do
     end
 
     it 'logs failures to HTTP Log' do
-      expect_any_instance_of(User).to receive(:save).and_raise('SOME ERROR')
+      expect_any_instance_of(User).to receive(:save!).and_raise('SOME ERROR')
       described_class.new(user_entry, ldap_config, user_roles, signup_role_ids)
 
       expect(HttpLog.last.status).to eq('failed')

spec/lib/import/statistical_factory_spec.rb

@@ -38,6 +38,8 @@ RSpec.describe Import::StatisticalFactory do
 
   context 'statistics' do
 
+    context 'live run' do
+
       it 'tracks created instances' do
 
         Import::Test::GroupFactory.import([attributes])
@@ -52,7 +54,8 @@ RSpec.describe Import::StatisticalFactory do
         expect(Import::Test::GroupFactory.statistics).to eq(statistics)
       end
 
-    it 'tracks updated instances' do
+      context 'updated instances' do
+        it 'tracks by regular attributes' do
 
           Import::Test::GroupFactory.import([attributes])
 
@@ -73,6 +76,53 @@ RSpec.describe Import::StatisticalFactory do
           expect(Import::Test::GroupFactory.statistics).to eq(statistics)
         end
 
+        it 'tracks by has_many association attributes' do
+
+          Import::Test::GroupFactory.import([attributes])
+
+          # simulate next import run
+          travel 20.minutes
+          Import::Test::GroupFactory.reset_statistics
+
+          new_users             = create_list(:user, 2)
+          attributes[:user_ids] = new_users.collect(&:id)
+
+          Import::Test::GroupFactory.import([attributes])
+
+          statistics = {
+            created:   0,
+            updated:   1,
+            unchanged: 0,
+            skipped:   0,
+            failed:    0,
+          }
+          expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+        end
+
+        it 'tracks by belongs_to association attributes' do
+
+          Import::Test::GroupFactory.import([attributes])
+
+          # simulate next import run
+          travel 20.minutes
+          Import::Test::GroupFactory.reset_statistics
+
+          new_signature             = create(:signature)
+          attributes[:signature_id] = new_signature.id
+
+          Import::Test::GroupFactory.import([attributes])
+
+          statistics = {
+            created:   0,
+            updated:   1,
+            unchanged: 0,
+            skipped:   0,
+            failed:    0,
+          }
+          expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+        end
+      end
+
       it 'tracks unchanged instances' do
 
         Import::Test::GroupFactory.import([attributes])
@@ -93,4 +143,112 @@ RSpec.describe Import::StatisticalFactory do
         expect(Import::Test::GroupFactory.statistics).to eq(statistics)
       end
     end
+
+    context 'dry run' do
+
+      it 'tracks created instances' do
+
+        Import::Test::GroupFactory.import([attributes], dry_run: true)
+
+        statistics = {
+          created:   1,
+          updated:   0,
+          unchanged: 0,
+          skipped:   0,
+          failed:    0,
+        }
+        expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+      end
+
+      context 'updated instances' do
+
+        let(:local_group) { create(:group) }
+
+        before(:each) do
+          ExternalSync.create(
+            source:    'RSpec-Test',
+            source_id: local_group.id,
+            object:    'Group',
+            o_id:      local_group.id
+          )
+        end
+
+        it 'tracks by regular attributes' do
+
+          update_attributes        = local_group.attributes
+          update_attributes[:note] = 'TEST'
+
+          Import::Test::GroupFactory.import([update_attributes], dry_run: true)
+
+          statistics = {
+            created:   0,
+            updated:   1,
+            unchanged: 0,
+            skipped:   0,
+            failed:    0,
+          }
+          expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+        end
+
+        it 'tracks by has_many association attributes' do
+
+          update_attributes            = local_group.attributes
+          new_users                    = create_list(:user, 2)
+          update_attributes[:user_ids] = new_users.collect(&:id)
+
+          Import::Test::GroupFactory.import([update_attributes], dry_run: true)
+
+          statistics = {
+            created:   0,
+            updated:   1,
+            unchanged: 0,
+            skipped:   0,
+            failed:    0,
+          }
+          expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+        end
+
+        it 'tracks by belongs_to association attributes' do
+
+          update_attributes                = local_group.attributes
+          new_signature                    = create(:signature)
+          update_attributes[:signature_id] = new_signature.id
+
+          Import::Test::GroupFactory.import([update_attributes], dry_run: true)
+
+          statistics = {
+            created:   0,
+            updated:   1,
+            unchanged: 0,
+            skipped:   0,
+            failed:    0,
+          }
+          expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+        end
+      end
+
+      it 'tracks unchanged instances' do
+
+        local_group = create(:group)
+
+        ExternalSync.create(
+          source:    'RSpec-Test',
+          source_id: local_group.id,
+          object:    'Group',
+          o_id:      local_group.id
+        )
+
+        Import::Test::GroupFactory.import([local_group.attributes], dry_run: true)
+
+        statistics = {
+          created:   0,
+          updated:   0,
+          unchanged: 1,
+          skipped:   0,
+          failed:    0,
+        }
+        expect(Import::Test::GroupFactory.statistics).to eq(statistics)
+      end
+    end
+  end
 end