From 3800db63866e45a728269bc545ee29bfdeb4e0c5 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 18 Jul 2018 17:29:44 +0800 Subject: [PATCH] Add nil handling to Associations::Assign sequencer unit (fixes #2110) --- .../common/model/associations/assign.rb | 47 +++++---- .../common/model/associations/assign_spec.rb | 95 +++++++++++++++++++ 2 files changed, 122 insertions(+), 20 deletions(-) create mode 100644 spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb diff --git a/lib/sequencer/unit/import/common/model/associations/assign.rb b/lib/sequencer/unit/import/common/model/associations/assign.rb index e548282bf..9e1fcc202 100644 --- a/lib/sequencer/unit/import/common/model/associations/assign.rb +++ b/lib/sequencer/unit/import/common/model/associations/assign.rb @@ -15,40 +15,47 @@ class Sequencer def process return if dry_run return if instance.blank? + return if associations.blank? && log_associations_error + register_changes instance.assign_attributes(associations) - - # execute associations check only if needed for performance reasons - return if action != :unchanged - return if !changed? - state.provide(:action, :changed) rescue => e handle_failure(e) end private - def changed? + # always returns true + def log_associations_error + return true if %i[failed deactivated].include?(action) + logger.error { 'associations cannot be nil' } if associations.nil? + true + end + + def register_changes + return if !(action == :unchanged && changes.any?) logger.debug { "Changed instance associations: #{changes.inspect}" } - changes.present? + state.provide(:action, :updated) end + # Why not just use instance.changes? + # Because it doesn't include associations + # stored on OTHER TABLES (has-one, has-many, HABTM) def changes - @changes ||= begin - return {} if associations.blank? - associations.collect do |association, value| - before = compareable(instance.send(association)) - after = compareable(value) - next if before == after - [association, [before, after]] - end.compact.to_h.with_indifferent_access - end + @changes ||= unfiltered_changes.reject(&method(:no_diff?)) end - def compareable(value) - return nil if value.blank? - return value.sort if value.respond_to?(:sort) - value + def unfiltered_changes + attrs = associations.keys + before = attrs.map(&instance.method(:send)) + after = associations.values + attrs.zip(before.zip(after)).to_h.with_indifferent_access + end + + def no_diff?(_, values) + values.map!(&:sort) if values.all? { |val| val.respond_to?(:sort) } + values.map!(&:presence) # [nil, []] -> [nil, nil] + values.uniq.length == 1 end end end diff --git a/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb b/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb new file mode 100644 index 000000000..02d8e13b8 --- /dev/null +++ b/spec/lib/sequencer/unit/import/common/model/associations/assign_spec.rb @@ -0,0 +1,95 @@ +require 'rails_helper' + +RSpec.describe Sequencer::Unit::Import::Common::Model::Associations::Assign, sequencer: :unit do + let(:parameters) do + { + instance: instance, + associations: associations, + action: action, + dry_run: false + } + end + + context 'when given an `associations` hash that changes the instance' do + let(:instance) { create(:user) } + let(:action) { :created } + let(:associations) do + alt_org = Organization.where('id <> ?', instance.organization_id.to_i).pluck(:id).sample + { organization_id: alt_org } + end + + it 'assigns (NOT updates) the associations' do + process(parameters) + expect(instance.changes).to include(:organization_id) + end + + it 'changes `:action => :unchanged` to `:action => :updated`' do + parameters[:action] = :unchanged + expect(process(parameters)).to include(action: :updated) + end + end + + context 'when given a `associations` hash that does NOT change the instance' do + let(:instance) { create(:user) } + let(:associations) { { organization_id: instance.organization_id } } + let(:action) { :unchanged } + + it 'keeps `:action => :unchanged`' do + expect(process(parameters)).to include(action: :unchanged) + end + end + + context 'when given an empty `associations` hash' do + let(:instance) { create(:user) } + let(:action) { :created } + let(:associations) { {} } + + it 'makes no changes' do + provided = process(parameters) + + expect(provided).to include(action: action) + expect(instance.changed?).to be(false) + end + end + + context 'when given nil for `associations`' do + let(:instance) { create(:user) } + let(:associations) { nil } + + context 'and `action == :failed`' do + let(:action) { :failed } + + it 'makes no changes' do + provided = process(parameters) + + expect(provided).to include(action: action) + expect(instance.changed?).to be(false) + end + end + + context 'and `action == :deactivated`' do + let(:action) { :deactivated } + + it 'makes no changes' do + provided = process(parameters) + + expect(provided).to include(action: action) + expect(instance.changed?).to be(false) + end + end + + context 'and any other value of `action`' do + let(:action) { :created } + + it 'makes no changes and logs an error' do + allow(Rails.logger).to receive(:error).with(any_args) + + provided = process(parameters) + + expect(provided).to include(action: action) + expect(instance.changed?).to be(false) + expect(Rails.logger).to have_received(:error) + end + end + end +end