From 3ce7e967772946728422d3e4e68fde1efd7ab778 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 20 Apr 2020 21:22:18 +0200
Subject: [PATCH 01/23] Add some debounce and an onwheel hack to product add to
 basket field

Debounce ensures we don't get a million requests if the up/down buttons are clicked rapidly. The onwheel hack adds some protection against scrolling triggering the quantity up/down. See: https://stackoverflow.com/a/51076231
---
 .../partials/shop_variant_no_group_buy.html.haml  |  2 ++
 .../shop_variant_with_group_buy.html.haml         |  4 ++++
 spec/features/consumer/shopping/shopping_spec.rb  | 15 +++++++++++----
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/app/assets/javascripts/templates/partials/shop_variant_no_group_buy.html.haml b/app/assets/javascripts/templates/partials/shop_variant_no_group_buy.html.haml
index 2e7e49a0ca..8ba787f20b 100644
--- a/app/assets/javascripts/templates/partials/shop_variant_no_group_buy.html.haml
+++ b/app/assets/javascripts/templates/partials/shop_variant_no_group_buy.html.haml
@@ -6,6 +6,8 @@
   min: 0,
   placeholder: "0",
   "ofn-disable-scroll" => true,
+  "ng-debounce" => "500",
+  onwheel: "this.blur()",
   "ng-model" => "variant.line_item.quantity",
   "ofn-on-hand" => "{{variant.on_demand && 9999 || variant.on_hand }}",
   "ng-disabled" => "!variant.on_demand && variant.on_hand == 0",
diff --git a/app/assets/javascripts/templates/partials/shop_variant_with_group_buy.html.haml b/app/assets/javascripts/templates/partials/shop_variant_with_group_buy.html.haml
index 169b33391c..c0cc46ac13 100644
--- a/app/assets/javascripts/templates/partials/shop_variant_with_group_buy.html.haml
+++ b/app/assets/javascripts/templates/partials/shop_variant_with_group_buy.html.haml
@@ -9,6 +9,8 @@
       "ng-model" => "variant.line_item.quantity",
       placeholder: "{{::'shop_variant_quantity_min' | t}}",
       "ofn-disable-scroll" => true,
+      "ng-debounce" => "500",
+      onwheel: "this.blur()",
       "ofn-on-hand" => "{{variant.on_demand && 9999 || variant.on_hand }}",
       name: "variants[{{::variant.id}}]", id: "variants_{{::variant.id}}"}
     %span.bulk-input
@@ -19,6 +21,8 @@
       "ng-model" => "variant.line_item.max_quantity",
       placeholder: "{{::'shop_variant_quantity_max' | t}}",
       "ofn-disable-scroll" => true,
+      "ng-debounce" => "500",
+      onwheel: "this.blur()",
       min: "{{variant.line_item.quantity}}",
       name: "variant_attributes[{{::variant.id}}][max_quantity]",
       id: "variants_{{::variant.id}}_max"}
diff --git a/spec/features/consumer/shopping/shopping_spec.rb b/spec/features/consumer/shopping/shopping_spec.rb
index b8a6727727..432f750f17 100644
--- a/spec/features/consumer/shopping/shopping_spec.rb
+++ b/spec/features/consumer/shopping/shopping_spec.rb
@@ -246,24 +246,25 @@ feature "As a consumer I want to shop with a distributor", js: true do
         before do
           add_variant_to_order_cycle(exchange, variant)
           set_order_cycle(order, oc1)
+          set_order(order)
           visit shop_path
         end
 
         it "should save group buy data to the cart and display it on shopfront reload" do
           # -- Quantity
           fill_in "variants[#{variant.id}]", with: 6
+          wait_for_debounce
           expect(page).to have_in_cart product.name
           wait_until { !cart_dirty }
 
-          li = Spree::Order.order(:created_at).last.line_items.order(:created_at).last
-          expect(li.quantity).to eq(6)
+          expect(order.reload.line_items.first.quantity).to eq(6)
 
           # -- Max quantity
           fill_in "variant_attributes[#{variant.id}][max_quantity]", with: 7
+          wait_for_debounce
           wait_until { !cart_dirty }
 
-          li = Spree::Order.order(:created_at).last.line_items.order(:created_at).last
-          expect(li.max_quantity).to eq(7)
+          expect(order.reload.line_items.first.max_quantity).to eq(7)
 
           # -- Reload
           visit shop_path
@@ -536,4 +537,10 @@ feature "As a consumer I want to shop with a distributor", js: true do
     expect(page).to have_no_content "This shop is for customers only."
     expect(page).to have_content product.name
   end
