From 57ca1d54bb8509e80b33b26b8f5072cd7a200f73 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 12 Jan 2020 12:47:26 +0100
Subject: [PATCH 1/6] Fix issue with each_serializer not being called in some
 cases in Rails 4.

---
 app/controllers/spree/admin/base_controller.rb | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/app/controllers/spree/admin/base_controller.rb b/app/controllers/spree/admin/base_controller.rb
index c1c0179133..67a44ad093 100644
--- a/app/controllers/spree/admin/base_controller.rb
+++ b/app/controllers/spree/admin/base_controller.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Metrics/ClassLength
 module Spree
   module Admin
     class BaseController < Spree::BaseController
@@ -121,13 +122,17 @@ module Spree
 
       def render_as_json(data, options = {})
         ams_prefix = options.delete :ams_prefix
-        if [Array, ActiveRecord::Relation].include? data.class
+        if each_serializer_required?(data)
           render options.merge(json: data, each_serializer: serializer(ams_prefix))
         else
           render options.merge(json: data, serializer: serializer(ams_prefix))
         end
       end
 
+      def each_serializer_required?(data)
+        ['Array', 'ActiveRecord::Relation'].include?(data.class.name)
+      end
+
       def serializer(ams_prefix)
         unless ams_prefix.nil? || ams_prefix_whitelist.include?(ams_prefix.to_sym)
           raise "Suffix '#{ams_prefix}' not found in ams_prefix_whitelist for #{self.class.name}."
@@ -140,3 +145,4 @@ module Spree
     end
   end
 end
+# rubocop:enable Metrics/ClassLength

From 59ebfb9bd4664c838d9d0531f3153d96710bd861 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 12 Jan 2020 12:51:49 +0100
Subject: [PATCH 2/6] Fix subquery errors triggered by
 #warn_invalid_order_cycles

---
 app/controllers/spree/admin/base_controller.rb | 2 +-
 app/models/enterprise.rb                       | 2 +-
 app/models/order_cycle.rb                      | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/spree/admin/base_controller.rb b/app/controllers/spree/admin/base_controller.rb
index 67a44ad093..7591e1b3ce 100644
--- a/app/controllers/spree/admin/base_controller.rb
+++ b/app/controllers/spree/admin/base_controller.rb
@@ -97,7 +97,7 @@ module Spree
       def active_distributors_not_ready_for_checkout
         ocs = OrderCycle.managed_by(spree_current_user).active
         distributors = ocs.includes(:distributors).map(&:distributors).flatten.uniq
-        Enterprise.where('enterprises.id IN (?)', distributors).not_ready_for_checkout
+        Enterprise.where(id: distributors.map(&:id)).not_ready_for_checkout
       end
 
       def active_distributors_not_ready_for_checkout_message(distributors)
diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index ff95102781..381613aaa0 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -115,7 +115,7 @@ class Enterprise < ActiveRecord::Base
   scope :not_ready_for_checkout, lambda {
     # When ready_for_checkout is empty, return all rows when there are no enterprises ready for
     # checkout.
-    ready_enterprises = Enterprise.ready_for_checkout.select('enterprises.id')
+    ready_enterprises = Enterprise.ready_for_checkout.pluck(:id)
     if ready_enterprises.present?
       where("enterprises.id NOT IN (?)", ready_enterprises)
     else
diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb
index b26625fe68..9a30c493e5 100644
--- a/app/models/order_cycle.rb
+++ b/app/models/order_cycle.rb
@@ -63,7 +63,7 @@ class OrderCycle < ActiveRecord::Base
     if user.has_spree_role?('admin')
       scoped
     else
-      where('coordinator_id IN (?)', user.enterprises.map(&:id))
+      where(coordinator_id: user.enterprises)
     end
   }
 

From a02c58e23101ffc7d0aec7792e387be9b04edcda Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sat, 21 Dec 2019 19:51:52 +0100
Subject: [PATCH 3/6] Add join_table to enterprise groups relation

has_and_belongs_to_many relationships now require a join_table
---
 app/models/enterprise.rb       | 3 ++-
 app/models/enterprise_group.rb | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index 381613aaa0..280b10b4aa 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -25,7 +25,8 @@ class Enterprise < ActiveRecord::Base
   has_many :relationships_as_child, class_name: 'EnterpriseRelationship',
                                     foreign_key: 'child_id',
                                     dependent: :destroy
-  has_and_belongs_to_many :groups, class_name: 'EnterpriseGroup'
+  has_and_belongs_to_many :groups, join_table: 'enterprise_groups_enterprises',
+                                   class_name: 'EnterpriseGroup'
   has_many :producer_properties, foreign_key: 'producer_id'
   has_many :properties, through: :producer_properties
   has_many :supplied_products, class_name: 'Spree::Product',
