diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 4ca328a5b1..1aa21bf193 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -102,6 +102,13 @@ Spree::Order.class_eval do end end + def remove_variant(variant) + line_items(:reload) + current_item = find_line_item_by_variant(variant) + current_item.destroy + end + + # Overridden to support max_quantity def add_variant(variant, quantity = 1, max_quantity = nil, currency = nil) line_items(:reload) diff --git a/app/models/spree/order_populator_decorator.rb b/app/models/spree/order_populator_decorator.rb index cd80d17b97..1ca0f9efc2 100644 --- a/app/models/spree/order_populator_decorator.rb +++ b/app/models/spree/order_populator_decorator.rb @@ -11,25 +11,42 @@ Spree::OrderPopulator.class_eval do if valid? @order.with_lock do - @order.empty! if overwrite + variants = read_products_hash(from_hash) + + read_variants_hash(from_hash) - from_hash[:products].each do |product_id, variant_id| - attempt_cart_add(variant_id, from_hash[:quantity]) - end if from_hash[:products] - - from_hash[:variants].each do |variant_id, quantity| - if quantity.is_a?(Hash) - attempt_cart_add(variant_id, quantity[:quantity], quantity[:max_quantity]) - else - attempt_cart_add(variant_id, quantity) + variants.each do |v| + if varies_from_cart(v) + attempt_cart_add(v[:variant_id], v[:quantity], v[:max_quantity]) end - end if from_hash[:variants] + end + + if overwrite + variants_removed(variants).each do |id| + cart_remove(id) + end + end end end valid? end + def read_products_hash(data) + (data[:products] || []).map do |product_id, variant_id| + {variant_id: variant_id, quantity: data[:quantity]} + end + end + + def read_variants_hash(data) + (data[:variants] || []).map do |variant_id, quantity| + if quantity.is_a?(Hash) + {variant_id: variant_id, quantity: quantity[:quantity], max_quantity: quantity[:max_quantity]} + else + {variant_id: variant_id, quantity: quantity} + end + end + end + def attempt_cart_add(variant_id, quantity, max_quantity = nil) quantity = quantity.to_i variant = Spree::Variant.find(variant_id) @@ -43,6 +60,11 @@ Spree::OrderPopulator.class_eval do end end + def cart_remove(variant_id) + variant = Spree::Variant.find(variant_id) + @order.remove_variant(variant) + end + private @@ -54,6 +76,22 @@ Spree::OrderPopulator.class_eval do DistributionChangeValidator.new(@order).can_change_to_distribution?(distributor, order_cycle) end + def varies_from_cart(variant_data) + li = line_item_for_variant_id variant_data[:variant_id] + + 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 + li_max_quantity_changed = li.present? && li.max_quantity.to_i != variant_data[:max_quantity].to_i + + li_added || li_quantity_changed || li_max_quantity_changed + end + + def variants_removed(variants_data) + variant_ids_given = variants_data.map { |data| data[:variant_id].to_i } + + (variant_ids_in_cart - variant_ids_given).uniq + end + def check_order_cycle_provided_for(variant) order_cycle_provided = (!order_cycle_required_for(variant) || @order_cycle.present?) errors.add(:base, "Please choose an order cycle for this order.") unless order_cycle_provided @@ -72,4 +110,12 @@ Spree::OrderPopulator.class_eval do def order_cycle_required_for(variant) variant.product.product_distributions.empty? end + + def line_item_for_variant_id(variant_id) + order.find_line_item_by_variant Spree::Variant.find(variant_id) + end + + def variant_ids_in_cart + @order.line_items.pluck :variant_id + end end diff --git a/app/views/shared/menu/_alert.html.haml b/app/views/shared/menu/_alert.html.haml index 60b1e9f18c..b6ee3acfb1 100644 --- a/app/views/shared/menu/_alert.html.haml +++ b/app/views/shared/menu/_alert.html.haml @@ -1,6 +1,6 @@ .text-center.page-alert.fixed{ "ofn-page-alert" => true } .alert-box - %a.alert-cta{href: "http://www.openfoodnetwork.org", target: "_blank"} + %a.alert-cta{href: registration_path, target: "_blank"} %h6 = t 'alert_selling_on_ofn'   diff --git a/app/views/shared/menu/_cart.html.haml b/app/views/shared/menu/_cart.html.haml index b81352df37..f67ff2ffe1 100644 --- a/app/views/shared/menu/_cart.html.haml +++ b/app/views/shared/menu/_cart.html.haml @@ -1,4 +1,4 @@ -%span.cart-span{"ng-controller" => "CartCtrl", "ng-class" => "{ dirty: Cart.dirty || Cart.empty() }"} +%span.cart-span{"ng-controller" => "CartCtrl", "ng-class" => "{ dirty: Cart.dirty || Cart.empty(), 'pure-dirty': Cart.dirty }"} %a#cart.icon{cart: true} %span.nav-branded %i.ofn-i_027-shopping-cart diff --git a/spec/features/consumer/shopping/shopping_spec.rb b/spec/features/consumer/shopping/shopping_spec.rb index f1bed3a276..12e9d8323b 100644 --- a/spec/features/consumer/shopping/shopping_spec.rb +++ b/spec/features/consumer/shopping/shopping_spec.rb @@ -206,18 +206,29 @@ feature "As a consumer I want to shop with a distributor", js: true do end end - describe "adding products to cart" do + describe "adding and removing products from cart" do let(:exchange) { Exchange.find(oc1.exchanges.to_enterprises(distributor).outgoing.first.id) } let(:product) { create(:simple_product) } let(:variant) { create(:variant, product: product) } + before do add_product_and_variant_to_order_cycle(exchange, product, variant) set_order_cycle(order, oc1) visit shop_path end - it "should let us add products to our cart" do - fill_in "variants[#{variant.id}]", with: "1" + + it "lets us add and remove products from our cart" do + fill_in "variants[#{variant.id}]", with: '1' page.should have_in_cart product.name + wait_until { !cart_dirty } + li = Spree::Order.order(:created_at).last.line_items.order(:created_at).last + li.quantity.should == 1 + + fill_in "variants[#{variant.id}]", with: '0' + within('li.cart') { page.should_not have_content product.name } + wait_until { !cart_dirty } + + Spree::LineItem.where(id: li).should be_empty end end diff --git a/spec/models/spree/order_populator_spec.rb b/spec/models/spree/order_populator_spec.rb index ef72106e16..b32e87f7f9 100644 --- a/spec/models/spree/order_populator_spec.rb +++ b/spec/models/spree/order_populator_spec.rb @@ -9,11 +9,49 @@ module Spree let(:order_cycle) { double(:order_cycle) } let(:op) { OrderPopulator.new(order, currency) } + context "end-to-end" do + let(:order) { create(:order, distributor: distributor, order_cycle: order_cycle) } + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor], variants: [v]) } + let(:op) { OrderPopulator.new(order, nil) } + let(:v) { create(:variant) } + + describe "populate" do + it "adds a variant" do + op.populate({variants: {v.id.to_s => {quantity: '1', max_quantity: '2'}}}, true) + li = order.find_line_item_by_variant(v) + li.should be + li.quantity.should == 1 + li.max_quantity.should == 2 + end + + it "updates a variant's quantity and max quantity" do + order.add_variant v, 1, 2 + + op.populate({variants: {v.id.to_s => {quantity: '2', max_quantity: '3'}}}, true) + li = order.find_line_item_by_variant(v) + li.should be + li.quantity.should == 2 + li.max_quantity.should == 3 + end + + it "removes a variant" do + order.add_variant v, 1, 2 + + op.populate({variants: {}}, true) + order.line_items(:reload) + li = order.find_line_item_by_variant(v) + li.should_not be + end + end + end + describe "populate" do before do op.should_receive(:distributor_and_order_cycle). and_return([distributor, order_cycle]) end + it "checks that distribution can supply all products in the cart" do op.should_receive(:distribution_can_supply_products_in_cart). with(distributor, order_cycle).and_return(false) @@ -22,13 +60,6 @@ module Spree op.errors.to_a.should == ["That distributor or order cycle can't supply all the products in your cart. Please choose another."] end - it "empties the order if override is true" do - op.stub(:distribution_can_supply_products_in_cart).and_return true - order.stub(:with_lock).and_yield - order.should_receive(:empty!) - op.populate(params, true) - end - it "locks the order" do op.stub(:distribution_can_supply_products_in_cart).and_return(true) order.should_receive(:with_lock) @@ -37,14 +68,84 @@ module Spree it "attempts cart add with max_quantity" do op.stub(:distribution_can_supply_products_in_cart).and_return true - order.should_receive(:empty!) params = {variants: {"1" => {quantity: 1, max_quantity: 2}}} order.stub(:with_lock).and_yield + op.stub(:varies_from_cart) { true } + op.stub(:variants_removed) { [] } op.should_receive(:attempt_cart_add).with("1", 1, 2).and_return true op.populate(params, true) end end + describe "varies_from_cart" do + let(:variant) { double(:variant, id: 123) } + + it "returns true when item is not in cart and a quantity is specified" do + op.should_receive(:line_item_for_variant_id).with(variant.id).and_return(nil) + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '2'}).should be_true + end + + it "returns true when item is not in cart and a max_quantity is specified" do + op.should_receive(:line_item_for_variant_id).with(variant.id).and_return(nil) + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '0', max_quantity: '2'}).should be_true + end + + it "returns false when item is not in cart and no quantity or max_quantity are specified" do + op.should_receive(:line_item_for_variant_id).with(variant.id).and_return(nil) + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '0'}).should be_false + end + + it "returns true when quantity varies" do + li = double(:line_item, quantity: 1, max_quantity: nil) + op.stub(:line_item_for_variant_id) { li } + + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '2'}).should be_true + end + + it "returns true when max_quantity varies" do + li = double(:line_item, quantity: 1, max_quantity: nil) + op.stub(:line_item_for_variant_id) { li } + + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '1', max_quantity: '3'}).should 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) + op.stub(:line_item_for_variant_id) { li } + + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '1'}).should be_false + end + + it "returns false when both are specified and neither varies" do + li = double(:line_item, quantity: 1, max_quantity: 2) + op.stub(:line_item_for_variant_id) { li } + + op.send(:varies_from_cart, {variant_id: variant.id, quantity: '1', max_quantity: '2'}).should be_false + end + end + + describe "variants_removed" do + it "returns the variant ids when one is in the cart but not in those given" do + op.stub(:variant_ids_in_cart) { [123] } + op.send(:variants_removed, []).should == [123] + end + + it "returns nothing when all items in the cart are provided" do + op.stub(:variant_ids_in_cart) { [123] } + op.send(:variants_removed, [{variant_id: '123'}]).should == [] + end + + it "returns nothing when items are added to cart" do + op.stub(:variant_ids_in_cart) { [123] } + op.send(:variants_removed, [{variant_id: '123'}, {variant_id: '456'}]).should == [] + end + + it "does not return duplicates" do + op.stub(:variant_ids_in_cart) { [123, 123] } + op.send(:variants_removed, []).should == [123] + end + end + describe "attempt_cart_add" do it "performs additional validations" do variant = double(:variant) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index d86916e24e..e50a64a4d9 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -329,6 +329,22 @@ describe Spree::Order do end end + describe "removing an item from the order" do + let(:order) { create(:order) } + let(:v1) { create(:variant) } + let(:v2) { create(:variant) } + + before do + order.add_variant v1 + order.add_variant v2 + end + + it "removes the variant's line item" do + order.remove_variant v1 + order.line_items(:reload).map(&:variant).should == [v2] + end + end + describe "emptying the order" do it "removes shipping method" do subject.shipping_method = create(:shipping_method) diff --git a/spec/performance/orders_controller_spec.rb b/spec/performance/orders_controller_spec.rb new file mode 100644 index 0000000000..e151e4f506 --- /dev/null +++ b/spec/performance/orders_controller_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe Spree::OrdersController, type: :controller, performance: true do + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor], variants: products.map { |p| p.variants.first }) } + let(:products) { (0...num_products).map { create(:product) } } + let(:order) { subject.current_order(true) } + let(:num_products) { 20 } + + before do + order.set_distribution! distributor, order_cycle + controller.stub(:current_order) { order } + + Spree::Config.currency = 'AUD' + end + + describe "adding products to cart" do + it "adds products to cart" do + puts "Pre-populating first product" + spree_post :populate, variants: {products[0].variants.first.id => 1} + + result = Benchmark.measure do + (1..num_products).each do |num_products| + puts "Populating #{num_products} products" + variants = Hash[ products.map { |p| [p.variants.first.id, 1] }.first(num_products) ] + spree_post :populate, variants: variants + end + end + + puts result + end + end +end diff --git a/spec/performance/shop_controller_spec.rb b/spec/performance/shop_controller_spec.rb index 6794c70da5..bfe49cd178 100644 --- a/spec/performance/shop_controller_spec.rb +++ b/spec/performance/shop_controller_spec.rb @@ -28,20 +28,10 @@ describe ShopController, type: :controller, performance: true do end it "returns products via json" do - results = [] - 4.times do |i| - ActiveRecord::Base.connection.query_cache.clear - Rails.cache.clear - result = Benchmark.measure do - xhr :get, :products - response.should be_success - end - - results << result.total if i > 0 - puts result + results = multi_benchmark(3) do + xhr :get, :products + response.should be_success end - - puts (results.sum / results.count * 1000).round 0 end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a309037ccb..155b617860 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -109,6 +109,7 @@ RSpec.configure do |config| config.include OpenFoodNetwork::HtmlHelper config.include ActionView::Helpers::DateHelper config.include OpenFoodNetwork::DelayedJobHelper + config.include OpenFoodNetwork::PerformanceHelper # FactoryGirl require 'factory_girl_rails' diff --git a/spec/support/performance_helper.rb b/spec/support/performance_helper.rb new file mode 100644 index 0000000000..a2d4fe3630 --- /dev/null +++ b/spec/support/performance_helper.rb @@ -0,0 +1,20 @@ +module OpenFoodNetwork + module PerformanceHelper + def multi_benchmark(num_samples) + results = (0..num_samples).map do |i| + ActiveRecord::Base.connection.query_cache.clear + Rails.cache.clear + + result = Benchmark.measure { yield } + + puts result + + result.total + end.drop(1) # Do not return the first sample + + puts (results.sum / results.count * 1000).round 0 + + results + end + end +end diff --git a/spec/support/request/ui_component_helper.rb b/spec/support/request/ui_component_helper.rb index 0b478a76ce..43872aee27 100644 --- a/spec/support/request/ui_component_helper.rb +++ b/spec/support/request/ui_component_helper.rb @@ -36,7 +36,7 @@ module UIComponentHelper end def have_login_modal - have_selector ".login-modal" + have_selector ".login-modal" end def open_product_modal(product) @@ -69,7 +69,7 @@ module UIComponentHelper end def cart_dirty - page.find("span.cart-span")[:class].include? 'dirty' + page.find("span.cart-span")[:class].include? 'pure-dirty' end def wait_for_ajax @@ -100,7 +100,7 @@ module UIComponentHelper def expand_active_table_node(name) find(".active_table_node", text: name).click end - + def follow_active_table_node(name) expand_active_table_node(name) find(".active_table_node a", text: "#{name}").click