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/app/controllers/cart_controller.rb b/app/controllers/cart_controller.rb index d452228299..c1a77b433b 100644 --- a/app/controllers/cart_controller.rb +++ b/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 diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index 1cc0ec4e04..dedf667c1b 100644 --- a/app/models/concerns/variant_stock.rb +++ b/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 diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index c4b0bbcda3..607d49b061 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)) + 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) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 7fa3af05e6..8c4e7dc6c7 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) @@ -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 diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb index 49d157a836..5818f8f91f 100644 --- a/app/services/cart_service.rb +++ b/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 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). 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 diff --git a/spec/models/concerns/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb index e417e3a203..160f802df9 100644 --- a/spec/models/concerns/variant_stock_spec.rb +++ b/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 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