diff --git a/app/models/order_updater.rb b/app/models/order_updater.rb new file mode 100644 index 0000000000..c798da087b --- /dev/null +++ b/app/models/order_updater.rb @@ -0,0 +1,57 @@ +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 + + def before_save_hook + shipping_address_from_distributor + 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 + + # 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 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/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/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 diff --git a/spec/models/order_updater_spec.rb b/spec/models/order_updater_spec.rb new file mode 100644 index 0000000000..054154aadd --- /dev/null +++ b/spec/models/order_updater_spec.rb @@ -0,0 +1,152 @@ +require 'spec_helper' + +describe OrderUpdater do + 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 } + + order_updater.update_payment_state + expect(order.payment_state).to eq('failed') + end + + context "payment total is greater than order total" do + before do + order.payment_total = 2 + order.total = 1 + end + + it "is credit_owed" do + expect { + order_updater.update_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "order total is greater than payment total" do + before do + order.payment_total = 1 + order.total = 2 + end + + it "is credit_owed" do + expect { + order_updater.update_payment_state + }.to change { order.payment_state }.to 'balance_due' + end + end + + context "order total equals payment total" do + before do + order.payment_total = 30 + order.total = 30 + end + + it "is paid" do + expect { + order_updater.update_payment_state + }.to change { order.payment_state }.to 'paid' + end + end + + context "order is canceled" do + before { order.state = 'canceled' } + + context "and is still unpaid" do + before do + order.payment_total = 0 + order.total = 30 + end + + it "is void" do + expect { + order_updater.update_payment_state + }.to change { order.payment_state }.to 'void' + end + end + + context "and is paid" 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 { + order_updater.update_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "and payment is refunded" 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 { + order_updater.update_payment_state + }.to change { order.payment_state }.to 'void' + end + end + end + end + + context '#before_save_hook' do + let(:distributor) { build(:distributor_enterprise) } + 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_rates << shipping_rate + order.shipments << shipment + end + + 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 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') } + + 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) } diff --git a/spec/models/spree/order_updater_spec.rb b/spec/models/spree/order_updater_spec.rb deleted file mode 100644 index 50ab49a8c7..0000000000 --- a/spec/models/spree/order_updater_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -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 } - - it "is failed if no valid payments" do - order.stub_chain(:payments, :valid, :empty?).and_return(true) - - updater.update_payment_state - order.payment_state.should == 'failed' - end - - context "payment total is greater than order total" do - it "is credit_owed" do - order.payment_total = 2 - order.total = 1 - - expect { - 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 - order.payment_total = 1 - order.total = 2 - - expect { - 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 - order.payment_total = 30 - order.total = 30 - - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'paid' - end - end - - context "order is canceled" do - before do - order.state = 'canceled' - end - - context "and is still unpaid" do - it "is void" do - order.payment_total = 0 - order.total = 30 - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'void' - end - end - - context "and is paid" do - it "is credit_owed" 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) - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'credit_owed' - end - end - - context "and payment is refunded" do - it "is void" 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) - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'void' - end - end - end - end -end