From fc4f951a1aa2d20b3062d904dce15f2b06c3ec43 Mon Sep 17 00:00:00 2001
From: Cillian O'Ruanaidh <contact@cilliano.com>
Date: Fri, 17 Jun 2022 10:48:36 +0100
Subject: [PATCH] Only require OrderCycleShippingMethod records if people want
 to override the default shipping methods

It makes things much simpler if we return all shipping methods by default without needing OrderCycleShippingMethod records to be added to the database.

Co-authored-by: Maikel <maikel@email.org.au>
---
 app/models/order_cycle.rb                     |  33 +--
 app/models/order_cycle_shipping_method.rb     |   7 -
 app/models/spree/shipping_method.rb           |  20 +-
 app/services/order_cycle_form.rb              |  22 +-
 .../permitted_attributes/order_cycle.rb       |   2 +-
 .../order_cycles/checkout_options.html.haml   |   2 +
 config/locales/en.yml                         |   1 -
 ...052_create_order_cycle_shipping_methods.rb |  89 +------
 db/schema.rb                                  |   2 +-
 lib/tasks/sample_data/order_cycle_factory.rb  |   3 -
 spec/factories/subscription_factory.rb        |   8 +-
 .../order_cycle_shipping_method_spec.rb       |  16 --
 spec/models/order_cycle_spec.rb               |  98 ++++----
 spec/models/spree/shipping_method_spec.rb     |  63 +++--
 .../order_available_shipping_methods_spec.rb  | 219 ++++++------------
 spec/services/order_cycle_form_spec.rb        |  27 ++-
 16 files changed, 243 insertions(+), 369 deletions(-)

diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb
index 84800086c9..c552c4e45c 100644
--- a/app/models/order_cycle.rb
+++ b/app/models/order_cycle.rb
@@ -25,20 +25,19 @@ class OrderCycle < ApplicationRecord
   has_many :order_cycle_schedules
   has_many :schedules, through: :order_cycle_schedules
   has_many :order_cycle_shipping_methods
-  has_many :shipping_methods, class_name: "Spree::ShippingMethod",
-                              through: :order_cycle_shipping_methods
+  has_many :preferred_shipping_methods, class_name: "Spree::ShippingMethod",
+                                        through: :order_cycle_shipping_methods,
+                                        source: :shipping_method
   has_paper_trail meta: { custom_data: proc { |order_cycle| order_cycle.schedule_ids.to_s } }
 
   attr_accessor :incoming_exchanges, :outgoing_exchanges
 
-  attribute :validate_shipping_methods, :boolean, default: true
-
   before_update :reset_processed_at, if: :will_save_change_to_orders_close_at?
   after_save :sync_subscriptions, if: :opening?
 
   validates :name, :coordinator_id, presence: true
   validate :at_least_one_shipping_method_selected_for_each_distributor
-  validate :no_invalid_shipping_methods
+  validate :no_invalid_order_cycle_shipping_methods
   validate :orders_close_at_after_orders_open_at?
 
   preference :product_selection_from_coordinator_inventory_only, :boolean, default: false
@@ -165,8 +164,6 @@ class OrderCycle < ApplicationRecord
   end
 
   def attachable_shipping_methods
-    return Spree::ShippingMethod.none if simple? || !shipping_methods_customisable?
-
     Spree::ShippingMethod.frontend.
       joins(:distributor_shipping_methods).
       where("distributor_id IN (?)", distributor_ids).
@@ -182,12 +179,9 @@ class OrderCycle < ApplicationRecord
     oc.preferred_product_selection_from_coordinator_inventory_only = preferred_product_selection_from_coordinator_inventory_only
     # rubocop:enable Layout/LineLength
     oc.schedule_ids = schedule_ids
-    oc.shipping_methods_customisable = true
     oc.save!
-    oc.validate_shipping_methods = false
     exchanges.each { |e| e.clone!(oc) }
-    oc.validate_shipping_methods = true
-    oc.shipping_method_ids = shipping_method_ids
+    oc.preferred_shipping_method_ids = preferred_shipping_method_ids
     sync_subscriptions
     oc.reload
   end
@@ -301,6 +295,14 @@ class OrderCycle < ApplicationRecord
     items.each { |li| scoper.scope(li.variant) }
   end
 
+  def shipping_methods
+    if simple? || preferred_shipping_methods.none?
+      attachable_shipping_methods
+    else
+      preferred_shipping_methods
+    end
+  end
+
   def simple?
     coordinator.sells == 'own'
   end
@@ -308,20 +310,21 @@ class OrderCycle < ApplicationRecord
   private
 
   def all_distributors_have_at_least_one_shipping_method?
-    distributors.all? { |distributor| (distributor.shipping_method_ids & shipping_method_ids).any? }
+    distributors.all? do |distributor|
+      (distributor.shipping_method_ids & preferred_shipping_method_ids).any?
+    end
   end
 
   def at_least_one_shipping_method_selected_for_each_distributor
-    return if !validate_shipping_methods? ||
+    return if preferred_shipping_methods.none? ||
               coordinator.nil? ||
               simple? ||
-              !shipping_methods_customisable? ||
               all_distributors_have_at_least_one_shipping_method?
 
     errors.add(:base, :at_least_one_shipping_method_per_distributor)
   end
 
