Merge pull request #5780 from coopdevs/handle-credit-validation-errors

Handle credit validation errors

app/controllers/spree/admin/payments_controller.rb

@@ -61,7 +61,7 @@ def fire
         else
           flash[:error] = t(:cannot_perform_operation)
         end
-      rescue Spree::Core::GatewayError => e
+      rescue StandardError => e
         flash[:error] = e.message
       ensure
         redirect_to request.referer

app/models/spree/payment.rb

@@ -22,7 +22,7 @@ class Payment < ActiveRecord::Base
 
     has_one :adjustment, as: :source, dependent: :destroy
 
-    before_validation :validate_source
+    validate :validate_source
     before_save :set_unique_identifier
 
     after_save :create_payment_profile, if: :profiles_supported?
@@ -32,7 +32,15 @@ class Payment < ActiveRecord::Base
     # invalidate previously entered payments
     after_create :invalidate_old_payments
 
+    # Skips the validation of the source (for example, credit card) of the payment.
+    #
+    # This is used in refunds as the validation of the card can fail but the refund can go through,
+    #    we trust the payment gateway in these cases. For example, Stripe is accepting refunds with
+    #    source cards that were valid when the payment was placed but are now expired, and we
+    #    consider them invalid.
+    attr_accessor :skip_source_validation
     attr_accessor :source_attributes
+
     after_initialize :build_source
 
     scope :from_credit_card, -> { where(source_type: 'Spree::CreditCard') }
@@ -151,7 +159,7 @@ def revoke_adjustment_eligibility
     end
 
     def validate_source
-      if source && !source.valid?
+      if source && !skip_source_validation && !source.valid?
         source.errors.each do |field, error|
           field_name = I18n.t("activerecord.attributes.#{source.class.to_s.underscore}.#{field}")
           errors.add(Spree.t(source.class.to_s.demodulize.underscore), "#{field_name} #{error}")
@@ -176,8 +184,15 @@ def create_payment_profile
       gateway_error e
     end
 
+    # Makes newly entered payments invalidate previously entered payments so the most recent payment
+    # is used by the gateway.
     def invalidate_old_payments
-      order.payments.with_state('checkout').where("id != ?", id).each(&:invalidate!)
+      order.payments.with_state('checkout').where.not(id: id).each do |payment|
+        # Using update_column skips validations and so it skips validate_source. As we are just
+        # invalidating past payments here, we don't want to validate all of them at this stage.
+        payment.update_column(:state, 'invalid')
+        payment.ensure_correct_adjustment
+      end
     end
 
     def update_order

app/models/spree/payment/processing.rb

@@ -108,13 +108,14 @@ def credit!(credit_amount = nil)
           record_response(response)
 
           if response.success?
