From 64c66ddedcc9a3eedecd80b7fce0312cc210044a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 14:21:09 +0200
Subject: [PATCH 01/10] Eager-load variant data for overridable products

Cuts query count and page load time in half for this endpoint.
---
 app/controllers/api/products_controller.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/controllers/api/products_controller.rb b/app/controllers/api/products_controller.rb
index a45e9a8aea..c0a46aca05 100644
--- a/app/controllers/api/products_controller.rb
+++ b/app/controllers/api/products_controller.rb
@@ -121,6 +121,7 @@ module Api
     def paged_products_for_producers(producers)
       Spree::Product.scoped.
         merge(product_scope).
+        includes(variants: [:product, :default_price, :stock_items]).
         where(supplier_id: producers).
         by_producer.by_name.
         ransack(params[:q]).result.

From 1b7ac1a252f2a06f6ee8b45814899b90587c5d8e Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 14:30:02 +0200
Subject: [PATCH 02/10] Don't re-use fat serializers when thin ones are needed.

This cuts the pageload and query count in half, again.
---
 app/controllers/api/products_controller.rb    |  6 ++---
 .../api/admin/product_simple_serializer.rb    | 16 +++++++++++
 .../api/admin/variant_simple_serializer.rb    | 27 +++++++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 app/serializers/api/admin/product_simple_serializer.rb
 create mode 100644 app/serializers/api/admin/variant_simple_serializer.rb

diff --git a/app/controllers/api/products_controller.rb b/app/controllers/api/products_controller.rb
index c0a46aca05..3182c79044 100644
--- a/app/controllers/api/products_controller.rb
+++ b/app/controllers/api/products_controller.rb
@@ -74,7 +74,7 @@ module Api
 
       @products = paged_products_for_producers producers
 
-      render_paged_products @products
+      render_paged_products @products, ::Api::Admin::ProductSimpleSerializer
     end
 
     # POST /api/products/:product_id/clone
@@ -128,10 +128,10 @@ module Api
         page(params[:page]).per(params[:per_page])
     end
 
-    def render_paged_products(products)
+    def render_paged_products(products, product_serializer = ::Api::Admin::ProductSerializer)
       serializer = ActiveModel::ArraySerializer.new(
         products,
-        each_serializer: ::Api::Admin::ProductSerializer
+        each_serializer: product_serializer
       )
 
       render text: {
diff --git a/app/serializers/api/admin/product_simple_serializer.rb b/app/serializers/api/admin/product_simple_serializer.rb
new file mode 100644
index 0000000000..181bb33f54
--- /dev/null
+++ b/app/serializers/api/admin/product_simple_serializer.rb
@@ -0,0 +1,16 @@
+class Api::Admin::ProductSimpleSerializer < ActiveModel::Serializer
+  attributes :id, :name
+
+  has_one :supplier, key: :producer_id, embed: :id
+  has_many :variants, key: :variants, serializer: Api::Admin::VariantSimpleSerializer
+
+  def on_hand
+    return 0 if object.on_hand.nil?
+
+    object.on_hand
+  end
+
+  def price
+    object.price.nil? ? '0.0' : object.price
+  end
+end
diff --git a/app/serializers/api/admin/variant_simple_serializer.rb b/app/serializers/api/admin/variant_simple_serializer.rb
new file mode 100644
index 0000000000..64aa6e433d
--- /dev/null
+++ b/app/serializers/api/admin/variant_simple_serializer.rb
@@ -0,0 +1,27 @@
+class Api::Admin::VariantSimpleSerializer < ActiveModel::Serializer
+  attributes :id, :name, :import_date,
+             :options_text, :unit_value, :unit_description, :unit_to_display,
+             :display_as, :display_name, :name_to_display,
+             :price, :on_demand, :on_hand
+
+  has_many :variant_overrides
+
+  def name
+    if object.full_name.present?
+      "#{object.name} - #{object.full_name}"
+    else
+      object.name
+    end
+  end
+
+  def on_hand
+    return 0 if object.on_hand.nil?
+
+    object.on_hand
+  end
+
+  def price
+    # Decimals are passed to json as strings, we need to run parseFloat.toFixed(2) on the client.
+    object.price.nil? ? 0.to_f : object.price
+  end
+end

From ececbce596a33bcd97f0902636a67eaddbe28fd3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 14:43:42 +0200
Subject: [PATCH 03/10] Only select id in producers query

---
 app/controllers/api/products_controller.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/controllers/api/products_controller.rb b/app/controllers/api/products_controller.rb
index 3182c79044..854d820a47 100644
--- a/app/controllers/api/products_controller.rb
+++ b/app/controllers/api/products_controller.rb
@@ -69,10 +69,10 @@ module Api
     end
 
     def overridable
-      producers = OpenFoodNetwork::Permissions.new(current_api_user).
-        variant_override_producers.by_name
+      producer_ids = OpenFoodNetwork::Permissions.new(current_api_user).
+        variant_override_producers.by_name.select('enterprises.id')
 
-      @products = paged_products_for_producers producers
+      @products = paged_products_for_producers producer_ids
 
       render_paged_products @products, ::Api::Admin::ProductSimpleSerializer
     end
@@ -118,11 +118,11 @@ module Api
       ]
     end
 
