From 40f9b063fe2f2e606d0e2ebc2a418da081ac1a7f Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 15 Jan 2021 10:24:50 +0000 Subject: [PATCH 01/16] Remove ability to create new product from products page, use /admin/products/new instead. It's simpler if there is just one place to add a new product. Closes #6650 This removes the 'creating directly from the new product path' test scenario because we have another 'assigning important attributes' scenario above which provides enough coverage. --- .../admin/products/index/_header.html.haml | 4 +--- app/views/spree/admin/products/new.js.erb | 12 ---------- spec/features/admin/products_spec.rb | 22 ------------------- 3 files changed, 1 insertion(+), 37 deletions(-) delete mode 100644 app/views/spree/admin/products/new.js.erb diff --git a/app/views/spree/admin/products/index/_header.html.haml b/app/views/spree/admin/products/index/_header.html.haml index 943abc021c..e48b5b7a89 100644 --- a/app/views/spree/admin/products/index/_header.html.haml +++ b/app/views/spree/admin/products/index/_header.html.haml @@ -5,8 +5,6 @@ %div{ :class => "toolbar", 'data-hook' => "toolbar" } %ul{ :class => "actions header-action-links inline-menu" } %li#new_product_link - = button_link_to t(:new_product), new_object_url, { :remote => true, :icon => 'icon-plus', :id => 'admin_new_product' } + = button_link_to t(:new_product), new_object_url, { :icon => 'icon-plus', :id => 'admin_new_product' } = render partial: 'spree/admin/shared/product_sub_menu' - -%div#new_product(data-hook) diff --git a/app/views/spree/admin/products/new.js.erb b/app/views/spree/admin/products/new.js.erb deleted file mode 100644 index 33c38aac13..0000000000 --- a/app/views/spree/admin/products/new.js.erb +++ /dev/null @@ -1,12 +0,0 @@ -<%# This chunk is just a copy of Spree's backend/app/views/spree/admin/products/new.js.erb %> -$("#new_product").html('<%= escape_javascript(render :template => "spree/admin/products/new", :formats => [:html], :handlers => [:haml]) %>'); -handle_date_picker_fields(); -<% unless Rails.env.test? %> - $('.select2').select2(); -<% end %> -$("#table-filter").hide(); -$("#admin_new_product").parent().hide(); - -<%# We need to replace the page's title as well. We're navigating to a new page - although through ajax %> -$('.js-admin-page-title').html('<%= t('.title') %>'); diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb index 60adf49939..e2a3b8a3c3 100644 --- a/spec/features/admin/products_spec.rb +++ b/spec/features/admin/products_spec.rb @@ -68,28 +68,6 @@ feature ' expect(product.master.options_text).to eq("5kg") end - scenario "creating directly from the new product path", js: true do - login_as_admin_and_visit spree.new_admin_product_path - - select 'New supplier', from: 'product_supplier_id' - fill_in 'product_name', with: 'A new product !!!' - select "Weight (kg)", from: 'product_variant_unit_with_scale' - fill_in 'product_unit_value_with_description', with: 5 - select taxon.name, from: "product_primary_taxon_id" - fill_in 'product_price', with: '19.99' - fill_in 'product_on_hand', with: 5 - select 'Test Tax Category', from: 'product_tax_category_id' - page.find("div[id^='taTextElement']").native.send_keys('A description...') - - click_button 'Create' - - expect(current_path).to eq spree.admin_products_path - expect(flash_message).to eq('Product "A new product !!!" has been successfully created!') - product = Spree::Product.find_by(name: 'A new product !!!') - expect(product.variant_unit).to eq('weight') - expect(product.variant_unit_scale).to eq(1000) - end - scenario "creating an on-demand product", js: true do login_as_admin_and_visit spree.admin_products_path From 802e49bed33c2c5f91b1127a123600c1fb76e213 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 14 Dec 2020 10:05:09 -0800 Subject: [PATCH 02/16] add PaymentMailer and send email if payment auth is required --- .../spree/admin/payments_controller.rb | 4 +++ app/controllers/spree/payments_controller.rb | 31 +++++++++++++++++++ app/mailers/spree/payment_mailer.rb | 16 ++++++++++ .../authorize_payment.html.haml | 2 ++ .../payment_mailer/authorize_payment.text.erb | 2 ++ config/locales/en.yml | 5 +++ config/routes.rb | 1 + .../payments/payments_controller_spec.rb | 13 ++++++++ 8 files changed, 74 insertions(+) create mode 100644 app/controllers/spree/payments_controller.rb create mode 100644 app/mailers/spree/payment_mailer.rb create mode 100644 app/views/spree/payment_mailer/authorize_payment.html.haml create mode 100644 app/views/spree/payment_mailer/authorize_payment.text.erb diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index e0a530a823..8d6fc851f3 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -154,6 +154,10 @@ module Spree return unless @payment.payment_method.class == Spree::Gateway::StripeSCA @payment.authorize! + if @payment.cvv_response_message.present? + PaymentMailer.authorize_payment(@payment).deliver + raise Spree::Core::GatewayError, I18n.t('action_required') + end return if @payment.pending? && @payment.cvv_response_message.nil? raise Spree::Core::GatewayError, I18n.t('authorization_failure') diff --git a/app/controllers/spree/payments_controller.rb b/app/controllers/spree/payments_controller.rb new file mode 100644 index 0000000000..ed93481c25 --- /dev/null +++ b/app/controllers/spree/payments_controller.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Spree + class PaymentsController < Spree::StoreController + ssl_required :redirect_to_authorize + + respond_to :html + + prepend_before_action :require_logged_in, only: :redirect_to_authorize + + def redirect_to_authorize + @payment = Spree::Payment.find(params[:id]) + authorize! :show, @payment + + if url = @payment.cvv_response_message + redirect_to url + else + redirect_to order_url(@payment.order) + end + end + + private + + def require_logged_in + return if session[:access_token] || params[:token] || spree_current_user + + flash[:error] = I18n.t("spree.orders.edit.login_to_view_order") + redirect_to main_app.root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}") + end + end +end diff --git a/app/mailers/spree/payment_mailer.rb b/app/mailers/spree/payment_mailer.rb new file mode 100644 index 0000000000..b76e91ea83 --- /dev/null +++ b/app/mailers/spree/payment_mailer.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Spree + class PaymentMailer < BaseMailer + include I18nHelper + + def authorize_payment(payment) + @payment = payment + subject = I18n.t('spree.payment_mailer.authorize_payment.subject', + distributor: @payment.order.distributor.name) + mail(to: payment.order.user.email, + from: from_address, + subject: subject) + end + end +end diff --git a/app/views/spree/payment_mailer/authorize_payment.html.haml b/app/views/spree/payment_mailer/authorize_payment.html.haml new file mode 100644 index 0000000000..438a51c416 --- /dev/null +++ b/app/views/spree/payment_mailer/authorize_payment.html.haml @@ -0,0 +1,2 @@ += t('.instructions', distributor: @payment.order.distributor.name, amount: @payment.display_amount) += main_app.authorize_payment_url(@payment) diff --git a/app/views/spree/payment_mailer/authorize_payment.text.erb b/app/views/spree/payment_mailer/authorize_payment.text.erb new file mode 100644 index 0000000000..e722b24997 --- /dev/null +++ b/app/views/spree/payment_mailer/authorize_payment.text.erb @@ -0,0 +1,2 @@ +<%= t('.instructions', distributor: @payment.order.distributor.name, amount: @payment.display_amount) %> +<%= main_app.authorize_payment_url(@payment) %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 3a4700a9f5..9d53a0358d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2455,6 +2455,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using payment_processing_failed: "Payment could not be processed, please check the details you entered" payment_method_not_supported: "That payment method is unsupported. Please choose another one." payment_updated: "Payment Updated" + action_required: "Action required" inventory_settings: "Inventory Settings" tag_rules: "Tag Rules" shop_preferences: "Shop Preferences" @@ -3644,6 +3645,10 @@ See the %{link} to find out more about %{sitename}'s features and to start using subject: "Reset password instructions" confirmation_instructions: subject: "Please confirm your OFN account" + payment_mailer: + authorize_payment: + subject: "Please authorize your payment to %{distributor} on OFN" + instructions: "Your payment of %{amount} to %{distributor} requires additional authentication. Please visit the following URL to authorize your payment:" shipment_mailer: shipped_email: dear_customer: "Dear Customer," diff --git a/config/routes.rb b/config/routes.rb index a657f7f234..795470cf3a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -29,6 +29,7 @@ Openfoodnetwork::Application.routes.draw do patch "/cart", :to => "spree/orders#update", :as => :update_cart put "/cart/empty", :to => 'spree/orders#empty', :as => :empty_cart get '/orders/:id/token/:token' => 'spree/orders#show', :as => :token_order + get '/payments/:id/authorize' => 'spree/payments#redirect_to_authorize', as: "authorize_payment" resource :cart, controller: "cart" do post :populate 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 2f327e2de2..e91586e4ce 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -91,6 +91,19 @@ describe Spree::Admin::PaymentsController, type: :controller do end end + context "where further action is required" do + before do + allow_any_instance_of(Spree::Payment).to receive(:authorize!) do |payment| + payment.update cvv_response_message: "http://redirect_url" + end + end + it "redirects to new payment page with flash error" do + spree_post :create, payment: params, order_id: order.number + + redirects_to_new_payment_page_with_flash_error(I18n.t('action_required')) + end + end + context "where both payment.process! and payment.authorize! work" do before do allow_any_instance_of(Spree::Payment).to receive(:authorize!) do |payment| From 8507dacc10a0f2ecd23550a2104832abbf790f6c Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 14 Dec 2020 14:06:10 -0800 Subject: [PATCH 03/16] pass return_url option to gateway authorize --- app/controllers/spree/admin/payments_controller.rb | 2 +- app/models/spree/gateway/stripe_sca.rb | 2 +- app/models/spree/payment/processing.rb | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index 8d6fc851f3..fb06010b63 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -153,7 +153,7 @@ module Spree def authorize_stripe_sca_payment return unless @payment.payment_method.class == Spree::Gateway::StripeSCA - @payment.authorize! + @payment.authorize!(spree.order_path(@payment.order, only_path: false)) if @payment.cvv_response_message.present? PaymentMailer.authorize_payment(@payment).deliver raise Spree::Core::GatewayError, I18n.t('action_required') diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index a3ccde45b2..fa5b4d8d4a 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -110,7 +110,7 @@ module Spree def options_for_authorize(money, creditcard, gateway_options) options = basic_options(gateway_options) - options[:return_url] = full_checkout_path + options[:return_url] = gateway_options[:return_url] || full_checkout_path customer_id, payment_method_id = Stripe::CreditCardCloner.new(creditcard, stripe_account_id).find_or_clone diff --git a/app/models/spree/payment/processing.rb b/app/models/spree/payment/processing.rb index 36704362a8..108cb1b61d 100644 --- a/app/models/spree/payment/processing.rb +++ b/app/models/spree/payment/processing.rb @@ -23,9 +23,9 @@ module Spree end end - def authorize! + def authorize!(return_url = nil) started_processing! - gateway_action(source, :authorize, :pend) + gateway_action(source, :authorize, :pend, return_url: return_url) end def purchase! @@ -222,7 +222,7 @@ module Spree refund_amount.to_f end - def gateway_action(source, action, success_state) + def gateway_action(source, action, success_state, options = {}) protect_from_connection_error do check_environment @@ -230,7 +230,7 @@ module Spree action, (amount * 100).round, source, - gateway_options + gateway_options.merge(options) ) handle_response(response, success_state, :failure) end From 5c0408c68c5fb063683968010485f3c86941529e Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 14 Dec 2020 14:10:11 -0800 Subject: [PATCH 04/16] pass paymentIntent ID to capture --- app/models/spree/gateway/stripe_sca.rb | 5 +++++ app/models/spree/payment/processing.rb | 14 +------------- spec/models/spree/payment_spec.rb | 2 +- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index fa5b4d8d4a..351adf019d 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -45,6 +45,11 @@ module Spree failed_activemerchant_billing_response(e.message) end + def capture(money, payment_intent_id, gateway_options) + options = basic_options(gateway_options) + provider.capture(money, payment_intent_id, options) + end + # NOTE: the name of this method is determined by Spree::Payment::Processing def charge_offline(money, creditcard, gateway_options) customer, payment_method = diff --git a/app/models/spree/payment/processing.rb b/app/models/spree/payment/processing.rb index 108cb1b61d..aabafeac37 100644 --- a/app/models/spree/payment/processing.rb +++ b/app/models/spree/payment/processing.rb @@ -44,19 +44,7 @@ module Spree started_processing! protect_from_connection_error do check_environment - - response = if payment_method.payment_profiles_supported? - # Gateways supporting payment profiles will need access to credit - # card object because this stores the payment profile information - # so supply the authorization itself as well as the credit card, - # rather than just the authorization code - payment_method.capture(self, source, gateway_options) - else - # Standard ActiveMerchant capture usage - payment_method.capture(money.money.cents, - response_code, - gateway_options) - end + response = payment_method.capture(money.money.cents, response_code, gateway_options) handle_response(response, :complete, :failure) end diff --git a/spec/models/spree/payment_spec.rb b/spec/models/spree/payment_spec.rb index 2420c7766d..2992713e7b 100644 --- a/spec/models/spree/payment_spec.rb +++ b/spec/models/spree/payment_spec.rb @@ -227,7 +227,7 @@ describe Spree::Payment do context "if successful" do before do - payment.payment_method.should_receive(:capture).with(payment, card, anything).and_return(success_response) + payment.payment_method.should_receive(:capture).and_return(success_response) end it "should make payment complete" do From 84b5fcf2ce9105261735338e1a6c03e28429fc1e Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 14 Dec 2020 19:48:54 -0800 Subject: [PATCH 05/16] add resend-authorization-email button to admin screen --- app/assets/stylesheets/admin/shared/icons.scss | 1 + app/models/spree/credit_card.rb | 8 +++++++- app/models/spree/payment.rb | 6 ++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/admin/shared/icons.scss b/app/assets/stylesheets/admin/shared/icons.scss index 04a3fb74c0..a2de27ba99 100644 --- a/app/assets/stylesheets/admin/shared/icons.scss +++ b/app/assets/stylesheets/admin/shared/icons.scss @@ -16,6 +16,7 @@ } .icon-email:before { @extend .icon-envelope:before } +.icon-resend_authorization_email:before { @extend .icon-envelope:before } .icon-resume:before { @extend .icon-refresh:before } .icon-cancel:before, diff --git a/app/models/spree/credit_card.rb b/app/models/spree/credit_card.rb index 948dcf829d..2a10e78794 100644 --- a/app/models/spree/credit_card.rb +++ b/app/models/spree/credit_card.rb @@ -79,11 +79,17 @@ module Spree end def actions - %w{capture void credit} + %w{capture void credit resend_authorization_email} + end + + def can_resend_authorization_email?(payment) + payment.pending? && payment.cvv_response_message.present? end # Indicates whether its possible to capture the payment def can_capture?(payment) + return false if payment.cvv_response_message.present? + payment.pending? || payment.checkout? end diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 578b91e3e5..954dfd5472 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -121,6 +121,12 @@ module Spree actions end + def resend_authorization_email! + return unless cvv_response_message.present? + + PaymentMailer.authorize_payment(self).deliver + end + def payment_source res = source.is_a?(Payment) ? source.source : source res || payment_method From b669ccdc7427a55f0824d557e486d7ac217a85f2 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 21 Jan 2021 09:02:27 -0800 Subject: [PATCH 06/16] refactor admin payments controller --- app/controllers/spree/admin/payments_controller.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index fb06010b63..3924ac9de6 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -154,13 +154,13 @@ module Spree return unless @payment.payment_method.class == Spree::Gateway::StripeSCA @payment.authorize!(spree.order_path(@payment.order, only_path: false)) - if @payment.cvv_response_message.present? - PaymentMailer.authorize_payment(@payment).deliver - raise Spree::Core::GatewayError, I18n.t('action_required') - end - return if @payment.pending? && @payment.cvv_response_message.nil? - raise Spree::Core::GatewayError, I18n.t('authorization_failure') + raise Spree::Core::GatewayError, I18n.t('authorization_failure') unless @payment.pending? + + return unless @payment.cvv_response_message.present? + + PaymentMailer.authorize_payment(@payment).deliver + raise Spree::Core::GatewayError, I18n.t('action_required') end end end From 5f1669280c46116ddefef4066e60b80626077915 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 7 Jan 2021 12:32:40 -0800 Subject: [PATCH 07/16] update to rspec 3 `expect` syntax --- spec/models/spree/payment_spec.rb | 90 ++++++++++++++++--------------- 1 file changed, 47 insertions(+), 43 deletions(-) diff --git a/spec/models/spree/payment_spec.rb b/spec/models/spree/payment_spec.rb index 2992713e7b..cbba2b4b9a 100644 --- a/spec/models/spree/payment_spec.rb +++ b/spec/models/spree/payment_spec.rb @@ -83,25 +83,25 @@ describe Spree::Payment do context "#process!" do it "should purchase if with auto_capture" do payment = build_stubbed(:payment, payment_method: gateway) - payment.payment_method.should_receive(:auto_capture?).and_return(true) - payment.should_receive(:purchase!) + expect(payment.payment_method).to receive(:auto_capture?).and_return(true) + expect(payment).to receive(:purchase!) payment.process! end it "should authorize without auto_capture" do payment = build_stubbed(:payment, payment_method: gateway) - payment.payment_method.should_receive(:auto_capture?).and_return(false) - payment.should_receive(:authorize!) + expect(payment.payment_method).to receive(:auto_capture?).and_return(false) + expect(payment).to receive(:authorize!) payment.process! end it "should make the state 'processing'" do - payment.should_receive(:started_processing!) + expect(payment).to receive(:started_processing!) payment.process! end it "should invalidate if payment method doesnt support source" do - payment.payment_method.should_receive(:supports?).with(payment.source).and_return(false) + expect(payment.payment_method).to receive(:supports?).with(payment.source).and_return(false) expect { payment.process! }.to raise_error(Spree::Core::GatewayError) expect(payment.state).to eq('invalid') end @@ -109,17 +109,17 @@ describe Spree::Payment do context "#authorize" do it "should call authorize on the gateway with the payment amount" do - payment.payment_method.should_receive(:authorize).with(amount_in_cents, - card, - anything).and_return(success_response) + expect(payment.payment_method).to receive(:authorize).with( + amount_in_cents, card, anything + ).and_return(success_response) payment.authorize! end it "should call authorize on the gateway with the currency code" do payment.stub currency: 'GBP' - payment.payment_method.should_receive(:authorize).with(amount_in_cents, - card, - hash_including({ currency: "GBP" })).and_return(success_response) + expect(payment.payment_method).to receive(:authorize).with( + amount_in_cents, card, hash_including({ currency: "GBP" }) + ).and_return(success_response) payment.authorize! end @@ -137,9 +137,9 @@ describe Spree::Payment do context "if successful" do before do - payment.payment_method.should_receive(:authorize).with(amount_in_cents, - card, - anything).and_return(success_response) + expect(payment.payment_method).to receive(:authorize).with( + amount_in_cents, card, anything + ).and_return(success_response) end it "should store the response_code, avs_response and cvv_response fields" do @@ -151,7 +151,7 @@ describe Spree::Payment do end it "should make payment pending" do - payment.should_receive(:pend!) + expect(payment).to receive(:pend!) payment.authorize! end end @@ -159,8 +159,8 @@ describe Spree::Payment do context "if unsuccessful" do it "should mark payment as failed" do gateway.stub(:authorize).and_return(failed_response) - payment.should_receive(:failure) - payment.should_not_receive(:pend) + expect(payment).to receive(:failure) + expect(payment).to_not receive(:pend) expect { payment.authorize! }.to raise_error(Spree::Core::GatewayError) @@ -170,7 +170,7 @@ describe Spree::Payment do context "purchase" do it "should call purchase on the gateway with the payment amount" do - gateway.should_receive(:purchase).with(amount_in_cents, card, anything).and_return(success_response) + expect(gateway).to receive(:purchase).with(amount_in_cents, card, anything).and_return(success_response) payment.purchase! end @@ -188,9 +188,9 @@ describe Spree::Payment do context "if successful" do before do - payment.payment_method.should_receive(:purchase).with(amount_in_cents, - card, - anything).and_return(success_response) + expect(payment.payment_method).to receive(:purchase).with( + amount_in_cents, card, anything + ).and_return(success_response) end it "should store the response_code and avs_response" do @@ -200,7 +200,7 @@ describe Spree::Payment do end it "should make payment complete" do - payment.should_receive(:complete!) + expect(payment).to receive(:complete!) payment.purchase! end end @@ -208,8 +208,8 @@ describe Spree::Payment do context "if unsuccessful" do it "should make payment failed" do gateway.stub(:purchase).and_return(failed_response) - payment.should_receive(:failure) - payment.should_not_receive(:pend) + expect(payment).to receive(:failure) + expect(payment).not_to receive(:pend) expect { payment.purchase! }.to raise_error(Spree::Core::GatewayError) end end @@ -227,11 +227,11 @@ describe Spree::Payment do context "if successful" do before do - payment.payment_method.should_receive(:capture).and_return(success_response) + expect(payment.payment_method).to receive(:capture).and_return(success_response) end it "should make payment complete" do - payment.should_receive(:complete) + expect(payment).to receive(:complete) payment.capture! end @@ -245,8 +245,8 @@ describe Spree::Payment do context "if unsuccessful" do it "should not make payment complete" do gateway.stub capture: failed_response - payment.should_receive(:failure) - payment.should_not_receive(:complete) + expect(payment).to receive(:failure) + expect(payment).to_not receive(:complete) expect { payment.capture! }.to raise_error(Spree::Core::GatewayError) end end @@ -256,9 +256,9 @@ describe Spree::Payment do context "when payment is completed" do it "should do nothing" do payment = build_stubbed(:payment, state: 'completed') - payment.should_not_receive(:complete) - payment.payment_method.should_not_receive(:capture) - payment.log_entries.should_not_receive(:create) + expect(payment).to_not receive(:complete) + expect(payment.payment_method).to_not receive(:capture) + expect(payment.log_entries).to_not receive(:create) payment.capture! end end @@ -273,7 +273,7 @@ describe Spree::Payment do context "when profiles are supported" do it "should call payment_gateway.void with the payment's response_code" do gateway.stub payment_profiles_supported?: true - gateway.should_receive(:void).with('123', card, anything).and_return(success_response) + expect(gateway).to receive(:void).with('123', card, anything).and_return(success_response) payment.void_transaction! end end @@ -281,7 +281,7 @@ describe Spree::Payment do context "when profiles are not supported" do it "should call payment_gateway.void with the payment's response_code" do gateway.stub payment_profiles_supported?: false - gateway.should_receive(:void).with('123', anything).and_return(success_response) + expect(gateway).to receive(:void).with('123', anything).and_return(success_response) payment.void_transaction! end end @@ -311,7 +311,7 @@ describe Spree::Payment do context "if unsuccessful" do it "should not void the payment" do gateway.stub void: failed_response - payment.should_not_receive(:void) + expect(payment).to_not receive(:void) expect { payment.void_transaction! }.to raise_error(Spree::Core::GatewayError) end end @@ -320,7 +320,7 @@ describe Spree::Payment do context "if payment is already voided" do it "should not void the payment" do payment = build_stubbed(:payment, payment_method: gateway, state: 'void') - payment.payment_method.should_not_receive(:void) + expect(payment.payment_method).to_not receive(:void) payment.void_transaction! end end @@ -339,7 +339,7 @@ describe Spree::Payment do end it "should call credit on the gateway with the credit amount and response_code" do - gateway.should_receive(:credit).with(1000, card, '123', anything).and_return(success_response) + expect(gateway).to receive(:credit).with(1000, card, '123', anything).and_return(success_response) payment.credit! end end @@ -350,7 +350,9 @@ describe Spree::Payment do end it "should call credit on the gateway with the credit amount and response_code" do - gateway.should_receive(:credit).with(amount_in_cents, card, '123', anything).and_return(success_response) + expect(gateway).to receive(:credit).with( + amount_in_cents, card, '123', anything + ).and_return(success_response) payment.credit! end end @@ -361,7 +363,9 @@ describe Spree::Payment do end it "should call credit on the gateway with the original payment amount and response_code" do - gateway.should_receive(:credit).with(amount_in_cents.to_f, card, '123', anything).and_return(success_response) + expect(gateway).to receive(:credit).with( + amount_in_cents.to_f, card, '123', anything + ).and_return(success_response) payment.credit! end end @@ -431,8 +435,8 @@ describe Spree::Payment do payment = build_stubbed(:payment) payment.state = 'processing' - payment.should_not_receive(:authorize!) - payment.should_not_receive(:purchase!) + expect(payment).to_not receive(:authorize!) + expect(payment).to_not receive(:purchase!) expect(payment.process!).to be_nil end end @@ -496,7 +500,7 @@ describe Spree::Payment do it "calls credit on the source with the payment and amount" do payment.state = 'completed' payment.stub(:credit_allowed).and_return(10) - payment.should_receive(:credit!).with(10) + expect(payment).to receive(:credit!).with(10) payment.partial_credit(10) end end @@ -519,7 +523,7 @@ describe Spree::Payment do order = create(:order) payment = Spree::Payment.create(amount: 100, order: order, payment_method: gateway) - order.should_receive(:update!) + expect(order).to receive(:update!) payment.save end From d9b27bc556481bf302fe04db9cde83966b937305 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 21 Jan 2021 09:10:23 -0800 Subject: [PATCH 08/16] move controller and mailer outside of spree namespace; use haml in template --- app/controllers/payments_controller.rb | 29 +++++++++++++++++ ... base authorization on the payment's order | 29 +++++++++++++++++ app/controllers/spree/payments_controller.rb | 31 ------------------- app/mailers/payment_mailer.rb | 14 +++++++++ app/mailers/spree/payment_mailer.rb | 16 ---------- .../authorize_payment.html.haml | 0 .../authorize_payment.text.haml | 3 ++ .../payment_mailer/authorize_payment.text.erb | 2 -- config/routes.rb | 2 +- 9 files changed, 76 insertions(+), 50 deletions(-) create mode 100644 app/controllers/payments_controller.rb create mode 100644 app/controllers/payments_controller.rb~aa3478fba... base authorization on the payment's order delete mode 100644 app/controllers/spree/payments_controller.rb create mode 100644 app/mailers/payment_mailer.rb delete mode 100644 app/mailers/spree/payment_mailer.rb rename app/views/{spree => }/payment_mailer/authorize_payment.html.haml (100%) create mode 100644 app/views/payment_mailer/authorize_payment.text.haml delete mode 100644 app/views/spree/payment_mailer/authorize_payment.text.erb diff --git a/app/controllers/payments_controller.rb b/app/controllers/payments_controller.rb new file mode 100644 index 0000000000..000ea22ffb --- /dev/null +++ b/app/controllers/payments_controller.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class PaymentsController < BaseController + ssl_required :redirect_to_authorize + + respond_to :html + + prepend_before_action :require_logged_in, only: :redirect_to_authorize + + def redirect_to_authorize + @payment = Spree::Payment.find(params[:id]) + authorize! :show, @payment + + if url = @payment.cvv_response_message + redirect_to url + else + redirect_to order_url(@payment.order) + end + end + + private + + def require_logged_in + return if session[:access_token] || params[:token] || spree_current_user + + flash[:error] = I18n.t("spree.orders.edit.login_to_view_order") + redirect_to main_app.root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}") + end +end diff --git a/app/controllers/payments_controller.rb~aa3478fba... base authorization on the payment's order b/app/controllers/payments_controller.rb~aa3478fba... base authorization on the payment's order new file mode 100644 index 0000000000..c0fb7eadcc --- /dev/null +++ b/app/controllers/payments_controller.rb~aa3478fba... base authorization on the payment's order @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class PaymentsController < BaseController + ssl_required :redirect_to_authorize + + respond_to :html + + prepend_before_action :require_logged_in, only: :redirect_to_authorize + + def redirect_to_authorize + @payment = Spree::Payment.find(params[:id]) + authorize! :show, @payment.order + + if url = @payment.cvv_response_message + redirect_to url + else + redirect_to order_url(@payment.order) + end + end + + private + + def require_logged_in + return if session[:access_token] || params[:token] || spree_current_user + + flash[:error] = I18n.t("spree.orders.edit.login_to_view_order") + redirect_to main_app.root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}") + end +end diff --git a/app/controllers/spree/payments_controller.rb b/app/controllers/spree/payments_controller.rb deleted file mode 100644 index ed93481c25..0000000000 --- a/app/controllers/spree/payments_controller.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Spree - class PaymentsController < Spree::StoreController - ssl_required :redirect_to_authorize - - respond_to :html - - prepend_before_action :require_logged_in, only: :redirect_to_authorize - - def redirect_to_authorize - @payment = Spree::Payment.find(params[:id]) - authorize! :show, @payment - - if url = @payment.cvv_response_message - redirect_to url - else - redirect_to order_url(@payment.order) - end - end - - private - - def require_logged_in - return if session[:access_token] || params[:token] || spree_current_user - - flash[:error] = I18n.t("spree.orders.edit.login_to_view_order") - redirect_to main_app.root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}") - end - end -end diff --git a/app/mailers/payment_mailer.rb b/app/mailers/payment_mailer.rb new file mode 100644 index 0000000000..b0c4a4bbff --- /dev/null +++ b/app/mailers/payment_mailer.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class PaymentMailer < Spree::BaseMailer + include I18nHelper + + def authorize_payment(payment) + @payment = payment + subject = I18n.t('spree.payment_mailer.authorize_payment.subject', + distributor: @payment.order.distributor.name) + mail(to: payment.order.user.email, + from: from_address, + subject: subject) + end +end diff --git a/app/mailers/spree/payment_mailer.rb b/app/mailers/spree/payment_mailer.rb deleted file mode 100644 index b76e91ea83..0000000000 --- a/app/mailers/spree/payment_mailer.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -module Spree - class PaymentMailer < BaseMailer - include I18nHelper - - def authorize_payment(payment) - @payment = payment - subject = I18n.t('spree.payment_mailer.authorize_payment.subject', - distributor: @payment.order.distributor.name) - mail(to: payment.order.user.email, - from: from_address, - subject: subject) - end - end -end diff --git a/app/views/spree/payment_mailer/authorize_payment.html.haml b/app/views/payment_mailer/authorize_payment.html.haml similarity index 100% rename from app/views/spree/payment_mailer/authorize_payment.html.haml rename to app/views/payment_mailer/authorize_payment.html.haml diff --git a/app/views/payment_mailer/authorize_payment.text.haml b/app/views/payment_mailer/authorize_payment.text.haml new file mode 100644 index 0000000000..956e63ec9f --- /dev/null +++ b/app/views/payment_mailer/authorize_payment.text.haml @@ -0,0 +1,3 @@ += t('.instructions', distributor: @payment.order.distributor.name, amount: @payment.display_amount) + += main_app.authorize_payment_url(@payment) diff --git a/app/views/spree/payment_mailer/authorize_payment.text.erb b/app/views/spree/payment_mailer/authorize_payment.text.erb deleted file mode 100644 index e722b24997..0000000000 --- a/app/views/spree/payment_mailer/authorize_payment.text.erb +++ /dev/null @@ -1,2 +0,0 @@ -<%= t('.instructions', distributor: @payment.order.distributor.name, amount: @payment.display_amount) %> -<%= main_app.authorize_payment_url(@payment) %> diff --git a/config/routes.rb b/config/routes.rb index 795470cf3a..5236a42c62 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -29,7 +29,7 @@ Openfoodnetwork::Application.routes.draw do patch "/cart", :to => "spree/orders#update", :as => :update_cart put "/cart/empty", :to => 'spree/orders#empty', :as => :empty_cart get '/orders/:id/token/:token' => 'spree/orders#show', :as => :token_order - get '/payments/:id/authorize' => 'spree/payments#redirect_to_authorize', as: "authorize_payment" + get '/payments/:id/authorize' => 'payments#redirect_to_authorize', as: "authorize_payment" resource :cart, controller: "cart" do post :populate From d76db9ee51cc332eeecc0c119cbb55a5175cb55d Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Fri, 8 Jan 2021 11:29:40 -0800 Subject: [PATCH 09/16] update payment controller spec to move payment to pending --- .../spree/admin/orders/payments/payments_controller_spec.rb | 1 + 1 file changed, 1 insertion(+) 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 e91586e4ce..68cd853908 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -95,6 +95,7 @@ describe Spree::Admin::PaymentsController, type: :controller do before do allow_any_instance_of(Spree::Payment).to receive(:authorize!) do |payment| payment.update cvv_response_message: "http://redirect_url" + payment.update state: "pending" end end it "redirects to new payment page with flash error" do From affc82b2b50359f9e341d1b925f4fde11552137b Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 21 Jan 2021 09:13:02 -0800 Subject: [PATCH 10/16] update payment jobs delivery methods --- app/controllers/spree/admin/payments_controller.rb | 2 +- app/models/spree/payment.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index 3924ac9de6..ee6847c10c 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -159,7 +159,7 @@ module Spree return unless @payment.cvv_response_message.present? - PaymentMailer.authorize_payment(@payment).deliver + PaymentMailer.authorize_payment(@payment).deliver_later raise Spree::Core::GatewayError, I18n.t('action_required') end end diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 954dfd5472..3bd1bcd4a3 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -124,7 +124,7 @@ module Spree def resend_authorization_email! return unless cvv_response_message.present? - PaymentMailer.authorize_payment(self).deliver + PaymentMailer.authorize_payment(self).deliver_later end def payment_source From ab5ffead1deb5ffabbc63d581defa8b3c9817567 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 7 Jan 2021 11:43:03 -0800 Subject: [PATCH 11/16] require that the redirect url be to stripe.com and over https --- lib/stripe/authorize_response_patcher.rb | 3 ++- .../spree/admin/orders/payments/payments_controller_spec.rb | 2 +- spec/features/admin/payments_stripe_spec.rb | 2 +- spec/lib/stripe/authorize_response_patcher_spec.rb | 4 ++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/stripe/authorize_response_patcher.rb b/lib/stripe/authorize_response_patcher.rb index 7614ff47be..d67b8dbca9 100644 --- a/lib/stripe/authorize_response_patcher.rb +++ b/lib/stripe/authorize_response_patcher.rb @@ -24,7 +24,8 @@ module Stripe next_action.present? && next_action["type"] == "authorize_with_url" - next_action["authorize_with_url"]["url"] + url = next_action["authorize_with_url"]["url"] + return url if url.match(%r{https?:\/\/[\S]+}) && url.include?("stripe.com") end # This field is used because the Spree code recognizes and stores it 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 68cd853908..f4cb9161c1 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -94,7 +94,7 @@ describe Spree::Admin::PaymentsController, type: :controller do context "where further action is required" do before do allow_any_instance_of(Spree::Payment).to receive(:authorize!) do |payment| - payment.update cvv_response_message: "http://redirect_url" + payment.update cvv_response_message: "https://www.stripe.com/authorize" payment.update state: "pending" end end diff --git a/spec/features/admin/payments_stripe_spec.rb b/spec/features/admin/payments_stripe_spec.rb index 00c110195a..a4467a4f5a 100644 --- a/spec/features/admin/payments_stripe_spec.rb +++ b/spec/features/admin/payments_stripe_spec.rb @@ -70,7 +70,7 @@ feature ' context "with a card that fails on registration because it requires(redirects) extra auth" do before do stub_payment_intents_post_request_with_redirect order: order, - redirect_url: "www.dummy.org" + redirect_url: "https://www.stripe.com/authorize" end it "fails to add a payment due to card error" do diff --git a/spec/lib/stripe/authorize_response_patcher_spec.rb b/spec/lib/stripe/authorize_response_patcher_spec.rb index 1572c1ced5..9ecb800ee2 100644 --- a/spec/lib/stripe/authorize_response_patcher_spec.rb +++ b/spec/lib/stripe/authorize_response_patcher_spec.rb @@ -20,12 +20,12 @@ module Stripe let(:params) { { "status" => "requires_source_action", "next_source_action" => { "type" => "authorize_with_url", - "authorize_with_url" => { "url" => "test_url" } } } + "authorize_with_url" => { "url" => "https://www.stripe.com/authorize" } } } } 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" + expect(new_response.cvv_result['message']).to eq "https://www.stripe.com/authorize" end end end From 903b2e7ff494c7b8ca04cd3de2598503b92390b8 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 11 Jan 2021 20:15:24 -0800 Subject: [PATCH 12/16] whitelist allowed events to be sent to a Payment --- .../spree/admin/payments_controller.rb | 6 +++- .../payments/payments_controller_spec.rb | 36 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index ee6847c10c..648632e537 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -56,7 +56,7 @@ module Spree # Because we have a transition method also called void, we do this to avoid conflicts. event = "void_transaction" if event == "void" - if @payment.public_send("#{event}!") + if allowed_events.include?(event) && @payment.public_send("#{event}!") flash[:success] = t(:payment_updated) else flash[:error] = t(:cannot_perform_operation) @@ -162,6 +162,10 @@ module Spree PaymentMailer.authorize_payment(@payment).deliver_later raise Spree::Core::GatewayError, I18n.t('action_required') end + + def allowed_events + %w{capture void_transaction credit refund resend_authorization_email} + end end end end 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 f4cb9161c1..1e1437aee2 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -241,5 +241,41 @@ describe Spree::Admin::PaymentsController, type: :controller do expect(flash[:success]).to eq(I18n.t(:payment_updated)) end end + + context 'on resend_authorization_email event' do + let(:params) { { e: 'resend_authorization_email', order_id: order.number, id: payment.id } } + let(:mail_mock) { double(:mailer_mock, deliver_later: true) } + + before do + allow(PaymentMailer).to receive(:authorize_payment) { mail_mock } + allow(request).to receive(:referer) { 'http://foo.com' } + allow(Spree::Payment).to receive(:find).with(payment.id.to_s) { payment } + allow(payment).to receive(:cvv_response_message).and_return("https://www.stripe.com/authorize") + end + + it "resends the authorization email" do + spree_put :fire, params + + expect(flash[:success]).to eq(I18n.t(:payment_updated)) + expect(PaymentMailer).to have_received(:authorize_payment) + expect(mail_mock).to have_received(:deliver_later) + end + end + + context 'on an unrecognized event' do + let(:params) { { e: 'unrecognized_event', 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 'does not process the event' do + spree_put :fire, params + + expect(payment).to_not receive(:unrecognized_event) + expect(flash[:error]).to eq(I18n.t(:cannot_perform_operation)) + end + end end end From 1635b83c1500334b1b6e19b4de5887c429755ed2 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 11 Jan 2021 20:15:51 -0800 Subject: [PATCH 13/16] add missing translation key for payment actions --- config/locales/en.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/locales/en.yml b/config/locales/en.yml index 9d53a0358d..b06e79a89a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2455,6 +2455,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using payment_processing_failed: "Payment could not be processed, please check the details you entered" payment_method_not_supported: "That payment method is unsupported. Please choose another one." payment_updated: "Payment Updated" + cannot_perform_operation: "Could not update the payment" action_required: "Action required" inventory_settings: "Inventory Settings" tag_rules: "Tag Rules" From 8bcaeff6c890f7cd6921a270be05eba126e09ab6 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 14 Jan 2021 10:03:59 -0800 Subject: [PATCH 14/16] resolve merge conflict; add ssl helper to base controller --- app/controllers/base_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/base_controller.rb b/app/controllers/base_controller.rb index 143370f3e9..8ea4718cb5 100644 --- a/app/controllers/base_controller.rb +++ b/app/controllers/base_controller.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true require 'spree/core/controller_helpers/order' +require 'spree/core/controller_helpers/ssl' require 'open_food_network/tag_rule_applicator' class BaseController < ApplicationController layout 'darkswarm' include Spree::Core::ControllerHelpers::Order + include Spree::Core::ControllerHelpers::SSL include I18nHelper include OrderCyclesHelper From ce4621858dfcde9d901e048600cef57c60c8d51e Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 14 Jan 2021 14:40:37 -0800 Subject: [PATCH 15/16] base authorization on the payment's order --- app/controllers/payments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/payments_controller.rb b/app/controllers/payments_controller.rb index 000ea22ffb..c0fb7eadcc 100644 --- a/app/controllers/payments_controller.rb +++ b/app/controllers/payments_controller.rb @@ -9,7 +9,7 @@ class PaymentsController < BaseController def redirect_to_authorize @payment = Spree::Payment.find(params[:id]) - authorize! :show, @payment + authorize! :show, @payment.order if url = @payment.cvv_response_message redirect_to url From bba9e550062c69672111945445796e8b633d19e8 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 20 Jan 2021 21:10:24 -0800 Subject: [PATCH 16/16] don't try to process a payment if it's pending auth --- app/models/spree/payment/processing.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/spree/payment/processing.rb b/app/models/spree/payment/processing.rb index aabafeac37..1648b0ce36 100644 --- a/app/models/spree/payment/processing.rb +++ b/app/models/spree/payment/processing.rb @@ -5,6 +5,7 @@ module Spree module Processing def process! return unless validate! + return if cvv_response_message.present? if payment_method.auto_capture? purchase! @@ -15,6 +16,7 @@ module Spree def process_offline! return unless validate! + return if cvv_response_message.present? if payment_method.auto_capture? charge_offline!