Dont assign distributor address to pickups

app/controllers/checkout_controller.rb

@@ -125,7 +125,8 @@ def advance_order_state(order)
 
 
   def update_failed
-    clear_ship_address
+    current_order.clear_ship_address
+
     respond_to do |format|
       format.html do
         render :edit
@@ -136,14 +137,6 @@ def update_failed
     end
   end
 
-  # When we have a pickup Shipping Method, we clone the distributor address into ship_address before_save
-  # We don't want this data in the form, so we clear it out
-  def clear_ship_address
-    unless current_order.shipping_method.andand.require_ship_address
-      current_order.ship_address = Spree::Address.default
-    end
-  end
-
   def skip_state_validation?
     true
   end
@@ -168,8 +161,8 @@ def before_address
 
     customer_preferred_bill_address, customer_preferred_ship_address = @order.customer.bill_address, @order.customer.ship_address if @order.customer
 
-    @order.bill_address ||= customer_preferred_bill_address ||= preferred_bill_address || last_used_bill_address || Spree::Address.default
-    @order.ship_address ||= customer_preferred_ship_address ||= preferred_ship_address || last_used_ship_address || Spree::Address.default
+    @order.bill_address ||= customer_preferred_bill_address || preferred_bill_address || last_used_bill_address || Spree::Address.default
+    @order.ship_address ||= customer_preferred_ship_address || preferred_ship_address || last_used_ship_address || Spree::Address.default
   end
 
   def after_payment

app/models/delivery.rb

@@ -0,0 +1,20 @@
+class Delivery
+
+  # Constructor
+  #
+  # @param order [Spree::Order]
+  def initialize(order)
+    @order = order
+  end
+
+  # Returns the appropriate ship address when the form field is cleared
+  #
+  # @return [Spree::Address]
+  def ship_address_on_clear
+    order.ship_address
+  end
+
+  private
+
+  attr_reader :order
+end

app/models/pickup.rb

@@ -0,0 +1,20 @@
+class Pickup
+
+  # Constructor
+  #
+  # @param order [Spree::Order]
+  def initialize(order)
+    @order = order
+  end
+
+  # Returns the appropriate ship address when the form field is cleared
+  #
+  # @return [Spree::Address]
+  def ship_address_on_clear
+    Spree::Address.default
+  end
+
+  private
+
+  attr_reader :order
+end

app/models/spree/order_decorator.rb

@@ -17,7 +17,6 @@
   validate :products_available_from_new_distribution, :if => lambda { distributor_id_changed? || order_cycle_id_changed? }
   attr_accessible :order_cycle_id, :distributor_id
 
-  before_validation :shipping_address_from_distributor
   before_validation :associate_customer, unless: :customer_id?
   before_validation :ensure_customer, unless: :customer_is_valid?
 
