From 49f98422fdf2b35d2483c98a02bc22afdb41c3cf Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Wed, 16 Oct 2019 16:27:05 +0100
Subject: [PATCH 01/11] Remove product from Order Cycles if supplier changes as
 with a new supplier the rules/permissions to add a product to an Order Cycle
 may be different

---
 .../spree/admin/products_controller_decorator.rb    | 10 ++++++++++
 .../spree/admin/products_controller_spec.rb         | 13 +++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb
index 3987de1577..10742f51cb 100644
--- a/app/controllers/spree/admin/products_controller_decorator.rb
+++ b/app/controllers/spree/admin/products_controller_decorator.rb
@@ -46,8 +46,11 @@ Spree::Admin::ProductsController.class_eval do
   end
 
   def update
+    original_supplier = @product.supplier_id
+
     delete_stock_params_and_set_after do
       super
+      remove_product_from_order_cycles if original_supplier != @product.supplier_id
     end
   end
 
@@ -190,6 +193,13 @@ Spree::Admin::ProductsController.class_eval do
     end
   end
 
+  def remove_product_from_order_cycles
+    variant_ids = @product.variants.map(&:id)
+    ExchangeVariant.
+      where(variant_id: variant_ids).
+      delete_all
+  end
+
   def set_product_master_variant_price_to_zero
     @product.price = 0 if @product.price.nil?
   end
diff --git a/spec/controllers/spree/admin/products_controller_spec.rb b/spec/controllers/spree/admin/products_controller_spec.rb
index 7a73954f42..e8ae658e9e 100644
--- a/spec/controllers/spree/admin/products_controller_spec.rb
+++ b/spec/controllers/spree/admin/products_controller_spec.rb
@@ -177,6 +177,19 @@ describe Spree::Admin::ProductsController, type: :controller do
       login_as_enterprise_user [producer]
     end
 
+    describe "change product supplier" do
+      let(:distributor) { create(:distributor_enterprise) }
+      let!(:order_cycle) { create(:simple_order_cycle, variants: [product.variants.first], coordinator: distributor, distributors: [distributor]) }
+
+      it "should remove product from existing Order Cycles" do
+        new_producer = create(:enterprise)
+        spree_put :update, id: product, product: { supplier_id: new_producer.id }
+
+        expect(product.reload.supplier.id).to eq new_producer.id
+        expect(order_cycle.reload.distributed_variants).to_not include product.variants.first
+      end
+    end
+
     describe "product stock setting with errors" do
       it "notifies bugsnag and still raise error" do
         # forces an error in the variant

From 8857404ddf762e2c13c367112987f3a57bd1dc9d Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Thu, 17 Oct 2019 18:13:02 +0100
Subject: [PATCH 02/11] Remove product variants from all Order Cycles if
 supplier is changed

---
 app/models/spree/product_set.rb       | 11 ++++++++++-
 spec/models/spree/product_set_spec.rb | 25 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index 4b78be91a0..bd825759a4 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -46,13 +46,15 @@ class Spree::ProductSet < ModelSet
 
     return true if product_related_attrs.blank?
 
+    original_supplier = product.supplier_id
     product.assign_attributes(product_related_attrs)
 
     product.variants.each do |variant|
       validate_presence_of_unit_value(product, variant)
     end
-
     product.save if errors.empty?
+
+    remove_product_from_order_cycles(product) if original_supplier != product.supplier_id
   end
 
   def validate_presence_of_unit_value(product, variant)
@@ -62,6 +64,13 @@ class Spree::ProductSet < ModelSet
     product.errors.add(:unit_value, "can't be blank")
   end
 
+  def remove_product_from_order_cycles(product)
+    variant_ids = product.variants.map(&:id)
+    ExchangeVariant.
+      where(variant_id: variant_ids).
+      delete_all
+  end
+
   def update_product_variants(product, attributes)
     return true unless attributes[:variants_attributes]
     update_variants_attributes(product, attributes[:variants_attributes])
diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb
index 3cf3a88975..af8d2ec64c 100644
--- a/spec/models/spree/product_set_spec.rb
+++ b/spec/models/spree/product_set_spec.rb
@@ -75,6 +75,31 @@ describe Spree::ProductSet do
           end
         end
 
