From 4b9be6f1a872d1bbf487283259575f53bbd437a9 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Tue, 15 Jan 2019 15:24:11 +0000 Subject: [PATCH 1/2] Change product import's product_reset_strategy from depending on the inexistent variant.count_on_hand DB field and instead make individual calls to variant.count_on_hand= defined in VariantStock. Also, added spec to test return value of the reset method: it should return number of updated records. --- .../product_import/products_reset_strategy.rb | 28 +++++-- .../products_reset_strategy_spec.rb | 82 ++++++++----------- 2 files changed, 55 insertions(+), 55 deletions(-) diff --git a/app/models/product_import/products_reset_strategy.rb b/app/models/product_import/products_reset_strategy.rb index c1ce9af695..b05bfa6906 100644 --- a/app/models/product_import/products_reset_strategy.rb +++ b/app/models/product_import/products_reset_strategy.rb @@ -7,18 +7,16 @@ module ProductImport def reset(enterprise_ids) @enterprise_ids = enterprise_ids - if enterprise_ids.present? - relation.update_all(count_on_hand: 0) - else - 0 - end + return 0 if enterprise_ids.blank? + + reset_variants_count_on_hand(enterprise_variants_relation) end private attr_reader :excluded_items_ids, :enterprise_ids - def relation + def enterprise_variants_relation relation = Spree::Variant .joins(:product) .where( @@ -30,5 +28,23 @@ module ProductImport relation.where('spree_variants.id NOT IN (?)', excluded_items_ids) end + + def reset_variants_count_on_hand(variants) + updated_records_count = 0 + variants.each do |variant| + updated_records_count += 1 if reset_variant_count_on_hand(variant) + end + updated_records_count + end + + def reset_variant_count_on_hand(variant) + begin + variant.count_on_hand = 0 + return true if variant.count_on_hand.zero? + rescue StandardError => e + Rails.logger.info "Could not reset variant count on hand: #{e.message}" + end + false + end end end diff --git a/spec/models/product_import/products_reset_strategy_spec.rb b/spec/models/product_import/products_reset_strategy_spec.rb index 8adfb417ff..1b7738262b 100644 --- a/spec/models/product_import/products_reset_strategy_spec.rb +++ b/spec/models/product_import/products_reset_strategy_spec.rb @@ -1,41 +1,34 @@ require 'spec_helper' -xdescribe ProductImport::ProductsResetStrategy do +describe ProductImport::ProductsResetStrategy do let(:products_reset) { described_class.new(excluded_items_ids) } describe '#reset' do let(:supplier_ids) { enterprise.id } - let(:enterprise) { variant.product.supplier } - let(:variant) { create(:variant, count_on_hand: 2) } + let(:product) { create(:product) } + let(:enterprise) { product.supplier } + let(:variant) { product.variants.first } + + before { variant.count_on_hand = 2 } context 'when there are excluded_items_ids' do let(:excluded_items_ids) { [variant.id] } context 'and supplier_ids is []' do let(:supplier_ids) { [] } - let(:relation) do - instance_double(ActiveRecord::Relation, update_all: true) - end - before { allow(VariantOverride).to receive(:where) { relation } } - - it 'does not update any DB record' do + it 'does not reset the variant.count_on_hand' do products_reset.reset(supplier_ids) - expect(relation).not_to have_received(:update_all) + expect(variant.reload.count_on_hand).to eq(2) end end context 'and supplier_ids is nil' do let(:supplier_ids) { nil } - let(:relation) do - instance_double(ActiveRecord::Relation, update_all: true) - end - before { allow(VariantOverride).to receive(:where) { relation } } - - it 'does not update any DB record' do + it 'does not reset the variant.count_on_hand' do products_reset.reset(supplier_ids) - expect(relation).not_to have_received(:update_all) + expect(variant.reload.count_on_hand).to eq(2) end end @@ -48,9 +41,9 @@ xdescribe ProductImport::ProductsResetStrategy do it 'updates the count_on_hand of the non-excluded items' do non_excluded_variant = create( :variant, - count_on_hand: 3, product: variant.product ) + non_excluded_variant.count_on_hand = 3 products_reset.reset(supplier_ids) expect(non_excluded_variant.reload.count_on_hand).to eq(0) end @@ -62,36 +55,38 @@ xdescribe ProductImport::ProductsResetStrategy do context 'and supplier_ids is []' do let(:supplier_ids) { [] } - let(:relation) do - instance_double(ActiveRecord::Relation, update_all: true) - end - before { allow(VariantOverride).to receive(:where) { relation } } - - it 'does not update any DB record' do + it 'does not reset the variant.count_on_hand' do products_reset.reset(supplier_ids) - expect(relation).not_to have_received(:update_all) + expect(variant.reload.count_on_hand).to eq(2) end end context 'and supplier_ids is nil' do let(:supplier_ids) { nil } - let(:relation) do - instance_double(ActiveRecord::Relation, update_all: true) - end - before { allow(VariantOverride).to receive(:where) { relation } } - - it 'does not update any DB record' do + it 'does not reset the variant.count_on_hand' do products_reset.reset(supplier_ids) - expect(relation).not_to have_received(:update_all) + expect(variant.reload.count_on_hand).to eq(2) end end - context 'and supplier_ids is nil' do + context 'and supplier_ids is not nil' do it 'sets all count_on_hand to 0' do - products_reset.reset(supplier_ids) + updated_records_count = products_reset.reset(supplier_ids) expect(variant.reload.count_on_hand).to eq(0) + expect(updated_records_count).to eq(1) + end + + context 'and there is an unresetable variant' do + before do + variant.stock_items = [] # this makes variant.count_on_hand fail + end + + it 'returns correct number of resetted variants' do + updated_records_count = products_reset.reset(supplier_ids) + expect(updated_records_count).to eq(0) # the variant is not updated + end end end end @@ -101,29 +96,18 @@ xdescribe ProductImport::ProductsResetStrategy do context 'and supplier_ids is []' do let(:supplier_ids) { [] } - let(:relation) do - instance_double(ActiveRecord::Relation, update_all: true) - end - before { allow(VariantOverride).to receive(:where) { relation } } - - it 'does not update any DB record' do + it 'does not reset the variant.count_on_hand' do products_reset.reset(supplier_ids) - expect(relation).not_to have_received(:update_all) + expect(variant.reload.count_on_hand).to eq(2) end end context 'and supplier_ids is nil' do let(:supplier_ids) { nil } - let(:relation) do - instance_double(ActiveRecord::Relation, update_all: true) - end - - before { allow(VariantOverride).to receive(:where) { relation } } - - it 'does not update any DB record' do + it 'does not reset the variant.count_on_hand' do products_reset.reset(supplier_ids) - expect(relation).not_to have_received(:update_all) + expect(variant.reload.count_on_hand).to eq(2) end end From 80af7c770b2ac6111b0ffa5bd461cc177cc26284 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 1 Feb 2019 08:32:45 +0000 Subject: [PATCH 2/2] Remove rescue from products_reset_strategy in product import: if setting count_on_hand fails the import will raise a RuntimeError --- app/models/product_import/products_reset_strategy.rb | 9 ++------- .../product_import/products_reset_strategy_spec.rb | 5 ++--- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/app/models/product_import/products_reset_strategy.rb b/app/models/product_import/products_reset_strategy.rb index b05bfa6906..0dc005af82 100644 --- a/app/models/product_import/products_reset_strategy.rb +++ b/app/models/product_import/products_reset_strategy.rb @@ -38,13 +38,8 @@ module ProductImport end def reset_variant_count_on_hand(variant) - begin - variant.count_on_hand = 0 - return true if variant.count_on_hand.zero? - rescue StandardError => e - Rails.logger.info "Could not reset variant count on hand: #{e.message}" - end - false + variant.count_on_hand = 0 + variant.count_on_hand.zero? end end end diff --git a/spec/models/product_import/products_reset_strategy_spec.rb b/spec/models/product_import/products_reset_strategy_spec.rb index 1b7738262b..c3aa64a00b 100644 --- a/spec/models/product_import/products_reset_strategy_spec.rb +++ b/spec/models/product_import/products_reset_strategy_spec.rb @@ -80,12 +80,11 @@ describe ProductImport::ProductsResetStrategy do context 'and there is an unresetable variant' do before do - variant.stock_items = [] # this makes variant.count_on_hand fail + variant.stock_items = [] # this makes variant.count_on_hand raise an error end it 'returns correct number of resetted variants' do - updated_records_count = products_reset.reset(supplier_ids) - expect(updated_records_count).to eq(0) # the variant is not updated + expect { products_reset.reset(supplier_ids) }.to raise_error RuntimeError end end end