From 0f0a7041471450379f0fa72c5d64148b5683d77e Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 17 Jul 2020 14:07:05 +0200 Subject: [PATCH] Skip source validation when applying credit The original payment may not be valid because its credit card may be expired. Stripe gives this as a valid scenario returning a success and we should do too. When creating the credit payment we end up validating all sources in a chain as follows. ``` Payment being persisted -> source payment -> original credit card. ``` The source payment was valid when created (It would not be persisted otherwise) but its source card may now be expired, and that's legit. There was also an issue with the `#invalidate_old_payments` callback. It was causing the original payment to be validated again and thus the credit payment failed to be persisted due to the original credit card being expired. Switching this callback to use `#update_column` skips validations and so we don't validate the source payment. We only care about the state there, so it should be fine. --- app/models/spree/payment.rb | 20 +++++++++++++++++-- app/models/spree/payment/processing.rb | 3 ++- .../payments/payments_controller_spec.rb | 10 ++++++++++ spec/models/spree/payment_spec.rb | 19 ++++++++++++++++++ 4 files changed, 49 insertions(+), 3 deletions(-) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 8126ad851c..f940db551f 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -32,7 +32,19 @@ module Spree # invalidate previously entered payments after_create :invalidate_old_payments + # Prevents #validate_source to end up in a chain of validations, from + # payment to payment up until the original credit card record. + # + # All payments are validated before saving thus, we get nothing by checking + # they are valid in the future. Quite the opposite. Stripe is accepting + # refunds whose source are cards that were valid when the payment was + # placed but are now expired, and we consider them invalid. + # + # Skipping the source validation when applying a refund avoids this + # situation. We trust the payment gateway. + attr_accessor :skip_source_validation attr_accessor :source_attributes + after_initialize :build_source scope :from_credit_card, -> { where(source_type: 'Spree::CreditCard') } @@ -151,7 +163,7 @@ module Spree 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 +188,12 @@ module Spree gateway_error e end + # Inherited from Spree, 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("id != ?", id).each do |payment| + payment.update_column(:state, 'invalid') + end end def update_order diff --git a/app/models/spree/payment/processing.rb b/app/models/spree/payment/processing.rb index 2d01a91aea..ec7d154f66 100644 --- a/app/models/spree/payment/processing.rb +++ b/app/models/spree/payment/processing.rb @@ -114,7 +114,8 @@ module Spree 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) diff --git a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb index 4f6a36f592..27ea7dac50 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -157,6 +157,8 @@ describe Spree::Admin::PaymentsController, type: :controller do let(:params) { { e: 'credit', order_id: order.number, id: payment.id } } + let(:successful_response) { ActiveMerchant::Billing::Response.new(true, "Yay!") } + before do allow(request).to receive(:referer) { 'http://foo.com' } allow(Spree::Payment).to receive(:find).with(payment.id.to_s) { payment } @@ -180,6 +182,14 @@ describe Spree::Admin::PaymentsController, type: :controller do 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 end end diff --git a/spec/models/spree/payment_spec.rb b/spec/models/spree/payment_spec.rb index 43a0a05127..144122631b 100644 --- a/spec/models/spree/payment_spec.rb +++ b/spec/models/spree/payment_spec.rb @@ -401,6 +401,25 @@ describe Spree::Payment do 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