Merge branch 'dont-render-discussion-notes-on-not-html-requests'

3 files changed, 35 insertions, 24 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8592696f58f..cc4af129bab 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -47,6 +47,7 @@ v 8.10.0 (unreleased)
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
   - Set import_url validation to be more strict
   - Memoize MR merged/closed events retrieval
+  - Don't render discussion notes when requesting diff tab through AJAX
   - Add basic system information like memory and disk usage to the admin panel
   - Don't garbage collect commits that have related DB records like comments
   - More descriptive message for git hooks and file locks
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index df1943dd9bb..8fda5618818 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -53,9 +53,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def show
-    @note_counts = Note.where(commit_id: @merge_request.commits.map(&:id)).
-      group(:commit_id).count
-
     respond_to do |format|
       format.html
       
@@ -80,6 +77,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def diffs
     apply_diff_view_cookie!
 
+    @merge_request_diff = @merge_request.merge_request_diff
+
     @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit || @merge_request.likely_diff_base_commit
 
@@ -109,7 +108,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def commits
     respond_to do |format|
       format.html { render 'show' }
-      format.json { render json: { html: view_to_html_string('projects/merge_requests/show/_commits') } }
+      format.json do
+        # Get commits from repository
+        # or from cache if already merged
+        @commits = @merge_request.commits
+        @note_counts = Note.where(commit_id: @commits.map(&:id)).
+          group(:commit_id).count
+
+        render json: { html: view_to_html_string('projects/merge_requests/show/_commits') }
+      end
     end
   end
 
@@ -340,14 +347,33 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def define_show_vars
+    @noteable = @merge_request
+    @commits_count = @merge_request.commits.count
+
+    @pipeline = @merge_request.pipeline
+    @statuses = @pipeline.statuses if @pipeline
+
+    if @merge_request.locked_long_ago?
+      @merge_request.unlock_mr
+      @merge_request.close
+    end
+
+    if request.format == :html || action_name == 'show'
+      define_show_html_vars
+    end
+  end
+
+  # Discussion tab data is only required on html requests
+  def define_show_html_vars
     # Build a note object for comment form
-    @note = @project.notes.new(noteable: @merge_request)
+    @note = @project.notes.new(noteable: @noteable)
 
-    @discussions = @merge_request.mr_and_commit_notes.
+    @discussions = @noteable.mr_and_commit_notes.
       inc_author_project_award_emoji.
       fresh.
       discussions
 
+    # This is not executed lazily
     @notes = Banzai::NoteRenderer.render(
       @discussions.flatten,
       @project,
@@ -356,22 +382,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       @project_wiki,
       @ref
     )
-
-    @noteable = @merge_request
-
-    # Get commits from repository
-    # or from cache if already merged
-    @commits = @merge_request.commits
-
-    @merge_request_diff = @merge_request.merge_request_diff
-
-    @pipeline = @merge_request.pipeline
-    @statuses = @pipeline.statuses if @pipeline
-
-    if @merge_request.locked_long_ago?
-      @merge_request.unlock_mr
-      @merge_request.close
-    end
   end
 
   def define_widget_vars
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 2ec96308fd7..873ed9b59ee 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -42,7 +42,7 @@
         = succeed '.' do
           = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal"
 
-    - if @commits.present?
+    - if @commits_count.nonzero?
       %ul.merge-request-tabs.nav-links.no-top.no-bottom
         %li.notes-tab
           = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: 'div#notes', action: 'notes', toggle: 'tab'} do
@@ -51,7 +51,7 @@
         %li.commits-tab
           = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: 'div#commits', action: 'commits', toggle: 'tab'} do
             Commits
-            %span.badge= @commits.size
+            %span.badge= @commits_count
         - if @pipeline
           %li.builds-tab
             = link_to builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#builds', action: 'builds', toggle: 'tab'} do