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 <maikel@email.org.au>

.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'

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

app/models/order_cycle_shipping_method.rb

@@ -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

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

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
 

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"

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

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"

spec/models/order_cycle_shipping_method_spec.rb

@@ -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

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