+
+  def wait_for_debounce
+    # The auto-submit on these specific form elements (add to cart) now has a small built-in
+    # waiting period before submitting the data...
+    sleep 0.6
+  end
 end

From 11af5dffdc27f048d147825059303df7f6b8c805 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 20 Apr 2020 22:51:59 +0200
Subject: [PATCH 02/23] Memoize scoper in cart_service

Avoids fetching all of the hub's variant overrides from the db every time it's initialized.
---
 app/services/cart_service.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index 49d157a836..8bb161a98f 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -34,7 +34,7 @@ class CartService
     quantity = quantity.to_i
     max_quantity = max_quantity.to_i if max_quantity
     variant = Spree::Variant.find(variant_id)
-    OpenFoodNetwork::ScopeVariantToHub.new(@distributor).scope(variant)
+    scoper.scope(variant)
 
     return unless quantity > 0 && valid_variant?(variant)
 
@@ -68,6 +68,10 @@ class CartService
 
   private
 
+  def scoper
+    @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
+  end
+
   def read_variants(data)
     @variants_h = read_products_hash(data) +
                   read_variants_hash(data)

From 2334ab6d00e7b5a5ba509cdb38a6a58cc855d5a8 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 20 Apr 2020 23:13:23 +0200
Subject: [PATCH 03/23] Delete some dead code in cart_service

This Bugsnag error was added a year ago and hasn't been seen
---
 app/services/cart_service.rb | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index 8bb161a98f..a7a48123d5 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -73,22 +73,7 @@ class CartService
   end
 
   def read_variants(data)
-    @variants_h = read_products_hash(data) +
-                  read_variants_hash(data)
-  end
-
-  def read_products_hash(data)
-    # This is most probably dead code, this bugsnag notification will confirm it
-    notify_bugsnag(data) if data[:products].present?
-
-    (data[:products] || []).map do |_product_id, variant_id|
-      { variant_id: variant_id, quantity: data[:quantity] }
-    end
-  end
-
-  def notify_bugsnag(data)
-    Bugsnag.notify(RuntimeError.new("CartService.populate called with products hash"),
-                   data: data.as_json)
+    @variants_h = read_variants_hash(data)
   end
 
   def read_variants_hash(data)

From 106bb7a27f9b09f6c61363ce4891c9c77bed8474 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 20 Apr 2020 23:00:50 +0200
Subject: [PATCH 04/23] Rename some variables and add comments for clarity in
 cart_service

---
 app/services/cart_service.rb | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index a7a48123d5..f47f27ccba 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -15,17 +15,17 @@ class CartService
     @distributor, @order_cycle = distributor_and_order_cycle
 
     @order.with_lock do
-      variants = read_variants from_hash
-      attempt_cart_add_variants variants
-      overwrite_variants variants unless !overwrite
+      variants_data = read_variants from_hash
+      attempt_cart_add_variants variants_data
+      overwrite_variants variants_data if overwrite
     end
     valid?
   end
 
-  def attempt_cart_add_variants(variants)
-    variants.each do |v|
-      if varies_from_cart(v)
-        attempt_cart_add(v[:variant_id], v[:quantity], v[:max_quantity])
+  def attempt_cart_add_variants(variants_data)
+    variants_data.each do |variant_data|
+      if varies_from_cart(variant_data)
+        attempt_cart_add(variant_data[:variant_id], variant_data[:quantity], variant_data[:max_quantity])
       end
     end
   end
@@ -99,6 +99,7 @@ class CartService
     [@order.distributor, @order.order_cycle]
   end
 
+  # Returns true if the saved cart differs from what's in the posted data, otherwise false
   def varies_from_cart(variant_data)
     li = line_item_for_variant_id variant_data[:variant_id]
 

From b3242041e5f93f1ff9ef54e7a06a56eadbffb3d2 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 21 Apr 2020 01:25:41 +0200
Subject: [PATCH 05/23] Return earlier (before scoping process, if possible)

---
 app/services/cart_service.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index f47f27ccba..dcd4c23138 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -33,10 +33,12 @@ class CartService
   def attempt_cart_add(variant_id, quantity, max_quantity = nil)
     quantity = quantity.to_i
     max_quantity = max_quantity.to_i if max_quantity
+    return unless quantity > 0
+
     variant = Spree::Variant.find(variant_id)
     scoper.scope(variant)
 
-    return unless quantity > 0 && valid_variant?(variant)
+    return unless valid_variant?(variant)
 
     cart_add(variant, quantity, max_quantity)
   end

