From 80d2214fe97bcf969d31f959c15d85a108591179 Mon Sep 17 00:00:00 2001 From: Phillip Birtcher Date: Mon, 14 Dec 2015 10:43:57 -0500 Subject: [PATCH 01/15] Fix customer returns admin hook for inventory unit checkbox --- .../admin/customer_returns/_return_item_selection.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 %> - +
<%= item_fields.hidden_field :inventory_unit_id %> <%= item_fields.hidden_field :return_authorization_id %> From ddeaccabcf33647d159af7c3fb7a9f0339791039 Mon Sep 17 00:00:00 2001 From: Phillip Birtcher Date: Mon, 14 Dec 2015 10:44:33 -0500 Subject: [PATCH 02/15] Fix admin customer returns edit pending_return_items view hook --- backend/app/views/spree/admin/customer_returns/edit.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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} %>
From a916a973b3d1f42509d0503a73bf1dac1a0cd579 Mon Sep 17 00:00:00 2001 From: exsuzme Date: Thu, 17 Dec 2015 10:32:08 -0500 Subject: [PATCH 03/15] Add some hooks to admin reimbursements views --- .../_reimbursements_table.html.erb | 2 +- .../spree/admin/reimbursements/edit.html.erb | 4 ++-- .../spree/admin/reimbursements/show.html.erb | 18 +++++++++--------- 3 files changed, 12 insertions(+), 12 deletions(-) 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/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) %> -
<%= Spree.t(:number) %>
+
@@ -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 %> From 299db9e651feeb3e4b56178a327ccc6dcc0ee6d4 Mon Sep 17 00:00:00 2001 From: Greg Date: Thu, 17 Dec 2015 08:43:41 -0600 Subject: [PATCH 04/15] refactor #determine_order_user helper method There's no behavior change to this. It allows subclasses to easily override how to determine the user. --- .../controllers/spree/api/orders_controller.rb | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) 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 From baeb2635f878bf366c819f03963b61d26ba2a890 Mon Sep 17 00:00:00 2001 From: exsuzme Date: Tue, 8 Dec 2015 14:27:45 -0500 Subject: [PATCH 05/15] HTML class hooks for return authorization form --- .../return_authorizations/_form.html.erb | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) 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 %> From 2440c8541d2e907a8df084b45cc0d232c0dcb902 Mon Sep 17 00:00:00 2001 From: Richard Nuno Date: Fri, 18 Dec 2015 10:46:48 -0500 Subject: [PATCH 06/15] Fix deleted tracking numbers on stock transfers When a stock transfer already has a tracking number, the value was being overridden by the default value of the ship method. --- .../spree/admin/stock_transfers_controller_spec.rb | 14 ++++++++------ core/app/models/spree/stock_transfer.rb | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) 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..ad9250a950e 100644 --- a/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb @@ -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/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 From 170b86f525861151c02cfa1a14e24e18a5f681e8 Mon Sep 17 00:00:00 2001 From: Richard Nuno Date: Mon, 21 Dec 2015 12:06:50 -0500 Subject: [PATCH 07/15] Add tests for new default value --- core/spec/models/spree/stock_transfer_spec.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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) } From 8e02765ed69c9ed16c192e0156430afa1a8decb9 Mon Sep 17 00:00:00 2001 From: Andrew Thal Date: Thu, 19 Nov 2015 10:51:53 -0500 Subject: [PATCH 08/15] Allow stock transfer permissions to view stock transfers to/from from viewable stock locations. When given the stock transfer display or stock transfer management permissions, there is a dropdown in admin/stock locations to filter by stock locations, which was filled by the ability to transfer_to or transfer_from a stock location. This change allows people with the stock transfer permission sets to use that feature of stock transfers without having to also have the ability to transfer_to or transfer_from a stock location. --- .../spree/admin/stock_transfers_controller.rb | 18 +++++------------- .../admin/stock_transfers_controller_spec.rb | 4 ++-- 2 files changed, 7 insertions(+), 15 deletions(-) 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/spec/controllers/spree/admin/stock_transfers_controller_spec.rb b/backend/spec/controllers/spree/admin/stock_transfers_controller_spec.rb index ad9250a950e..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) From e4901a18a9f5bee329a66a120349c6733bf115da Mon Sep 17 00:00:00 2001 From: Phillip Birtcher Date: Wed, 11 Nov 2015 11:54:22 -0500 Subject: [PATCH 09/15] Add OrderCancellations#cancel_unit + #reimburse_units * Ability to cancel a unit after it has been marked shipped * Easy way to reimburse units after Order has been completed --- core/app/models/spree/inventory_unit.rb | 2 +- core/app/models/spree/order_cancellations.rb | 51 +++++++++++++++ core/app/models/spree/reimbursement.rb | 10 +++ core/app/models/spree/unit_cancel.rb | 2 + .../models/spree/order_cancellations_spec.rb | 63 +++++++++++++++++++ core/spec/models/spree/reimbursement_spec.rb | 22 +++++++ 6 files changed, 149 insertions(+), 1 deletion(-) 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_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/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/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/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/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 From 19b501396fa528c60d92c771739fe11daebbc2f2 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 17 Dec 2015 14:00:39 -0800 Subject: [PATCH 10/15] Lock frontend and backend sprockets-rails 2.x This is causing errors in our builds: Asset was not declared to be precompiled in production. and NoMethodError: undefined method `register_engine' for nil:NilClass --- backend/solidus_backend.gemspec | 1 + frontend/solidus_frontend.gemspec | 1 + 2 files changed, 2 insertions(+) diff --git a/backend/solidus_backend.gemspec b/backend/solidus_backend.gemspec index 0e70262e608..66b788315ee 100644 --- a/backend/solidus_backend.gemspec +++ b/backend/solidus_backend.gemspec @@ -27,4 +27,5 @@ Gem::Specification.new do |s| s.add_dependency 'select2-rails', '3.5.9.1' # 3.5.9.2 breaks forms s.add_dependency 'handlebars_assets' + s.add_dependency 'sprockets-rails', '~> 2.0' # 3.0 breaks handlebars_assets end diff --git a/frontend/solidus_frontend.gemspec b/frontend/solidus_frontend.gemspec index 5cbe2aa8d87..722daca09c8 100644 --- a/frontend/solidus_frontend.gemspec +++ b/frontend/solidus_frontend.gemspec @@ -25,4 +25,5 @@ Gem::Specification.new do |s| s.add_dependency 'jquery-rails' s.add_development_dependency 'capybara-accessible' + s.add_development_dependency 'sprockets-rails', '~> 2.0' end From 2d09abe6619fc35cf15af809330e34059486095c Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Fri, 18 Dec 2015 23:31:42 -0700 Subject: [PATCH 11/15] Lock sprockets-rails to 2.x everywhere Same thing as 5988804b30805419e42d116a17746e0c22842ee4. This was making api, sample, and core specs fail. --- backend/solidus_backend.gemspec | 1 - core/solidus_core.gemspec | 1 + frontend/solidus_frontend.gemspec | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/backend/solidus_backend.gemspec b/backend/solidus_backend.gemspec index 66b788315ee..0e70262e608 100644 --- a/backend/solidus_backend.gemspec +++ b/backend/solidus_backend.gemspec @@ -27,5 +27,4 @@ Gem::Specification.new do |s| s.add_dependency 'select2-rails', '3.5.9.1' # 3.5.9.2 breaks forms s.add_dependency 'handlebars_assets' - s.add_dependency 'sprockets-rails', '~> 2.0' # 3.0 breaks handlebars_assets end 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/frontend/solidus_frontend.gemspec b/frontend/solidus_frontend.gemspec index 722daca09c8..5cbe2aa8d87 100644 --- a/frontend/solidus_frontend.gemspec +++ b/frontend/solidus_frontend.gemspec @@ -25,5 +25,4 @@ Gem::Specification.new do |s| s.add_dependency 'jquery-rails' s.add_development_dependency 'capybara-accessible' - s.add_development_dependency 'sprockets-rails', '~> 2.0' end From 57c12fc5b0c7b9070e2d97a56ee50214b89108c5 Mon Sep 17 00:00:00 2001 From: Greg Date: Mon, 4 Jan 2016 13:52:36 -0600 Subject: [PATCH 12/15] preserve shipping method when shipments change The main impetus of this is Tulip in which the guides have a much more non-linear approach to creating an order, but the same problem exists through the web client too. Other approaches have been tried, but lets try this within the Estimator itself. An extra side-effect of this is that we don't delete shipments in Order#ensure_updated_shipments so that's poorly named now, but it also raises a question of why it's called when it is. This means a shipment will stick with an order if created before we reach the "delivery" state of the order. Upon transition into "delivery" then #created_proposed_shipments gets called to ensure they're correct. The new logic in the Estimator now will re-select any prior shipping method/rate selection the customer had previously made. --- .../spree/api/line_items_controller_spec.rb | 38 ++++--------------- .../spree/api/orders_controller_spec.rb | 14 ------- core/app/models/spree/order.rb | 6 +-- core/app/models/spree/shipment.rb | 20 ++-------- core/app/models/spree/stock/estimator.rb | 21 ++++++++-- .../factories/order_factory.rb | 5 ++- .../factories/shipment_factory.rb | 7 ++-- .../core/unreturned_item_charger_spec.rb | 1 + core/spec/models/spree/order/checkout_spec.rb | 2 +- .../spree/order_shipping_method_spec.rb | 33 ++++++++++++++++ core/spec/models/spree/order_spec.rb | 22 ----------- core/spec/models/spree/shipment_spec.rb | 8 ++-- .../spec/models/spree/stock/estimator_spec.rb | 17 ++++++++- 13 files changed, 93 insertions(+), 101 deletions(-) create mode 100644 core/spec/models/spree/order_shipping_method_spec.rb 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..d9727e6c779 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 @@ -516,8 +515,6 @@ def apply_free_shipping_promotions # e.g. customer goes back from payment step and changes order items def ensure_updated_shipments if !completed? && shipments.all?(&:pending?) - self.shipments.destroy_all - self.update_column(:shipment_total, 0) restart_checkout_flow end @@ -530,6 +527,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/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_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..05b83d00972 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -268,22 +268,11 @@ 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 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 } @@ -305,17 +294,6 @@ 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 expect(order.state).to eq "cart" 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 From 84680845cb07b0866f818527346ad527edd41cc8 Mon Sep 17 00:00:00 2001 From: Greg Date: Thu, 7 Jan 2016 16:08:48 -0600 Subject: [PATCH 13/15] rename and comment ensure_updated_shipments --- core/app/models/spree/order.rb | 19 +++++++---- core/app/models/spree/order_contents.rb | 4 +-- core/spec/models/spree/order_contents_spec.rb | 20 ++++++------ core/spec/models/spree/order_spec.rb | 32 ++++--------------- 4 files changed, 30 insertions(+), 45 deletions(-) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index d9727e6c779..6129a93c76c 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -508,17 +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?) + # 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' 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/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_spec.rb b/core/spec/models/spree/order_spec.rb index 05b83d00972..bb42443953d 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -269,19 +269,19 @@ def merge!(other_order, user = nil) 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 "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 @@ -295,7 +295,7 @@ def merge!(other_order, user = nil) 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 @@ -308,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 @@ -334,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 From 07e5f4a9a0fc4b2225cc52bf1007a712ab1f37dc Mon Sep 17 00:00:00 2001 From: Richard Wilson Date: Fri, 22 Jan 2016 10:21:21 -0800 Subject: [PATCH 14/15] Add a carrier and service level column to Spree::ShippingMethods. When we're dealing with a shipping method, we have no formal way of adding additional information that would be useful for fulfillment purposes. We can add an "internal" name to the shipping method, but that would require pattern matching to get any useful information from. Often, in most fulfillment scenarios, we have a desire to know _exactly_ what carrier and service level a specific shipping method is representative of. Given a shipment I can then infer that it should be shipped with UPS one day shipping by introspecting on the methods' carrier and service_level columns as necessary. This is optional as it stands, but validations can be added in stores as required for complex fulfillment scenarios. --- ...d_carrier_and_service_level_to_spree_shipping_methods.rb | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 core/db/migrate/20160122182105_add_carrier_and_service_level_to_spree_shipping_methods.rb 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 From 43b90bdd07567128f819a07bac394739ee9398e9 Mon Sep 17 00:00:00 2001 From: Richard Wilson Date: Fri, 22 Jan 2016 11:12:20 -0800 Subject: [PATCH 15/15] Add carrier and service level to the backend. Allows users to set service levels and carriers for shipping methods on the backend. --- .../admin/shipping_methods/_form.html.erb | 18 ++++++++++++++++++ core/config/locales/en.yml | 3 +++ 2 files changed, 21 insertions(+) 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/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