Merge pull request #3560 from kristinalim/fix/2788-shipping_method_id_in_shipments

2696,2788 [Spree Upgrade] Fix use of shipping method ID for subscriptions

app/controllers/spree/admin/orders/customer_details_controller_decorator.rb

@@ -1,6 +1,22 @@
 Spree::Admin::Orders::CustomerDetailsController.class_eval do
   before_filter :set_guest_checkout_status, only: :update
 
+  def update
+    if @order.update_attributes(params[:order])
+      if params[:guest_checkout] == "false"
+        @order.associate_user!(Spree.user_class.find_by_email(@order.email))
+      end
+
+      AdvanceOrderService.new(@order).call
+
+      @order.shipments.map &:refresh_rates
+      flash[:success] = Spree.t('customer_details_updated')
+      redirect_to admin_order_customer_path(@order)
+    else
+      render :action => :edit
+    end
+  end
+
   # Inherit CanCan permissions for the current order
   def model_class
     load_order unless @order

app/controllers/spree/admin/orders_controller_decorator.rb

@@ -27,6 +27,17 @@ Spree::Admin::OrdersController.class_eval do
     # within the page then fetches the data it needs from Api::OrdersController
   end
 
+  def edit
+    @order.shipments.map &:refresh_rates
+
+    AdvanceOrderService.new(@order).call
+
+    # The payment step shows an error of 'No pending payments'
+    # Clearing the errors from the order object will stop this error
+    # appearing on the edit page where we don't want it to.
+    @order.errors.clear
+  end
+
   # Re-implement spree method so that it redirects to edit instead of rendering edit
   #   This allows page reloads while adding variants to the order (/edit), without being redirected to customer details page (/update)
   def update

app/controllers/spree/admin/payments_controller_decorator.rb

@@ -1,6 +1,35 @@
 Spree::Admin::PaymentsController.class_eval do
   append_before_filter :filter_payment_methods
 
+  def create
+    @payment = @order.payments.build(object_params)
+    if @payment.payment_method.is_a?(Spree::Gateway) && @payment.payment_method.payment_profiles_supported? && params[:card].present? and params[:card] != 'new'
+      @payment.source = CreditCard.find_by_id(params[:card])
+    end
+
+    begin
+      unless @payment.save
+        redirect_to admin_order_payments_path(@order)
+        return
+      end
+
+      if @order.completed?
+        @payment.process!
+        flash[:success] = flash_message_for(@payment, :successfully_created)
+
+         redirect_to admin_order_payments_path(@order)
+      else
+        AdvanceOrderService.new(@order).call!
+
+        flash[:success] = Spree.t(:new_order_completed)
+        redirect_to edit_admin_order_url(@order)
+      end
+
+    rescue Spree::Core::GatewayError => e
+      flash[:error] = "#{e.message}"
+      redirect_to new_admin_order_payment_path(@order)
+    end
+  end
 
   # When a user fires an event, take them back to where they came from
   # (we can't use respond_override because Spree no longer uses respond_with)

app/services/advance_order_service.rb

@@ -0,0 +1,42 @@
+class AdvanceOrderService
+  attr_reader :order
+
+  def initialize(order)
+    @order = order
+  end
+
+  def call
+    advance_order(advance_order_options)
+  end
+
+  def call!
+    advance_order!(advance_order_options)
+  end
+
+  private
+
+  def advance_order_options
+    shipping_method_id = order.shipping_method.id if order.shipping_method.present?
+    { shipping_method_id: shipping_method_id }
+  end
+
+  def advance_order(options)
+    until order.state == "complete"
+      break unless order.next
+      after_transition_hook(options)
+    end
+  end
+
+  def advance_order!(options)
+    until order.completed?
+      order.next!
+      after_transition_hook(options)
+    end
+  end
+
+  def after_transition_hook(options)
+    if order.state == "delivery"
+      order.select_shipping_method(options[:shipping_method_id]) if options[:shipping_method_id]
+    end
+  end
+end

app/services/order_factory.rb

@@ -15,7 +15,10 @@ class OrderFactory
     set_user
     build_line_items
     set_addresses
