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

@@ -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

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
 

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