From 5b27ed6b9f8a49dffefa794739084e01c262f67d Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 27 Sep 2019 01:52:29 +0100
Subject: [PATCH 1/9] Remove unnecessary #deleted? check

It should be included in te default product scope
---
 lib/open_food_network/products_renderer.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 350395ca40..4507839915 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -38,7 +38,7 @@ module OpenFoodNetwork
         order(taxon_order).
         each { |product| scoper.scope(product) }.
         select do |product|
-          !product.deleted? && product.has_stock_for_distribution?(@order_cycle, @distributor)
+          product.has_stock_for_distribution?(@order_cycle, @distributor)
         end
     end
 

From fe0b3172c7773a915e4eab17927277d3ef3345ef Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 27 Sep 2019 01:52:45 +0100
Subject: [PATCH 2/9] Move scoper to method

---
 lib/open_food_network/products_renderer.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 4507839915..6ce36e97c9 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -31,7 +31,6 @@ module OpenFoodNetwork
 
     def load_products
       return unless @order_cycle
-      scoper = ScopeProductToHub.new(@distributor)
 
       OrderCycleDistributedProducts.new(@order_cycle, @distributor).
         relation.
@@ -42,6 +41,10 @@ module OpenFoodNetwork
         end
     end
 
+    def scoper
+      ScopeProductToHub.new(@distributor)
+    end
+
     def taxon_order
       if @distributor.preferred_shopfront_taxon_order.present?
         @distributor

From cecebb82f4dc7a7dbc589a283e88a8479523175a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 27 Sep 2019 01:55:25 +0100
Subject: [PATCH 3/9] Move distributed products relation out from
 OrderCycleDistributedProducts

---
 .../order_cycle_distributed_products.rb       | 41 --------
 lib/open_food_network/products_renderer.rb    | 10 +-
 .../order_cycle_distributed_products_spec.rb  | 93 -------------------
 3 files changed, 8 insertions(+), 136 deletions(-)
 delete mode 100644 app/services/order_cycle_distributed_products.rb
 delete mode 100644 spec/services/order_cycle_distributed_products_spec.rb

diff --git a/app/services/order_cycle_distributed_products.rb b/app/services/order_cycle_distributed_products.rb
deleted file mode 100644
index c07ddd849f..0000000000
--- a/app/services/order_cycle_distributed_products.rb
+++ /dev/null
@@ -1,41 +0,0 @@
-# Finds valid products distributed by a particular distributor in an order cycle
-#
-# If a product without variants is added to an order cycle, and then some
-# variants are added to that product, but not the order cycle, then the master
-# variant should not available for customers to purchase. This class filters
-# out such products so that the customer cannot purchase them.
-class OrderCycleDistributedProducts
-  def initialize(order_cycle, distributor)
-    @order_cycle = order_cycle
-    @distributor = distributor
-  end
-
-  # Returns an ActiveRecord relation without invalid products. Check
-  # #valid_products_distributed_by for details
-  #
-  # @return [ActiveRecord::Relation<Spree::Product>]
-  def relation
-    variants = order_cycle.variants_distributed_by(distributor)
-    products = variants.map(&:product).uniq
-
-    valid_products = products.reject do |product|
-      product_has_only_obsolete_master_in_distribution?(product, variants)
-    end
-    product_ids = valid_products.map(&:id)
-
-    Spree::Product.where(id: product_ids)
-  end
-
-  private
-
-  attr_reader :order_cycle, :distributor
-
-  # If a product without variants is added to an order cycle, and then some variants are added
-  # to that product, but not the order cycle, then the master variant should not available for
-  # customers to purchase.
-  def product_has_only_obsolete_master_in_distribution?(product, distributed_variants)
-    product.has_variants? &&
-      distributed_variants.include?(product.master) &&
-      (product.variants & distributed_variants).empty?
-  end
-end
diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 6ce36e97c9..87d9052a35 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -32,8 +32,7 @@ module OpenFoodNetwork
     def load_products
       return unless @order_cycle
 
-      OrderCycleDistributedProducts.new(@order_cycle, @distributor).
-        relation.
+      Spree::Product.where(id: distributed_products).
         order(taxon_order).
         each { |product| scoper.scope(product) }.
         select do |product|
@@ -41,6 +40,13 @@ module OpenFoodNetwork
         end
     end
 
