From f7890bd94df1f565877a2ef70c34764bd901aca9 Mon Sep 17 00:00:00 2001 From: Rob H Date: Fri, 18 Jan 2013 19:17:55 +1100 Subject: [PATCH] Fixes following code review for checkout changes --- app/helpers/application_helper.rb | 1 - app/helpers/enterprises_helper.rb | 4 +++ app/helpers/spree/orders_helper.rb | 4 +++ app/models/spree/order_decorator.rb | 9 ------ .../_other_available_distributors.html.erb | 9 ++---- .../orders/_order_item_description.html.haml | 2 +- .../spree/products/_add_to_cart.html.haml | 2 +- spec/models/order_spec.rb | 32 ------------------- spec/requests/consumer/checkout_spec.rb | 16 +++++++++- spec/requests/consumer/distributors_spec.rb | 1 - 10 files changed, 27 insertions(+), 53 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4f41a1a0ed..620dd8bb29 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -5,7 +5,6 @@ module ApplicationHelper end end - # Pass URL helper calls on to spree where applicable so that we don't need to use # spree.foo_path in any view rendered from non-spree-namespaced controllers. def method_missing(method, *args, &block) diff --git a/app/helpers/enterprises_helper.rb b/app/helpers/enterprises_helper.rb index fb8a9a13bc..a1012dbe1c 100644 --- a/app/helpers/enterprises_helper.rb +++ b/app/helpers/enterprises_helper.rb @@ -2,4 +2,8 @@ module EnterprisesHelper def current_distributor @current_distributor ||= current_order(false).andand.distributor end + + def enterprises_options enterprises + enterprises.map { |enterprise| [enterprise.name + ": " + enterprise.address.address1 + ", " + enterprise.address.city, enterprise.id.to_i] } + end end diff --git a/app/helpers/spree/orders_helper.rb b/app/helpers/spree/orders_helper.rb index 7c89d20bf8..08d5e5492f 100644 --- a/app/helpers/spree/orders_helper.rb +++ b/app/helpers/spree/orders_helper.rb @@ -5,5 +5,9 @@ module Spree amount = order.line_items.map { |li| li.itemwise_shipping_cost }.sum options.delete(:format_as_currency) ? number_to_currency(amount) : amount end + + def alternative_available_distributors(order) + DistributorChangeValidator.new(order).available_distributors(Enterprise.all) - [order.distributor] + end end end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index f588ab1353..17dbe09405 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -15,11 +15,6 @@ Spree::Order.class_eval do 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 || DistributorChangeValidator.new(self).can_change_to_distributor?(distributor) - super(distributor) - end - def set_distributor!(distributor) self.distributor = distributor save! @@ -36,10 +31,6 @@ Spree::Order.class_eval do 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 diff --git a/app/views/spree/checkout/_other_available_distributors.html.erb b/app/views/spree/checkout/_other_available_distributors.html.erb index 2b84986dd4..d9f4560cb7 100644 --- a/app/views/spree/checkout/_other_available_distributors.html.erb +++ b/app/views/spree/checkout/_other_available_distributors.html.erb @@ -1,14 +1,9 @@ <% unless @order.state != 'address' %>
- <% - other_available_distributors = DistributorChangeValidator.new(@order).available_distributors(Enterprise.all) - other_available_distributors.delete(@order.distributor) - unless other_available_distributors.empty? - %> + <% unless alternative_available_distributors(@order).empty? %> <%= form_for(@order) do |f| %> <%= f.label :distributor_label, "Alternative distributors for this order:" %> - <% available_distributors_array = other_available_distributors.map { |distributor| [distributor.name + ": " + distributor.address.address1 + ", " + distributor.address.city, distributor.id.to_i] } %> - <%= f.select :distributor_id, options_for_select( available_distributors_array ) %> + <%= f.select :distributor_id, options_for_select( enterprises_options(alternative_available_distributors(@order)) ) %> <%= f.submit "Change Distributor" %> <% end %> <% else %> diff --git a/app/views/spree/orders/_order_item_description.html.haml b/app/views/spree/orders/_order_item_description.html.haml index d7a9184736..d4531e8976 100644 --- a/app/views/spree/orders/_order_item_description.html.haml +++ b/app/views/spree/orders/_order_item_description.html.haml @@ -1,3 +1,3 @@ %td(data-hook = "order_item_description") %h4= item.variant.product.name - = "(" + variant_options(item.variant) + ")" unless item.variant .option_values.empty? \ No newline at end of file + = "(" + variant_options(item.variant) + ")" unless item.variant.option_values.empty? \ No newline at end of file diff --git a/app/views/spree/products/_add_to_cart.html.haml b/app/views/spree/products/_add_to_cart.html.haml index 495ea77bbb..0b0e118e54 100644 --- a/app/views/spree/products/_add_to_cart.html.haml +++ b/app/views/spree/products/_add_to_cart.html.haml @@ -3,7 +3,7 @@ - if !@product.has_stock? && !Spree::Config[:allow_backorders] = content_tag('strong', t(:out_of_stock)) - - elsif !order.nil? && !order.can_add_product_to_cart?(@product) + - elsif !order.nil? && !DistributorChangeValidator.new(order).can_change_distributor? && !@product.distributors.include?(order.distributor) .error-distributor Please complete your order at = link_to current_distributor.name, root_path diff --git a/spec/models/order_spec.rb b/spec/models/order_spec.rb index 171258579c..e3d52db8bb 100644 --- a/spec/models/order_spec.rb +++ b/spec/models/order_spec.rb @@ -28,38 +28,6 @@ describe Spree::Order do li.max_quantity.should == 3 end - 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 - - 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 - - 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 - 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") diff --git a/spec/requests/consumer/checkout_spec.rb b/spec/requests/consumer/checkout_spec.rb index 320dfcf281..c88f0da8ca 100644 --- a/spec/requests/consumer/checkout_spec.rb +++ b/spec/requests/consumer/checkout_spec.rb @@ -17,6 +17,16 @@ feature %q{ :state => Spree::State.find_by_name('Victoria'), :country => Spree::Country.find_by_name('Australia')), :pickup_times => 'Tuesday, 4 PM') + + + @distributor_alternative = create(:distributor_enterprise, :name => 'Alternative Distributor', + :address => create(:address, + :address1 => '1600 Rathdowne St', + :city => 'Carlton North', + :zipcode => 3054, + :state => Spree::State.find_by_name('Victoria'), + :country => Spree::Country.find_by_name('Australia')), + :pickup_times => 'Tuesday, 4 PM') @shipping_method_1 = create(:shipping_method, :name => 'Shipping Method One') @shipping_method_1.calculator.set_preference :amount, 1 @@ -28,9 +38,11 @@ feature %q{ @product_1 = create(:product, :name => 'Fuji apples') @product_1.product_distributions.create(:distributor => @distributor, :shipping_method => @shipping_method_1) + @product_1.product_distributions.create(:distributor => @distributor_alternative, :shipping_method => @shipping_method_1) @product_2 = create(:product, :name => 'Garlic') @product_2.product_distributions.create(:distributor => @distributor, :shipping_method => @shipping_method_2) + @product_2.product_distributions.create(:distributor => @distributor_alternative, :shipping_method => @shipping_method_2) @zone = create(:zone) c = Spree::Country.find_by_name('Australia') @@ -110,7 +122,9 @@ feature %q{ page.should have_content value end end - + + page.should have_selector "select#order_distributor_id option[value='#{@distributor_alternative.id}']" + click_button 'Save and Continue' # -- Checkout: Delivery diff --git a/spec/requests/consumer/distributors_spec.rb b/spec/requests/consumer/distributors_spec.rb index cd03a67def..4a747e5c0f 100644 --- a/spec/requests/consumer/distributors_spec.rb +++ b/spec/requests/consumer/distributors_spec.rb @@ -144,7 +144,6 @@ feature %q{ # When we select the distributor and view the product visit spree.select_distributor_order_path(distributor1) visit spree.product_path(product) - binding.pry # Then we should see our distributor as the default option when adding the item to our cart page.should have_selector "select#distributor_id option[value='#{distributor1.id}'][selected='selected']"