-  def no_invalid_shipping_methods
+  def no_invalid_order_cycle_shipping_methods
     return if order_cycle_shipping_methods.all?(&:valid?)
 
     errors.add(
diff --git a/app/models/order_cycle_shipping_method.rb b/app/models/order_cycle_shipping_method.rb
index dbdd0ccd0d..ebb0420660 100644
--- a/app/models/order_cycle_shipping_method.rb
+++ b/app/models/order_cycle_shipping_method.rb
@@ -7,7 +7,6 @@ class OrderCycleShippingMethod < ActiveRecord::Base
   validate :shipping_method_belongs_to_order_cycle_distributor
   validate :shipping_method_available_at_checkout
   validate :order_cycle_not_simple
-  validate :order_cycle_shipping_methods_customisable
   validates_uniqueness_of :shipping_method, scope: :order_cycle_id
 
   before_destroy :check_shipping_method_not_selected_on_any_orders
@@ -36,12 +35,6 @@ class OrderCycleShippingMethod < ActiveRecord::Base
     errors.add(:order_cycle, :must_not_be_simple)
   end
 
-  def order_cycle_shipping_methods_customisable
-    return if order_cycle.nil? || order_cycle.shipping_methods_customisable?
-
-    errors.add(:order_cycle, :must_support_customisable_shipping_methods)
-  end
-
   def shipping_method_available_at_checkout
     return if shipping_method.nil? || shipping_method.frontend?
 
diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb
index 7a7eb44487..b7bd7e4092 100644
--- a/app/models/spree/shipping_method.rb
+++ b/app/models/spree/shipping_method.rb
@@ -115,18 +115,14 @@ module Spree
 
     private
 
-    def no_active_or_upcoming_non_simple_order_cycles_with_only_one_shipping_method?
+    def no_active_or_upcoming_order_cycle_distributors_with_only_one_shipping_method?
       return true if new_record?
 
-      OrderCycle.active.or(OrderCycle.upcoming).joins(:coordinator, :shipping_methods).where("
-        sells != 'own' AND
-        spree_shipping_methods.id = ? AND
-        NOT EXISTS(
-          SELECT 1
-          FROM order_cycle_shipping_methods
-          WHERE order_cycle_id = order_cycles.id AND
-          shipping_method_id != ?
-        )", id, id).none?
+      distributors.
+        with_order_cycles_as_distributor_outer.
+        merge(OrderCycle.active.or(OrderCycle.upcoming)).none? do |distributor|
+        distributor.shipping_method_ids.one?
+      end
     end
 
     def at_least_one_shipping_category
@@ -146,7 +142,7 @@ module Spree
     end
 
     def check_destroy_wont_leave_order_cycles_without_shipping_methods
-      return if no_active_or_upcoming_non_simple_order_cycles_with_only_one_shipping_method?
+      return if no_active_or_upcoming_order_cycle_distributors_with_only_one_shipping_method?
 
       errors.add(:base, :destroy_leaves_order_cycles_without_shipping_methods)
       throw :abort
@@ -154,7 +150,7 @@ module Spree
 
     def switching_to_backoffice_only_wont_leave_order_cycles_without_shipping_methods
       return if frontend? ||
-                no_active_or_upcoming_non_simple_order_cycles_with_only_one_shipping_method?
+                no_active_or_upcoming_order_cycle_distributors_with_only_one_shipping_method?
 
       errors.add(:base, :switching_to_backoffice_only_leaves_order_cycles_without_shipping_methods)
     end
diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb
index ba6aa590e7..18e07553a8 100644
--- a/app/services/order_cycle_form.rb
+++ b/app/services/order_cycle_form.rb
@@ -11,13 +11,12 @@ class OrderCycleForm
     @user = user
     @permissions = OpenFoodNetwork::Permissions.new(user)
     @schedule_ids = order_cycle_params.delete(:schedule_ids)
-    @shipping_method_ids = order_cycle_params.delete(:shipping_method_ids)
+    @preferred_shipping_method_ids = order_cycle_params.delete(:preferred_shipping_method_ids)
   end
 
   def save
     schedule_ids = build_schedule_ids
     order_cycle.assign_attributes(order_cycle_params)
-    order_cycle.validate_shipping_methods = false
     return false unless order_cycle.valid?
 
     order_cycle.transaction do
@@ -25,7 +24,7 @@ class OrderCycleForm
       order_cycle.schedule_ids = schedule_ids
       order_cycle.save!
       apply_exchange_changes
-      attach_shipping_methods
+      attach_preferred_shipping_methods
       sync_subscriptions
       true
     end
@@ -49,12 +48,11 @@ class OrderCycleForm
     OpenFoodNetwork::OrderCycleFormApplicator.new(order_cycle, user).go!
   end
 
-  def attach_shipping_methods
-    return if @shipping_method_ids.nil?
+  def attach_preferred_shipping_methods
+    return if @preferred_shipping_method_ids.nil?
 
     order_cycle.reload # so outgoing exchanges are up-to-date for shipping method validations
-    order_cycle.validate_shipping_methods = true
-    order_cycle.shipping_method_ids = @shipping_method_ids
+    order_cycle.preferred_shipping_method_ids = preferred_shipping_method_ids
     order_cycle.save!
   end
 
@@ -64,6 +62,16 @@ class OrderCycleForm
     end
   end
 
+  def preferred_shipping_method_ids
+    @preferred_shipping_method_ids = @preferred_shipping_method_ids.reject(&:blank?).map(&:to_i)
+
+    if order_cycle.attachable_shipping_methods.map(&:id).sort == @preferred_shipping_method_ids.sort
+      @preferred_shipping_method_ids = []
+    end
+
+    @preferred_shipping_method_ids
+  end
+
   def schedule_ids?
     @schedule_ids.present?
   end
diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb
index 97282acabf..8f5998b70d 100644
--- a/app/services/permitted_attributes/order_cycle.rb
+++ b/app/services/permitted_attributes/order_cycle.rb
@@ -17,7 +17,7 @@ module PermittedAttributes
         :name, :orders_open_at, :orders_close_at, :coordinator_id,
         :preferred_product_selection_from_coordinator_inventory_only,
         :automatic_notifications,
-        { schedule_ids: [], shipping_method_ids: [], coordinator_fee_ids: [] }
+        { schedule_ids: [], preferred_shipping_method_ids: [], coordinator_fee_ids: [] }
       ]
     end
 
diff --git a/app/views/admin/order_cycles/checkout_options.html.haml b/app/views/admin/order_cycles/checkout_options.html.haml
index 97f436308c..ca76377eda 100644
--- a/app/views/admin/order_cycles/checkout_options.html.haml
+++ b/app/views/admin/order_cycles/checkout_options.html.haml
@@ -13,6 +13,8 @@
   %fieldset.no-border-bottom
     %legend{ align: 'center'}= t('.checkout_options')
 
+  = hidden_field_tag "order_cycle[preferred_shipping_method_ids][]", ""
+
   %table.checkout-options
     %thead
       %tr
diff --git a/config/locales/en.yml b/config/locales/en.yml
index ef52faec39..1321a077f4 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -90,7 +90,6 @@ en:
               shipping_method_already_used_in_order_cycle: "This shipping method has already been selected on orders in this order cycle and cannot be removed"
             order_cycle:
               must_not_be_simple: "is simple, all shipping methods are available by default and cannot be customised"
-              must_support_customisable_shipping_methods: "shipping methods cannot be customised, all shipping methods are available by default"
             shipping_method:
               must_be_available_at_checkout: "must be available at checkout"
               must_belong_to_order_cycle_distributor: "must be from a distributor on the order cycle"
diff --git a/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb b/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb
index ae78acc94a..cc16a501d2 100644
--- a/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb
+++ b/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb
@@ -1,98 +1,17 @@
 class CreateOrderCycleShippingMethods < ActiveRecord::Migration[6.1]
-  # Before this migration every available shipping method was available to customers on order
-  # cycles by default. However this migration only populates :order_cycles_shipping_methods records
-  # for active or upcoming order cycles because retroactively calculating which shipping methods
-  # should be attached to past, closed order cycles is probaby tricky so skipping that because it
-  # may not be even necessary. Instead this adds a :shipping_methods_customisable flag to order
-  # cycles so we have a record of order cycles created before this feature was deployed.
-  #
-  # Note: Redefining the Spree::ShippingMethod class in this migration as suggested by the
-  # :good_migrations gem was not passing Good Migrations checks. This redefines the classes inside
-  # a Migration class to to bypass this problem.
-
-  class Migration
-    class DistributorShippingMethod < ActiveRecord::Base
-      self.table_name = "distributors_shipping_methods"
-      belongs_to :shipping_method, class_name: "Migration::ShippingMethod", touch: true
-      belongs_to :distributor, class_name: "Enterprise", touch: true
-    end
-
-    class Enterprise < ActiveRecord::Base
-    end
-
-    class Exchange < ActiveRecord::Base
-      self.table_name = "exchanges"
-      belongs_to :receiver, class_name: 'Migration::Enterprise'
-    end
-
-    class OrderCycle < ActiveRecord::Base
-      self.table_name = "order_cycles"
-      has_many :order_cycle_shipping_methods, class_name: "Migration::OrderCycleShippingMethod"
-      has_many :shipping_methods, class_name: "Migration::ShippingMethod", through: :order_cycle_shipping_methods
-
-      has_many :cached_outgoing_exchanges, -> { where incoming: false }, class_name: "Migration::Exchange"
-      has_many :distributors, -> { distinct }, source: :receiver, through: :cached_outgoing_exchanges
-
-      belongs_to :coordinator, class_name: 'Migration::Enterprise'
-
-      scope :active, lambda {
-        where('order_cycles.orders_open_at <= ? AND order_cycles.orders_close_at >= ?',
-              Time.zone.now,
-              Time.zone.now)
-      }
-      scope :upcoming, lambda { where('order_cycles.orders_open_at > ?', Time.zone.now) }
-    end
-
-    class OrderCycleShippingMethod < ActiveRecord::Base
-      self.table_name = "order_cycle_shipping_methods"
-      belongs_to :shipping_method, class_name: "Migration::ShippingMethod"
-    end
-
-    class ShippingMethod < ActiveRecord::Base
-      self.table_name = "spree_shipping_methods"
-    end
-
-    def self.attach_all_shipping_methods_to_non_simple_active_or_upcoming_order_cycles
-      non_simple_active_or_upcoming_order_cycles.find_each do |order_cycle|
-        order_cycle.shipping_method_ids = DistributorShippingMethod.
-          where("display_on != 'back_end'").
-          where(distributor_id: order_cycle.distributor_ids).
-          joins(:shipping_method).
-          pluck(:shipping_method_id)
-      end
-    end
-
-    def self.set_shipping_methods_customisable_to_false_on_past_order_cycles
-      OrderCycle.update_all(shipping_methods_customisable: false)
-      active_or_upcoming_order_cycles.update_all(shipping_methods_customisable: true)
-    end
-
-    private
-
-    def self.active_or_upcoming_order_cycles
-      OrderCycle.active.or(OrderCycle.upcoming)
-    end
-
-    def self.non_simple_active_or_upcoming_order_cycles
-      active_or_upcoming_order_cycles.joins(:coordinator).where("sells != 'own'")
-    end
-  end
-
   def up
     create_table :order_cycle_shipping_methods do |t|
       t.references :order_cycle
       t.references :shipping_method, foreign_key: { to_table: :spree_shipping_methods }
       t.timestamps
     end
-
-    add_column :order_cycles, :shipping_methods_customisable, :boolean, default: true
-
-    Migration.set_shipping_methods_customisable_to_false_on_past_order_cycles
-    Migration.attach_all_shipping_methods_to_non_simple_active_or_upcoming_order_cycles
+    add_index :order_cycle_shipping_methods,
+      [:order_cycle_id, :shipping_method_id],
+      name: "order_cycle_shipping_methods_join_index",
+      unique: true
   end
 
   def down
-    remove_column :order_cycles, :shipping_methods_customisable
     drop_table :order_cycle_shipping_methods
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index bca8dabe52..3f866caefd 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -323,6 +323,7 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do
     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
@@ -337,7 +338,6 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do
     t.datetime "processed_at"
     t.boolean "automatic_notifications", default: false
     t.boolean "mails_sent", default: false
-    t.boolean "shipping_methods_customisable", default: true
   end
 
   create_table "producer_properties", id: :serial, force: :cascade do |t|
diff --git a/lib/tasks/sample_data/order_cycle_factory.rb b/lib/tasks/sample_data/order_cycle_factory.rb
index d6701d5177..0f9dc258e8 100644
--- a/lib/tasks/sample_data/order_cycle_factory.rb
+++ b/lib/tasks/sample_data/order_cycle_factory.rb
@@ -57,9 +57,6 @@ module SampleData
       log "- #{name}"
       cycle = create_order_cycle_with_fee(name, coordinator)
       create_exchanges(cycle, supplier_names, distributor_names, data)
-      return if cycle.reload.attachable_shipping_methods.none?
-
-      cycle.shipping_method_ids = cycle.attachable_shipping_methods.pluck(:id)
     end
 
     def create_order_cycle_with_fee(name, coordinator)
diff --git a/spec/factories/subscription_factory.rb b/spec/factories/subscription_factory.rb
index 5bccb7eaf0..f0b67c0631 100644
--- a/spec/factories/subscription_factory.rb
+++ b/spec/factories/subscription_factory.rb
@@ -3,13 +3,7 @@
 FactoryBot.define do
   factory :subscription, class: Subscription do
     shop { create :enterprise }
-    schedule do
-      order_cycle = create(:distributor_order_cycle,
-                           coordinator: shop,
-                           distributors: [shop],
-                           shipping_methods: [shipping_method])
-      create(:schedule, order_cycles: [order_cycle])
-    end
+    schedule { create(:schedule, order_cycles: [create(:simple_order_cycle, coordinator: shop)]) }
     customer { create(:customer, enterprise: shop) }
     bill_address { create(:address, :randomized) }
     ship_address { create(:address, :randomized) }
diff --git a/spec/models/order_cycle_shipping_method_spec.rb b/spec/models/order_cycle_shipping_method_spec.rb
index fcc5fd5290..bb503182fb 100644
--- a/spec/models/order_cycle_shipping_method_spec.rb
+++ b/spec/models/order_cycle_shipping_method_spec.rb
@@ -51,22 +51,6 @@ describe OrderCycleShippingMethod do
     )
   end
 
