From c77083a4938b42e1901a4cb69106a39a04f5c30d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= <mbergeron@gitlab.com>
Date: Mon, 18 Dec 2017 10:21:33 -0500
Subject: [PATCH] fix the commit diff discussion sending the wrong url

it should now send you to the merge request diff path scoped to the commit.
---
 app/models/diff_discussion.rb     | 19 ++++++++++++++-----
 spec/helpers/notes_helper_spec.rb | 14 ++++++++++++--
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 4a65738214b..758a26d3c70 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -22,12 +22,10 @@ class DiffDiscussion < Discussion
 
   def merge_request_version_params
     return unless for_merge_request?
-    return {} if active?
 
-    if on_merge_request_commit?
-      { commit_id: commit_id }
-    else
-      noteable.version_params_for(position.diff_refs)
+    version_params do |params|
+      params[:commit_id] = commit_id if on_merge_request_commit?
+      params
     end
   end
 
@@ -37,4 +35,15 @@ class DiffDiscussion < Discussion
       position: position.to_json
     )
   end
+
+  private
+
+  def version_params
+    params = if active?
+               {}
+             else
+               noteable.version_params_for(position.diff_refs)
+             end
+    yield params
+  end
 end
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index cd15e27b497..36a44f8567a 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -41,6 +41,7 @@ describe NotesHelper do
 
   describe '#discussion_path' do
     let(:project) { create(:project, :repository) }
+    let(:anchor) { discussion.line_code }
 
     context 'for a merge request discusion' do
       let(:merge_request) { create(:merge_request, source_project: project, target_project: project, importing: true) }
@@ -151,6 +152,15 @@ describe NotesHelper do
           expect(helper.discussion_path(discussion)).to be_nil
         end
       end
+
+      context 'for a contextual commit discussion' do
+        let(:commit) { merge_request.commits.last }
+        let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, commit_id: commit.id).to_discussion }
+
+        it 'returns the merge request diff discussion scoped in the commit' do
+          expect(helper.discussion_path(discussion)).to eq(diffs_project_merge_request_path(project, merge_request, commit_id: commit.id, anchor: anchor))
+        end
+      end
     end
 
     context 'for a commit discussion' do
@@ -160,7 +170,7 @@ describe NotesHelper do
         let(:discussion) { create(:diff_note_on_commit, project: project).to_discussion }
 
         it 'returns the commit path with the line code' do
-          expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: discussion.line_code))
+          expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: anchor))
         end
       end
 
@@ -168,7 +178,7 @@ describe NotesHelper do
         let(:discussion) { create(:legacy_diff_note_on_commit, project: project).to_discussion }
 
         it 'returns the commit path with the line code' do
-          expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: discussion.line_code))
+          expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: anchor))
         end
       end