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] 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