Merge pull request openfoodfoundation#8592 from openfoodfoundation/ga…

…teway-redirects

Refactor payment gateway redirects handling

app/controllers/checkout_controller.rb

@@ -178,6 +178,7 @@ def checkout_workflow(shipping_method_id)
       if @order.state == "payment"
         return if redirect_to_payment_gateway
 
+        return action_failed if @order.errors.any?
         return action_failed unless @order.process_payments!
       end
 
@@ -190,14 +191,19 @@ def checkout_workflow(shipping_method_id)
   end
 
   def redirect_to_payment_gateway
-    redirect_path = Checkout::PaypalRedirect.new(params).path
-    redirect_path = Checkout::StripeRedirect.new(params, @order).path if redirect_path.blank?
-    return if redirect_path.blank?
+    return unless selected_payment_method.external_gateway?
+    return unless (redirect_url = selected_payment_method.external_payment_url(order: @order))
 
-    render json: { path: redirect_path }, status: :ok
+    render json: { path: redirect_url }, status: :ok
     true
   end
 
+  def selected_payment_method
+    @selected_payment_method ||= Spree::PaymentMethod.find(
+      params.dig(:order, :payments_attributes, 0, :payment_method_id)
+    )
+  end
+
   def order_error
     if @order.errors.present?
       @order.errors.full_messages.to_sentence

app/models/spree/gateway/pay_pal_express.rb

@@ -13,6 +13,15 @@ class PayPalExpress < Gateway
       preference :landing_page, :string, default: 'Billing'
       preference :logourl, :string, default: ''
 
+      def external_gateway?
+        true
+      end
+
+      def external_payment_url(_options)
+        Rails.application.routes.url_helpers.
+          payment_gateways_paypal_express_path(payment_method_id: id)
+      end
+
       def supports?(_source)
         true
       end

app/models/spree/gateway/stripe_sca.rb

@@ -20,6 +20,16 @@ class StripeSCA < Gateway
 
       validate :ensure_enterprise_selected
 
+      def external_gateway?
+        true
+      end
+
+      def external_payment_url(options)
+        return if options[:order].blank?
+
+        Checkout::StripeRedirect.new(self, options[:order]).path
+      end
+
       def method_type
         'stripe_sca'
       end

app/models/spree/payment_method.rb

@@ -61,6 +61,16 @@ def provider_class
       raise 'You must implement provider_class method for this gateway.'
     end
 
+    # Does the PaymentMethod require redirecting to an external gateway?
+    def external_gateway?
+      false
+    end
+
+    # Inheriting PaymentMethods can implement this method if needed
+    def external_payment_url(_options)
+      nil
+    end
+
     # The class that will process payments for this payment type, used for @payment.source
     # e.g. CreditCard in the case of a the Gateway payment type
     # nil means the payment method doesn't require a source e.g. check

app/services/checkout/stripe_redirect.rb

@@ -5,35 +5,41 @@ module Checkout
   class StripeRedirect
     include FullUrlHelper
 
-    def initialize(params, order)
-      @params = params
+    def initialize(payment_method, order)
+      @payment_method = payment_method
       @order = order
     end
 
-    # Returns the path to the authentication form if a redirect is needed
+    # Starts the payment process and returns the external URL if a redirect is needed
     def path
       return unless stripe_payment_method?
 
-      payment = OrderManagement::Order::StripeScaPaymentAuthorize.new(@order).
-        call!(full_checkout_path)
-      raise if @order.errors.any?
+      payment = payment_authorizer.call!(return_url)
 
-      field_with_url(payment)
+      return if order.errors.any?
+
+      stripe_payment_url(payment)
     end
 
     private
 
-    def stripe_payment_method?
-      return unless @params[:order][:payments_attributes]
+    attr_accessor :payment_method, :order
 
-      payment_method_id = @params[:order][:payments_attributes].first[:payment_method_id]
-      payment_method = Spree::PaymentMethod.find(payment_method_id)
+    def stripe_payment_method?
       payment_method.is_a?(Spree::Gateway::StripeSCA)
     end
 