-    def paged_products_for_producers(producers)
+    def paged_products_for_producers(producer_ids)
       Spree::Product.scoped.
         merge(product_scope).
         includes(variants: [:product, :default_price, :stock_items]).
-        where(supplier_id: producers).
+        where(supplier_id: producer_ids).
         by_producer.by_name.
         ransack(params[:q]).result.
         page(params[:page]).per(params[:per_page])

From f9cf826f1c190894083338c3f2b9c53786eead68 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 14:53:26 +0200
Subject: [PATCH 04/10] Bring Spree::Stock::Quantifier in to OFN

This is the original unmodified Class from Spree. Modifications added in subsequent commits.
---
 app/models/spree/stock/quantifier.rb | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 app/models/spree/stock/quantifier.rb

diff --git a/app/models/spree/stock/quantifier.rb b/app/models/spree/stock/quantifier.rb
new file mode 100644
index 0000000000..6b1c11de62
--- /dev/null
+++ b/app/models/spree/stock/quantifier.rb
@@ -0,0 +1,28 @@
+module Spree
+  module Stock
+    class Quantifier
+      attr_reader :stock_items
+
+      def initialize(variant)
+        @variant = variant
+        @stock_items = Spree::StockItem.joins(:stock_location).where(:variant_id => @variant, Spree::StockLocation.table_name =>{ :active => true})
+      end
+
+      def total_on_hand
+        if Spree::Config.track_inventory_levels
+          stock_items.sum(&:count_on_hand)
+        else
+          Float::INFINITY
+        end
+      end
+
+      def backorderable?
+        stock_items.any?(&:backorderable)
+      end
+
+      def can_supply?(required)
+        total_on_hand >= required || backorderable?
+      end
+    end
+  end
+end

From f959e632eaeddfc450401509f5c3028e9b6ca2f3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 15:15:25 +0200
Subject: [PATCH 05/10] Modify Spree::Stock::Quantifier to not re-fetch stock
 items if they are already eager-loaded

This helps to remove a big N+1 here, and will also be very helpful elsewhere in the app
---
 app/models/spree/stock/quantifier.rb | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/app/models/spree/stock/quantifier.rb b/app/models/spree/stock/quantifier.rb
index 6b1c11de62..806fdf33dd 100644
--- a/app/models/spree/stock/quantifier.rb
+++ b/app/models/spree/stock/quantifier.rb
@@ -5,7 +5,7 @@ module Spree
 
       def initialize(variant)
         @variant = variant
-        @stock_items = Spree::StockItem.joins(:stock_location).where(:variant_id => @variant, Spree::StockLocation.table_name =>{ :active => true})
+        @stock_items = fetch_stock_items
       end
 
       def total_on_hand
@@ -23,6 +23,16 @@ module Spree
       def can_supply?(required)
         total_on_hand >= required || backorderable?
       end
+
+      private
+
+      def fetch_stock_items
+        # Don't re-fetch associated stock items from the DB if we've already eager-loaded them
+        return @variant.stock_items.to_a if @variant.stock_items.loaded?
+
+        Spree::StockItem.joins(:stock_location).
+          where(:variant_id => @variant, Spree::StockLocation.table_name => { active: true })
+      end
     end
   end
 end

From b0a7497f2ae8778ae265aaf6facc7a5f17c1789a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 15:22:00 +0200
Subject: [PATCH 06/10] Remove another N+1 in product serialization

---
 app/serializers/api/admin/product_simple_serializer.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/serializers/api/admin/product_simple_serializer.rb b/app/serializers/api/admin/product_simple_serializer.rb
index 181bb33f54..b5af292e81 100644
--- a/app/serializers/api/admin/product_simple_serializer.rb
+++ b/app/serializers/api/admin/product_simple_serializer.rb
@@ -1,9 +1,12 @@
 class Api::Admin::ProductSimpleSerializer < ActiveModel::Serializer
-  attributes :id, :name
+  attributes :id, :name, :producer_id
 
-  has_one :supplier, key: :producer_id, embed: :id
   has_many :variants, key: :variants, serializer: Api::Admin::VariantSimpleSerializer
 
+  def producer_id
+    object.supplier_id
+  end
+
   def on_hand
     return 0 if object.on_hand.nil?
 

