diff --git a/api/spec/controllers/spree/api/line_items_controller_spec.rb b/api/spec/controllers/spree/api/line_items_controller_spec.rb index 3f63317b1f7..90d6c4f73e3 100644 --- a/api/spec/controllers/spree/api/line_items_controller_spec.rb +++ b/api/spec/controllers/spree/api/line_items_controller_spec.rb @@ -94,7 +94,7 @@ module Spree api_put :update, :id => line_item.id, :line_item => { :quantity => 101 } expect(response.status).to eq(200) order.reload - expect(order.total).to eq(1010) # 10 original due to factory, + 1000 in this test + expect(order.total).to eq(1110) # 101 quantity * 10 item price + 100 shipping cost expect(json_response).to have_attributes(attributes) expect(json_response["quantity"]).to eq(101) end @@ -117,41 +117,17 @@ module Spree expect { line_item.reload }.to raise_error(ActiveRecord::RecordNotFound) end - context "order contents changed after shipments were created" do - let!(:order) { Order.create } - let!(:line_item) { order.contents.add(product.master) } + context "order contents changed after shipments were created and order is completed" do - before { order.create_proposed_shipments } - - it "clear out shipments on create" do - expect(order.reload.shipments).not_to be_empty - api_post :create, :line_item => { :variant_id => product.master.to_param, :quantity => 1 } - expect(order.reload.shipments).to be_empty + before do + allow(order).to receive_messages completed?: true + allow(Order).to receive_message_chain :includes, find_by!: order end - it "clear out shipments on update" do + it "doesn't destroy shipments or restart checkout flow" do expect(order.reload.shipments).not_to be_empty - api_put :update, :id => line_item.id, :line_item => { :quantity => 1000 } - expect(order.reload.shipments).to be_empty - end - - it "clear out shipments on delete" do + api_post :create, :line_item => { :variant_id => product.master.to_param, :quantity => 1 } expect(order.reload.shipments).not_to be_empty - api_delete :destroy, :id => line_item.id - expect(order.reload.shipments).to be_empty - end - - context "order is completed" do - before do - allow(order).to receive_messages completed?: true - allow(Order).to receive_message_chain :includes, find_by!: order - end - - it "doesn't destroy shipments or restart checkout flow" do - expect(order.reload.shipments).not_to be_empty - api_post :create, :line_item => { :variant_id => product.master.to_param, :quantity => 1 } - expect(order.reload.shipments).not_to be_empty - end end end end diff --git a/api/spec/controllers/spree/api/orders_controller_spec.rb b/api/spec/controllers/spree/api/orders_controller_spec.rb index cd7f947466a..c0aceb5c55e 100644 --- a/api/spec/controllers/spree/api/orders_controller_spec.rb +++ b/api/spec/controllers/spree/api/orders_controller_spec.rb @@ -499,20 +499,6 @@ module Spree expect(json_response["user_id"]).to eq(original_id) end - context "order has shipments" do - before { order.create_proposed_shipments } - - it "clears out all existing shipments on line item udpate" do - previous_shipments = order.shipments - api_put :update, :id => order.to_param, :order => { - :line_items => { - 0 => { :id => line_item.id, :quantity => 10 } - } - } - expect(order.reload.shipments).to be_empty - end - end - context "with a line item" do let(:order) { create(:order_with_line_items) } diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 02c90c4c017..6129a93c76c 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -497,8 +497,7 @@ def create_proposed_shipments raise CannotRebuildShipments.new(Spree.t(:cannot_rebuild_shipments_shipments_not_pending)) else adjustments.shipping.destroy_all - shipments.destroy_all - self.shipments = Spree::Stock::Coordinator.new(self).shipments + self.shipments.replace(Spree::Stock::Coordinator.new(self).shipments) end end @@ -509,19 +508,22 @@ def apply_free_shipping_promotions persist_totals end - # Clean shipments and make order back to address state - # - # At some point the might need to force the order to transition from address - # to delivery again so that proper updated shipments are created. - # e.g. customer goes back from payment step and changes order items - def ensure_updated_shipments + # This method used to be known as ensure_updated_shipments but it stopped + # messing with shipments in order to preserve a user's selected shipping method. + # That exposed some oddness, which the new name represents. + # As of this writing, this method is only called from OrderContents, but is + # called every time the order is modified. Which means that we're almost + # always restarting the checkout flow at the 'cart' state for every modification + # to a pre-completed order. + # TODO understand this at a coarser grain and improve it + def probably_restart_checkout_flow if !completed? && shipments.all?(&:pending?) - self.shipments.destroy_all - self.update_column(:shipment_total, 0) + # NOTE: if there are no shipments, [].all?(&:pending?) is true + # which is probably not what we want restart_checkout_flow end - end + alias :ensure_updated_shipments :probably_restart_checkout_flow def restart_checkout_flow return if self.state == 'cart' @@ -530,6 +532,7 @@ def restart_checkout_flow state: 'cart', updated_at: Time.now, ) + # Note: what if this did contents.advance instead of self.next! ? self.next! if self.line_items.size > 0 end diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb index 9d3610162b1..a166ab13163 100644 --- a/core/app/models/spree/order_contents.rb +++ b/core/app/models/spree/order_contents.rb @@ -30,7 +30,7 @@ def update_cart(params) # promotion rules would not be triggered. reload_totals PromotionHandler::Cart.new(order).activate - order.ensure_updated_shipments + order.probably_restart_checkout_flow end reload_totals true @@ -60,7 +60,7 @@ def approve(user: nil, name: nil) def after_add_or_remove(line_item, options = {}) reload_totals shipment = options[:shipment] - shipment.present? ? shipment.update_amounts : order.ensure_updated_shipments + shipment.present? ? shipment.update_amounts : order.probably_restart_checkout_flow PromotionHandler::Cart.new(order, line_item).activate ItemAdjustments.new(line_item).update reload_totals diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 40a6143e610..40ff6f11543 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -94,6 +94,9 @@ def can_transition_from_canceled_to_ready? alias display_amount display_cost def add_shipping_method(shipping_method, selected = false) + # Note: there's some danger if selected = true that we end up with more than 1 + # selected shipping rate since this does not clear the flag from others. + # #selected_shipping_rate_id= handles that case though. shipping_rates.create(shipping_method: shipping_method, selected: selected, cost: cost) end @@ -172,22 +175,7 @@ def refresh_rates return shipping_rates if shipped? || order.completed? return [] unless can_get_rates? - # StockEstimator.new assigment below will replace the current shipping_method - original_shipping_method_id = shipping_method.try!(:id) - - new_rates = Stock::Estimator.new(order).shipping_rates(to_package) - - # If one of the new rates matches the previously selected shipping - # method, select that instead of the default provided by the estimator. - # Otherwise, keep the default. - selected_rate = new_rates.detect{ |rate| rate.shipping_method_id == original_shipping_method_id } - if selected_rate - new_rates.each do |rate| - rate.selected = (rate == selected_rate) - end - end - - self.shipping_rates = new_rates + self.shipping_rates.replace(Stock::Estimator.new(order).shipping_rates(to_package)) self.save! shipping_rates diff --git a/core/app/models/spree/stock/estimator.rb b/core/app/models/spree/stock/estimator.rb index 10d5d9603bd..6fe77a36850 100644 --- a/core/app/models/spree/stock/estimator.rb +++ b/core/app/models/spree/stock/estimator.rb @@ -19,14 +19,27 @@ def initialize(order) def shipping_rates(package, frontend_only = true) rates = calculate_shipping_rates(package) rates.select! { |rate| rate.shipping_method.frontend? } if frontend_only - choose_default_shipping_rate(rates) + choose_selected_shipping_rate(rates) sort_shipping_rates(rates) end private - def choose_default_shipping_rate(shipping_rates) - unless shipping_rates.empty? - shipping_rates.min_by(&:cost).selected = true + def choose_selected_shipping_rate(shipping_rates) + return if shipping_rates.empty? + shipping_rate = find_preselected_shipping_rate(shipping_rates) || shipping_rates.min_by(&:cost) + shipping_rate.selected = true + end + + def find_preselected_shipping_rate(shipping_rates) + # TODO this method assumes a single shipment per order. + # If we have multiple shipments this will assign one of the preselected + # shipping methods to one of the shipments, which is far from ideal, but + # no worse than the prior behavior of always resetting to cheapest. + if order.shipments.count > 0 + shipping_method = order.shipments.map {|s| s.selected_shipping_rate.try!(:shipping_method) }.compact.uniq.first + if shipping_method + shipping_rates.detect {|rate| rate.shipping_method_id == shipping_method.id } + end end end diff --git a/core/lib/spree/testing_support/factories/order_factory.rb b/core/lib/spree/testing_support/factories/order_factory.rb index 044600bbf5f..221938ffaf0 100644 --- a/core/lib/spree/testing_support/factories/order_factory.rb +++ b/core/lib/spree/testing_support/factories/order_factory.rb @@ -27,6 +27,7 @@ line_items_attributes { [{}] * line_items_count } shipment_cost 100 shipping_method nil + select_shipping_rate false end after(:create) do |order, evaluator| @@ -36,7 +37,9 @@ end order.line_items.reload - create(:shipment, order: order, cost: evaluator.shipment_cost, shipping_method: evaluator.shipping_method, address: evaluator.ship_address) + create(:shipment, order: order, cost: evaluator.shipment_cost, + shipping_method: evaluator.shipping_method, address: evaluator.ship_address, + select_shipping_rate: evaluator.select_shipping_rate) order.shipments.reload order.update! diff --git a/core/lib/spree/testing_support/factories/shipment_factory.rb b/core/lib/spree/testing_support/factories/shipment_factory.rb index 7e8c6ed9b7b..d3cd32e9a62 100644 --- a/core/lib/spree/testing_support/factories/shipment_factory.rb +++ b/core/lib/spree/testing_support/factories/shipment_factory.rb @@ -8,11 +8,12 @@ transient do shipping_method nil + select_shipping_rate false end - after(:create) do |shipment, evalulator| - shipping_method = evalulator.shipping_method || create(:shipping_method, cost: evalulator.cost) - shipment.add_shipping_method(shipping_method, true) + after(:create) do |shipment, evaluator| + shipping_method = evaluator.shipping_method || create(:shipping_method, cost: evaluator.cost) + shipment.add_shipping_method(shipping_method, evaluator.select_shipping_rate) shipment.order.line_items.each do |line_item| line_item.quantity.times do diff --git a/core/spec/lib/spree/core/unreturned_item_charger_spec.rb b/core/spec/lib/spree/core/unreturned_item_charger_spec.rb index 581a624a304..d189f33fee3 100644 --- a/core/spec/lib/spree/core/unreturned_item_charger_spec.rb +++ b/core/spec/lib/spree/core/unreturned_item_charger_spec.rb @@ -9,6 +9,7 @@ let(:exchange_shipment) do create(:shipment, order: shipped_order, + select_shipping_rate: true, state: 'shipped', stock_location: original_stock_location, created_at: 5.days.ago) diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb index 44ec98e74fc..74415444edf 100644 --- a/core/spec/models/spree/order/checkout_spec.rb +++ b/core/spec/models/spree/order/checkout_spec.rb @@ -325,7 +325,7 @@ def assert_state_changed(order, from, to) context "correctly determining payment required based on shipping information" do let(:shipment) do - FactoryGirl.create(:shipment) + FactoryGirl.create(:shipment, select_shipping_rate: true) end before do diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb index d5a3e4e470f..3010c5f6416 100644 --- a/core/spec/models/spree/order_contents_spec.rb +++ b/core/spec/models/spree/order_contents_spec.rb @@ -18,9 +18,9 @@ end context 'given a shipment' do - it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do + it "ensure shipment calls update_amounts instead of order calling probably_restart_checkout_flow" do shipment = create(:shipment) - expect(subject.order).to_not receive(:ensure_updated_shipments) + expect(subject.order).to_not receive(:probably_restart_checkout_flow) expect(shipment).to receive(:update_amounts) subject.add(variant, 1, shipment: shipment) end @@ -28,7 +28,7 @@ context 'not given a shipment' do it "ensures updated shipments" do - expect(subject.order).to receive(:ensure_updated_shipments) + expect(subject.order).to receive(:probably_restart_checkout_flow) subject.add(variant) end end @@ -116,10 +116,10 @@ end context 'given a shipment' do - it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do + it "ensure shipment calls update_amounts instead of order calling probably_restart_checkout_flow" do line_item = subject.add(variant, 1) shipment = create(:shipment) - expect(subject.order).to_not receive(:ensure_updated_shipments) + expect(subject.order).to_not receive(:probably_restart_checkout_flow) expect(shipment).to receive(:update_amounts) subject.remove(variant, 1, shipment: shipment) end @@ -128,7 +128,7 @@ context 'not given a shipment' do it "ensures updated shipments" do line_item = subject.add(variant, 1) - expect(subject.order).to receive(:ensure_updated_shipments) + expect(subject.order).to receive(:probably_restart_checkout_flow) subject.remove(variant) end end @@ -164,10 +164,10 @@ context "#remove_line_item" do context 'given a shipment' do - it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do + it "ensure shipment calls update_amounts instead of order calling probably_restart_checkout_flow" do line_item = subject.add(variant, 1) shipment = create(:shipment) - expect(subject.order).to_not receive(:ensure_updated_shipments) + expect(subject.order).to_not receive(:probably_restart_checkout_flow) expect(shipment).to receive(:update_amounts) subject.remove_line_item(line_item, shipment: shipment) end @@ -176,7 +176,7 @@ context 'not given a shipment' do it "ensures updated shipments" do line_item = subject.add(variant, 1) - expect(subject.order).to receive(:ensure_updated_shipments) + expect(subject.order).to receive(:probably_restart_checkout_flow) subject.remove_line_item(line_item) end end @@ -239,7 +239,7 @@ end it "ensures updated shipments" do - expect(subject.order).to receive(:ensure_updated_shipments) + expect(subject.order).to receive(:probably_restart_checkout_flow) subject.update_cart params end end diff --git a/core/spec/models/spree/order_shipping_method_spec.rb b/core/spec/models/spree/order_shipping_method_spec.rb new file mode 100644 index 00000000000..f182673da33 --- /dev/null +++ b/core/spec/models/spree/order_shipping_method_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe Spree::Order do + let(:order) { create(:order_with_line_items) } + let(:quadcopter) { create(:shipping_method, name: "Quads'r'us", code: "QUADSRUS", cost: 200) } + + it "preserves selected shipping method when update_cart and next! called" do + #choose more expensive quadcopter shipping method + original_shipment = order.shipments.first + quadcopter_rate = original_shipment.add_shipping_method(quadcopter) + original_shipment.selected_shipping_rate_id = quadcopter_rate.id + expect(order.shipments.first.shipping_method).to eq quadcopter + + order.contents.update_cart({}) #contents don't really matter + order.contents.advance + + expect(order.shipments.first.shipping_method).to eq quadcopter + end + + let(:cheap_ups_cost) { 10 } + + it "reaching delivery step with no explicit selection selects cheapest shipping rate" do + # ... even if the shipping method is created after the order + order.contents.add(create(:variant), 1) + cheap_ups = create(:shipping_method, name: "Cheap UPS Ground", cost: cheap_ups_cost) + + order.contents.advance + final_selected_shipping_rate = order.shipments.first.selected_shipping_rate + + expect(final_selected_shipping_rate.shipping_method).to eq cheap_ups + expect(final_selected_shipping_rate.cost).to eq cheap_ups_cost + end +end diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index 6891b1ec19d..bb42443953d 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -268,31 +268,20 @@ def merge!(other_order, user = nil) order.shipments.create! end - it "destroys current shipments" do - order.ensure_updated_shipments - expect(order.shipments).to be_empty - end - it "puts order back in address state" do - order.ensure_updated_shipments + order.probably_restart_checkout_flow expect(order.state).to eql "cart" end - it "resets shipment_total" do - order.update_column(:shipment_total, 5) - order.ensure_updated_shipments - expect(order.shipment_total).to eq(0) - end - it "does nothing if any shipments are ready" do shipment = create(:shipment, order: subject, state: "ready") - expect { subject.ensure_updated_shipments }.not_to change { subject.reload.shipments } + expect { subject.probably_restart_checkout_flow }.not_to change { subject.reload.shipments } expect { shipment.reload }.not_to raise_error end it "does nothing if any shipments are shipped" do shipment = create(:shipment, order: subject, state: "shipped") - expect { subject.ensure_updated_shipments }.not_to change { subject.reload.shipments } + expect { subject.probably_restart_checkout_flow }.not_to change { subject.reload.shipments } expect { shipment.reload }.not_to raise_error end end @@ -305,19 +294,8 @@ def merge!(other_order, user = nil) order.shipments.create! end - it "destroys current shipments" do - order.ensure_updated_shipments - expect(order.shipments).to be_empty - end - - it "resets shipment_total" do - order.update_column(:shipment_total, 5) - order.ensure_updated_shipments - expect(order.shipment_total).to eq(0) - end - it "puts the order in the cart state" do - order.ensure_updated_shipments + order.probably_restart_checkout_flow expect(order.state).to eq "cart" end end @@ -330,21 +308,9 @@ def merge!(other_order, user = nil) order.shipments.create! end - it "does not destroy the current shipments" do - expect { - order.ensure_updated_shipments - }.not_to change { order.shipments } - end - - it "does not reset the shipment total" do - expect { - order.ensure_updated_shipments - }.not_to change { order.shipment_total } - end - it "does not put the order back in the address state" do expect { - order.ensure_updated_shipments + order.probably_restart_checkout_flow }.not_to change { order.state } end end @@ -356,15 +322,7 @@ def merge!(other_order, user = nil) order.shipments.create! expect { - order.ensure_updated_shipments - }.not_to change { order.shipment_total } - - expect { - order.ensure_updated_shipments - }.not_to change { order.shipments } - - expect { - order.ensure_updated_shipments + order.probably_restart_checkout_flow }.not_to change { order.state } end end diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 47131ce7b9f..f0363c0f800 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -184,11 +184,11 @@ before { allow(shipment).to receive(:can_get_rates?){ true } } it 'should request new rates, and maintain shipping_method selection' do - expect(Spree::Stock::Estimator).to receive(:new).with(shipment.order).and_return(mock_estimator) - allow(shipment).to receive_messages(shipping_method: shipping_method2) + # select more expensive shipping_method2 + shipping_method2_rate = shipment.add_shipping_method(shipping_method2) + shipment.selected_shipping_rate_id = shipping_method2_rate.id - expect(shipment.refresh_rates).to eq(shipping_rates) - expect(shipment.reload.selected_shipping_rate.shipping_method_id).to eq(shipping_method2.id) + expect(shipment.selected_shipping_rate.shipping_method_id).to eq(shipping_method2.id) end it 'should handle no shipping_method selection' do diff --git a/core/spec/models/spree/stock/estimator_spec.rb b/core/spec/models/spree/stock/estimator_spec.rb index 112fe05caca..2399821740c 100644 --- a/core/spec/models/spree/stock/estimator_spec.rb +++ b/core/spec/models/spree/stock/estimator_spec.rb @@ -4,7 +4,7 @@ module Spree module Stock describe Estimator, :type => :model do let!(:shipping_method) { create(:shipping_method) } - let(:package) { build(:stock_package, contents: inventory_units.map { |i| ContentItem.new(inventory_unit) }) } + let(:package) { build(:stock_package, contents: inventory_units.map { |i| ContentItem.new(i) }) } let(:order) { build(:order_with_line_items) } let(:inventory_units) { order.inventory_units } @@ -101,6 +101,21 @@ module Stock subject.shipping_rates(package) end + + context "user has a selected shipping rate" do + let(:order) { create(:order_with_line_items, select_shipping_rate: true) } + + before do + #need to override from parent context + allow(package).to receive_messages(:shipping_methods => Spree::ShippingMethod.all.to_a) + end + + it "preserves the user's choice" do + preselected_rate = order.shipments.first.selected_shipping_rate + rates = subject.shipping_rates(package) + expect(rates.detect(&:selected).shipping_method_id).to eq preselected_rate.shipping_method_id + end + end end context "involves backend only shipping methods" do