+    def payment_authorizer
+      OrderManagement::Order::StripeScaPaymentAuthorize.new(order)
+    end
+
+    def return_url
+      full_checkout_path
+    end
+
     # Stripe::AuthorizeResponsePatcher patches the Stripe authorization response
     #   so that this field stores the redirect URL. It also verifies that it is a Stripe URL.
-    def field_with_url(payment)
+    def stripe_payment_url(payment)
       payment.cvv_response_message
     end
   end

spec/controllers/checkout_controller_spec.rb

@@ -418,28 +418,13 @@
       allow(order).to receive(:state) { "payment" }
     end
 
-    describe "paypal redirect" do
-      let(:payment_method) { create(:payment_method, type: "Spree::Gateway::PayPalExpress") }
-      let(:paypal_redirect) { instance_double(Checkout::PaypalRedirect) }
-
-      it "should call Paypal redirect and redirect if a path is provided" do
-        expect(Checkout::PaypalRedirect).to receive(:new).and_return(paypal_redirect)
-        expect(paypal_redirect).to receive(:path).and_return("test_path")
-
-        spree_post :update,
-                   order: { payments_attributes: [{ payment_method_id: payment_method.id }] }
-
-        expect(response.body).to eq({ path: "test_path" }.to_json)
-      end
-    end
-
-    describe "stripe redirect" do
-      let(:payment_method) { create(:payment_method, type: "Spree::Gateway::StripeSCA") }
-      let(:stripe_redirect) { instance_double(Checkout::StripeRedirect) }
+    describe "redirecting to an external payment gateway" do
+      let(:payment_method) { create(:payment_method) }
 
       it "should call Stripe redirect and redirect if a path is provided" do
-        expect(Checkout::StripeRedirect).to receive(:new).and_return(stripe_redirect)
-        expect(stripe_redirect).to receive(:path).and_return("test_path")
+        expect(Spree::PaymentMethod).to receive(:find).and_return(payment_method)
+        expect(payment_method).to receive(:external_gateway?).and_return(true)
+        expect(payment_method).to receive(:external_payment_url).and_return("test_path")
 
         spree_post :update,
                    order: { payments_attributes: [{ payment_method_id: payment_method.id }] }

spec/models/spree/gateway/stripe_sca_spec.rb

@@ -5,30 +5,30 @@
 describe Spree::Gateway::StripeSCA, type: :model do
   before { Stripe.api_key = "sk_test_12345" }
 
-  describe "#purchase" do
-    let(:order) { create(:order_with_totals_and_distribution) }
-    let(:credit_card) { create(:credit_card) }
-    let(:payment) {
-      create(
-        :payment,
-        state: "checkout",
-        order: order,
-        amount: order.total,
-        payment_method: subject,
-        source: credit_card,
-        response_code: "12345"
-      )
-    }
-    let(:gateway_options) {
-      { order_id: order.number }
-    }
-    let(:payment_authorised) {
-      payment_intent(payment.amount, "requires_capture")
-    }
-    let(:capture_successful) {
-      payment_intent(payment.amount, "succeeded")
-    }
+  let(:order) { create(:order_with_totals_and_distribution) }
+  let(:credit_card) { create(:credit_card) }
+  let(:payment) {
+    create(
+      :payment,
+      state: "checkout",
+      order: order,
+      amount: order.total,
+      payment_method: subject,
+      source: credit_card,
+      response_code: "12345"
+    )
+  }
+  let(:gateway_options) {
+    { order_id: order.number }
+  }
+  let(:payment_authorised) {
+    payment_intent(payment.amount, "requires_capture")
+  }
+  let(:capture_successful) {
+    payment_intent(payment.amount, "succeeded")
+  }
 
+  describe "#purchase" do
     it "captures the payment" do
       stub_request(:get, "https://api.stripe.com/v1/payment_intents/12345").
         to_return(status: 200, body: payment_authorised)
@@ -81,6 +81,23 @@
     end
   end
 
