From 5e6dd1e6e1facc47b5fdf581ea335c6d57530c3a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 29 Jan 2022 12:10:55 +0000 Subject: [PATCH 1/3] Move #check_order_cycle_expiry method to OrderStockCheck and don't call it from BaseController :before_action callback --- app/controllers/application_controller.rb | 11 ----------- app/controllers/base_controller.rb | 1 - app/controllers/concerns/order_stock_check.rb | 11 +++++++++++ spec/controllers/base_controller_spec.rb | 13 ------------- spec/controllers/checkout_controller_spec.rb | 14 ++++++++++++++ 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0b8b45d19f..d62aa2b9d9 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -139,17 +139,6 @@ class ApplicationController < ActionController::Base !current_distributor.ready_for_checkout? end - 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 - # All render calls within the block will be performed with the specified format # Useful for rendering html within a JSON response, particularly if the specified # template or partial then goes on to render further partials without specifying diff --git a/app/controllers/base_controller.rb b/app/controllers/base_controller.rb index beaf68ef28..7816066b8b 100644 --- a/app/controllers/base_controller.rb +++ b/app/controllers/base_controller.rb @@ -12,7 +12,6 @@ class BaseController < ApplicationController include OrderCyclesHelper before_action :set_locale - before_action :check_order_cycle_expiry private diff --git a/app/controllers/concerns/order_stock_check.rb b/app/controllers/concerns/order_stock_check.rb index a3c09d1838..3a46ee9ea0 100644 --- a/app/controllers/concerns/order_stock_check.rb +++ b/app/controllers/concerns/order_stock_check.rb @@ -18,6 +18,17 @@ module OrderStockCheck redirect_to main_app.cart_path end + 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 + private def sufficient_stock? diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index 98d6183ea4..edad99c70b 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -99,17 +99,4 @@ describe BaseController, type: :controller do controller.current_order(true) end end - - 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).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) - - get :index - - expect(response).to redirect_to shop_url - expect(flash[:info]).to eq I18n.t('order_cycle_closed') - end end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 844ffac75e..342c6444ee 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -25,6 +25,20 @@ describe CheckoutController, type: :controller do expect(response).to redirect_to shop_path end + it "redirects to shopfront with message if order cycle is expired" do + allow(controller).to receive(:current_distributor).and_return(distributor) + expect(controller).to receive(:current_order_cycle).and_return(order_cycle).at_least(:once) + expect(controller).to receive(:current_order).and_return(order).at_least(:once) + expect(order_cycle).to receive(:closed?).and_return(true) + expect(order).to receive(:empty!) + expect(order).to receive(:set_order_cycle!).with(nil) + + get :edit + + expect(response).to redirect_to shop_url + expect(flash[:info]).to eq I18n.t('order_cycle_closed') + end + it "redirects home with message if hub is not ready for checkout" do allow(distributor).to receive(:ready_for_checkout?) { false } allow(order).to receive_messages(distributor: distributor, order_cycle: order_cycle) From 11ed1574cae96983dbc51cf414863b91d727242b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 29 Jan 2022 12:15:30 +0000 Subject: [PATCH 2/3] Call #check_order_cycle_expiry in PaypalController and StripeController, but avoid it on #authorize action The authorize action is used for authorizing off-session payments where the order is *already complete* and the order cycle may have closed (backoffice and subscriptions). They are essentially asynchronous and not coupled to the current open/closed state of the order cycle. --- .../payment_gateways/paypal_controller.rb | 1 + .../payment_gateways/stripe_controller.rb | 1 + .../stripe_controller_spec.rb | 31 +++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/app/controllers/payment_gateways/paypal_controller.rb b/app/controllers/payment_gateways/paypal_controller.rb index 4519ef74e2..c0c41fd77d 100644 --- a/app/controllers/payment_gateways/paypal_controller.rb +++ b/app/controllers/payment_gateways/paypal_controller.rb @@ -8,6 +8,7 @@ module PaymentGateways before_action :destroy_orphaned_paypal_payments, only: :confirm before_action :load_checkout_order, only: [:express, :confirm] before_action :handle_insufficient_stock, only: [:express, :confirm] + before_action :check_order_cycle_expiry, only: [:express, :confirm] before_action :permit_parameters! after_action :reset_order_when_complete, only: :confirm diff --git a/app/controllers/payment_gateways/stripe_controller.rb b/app/controllers/payment_gateways/stripe_controller.rb index fc9cb1dd93..6cbd3ee871 100644 --- a/app/controllers/payment_gateways/stripe_controller.rb +++ b/app/controllers/payment_gateways/stripe_controller.rb @@ -7,6 +7,7 @@ module PaymentGateways before_action :load_checkout_order, only: :confirm before_action :validate_payment_intent, only: :confirm + before_action :check_order_cycle_expiry, only: :confirm before_action :validate_stock, only: :confirm def confirm diff --git a/spec/controllers/payment_gateways/stripe_controller_spec.rb b/spec/controllers/payment_gateways/stripe_controller_spec.rb index d1819cccfb..fe68206148 100644 --- a/spec/controllers/payment_gateways/stripe_controller_spec.rb +++ b/spec/controllers/payment_gateways/stripe_controller_spec.rb @@ -70,6 +70,22 @@ module PaymentGateways }.to change { Customer.count }.by(1) end + context "when the order cycle has closed" do + it "redirects to shopfront with message if order cycle is expired" do + allow(controller).to receive(:current_distributor).and_return(distributor) + expect(controller).to receive(:current_order_cycle).and_return(order_cycle) + expect(controller).to receive(:current_order).and_return(order).at_least(:once) + expect(order_cycle).to receive(:closed?).and_return(true) + expect(order).to receive(:empty!) + expect(order).to receive(:set_order_cycle!).with(nil) + + get :confirm, params: { payment_intent: "pi_123" } + + expect(response).to redirect_to shop_url + expect(flash[:info]).to eq I18n.t('order_cycle_closed') + end + end + context "using split checkout" do before do allow(Flipper).to receive(:enabled?).with(:split_checkout) { true } @@ -236,6 +252,21 @@ module PaymentGateways expect(payment.cvv_response_message).to be nil end end + + context "when the order cycle has closed" do + it "should still authorize the payment successfully" do + expect(order).to receive(:process_payments!) do + payment.complete! + end + + get :authorize, params: { order_number: order.number, payment_intent: payment_intent } + + expect(response).to redirect_to order_path(order) + payment.reload + expect(payment.state).to eq("completed") + expect(payment.cvv_response_message).to be nil + end + end end context "when the payment intent response has errors" do From 5b434ca7c40f3b793b77597586bda9ba341cc1cb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 29 Jan 2022 12:57:51 +0000 Subject: [PATCH 3/3] Replace conditional with a guard clause --- app/controllers/concerns/order_stock_check.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/concerns/order_stock_check.rb b/app/controllers/concerns/order_stock_check.rb index 3a46ee9ea0..bcbb9fb87d 100644 --- a/app/controllers/concerns/order_stock_check.rb +++ b/app/controllers/concerns/order_stock_check.rb @@ -19,14 +19,14 @@ module OrderStockCheck end 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') + return unless current_order_cycle&.closed? - redirect_to main_app.shop_path - end + 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 private