+        context 'when a different supplier is passed' do
+          let!(:producer) { create(:enterprise) }
+          let!(:product) { create(:simple_product) }
+          let(:collection_hash) do
+            {
+              0 => {
+                id: product.id,
+                supplier_id: producer.id
+              }
+            }
+          end
+
+          let(:distributor) { create(:distributor_enterprise) }
+          let!(:order_cycle) { create(:simple_order_cycle, variants: [product.variants.first], coordinator: distributor, distributors: [distributor]) }
+
+          it 'updates the product and removes the product from order cycles' do
+            product_set.save
+
+            expect(product.reload.attributes).to include(
+              'supplier_id' => producer.id
+            )
+            expect(order_cycle.reload.distributed_variants).to_not include product.variants.first
+          end
+        end
+
         context 'when :master_attributes is passed' do
           let!(:product) { create(:simple_product) }
           let(:collection_hash) { { 0 => { id: product.id } } }

From b625ea0c619a13a359bcbebe3c1e7b61723c8b29 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Thu, 17 Oct 2019 18:21:49 +0100
Subject: [PATCH 03/11] Extract to class ExchangeVariantDeleter

---
 .../spree/admin/products_controller_decorator.rb         | 9 +--------
 app/models/spree/product_set.rb                          | 9 +--------
 app/services/exchange_variant_deleter.rb                 | 8 ++++++++
 3 files changed, 10 insertions(+), 16 deletions(-)
 create mode 100644 app/services/exchange_variant_deleter.rb

diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb
index 10742f51cb..beb7865d5d 100644
--- a/app/controllers/spree/admin/products_controller_decorator.rb
+++ b/app/controllers/spree/admin/products_controller_decorator.rb
@@ -50,7 +50,7 @@ Spree::Admin::ProductsController.class_eval do
 
     delete_stock_params_and_set_after do
       super
-      remove_product_from_order_cycles if original_supplier != @product.supplier_id
+      ExchangeVariantDeleter.new.delete(@product) if original_supplier != @product.supplier_id
     end
   end
 
@@ -193,13 +193,6 @@ Spree::Admin::ProductsController.class_eval do
     end
   end
 
-  def remove_product_from_order_cycles
-    variant_ids = @product.variants.map(&:id)
-    ExchangeVariant.
-      where(variant_id: variant_ids).
-      delete_all
-  end
-
   def set_product_master_variant_price_to_zero
     @product.price = 0 if @product.price.nil?
   end
diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index bd825759a4..e4c3eac1fb 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -54,7 +54,7 @@ class Spree::ProductSet < ModelSet
     end
     product.save if errors.empty?
 
-    remove_product_from_order_cycles(product) if original_supplier != product.supplier_id
+    ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id
   end
 
   def validate_presence_of_unit_value(product, variant)
@@ -64,13 +64,6 @@ class Spree::ProductSet < ModelSet
     product.errors.add(:unit_value, "can't be blank")
   end
 
-  def remove_product_from_order_cycles(product)
-    variant_ids = product.variants.map(&:id)
-    ExchangeVariant.
-      where(variant_id: variant_ids).
-      delete_all
-  end
-
   def update_product_variants(product, attributes)
     return true unless attributes[:variants_attributes]
     update_variants_attributes(product, attributes[:variants_attributes])
diff --git a/app/services/exchange_variant_deleter.rb b/app/services/exchange_variant_deleter.rb
new file mode 100644
index 0000000000..21e7f909d5
--- /dev/null
+++ b/app/services/exchange_variant_deleter.rb
@@ -0,0 +1,8 @@
+class ExchangeVariantDeleter
+  def delete(product)
+    variant_ids = product.variants.map(&:id)
+    ExchangeVariant.
+      where(variant_id: variant_ids).
+      delete_all
+  end
+end

From 9f3b4100c31a570b678cb4053a091e0f1f55b282 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 18 Oct 2019 10:26:03 +0100
Subject: [PATCH 04/11] Improve code by incorporating code review feedback

---
 app/controllers/spree/admin/products_controller_decorator.rb | 4 ++--
 app/services/exchange_variant_deleter.rb                     | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb
index beb7865d5d..32b6b67b2b 100644
--- a/app/controllers/spree/admin/products_controller_decorator.rb
+++ b/app/controllers/spree/admin/products_controller_decorator.rb
@@ -46,11 +46,11 @@ Spree::Admin::ProductsController.class_eval do
   end
 
   def update
