Merge branch '355027-fix-n-plus-one-queries' into 'master'

Fix N plus one queries for new review email

See merge request gitlab-org/gitlab!83544

app/mailers/emails/reviews.rb

@@ -22,6 +22,10 @@ module Emails
       review = Review.find_by_id(review_id)
 
       @notes = review.notes
+      @discussions = Discussion.build_discussions(review.discussion_ids, preload_note_diff_file: true)
+      @include_diff_discussion_stylesheet = @discussions.values.any? do |discussion|
+        discussion.diff_discussion? && discussion.on_text?
+      end
       @author = review.author
       @author_name = review.author_name
       @project = review.project

app/models/discussion.rb

@@ -47,6 +47,14 @@ class Discussion
     grouped_notes.values.map { |notes| build(notes, context_noteable) }
   end
 
+  def self.build_discussions(discussion_ids, context_noteable = nil, preload_note_diff_file: false)
+    notes = Note.where(discussion_id: discussion_ids).fresh
+    notes = notes.inc_note_diff_file if preload_note_diff_file
+
+    grouped_notes = notes.group_by { |n| n.discussion_id }
+    grouped_notes.transform_values { |notes| Discussion.build(notes, context_noteable) }
+  end
+
   def self.lazy_find(discussion_id)
     BatchLoader.for(discussion_id).batch do |discussion_ids, loader|
       results = Note.where(discussion_id: discussion_ids).fresh.to_a.group_by(&:discussion_id)

app/models/note.rb

@@ -121,6 +121,7 @@ class Note < ApplicationRecord
   scope :with_discussion_ids, ->(discussion_ids) { where(discussion_id: discussion_ids) }
   scope :with_suggestions, -> { joins(:suggestions) }
   scope :inc_author, -> { includes(:author) }
