From 8a37b4e9184317932cb711270bf4a95d797191b7 Mon Sep 17 00:00:00 2001 From: Rob H Date: Fri, 14 Dec 2012 11:02:01 +1100 Subject: [PATCH] Push validation logic for changing distributor down into lib class --- .../spree/orders_controller_decorator.rb | 15 +- app/models/enterprise.rb | 4 + app/models/spree/order_decorator.rb | 49 ++---- .../_other_available_distributors.html.erb | 2 +- .../spree/products/_add_to_cart.html.haml | 6 +- .../spree/products/_source_sidebar.html.haml | 5 +- .../distributor_change_validator.rb | 22 +++ .../distributor_change_validator_spec.rb | 80 +++++++++ spec/models/order_spec.rb | 156 ++++++------------ 9 files changed, 188 insertions(+), 151 deletions(-) create mode 100644 lib/open_food_web/distributor_change_validator.rb create mode 100644 spec/lib/open_food_web/distributor_change_validator_spec.rb diff --git a/app/controllers/spree/orders_controller_decorator.rb b/app/controllers/spree/orders_controller_decorator.rb index 2342665975..455d2c4ef2 100644 --- a/app/controllers/spree/orders_controller_decorator.rb +++ b/app/controllers/spree/orders_controller_decorator.rb @@ -34,11 +34,8 @@ Spree::OrdersController.class_eval do distributor = Enterprise.is_distributor.find params[:id] order = current_order(true) - - if order.can_change_to_distributor?(distributor) - order.distributor = distributor - order.save! - end + order.distributor = distributor + order.save! redirect_to main_app.enterprise_path(distributor) end @@ -46,10 +43,8 @@ Spree::OrdersController.class_eval do def deselect_distributor order = current_order(true) - if order.can_change_distributor? - order.distributor = nil - order.save! - end + order.distributor = nil + order.save! redirect_to root_path end @@ -73,7 +68,7 @@ Spree::OrdersController.class_eval do # -- If products in cart, distributor can't be changed order = current_order(false) - if !order.nil? && !order.can_change_distributor? && order.distributor != distributor + if !order.nil? && !DistributorChangeValidator.new(order).can_change_to_distributor?(distributor) return false end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index d4679f6424..9afcaaff58 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -27,6 +27,10 @@ class Enterprise < ActiveRecord::Base def to_param "#{id}-#{name.parameterize}" end + + def available_variants + ProductDistribution.find_all_by_distributor_id( self.id ).map{ |pd| pd.product.variants }.flatten + end private diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 14b626a189..f588ab1353 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -1,44 +1,22 @@ +require 'open_food_web/distributor_change_validator' + Spree::Order.class_eval do belongs_to :distributor, :class_name => 'Enterprise' before_validation :shipping_address_from_distributor - validate :change_distributor_validation, :if => :distributor_id_changed? + validate :products_available_from_new_distributor, :if => :distributor_id_changed? attr_accessible :distributor_id after_create :set_default_shipping_method - def change_distributor_validation + + def products_available_from_new_distributor # Check that the line_items in the current order are available from a newly selected distributor - errors.add(:distributor_id, "The products in your cart are not available from '" + distributor.name + "'") unless can_change_to_distributor? distributor - end - - def can_change_to_distributor? distributor - # Distributor may not be changed once an item has been added to the cart/order, unless all items are available from the specified distributor - line_items.empty? || (available_distributors || []).include?(distributor) - end - - def can_change_distributor? - # Distributor may not be changed once an item has been added to the cart/order - line_items.empty? - end - - def available_distributors - # Find all other enterprises which offer all product variants contained within the current order - distributors_with_all_variants = get_distributors_with_all_variants(Enterprise.all) - end - - def get_distributors_with_all_variants(enterprises) - variants_in_current_order = line_items.map{ |li| li.variant } - distributors_with_all_variants = [] - enterprises.each do |e| - variants_available_from_enterprise = ProductDistribution.find_all_by_distributor_id( e.id ).map{ |pd| pd.product.variants }.flatten - distributors_with_all_variants << e if ( variants_in_current_order - variants_available_from_enterprise ).empty? - end - distributors_with_all_variants + errors.add(:distributor_id, "cannot supply the products in your cart") unless DistributorChangeValidator.new(self).can_change_to_distributor?(distributor) end def distributor=(distributor) - raise "You cannot change the distributor of an order with products" unless distributor == self.distributor || can_change_to_distributor?(distributor) + #raise "You cannot change the distributor of an order with products" unless distributor == self.distributor || DistributorChangeValidator.new(self).can_change_to_distributor?(distributor) super(distributor) end @@ -47,11 +25,6 @@ Spree::Order.class_eval do save! end - def can_add_product_to_cart?(product) - # Products may be added if no line items are currently in the cart or if the product is available from the current distributor - line_items.empty? || product.distributors.include?(distributor) - end - def set_variant_attributes(variant, attributes) line_item = contains?(variant) @@ -62,6 +35,14 @@ Spree::Order.class_eval do line_item.assign_attributes(attributes) line_item.save! end + + def can_add_product_to_cart? product + DistributorChangeValidator.new(self).can_change_distributor? || product.distributors.include?(distributor) + end + + def line_item_variants + line_items.map{ |li| li.variant } + end private diff --git a/app/views/spree/checkout/_other_available_distributors.html.erb b/app/views/spree/checkout/_other_available_distributors.html.erb index 026b320b90..2b84986dd4 100644 --- a/app/views/spree/checkout/_other_available_distributors.html.erb +++ b/app/views/spree/checkout/_other_available_distributors.html.erb @@ -1,7 +1,7 @@ <% unless @order.state != 'address' %>
<% - other_available_distributors = @order.available_distributors + other_available_distributors = DistributorChangeValidator.new(@order).available_distributors(Enterprise.all) other_available_distributors.delete(@order.distributor) unless other_available_distributors.empty? %> diff --git a/app/views/spree/products/_add_to_cart.html.haml b/app/views/spree/products/_add_to_cart.html.haml index e934cebe21..3f48211f6c 100644 --- a/app/views/spree/products/_add_to_cart.html.haml +++ b/app/views/spree/products/_add_to_cart.html.haml @@ -1,8 +1,9 @@ .add-to-cart + - order = current_order(false) - if !@product.has_stock? && !Spree::Config[:allow_backorders] = content_tag('strong', t(:out_of_stock)) - - elsif current_order(false) && !current_order(false).can_add_product_to_cart?(@product) + - elsif current_order(false) && !order.can_add_product_to_cart?(@product) .error-distributor Please complete your order at = link_to current_distributor.name, root_path @@ -14,8 +15,7 @@ - if @product.group_buy %p Max quantity = number_field_tag (@product.has_variants? ? :max_quantity : "variant_attributes[#{@product.master.id}][max_quantity]"), 1, :class => 'title max_quantity', :in => 1..@product.on_hand - - order = current_order(false) - - if order.nil? || order.can_change_distributor? + - if order.nil? || DistributorChangeValidator.new(order).can_change_distributor? %p Distributor = select_tag "distributor_id", options_from_collection_for_select([Enterprise.new]+@product.distributors, "id", "name", current_distributor.andand.id) - else diff --git a/app/views/spree/products/_source_sidebar.html.haml b/app/views/spree/products/_source_sidebar.html.haml index e5a03d2896..c91627596e 100644 --- a/app/views/spree/products/_source_sidebar.html.haml +++ b/app/views/spree/products/_source_sidebar.html.haml @@ -9,13 +9,14 @@ %h6.filter_name Shop by Distributor %ul.filter_choices - order = current_order(false) + - validator = DistributorChangeValidator.new(order) - @distributors.each do |distributor| %li.nowrap - - if order.nil? || order.can_change_to_distributor?(distributor) + - if order.nil? || validator.can_change_to_distributor?(distributor) = link_to distributor.name, select_distributor_order_path(distributor) - elsif order.distributor == distributor = link_to distributor.name, [main_app, distributor] - else %span.inactive= distributor.name - - if current_distributor && order.can_change_distributor? + - if current_distributor && validator.can_change_distributor? = button_to 'Browse All Distributors', deselect_distributor_orders_path, :method => :get diff --git a/lib/open_food_web/distributor_change_validator.rb b/lib/open_food_web/distributor_change_validator.rb new file mode 100644 index 0000000000..ca7545eefc --- /dev/null +++ b/lib/open_food_web/distributor_change_validator.rb @@ -0,0 +1,22 @@ +class DistributorChangeValidator + + def initialize order + @order = order + end + + def can_change_distributor? + # Distributor may not be changed once an item has been added to the cart/order + @order.line_items.empty? + end + + def can_change_to_distributor? distributor + # Distributor may not be changed once an item has been added to the cart/order, unless all items are available from the specified distributor + @order.line_items.empty? || (available_distributors(Enterprise.all) || []).include?(distributor) + end + + def available_distributors enterprises + enterprises.select do |e| + (@order.line_item_variants - e.available_variants).empty? + end + end +end \ No newline at end of file diff --git a/spec/lib/open_food_web/distributor_change_validator_spec.rb b/spec/lib/open_food_web/distributor_change_validator_spec.rb new file mode 100644 index 0000000000..d8f8967000 --- /dev/null +++ b/spec/lib/open_food_web/distributor_change_validator_spec.rb @@ -0,0 +1,80 @@ +require 'open_food_web/distributor_change_validator' + +describe DistributorChangeValidator do + let(:order) { double(:order) } + let(:subject) { DistributorChangeValidator.new(order) } + + context "permissions for changing distributor" do + it "allows distributor to be changed if line_items is empty" do + order.stub(:line_items) { [] } + subject.can_change_distributor?.should be_true + end + + it "does not allow distributor to be changed if line_items is not empty" do + order.stub(:line_items) { [1, 2, 3] } + subject.can_change_distributor?.should be_false + end + end + + context "finding distributors which have the same variants" do + it "matches enterprises which offer all products within the order" do + variant1 = double(:variant) + variant2 = double(:variant) + variant3 = double(:variant) + variant5 = double(:variant) + line_item_variants = [variant1, variant3, variant5] + order.stub(:line_item_variants){ line_item_variants } + enterprise = double(:enterprise) + enterprise.stub(:available_variants){ line_item_variants } # Exactly the same variants as the order + + subject.available_distributors([enterprise]).should == [enterprise] + end + + it "does not match enterprises with no products available" do + variant1 = double(:variant) + variant3 = double(:variant) + variant5 = double(:variant) + line_item_variants = [variant1, variant3, variant5] + order.stub(:line_item_variants){ line_item_variants } + enterprise = double(:enterprise) + enterprise.stub(:available_variants){ [] } # No variants + + subject.available_distributors([enterprise]).should_not include enterprise + end + + it "does not match enterprises with only some of the same variants in the order available" do + variant1 = double(:variant) + variant2 = double(:variant) + variant3 = double(:variant) + variant4 = double(:variant) + variant5 = double(:variant) + line_item_variants = [variant1, variant3, variant5] + order.stub(:line_item_variants){ line_item_variants } + enterprise_with_some_variants = double(:enterprise) + enterprise_with_some_variants.stub(:available_variants){ [variant1, variant3] } # Only some variants + enterprise_with_some_plus_extras = double(:enterprise) + enterprise_with_some_plus_extras.stub(:available_variants){ [variant1, variant2, variant3, variant4] } # Only some variants, plus extras + + subject.available_distributors([enterprise_with_some_variants]).should_not include enterprise_with_some_variants + subject.available_distributors([enterprise_with_some_plus_extras]).should_not include enterprise_with_some_plus_extras + end + + it "matches enteprises which offer all products in the order, plus additional products" do + variant1 = double(:variant) + variant2 = double(:variant) + variant3 = double(:variant) + variant4 = double(:variant) + variant5 = double(:variant) + line_item_variants = [variant1, variant3, variant5] + order.stub(:line_item_variants){ line_item_variants } + enterprise = double(:enterprise) + enterprise.stub(:available_variants){ [variant1, variant2, variant3, variant4, variant5] } # Excess variants + + subject.available_distributors([enterprise]).should == [enterprise] + end + + it "matches no enterprises when none are provided" do + subject.available_distributors([]).should == [] + end + end +end \ No newline at end of file diff --git a/spec/models/order_spec.rb b/spec/models/order_spec.rb index f80c1d1fa9..171258579c 100644 --- a/spec/models/order_spec.rb +++ b/spec/models/order_spec.rb @@ -14,59 +14,6 @@ describe Spree::Order do subject.adjustments.where(:label => "Shipping").should be_present end - it "reveals permission for changing distributor" do - d = create(:distributor_enterprise) - p = create(:product, :distributors => [d]) - - subject.distributor = d - subject.save! - - subject.can_change_distributor?.should be_true - subject.add_variant(p.master, 1) - subject.can_change_distributor?.should be_false - end - - it "checks that distributor is available when changing, and raises an exception if distributor is changed without permission" do - d = create(:distributor_enterprise) - p = create(:product, :distributors => [d]) - subject.distributor = d - subject.save! - - subject.add_variant(p.master, 1) - subject.can_change_distributor?.should be_false - subject.should_receive(:available_distributors) - - expect do - subject.distributor = nil - end.to raise_error "You cannot change the distributor of an order with products" - end - - it "reveals permission for adding products to the cart" do - d1 = create(:distributor_enterprise) - d2 = create(:distributor_enterprise) - - p_first = create(:product, :distributors => [d1]) - p_subsequent_same_dist = create(:product, :distributors => [d1]) - p_subsequent_other_dist = create(:product, :distributors => [d2]) - - # We need to set distributor, since order.add_variant does not, and - # we also need to save the order so that line items can be added to - # the association. - subject.distributor = d1 - subject.save! - - # The first product in the cart can be added - subject.can_add_product_to_cart?(p_first).should be_true - subject.add_variant(p_first.master, 1) - - # A subsequent product can be added if the distributor matches - subject.can_add_product_to_cart?(p_subsequent_same_dist).should be_true - subject.add_variant(p_subsequent_same_dist.master, 1) - - # And cannot be added if it does not match - subject.can_add_product_to_cart?(p_subsequent_other_dist).should be_false - end - it "sets attributes on line items for variants" do d = create(:distributor_enterprise) p = create(:product, :distributors => [d]) @@ -81,62 +28,69 @@ describe Spree::Order do li.max_quantity.should == 3 end - context "finding alternative distributors" do - it "checks that variants are available" do - distributors_with_all_variants = double(:distributors_with_all_variants) - subject.should_receive(:get_distributors_with_all_variants).with(Enterprise.all) - subject.available_distributors + context "permissions for adding products to the cart" do + it "allows products to be added to cart when cart is empty" do + p_first = double(:product) + p_first.stub(:distributors) { [d1] } + subject.stub(:line_items) { [] } + subject.can_add_product_to_cart?(p_first).should be_true end - context "finding distributors which have the same variants" do - before(:each) do - @enterprise1 = FactoryGirl.create(:enterprise, id: 1) - subject.distributor = @enterprise1 - @product1 = FactoryGirl.create(:product) - @product2 = FactoryGirl.create(:product) - @product3 = FactoryGirl.create(:product) - variant11 = FactoryGirl.create(:variant, product: @product1) - variant12 = FactoryGirl.create(:variant, product: @product1) - variant21 = FactoryGirl.create(:variant, product: @product2) - variant31 = FactoryGirl.create(:variant, product: @product3) - variant32 = FactoryGirl.create(:variant, product: @product3) + it "allows products to be added to cart when they are available from the current distributor" do + d1 = double(:distributor) + p_first = double(:product) + p_first.stub(:distributors) { [d1] } + p_subsequent_same_dist = double(:product) + p_subsequent_same_dist.stub(:distributors) { [d1] } + subject.stub(:line_items) { [1, 2, 3] } + subject.stub(:distributor) { d1 } + subject.can_add_product_to_cart?(p_subsequent_same_dist).should be_true + end - # Product Distributions - # Enterprise 1 sells product 1 and product 3 - FactoryGirl.create(:product_distribution, product: @product1, distributor: @enterprise1) - FactoryGirl.create(:product_distribution, product: @product3, distributor: @enterprise1) + it "does not allow products to be added to cart when they are not available from the current distributor" do + d1 = double(:distributor) + d2 = double(:distributor) + p_first = double(:product) + p_first.stub(:distributors) { [d1] } + p_subsequent_other_dist = double(:product) + p_subsequent_other_dist.stub(:distributors) { [d2] } + subject.stub(:line_items) { [1, 2, 3] } + subject.stub(:distributor) { d1 } + subject.can_add_product_to_cart?(p_subsequent_other_dist).should be_false + end + end - # Build the current order - line_item1 = FactoryGirl.create(:line_item, order: subject, variant: variant11) - line_item2 = FactoryGirl.create(:line_item, order: subject, variant: variant12) - line_item3 = FactoryGirl.create(:line_item, order: subject, variant: variant31) - subject.line_items = [line_item1,line_item2,line_item3] - end + context "validating distributor changes" do + it "checks that a distributor is available when changing" do + order_enterprise = FactoryGirl.create(:enterprise, id: 1, :name => "Order Enterprise") + subject.distributor = order_enterprise + product1 = FactoryGirl.create(:product) + product2 = FactoryGirl.create(:product) + product3 = FactoryGirl.create(:product) + variant11 = FactoryGirl.create(:variant, product: product1) + variant12 = FactoryGirl.create(:variant, product: product1) + variant21 = FactoryGirl.create(:variant, product: product2) + variant31 = FactoryGirl.create(:variant, product: product3) + variant32 = FactoryGirl.create(:variant, product: product3) - it "matches the distributor enterprise of the current order" do - subject.get_distributors_with_all_variants([@enterprise1]).should == [@enterprise1] - end + # Product Distributions + # Order Enterprise sells product 1 and product 3 + FactoryGirl.create(:product_distribution, product: product1, distributor: order_enterprise) + FactoryGirl.create(:product_distribution, product: product3, distributor: order_enterprise) - it "does not match enterprises with no products available" do - test_enterprise = FactoryGirl.create(:enterprise, id: 2) - subject.get_distributors_with_all_variants([@enterprise1, test_enterprise]).should_not include test_enterprise - end + # Build the current order + line_item1 = FactoryGirl.create(:line_item, order: subject, variant: variant11) + line_item2 = FactoryGirl.create(:line_item, order: subject, variant: variant12) + line_item3 = FactoryGirl.create(:line_item, order: subject, variant: variant31) + subject.line_items = [line_item1,line_item2,line_item3] - it "does not match enterprises with only some of the same variants in the current order available" do - test_enterprise = FactoryGirl.create(:enterprise, id: 2) - # Test Enterprise sells only product 1 - FactoryGirl.create(:product_distribution, product: @product1, distributor: test_enterprise) - subject.get_distributors_with_all_variants([@enterprise1, test_enterprise]).should_not include test_enterprise - end + test_enterprise = FactoryGirl.create(:enterprise, id: 2, :name => "Test Enterprise") + # Test Enterprise sells only product 1 + FactoryGirl.create(:product_distribution, product: product1, distributor: test_enterprise) - it "matches enteprises which offer all products in the current order" do - test_enterprise = FactoryGirl.create(:enterprise, id: 2) - # Enterprise 3 Sells Products 1, 2 and 3 - FactoryGirl.create(:product_distribution, product: @product1, distributor: test_enterprise) - FactoryGirl.create(:product_distribution, product: @product2, distributor: test_enterprise) - FactoryGirl.create(:product_distribution, product: @product3, distributor: test_enterprise) - subject.get_distributors_with_all_variants([@enterprise1, test_enterprise]).should include test_enterprise - end + subject.distributor = test_enterprise + subject.should_not be_valid + subject.errors.should include :distributor_id end end end