diff --git a/api/app/controllers/spree/api/orders_controller.rb b/api/app/controllers/spree/api/orders_controller.rb index aa32ca5687a..003dc400340 100644 --- a/api/app/controllers/spree/api/orders_controller.rb +++ b/api/app/controllers/spree/api/orders_controller.rb @@ -27,14 +27,7 @@ def cancel def create authorize! :create, Order - - order_user = if order_params[:user_id] - Spree.user_class.find(order_params[:user_id]) - else - current_api_user - end - - @order = Spree::Core::Importer::Order.import(order_user, order_params) + @order = Spree::Core::Importer::Order.import(determine_order_user, order_params) respond_with(@order, default_template: :show, status: 201) end @@ -116,6 +109,15 @@ def normalize_params params[:order][:bill_address_attributes] = params[:order].delete(:bill_address) if params[:order][:bill_address].present? end + # @api public + def determine_order_user + if order_params[:user_id].present? + Spree.user_class.find(order_params[:user_id]) + else + current_api_user + end + end + def permitted_order_attributes can?(:admin, Spree::Order) ? (super + admin_order_attributes) : super end 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/backend/app/controllers/spree/admin/stock_transfers_controller.rb b/backend/app/controllers/spree/admin/stock_transfers_controller.rb index b4ed978ec55..71d5f6321c9 100644 --- a/backend/app/controllers/spree/admin/stock_transfers_controller.rb +++ b/backend/app/controllers/spree/admin/stock_transfers_controller.rb @@ -7,7 +7,7 @@ class StockTransfersController < ResourceController { translation_key: :name, attr_name: :name } ] - before_filter :load_transferable_stock_locations, only: :index + before_filter :load_viewable_stock_locations, only: :index before_filter :load_variant_display_attributes, only: [:receive, :edit, :show, :tracking_info] before_filter :load_source_stock_locations, only: :new before_filter :load_destination_stock_locations, only: :edit @@ -100,16 +100,16 @@ def authorize_transfer_attributes! authorize! :create, duplicate end - def load_transferable_stock_locations - @stock_locations = accessible_source_stock_locations | accessible_destination_stock_locations + def load_viewable_stock_locations + @stock_locations = Spree::StockLocation.accessible_by(current_ability, :read) end def load_source_stock_locations - @source_stock_locations = accessible_source_stock_locations + @source_stock_locations ||= Spree::StockLocation.accessible_by(current_ability, :transfer_from) end def load_destination_stock_locations - @destination_stock_locations = accessible_destination_stock_locations.where.not(id: @stock_transfer.source_location_id) + @destination_stock_locations ||= Spree::StockLocation.accessible_by(current_ability, :transfer_to).where.not(id: @stock_transfer.source_location_id) end def load_variant_display_attributes @@ -142,14 +142,6 @@ def adjust_inventory @stock_transfer.destination_location.move(transfer_item.variant, transfer_item.received_quantity, @stock_transfer) end end - - def accessible_source_stock_locations - @source_locations ||= Spree::StockLocation.accessible_by(current_ability, :transfer_from) - end - - def accessible_destination_stock_locations - @destination_locations ||= Spree::StockLocation.accessible_by(current_ability, :transfer_to) - end end end end diff --git a/backend/app/views/spree/admin/customer_returns/_reimbursements_table.html.erb b/backend/app/views/spree/admin/customer_returns/_reimbursements_table.html.erb index d82753f0bec..d81074bfd64 100644 --- a/backend/app/views/spree/admin/customer_returns/_reimbursements_table.html.erb +++ b/backend/app/views/spree/admin/customer_returns/_reimbursements_table.html.erb @@ -1,4 +1,4 @@ - +
diff --git a/backend/app/views/spree/admin/customer_returns/_return_item_selection.html.erb b/backend/app/views/spree/admin/customer_returns/_return_item_selection.html.erb index 57ee4b70496..8f64bec2c87 100644 --- a/backend/app/views/spree/admin/customer_returns/_return_item_selection.html.erb +++ b/backend/app/views/spree/admin/customer_returns/_return_item_selection.html.erb @@ -19,7 +19,7 @@ <% return_item = item_fields.object %> -
<%= Spree.t(:number) %>
+
<%= item_fields.hidden_field :inventory_unit_id %> <%= item_fields.hidden_field :return_authorization_id %> diff --git a/backend/app/views/spree/admin/customer_returns/edit.html.erb b/backend/app/views/spree/admin/customer_returns/edit.html.erb index 1edd5cc2ba3..b646be22931 100644 --- a/backend/app/views/spree/admin/customer_returns/edit.html.erb +++ b/backend/app/views/spree/admin/customer_returns/edit.html.erb @@ -16,7 +16,7 @@ <% end %> <% if @pending_return_items.any? %> -
+
<%= Spree.t(:pending) %> <%= render partial: 'return_item_decision', locals: {return_items: @pending_return_items, show_decision: true} %>
diff --git a/backend/app/views/spree/admin/reimbursements/edit.html.erb b/backend/app/views/spree/admin/reimbursements/edit.html.erb index cf3981bac26..4db8274783e 100644 --- a/backend/app/views/spree/admin/reimbursements/edit.html.erb +++ b/backend/app/views/spree/admin/reimbursements/edit.html.erb @@ -13,7 +13,7 @@ <%= form_for [:admin, @order, @reimbursement] do |f| %>
<%= Spree.t(:items_to_be_reimbursed) %> - +
@@ -73,7 +73,7 @@
<%= Spree.t(:calculated_reimbursements) %> -
<%= Spree.t(:product) %>
+
diff --git a/backend/app/views/spree/admin/reimbursements/show.html.erb b/backend/app/views/spree/admin/reimbursements/show.html.erb index 8c76ffe285e..7e098dbcd2b 100644 --- a/backend/app/views/spree/admin/reimbursements/show.html.erb +++ b/backend/app/views/spree/admin/reimbursements/show.html.erb @@ -12,7 +12,7 @@
<%= Spree.t(:items_reimbursed) %> -
<%= Spree.t(:reimbursement_type) %>
+
@@ -55,7 +55,7 @@
<%= Spree.t(:refunds) %> -
<%= Spree.t(:product) %>
+
@@ -64,9 +64,9 @@ <% @reimbursement.refunds.each do |refund| %> - - - + + + <% end %> @@ -75,7 +75,7 @@
<%= Spree.t(:credits) %> -
<%= Spree.t(:description) %>
<%= refund.description %><%= refund.display_amount %>
<%= refund.description %><%= refund.display_amount %>
+
@@ -84,9 +84,9 @@ <% @reimbursement.credits.each do |credit| %> - - - + + + <% end %> diff --git a/backend/app/views/spree/admin/return_authorizations/_form.html.erb b/backend/app/views/spree/admin/return_authorizations/_form.html.erb index cf9fda03b93..255b6606d8d 100644 --- a/backend/app/views/spree/admin/return_authorizations/_form.html.erb +++ b/backend/app/views/spree/admin/return_authorizations/_form.html.erb @@ -4,18 +4,18 @@
<%= Spree.t(:description) %>
<%= credit.description %><%= credit.display_amount %>
<%= credit.description %><%= credit.display_amount %>
- - - - - - - - + + + + + + + @@ -24,42 +24,42 @@ <% inventory_unit = return_item.inventory_unit %> <% editable = inventory_unit.shipped? && !inventory_unit.exchange_requested? && allow_return_item_changes && return_item.reimbursement.nil? %> - - - - + - - - -
+ <% if allow_return_item_changes %> <%= check_box_tag 'select-all' %> <% end %> <%= Spree.t(:product) %><%= Spree.t(:state) %><%= Spree.t(:charged) %><%= Spree.t(:pre_tax_refund_amount) %><%= Spree.t(:reimbursement_type) %><%= Spree.t(:exchange_for) %><%= Spree.t(:reason) %><%= Spree.t(:product) %><%= Spree.t(:state) %><%= Spree.t(:charged) %><%= Spree.t(:pre_tax_refund_amount) %><%= Spree.t(:reimbursement_type) %><%= Spree.t(:exchange_for) %><%= Spree.t(:reason) %>
+ <% if editable %> <%= item_fields.hidden_field :inventory_unit_id %> <%= item_fields.check_box :_destroy, {checked: return_item.persisted?, class: 'add-item', "data-price" => return_item.pre_tax_amount}, '0', '1' %> <% end %> +
<%= inventory_unit.variant.name %>
<%= inventory_unit.variant.options_text %>
<%= inventory_unit.state.humanize %> + <%= inventory_unit.state.humanize %> <%= return_item.display_pre_tax_amount %> + <% if editable %> <%= item_fields.text_field :pre_tax_amount, { class: 'refund-amount-input' } %> <% else %> <%= return_item.display_pre_tax_amount %> <% end %> + <% if editable %> <%= item_fields.select :preferred_reimbursement_type_id, @reimbursement_types.collect{|r|[r.name.humanize, r.id]}, {include_blank: true}, {class: 'select2 fullwidth'} %> <% else %> <%= return_item.preferred_reimbursement_type.try(:name) %> <% end %> + <% if editable %> <%= item_fields.collection_select :exchange_variant_id, return_item.eligible_exchange_variants(@stock_locations), :id, :options_text, { include_blank: true }, { class: "select2 fullwidth return-item-exchange-selection" } %> <% elsif return_item.exchange_processed? %> <%= return_item.exchange_variant.options_text %> <% end %> + <% if editable %> <%= item_fields.select :return_reason_id, @reasons.collect{|r|[r.name, r.id]}, {include_blank: true}, {class: 'select2 fullwidth'} %> <% else %> diff --git a/backend/app/views/spree/admin/shipping_methods/_form.html.erb b/backend/app/views/spree/admin/shipping_methods/_form.html.erb index 5356fb61b80..d5d3828676e 100644 --- a/backend/app/views/spree/admin/shipping_methods/_form.html.erb +++ b/backend/app/views/spree/admin/shipping_methods/_form.html.erb @@ -33,6 +33,24 @@ +
+
+ <%= f.field_container :carrier do %> + <%= f.label :carrier %>
+ <%= f.text_field :carrier, :class => 'fullwidth', :label => false %> + <%= error_message_on :shipping_method, :carrier %> + <% end %> +
+ +
+ <%= f.field_container :service_level do %> + <%= f.label :service_level %>
+ <%= f.text_field :service_level, :class => 'fullwidth', :label => false %> + <%= error_message_on :shipping_method, :service_level %> + <% end %> +
+
+
<%= f.field_container :tracking_url do %> <%= f.label :tracking_url, Spree.t(:tracking_url) %>
diff --git a/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb b/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb index 8d4de9959df..7d940ad084b 100644 --- a/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb @@ -31,8 +31,8 @@ module Spree before do ability.cannot :manage, Spree::StockLocation - ability.can :transfer_from, Spree::StockLocation, id: [warehouse.id] - ability.can :transfer_to, Spree::StockLocation, id: [ny_store.id, la_store.id] + ability.can :display, Spree::StockLocation, id: [warehouse.id] + ability.can :display, Spree::StockLocation, id: [ny_store.id, la_store.id] allow_any_instance_of(Spree::Admin::BaseController).to receive(:spree_current_user).and_return(user) allow_any_instance_of(Spree::Admin::BaseController).to receive(:current_ability).and_return(ability) @@ -302,12 +302,14 @@ module Spree end end - context "#finish" do - let(:stock_transfer) { Spree::StockTransfer.create(source_location: warehouse, destination_location: ny_store, created_by: create(:admin_user))} + context "#ship" do + let(:stock_transfer) { Spree::StockTransfer.create(source_location: warehouse, destination_location: ny_store, created_by: create(:admin_user)) } let(:transfer_variant) { create(:variant) } let(:warehouse_stock_item) { warehouse.stock_items.find_by(variant: transfer_variant) } let(:ny_stock_item) { ny_store.stock_items.find_by(variant: transfer_variant) } + subject { spree_put :ship, id: stock_transfer.number } + before do warehouse_stock_item.set_count_on_hand(1) stock_transfer.transfer_items.create!(variant: transfer_variant, expected_quantity: 1) @@ -316,14 +318,14 @@ module Spree context "with transferable items" do it "marks the transfer shipped" do - spree_put :ship, :id => stock_transfer.number + subject expect(stock_transfer.reload.shipped_at).to_not be_nil expect(flash[:success]).to be_present end it "makes stock movements for the transferred items" do - spree_put :ship, :id => stock_transfer.number + subject expect(Spree::StockMovement.count).to eq 1 expect(warehouse_stock_item.reload.count_on_hand).to eq 0 @@ -334,13 +336,13 @@ module Spree before { warehouse_stock_item.set_count_on_hand(0) } it "does not mark the transfer shipped" do - spree_put :ship, :id => stock_transfer.number + subject expect(stock_transfer.reload.shipped_at).to be_nil end it "errors and redirects to tracking_info page" do - spree_put :ship, :id => stock_transfer.number + subject expect(flash[:error]).to match /not enough inventory/ expect(response).to redirect_to(spree.tracking_info_admin_stock_transfer_path(stock_transfer)) diff --git a/core/app/models/spree/inventory_unit.rb b/core/app/models/spree/inventory_unit.rb index 6bbec87786b..2f8cb23c0fd 100644 --- a/core/app/models/spree/inventory_unit.rb +++ b/core/app/models/spree/inventory_unit.rb @@ -2,7 +2,7 @@ module Spree class InventoryUnit < Spree::Base PRE_SHIPMENT_STATES = %w(backordered on_hand) POST_SHIPMENT_STATES = %w(returned) - CANCELABLE_STATES = ['on_hand', 'backordered'] + CANCELABLE_STATES = ['on_hand', 'backordered', 'shipped'] belongs_to :variant, class_name: "Spree::Variant", inverse_of: :inventory_units belongs_to :order, class_name: "Spree::Order", inverse_of: :inventory_units 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_cancellations.rb b/core/app/models/spree/order_cancellations.rb index c7dd27f2287..fe90293c070 100644 --- a/core/app/models/spree/order_cancellations.rb +++ b/core/app/models/spree/order_cancellations.rb @@ -14,6 +14,15 @@ def initialize(order) @order = order end + # Marks inventory units short shipped. Adjusts the order based on the value of the inventory. + # Sends an email to the customer about what inventory has been short shipped. + # + # @api public + # + # @param [Array] inventory_units the inventory units to be short shipped + # @param [String] whodunnit the system or person that is short shipping the inventory units + # + # @return [Array] the units that have been canceled due to short shipping def short_ship(inventory_units, whodunnit:nil) if inventory_units.map(&:order_id).uniq != [@order.id] raise ArgumentError, "Not all inventory units belong to this order" @@ -42,6 +51,48 @@ def short_ship(inventory_units, whodunnit:nil) unit_cancels end + # Marks inventory unit canceled. Optionally allows specifying the reason why and who is performing the action. + # + # @api public + # + # @param [InventoryUnit] inventory_unit the inventory unit to be canceled + # @param [String] reason the reason that you are canceling the inventory unit + # @param [String] whodunnit the system or person that is canceling the inventory unit + # + # @return [UnitCancel] the unit that has been canceled + def cancel_unit(inventory_unit, reason: Spree::UnitCancel::DEFAULT_REASON, whodunnit:nil) + unit_cancel = nil + + Spree::OrderMutex.with_lock!(@order) do + unit_cancel = Spree::UnitCancel.create!( + inventory_unit: inventory_unit, + reason: reason, + created_by: whodunnit, + ) + + inventory_unit.cancel! + end + + unit_cancel + end + + # Reimburses inventory units due to cancellation. + # + # @api public + # @param [Array] inventory_units the inventory units to be reimbursed + # @return [Reimbursement] the reimbursement for inventory being canceled + def reimburse_units(inventory_units) + reimbursement = nil + + Spree::OrderMutex.with_lock!(@order) do + return_items = inventory_units.map(&:current_or_new_return_item) + reimbursement = Spree::Reimbursement.new(order: @order, return_items: return_items) + reimbursement.return_all + end + + reimbursement + end + private def short_ship_unit(inventory_unit, whodunnit:nil) 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/reimbursement.rb b/core/app/models/spree/reimbursement.rb index dd03cdc20a4..6d564456874 100644 --- a/core/app/models/spree/reimbursement.rb +++ b/core/app/models/spree/reimbursement.rb @@ -136,6 +136,16 @@ def all_exchanges? return_items.all?(&:exchange_processed?) end + # Accepts all return items, saves the reimbursement, and performs the reimbursement + # + # @api public + # @return [void] + def return_all + return_items.each(&:accept!) + save! + perform! + end + private def generate_number 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/app/models/spree/stock_transfer.rb b/core/app/models/spree/stock_transfer.rb index 79c2ab08eb2..6902a9a50f2 100644 --- a/core/app/models/spree/stock_transfer.rb +++ b/core/app/models/spree/stock_transfer.rb @@ -46,7 +46,7 @@ def receivable? finalized? && shipped? && !closed? end - def ship(tracking_number: nil, shipped_at: nil) + def ship(tracking_number: self.tracking_number, shipped_at: nil) update_attributes!(tracking_number: tracking_number, shipped_at: shipped_at) end diff --git a/core/app/models/spree/unit_cancel.rb b/core/app/models/spree/unit_cancel.rb index edcb1ebee3f..0e33d09f2ac 100644 --- a/core/app/models/spree/unit_cancel.rb +++ b/core/app/models/spree/unit_cancel.rb @@ -3,6 +3,8 @@ # This class should encapsulate logic related to canceling inventory after order complete class Spree::UnitCancel < Spree::Base SHORT_SHIP = 'Short Ship' + DEFAULT_REASON = 'Cancel' + belongs_to :inventory_unit has_one :adjustment, as: :source, dependent: :destroy diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 071c6266da0..a13f02438b8 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -105,6 +105,9 @@ en: amount: Amount spree/role: name: Name + spree/shipping_method: + carrier: Carrier + service_level: Service Level spree/state: abbr: Abbreviation name: Name diff --git a/core/db/migrate/20160122182105_add_carrier_and_service_level_to_spree_shipping_methods.rb b/core/db/migrate/20160122182105_add_carrier_and_service_level_to_spree_shipping_methods.rb new file mode 100644 index 00000000000..2653a55de4d --- /dev/null +++ b/core/db/migrate/20160122182105_add_carrier_and_service_level_to_spree_shipping_methods.rb @@ -0,0 +1,6 @@ +class AddCarrierAndServiceLevelToSpreeShippingMethods < ActiveRecord::Migration + def change + add_column :spree_shipping_methods, :carrier, :string + add_column :spree_shipping_methods, :service_level, :string + 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/solidus_core.gemspec b/core/solidus_core.gemspec index 0a1d368555f..87af8816b8b 100644 --- a/core/solidus_core.gemspec +++ b/core/solidus_core.gemspec @@ -44,4 +44,5 @@ Gem::Specification.new do |s| s.add_dependency 'twitter_cldr', '~> 3.0' s.add_development_dependency 'email_spec', '~> 1.6' + s.add_dependency 'sprockets-rails', '~> 2.0' end 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_cancellations_spec.rb b/core/spec/models/spree/order_cancellations_spec.rb index 297aec80494..12aa46f2440 100644 --- a/core/spec/models/spree/order_cancellations_spec.rb +++ b/core/spec/models/spree/order_cancellations_spec.rb @@ -1,6 +1,69 @@ require 'spec_helper' describe Spree::OrderCancellations do + describe "#cancel_unit" do + subject { Spree::OrderCancellations.new(order).cancel_unit(inventory_unit) } + let(:order) { create(:shipped_order, line_items_count: 1) } + let(:inventory_unit) { order.inventory_units.first } + + it "creates a UnitCancel record" do + expect { subject }.to change { Spree::UnitCancel.count }.by(1) + expect(subject.inventory_unit).to eq inventory_unit + end + + it "cancels the inventory unit" do + expect { subject }.to change { inventory_unit.state }.to "canceled" + end + + context "when a reason is specified" do + subject { order.cancellations.cancel_unit(inventory_unit, reason: "some reason") } + + it "sets the reason on the UnitCancel" do + expect(subject.reason).to eq("some reason") + end + end + + context "when a reason is not specified" do + it "sets a default reason on the UnitCancel" do + expect(subject.reason).to eq Spree::UnitCancel::DEFAULT_REASON + end + end + + context "when a whodunnit is specified" do + subject { order.cancellations.cancel_unit(inventory_unit, whodunnit: "some automated system") } + + it "sets the user on the UnitCancel" do + expect(subject.created_by).to eq("some automated system") + end + end + + context "when a whodunnit is not specified" do + it "does not set created_by on the UnitCancel" do + expect(subject.created_by).to be_nil + end + end + end + + describe "#reimburse_units" do + subject { Spree::OrderCancellations.new(order).reimburse_units(inventory_units) } + let(:order) { create(:shipped_order, line_items_count: 2) } + let(:inventory_units) { order.inventory_units } + let!(:default_refund_reason) { Spree::RefundReason.find_or_create_by!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false) } + + it "creates and performs a reimbursement" do + expect { subject }.to change { Spree::Reimbursement.count }.by(1) + expect(subject.refunds.size).to eq 1 + end + + it "creates return items for the inventory units and accepts them" do + expect { subject }.to change { Spree::ReturnItem.count }.by(inventory_units.count) + + return_items = subject.return_items + expect(return_items.map(&:acceptance_status)).to all eq "accepted" + expect(return_items.map(&:inventory_unit)).to match_array(inventory_units) + end + end + describe "#short_ship" do subject { Spree::OrderCancellations.new(order).short_ship([inventory_unit]) } 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/reimbursement_spec.rb b/core/spec/models/spree/reimbursement_spec.rb index 60eebaa4410..e0591aeaf01 100644 --- a/core/spec/models/spree/reimbursement_spec.rb +++ b/core/spec/models/spree/reimbursement_spec.rb @@ -228,4 +228,26 @@ expect(subject.customer_return).to eq customer_return end end + + describe "#return_all" do + subject { reimbursement.return_all } + + let!(:default_refund_reason) { Spree::RefundReason.find_or_create_by!(name: Spree::RefundReason::RETURN_PROCESSING_REASON, mutable: false) } + let(:order) { create(:shipped_order, line_items_count: 1) } + let(:inventory_unit) { order.inventory_units.first } + let(:return_item) { build(:return_item, inventory_unit: inventory_unit) } + let(:reimbursement) { build(:reimbursement, order: order, return_items: [return_item]) } + + it "accepts all the return items" do + expect { subject }.to change { return_item.acceptance_status }.to "accepted" + end + + it "persists the reimbursement" do + expect { subject }.to change { reimbursement.persisted? }.to true + end + + it "performs a reimbursment" do + expect { subject }.to change { reimbursement.refunds.count }.by(1) + end + 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 diff --git a/core/spec/models/spree/stock_transfer_spec.rb b/core/spec/models/spree/stock_transfer_spec.rb index 1b0192d0dbb..10836d2625c 100644 --- a/core/spec/models/spree/stock_transfer_spec.rb +++ b/core/spec/models/spree/stock_transfer_spec.rb @@ -249,6 +249,26 @@ module Spree end end + describe '#ship' do + let(:stock_transfer) { create(:stock_transfer, tracking_number: "ABC123") } + + context "tracking number is provided" do + subject { stock_transfer.ship(tracking_number: "XYZ123") } + + it "updates the tracking number" do + expect { subject }.to change { stock_transfer.tracking_number }.from("ABC123").to("XYZ123") + end + end + + context "tracking number is not provided" do + subject { stock_transfer.ship } + + it "preserves the existing tracking number" do + expect { subject }.to_not change { stock_transfer.tracking_number }.from("ABC123") + end + end + end + describe '#transfer' do let(:stock_transfer) { create(:stock_transfer_with_items) }