+    def distributed_products
+      @order_cycle.
+        variants_distributed_by(@distributor).
+        includes(:product).
+        select(:product_id)
+    end
+
     def scoper
       ScopeProductToHub.new(@distributor)
     end
diff --git a/spec/services/order_cycle_distributed_products_spec.rb b/spec/services/order_cycle_distributed_products_spec.rb
deleted file mode 100644
index 9670c179dd..0000000000
--- a/spec/services/order_cycle_distributed_products_spec.rb
+++ /dev/null
@@ -1,93 +0,0 @@
-require 'spec_helper'
-
-describe OrderCycleDistributedProducts do
-  let(:order_cycle) { OrderCycle.new }
-  let(:distributor) { instance_double(Enterprise) }
-
-  it 'returns valid products but not invalid products' do
-    valid_product = create(:product)
-    invalid_product = create(:product)
-    valid_variant = valid_product.variants.first
-
-    distributor = create(:distributor_enterprise)
-    order_cycle = create(
-      :simple_order_cycle,
-      distributors: [distributor],
-      variants: [valid_variant, invalid_product.master]
-    )
-
-    distributed_valid_products = described_class.new(order_cycle, distributor)
-
-    expect(distributed_valid_products.relation).to eq([valid_product])
-  end
-
-  context 'when the product has only an obsolete master variant in a distribution' do
-    let(:master) { create(:variant, product: product) }
-    let(:product) { create(:product, variants: [build(:variant)]) }
-    let(:unassociated_variant) { create(:variant) }
-    let(:distributed_variants) { [product.master, unassociated_variant] }
-
-    before do
-      allow(order_cycle)
-        .to receive(:variants_distributed_by).with(distributor) { distributed_variants }
-    end
-
-    it 'does not return the obsolete product' do
-      distributed_valid_products = described_class.new(order_cycle, distributor)
-      expect(distributed_valid_products.relation).to eq([unassociated_variant.product])
-    end
-  end
-
-  context "when the product doesn't have variants" do
-    let(:master) { build(:variant) }
-    let(:product) { create(:product, master: master) }
-    let(:distributed_variants) { [master] }
-
-    before do
-      allow(product).to receive(:has_variants?) { false }
-      allow(order_cycle)
-        .to receive(:variants_distributed_by).with(distributor) { distributed_variants }
-    end
-
-    it 'returns the product' do
-      distributed_valid_products = described_class.new(order_cycle, distributor)
-      expect(distributed_valid_products.relation).to eq([product])
-    end
-  end
-
-  context "when the master isn't distributed" do
-    let(:master) { build(:variant) }
-    let(:variant) { build(:variant) }
-    let(:product) { create(:product, master: master, variants: [variant]) }
-    let(:distributed_variants) { [variant] }
-
-    before do
-      allow(product).to receive(:has_variants?) { true }
-      allow(order_cycle)
-        .to receive(:variants_distributed_by).with(distributor) { distributed_variants }
-    end
-
-    it 'returns the product' do
-      distributed_valid_products = described_class.new(order_cycle, distributor)
-      expect(distributed_valid_products.relation).to eq([product])
-    end
-  end
-
-  context 'when the product has the master and other variants distributed' do
-    let(:master) { build(:variant) }
-    let(:variant) { build(:variant) }
-    let(:product) { create(:product, master: master, variants: [variant]) }
-    let(:distributed_variants) { [master, variant] }
-
-    before do
-      allow(product).to receive(:has_variants?) { true }
-      allow(order_cycle)
-        .to receive(:variants_distributed_by).with(distributor) { distributed_variants }
-    end
-
-    it 'returns the product' do
-      distributed_valid_products = described_class.new(order_cycle, distributor)
-      expect(distributed_valid_products.relation).to eq([product])
-    end
-  end
-end

From 535e389fb4a8f2aff8369f1fbbca15e9c4a5f711 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 27 Sep 2019 12:43:22 +0100
Subject: [PATCH 4/9] Query variant stock including overrides

This allows the results to be properly filtered and paginated whilst showing the correct stock, and removes a big N+1
---
 lib/open_food_network/products_renderer.rb | 55 +++++++++++++++++++---
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 87d9052a35..e6e3fbe460 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -34,17 +34,60 @@ module OpenFoodNetwork
 
       Spree::Product.where(id: distributed_products).
         order(taxon_order).
-        each { |product| scoper.scope(product) }.
-        select do |product|
-          product.has_stock_for_distribution?(@order_cycle, @distributor)
-        end
+        each { |product| scoper.scope(product) }
     end
 
     def distributed_products
       @order_cycle.
         variants_distributed_by(@distributor).