-  it "is not valid if order cycle doesn't support customised shipping methods
-      e.g. the order cycle was created before the custom shipping methods feature was available" do
-    order_cycle = create(:distributor_order_cycle, shipping_methods_customisable: false)
-    shipping_method = create(:shipping_method, distributors: [order_cycle.coordinator])
-
-    order_cycle_shipping_method = OrderCycleShippingMethod.new(
-      order_cycle: order_cycle,
-      shipping_method: shipping_method
-    )
-
-    expect(order_cycle_shipping_method).to_not be_valid
-    expect(order_cycle_shipping_method.errors.to_a).to include(
-      "Order cycle shipping methods cannot be customised, all shipping methods are available by default"
-    )
-  end
-
   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])
diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb
index ab66327504..0da971b524 100644
--- a/spec/models/order_cycle_spec.rb
+++ b/spec/models/order_cycle_spec.rb
@@ -28,9 +28,13 @@ describe OrderCycle do
           distributor_ii = create(:distributor_enterprise)
           distributor_i_shipping_method = create(:shipping_method, distributors: [distributor_i])
           distributor_ii_shipping_method = create(:shipping_method, distributors: [distributor_ii])
-          order_cycle = create(:distributor_order_cycle, distributors: [distributor_i, distributor_ii])
+          order_cycle = create(:distributor_order_cycle,
+                               distributors: [distributor_i, distributor_ii])
 
