From 66ec925557d63f3f168b03a8b85e15a199da7307 Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Tue, 21 Jun 2016 07:26:43 +0200 Subject: [PATCH] Preload notes/discussions associations (award_emoji: :user) --- CHANGELOG | 1 + .../projects/merge_requests_controller.rb | 4 ++-- app/models/concerns/awardable.rb | 12 +++++------- app/models/concerns/issuable.rb | 18 +++++++++++++++--- app/models/merge_request.rb | 2 +- app/models/note.rb | 4 +++- 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