From a759d8c7c72391a5b472f3cdfd1eff983f0701b4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 21 Apr 2020 01:29:21 +0200
Subject: [PATCH 06/23] Avoid N+1s for variants and for line_items of variants

---
 app/services/cart_service.rb | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index dcd4c23138..c1736f60d6 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -23,21 +23,25 @@ class CartService
   end
 
   def attempt_cart_add_variants(variants_data)
+    loaded_variants = indexed_variants(variants_data)
+
     variants_data.each do |variant_data|
-      if varies_from_cart(variant_data)
-        attempt_cart_add(variant_data[:variant_id], variant_data[:quantity], variant_data[:max_quantity])
+      loaded_variant = loaded_variants[variant_data[:variant_id].to_i]
+
+      if varies_from_cart(variant_data, loaded_variant)
+        attempt_cart_add(
+          loaded_variant, variant_data[:quantity], variant_data[:max_quantity]
+        )
       end
     end
   end
 
-  def attempt_cart_add(variant_id, quantity, max_quantity = nil)
+  def attempt_cart_add(variant, quantity, max_quantity = nil)
     quantity = quantity.to_i
     max_quantity = max_quantity.to_i if max_quantity
     return unless quantity > 0
 
-    variant = Spree::Variant.find(variant_id)
     scoper.scope(variant)
-
     return unless valid_variant?(variant)
 
     cart_add(variant, quantity, max_quantity)
@@ -102,8 +106,8 @@ class CartService
   end
 
   # Returns true if the saved cart differs from what's in the posted data, otherwise false
-  def varies_from_cart(variant_data)
-    li = line_item_for_variant_id variant_data[:variant_id]
+  def varies_from_cart(variant_data, loaded_variant)
+    li = line_item_for_variant loaded_variant
 
     li_added = li.nil? && (variant_data[:quantity].to_i > 0 || variant_data[:max_quantity].to_i > 0)
     li_quantity_changed     = li.present? && li.quantity.to_i     != variant_data[:quantity].to_i
@@ -135,11 +139,22 @@ class CartService
     false
   end
 
-  def line_item_for_variant_id(variant_id)
-    order.find_line_item_by_variant Spree::Variant.find(variant_id)
+  def line_item_for_variant(variant)
+    order.find_line_item_by_variant variant
   end
 
   def variant_ids_in_cart
     @order.line_items.pluck :variant_id
   end
+
+  def indexed_variants(variants_data)
+    @indexed_variants ||= begin
+      variant_ids_in_data = variants_data.map{ |v| v[:variant_id] }
+
+      Spree::Variant.where(id: variant_ids_in_data).
+        includes(:stock_items, :product).
+        all.
+        index_by(&:id)
+    end
+  end
 end

From 25525d4f75aff5cddc0859f9722198e8b18b239c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 21 Apr 2020 01:31:55 +0200
Subject: [PATCH 07/23] Use guard clause in each block

---
 app/services/cart_service.rb | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index c1736f60d6..cf6ffc40b4 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -27,12 +27,11 @@ class CartService
 
     variants_data.each do |variant_data|
       loaded_variant = loaded_variants[variant_data[:variant_id].to_i]
+      next unless varies_from_cart(variant_data, loaded_variant)
 
-      if varies_from_cart(variant_data, loaded_variant)
-        attempt_cart_add(
-          loaded_variant, variant_data[:quantity], variant_data[:max_quantity]
-        )
-      end
+      attempt_cart_add(
+        loaded_variant, variant_data[:quantity], variant_data[:max_quantity]
+      )
     end
   end
 

From 1152f307e2903c1a0a8ba3d4f6094e8a155e6829 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 21 Apr 2020 01:33:59 +0200
Subject: [PATCH 08/23] Eager-load associated line_items data in
 #update_distribution_charge!

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

diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb
index 85b86c4169..c202130632 100644
--- a/app/models/spree/order_decorator.rb
+++ b/app/models/spree/order_decorator.rb
@@ -237,7 +237,10 @@ Spree::Order.class_eval do
     with_lock do
       EnterpriseFee.clear_all_adjustments_on_order self
 
-      line_items.each do |line_item|
+      loaded_line_items =
+        line_items.includes(variant: :product, order: [:distributor, :order_cycle]).all
+
+      loaded_line_items.each do |line_item|
         if provided_by_order_cycle? line_item
           OpenFoodNetwork::EnterpriseFeeCalculator.new.create_line_item_adjustments_for line_item
         end

From 468cb3f57e12e7006d7c0f92615b79f2575ffbd4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 24 Apr 2020 15:08:42 +0200
Subject: [PATCH 09/23] Remove obviously private methods from the public
 interface in CartService

