Gather issuable metadata to avoid n+ queries on index viewissue_25900_2

11 files changed, 78 insertions, 36 deletions

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 6247934f81e..a6e158ebae6 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -9,6 +9,28 @@ module IssuableCollections
 
   private
 
+  def issuable_meta_data(issuable_collection)
+    # map has to be used here since using pluck or select will
+    # throw an error when ordering issuables by priority which inserts
+    # a new order into the collection.
+    # We cannot use reorder to not mess up the paginated collection.
+    issuable_ids         = issuable_collection.map(&:id)
+    issuable_note_count  = Note.count_for_collection(issuable_ids, @collection_type)
+    issuable_votes_count = AwardEmoji.votes_for_collection(issuable_ids, @collection_type)
+
+    issuable_ids.each_with_object({}) do |id, issuable_meta|
+      downvotes = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.downvote? }
+      upvotes   = issuable_votes_count.find { |votes| votes.awardable_id == id && votes.upvote? }
+      notes     = issuable_note_count.find  { |notes| notes.noteable_id == id }
+
+      issuable_meta[id] = Issuable::IssuableMeta.new(
+        upvotes.try(:count).to_i,
+        downvotes.try(:count).to_i,
+        notes.try(:count).to_i
+      )
+    end
+  end
+
   def issues_collection
     issues_finder.execute.preload(:project, :author, :assignee, :labels, :milestone, project: :namespace)
   end
diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb
index b46adcceb60..fb5edb34370 100644
--- a/app/controllers/concerns/issues_action.rb
+++ b/app/controllers/concerns/issues_action.rb
@@ -9,6 +9,9 @@ module IssuesAction
               .non_archived
               .page(params[:page])
 
+    @collection_type    = "Issue"
+    @issuable_meta_data = issuable_meta_data(@issues)
+
     respond_to do |format|
       format.html
       format.atom { render layout: false }
diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb
index fdb05bb3228..6229759dcf1 100644
--- a/app/controllers/concerns/merge_requests_action.rb
+++ b/app/controllers/concerns/merge_requests_action.rb
@@ -7,6 +7,9 @@ module MergeRequestsAction
 
     @merge_requests = merge_requests_collection
                       .page(params[:page])
+
+    @collection_type    = "MergeRequest"
+    @issuable_meta_data = issuable_meta_data(@merge_requests)
   end
 
   private
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index c75b8987a4b..744a4af1c51 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -23,8 +23,11 @@ class Projects::IssuesController < Projects::ApplicationController
   respond_to :html
 
   def index
-    @issues = issues_collection
-    @issues = @issues.page(params[:page])
+    @collection_type    = "Issue"
+    @issues             = issues_collection
+    @issues             = @issues.page(params[:page])
+    @issuable_meta_data = issuable_meta_data(@issues)
+
     if @issues.out_of_range? && @issues.total_pages != 0
       return redirect_to url_for(params.merge(page: @issues.total_pages))
     end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index fbad66c5c40..c3e1760f168 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -36,8 +36,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts]
 
   def index
-    @merge_requests = merge_requests_collection
-    @merge_requests = @merge_requests.page(params[:page])
+    @collection_type    = "MergeRequest"
+    @merge_requests     = merge_requests_collection
+    @merge_requests     = @merge_requests.page(params[:page])
+    @issuable_meta_data = issuable_meta_data(@merge_requests)
+
     if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
       return redirect_to url_for(params.merge(page: @merge_requests.total_pages))
     end
diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb
index 46b17479d6d..6937ad3bdd9 100644
--- a/app/models/award_emoji.rb
+++ b/app/models/award_emoji.rb
@@ -16,6 +16,14 @@ class AwardEmoji < ActiveRecord::Base
   scope :downvotes, -> { where(name: DOWNVOTE_NAME) }
   scope :upvotes,   -> { where(name: UPVOTE_NAME) }
 