-    original_supplier = @product.supplier_id
+    original_supplier_id = @product.supplier_id
 
     delete_stock_params_and_set_after do
       super
-      ExchangeVariantDeleter.new.delete(@product) if original_supplier != @product.supplier_id
+      ExchangeVariantDeleter.new.delete(@product) if original_supplier_id != @product.supplier_id
     end
   end
 
diff --git a/app/services/exchange_variant_deleter.rb b/app/services/exchange_variant_deleter.rb
index 21e7f909d5..190b318711 100644
--- a/app/services/exchange_variant_deleter.rb
+++ b/app/services/exchange_variant_deleter.rb
@@ -1,8 +1,7 @@
 class ExchangeVariantDeleter
   def delete(product)
-    variant_ids = product.variants.map(&:id)
     ExchangeVariant.
-      where(variant_id: variant_ids).
+      where(variant_id: product.variants.select(:id)).
       delete_all
   end
 end

From a50ae3f8ce9b36eeea247c6004139e717303f570 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Fri, 18 Oct 2019 10:37:23 +0100
Subject: [PATCH 05/11] Clarify the API of product_set class by making all
 other methods to private

---
 app/models/spree/product_set.rb | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index e4c3eac1fb..e1c38951c4 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -3,6 +3,20 @@ class Spree::ProductSet < ModelSet
     super(Spree::Product, [], attributes, proc { |attrs| attrs[:product_id].blank? })
   end
 
+  def save
+    @collection_hash.each_value.all? do |product_attributes|
+      update_attributes(product_attributes)
+    end
+  end
+
+  def collection_attributes=(attributes)
+    @collection = Spree::Product
+      .where(id: attributes.each_value.map { |product| product[:id] })
+    @collection_hash = attributes
+  end
+
+  private
+
   # A separate method of updating products was required due to an issue with
   # the way Rails' assign_attributes and updates_attributes behave when
   # delegated attributes of a nested object are updated via the parent object
@@ -116,16 +130,4 @@ class Spree::ProductSet < ModelSet
       report.add_tab(:variant_error, variant.errors.first) unless variant.valid?
     end
   end
-
-  def collection_attributes=(attributes)
-    @collection = Spree::Product
-      .where(id: attributes.each_value.map { |product| product[:id] })
-    @collection_hash = attributes
-  end
-
-  def save
-    @collection_hash.each_value.all? do |product_attributes|
-      update_attributes(product_attributes)
-    end
-  end
 end

From dd7d5803bab6fc827ea769f21ba04a6cd113b3eb Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 21 Oct 2019 13:01:39 +0100
Subject: [PATCH 06/11] Add new context to spec so that some basic setup can be
 shared with new specs that will be added, it's mostly indentation here

---
 spec/models/spree/product_set_spec.rb | 73 ++++++++++++++-------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb
index af8d2ec64c..c5c43f720b 100644
--- a/spec/models/spree/product_set_spec.rb
+++ b/spec/models/spree/product_set_spec.rb
@@ -100,54 +100,57 @@ describe Spree::ProductSet do
           end
         end
 
-        context 'when :master_attributes is passed' do
+        context 'when attributes of the variants are passed' do
           let!(:product) { create(:simple_product) }
           let(:collection_hash) { { 0 => { id: product.id } } }
-          let(:master_attributes) { { sku: '123' } }
 
-          before do
-            collection_hash[0][:master_attributes] = master_attributes
-          end
+          context 'when :master_attributes is passed' do
+            let(:master_attributes) { { sku: '123' } }
 
-          context 'and the variant does exist' do
-            let!(:variant) { create(:variant, product: product) }
-
-            before { master_attributes[:id] = variant.id }
-
-            it 'updates the attributes of the master variant' do
-              product_set.save
-              expect(variant.reload.sku).to eq('123')
+            before do
+              collection_hash[0][:master_attributes] = master_attributes
             end
-          end
 
-          context 'and the variant does not exist' do
-            context 'and attributes provided are valid' do
-              let(:master_attributes) do
-                attributes_for(:variant).merge(sku: '123')
-              end
+            context 'and the variant does exist' do
+              let!(:variant) { create(:variant, product: product) }
 
