Merge pull request #6815 from tanmay3011/refactor-validations

Refactor validations

api/spec/controllers/spree/api/base_controller_spec.rb

@@ -75,7 +75,7 @@ def index
     resource.valid? # get some errors
     expect(subject).to receive(:index).and_raise(ActiveRecord::RecordInvalid.new(resource))
     get :index, token: 'exception-message'
-    expect(json_response).to eql('exception' => "Validation failed: Name can't be blank, Price can't be blank, Shipping category can't be blank")
+    expect(json_response).to eql('exception' => "Validation failed: Name can't be blank, Shipping category can't be blank, Price can't be blank")
   end
 
   it "maps semantic keys to nested_attributes keys" do

core/app/models/spree/address.rb

@@ -7,16 +7,17 @@ class Address < Spree::Base
 
     has_many :shipments, inverse_of: :address
 
-    validates :firstname, :lastname, :address1, :city, :country, presence: true
-    validates :zipcode, presence: true, if: :require_zipcode?
-    validates :phone, presence: true, if: :require_phone?
+    with_options presence: true do
+      validates :firstname, :lastname, :address1, :city, :country
+      validates :zipcode, if: :require_zipcode?
+      validates :phone, if: :require_phone?
+    end
 
     validate :state_validate, :postal_code_validate
 
     alias_attribute :first_name, :firstname
     alias_attribute :last_name, :lastname
 
-
     self.whitelisted_ransackable_attributes = %w[firstname lastname company]
 
     def self.build_default

core/app/models/spree/adjustment.rb

@@ -28,9 +28,7 @@ class Adjustment < Spree::Base
     end
     belongs_to :order, class_name: 'Spree::Order', inverse_of: :all_adjustments
 
-    validates :adjustable, presence: true
-    validates :order, presence: true
-    validates :label, presence: true
+    validates :adjustable, :order, :label, presence: true
     validates :amount, numericality: true
 
     state_machine :state, initial: :open do

core/app/models/spree/customer_return.rb

@@ -9,9 +9,7 @@ class CustomerReturn < Spree::Base
 
     after_create :process_return!
 
-    validates :return_items, presence: true
-    validates :stock_location, presence: true
-
+    validates :return_items, :stock_location, presence: true
     validate :must_have_return_authorization, on: :create
     validate :return_items_belong_to_same_order
 

core/app/models/spree/line_item.rb

@@ -25,8 +25,8 @@ class LineItem < Spree::Base
                         }
     validates :price, numericality: true
     validates_with Stock::AvailabilityValidator
-
     validate :ensure_proper_currency
+
     before_destroy :update_inventory
     before_destroy :destroy_inventory_units
 

core/app/models/spree/option_type.rb

@@ -12,8 +12,10 @@ class OptionType < Spree::Base
     has_many :option_type_prototypes, class_name: 'Spree::OptionTypePrototype'
     has_many :prototypes, through: :option_type_prototypes, class_name: 'Spree::Prototype'
 
-    validates :name, presence: true, uniqueness: { allow_blank: true }
-    validates :presentation, presence: true
+    with_options presence: true do
+      validates :name, uniqueness: { allow_blank: true }
+      validates :presentation
+    end
 
     default_scope { order(:position) }
 

core/app/models/spree/option_value.rb

@@ -6,8 +6,10 @@ class OptionValue < Spree::Base
     has_many :option_value_variants, class_name: 'Spree::OptionValueVariant'
     has_many :variants, through: :option_value_variants, class_name: 'Spree::Variant'
 
-    validates :name, presence: true, uniqueness: { scope: :option_type_id, allow_blank: true }
-    validates :presentation, presence: true
+    with_options presence: true do
+      validates :name, uniqueness: { scope: :option_type_id, allow_blank: true }
+      validates :presentation
+    end
 
     after_touch :touch_all_variants
 

core/app/models/spree/order.rb