These methods are not called from anywhere in the app, only in a couple of tests in cart_service_spec.
---
 app/services/cart_service.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index cf6ffc40b4..5bb54c6dbb 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -22,6 +22,8 @@ class CartService
     valid?
   end
 
+  private
+
   def attempt_cart_add_variants(variants_data)
     loaded_variants = indexed_variants(variants_data)
 
@@ -71,8 +73,6 @@ class CartService
     end
   end
 
-  private
-
   def scoper
     @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
   end

From e33de8a20e0c0167e2c33b8f2ebbf9de9321bf68 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 24 Apr 2020 15:59:10 +0200
Subject: [PATCH 10/23] Update specs and refactor a bit

---
 spec/models/spree/order_spec.rb    |  5 +--
 spec/services/cart_service_spec.rb | 72 +++++++++++++++++-------------
 2 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb
index 893e450eff..1b5d6cdb35 100644
--- a/spec/models/spree/order_spec.rb
+++ b/spec/models/spree/order_spec.rb
@@ -33,7 +33,6 @@ describe Spree::Order do
 
     it "skips order cycle per-order adjustments for orders that don't have an order cycle" do
       allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
-      allow(subject).to receive(:line_items) { [] }
 
       allow(subject).to receive(:order_cycle) { nil }
 
@@ -42,8 +41,7 @@ describe Spree::Order do
 
     it "ensures the correct adjustment(s) are created for order cycles" do
       allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
-      line_item = double(:line_item)
-      allow(subject).to receive(:line_items) { [line_item] }
+      line_item = create(:line_item, order: subject)
       allow(subject).to receive(:provided_by_order_cycle?) { true }
 
       order_cycle = double(:order_cycle)
@@ -58,7 +56,6 @@ describe Spree::Order do
 
     it "ensures the correct per-order adjustment(s) are created for order cycles" do
       allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
-      allow(subject).to receive(:line_items) { [] }
 
       order_cycle = double(:order_cycle)
       expect_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator).
diff --git a/spec/services/cart_service_spec.rb b/spec/services/cart_service_spec.rb
index 1f6b03949d..0baa3abe26 100644
--- a/spec/services/cart_service_spec.rb
+++ b/spec/services/cart_service_spec.rb
@@ -52,49 +52,59 @@ describe CartService do
   end
 
   describe "varies_from_cart" do
-    let(:variant) { double(:variant, id: 123) }
+    let!(:variant) { create(:variant) }
 
     it "returns true when item is not in cart and a quantity is specified" do
-      expect(cart_service).to receive(:line_item_for_variant_id).with(variant.id).and_return(nil)
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '2')).to be true
+      variant_data = { variant_id: variant.id, quantity: '2'}
+
+      expect(cart_service).to receive(:line_item_for_variant).with(variant).and_return(nil)
+      expect(cart_service.send(:varies_from_cart, variant_data, variant )).to be true
     end
 
     it "returns true when item is not in cart and a max_quantity is specified" do
-      expect(cart_service).to receive(:line_item_for_variant_id).with(variant.id).and_return(nil)
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '0', max_quantity: '2')).to be true
+      variant_data = { variant_id: variant.id, quantity: '0', max_quantity: '2' }
+
+      expect(cart_service).to receive(:line_item_for_variant).with(variant).and_return(nil)
+      expect(cart_service.send(:varies_from_cart, variant_data, variant)).to be true
     end
 
     it "returns false when item is not in cart and no quantity or max_quantity are specified" do
-      expect(cart_service).to receive(:line_item_for_variant_id).with(variant.id).and_return(nil)
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '0')).to be false
+      variant_data = { variant_id: variant.id, quantity: '0' }
+
+      expect(cart_service).to receive(:line_item_for_variant).with(variant).and_return(nil)
+      expect(cart_service.send(:varies_from_cart, variant_data, variant)).to be false
     end
 
     it "returns true when quantity varies" do
-      li = double(:line_item, quantity: 1, max_quantity: nil)
-      allow(cart_service).to receive(:line_item_for_variant_id) { li }
+      variant_data = { variant_id: variant.id, quantity: '2' }
+      line_item = double(:line_item, quantity: 1, max_quantity: nil)
+      allow(cart_service).to receive(:line_item_for_variant) { line_item }
 
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '2')).to be true
+      expect(cart_service.send(:varies_from_cart, variant_data, variant)).to be true
     end
 
     it "returns true when max_quantity varies" do