+    create_shipment
+    set_shipping_method
     create_payment
+
     @order
   end
 
@@ -65,6 +68,14 @@ class OrderFactory
     @order.update_attributes(attrs.slice(:bill_address_attributes, :ship_address_attributes))
   end
 
+  def create_shipment
+    @order.create_proposed_shipments
+  end
+
+  def set_shipping_method
+    @order.select_shipping_method(attrs[:shipping_method_id])
+  end
+
   def create_payment
     @order.update_distribution_charge!
     @order.payments.create(payment_method_id: attrs[:payment_method_id], amount: @order.reload.total)

app/services/order_syncer.rb

@@ -66,14 +66,12 @@ class OrderSyncer
   end
 
   def update_shipment_for(order)
-    shipment = order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id_was).last
-    if shipment
-      shipment.update_attributes(shipping_method_id: shipping_method_id)
-      order.update_attribute(:shipping_method_id, shipping_method_id)
+    return if pending_shipment_with?(order, shipping_method_id) # No need to do anything.
+
+    if pending_shipment_with?(order, shipping_method_id_was)
+      order.select_shipping_method(shipping_method_id)
     else
-      unless order.shipments.with_state('pending').where(shipping_method_id: shipping_method_id).any?
-        order_update_issues.add(order, I18n.t('admin.shipping_method'))
-      end
+      order_update_issues.add(order, I18n.t('admin.shipping_method'))
     end
   end
 
@@ -106,4 +104,9 @@ class OrderSyncer
       order.ship_address[attr] == distributor_address[attr]
     end
   end
+
+  def pending_shipment_with?(order, shipping_method_id)
+    return false unless order.shipment.present? && order.shipment.state == "pending"
+    order.shipping_method.id == shipping_method_id
+  end
 end

spec/controllers/admin/proxy_orders_controller_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-xdescribe Admin::ProxyOrdersController, type: :controller do
+describe Admin::ProxyOrdersController, type: :controller do
   include AuthenticationWorkflow
 
   describe 'cancel' do
@@ -107,7 +107,7 @@ xdescribe Admin::ProxyOrdersController, type: :controller do
           before { shop.update_attributes(owner: user) }
 
           context "when resuming succeeds" do
-            it 'renders the resumed proxy_order as json' do
+            xit 'renders the resumed proxy_order as json' do
               spree_get :resume, params
               json_response = JSON.parse(response.body)
               expect(json_response['state']).to eq "resumed"

spec/controllers/spree/admin/orders/customer_details_controller_spec.rb

@@ -39,6 +39,14 @@ describe Spree::Admin::Orders::CustomerDetailsController, type: :controller do
         login_as_enterprise_user [order.distributor]
       end
 
+      it "advances the order state" do
+        expect {
+          spree_post :update, order: { email: user.email, bill_address_attributes: address_params,
+                                       ship_address_attributes: address_params },
+                              order_id: order.number
+        }.to change { order.reload.state }.from("cart").to("complete")
+      end
+
       context "when adding details of a registered user" do
         it "redirects to shipments on success" do
           spree_post :update, order: { email: user.email, bill_address_attributes: address_params, ship_address_attributes: address_params }, order_id: order.number

spec/controllers/spree/admin/orders_controller_spec.rb

@@ -4,6 +4,18 @@ describe Spree::Admin::OrdersController, type: :controller do
   include AuthenticationWorkflow
   include OpenFoodNetwork::EmailHelper
 
