From 78aa9da2cfd3c77b080b6035319aee05e76a13f5 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Sun, 17 Mar 2019 20:15:15 +0800 Subject: [PATCH] Uncomment and update OrderSyncer failing specs This still achieves a logic issue before the Spree upgrade, where switching from pick-up to delivery affects whether simultaneous changes to shipping address are ignored or not. This behaviour can be fixed in a separate PR. --- app/services/order_syncer.rb | 13 +++++++- spec/services/order_syncer_spec.rb | 52 +++++++++++++++++++----------- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/app/services/order_syncer.rb b/app/services/order_syncer.rb index cbe43d8bd2..cd1d7c998a 100644 --- a/app/services/order_syncer.rb +++ b/app/services/order_syncer.rb @@ -31,8 +31,19 @@ class OrderSyncer 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? + + # 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 (ship_address.changes.keys & relevant_address_attrs).any? + update_ship_address_for(order) unless pickup_to_delivery && order.shipment.blank? + end + unless pickup_to_delivery && order.shipment.present? + order.updater.__send__(:shipping_address_from_distributor) + end + update_payment_for(order) if payment_method_id_changed? end diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index 841658f1a8..bc74fefd49 100644 --- a/spec/services/order_syncer_spec.rb +++ b/spec/services/order_syncer_spec.rb @@ -156,7 +156,7 @@ xdescribe OrderSyncer do 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 @@ xdescribe OrderSyncer do 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 @@ xdescribe OrderSyncer do 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 @@ xdescribe OrderSyncer do 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 @@ xdescribe OrderSyncer do 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 @@ xdescribe OrderSyncer do 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