From 90a1046106603e082d55f814ce3711bd26816b71 Mon Sep 17 00:00:00 2001 From: Edwin Cruz Date: Wed, 23 Oct 2019 16:09:34 -0600 Subject: [PATCH 1/6] Added eager loading to product variant images With the concept of galleries, the relationship changed and it now uses variant images --- backend/app/controllers/spree/admin/products_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/app/controllers/spree/admin/products_controller.rb b/backend/app/controllers/spree/admin/products_controller.rb index 27c99426050..01a1b82c5ba 100644 --- a/backend/app/controllers/spree/admin/products_controller.rb +++ b/backend/app/controllers/spree/admin/products_controller.rb @@ -120,7 +120,7 @@ def update_before end def product_includes - [{ variants: [:images], master: [:images, :default_price] }] + [:variant_images, { variants: [:images], master: [:images, :default_price] }] end def clone_object_url(resource) From 1e8499439b86fb208fd9a9455a8e82b64973e5da Mon Sep 17 00:00:00 2001 From: Edwin Cruz Date: Wed, 23 Oct 2019 16:11:44 -0600 Subject: [PATCH 2/6] Allowing resources controller to eager load objects This changes allows to specify eager loading in the belongs_to definition at the conrtoller level class Controller < Spree::Admin::ResourceController belongs_to :product, find_by: :slug, includes: {variant: :prices} end --- backend/app/controllers/spree/admin/resource_controller.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/app/controllers/spree/admin/resource_controller.rb b/backend/app/controllers/spree/admin/resource_controller.rb index 58e860e5061..4af04a5e081 100644 --- a/backend/app/controllers/spree/admin/resource_controller.rb +++ b/backend/app/controllers/spree/admin/resource_controller.rb @@ -123,6 +123,7 @@ def belongs_to(model_name, options = {}) @parent_data[:model_name] = model_name @parent_data[:model_class] = (options[:model_class] || model_name.to_s.classify.constantize) @parent_data[:find_by] = options[:find_by] || :id + @parent_data[:includes] = options[:includes] end end @@ -183,7 +184,9 @@ def parent_data def parent if parent? - @parent ||= self.class.parent_data[:model_class].find_by(self.class.parent_data[:find_by] => params["#{parent_model_name}_id"]) + @parent ||= self.class.parent_data[:model_class] + .includes(self.class.parent_data[:includes]) + .find_by(self.class.parent_data[:find_by] => params["#{parent_model_name}_id"]) instance_variable_set("@#{parent_model_name}", @parent) else Spree::Deprecation.warn "Calling #parent is deprecated on a ResourceController which has not defined a belongs_to" From 22c25cb483367baae0495b5f5dba97de4804ce8e Mon Sep 17 00:00:00 2001 From: Edwin Cruz Date: Wed, 23 Oct 2019 16:14:34 -0600 Subject: [PATCH 3/6] Eager loading product properties Calling product property name was causing query n+1, eager loading properties solved the problem --- .../controllers/spree/admin/product_properties_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/app/controllers/spree/admin/product_properties_controller.rb b/backend/app/controllers/spree/admin/product_properties_controller.rb index fe4a34da151..256d80d65a6 100644 --- a/backend/app/controllers/spree/admin/product_properties_controller.rb +++ b/backend/app/controllers/spree/admin/product_properties_controller.rb @@ -3,7 +3,7 @@ module Spree module Admin class ProductPropertiesController < ResourceController - belongs_to 'spree/product', find_by: :slug + belongs_to 'spree/product', find_by: :slug, includes: { product_properties: :property } before_action :find_properties before_action :setup_property, only: :index, if: -> { can?(:create, model_class) } before_action :setup_variant_property_rules, only: :index From 4dd5e6869fa6a2b9cb6f05c8ecc6d5f1493e687e Mon Sep 17 00:00:00 2001 From: Edwin Cruz Date: Wed, 23 Oct 2019 18:41:56 -0600 Subject: [PATCH 4/6] Eager loading taxons information in taxonomy endpoint We can eager load only the first level --- api/app/controllers/spree/api/taxonomies_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/app/controllers/spree/api/taxonomies_controller.rb b/api/app/controllers/spree/api/taxonomies_controller.rb index d5c3cf209a9..28e1815537f 100644 --- a/api/app/controllers/spree/api/taxonomies_controller.rb +++ b/api/app/controllers/spree/api/taxonomies_controller.rb @@ -58,7 +58,9 @@ def taxonomies end def taxonomy - @taxonomy ||= Spree::Taxonomy.accessible_by(current_ability, :read).find(params[:id]) + @taxonomy ||= Spree::Taxonomy.accessible_by(current_ability, :read). + includes(root: :children). + find(params[:id]) end def taxonomy_params From 10c4709f047d195aa151e89264d89e1cc18d864d Mon Sep 17 00:00:00 2001 From: Edwin Cruz Date: Wed, 23 Oct 2019 18:43:01 -0600 Subject: [PATCH 5/6] Eager loading order data in orders api endpoint It requires a lot of information to be rendered, this change tries to eager load as much data as possible, perfomance gains are notable in orders with multiple line items --- api/app/controllers/spree/api/orders_controller.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/app/controllers/spree/api/orders_controller.rb b/api/app/controllers/spree/api/orders_controller.rb index ecd9361a888..940bab0f7fa 100644 --- a/api/app/controllers/spree/api/orders_controller.rb +++ b/api/app/controllers/spree/api/orders_controller.rb @@ -168,7 +168,13 @@ def permitted_payment_attributes end def find_order(_lock = false) - @order = Spree::Order.find_by!(number: params[:id]) + @order = Spree::Order. + includes(line_items: [:adjustments, { variant: :images }], + payments: :payment_method, + shipments: { + shipping_rates: { shipping_method: :zones, taxes: :tax_rate } + }). + find_by!(number: params[:id]) end def order_id From 234a36de43a719397bd133d153fd51f4cb64fd95 Mon Sep 17 00:00:00 2001 From: Edwin Cruz Date: Thu, 24 Oct 2019 11:36:44 -0600 Subject: [PATCH 6/6] Updating eager loading in api orders index endpoint Updated eager loading relationships, it no renders big partial so no need to eager load extra information, created new relationship to fetch valid store credits payments so rails can load them automatically and also used previous relationship with .to_a instead of active record executing extra queries --- api/app/controllers/spree/api/orders_controller.rb | 7 +++---- api/app/views/spree/api/orders/_order.json.jbuilder | 2 +- core/app/models/concerns/spree/user_methods.rb | 2 +- core/app/models/spree/order.rb | 3 ++- core/spec/models/spree/concerns/user_methods_spec.rb | 3 ++- core/spec/models/spree/store_credit_spec.rb | 2 +- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/api/app/controllers/spree/api/orders_controller.rb b/api/app/controllers/spree/api/orders_controller.rb index 940bab0f7fa..45c7f33919a 100644 --- a/api/app/controllers/spree/api/orders_controller.rb +++ b/api/app/controllers/spree/api/orders_controller.rb @@ -55,10 +55,9 @@ def empty def index authorize! :index, Order orders_includes = [ - :user, - :payments, - :adjustments, - :line_items + { user: :store_credits }, + :line_items, + :valid_store_credit_payments ] @orders = paginate( Spree::Order diff --git a/api/app/views/spree/api/orders/_order.json.jbuilder b/api/app/views/spree/api/orders/_order.json.jbuilder index 27780ff2a0a..13990c78799 100644 --- a/api/app/views/spree/api/orders/_order.json.jbuilder +++ b/api/app/views/spree/api/orders/_order.json.jbuilder @@ -3,7 +3,7 @@ json.cache! [I18n.locale, order] do json.(order, *order_attributes) json.display_item_total(order.display_item_total.to_s) - json.total_quantity(order.line_items.sum(:quantity)) + json.total_quantity(order.line_items.to_a.sum(&:quantity)) json.display_total(order.display_total.to_s) json.display_ship_total(order.display_ship_total) json.display_tax_total(order.display_tax_total) diff --git a/core/app/models/concerns/spree/user_methods.rb b/core/app/models/concerns/spree/user_methods.rb index 23203c3bf84..21057dfb7dd 100644 --- a/core/app/models/concerns/spree/user_methods.rb +++ b/core/app/models/concerns/spree/user_methods.rb @@ -73,7 +73,7 @@ def total_available_store_credit deprecate total_available_store_credit: :available_store_credit_total, deprecator: Spree::Deprecation def available_store_credit_total(currency:) - store_credits.reload.to_a. + store_credits.to_a. select { |credit| credit.currency == currency }. sum(&:amount_remaining) end diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 72bd44ca516..d1965de926f 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -93,6 +93,7 @@ def states # Payments has_many :payments, dependent: :destroy, inverse_of: :order + has_many :valid_store_credit_payments, -> { store_credits.valid }, inverse_of: :order, class_name: 'Spree::Payment', foreign_key: :order_id # Returns has_many :return_authorizations, dependent: :destroy, inverse_of: :order @@ -686,7 +687,7 @@ def order_total_after_store_credit def total_applicable_store_credit if can_complete? || complete? - payments.store_credits.valid.sum(:amount) + valid_store_credit_payments.to_a.sum(&:amount) else [total, (user.try(:available_store_credit_total, currency: currency) || 0.0)].min end diff --git a/core/spec/models/spree/concerns/user_methods_spec.rb b/core/spec/models/spree/concerns/user_methods_spec.rb index a2cd85d777e..63abb1c82ad 100644 --- a/core/spec/models/spree/concerns/user_methods_spec.rb +++ b/core/spec/models/spree/concerns/user_methods_spec.rb @@ -62,7 +62,7 @@ describe '#available_store_credit_total' do subject do - test_user.available_store_credit_total(currency: 'USD') + test_user.reload.available_store_credit_total(currency: 'USD') end context 'when the user does not have any credit' do @@ -95,6 +95,7 @@ context 'with credits of multiple currencies' do let!(:credit_3) { create(:store_credit, user: test_user, amount: 400, currency: 'GBP') } + before { test_user.reload } it 'separates the currencies' do expect(test_user.available_store_credit_total(currency: 'USD')).to eq(100 + 200) diff --git a/core/spec/models/spree/store_credit_spec.rb b/core/spec/models/spree/store_credit_spec.rb index f38ef98a804..badc26750b6 100644 --- a/core/spec/models/spree/store_credit_spec.rb +++ b/core/spec/models/spree/store_credit_spec.rb @@ -777,7 +777,7 @@ let!(:store_credits) do [ create(:store_credit, user: user, amount: store_credit_amount), - create(:store_credit, user: user, amount: additional_store_credit_amount) + create(:store_credit, user: user.reload, amount: additional_store_credit_amount) ] end