Skip to content

Commit

Permalink
Fix cop RedundantPresenceValidationOnBelongs
Browse files Browse the repository at this point in the history
- presence: true is redundant since Rails 5.0 BUT applies
  with new default config of
  belongs_to_required_by_default to true
  Lots of files with belongs_to_required_by_default = false
  (backward compatibility)
  So: deleting this setting implies to adding optional: true
  on belongs_to relations where there is no explicit check for presence.
  And also to delete redundant presence: true
  The implicit becomes the explicit and vice versa
- updated toto list
  • Loading branch information
cyrillefr committed Apr 15, 2024
1 parent 4de1905 commit baa2169
Show file tree
Hide file tree
Showing 17 changed files with 14 additions and 84 deletions.
21 changes: 0 additions & 21 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -660,27 +660,6 @@ Rails/RedundantActiveRecordAllMethod:
- 'app/models/spree/variant.rb'
- 'spec/system/admin/product_import_spec.rb'

# Offense count: 20
# This cop supports unsafe autocorrection (--autocorrect-all).
Rails/RedundantPresenceValidationOnBelongsTo:
Exclude:
- 'app/models/enterprise_fee.rb'
- 'app/models/exchange.rb'
- 'app/models/inventory_item.rb'
- 'app/models/order_cycle.rb'
- 'app/models/spree/address.rb'
- 'app/models/spree/line_item.rb'
- 'app/models/spree/order.rb'
- 'app/models/spree/product_property.rb'
- 'app/models/spree/return_authorization.rb'
- 'app/models/spree/state.rb'
- 'app/models/spree/stock_item.rb'
- 'app/models/spree/stock_movement.rb'
- 'app/models/spree/tax_rate.rb'
- 'app/models/subscription_line_item.rb'
- 'app/models/tag_rule.rb'
- 'app/models/variant_override.rb'

# Offense count: 1
# This cop supports unsafe autocorrection (--autocorrect-all).
Rails/RelativeDateConstant:
Expand Down
1 change: 0 additions & 1 deletion app/models/enterprise_fee.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class EnterpriseFee < ApplicationRecord

validates :fee_type, inclusion: { in: FEE_TYPES }
validates :name, presence: true
validates :enterprise_id, presence: true

before_save :ensure_valid_tax_category_settings

Expand Down
3 changes: 0 additions & 3 deletions app/models/exchange.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
# shopfront (outgoing products). But the set of shown products can be smaller
# than all incoming products.
class Exchange < ApplicationRecord
self.belongs_to_required_by_default = false

acts_as_taggable

belongs_to :order_cycle
Expand All @@ -24,7 +22,6 @@ class Exchange < ApplicationRecord
has_many :exchange_fees, dependent: :destroy
has_many :enterprise_fees, through: :exchange_fees

validates :order_cycle, :sender, :receiver, presence: true
validates :sender_id, uniqueness: { scope: [:order_cycle_id, :receiver_id, :incoming] }

before_destroy :delete_related_exchange_variants, prepend: true
Expand Down
4 changes: 0 additions & 4 deletions app/models/inventory_item.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
# frozen_string_literal: true

class InventoryItem < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :enterprise
belongs_to :variant, class_name: "Spree::Variant"

validates :variant_id, uniqueness: { scope: :enterprise_id }
validates :enterprise, presence: true
validates :variant, presence: true
validates :visible,
inclusion: { in: [true, false], message: I18n.t(:inventory_item_visibility_error) }

Expand Down
4 changes: 1 addition & 3 deletions app/models/order_cycle.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
require 'open_food_network/scope_variant_to_hub'

class OrderCycle < ApplicationRecord
self.belongs_to_required_by_default = false

searchable_attributes :orders_open_at, :orders_close_at, :coordinator_id
searchable_scopes :active, :inactive, :active_or_complete, :upcoming, :closed, :not_closed,
:dated, :undated, :soonest_opening, :soonest_closing, :most_recently_closed
Expand Down Expand Up @@ -44,7 +42,7 @@ class OrderCycle < ApplicationRecord
before_update :reset_processed_at, if: :will_save_change_to_orders_close_at?
after_save :sync_subscriptions, if: :opening?