-        includes(:product).
-        select(:product_id)
+        merge(stocked_variants_with_overrides).
+        select("DISTINCT spree_variants.product_id")
+    end
+
+    def stocked_variants_with_overrides
+      Spree::Variant.
+        joins("LEFT OUTER JOIN variant_overrides ON variant_overrides.variant_id = spree_variants.id AND variant_overrides.hub_id = #{@distributor.id}").
+        joins(:stock_items).
+        where(query_stock_with_overrides)
+    end
+
+    def query_stock_with_overrides
+      "( #{variant_not_overriden} AND ( #{variant_in_stock} OR #{variant_on_demand} ) )
+        OR ( #{variant_overriden} AND ( #{override_on_demand} OR #{override_in_stock} ) )
+        OR ( #{variant_overriden} AND ( #{override_on_demand_null} AND #{variant_on_demand} ) )
+        OR ( #{variant_overriden} AND ( #{override_on_demand_null} AND #{variant_not_on_demand} AND #{variant_in_stock} ) )"
+    end
+
+    def variant_not_overriden
+      "variant_overrides.id IS NULL"
+    end
+
+    def variant_overriden
+      "variant_overrides.id IS NOT NULL"
+    end
+
+    def variant_in_stock
+      "spree_stock_items.count_on_hand > 0"
+    end
+
+    def variant_on_demand
+      "spree_stock_items.backorderable IS TRUE"
+    end
+
+    def variant_not_on_demand
+      "spree_stock_items.backorderable IS FALSE"
+    end
+
+    def override_on_demand
+      "variant_overrides.on_demand IS TRUE"
+    end
+
+    def override_in_stock
+      "variant_overrides.count_on_hand > 0"
+    end
+
+    def override_on_demand_null
+      "variant_overrides.on_demand IS NULL"
     end
 
     def scoper

From d5e90c3c6cfb3c4ac8f1b4dca42b1fd672e76f07 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 27 Sep 2019 16:35:40 +0100
Subject: [PATCH 5/9] Extract #load_products logic into a new service

---
 app/services/shop_products_service.rb       | 70 +++++++++++++++++++++
 lib/open_food_network/products_renderer.rb  | 55 +---------------
 spec/services/shop_products_service_spec.rb | 68 ++++++++++++++++++++
 3 files changed, 139 insertions(+), 54 deletions(-)
 create mode 100644 app/services/shop_products_service.rb
 create mode 100644 spec/services/shop_products_service_spec.rb

diff --git a/app/services/shop_products_service.rb b/app/services/shop_products_service.rb
new file mode 100644
index 0000000000..85497356f7
--- /dev/null
+++ b/app/services/shop_products_service.rb
@@ -0,0 +1,70 @@
+# Returns a (paginatable) AR object for the products in stock for a given distributor and OC.
+# The stock-checking includes on_demand and stock level overrides from variant_overrides.
+
+class ShopProductsService
+  def initialize(distributor, order_cycle)
+    @distributor = distributor
+    @order_cycle = order_cycle
+  end
+
+  def relation
+    Spree::Product.where(id: distributed_products)
+  end
+
+  private
+
+  def distributed_products
+    @order_cycle.
+      variants_distributed_by(@distributor).
+      merge(stocked_variants_and_overrides).
+      select("DISTINCT spree_variants.product_id")
+  end
+
+  def stocked_variants_and_overrides
+    Spree::Variant.
+      joins("LEFT OUTER JOIN variant_overrides ON variant_overrides.variant_id = spree_variants.id
+            AND variant_overrides.hub_id = #{@distributor.id}").
+      joins(:stock_items).
+      where(query_stock_with_overrides)
+  end
+
+  def query_stock_with_overrides
+    "( #{variant_not_overriden} AND ( #{variant_on_demand} OR #{variant_in_stock} ) )
+      OR ( #{variant_overriden} AND ( #{override_on_demand} OR #{override_in_stock} ) )
+      OR ( #{variant_overriden} AND ( #{override_on_demand_null} AND #{variant_on_demand} ) )
+      OR ( #{variant_overriden} AND ( #{override_on_demand_null}
+                                      AND #{variant_not_on_demand} AND #{variant_in_stock} ) )"
+  end
+
+  def variant_not_overriden
+    "variant_overrides.id IS NULL"
+  end
+
+  def variant_overriden
+    "variant_overrides.id IS NOT NULL"
+  end
+
+  def variant_in_stock
+    "spree_stock_items.count_on_hand > 0"
+  end
+
+  def variant_on_demand
+    "spree_stock_items.backorderable IS TRUE"
+  end
+
+  def variant_not_on_demand
+    "spree_stock_items.backorderable IS FALSE"
+  end
+
+  def override_on_demand
+    "variant_overrides.on_demand IS TRUE"
+  end
+
+  def override_in_stock
+    "variant_overrides.count_on_hand > 0"
+  end
+
+  def override_on_demand_null
+    "variant_overrides.on_demand IS NULL"
+  end
+end
diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index e6e3fbe460..3de2177d3b 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -32,64 +32,11 @@ module OpenFoodNetwork
     def load_products
       return unless @order_cycle
 
-      Spree::Product.where(id: distributed_products).
+      ShopProductsService.new(@distributor, @order_cycle).relation.
         order(taxon_order).
         each { |product| scoper.scope(product) }
     end
 
-    def distributed_products
-      @order_cycle.
-        variants_distributed_by(@distributor).
-        merge(stocked_variants_with_overrides).
-        select("DISTINCT spree_variants.product_id")
-    end
-
-    def stocked_variants_with_overrides
-      Spree::Variant.
-        joins("LEFT OUTER JOIN variant_overrides ON variant_overrides.variant_id = spree_variants.id AND variant_overrides.hub_id = #{@distributor.id}").
-        joins(:stock_items).
-        where(query_stock_with_overrides)
-    end
-
-    def query_stock_with_overrides
-      "( #{variant_not_overriden} AND ( #{variant_in_stock} OR #{variant_on_demand} ) )
-        OR ( #{variant_overriden} AND ( #{override_on_demand} OR #{override_in_stock} ) )
-        OR ( #{variant_overriden} AND ( #{override_on_demand_null} AND #{variant_on_demand} ) )
-        OR ( #{variant_overriden} AND ( #{override_on_demand_null} AND #{variant_not_on_demand} AND #{variant_in_stock} ) )"
-    end
-
-    def variant_not_overriden
-      "variant_overrides.id IS NULL"
-    end
-
-    def variant_overriden
-      "variant_overrides.id IS NOT NULL"
-    end
-
-    def variant_in_stock
-      "spree_stock_items.count_on_hand > 0"
-    end
-
-    def variant_on_demand
-      "spree_stock_items.backorderable IS TRUE"
-    end
-
-    def variant_not_on_demand
-      "spree_stock_items.backorderable IS FALSE"
-    end
-
-    def override_on_demand
-      "variant_overrides.on_demand IS TRUE"
-    end
-
-    def override_in_stock
-      "variant_overrides.count_on_hand > 0"
-    end
-
-    def override_on_demand_null
-      "variant_overrides.on_demand IS NULL"
-    end
-
     def scoper
       ScopeProductToHub.new(@distributor)
     end
diff --git a/spec/services/shop_products_service_spec.rb b/spec/services/shop_products_service_spec.rb
new file mode 100644
index 0000000000..70840418ad
--- /dev/null
+++ b/spec/services/shop_products_service_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe ShopProductsService do
+  let(:distributor) { create(:distributor_enterprise) }
+  let(:product) { create(:product) }
+  let(:variant) { product.variants.first }
+  let(:order_cycle) do
+    create(:simple_order_cycle, distributors: [distributor], variants: [variant])
+  end
+
+  describe "product distributed by distributor in the OC" do
+    it "returns products" do
+      expect(described_class.new(distributor, order_cycle).relation).to eq([product])
+    end
+  end
+
+  describe "product distributed by distributor in another OC" do
+    let(:reference_variant) { create(:product).variants.first }
+    let(:order_cycle) do
+      create(:simple_order_cycle, distributors: [distributor], variants: [reference_variant])
+    end
+    let(:another_order_cycle) do
+      create(:simple_order_cycle, distributors: [distributor], variants: [variant])
+    end
+
+    it "does not return product" do
+      expect(described_class.new(distributor, order_cycle).relation).to_not include product
+    end
+  end
+
+  describe "product distributed by another distributor in the OC" do
+    let(:another_distributor) { create(:distributor_enterprise) }
+    let(:order_cycle) do
+      create(:simple_order_cycle, distributors: [another_distributor], variants: [variant])
+    end
+
+    it "does not return product" do
+      expect(described_class.new(distributor, order_cycle).relation).to_not include product
+    end
+  end
+
+  describe "filtering products that are out of stock" do
+    context "with regular variants" do
+      it "returns product when variant is in stock" do
+        expect(described_class.new(distributor, order_cycle).relation).to include product
+      end
+
+      it "does not return product when variant is out of stock" do
+        variant.update_attribute(:on_hand, 0)
+        expect(described_class.new(distributor, order_cycle).relation).to_not include product
+      end
+    end
+
+    context "with variant overrides" do
+      let!(:override) { create(:variant_override, hub: distributor, variant: variant, count_on_hand: 0) }
+
+      it "does not return product when an override is out of stock" do
+        expect(described_class.new(distributor, order_cycle).relation).to_not include product
+      end
+
+      it "returns product when an override is in stock" do
+        variant.update_attribute(:on_hand, 0)
+        override.update_attribute(:count_on_hand, 10)
+        expect(described_class.new(distributor, order_cycle).relation).to include product
+      end
+    end
+  end
+end

From e9acf6e0de026f4fd8e500512173dfa811cbb8be Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 29 Sep 2019 14:14:52 +0100
Subject: [PATCH 6/9] Refactor #load_products and memoize

---
 lib/open_food_network/products_renderer.rb           | 10 ++++------
 spec/lib/open_food_network/products_renderer_spec.rb |  4 ++--
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 3de2177d3b..4c2e876c08 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -10,12 +10,10 @@ module OpenFoodNetwork
     end
 
     def products_json
-      products = load_products
-
-      if products
+      if shop_products
         enterprise_fee_calculator = EnterpriseFeeCalculator.new @distributor, @order_cycle
 
-        ActiveModel::ArraySerializer.new(products,
+        ActiveModel::ArraySerializer.new(shop_products,
                                          each_serializer: Api::ProductSerializer,
                                          current_order_cycle: @order_cycle,
                                          current_distributor: @distributor,
@@ -29,10 +27,10 @@ module OpenFoodNetwork
 
     private
 
-    def load_products
+    def shop_products
       return unless @order_cycle
 
-      ShopProductsService.new(@distributor, @order_cycle).relation.
+      @shop_products ||= ShopProductsService.new(@distributor, @order_cycle).relation.
         order(taxon_order).
         each { |product| scoper.scope(product) }
     end
diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb
index e7ee727c41..1c970443d0 100644
--- a/spec/lib/open_food_network/products_renderer_spec.rb
+++ b/spec/lib/open_food_network/products_renderer_spec.rb
@@ -25,13 +25,13 @@ module OpenFoodNetwork
 
       it "sorts products by the distributor's preferred taxon list" do
         allow(distributor).to receive(:preferred_shopfront_taxon_order) { "#{t1.id},#{t2.id}" }
-        products = pr.send(:load_products)
+        products = pr.send(:shop_products)
         expect(products).to eq([p2, p4, p1, p3])
       end
 
       it "alphabetizes products by name when taxon list is not set" do
         allow(distributor).to receive(:preferred_shopfront_taxon_order) { "" }
-        products = pr.send(:load_products)
+        products = pr.send(:shop_products)
         expect(products).to eq([p1, p2, p3, p4])
       end
     end

From 032741c54f5395fd5332ed07f5206b4be7a2ae4f Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 29 Sep 2019 16:20:20 +0100
Subject: [PATCH 7/9] Refactor ProductsRenderer variants queries

This removes another N+1 and allows pagination applied to the inital query to also affect the returned variants
---
 app/services/shop_products_service.rb       |  14 ++-
 lib/open_food_network/products_renderer.rb  |  37 +++---
 spec/services/shop_products_service_spec.rb | 128 ++++++++++++--------
 3 files changed, 104 insertions(+), 75 deletions(-)

diff --git a/app/services/shop_products_service.rb b/app/services/shop_products_service.rb
index 85497356f7..795cecc6b9 100644
--- a/app/services/shop_products_service.rb
+++ b/app/services/shop_products_service.rb
@@ -1,4 +1,4 @@
-# Returns a (paginatable) AR object for the products in stock for a given distributor and OC.
+# Returns a (paginatable) AR object for the products or variants in stock for a given shop and OC.
 # The stock-checking includes on_demand and stock level overrides from variant_overrides.
 
 class ShopProductsService
@@ -7,13 +7,19 @@ class ShopProductsService
     @order_cycle = order_cycle
   end
 
-  def relation
-    Spree::Product.where(id: distributed_products)
+  def products_relation
+    Spree::Product.where(id: stocked_products)
+  end
+
+  def variants_relation
+    @order_cycle.
+      variants_distributed_by(@distributor).
+      merge(stocked_variants_and_overrides)
   end
 
   private
 
-  def distributed_products
+  def stocked_products
     @order_cycle.
       variants_distributed_by(@distributor).
       merge(stocked_variants_and_overrides).
diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 4c2e876c08..297209fee9 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -30,13 +30,17 @@ module OpenFoodNetwork
     def shop_products
       return unless @order_cycle
 
-      @shop_products ||= ShopProductsService.new(@distributor, @order_cycle).relation.
-        order(taxon_order).
-        each { |product| scoper.scope(product) }
+      @shop_products ||= begin
+        scoper = ScopeProductToHub.new(@distributor)
+
+        shop_products_service.products_relation.
+          order(taxon_order).
+          each { |product| scoper.scope(product) }
+      end
     end
 
-    def scoper
-      ScopeProductToHub.new(@distributor)
+    def shop_products_service
+      ShopProductsService.new(@distributor, @order_cycle)
     end
 
     def taxon_order
@@ -50,25 +54,22 @@ module OpenFoodNetwork
       end
     end
 
-    def all_variants_for_shop
-      @all_variants_for_shop ||= begin
-                                   # We use the in_stock? method here instead of the in_stock scope
-                                   # because we need to look up the stock as overridden by
-                                   # VariantOverrides, and the scope method is not affected by them.
-                                   scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
-                                   Spree::Variant.
-                                     for_distribution(@order_cycle, @distributor).
-                                     each { |v| scoper.scope(v) }.
-                                     select(&:in_stock?)
-                                 end
+    def variants_for_shop
+      @variants_for_shop ||= begin
+        scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
+
+        shop_products_service.variants_relation.
+          where(product_id: shop_products).
+          each { |v| scoper.scope(v) }
+      end
     end
 
     def variants_for_shop_by_id
-      index_by_product_id all_variants_for_shop.reject(&:is_master)
+      index_by_product_id variants_for_shop.reject(&:is_master)
     end
 
     def master_variants_for_shop_by_id
-      index_by_product_id all_variants_for_shop.select(&:is_master)
+      index_by_product_id variants_for_shop.select(&:is_master)
     end
 
     def index_by_product_id(variants)
diff --git a/spec/services/shop_products_service_spec.rb b/spec/services/shop_products_service_spec.rb
index 70840418ad..fa0c51c01e 100644
--- a/spec/services/shop_products_service_spec.rb
+++ b/spec/services/shop_products_service_spec.rb
@@ -1,68 +1,90 @@
 require 'spec_helper'
 
 describe ShopProductsService do
-  let(:distributor) { create(:distributor_enterprise) }
-  let(:product) { create(:product) }
-  let(:variant) { product.variants.first }
-  let(:order_cycle) do
-    create(:simple_order_cycle, distributors: [distributor], variants: [variant])
-  end
-
-  describe "product distributed by distributor in the OC" do
-    it "returns products" do
-      expect(described_class.new(distributor, order_cycle).relation).to eq([product])
-    end
-  end
-
-  describe "product distributed by distributor in another OC" do
-    let(:reference_variant) { create(:product).variants.first }
+  describe "#products_relation" do
+    let(:distributor) { create(:distributor_enterprise) }
+    let(:product) { create(:product) }
+    let(:variant) { product.variants.first }
     let(:order_cycle) do
-      create(:simple_order_cycle, distributors: [distributor], variants: [reference_variant])
-    end
-    let(:another_order_cycle) do
       create(:simple_order_cycle, distributors: [distributor], variants: [variant])
     end
 
-    it "does not return product" do
-      expect(described_class.new(distributor, order_cycle).relation).to_not include product
+    describe "product distributed by distributor in the OC" do
+      it "returns products" do
+        expect(described_class.new(distributor, order_cycle).products_relation).to eq([product])
+      end
+    end
+
+    describe "product distributed by distributor in another OC" do
+      let(:reference_variant) { create(:product).variants.first }
+      let(:order_cycle) do
+        create(:simple_order_cycle, distributors: [distributor], variants: [reference_variant])
+      end
+      let(:another_order_cycle) do
+        create(:simple_order_cycle, distributors: [distributor], variants: [variant])
+      end
+
+      it "does not return product" do
+        expect(described_class.new(distributor, order_cycle).products_relation).to_not include product
+      end
+    end
+
+    describe "product distributed by another distributor in the OC" do
+      let(:another_distributor) { create(:distributor_enterprise) }
+      let(:order_cycle) do
+        create(:simple_order_cycle, distributors: [another_distributor], variants: [variant])
+      end
+
+      it "does not return product" do
+        expect(described_class.new(distributor, order_cycle).products_relation).to_not include product
+      end
+    end
+
+    describe "filtering products that are out of stock" do
+      context "with regular variants" do
+        it "returns product when variant is in stock" do
+          expect(described_class.new(distributor, order_cycle).products_relation).to include product
+        end
+
+        it "does not return product when variant is out of stock" do
+          variant.update_attribute(:on_hand, 0)
+          expect(described_class.new(distributor, order_cycle).products_relation).to_not include product
+        end
+      end
+
+      context "with variant overrides" do
+        let!(:override) { create(:variant_override, hub: distributor, variant: variant, count_on_hand: 0) }
+
+        it "does not return product when an override is out of stock" do
+          expect(described_class.new(distributor, order_cycle).products_relation).to_not include product
+        end
+
+        it "returns product when an override is in stock" do
+          variant.update_attribute(:on_hand, 0)
+          override.update_attribute(:count_on_hand, 10)
+          expect(described_class.new(distributor, order_cycle).products_relation).to include product
+        end
+      end
     end
   end
 
-  describe "product distributed by another distributor in the OC" do
-    let(:another_distributor) { create(:distributor_enterprise) }
-    let(:order_cycle) do
-      create(:simple_order_cycle, distributors: [another_distributor], variants: [variant])
+  describe "#variants_relation" do
+    let(:distributor) { create(:distributor_enterprise) }
+    let(:oc) { create(:simple_order_cycle, distributors: [distributor], variants: [v1, v3]) }
+    let(:product) { create(:simple_product) }
+    let!(:v1) { create(:variant, product: product) }
+    let!(:v2) { create(:variant, product: product) }
+    let!(:v3) { create(:variant, product: product) }
+    let!(:vo) { create(:variant_override, hub: distributor, variant_id: v3.id, count_on_hand: 0) }
+    let(:variants) { described_class.new(distributor, oc).variants_relation }
+
+    it "returns variants in the oc" do
+      expect(variants).to include v1
+      expect(variants).to_not include v2
     end
 
-    it "does not return product" do
-      expect(described_class.new(distributor, order_cycle).relation).to_not include product
-    end
-  end
-
-  describe "filtering products that are out of stock" do
-    context "with regular variants" do
-      it "returns product when variant is in stock" do
-        expect(described_class.new(distributor, order_cycle).relation).to include product
-      end
-
-      it "does not return product when variant is out of stock" do
-        variant.update_attribute(:on_hand, 0)
-        expect(described_class.new(distributor, order_cycle).relation).to_not include product
-      end
-    end
-
-    context "with variant overrides" do
-      let!(:override) { create(:variant_override, hub: distributor, variant: variant, count_on_hand: 0) }
-
-      it "does not return product when an override is out of stock" do
-        expect(described_class.new(distributor, order_cycle).relation).to_not include product
-      end
-
-      it "returns product when an override is in stock" do
-        variant.update_attribute(:on_hand, 0)
-        override.update_attribute(:count_on_hand, 10)
-        expect(described_class.new(distributor, order_cycle).relation).to include product
-      end
+    it "does not return variants where override is out of stock" do
+      expect(variants).to_not include v3
     end
   end
 end

From 6153789055aaee0dd6471c830b835d0a5b75cf40 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 29 Sep 2019 20:44:48 +0100
Subject: [PATCH 8/9] Eager-load serialized objects in variant query

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

diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index 297209fee9..d6395e483e 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -59,6 +59,7 @@ module OpenFoodNetwork
         scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
 
         shop_products_service.variants_relation.
+          includes(:default_price, :stock_locations, :product).
           where(product_id: shop_products).
           each { |v| scoper.scope(v) }
       end

From c038b485b198d1678a41aaa25a4c13e5c7286bd3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 1 Oct 2019 14:43:47 +0100
Subject: [PATCH 9/9] Rename service and methods to remove use of "shop" term

---
 ....rb => order_cycle_distributed_products.rb} |  2 +-
 lib/open_food_network/products_renderer.rb     | 18 +++++++++---------
 .../products_renderer_spec.rb                  |  4 ++--
 ...> order_cycle_distributed_products_spec.rb} |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)
 rename app/services/{shop_products_service.rb => order_cycle_distributed_products.rb} (98%)
 rename spec/services/{shop_products_service_spec.rb => order_cycle_distributed_products_spec.rb} (98%)

diff --git a/app/services/shop_products_service.rb b/app/services/order_cycle_distributed_products.rb
similarity index 98%
rename from app/services/shop_products_service.rb
rename to app/services/order_cycle_distributed_products.rb
index 795cecc6b9..d2c9d3c4ca 100644
--- a/app/services/shop_products_service.rb
+++ b/app/services/order_cycle_distributed_products.rb
@@ -1,7 +1,7 @@
 # Returns a (paginatable) AR object for the products or variants in stock for a given shop and OC.
 # The stock-checking includes on_demand and stock level overrides from variant_overrides.
 
-class ShopProductsService
+class OrderCycleDistributedProducts
   def initialize(distributor, order_cycle)
     @distributor = distributor
     @order_cycle = order_cycle
diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb
index d6395e483e..527385b6e8 100644
--- a/lib/open_food_network/products_renderer.rb
+++ b/lib/open_food_network/products_renderer.rb
@@ -10,10 +10,10 @@ module OpenFoodNetwork
     end
 
     def products_json
-      if shop_products
+      if products
         enterprise_fee_calculator = EnterpriseFeeCalculator.new @distributor, @order_cycle
 
-        ActiveModel::ArraySerializer.new(shop_products,
+        ActiveModel::ArraySerializer.new(products,
                                          each_serializer: Api::ProductSerializer,
                                          current_order_cycle: @order_cycle,
                                          current_distributor: @distributor,
@@ -27,20 +27,20 @@ module OpenFoodNetwork
 
     private
 
-    def shop_products
+    def products
       return unless @order_cycle
 
-      @shop_products ||= begin
+      @products ||= begin
         scoper = ScopeProductToHub.new(@distributor)
 
-        shop_products_service.products_relation.
+        distributed_products.products_relation.
           order(taxon_order).
           each { |product| scoper.scope(product) }
       end
     end
 
-    def shop_products_service
-      ShopProductsService.new(@distributor, @order_cycle)
+    def distributed_products
+      OrderCycleDistributedProducts.new(@distributor, @order_cycle)
     end
 
     def taxon_order
@@ -58,9 +58,9 @@ module OpenFoodNetwork
       @variants_for_shop ||= begin
         scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
 
-        shop_products_service.variants_relation.
+        distributed_products.variants_relation.
           includes(:default_price, :stock_locations, :product).
-          where(product_id: shop_products).
+          where(product_id: products).
           each { |v| scoper.scope(v) }
       end
     end
diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb
index 1c970443d0..252f26f517 100644
--- a/spec/lib/open_food_network/products_renderer_spec.rb
+++ b/spec/lib/open_food_network/products_renderer_spec.rb
@@ -25,13 +25,13 @@ module OpenFoodNetwork
 
       it "sorts products by the distributor's preferred taxon list" do
         allow(distributor).to receive(:preferred_shopfront_taxon_order) { "#{t1.id},#{t2.id}" }
-        products = pr.send(:shop_products)
+        products = pr.send(:products)
         expect(products).to eq([p2, p4, p1, p3])
       end
 
       it "alphabetizes products by name when taxon list is not set" do
         allow(distributor).to receive(:preferred_shopfront_taxon_order) { "" }
-        products = pr.send(:shop_products)
+        products = pr.send(:products)
         expect(products).to eq([p1, p2, p3, p4])
       end
     end
diff --git a/spec/services/shop_products_service_spec.rb b/spec/services/order_cycle_distributed_products_spec.rb
similarity index 98%
rename from spec/services/shop_products_service_spec.rb
rename to spec/services/order_cycle_distributed_products_spec.rb
index fa0c51c01e..b02baf4974 100644
--- a/spec/services/shop_products_service_spec.rb
+++ b/spec/services/order_cycle_distributed_products_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe ShopProductsService do
+describe OrderCycleDistributedProducts do
   describe "#products_relation" do
     let(:distributor) { create(:distributor_enterprise) }
     let(:product) { create(:product) }