2788 [Subscriptions] Fix address issues when syncing subscriptions and orders

app/models/order_updater.rb

@@ -21,6 +21,15 @@ def before_save_hook
     shipping_address_from_distributor
   end
 
+  # Sets the distributor's address as shipping address of the order for those
+  # shipments using a shipping method that doesn't require address, such us
+  # a pickup.
+  def shipping_address_from_distributor
+    return if order.shipping_method.blank? || order.shipping_method.require_ship_address
+
+    order.ship_address = order.address_from_distributor
+  end
+
   private
 
   def infer_payment_state
@@ -79,16 +88,4 @@ def paid?
   def failed_payments?
     payments.present? && payments.valid.empty?
   end
-
-  # Sets the distributor's address as shipping address of the order for those
-  # shipments using a shipping method that doesn't require address, such us
-  # a pickup.
-  def shipping_address_from_distributor
-    shipments.each do |shipment|
-      shipping_method = shipment.shipping_method
-      next if shipping_method.require_ship_address
-
-      order.ship_address = order.distributor.address
-    end
-  end
 end

app/models/spree/order_decorator.rb

@@ -326,12 +326,6 @@ def payment_required?
     total.to_f > 0.0 && !skip_payment_for_subscription?
   end
 
-  private
-
-  def skip_payment_for_subscription?
-    subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
-  end
-
   def address_from_distributor
     address = distributor.address.clone
     if bill_address
@@ -342,6 +336,12 @@ def address_from_distributor
     address
   end
 
+  private
+
+  def skip_payment_for_subscription?
+    subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
+  end
+
   def provided_by_order_cycle?(line_item)
     order_cycle_variants = order_cycle.andand.variants || []
     order_cycle_variants.include? line_item.variant

app/services/order_syncer.rb

@@ -31,8 +31,8 @@ def sync!
 
   def update_associations_for(order)
     update_bill_address_for(order) if (bill_address.changes.keys & relevant_address_attrs).any?
-    update_ship_address_for(order) if (ship_address.changes.keys & relevant_address_attrs).any?
     update_shipment_for(order) if shipping_method_id_changed?
+    update_ship_address_for(order)
     update_payment_for(order) if payment_method_id_changed?
   end
 
@@ -48,11 +48,6 @@ def update_bill_address_for(order)
     order.bill_address.update_attributes(bill_address.attributes.slice(*relevant_address_attrs))
   end
 
-  def update_ship_address_for(order)
-    return unless ship_address_updatable?(order)
-    order.ship_address.update_attributes(ship_address.attributes.slice(*relevant_address_attrs))
-  end
-
   def update_payment_for(order)
     payment = order.payments.with_state('checkout').where(payment_method_id: payment_method_id_was).last
     if payment
@@ -75,6 +70,19 @@ def update_shipment_for(order)
     end
   end
 
+  def update_ship_address_for(order)
+    # The conditions here are to achieve the same behaviour in earlier versions of Spree, where
+    # switching from pick-up to delivery affects whether simultaneous changes to shipping address
+    # are ignored or not.
+    pickup_to_delivery = force_ship_address_required?(order)
+    if !pickup_to_delivery || order.shipment.present?
+      save_ship_address_in_order(order) if (ship_address.changes.keys & relevant_address_attrs).any?
+    end
+    if !pickup_to_delivery || order.shipment.blank?
+      order.updater.shipping_address_from_distributor
+    end
+  end
+
   def relevant_address_attrs
     ["firstname", "lastname", "address1", "zipcode", "city", "state_id", "country_id", "phone"]
   end
@@ -99,12 +107,17 @@ def ship_address_updatable?(order)
   # address on the order matches the shop's address
   def force_ship_address_required?(order)
     return false unless shipping_method.require_ship_address?
-    distributor_address = order.__send__(:address_from_distributor)
+    distributor_address = order.address_from_distributor
     relevant_address_attrs.all? do |attr|
       order.ship_address[attr] == distributor_address[attr]
     end
   end
 
+  def save_ship_address_in_order(order)
+    return unless ship_address_updatable?(order)
+    order.ship_address.update_attributes(ship_address.attributes.slice(*relevant_address_attrs))
+  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

spec/jobs/subscription_placement_job_spec.rb

@@ -82,7 +82,7 @@
           variant3.update_attribute(:on_hand, 0)
         end
 
-        xit "caps quantity at the stock level for stock-limited items, and reports the change" do
+        it "caps quantity at the stock level for stock-limited items, and reports the change" do
           changes = job.send(:cap_quantity_and_store_changes, order.reload)
           expect(line_item1.reload.quantity).to be 3 # not capped
           expect(line_item2.reload.quantity).to be 2 # capped
@@ -104,7 +104,7 @@
           variant3.update_attribute(:on_hand, 0)
         end
 
-        xit "sets quantity to 0 for unavailable items, and reports the change" do
+        it "sets quantity to 0 for unavailable items, and reports the change" do
           changes = job.send(:cap_quantity_and_store_changes, order.reload)
           expect(line_item1.reload.quantity).to be 0 # unavailable
           expect(line_item2.reload.quantity).to be 2 # capped
@@ -151,7 +151,7 @@
           allow(job).to receive(:unavailable_stock_lines_for) { order.line_items }
         end
 