validates :name, :coordinator_id, presence: true
validates :name, presence: true
validate :orders_close_at_after_orders_open_at?

preference :product_selection_from_coordinator_inventory_only, :boolean, default: false
Expand Down
6 changes: 2 additions & 4 deletions app/models/spree/address.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,17 @@ module Spree
class Address < ApplicationRecord
include AddressDisplay

self.belongs_to_required_by_default = false

searchable_attributes :firstname, :lastname, :phone, :full_name, :full_name_reversed,
:full_name_with_comma, :full_name_with_comma_reversed
searchable_associations :country, :state

belongs_to :country, class_name: "Spree::Country"
belongs_to :state, class_name: "Spree::State"
belongs_to :state, class_name: "Spree::State", optional: true

has_one :enterprise, dependent: :restrict_with_exception
has_many :shipments

validates :address1, :city, :country, :phone, presence: true
validates :address1, :city, :phone, presence: true
validates :company, presence: true, unless: -> { first_name.blank? || last_name.blank? }
validates :firstname, :lastname, presence: true, if: -> do
company.blank? || company == 'unused'
Expand Down
7 changes: 2 additions & 5 deletions app/models/spree/line_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,17 @@ class LineItem < ApplicationRecord
include VariantUnits::VariantAndLineItemNaming
include LineItemStockChanges

self.belongs_to_required_by_default = false

searchable_attributes :price, :quantity, :order_id, :variant_id, :tax_category_id
searchable_associations :order, :order_cycle, :variant, :product, :supplier, :tax_category
searchable_scopes :with_tax, :without_tax

belongs_to :order, class_name: "Spree::Order", inverse_of: :line_items
belongs_to :order, class_name: "Spree::Order", inverse_of: :line_items, optional: true
has_one :order_cycle, through: :order

belongs_to :variant, -> { with_deleted }, class_name: "Spree::Variant"
has_one :product, through: :variant
has_one :supplier, through: :product
belongs_to :tax_category, class_name: "Spree::TaxCategory"
belongs_to :tax_category, class_name: "Spree::TaxCategory", optional: true

has_many :adjustments, as: :adjustable, dependent: :destroy

Expand All @@ -28,7 +26,6 @@ class LineItem < ApplicationRecord
before_validation :copy_tax_category
before_validation :copy_dimensions