From b3c968856bdf99123c173065ea85918f5b7e9212 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 15:57:26 +0200
Subject: [PATCH 07/10] Fix some rubocop issues

---
 app/models/spree/stock/quantifier.rb          |  2 +
 .../api/admin/product_simple_serializer.rb    | 30 +++++++-----
 .../api/admin/variant_simple_serializer.rb    | 49 ++++++++++---------
 3 files changed, 47 insertions(+), 34 deletions(-)

diff --git a/app/models/spree/stock/quantifier.rb b/app/models/spree/stock/quantifier.rb
index 806fdf33dd..b046dc2b17 100644
--- a/app/models/spree/stock/quantifier.rb
+++ b/app/models/spree/stock/quantifier.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   module Stock
     class Quantifier
diff --git a/app/serializers/api/admin/product_simple_serializer.rb b/app/serializers/api/admin/product_simple_serializer.rb
index b5af292e81..ad7ad16ba1 100644
--- a/app/serializers/api/admin/product_simple_serializer.rb
+++ b/app/serializers/api/admin/product_simple_serializer.rb
@@ -1,19 +1,25 @@
-class Api::Admin::ProductSimpleSerializer < ActiveModel::Serializer
-  attributes :id, :name, :producer_id
+# frozen_string_literal: true
 
-  has_many :variants, key: :variants, serializer: Api::Admin::VariantSimpleSerializer
+module Api
+  module Admin
+    class ProductSimpleSerializer < ActiveModel::Serializer
+      attributes :id, :name, :producer_id
 
-  def producer_id
-    object.supplier_id
-  end
+      has_many :variants, key: :variants, serializer: Api::Admin::VariantSimpleSerializer
 
-  def on_hand
-    return 0 if object.on_hand.nil?
+      def producer_id
+        object.supplier_id
+      end
 
-    object.on_hand
-  end
+      def on_hand
+        return 0 if object.on_hand.nil?
 
-  def price
-    object.price.nil? ? '0.0' : object.price
+        object.on_hand
+      end
+
+      def price
+        object.price.nil? ? '0.0' : object.price
+      end
+    end
   end
 end
diff --git a/app/serializers/api/admin/variant_simple_serializer.rb b/app/serializers/api/admin/variant_simple_serializer.rb
index 64aa6e433d..d94421321f 100644
--- a/app/serializers/api/admin/variant_simple_serializer.rb
+++ b/app/serializers/api/admin/variant_simple_serializer.rb
@@ -1,27 +1,32 @@
-class Api::Admin::VariantSimpleSerializer < ActiveModel::Serializer
-  attributes :id, :name, :import_date,
-             :options_text, :unit_value, :unit_description, :unit_to_display,
-             :display_as, :display_name, :name_to_display,
-             :price, :on_demand, :on_hand
+# frozen_string_literal: true
 
-  has_many :variant_overrides
+module Api
+  module Admin
+    class VariantSimpleSerializer < ActiveModel::Serializer
+      attributes :id, :name, :import_date,
+                 :options_text, :unit_value, :unit_description, :unit_to_display,
+                 :display_as, :display_name, :name_to_display,
+                 :price, :on_demand, :on_hand
 
-  def name
-    if object.full_name.present?
-      "#{object.name} - #{object.full_name}"
-    else
-      object.name
+      has_many :variant_overrides
+
+      def name
+        if object.full_name.present?
+          "#{object.name} - #{object.full_name}"
+        else
+          object.name
+        end
+      end
+
+      def on_hand
+        return 0 if object.on_hand.nil?
+
+        object.on_hand
+      end
+
+      def price
+        object.price.nil? ? 0.to_f : object.price
+      end
     end
   end
-
-  def on_hand
-    return 0 if object.on_hand.nil?
-
-    object.on_hand
-  end
-
-  def price
-    # Decimals are passed to json as strings, we need to run parseFloat.toFixed(2) on the client.
-    object.price.nil? ? 0.to_f : object.price
-  end
 end

From 5bb2614f9d4bac48b927183036604217149afe0c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 7 Apr 2020 17:58:37 +0200
Subject: [PATCH 08/10] Refactor PagedFetcher so it makes one request at a time

---
 .../services/paged_fetcher.js.coffee          | 29 +++++++++++--------
 .../variant_overrides_controller.js.coffee    |  9 +++---
 ...ariant_overrides_controller_spec.js.coffee |  4 +--
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/app/assets/javascripts/admin/index_utils/services/paged_fetcher.js.coffee b/app/assets/javascripts/admin/index_utils/services/paged_fetcher.js.coffee
index 82487996ae..96836ef9cc 100644
--- a/app/assets/javascripts/admin/index_utils/services/paged_fetcher.js.coffee
+++ b/app/assets/javascripts/admin/index_utils/services/paged_fetcher.js.coffee
@@ -2,19 +2,24 @@ angular.module("admin.indexUtils").factory "PagedFetcher", (dataFetcher) ->
   new class PagedFetcher
     # Given a URL like http://example.com/foo?page=::page::&per_page=20
     # And the response includes an attribute pages with the number of pages to fetch
