Merge pull request #4376 from luisramos0/ghosts

Remove product from Order Cycles if product supplier changes

app/controllers/spree/admin/products_controller_decorator.rb

@@ -46,8 +46,11 @@ Spree::Admin::ProductsController.class_eval do
   end
 
   def update
+    original_supplier_id = @product.supplier_id
+
     delete_stock_params_and_set_after do
       super
+      ExchangeVariantDeleter.new.delete(@product) if original_supplier_id != @product.supplier_id
     end
   end
 

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
@@ -19,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_model(@collection, 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
 
@@ -34,28 +45,34 @@ 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 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)
     variant_related_attrs = [:id, :variants_attributes, :master_attributes]
     product_related_attrs = attributes.except(*variant_related_attrs)
-
     return true if product_related_attrs.blank?
 
     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)
 
-    product.save if errors.empty?
+    product.errors.empty? && product.save
   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?
 
@@ -79,12 +96,9 @@ 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_model(product.variants_including_master, variant_attributes[:id])
+    if variant.present?
+      variant.update_attributes(variant_attributes.except(:id))
     else
       create_variant(product, variant_attributes)
     end
@@ -115,15 +129,9 @@ class Spree::ProductSet < ModelSet
     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)
+  def find_model(collection, model_id)
+    collection.find do |model|
+      model.id.to_s == model_id.to_s && model.persisted?
     end
   end
 end

app/services/exchange_variant_deleter.rb

@@ -0,0 +1,7 @@
+class ExchangeVariantDeleter
+  def delete(product)
+    ExchangeVariant.
+      where(variant_id: product.variants.select(:id)).
+      delete_all
+  end
+end

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

spec/models/spree/product_set_spec.rb

@@ -75,54 +75,107 @@ describe Spree::ProductSet do
           end
         end
 
-        context 'when :master_attributes is passed' do
+        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.distributed_variants).to_not include product.variants.first
+          end
+        end
+
+        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 :variants_attributes are passed' do
+            let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
 
-          context 'and the variant does exist' do
-            let!(:variant) { create(:variant, product: product) }
+            before { collection_hash[0][:variants_attributes] = variants_attributes }
 
-            before { master_attributes[:id] = variant.id }
-
-            it 'updates the attributes of the master variant' do
+            it 'updates the attributes of the variant' do
               product_set.save
-              expect(variant.reload.sku).to eq('123')
+
+              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 '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 'when :master_attributes is passed' do
+            let(:master_attributes) { { sku: '123' } }
 
-              it 'creates it with the specified attributes' do
-                number_of_variants = Spree::Variant.all.size
+            before do
+              collection_hash[0][:master_attributes] = master_attributes
+            end
+
+            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(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