Preload notes/discussions associations (award_emoji: :user)optimize-award-emoji

6 files changed, 27 insertions, 14 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4f98d0a6915..55211a79a0c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.10.0 (unreleased)
   - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
   - Add Sidekiq queue duration to transaction metrics.
   - Fix MR-auto-close text added to description. !4836
+  - Eager load award emoji on notes
   - Fix pagination when sorting by columns with lots of ties (like priority)
   - Implement Subresource Integrity for CSS and JavaScript assets. This prevents malicious assets from loading in the case of a CDN compromise.
   - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 851822d805a..6c26c7f7658 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -320,8 +320,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def define_show_vars
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @merge_request)
-    @notes = @merge_request.mr_and_commit_notes.inc_author.fresh
-    @discussions = @notes.discussions
+    @discussions = @merge_request.mr_and_commit_notes.inc_author_project_award_emoji.fresh.discussions
+    @notes = @discussions.flatten
     @noteable = @merge_request
 
     # Get commits from repository
diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb
index 539c7c31e30..06beff177b1 100644
--- a/app/models/concerns/awardable.rb
+++ b/app/models/concerns/awardable.rb
@@ -2,10 +2,11 @@ module Awardable
   extend ActiveSupport::Concern
 
   included do
-    has_many :award_emoji, as: :awardable, dependent: :destroy
+    has_many :award_emoji, -> { includes(:user) }, as: :awardable, dependent: :destroy
 
     if self < Participable
-      participant :award_emoji_with_associations
+      # By default we always load award_emoji user association
+      participant :award_emoji
     end
   end
 
@@ -34,12 +35,9 @@ module Awardable
     end
   end
 
-  def award_emoji_with_associations
-    award_emoji.includes(:user)
-  end
-
   def grouped_awards(with_thumbs: true)
-    awards = award_emoji_with_associations.group_by(&:name)
+    # By default we always load award_emoji user association
+    awards = award_emoji.group_by(&:name)
 
     if with_thumbs
       awards[AwardEmoji::UPVOTE_NAME]   ||= []
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 38da0dafad2..d6f55885dd6 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -19,9 +19,14 @@ module Issuable
     belongs_to :milestone
     has_many :notes, as: :noteable, dependent: :destroy do
       def authors_loaded?
-        # We check first if we're loaded to not load unnecesarily.
+        # We check first if we're loaded to not load unnecessarily.
         loaded? && to_a.all? { |note| note.association(:author).loaded? }
       end
+
+      def award_emojis_loaded?
+        # We check first if we're loaded to not load unnecessarily.
+        loaded? && to_a.all? { |note| note.association(:award_emoji).loaded? }
+      end
     end
     has_many :label_links, as: :target, dependent: :destroy
     has_many :labels, through: :label_links
@@ -49,7 +54,7 @@ module Issuable
 
     scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) }
     scope :join_project, -> { joins(:project) }
-    scope :inc_notes_with_associations, -> { includes(notes: :author) }
+    scope :inc_notes_with_associations, -> { includes(notes: [ :project, :author, :award_emoji ]) }
     scope :references_project, -> { references(:project) }
     scope :non_archived, -> { join_project.where(projects: { archived: false }) }
 
@@ -260,7 +265,14 @@ module Issuable
     # already have their authors loaded (possibly because the scope
     # `inc_notes_with_associations` was used) and skip the inclusion if that's
     # the case.
-    notes.authors_loaded? ? notes : notes.includes(:author)
+    includes = []
+    includes << :author unless notes.authors_loaded?
+    includes << :award_emoji unless notes.award_emojis_loaded?
+    if includes.any?
+      notes.includes(includes)
+    else
+      notes
+    end
   end
 
   def updated_tasks
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 478827794ed..36bc98bdb1e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -309,7 +309,7 @@ class MergeRequest < ActiveRecord::Base
     commits_for_notes_limit = 100
     commit_ids = commits.last(commits_for_notes_limit).map(&:id)
 
-    Note.includes(:award_emoji).where(
+    Note.where(
       "(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" +
       "((project_id = :source_project_id OR project_id = :target_project_id) AND noteable_type = 'Commit' AND commit_id IN (:commit_ids))",
       mr_id: id,
diff --git a/app/models/note.rb b/app/models/note.rb
index 4bc9d1a92f8..e510525b89d 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -49,12 +49,14 @@ class Note < ActiveRecord::Base
   scope :fresh, ->{ order(created_at: :asc, id: :asc) }
   scope :inc_author_project, ->{ includes(:project, :author) }
   scope :inc_author, ->{ includes(:author) }
+  scope :inc_author_project_award_emoji, ->{ includes(:project, :author, :award_emoji) }
 
   scope :legacy_diff_notes, ->{ where(type: 'LegacyDiffNote') }
   scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
 
   scope :with_associations, -> do
-    includes(:author, :noteable, :updated_by, :award_emoji,
+    # FYI noteable cannot be loaded for LegacyDiffNote for commits
+    includes(:author, :noteable, :updated_by,
              project: [:project_members, { group: [:group_members] }])
   end