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

add a spec for commit merge request diff notes

app/controllers/projects/merge_requests/application_controller.rb

@@ -3,6 +3,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
 
   before_action :check_merge_requests_available!
   before_action :merge_request
+  before_action :commit
   before_action :authorize_read_merge_request!
 
   private
@@ -11,6 +12,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
@@ -30,7 +36,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
       :task_num,
       :title,
       :discussion_locked,
-
       label_ids: []
     ]
   end

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)

app/helpers/merge_requests_helper.rb

@@ -140,6 +140,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)
     { squash: merge_request.squash }
   end

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

app/services/system_note_service.rb

@@ -725,8 +725,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
     )

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

app/views/projects/merge_requests/show.html.haml

@@ -46,21 +46,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 }

spec/controllers/projects/commit_controller_spec.rb

@@ -132,6 +132,21 @@ describe Projects::CommitController do
         expect(response).to be_success
       end
     end
+
+    context 'in the context of a merge_request' do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+      let(:commit) { merge_request.commits.first }
+
+      it 'prepare diff notes in the context of the merge request' do
+        go(id: commit.id, merge_request_iid: merge_request.iid)
+
+        expect(assigns(:new_diff_note_attrs)).to eq({ noteable_type: 'MergeRequest',
+                                                      noteable_id: merge_request.id,
+                                                      commit_id: commit.id
+                                                    })
+        expect(response).to be_ok
+      end
+    end
   end
 
   describe 'GET branches' do