Merge pull request #3653 from mkllnk/2520-spree2-order-subscription

2520 Remove `process_payments!` override

app/models/spree/order_decorator.rb

@@ -316,33 +316,22 @@ 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 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/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,

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

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/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 }

spec/models/spree/order_spec.rb

@@ -739,13 +739,22 @@ 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_and_distribution) }
+
+    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)
+
+        expect { order.next! }.to change { order.state }.from("delivery").to("payment")
       end
     end
 
@@ -756,8 +765,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 +774,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 +782,27 @@ 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)
+
+          expect { order.next! }.to change { order.state }.from("delivery").to("complete")
         end
       end
     end
+
+    def advance_to_delivery_state(order)
+      # advance to address state
+      order.ship_address = create(:address)
+      order.next!
+      expect(order.state).to eq "address"
+
+      # advance to delivery state
+      order.next!
+      expect(order.state).to eq "delivery"
+    end
   end
 
   describe '#restart_checkout!' do

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) }