-          order_cycle.shipping_method_ids = [distributor_i_shipping_method.id, distributor_ii_shipping_method.id]
+          order_cycle.preferred_shipping_method_ids = [
+            distributor_i_shipping_method.id,
+            distributor_ii_shipping_method.id
+          ]
 
           expect(order_cycle).to be_valid
         end
@@ -43,19 +47,24 @@ describe OrderCycle do
         distributor_ii_shipping_method = create(:shipping_method, distributors: [distributor_i])
         order_cycle = create(:distributor_order_cycle, distributors: [distributor_i, distributor_ii])
 
-        order_cycle.shipping_method_ids = [distributor_i_shipping_method.id]
+        order_cycle.preferred_shipping_method_ids = [distributor_i_shipping_method.id]
 
         expect(order_cycle).to be_invalid
-        expect(order_cycle.errors.to_a).to eq ["You need to select at least one shipping method for each distributor"]
+        expect(order_cycle.errors.to_a).to eq [
+          "You need to select at least one shipping method for each distributor"
+        ]
       end
     end
   end
 
-  describe "#no_invalid_shipping_methods" do
-    context "when a shipping method is not valid" do
-      it "adds a validation error, and it is more meaningful than the default 'Order cycle shipping methods is invalid'" do
-        order_cycle = create(:distributor_order_cycle, with_distributor_and_shipping_method: true)
-        shipping_method = order_cycle.shipping_methods.first
+  describe "#no_invalid_order_cycle_shipping_methods" do
+    context "when a order cycle shipping method is not valid" do
+      it "adds a validation error,
+          and it is more meaningful than the default 'Order cycle shipping methods is invalid'" do
+        shipping_method = create(:shipping_method)
+        distributor = create(:distributor_enterprise, shipping_methods: [shipping_method])
+        order_cycle = create(:distributor_order_cycle, distributors: [distributor])
+        order_cycle.preferred_shipping_methods << shipping_method
 
         shipping_method.update_column(:display_on, "back_end")
 