-            self.class.create(
+            self.class.create!(
               order: order,
               source: self,
               payment_method: payment_method,
               amount: credit_amount.abs * -1,
               response_code: response.authorization,
-              state: 'completed'
+              state: 'completed',
+              skip_source_validation: true
             )
           else
             gateway_error(response)
@@ -146,12 +147,15 @@ def refund!(refund_amount = nil)
           record_response(response)
 
           if response.success?
-            self.class.create(order: order,
-                              source: self,
-                              payment_method: payment_method,
-                              amount: refund_amount.abs * -1,
-                              response_code: response.authorization,
-                              state: 'completed')
+            self.class.create!(
+              order: order,
+              source: self,
+              payment_method: payment_method,
+              amount: refund_amount.abs * -1,
+              response_code: response.authorization,
+              state: 'completed',
+              skip_source_validation: true
+            )
           else
             gateway_error(response)
           end

config/locales/en.yml

@@ -35,6 +35,7 @@ en:
       spree/payment:
         amount: Amount
         state: State
+        source: Source
       spree/product:
         primary_taxon: "Product Category"
         supplier: "Supplier"

spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 describe Spree::Admin::PaymentsController, type: :controller do
@@ -10,7 +12,7 @@
     allow(controller).to receive(:spree_current_user) { user }
   end
 
-  context "#create" do
+  describe "#create" do
     let!(:payment_method) { create(:payment_method, distributors: [shop]) }
     let(:params) { { amount: order.total, payment_method_id: payment_method.id } }
 
@@ -138,4 +140,92 @@ def expect_redirect_to(path)
       end
     end
   end
+
+  describe '#fire' do
+    let(:payment_method) do
+      create(
+        :stripe_sca_payment_method,
+        distributor_ids: [create(:distributor_enterprise).id],
+        preferred_enterprise_id: create(:enterprise).id
+      )
+    end
+    let(:order) { create(:order, state: 'complete') }
+    let(:payment) do
+      create(:payment, order: order, payment_method: payment_method, amount: order.total)
+    end
+
+    let(:successful_response) { ActiveMerchant::Billing::Response.new(true, "Yay!") }
+
+    context 'on credit event' do
+      let(:params) { { e: 'credit', order_id: order.number, id: payment.id } }
+
+      before do
+        allow(request).to receive(:referer) { 'http://foo.com' }
+        allow(Spree::Payment).to receive(:find).with(payment.id.to_s) { payment }
+      end
+
+      it 'handles gateway errors' do
+        allow(payment.payment_method)
+          .to receive(:credit).and_raise(Spree::Core::GatewayError, 'error message')
+
+        spree_put :fire, params
+
+        expect(flash[:error]).to eq('error message')
+        expect(response).to redirect_to('http://foo.com')
+      end
+
+      it 'handles validation errors' do
+        allow(payment).to receive(:credit!).and_raise(StandardError, 'validation error')
+
+        spree_put :fire, params
+
+        expect(flash[:error]).to eq('validation error')
+        expect(response).to redirect_to('http://foo.com')
+      end
+
+      it 'displays a success message and redirects to the referer' do
+        allow(payment_method).to receive(:credit) { successful_response }
+
+        spree_put :fire, params
+
+        expect(flash[:success]).to eq(I18n.t(:payment_updated))
+      end
+    end
+
+    context 'on refund event' do
+      let(:params) { { e: 'refund', order_id: order.number, id: payment.id } }
+
+      before do
+        allow(request).to receive(:referer) { 'http://foo.com' }
+        allow(Spree::Payment).to receive(:find).with(payment.id.to_s) { payment }
+      end
+
+      it 'handles gateway errors' do
+        allow(payment.payment_method)
+          .to receive(:refund).and_raise(Spree::Core::GatewayError, 'error message')
+
+        spree_put :fire, params
+
+        expect(flash[:error]).to eq('error message')
+        expect(response).to redirect_to('http://foo.com')
+      end
+
+      it 'handles validation errors' do
+        allow(payment).to receive(:refund!).and_raise(StandardError, 'validation error')
+
+        spree_put :fire, params
+
+        expect(flash[:error]).to eq('validation error')
+        expect(response).to redirect_to('http://foo.com')
+      end
+
+      it 'displays a success message and redirects to the referer' do
+        allow(payment_method).to receive(:refund) { successful_response }
+
+        spree_put :fire, params
+
+        expect(flash[:success]).to eq(I18n.t(:payment_updated))
+      end
+    end
+  end
 end

spec/models/spree/payment_spec.rb

@@ -2,11 +2,7 @@
 
 describe Spree::Payment do
   context 'original specs from Spree' do
-    let(:order) do
-      order = Spree::Order.new(:bill_address => Spree::Address.new,
-                               :ship_address => Spree::Address.new)
-    end
-
+    let(:order) { create(:order) }
     let(:gateway) do
       gateway = Spree::Gateway::Bogus.new(:environment => 'test', :active => true)
       gateway.stub :source_required => true
@@ -339,7 +335,7 @@
 
       context "#credit" do
         before do
-          payment.state = 'complete'
+          payment.state = 'completed'
           payment.response_code = '123'
         end
 
@@ -391,20 +387,39 @@
 
         context "when response is successful" do
           it "should create an offsetting payment" do
-            Spree::Payment.should_receive(:create)
+            expect(Spree::Payment).to receive(:create!)
             payment.credit!
           end
 
           it "resulting payment should have correct values" do
-            payment.order.stub :outstanding_balance => 100
-            payment.stub :credit_allowed => 10
+            allow(payment.order).to receive(:outstanding_balance) { 100 }
+            allow(payment).to receive(:credit_allowed) { 10 }
 
             offsetting_payment = payment.credit!
             expect(offsetting_payment.amount.to_f).to eq(-10)
             expect(offsetting_payment).to be_completed
             expect(offsetting_payment.response_code).to eq('12345')
             expect(offsetting_payment.source).to eq(payment)
           end
+
+          context 'and the source payment card is expired' do
+            let(:card) do
+              Spree::CreditCard.new(month: 12, year: 1995, number: '4111111111111111')
+            end
+
+            let(:successful_response) do
+              ActiveMerchant::Billing::Response.new(true, "Yay!")
+            end
+
+            it 'lets the new payment to be saved' do
+              allow(payment.order).to receive(:outstanding_balance) { 100 }
+              allow(payment).to receive(:credit_allowed) { 10 }
+
+              offsetting_payment = payment.credit!
+
+              expect(offsetting_payment).to be_valid
+            end
+          end
         end
       end
     end
@@ -710,7 +725,7 @@
       end
     end
 
-    describe "refunding" do
+    describe "refund!" do
       let(:payment) { create(:payment) }
       let(:success) { double(success?: true, authorization: 'abc123') }
       let(:failure) { double(success?: false) }