From 00862844d57c2f46762d5d4140302838d161e79c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 14 Nov 2021 11:39:36 +0000 Subject: [PATCH 1/6] Memoize Checkout#valid_payment_intent_provided? --- app/controllers/checkout_controller.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 6b0f08ba99..8567f30502 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -148,12 +148,14 @@ class CheckoutController < ::BaseController end def valid_payment_intent_provided? - return false unless params["payment_intent"]&.starts_with?("pi_") + @valid_payment_intent_provided ||= begin + return false unless params["payment_intent"]&.starts_with?("pi_") - last_payment = OrderPaymentFinder.new(@order).last_payment - @order.state == "payment" && - last_payment&.state == "requires_authorization" && - last_payment&.response_code == params["payment_intent"] + last_payment = OrderPaymentFinder.new(@order).last_payment + @order.state == "payment" && + last_payment&.state == "requires_authorization" && + last_payment&.response_code == params["payment_intent"] + end end def handle_redirect_from_stripe From 48acc0ecd031bc403450ba1e1f0cbea1e31d72de Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 14 Nov 2021 12:34:49 +0000 Subject: [PATCH 2/6] Add Bugsnag notice when an OC closes during checkout completion This info could be useful to know whilst debugging order completion issues. --- app/controllers/application_controller.rb | 2 ++ spec/controllers/base_controller_spec.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a3c4433397..af057db34e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -153,9 +153,11 @@ class ApplicationController < ActionController::Base def check_order_cycle_expiry if current_order_cycle&.closed? + Bugsnag.notify("Notice: order cycle closed during checkout completion", order: current_order) current_order.empty! current_order.set_order_cycle! nil flash[:info] = I18n.t('order_cycle_closed') + redirect_to main_app.shop_path end end diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index 02b68b3750..98d6183ea4 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -102,7 +102,7 @@ describe BaseController, type: :controller do it "redirects to shopfront with message if order cycle is expired" do expect(controller).to receive(:current_order_cycle).and_return(oc) - expect(controller).to receive(:current_order).and_return(order).twice + expect(controller).to receive(:current_order).and_return(order).at_least(:twice) expect(oc).to receive(:closed?).and_return(true) expect(order).to receive(:empty!) expect(order).to receive(:set_order_cycle!).with(nil) From 8d0cbe886aa29137b971d5e76346c9ff2e5b7dcc Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 14 Nov 2021 12:38:53 +0000 Subject: [PATCH 3/6] Rename method for clarity --- app/controllers/checkout_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 8567f30502..6c22089cb4 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -81,14 +81,14 @@ class CheckoutController < ::BaseController def load_order @order = current_order - redirect_to(main_app.shop_path) && return if redirect_to_shop? + redirect_to(main_app.shop_path) && return if order_invalid_for_checkout? handle_invalid_stock && return unless valid_order_line_items? before_address setup_for_current_state end - def redirect_to_shop? + def order_invalid_for_checkout? !@order || !@order.checkout_allowed? || @order.completed? From 1bf4e6fa56f2f3862daecfb4ecd9350024410a10 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 14 Nov 2021 12:41:14 +0000 Subject: [PATCH 4/6] Reorder conditions for performance A little micro-optimisation: `@order.checkout_allowed?` requires a database query, whereas `@order.completed?` does not. So in cases where the order is completed we can return early here before hitting the database. --- app/controllers/checkout_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 6c22089cb4..cc7ba97144 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -89,9 +89,7 @@ class CheckoutController < ::BaseController end def order_invalid_for_checkout? - !@order || - !@order.checkout_allowed? || - @order.completed? + !@order || @order.completed? || !@order.checkout_allowed? end def valid_order_line_items? From bbb47964db117a9439afc7d2861d8e3b1a5db6e2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 14 Nov 2021 12:47:21 +0000 Subject: [PATCH 5/6] Notify Bugsnag if the order loaded at checkout is invalid after being redirected back from Stripe during payment processing. --- app/controllers/checkout_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index cc7ba97144..d4c36f46ff 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -81,7 +81,11 @@ class CheckoutController < ::BaseController def load_order @order = current_order - redirect_to(main_app.shop_path) && return if order_invalid_for_checkout? + if order_invalid_for_checkout? + Bugsnag.notify("Notice: invalid order loaded during Stripe processing", order: @order) if valid_payment_intent_provided? + redirect_to(main_app.shop_path) && return + end + handle_invalid_stock && return unless valid_order_line_items? before_address From ac5766b00bcedbf4fa91080e4389cc705512e974 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 14 Nov 2021 20:01:32 +0000 Subject: [PATCH 6/6] Report current order info on all checkout failures --- 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 d4c36f46ff..65a59efd72 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -241,7 +241,7 @@ class CheckoutController < ::BaseController end def checkout_failed(error = RuntimeError.new(order_error)) - Bugsnag.notify(error) + Bugsnag.notify(error, order: @order) flash[:error] = order_error if flash.blank? Checkout::PostCheckoutActions.new(@order).failure end