From ca5ecc4696d110ec949f1945e68dc193bcb919dc Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 7 Aug 2019 17:45:07 +0100
Subject: [PATCH 01/14] Refactor "active outgoing variants"

---
 .../order_cycle_permissions.rb                | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index 73264ce414..cb907fcc07 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -182,11 +182,7 @@ module OpenFoodNetwork
         hub_variants = Spree::Variant.joins(:product).where('spree_products.supplier_id = (?)', hub)
 
         # PLUS variants that are already in an outgoing exchange of this hub, so things don't break
-        # TODO: Remove this when all P-OC are sorted out
-        active_variants = []
-        @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|
-          active_variants = exchange.variants
-        end
+        active_variants = active_outgoing_variants(hub)
 
         Spree::Variant.
           where(id: coordinator_variants | hub_variants | permitted_variants | active_variants)
@@ -234,11 +230,7 @@ module OpenFoodNetwork
         hub_variants = Spree::Variant.joins(:product).where('spree_products.supplier_id = (?)', hub)
 
         # PLUS variants that are already in an outgoing exchange of this hub, so things don't break
-        # TODO: Remove this when all P-OC are sorted out
-        active_variants = []
-        @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|
-          active_variants = exchange.variants
-        end
+        active_variants = active_outgoing_variants(hub)
 
         Spree::Variant.
           where(id: coordinator_variants | hub_variants | permitted_variants | active_variants)
@@ -262,6 +254,14 @@ module OpenFoodNetwork
 
     private
 
+    def active_outgoing_variants(hub)
+      active_variants = []
+      @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|
+        active_variants = exchange.variants
+      end
+      active_variants
+    end
+
     def user_manages_coordinator_or(enterprise)
       managed_enterprises.pluck(:id).include?(@coordinator.id) ||
         managed_enterprises.pluck(:id).include?(enterprise.id)

From dc540444a22bfe28aa01dd9df89392069da0f0b5 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 7 Aug 2019 18:41:17 +0100
Subject: [PATCH 02/14] Remove pointless code

---
 .../order_cycle_permissions.rb                | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index cb907fcc07..bff84a1477 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -164,14 +164,6 @@ module OpenFoodNetwork
       if user_manages_coordinator_or(hub)
         # TODO: Use variants_stockable_by(hub) for this?
 
