From 9db417319dd6e710b1877a63bfa4b965aba59f10 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 19 Aug 2024 10:19:22 +1000 Subject: [PATCH] Improve variant related validation when creating product I disabled Metrics/AbcSize for ensure_standard_variant as I don't think that's hard to understand the code. And utimately it will be removed once product actually becomes optional. --- app/controllers/api/v0/products_controller.rb | 2 +- .../spree/admin/products_controller.rb | 2 +- app/models/spree/product.rb | 37 ++++--- .../api/v0/products_controller_spec.rb | 4 +- spec/models/spree/product_spec.rb | 101 +++++++++++++++++- spec/system/admin/products_spec.rb | 4 +- 6 files changed, 127 insertions(+), 23 deletions(-) diff --git a/app/controllers/api/v0/products_controller.rb b/app/controllers/api/v0/products_controller.rb index 6df0356cf5..801b6a343a 100644 --- a/app/controllers/api/v0/products_controller.rb +++ b/app/controllers/api/v0/products_controller.rb @@ -21,7 +21,7 @@ module Api authorize! :create, Spree::Product @product = Spree::Product.new(product_params) - if @product.save + if @product.save(context: :create_and_create_standard_variant) render json: @product, serializer: Api::Admin::ProductSerializer, status: :created else invalid_resource!(@product) diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb index 4580a751f0..d383bee4e8 100644 --- a/app/controllers/spree/admin/products_controller.rb +++ b/app/controllers/spree/admin/products_controller.rb @@ -39,7 +39,7 @@ module Spree def create delete_stock_params_and_set_after do @object.attributes = permitted_resource_params - if @object.save + if @object.save(context: :create_and_create_standard_variant) flash[:success] = flash_message_for(@object, :successfully_created) redirect_after_save else diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 9232b78120..e1228f56a4 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -48,7 +48,27 @@ module Spree validate :validate_image validates :price, numericality: { greater_than_or_equal_to: 0, if: ->{ new_record? } } - accepts_nested_attributes_for :variants, allow_destroy: true + # These validators are used to make sure the standard variant created via + # `ensure_standard_variant` will be valid. The are only used when creating a new product + with_options on: :create_and_create_standard_variant do + validates :supplier_id, presence: true + validates :primary_taxon_id, presence: true + validates :variant_unit, presence: true + validates :unit_value, presence: true, if: ->(product) { + %w(weight volume).include?(product.variant_unit) + } + validates :unit_value, numericality: { greater_than: 0 }, allow_blank: true + validates :unit_description, presence: true, if: ->(product) { + product.variant_unit.present? && product.unit_value.nil? + } + validates :variant_unit_scale, presence: true, if: ->(product) { + %w(weight volume).include?(product.variant_unit) + } + validates :variant_unit_name, presence: true, if: ->(product) { + product.variant_unit == 'items' + } + end + accepts_nested_attributes_for :image accepts_nested_attributes_for :product_properties, allow_destroy: true, @@ -60,9 +80,8 @@ module Spree :variant_unit_name, :variant_unit_scale, :tax_category_id, :shipping_category_id, :primary_taxon_id, :supplier_id - after_validation :validate_variant_attrs, on: :create after_create :ensure_standard_variant - after_update :touch_supplier, if: :saved_change_to_primary_taxon_id? + # after_update :touch_supplier, if: :saved_change_to_primary_taxon_id? around_destroy :destruction after_touch :touch_supplier @@ -235,6 +254,7 @@ module Spree end end + # rubocop:disable Metrics/AbcSize def ensure_standard_variant return unless variants.empty? @@ -253,6 +273,7 @@ module Spree variant.supplier_id = supplier_id variants << variant end + # rubocop:enable Metrics/AbcSize # Remove any unsupported HTML. def description @@ -266,15 +287,6 @@ module Spree private - def validate_variant_attrs - # Avoid running validation when we can't set variant attrs - # eg clone product. Will raise error if clonning a product with no variant - return if variants.first&.valid? - - errors.add(:primary_taxon_id, :blank) unless Spree::Taxon.find_by(id: primary_taxon_id) - errors.add(:supplier_id, :blank) unless Enterprise.find_by(id: supplier_id) - end - def touch_supplier return if variants.empty? @@ -286,7 +298,6 @@ module Spree # importing product. In this scenario the variant has not been updated with the supplier yet # hence the check. first_variant.supplier.touch if first_variant.supplier.present? - end def validate_image diff --git a/spec/controllers/api/v0/products_controller_spec.rb b/spec/controllers/api/v0/products_controller_spec.rb index 1a6030240c..fd3a0ade81 100644 --- a/spec/controllers/api/v0/products_controller_spec.rb +++ b/spec/controllers/api/v0/products_controller_spec.rb @@ -121,8 +121,8 @@ RSpec.describe Api::V0::ProductsController, type: :controller do expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") errors = json_response["errors"] expect(errors.keys).to match_array([ - "name", "price", - "primary_taxon_id", "supplier_id" + "name", "price", "primary_taxon_id", + "supplier_id", "variant_unit" ]) end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 91edc40ba0..e5919a4463 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -133,6 +133,9 @@ module Spree before do create(:stock_location) + end + + it "copies properties to the first standard variant" do product.primary_taxon_id = taxon.id product.name = "Product1" product.variant_unit = "weight" @@ -142,10 +145,8 @@ module Spree product.price = 4.27 product.shipping_category_id = shipping_category.id product.supplier_id = supplier.id - product.save! - end + product.save(context: :create_and_create_standard_variant) - it "copies properties to the first standard variant" do expect(product.variants.reload.length).to eq 1 standard_variant = product.variants.reload.first @@ -159,6 +160,100 @@ module Spree expect(standard_variant.primary_taxon).to eq taxon expect(standard_variant.supplier).to eq supplier end + + context "with variant attributes" do + it { + is_expected.to validate_presence_of(:variant_unit) + .on(:create_and_create_standard_variant) + } + it { + is_expected.to validate_presence_of(:supplier_id) + .on(:create_and_create_standard_variant) + } + it { + is_expected.to validate_presence_of(:primary_taxon_id) + .on(:create_and_create_standard_variant) + } + + describe "unit_value" do + subject { build(:simple_product, variant_unit: "items") } + + it { + is_expected.to validate_numericality_of(:unit_value).is_greater_than(0) + .on(:create_and_create_standard_variant) + } + it { + is_expected.not_to validate_presence_of(:unit_value) + .on(:create_and_create_standard_variant) + } + + ["weight", "volume"].each do |variant_unit| + context "when variant_unit is #{variant_unit}" do + subject { build(:simple_product, variant_unit:) } + + it { + is_expected.to validate_presence_of(:unit_value) + .on(:create_and_create_standard_variant) + } + end + end + + describe "unit_description" do + it { + is_expected.not_to validate_presence_of(:unit_description) + .on(:create_and_create_standard_variant) + } + + context "when variant_unit is et and unit_value is nil" do + subject { + build(:simple_product, variant_unit: "items", unit_value: nil, + unit_description: "box") + } + + it { + is_expected.to validate_presence_of(:unit_description) + .on(:create_and_create_standard_variant) + } + end + end + + describe "variant_unit_scale" do + it { + is_expected.not_to validate_presence_of(:variant_unit_scale) + .on(:create_and_create_standard_variant) + } + + ["weight", "volume"].each do |variant_unit| + context "when variant_unit is #{variant_unit}" do + subject { build(:simple_product, variant_unit:) } + + it { + is_expected.to validate_presence_of(:variant_unit_scale) + .on(:create_and_create_standard_variant) + } + end + end + end + + describe "variant_unit_name" do + subject { build(:simple_product, variant_unit: "volume") } + + it { + is_expected.not_to validate_presence_of(:variant_unit_name) + .on(:create_and_create_standard_variant) + } + + context "when variant_unit is items" do + subject { build(:simple_product, variant_unit: "items") } + + it { + is_expected.to validate_presence_of(:variant_unit_name) + .on(:create_and_create_standard_variant) + } + end + end + end + end end end diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 9545f05733..f2a1c322f3 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -53,7 +53,6 @@ RSpec.describe ' end it "display all attributes when submitting with error: Unit Value must be grater than 0" do - pending "rebase so we can add needed validation" select 'New supplier', from: 'product_supplier_id' fill_in 'product_name', with: "new product name" select "Weight (kg)", from: 'product_variant_unit_with_scale' @@ -154,7 +153,6 @@ RSpec.describe ' end it "creating product with empty unit value" do - pending "rebase" fill_in 'product_name', with: 'Hot Cakes' select 'New supplier', from: 'product_supplier_id' select "Weight (kg)", from: 'product_variant_unit_with_scale' @@ -169,7 +167,7 @@ RSpec.describe ' click_button 'Create' expect(current_path).to eq spree.admin_products_path - expect(page).to have_content "Unit value is not a number" + expect(page).to have_content "Unit value can't be blank" end it "creating product with empty product category fails" do