From 1289c3f1a252090911082c0b7b337ed9519f6a0c Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 13 Jan 2020 19:27:16 +0000 Subject: [PATCH 01/23] Fix rubocop issues in credit_cards_controller --- app/controllers/spree/credit_cards_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/spree/credit_cards_controller.rb b/app/controllers/spree/credit_cards_controller.rb index d2d7939d0f..81f3828b88 100644 --- a/app/controllers/spree/credit_cards_controller.rb +++ b/app/controllers/spree/credit_cards_controller.rb @@ -59,6 +59,7 @@ module Spree # It destroys the whole customer object def destroy_at_stripe stripe_customer = Stripe::Customer.retrieve(@credit_card.gateway_customer_profile_id, {}) + stripe_customer.delete if stripe_customer end From 873dcc373f4fa9652c28d11e26c03e27a2611ff6 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 6 Feb 2020 15:06:50 +0000 Subject: [PATCH 02/23] Small refactoring to make next commit easier --- app/controllers/checkout_controller.rb | 41 ++++++++++++++++++-------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 3e00127d8a..309ff0701a 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -165,7 +165,7 @@ class CheckoutController < Spree::StoreController return update_failed end - update_result + update_response end def redirect_to_payment_gateway @@ -193,27 +193,37 @@ class CheckoutController < Spree::StoreController end end - def update_result - if @order.state == "complete" || @order.completed? - save_order_addresses_as_user_default - ResetOrderService.new(self, current_order).call - - update_succeeded + def update_response + if order_complete? + checkout_succeeded + update_succeeded_response else - update_failed + checkout_failed + update_failed_response end end + def order_complete? + @order.state == "complete" || @order.completed? + end + + def checkout_succeeded + save_order_addresses_as_user_default + ResetOrderService.new(self, current_order).call + + session[:access_token] = current_order.token + flash[:notice] = t(:order_processed_successfully) + end + def save_order_addresses_as_user_default + return unless params[:order] + user_default_address_setter = UserDefaultAddressSetter.new(@order, spree_current_user) user_default_address_setter.set_default_bill_address if params[:order][:default_bill_address] user_default_address_setter.set_default_ship_address if params[:order][:default_ship_address] end - def update_succeeded - session[:access_token] = current_order.token - flash[:notice] = t(:order_processed_successfully) - + def update_succeeded_response respond_to do |format| format.html do respond_with(@order, location: order_path(@order)) @@ -225,9 +235,16 @@ class CheckoutController < Spree::StoreController end def update_failed + checkout_failed + update_failed_response + end + + def checkout_failed current_order.updater.shipping_address_from_distributor RestartCheckout.new(@order).call + end + def update_failed_response respond_to do |format| format.html do render :edit From d91578ab803cc00ba4a997fd65508ace29877ab5 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 6 Feb 2020 16:01:08 +0000 Subject: [PATCH 03/23] Rename checkout payment redirect to checkout paypal redirect --- app/controllers/checkout_controller.rb | 2 +- .../checkout/{payment_redirect.rb => paypal_redirect.rb} | 2 +- .../{payment_redirect_spec.rb => paypal_redirect_spec.rb} | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) rename app/services/checkout/{payment_redirect.rb => paypal_redirect.rb} (96%) rename spec/services/checkout/{payment_redirect_spec.rb => paypal_redirect_spec.rb} (93%) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 309ff0701a..52c3ba9543 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -169,7 +169,7 @@ class CheckoutController < Spree::StoreController end def redirect_to_payment_gateway - redirect_path = Checkout::PaymentRedirect.new(params).path + redirect_path = Checkout::PaypalRedirect.new(params).path return if redirect_path.blank? render json: { path: redirect_path }, status: :ok diff --git a/app/services/checkout/payment_redirect.rb b/app/services/checkout/paypal_redirect.rb similarity index 96% rename from app/services/checkout/payment_redirect.rb rename to app/services/checkout/paypal_redirect.rb index 1e74a9daa7..ca7a34172f 100644 --- a/app/services/checkout/payment_redirect.rb +++ b/app/services/checkout/paypal_redirect.rb @@ -2,7 +2,7 @@ # Provides the redirect path if a redirect to the payment gateway is needed module Checkout - class PaymentRedirect + class PaypalRedirect def initialize(params) @params = params end diff --git a/spec/services/checkout/payment_redirect_spec.rb b/spec/services/checkout/paypal_redirect_spec.rb similarity index 93% rename from spec/services/checkout/payment_redirect_spec.rb rename to spec/services/checkout/paypal_redirect_spec.rb index 05e5bdb879..40545f2588 100644 --- a/spec/services/checkout/payment_redirect_spec.rb +++ b/spec/services/checkout/paypal_redirect_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' -describe Checkout::PaymentRedirect do +describe Checkout::PaypalRedirect do describe '#order_params' do let(:params) { { order: { order_id: "123" } } } - let(:redirect) { Checkout::PaymentRedirect.new(params) } + let(:redirect) { Checkout::PaypalRedirect.new(params) } it "returns nil if payment_attributes are not provided" do expect(redirect.path).to be nil From e3ffe8fe6b4d8c0507e36c852cc1a36bb3d2aa6a Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 6 Feb 2020 19:44:38 +0000 Subject: [PATCH 04/23] Add Checkout Stripe redirect logic to get users redirected to stripe authentication pages provided by the stripe API --- app/controllers/checkout_controller.rb | 1 + app/services/checkout/stripe_redirect.rb | 40 ++++++++++++++ spec/controllers/checkout_controller_spec.rb | 2 +- .../services/checkout/paypal_redirect_spec.rb | 2 +- .../services/checkout/stripe_redirect_spec.rb | 55 +++++++++++++++++++ 5 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 app/services/checkout/stripe_redirect.rb create mode 100644 spec/services/checkout/stripe_redirect_spec.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 52c3ba9543..90bc4357d2 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -170,6 +170,7 @@ class CheckoutController < Spree::StoreController 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? render json: { path: redirect_path }, status: :ok diff --git a/app/services/checkout/stripe_redirect.rb b/app/services/checkout/stripe_redirect.rb new file mode 100644 index 0000000000..0fe6b90f09 --- /dev/null +++ b/app/services/checkout/stripe_redirect.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +# Provides the redirect path if a redirect to the payment gateway is needed +module Checkout + class StripeRedirect + def initialize(params, order) + @params = params + @order = order + end + + # Returns the path to the authentication form if a redirect is needed + def path + return unless stripe_payment_method? + + payment = @order.pending_payments.last + return unless payment&.checkout? + + payment.authorize! + raise unless payment.pending? + + payment.cvv_response_message if url?(payment.cvv_response_message) + end + + private + + def stripe_payment_method? + return unless @params[:order][:payments_attributes] + + payment_method_id = @params[:order][:payments_attributes].first[:payment_method_id] + payment_method = Spree::PaymentMethod.find(payment_method_id) + payment_method.is_a?(Spree::Gateway::StripeSCA) + end + + def url?(string) + return false if string.blank? + + string.starts_with?("http") + end + end +end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index a2ac420a2e..8f9b429d2e 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -251,7 +251,7 @@ describe CheckoutController, type: :controller do end it "should check the payment method for Paypalness if we've selected one" do - expect(Spree::PaymentMethod).to receive(:find).with(payment_method.id.to_s) { payment_method } + expect(Spree::PaymentMethod).to receive(:find).twice.with(payment_method.id.to_s) { payment_method } allow(order).to receive(:update_attributes) { true } allow(order).to receive(:state) { "payment" } spree_post :update, order: { payments_attributes: [{ payment_method_id: payment_method.id }] } diff --git a/spec/services/checkout/paypal_redirect_spec.rb b/spec/services/checkout/paypal_redirect_spec.rb index 40545f2588..e028be4486 100644 --- a/spec/services/checkout/paypal_redirect_spec.rb +++ b/spec/services/checkout/paypal_redirect_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Checkout::PaypalRedirect do - describe '#order_params' do + describe '#path' do let(:params) { { order: { order_id: "123" } } } let(:redirect) { Checkout::PaypalRedirect.new(params) } diff --git a/spec/services/checkout/stripe_redirect_spec.rb b/spec/services/checkout/stripe_redirect_spec.rb new file mode 100644 index 0000000000..d85152ecbf --- /dev/null +++ b/spec/services/checkout/stripe_redirect_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Checkout::StripeRedirect do + describe '#path' do + let(:order) { create(:order) } + let(:params) { { order: { order_id: order.id } } } + + let(:redirect) { Checkout::StripeRedirect.new(params, order) } + + it "returns nil if payment_attributes are not provided" do + expect(redirect.path).to be nil + 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" }] + + expect { redirect.path }.to raise_error ActiveRecord::RecordNotFound + end + + describe "when payment method provided exists" do + before { params[:order][:payments_attributes] = [{ payment_method_id: payment_method.id }] } + + describe "and the payment method is not a stripe payment method" do + let(:payment_method) { create(:payment_method) } + + it "returns nil" do + expect(redirect.path).to be nil + end + 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(stripe_payment).to receive(:authorize!) do + # Authorization moves the payment state from checkout/processing to pending + stripe_payment.state = 'pending' + true + end + 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 + end + end + end +end From 4b3b4e00ff28a54035adb00923bc11886dee48ac Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 11:26:08 +0000 Subject: [PATCH 05/23] Add authorize action to the stripe_sca gateway and make it fetch the redirect url if provided Change the purchase action to only capture the alrteady authorized payment intent --- app/models/spree/gateway/stripe_sca.rb | 71 ++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index d5ac331b89..dcc585ff5a 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -32,9 +32,41 @@ module Spree # NOTE: the name of this method is determined by Spree::Payment::Processing def purchase(money, creditcard, gateway_options) - provider.purchase(*options_for_purchase_or_auth(money, creditcard, gateway_options)) + payment_intent_id = payment_intent(creditcard, gateway_options) + unless payment_intent_id + return failed_activemerchant_billing_response("No Pending Payment Found") + end + + payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, + stripe_account: stripe_account_id) + if payment_intent_response.last_payment_error.present? + return failed_activemerchant_billing_response(payment_intent_response. + last_payment_error. + message) + end + if payment_intent_response.status != 'requires_capture' + return failed_activemerchant_billing_response("Invalid PaymentIntent status.") + end + + options = basic_options(gateway_options) + options[:customer] = creditcard.gateway_customer_profile_id + provider.capture(money, payment_intent_id, options) + rescue Stripe::StripeError => e + failed_activemerchant_billing_response(e.message) + end + + # NOTE: the name of this method is determined by Spree::Payment::Processing + def authorize(money, creditcard, gateway_options) + authorize_response = provider.authorize(*options_for_authorize(money, + creditcard, + gateway_options)) + + if url = url_for_authorization(authorize_response) + authorize_response.cvv_result['message'] = url if authorize_response.cvv_result.present? + end + + authorize_response rescue Stripe::StripeError => e - # This will be an error caused by generating a stripe token failed_activemerchant_billing_response(e.message) end @@ -65,11 +97,17 @@ module Spree options.merge(login: Stripe.api_key) end - def options_for_purchase_or_auth(money, creditcard, gateway_options) + def basic_options(gateway_options) options = {} options[:description] = "Spree Order ID: #{gateway_options[:order_id]}" options[:currency] = gateway_options[:currency] options[:stripe_account] = stripe_account_id + options + end + + def options_for_authorize(money, creditcard, gateway_options) + options = basic_options(gateway_options) + options[:return_url] = full_checkout_path customer_id, payment_method_id = Stripe::CreditCardCloner.new.clone(creditcard, stripe_account_id) @@ -77,6 +115,23 @@ module Spree [money, payment_method_id, options] end + def url_for_authorization(response) + next_action = response.params["next_source_action"] + return unless response.params["status"] == "requires_source_action" && + next_action.present? && + next_action["type"] == "authorize_with_url" + + next_action["authorize_with_url"]["url"] + end + + def payment_intent(creditcard, gateway_options) + order_number = gateway_options[:order_id].split('-').first + payment = Spree::Order.find_by_number(order_number).payments.merge(creditcard.payments).last + return unless payment + + payment.response_code + end + def failed_activemerchant_billing_response(error_message) ActiveMerchant::Billing::Response.new(false, error_message) end @@ -86,6 +141,16 @@ module Spree errors.add(:stripe_account_owner, I18n.t(:error_required)) end + + def full_checkout_path + URI.join(url_helpers.root_url, url_helpers.checkout_path).to_s + end + + def url_helpers + # This is how we can get the helpers with a usable root_url outside the controllers + Rails.application.routes.default_url_options = ActionMailer::Base.default_url_options + Rails.application.routes.url_helpers + end end end end From 19042e0d37e07e158af5ef2b032cee8f6bb43f96 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 11:30:03 +0000 Subject: [PATCH 06/23] Make checkout controller able to receive a redirect from stripe with a payment_intent as parameter --- app/controllers/checkout_controller.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 90bc4357d2..f593e8896d 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -35,6 +35,16 @@ class CheckoutController < Spree::StoreController rescue_from Spree::Core::GatewayError, with: :rescue_from_spree_gateway_error def edit + if valid_payment_intent_provided? + if advance_order_state(@order) && order_complete? + checkout_succeeded + redirect_to(order_path(@order)) && return + else + flash[:error] = order_workflow_error + current_order.updater.shipping_address_from_distributor + end + end + # This is only required because of spree_paypal_express. If we implement # a version of paypal that uses this controller, and more specifically # the #update_failed method, then we can remove this call @@ -151,6 +161,13 @@ class CheckoutController < Spree::StoreController end end + def valid_payment_intent_provided? + params["payment_intent"]&.starts_with?("pi_") && + @order.state == "payment" && + @order.payments.last.state == "pending" && + @order.payments.last.response_code == params["payment_intent"] + end + def checkout_workflow(shipping_method_id) while @order.state != "complete" if @order.state == "payment" From c0bf09131f83e78ec1674de4b06017addd4c4e4e Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 15:47:25 +0000 Subject: [PATCH 07/23] Make order.pending_payments include payments in pending state so that these payments (pending is the state after authorization for credit cards) are also processed as part of the normal order workflow --- app/models/spree/order_decorator.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 28d487d6fd..1cde659f65 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -115,6 +115,12 @@ Spree::Order.class_eval do end end + # "Checkout" is the initial state and, for card payments, "pending" is the state after authorization + # These are both valid states to process the payment + def pending_payments + (payments.select(&:pending?) + payments.select(&:checkout?)).uniq + end + def remove_variant(variant) line_items(:reload) current_item = find_line_item_by_variant(variant) From 08e729673fe5a9538dad0fcbc65dfa86388966f5 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 15:59:44 +0000 Subject: [PATCH 08/23] Move stripe sca gateway error messages to translatable keys --- app/models/spree/gateway/stripe_sca.rb | 4 ++-- config/locales/en.yml | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index dcc585ff5a..65ee527317 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -34,7 +34,7 @@ module Spree def purchase(money, creditcard, gateway_options) payment_intent_id = payment_intent(creditcard, gateway_options) unless payment_intent_id - return failed_activemerchant_billing_response("No Pending Payment Found") + return failed_activemerchant_billing_response(I18n.t(:no_pending_payments)) end payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, @@ -45,7 +45,7 @@ module Spree message) end if payment_intent_response.status != 'requires_capture' - return failed_activemerchant_billing_response("Invalid PaymentIntent status.") + return failed_activemerchant_billing_response(I18n.t(:invalid_payment_state)) end options = basic_options(gateway_options) diff --git a/config/locales/en.yml b/config/locales/en.yml index 25af1defa2..fb59f0f086 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -275,7 +275,9 @@ en: none: None notes: Notes error: Error - processing_payment: Processing payment... + processing_payment: "Processing payment..." + no_pending_payments: "No pending payments" + invalid_payment_state: "Invalid Payment state" filter_results: Filter Results quantity: Quantity pick_up: Pick up From e2cdb01a28295ebf7f47f6027a63fabca3eb7cf3 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 16:14:33 +0000 Subject: [PATCH 09/23] Improve readability of stripe sca gateway code --- app/models/spree/gateway/stripe_sca.rb | 36 ++++++++++++++------------ 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index 65ee527317..d40a8d1b8f 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -32,20 +32,10 @@ module Spree # NOTE: the name of this method is determined by Spree::Payment::Processing def purchase(money, creditcard, gateway_options) - payment_intent_id = payment_intent(creditcard, gateway_options) - unless payment_intent_id - return failed_activemerchant_billing_response(I18n.t(:no_pending_payments)) - end - - payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, - stripe_account: stripe_account_id) - if payment_intent_response.last_payment_error.present? - return failed_activemerchant_billing_response(payment_intent_response. - last_payment_error. - message) - end - if payment_intent_response.status != 'requires_capture' - return failed_activemerchant_billing_response(I18n.t(:invalid_payment_state)) + begin + payment_intent_id = payment_intent(creditcard, gateway_options) + rescue Stripe::StripeError => e + return failed_activemerchant_billing_response(e.message) end options = basic_options(gateway_options) @@ -127,9 +117,23 @@ module Spree def payment_intent(creditcard, gateway_options) order_number = gateway_options[:order_id].split('-').first payment = Spree::Order.find_by_number(order_number).payments.merge(creditcard.payments).last - return unless payment + raise Stripe::StripeError, I18n.t(:no_pending_payments) unless payment&.response_code - payment.response_code + validate_payment_intent(payment.response_code) + end + + def validate_payment_intent(payment_intent_id) + payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, + stripe_account: stripe_account_id) + if payment_intent_response.last_payment_error.present? + raise Stripe::StripeError, payment_intent_response.last_payment_error.message + end + + if payment_intent_response.status != 'requires_capture' + raise Stripe::StripeError, I18n.t(:invalid_payment_state) + end + + payment_intent_id end def failed_activemerchant_billing_response(error_message) From 3fcf2865163cf388db0395ca4e6387942a4a0542 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 17:04:47 +0000 Subject: [PATCH 10/23] Extract PostCheckoutActions from checkout controller --- app/controllers/checkout_controller.rb | 37 +++++++------------ .../checkout/post_checkout_actions.rb | 30 +++++++++++++++ 2 files changed, 44 insertions(+), 23 deletions(-) create mode 100644 app/services/checkout/post_checkout_actions.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index f593e8896d..65d308bc0d 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -35,15 +35,7 @@ class CheckoutController < Spree::StoreController rescue_from Spree::Core::GatewayError, with: :rescue_from_spree_gateway_error def edit - if valid_payment_intent_provided? - if advance_order_state(@order) && order_complete? - checkout_succeeded - redirect_to(order_path(@order)) && return - else - flash[:error] = order_workflow_error - current_order.updater.shipping_address_from_distributor - end - end + return handle_redirect_from_stripe if valid_payment_intent_provided? # This is only required because of spree_paypal_express. If we implement # a version of paypal that uses this controller, and more specifically @@ -168,6 +160,16 @@ class CheckoutController < Spree::StoreController @order.payments.last.response_code == params["payment_intent"] end + def handle_redirect_from_stripe + if advance_order_state(@order) && order_complete? + checkout_succeeded + redirect_to(order_path(@order)) && return + else + flash[:error] = order_workflow_error + checkout_failed + end + end + def checkout_workflow(shipping_method_id) while @order.state != "complete" if @order.state == "payment" @@ -216,8 +218,7 @@ class CheckoutController < Spree::StoreController checkout_succeeded update_succeeded_response else - checkout_failed - update_failed_response + update_failed end end @@ -226,21 +227,12 @@ class CheckoutController < Spree::StoreController end def checkout_succeeded - save_order_addresses_as_user_default - ResetOrderService.new(self, current_order).call + Checkout::PostCheckoutActions.new(@order).success(self, params, spree_current_user) session[:access_token] = current_order.token flash[:notice] = t(:order_processed_successfully) end - def save_order_addresses_as_user_default - return unless params[:order] - - user_default_address_setter = UserDefaultAddressSetter.new(@order, spree_current_user) - user_default_address_setter.set_default_bill_address if params[:order][:default_bill_address] - user_default_address_setter.set_default_ship_address if params[:order][:default_ship_address] - end - def update_succeeded_response respond_to do |format| format.html do @@ -258,8 +250,7 @@ class CheckoutController < Spree::StoreController end def checkout_failed - current_order.updater.shipping_address_from_distributor - RestartCheckout.new(@order).call + Checkout::PostCheckoutActions.new(@order).failure end def update_failed_response diff --git a/app/services/checkout/post_checkout_actions.rb b/app/services/checkout/post_checkout_actions.rb new file mode 100644 index 0000000000..63e32e9185 --- /dev/null +++ b/app/services/checkout/post_checkout_actions.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# Executes actions after checkout +module Checkout + class PostCheckoutActions + def initialize(order) + @order = order + end + + def success(controller, params, current_user) + save_order_addresses_as_user_default(params, current_user) + ResetOrderService.new(controller, @order).call + end + + def failure + @order.updater.shipping_address_from_distributor + RestartCheckout.new(@order).call + end + + private + + def save_order_addresses_as_user_default(params, current_user) + return unless params[:order] + + user_default_address_setter = UserDefaultAddressSetter.new(@order, current_user) + user_default_address_setter.set_default_bill_address if params[:order][:default_bill_address] + user_default_address_setter.set_default_ship_address if params[:order][:default_ship_address] + end + end +end From 95c1b7f7a6df446ad63b4380613d2e746ed76648 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 17:54:37 +0000 Subject: [PATCH 11/23] Extract PaymentIntentValidator from StripeSCA gateway --- app/models/spree/gateway/stripe_sca.rb | 16 +--------------- lib/stripe/payment_intent_validator.rb | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 15 deletions(-) create mode 100644 lib/stripe/payment_intent_validator.rb diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index d40a8d1b8f..ba8469ede9 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -119,21 +119,7 @@ module Spree payment = Spree::Order.find_by_number(order_number).payments.merge(creditcard.payments).last raise Stripe::StripeError, I18n.t(:no_pending_payments) unless payment&.response_code - validate_payment_intent(payment.response_code) - end - - def validate_payment_intent(payment_intent_id) - payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, - stripe_account: stripe_account_id) - if payment_intent_response.last_payment_error.present? - raise Stripe::StripeError, payment_intent_response.last_payment_error.message - end - - if payment_intent_response.status != 'requires_capture' - raise Stripe::StripeError, I18n.t(:invalid_payment_state) - end - - payment_intent_id + Stripe::PaymentIntentValidator.new.call(payment.response_code, stripe_account_id) end def failed_activemerchant_billing_response(error_message) diff --git a/lib/stripe/payment_intent_validator.rb b/lib/stripe/payment_intent_validator.rb new file mode 100644 index 0000000000..77e532284d --- /dev/null +++ b/lib/stripe/payment_intent_validator.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# This class validates if a given payment intent ID is valid in Stripe +module Stripe + class PaymentIntentValidator + def call(payment_intent_id, stripe_account_id) + payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, + stripe_account: stripe_account_id) + if payment_intent_response.last_payment_error.present? + raise Stripe::StripeError, payment_intent_response.last_payment_error.message + end + + if payment_intent_response.status != 'requires_capture' + raise Stripe::StripeError, I18n.t(:invalid_payment_state) + end + + payment_intent_id + end + end +end From 6877485c904b9bc5284c1a57649a7c309bd3f9fc Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Fri, 7 Feb 2020 18:11:54 +0000 Subject: [PATCH 12/23] Extract AuthorizeResponsePatcher from stripeSCA gateway --- app/models/spree/gateway/stripe_sca.rb | 31 ++++++++------------ app/services/checkout/stripe_redirect.rb | 8 +++++- lib/stripe/authorize_response_patcher.rb | 36 ++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 20 deletions(-) create mode 100644 lib/stripe/authorize_response_patcher.rb diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index ba8469ede9..a970aa87ed 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -2,6 +2,8 @@ require 'stripe/profile_storer' require 'stripe/credit_card_cloner' +require 'stripe/authorize_response_patcher' +require 'stripe/payment_intent_validator' require 'active_merchant/billing/gateways/stripe_payment_intents' require 'active_merchant/billing/gateways/stripe_decorator' @@ -33,7 +35,7 @@ module Spree # NOTE: the name of this method is determined by Spree::Payment::Processing def purchase(money, creditcard, gateway_options) begin - payment_intent_id = payment_intent(creditcard, gateway_options) + payment_intent_id = fetch_payment_intent(creditcard, gateway_options) rescue Stripe::StripeError => e return failed_activemerchant_billing_response(e.message) end @@ -50,12 +52,7 @@ module Spree authorize_response = provider.authorize(*options_for_authorize(money, creditcard, gateway_options)) - - if url = url_for_authorization(authorize_response) - authorize_response.cvv_result['message'] = url if authorize_response.cvv_result.present? - end - - authorize_response + Stripe::AuthorizeResponsePatcher.new(authorize_response).call! rescue Stripe::StripeError => e failed_activemerchant_billing_response(e.message) end @@ -105,23 +102,19 @@ module Spree [money, payment_method_id, options] end - def url_for_authorization(response) - next_action = response.params["next_source_action"] - return unless response.params["status"] == "requires_source_action" && - next_action.present? && - next_action["type"] == "authorize_with_url" - - next_action["authorize_with_url"]["url"] - end - - def payment_intent(creditcard, gateway_options) - order_number = gateway_options[:order_id].split('-').first - payment = Spree::Order.find_by_number(order_number).payments.merge(creditcard.payments).last + def fetch_payment_intent(creditcard, gateway_options) + payment = fetch_payment(creditcard, gateway_options) raise Stripe::StripeError, I18n.t(:no_pending_payments) unless payment&.response_code Stripe::PaymentIntentValidator.new.call(payment.response_code, stripe_account_id) end + def fetch_payment(creditcard, gateway_options) + order_number = gateway_options[:order_id].split('-').first + + Spree::Order.find_by_number(order_number).payments.merge(creditcard.payments).last + end + def failed_activemerchant_billing_response(error_message) ActiveMerchant::Billing::Response.new(false, error_message) end diff --git a/app/services/checkout/stripe_redirect.rb b/app/services/checkout/stripe_redirect.rb index 0fe6b90f09..40feebfde8 100644 --- a/app/services/checkout/stripe_redirect.rb +++ b/app/services/checkout/stripe_redirect.rb @@ -18,7 +18,7 @@ module Checkout payment.authorize! raise unless payment.pending? - payment.cvv_response_message if url?(payment.cvv_response_message) + field_with_url(payment) if url?(field_with_url(payment)) end private @@ -36,5 +36,11 @@ module Checkout string.starts_with?("http") end + + # Stripe::AuthorizeResponsePatcher patches the Stripe authorization response + # so that this field stores the redirect URL + def field_with_url(payment) + payment.cvv_response_message + end end end diff --git a/lib/stripe/authorize_response_patcher.rb b/lib/stripe/authorize_response_patcher.rb new file mode 100644 index 0000000000..7614ff47be --- /dev/null +++ b/lib/stripe/authorize_response_patcher.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +# This class patches the Stripe API response to the authorize action +# It copies the authorization URL to a field that is recognized and persisted by Spree payments +module Stripe + class AuthorizeResponsePatcher + def initialize(response) + @response = response + end + + def call! + if (url = url_for_authorization(@response)) && field_to_patch(@response).present? + field_to_patch(@response)['message'] = url + end + + @response + end + + private + + def url_for_authorization(response) + next_action = response.params["next_source_action"] + return unless response.params["status"] == "requires_source_action" && + next_action.present? && + next_action["type"] == "authorize_with_url" + + next_action["authorize_with_url"]["url"] + end + + # This field is used because the Spree code recognizes and stores it + # This data is then used in Checkout::StripeRedirect + def field_to_patch(response) + response.cvv_result + end + end +end From b5038c57458255758a6b32b186df5f148760789e Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 8 Feb 2020 11:14:50 +0000 Subject: [PATCH 13/23] Adapt subscriptionPaymentUpdater to include pending payments as pending payments! Pending payments of credit cards are payments already authorized, ready to be captured This is problably what will happen with subscriptions credit cards where payments will be authorized by the customer and the confirmation process on process_payments! will just capture the payment --- .../subscription_payment_updater_spec.rb | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/spec/lib/open_food_network/subscription_payment_updater_spec.rb b/spec/lib/open_food_network/subscription_payment_updater_spec.rb index 4e238476d5..77be81020e 100644 --- a/spec/lib/open_food_network/subscription_payment_updater_spec.rb +++ b/spec/lib/open_food_network/subscription_payment_updater_spec.rb @@ -1,3 +1,4 @@ +require 'spec_helper' require 'open_food_network/subscription_payment_updater' module OpenFoodNetwork @@ -9,12 +10,12 @@ module OpenFoodNetwork context "when only one payment exists on the order" do let!(:payment) { create(:payment, order: order) } - context "where the payment is in the 'checkout' state" do + context "where the payment is pending" do it { expect(updater.send(:payment)).to eq payment } end - context "where the payment is in some other state" do - before { payment.update_attribute(:state, 'pending') } + context "where the payment is failed" do + before { payment.update_attribute(:state, 'failed') } it { expect(updater.send(:payment)).to be nil } end end @@ -23,19 +24,19 @@ module OpenFoodNetwork let!(:payment1) { create(:payment, order: order) } let!(:payment2) { create(:payment, order: order) } - context "where more than one payment is in the 'checkout' state" do + context "where more than one payment is pending" do it { expect([payment1, payment2]).to include updater.send(:payment) } end - context "where only one payment is in the 'checkout' state" do - before { payment1.update_attribute(:state, 'pending') } + context "where only one payment is pending" do + before { payment1.update_attribute(:state, 'failed') } it { expect(updater.send(:payment)).to eq payment2 } end - context "where no payments are in the 'checkout' state" do + context "where no payments are pending" do before do - payment1.update_attribute(:state, 'pending') - payment2.update_attribute(:state, 'pending') + payment1.update_attribute(:state, 'failed') + payment2.update_attribute(:state, 'failed') end it { expect(updater.send(:payment)).to be nil } From d0a3ab68f3402095a2c4deae41606374c683d7c8 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 13:19:19 +0000 Subject: [PATCH 14/23] Make processing payments also pending payments --- app/models/spree/order_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 1cde659f65..0b8311372e 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -118,7 +118,7 @@ Spree::Order.class_eval do # "Checkout" is the initial state and, for card payments, "pending" is the state after authorization # These are both valid states to process the payment def pending_payments - (payments.select(&:pending?) + payments.select(&:checkout?)).uniq + (payments.select(&:pending?) + payments.select(&:processing?) + payments.select(&:checkout?)).uniq end def remove_variant(variant) From 531c385aaeb1c5b482eb0ffc7bccbeedeb9d826d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 13:24:46 +0000 Subject: [PATCH 15/23] Adapt stripe sca spec to new double step auth+capture payment process --- spec/requests/checkout/stripe_sca_spec.rb | 51 ++++++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/spec/requests/checkout/stripe_sca_spec.rb b/spec/requests/checkout/stripe_sca_spec.rb index 9c8f3d1b96..6001de71d9 100644 --- a/spec/requests/checkout/stripe_sca_spec.rb +++ b/spec/requests/checkout/stripe_sca_spec.rb @@ -22,6 +22,8 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques let(:stripe_payment_method) { "pm_123" } let(:customer_id) { "cus_A123" } let(:hubs_stripe_payment_method) { "pm_456" } + let(:payment_intent_id) { "pi_123" } + let(:stripe_redirect_url) { "http://stripe.com/redirect" } let(:payments_attributes) do { payment_method_id: payment_method.id, @@ -62,6 +64,11 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques let(:payment_intent_response_mock) do { status: 200, body: JSON.generate(object: "payment_intent", amount: 2000, charges: { data: [{ id: "ch_1234", amount: 2000 }]}) } end + let(:payment_intent_authorize_response_mock) do + { status: 200, body: JSON.generate(id: payment_intent_id, object: "payment_intent", amount: 2000, + status: "requires_capture", last_payment_error: nil, + charges: { data: [{ id: "ch_1234", amount: 2000 }]}) } + end before do order_cycle_distributed_variants = double(:order_cycle_distributed_variants) @@ -86,13 +93,23 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques headers: { 'Stripe-Account' => 'abc123' }) .to_return(hubs_payment_method_response_mock) - # Charges the card + # Authorizes the payment stub_request(:post, "https://api.stripe.com/v1/payment_intents") .with(basic_auth: ["sk_test_12345", ""], body: /#{hubs_stripe_payment_method}.*#{order.number}/) + .to_return(payment_intent_authorize_response_mock) + + # Retrieves paymeent intent info + stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") + .with(headers: { 'Stripe-Account' => 'abc123' }) + .to_return(payment_intent_authorize_response_mock) + + # Captures the payment + stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") + .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) .to_return(payment_intent_response_mock) end - context "and the paymeent intent request is successful" do + context "and the payment intent request is successful" do it "should process the payment without storing card details" do put update_checkout_path, params @@ -176,12 +193,22 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques .with(body: { customer: customer_id }) .to_return(payment_method_attach_response_mock) - # Charges the card + # Authorizes the payment stub_request(:post, "https://api.stripe.com/v1/payment_intents") .with( basic_auth: ["sk_test_12345", ""], body: /.*#{order.number}/ - ).to_return(payment_intent_response_mock) + ).to_return(payment_intent_authorize_response_mock) + + # Retrieves paymeent intent info + stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") + .with(headers: { 'Stripe-Account' => 'abc123' }) + .to_return(payment_intent_authorize_response_mock) + + # Captures the payment + stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") + .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) + .to_return(payment_intent_response_mock) end context "and the customer, payment_method and payment_intent requests are successful" do @@ -268,9 +295,21 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques params[:order][:existing_card_id] = credit_card.id quick_login_as(order.user) - # Charges the card + # Authorizes the payment stub_request(:post, "https://api.stripe.com/v1/payment_intents") - .with(basic_auth: ["sk_test_12345", ""], body: %r{#{customer_id}.*#{hubs_stripe_payment_method}}) + .with( + basic_auth: ["sk_test_12345", ""], + body: /.*#{order.number}/ + ).to_return(payment_intent_authorize_response_mock) + + # Retrieves paymeent intent info + stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") + .with(headers: { 'Stripe-Account' => 'abc123' }) + .to_return(payment_intent_authorize_response_mock) + + # Captures the payment + stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") + .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) .to_return(payment_intent_response_mock) end From 895032fe6a97f27bae9f6c2ca6a0505adb48b23a Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 14:59:40 +0000 Subject: [PATCH 16/23] Move stubbed requests to the top as they are the same for every test --- spec/requests/checkout/stripe_sca_spec.rb | 64 ++++++----------------- 1 file changed, 15 insertions(+), 49 deletions(-) diff --git a/spec/requests/checkout/stripe_sca_spec.rb b/spec/requests/checkout/stripe_sca_spec.rb index 6001de71d9..acb1eca2ea 100644 --- a/spec/requests/checkout/stripe_sca_spec.rb +++ b/spec/requests/checkout/stripe_sca_spec.rb @@ -79,6 +79,21 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques order.update_attributes(distributor_id: enterprise.id, order_cycle_id: order_cycle.id) order.reload.update_totals set_order order + + # Authorizes the payment + stub_request(:post, "https://api.stripe.com/v1/payment_intents") + .with(basic_auth: ["sk_test_12345", ""], body: /.*#{order.number}/) + .to_return(payment_intent_authorize_response_mock) + + # Retrieves payment intent info + stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") + .with(headers: { 'Stripe-Account' => 'abc123' }) + .to_return(payment_intent_authorize_response_mock) + + # Captures the payment + stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") + .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) + .to_return(payment_intent_response_mock) end context "when the user submits a new card and doesn't request that the card is saved for later" do @@ -92,21 +107,6 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques .with(body: { payment_method: stripe_payment_method }, headers: { 'Stripe-Account' => 'abc123' }) .to_return(hubs_payment_method_response_mock) - - # Authorizes the payment - stub_request(:post, "https://api.stripe.com/v1/payment_intents") - .with(basic_auth: ["sk_test_12345", ""], body: /#{hubs_stripe_payment_method}.*#{order.number}/) - .to_return(payment_intent_authorize_response_mock) - - # Retrieves paymeent intent info - stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") - .with(headers: { 'Stripe-Account' => 'abc123' }) - .to_return(payment_intent_authorize_response_mock) - - # Captures the payment - stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") - .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) - .to_return(payment_intent_response_mock) end context "and the payment intent request is successful" do @@ -192,23 +192,6 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques stub_request(:post, "https://api.stripe.com/v1/payment_methods/#{stripe_payment_method}/attach") .with(body: { customer: customer_id }) .to_return(payment_method_attach_response_mock) - - # Authorizes the payment - stub_request(:post, "https://api.stripe.com/v1/payment_intents") - .with( - basic_auth: ["sk_test_12345", ""], - body: /.*#{order.number}/ - ).to_return(payment_intent_authorize_response_mock) - - # Retrieves paymeent intent info - stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") - .with(headers: { 'Stripe-Account' => 'abc123' }) - .to_return(payment_intent_authorize_response_mock) - - # Captures the payment - stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") - .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) - .to_return(payment_intent_response_mock) end context "and the customer, payment_method and payment_intent requests are successful" do @@ -294,23 +277,6 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques before do params[:order][:existing_card_id] = credit_card.id quick_login_as(order.user) - - # Authorizes the payment - stub_request(:post, "https://api.stripe.com/v1/payment_intents") - .with( - basic_auth: ["sk_test_12345", ""], - body: /.*#{order.number}/ - ).to_return(payment_intent_authorize_response_mock) - - # Retrieves paymeent intent info - stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") - .with(headers: { 'Stripe-Account' => 'abc123' }) - .to_return(payment_intent_authorize_response_mock) - - # Captures the payment - stub_request(:post, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}/capture") - .with(basic_auth: ["sk_test_12345", ""], body: { amount_to_capture: "1234" }) - .to_return(payment_intent_response_mock) end context "and the payment intent and payment method requests are accepted" do From b54b9817408e86a2446d5dbda3d7a9cb1c956778 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 16:20:41 +0000 Subject: [PATCH 17/23] Improve readability of PaymentIntentValidator and cover with specs --- config/locales/en.yml | 2 +- lib/stripe/payment_intent_validator.rb | 23 +++++-- .../stripe/payment_intent_validator_spec.rb | 60 +++++++++++++++++++ 3 files changed, 78 insertions(+), 7 deletions(-) create mode 100644 spec/lib/stripe/payment_intent_validator_spec.rb diff --git a/config/locales/en.yml b/config/locales/en.yml index fb59f0f086..30701d0f21 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -277,7 +277,7 @@ en: error: Error processing_payment: "Processing payment..." no_pending_payments: "No pending payments" - invalid_payment_state: "Invalid Payment state" + invalid_payment_state: "Invalid payment state" filter_results: Filter Results quantity: Quantity pick_up: Pick up diff --git a/lib/stripe/payment_intent_validator.rb b/lib/stripe/payment_intent_validator.rb index 77e532284d..b6d1e52b7a 100644 --- a/lib/stripe/payment_intent_validator.rb +++ b/lib/stripe/payment_intent_validator.rb @@ -6,15 +6,26 @@ module Stripe def call(payment_intent_id, stripe_account_id) payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, stripe_account: stripe_account_id) - if payment_intent_response.last_payment_error.present? - raise Stripe::StripeError, payment_intent_response.last_payment_error.message - end - if payment_intent_response.status != 'requires_capture' - raise Stripe::StripeError, I18n.t(:invalid_payment_state) - end + raise_if_last_payment_error_present(payment_intent_response) + raise_if_not_in_capture_state(payment_intent_response) payment_intent_id end + + private + + def raise_if_last_payment_error_present(payment_intent_response) + return unless payment_intent_response.respond_to?(:last_payment_error) && + payment_intent_response.last_payment_error.present? + + raise Stripe::StripeError, payment_intent_response.last_payment_error.message + end + + def raise_if_not_in_capture_state(payment_intent_response) + return unless payment_intent_response.status != 'requires_capture' + + raise Stripe::StripeError, I18n.t(:invalid_payment_state) + end end end diff --git a/spec/lib/stripe/payment_intent_validator_spec.rb b/spec/lib/stripe/payment_intent_validator_spec.rb new file mode 100644 index 0000000000..0488c0b56f --- /dev/null +++ b/spec/lib/stripe/payment_intent_validator_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'stripe/payment_intent_validator' + +module Stripe + describe PaymentIntentValidator do + describe "#call" do + let(:validator) { Stripe::PaymentIntentValidator.new } + let(:payment_intent_id) { "pi_123" } + let(:stripe_account_id) { "acct_456" } + let(:payment_intent_response_mock) { { status: 200, body: payment_intent_response_body } } + + before do + allow(Stripe).to receive(:api_key) { "sk_test_12345" } + + stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") + .with(headers: { 'Stripe-Account' => stripe_account_id }) + .to_return(payment_intent_response_mock) + end + + context "when payment intent is valid" do + let(:payment_intent_response_body) { + JSON.generate(id: payment_intent_id, status: "requires_capture") + } + + it "returns payment intent id and does not raise" do + expect { + result = validator.call(payment_intent_id, stripe_account_id) + expect(result).to eq payment_intent_id + }.to_not raise_error Stripe::StripeError + end + end + + context "when payment intent status is not requires status" do + let(:payment_intent_response_body) { + JSON.generate(id: payment_intent_id, status: "failed") + } + + it "raises Stripe error with an invalid_payment_state message" do + expect { + validator.call(payment_intent_id, stripe_account_id) + }.to raise_error Stripe::StripeError, "Invalid payment state" + end + end + + context "when payment intent contains an error" do + let(:payment_intent_response_body) { + JSON.generate(id: payment_intent_id, last_payment_error: { message: "No money" }) + } + + it "raises Stripe error with payment intent last_payment_error as message" do + expect { + validator.call(payment_intent_id, stripe_account_id) + }.to raise_error Stripe::StripeError, "No money" + end + end + end + end +end From 6b80eb2c1626365615ae33a212f5b9e26a7e63a1 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 17:32:41 +0000 Subject: [PATCH 18/23] Add spec for AuthorizeResponsePatcher --- .../stripe/authorize_response_patcher_spec.rb | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 spec/lib/stripe/authorize_response_patcher_spec.rb diff --git a/spec/lib/stripe/authorize_response_patcher_spec.rb b/spec/lib/stripe/authorize_response_patcher_spec.rb new file mode 100644 index 0000000000..1572c1ced5 --- /dev/null +++ b/spec/lib/stripe/authorize_response_patcher_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +module Stripe + describe AuthorizeResponsePatcher do + describe "#call!" do + let(:patcher) { Stripe::AuthorizeResponsePatcher.new(response) } + let(:params) { {} } + let(:response) { ActiveMerchant::Billing::Response.new(true, "Transaction approved", params) } + + context "when url not found in response" do + it "does nothing" do + new_response = patcher.call! + expect(new_response).to eq response + end + end + + context "when url is found in response" do + let(:params) { + { "status" => "requires_source_action", + "next_source_action" => { "type" => "authorize_with_url", + "authorize_with_url" => { "url" => "test_url" } } } + } + + it "patches response.cvv_result.message with the url in the response" do + new_response = patcher.call! + expect(new_response.cvv_result['message']).to eq "test_url" + end + end + end + end +end From d5287026f8af7ceb568d8d149ba2eaf2b12fb503 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 18:11:41 +0000 Subject: [PATCH 19/23] Add spec for Checkout::PostCheckoutActions --- .../checkout/post_checkout_actions_spec.rb | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 spec/services/checkout/post_checkout_actions_spec.rb diff --git a/spec/services/checkout/post_checkout_actions_spec.rb b/spec/services/checkout/post_checkout_actions_spec.rb new file mode 100644 index 0000000000..594671a908 --- /dev/null +++ b/spec/services/checkout/post_checkout_actions_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Checkout::PostCheckoutActions do + let(:order) { create(:order_with_distributor) } + let(:postCheckoutActions) { Checkout::PostCheckoutActions.new(order) } + + describe "#success" do + let(:controller) {} + let(:params) { { order: {} } } + let(:current_user) { order.distributor.owner } + + let(:reset_order_service) { instance_double(ResetOrderService) } + + before do + expect(ResetOrderService).to receive(:new). + with(controller, order).and_return(reset_order_service) + expect(reset_order_service).to receive(:call) + end + + it "resets the order" do + postCheckoutActions.success(controller, params, current_user) + end + + describe "setting the user default address" do + let(:user_default_address_setter) { instance_double(UserDefaultAddressSetter) } + + before do + expect(UserDefaultAddressSetter).to receive(:new). + with(order, current_user).and_return(user_default_address_setter) + end + + it "sets user default bill address is option selected in params" do + params[:order][:default_bill_address] = true + expect(user_default_address_setter).to receive(:set_default_bill_address) + + postCheckoutActions.success(controller, params, current_user) + end + + it "sets user default ship address is option selected in params" do + params[:order][:default_ship_address] = true + expect(user_default_address_setter).to receive(:set_default_ship_address) + + postCheckoutActions.success(controller, params, current_user) + end + end + end + + describe "#failure" do + let(:restart_checkout_service) { instance_double(RestartCheckout) } + + it "restarts the checkout process" do + expect(RestartCheckout).to receive(:new).with(order).and_return(restart_checkout_service) + expect(restart_checkout_service).to receive(:call) + + postCheckoutActions.failure + end + + it "fixes the ship address for collection orders with the distributor's address" do + expect(order.updater).to receive(:shipping_address_from_distributor) + + postCheckoutActions.failure + end + end +end From 66f07c0d1cb4ea031ef364cce9aa11ea7e719483 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 8 Feb 2020 21:22:42 +0000 Subject: [PATCH 20/23] Make checkout controller spec test both extracted payment redirects --- spec/controllers/checkout_controller_spec.rb | 40 ++++++++++++++------ 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 8f9b429d2e..a4ba67715b 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -238,23 +238,41 @@ describe CheckoutController, type: :controller do end end - describe "Paypal routing" do - let(:payment_method) { create(:payment_method, type: "Spree::Gateway::PayPalExpress") } - let(:restart_checkout) { instance_double(RestartCheckout, call: true) } - + describe "Payment redirects" do before do allow(controller).to receive(:current_distributor) { distributor } allow(controller).to receive(:current_order_cycle) { order_cycle } allow(controller).to receive(:current_order) { order } - - allow(RestartCheckout).to receive(:new) { restart_checkout } - end - - it "should check the payment method for Paypalness if we've selected one" do - expect(Spree::PaymentMethod).to receive(:find).twice.with(payment_method.id.to_s) { payment_method } allow(order).to receive(:update_attributes) { true } allow(order).to receive(:state) { "payment" } - spree_post :update, order: { payments_attributes: [{ payment_method_id: payment_method.id }] } + 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) } + + 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") + + spree_post :update, order: { payments_attributes: [{ payment_method_id: payment_method.id }] } + + expect(response.body).to eq({ path: "test_path" }.to_json) + end end end From a224c53200ddbd3e99f1ed165edee6acee842f18 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 9 Feb 2020 12:43:45 +0000 Subject: [PATCH 21/23] Add spec to test receiving a redirect from stripe with a valid payment intent id --- spec/controllers/checkout_controller_spec.rb | 38 +++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index a4ba67715b..b11ca1ab78 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe CheckoutController, type: :controller do - let(:distributor) { double(:distributor) } + let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } let(:order_cycle) { create(:simple_order_cycle) } let(:order) { create(:order) } let(:reset_order_service) { double(ResetOrderService) } @@ -36,7 +36,7 @@ describe CheckoutController, type: :controller do expect(flash[:info]).to eq("The hub you have selected is temporarily closed for orders. Please try again later.") end - describe "redirection to the cart" do + describe "redirection to cart and stripe" do let(:order_cycle_distributed_variants) { double(:order_cycle_distributed_variants) } before do @@ -44,7 +44,7 @@ describe CheckoutController, type: :controller do allow(order).to receive(:distributor).and_return(distributor) order.order_cycle = order_cycle - allow(OrderCycleDistributedVariants).to receive(:new).with(order_cycle, distributor).and_return(order_cycle_distributed_variants) + allow(OrderCycleDistributedVariants).to receive(:new).and_return(order_cycle_distributed_variants) end it "redirects when some items are out of stock" do @@ -62,12 +62,34 @@ describe CheckoutController, type: :controller do expect(response).to redirect_to cart_path end - it "does not redirect when items are available and in stock" do - allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true - expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(true) + describe "when items are available and in stock" do + before do + allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true + end - get :edit - expect(response).to be_success + it "does not redirect" do + expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(true) + get :edit + expect(response).to be_success + end + + describe "when the order is in payment state and a stripe payment intent is provided" do + before do + order.update_attribute :state, "payment" + order.ship_address = create(:address) + order.save! + order.payments << create(:payment, state: "pending", response_code: "pi_123") + + # this is called a 2nd time after order completion from the reset_order_service + expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).twice.and_return(true) + end + + it "completes the order and redirects to the order confirmation page" do + get :edit, { payment_intent: "pi_123" } + expect(order.completed?).to be true + expect(response).to redirect_to spree.order_path(order) + end + end end end From 65dd9f51cf36455cf7c61a888fd690e5e55572dc Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 9 Feb 2020 16:07:40 +0000 Subject: [PATCH 22/23] Add spec to cover update request where a stripe redirect must happen --- spec/requests/checkout/stripe_sca_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/requests/checkout/stripe_sca_spec.rb b/spec/requests/checkout/stripe_sca_spec.rb index acb1eca2ea..b3eb02014d 100644 --- a/spec/requests/checkout/stripe_sca_spec.rb +++ b/spec/requests/checkout/stripe_sca_spec.rb @@ -311,6 +311,21 @@ describe "checking out an order with a Stripe SCA payment method", type: :reques expect(order.payments.completed.count).to be 0 end end + + context "when the stripe API sends a url for the authorization of the transaction" do + let(:payment_intent_authorize_response_mock) do + { status: 200, body: JSON.generate(id: payment_intent_id, object: "payment_intent", + next_source_action: { type: "authorize_with_url", authorize_with_url: { url: stripe_redirect_url }}, + status: "requires_source_action" )} + end + + it "redirects the user to the authorization stripe url" do + put update_checkout_path, params + + expect(response.status).to be 200 + expect(response.body).to include stripe_redirect_url + end + end end end end From 02008769e96c2d61d9f2f2a0461eef30527f649e Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 9 Feb 2020 16:44:03 +0000 Subject: [PATCH 23/23] Make spree_payment.cvv_response_message without size limit so that long stripe redirect URLs can be stored there --- ...nge_cvv_response_message_to_text_in_spree_payments.rb | 9 +++++++++ db/schema.rb | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20200209163549_change_cvv_response_message_to_text_in_spree_payments.rb diff --git a/db/migrate/20200209163549_change_cvv_response_message_to_text_in_spree_payments.rb b/db/migrate/20200209163549_change_cvv_response_message_to_text_in_spree_payments.rb new file mode 100644 index 0000000000..5e2e8d88ad --- /dev/null +++ b/db/migrate/20200209163549_change_cvv_response_message_to_text_in_spree_payments.rb @@ -0,0 +1,9 @@ +class ChangeCvvResponseMessageToTextInSpreePayments < ActiveRecord::Migration + def up + change_column :spree_payments, :cvv_response_message, :text + end + + def down + change_column :spree_payments, :cvv_response_message, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index cbac7ede25..e064256c53 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20191202165700) do +ActiveRecord::Schema.define(:version => 20200209163549) do create_table "adjustment_metadata", :force => true do |t| t.integer "adjustment_id" @@ -601,7 +601,7 @@ ActiveRecord::Schema.define(:version => 20191202165700) do t.string "avs_response" t.string "identifier" t.string "cvv_response_code" - t.string "cvv_response_message" + t.text "cvv_response_message" end add_index "spree_payments", ["order_id"], :name => "index_spree_payments_on_order_id"