Use order shipments

app/assets/javascripts/darkswarm/controllers/checkout/checkout_controller.js.coffee

@@ -7,6 +7,7 @@ Darkswarm.controller "CheckoutCtrl", ($scope, localStorageService, Checkout, Cur
   prefix = "order_#{Checkout.order.id}#{CurrentUser.id or ""}#{CurrentHub.hub.id}"
 
   for field in $scope.fieldsToBind
+    # SHIPPING_METHOD
     localStorageService.bind $scope, "Checkout.order.#{field}", Checkout.order[field], "#{prefix}_#{field}"
 
   localStorageService.bind $scope, "Checkout.ship_address_same_as_billing", true, "#{prefix}_sameasbilling"

app/assets/javascripts/darkswarm/services/checkout.js.coffee

@@ -30,6 +30,7 @@ Darkswarm.factory 'Checkout', (CurrentOrder, ShippingMethods, PaymentMethods, $h
             munged_order["ship_address_attributes"] = value
           when "payment_method_id"
             munged_order["payments_attributes"] = [{payment_method_id: value}]
+          # SHIPPING_METHOD
           when "shipping_method_id", "payment_method_id", "email", "special_instructions"
             munged_order[name] = value
           else
@@ -56,6 +57,12 @@ Darkswarm.factory 'Checkout', (CurrentOrder, ShippingMethods, PaymentMethods, $h
       munged_order
 
     shippingMethod: ->
+      # Used to get the shiping method data from the passed in id.
+      #
+      # We should store the shipping method object once retrieved. Otherwise we
+      # fetch it every time we call the method, which is few times.
+      #
+      # SHIPPING_METHOD
       ShippingMethods.shipping_methods_by_id[@order.shipping_method_id] if @order.shipping_method_id
 
     requireShipAddress: ->

app/controllers/spree/admin/shipping_methods_controller_decorator.rb

@@ -1,16 +1,16 @@
 module Spree
   module Admin
     ShippingMethodsController.class_eval do
-      before_filter :do_not_destroy_referenced_shipping_methods, :only => :destroy
-      before_filter :load_hubs, only: [:new, :edit, :create, :update]
+      before_filter :do_not_destroy_referenced_shipping_methods, only: :destroy
+      before_filter :load_hubs, only: %i(new edit create update)
 
       # Sort shipping methods by distributor name
       # ! Code copied from Spree::Admin::ResourceController with two added lines
       def collection
         return parent.send(controller_name) if parent_data.present?
 
         collection = if model_class.respond_to?(:accessible_by) &&
-                         !current_ability.has_block?(params[:action], model_class)
+                        !current_ability.has_block?(params[:action], model_class)
 
                        model_class.accessible_by(current_ability, action)
 
@@ -29,21 +29,24 @@ def collection
         collection
       end
 
+      # SHIPPING_METHOD
+      #
       # This method was originally written because ProductDistributions referenced shipping
       # methods, and deleting a referenced shipping method would break all the reports that
       # queried it.
       # This has changed, and now all we're protecting is Orders, which is a spree resource.
       # Do we really need to protect it ourselves? Does spree do this, or provide some means
       # of preserving the shipping method information for past orders?
       def do_not_destroy_referenced_shipping_methods
-        order = Order.where(:shipping_method_id => @object).first
+        order = Order.where(shipping_method_id: @object).first
         if order
           flash[:error] = "That shipping method cannot be deleted as it is referenced by an order: #{order.number}."
-          redirect_to collection_url and return
+          redirect_to(collection_url) && return
         end
       end
 
       private
+
       def load_hubs
         @hubs = Enterprise.managed_by(spree_current_user).is_distributor.sort_by!{ |d| [(@shipping_method.has_distributor? d) ? 0 : 1, d.name] }
       end

app/jobs/finalize_account_invoices.rb

@@ -9,19 +9,21 @@ def initialize(year = nil, month = nil)
     @end_date = Time.zone.local(@year, @month) + 1.month
   end
 
-  def before(job)
+  def before(_job)
     UpdateBillablePeriods.new(year, month).perform
     UpdateAccountInvoices.new(year, month).perform
   end
 
   def perform
     return unless settings_are_valid?
 
-
     invoice_orders = AccountInvoice.where(year: year, month: month).map(&:order)
-    invoice_orders.select{ |order| order.present? && order.completed_at.nil? }.each{ |order| finalize(order) }
+    invoice_orders
+      .select { |order| order.present? && order.completed_at.nil? }
+      .each { |order| finalize(order) }
   end
 
+  # SHIPPING_METHOD
   def finalize(invoice_order)
     # TODO: When we implement per-customer and/or per-user preferences around shipping and payment methods
     # we can update these to read from those preferences
@@ -30,12 +32,12 @@ def finalize(invoice_order)
     while invoice_order.state != "complete"
       if invoice_order.errors.any?
         Bugsnag.notify(RuntimeError.new("FinalizeInvoiceError"), {
-          job: "FinalizeAccountInvoices",
-          error: "Cannot finalize invoice due to errors",
-          data: {
-            errors: invoice_order.errors.full_messages
-          }
-        })
+                         job: "FinalizeAccountInvoices",
+                         error: "Cannot finalize invoice due to errors",
+                         data: {
+                           errors: invoice_order.errors.full_messages
+                         }
+                       })
         break
       else
         invoice_order.next
@@ -48,46 +50,46 @@ def finalize(invoice_order)
   def settings_are_valid?
     unless end_date <= Time.zone.now
       Bugsnag.notify(RuntimeError.new("InvalidJobSettings"), {
-        job: "FinalizeAccountInvoices",
-        error: "end_date is in the future",
-        data: {
-          end_date: end_date.in_time_zone.strftime("%F %T"),
-          now: Time.zone.now.strftime("%F %T")
-        }
-      })
+                       job: "FinalizeAccountInvoices",
+                       error: "end_date is in the future",
+                       data: {
+                         end_date: end_date.in_time_zone.strftime("%F %T"),
+                         now: Time.zone.now.strftime("%F %T")
+                       }
+                     })
       return false
     end
 
     unless @accounts_distributor = Enterprise.find_by_id(Spree::Config.accounts_distributor_id)
       Bugsnag.notify(RuntimeError.new("InvalidJobSettings"), {
-        job: "FinalizeAccountInvoices",
-        error: "accounts_distributor_id is invalid",
-        data: {
-          accounts_distributor_id: Spree::Config.accounts_distributor_id
-        }
-      })
+                       job: "FinalizeAccountInvoices",
+                       error: "accounts_distributor_id is invalid",
+                       data: {
+                         accounts_distributor_id: Spree::Config.accounts_distributor_id
+                       }
+                     })
       return false
     end
 
     unless @accounts_distributor.payment_methods.find_by_id(Spree::Config.default_accounts_payment_method_id)
       Bugsnag.notify(RuntimeError.new("InvalidJobSettings"), {
-        job: "FinalizeAccountInvoices",
-        error: "default_accounts_payment_method_id is invalid",
-        data: {
-          default_accounts_payment_method_id: Spree::Config.default_accounts_payment_method_id
-        }
-      })
+                       job: "FinalizeAccountInvoices",
+                       error: "default_accounts_payment_method_id is invalid",
+                       data: {
+                         default_accounts_payment_method_id: Spree::Config.default_accounts_payment_method_id
+                       }
+                     })
       return false
     end
 
     unless @accounts_distributor.shipping_methods.find_by_id(Spree::Config.default_accounts_shipping_method_id)
       Bugsnag.notify(RuntimeError.new("InvalidJobSettings"), {
-        job: "FinalizeAccountInvoices",
-        error: "default_accounts_shipping_method_id is invalid",
-        data: {
-          default_accounts_shipping_method_id: Spree::Config.default_accounts_shipping_method_id
-        }
-      })
+                       job: "FinalizeAccountInvoices",
+                       error: "default_accounts_shipping_method_id is invalid",
+                       data: {
+                         default_accounts_shipping_method_id: Spree::Config.default_accounts_shipping_method_id
+                       }
+                     })
       return false
     end
 

