Merge branch 'master' of github.com:openfoodfoundation/openfoodnetwork into i18n

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)

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

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'
          

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

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
 

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)

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)

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

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

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'

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

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