summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJarka Kadlecová <jarka@gitlab.com>2017-12-28 12:17:31 +0100
committerJarka Kadlecová <jarka@gitlab.com>2017-12-29 14:44:39 +0100
commitc1ea4afad760af7bbb0a38a2519445e0717b7b2e (patch)
tree7ceb8b263a31d885ed4d3d1ed9a758c2e521004d
parent4e0b6bf714f6e009c5a3ded5c3250ac9e178412f (diff)
downloadgitlab-ce-41492-mr-comment-fix.tar.gz
Fix links to old commits in merge requests41492-mr-comment-fix
-rw-r--r--app/models/diff_discussion.rb9
-rw-r--r--changelogs/unreleased/41492-mr-comment-fix.yml5
-rw-r--r--spec/models/diff_discussion_spec.rb48
-rw-r--r--spec/services/system_note_service_spec.rb32
4 files changed, 78 insertions, 16 deletions
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index d67b16584a4..bd6af622bfb 100644
--- a/app/models/diff_discussion.rb
+++ b/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)
diff --git a/changelogs/unreleased/41492-mr-comment-fix.yml b/changelogs/unreleased/41492-mr-comment-fix.yml
new file mode 100644
index 00000000000..45ddc16aad0
--- /dev/null
+++ b/changelogs/unreleased/41492-mr-comment-fix.yml
@@ -0,0 +1,5 @@
+---
+title: Fix links to old commits in merge request comments
+merge_request:
+author:
+type: fixed
diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb
index fa02434b0fd..50b19000799 100644
--- a/spec/models/diff_discussion_spec.rb
+++ b/spec/models/diff_discussion_spec.rb
@@ -47,8 +47,20 @@ describe DiffDiscussion do
diff_note.save!
end
- 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)
+ 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
@@ -70,8 +82,20 @@ describe DiffDiscussion do
diff_note.save!
end
- 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)
+ 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
@@ -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
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 9025589ae0b..4e640a82dfc 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/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,17 +1071,32 @@ describe SystemNoteService do
let(:action) { 'outdated' }
end
- 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)
+ 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)
+ end
+
+ it 'links to the diff in the system note' do
+ expect(subject.note).to include('version 1')
+
+ diff_id = merge_request.merge_request_diff.id
+ line_code = change_position.line_code(project.repository)
+ expect(subject.note).to include(diffs_project_merge_request_url(project, merge_request, diff_id: diff_id, anchor: line_code))
+ end
end
- it 'links to the diff in the system note' do
- expect(subject.note).to include('version 1')
+ context 'when the change_position is invalid for the discussion' do
+ let(:change_position) { project.commit(sample_commit.id) }
- diff_id = merge_request.merge_request_diff.id
- line_code = change_position.line_code(project.repository)
- expect(subject.note).to include(diffs_project_merge_request_url(project, merge_request, diff_id: diff_id, anchor: line_code))
+ 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