From 47a43163d800a911327f122bbe102c475de22ccb Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 6 Oct 2022 14:45:18 +1100 Subject: [PATCH 1/3] Avoid error on empty variant attributes In 2019 Pau observed an error where an empty attributes hash was passed to the bulk product update method. He added an automated test but wasn't able to reproduce the error. A recent change in logic made the spec fail but rspec-retry hid that fail because it would succeed on a second try (not sure why). I now changed the logic to ignore empty attributes properly and avoid an error trying to create a new variant when no attributes are given. * f940397781cc5387fa73f1b0a5e15d72d681175d Pau's spec * 6f228781d47552eabf2de7a1a5c020504a9d28ca Fixing error detection --- app/services/sets/product_set.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index 664d202eff..dc781b3005 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -110,6 +110,8 @@ module Sets end def create_variant(product, variant_attributes) + return if variant_attributes.blank? + on_hand = variant_attributes.delete(:on_hand) on_demand = variant_attributes.delete(:on_demand) From 95d68b98a982d06776e99817db444d4b3281c3e7 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 6 Oct 2022 16:08:33 +1100 Subject: [PATCH 2/3] Spec both cases of removing variants from order cycles The spec tested only the removal but not he keeping of variants. I want to change this in the next commit to simplify the class. --- spec/services/sets/product_set_spec.rb | 49 +++++++++++++++++--------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index 026d3b3ff5..38d54a7f1a 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -75,17 +75,9 @@ describe Sets::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 + context "when the product is in an order cycle" do + let(:producer) { create(:enterprise) } + let(:product) { create(:simple_product) } let(:distributor) { create(:distributor_enterprise) } let!(:order_cycle) { @@ -94,13 +86,36 @@ describe Sets::ProductSet do distributors: [distributor]) } - it 'updates the product and removes the product from order cycles' do - product_set.save + context 'and only the name changes' do + let(:collection_hash) do + { 0 => { id: product.id, name: "New season product" } } + end - expect(product.reload.attributes).to include( - 'supplier_id' => producer.id - ) - expect(order_cycle.distributed_variants).to_not include product.variants.first + it 'updates the product and keeps it in order cycles' do + expect { + product_set.save + product.reload + }.to change { product.name }.to("New season product"). + and change { order_cycle.distributed_variants.count }.by(0) + + expect(order_cycle.distributed_variants).to include product.variants.first + end + end + + context 'and a different supplier is passed' do + let(:collection_hash) do + { 0 => { id: product.id, supplier_id: producer.id } } + end + + it 'updates the product and removes the product from order cycles' do + expect { + product_set.save + product.reload + }.to change { product.supplier }.to(producer). + and change { order_cycle.distributed_variants.count }.by(-1) + + expect(order_cycle.distributed_variants).to_not include product.variants.first + end end end From 47ac118cf7c310fe877d1e7b72e0f996ad364f9e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 6 Oct 2022 16:10:40 +1100 Subject: [PATCH 3/3] Simplify ProductSet by using Rails changes feature I added a line to this class before and that made it too long. Now I removed a line again to keep code quality up. --- app/services/sets/product_set.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index dc781b3005..1928c835e6 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -47,10 +47,9 @@ module Sets 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 + ExchangeVariantDeleter.new.delete(product) if product.saved_change_to_supplier_id? update_product_variants(product, attributes) && update_product_master(product, attributes)