@@ -69,23 +78,6 @@ describe OrderCycle do
     end
   end
 
-  describe "#validate_shipping_methods" do
-    it "doesn't skip shipping method validations by default" do
-      order_cycle = create(:distributor_order_cycle, distributors: [create(:distributor_enterprise)])
-
-      expect(order_cycle).to be_invalid
-      expect(order_cycle.errors.to_a).to eq ["You need to select at least one shipping method for each distributor"]
-    end
-
-    it "allows shipping method validations to be skipped because distributors need to be saved before shipping methods" do
-      order_cycle = create(:distributor_order_cycle, distributors: [create(:distributor_enterprise)])
-
-      order_cycle.validate_shipping_methods = false
-
-      expect(order_cycle).to be_valid
-    end
-  end
-
   it "has a coordinator and associated fees" do
     oc = create(:simple_order_cycle)
 
@@ -198,7 +190,7 @@ describe OrderCycle do
   end
 
   it "reports its distributors" do
-    oc = create(:simple_order_cycle, validate_shipping_methods: false)
+    oc = create(:simple_order_cycle)
 
     e1 = create(:exchange, incoming: false,
                            order_cycle: oc, sender: oc.coordinator, receiver: create(:enterprise))
@@ -209,7 +201,7 @@ describe OrderCycle do
   end
 
   it "checks for existance of distributors" do
-    oc = create(:simple_order_cycle, validate_shipping_methods: false)
+    oc = create(:simple_order_cycle)
     d1 = create(:distributor_enterprise)
     d2 = create(:distributor_enterprise)
     create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, incoming: false)
@@ -470,22 +462,14 @@ describe OrderCycle do
       expect(cloned_exchange_attributes).to match_array original_exchange_attributes
     end
 
-    context "distributor order cycle created before the customisable shipping methods feature was available" do
-      it "allows the clone to have customisable shipping methods" do
-        order_cycle = create(:distributor_order_cycle, shipping_methods_customisable: false)
-
-        order_cycle.clone!
-
-        order_cycle_clone = OrderCycle.last
-        expect(order_cycle_clone.shipping_methods_customisable).to eq(true)
-      end
-    end
-
-    context "when it has shipping methods which can longer be applied validly e.g. shipping method is backoffice only" do
+    context "when it has preferred shipping methods which can longer be applied validly
+             e.g. shipping method is backoffice only" do
       it "raises an error (TODO: display a message to user explaining why clone failed)" do
         distributor = create(:distributor_enterprise)
         shipping_method = create(:shipping_method, distributors: [distributor])
