diff notes created in merge request on a commit have the right context

add a spec for commit merge request diff notes

7 files changed, 39 insertions, 16 deletions

diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb
index 1269759fc2b..3b764433c01 100644
--- a/app/controllers/projects/merge_requests/application_controller.rb
+++ b/app/controllers/projects/merge_requests/application_controller.rb
@@ -1,6 +1,7 @@
 class Projects::MergeRequests::ApplicationController < Projects::ApplicationController
   before_action :check_merge_requests_available!
   before_action :merge_request
+  before_action :commit
   before_action :authorize_read_merge_request!
 
   private
@@ -9,6 +10,11 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
     @issuable = @merge_request ||= @project.merge_requests.find_by!(iid: params[:id])
   end
 
+  def commit
+    return nil unless commit_id = params[:commit_id].presence
+    @commit ||= merge_request.target_project.commit(commit_id)
+  end
+
   def merge_request_params
     params.require(:merge_request).permit(merge_request_params_attributes)
   end
@@ -28,7 +34,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
       :task_num,
       :title,
       :discussion_locked,
-
       label_ids: []
     ]
   end
diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb
index 42a6e5be14f..1312c83373f 100644
--- a/app/controllers/projects/merge_requests/diffs_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_controller.rb
@@ -22,13 +22,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   def define_diff_vars
     @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff.order_id_desc
 
-    if commit_id = params[:commit_id].presence
-      @commit = @merge_request.target_project.commit(commit_id)
-      @compare = @commit
-    else
-      @compare = find_merge_request_diff_compare
-    end
-
+    @compare = commit || find_merge_request_diff_compare
     return render_404 unless @compare
 
     @diffs = @compare.diffs(diff_options)
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 5b2c58d193d..004aaeb2c56 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -101,6 +101,30 @@ module MergeRequestsHelper
     }.merge(merge_params_ee(merge_request))
   end
 
+  def tab_link_for(tab, options={}, &block)
+    data_attrs = {
+      action: tab.to_s,
+      target: "##{tab.to_s}",
+      toggle: options.fetch(:force_link, false) ? '' : 'tab'
+    }
+
+    url = case tab
+    when :show
+      data_attrs.merge!(target: '#notes')
+      project_merge_request_path(@project, @merge_request)
+    when :commits
+      commits_project_merge_request_path(@project, @merge_request)
+    when :pipelines
+      pipelines_project_merge_request_path(@project, @merge_request)
+    when :diffs
+      diffs_project_merge_request_path(@project, @merge_request)
+    else
+      raise "Cannot create tab #{tab}."
+    end
+
+    link_to(url, data: data_attrs, &block)
+  end
+
   def merge_params_ee(merge_request)
     {}
   end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 434dda89db0..9f05535d4d4 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -6,7 +6,7 @@ module MergeRequests
       @oldrev, @newrev = oldrev, newrev
       @branch_name = Gitlab::Git.ref_name(ref)
 
-      find_new_commits
+      Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge
       close_merge_requests
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index f90c6fcafb8..385b34120ef 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -671,8 +671,8 @@ module SystemNoteService
 
   def merge_request_commit_url(merge_request, commit)
     url_helpers.diffs_namespace_project_merge_request_url(
-      project.namespace,
-      project,
+      merge_request.target_project.namespace,
+      merge_request.target_project,
       merge_request.iid,
       commit_id: commit.id
     )
diff --git a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml
index 26988d34917..60c419a3cda 100644
--- a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml
+++ b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml
@@ -13,7 +13,7 @@
         - else
           viewing an old version of the diff
 
-      .pull-right
+      .text-right
         = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm' do
           Show latest version
           = "of the diff" if @commit
diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index 4e3f6f9edc9..0d7abe8137f 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/app/views/projects/merge_requests/show.html.haml
@@ -38,21 +38,21 @@
           .nav-links.scrolling-tabs
             %ul.merge-request-tabs
               %li.notes-tab
-                = link_to project_merge_request_path(@project, @merge_request), data: { target: 'div#notes', action: 'show', toggle: 'tab' } do
+                = tab_link_for :show, force_link: @commit.present? do
                   Discussion
                   %span.badge= @merge_request.related_notes.user.count
               - if @merge_request.source_project
                 %li.commits-tab
-                  = link_to commits_project_merge_request_path(@project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do
+                  = tab_link_for :commits do
                     Commits
                     %span.badge= @commits_count
               - if @pipelines.any?
                 %li.pipelines-tab
-                  = link_to pipelines_project_merge_request_path(@project, @merge_request), data: { target: '#pipelines', action: 'pipelines', toggle: 'tab' } do
+                  = tab_link_for :pipelines do
                     Pipelines
                     %span.badge.js-pipelines-mr-count= @pipelines.size
               %li.diffs-tab
-                = link_to diffs_project_merge_request_path(@project, @merge_request), data: { target: 'div#diffs', action: 'diffs', toggle: 'tab' } do
+                = tab_link_for :diffs do
                   Changes
                   %span.badge= @merge_request.diff_size
         #resolve-count-app.line-resolve-all-container.prepend-top-10{ "v-cloak" => true }