diff --git a/app/models/enterprise_group.rb b/app/models/enterprise_group.rb
index 7ffc9d3fb7..4d8ff217f2 100644
--- a/app/models/enterprise_group.rb
+++ b/app/models/enterprise_group.rb
@@ -5,7 +5,7 @@ class EnterpriseGroup < ActiveRecord::Base
   include PermalinkGenerator
   acts_as_list
 
-  has_and_belongs_to_many :enterprises
+  has_and_belongs_to_many :enterprises, join_table: 'enterprise_groups_enterprises'
   belongs_to :owner, class_name: 'Spree::User', foreign_key: :owner_id, inverse_of: :owned_groups
   belongs_to :address, class_name: 'Spree::Address'
   accepts_nested_attributes_for :address

From 5f3abbf00ec10bc1d38bdbbe2e7ad6283a53b1e2 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 13 Jan 2020 11:27:26 +0100
Subject: [PATCH 4/6] Refactor BaseController

---
 .../spree/admin/base_controller.rb            | 27 ++------------
 app/services/order_cycle_warning.rb           | 35 +++++++++++++++++++
 .../spree/admin/base_controller_spec.rb       | 25 -------------
 spec/services/order_cycle_warning_spec.rb     | 31 ++++++++++++++++
 4 files changed, 69 insertions(+), 49 deletions(-)
 create mode 100644 app/services/order_cycle_warning.rb
 create mode 100644 spec/services/order_cycle_warning_spec.rb

diff --git a/app/controllers/spree/admin/base_controller.rb b/app/controllers/spree/admin/base_controller.rb
index 7591e1b3ce..73a18752aa 100644
--- a/app/controllers/spree/admin/base_controller.rb
+++ b/app/controllers/spree/admin/base_controller.rb
@@ -1,4 +1,3 @@
-# rubocop:disable Metrics/ClassLength
 module Spree
   module Admin
     class BaseController < Spree::BaseController
@@ -16,11 +15,10 @@ module Spree
       # Warn the user when they have an active order cycle with hubs that are not ready
       # for checkout (ie. does not have valid shipping and payment methods).
       def warn_invalid_order_cycles
-        distributors = active_distributors_not_ready_for_checkout
+        return if flash[:notice].present?
 
-        return if distributors.empty? || flash[:notice].present?
-
-        flash[:notice] = active_distributors_not_ready_for_checkout_message(distributors)
+        warning = OrderCycleWarning.new(spree_current_user).call
+        flash[:notice] = warning if warning.present?
       end
 
       # This is in Spree::Core::ControllerHelpers::Auth
@@ -94,24 +92,6 @@ module Spree
 
       private
 
-      def active_distributors_not_ready_for_checkout
-        ocs = OrderCycle.managed_by(spree_current_user).active
-        distributors = ocs.includes(:distributors).map(&:distributors).flatten.uniq
-        Enterprise.where(id: distributors.map(&:id)).not_ready_for_checkout
-      end
-
-      def active_distributors_not_ready_for_checkout_message(distributors)
-        distributor_names = distributors.map(&:name).join ', '
-
-        if distributors.count > 1
-          I18n.t(:active_distributors_not_ready_for_checkout_message_plural,
-                 distributor_names: distributor_names)
-        else
-          I18n.t(:active_distributors_not_ready_for_checkout_message_singular,
-                 distributor_names: distributor_names)
-        end
-      end
-
       def html_request?
         request.format.html?
       end
@@ -145,4 +125,3 @@ module Spree
     end
   end
 end
-# rubocop:enable Metrics/ClassLength
diff --git a/app/services/order_cycle_warning.rb b/app/services/order_cycle_warning.rb
new file mode 100644
index 0000000000..ffe3f9da21
--- /dev/null
+++ b/app/services/order_cycle_warning.rb
@@ -0,0 +1,35 @@
+class OrderCycleWarning
+  def initialize(current_user)
+    @current_user = current_user
+  end
+
+  def call
+    distributors = active_distributors_not_ready_for_checkout
+
+    return if distributors.empty?
+
+    active_distributors_not_ready_for_checkout_message(distributors)
+  end
+
+  private
+
+  attr_reader :current_user
+
+  def active_distributors_not_ready_for_checkout
+    ocs = OrderCycle.managed_by(current_user).active
+    distributors = ocs.includes(:distributors).map(&:distributors).flatten.uniq
+    Enterprise.where(id: distributors.map(&:id)).not_ready_for_checkout
+  end
+
+  def active_distributors_not_ready_for_checkout_message(distributors)
+    distributor_names = distributors.map(&:name).join ', '
+
+    if distributors.count > 1
+      I18n.t(:active_distributors_not_ready_for_checkout_message_plural,
+             distributor_names: distributor_names)
+    else
+      I18n.t(:active_distributors_not_ready_for_checkout_message_singular,
+             distributor_names: distributor_names)
+    end
+  end
+end
diff --git a/spec/controllers/spree/admin/base_controller_spec.rb b/spec/controllers/spree/admin/base_controller_spec.rb
index fa6899781d..d5b1015635 100644
--- a/spec/controllers/spree/admin/base_controller_spec.rb
+++ b/spec/controllers/spree/admin/base_controller_spec.rb
@@ -13,31 +13,6 @@ describe Spree::Admin::BaseController, type: :controller do
     expect(response).to redirect_to root_path(anchor: "login?after_login=/spree/admin/base")
   end
 