@@ -29,7 +28,7 @@
     go_to_state :delivery
     go_to_state :payment, :if => lambda { |order|
       # Fix for #2191
-      if order.shipping_method.andand.require_ship_address and
+      if order.shipping_method.andand.delivery?
         if order.ship_address.andand.valid?
           order.create_shipment!
           order.update_totals
@@ -290,23 +289,22 @@ def changes_allowed?
     complete? && distributor.andand.allow_order_changes? && order_cycle.andand.open?
   end
 
+  # When we have a pickup Shipping Method we don't want any data in the form,
+  # so we clear it out
+  def clear_ship_address
+    self.ship_address = shipping_method_factory.ship_address_on_clear
+  end
+
   private
 
-  def shipping_address_from_distributor
-    if distributor
-      # This method is confusing to conform to the vagaries of the multi-step checkout
-      # We copy over the shipping address when we have no shipping method selected
-      # We can refactor this when we drop the multi-step checkout option
-      #
-      if shipping_method.andand.require_ship_address == false
-        self.ship_address = distributor.address.clone
-
-        if bill_address
-          ship_address.firstname = bill_address.firstname
-          ship_address.lastname = bill_address.lastname
-          ship_address.phone = bill_address.phone
-        end
-      end
+  # Returns the appropriate shipping method instance for the order
+  #
+  # @return [#ship_address_on_clear]
+  def shipping_method_factory
+    if shipping_method
+      shipping_method.instance_for(self)
+    else
+      Pickup.new(self)
     end
   end
 

app/models/spree/shipping_method_decorator.rb

@@ -65,6 +65,21 @@ def adjustment_label
     'Shipping'
   end
 
+  def delivery?
+    require_ship_address
+  end
+
+  # Returns a particular instance of shipping method
+  #
+  # @return [#ship_address_on_clear]
+  def instance_for(order)
+    if delivery?
+      Delivery.new(order)
+    else
+      Pickup.new(order)
+    end
+  end
+
   private
 
   def touch_distributors

app/views/spree/order_mailer/_shipping.html.haml

@@ -1,4 +1,4 @@
-- if @order.shipping_method.andand.require_ship_address
+- if @order.shipping_method.andand.delivery?
   / Delivery details
   %p.callout
     %strong

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

@@ -29,7 +29,7 @@
         = order.bill_address.phone
 
   .columns.large-6
-    - if order.shipping_method.andand.require_ship_address
+    - if order.shipping_method.andand.delivery?
       // Delivery option
       .order-summary.text-small
         %strong= order.shipping_method.name

lib/open_food_network/last_used_address.rb

@@ -9,9 +9,9 @@ def last_used_bill_address
     end
 
     def last_used_ship_address
-      recent_orders.detect { |o|
-        o.ship_address && o.shipping_method.andand.require_ship_address
-       }.andand.ship_address
+      recent_orders.detect { |order|
+        order.ship_address && order.shipping_method.andand.delivery?
+      }.andand.ship_address
     end
 
 

lib/open_food_network/orders_and_fulfillments_report.rb

@@ -199,7 +199,7 @@ def columns
           proc { |line_items| "" },
           proc { |line_items| "shipping method" } ]
       when "order_cycle_customer_totals"
