From 9c07cecff5f554eef048ae8f39c4a8c34d05f88e Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 5 May 2017 10:52:40 +0200 Subject: [PATCH] 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 | 47 ++-- spec/factories/signature.rb | 14 ++ spec/lib/import/base_resource_spec.rb | 32 +++ spec/lib/import/ldap/user_spec.rb | 4 +- spec/lib/import/statistical_factory_spec.rb | 238 ++++++++++++++++---- 5 files changed, 275 insertions(+), 60 deletions(-) create mode 100644 spec/factories/signature.rb diff --git a/lib/import/base_resource.rb b/lib/import/base_resource.rb index 0e3d12c46..d2b5dc0a2 100644 --- a/lib/import/base_resource.rb +++ b/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? - :updated + @action 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) - @associations[state] = associations_state(instance) + def store_associations(state, source) + @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( diff --git a/spec/factories/signature.rb b/spec/factories/signature.rb new file mode 100644 index 000000000..acbee4f72 --- /dev/null +++ b/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 diff --git a/spec/lib/import/base_resource_spec.rb b/spec/lib/import/base_resource_spec.rb index 8fedd9370..ef66bd0f8 100644 --- a/spec/lib/import/base_resource_spec.rb +++ b/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 diff --git a/spec/lib/import/ldap/user_spec.rb b/spec/lib/import/ldap/user_spec.rb index e7dc10e08..2aa073275 100644 --- a/spec/lib/import/ldap/user_spec.rb +++ b/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') diff --git a/spec/lib/import/statistical_factory_spec.rb b/spec/lib/import/statistical_factory_spec.rb index 06e3d11b2..4cc59483a 100644 --- a/spec/lib/import/statistical_factory_spec.rb +++ b/spec/lib/import/statistical_factory_spec.rb @@ -38,59 +38,217 @@ RSpec.describe Import::StatisticalFactory do context 'statistics' do - it 'tracks created instances' do + context 'live run' do - Import::Test::GroupFactory.import([attributes]) + it 'tracks created instances' do - statistics = { - created: 1, - updated: 0, - unchanged: 0, - skipped: 0, - failed: 0, - } - expect(Import::Test::GroupFactory.statistics).to eq(statistics) + Import::Test::GroupFactory.import([attributes]) + + statistics = { + created: 1, + updated: 0, + unchanged: 0, + skipped: 0, + failed: 0, + } + expect(Import::Test::GroupFactory.statistics).to eq(statistics) + end + + context 'updated instances' do + it 'tracks by regular attributes' do + + Import::Test::GroupFactory.import([attributes]) + + # simulate next import run + travel 20.minutes + Import::Test::GroupFactory.reset_statistics + + attributes[:note] = 'TEST' + 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 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]) + + # simulate next import run + travel 20.minutes + Import::Test::GroupFactory.reset_statistics + + Import::Test::GroupFactory.import([attributes]) + + statistics = { + created: 0, + updated: 0, + unchanged: 1, + skipped: 0, + failed: 0, + } + expect(Import::Test::GroupFactory.statistics).to eq(statistics) + end end - it 'tracks updated instances' do + context 'dry run' do - Import::Test::GroupFactory.import([attributes]) + it 'tracks created instances' do - # simulate next import run - travel 20.minutes - Import::Test::GroupFactory.reset_statistics + Import::Test::GroupFactory.import([attributes], dry_run: true) - attributes[:note] = 'TEST' - Import::Test::GroupFactory.import([attributes]) + statistics = { + created: 1, + updated: 0, + unchanged: 0, + skipped: 0, + failed: 0, + } + expect(Import::Test::GroupFactory.statistics).to eq(statistics) + end - statistics = { - created: 0, - updated: 1, - unchanged: 0, - skipped: 0, - failed: 0, - } - expect(Import::Test::GroupFactory.statistics).to eq(statistics) - end + context 'updated instances' do - it 'tracks unchanged instances' do + let(:local_group) { create(:group) } - Import::Test::GroupFactory.import([attributes]) + before(:each) do + ExternalSync.create( + source: 'RSpec-Test', + source_id: local_group.id, + object: 'Group', + o_id: local_group.id + ) + end - # simulate next import run - travel 20.minutes - Import::Test::GroupFactory.reset_statistics + it 'tracks by regular attributes' do - Import::Test::GroupFactory.import([attributes]) + update_attributes = local_group.attributes + update_attributes[:note] = 'TEST' - statistics = { - created: 0, - updated: 0, - unchanged: 1, - skipped: 0, - failed: 0, - } - expect(Import::Test::GroupFactory.statistics).to eq(statistics) + 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