From d62a984939af544ae57da588d790b3a835f7b097 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 17 Feb 2021 14:02:42 -0800 Subject: [PATCH 1/3] make orders not capturable if they have a payment pending authorization --- app/serializers/api/admin/order_serializer.rb | 4 +- .../api/admin/order_serializer_spec.rb | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/app/serializers/api/admin/order_serializer.rb b/app/serializers/api/admin/order_serializer.rb index e5e3a36510..2fac0128af 100644 --- a/app/serializers/api/admin/order_serializer.rb +++ b/app/serializers/api/admin/order_serializer.rb @@ -39,8 +39,8 @@ module Api end def ready_to_capture - pending_payment = object.pending_payments.first - object.payment_required? && pending_payment + pending_payment = object.pending_payments.reject(&:authorization_action_required?).first + object.payment_required? && pending_payment.present? end def ready_to_ship diff --git a/spec/serializers/api/admin/order_serializer_spec.rb b/spec/serializers/api/admin/order_serializer_spec.rb index a29db28d24..af08a20006 100644 --- a/spec/serializers/api/admin/order_serializer_spec.rb +++ b/spec/serializers/api/admin/order_serializer_spec.rb @@ -28,4 +28,43 @@ describe Api::Admin::OrderSerializer do end end end + + describe '#ready_to_capture' do + let(:order) { create(:order) } + + before do + allow(order).to receive(:payment_required?) { true } + end + + context "there is a payment pending authorization" do + let!(:pending_payment) { + create( + :payment, + order: order, + state: 'pending', + amount: 123.45, + cvv_response_message: "https://stripe.com/redirect" + ) + } + + it "returns false if there is a payment requiring authorization" do + expect(serializer.ready_to_capture).to be false + end + end + + context "there is a pending payment but it does not require authorization" do + let!(:pending_payment) { + create( + :payment, + order: order, + state: 'pending', + amount: 123.45, + ) + } + + it "returns true if there is no payment requiring authorization" do + expect(serializer.ready_to_capture).to be true + end + end + end end From 20a7f2f24e34f79e62a52278a9da8ba375273774 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 23 Feb 2021 17:02:37 +0100 Subject: [PATCH 2/3] Eager load payment and subs. order associations This removes the N+1 queries caused by `Api::Admin::OrderSerialier#ready_to_capture` when used from `Api::OrdersController#index`. While it's fine for the single-order controller actions, it's not for this one that deals with a collection of orders. Fortunately, `SearchOrders` is used only in this controller action so we can put the `includes` calls there, otherwise, we would need to refactor it a bit to pass in a context-specific AR relation. --- app/serializers/api/admin/order_serializer.rb | 6 ++- app/services/search_orders.rb | 4 +- .../controllers/api/orders_controller_spec.rb | 41 +++++++++++++++++++ .../api/admin/order_serializer_spec.rb | 14 +++---- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/app/serializers/api/admin/order_serializer.rb b/app/serializers/api/admin/order_serializer.rb index 2fac0128af..d3ebe81bba 100644 --- a/app/serializers/api/admin/order_serializer.rb +++ b/app/serializers/api/admin/order_serializer.rb @@ -38,9 +38,11 @@ module Api spree_routes_helper.admin_order_payments_path(object) end + # This methods requires to eager load the payment association (with its required WHERE + # constraints) so as not to cause and N+1. def ready_to_capture - pending_payment = object.pending_payments.reject(&:authorization_action_required?).first - object.payment_required? && pending_payment.present? + pending_payments = object.pending_payments.reject(&:authorization_action_required?) + object.payment_required? && pending_payments.any? end def ready_to_ship diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb index 351dccddb2..ab261962bb 100644 --- a/app/services/search_orders.rb +++ b/app/services/search_orders.rb @@ -13,7 +13,9 @@ class SearchOrders attr_reader :params, :current_user def fetch_orders - @search = search_query.ransack(params[:q]) + @search = search_query. + includes(:payments, :subscription). + ransack(params[:q]) return paginated_results if using_pagination? diff --git a/spec/controllers/api/orders_controller_spec.rb b/spec/controllers/api/orders_controller_spec.rb index 6b182578cd..0b8f4b2d30 100644 --- a/spec/controllers/api/orders_controller_spec.rb +++ b/spec/controllers/api/orders_controller_spec.rb @@ -157,6 +157,47 @@ module Api expect(json_response['pagination']).to eq pagination_data end end + + context "when there is a pending payment requiring authorization" do + let!(:pending_payment) do + create( + :payment, + order: order1, + state: 'pending', + amount: 123.45, + cvv_response_message: "https://stripe.com/redirect" + ) + end + + before do + allow(controller).to receive(:spree_current_user) { distributor.owner } + end + + it "returns false" do + get :index + expect(json_response['orders'].first['ready_to_capture']).to eq(false) + end + end + + context "when there is a pending payment but it does not require authorization" do + let!(:pending_payment) do + create( + :payment, + order: order1, + state: 'pending', + amount: 123.45, + ) + end + + before do + allow(controller).to receive(:spree_current_user) { distributor.owner } + end + + it "returns true" do + get :index + expect(json_response['orders'].first['ready_to_capture']).to eq(true) + end + end end describe "#show" do diff --git a/spec/serializers/api/admin/order_serializer_spec.rb b/spec/serializers/api/admin/order_serializer_spec.rb index af08a20006..644ec45dd2 100644 --- a/spec/serializers/api/admin/order_serializer_spec.rb +++ b/spec/serializers/api/admin/order_serializer_spec.rb @@ -36,8 +36,8 @@ describe Api::Admin::OrderSerializer do allow(order).to receive(:payment_required?) { true } end - context "there is a payment pending authorization" do - let!(:pending_payment) { + context "there is a pending payment requiring authorization" do + let!(:pending_payment) do create( :payment, order: order, @@ -45,24 +45,24 @@ describe Api::Admin::OrderSerializer do amount: 123.45, cvv_response_message: "https://stripe.com/redirect" ) - } + end - it "returns false if there is a payment requiring authorization" do + it "returns false" do expect(serializer.ready_to_capture).to be false end end context "there is a pending payment but it does not require authorization" do - let!(:pending_payment) { + let!(:pending_payment) do create( :payment, order: order, state: 'pending', amount: 123.45, ) - } + end - it "returns true if there is no payment requiring authorization" do + it "returns true" do expect(serializer.ready_to_capture).to be true end end From 116109c63dfaf504c8846e4a5c81c5c12d227b9d Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 23 Feb 2021 17:13:03 +0100 Subject: [PATCH 3/3] Make /api/orders N+1 free With the help of the bullet gem, and since we remove a couple of N+1s already, remove them all was just a few keystrokes away. This commits gets us from 42 SQL queries to 17, and 364.5ms to 253.9ms on my machine where I just have the sample data's orders. As usual, this will have a much bigger impact in scenarios with more data. --- app/services/search_orders.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/search_orders.rb b/app/services/search_orders.rb index ab261962bb..8e96cfbfa0 100644 --- a/app/services/search_orders.rb +++ b/app/services/search_orders.rb @@ -14,7 +14,7 @@ class SearchOrders def fetch_orders @search = search_query. - includes(:payments, :subscription). + includes(:payments, :subscription, :shipments, :bill_address, :distributor, :order_cycle). ransack(params[:q]) return paginated_results if using_pagination?