-      li = double(:line_item, quantity: 1, max_quantity: nil)
-      allow(cart_service).to receive(:line_item_for_variant_id) { li }
+      variant_data = { variant_id: variant.id, quantity: '1', max_quantity: '3' }
+      line_item = double(:line_item, quantity: 1, max_quantity: nil)
+      allow(cart_service).to receive(:line_item_for_variant) { line_item }
 
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '1', max_quantity: '3')).to be true
+      expect(cart_service.send(:varies_from_cart, variant_data, variant)).to be true
     end
 
     it "returns false when max_quantity varies only in nil vs 0" do
-      li = double(:line_item, quantity: 1, max_quantity: nil)
-      allow(cart_service).to receive(:line_item_for_variant_id) { li }
+      variant_data = { variant_id: variant.id, quantity: '1' }
+      line_item = double(:line_item, quantity: 1, max_quantity: nil)
+      allow(cart_service).to receive(:line_item_for_variant) { line_item }
 
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '1')).to be false
+      expect(cart_service.send(:varies_from_cart, variant_data, variant)).to be false
     end
 
     it "returns false when both are specified and neither varies" do
-      li = double(:line_item, quantity: 1, max_quantity: 2)
-      allow(cart_service).to receive(:line_item_for_variant_id) { li }
+      variant_data = { variant_id: variant.id, quantity: '1', max_quantity: '2' }
+      line_item = double(:line_item, quantity: 1, max_quantity: 2)
+      allow(cart_service).to receive(:line_item_for_variant) { line_item }
 
-      expect(cart_service.send(:varies_from_cart, variant_id: variant.id, quantity: '1', max_quantity: '2')).to be false
+      expect(cart_service.send(:varies_from_cart, variant_data, variant)).to be false
     end
   end
 
@@ -111,7 +121,9 @@ describe CartService do
 
     it "returns nothing when items are added to cart" do
       allow(cart_service).to receive(:variant_ids_in_cart) { [123] }
-      expect(cart_service.send(:variants_removed, [{ variant_id: '123' }, { variant_id: '456' }])).to eq([])
+      expect(
+        cart_service.send(:variants_removed, [{ variant_id: '123' }, { variant_id: '456' }])
+      ).to eq([])
     end
 
     it "does not return duplicates" do
@@ -121,7 +133,7 @@ describe CartService do
   end
 
   describe "attempt_cart_add" do
-    let(:variant) { double(:variant, on_hand: 250) }
+    let!(:variant) { create(:variant, on_hand: 250) }
     let(:quantity) { 123 }
 
     before do
@@ -136,7 +148,7 @@ describe CartService do
       expect(variant).to receive(:on_demand).and_return(false)
       expect(order).to receive(:add_variant).with(variant, quantity, nil, currency)
 
-      cart_service.attempt_cart_add(333, quantity.to_s)
+      cart_service.send(:attempt_cart_add, variant, quantity.to_s)
     end
 
     it "filters quantities through #quantities_to_add" do
@@ -148,7 +160,7 @@ describe CartService do
 
       expect(order).to receive(:add_variant).with(variant, 5, 5, currency)
 
-      cart_service.attempt_cart_add(333, quantity.to_s, quantity.to_s)
+      cart_service.send(:attempt_cart_add, variant, quantity.to_s, quantity.to_s)
     end
 
     it "removes variants which have become out of stock" do
@@ -161,7 +173,7 @@ describe CartService do
       expect(order).to receive(:remove_variant).with(variant)
       expect(order).to receive(:add_variant).never
 
-      cart_service.attempt_cart_add(333, quantity.to_s, quantity.to_s)
+      cart_service.send(:attempt_cart_add, variant, quantity.to_s, quantity.to_s)
     end
   end
 
@@ -175,21 +187,21 @@ describe CartService do
 
       context "when max_quantity is not provided" do
         it "returns full amount when available" do
-          expect(cart_service.quantities_to_add(v, 5, nil)).to eq([5, nil])
+          expect(cart_service.send(:quantities_to_add, v, 5, nil)).to eq([5, nil])
         end
 
         it "returns a limited amount when not entirely available" do
-          expect(cart_service.quantities_to_add(v, 15, nil)).to eq([10, nil])
+          expect(cart_service.send(:quantities_to_add, v, 15, nil)).to eq([10, nil])
         end
       end
 
       context "when max_quantity is provided" do
         it "returns full amount when available" do
-          expect(cart_service.quantities_to_add(v, 5, 6)).to eq([5, 6])
+          expect(cart_service.send(:quantities_to_add, v, 5, 6)).to eq([5, 6])
         end
 
         it "also returns the full amount when not entirely available" do
-          expect(cart_service.quantities_to_add(v, 15, 16)).to eq([10, 16])
+          expect(cart_service.send(:quantities_to_add, v, 15, 16)).to eq([10, 16])
         end
       end
     end