-        order_cycle = create(:distributor_order_cycle, distributors: [distributor], shipping_methods: [shipping_method])
+        order_cycle = create(:distributor_order_cycle, distributors: [distributor])
+        order_cycle.preferred_shipping_methods << shipping_method
+
         shipping_method.update_column(:display_on, "back_end")
 
         expect {
@@ -721,6 +705,38 @@ describe OrderCycle do
     end
   end
 
+  describe "#shipping_methods" do
+    let(:distributor) { create(:distributor_enterprise) }
+
+    it "returns all attachable shipping methods if the order cycle is simple" do
+      oc = create(:sells_own_order_cycle, distributors: [distributor])
+
+      shipping_method = create(:shipping_method, distributors: [distributor])
+
+      expect(oc.shipping_methods).to eq [shipping_method]
+    end
+
+    context "distributor order cycle i.e. non-simple" do
+      let(:oc) { create(:distributor_order_cycle, distributors: [distributor]) }
+
+      it "returns all attachable shipping methods if no preferred shipping methods have been chosen" do
+        shipping_method = create(:shipping_method, distributors: [distributor])
+
+        expect(oc.preferred_shipping_methods).to be_empty
+        expect(oc.shipping_methods).to eq [shipping_method]
+      end
+
+      it "returns preferred shipping methods if they have been specified" do
+        shipping_method_i = create(:shipping_method, distributors: [distributor])
+        shipping_method_ii = create(:shipping_method, distributors: [distributor])
+
+        oc.preferred_shipping_methods << shipping_method_ii
+
+        expect(oc.shipping_methods).to eq [shipping_method_ii]
+      end
+    end
+  end
+
   describe "#simple?" do
     it "returns true if the coordinator sells their own products i.e. shops" do
       order_cycle = build(:simple_order_cycle, coordinator: build(:enterprise, sells: "own"))
diff --git a/spec/models/spree/shipping_method_spec.rb b/spec/models/spree/shipping_method_spec.rb
index 8c030fa02d..ba4b5d84fb 100644
--- a/spec/models/spree/shipping_method_spec.rb
+++ b/spec/models/spree/shipping_method_spec.rb
@@ -182,22 +182,38 @@ module Spree
       end
 
       context "when it is being changed to backoffice only" do
-        let!(:order_cycle) { create(:distributor_order_cycle, with_distributor_and_shipping_method: true) }
-        let(:distributor) { order_cycle.distributors.first }
-        let(:shipping_method) { order_cycle.shipping_methods.first }
+        let!(:order_cycle) { create(:distributor_order_cycle, distributors: [distributor]) }
+        let(:distributor) { create(:distributor_enterprise, shipping_methods: [shipping_method]) }
+        let(:shipping_method) { create(:shipping_method) }
 
-        context "when the shipping method is the only shipping method on a distributor order cycle" do
+        context "when one of its distributors has no other shipping methods available
+                 on an active or upcoming order cycle" do
           it "should not be valid" do
             shipping_method.display_on = "back_end"
 
             expect(shipping_method).not_to be_valid
-            expect(shipping_method.errors.to_a).to eq ["Unable to switch to backoffice only, some open or upcoming order cycles would be left without any shipping methods"]
+            expect(shipping_method.errors.to_a).to eq [
+              "Unable to switch to backoffice only, some open or upcoming order cycles would be " \
+              "left without any shipping methods"
+            ]
           end
         end
 
-        context "when the order cycles the shipping method is attached to has other valid shipping methods" do
-          it "is is valid" do
-            order_cycle.shipping_methods << create(:shipping_method, distributors: [distributor])
+        context "when one of its distributors has no other shipping methods available
+                 on an order cycle which isn't active or upcoming" do
+          it "is valid" do
+            order_cycle.update!(orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago)
+
+            shipping_method.display_on = "back_end"
+
+            expect(shipping_method).to be_valid
+          end
+        end
+
+        context "when one of its distributors has other shipping methods available
+                 on an active or upcoming order cycle" do
+          it "is valid" do
+            create(:shipping_method, distributors: [distributor])
 
             shipping_method.display_on = "back_end"
 
@@ -242,9 +258,10 @@ module Spree
     context "#destroy" do
       let(:shipping_method) { create(:shipping_method) }
       let(:distributor) { create(:distributor_enterprise, shipping_methods: [shipping_method]) }
-      let!(:order_cycle) { create(:distributor_order_cycle, distributors: [distributor], shipping_methods: [shipping_method]) }
+      let!(:order_cycle) { create(:distributor_order_cycle, distributors: [distributor]) }
 
-      context "when the shipping method is the only shipping method on a distributor order cycle" do
+      context "when its distributors are part of some order cycles
+               and have no other shipping methods available" do
         it "can be deleted if the order cycle is closed" do
           order_cycle.update!(orders_close_at: 1.minute.ago)
 
@@ -259,7 +276,10 @@ module Spree
           shipping_method.destroy
 
           expect(shipping_method).not_to be_deleted
-          expect(shipping_method.errors.to_a).to eq ["Unable to delete, some open or upcoming order cycles would be left without any shipping methods"]
+          expect(shipping_method.errors.to_a).to eq [
+            "Unable to delete, some open or upcoming order cycles would be left without any " \
+            "shipping methods"
+          ]
         end
 
         it "cannot be deleted if the order cycle is upcoming" do
@@ -268,13 +288,28 @@ module Spree
           shipping_method.destroy
 
           expect(shipping_method).not_to be_deleted
-          expect(shipping_method.errors.to_a).to eq ["Unable to delete, some open or upcoming order cycles would be left without any shipping methods"]
+          expect(shipping_method.errors.to_a).to eq [
+            "Unable to delete, some open or upcoming order cycles would be left without any " \
+            "shipping methods"
+          ]
         end
       end
 
-      context "when the order cycles the shipping method is attached to has other valid shipping methods" do
+      context "when its distributors are part of some active or upcoming order cycles
+               and have other shipping methods available" do
         it "can be deleted" do
-          order_cycle.shipping_methods << create(:shipping_method, distributors: [distributor])
+          create(:shipping_method, distributors: [distributor])
+
+          shipping_method.destroy
+
+          expect(shipping_method).to be_deleted
+        end
+      end
+
+      context "when its distributors have no other shipping methods available
+               but aren't part of active/upcoming order cycles" do
+        it "can be deleted" do
+          order_cycle.update!(orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago)
 
           shipping_method.destroy
 
diff --git a/spec/services/order_available_shipping_methods_spec.rb b/spec/services/order_available_shipping_methods_spec.rb
index 814853d6eb..5cfdde3e21 100644
--- a/spec/services/order_available_shipping_methods_spec.rb
+++ b/spec/services/order_available_shipping_methods_spec.rb
@@ -26,54 +26,41 @@ describe OrderAvailableShippingMethods do
   end
 
   context "when no tag rules are in effect" do
-    context "order cycle selling own produce only i.e. shipping methods cannot be customised" do
-      it "returns all shipping methods belonging to the enterprise" do
-        order_cycle = create(:sells_own_order_cycle)
-        enterprise = order_cycle.coordinator
-        shipping_method = create(:shipping_method, distributors: [enterprise])
-        other_enterprise = create(:enterprise)
-        other_enterprise_shipping_method = create(:shipping_method,
-                                                  distributors: [other_enterprise])
-        order = build(:order, distributor: enterprise, order_cycle: order_cycle)
+    context "sells own order cycle i.e. simple" do
+      it "only returns the shipping methods which are available on the order cycle
+          and belong to the order distributor" do
+        distributor_i = create(:distributor_enterprise)
+        distributor_ii = create(:distributor_enterprise)
+        distributor_iii = create(:distributor_enterprise)
+        shipping_method_i = create(:shipping_method, distributors: [distributor_i])
+        shipping_method_ii = create(:shipping_method, distributors: [distributor_ii])
+        shipping_method_iii = create(:shipping_method, distributors: [distributor_iii])
+        order_cycle = create(:sells_own_order_cycle, distributors: [distributor_i, distributor_ii])
+        order = build(:order, distributor: distributor_i, order_cycle: order_cycle)
 
         available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a
 
-        expect(order_cycle.shipping_methods).to be_empty
-        expect(available_shipping_methods).to eq [shipping_method]
+        expect(available_shipping_methods).to eq [shipping_method_i]
       end
     end
 
-    context "distributor order cycle" do
-      it "only returns shipping methods which belong to the order distributor
-          and have been added to the order cycle" do
-        distributor = create(:distributor_enterprise)
-        shipping_method_i = create(:shipping_method, distributors: [distributor])
-        shipping_method_ii = create(:shipping_method, distributors: [distributor])
-        order_cycle = create(:simple_order_cycle,
-                             distributors: [distributor], shipping_methods: [shipping_method_i])
-        order = build(:order, distributor: distributor, order_cycle: order_cycle)
-
-        available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a
-
-        expect(available_shipping_methods).to eq order_cycle.shipping_methods
-        expect(available_shipping_methods).to eq [shipping_method_i]
-      end
-
-      it "doesn't return shipping methods which have been added to the order cycle
-          when they don't belong to the order distributor" do
+    context "distributor order cycle i.e. not simple" do
+      it "only returns the shipping methods which are available on the order cycle
+          and belong to the order distributor" do
         distributor_i = create(:distributor_enterprise)
         distributor_ii = create(:distributor_enterprise)
         shipping_method_i = create(:shipping_method, distributors: [distributor_i])
-        shipping_method_ii = create(:shipping_method, distributors: [distributor_ii])
-        order_cycle = create(:simple_order_cycle,
-                             distributors: [distributor_i, distributor_ii],
-                             shipping_methods: [shipping_method_i, shipping_method_ii])
-        order = build(:order, distributor: distributor_ii, order_cycle: order_cycle)
+        shipping_method_ii = create(:shipping_method, distributors: [distributor_i])
+        shipping_method_iii = create(:shipping_method, distributors: [distributor_ii])
+        shipping_method_iv = create(:shipping_method, distributors: [distributor_ii])
+        order_cycle = create(:distributor_order_cycle,
+                             distributors: [distributor_i, distributor_ii])
+        order_cycle.preferred_shipping_methods << [shipping_method_i, shipping_method_iii]
+        order = build(:order, distributor: distributor_i, order_cycle: order_cycle)
 
         available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a
 
-        expect(available_shipping_methods).not_to eq order_cycle.shipping_methods
-        expect(available_shipping_methods).to eq [shipping_method_ii]
+        expect(available_shipping_methods).to eq [shipping_method_i]
       end
     end
   end
@@ -116,66 +103,39 @@ describe OrderAvailableShippingMethods do
                                           'hidden')
       }
 
