Merge branch '41492-mr-comment-fix' into 'master'

Fix links to old commits in merge request comments

Closes #41492

See merge request gitlab-org/gitlab-ce!16152

app/models/diff_discussion.rb

@@ -23,8 +23,13 @@ class DiffDiscussion < Discussion
   def merge_request_version_params
     return unless for_merge_request?
 
+    version_params = get_params
+
+    return version_params unless on_merge_request_commit? && commit_id
+
+    version_params ||= {}
     version_params.tap do |params|
-      params[:commit_id] = commit_id if on_merge_request_commit?
+      params[:commit_id] = commit_id
     end
   end
 
@@ -37,7 +42,7 @@ class DiffDiscussion < Discussion
 
   private
 
-  def version_params
+  def get_params
     return {} if active?
 
     noteable.version_params_for(position.diff_refs)

changelogs/unreleased/41492-mr-comment-fix.yml

+---
+title: Fix links to old commits in merge request comments
+merge_request:
+author:
+type: fixed

spec/models/diff_discussion_spec.rb

@@ -47,11 +47,23 @@ describe DiffDiscussion do
         diff_note.save!
       end
 
+      context 'when commit_id is not present' do
         it 'returns the diff ID for the version to show' do
           expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id)
         end
       end
 
+      context 'when commit_id is present' do
+        before do
+          diff_note.update_attribute(:commit_id, 'commit_123')
+        end
+
+        it 'includes the commit_id in the result' do
+          expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff1.id, commit_id: 'commit_123')
+        end
+      end
+    end
+
     context 'when the discussion is on a comparison between merge request versions' do
       let(:position) do
         Gitlab::Diff::Position.new(
@@ -70,11 +82,23 @@ describe DiffDiscussion do
         diff_note.save!
       end
 
+      context 'when commit_id is not present' do
         it 'returns the diff ID and start sha of the versions to compare' do
           expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
         end
       end
 
+      context 'when commit_id is present' do
+        before do
+          diff_note.update_attribute(:commit_id, 'commit_123')
+        end
+
+        it 'includes the commit_id in the result' do
+          expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha, commit_id: 'commit_123')
+        end
+      end
+    end
+
     context 'when the discussion does not have a merge request version' do
       let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, diff_refs: project.commit(sample_commit.id).diff_refs) }
 
@@ -83,8 +107,20 @@ describe DiffDiscussion do
         diff_note.save!
       end
 
-      it 'returns nil' do
-        expect(subject.merge_request_version_params).to be_nil
+      context 'when commit_id is not present' do
+        it 'returns empty hash' do
+          expect(subject.merge_request_version_params).to eq(nil)
+        end
+      end
+
+      context 'when commit_id is present' do
+        before do
+          diff_note.update_attribute(:commit_id, 'commit_123')
+        end
+
+        it 'returns the commit_id' do
+          expect(subject.merge_request_version_params).to eq(commit_id: 'commit_123')
+        end
       end
     end
   end

spec/services/system_note_service_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe SystemNoteService do
   include Gitlab::Routing
+  include RepoHelpers
 
   set(:group)    { create(:group) }
   set(:project)  { create(:project, :repository, group: group) }
@@ -1070,6 +1071,7 @@ describe SystemNoteService do
       let(:action)            { 'outdated' }
     end
 
+    context 'when the change_position is valid for the discussion' do
       it 'creates a new note in the discussion' do
         # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
         expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
@@ -1084,6 +1086,20 @@ describe SystemNoteService do
       end
     end
 
+    context 'when the change_position is invalid for the discussion' do
+      let(:change_position) { project.commit(sample_commit.id) }
+
+      it 'creates a new note in the discussion' do
+        # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+        expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      end
+
+      it 'does not create a link' do
+        expect(subject.note).to eq('changed this line in version 1 of the diff')
+      end
+    end
+  end
+
   describe '.mark_duplicate_issue' do
     subject { described_class.mark_duplicate_issue(noteable, project, author, canonical_issue) }