From 7565825b612e25261f68bfb6baf77898b7bc6557 Mon Sep 17 00:00:00 2001 From: isidzukuri Date: Fri, 24 May 2024 11:56:16 +0300 Subject: [PATCH 1/5] Do not commit to db unchanged products is bulk save --- app/services/sets/product_set.rb | 8 ++++-- spec/services/sets/product_set_spec.rb | 38 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index ba3fb40240..d72e391beb 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -67,11 +67,12 @@ module Sets product.assign_attributes(product_related_attrs) + return true unless product.changed? + validate_presence_of_unit_value_in_product(product) - changed = product.changed? success = product.errors.empty? && product.save - count_result(success && changed) + count_result(success) success end @@ -104,7 +105,8 @@ module Sets def create_or_update_variant(product, variant_attributes) variant = find_model(product.variants, variant_attributes[:id]) if variant.present? - variant.update(variant_attributes.except(:id)) + variant.assign_attributes(variant_attributes.except(:id)) + variant.update(variant_attributes.except(:id)) if variant.changed? else variant = create_variant(product, variant_attributes) end diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index b60dd0f2a5..3d2181c493 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -134,6 +134,27 @@ RSpec.describe Sets::ProductSet do end end end + + context "when product attributes are not changed" do + let(:collection_hash) { + { 0 => { id: product.id, name: product.name } } + } + + it 'returns true' do + is_expected.to eq true + end + + it 'does not increase saved_count' do + subject + expect(product_set.saved_count).to eq 0 + end + + it 'does not update any product by calling save' do + expect_any_instance_of(Spree::Product).not_to receive(:save) + + subject + end + end end describe "updating a product's variants" do @@ -190,6 +211,23 @@ RSpec.describe Sets::ProductSet do include_examples "nothing saved" end + context "when attributes are not changed" do + let(:variant_attributes) { { sku: variant.sku } } + + before { variant } + + it 'updates product by calling save' do + expect_any_instance_of(Spree::Variant).not_to receive(:save) + + subject + end + + it 'does not increase saved_count' do + subject + expect(product_set.saved_count).to eq 0 + end + end + context "when products attributes are also updated" do let(:product_attributes) { { sku: "prod_sku" } From c71eb2d6b56c6168dfb7c962194ebd4e0640d874 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 27 May 2024 10:19:37 +1000 Subject: [PATCH 2/5] Remove duplicate assign --- app/services/sets/product_set.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index d72e391beb..04138eedbc 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -106,7 +106,7 @@ module Sets variant = find_model(product.variants, variant_attributes[:id]) if variant.present? variant.assign_attributes(variant_attributes.except(:id)) - variant.update(variant_attributes.except(:id)) if variant.changed? + variant.save if variant.changed? else variant = create_variant(product, variant_attributes) end From 35c2297d55700541bbd3fa7b33244bf8db7f6226 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 27 May 2024 10:21:49 +1000 Subject: [PATCH 3/5] Detect changes in price Price is actually an association with lots of custom methods to make it look like a field, and so changes were ignored. Now this issue is fixed, perhaps it should be moved to a concern.. Note, there are other delegated fields: product name and description may be assigned from the variant. But there's no hooks to save the prroduct, so I didn't include it when checking for changes. --- app/models/spree/variant.rb | 5 +++++ spec/models/spree/variant_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 294c1edfe0..78e5819107 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -199,6 +199,11 @@ module Spree price_in(currency).try(:amount) end + def changed? + # Changes to price are saved after_save + super || default_price.changed? + end + # can_supply? is implemented in VariantStock def in_stock?(quantity = 1) can_supply?(quantity) diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 832cb6d931..3cea5ba96f 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -58,6 +58,28 @@ RSpec.describe Spree::Variant do end end + describe "#changed?" do + subject(:variant) { create(:variant) } + + it { is_expected.not_to be_changed } + + it "is changed when basic fields are changed" do + subject.display_name = "blah" + expect(subject).to be_changed + end + + describe "default_price" do + it "price" do + subject.price = 100 + expect(subject).to be_changed + end + it "currency" do + subject.currency = "USD" + expect(subject).to be_changed + end + end + end + context "price parsing" do context "price=" do context "with decimal point" do From 90c71c6a1a8391e5b7a2e24027bc1940834370f2 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 28 May 2024 11:38:41 +1000 Subject: [PATCH 4/5] Remove unused method --- app/models/spree/price.rb | 4 ---- app/models/spree/variant.rb | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb index 668f53b6b9..0c2d12c285 100644 --- a/app/models/spree/price.rb +++ b/app/models/spree/price.rb @@ -24,10 +24,6 @@ module Spree amount end - def price_changed? - amount_changed? - end - def price=(price) self[:amount] = parse_price(price) end diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 78e5819107..9a48caf0b5 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -49,7 +49,7 @@ module Spree has_many :prices, class_name: 'Spree::Price', dependent: :destroy - delegate :display_price, :display_amount, :price, :price_changed?, :price=, + delegate :display_price, :display_amount, :price, :price=, :currency, :currency=, to: :find_or_build_default_price From f8f0a1bf583ba51d141f167611c8ef7c5b8b9571 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 29 May 2024 10:55:09 +1000 Subject: [PATCH 5/5] Update comment [skip ci] --- app/models/spree/variant.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 9a48caf0b5..18b7cb0d24 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -200,7 +200,7 @@ module Spree end def changed? - # Changes to price are saved after_save + # We consider the variant changed if associated price is changed (it is saved after_save) super || default_price.changed? end