+  scope :inc_note_diff_file, -> { includes(:note_diff_file) }
   scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }
   scope :inc_relations_for_view, -> do
     includes({ project: :group }, { author: :status }, :updated_by, :resolved_by, :award_emoji,

app/models/review.rb

@@ -14,6 +14,10 @@ class Review < ApplicationRecord
 
   participant :author
 
+  def discussion_ids
+    notes.select(:discussion_id)
+  end
+
   def all_references(current_user = nil, extractor: nil)
     ext = super
 

app/views/notify/_note_email.html.haml

@@ -2,17 +2,18 @@
 - diff_limit = local_assigns.fetch(:diff_limit, nil)
 - target_url = local_assigns.fetch(:target_url, @target_url)
 - note_style = local_assigns.fetch(:note_style, "")
-- skip_stylesheet_link = local_assigns.fetch(:skip_stylesheet_link, false)
+- include_stylesheet_link = local_assigns.fetch(:include_stylesheet_link, true)
 
-- discussion = note.discussion if note.part_of_discussion?
+- author = local_assigns.fetch(:author) { note.author }
+- discussion = local_assigns.fetch(:discussion) { note.discussion } if note.part_of_discussion?
 
 %p{ style: "color: #777777;" }
   = succeed ':' do
-    = link_to note.author_name, user_url(note.author)
+    = link_to author.name, user_url(author)
     - if discussion.nil?
       = link_to 'commented', target_url
     - else
-      - if note.start_of_discussion?
+      - if discussion.first_note == note
         started a new
       - else
         commented on a
@@ -22,8 +23,7 @@
       - else
         = link_to 'discussion', target_url
 
-- if discussion&.diff_discussion? && discussion.on_text?
-  - unless skip_stylesheet_link
+- if include_stylesheet_link && discussion&.diff_discussion? && discussion.on_text?
   = content_for :head do
     = stylesheet_link_tag 'mailers/highlighted_diff_email'
 
@@ -34,4 +34,4 @@
       locals: { diff_file: discussion.diff_file }
 
 .md{ style: note_style }
-  = markdown(note.note, pipeline: :email, author: note.author, current_user: @recipient, issuable_reference_expansion_enabled: true)
+  = markdown(note.note, pipeline: :email, author: author, current_user: @recipient, issuable_reference_expansion_enabled: true)

app/views/notify/_note_email.text.erb

 <% note = local_assigns.fetch(:note, @note) -%>
 <% diff_limit = local_assigns.fetch(:diff_limit, nil) -%>
 <% target_url = local_assigns.fetch(:target_url, @target_url) -%>
-<% discussion = note.discussion if note.part_of_discussion? -%>
+<% author = local_assigns.fetch(:author) { note.author } -%>
+<% discussion = local_assigns.fetch(:discussion) { note.discussion } if note.part_of_discussion? -%>
 
-<%= sanitize_name(note.author_name) -%>
+<%= sanitize_name(author.name) -%>
 <%  if discussion.nil? -%>
  <%= 'commented' -%>:
 <%  else -%>
-<%    if note.start_of_discussion? -%>
+<%    if discussion.first_note == note -%>
  <%=    'started a new discussion' -%>
 <%    else -%>
  <%=    'commented on a discussion' -%>

app/views/notify/new_review_email.html.haml

-= content_for :head do
+- if @include_diff_discussion_stylesheet
+  = content_for :head do
     = stylesheet_link_tag 'mailers/highlighted_diff_email'
 
 %table{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;margin:0 auto;border-collapse:separate;border-spacing:0;" }
@@ -15,5 +16,9 @@
             %tr
               %td{ style: "overflow:hidden;font-size:14px;line-height:1.4;display:grid;" }
                 - @notes.each do |note|
+                  -# Get preloaded note discussion
+                  - discussion = @discussions[note.discussion_id] if note.part_of_discussion?
+                  -# Preload project for discussions first note
+                  - discussion.first_note.project = @project if discussion&.first_note
                   - target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{note.id}")
-                  = render 'note_email', note: note, diff_limit: 3, target_url: target_url, note_style: "border-bottom:1px solid #ededed;", skip_stylesheet_link: true
+                  = render 'note_email', note: note, diff_limit: 3, target_url: target_url, note_style: "border-bottom:1px solid #ededed;", include_stylesheet_link: false, discussion: discussion, author: @author

app/views/notify/new_review_email.text.erb

@@ -4,8 +4,10 @@
 
 --
 <% @notes.each_with_index do |note, index| %>
+  <!-- Get preloaded note discussion-->
+  <% discussion = @discussions[note.discussion_id] if note.part_of_discussion?%>
   <% target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{note.id}") %>
-  <%= render 'note_email', note: note, diff_limit: 3, target_url: target_url %>
+  <%= render 'note_email', note: note, diff_limit: 3, target_url: target_url, discussion: discussion, author: @author %>
 
   <% if index != @notes.length-1 %>
 --

spec/mailers/notify_spec.rb

@@ -2181,18 +2181,46 @@ RSpec.describe Notify do
     context 'when diff note' do
       let!(:notes) { create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request) }
 
-      it 'links to notes' do
+      it 'links to notes and discussions', :aggregate_failures do
+        reply_note = create(:diff_note_on_merge_request, review: review, project: project, author: review.author, noteable: merge_request, in_reply_to: notes.first)
+
         review.notes.each do |note|
           # Text part
           expect(subject.text_part.body.raw_source).to include(
             project_merge_request_url(project, merge_request, anchor: "note_#{note.id}")
           )
+
+          if note == reply_note
+            expect(subject.text_part.body.raw_source).to include("commented on a discussion on #{note.discussion.file_path}")
+          else
+            expect(subject.text_part.body.raw_source).to include("started a new discussion on #{note.discussion.file_path}")
+          end
         end
       end
 
       it 'includes only one link to the highlighted_diff_email' do
         expect(subject.html_part.body.raw_source).to include('assets/mailers/highlighted_diff_email').once
       end
+
+      it 'avoids N+1 cached queries when rendering html', :use_sql_query_cache, :request_store do
+        control_count = ActiveRecord::QueryRecorder.new(query_recorder_debug: true, skip_cached: false) do
+          subject.html_part
+        end
+
+        create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request)
+
+        expect { described_class.new_review_email(recipient.id, review.id).html_part }.not_to exceed_all_query_limit(control_count)
+      end
+
+      it 'avoids N+1 cached queries when rendering text', :use_sql_query_cache, :request_store do
+        control_count = ActiveRecord::QueryRecorder.new(query_recorder_debug: true, skip_cached: false) do
+          subject.text_part
+        end
+
+        create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request)
+
+        expect { described_class.new_review_email(recipient.id, review.id).text_part }.not_to exceed_all_query_limit(control_count)
+      end
     end
 
     it 'contains review author name' do