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