validates :variant, presence: true
validates :quantity, numericality: {
only_integer: true,
greater_than: -1,
Expand Down
15 changes: 5 additions & 10 deletions app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ class Order < ApplicationRecord
include Balance
include SetUnusedAddressFields

self.belongs_to_required_by_default = false

searchable_attributes :number, :state, :shipment_state, :payment_state, :distributor_id,
:order_cycle_id, :email, :total, :customer_id
searchable_associations :shipping_method, :bill_address, :distributor
Expand All @@ -33,13 +31,13 @@ class Order < ApplicationRecord

token_resource

belongs_to :user, class_name: "Spree::User"
belongs_to :created_by, class_name: "Spree::User"
belongs_to :user, class_name: "Spree::User", optional: true
belongs_to :created_by, class_name: "Spree::User", optional: true

belongs_to :bill_address, class_name: 'Spree::Address'
belongs_to :bill_address, class_name: 'Spree::Address', optional: true
alias_attribute :billing_address, :bill_address

belongs_to :ship_address, class_name: 'Spree::Address'
belongs_to :ship_address, class_name: 'Spree::Address', optional: true
alias_attribute :shipping_address, :ship_address

has_many :state_changes, as: :stateful, dependent: :destroy
Expand Down Expand Up @@ -72,7 +70,7 @@ def states

belongs_to :order_cycle
belongs_to :distributor, class_name: 'Enterprise'
belongs_to :customer
belongs_to :customer, optional: true
has_one :proxy_order, dependent: :destroy
has_one :subscription, through: :proxy_order

Expand All @@ -96,9 +94,6 @@ def states
format: /\A([\w.%+\-']+)@([\w\-]+\.)+(\w{2,})\z/i,
if: :require_email

validates :order_cycle, presence: true, on: :require_distribution
validates :distributor, presence: true, on: :require_distribution

before_validation :set_currency
before_validation :generate_order_number, if: :new_record?
before_validation :clone_billing_address, if: :use_billing?
Expand Down
5 changes: 1 addition & 4 deletions app/models/spree/product_property.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@

module Spree
class ProductProperty < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :product, class_name: "Spree::Product", touch: true
belongs_to :product, class_name: "Spree::Product", touch: true, optional: true
belongs_to :property, class_name: 'Spree::Property'

validates :property, presence: true
validates :value, length: { maximum: 255 }

default_scope -> { order("#{table_name}.position") }
Expand Down
3 changes: 0 additions & 3 deletions app/models/spree/return_authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

module Spree
class ReturnAuthorization < ApplicationRecord
self.belongs_to_required_by_default = false

acts_as_paranoid

belongs_to :order, class_name: 'Spree::Order', inverse_of: :return_authorizations
Expand All @@ -13,7 +11,6 @@ class ReturnAuthorization < ApplicationRecord
before_save :force_positive_amount
before_create :generate_number

validates :order, presence: true
validates :amount, numericality: true
validate :must_have_shipped_units

Expand Down
4 changes: 1 addition & 3 deletions app/models/spree/state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@

module Spree
class State < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :country, class_name: 'Spree::Country'

validates :country, :name, presence: true
validates :name, presence: true

def self.find_all_by_name_or_abbr(name_or_abbr)
where('name = ? OR abbr = ?', name_or_abbr, name_or_abbr)
Expand Down
3 changes: 0 additions & 3 deletions app/models/spree/stock_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,12 @@

module Spree
class StockItem < ApplicationRecord
self.belongs_to_required_by_default = false

acts_as_paranoid

belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items
belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant'
has_many :stock_movements

validates :stock_location, :variant, presence: true
validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }
validates :count_on_hand, numericality: { greater_than_or_equal_to: 0, unless: :backorderable? }

Expand Down
5 changes: 1 addition & 4 deletions app/models/spree/stock_movement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,11 @@

module Spree
class StockMovement < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :stock_item, class_name: 'Spree::StockItem'
belongs_to :originator, polymorphic: true
belongs_to :originator, polymorphic: true, optional: true

after_create :update_stock_item_quantity

validates :stock_item, presence: true
validates :quantity, presence: true

scope :recent, -> { order('created_at DESC') }
Expand Down
5 changes: 1 addition & 4 deletions app/models/spree/tax_rate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,14 @@ def validate(record)

module Spree
class TaxRate < ApplicationRecord
self.belongs_to_required_by_default = false

acts_as_paranoid
include CalculatedAdjustments

belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates
belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates, optional: true
belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates
has_many :adjustments, as: :originator

validates :amount, presence: true, numericality: true
validates :tax_category, presence: true
validates_with DefaultTaxZoneValidator

scope :by_zone, ->(zone) { where(zone_id: zone) }
Expand Down
4 changes: 0 additions & 4 deletions app/models/subscription_line_item.rb
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
# frozen_string_literal: true

class SubscriptionLineItem < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :subscription, inverse_of: :subscription_line_items
belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant'

validates :subscription, presence: true
validates :variant, presence: true
validates :quantity, presence: true, numericality: { only_integer: true }

default_scope { order('id ASC') }
Expand Down
4 changes: 0 additions & 4 deletions app/models/tag_rule.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
# frozen_string_literal: true

class TagRule < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :enterprise

preference :customer_tags, :string, default: ""

validates :enterprise, presence: true

scope :for, ->(enterprise) { where(enterprise_id: enterprise) }
scope :prioritised, -> { order('priority ASC') }

Expand Down
4 changes: 0 additions & 4 deletions app/models/variant_override.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,11 @@ class VariantOverride < ApplicationRecord
extend Spree::LocalizedNumber
include StockSettingsOverrideValidation

self.belongs_to_required_by_default = false

acts_as_taggable

belongs_to :hub, class_name: 'Enterprise'
belongs_to :variant, class_name: 'Spree::Variant'

validates :hub, presence: true
validates :variant, presence: true
# Default stock can be nil, indicating stock should not be reset or zero, meaning reset to zero.
# Need to ensure this can be set by the user.
validates :default_stock, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true
Expand Down

0 comments on commit baa2169

Please sign in to comment.