@@ -200,11 +212,11 @@ describe CartService do
       end
 
       it "does not limit quantity" do
-        expect(cart_service.quantities_to_add(v, 15, nil)).to eq([15, nil])
+        expect(cart_service.send(:quantities_to_add, v, 15, nil)).to eq([15, nil])
       end
 
       it "does not limit max_quantity" do
-        expect(cart_service.quantities_to_add(v, 15, 16)).to eq([15, 16])
+        expect(cart_service.send(:quantities_to_add, v, 15, 16)).to eq([15, 16])
       end
     end
   end

From a5c4364f929dd06b0bec70de6ad153b9c9471cb4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 00:52:09 +0200
Subject: [PATCH 11/23] Fetch (or create) current_order only once

---
 app/controllers/cart_controller.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/app/controllers/cart_controller.rb b/app/controllers/cart_controller.rb
index d452228299..52c8c3caac 100644
--- a/app/controllers/cart_controller.rb
+++ b/app/controllers/cart_controller.rb
@@ -4,24 +4,26 @@ class CartController < BaseController
   before_filter :check_authorization
 
   def populate
+    order = current_order(true)
+
     # Without intervention, the Spree::Adjustment#update_adjustable callback is called many times
     # during cart population, for both taxation and enterprise fees. This operation triggers a
     # costly Spree::Order#update!, which only needs to be run once. We avoid this by disabling
     # callbacks on Spree::Adjustment and then manually invoke Spree::Order#update! on success.
     Spree::Adjustment.without_callbacks do
-      cart_service = CartService.new(current_order(true))
+      cart_service = CartService.new(order)
 
       if cart_service.populate(params.slice(:products, :variants, :quantity), true)
         fire_event('spree.cart.add')
         fire_event('spree.order.contents_changed')
 
-        current_order.cap_quantity_at_stock!
-        current_order.update!
+        order.cap_quantity_at_stock!
+        order.update!
 
         variant_ids = variant_ids_in(cart_service.variants_h)
 
         render json: { error: false,
-                       stock_levels: VariantsStockLevels.new.call(current_order, variant_ids) },
+                       stock_levels: VariantsStockLevels.new.call(order, variant_ids) },
                status: :ok
       else
         render json: { error: true }, status: :precondition_failed

From 43869fc140b3adabc03f40209997a413d873170d Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 00:56:18 +0200
Subject: [PATCH 12/23] Replace fired events with clearer method call

The 'spree.cart.add' event has no listeners in spree_core on in ofn. The 'spree.order.contents_changed' just has a single listener that calls `order.update_distribution_charge`.
---
 app/controllers/cart_controller.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/controllers/cart_controller.rb b/app/controllers/cart_controller.rb
index 52c8c3caac..c1a77b433b 100644
--- a/app/controllers/cart_controller.rb
+++ b/app/controllers/cart_controller.rb
@@ -14,9 +14,7 @@ class CartController < BaseController
       cart_service = CartService.new(order)
 
       if cart_service.populate(params.slice(:products, :variants, :quantity), true)
-        fire_event('spree.cart.add')
-        fire_event('spree.order.contents_changed')
-
+        order.update_distribution_charge!
         order.cap_quantity_at_stock!
         order.update!
 

From eb858159ce44b520218f5affac35e619f78fb803 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 01:04:22 +0200
Subject: [PATCH 13/23] Eager-load :default_price on variants

---
 app/services/cart_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index 5bb54c6dbb..8d837d9eb8 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -151,7 +151,7 @@ class CartService
       variant_ids_in_data = variants_data.map{ |v| v[:variant_id] }
 
       Spree::Variant.where(id: variant_ids_in_data).
-        includes(:stock_items, :product).
+        includes(:default_price, :stock_items, :product).
         all.
         index_by(&:id)
     end

From fe2bf8d53194b903b4b7c1779668b613172491a4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 01:05:42 +0200
Subject: [PATCH 14/23] Eager-load variants and stock items when doing stock
 checks on multiple variants

---
 app/services/variants_stock_levels.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb
index f06002db19..0d778b9a0d 100644
--- a/app/services/variants_stock_levels.rb
+++ b/app/services/variants_stock_levels.rb
@@ -5,7 +5,7 @@ require 'open_food_network/scope_variant_to_hub'
 
 class VariantsStockLevels
   def call(order, requested_variant_ids)
-    variant_stock_levels = variant_stock_levels(order.line_items)
+    variant_stock_levels = variant_stock_levels(order.line_items.includes(variant: :stock_items))
 
     order_variant_ids = variant_stock_levels.keys
     missing_variants = Spree::Variant.includes(:stock_items).

