diff --git a/app/models/order_updater.rb b/app/models/order_updater.rb index a38270f988..95d2b96cfb 100644 --- a/app/models/order_updater.rb +++ b/app/models/order_updater.rb @@ -21,6 +21,15 @@ class OrderUpdater < SimpleDelegator 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 @@ class OrderUpdater < SimpleDelegator 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 diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 4c3ee93276..f3aadce330 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -325,12 +325,6 @@ Spree::Order.class_eval do 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 @@ -341,6 +335,12 @@ Spree::Order.class_eval do 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 diff --git a/app/services/order_syncer.rb b/app/services/order_syncer.rb index cbe43d8bd2..b3e37c0ea4 100644 --- a/app/services/order_syncer.rb +++ b/app/services/order_syncer.rb @@ -31,8 +31,8 @@ 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? + update_ship_address_for(order) update_payment_for(order) if payment_method_id_changed? end @@ -48,11 +48,6 @@ class OrderSyncer 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 @@ class OrderSyncer 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 @@ class OrderSyncer # 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 diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb index 9b9ff1f4f7..72a0ed6899 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -82,7 +82,7 @@ describe SubscriptionPlacementJob do 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 @@ describe SubscriptionPlacementJob do 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 @@ describe SubscriptionPlacementJob do 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,7 +162,7 @@ describe SubscriptionPlacementJob do 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) @@ -170,7 +170,7 @@ describe SubscriptionPlacementJob do 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 @@ -178,7 +178,7 @@ describe SubscriptionPlacementJob do 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) diff --git a/spec/models/order_updater_spec.rb b/spec/models/order_updater_spec.rb index 9eec3a1232..40a12a9119 100644 --- a/spec/models/order_updater_spec.rb +++ b/spec/models/order_updater_spec.rb @@ -131,7 +131,7 @@ describe OrderUpdater do 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 diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb index 841658f1a8..0b37490fb2 100644 --- a/spec/services/order_syncer_spec.rb +++ b/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 @@ 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