Merge pull request #5289 from Matt-Yorkley/cart-populate

Cart populate

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",

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"}

app/controllers/cart_controller.rb

@@ -4,24 +4,24 @@ 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.update_distribution_charge!
+        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

app/models/concerns/variant_stock.rb

@@ -40,9 +40,18 @@ 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?
+
+    # 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

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))
+    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)

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)
@@ -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

app/services/cart_service.rb

@@ -15,28 +15,45 @@ 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])
-      end
+  private
+
+  def attempt_cart_add_variants(variants_data)
+    loaded_variants = indexed_variants(variants_data)
+
+    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)
+
+      attempt_cart_add(
+        loaded_variant, variant_data[:quantity], variant_data[:max_quantity]
+      )
     end
   end
 
-  def attempt_cart_add(variant_id, quantity, max_quantity = nil)
+  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).
+        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
-    variant = Spree::Variant.find(variant_id)
-    OpenFoodNetwork::ScopeVariantToHub.new(@distributor).scope(variant)
+    return unless quantity > 0
 
-    return unless quantity > 0 && valid_variant?(variant)
+    scoper.scope(variant)
+    return unless valid_variant?(variant)
 
     cart_add(variant, quantity, max_quantity)
   end
@@ -66,25 +83,12 @@ class CartService
     end
   end
 
-  private
+  def scoper
+    @scoper ||= OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
+  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)
@@ -110,8 +114,9 @@ class CartService
     [@order.distributor, @order.order_cycle]
   end
 
-  def varies_from_cart(variant_data)
-    li = line_item_for_variant_id variant_data[:variant_id]
+  # Returns true if the saved cart differs from what's in the posted data, otherwise false
+  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
@@ -143,8 +148,8 @@ 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

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).

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

spec/models/concerns/variant_stock_spec.rb

@@ -90,13 +90,29 @@ 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).tap(&:destroy) }
+
+      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

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).

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