From 3fa2b3161fb69bdd6a7eb1410bb1e8a7b5e3d223 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 01:07:15 +0200
Subject: [PATCH 15/23] Avoid N+1s when using OrderCycle#exchanges_supplying

---
 app/models/order_cycle.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb
index c4b0bbcda3..9c7837c4c4 100644
--- a/app/models/order_cycle.rb
+++ b/app/models/order_cycle.rb
@@ -240,7 +240,8 @@ class OrderCycle < ActiveRecord::Base
   end
 
   def exchanges_supplying(order)
-    exchanges.supplying_to(order.distributor).with_any_variant(order.variants.map(&:id))
+    variants_relation = Spree::Variant.joins(:line_items).merge(Spree::LineItem.in_orders(order))
+    exchanges.supplying_to(order.distributor).with_any_variant(variants_relation)
   end
 
   def coordinated_by?(user)

From 74e81b078f6fbf115d302074c8ac888f5b3f6eea Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 01:11:28 +0200
Subject: [PATCH 16/23] Avoid N+1s in Order#cap_quantity_at_stock!

---
 app/models/spree/order_decorator.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb
index c202130632..8c2bc9496c 100644
--- a/app/models/spree/order_decorator.rb
+++ b/app/models/spree/order_decorator.rb
@@ -210,7 +210,7 @@ Spree::Order.class_eval do
   end
 
   def cap_quantity_at_stock!
-    line_items.each(&:cap_quantity_at_stock!)
+    line_items.includes(variant: :stock_items).all.each(&:cap_quantity_at_stock!)
   end
 
   def set_distributor!(distributor)

From d3af3d3f27dd1df9ab7f55e1191eb3d01fff7724 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 28 Apr 2020 01:15:10 +0200
Subject: [PATCH 17/23] Avoid extra query on stock_items every time #on_demand
 is called on a variant.

In the case where the variant has not been saved yet, we can use #new_record? here instead of #stock_items.empty?, to avoid an additional query. This can be called a vast number of times per request, in various N+1s. The other case where we need to return here is when a variant has been deleted, so #stock_items will be empty and #stock_item will be nil. Likewise, we can just check that with #deleted? and avoid #stock_items.empty?
---
 app/models/concerns/variant_stock.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb
index 1cc0ec4e04..5a6094adf4 100644
--- a/app/models/concerns/variant_stock.rb
+++ b/app/models/concerns/variant_stock.rb
@@ -40,9 +40,9 @@ module VariantStock
 
   # Checks whether this variant is produced on demand.
   def on_demand
-    # A variant that has not been saved yet, doesn't have a stock item
+    # A variant that has not been saved yet or has been soft-deleted doesn't have a stock item
     #   This provides a default value for variant.on_demand using Spree::StockLocation.backorderable_default
-    return Spree::StockLocation.first.backorderable_default if stock_items.empty?
+    return Spree::StockLocation.first.backorderable_default if new_record? || deleted?
 
     stock_item.backorderable?
   end

From 92a881c584eca0f2bd62a685c26b03489a4b5d73 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 29 Apr 2020 17:19:16 +0200
Subject: [PATCH 18/23] Simplify relation used in #exchanges_supplying

---
 app/models/order_cycle.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb
index 9c7837c4c4..607d49b061 100644
--- a/app/models/order_cycle.rb
+++ b/app/models/order_cycle.rb
@@ -240,8 +240,8 @@ class OrderCycle < ActiveRecord::Base
   end
 
   def exchanges_supplying(order)
-    variants_relation = Spree::Variant.joins(:line_items).merge(Spree::LineItem.in_orders(order))
-    exchanges.supplying_to(order.distributor).with_any_variant(variants_relation)
+    variant_ids_relation = Spree::LineItem.in_orders(order).select(:variant_id)
+    exchanges.supplying_to(order.distributor).with_any_variant(variant_ids_relation)
   end
 
   def coordinated_by?(user)

From 2bdda7de04fe0e0a9f7bd288d4b095e8dd24bda5 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 29 Apr 2020 17:22:46 +0200
Subject: [PATCH 19/23] Change order of methods for easier reading

---
 app/services/cart_service.rb | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index 8d837d9eb8..90b67450cf 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -37,6 +37,17 @@ class CartService
     end
   end
 
