From a9e415983903b0c6358835f8523c6191457ab537 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Wed, 16 Aug 2023 11:15:59 +0100 Subject: [PATCH 1/5] validate unit_value > 0 on product model --- app/models/spree/product.rb | 2 +- lib/spree/core/product_duplicator.rb | 2 +- .../lib/spree/core/product_duplicator_spec.rb | 2 +- spec/models/spree/product_spec.rb | 2 +- spec/system/admin/products_spec.rb | 36 +++++++++++++++++++ 5 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 0378df7322..f3932dac96 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -53,7 +53,7 @@ module Spree validates :name, presence: true validates :variant_unit, presence: true - validates :unit_value, presence: + validates :unit_value, numericality: { greater_than: 0 }, allow_nil: true, presence: { if: ->(p) { %w(weight volume).include?(p.variant_unit) && new_record? } } validates :variant_unit_scale, presence: { if: ->(p) { %w(weight volume).include? p.variant_unit } } diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index 40d1103165..0bcbc3df93 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -24,7 +24,7 @@ module Spree new_product.created_at = nil new_product.deleted_at = nil new_product.updated_at = nil - new_product.unit_value = true + new_product.unit_value = nil new_product.product_properties = reset_properties new_product.image = duplicate_image(product.image) if product.image new_product.variants = duplicate_variants diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 2d2d47243b..8e3385da94 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -70,7 +70,7 @@ describe Spree::Core::ProductDuplicator do expect(new_product).to receive(:sku=).with("") expect(new_product).to receive(:product_properties=).with([new_property]) expect(new_product).to receive(:created_at=).with(nil) - expect(new_product).to receive(:unit_value=).with(true) + expect(new_product).to receive(:unit_value=).with(nil) expect(new_product).to receive(:updated_at=).with(nil) expect(new_product).to receive(:deleted_at=).with(nil) expect(new_product).to receive(:variants=).with([new_variant]) diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index d0f7a89eeb..e646c95ec1 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -168,7 +168,7 @@ module Spree end it "requires a unit value" do - expect(build(:simple_product, unit_value: nil)).not_to be_valid + expect(build(:simple_product, unit_value: nil)).to be_valid end it "requires a supplier" do diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 386d0a5239..63fa35e624 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -64,6 +64,42 @@ describe ' expect(page).to have_content "Name can't be blank" end + it "display all attributes when submitting with error: Unit Value must be grater than 0" do + login_to_admin_section + + click_link 'Products' + click_link 'New Product' + + select @supplier.name, from: 'product_supplier_id' + fill_in 'product_name', with: "new product name" + select "Weight (kg)", from: 'product_variant_unit_with_scale' + fill_in 'product_unit_value', with: "0.00 g" + assert_selector(:field, placeholder: "0g g") + fill_in 'product_display_as', with: "Big Box of Chocolates" + select taxon.name, from: "product_primary_taxon_id" + fill_in 'product_price', with: '19.99' + fill_in 'product_on_hand', with: 5 + check 'product_on_demand' + select 'Test Tax Category', from: 'product_tax_category_id' + page.find("div[id^='taTextElement']").native.send_keys('A description...') + + click_button 'Create' + + expect(page).to have_field 'product_name', with: "new product name" + expect(page).to have_field 'product_supplier_id', with: @supplier.id + expect(page).to have_field 'product_unit_value', with: "0 g" + expect(page).to have_field 'product_display_as', with: "Big Box of Chocolates" + expect(page).to have_field 'product_primary_taxon_id', with: taxon.id + expect(page).to have_field 'product_price', with: '19.99' + expect(page).to have_field 'product_on_hand', with: 5 + expect(page).to have_field 'product_on_demand', checked: true + expect(page).to have_field 'product_tax_category_id', with: tax_category.id + expect(page.find("div[id^='taTextElement']")).to have_content 'A description...' + expect(page.find("#product_variant_unit_field")).to have_content 'Weight (kg)' + + expect(page).to have_content "Unit value must be greater than 0" + end + it "preserves 'Items' 'Unit Size' selection when submitting with error" do login_to_admin_section From d4dbc0adb50326374c34a2f79124406310c83db9 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Sun, 20 Aug 2023 08:36:35 +0100 Subject: [PATCH 2/5] fix unit_value initialization on the product duplicator --- lib/spree/core/product_duplicator.rb | 2 +- spec/lib/spree/core/product_duplicator_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index 0bcbc3df93..a99419bc19 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -24,7 +24,7 @@ module Spree new_product.created_at = nil new_product.deleted_at = nil new_product.updated_at = nil - new_product.unit_value = nil + new_product.unit_value = %w(weight volume).include?(product.variant_unit) ? 1.0 : nil new_product.product_properties = reset_properties new_product.image = duplicate_image(product.image) if product.image new_product.variants = duplicate_variants diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 8e3385da94..0375b19597 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -8,7 +8,8 @@ describe Spree::Core::ProductDuplicator do name: "foo", product_properties: [property], variants: [variant], - image: image + image: image, + variant_unit: 'item' end let(:new_product) do From 3ab288f4355aa801004ec26b68bde6278fb85c0b Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Sun, 20 Aug 2023 09:33:28 +0100 Subject: [PATCH 3/5] fix product's unit value validation --- app/models/spree/product.rb | 20 ++++++++++++++++++-- spec/models/spree/product_spec.rb | 21 +++++++++++++++++++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index f3932dac96..76284f532c 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -53,8 +53,7 @@ module Spree validates :name, presence: true validates :variant_unit, presence: true - validates :unit_value, numericality: { greater_than: 0 }, allow_nil: true, presence: - { if: ->(p) { %w(weight volume).include?(p.variant_unit) && new_record? } } + validate :validate_unit_value validates :variant_unit_scale, presence: { if: ->(p) { %w(weight volume).include? p.variant_unit } } validates :variant_unit_name, @@ -207,6 +206,23 @@ module Spree }.inject(:or) end + def validate_unit_value + return unless %w(weight volume).include?(variant_unit) && new_record? + + return errors.add(:unit_value, I18n.t('errors.messages.blank')) if unit_value.blank? + + value = Float(unit_value, exception: false) + + return if value.is_a?(Numeric) && value > 0 + + error = if value.nil? + I18n.t('errors.messages.not_a_number') + else + I18n.t('errors.messages.greater_than', count: 0) + end + errors.add(:unit_value, error) + end + def property(property_name) return nil unless prop = properties.find_by(name: property_name) diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index e646c95ec1..b006d10fec 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -167,8 +167,25 @@ module Spree expect(build(:simple_product, primary_taxon: nil)).not_to be_valid end - it "requires a unit value" do - expect(build(:simple_product, unit_value: nil)).to be_valid + context "unit value" do + it "requires a unit value when variant unit is weight" do + expect(build(:simple_product, variant_unit: 'weight', variant_unit_name: 'name', + unit_value: nil)).not_to be_valid + expect(build(:simple_product, variant_unit: 'weight', variant_unit_name: 'name', + unit_value: 0)).not_to be_valid + end + + it "requires a unit value when variant unit is volume" do + expect(build(:simple_product, variant_unit: 'volume', variant_unit_name: 'name', + unit_value: nil)).not_to be_valid + expect(build(:simple_product, variant_unit: 'volume', variant_unit_name: 'name', + unit_value: 0)).not_to be_valid + end + + it "does not require a unit value when variant unit is items" do + expect(build(:simple_product, variant_unit: 'items', variant_unit_name: 'name', + unit_value: nil)).to be_valid + end end it "requires a supplier" do From a896d414c216168f94b0495b73d5247d201c724f Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Sun, 20 Aug 2023 09:46:46 +0100 Subject: [PATCH 4/5] optimize system test --- spec/system/admin/products_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 63fa35e624..fe1716596a 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -67,10 +67,9 @@ describe ' it "display all attributes when submitting with error: Unit Value must be grater than 0" do login_to_admin_section - click_link 'Products' - click_link 'New Product' + visit spree.new_admin_product_path - select @supplier.name, from: 'product_supplier_id' + select 'New supplier', from: 'product_supplier_id' fill_in 'product_name', with: "new product name" select "Weight (kg)", from: 'product_variant_unit_with_scale' fill_in 'product_unit_value', with: "0.00 g" From b082475c3537f310cbe7092ff9fde11ec1f6b139 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Mon, 21 Aug 2023 15:29:27 +0100 Subject: [PATCH 5/5] update product's unit_value validation --- app/models/spree/product.rb | 22 ++++------------------ spec/system/admin/products_spec.rb | 2 +- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 76284f532c..71e6ae5dea 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -53,7 +53,10 @@ module Spree validates :name, presence: true validates :variant_unit, presence: true - validate :validate_unit_value + validates :unit_value, numericality: { + greater_than: 0, + if: ->(p) { p.variant_unit.in?(%w(weight volume)) && new_record? } + } validates :variant_unit_scale, presence: { if: ->(p) { %w(weight volume).include? p.variant_unit } } validates :variant_unit_name, @@ -206,23 +209,6 @@ module Spree }.inject(:or) end - def validate_unit_value - return unless %w(weight volume).include?(variant_unit) && new_record? - - return errors.add(:unit_value, I18n.t('errors.messages.blank')) if unit_value.blank? - - value = Float(unit_value, exception: false) - - return if value.is_a?(Numeric) && value > 0 - - error = if value.nil? - I18n.t('errors.messages.not_a_number') - else - I18n.t('errors.messages.greater_than', count: 0) - end - errors.add(:unit_value, error) - end - def property(property_name) return nil unless prop = properties.find_by(name: property_name) diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index fe1716596a..a6c2183b6e 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -199,7 +199,7 @@ describe ' click_button 'Create' expect(current_path).to eq spree.admin_products_path - expect(page).to have_content "Unit value can't be blank" + expect(page).to have_content "Unit value is not a number" end end