diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index f12cdb341f..076c00e0f6 100644 --- a/app/models/spree/order_decorator.rb +++ b/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 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/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/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 03036f17b7..54d181916c 100644 --- a/spec/models/spree/order_spec.rb +++ b/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 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) }