-        xit "does not place the order, clears, all adjustments, and sends an empty_order email" do
+        it "does not place the order, clears, all adjustments, and sends an empty_order email" do
           expect{ job.send(:process, order) }.to_not change{ order.reload.completed_at }.from(nil)
           expect(order.adjustments).to be_empty
           expect(order.total).to eq 0
@@ -162,23 +162,23 @@
       end
 
       context "when at least one stock item is available after capping stock" do
-        xit "processes the order to completion, but does not process the payment" do
+        it "processes the order to completion, but does not process the payment" do
           # If this spec starts complaining about no shipping methods being available
           # on CI, there is probably another spec resetting the currency though Rails.cache.clear
           expect{ job.send(:process, order) }.to change{ order.reload.completed_at }.from(nil)
           expect(order.completed_at).to be_within(5.seconds).of Time.zone.now
           expect(order.payments.first.state).to eq "checkout"
         end
 
-        xit "does not enqueue confirmation emails" do
+        it "does not enqueue confirmation emails" do
           expect{ job.send(:process, order) }.to_not enqueue_job ConfirmOrderJob
           expect(job).to have_received(:send_placement_email).with(order, anything).once
         end
 
         context "when progression of the order fails" do
           before { allow(order).to receive(:next) { false } }
 
-          xit "records an error and does not attempt to send an email" do
+          it "records an error and does not attempt to send an email" do
             expect(job).to_not receive(:send_placement_email)
             expect(job).to receive(:record_and_log_error).once
             job.send(:process, order)

spec/models/order_updater_spec.rb

@@ -131,7 +131,7 @@
 
       it "populates the shipping address from distributor" do
         order_updater.before_save_hook
-        expect(order.ship_address.firstname).to eq(distributor.address.firstname)
+        expect(order.ship_address.address1).to eq(distributor.address.address1)
       end
     end
 

spec/services/order_syncer_spec.rb

@@ -1,6 +1,6 @@
 require "spec_helper"
 
-xdescribe OrderSyncer do
+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! }
@@ -156,7 +156,7 @@
       end
 
       context "when the bill_address on the order matches that on the subscription" do
-        xit "updates all bill_address attrs and ship_address names + phone" do
+        it "updates all bill_address attrs and ship_address names + phone" do
           subscription.assign_attributes(params)
           expect(syncer.sync!).to be true
           expect(syncer.order_update_issues.keys).to_not include order.id
@@ -173,8 +173,12 @@
       end
 
       context "when the bill_address on the order doesn't match that on the subscription" do
-        before { order.bill_address.update_attributes(firstname: "Jane") }
-        xit "does not update bill_address or ship_address on the order" do
+        before do
+          order.bill_address.update_attributes!(firstname: "Jane")
+          order.update!
+        end
+
+        it "does not update bill_address or 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
@@ -214,7 +218,10 @@
       end
 
       context "when the bill_address on the order doesn't match that on the subscription" do
-        before { order.bill_address.update_attributes(firstname: "Jane") }
+        before do
+          order.bill_address.update_attributes!(firstname: "Jane")
+          order.update!
+        end
 
         it "does not update bill_address or ship_address on the order" do
           subscription.assign_attributes(params)
@@ -265,7 +272,7 @@
       end
 
       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) }
+        let(:new_shipping_method) { create(:shipping_method, distributors: [distributor], require_ship_address: true) }
 
         before { params.merge!(shipping_method_id: new_shipping_method.id) }
 
@@ -284,22 +291,27 @@
                                   with_proxy_orders: true)
           end
 
-          before do
-            subscription.assign_attributes(params)
-          end
+          context "when there is no pending shipment using the former shipping method" do
+            before do
+              order.shipment.destroy
+              subscription.assign_attributes(params)
+            end
 
-          it "updates ship_address attrs" do
-            expect(syncer.sync!).to be true
-            expect(syncer.order_update_issues.keys).to include order.id
-            order.reload
-            expect(order.ship_address.firstname).to eq ship_address_attrs["firstname"]
-            expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"]
-            expect(order.ship_address.address1).to eq ship_address_attrs["address1"]
-            expect(order.ship_address.phone).to eq ship_address_attrs["phone"]
+            it "updates ship_address attrs" do
+              expect(syncer.sync!).to be true
+              expect(syncer.order_update_issues.keys).to include order.id
+              order.reload
+              expect(order.ship_address.firstname).to eq ship_address_attrs["firstname"]
+              expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"]
+              expect(order.ship_address.address1).to eq ship_address_attrs["address1"]
+              expect(order.ship_address.phone).to eq ship_address_attrs["phone"]
+            end
           end
 
           context "when the order has a pending shipment using the former shipping method" do
-            let!(:shipment) { create(:shipment, order: order, shipping_method: shipping_method) }
+            before do
+              subscription.assign_attributes(params)
+            end
 
             it "updates ship_address attrs" do
               expect(syncer.sync!).to be true
@@ -334,7 +346,11 @@
       end
 
       context "when the ship address on the order doesn't match that on the subscription" do
-        before { order.ship_address.update_attributes(firstname: "Jane") }
+        before do
+          order.ship_address.update_attributes(firstname: "Jane")
+          order.update!
+        end
+
         it "does not update ship_address on the order" do
           subscription.assign_attributes(params)
           expect(syncer.sync!).to be true