-              it 'creates it with the specified attributes' do
-                number_of_variants = Spree::Variant.all.size
+              before { master_attributes[:id] = variant.id }
+
+              it 'updates the attributes of the master variant' do
                 product_set.save
-                expect(Spree::Variant.last.sku).to eq('123')
-                expect(Spree::Variant.all.size).to eq number_of_variants + 1
+                expect(variant.reload.sku).to eq('123')
               end
             end
 
-            context 'and attributes provided are not valid' do
-              let(:master_attributes) do
-                # unit_value nil makes the variant invalid
-                # on_hand with a value would make on_hand be updated and fail with exception
-                attributes_for(:variant).merge(unit_value: nil, on_hand: 1, sku: '321')
+            context 'and the variant does not exist' do
+              context 'and attributes provided are valid' do
+                let(:master_attributes) do
+                  attributes_for(:variant).merge(sku: '123')
+                end
+
+                it 'creates it with the specified attributes' do
+                  number_of_variants = Spree::Variant.all.size
+                  product_set.save
+                  expect(Spree::Variant.last.sku).to eq('123')
+                  expect(Spree::Variant.all.size).to eq number_of_variants + 1
+                end
               end
 
-              it 'does not create variant and notifies bugsnag still raising the exception' do
-                expect(Bugsnag).to receive(:notify)
-                number_of_variants = Spree::Variant.all.size
-                expect { product_set.save }
-                  .to raise_error(StandardError)
-                expect(Spree::Variant.all.size).to eq number_of_variants
-                expect(Spree::Variant.last.sku).not_to eq('321')
+              context 'and attributes provided are not valid' do
+                let(:master_attributes) do
+                  # unit_value nil makes the variant invalid
+                  # on_hand with a value would make on_hand be updated and fail with exception
+                  attributes_for(:variant).merge(unit_value: nil, on_hand: 1, sku: '321')
+                end
+
+                it 'does not create variant and notifies bugsnag still raising the exception' do
+                  expect(Bugsnag).to receive(:notify)
+                  number_of_variants = Spree::Variant.all.size
+                  expect { product_set.save }
+                    .to raise_error(StandardError)
+                  expect(Spree::Variant.all.size).to eq number_of_variants
+                  expect(Spree::Variant.last.sku).not_to eq('321')
+                end
               end
             end
           end

From 44753d03201238b94f1660d76cc3ae74e46058b5 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 21 Oct 2019 13:27:21 +0100
Subject: [PATCH 07/11] Add spec coverage for case in product_set where
 variants_attributes are used

---
 spec/models/spree/product_set_spec.rb | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb
index c5c43f720b..23ad804ec0 100644
--- a/spec/models/spree/product_set_spec.rb
+++ b/spec/models/spree/product_set_spec.rb
@@ -104,6 +104,17 @@ describe Spree::ProductSet do
           let!(:product) { create(:simple_product) }
           let(:collection_hash) { { 0 => { id: product.id } } }
 
+          context 'when :variants_attributes are passed' do
+            let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
+
+            it 'updates the attributes of the variant' do
+              collection_hash[0][:variants_attributes] = variants_attributes
+              product_set.save
+
+              expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
+            end
+          end
+
           context 'when :master_attributes is passed' do
             let(:master_attributes) { { sku: '123' } }
 

From 783c3c9e909012436b4a128f6d10701a9553370f Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 21 Oct 2019 14:45:27 +0100
Subject: [PATCH 08/11] Add spec to product set to cover case where product and
 variants attributes are both provided and the product supplier is not, in
 that case, ExchangeVariantDeleter would not execute and
 update_product_only_attributes would return nil cancelling
 update_product_variants from being executed. Now,
 update_product_only_attributes always returns true if product.save suceeeds,
 no matter what ExchangeVariantDeleter returns

---
 app/models/spree/product_set.rb       |  4 ++--
 spec/models/spree/product_set_spec.rb | 18 ++++++++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index e1c38951c4..b36ddd053c 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -57,7 +57,6 @@ class Spree::ProductSet < ModelSet
   def update_product_only_attributes(product, attributes)
     variant_related_attrs = [:id, :variants_attributes, :master_attributes]
     product_related_attrs = attributes.except(*variant_related_attrs)
-
     return true if product_related_attrs.blank?
 
     original_supplier = product.supplier_id
