diff --git a/app/controllers/spree/admin/products_controller_decorator.rb b/app/controllers/spree/admin/products_controller_decorator.rb index 3987de1577..32b6b67b2b 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_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 diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index 4b78be91a0..3e69c89bc7 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 @@ -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 diff --git a/app/services/exchange_variant_deleter.rb b/app/services/exchange_variant_deleter.rb new file mode 100644 index 0000000000..190b318711 --- /dev/null +++ b/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 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 diff --git a/spec/models/spree/product_set_spec.rb b/spec/models/spree/product_set_spec.rb index 3cf3a88975..ee5364bbd2 100644 --- a/spec/models/spree/product_set_spec.rb +++ b/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