From af5ac2a25ec958d8c122830369e7a3be1ea5c865 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 24 Mar 2021 16:56:23 +1100 Subject: [PATCH 1/5] Extend test coverage of CheckoutController I'm covering the interaction with Stripe here to test for error cases in the future. --- spec/controllers/checkout_controller_spec.rb | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 56129a6119..c9d1a5f353 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe CheckoutController, type: :controller do + include StripeStubs + let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } let(:order_cycle) { create(:simple_order_cycle) } let(:order) { create(:order) } @@ -97,11 +99,27 @@ describe CheckoutController, type: :controller do end describe "when the order is in payment state and a stripe payment intent is provided" do + let(:order) { create(:order_with_totals) } + let(:payment_method) { create(:stripe_sca_payment_method) } + let(:payment) { + create( + :payment, + amount: order.total, + state: "pending", + payment_method: payment_method, + response_code: "pi_123" + ) + } + before do + allow(Stripe).to receive(:api_key) { "sk_test_12345" } + stub_payment_intent_get_request + stub_successful_capture_request(order: order) + order.update_attribute :state, "payment" order.ship_address = create(:address) order.save! - order.payments << create(:payment, state: "pending", response_code: "pi_123") + order.payments << payment # 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) From 9fabca134a573b057d99008bec29c1256e05f448 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 14:30:43 +1100 Subject: [PATCH 2/5] Fix and re-activate concurrency spec --- spec/controllers/checkout_controller_concurrency_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/controllers/checkout_controller_concurrency_spec.rb b/spec/controllers/checkout_controller_concurrency_spec.rb index 0804bc7492..3d718a0a07 100644 --- a/spec/controllers/checkout_controller_concurrency_spec.rb +++ b/spec/controllers/checkout_controller_concurrency_spec.rb @@ -42,12 +42,15 @@ describe CheckoutController, concurrency: true, type: :controller do allow(controller).to receive(:spree_current_user).and_return(order.user) allow(controller).to receive(:current_distributor).and_return(order.distributor) allow(controller).to receive(:current_order_cycle).and_return(order.order_cycle) + + # Avoid setting headers as Rails 5 is not thread-safe here: + allow(controller).to receive(:restrict_iframes) end # This spec does not seem to be running in two threads in Rails 5. There are errors for the # same response headers being set twice, possibly indicating that there is only one response # as opposed to two separate requests in two threads? - xit "handles two concurrent orders successfully" do + it "handles two concurrent orders successfully" do # New threads start running straight away. The breakpoint is after loading # the order and before advancing the order's state and making payments. breakpoint.lock From 4d242af007124fe75201ebff948067b8794449d7 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Mar 2021 14:32:08 +1100 Subject: [PATCH 3/5] Use pessimistic locking processing Stripe payment We used pessimistic locking around the `update` action already but when Stripe redirects back to us we complete the payment in the `edit` action. --- app/controllers/checkout_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 1b8cd07d67..a1c69500bf 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -14,7 +14,7 @@ class CheckoutController < ::BaseController # We need pessimistic locking to avoid race conditions. # Otherwise we fail on duplicate indexes or end up with negative stock. - prepend_around_action CurrentOrderLocker, only: :update + prepend_around_action CurrentOrderLocker, only: [:edit, :update] prepend_before_action :check_hub_ready_for_checkout prepend_before_action :check_order_cycle_expiry From cbd730f4eb835c464d221bdadc199d1f109af9ea Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 26 Mar 2021 11:04:58 +1100 Subject: [PATCH 4/5] Remove left-over comment This spec was fixed before and the comment was outdated. --- spec/controllers/checkout_controller_concurrency_spec.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/spec/controllers/checkout_controller_concurrency_spec.rb b/spec/controllers/checkout_controller_concurrency_spec.rb index 3d718a0a07..3ed70c4edf 100644 --- a/spec/controllers/checkout_controller_concurrency_spec.rb +++ b/spec/controllers/checkout_controller_concurrency_spec.rb @@ -47,9 +47,6 @@ describe CheckoutController, concurrency: true, type: :controller do allow(controller).to receive(:restrict_iframes) end - # This spec does not seem to be running in two threads in Rails 5. There are errors for the - # same response headers being set twice, possibly indicating that there is only one response - # as opposed to two separate requests in two threads? it "handles two concurrent orders successfully" do # New threads start running straight away. The breakpoint is after loading # the order and before advancing the order's state and making payments. From 4c1f977d266b4707568fc3bd693644813023c0d5 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 26 Mar 2021 12:02:26 +1100 Subject: [PATCH 5/5] Return thread syncing to concurrency spec Without syncing the two threads with a lock, the spec could accidentally pass even if the code is broken. It was accidentally removed during refactoring in b0fa1464f. --- spec/controllers/checkout_controller_concurrency_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/controllers/checkout_controller_concurrency_spec.rb b/spec/controllers/checkout_controller_concurrency_spec.rb index 3ed70c4edf..63860717e1 100644 --- a/spec/controllers/checkout_controller_concurrency_spec.rb +++ b/spec/controllers/checkout_controller_concurrency_spec.rb @@ -52,6 +52,12 @@ describe CheckoutController, concurrency: true, type: :controller do # the order and before advancing the order's state and making payments. breakpoint.lock + checkout_workflow_original = controller.method(:checkout_workflow) + expect(controller).to receive(:checkout_workflow) do |shipping_method_id| + breakpoint.synchronize {} + checkout_workflow_original.call(shipping_method_id) + end + # Starting two checkout threads. The controller code will determine if # these two threads are synchronised correctly or run into a race condition. #