From 80b7a5d39ab272045b69080488aa44e7cc893b4c Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Wed, 29 Jun 2022 12:50:22 +0100 Subject: [PATCH] Remove OrderCycleShippingMethod model and use a :has_and_belongs_to_many association instead Before the OrderCycleShippingMethod had a validation which checked the shipping method belonged to the order cycle distributor. Instead of this validation this just ignores shipping methods which don't belong to one of the order cycle's distributors when they are being attached in the OrderCycleForm service. This pattern is already being used in the OrderCycleForm service for ignoring Schedules that the person doesn't own. Co-authored-by: Maikel --- .rubocop_todo.yml | 3 +- app/models/order_cycle.rb | 7 ++-- app/models/order_cycle_shipping_method.rb | 19 ----------- app/models/spree/shipping_method.rb | 1 - app/services/order_cycle_form.rb | 9 +++-- config/locales/en.yml | 4 --- ...2_create_order_cycles_shipping_methods.rb} | 9 +++-- db/schema.rb | 20 +++++------ .../order_cycle_shipping_method_spec.rb | 34 ------------------- spec/services/order_cycle_form_spec.rb | 22 ++++++------ 10 files changed, 35 insertions(+), 93 deletions(-) delete mode 100644 app/models/order_cycle_shipping_method.rb rename db/migrate/{20220429092052_create_order_cycle_shipping_methods.rb => 20220429092052_create_order_cycles_shipping_methods.rb} (57%) delete mode 100644 spec/models/order_cycle_shipping_method_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 72e4621f40..c8e4d81993 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -732,7 +732,7 @@ Rails/FilePath: - 'spec/models/content_configuration_spec.rb' - 'spec/support/downloads_helper.rb' -# Offense count: 11 +# Offense count: 12 # Configuration parameters: Include. # Include: app/models/**/*.rb Rails/HasAndBelongsToMany: @@ -740,6 +740,7 @@ Rails/HasAndBelongsToMany: - 'app/models/concerns/payment_method_distributors.rb' - 'app/models/enterprise.rb' - 'app/models/enterprise_group.rb' + - 'app/models/order_cycle.rb' - 'app/models/spree/line_item.rb' - 'app/models/spree/option_value.rb' - 'app/models/spree/role.rb' diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index fa0a5f2292..923dd659f1 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -24,10 +24,9 @@ class OrderCycle < ApplicationRecord has_many :distributors, -> { distinct }, source: :receiver, through: :cached_outgoing_exchanges has_many :order_cycle_schedules has_many :schedules, through: :order_cycle_schedules - has_many :order_cycle_shipping_methods, dependent: :destroy - has_many :selected_shipping_methods, class_name: "Spree::ShippingMethod", - through: :order_cycle_shipping_methods, - source: :shipping_method + has_and_belongs_to_many :selected_shipping_methods, + class_name: 'Spree::ShippingMethod', + join_table: 'order_cycles_shipping_methods' has_paper_trail meta: { custom_data: proc { |order_cycle| order_cycle.schedule_ids.to_s } } attr_accessor :incoming_exchanges, :outgoing_exchanges diff --git a/app/models/order_cycle_shipping_method.rb b/app/models/order_cycle_shipping_method.rb deleted file mode 100644 index 8b8f08d7ef..0000000000 --- a/app/models/order_cycle_shipping_method.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -class OrderCycleShippingMethod < ApplicationRecord - belongs_to :order_cycle - belongs_to :shipping_method, class_name: "Spree::ShippingMethod" - - validate :shipping_method_belongs_to_order_cycle_distributor - validates :shipping_method, uniqueness: { scope: :order_cycle_id } - - private - - def shipping_method_belongs_to_order_cycle_distributor - return if order_cycle.nil? || - shipping_method.nil? || - shipping_method.distributors.where(id: order_cycle.distributor_ids).exists? - - errors.add(:shipping_method, :must_belong_to_order_cycle_distributor) - end -end diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index 7d42b4d0af..589bc8ce8c 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -13,7 +13,6 @@ module Spree default_scope -> { where(deleted_at: nil) } - has_many :order_cycle_shipping_methods, dependent: :destroy has_many :shipping_rates, inverse_of: :shipping_method has_many :shipments, through: :shipping_rates has_many :shipping_method_categories diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index 926c49fbc3..88280984d5 100644 --- a/app/services/order_cycle_form.rb +++ b/app/services/order_cycle_form.rb @@ -56,6 +56,10 @@ class OrderCycleForm order_cycle.save! end + def attachable_shipping_method_ids + @attachable_shipping_method_ids ||= order_cycle.attachable_shipping_methods.map(&:id) + end + def exchanges_unchanged? [:incoming_exchanges, :outgoing_exchanges].all? do |direction| order_cycle_params[direction].nil? @@ -63,9 +67,10 @@ class OrderCycleForm end def selected_shipping_method_ids - @selected_shipping_method_ids = @selected_shipping_method_ids.reject(&:blank?).map(&:to_i) + @selected_shipping_method_ids = attachable_shipping_method_ids & + @selected_shipping_method_ids.reject(&:blank?).map(&:to_i) - if order_cycle.attachable_shipping_methods.map(&:id).sort == @selected_shipping_method_ids.sort + if attachable_shipping_method_ids.sort == @selected_shipping_method_ids.sort @selected_shipping_method_ids = [] end diff --git a/config/locales/en.yml b/config/locales/en.yml index 014d8dd07f..9d2171d249 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -77,10 +77,6 @@ en: attributes: orders_close_at: after_orders_open_at: must be after open date - order_cycle_shipping_method: - attributes: - shipping_method: - must_belong_to_order_cycle_distributor: "must be from a distributor on the order cycle" variant_override: count_on_hand: using_producer_stock_settings_but_count_on_hand_set: "must be blank because using producer stock settings" diff --git a/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb b/db/migrate/20220429092052_create_order_cycles_shipping_methods.rb similarity index 57% rename from db/migrate/20220429092052_create_order_cycle_shipping_methods.rb rename to db/migrate/20220429092052_create_order_cycles_shipping_methods.rb index cc16a501d2..a4596d69d4 100644 --- a/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb +++ b/db/migrate/20220429092052_create_order_cycles_shipping_methods.rb @@ -1,17 +1,16 @@ class CreateOrderCycleShippingMethods < ActiveRecord::Migration[6.1] def up - create_table :order_cycle_shipping_methods do |t| + create_table :order_cycles_shipping_methods, id: false do |t| t.references :order_cycle t.references :shipping_method, foreign_key: { to_table: :spree_shipping_methods } - t.timestamps end - add_index :order_cycle_shipping_methods, + add_index :order_cycles_shipping_methods, [:order_cycle_id, :shipping_method_id], - name: "order_cycle_shipping_methods_join_index", + name: "order_cycles_shipping_methods_join_index", unique: true end def down - drop_table :order_cycle_shipping_methods + drop_table :order_cycles_shipping_methods end end diff --git a/db/schema.rb b/db/schema.rb index 3f866caefd..1ae02469f3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -318,16 +318,6 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do t.index ["schedule_id"], name: "index_order_cycle_schedules_on_schedule_id" end - create_table "order_cycle_shipping_methods", force: :cascade do |t| - t.bigint "order_cycle_id" - t.bigint "shipping_method_id" - t.datetime "created_at", precision: 6, null: false - t.datetime "updated_at", precision: 6, null: false - t.index ["order_cycle_id", "shipping_method_id"], name: "order_cycle_shipping_methods_join_index", unique: true - t.index ["order_cycle_id"], name: "index_order_cycle_shipping_methods_on_order_cycle_id" - t.index ["shipping_method_id"], name: "index_order_cycle_shipping_methods_on_shipping_method_id" - end - create_table "order_cycles", id: :serial, force: :cascade do |t| t.string "name", limit: 255 t.datetime "orders_open_at" @@ -340,6 +330,14 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do t.boolean "mails_sent", default: false end + create_table "order_cycles_shipping_methods", id: false, force: :cascade do |t| + t.bigint "order_cycle_id" + t.bigint "shipping_method_id" + t.index ["order_cycle_id", "shipping_method_id"], name: "order_cycles_shipping_methods_join_index", unique: true + t.index ["order_cycle_id"], name: "index_order_cycles_shipping_methods_on_order_cycle_id" + t.index ["shipping_method_id"], name: "index_order_cycles_shipping_methods_on_shipping_method_id" + end + create_table "producer_properties", id: :serial, force: :cascade do |t| t.string "value", limit: 255 t.integer "producer_id" @@ -1263,8 +1261,8 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do add_foreign_key "exchanges", "order_cycles", name: "exchanges_order_cycle_id_fk" add_foreign_key "order_cycle_schedules", "order_cycles", name: "oc_schedules_order_cycle_id_fk" add_foreign_key "order_cycle_schedules", "schedules", name: "oc_schedules_schedule_id_fk" - add_foreign_key "order_cycle_shipping_methods", "spree_shipping_methods", column: "shipping_method_id" add_foreign_key "order_cycles", "enterprises", column: "coordinator_id", name: "order_cycles_coordinator_id_fk" + add_foreign_key "order_cycles_shipping_methods", "spree_shipping_methods", column: "shipping_method_id" add_foreign_key "producer_properties", "enterprises", column: "producer_id", name: "producer_properties_producer_id_fk" add_foreign_key "producer_properties", "spree_properties", column: "property_id", name: "producer_properties_property_id_fk" add_foreign_key "proxy_orders", "order_cycles", name: "proxy_orders_order_cycle_id_fk" diff --git a/spec/models/order_cycle_shipping_method_spec.rb b/spec/models/order_cycle_shipping_method_spec.rb deleted file mode 100644 index 4914313be7..0000000000 --- a/spec/models/order_cycle_shipping_method_spec.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe OrderCycleShippingMethod do - it "is valid if the shipping method belongs to one of the order cycle distributors" do - shipping_method = create(:shipping_method) - enterprise = create(:enterprise, shipping_methods: [shipping_method]) - order_cycle = create(:simple_order_cycle, distributors: [enterprise]) - - order_cycle_shipping_method = OrderCycleShippingMethod.new( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - - expect(order_cycle_shipping_method).to be_valid - end - - it "is not valid if the shipping method does not belong to one of the order cycle distributors" do - shipping_method = create(:shipping_method) - enterprise = create(:enterprise) - order_cycle = create(:simple_order_cycle, distributors: [enterprise]) - - order_cycle_shipping_method = OrderCycleShippingMethod.new( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - - expect(order_cycle_shipping_method).not_to be_valid - expect(order_cycle_shipping_method.errors.to_a).to eq [ - "Shipping method must be from a distributor on the order cycle" - ] - end -end diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index fc3a137030..d92f64aeef 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -187,7 +187,8 @@ describe OrderCycleForm do end end - context "updating outgoing exchanges but specifying an invalid shipping method" do + context "updating outgoing exchanges and shipping methods simultaneously but the shipping + method doesn't belong to the new or any existing order cycle distributor" do let(:other_distributor_shipping_method) do create(:shipping_method, distributors: [create(:distributor_enterprise)]) end @@ -199,11 +200,10 @@ describe OrderCycleForm do ) end - it "returns a validation error" do - expect(form.save).to be false - expect(order_cycle.errors.to_a).to eq [ - "Shipping method must be from a distributor on the order cycle" - ] + it "saves the outgoing exchange but ignores the shipping method" do + expect(form.save).to be true + expect(order_cycle.distributors).to eq [distributor] + expect(order_cycle.shipping_methods).to be_empty end end @@ -240,8 +240,8 @@ describe OrderCycleForm do end end - context "and it's invalid" do - it "returns a validation error" do + context "with a shipping method which doesn't belong to one of the order cycle's distributors" do + it "ignores it" do distributor_i = create(:distributor_enterprise) distributor_ii = create(:distributor_enterprise) shipping_method_i = create(:shipping_method, distributors: [distributor_i]) @@ -253,10 +253,8 @@ describe OrderCycleForm do { selected_shipping_method_ids: [shipping_method_ii.id] }, order_cycle.coordinator) - expect(form.save).to be false - expect(order_cycle.errors.to_a).to eq [ - "Shipping method must be from a distributor on the order cycle" - ] + expect(form.save).to be true + expect(order_cycle.shipping_methods).to eq [shipping_method_i] end end end