Fix discussions N+1 queries

- Add specs for html and text node
- Fix N plus one queries for new review email

Changelog: performance

app/mailers/emails/reviews.rb

@@ -22,6 +22,8 @@ module Emails
       review = Review.find_by_id(review_id)
 
       @notes = review.notes
+      discussion_ids = @notes.pluck(:discussion_id)
+      @discussions = discussions(discussion_ids)
       @author = review.author
       @author_name = review.author_name
       @project = review.project
@@ -29,5 +31,11 @@ module Emails
       @target_url = project_merge_request_url(@project, @merge_request)
       @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
     end
+
+    def discussions(discussion_ids)
+      notes = Note.where(discussion_id: discussion_ids).inc_note_diff_file.fresh
+      grouped_notes = notes.group_by { |n| n.discussion_id }
+      grouped_notes.transform_values { |notes| Discussion.build(notes) }
+    end
   end
 end

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/views/notify/_note_email.html.haml

@@ -4,7 +4,7 @@
 - note_style = local_assigns.fetch(:note_style, "")
 - skip_stylesheet_link = local_assigns.fetch(:skip_stylesheet_link, false)
 
-- discussion = note.discussion if note.part_of_discussion?
+- discussion = local_assigns.fetch(:discussion){ note.discussion } if note.part_of_discussion?
 
 %p{ style: "color: #777777;" }
   = succeed ':' do
@@ -12,7 +12,7 @@
     - 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

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? -%>
+<% discussion = local_assigns.fetch(:discussion){ note.discussion } if note.part_of_discussion? -%>
 
 <%= sanitize_name(note.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

@@ -15,5 +15,11 @@
             %tr
               %td{ style: "overflow:hidden;font-size:14px;line-height:1.4;display:grid;" }
                 - @notes.each do |note|
+                  -# Preload author, to prevent N+1 queries
+                  - note.author = @author
+                  -# Get preloaded note discussion
+                  - discussion = @discussions.try(:[], 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;", skip_stylesheet_link: true, discussion: discussion

app/views/notify/new_review_email.text.erb

@@ -4,8 +4,14 @@
 
 --
 <% @notes.each_with_index do |note, index| %>
+  <!-- Preload author, to prevent N+1 queries-->
+  <% note.author = @author %>
+  <!-- Get preloaded note discussion-->
+  <% discussion = @discussions.try(:[], 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 %>
+  <%= render 'note_email', note: note, diff_limit: 3, target_url: target_url, discussion: discussion %>
 
   <% if index != @notes.length-1 %>
 --

spec/mailers/notify_spec.rb

@@ -2181,18 +2181,47 @@ 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