-  describe "displaying error messages for active distributors not ready for checkout" do
-    it "generates an error message when there is one distributor" do
-      distributor = double(:distributor, name: 'My Hub')
-      expect(controller.
-        send(:active_distributors_not_ready_for_checkout_message, [distributor])).
-        to eq(
-          "The hub My Hub is listed in an active order cycle, " \
-          "but does not have valid shipping and payment methods. " \
-          "Until you set these up, customers will not be able to shop at this hub."
-        )
-    end
-
-    it "generates an error message when there are several distributors" do
-      d1 = double(:distributor, name: 'Hub One')
-      d2 = double(:distributor, name: 'Hub Two')
-      expect(controller.
-        send(:active_distributors_not_ready_for_checkout_message, [d1, d2])).
-        to eq(
-          "The hubs Hub One, Hub Two are listed in an active order cycle, " \
-          "but do not have valid shipping and payment methods. " \
-          "Until you set these up, customers will not be able to shop at these hubs."
-        )
-    end
-  end
-
   describe "rendering as json ActiveModelSerializer" do
     context "when data is an object" do
       let(:data) { { attr: 'value' } }
diff --git a/spec/services/order_cycle_warning_spec.rb b/spec/services/order_cycle_warning_spec.rb
new file mode 100644
index 0000000000..e2c8457e4b
--- /dev/null
+++ b/spec/services/order_cycle_warning_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe OrderCycleWarning do
+  let(:user) { create(:user) }
+  let(:subject) { OrderCycleWarning }
+  let!(:distributor) { create(:enterprise, owner: user) }
+  let!(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) }
+
+  describe "checking if user's managed order cycles have distributors not ready for checkout" do
+    context "with an invalid distributor" do
+      it "returns a warning message" do
+        expect(subject.new(user).call).to eq(
+          I18n.t(:active_distributors_not_ready_for_checkout_message_singular,
+                 distributor_names: distributor.name)
+        )
+      end
+    end
+
+    context "with a valid distributor" do
+      let!(:distributor) {
+        create(:distributor_enterprise,
+               shipping_methods: [create(:shipping_method)],
+               payment_methods: [create(:payment_method)])
+      }
+
+      it "returns nil" do
+        expect(subject.new(user).call).to eq nil
+      end
+    end
+  end
+end

From 285c78a5e492d1a83f37485d374a7c39f75248df Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 4 Feb 2020 00:57:50 +0100
Subject: [PATCH 5/6] Remove use of #pluck and ensure subquery does not include
 all columns

---
 app/models/enterprise.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index 280b10b4aa..1deee05a40 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -116,7 +116,10 @@ class Enterprise < ActiveRecord::Base
   scope :not_ready_for_checkout, lambda {
     # When ready_for_checkout is empty, return all rows when there are no enterprises ready for
     # checkout.
-    ready_enterprises = Enterprise.ready_for_checkout.pluck(:id)
+    ready_enterprises = Enterprise.ready_for_checkout.
+      except(:select).
+      select('DISTINCT enterprises.id')
+
     if ready_enterprises.present?
       where("enterprises.id NOT IN (?)", ready_enterprises)
     else

From 7d71f217536acf2b65d6bdce85bfe11ea53267f3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 4 Feb 2020 11:11:24 +0100
Subject: [PATCH 6/6] Add frozen_string_literal comment to new class

---
 app/services/order_cycle_warning.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/services/order_cycle_warning.rb b/app/services/order_cycle_warning.rb
index ffe3f9da21..5f8b532f68 100644
--- a/app/services/order_cycle_warning.rb
+++ b/app/services/order_cycle_warning.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class OrderCycleWarning
   def initialize(current_user)
     @current_user = current_user