From d88781a083d3c11e84d1a283e1688e7e55c881f9 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 May 2023 14:52:20 +1000 Subject: [PATCH 1/7] Start to adopt new default to require belongs_to Newer version of Rails have this option as default. We can slowly transition to opt in gradually, model by model. Once all models are covered, we can change the default and remove the setting from the models again. The style violation was added to the todo file because it's temporary. We have 60 models using `belongs_to`. And changing them all at once breaks some specs. So let's do it gradually. --- .rubocop_todo.yml | 1 + app/models/spree/shipping_method.rb | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6eb6712cce..de959163db 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -681,6 +681,7 @@ Metrics/ClassLength: - 'app/models/spree/payment.rb' - 'app/models/spree/product.rb' - 'app/models/spree/shipment.rb' + - 'app/models/spree/shipping_method.rb' - 'app/models/spree/user.rb' - 'app/models/spree/variant.rb' - 'app/models/spree/zone.rb' diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index 89941d6ccd..0e023413ab 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -8,6 +8,8 @@ module Spree back_end: "back_end" }.freeze + self.belongs_to_required_by_default = true + acts_as_paranoid acts_as_taggable @@ -25,7 +27,7 @@ module Spree has_and_belongs_to_many :zones, join_table: 'spree_shipping_methods_zones', class_name: 'Spree::Zone' - belongs_to :tax_category, class_name: 'Spree::TaxCategory' + belongs_to :tax_category, class_name: 'Spree::TaxCategory', optional: true validates :name, presence: true validate :distributor_validation From 2d4cfd754856ea4f04ee302bd13963db54c1e1db Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 3 May 2023 16:16:33 +1000 Subject: [PATCH 2/7] Validate and enforce AdjustmentMetadata associations --- app/models/adjustment_metadata.rb | 2 ++ ...re_adjustment_and_enterprise_on_adjustment_metadata.rb | 8 ++++++++ db/schema.rb | 4 ++-- spec/models/adjustment_metadata_spec.rb | 3 +++ 4 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20230503060530_require_adjustment_and_enterprise_on_adjustment_metadata.rb diff --git a/app/models/adjustment_metadata.rb b/app/models/adjustment_metadata.rb index eb99ad1de7..4753592a46 100644 --- a/app/models/adjustment_metadata.rb +++ b/app/models/adjustment_metadata.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class AdjustmentMetadata < ApplicationRecord + self.belongs_to_required_by_default = true + belongs_to :adjustment, class_name: 'Spree::Adjustment' belongs_to :enterprise end diff --git a/db/migrate/20230503060530_require_adjustment_and_enterprise_on_adjustment_metadata.rb b/db/migrate/20230503060530_require_adjustment_and_enterprise_on_adjustment_metadata.rb new file mode 100644 index 0000000000..e13fdf4b72 --- /dev/null +++ b/db/migrate/20230503060530_require_adjustment_and_enterprise_on_adjustment_metadata.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class RequireAdjustmentAndEnterpriseOnAdjustmentMetadata < ActiveRecord::Migration[7.0] + def change + change_column_null :adjustment_metadata, :adjustment_id, false + change_column_null :adjustment_metadata, :enterprise_id, false + end +end diff --git a/db/schema.rb b/db/schema.rb index 1241b84131..1e7f7ef0aa 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -44,8 +44,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_22_120633) do end create_table "adjustment_metadata", id: :serial, force: :cascade do |t| - t.integer "adjustment_id" - t.integer "enterprise_id" + t.integer "adjustment_id", null: false + t.integer "enterprise_id", null: false t.string "fee_name", limit: 255 t.string "fee_type", limit: 255 t.string "enterprise_role", limit: 255 diff --git a/spec/models/adjustment_metadata_spec.rb b/spec/models/adjustment_metadata_spec.rb index 13657826b4..66ae5bc6f0 100644 --- a/spec/models/adjustment_metadata_spec.rb +++ b/spec/models/adjustment_metadata_spec.rb @@ -3,6 +3,9 @@ require 'spec_helper' describe AdjustmentMetadata do + it { is_expected.to belong_to(:adjustment).required } + it { is_expected.to belong_to(:enterprise).required } + it "is valid when built from factory" do adjustment_metadata = build(:adjustment_metadata) expect(adjustment_metadata).to be_valid From c02c90317f25cd9d3fdae4060e48262476588d16 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 9 May 2023 15:54:21 +1000 Subject: [PATCH 3/7] Require user of column preference record It doesn't make sense to have a preference without a user. And it was already enforced in the database. --- app/models/column_preference.rb | 2 ++ spec/models/column_preference_spec.rb | 14 +++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/models/column_preference.rb b/app/models/column_preference.rb index 3a08aa3a85..adcc6999bd 100644 --- a/app/models/column_preference.rb +++ b/app/models/column_preference.rb @@ -5,6 +5,8 @@ require 'open_food_network/column_preference_defaults' class ColumnPreference < ApplicationRecord extend OpenFoodNetwork::ColumnPreferenceDefaults + self.belongs_to_required_by_default = true + # Non-persisted attributes that only have one # setting (ie. the default) for a given column attr_accessor :name diff --git a/spec/models/column_preference_spec.rb b/spec/models/column_preference_spec.rb index 108c76b4ac..cfb51edb3d 100644 --- a/spec/models/column_preference_spec.rb +++ b/spec/models/column_preference_spec.rb @@ -3,19 +3,27 @@ require 'spec_helper' describe ColumnPreference, type: :model do + subject { + ColumnPreference.new( + user: user, action_name: :customers_index, column_name: :email + ) + } + let(:user) { build(:user) } + + it { is_expected.to belong_to(:user).required } + describe "finding stored preferences for a user and action" do before do allow(ColumnPreference).to receive(:known_actions) { ['some_action'] } allow(ColumnPreference).to receive(:valid_columns_for) { ['col1', 'col2', 'col3'] } end - let(:user) { create(:user) } let!(:col1_pref) { - ColumnPreference.create(user_id: user.id, action_name: 'some_action', column_name: 'col1', + ColumnPreference.create(user: user, action_name: 'some_action', column_name: 'col1', visible: true) } let!(:col2_pref) { - ColumnPreference.create(user_id: user.id, action_name: 'some_action', column_name: 'col2', + ColumnPreference.create(user: user, action_name: 'some_action', column_name: 'col2', visible: false) } let(:defaults) { From fc00a48d67041390024d385969f034e2ef58f7c5 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 May 2023 15:31:50 +1000 Subject: [PATCH 4/7] Require associations of CoordinatorFee --- app/models/coordinator_fee.rb | 2 ++ ..._order_cycle_and_enterprise_fee_on_coordinator_fees.rb | 8 ++++++++ db/schema.rb | 4 ++-- spec/models/coordinator_fee_spec.rb | 8 ++++++++ 4 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20230516052547_require_order_cycle_and_enterprise_fee_on_coordinator_fees.rb create mode 100644 spec/models/coordinator_fee_spec.rb diff --git a/app/models/coordinator_fee.rb b/app/models/coordinator_fee.rb index b308c611b1..4a69085e03 100644 --- a/app/models/coordinator_fee.rb +++ b/app/models/coordinator_fee.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class CoordinatorFee < ApplicationRecord + self.belongs_to_required_by_default = true + belongs_to :order_cycle belongs_to :enterprise_fee end diff --git a/db/migrate/20230516052547_require_order_cycle_and_enterprise_fee_on_coordinator_fees.rb b/db/migrate/20230516052547_require_order_cycle_and_enterprise_fee_on_coordinator_fees.rb new file mode 100644 index 0000000000..6cef109289 --- /dev/null +++ b/db/migrate/20230516052547_require_order_cycle_and_enterprise_fee_on_coordinator_fees.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class RequireOrderCycleAndEnterpriseFeeOnCoordinatorFees < ActiveRecord::Migration[7.0] + def change + change_column_null :coordinator_fees, :order_cycle_id, false + change_column_null :coordinator_fees, :enterprise_fee_id, false + end +end diff --git a/db/schema.rb b/db/schema.rb index 1e7f7ef0aa..a7e99573f2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -64,8 +64,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_22_120633) do end create_table "coordinator_fees", id: :serial, force: :cascade do |t| - t.integer "order_cycle_id" - t.integer "enterprise_fee_id" + t.integer "order_cycle_id", null: false + t.integer "enterprise_fee_id", null: false t.index ["enterprise_fee_id"], name: "index_coordinator_fees_on_enterprise_fee_id" t.index ["order_cycle_id"], name: "index_coordinator_fees_on_order_cycle_id" end diff --git a/spec/models/coordinator_fee_spec.rb b/spec/models/coordinator_fee_spec.rb new file mode 100644 index 0000000000..5e3a8765b8 --- /dev/null +++ b/spec/models/coordinator_fee_spec.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CoordinatorFee do + it { is_expected.to belong_to(:order_cycle).required } + it { is_expected.to belong_to(:enterprise_fee).required } +end From ed231ec5122f8be8d7708da1a98eeedafce6350c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 May 2023 15:38:22 +1000 Subject: [PATCH 5/7] Update belongs_to default in Customer model --- app/models/customer.rb | 10 ++++++---- spec/models/customer_spec.rb | 5 +++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index 6e5468fed1..c0dbf6d8a7 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -10,20 +10,22 @@ class Customer < ApplicationRecord include SetUnusedAddressFields + self.belongs_to_required_by_default = true + acts_as_taggable searchable_attributes :first_name, :last_name, :email, :code - belongs_to :enterprise, optional: false - belongs_to :user, class_name: "Spree::User" + belongs_to :enterprise + belongs_to :user, class_name: "Spree::User", optional: true has_many :orders, class_name: "Spree::Order" before_destroy :update_orders_and_delete_canceled_subscriptions - belongs_to :bill_address, class_name: "Spree::Address" + belongs_to :bill_address, class_name: "Spree::Address", optional: true alias_attribute :billing_address, :bill_address accepts_nested_attributes_for :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 accepts_nested_attributes_for :ship_address diff --git a/spec/models/customer_spec.rb b/spec/models/customer_spec.rb index 1b4f93cf02..8cf26c69d0 100644 --- a/spec/models/customer_spec.rb +++ b/spec/models/customer_spec.rb @@ -3,6 +3,11 @@ require 'spec_helper' describe Customer, type: :model do + it { is_expected.to belong_to(:enterprise).required } + it { is_expected.to belong_to(:user).optional } + it { is_expected.to belong_to(:bill_address).optional } + it { is_expected.to belong_to(:ship_address).optional } + describe 'an existing customer' do let(:customer) { create(:customer) } From bd11475fe15e94102db9a096d5844365ba239f19 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 May 2023 15:44:56 +1000 Subject: [PATCH 6/7] Require associations on DistributorPaymentMethod --- app/models/distributor_payment_method.rb | 2 ++ app/models/enterprise.rb | 3 ++- ...thod_and_distributor_on_distributor_payment_methods.rb | 8 ++++++++ db/schema.rb | 4 ++-- 4 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20230516054204_require_payment_method_and_distributor_on_distributor_payment_methods.rb diff --git a/app/models/distributor_payment_method.rb b/app/models/distributor_payment_method.rb index d4cf9f6d1d..57287f4422 100644 --- a/app/models/distributor_payment_method.rb +++ b/app/models/distributor_payment_method.rb @@ -2,6 +2,8 @@ class DistributorPaymentMethod < ApplicationRecord self.table_name = "distributors_payment_methods" + self.belongs_to_required_by_default = true + belongs_to :payment_method, class_name: "Spree::PaymentMethod", touch: true belongs_to :distributor, class_name: "Enterprise", touch: true end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index e7b81947ae..29b9422667 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -61,7 +61,8 @@ class Enterprise < ApplicationRecord has_many :users, through: :enterprise_roles belongs_to :owner, class_name: 'Spree::User', inverse_of: :owned_enterprises - has_many :distributor_payment_methods, foreign_key: :distributor_id + has_many :distributor_payment_methods, + inverse_of: :distributor, foreign_key: :distributor_id has_many :distributor_shipping_methods, foreign_key: :distributor_id has_many :payment_methods, through: :distributor_payment_methods has_many :shipping_methods, through: :distributor_shipping_methods diff --git a/db/migrate/20230516054204_require_payment_method_and_distributor_on_distributor_payment_methods.rb b/db/migrate/20230516054204_require_payment_method_and_distributor_on_distributor_payment_methods.rb new file mode 100644 index 0000000000..38ead36d6f --- /dev/null +++ b/db/migrate/20230516054204_require_payment_method_and_distributor_on_distributor_payment_methods.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class RequirePaymentMethodAndDistributorOnDistributorPaymentMethods < ActiveRecord::Migration[7.0] + def change + change_column_null :distributors_payment_methods, :payment_method_id, false + change_column_null :distributors_payment_methods, :distributor_id, false + end +end diff --git a/db/schema.rb b/db/schema.rb index a7e99573f2..dff9670a09 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -116,8 +116,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_22_120633) do end create_table "distributors_payment_methods", force: :cascade do |t| - t.integer "distributor_id" - t.integer "payment_method_id" + t.integer "distributor_id", null: false + t.integer "payment_method_id", null: false t.datetime "created_at", precision: nil t.datetime "updated_at", precision: nil t.index ["distributor_id"], name: "index_distributors_payment_methods_on_distributor_id" From 91d0dabc1d43f544700dc5e6e7f1e307f08e1d8d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 May 2023 15:49:03 +1000 Subject: [PATCH 7/7] Require associations on DistributorShippingMethod --- app/models/distributor_shipping_method.rb | 2 ++ app/models/enterprise.rb | 3 ++- ...hod_and_distributor_on_distributor_shipping_methods.rb | 8 ++++++++ db/schema.rb | 4 ++-- 4 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20230516054745_require_shipping_method_and_distributor_on_distributor_shipping_methods.rb diff --git a/app/models/distributor_shipping_method.rb b/app/models/distributor_shipping_method.rb index ebce5682bc..bb09eaec5f 100644 --- a/app/models/distributor_shipping_method.rb +++ b/app/models/distributor_shipping_method.rb @@ -2,6 +2,8 @@ class DistributorShippingMethod < ApplicationRecord self.table_name = "distributors_shipping_methods" + self.belongs_to_required_by_default = true + belongs_to :shipping_method, class_name: "Spree::ShippingMethod", touch: true belongs_to :distributor, class_name: "Enterprise", touch: true end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 29b9422667..9b74a711a5 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -63,7 +63,8 @@ class Enterprise < ApplicationRecord inverse_of: :owned_enterprises has_many :distributor_payment_methods, inverse_of: :distributor, foreign_key: :distributor_id - has_many :distributor_shipping_methods, foreign_key: :distributor_id + has_many :distributor_shipping_methods, + inverse_of: :distributor, foreign_key: :distributor_id has_many :payment_methods, through: :distributor_payment_methods has_many :shipping_methods, through: :distributor_shipping_methods has_many :customers diff --git a/db/migrate/20230516054745_require_shipping_method_and_distributor_on_distributor_shipping_methods.rb b/db/migrate/20230516054745_require_shipping_method_and_distributor_on_distributor_shipping_methods.rb new file mode 100644 index 0000000000..0a55501242 --- /dev/null +++ b/db/migrate/20230516054745_require_shipping_method_and_distributor_on_distributor_shipping_methods.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class RequireShippingMethodAndDistributorOnDistributorShippingMethods < ActiveRecord::Migration[7.0] + def change + change_column_null :distributors_shipping_methods, :shipping_method_id, false + change_column_null :distributors_shipping_methods, :distributor_id, false + end +end diff --git a/db/schema.rb b/db/schema.rb index dff9670a09..71537c92ce 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -125,8 +125,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_05_22_120633) do end create_table "distributors_shipping_methods", id: :serial, force: :cascade do |t| - t.integer "distributor_id" - t.integer "shipping_method_id" + t.integer "distributor_id", null: false + t.integer "shipping_method_id", null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.index ["distributor_id"], name: "index_distributors_shipping_methods_on_distributor_id"