+  def indexed_variants(variants_data)
+    @indexed_variants ||= begin
+      variant_ids_in_data = variants_data.map{ |v| v[:variant_id] }
+
+      Spree::Variant.where(id: variant_ids_in_data).
+        includes(:default_price, :stock_items, :product).
+        all.
+        index_by(&:id)
+    end
+  end
+
   def attempt_cart_add(variant, quantity, max_quantity = nil)
     quantity = quantity.to_i
     max_quantity = max_quantity.to_i if max_quantity
@@ -145,15 +156,4 @@ class CartService
   def variant_ids_in_cart
     @order.line_items.pluck :variant_id
   end
-
-  def indexed_variants(variants_data)
-    @indexed_variants ||= begin
-      variant_ids_in_data = variants_data.map{ |v| v[:variant_id] }
-
-      Spree::Variant.where(id: variant_ids_in_data).
-        includes(:default_price, :stock_items, :product).
-        all.
-        index_by(&:id)
-    end
-  end
 end

From 4054bdd7225943ad2803a3d4f3d8a1f42b9b2a70 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 29 Apr 2020 17:33:49 +0200
Subject: [PATCH 20/23] Add Bugsnag call for "variant with no stock item" case

---
 app/models/concerns/variant_stock.rb | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb
index 5a6094adf4..dedf667c1b 100644
--- a/app/models/concerns/variant_stock.rb
+++ b/app/models/concerns/variant_stock.rb
@@ -44,6 +44,15 @@ module VariantStock
     #   This provides a default value for variant.on_demand using Spree::StockLocation.backorderable_default
     return Spree::StockLocation.first.backorderable_default if new_record? || deleted?
 
+    # This can be removed unless we have seen this error in Bugsnag recently
+    if stock_item.nil?
+      Bugsnag.notify(
+        RuntimeError.new("Variant #stock_item called, but the stock_item does not exist!"),
+        object: as_json
+      )
+      return Spree::StockLocation.first.backorderable_default
+    end
+
     stock_item.backorderable?
   end
 

From f2cd122ec8937824a22bc5e795e4764acf69d4b3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 29 Apr 2020 18:26:17 +0200
Subject: [PATCH 21/23] Update variant_stock_spec for unsaved and soft-deleted
 cases

---
 spec/models/concerns/variant_stock_spec.rb | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/spec/models/concerns/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb
index e417e3a203..0c9aa30a25 100644
--- a/spec/models/concerns/variant_stock_spec.rb
+++ b/spec/models/concerns/variant_stock_spec.rb
@@ -90,13 +90,33 @@ describe VariantStock do
       end
     end
 
-    context 'when the variant has no stock item' do
+    context 'when the variant has not been saved yet' do
       let(:variant) { build(:variant) }
 
+      it 'has no stock items' do
+        expect(variant.stock_items.count).to eq 0
+      end
+
       it 'returns stock location default' do
         expect(variant.on_demand).to be_falsy
       end
     end
+
+    context 'when the variant has been soft-deleted' do
+      let(:deleted_variant) { create(:variant) }
+
+      before do
+        deleted_variant.destroy
+      end
+
+      it 'has no stock items' do
+        expect(deleted_variant.stock_items.count).to eq 0
+      end
+
+      it 'returns stock location default' do
+        expect(deleted_variant.on_demand).to be_falsy
+      end
+    end
   end
 
   describe '#on_demand=' do

From a3757992b5e0aa5408e478a7cf9ebe9993ed9a85 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 5 May 2020 09:55:14 +0200
Subject: [PATCH 22/23] Use #tap to destroy and remove before block

---
 spec/models/concerns/variant_stock_spec.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/spec/models/concerns/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb
index 0c9aa30a25..160f802df9 100644
--- a/spec/models/concerns/variant_stock_spec.rb
+++ b/spec/models/concerns/variant_stock_spec.rb
@@ -103,11 +103,7 @@ describe VariantStock do
     end
 
     context 'when the variant has been soft-deleted' do
-      let(:deleted_variant) { create(:variant) }
-
-      before do
-        deleted_variant.destroy
-      end
+      let(:deleted_variant) { create(:variant).tap(&:destroy) }
 
       it 'has no stock items' do
         expect(deleted_variant.stock_items.count).to eq 0

From 805f91e838913f02f0cc08504165f47134330a55 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Tue, 5 May 2020 09:56:08 +0200
Subject: [PATCH 23/23] Remove unnecessary #all call

---
 app/services/cart_service.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb
index 90b67450cf..5818f8f91f 100644
--- a/app/services/cart_service.rb
+++ b/app/services/cart_service.rb
@@ -43,7 +43,6 @@ class CartService
 
       Spree::Variant.where(id: variant_ids_in_data).
         includes(:default_price, :stock_items, :product).
-        all.
         index_by(&:id)
     end
   end