-        # Any variants produced by the coordinator, for outgoing exchanges with itself
-        # TODO: isn't this completely redundant given the assignment of hub_variants below?
-        coordinator_variants = []
-        if hub == @coordinator
-          coordinator_variants = Spree::Variant.joins(:product).
-            where('spree_products.supplier_id = (?)', @coordinator)
-        end
-
         # Any variants of any producers that have granted the hub P-OC
         producer_ids = related_enterprises_granting(:add_to_order_cycle,
                                                     to: [hub.id],
@@ -185,7 +177,7 @@ module OpenFoodNetwork
         active_variants = active_outgoing_variants(hub)
 
         Spree::Variant.
-          where(id: coordinator_variants | hub_variants | permitted_variants | active_variants)
+          where(id: hub_variants | permitted_variants | active_variants)
       else
         # Variants produced by MY PRODUCERS that are in this OC,
         #   where my producer has granted P-OC to the hub
@@ -213,13 +205,6 @@ module OpenFoodNetwork
       return Spree::Variant.where("1=0") unless @order_cycle
 
       if user_manages_coordinator_or(hub)
-        # Any variants produced by the coordinator, for outgoing exchanges with itself
-        coordinator_variants = []
-        if hub == @coordinator
-          coordinator_variants = Spree::Variant.joins(:product).
-            where('spree_products.supplier_id = (?)', @coordinator)
-        end
-
         # Any variants of any producers that have granted the hub P-OC
         producer_ids = related_enterprises_granting(:add_to_order_cycle,
                                                     to: [hub.id],
@@ -233,7 +218,7 @@ module OpenFoodNetwork
         active_variants = active_outgoing_variants(hub)
 
         Spree::Variant.
-          where(id: coordinator_variants | hub_variants | permitted_variants | active_variants)
+          where(id: hub_variants | permitted_variants | active_variants)
       else
         # Any of my managed producers in this order cycle granted P-OC by the hub
         granted_producers = related_enterprises_granted(:add_to_order_cycle,

From 2a5403e23f92801c09d4f3d547c83b47935be765 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 7 Aug 2019 19:39:51 +0100
Subject: [PATCH 03/14] Clean up "visible and editable" variants

---
 .../order_cycle_permissions.rb                | 52 ++++++++-----------
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index bff84a1477..bfd3631120 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -162,22 +162,7 @@ module OpenFoodNetwork
       return Spree::Variant.where("1=0") unless @order_cycle
 
       if user_manages_coordinator_or(hub)
-        # TODO: Use variants_stockable_by(hub) for this?
-
-        # Any variants of any producers that have granted the hub P-OC
-        producer_ids = related_enterprises_granting(:add_to_order_cycle,
-                                                    to: [hub.id],
-                                                    scope: Enterprise.is_primary_producer)
-        permitted_variants = Spree::Variant.joins(:product).
-          where('spree_products.supplier_id IN (?)', producer_ids)
-
-        hub_variants = Spree::Variant.joins(:product).where('spree_products.supplier_id = (?)', hub)
-
-        # PLUS variants that are already in an outgoing exchange of this hub, so things don't break
-        active_variants = active_outgoing_variants(hub)
-
-        Spree::Variant.
-          where(id: hub_variants | permitted_variants | active_variants)
+        visible_and_editable_variants(hub)
       else
         # Variants produced by MY PRODUCERS that are in this OC,
         #   where my producer has granted P-OC to the hub
@@ -205,20 +190,7 @@ module OpenFoodNetwork
       return Spree::Variant.where("1=0") unless @order_cycle
 
       if user_manages_coordinator_or(hub)
-        # Any variants of any producers that have granted the hub P-OC
-        producer_ids = related_enterprises_granting(:add_to_order_cycle,
-                                                    to: [hub.id],
-                                                    scope: Enterprise.is_primary_producer)
-        permitted_variants = Spree::Variant.joins(:product).
-          where('spree_products.supplier_id IN (?)', producer_ids)
-
-        hub_variants = Spree::Variant.joins(:product).where('spree_products.supplier_id = (?)', hub)
-
-        # PLUS variants that are already in an outgoing exchange of this hub, so things don't break
-        active_variants = active_outgoing_variants(hub)
-
-        Spree::Variant.
-          where(id: hub_variants | permitted_variants | active_variants)
+        visible_and_editable_variants(hub)
       else
         # Any of my managed producers in this order cycle granted P-OC by the hub
         granted_producers = related_enterprises_granted(:add_to_order_cycle,
@@ -239,6 +211,26 @@ module OpenFoodNetwork
 
     private
 
+    def visible_and_editable_variants(hub)
+      # Producers that have granted the hub P-OC
+      producer_ids = related_enterprises_granting(:add_to_order_cycle,
+                                                  to: [hub.id],
+                                                  scope: Enterprise.is_primary_producer)
+
+      # Variants from Producers via permissions, and from the hub itself
+      available_variants = variants_from_suppliers(producer_ids.push(hub.id))
+
+      # PLUS variants that are already in an outgoing exchange of this hub, so things don't break
+      active_variants = active_outgoing_variants(hub)
+
+      Spree::Variant.where(id: available_variants | active_variants)
+    end
+
+    def variants_from_suppliers(supplier_ids)
+      Spree::Variant.joins(:product).
+        where('spree_products.supplier_id IN (?)', supplier_ids)
+    end
+
     def active_outgoing_variants(hub)
       active_variants = []
       @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|

From 170bc94d92b2c85eebc4f02b053d14735eb94991 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 7 Aug 2019 21:49:24 +0100
Subject: [PATCH 04/14] Refactor order_cycle_serializer

---
 .../api/admin/order_cycle_serializer.rb       | 27 ++++++++++---------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/app/serializers/api/admin/order_cycle_serializer.rb b/app/serializers/api/admin/order_cycle_serializer.rb
index a782c26048..bd8c6407f1 100644
--- a/app/serializers/api/admin/order_cycle_serializer.rb
+++ b/app/serializers/api/admin/order_cycle_serializer.rb
@@ -25,10 +25,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
   end
 
   def exchanges
-    scoped_exchanges =
-      OpenFoodNetwork::OrderCyclePermissions.
-        new(options[:current_user], object).
-        visible_exchanges.by_enterprise_name
+    scoped_exchanges = permissions.visible_exchanges.by_enterprise_name
 
     ActiveModel::ArraySerializer.
       new(scoped_exchanges, each_serializer: Api::Admin::ExchangeSerializer,
@@ -39,9 +36,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
     # For each enterprise that the current user is able to see in this order cycle,
     # work out which variants should be editable within incoming exchanges from that enterprise
     editable = {}
-    permissions = OpenFoodNetwork::OrderCyclePermissions.new(options[:current_user], object)
-    enterprises = permissions.visible_enterprises
-    enterprises.each do |enterprise|
+    visible_enterprises.each do |enterprise|
       variants = permissions.editable_variants_for_incoming_exchanges_from(enterprise).pluck(:id)
       editable[enterprise.id] = variants if variants.any?
     end
@@ -52,9 +47,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
     # For each enterprise that the current user is able to see in this order cycle,
     # work out which variants should be editable within incoming exchanges from that enterprise
     editable = {}
-    permissions = OpenFoodNetwork::OrderCyclePermissions.new(options[:current_user], object)
-    enterprises = permissions.visible_enterprises
-    enterprises.each do |enterprise|
+    visible_enterprises.each do |enterprise|
       variants = permissions.editable_variants_for_outgoing_exchanges_to(enterprise).pluck(:id)
       editable[enterprise.id] = variants if variants.any?
     end
@@ -65,9 +58,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
     # For each enterprise that the current user is able to see in this order cycle,
     # work out which variants should be visible within outgoing exchanges from that enterprise
     visible = {}
-    permissions = OpenFoodNetwork::OrderCyclePermissions.new(options[:current_user], object)
-    enterprises = permissions.visible_enterprises
-    enterprises.each do |enterprise|
+    visible_enterprises.each do |enterprise|
       # This is hopefully a temporary measure, pending the arrival of multiple named inventories
       # for shops. We need this here to allow hubs to restrict visible variants to only those in
       # their inventory if they so choose
@@ -84,4 +75,14 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
     end
     visible
   end
+
+  private
+
+  def permissions
+    @permissions ||= OpenFoodNetwork::OrderCyclePermissions.new(options[:current_user], object)
+  end
+
+  def visible_enterprises
+    @visible_enterprises ||= permissions.visible_enterprises
+  end
 end

From c1366fced5984b6a44ef513ac10e7754c898e050 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 7 Aug 2019 22:01:48 +0100
Subject: [PATCH 05/14] DRY some more variant queries

---
 .../order_cycle_permissions.rb                | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index bfd3631120..02100aa77e 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -76,10 +76,9 @@ module OpenFoodNetwork
 
           # TODO: Remove this when all P-OC are sorted out
           # Hubs that currently have outgoing exchanges distributing variants of producers I manage
-          variants = Spree::Variant.
-            joins(:product).
-            where("spree_products.supplier_id IN (?)",
-                  managed_enterprises.is_primary_producer.select("enterprises.id"))
+          variants = variants_from_suppliers(
+            managed_enterprises.is_primary_producer.select("enterprises.id")
+          )
 
           active_exchanges = @order_cycle.
             exchanges.outgoing.with_any_variant(variants.select("spree_variants.id"))
@@ -169,8 +168,7 @@ module OpenFoodNetwork
         producer_ids = related_enterprises_granting(:add_to_order_cycle,
                                                     to: [hub.id],
                                                     scope: managed_participating_producers)
-        permitted_variants = Spree::Variant.joins(:product).
-          where('spree_products.supplier_id IN (?)', producer_ids)
+        permitted_variants = variants_from_suppliers(producer_ids)
 
         # PLUS my incoming producers' variants that are already in an outgoing exchange of this hub,
         #   so things don't break. TODO: Remove this when all P-OC are sorted out
@@ -202,8 +200,7 @@ module OpenFoodNetwork
         granting_producer_ids = related_enterprises_granting(:add_to_order_cycle,
                                                              to: [hub.id],
                                                              scope: granted_producers)
-        permitted_variants = Spree::Variant.joins(:product).
-          where('spree_products.supplier_id IN (?)', granting_producer_ids)
+        permitted_variants = variants_from_suppliers(granting_producer_ids)
 
         Spree::Variant.where(id: permitted_variants)
       end
@@ -227,8 +224,7 @@ module OpenFoodNetwork
     end
 
     def variants_from_suppliers(supplier_ids)
-      Spree::Variant.joins(:product).
-        where('spree_products.supplier_id IN (?)', supplier_ids)
+      Spree::Variant.joins(:product).where('spree_products.supplier_id IN (?)', supplier_ids)
     end
 
     def active_outgoing_variants(hub)
@@ -314,8 +310,7 @@ module OpenFoodNetwork
       # active_exchanges is for backward compatability, before we restricted variants in each
       # outgoing exchange to those where the producer had granted P-OC to the distributor
       # For any of my managed producers, any outgoing exchanges with their variants
-      variants = Spree::Variant.
-        joins(:product).where('spree_products.supplier_id IN (?)', producer_ids)
+      variants = variants_from_suppliers(producer_ids)
       active_exchange_ids = @order_cycle.
         exchanges.outgoing.with_any_variant(variants.select("spree_variants.id")).pluck :id
 

From c2823b3ffea54f1c638d50f84666d3af3e432c18 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 7 Aug 2019 22:47:14 +0100
Subject: [PATCH 06/14] Memoize #user_manages_coordinator_or result

---
 lib/open_food_network/order_cycle_permissions.rb | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index 02100aa77e..78bdce432e 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -236,8 +236,12 @@ module OpenFoodNetwork
     end
 
     def user_manages_coordinator_or(enterprise)
-      managed_enterprises.pluck(:id).include?(@coordinator.id) ||
-        managed_enterprises.pluck(:id).include?(enterprise.id)
+      managed_enterprise_ids.include?(@coordinator.id) ||
+        managed_enterprise_ids.include?(enterprise.id)
+    end
+
+    def managed_enterprise_ids
+      @managed_enterprise_ids ||= managed_enterprises.pluck(:id)
     end
 
     def managed_participating_enterprises

From f4d71ae352977b21f39a506297b6bd9791cc91c0 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 9 Aug 2019 09:31:27 +0100
Subject: [PATCH 07/14] Fix order_cycle_permissions tests not running locally

---
 spec/lib/open_food_network/order_cycle_permissions_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/lib/open_food_network/order_cycle_permissions_spec.rb b/spec/lib/open_food_network/order_cycle_permissions_spec.rb
index d9299995df..930239f410 100644
--- a/spec/lib/open_food_network/order_cycle_permissions_spec.rb
+++ b/spec/lib/open_food_network/order_cycle_permissions_spec.rb
@@ -1,3 +1,4 @@
+require 'spec_helper'
 require 'open_food_network/order_cycle_permissions'
 
 module OpenFoodNetwork

From e73f2d682c746858983877aef65cda3d9837dca9 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 9 Aug 2019 09:32:18 +0100
Subject: [PATCH 08/14] Remove N+1 in editable_variants_for_incoming_exchanges

---
 .../api/admin/order_cycle_serializer.rb       | 23 ++++++++++++-------
 .../order_cycle_permissions.rb                |  9 ++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/app/serializers/api/admin/order_cycle_serializer.rb b/app/serializers/api/admin/order_cycle_serializer.rb
index bd8c6407f1..7e165cf547 100644
--- a/app/serializers/api/admin/order_cycle_serializer.rb
+++ b/app/serializers/api/admin/order_cycle_serializer.rb
@@ -33,14 +33,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
   end
 
   def editable_variants_for_incoming_exchanges
-    # For each enterprise that the current user is able to see in this order cycle,
-    # work out which variants should be editable within incoming exchanges from that enterprise
-    editable = {}
-    visible_enterprises.each do |enterprise|
-      variants = permissions.editable_variants_for_incoming_exchanges_from(enterprise).pluck(:id)
-      editable[enterprise.id] = variants if variants.any?
-    end
-    editable
+    sort_by_supplier_id(permissions.all_incoming_editable_variants.all)
   end
 
   def editable_variants_for_outgoing_exchanges
@@ -85,4 +78,18 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
   def visible_enterprises
     @visible_enterprises ||= permissions.visible_enterprises
   end
+
+  def sort_by_supplier_id(variants)
+    collection = {}
+    variants.map do |variant|
+      supplier_id = variant.product.supplier_id
+
+      if collection.key? supplier_id
+        collection[supplier_id] << variant.id
+      else
+        collection[supplier_id] = [variant.id]
+      end
+    end
+    collection
+  end
 end
diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index 78bdce432e..21c88a25eb 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -154,6 +154,15 @@ module OpenFoodNetwork
       end
     end
 
+    def all_incoming_editable_variants
+      valid_suppliers = visible_enterprises.map do |enterprise|
+        enterprise.id if user_manages_coordinator_or(enterprise)
+      end
+
+      Spree::Variant.includes(product: :supplier).
+        joins(:product).where('spree_products.supplier_id IN (?)', valid_suppliers)
+    end
+
     # Find the variants that a user is permitted see within outgoing exchanges
     # Note that this does not determine whether they actually appear in outgoing exchanges
     # as this requires first that the variant is included in an incoming exchange

From 77105e265e2e5f7626bd305492aa848e73ab837a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 9 Aug 2019 09:58:09 +0100
Subject: [PATCH 09/14] Memoize more repeated queries in #managed_enterprises

---
 lib/open_food_network/order_cycle_permissions.rb | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index 21c88a25eb..261038a247 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -14,7 +14,7 @@ module OpenFoodNetwork
     def visible_enterprises
       return Enterprise.where("1=0") if @coordinator.blank?
 
-      if managed_enterprises.include? @coordinator
+      if managed_enterprise_ids.include? @coordinator.id
         coordinator_permitted_ids = [@coordinator]
         all_active_ids = []
 
@@ -76,9 +76,7 @@ module OpenFoodNetwork
 
           # TODO: Remove this when all P-OC are sorted out
           # Hubs that currently have outgoing exchanges distributing variants of producers I manage
-          variants = variants_from_suppliers(
-            managed_enterprises.is_primary_producer.select("enterprises.id")
-          )
+          variants = variants_from_suppliers(managed_producer_ids)
 
           active_exchanges = @order_cycle.
             exchanges.outgoing.with_any_variant(variants.select("spree_variants.id"))
@@ -186,7 +184,7 @@ module OpenFoodNetwork
                   AND spree_products.supplier_id IN (?)
                   AND incoming = 'f'",
                 hub.id,
-                managed_enterprises.is_primary_producer.select("enterprises.id"))
+                managed_producer_ids)
 
         Spree::Variant.where(id: permitted_variants | active_variants)
       end
@@ -253,6 +251,10 @@ module OpenFoodNetwork
       @managed_enterprise_ids ||= managed_enterprises.pluck(:id)
     end
 
+    def managed_producer_ids
+      @managed_producer_ids ||= managed_enterprises.is_primary_producer.pluck(:id)
+    end
+
     def managed_participating_enterprises
       return @managed_participating_enterprises unless @managed_participating_enterprises.nil?
 
@@ -274,7 +276,7 @@ module OpenFoodNetwork
 
     def order_cycle_exchange_ids_involving_my_enterprises
       # Any exchanges that my managed enterprises are involved in directly
-      @order_cycle.exchanges.involving(managed_enterprises.select("enterprises.id")).pluck :id
+      @order_cycle.exchanges.involving(managed_enterprise_ids).pluck :id
     end
 
     def order_cycle_exchange_ids_with_distributable_variants

From 9e09a3b37911fec38aa4e623612043f1e5768ac8 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 9 Aug 2019 10:41:09 +0100
Subject: [PATCH 10/14] Memoize #active_outgoing_variants result

---
 lib/open_food_network/order_cycle_permissions.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index 261038a247..cf149eca8a 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -235,11 +235,13 @@ module OpenFoodNetwork
     end
 
     def active_outgoing_variants(hub)
-      active_variants = []
-      @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|
-        active_variants = exchange.variants
+      @active_outgoing_variants ||= begin
+        active_variants = []
+        @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|
+          active_variants = exchange.variants
+        end
+        active_variants
       end
-      active_variants
     end
 
     def user_manages_coordinator_or(enterprise)

From 35b68239b06e2225353ed6b57a81938c1ff4180b Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 22 Oct 2019 21:31:35 +0100
Subject: [PATCH 11/14] Tidy up AR query syntax

---
 lib/open_food_network/order_cycle_permissions.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index cf149eca8a..f72fcec198 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -158,7 +158,7 @@ module OpenFoodNetwork
       end
 
       Spree::Variant.includes(product: :supplier).
-        joins(:product).where('spree_products.supplier_id IN (?)', valid_suppliers)
+        joins(:product).where(spree_products: { supplier_id: valid_suppliers })
     end
 
     # Find the variants that a user is permitted see within outgoing exchanges
@@ -231,7 +231,7 @@ module OpenFoodNetwork
     end
 
     def variants_from_suppliers(supplier_ids)
-      Spree::Variant.joins(:product).where('spree_products.supplier_id IN (?)', supplier_ids)
+      Spree::Variant.joins(:product).where(spree_products: { supplier_id: supplier_ids })
     end
 
     def active_outgoing_variants(hub)
@@ -304,10 +304,10 @@ module OpenFoodNetwork
               @order_cycle).pluck(:id).uniq
 
       product_ids = Spree::Product.joins(:variants_including_master).
-        where("spree_variants.id IN (?)", variant_ids).pluck(:id).uniq
+        where(spree_variants: { id: variant_ids }).pluck(:id).uniq
 
       producer_ids = Enterprise.joins(:supplied_products).
-        where("spree_products.id IN (?)", product_ids).pluck(:id).uniq
+        where(spree_products: { id: product_ids }).pluck(:id).uniq
 
       active_exchange_ids = @order_cycle.exchanges.incoming.where(sender_id: producer_ids).pluck :id
 

From ab30307b1a340492f0684b24bc3d7d78e0d41ae0 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 22 Oct 2019 21:58:43 +0100
Subject: [PATCH 12/14] Simplify #active_outgoing_variants

---
 lib/open_food_network/order_cycle_permissions.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index f72fcec198..30bb031852 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -236,11 +236,7 @@ module OpenFoodNetwork
 
     def active_outgoing_variants(hub)
       @active_outgoing_variants ||= begin
-        active_variants = []
-        @order_cycle.exchanges.outgoing.where(receiver_id: hub).limit(1).each do |exchange|
-          active_variants = exchange.variants
-        end
-        active_variants
+        @order_cycle.exchanges.outgoing.where(receiver_id: hub).first.andand.variants || []
       end
     end
 

From a1146aed1b20d2636a7299601cd55b3b2da3a099 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 3 Nov 2019 14:37:38 +0000
Subject: [PATCH 13/14] Refactor #sort_by_supplier_id

---
 .../api/admin/order_cycle_serializer.rb         | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/app/serializers/api/admin/order_cycle_serializer.rb b/app/serializers/api/admin/order_cycle_serializer.rb
index 7e165cf547..8a0f66640a 100644
--- a/app/serializers/api/admin/order_cycle_serializer.rb
+++ b/app/serializers/api/admin/order_cycle_serializer.rb
@@ -33,7 +33,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
   end
 
   def editable_variants_for_incoming_exchanges
-    sort_by_supplier_id(permissions.all_incoming_editable_variants.all)
+    variant_ids_by_supplier_id(permissions.all_incoming_editable_variants.all)
   end
 
   def editable_variants_for_outgoing_exchanges
@@ -79,17 +79,10 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer
     @visible_enterprises ||= permissions.visible_enterprises
   end
 
-  def sort_by_supplier_id(variants)
-    collection = {}
-    variants.map do |variant|
-      supplier_id = variant.product.supplier_id
-
-      if collection.key? supplier_id
-        collection[supplier_id] << variant.id
-      else
-        collection[supplier_id] = [variant.id]
-      end
+  def variant_ids_by_supplier_id(variants)
+    grouped_by_supplier = variants.group_by(&:supplier_id)
+    grouped_by_supplier.each do |supplier_id, grouped_variants|
+      grouped_by_supplier[supplier_id] = grouped_variants.map(&:id)
     end
-    collection
   end
 end

From 1a5eea33036bb34f3a87a9fb3b26b490ec7f4dbf Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 3 Nov 2019 14:39:24 +0000
Subject: [PATCH 14/14] Refactor #all_incoming_editable_variants

---
 lib/open_food_network/order_cycle_permissions.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/open_food_network/order_cycle_permissions.rb b/lib/open_food_network/order_cycle_permissions.rb
index 30bb031852..a87964a835 100644
--- a/lib/open_food_network/order_cycle_permissions.rb
+++ b/lib/open_food_network/order_cycle_permissions.rb
@@ -153,11 +153,12 @@ module OpenFoodNetwork
     end
 
     def all_incoming_editable_variants
-      valid_suppliers = visible_enterprises.map do |enterprise|
-        enterprise.id if user_manages_coordinator_or(enterprise)
-      end
+      valid_suppliers = visible_enterprises.select do |enterprise|
+        user_manages_coordinator_or(enterprise)
+      end.map(&:id)
 
       Spree::Variant.includes(product: :supplier).
+        select("spree_variants.id, spree_products.supplier_id").
         joins(:product).where(spree_products: { supplier_id: valid_suppliers })
     end