@@ -120,19 +120,14 @@ def states
     before_create :link_by_email
     before_update :homogenize_line_item_currencies, if: :currency_changed?
 
-    validates :number,               presence:     true,
-                                     length:       { maximum: 32, allow_blank: true },
-                                     uniqueness:   { allow_blank: true }
-    validates :email,                presence:     true,
-                                     length:       { maximum: 254, allow_blank: true },
-                                     email:        { allow_blank: true },
-                                     if:           :require_email
-    validates :state,                presence:     true,
-                                     inclusion:    { in: state_machine.states.map { |state| state.name.to_s }, allow_blank: true }
-    validates :payment_state,        inclusion:    { in: %w[balance_due paid credit_owed failed void], allow_blank: true }
-    validates :shipment_state,       inclusion:    { in: %w[ready pending partial shipped backorder canceled], allow_blank: true }
-    validates :item_count,           presence:     true,
-                                     numericality: { greater_than_or_equal_to: 0, less_than: 2**31, only_integer: true, allow_blank: true }
+    with_options presence: true do
+      validates :number, length: { maximum: 32, allow_blank: true }, uniqueness: { allow_blank: true }
+      validates :email, length: { maximum: 254, allow_blank: true }, email: { allow_blank: true }, if: :require_email
+      validates :state, inclusion: { in: state_machine.states.map { |state| state.name.to_s }, allow_blank: true }
+      validates :item_count, numericality: { greater_than_or_equal_to: 0, less_than: 2**31, only_integer: true, allow_blank: true }
+    end
+    validates :payment_state,        inclusion:    { in: ['balance_due', 'paid', 'credit_owed', 'failed', 'void'], allow_blank: true }
+    validates :shipment_state,       inclusion:    { in: ['ready', 'pending', 'partial', 'shipped', 'backorder', 'canceled'], allow_blank: true }
     validates :item_total,           POSITIVE_MONEY_VALIDATION
     validates :adjustment_total,     MONEY_VALIDATION
     validates :included_tax_total,   POSITIVE_MONEY_VALIDATION
@@ -144,7 +139,7 @@ def states
 
     validate :has_available_shipment
 
-    delegate :update_totals, :persist_totals, :to => :updater
+    delegate :update_totals, :persist_totals, to: :updater
     delegate :merge!, to: :merger
     delegate :firstname, :lastname, to: :bill_address, prefix: true, allow_nil: true
 

core/app/models/spree/product.rb

@@ -87,11 +87,14 @@ class Product < Spree::Base
     before_validation :normalize_slug, on: :update
     before_validation :validate_master
 
-    validates :meta_keywords, length: { maximum: 255 }, allow_blank: true
-    validates :meta_title, length: { maximum: 255 }, allow_blank: true
-    validates :name, presence: true
-    validates :price, presence: true, if: proc { Spree::Config[:require_master_price] }
-    validates :shipping_category_id, presence: true
+    with_options length: { maximum: 255 }, allow_blank: true do
+      validates :meta_keywords
+      validates :meta_title
+    end
+    with_options presence: true do
+      validates :name, :shipping_category_id
+      validates :price, if: proc { Spree::Config[:require_master_price] }
+    end
     validates :slug, length: { minimum: 3 }, allow_blank: true, uniqueness: true
 
     attr_accessor :option_values_hash

core/app/models/spree/refund.rb

@@ -8,12 +8,12 @@ class Refund < Spree::Base
 
     has_many :log_entries, as: :source
 
-    validates :payment, presence: true
-    validates :reason, presence: true
-    # can't require this on create because the perform! in after_create needs to run first
-    validates :transaction_id, presence: true, on: :update
-    validates :amount, presence: true, numericality: {greater_than: 0}
-
+    with_options presence: true do
+      validates :payment, :reason
+      # can't require this on create because the perform! in after_create needs to run first
+      validates :transaction_id, on: :update
+      validates :amount, numericality: { greater_than: 0 }
+    end
     validate :amount_is_less_than_or_equal_to_allowed_amount, on: :create
 
     after_create :perform!

