From 6f12eee8ae49ce8dc81def9f20386841de012f72 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 25 Sep 2018 16:42:29 +1000
Subject: [PATCH] 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