From 6d0d4b50963d86adfec6854de93d4380c3e7d2f8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 18 Nov 2020 13:41:37 +0000 Subject: [PATCH 01/12] Bring in Spree::PaypalController#payment_method Original method from the gem. Modified in preceding commit. --- app/controllers/spree/paypal_controller_decorator.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index ff03c823ef..0864b649f1 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -66,6 +66,10 @@ Spree::PaypalController.class_eval do private + def payment_method + Spree::PaymentMethod.find(params[:payment_method_id]) + end + def permit_parameters! params.permit(:token, :payment_method_id, :PayerID) end From dd8f139c1b58043fbf90da54da9d1fcc0225cb46 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 18 Nov 2020 13:44:11 +0000 Subject: [PATCH 02/12] Memoize Spree::PaypalController#payment_method This gets called 4 or 5 times in a single request just to read basic attributes from the object. The query doesn't need to be repeated each time --- app/controllers/spree/paypal_controller_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index 0864b649f1..1cade23304 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -67,7 +67,7 @@ Spree::PaypalController.class_eval do private def payment_method - Spree::PaymentMethod.find(params[:payment_method_id]) + @payment_method ||= Spree::PaymentMethod.find(params[:payment_method_id]) end def permit_parameters! From 2faea65f82d427abd8f274711424a4cd05d4124f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 18 Nov 2020 14:51:23 +0000 Subject: [PATCH 03/12] Bring in Spree::PaypalController#confirm method Original method from the gem. This handles the post-payment response from paypal. --- .../spree/paypal_controller_decorator.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index 1cade23304..a012978600 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -51,6 +51,27 @@ Spree::PaypalController.class_eval do end end + def confirm + order = current_order || raise(ActiveRecord::RecordNotFound) + order.payments.create!({ + source: Spree::PaypalExpressCheckout.create({ + token: params[:token], + payer_id: params[:PayerID] + }), + amount: order.total, + payment_method: payment_method + }) + order.next + if order.complete? + flash.notice = Spree.t(:order_processed_successfully) + flash[:commerce_tracking] = "nothing special" + session[:order_id] = nil + redirect_to completion_route(order) + else + redirect_to checkout_state_path(order.state) + end + end + def cancel flash[:notice] = Spree.t('flash.cancel', scope: 'paypal') redirect_to main_app.checkout_path From 010c1c799d2f1dd9a2468b71ae72bd30b8604122 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 18 Nov 2020 14:55:29 +0000 Subject: [PATCH 04/12] Add some notes on paypal checkout flow --- app/controllers/spree/paypal_controller_decorator.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index a012978600..e454442a36 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -40,6 +40,9 @@ Spree::PaypalController.class_eval do begin pp_response = provider.set_express_checkout(pp_request) if pp_response.success? + # At this point Paypal has *provisionally* accepted that the payment can now be placed, + # and the user will be redirected to a Paypal payment page. On completion, the user is + # sent back and the response is handled in the #confirm action in this controller. redirect_to provider.express_checkout_url(pp_response, useraction: 'commit') else flash[:error] = Spree.t('flash.generic_error', scope: 'paypal', reasons: pp_response.errors.map(&:long_message).join(" ")) From 41a578783044b4e0d5d473b8e0e1eef600d48248 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 18 Nov 2020 15:39:01 +0000 Subject: [PATCH 05/12] Extract paypal response stubbing to helper --- .../consumer/shopping/checkout_paypal_spec.rb | 12 +++--------- spec/support/request/paypal_helper.rb | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 9 deletions(-) create mode 100644 spec/support/request/paypal_helper.rb diff --git a/spec/features/consumer/shopping/checkout_paypal_spec.rb b/spec/features/consumer/shopping/checkout_paypal_spec.rb index 107d425761..0a96e910cd 100644 --- a/spec/features/consumer/shopping/checkout_paypal_spec.rb +++ b/spec/features/consumer/shopping/checkout_paypal_spec.rb @@ -3,6 +3,7 @@ require "spec_helper" feature "Check out with Paypal", js: true do include ShopWorkflow include CheckoutHelper + include PaypalHelper let(:distributor) { create(:distributor_enterprise) } let(:supplier) { create(:supplier_enterprise) } @@ -41,20 +42,13 @@ feature "Check out with Paypal", js: true do add_product_to_cart order, product end - describe "as a guest" do + context "as a guest" do it "fails with an error message" do visit checkout_path checkout_as_guest fill_out_form(free_shipping.name, paypal.name, save_default_addresses: false) - paypal_response = double(:response, success?: false, errors: []) - paypal_provider = double( - :provider, - build_set_express_checkout: nil, - set_express_checkout: paypal_response - ) - allow_any_instance_of(Spree::PaypalController).to receive(:provider). - and_return(paypal_provider) + stub_paypal_response success: false place_order expect(page).to have_content "PayPal failed." diff --git a/spec/support/request/paypal_helper.rb b/spec/support/request/paypal_helper.rb new file mode 100644 index 0000000000..7f0ad8b5c2 --- /dev/null +++ b/spec/support/request/paypal_helper.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module PaypalHelper + def stub_paypal_response(options) + paypal_response = double(:response, success?: options[:success], errors: []) + paypal_provider = double( + :provider, + build_set_express_checkout: nil, + set_express_checkout: paypal_response + ) + allow_any_instance_of(Spree::PaypalController).to receive(:provider). + and_return(paypal_provider) + end +end From 21d67a0723c679a0bcc13fdcfe2e6c89baa0abb1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 19 Nov 2020 01:44:38 +0000 Subject: [PATCH 06/12] Extract some more paypal-specific test code to new helper --- spec/requests/checkout/paypal_spec.rb | 16 ++-------------- spec/support/request/paypal_helper.rb | 25 ++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/spec/requests/checkout/paypal_spec.rb b/spec/requests/checkout/paypal_spec.rb index e309d0ea5d..85ce495b9d 100644 --- a/spec/requests/checkout/paypal_spec.rb +++ b/spec/requests/checkout/paypal_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe "checking out an order with a paypal express payment method", type: :request do include ShopWorkflow + include PaypalHelper let!(:address) { create(:address) } let!(:shop) { create(:enterprise) } @@ -25,18 +26,6 @@ describe "checking out an order with a paypal express payment method", type: :re ) end let(:params) { { token: 'lalalala', PayerID: 'payer1', payment_method_id: payment_method.id } } - let(:mocked_xml_response) { - " - - - Success - Something - - s0metran$act10n - - - " - } before do order.reload.update_totals @@ -45,8 +34,7 @@ describe "checking out an order with a paypal express payment method", type: :re expect(order.next).to be true # => payment set_order order - stub_request(:post, "https://api-3t.sandbox.paypal.com/2.0/") - .to_return(status: 200, body: mocked_xml_response ) + stub_paypal_confirm end context "with a flat percent calculator" do diff --git a/spec/support/request/paypal_helper.rb b/spec/support/request/paypal_helper.rb index 7f0ad8b5c2..6796bb14b5 100644 --- a/spec/support/request/paypal_helper.rb +++ b/spec/support/request/paypal_helper.rb @@ -1,14 +1,37 @@ # frozen_string_literal: true module PaypalHelper + # Initial request to confirm the payment can be placed (before redirecting to paypal) def stub_paypal_response(options) paypal_response = double(:response, success?: options[:success], errors: []) paypal_provider = double( :provider, build_set_express_checkout: nil, - set_express_checkout: paypal_response + set_express_checkout: paypal_response, + express_checkout_url: options[:redirect] ) allow_any_instance_of(Spree::PaypalController).to receive(:provider). and_return(paypal_provider) end + + # Additional request to re-confirm the payment, when the order is finalised. + def stub_paypal_confirm + stub_request(:post, "https://api-3t.sandbox.paypal.com/2.0/") + .to_return(status: 200, body: mocked_xml_response ) + end + + private + + def mocked_xml_response + " + + + Success + Something + + s0metran$act10n + + + " + end end From 242c1a2715f1680a2e1080860594d58ffb71f99f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 19 Nov 2020 01:44:53 +0000 Subject: [PATCH 07/12] Add new Paypal feature spec --- .../consumer/shopping/checkout_paypal_spec.rb | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/spec/features/consumer/shopping/checkout_paypal_spec.rb b/spec/features/consumer/shopping/checkout_paypal_spec.rb index 0a96e910cd..fc710e77f3 100644 --- a/spec/features/consumer/shopping/checkout_paypal_spec.rb +++ b/spec/features/consumer/shopping/checkout_paypal_spec.rb @@ -3,6 +3,7 @@ require "spec_helper" feature "Check out with Paypal", js: true do include ShopWorkflow include CheckoutHelper + include AuthenticationHelper include PaypalHelper let(:distributor) { create(:distributor_enterprise) } @@ -35,6 +36,7 @@ feature "Check out with Paypal", js: true do distributor_ids: [distributor.id] ) end + let(:user) { create(:user) } before do distributor.shipping_methods << free_shipping @@ -54,4 +56,31 @@ feature "Check out with Paypal", js: true do expect(page).to have_content "PayPal failed." end end + + context "as a registered user" do + before { login_as user } + + it "completes the checkout after successful Paypal payment" do + visit checkout_path + fill_out_details + fill_out_form(free_shipping.name, paypal.name, save_default_addresses: false) + + # Normally the checkout would redirect to Paypal, a form would be filled out there, and the + # user would be redirected back to #confirm_paypal_path. Here we skip the PayPal part and + # jump straight to being redirected back to OFN with a "confirmed" payment. + stub_paypal_response( + success: true, + redirect: spree.confirm_paypal_path( + payment_method_id: paypal.id, token: "t123", PayerID: 'p123' + ) + ) + stub_paypal_confirm + + place_order + expect(page).to have_content "Your order has been processed successfully" + + expect(order.reload.state).to eq "complete" + expect(order.payments.count).to eq 1 + end + end end From 87df44764f0603beabbca74e887e12e0ff6a2c93 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 22 Nov 2020 17:50:43 +0000 Subject: [PATCH 08/12] Extract stock-check logic to controller concern and inject prior to final Paypal payment confirmation. --- app/controllers/checkout_controller.rb | 8 +------- app/controllers/concerns/order_stock_check.rb | 12 ++++++++++++ .../spree/paypal_controller_decorator.rb | 19 ++++++++++++------- 3 files changed, 25 insertions(+), 14 deletions(-) create mode 100644 app/controllers/concerns/order_stock_check.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 299a8738ff..a63d1db998 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -5,6 +5,7 @@ require 'open_food_network/address_finder' class CheckoutController < Spree::StoreController layout 'darkswarm' + include OrderStockCheck include CheckoutHelper include OrderCyclesHelper include EnterprisesHelper @@ -77,13 +78,6 @@ class CheckoutController < Spree::StoreController redirect_to main_app.cart_path if @order.completed? end - def ensure_sufficient_stock_lines - if @order.insufficient_stock_lines.present? - flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity) - redirect_to main_app.cart_path - end - end - def load_order @order = current_order diff --git a/app/controllers/concerns/order_stock_check.rb b/app/controllers/concerns/order_stock_check.rb new file mode 100644 index 0000000000..fc2346d5e8 --- /dev/null +++ b/app/controllers/concerns/order_stock_check.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module OrderStockCheck + extend ActiveSupport::Concern + + def ensure_sufficient_stock_lines + if @order.insufficient_stock_lines.present? + flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity) + redirect_to main_app.cart_path + end + end +end diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index e454442a36..3324863b95 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true Spree::PaypalController.class_eval do + include OrderStockCheck + before_action :enable_embedded_shopfront before_action :destroy_orphaned_paypal_payments, only: :confirm after_action :reset_order_when_complete, only: :confirm @@ -55,23 +57,26 @@ Spree::PaypalController.class_eval do end def confirm - order = current_order || raise(ActiveRecord::RecordNotFound) - order.payments.create!({ + @order = current_order || raise(ActiveRecord::RecordNotFound) + # At this point the user has come back from the Paypal form, and we get one + # last chance to interact with the payment process before the money moves... + ensure_sufficient_stock_lines + @order.payments.create!({ source: Spree::PaypalExpressCheckout.create({ token: params[:token], payer_id: params[:PayerID] }), - amount: order.total, + amount: @order.total, payment_method: payment_method }) - order.next - if order.complete? + @order.next + if @order.complete? flash.notice = Spree.t(:order_processed_successfully) flash[:commerce_tracking] = "nothing special" session[:order_id] = nil - redirect_to completion_route(order) + redirect_to completion_route(@order) else - redirect_to checkout_state_path(order.state) + redirect_to checkout_state_path(@order.state) end end From cabec7e73ff0c7c68f7c9f31c17f6e5c516288e5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 22 Nov 2020 17:57:01 +0000 Subject: [PATCH 09/12] Fix Rubocop warnings and tidy up --- app/controllers/concerns/order_stock_check.rb | 8 ++++---- app/controllers/spree/paypal_controller_decorator.rb | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/controllers/concerns/order_stock_check.rb b/app/controllers/concerns/order_stock_check.rb index fc2346d5e8..1578677fd4 100644 --- a/app/controllers/concerns/order_stock_check.rb +++ b/app/controllers/concerns/order_stock_check.rb @@ -4,9 +4,9 @@ module OrderStockCheck extend ActiveSupport::Concern def ensure_sufficient_stock_lines - if @order.insufficient_stock_lines.present? - flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity) - redirect_to main_app.cart_path - end + return true if @order.insufficient_stock_lines.blank? + + flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity) + redirect_to main_app.cart_path end end diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index 3324863b95..f28c75a761 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -60,7 +60,8 @@ Spree::PaypalController.class_eval do @order = current_order || raise(ActiveRecord::RecordNotFound) # At this point the user has come back from the Paypal form, and we get one # last chance to interact with the payment process before the money moves... - ensure_sufficient_stock_lines + return unless ensure_sufficient_stock_lines + @order.payments.create!({ source: Spree::PaypalExpressCheckout.create({ token: params[:token], From 2fa2a30c67a71fb7622f630e3dabd24861df5012 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 22 Nov 2020 18:37:31 +0000 Subject: [PATCH 10/12] Add spec coverage, refactor, avoid double-render errors :+1: --- app/controllers/checkout_controller.rb | 2 +- app/controllers/concerns/order_stock_check.rb | 10 ++++++++-- .../spree/paypal_controller_decorator.rb | 3 ++- spec/controllers/spree/paypal_controller_spec.rb | 14 ++++++++++++++ 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index a63d1db998..4cedfbbac9 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -25,7 +25,7 @@ class CheckoutController < Spree::StoreController before_action :ensure_order_not_completed before_action :ensure_checkout_allowed - before_action :ensure_sufficient_stock_lines + before_action :handle_insufficient_stock before_action :associate_user before_action :check_authorization diff --git a/app/controllers/concerns/order_stock_check.rb b/app/controllers/concerns/order_stock_check.rb index 1578677fd4..2adfaf67be 100644 --- a/app/controllers/concerns/order_stock_check.rb +++ b/app/controllers/concerns/order_stock_check.rb @@ -3,10 +3,16 @@ module OrderStockCheck extend ActiveSupport::Concern - def ensure_sufficient_stock_lines - return true if @order.insufficient_stock_lines.blank? + def handle_insufficient_stock + return if sufficient_stock? flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity) redirect_to main_app.cart_path end + + private + + def sufficient_stock? + @sufficient_stock ||= @order.insufficient_stock_lines.blank? + end end diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index f28c75a761..9d20713efa 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -58,9 +58,10 @@ Spree::PaypalController.class_eval do def confirm @order = current_order || raise(ActiveRecord::RecordNotFound) + # At this point the user has come back from the Paypal form, and we get one # last chance to interact with the payment process before the money moves... - return unless ensure_sufficient_stock_lines + return handle_insufficient_stock unless sufficient_stock? @order.payments.create!({ source: Spree::PaypalExpressCheckout.create({ diff --git a/spec/controllers/spree/paypal_controller_spec.rb b/spec/controllers/spree/paypal_controller_spec.rb index 9f630e892d..3182048ed4 100644 --- a/spec/controllers/spree/paypal_controller_spec.rb +++ b/spec/controllers/spree/paypal_controller_spec.rb @@ -27,6 +27,20 @@ module Spree spree_post :confirm, payment_method_id: payment_method.id expect(session[:access_token]).to eq(controller.current_order.token) end + + context "if the stock ran out whilst the payment was being placed" do + before do + allow(controller.current_order).to receive(:insufficient_stock_lines).and_return(true) + end + + it "redirects to the cart with out of stock error" do + expect(spree_post(:confirm, payment_method_id: payment_method.id)). + to redirect_to cart_path + + # And does not complete processing of the payment + expect(controller.current_order.reload.payments.count).to eq 0 + end + end end describe '#expire_current_order' do From c4cfc1dc05bf004222fa32af7b056e6ea9ab5d0a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Nov 2020 11:42:06 +0000 Subject: [PATCH 11/12] Improve order test in paypal controller spec --- spec/controllers/spree/paypal_controller_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/controllers/spree/paypal_controller_spec.rb b/spec/controllers/spree/paypal_controller_spec.rb index 3182048ed4..5e55a6d02d 100644 --- a/spec/controllers/spree/paypal_controller_spec.rb +++ b/spec/controllers/spree/paypal_controller_spec.rb @@ -37,8 +37,11 @@ module Spree expect(spree_post(:confirm, payment_method_id: payment_method.id)). to redirect_to cart_path - # And does not complete processing of the payment - expect(controller.current_order.reload.payments.count).to eq 0 + order = controller.current_order.reload + + # Order is in "cart" state and did not complete processing of the payment + expect(order.state).to eq "cart" + expect(order.payments.count).to eq 0 end end end From 0ba670b1807be507a6766122ddd65d90d4724d18 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 27 Nov 2020 11:42:46 +0000 Subject: [PATCH 12/12] Ensure order is cleanly reset to cart state when redirecting to cart --- app/controllers/spree/paypal_controller_decorator.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/paypal_controller_decorator.rb b/app/controllers/spree/paypal_controller_decorator.rb index 9d20713efa..71d559590a 100644 --- a/app/controllers/spree/paypal_controller_decorator.rb +++ b/app/controllers/spree/paypal_controller_decorator.rb @@ -61,7 +61,7 @@ Spree::PaypalController.class_eval do # At this point the user has come back from the Paypal form, and we get one # last chance to interact with the payment process before the money moves... - return handle_insufficient_stock unless sufficient_stock? + return reset_to_cart unless sufficient_stock? @order.payments.create!({ source: Spree::PaypalExpressCheckout.create({ @@ -114,6 +114,11 @@ Spree::PaypalController.class_eval do end end + def reset_to_cart + OrderCheckoutRestart.new(@order).call + handle_insufficient_stock + end + # See #1074 and #1837 for more detail on why we need this # An 'orphaned' Spree::Payment is created for every call to CheckoutController#update # for orders that are processed using a Spree::Gateway::PayPalExpress payment method