-      context "order cycle selling own produce only" do
-        let(:order_cycle) { create(:sells_own_order_cycle) }
-        let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) }
+      let(:order_cycle) { create(:sells_own_order_cycle) }
+      let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) }
 
-        context "when the customer is nil" do
-          let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a }
+      context "when the customer is nil" do
+        let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a }
 
-          it "applies default action (hide)" do
-            expect(available_shipping_methods).to include untagged_sm
-            expect(available_shipping_methods).to_not include tagged_sm
-          end
-        end
-
-        context "when a customer is present" do
-          let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a }
-
-          context "and the customer's tags match" do
-            before do
-              customer.update_attribute(:tag_list, 'local')
-            end
-
-            it "applies the action (show)" do
-              expect(available_shipping_methods).to include tagged_sm, untagged_sm
-            end
-          end
-
-          context "and the customer's tags don't match" do
-            before do
-              customer.update_attribute(:tag_list, 'something')
-            end
-
-            it "applies the default action (hide)" do
-              expect(available_shipping_methods).to include untagged_sm
-              expect(available_shipping_methods).to_not include tagged_sm
-            end
-          end
+        it "applies default action (hide)" do
+          expect(available_shipping_methods).to include untagged_sm
+          expect(available_shipping_methods).to_not include tagged_sm
         end
       end
 
-      context "distributor order cycle" do
-        context "when the shipping method without the tag rule is attached to the order cycle
-                 and the shipping method with the tag rule is not" do
-          let(:order_cycle) do
-            create(:distributor_order_cycle,
-                   distributors: [distributor], shipping_methods: [untagged_sm])
+      context "when a customer is present" do
+        let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a }
+
+        context "and the customer's tags match" do
+          before do
+            customer.update_attribute(:tag_list, 'local')
           end
-          let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) }
-          let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a }
 
-          context "when the customer's tags match" do
-            before do
-              customer.update_attribute(:tag_list, 'local')
-            end
+          it "applies the action (show)" do
+            expect(available_shipping_methods).to include tagged_sm, untagged_sm
+          end
+        end
 
-            it "doesn't display the shipping method with the prefered visibility 'visible' tag
-                even though the customer's tags match
-                because it hasn't been attached to the order cycle" do
-              expect(available_shipping_methods).to include untagged_sm
-              expect(available_shipping_methods).to_not include tagged_sm
-            end
+        context "and the customer's tags don't match" do
+          before do
+            customer.update_attribute(:tag_list, 'something')
+          end
+
+          it "applies the default action (hide)" do
+            expect(available_shipping_methods).to include untagged_sm
+            expect(available_shipping_methods).to_not include tagged_sm
           end
         end
       end
@@ -190,77 +150,38 @@ describe OrderAvailableShippingMethods do
                                           'visible')
       }
 
-      context "order cycle selling own produce only" do
-        let(:order_cycle) { create(:sells_own_order_cycle) }
-        let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) }
+      let(:order_cycle) { create(:sells_own_order_cycle) }
+      let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) }
 
-        context "when the customer is nil" do
-          let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a }
+      context "when the customer is nil" do
+        let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a }
 
