From 72f02ee1eeb797952cf369bf4f35b067e6e63ad1 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 8 Jun 2018 11:23:27 +0200 Subject: [PATCH 1/6] RSpec3-ize OrderUpdater spec --- spec/models/spree/order_updater_spec.rb | 58 +++++++++++++++---------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/spec/models/spree/order_updater_spec.rb b/spec/models/spree/order_updater_spec.rb index 50ab49a8c7..1fc13f11e3 100644 --- a/spec/models/spree/order_updater_spec.rb +++ b/spec/models/spree/order_updater_spec.rb @@ -1,88 +1,98 @@ require 'spec_helper' describe Spree::OrderUpdater do - # Copied pretty much verbatim from Spree 2.4. Remove this file once we get there, - # assuming the unchanged 2.4 logic still works for us. - # Only changes are stubs of :empty? instead of :size - context "updating payment state" do - let(:order) { Spree::Order.new } - let(:updater) { order.updater } + context "#updating_payment_state" do + let(:order) { build(:order) } + let(:order_updater) { described_class.new(order) } it "is failed if no valid payments" do - order.stub_chain(:payments, :valid, :empty?).and_return(true) + allow(order).to receive_message_chain(:payments, :valid, :empty?) { true } - updater.update_payment_state - order.payment_state.should == 'failed' + order_updater.update_payment_state + expect(order.payment_state).to eq('failed') end context "payment total is greater than order total" do - it "is credit_owed" do + before do order.payment_total = 2 order.total = 1 + end + it "is credit_owed" do expect { - updater.update_payment_state + order_updater.update_payment_state }.to change { order.payment_state }.to 'credit_owed' end end context "order total is greater than payment total" do - it "is credit_owed" do + before do order.payment_total = 1 order.total = 2 + end + it "is credit_owed" do expect { - updater.update_payment_state + order_updater.update_payment_state }.to change { order.payment_state }.to 'balance_due' end end context "order total equals payment total" do - it "is paid" do + before do order.payment_total = 30 order.total = 30 + end + it "is paid" do expect { - updater.update_payment_state + order_updater.update_payment_state }.to change { order.payment_state }.to 'paid' end end context "order is canceled" do - before do - order.state = 'canceled' - end + before { order.state = 'canceled' } context "and is still unpaid" do - it "is void" do + before do order.payment_total = 0 order.total = 30 + end + + it "is void" do expect { - updater.update_payment_state + order_updater.update_payment_state }.to change { order.payment_state }.to 'void' end end context "and is paid" do - it "is credit_owed" do + before do order.payment_total = 30 order.total = 30 order.stub_chain(:payments, :valid, :empty?).and_return(false) order.stub_chain(:payments, :completed, :empty?).and_return(false) + end + + it "is credit_owed" do expect { - updater.update_payment_state + order_updater.update_payment_state }.to change { order.payment_state }.to 'credit_owed' end end context "and payment is refunded" do - it "is void" do + before do order.payment_total = 0 order.total = 30 order.stub_chain(:payments, :valid, :empty?).and_return(false) order.stub_chain(:payments, :completed, :empty?).and_return(false) + end + + it "is void" do expect { - updater.update_payment_state + order_updater.update_payment_state }.to change { order.payment_state }.to 'void' end end From 21d905a0927ac0117639d2a3a4e85a0459c319a4 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 8 Jun 2018 11:31:57 +0200 Subject: [PATCH 2/6] Refactor OrderUpdater to decorate Spree's one This replaces the nasty class eval and provides some encapsulation --- app/models/order_updater.rb | 41 +++++++++++++++++++ app/models/spree/order_updater_decorator.rb | 41 ------------------- spec/models/{spree => }/order_updater_spec.rb | 4 +- 3 files changed, 43 insertions(+), 43 deletions(-) create mode 100644 app/models/order_updater.rb delete mode 100644 app/models/spree/order_updater_decorator.rb rename spec/models/{spree => }/order_updater_spec.rb (96%) diff --git a/app/models/order_updater.rb b/app/models/order_updater.rb new file mode 100644 index 0000000000..9b41145ea3 --- /dev/null +++ b/app/models/order_updater.rb @@ -0,0 +1,41 @@ +require 'delegate' + +class OrderUpdater < SimpleDelegator + # TODO: This logic adapted from Spree 2.4, remove when we get there + # Handles state updating in a much more logical way than < 2.4 + # Specifically, doesn't depend on payments.last to determine payment state + # Also swapped: == 0 for .zero?, .size == 0 for empty? and .size > 0 for !empty? + # See: + # https://github.com/spree/spree/commit/38b8456183d11fc1e00e395e7c9154c76ef65b85 + # https://github.com/spree/spree/commit/7b264acff7824f5b3dc6651c106631d8f30b147a + def update_payment_state + last_state = order.payment_state + if payments.present? && payments.valid.empty? + order.payment_state = 'failed' + elsif order.state == 'canceled' && order.payment_total.zero? + order.payment_state = 'void' + else + # This part added so that we don't need to override order.outstanding_balance + balance = order.outstanding_balance + balance = -1 * order.payment_total if canceled_and_paid_for? + order.payment_state = 'balance_due' if balance > 0 + order.payment_state = 'credit_owed' if balance < 0 + order.payment_state = 'paid' if balance.zero? + + # Original logic + # order.payment_state = 'balance_due' if order.outstanding_balance > 0 + # order.payment_state = 'credit_owed' if order.outstanding_balance < 0 + # order.payment_state = 'paid' if !order.outstanding_balance? + end + order.state_changed('payment') if last_state != order.payment_state + order.payment_state + end + + private + + # Taken from order.outstanding_balance in Spree 2.4 + # See: https://github.com/spree/spree/commit/7b264acff7824f5b3dc6651c106631d8f30b147a + def canceled_and_paid_for? + order.canceled? && order.payments.present? && !order.payments.completed.empty? + end +end diff --git a/app/models/spree/order_updater_decorator.rb b/app/models/spree/order_updater_decorator.rb deleted file mode 100644 index ab1d4eaf26..0000000000 --- a/app/models/spree/order_updater_decorator.rb +++ /dev/null @@ -1,41 +0,0 @@ -module Spree - OrderUpdater.class_eval do - # TODO: This logic adapted from Spree 2.4, remove when we get there - # Handles state updating in a much more logical way than < 2.4 - # Specifically, doesn't depend on payments.last to determine payment state - # Also swapped: == 0 for .zero?, .size == 0 for empty? and .size > 0 for !empty? - # See: - # https://github.com/spree/spree/commit/38b8456183d11fc1e00e395e7c9154c76ef65b85 - # https://github.com/spree/spree/commit/7b264acff7824f5b3dc6651c106631d8f30b147a - def update_payment_state - last_state = order.payment_state - if payments.present? && payments.valid.empty? - order.payment_state = 'failed' - elsif order.state == 'canceled' && order.payment_total.zero? - order.payment_state = 'void' - else - # This part added so that we don't need to override order.outstanding_balance - balance = order.outstanding_balance - balance = -1 * order.payment_total if canceled_and_paid_for? - order.payment_state = 'balance_due' if balance > 0 - order.payment_state = 'credit_owed' if balance < 0 - order.payment_state = 'paid' if balance.zero? - - # Original logic - # order.payment_state = 'balance_due' if order.outstanding_balance > 0 - # order.payment_state = 'credit_owed' if order.outstanding_balance < 0 - # order.payment_state = 'paid' if !order.outstanding_balance? - end - order.state_changed('payment') if last_state != order.payment_state - order.payment_state - end - - private - - # Taken from order.outstanding_balance in Spree 2.4 - # See: https://github.com/spree/spree/commit/7b264acff7824f5b3dc6651c106631d8f30b147a - def canceled_and_paid_for? - order.canceled? && order.payments.present? && !order.payments.completed.empty? - end - end -end diff --git a/spec/models/spree/order_updater_spec.rb b/spec/models/order_updater_spec.rb similarity index 96% rename from spec/models/spree/order_updater_spec.rb rename to spec/models/order_updater_spec.rb index 1fc13f11e3..217a5dd491 100644 --- a/spec/models/spree/order_updater_spec.rb +++ b/spec/models/order_updater_spec.rb @@ -1,9 +1,9 @@ require 'spec_helper' -describe Spree::OrderUpdater do +describe OrderUpdater do context "#updating_payment_state" do let(:order) { build(:order) } - let(:order_updater) { described_class.new(order) } + let(:order_updater) { described_class.new(Spree::OrderUpdater.new(order)) } it "is failed if no valid payments" do allow(order).to receive_message_chain(:payments, :valid, :empty?) { true } From f1896313b3306757fa0c291349f2de108be69547 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 8 Jun 2018 12:32:38 +0200 Subject: [PATCH 3/6] Move shipping_address_from_distributor to OrderUpdater This is still Spree-1.3 compatible not considering multiple shipments and shipping methods. --- app/models/order_updater.rb | 11 +++++++++ app/models/spree/order_decorator.rb | 13 ---------- spec/models/order_updater_spec.rb | 38 ++++++++++++++++++++++++++--- spec/models/spree/order_spec.rb | 35 -------------------------- 4 files changed, 46 insertions(+), 51 deletions(-) diff --git a/app/models/order_updater.rb b/app/models/order_updater.rb index 9b41145ea3..f27103385e 100644 --- a/app/models/order_updater.rb +++ b/app/models/order_updater.rb @@ -31,6 +31,10 @@ class OrderUpdater < SimpleDelegator order.payment_state end + def before_save_hook + shipping_address_from_distributor + end + private # Taken from order.outstanding_balance in Spree 2.4 @@ -38,4 +42,11 @@ class OrderUpdater < SimpleDelegator def canceled_and_paid_for? order.canceled? && order.payments.present? && !order.payments.completed.empty? end + + def shipping_address_from_distributor + shipping_method = order.shipments.first.shipping_methods.first + return if shipping_method.require_ship_address + + order.ship_address = order.distributor.address + end end diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index eed4cefd0b..e7e208acd1 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -20,7 +20,6 @@ Spree::Order.class_eval do validate :disallow_guest_order attr_accessible :order_cycle_id, :distributor_id, :customer_id - before_validation :shipping_address_from_distributor before_validation :associate_customer, unless: :customer_id? before_validation :ensure_customer, unless: :customer_is_valid? @@ -337,18 +336,6 @@ Spree::Order.class_eval do 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 = address_from_distributor - end - end - end - def address_from_distributor address = distributor.address.clone if bill_address diff --git a/spec/models/order_updater_spec.rb b/spec/models/order_updater_spec.rb index 217a5dd491..bb1e01ea5c 100644 --- a/spec/models/order_updater_spec.rb +++ b/spec/models/order_updater_spec.rb @@ -1,10 +1,10 @@ require 'spec_helper' describe OrderUpdater do - context "#updating_payment_state" do - let(:order) { build(:order) } - let(:order_updater) { described_class.new(Spree::OrderUpdater.new(order)) } + let(:order) { build(:order) } + let(:order_updater) { described_class.new(Spree::OrderUpdater.new(order)) } + context "#updating_payment_state" do it "is failed if no valid payments" do allow(order).to receive_message_chain(:payments, :valid, :empty?) { true } @@ -98,4 +98,36 @@ describe OrderUpdater do end end end + + context '#before_save_hook' do + let(:distributor) { build(:distributor_enterprise) } + let(:shipment) { build(:shipment) } + let(:order) { build(:order, distributor: distributor) } + + before do + shipment.shipping_methods << shipping_method + order.shipments << shipment + end + + context 'when the shipping method doesn\'t require a delivery address' do + let(:shipping_method) { build(:base_shipping_method, require_ship_address: false) } + + it "populates the shipping address" do + order_updater.before_save_hook + expect(order.ship_address.firstname).to eq(distributor.address.firstname) + end + end + + context 'when the shipping method requires a delivery address' do + let(:shipping_method) { build(:base_shipping_method, require_ship_address: true) } + let(:address) { build(:address, firstname: 'will') } + + before { order.ship_address = address } + + it "does not populate the shipping address" do + order_updater.before_save_hook + expect(order.ship_address.firstname).to eq("will") + end + end + end end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 345cbb661f..4e23a102ef 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -509,41 +509,6 @@ describe Spree::Order do end 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 - - 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! - end - end - describe "checking if an order is an account invoice" do let(:accounts_distributor) { create(:distributor_enterprise) } let(:order_account_invoice) { create(:order, distributor: accounts_distributor) } From d1e65691cfe4c06349667984a4e6358df01be024 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 8 Jun 2018 12:57:17 +0200 Subject: [PATCH 4/6] Copy distributor addr. when using a pick up method This copies the distributor's address when at least one of the order shipments uses a shipping method that requires shipping address. --- app/models/order_updater.rb | 8 +++++--- spec/models/order_updater_spec.rb | 27 +++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/models/order_updater.rb b/app/models/order_updater.rb index f27103385e..5534e8d2a4 100644 --- a/app/models/order_updater.rb +++ b/app/models/order_updater.rb @@ -44,9 +44,11 @@ class OrderUpdater < SimpleDelegator end def shipping_address_from_distributor - shipping_method = order.shipments.first.shipping_methods.first - return if shipping_method.require_ship_address + shipments.each do |shipment| + shipping_method = shipment.shipping_method + next if shipping_method.require_ship_address - order.ship_address = order.distributor.address + order.ship_address = order.distributor.address + end end end diff --git a/spec/models/order_updater_spec.rb b/spec/models/order_updater_spec.rb index bb1e01ea5c..054154aadd 100644 --- a/spec/models/order_updater_spec.rb +++ b/spec/models/order_updater_spec.rb @@ -101,24 +101,43 @@ describe OrderUpdater do context '#before_save_hook' do let(:distributor) { build(:distributor_enterprise) } - let(:shipment) { build(:shipment) } let(:order) { build(:order, distributor: distributor) } + let(:shipment) { build(:shipment) } + let(:shipping_rate) do + Spree::ShippingRate.new( + shipping_method: shipping_method, + selected: true + ) + end before do - shipment.shipping_methods << shipping_method + shipment.shipping_rates << shipping_rate order.shipments << shipment end - context 'when the shipping method doesn\'t require a delivery address' do + context 'when any of the shipping methods doesn\'t require a delivery address' do let(:shipping_method) { build(:base_shipping_method, require_ship_address: false) } + let(:delivery_shipment) { build(:shipment) } + let(:delivery_shipping_rate) do + Spree::ShippingRate.new( + shipping_method: build(:base_shipping_method, require_ship_address: true), + selected: true + ) + end + + before do + delivery_shipment.shipping_rates << delivery_shipping_rate + order.shipments << delivery_shipment + end + it "populates the shipping address" do order_updater.before_save_hook expect(order.ship_address.firstname).to eq(distributor.address.firstname) end end - context 'when the shipping method requires a delivery address' do + context 'when any of the shipping methods requires a delivery address' do let(:shipping_method) { build(:base_shipping_method, require_ship_address: true) } let(:address) { build(:address, firstname: 'will') } From a0d2ec5666a97198ccfb3534e0d33d29f8ea6142 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 25 Jul 2018 13:51:01 +0200 Subject: [PATCH 5/6] Enable OFN's custom OrderUpdater --- config/initializers/spree.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index 77243c8ebb..83e854a08f 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -22,6 +22,7 @@ Spree.config do |config| #config.override_actionmailer_config = false config.package_factory = Stock::Package + config.order_updater_decorator = OrderUpdater end # TODO Work out why this is necessary From eaf108718c8b946646ffb0f0b974b95d4a3db563 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 25 Jul 2018 13:52:05 +0200 Subject: [PATCH 6/6] Add doc to method --- app/models/order_updater.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/order_updater.rb b/app/models/order_updater.rb index 5534e8d2a4..c798da087b 100644 --- a/app/models/order_updater.rb +++ b/app/models/order_updater.rb @@ -43,6 +43,9 @@ class OrderUpdater < SimpleDelegator order.canceled? && order.payments.present? && !order.payments.completed.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