@@ -66,9 +65,10 @@ class Spree::ProductSet < ModelSet
     product.variants.each do |variant|
       validate_presence_of_unit_value(product, variant)
     end
-    product.save if errors.empty?
+    return false unless product.errors.empty? && product.save
 
     ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id
+    true
   end
 
   def validate_presence_of_unit_value(product, variant)
diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb
index 23ad804ec0..ee5364bbd2 100644
--- a/spec/models/spree/product_set_spec.rb
+++ b/spec/models/spree/product_set_spec.rb
@@ -96,7 +96,7 @@ describe Spree::ProductSet do
             expect(product.reload.attributes).to include(
               'supplier_id' => producer.id
             )
-            expect(order_cycle.reload.distributed_variants).to_not include product.variants.first
+            expect(order_cycle.distributed_variants).to_not include product.variants.first
           end
         end
 
@@ -107,12 +107,26 @@ describe Spree::ProductSet do
           context 'when :variants_attributes are passed' do
             let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
 
+            before { collection_hash[0][:variants_attributes] = variants_attributes }
+
             it 'updates the attributes of the variant' do
-              collection_hash[0][:variants_attributes] = variants_attributes
               product_set.save
 
               expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
             end
+
+            context 'and when product attributes are also passed' do
+              it 'updates product and variant attributes' do
+                collection_hash[0][:permalink] = "test_permalink"
+
+                product_set.save
+
+                expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
+                expect(product.reload.attributes).to include(
+                  'permalink' => "test_permalink"
+                )
+              end
+            end
           end
 
           context 'when :master_attributes is passed' do

From f57c9d4a25c7471e53e924af972b82c0bd539adc Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 21 Oct 2019 14:58:25 +0100
Subject: [PATCH 09/11] Fix rubocop issue in
 product_set.update_product_only_attributes: method has too many lines

---
 app/models/spree/product_set.rb | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index b36ddd053c..c40cd5d6ff 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -62,16 +62,21 @@ class Spree::ProductSet < ModelSet
     original_supplier = product.supplier_id
     product.assign_attributes(product_related_attrs)
 
-    product.variants.each do |variant|
-      validate_presence_of_unit_value(product, variant)
-    end
+    validate_presence_of_unit_value_in_product(product)
+
     return false unless product.errors.empty? && product.save
 
     ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id
     true
   end
 
-  def validate_presence_of_unit_value(product, variant)
+  def validate_presence_of_unit_value_in_product(product)
+    product.variants.each do |variant|
+      validate_presence_of_unit_value_in_variant(product, variant)
+    end
+  end
+
+  def validate_presence_of_unit_value_in_variant(product, variant)
     return unless %w(weight volume).include?(product.variant_unit)
     return if variant.unit_value.present?
 

From 07fcc8f3611484acd67cc3586f8f1450d01dc107 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Sun, 27 Oct 2019 19:10:55 +0000
Subject: [PATCH 10/11] Refactor ExchangeVariantDeleter.new.delete out of
 update_product_only_attributes into correct place update_product

Also extracted find_product from update_attributes and find_variant out of create_or_update_variant to make code simpler
---
 app/models/spree/product_set.rb | 47 ++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index c40cd5d6ff..e0a487db7c 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -33,14 +33,11 @@ class Spree::ProductSet < ModelSet
   def update_attributes(attributes)
     split_taxon_ids!(attributes)
 
-    found_model = @collection.find do |model|
-      model.id.to_s == attributes[:id].to_s && model.persisted?
-    end
-
-    if found_model.nil?
+    product = find_product(attributes[:id])
+    if product.nil?
       @klass.new(attributes).save unless @reject_if.andand.call(attributes)
     else
-      update_product(found_model, attributes)
+      update_product(product, attributes)
     end
   end
 
@@ -48,10 +45,19 @@ class Spree::ProductSet < ModelSet
     attributes[:taxon_ids] = attributes[:taxon_ids].split(',') if attributes[:taxon_ids].present?
   end
 
-  def update_product(found_model, attributes)
-    update_product_only_attributes(found_model, attributes) &&
-      update_product_variants(found_model, attributes) &&
-      update_product_master(found_model, attributes)
+  def find_product(product_id)
+    @collection.find do |model|
+      model.id.to_s == product_id.to_s && model.persisted?
+    end
+  end
+
+  def update_product(product, attributes)
+    original_supplier = product.supplier_id
+    return false unless update_product_only_attributes(product, attributes)
+    ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id
+
+    update_product_variants(product, attributes) &&
+      update_product_master(product, attributes)
   end
 
   def update_product_only_attributes(product, attributes)