app/models/spree/order_decorator.rb

@@ -79,10 +79,11 @@ def products_available_from_new_distribution
     errors.add(:base, "Distributor or order cycle cannot supply the products in your cart") unless DistributionChangeValidator.new(self).can_change_to_distribution?(distributor, order_cycle)
   end
 
+  # SHIPPING_METHOD
   def empty_with_clear_shipping_and_payments!
     empty_without_clear_shipping_and_payments!
     payments.clear
-    update_attributes(shipping_method_id: nil)
+    shipments.destroy_all
   end
   alias_method_chain :empty!, :clear_shipping_and_payments
 

app/serializers/api/current_order_serializer.rb

@@ -15,4 +15,9 @@ def payment_method_id
   def display_total
     object.display_total.money.to_f
   end
+
+  def shipping_method_id
+    return unless object.shipments.any?
+    object.shipments.last.shipping_method_id
+  end
 end

app/views/checkout/_shipping.html.haml

@@ -15,6 +15,7 @@
 
       .small-12.columns.medium-12.columns.large-6.columns
         %label{"ng-repeat" => "method in ShippingMethods.shipping_methods"}
+          -# SHIPPING_METHOD
           %input{type: :radio,
             required: true,
             name: "order.shipping_method_id",
@@ -27,6 +28,7 @@
               ({{ method.price | localizeCurrency }})
 
         %small.error.medium.input-text{"ng-show" => "!fieldValid('order.shipping_method_id')"}
+          -# SHIPPING_METHOD
           = "{{ fieldErrors('order.shipping_method_id') }}"
 
         %label{"ng-if" => "Checkout.requireShipAddress()"}

app/views/spree/shared/_delivery_shipment_details.html.haml

@@ -0,0 +1,25 @@
+.order-summary.text-small
+  %strong= shipment.shipping_method.name
+.pad
+  .text-big
+    = t :order_delivery_time
+    %strong #{order.order_cycle.pickup_time_for(order.distributor)}
+  %p.text-small.text-skinny.pre-line
+    %em= shipment.shipping_method.description.andand.html_safe || ""
+.order-summary.text-small
+  %strong
+    = t :order_delivery_address
+.pad
+  %p.text-small
+    = order.ship_address.firstname + " " + order.ship_address.lastname
+    %br
+    = order.ship_address.full_address
+    %br
+    = order.ship_address.phone
+  - if order.special_instructions.present?
+    %br
+    %p.light.small
+      %strong
+        = t :order_special_instructions
+      %br
+      = order.special_instructions

app/views/spree/shared/_order_details.html.haml

@@ -29,60 +29,11 @@
         = order.bill_address.phone
 
   .columns.large-6
-    - if order.shipping_method.andand.require_ship_address
-      // Delivery option
-      .order-summary.text-small
-        %strong= order.shipping_method.name
-      .pad
-        .text-big
-          = t :order_delivery_time
-          %strong #{order.order_cycle.pickup_time_for(order.distributor)}
-        %p.text-small.text-skinny.pre-line
-          %em= order.shipping_method.description.andand.html_safe || ""
-      .order-summary.text-small
-        %strong
-          = t :order_delivery_address
-      .pad
-        %p.text-small
-          = order.ship_address.firstname + " " + order.ship_address.lastname
-          %br
-          = order.ship_address.full_address
-          %br
-          = order.ship_address.phone
-        - if order.special_instructions.present?
-          %br
-          %p.light.small
-            %strong
-              = t :order_special_instructions
-            %br
-            = order.special_instructions
+    - shipment = order.shipments.last
+    - if shipment.shipping_method.andand.require_ship_address
+      = render 'spree/shared/delivery_shipment_details', order: order, shipment: shipment
     - else
-      // Collection option
-      .order-summary.text-small
-        %strong= order.shipping_method.name
-      .pad
-        .text-big
-          = t :order_pickup_time
-          %strong #{order.order_cycle.pickup_time_for(order.distributor)}
-        %p.text-small.text-skinny.pre-line
-          %em= order.shipping_method.description.andand.html_safe || ""
-
-        - if order.order_cycle.pickup_instructions_for(order.distributor).present?
-          %br
-          %p.text-small
-            %strong
-              = t :order_pickup_instructions
-            %br
-            #{order.order_cycle.pickup_instructions_for(order.distributor)}
-
-        - if order.special_instructions.present?
-          %br
-          %p.light.small
-            %strong
-              = t :order_special_instructions
-            %br
-            = order.special_instructions
-
+      = render 'spree/shared/pickup_shipment_details', order: order, shipment: shipment
 %br
 .row
   .columns.large-12

app/views/spree/shared/_pickup_shipment_details.html.haml

@@ -0,0 +1,25 @@
+.order-summary.text-small
+  %strong= order.shipments.last.shipping_method.name
+.pad
+  .text-big
+    = t :order_pickup_time
+    %strong #{order.order_cycle.pickup_time_for(order.distributor)}
+  %p.text-small.text-skinny.pre-line
+    %em= order.shipments.last.shipping_method.description.andand.html_safe || ""
+
+  - if order.order_cycle.pickup_instructions_for(order.distributor).present?
+    %br
+    %p.text-small
+      %strong
+        = t :order_pickup_instructions
+      %br
+      #{order.order_cycle.pickup_instructions_for(order.distributor)}
+
+  - if order.special_instructions.present?
+    %br
+    %p.light.small
+      %strong
+        = t :order_special_instructions
+      %br
+      = order.special_instructions
+

spec/controllers/spree/admin/shipping_methods_controller_decorator_spec.rb

@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe Spree::Admin::ShippingMethodsController do
+  include AuthenticationWorkflow
+
+  describe '#destroy' do
+    let!(:shipping_method) { create(:shipping_method) }
+
+    before { login_as_admin }
+
+    context 'when the shipping method is in use' do
+      before { create(:order, shipping_method: shipping_method) }
+
+      it 'does not allow to destroy the shipping method' do
+        expect { delete :destroy, id: shipping_method.id }
+          .not_to change(Spree::ShippingMethod, :count)
+      end
+
+      it 'shows a flash error message' do
+        spree_delete :destroy, id: shipping_method.id
+        expect(flash[:error]).to match('That shipping method cannot be deleted')
+      end
+
+      it 'redirects to the collection url' do
+        expect(spree_delete(:destroy, id: shipping_method.id))
+          .to redirect_to('/admin/shipping_methods')
+      end
+    end
+
+    context 'when the shipping method is not in use' do
+      it 'allows to destroy the shipping method' do
+        expect { delete :destroy, id: shipping_method.id }
+          .to change(Spree::ShippingMethod, :count)
+      end
+    end
+  end
+end

spec/features/consumer/shopping/orders_spec.rb

@@ -21,6 +21,11 @@
       quick_login_as user
     end
 
+    it 'shows the name of the shipping method' do
+      visit spree.order_path(order)
+      expect(find('#order')).to have_content(shipping_method.name)
+    end
+
     context "when the distributor doesn't allow changes to be made to orders" do
       before do
         order.distributor.update_attributes(allow_order_changes: false)