From cf712e947878416d3f11d2fe67408cdefa9e5945 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 9 Apr 2020 13:57:24 +0200 Subject: [PATCH 1/6] Select only enterprise id --- app/controllers/admin/customers_controller.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index a2109c3fca..902b458dab 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -64,8 +64,10 @@ module Admin def collection return Customer.where("1=0") unless json_request? && params[:enterprise_id].present? - enterprise = Enterprise.managed_by(spree_current_user).find_by_id(params[:enterprise_id]) - Customer.of(enterprise) + enterprise_id = Enterprise.managed_by(spree_current_user). + select('enterprises.id').find_by_id(params[:enterprise_id]) + + Customer.of(enterprise_id) end def load_managed_shops From 31a54e49c5c95281df019cfbe1593232886eaeaa Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 9 Apr 2020 14:06:18 +0200 Subject: [PATCH 2/6] Allow User#default_card to work with eager-loading --- app/models/spree/user.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index 135e54d306..63d51f657c 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -125,7 +125,12 @@ module Spree end def default_card - credit_cards.where(is_default: true).first + # Don't re-fetch associated cards from the DB if they're already eager-loaded + if credit_cards.loaded? + credit_cards.to_a.find(&:is_default) + else + credit_cards.where(is_default: true).first + end end # Checks whether the specified user is a superadmin, with full control of the From 2a8809e6e8fed014ed7be093f4e7d99be5f73a42 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 9 Apr 2020 14:06:49 +0200 Subject: [PATCH 3/6] Eager-load default card in customer serializer --- app/controllers/admin/customers_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index 902b458dab..8143df7ee2 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -67,7 +67,7 @@ module Admin enterprise_id = Enterprise.managed_by(spree_current_user). select('enterprises.id').find_by_id(params[:enterprise_id]) - Customer.of(enterprise_id) + Customer.of(enterprise_id).includes(user: :credit_cards) end def load_managed_shops From e53f733966ab51560b3db513d2e343bf38be8258 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 9 Apr 2020 14:48:00 +0200 Subject: [PATCH 4/6] Eager-load addresses in customer serializer --- app/controllers/admin/customers_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index 8143df7ee2..680248e94f 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -67,7 +67,7 @@ module Admin enterprise_id = Enterprise.managed_by(spree_current_user). select('enterprises.id').find_by_id(params[:enterprise_id]) - Customer.of(enterprise_id).includes(user: :credit_cards) + Customer.of(enterprise_id).includes(:bill_address, :ship_address, user: :credit_cards) end def load_managed_shops From 4c41c84cc1688670e6dde958d366eb39e28d9139 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 9 Apr 2020 16:28:12 +0200 Subject: [PATCH 5/6] Refactor tag rules loading for customers Fixes N+1 queries on customer tags --- app/controllers/admin/customers_controller.rb | 32 ++++++++++++++++--- .../api/admin/customer_serializer.rb | 12 +++++-- .../api/admin/customer_serializer_spec.rb | 15 +++++++-- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index 680248e94f..91eda7d4a7 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -17,8 +17,9 @@ module Admin respond_to do |format| format.html format.json do - tag_rule_mapping = TagRule.mapping_for(Enterprise.where(id: params[:enterprise_id])) - render_as_json @collection, tag_rule_mapping: tag_rule_mapping + render_as_json @collection, + tag_rule_mapping: tag_rule_mapping, + customer_tags: customer_tags_by_id end end end @@ -64,10 +65,13 @@ module Admin def collection return Customer.where("1=0") unless json_request? && params[:enterprise_id].present? - enterprise_id = Enterprise.managed_by(spree_current_user). - select('enterprises.id').find_by_id(params[:enterprise_id]) + Customer.of(managed_enterprise_id). + includes(:bill_address, :ship_address, user: :credit_cards) + end - Customer.of(enterprise_id).includes(:bill_address, :ship_address, user: :credit_cards) + def managed_enterprise_id + @managed_enterprise_id ||= Enterprise.managed_by(spree_current_user). + select('enterprises.id').find_by_id(params[:enterprise_id]) end def load_managed_shops @@ -82,5 +86,23 @@ module Admin def ams_prefix_whitelist [:subscription] end + + def tag_rule_mapping + TagRule.mapping_for(Enterprise.where(id: managed_enterprise_id)) + end + + # Fetches tags for all customers of the enterprise and returns a hash indexed by customer_id + def customer_tags_by_id + customer_tags = ::ActsAsTaggableOn::Tag. + joins(:taggings). + includes(:taggings). + where(taggings: { taggable_id: Customer.of(managed_enterprise_id), context: 'tags' }) + + customer_tags.each_with_object({}) do |tag, indexed_hash| + customer_id = tag.taggings.first.taggable_id + indexed_hash[customer_id] ||= [] + indexed_hash[customer_id] << tag.name + end + end end end diff --git a/app/serializers/api/admin/customer_serializer.rb b/app/serializers/api/admin/customer_serializer.rb index 070e7927da..7648104600 100644 --- a/app/serializers/api/admin/customer_serializer.rb +++ b/app/serializers/api/admin/customer_serializer.rb @@ -6,7 +6,7 @@ class Api::Admin::CustomerSerializer < ActiveModel::Serializer has_one :bill_address, serializer: Api::AddressSerializer def tag_list - object.tag_list.join(",") + customer_tag_list.join(",") end def name @@ -14,7 +14,7 @@ class Api::Admin::CustomerSerializer < ActiveModel::Serializer end def tags - object.tag_list.map do |tag| + customer_tag_list.map do |tag| tag_rule_map = options[:tag_rule_mapping].andand[tag] tag_rule_map || { text: tag, rules: nil } end @@ -25,4 +25,12 @@ class Api::Admin::CustomerSerializer < ActiveModel::Serializer object.user.default_card.present? end + + private + + def customer_tag_list + return object.tag_list unless options[:customer_tags] + + options[:customer_tags].andand[object.id] || [] + end end diff --git a/spec/serializers/api/admin/customer_serializer_spec.rb b/spec/serializers/api/admin/customer_serializer_spec.rb index 02c97e3948..0d9b1625d9 100644 --- a/spec/serializers/api/admin/customer_serializer_spec.rb +++ b/spec/serializers/api/admin/customer_serializer_spec.rb @@ -1,12 +1,16 @@ require 'spec_helper' describe Api::Admin::CustomerSerializer do - let(:customer) { create(:customer, tag_list: "one, two, three") } + let(:tag_list) { ["one", "two", "three"] } + let(:customer) { create(:customer, tag_list: tag_list) } let!(:tag_rule) { create(:tag_rule, enterprise: customer.enterprise, preferred_customer_tags: "two") } it "serializes a customer with tags" do tag_rule_mapping = TagRule.mapping_for(Enterprise.where(id: customer.enterprise_id)) - serializer = Api::Admin::CustomerSerializer.new customer, tag_rule_mapping: tag_rule_mapping + customer_tag_list = { customer.id => tag_list } + serializer = Api::Admin::CustomerSerializer.new customer, + tag_rule_mapping: tag_rule_mapping, + customer_tags: customer_tag_list result = JSON.parse(serializer.to_json) expect(result['email']).to eq customer.email tags = result['tags'] @@ -27,4 +31,11 @@ describe Api::Admin::CustomerSerializer do expect(tag['rules']).to be nil end end + + it 'serializes a customer without customer_tags' do + serializer = Api::Admin::CustomerSerializer.new customer + result = JSON.parse(serializer.to_json) + + expect(result['tags'].first['text']).to eq tag_list.first + end end From a6414b6dbed387aa0cb738ac6ffc4fb7a10b8fe0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 10 Apr 2020 20:14:14 +0200 Subject: [PATCH 6/6] Make sure taggable_type is 'Customer' when querying customer tags --- app/controllers/admin/customers_controller.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index 91eda7d4a7..dac3b209dc 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -96,7 +96,10 @@ module Admin customer_tags = ::ActsAsTaggableOn::Tag. joins(:taggings). includes(:taggings). - where(taggings: { taggable_id: Customer.of(managed_enterprise_id), context: 'tags' }) + where(taggings: + { taggable_type: 'Customer', + taggable_id: Customer.of(managed_enterprise_id), + context: 'tags' }) customer_tags.each_with_object({}) do |tag, indexed_hash| customer_id = tag.taggings.first.taggable_id