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

@@ -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)

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

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

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
 

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
 

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