Add nil handling to Associations::Assign sequencer unit (fixes #2110)

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

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