+  describe "#external_payment_url" do
+    let(:redirect_double) { instance_double(Checkout::StripeRedirect) }
+
+    it "returns nil when an order is not supplied" do
+      expect(subject.external_payment_url({})).to eq nil
+    end
+
+    it "calls Checkout::StripeRedirect" do
+      expect(Checkout::StripeRedirect).to receive(:new).with(subject, order) { redirect_double }
+      expect(redirect_double).to receive(:path).and_return("http://stripe-test.org")
+
+      expect(subject.external_payment_url(order: order)).to eq "http://stripe-test.org"
+    end
+  end
+
+  private
+
   def payment_intent(amount, status)
     JSON.generate(
       object: "payment_intent",

spec/services/checkout/stripe_redirect_spec.rb

@@ -5,53 +5,40 @@
 describe Checkout::StripeRedirect do
   describe '#path' do
     let(:order) { create(:order) }
-    let(:params) { { order: { order_id: order.id } } }
+    let(:service) { Checkout::StripeRedirect.new(payment_method, order) }
 
-    let(:redirect) { Checkout::StripeRedirect.new(params, order) }
+    context "when the given payment method is not Stripe SCA" do
+      let(:payment_method) { build(:payment_method) }
 
-    it "returns nil if payment_attributes are not provided" do
-      expect(redirect.path).to be nil
+      it "returns nil" do
+        expect(service.path).to be nil
+      end
     end
 
-    describe "when payment_attributes are provided" do
-      it "raises an error if payment method does not exist" do
-        params[:order][:payments_attributes] = [{ payment_method_id: "123" }]
+    context "when the payment method is a Stripe method" do
+      let(:payment_method) { create(:stripe_sca_payment_method) }
+      let(:stripe_payment) { create(:payment, payment_method_id: payment_method.id) }
+      let(:test_redirect_url) { "http://stripe_auth_url/" }
 
-        expect { redirect.path }.to raise_error ActiveRecord::RecordNotFound
+      before do
+        order.payments << stripe_payment
       end
 
-      describe "when payment method provided exists" do
-        before { params[:order][:payments_attributes] = [{ payment_method_id: payment_method.id }] }
+      it "authorizes the payment and returns the redirect path" do
+        expect(OrderPaymentFinder).to receive_message_chain(:new, :last_pending_payment).
+          and_return(stripe_payment)
 
-        describe "and the payment method is not a stripe payment method" do
-          let(:payment_method) { create(:payment_method) }
+        expect(OrderManagement::Order::StripeScaPaymentAuthorize).to receive(:new).and_call_original
 
-          it "returns nil" do
-            expect(redirect.path).to be nil
-          end
+        expect(stripe_payment).to receive(:authorize!) do
+          # Authorization moves the payment state from checkout/processing to pending
+          stripe_payment.state = 'pending'
+          true
         end
 
-        describe "and the payment method is a stripe method" do
-          let(:distributor) { create(:distributor_enterprise) }
-          let(:payment_method) { create(:stripe_sca_payment_method) }
-
-          it "returns the redirect path" do
-            stripe_payment = create(:payment, payment_method_id: payment_method.id)
-            order.payments << stripe_payment
-            allow(OrderPaymentFinder).to receive_message_chain(:new, :last_pending_payment).
-              and_return(stripe_payment)
-            allow(stripe_payment).to receive(:authorize!) do
-              # Authorization moves the payment state from checkout/processing to pending
-              stripe_payment.state = 'pending'
-              true
-            end
-            allow(stripe_payment.order).to receive(:distributor) { distributor }
-            test_redirect_url = "http://stripe_auth_url/"
-            allow(stripe_payment).to receive(:cvv_response_message).and_return(test_redirect_url)
-
-            expect(redirect.path).to eq test_redirect_url
-          end
-        end
+        expect(stripe_payment).to receive(:cvv_response_message).and_return(test_redirect_url)
+
+        expect(service.path).to eq test_redirect_url
       end
     end
   end