-    # Fetch each page async, and call the processData callback with the resulting data
-    fetch: (url, processData, onLastPageComplete) ->
-      dataFetcher(@urlForPage(url, 1)).then (data) =>
-        processData data
+    # Fetch each page async, and call the pageCallback callback with the resulting data
+    # Developer note: this class should not be re-used!
+    page: 1
+    last_page: 1
 
-        if data.pages > 1
-          for page in [2..data.pages]
-            lastPromise = dataFetcher(@urlForPage(url, page)).then (data) ->
-              processData data
-          onLastPageComplete && lastPromise.then onLastPageComplete
-          return
-        else
-          onLastPageComplete && onLastPageComplete()
+    fetch: (url, pageCallback) ->
+      @fetchPages(url, @page, pageCallback)
 
     urlForPage: (url, page) ->
       url.replace("::page::", page)
+
+    fetchPages: (url, page, pageCallback) ->
+      dataFetcher(@urlForPage(url, page)).then (data) =>
+        @page++
+        @last_page = data.pages
+
+        pageCallback(data) if pageCallback
+
+        if @page <= @last_page
+          @fetchPages(url, @page, pageCallback)
+
diff --git a/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee b/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee
index c37ba72071..b890d2ffd5 100644
--- a/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee
+++ b/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee
@@ -43,12 +43,11 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl",
 
   $scope.fetchProducts = ->
     url = "/api/products/overridable?page=::page::;per_page=100"
-    PagedFetcher.fetch url, (data) => $scope.addProducts data.products
+    PagedFetcher.fetch url, $scope.addProducts
 
-
-  $scope.addProducts = (products) ->
-    $scope.products = $scope.products.concat products
-    VariantOverrides.ensureDataFor hubs, products
+  $scope.addProducts = (data) ->
+    $scope.products = $scope.products.concat data.products
+    VariantOverrides.ensureDataFor hubs, data.products
 
   $scope.displayDirty = ->
     if DirtyVariantOverrides.count() > 0
diff --git a/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee b/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee
index 5724fe1353..1c860e8e91 100644
--- a/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee
+++ b/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee
@@ -51,9 +51,9 @@ describe "VariantOverridesCtrl", ->
   it "adds products", ->
     spyOn(VariantOverrides, "ensureDataFor")
     expect(scope.products).toEqual []
-    scope.addProducts ['a', 'b']
+    scope.addProducts { products: ['a', 'b'] }
     expect(scope.products).toEqual ['a', 'b']
-    scope.addProducts ['c', 'd']
+    scope.addProducts { products: ['c', 'd'] }
     expect(scope.products).toEqual ['a', 'b', 'c', 'd']
     expect(VariantOverrides.ensureDataFor).toHaveBeenCalled()
 

From 6afda141a18e4664d4a194a7086244bfedafd382 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 9 Apr 2020 09:18:28 +0200
Subject: [PATCH 09/10] Remove track_inventory_levels conditional

This value is always true in OFN
---
 app/models/spree/stock/quantifier.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/app/models/spree/stock/quantifier.rb b/app/models/spree/stock/quantifier.rb
index b046dc2b17..8c437dc580 100644
--- a/app/models/spree/stock/quantifier.rb
+++ b/app/models/spree/stock/quantifier.rb
@@ -11,11 +11,7 @@ module Spree
       end
 
       def total_on_hand
-        if Spree::Config.track_inventory_levels
-          stock_items.sum(&:count_on_hand)
-        else
-          Float::INFINITY
-        end
+        stock_items.sum(&:count_on_hand)
       end
 
       def backorderable?

From 47ac6c14916f2418a476db6011bae4356bcaad3b Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 9 Apr 2020 09:51:32 +0200
Subject: [PATCH 10/10] Remove unused methods from ProductSimpleSerializer

---
 app/serializers/api/admin/product_simple_serializer.rb | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/app/serializers/api/admin/product_simple_serializer.rb b/app/serializers/api/admin/product_simple_serializer.rb
index ad7ad16ba1..6b968ab8d3 100644
--- a/app/serializers/api/admin/product_simple_serializer.rb
+++ b/app/serializers/api/admin/product_simple_serializer.rb
@@ -10,16 +10,6 @@ module Api
       def producer_id
         object.supplier_id
       end
-
-      def on_hand
-        return 0 if object.on_hand.nil?
-
-        object.on_hand
-      end
-
-      def price
-        object.price.nil? ? '0.0' : object.price
-      end
     end
   end
 end