From 5e18038a68c2b19d1dff01a820f9c321cd8bc04c Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 1 May 2024 13:31:49 +0200 Subject: [PATCH] Fix RedundantPresenceValidationOnBelongs on two 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 | 4 +-- app/models/spree/stock_item.rb | 3 -- app/models/spree/stock_movement.rb | 5 +--- ...ariant_and_stock_location_on_stock_item.rb | 6 ++++ ...ock_item_and_quantity_on_stock_movement.rb | 6 ++++ db/schema.rb | 10 +++---- ...k_missing_required_ids_in_stock_items.rake | 28 +++++++++++++++++++ ...ssing_required_ids_in_stock_movements.rake | 28 +++++++++++++++++++ 8 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20240501072547_require_variant_and_stock_location_on_stock_item.rb create mode 100644 db/migrate/20240501075735_require_stock_item_and_quantity_on_stock_movement.rb create mode 100644 lib/tasks/data/check_missing_required_ids_in_stock_items.rake create mode 100644 lib/tasks/data/check_missing_required_ids_in_stock_movements.rake diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 7f40f94d2b..6eb925adc6 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -649,14 +649,12 @@ Rails/RedundantActiveRecordAllMethod: - 'app/models/spree/variant.rb' - 'spec/system/admin/product_import_spec.rb' -# Offense count: 11 +# Offense count: 9 # This cop supports unsafe autocorrection (--autocorrect-all). Rails/RedundantPresenceValidationOnBelongsTo: Exclude: - 'app/models/spree/line_item.rb' - 'app/models/spree/order.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' diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 565a165e6f..a89a44975e 100644 --- a/app/models/spree/stock_item.rb +++ b/app/models/spree/stock_item.rb @@ -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, dependent: :destroy - 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? } diff --git a/app/models/spree/stock_movement.rb b/app/models/spree/stock_movement.rb index 02352588e2..4de0dea3a0 100644 --- a/app/models/spree/stock_movement.rb +++ b/app/models/spree/stock_movement.rb @@ -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') } diff --git a/db/migrate/20240501072547_require_variant_and_stock_location_on_stock_item.rb b/db/migrate/20240501072547_require_variant_and_stock_location_on_stock_item.rb new file mode 100644 index 0000000000..c68cd74d0d --- /dev/null +++ b/db/migrate/20240501072547_require_variant_and_stock_location_on_stock_item.rb @@ -0,0 +1,6 @@ +class RequireVariantAndStockLocationOnStockItem < ActiveRecord::Migration[7.0] + def change + change_column_null :spree_stock_items, :stock_location_id, false + change_column_null :spree_stock_items, :variant_id, false + end +end diff --git a/db/migrate/20240501075735_require_stock_item_and_quantity_on_stock_movement.rb b/db/migrate/20240501075735_require_stock_item_and_quantity_on_stock_movement.rb new file mode 100644 index 0000000000..52d21d7f6d --- /dev/null +++ b/db/migrate/20240501075735_require_stock_item_and_quantity_on_stock_movement.rb @@ -0,0 +1,6 @@ +class RequireStockItemAndQuantityOnStockMovement < ActiveRecord::Migration[7.0] + def change + change_column_null :spree_stock_movements, :stock_item_id, false + change_column_null :spree_stock_movements, :quantity, false + end +end diff --git a/db/schema.rb b/db/schema.rb index 42e4b6ac40..122771b30f 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_30_075133) do +ActiveRecord::Schema[7.0].define(version: 2024_05_01_075735) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "plpgsql" @@ -818,8 +818,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_30_075133) do end create_table "spree_stock_items", id: :serial, force: :cascade do |t| - t.integer "stock_location_id" - t.integer "variant_id" + t.integer "stock_location_id", null: false + t.integer "variant_id", null: false t.integer "count_on_hand", default: 0, null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false @@ -849,8 +849,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_30_075133) do end create_table "spree_stock_movements", id: :serial, force: :cascade do |t| - t.integer "stock_item_id" - t.integer "quantity", default: 0 + t.integer "stock_item_id", null: false + t.integer "quantity", default: 0, null: false t.string "action", limit: 255 t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false diff --git a/lib/tasks/data/check_missing_required_ids_in_stock_items.rake b/lib/tasks/data/check_missing_required_ids_in_stock_items.rake new file mode 100644 index 0000000000..e265a012b4 --- /dev/null +++ b/lib/tasks/data/check_missing_required_ids_in_stock_items.rake @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +namespace :ofn do + namespace :data do + desc 'Checking missing required ids in Spree::StockItem' + task check_missing_required_missing_ids_in_spree_stock_items: :environment do + puts 'Checking for null stock_location_id' + ids = Spree::StockItem.where(stock_location_id: nil).pluck(:id) + + if ids.empty? + puts 'No NULL stock_location_id found in spree_stock_items' + else + puts 'NULL stock_location_ids s have been found in spree_stock_items:' + print ids + end + + puts 'Checking for null variant_id' + ids = Spree::StockItem.where(variant_id: nil).pluck(:id) + + if ids.empty? + puts 'No NULL variant_id found in spree_stock_items' + else + puts 'NULL variant_ids s have been found in spree_stock_items:' + print ids + end + end + end +end diff --git a/lib/tasks/data/check_missing_required_ids_in_stock_movements.rake b/lib/tasks/data/check_missing_required_ids_in_stock_movements.rake new file mode 100644 index 0000000000..56442255aa --- /dev/null +++ b/lib/tasks/data/check_missing_required_ids_in_stock_movements.rake @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +namespace :ofn do + namespace :data do + desc 'Checking missing required ids in Spree::StockMovement' + task check_missing_required_missing_ids_in_spree_stock_movements: :environment do + puts 'Checking for null stock_item_id' + ids = Spree::StockMovement.where(stock_item_id: nil).pluck(:id) + + if ids.empty? + puts 'No NULL stock_item_id found in spree_stock_movements' + else + puts 'NULL stock_item_ids s have been found in spree_stock_movements:' + print ids + end + + puts 'Checking for null quantity' + ids = Spree::StockMovement.where(quantity: nil).pluck(:id) + + if ids.empty? + puts 'No NULL quantity found in spree_stock_movements' + else + puts 'NULL quantity s have been found in spree_stock_movements:' + print ids + end + end + end +end