-        rsa = proc { |line_items| line_items.first.order.shipping_method.andand.require_ship_address }
+        rsa = proc { |line_items| line_items.first.order.shipping_method.andand.delivery? }
         [
           proc { |line_items| line_items.first.order.distributor.name },
           proc { |line_items| line_items.first.order.bill_address.firstname + " " + line_items.first.order.bill_address.lastname },

spec/controllers/checkout_controller_spec.rb

@@ -65,22 +65,30 @@
     end
 
     it "clears the ship address when re-rendering edit" do
-      controller.should_receive(:clear_ship_address).and_return true
       order.stub(:update_attributes).and_return false
+      expect(order).to receive(:clear_ship_address).and_return(true)
       spree_post :update, order: {}
     end
 
     it "clears the ship address when the order state cannot be advanced" do
-      controller.should_receive(:clear_ship_address).and_return true
       order.stub(:update_attributes).and_return true
       order.stub(:next).and_return false
+      expect(order).to receive(:clear_ship_address).and_return(true)
+
       spree_post :update, order: {}
     end
 
     it "only clears the ship address with a pickup shipping method" do
-      order.stub_chain(:shipping_method, :andand, :require_ship_address).and_return false
-      order.should_receive(:ship_address=)
-      controller.send(:clear_ship_address)
+      shipping_method = build(:shipping_method)
+
+      order.stub(:shipping_method).and_return(shipping_method)
+      shipping_method.stub(:instance_for).with(order).and_return(Pickup.new(order))
+      order.stub_chain(:shipping_method, :andand, :delivery?).and_return false
+
+      # We render the page populating the last one the user used
+      expect(order).to receive(:ship_address=).with(Spree::Address.default).twice
+
+      spree_post :update, order: {}
     end
   end
 

spec/lib/open_food_network/last_used_address_spec.rb

@@ -1,3 +1,4 @@
+require 'spec_helper'
 require 'open_food_network/last_used_address'
 
 module OpenFoodNetwork
@@ -35,11 +36,16 @@ module OpenFoodNetwork
       let(:order_without_ship_address) { double(:order, ship_address: nil) }
 
       it "returns the ship address when present" do
+        allow(delivery).to receive(:delivery?).and_return(true)
         lua.stub(:recent_orders) { [order_with_ship_address] }
         lua.last_used_ship_address.should == address
       end
 
       it "returns nil when the order doesn't require a ship address" do
+        allow(order_with_unrequired_ship_address.shipping_method)
+          .to receive(:delivery?)
+          .and_return(false)
+
         lua.stub(:recent_orders) { [order_with_unrequired_ship_address] }
         lua.last_used_ship_address.should be_nil
       end

spec/models/delivery_spec.rb

@@ -0,0 +1,10 @@
+require 'spec_helper'
+
+describe Delivery do
+  let(:order) { build(:order) }
+  let(:delivery) { described_class.new(order) }
+
+  describe '#ship_address_on_clear' do
+    it { expect(delivery.ship_address_on_clear).to eq(order.ship_address) }
+  end
+end

spec/models/pickup_spec.rb

@@ -0,0 +1,10 @@
+require 'spec_helper'
+
+describe Pickup do
+  let(:order) { build(:order) }
+  let(:pickup) { described_class.new(order) }
+
+  describe '#ship_address_on_clear' do
+    it { expect(pickup.ship_address_on_clear).to eq(Spree::Address.default) }
+  end
+end

spec/models/spree/order_spec.rb

@@ -197,7 +197,7 @@
   end
 
   describe "a paid order without a shipment" do
-    let(:order)           { create(:order) }
+    let(:order) { create(:order) }
 
     before do
       order.payment_state = 'paid'
@@ -496,37 +496,13 @@
   end
 
   describe "shipping address prepopulation" do
-    let(:distributor) { create(:distributor_enterprise) }
-    let(:order) { build(:order, distributor: distributor) }
-
-    before do
-      order.ship_address = distributor.address.clone
-      order.save # just to trigger our autopopulate the first time ;)
-    end
-
-    it "autopopulates the shipping address on save" do
-      order.should_receive(:shipping_address_from_distributor).and_return true
-      order.save
-    end
-
-    it "populates the shipping address if the shipping method doesn't require a delivery address" do
-      order.shipping_method = create(:shipping_method, require_ship_address: false)
-      order.ship_address.update_attribute :firstname, "will"
-      order.save
-      order.ship_address.firstname.should == distributor.address.firstname
-    end
-
-    it "does not populate the shipping address if the shipping method requires a delivery address" do
-      order.shipping_method = create(:shipping_method, require_ship_address: true)
-      order.ship_address.update_attribute :firstname, "will"
-      order.save
-      order.ship_address.firstname.should == "will"
-    end
+    let(:order) { build(:order) }
 
     it "doesn't attempt to create a shipment if the order is not yet valid" do
       order.shipping_method = create(:shipping_method, require_ship_address: false)
-      #Shipment.should_not_r
-      order.create_shipment!
+      allow(order.ship_address).to receive(:valid?).and_return(false)
+      order.save
+      expect(order.shipments).to be_empty
     end
   end
 
@@ -743,4 +719,36 @@
       end
     end
   end
+
+  describe '#clear_ship_address' do
+    let(:address) { build(:address) }
+    let(:order) { build(:order, ship_address: address, shipping_method: shipping_method) }
+
+    context 'when the shipping method is a delivery' do
+      let(:shipping_method) { build(:shipping_method, require_ship_address: true) }
+
+      it 'does not change the ship address' do
+        order.clear_ship_address
+        expect(order.ship_address).to eq(address)
+      end
+    end
+
+    context 'when the shipping method is a pickup' do
+      let(:shipping_method) { build(:shipping_method, require_ship_address: false) }
+
+      it 'sets the default ship address' do
+        order.clear_ship_address
+        expect(order.ship_address).to eq(Spree::Address.default)
+      end
+    end
+
+    context 'when there is no shipping method' do
+      let(:shipping_method) { nil }
+
+      it 'does not change the ship address' do
+        order.clear_ship_address
+        expect(order.ship_address).to eq(Spree::Address.default)
+      end
+    end
+  end
 end

spec/models/spree/shipping_method_spec.rb

@@ -89,5 +89,17 @@ module Spree
         ShippingMethod.services[d4.id].should be_nil
       end
     end
+
+    describe '#delivery?' do
+      context 'when the shipping method requires an address' do
+        let(:shipping_method) { build(:shipping_method, require_ship_address: true) }
+        it { expect(shipping_method.delivery?).to be_true }
+      end
+
+      context 'when the shipping method does not require address' do
+        let(:shipping_method) { build(:shipping_method, require_ship_address: false) }
+        it { expect(shipping_method.delivery?).to be_false }
+      end
+    end
   end
 end