core/app/models/spree/return_authorization.rb

@@ -17,9 +17,7 @@ class ReturnAuthorization < Spree::Base
 
     accepts_nested_attributes_for :return_items, allow_destroy: true
 
-    validates :order, presence: true
-    validates :reason, presence: true
-    validates :stock_location, presence: true
+    validates :order, :reason, :stock_location, presence: true
     validate :must_have_shipped_units, on: :create
 
 

core/app/models/spree/stock_movement.rb

@@ -5,13 +5,15 @@ class StockMovement < Spree::Base
 
     after_create :update_stock_item_quantity
 
-    validates :stock_item, presence: true
-    validates :quantity, presence: true, numericality: {
-                 greater_than_or_equal_to: -2**31,
-                 less_than_or_equal_to: 2**31-1,
-                 only_integer: true,
-                 allow_nil: true
-              }
+    with_options presence: true do
+      validates :stock_item
+      validates :quantity, numericality: {
+                                            greater_than_or_equal_to: -2**31,
+                                            less_than_or_equal_to: 2**31 - 1,
+                                            only_integer: true,
+                                            allow_nil: true
+                                          }
+    end
 
     scope :recent, -> { order(created_at: :desc) }
 

core/app/models/spree/store.rb

@@ -2,10 +2,10 @@ module Spree
   class Store < Spree::Base
     has_many :orders, class_name: "Spree::Order"
 
-    validates :code, presence: true, uniqueness: { allow_blank: true }
-    validates :name, presence: true
-    validates :url, presence: true
-    validates :mail_from_address, presence: true
+    with_options presence: true do
+      validates :code, uniqueness: { allow_blank: true }
+      validates :name, :url, :mail_from_address
+    end
 
     before_save :ensure_default_exists_and_is_unique
     before_destroy :validate_not_default

core/app/models/spree/tax_rate.rb

@@ -11,8 +11,10 @@ class TaxRate < Spree::Base
                  class_name: "Spree::TaxCategory"
     end
 
-    validates :amount, presence: true, numericality: { allow_nil: true }
-    validates :tax_category_id, presence: true
+    with_options presence: true do
+      validates :amount, numericality: { allow_nil: true }
+      validates :tax_category_id
+    end
 
     scope :by_zone, -> (zone) { where(zone_id: zone.id) }
     scope :potential_rates_for_zone,

core/app/models/spree/taxon.rb

@@ -20,9 +20,11 @@ class Taxon < Spree::Base
     has_many :promotion_rules, through: :promotion_rule_taxons, class_name: 'Spree::PromotionRule'
 
     validates :name, presence: true
-    validates :meta_keywords, length: { maximum: 255 }, allow_blank: true
-    validates :meta_description, length: { maximum: 255 }, allow_blank: true
-    validates :meta_title, length: { maximum: 255 }, allow_blank: true
+    with_options length: { maximum: 255 }, allow_blank: true do
+      validates :meta_keywords
+      validates :meta_description
+      validates :meta_title
+    end
 
     after_save :touch_ancestors_and_taxonomy
     after_touch :touch_ancestors_and_taxonomy

core/app/models/spree/variant.rb

@@ -38,8 +38,10 @@ class Variant < Spree::Base
 
     validate :check_price
 
-    validates :cost_price, numericality: { greater_than_or_equal_to: 0, allow_nil: true }
-    validates :price,      numericality: { greater_than_or_equal_to: 0, allow_nil: true }
+    with_options numericality: { greater_than_or_equal_to: 0, allow_nil: true } do
+      validates :cost_price
+      validates :price
+    end
     validates :sku, uniqueness: { conditions: -> { where(deleted_at: nil) } }, allow_blank: true
 
     after_create :create_stock_items