From 43005672a9ab8dbcbbd5ea13632f373254248c6a Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 24 Apr 2024 15:19:17 +0200 Subject: [PATCH] Fix RedundantPresenceValidationOnBelongs on other files - 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 - added 'NOT NULL' constraints so model constraints match with contraints on DB tables. - corresponding migration files to match AR Models & DB tables - rake tasks to check corrupt data (ie: NULL/nil in id fields) - updated the todo --- .rubocop_todo.yml | 5 +---- app/models/spree/product_property.rb | 5 +---- app/models/spree/return_authorization.rb | 3 --- app/models/spree/state.rb | 4 +--- ...646_require_property_on_product_property.rb | 7 +++++++ ...54_require_order_on_return_authorization.rb | 7 +++++++ .../20240424121221_require_country_on_state.rb | 7 +++++++ db/schema.rb | 8 ++++---- .../check_missing_country_id_in_state.rake | 18 ++++++++++++++++++ ...ssing_order_id_in_return_authorization.rake | 18 ++++++++++++++++++ ...operty_in_joint_table_product_property.rake | 18 ++++++++++++++++++ 11 files changed, 82 insertions(+), 18 deletions(-) create mode 100644 db/migrate/20240424075646_require_property_on_product_property.rb create mode 100644 db/migrate/20240424113354_require_order_on_return_authorization.rb create mode 100644 db/migrate/20240424121221_require_country_on_state.rb create mode 100644 lib/tasks/data/check_missing_country_id_in_state.rake create mode 100644 lib/tasks/data/check_missing_order_id_in_return_authorization.rake create mode 100644 lib/tasks/data/check_missing_property_in_joint_table_product_property.rake diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 3092909883..95b4ce01c7 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -660,15 +660,12 @@ Rails/RedundantActiveRecordAllMethod: - 'app/models/spree/variant.rb' - 'spec/system/admin/product_import_spec.rb' -# Offense count: 14 +# Offense count: 11 # This cop supports unsafe autocorrection (--autocorrect-all). Rails/RedundantPresenceValidationOnBelongsTo: Exclude: - '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' diff --git a/app/models/spree/product_property.rb b/app/models/spree/product_property.rb index 653c446464..c14d7f9096 100644 --- a/app/models/spree/product_property.rb +++ b/app/models/spree/product_property.rb @@ -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") } diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb index 1b18b9ea4f..9d7a7dea2d 100644 --- a/app/models/spree/return_authorization.rb +++ b/app/models/spree/return_authorization.rb @@ -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 @@ -13,7 +11,6 @@ module Spree before_save :force_positive_amount before_create :generate_number - validates :order, presence: true validates :amount, numericality: true validate :must_have_shipped_units diff --git a/app/models/spree/state.rb b/app/models/spree/state.rb index cbbd159a55..1d310636da 100644 --- a/app/models/spree/state.rb +++ b/app/models/spree/state.rb @@ -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) diff --git a/db/migrate/20240424075646_require_property_on_product_property.rb b/db/migrate/20240424075646_require_property_on_product_property.rb new file mode 100644 index 0000000000..d8f8502e12 --- /dev/null +++ b/db/migrate/20240424075646_require_property_on_product_property.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class RequirePropertyOnProductProperty < ActiveRecord::Migration[7.0] + def change + change_column_null :spree_product_properties, :property_id, false + end +end diff --git a/db/migrate/20240424113354_require_order_on_return_authorization.rb b/db/migrate/20240424113354_require_order_on_return_authorization.rb new file mode 100644 index 0000000000..13aaea40aa --- /dev/null +++ b/db/migrate/20240424113354_require_order_on_return_authorization.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class RequireOrderOnReturnAuthorization < ActiveRecord::Migration[7.0] + def change + change_column_null :spree_return_authorizations, :order_id, false + end +end diff --git a/db/migrate/20240424121221_require_country_on_state.rb b/db/migrate/20240424121221_require_country_on_state.rb new file mode 100644 index 0000000000..b2862c4898 --- /dev/null +++ b/db/migrate/20240424121221_require_country_on_state.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class RequireCountryOnState < ActiveRecord::Migration[7.0] + def change + change_column_null :spree_states, :country_id, false + end +end diff --git a/db/schema.rb b/db/schema.rb index 68d8d5206d..97171b74ef 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_04_22_150502) do +ActiveRecord::Schema[7.0].define(version: 2024_04_24_121221) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "plpgsql" @@ -675,7 +675,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_22_150502) do create_table "spree_product_properties", id: :serial, force: :cascade do |t| t.string "value", limit: 255 t.integer "product_id" - t.integer "property_id" + t.integer "property_id", null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.integer "position", default: 0 @@ -718,7 +718,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_22_150502) do t.string "number", limit: 255 t.string "state", limit: 255 t.decimal "amount", precision: 10, scale: 2, default: "0.0", null: false - t.integer "order_id" + t.integer "order_id", null: false t.text "reason" t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false @@ -814,7 +814,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_22_150502) do create_table "spree_states", id: :serial, force: :cascade do |t| t.string "name", limit: 255 t.string "abbr", limit: 255 - t.integer "country_id" + t.integer "country_id", null: false end create_table "spree_stock_items", id: :serial, force: :cascade do |t| diff --git a/lib/tasks/data/check_missing_country_id_in_state.rake b/lib/tasks/data/check_missing_country_id_in_state.rake new file mode 100644 index 0000000000..0b56f49a71 --- /dev/null +++ b/lib/tasks/data/check_missing_country_id_in_state.rake @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +namespace :ofn do + namespace :data do + desc 'Checking missing country_id in Spree::State' + task check_missing_country_id_in_spree_states: :environment do + puts 'Checking for null country_id' + ids = Spree::State.where(country_id: nil).pluck(:id) + + if ids.empty? + puts 'No NULL country_id found in spree_states' + else + puts 'NULL country_ids s have been found in spree_states:' + print ids + end + end + end +end diff --git a/lib/tasks/data/check_missing_order_id_in_return_authorization.rake b/lib/tasks/data/check_missing_order_id_in_return_authorization.rake new file mode 100644 index 0000000000..d69bdb9bff --- /dev/null +++ b/lib/tasks/data/check_missing_order_id_in_return_authorization.rake @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +namespace :ofn do + namespace :data do + desc 'Checking order_id in ReturnAuthorization' + task check_missing_order_id_in_return_authorizations: :environment do + puts 'Checking for null order_id' + ids = Spree::ReturnAuthorization.where(order_id: nil).pluck(:id) + + if ids.empty? + puts 'No NULL order_id found in spree_return_authorizations' + else + puts 'NULL order_id s have been found in spree_return_authorizations:' + print ids + end + end + end +end diff --git a/lib/tasks/data/check_missing_property_in_joint_table_product_property.rake b/lib/tasks/data/check_missing_property_in_joint_table_product_property.rake new file mode 100644 index 0000000000..52949e83c6 --- /dev/null +++ b/lib/tasks/data/check_missing_property_in_joint_table_product_property.rake @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +namespace :ofn do + namespace :data do + desc 'Checking missing property_id in ProductProperty' + task check_missing_property_in_joint_table_product_property: :environment do + puts 'Checking for null property_id' + ids = Spree::ProductProperty.where(property_id: nil).pluck(:id) + + if ids.empty? + puts 'No NULL property_id found in spree_product_properties' + else + puts 'NULL property_ids s have been found in spree_product_properties:' + print ids + end + end + end +end