+  class << self
+    def votes_for_collection(ids, type)
+      select('name', 'awardable_id', 'COUNT(*) as count').
+        where('name IN (?) AND awardable_type = ? AND awardable_id IN (?)', [DOWNVOTE_NAME, UPVOTE_NAME], type, ids).
+        group('name', 'awardable_id')
+    end
+  end
+
   def downvote?
     self.name == DOWNVOTE_NAME
   end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 3517969eabc..bfb54e878fc 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -15,6 +15,11 @@ module Issuable
   include Taskable
   include TimeTrackable
 
+  # This object is used to gather issuable meta data for displaying
+  # upvotes, downvotes and notes count for issues and merge requests
+  # lists avoiding n+1 queries and improving performance.
+  IssuableMeta = Struct.new(:upvotes, :downvotes, :notes_count)
+
   included do
     cache_markdown_field :title, pipeline: :single_line
     cache_markdown_field :description
diff --git a/app/models/note.rb b/app/models/note.rb
index bf090a0438c..029fe667a45 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -108,6 +108,12 @@ class Note < ActiveRecord::Base
       Discussion.for_diff_notes(active_notes).
         map { |d| [d.line_code, d] }.to_h
     end
+
+    def count_for_collection(ids, type)
+      user.select('noteable_id', 'COUNT(*) as count').
+        group(:noteable_id).
+        where(noteable_type: type, noteable_id: ids)
+    end
   end
 
   def cross_reference?
diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml
index 5c9839cb330..0e3902c066a 100644
--- a/app/views/projects/issues/_issue.html.haml
+++ b/app/views/projects/issues/_issue.html.haml
@@ -17,22 +17,7 @@
             %li
               = link_to_member(@project, issue.assignee, name: false, title: "Assigned to :name")
 
-          - upvotes, downvotes = issue.upvotes, issue.downvotes
-          - if upvotes > 0
-            %li
-              = icon('thumbs-up')
-              = upvotes
-
-          - if downvotes > 0
-            %li
-              = icon('thumbs-down')
-              = downvotes
-
-          - note_count = issue.notes.user.count
-          %li
-            = link_to issue_path(issue, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
-              = icon('comments')
-              = note_count
+          = render 'shared/issuable_meta_data', issuable: issue
 
       .issue-info
         #{issuable_reference(issue)} &middot;
diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml
index a5fbe9d6128..11b7aaec704 100644
--- a/app/views/projects/merge_requests/_merge_request.html.haml
+++ b/app/views/projects/merge_requests/_merge_request.html.haml
@@ -29,22 +29,7 @@
           %li
             = link_to_member(merge_request.source_project, merge_request.assignee, name: false, title: "Assigned to :name")
 
-        - upvotes, downvotes = merge_request.upvotes, merge_request.downvotes
-        - if upvotes > 0
-          %li
-            = icon('thumbs-up')
-            = upvotes
-
-        - if downvotes > 0
-          %li
-            = icon('thumbs-down')
-            = downvotes
-
-        - note_count = merge_request.related_notes.user.count
-        %li
-          = link_to merge_request_path(merge_request, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
-            = icon('comments')
-            = note_count
+        = render 'shared/issuable_meta_data', issuable: merge_request
 
     .merge-request-info
       #{issuable_reference(merge_request)} &middot;
diff --git a/app/views/shared/_issuable_meta_data.html.haml b/app/views/shared/_issuable_meta_data.html.haml
new file mode 100644
index 00000000000..1264e524d86
--- /dev/null
+++ b/app/views/shared/_issuable_meta_data.html.haml
@@ -0,0 +1,19 @@
+- note_count         = @issuable_meta_data[issuable.id].notes_count
+- issue_votes        = @issuable_meta_data[issuable.id]
+- upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
+- issuable_url       = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')
+
+- if upvotes > 0
+  %li
+    = icon('thumbs-up')
+    = upvotes
+
+- if downvotes > 0
+  %li
+    = icon('thumbs-down')
+    = downvotes
+
+%li
+  = link_to issuable_url, class: ('no-comments' if note_count.zero?) do
+    = icon('comments')
+    = note_count