@@ -59,15 +65,11 @@ class Spree::ProductSet < ModelSet
     product_related_attrs = attributes.except(*variant_related_attrs)
     return true if product_related_attrs.blank?
 
-    original_supplier = product.supplier_id
     product.assign_attributes(product_related_attrs)
 
     validate_presence_of_unit_value_in_product(product)
 
-    return false unless product.errors.empty? && product.save
-
-    ExchangeVariantDeleter.new.delete(product) if original_supplier != product.supplier_id
-    true
+    product.errors.empty? && product.save
   end
 
   def validate_presence_of_unit_value_in_product(product)
@@ -100,17 +102,20 @@ class Spree::ProductSet < ModelSet
   end
 
   def create_or_update_variant(product, variant_attributes)
-    found_variant = product.variants_including_master.find do |variant|
-      variant.id.to_s == variant_attributes[:id].to_s && variant.persisted?
-    end
-
-    if found_variant.present?
-      found_variant.update_attributes(variant_attributes.except(:id))
+    variant = find_variant(product, variant_attributes[:id])
+    if variant.present?
+      variant.update_attributes(variant_attributes.except(:id))
     else
       create_variant(product, variant_attributes)
     end
   end
 
+  def find_variant(product, variant_id)
+    product.variants_including_master.find do |variant|
+      variant.id.to_s == variant_id.to_s && variant.persisted?
+    end
+  end
+
   def create_variant(product, variant_attributes)
     on_hand = variant_attributes.delete(:on_hand)
     on_demand = variant_attributes.delete(:on_demand)

From 0dc8ae15610202f9943c9222396d10c519ff481e Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Sun, 27 Oct 2019 20:09:42 +0000
Subject: [PATCH 11/11] Merging find_product and find_variant into one single
 method

This fixes rubocop issue, class has too many lines
---
 app/models/spree/product_set.rb | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb
index e0a487db7c..3e69c89bc7 100644
--- a/app/models/spree/product_set.rb
+++ b/app/models/spree/product_set.rb
@@ -33,7 +33,7 @@ class Spree::ProductSet < ModelSet
   def update_attributes(attributes)
     split_taxon_ids!(attributes)
 
-    product = find_product(attributes[:id])
+    product = find_model(@collection, attributes[:id])
     if product.nil?
       @klass.new(attributes).save unless @reject_if.andand.call(attributes)
     else
@@ -45,12 +45,6 @@ class Spree::ProductSet < ModelSet
     attributes[:taxon_ids] = attributes[:taxon_ids].split(',') if attributes[:taxon_ids].present?
   end
 
-  def find_product(product_id)
-    @collection.find do |model|
-      model.id.to_s == product_id.to_s && model.persisted?
-    end
-  end
-
   def update_product(product, attributes)
     original_supplier = product.supplier_id
     return false unless update_product_only_attributes(product, attributes)
@@ -102,7 +96,7 @@ class Spree::ProductSet < ModelSet
   end
 
   def create_or_update_variant(product, variant_attributes)
-    variant = find_variant(product, variant_attributes[:id])
+    variant = find_model(product.variants_including_master, variant_attributes[:id])
     if variant.present?
       variant.update_attributes(variant_attributes.except(:id))
     else
@@ -110,12 +104,6 @@ class Spree::ProductSet < ModelSet
     end
   end
 
-  def find_variant(product, variant_id)
-    product.variants_including_master.find do |variant|
-      variant.id.to_s == variant_id.to_s && variant.persisted?
-    end
-  end
-
   def create_variant(product, variant_attributes)
     on_hand = variant_attributes.delete(:on_hand)
     on_demand = variant_attributes.delete(:on_demand)
@@ -140,4 +128,10 @@ class Spree::ProductSet < ModelSet
       report.add_tab(:variant_error, variant.errors.first) unless variant.valid?
     end
   end
+
+  def find_model(collection, model_id)
+    collection.find do |model|
+      model.id.to_s == model_id.to_s && model.persisted?
+    end
+  end
 end