From 6f12eee8ae49ce8dc81def9f20386841de012f72 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 25 Sep 2018 16:42:29 +1000 Subject: [PATCH 1/3] Reduce the override of Order for subscriptions Orders belonging to subscriptions get completed without payment. That requires overriding Spree's functionality. In Spree 2, an order in payment state without pending orders is invalid. Instead we skip the payment state by not requiring a payment for automatically generated orders until the order cycle is closed. --- app/models/spree/order_decorator.rb | 17 +++++--- spec/jobs/subscription_confirm_job_spec.rb | 1 + spec/models/spree/order_spec.rb | 49 ++++++++++++++++++---- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index f12cdb341f..7deccb9607 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -334,15 +334,22 @@ Spree::Order.class_eval do errors.add(:base, e.message) and return result end - # Override or Spree method. Used to prevent payments on subscriptions from being processed in the normal way. - # ie. they are 'hidden' from processing logic until after the order cycle has closed. - def pending_payments - return [] if subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now - payments.select {|p| p.state == "checkout"} # Original definition + # Override Spree method to allow unpaid orders to be completed. + # Subscriptions place orders at the beginning of an order cycle. They need to + # be completed to draw from stock levels and trigger emails. + # Spree doesn't allow this. Other options would be to introduce an additional + # order state or implement a special proxy payment method. + # https://github.com/openfoodfoundation/openfoodnetwork/pull/3012#issuecomment-438146484 + 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 diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb index 024a8bb0ec..3c433bf94d 100644 --- a/spec/jobs/subscription_confirm_job_spec.rb +++ b/spec/jobs/subscription_confirm_job_spec.rb @@ -141,6 +141,7 @@ describe SubscriptionConfirmJob do before do allow(order).to receive(:payment_total) { 0 } allow(order).to receive(:total) { 10 } + allow(order).to receive(:payment_required?) { true } allow(order).to receive(:pending_payments) { [payment] } end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index ce3a4acea1..e84c225569 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -739,13 +739,24 @@ describe Spree::Order do end end - describe "finding pending_payments" do - let!(:order) { create(:order ) } - let!(:payment) { create(:payment, order: order, state: 'checkout') } + describe "payments" do + let(:payment_method) { create(:payment_method) } + let(:shipping_method) { create(:shipping_method) } + let(:order) { create(:order_with_totals) } + + before { order.update_totals } context "when the order is not a subscription" do - it "returns the payments on the order" do - expect(order.reload.pending_payments).to eq [payment] + it "it requires a payment" do + expect(order.payment_required?).to be true + end + + it "advances to payment state" do + advance_to_delivery_state(order) + + order.next! + + expect(order.state).to eq "payment" end end @@ -756,8 +767,8 @@ describe Spree::Order do context "and order_cycle has no order_close_at set" do before { order.order_cycle.update_attributes(orders_close_at: nil) } - it "returns the payments on the order" do - expect(order.reload.pending_payments).to eq [payment] + it "requires a payment" do + expect(order.payment_required?).to be true end end @@ -765,7 +776,7 @@ describe Spree::Order do before { order.order_cycle.update_attributes(orders_close_at: 5.minutes.ago) } it "returns the payments on the order" do - expect(order.reload.pending_payments).to eq [payment] + expect(order.payment_required?).to be true end end @@ -773,10 +784,30 @@ describe Spree::Order do before { order.order_cycle.update_attributes(orders_close_at: 5.minutes.from_now) } it "returns an empty array" do - expect(order.reload.pending_payments).to eq [] + expect(order.payment_required?).to be false + end + + it "skips the payment state" do + advance_to_delivery_state(order) + + order.next! + + expect(order.state).to eq "complete" end end end + + def advance_to_delivery_state(order) + # advance to address state + create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) + order.reload + order.ship_address = create(:address) + order.next! + + # advance to delivery state + create(:payment, order: order, payment_method: payment_method) + order.next! + end end describe '#restart_checkout!' do From 8b9a81413186a3698223d3c1316ea709092a0588 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 25 Sep 2018 11:59:14 +1000 Subject: [PATCH 2/3] Remove obsolete Spree 2 backport on Order --- app/models/spree/order_decorator.rb | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 7deccb9607..076c00e0f6 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -316,24 +316,6 @@ Spree::Order.class_eval do complete? && distributor.andand.allow_order_changes? && order_cycle.andand.open? end - # Override of existing Spree method. Can remove when we reach 2-0-stable - # See commit: https://github.com/spree/spree/commit/5fca58f658273451193d5711081d018c317814ed - # Allows GatewayError to show useful error messages in checkout - def process_payments! - pending_payments.each do |payment| - break if payment_total >= total - - payment.process! - - if payment.completed? - self.payment_total += payment.amount - end - end - rescue Spree::Core::GatewayError => e # This section changed - result = !!Spree::Config[:allow_checkout_on_gateway_error] - errors.add(:base, e.message) and return result - end - # Override Spree method to allow unpaid orders to be completed. # Subscriptions place orders at the beginning of an order cycle. They need to # be completed to draw from stock levels and trigger emails. From 278190a25ca98c479f9f070f47ea5383825b0295 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 10 Apr 2019 14:39:47 +1000 Subject: [PATCH 3/3] Update specs for Spree v2 payment requirement This pull request removed the override of `process_payments!` which was based on v1. Spree v2 has an additional check: An order in payment state requires a payment. Some specs didn't care and didn't create payments before transitioning to `complete`. --- .../orders/customer_details_controller_spec.rb | 1 + .../spree/admin/orders_controller_spec.rb | 2 +- spec/models/proxy_order_spec.rb | 2 +- spec/models/spree/order_spec.rb | 15 +++++---------- spec/services/advance_order_service_spec.rb | 3 ++- 5 files changed, 10 insertions(+), 13 deletions(-) diff --git a/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb b/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb index a8b193458d..14176aa0e7 100644 --- a/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb @@ -14,6 +14,7 @@ describe Spree::Admin::Orders::CustomerDetailsController, type: :controller do :order_with_totals_and_distribution, state: 'cart', shipments: [shipment], + payments: [create(:payment)], distributor: distributor, user: nil, email: nil, diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index aa1bddba97..59db9fc05f 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -12,7 +12,7 @@ describe Spree::Admin::OrdersController, type: :controller do it "advances the order state" do expect { spree_get :edit, id: order - }.to change { order.reload.state }.from("cart").to("complete") + }.to change { order.reload.state }.from("cart").to("payment") end describe "view" do diff --git a/spec/models/proxy_order_spec.rb b/spec/models/proxy_order_spec.rb index 2e15c39cc6..3bbf428afc 100644 --- a/spec/models/proxy_order_spec.rb +++ b/spec/models/proxy_order_spec.rb @@ -76,10 +76,10 @@ describe ProxyOrder, type: :model do end describe "resume" do - let!(:payment_method) { create(:payment_method) } let!(:shipment) { create(:shipment) } let(:order) { create(:order_with_totals, ship_address: create(:address), shipments: [shipment], + payments: [create(:payment)], distributor: shipment.shipping_method.distributors.first) } let(:proxy_order) { create(:proxy_order, order: order, canceled_at: Time.zone.now) } let(:order_cycle) { proxy_order.order_cycle } diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index e84c225569..e47b99c2a4 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -742,7 +742,7 @@ describe Spree::Order do describe "payments" do let(:payment_method) { create(:payment_method) } let(:shipping_method) { create(:shipping_method) } - let(:order) { create(:order_with_totals) } + let(:order) { create(:order_with_totals_and_distribution) } before { order.update_totals } @@ -754,9 +754,7 @@ describe Spree::Order do it "advances to payment state" do advance_to_delivery_state(order) - order.next! - - expect(order.state).to eq "payment" + expect { order.next! }.to change { order.state }.from("delivery").to("payment") end end @@ -790,23 +788,20 @@ describe Spree::Order do it "skips the payment state" do advance_to_delivery_state(order) - order.next! - - expect(order.state).to eq "complete" + expect { order.next! }.to change { order.state }.from("delivery").to("complete") end end end def advance_to_delivery_state(order) # advance to address state - create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) - order.reload order.ship_address = create(:address) order.next! + expect(order.state).to eq "address" # advance to delivery state - create(:payment, order: order, payment_method: payment_method) order.next! + expect(order.state).to eq "delivery" end end diff --git a/spec/services/advance_order_service_spec.rb b/spec/services/advance_order_service_spec.rb index 60bcec75df..36d2eed834 100644 --- a/spec/services/advance_order_service_spec.rb +++ b/spec/services/advance_order_service_spec.rb @@ -5,7 +5,8 @@ describe AdvanceOrderService do let!(:order) do create(:order_with_totals_and_distribution, distributor: distributor, bill_address: create(:address), - ship_address: create(:address)) + ship_address: create(:address), + payments: [create(:payment)]) end let(:service) { described_class.new(order) }