+  describe "#edit" do
+    let!(:order) { create(:order_with_totals_and_distribution, ship_address: create(:address)) }
+
+    before { login_as_admin }
+
+    it "advances the order state" do
+      expect {
+        spree_get :edit, id: order
+      }.to change { order.reload.state }.from("cart").to("complete")
+    end
+  end
+
   context "#update" do
     let(:params) do
       { id: order,

spec/controllers/spree/admin/payments_controller_spec.rb

@@ -6,9 +6,27 @@ describe Spree::Admin::PaymentsController, type: :controller do
   let!(:order) { create(:order, distributor: shop, state: 'complete') }
   let!(:line_item) { create(:line_item, order: order, price: 5.0) }
 
+  before do
+    allow(controller).to receive(:spree_current_user) { user }
+  end
+
+  context "#create" do
+    let!(:payment_method) { create(:payment_method, distributors: [ shop ]) }
+    let!(:order) do
+      create(:order_with_totals_and_distribution, distributor: shop, state: "payment")
+    end
+
+    let(:params) { { amount: order.total, payment_method_id: payment_method.id } }
+
+    it "advances the order state" do
+      expect {
+        spree_post :create, payment: params, order_id: order.number
+      }.to change { order.reload.state }.from("payment").to("complete")
+    end
+  end
+
   context "as an enterprise user" do
     before do
-      allow(controller).to receive(:spree_current_user) { user }
       order.reload.update_totals
     end
 

spec/features/admin/subscriptions_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-xfeature 'Subscriptions' do
+feature 'Subscriptions' do
   include AdminHelper
   include AuthenticationWorkflow
   include WebHelper

spec/jobs/subscription_placement_job_spec.rb

@@ -40,7 +40,8 @@ describe SubscriptionPlacementJob do
 
   describe "performing the job" do
     context "when unplaced proxy_orders exist" do
-      let!(:proxy_order) { create(:proxy_order) }
+      let!(:subscription) { create(:subscription, with_items: true) }
+      let!(:proxy_order) { create(:proxy_order, subscription: subscription) }
 
       before do
         allow(job).to receive(:proxy_orders) { ProxyOrder.where(id: proxy_order.id) }

spec/models/proxy_order_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-xdescribe ProxyOrder, type: :model do
+describe ProxyOrder, type: :model do
   describe "cancel" do
     let(:order_cycle) { create(:simple_order_cycle) }
     let(:subscription) { create(:subscription) }
@@ -78,7 +78,7 @@ xdescribe ProxyOrder, type: :model do
   describe "resume" do
     let!(:payment_method) { create(:payment_method) }
     let!(:shipment) { create(:shipment) }
-    let(:order) { create(:order_with_totals, shipments: [shipment]) }
+    let(:order) { create(:order_with_totals, ship_address: create(:address), shipments: [shipment]) }
     let(:proxy_order) { create(:proxy_order, order: order, canceled_at: Time.zone.now) }
     let(:order_cycle) { proxy_order.order_cycle }
 

spec/services/advance_order_service_spec.rb

@@ -0,0 +1,55 @@
+require "spec_helper"
+
+describe AdvanceOrderService do
+  let!(:distributor) { create(:distributor_enterprise) }
+  let!(:order) do
+    create(:order_with_totals_and_distribution, distributor: distributor,
+                                                bill_address: create(:address),
+                                                ship_address: create(:address))
+  end
+
+  let(:service) { described_class.new(order) }
+
+  it "transitions the order multiple steps" do
+    expect(order.state).to eq("cart")
+    service.call
+    order.reload
+    expect(order.state).to eq("complete")
+  end
+
+  describe "transition from delivery" do
+    let!(:shipping_method_a) { create(:shipping_method, distributors: [ distributor ]) }
+    let!(:shipping_method_b) { create(:shipping_method, distributors: [ distributor ]) }
+    let!(:shipping_method_c) { create(:shipping_method, distributors: [ distributor ]) }
+
+    before do
+      # Create shipping rates for available shipping methods.
+      order.shipments.each(&:refresh_rates)
+    end
+
+    it "retains delivery method of the order" do
+      order.select_shipping_method(shipping_method_b.id)
+      service.call
+      order.reload
+      expect(order.shipping_method).to eq(shipping_method_b)
+    end
+  end
+
+  context "when raising on error" do
+    it "transitions the order multiple steps" do
+      service.call!
+      order.reload
+      expect(order.state).to eq("complete")
+    end
+
+    context "when order cannot advance to the next state" do
+      let!(:order) do
+        create(:order, distributor: distributor)
+      end
+
+      it "raises error" do
+        expect { service.call! }.to raise_error(StateMachine::InvalidTransition)
+      end
+    end
+  end
+end

spec/services/order_factory_spec.rb

@@ -7,6 +7,9 @@ describe OrderFactory do
   let(:customer) { create(:customer, user: user) }
   let(:shop) { create(:distributor_enterprise) }
   let(:order_cycle) { create(:simple_order_cycle) }
+  let!(:other_shipping_method_a) { create(:shipping_method) }
+  let!(:shipping_method) { create(:shipping_method) }
+  let!(:other_shipping_method_b) { create(:shipping_method) }
   let(:payment_method) { create(:payment_method) }
   let(:ship_address) { create(:address) }
   let(:bill_address) { create(:address) }
@@ -21,6 +24,7 @@ describe OrderFactory do
       attrs[:customer_id] = customer.id
       attrs[:distributor_id] = shop.id
       attrs[:order_cycle_id] = order_cycle.id
+      attrs[:shipping_method_id] = shipping_method.id
       attrs[:payment_method_id] = payment_method.id
       attrs[:bill_address_attributes] = bill_address.attributes.except("id")
       attrs[:ship_address_attributes] = ship_address.attributes.except("id")
@@ -35,6 +39,7 @@ describe OrderFactory do
       expect(order.user).to eq user
       expect(order.distributor).to eq shop
       expect(order.order_cycle).to eq order_cycle
+      expect(order.shipments.first.shipping_method).to eq shipping_method
       expect(order.payments.first.payment_method).to eq payment_method
       expect(order.bill_address).to eq bill_address
       expect(order.ship_address).to eq ship_address
@@ -42,6 +47,19 @@ describe OrderFactory do
       expect(order.complete?).to be false
     end
 
+    it "retains address, delivery, and payment attributes until completion of the order" do
+      AdvanceOrderService.new(order).call
+
+      order.reload
+
+      expect(order.customer).to eq customer
+      expect(order.shipping_method).to eq shipping_method
+      expect(order.payments.first.payment_method).to eq payment_method
+      expect(order.bill_address).to eq bill_address
+      expect(order.ship_address).to eq ship_address
+      expect(order.total).to eq 38.0
+    end
+
     context "when the customer does not have a user associated with it" do
       before { customer.update_attribute(:user_id, nil) }
 

spec/services/order_syncer_spec.rb

@@ -2,17 +2,23 @@ require 'spec_helper'
 
 describe OrderSyncer do
   describe "updating the shipping method" do
-    let(:subscription) { create(:subscription, with_items: true, with_proxy_orders: true) }
-    let(:order) { subscription.proxy_orders.first.initialise_order! }
-    let(:shipping_method) { subscription.shipping_method }
-    let(:new_shipping_method) { create(:shipping_method, distributors: [subscription.shop]) }
+    let!(:subscription) { create(:subscription, with_items: true, with_proxy_orders: true) }
+    let!(:order) { subscription.proxy_orders.first.initialise_order! }
+    let!(:shipping_method) { subscription.shipping_method }
+    let!(:new_shipping_method) { create(:shipping_method, distributors: [subscription.shop]) }
+
     let(:syncer) { OrderSyncer.new(subscription) }
 
     context "when the shipping method on an order is the same as the subscription" do
       let(:params) { { shipping_method_id: new_shipping_method.id } }
 
-      xit "updates the shipping_method on the order and on shipments" do
-        expect(order.shipments.first.shipping_method_id_was).to eq shipping_method.id
+      before do
+        # Create shipping rates for available shipping methods.
+        order.shipments.each(&:refresh_rates)
+      end
+
+      it "updates the shipping_method on the order and on shipments" do
+        expect(order.shipments.first.shipping_method).to eq shipping_method
         subscription.assign_attributes(params)
         expect(syncer.sync!).to be true
         expect(order.reload.shipping_method).to eq new_shipping_method
@@ -25,16 +31,18 @@ describe OrderSyncer do
 
       context "when the shipping method on a shipment is the same as the new shipping method on the subscription" do
         before do
+          # Create shipping rates for available shipping methods.
+          order.shipments.each(&:refresh_rates)
+
           # Updating the shipping method on a shipment updates the shipping method on the order,
           # and vice-versa via logic in Spree's shipments controller. So updating both here mimics that
           # behaviour.
-          order.shipments.first.update_attributes(shipping_method_id: new_shipping_method.id)
-          order.update_attributes(shipping_method_id: new_shipping_method.id)
+          order.select_shipping_method(new_shipping_method.id)
           subscription.assign_attributes(params)
           expect(syncer.sync!).to be true
         end
 
-        xit "does not update the shipping_method on the subscription or on the pre-altered shipment" do
+        it "does not update the shipping_method on the subscription or on the pre-altered shipment" do
           expect(order.reload.shipping_method).to eq new_shipping_method
           expect(order.reload.shipments.first.shipping_method).to eq new_shipping_method
           expect(syncer.order_update_issues[order.id]).to be nil
@@ -42,19 +50,22 @@ describe OrderSyncer do
       end
 
       context "when the shipping method on a shipment is not the same as the new shipping method on the subscription" do
-        let(:changed_shipping_method) { create(:shipping_method) }
+        let!(:changed_shipping_method) { create(:shipping_method) }
 
         before do
+          # Create shipping rates for available shipping methods.
+          order.shipments.each(&:refresh_rates)
+
           # Updating the shipping method on a shipment updates the shipping method on the order,
           # and vice-versa via logic in Spree's shipments controller. So updating both here mimics that
           # behaviour.
-          order.shipments.first.update_attributes(shipping_method_id: changed_shipping_method.id)
-          order.update_attributes(shipping_method_id: changed_shipping_method.id)
+          order.select_shipping_method(changed_shipping_method.id)
           subscription.assign_attributes(params)
+
           expect(syncer.sync!).to be true
         end
 
-        xit "does not update the shipping_method on the subscription or on the pre-altered shipment" do
+        it "does not update the shipping_method on the subscription or on the pre-altered shipment" do
           expect(order.reload.shipping_method).to eq changed_shipping_method
           expect(order.reload.shipments.first.shipping_method).to eq changed_shipping_method
           expect(syncer.order_update_issues[order.id]).to include "Shipping Method"
@@ -226,7 +237,7 @@ describe OrderSyncer do
     context "when a ship address is not required" do
       before { shipping_method.update_attributes(require_ship_address: false) }
 
-      xit "does not change the ship address" do
+      it "does not change the ship address" do
         subscription.assign_attributes(params)
         expect(syncer.sync!).to be true
         expect(syncer.order_update_issues.keys).to_not include order.id
@@ -239,9 +250,10 @@ describe OrderSyncer do
 
       context "but the shipping method is being changed to one that requires a ship_address" do
         let(:new_shipping_method) { create(:shipping_method, require_ship_address: true) }
+
         before { params.merge!(shipping_method_id: new_shipping_method.id) }
 
-        xit "updates ship_address attrs" do
+        it "updates ship_address attrs" do
           subscription.assign_attributes(params)
           expect(syncer.sync!).to be true
           expect(syncer.order_update_issues.keys).to_not include order.id
@@ -272,7 +284,7 @@ describe OrderSyncer do
 
       context "when the ship address on the order doesn't match that on the subscription" do
         before { order.ship_address.update_attributes(firstname: "Jane") }
-        xit "does not update ship_address on the order" do
+        it "does not update ship_address on the order" do
           subscription.assign_attributes(params)
           expect(syncer.sync!).to be true
           expect(syncer.order_update_issues.keys).to include order.id
@@ -312,7 +324,7 @@ describe OrderSyncer do
       let(:params) { { subscription_line_items_attributes: [{ id: sli.id, quantity: 3}] } }
       let(:syncer) { OrderSyncer.new(subscription) }
 
-      xit "updates the line_item quantities and totals on all orders" do
+      it "updates the line_item quantities and totals on all orders" do
         expect(order.reload.total.to_f).to eq 59.97
         subscription.assign_attributes(params)
         expect(syncer.sync!).to be true

spec/services/subscription_form_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-xdescribe SubscriptionForm do
+describe SubscriptionForm do
   describe "creating a new subscription" do
     let!(:shop) { create(:distributor_enterprise) }
     let!(:customer) { create(:customer, enterprise: shop) }