-          it "applies default action (show)" do
-            expect(available_shipping_methods).to include tagged_sm, untagged_sm
-          end
-        end
-
-        context "when a customer is present" do
-          let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a }
-
-          context "and the customer's tags match" do
-            before do
-              customer.update_attribute(:tag_list, 'local')
-            end
-
-            it "applies the action (hide)" do
-              expect(available_shipping_methods).to include untagged_sm
-              expect(available_shipping_methods).to_not include tagged_sm
-            end
-          end
-
-          context "and the customer's tags don't match" do
-            before do
-              customer.update_attribute(:tag_list, 'something')
-            end
-
-            it "applies the default action (show)" do
-              expect(available_shipping_methods).to include tagged_sm, untagged_sm
-            end
-          end
+        it "applies default action (show)" do
+          expect(available_shipping_methods).to include tagged_sm, untagged_sm
         end
       end
 
-      context "distributor order cycle" do
-        context "when the shipping method without the tag rule is attached to the order cycle
-                 and the shipping method with the tag rule is not" do
-          let(:order_cycle) do
-            create(:distributor_order_cycle,
-                   distributors: [distributor], shipping_methods: [untagged_sm])
-          end
-          let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) }
+      context "when a customer is present" do
+        let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a }
 
-          context "when the customer is nil" do
-            let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a }
-
-            it "doesn't display the shipping method tagged to be visible by default
-                because it is not attached to the order cycle" do
-              expect(available_shipping_methods).to include untagged_sm
-              expect(available_shipping_methods).to_not include tagged_sm
-            end
+        context "and the customer's tags match" do
+          before do
+            customer.update_attribute(:tag_list, 'local')
           end
 
-          context "when a customer is present" do
-            let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a }
+          it "applies the action (hide)" do
+            expect(available_shipping_methods).to include untagged_sm
+            expect(available_shipping_methods).to_not include tagged_sm
+          end
+        end
 
-            context "when the customer's tags don't match" do
-              before do
-                customer.update_attribute(:tag_list, 'something')
-              end
+        context "and the customer's tags don't match" do
+          before do
+            customer.update_attribute(:tag_list, 'something')
+          end
 
-              it "doesn't display the shipping method tagged to be visible by default
-                  because it is not attached to the order cycle" do
-                expect(available_shipping_methods).to include untagged_sm
-                expect(available_shipping_methods).to_not include tagged_sm
-              end
-            end
+          it "applies the default action (show)" do
+            expect(available_shipping_methods).to include tagged_sm, untagged_sm
           end
         end
       end
diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb
index c7b472257d..f2edeacbce 100644
--- a/spec/services/order_cycle_form_spec.rb
+++ b/spec/services/order_cycle_form_spec.rb
@@ -146,7 +146,8 @@ describe OrderCycleForm do
       end
     end
 
-    context "updating basics, incoming and outcoming exchanges, shipping methods simultaneously" do
+    context "updating basics, incoming exchanges, outcoming exchanges
+             and preferred shipping methods simultaneously" do
       before do
         params.merge!(
           incoming_exchanges: [{
@@ -158,7 +159,7 @@ describe OrderCycleForm do
             enterprise_fee_ids: []
           }],
           outgoing_exchanges: [outgoing_exchange_params],
-          shipping_method_ids: [shipping_method.id]
+          preferred_shipping_method_ids: [shipping_method.id]
         )
       end
 
@@ -175,7 +176,7 @@ describe OrderCycleForm do
       before do
         params.merge!(
           outgoing_exchanges: [outgoing_exchange_params],
-          shipping_method_ids: nil
+          preferred_shipping_method_ids: nil
         )
       end
 
@@ -194,7 +195,7 @@ describe OrderCycleForm do
       before do
         params.merge!(
           outgoing_exchanges: [outgoing_exchange_params],
-          shipping_method_ids: [other_distributor_shipping_method.id]
+          preferred_shipping_method_ids: [other_distributor_shipping_method.id]
         )
       end
 
@@ -209,11 +210,13 @@ describe OrderCycleForm do
     context "when shipping methods already exist
              and doing an update without the :shipping_methods_id parameter" do
       it "doesn't return a validation error on shipping methods" do
-        order_cycle = create(:distributor_order_cycle, with_distributor_and_shipping_method: true)
+        distributor = create(:distributor_enterprise)
+        shipping_method = create(:shipping_method, distributors: [distributor])
+        order_cycle = create(:distributor_order_cycle, distributors: [distributor])
 
         form = OrderCycleForm.new(
           order_cycle,
-          params.except(:shipping_method_ids),
+          params.except(:preferred_shipping_method_ids),
           order_cycle.coordinator
         )
 
@@ -229,7 +232,7 @@ describe OrderCycleForm do
           order_cycle = create(:distributor_order_cycle, distributors: [distributor])
 
           form = OrderCycleForm.new(order_cycle,
-                                    { shipping_method_ids: [shipping_method.id] },
+                                    { preferred_shipping_method_ids: [shipping_method.id] },
                                     order_cycle.coordinator)
 
           expect(form.save).to be true
@@ -239,11 +242,15 @@ describe OrderCycleForm do
 
       context "and it's invalid" do
         it "returns a validation error" do
-          distributor = create(:distributor_enterprise)
-          order_cycle = create(:distributor_order_cycle, distributors: [distributor])
+          distributor_i = create(:distributor_enterprise)
+          distributor_ii = create(:distributor_enterprise)
+          shipping_method_i = create(:shipping_method, distributors: [distributor_i])
+          shipping_method_ii = create(:shipping_method, distributors: [distributor_ii])
+          order_cycle = create(:distributor_order_cycle,
+                               distributors: [distributor_i, distributor_ii])
 
           form = OrderCycleForm.new(order_cycle,
-                                    { shipping_method_ids: [] },
+                                    { preferred_shipping_method_ids: [shipping_method_i.id] },
                                     order_cycle.coordinator)
 
           expect(form.save).to be false