Commit dc0bc246 authored by Igor Drozdov's avatar Igor Drozdov Committed by Ash McKenzie

Fix merge ref head comments for removed lines

Add a test which verifies the behavior
parent 3d9857f7
...@@ -50,9 +50,9 @@ module Discussions ...@@ -50,9 +50,9 @@ module Discussions
merge_ref_head = merge_request.merge_ref_head merge_ref_head = merge_request.merge_ref_head
return unless merge_ref_head return unless merge_ref_head
start_sha, base_sha = merge_ref_head.parent_ids start_sha, _ = merge_ref_head.parent_ids
new_diff_refs = Gitlab::Diff::DiffRefs.new( new_diff_refs = Gitlab::Diff::DiffRefs.new(
base_sha: base_sha, base_sha: start_sha,
start_sha: start_sha, start_sha: start_sha,
head_sha: merge_ref_head.id) head_sha: merge_ref_head.id)
......
---
title: Fix merge ref head comments for removed lines
merge_request: 37755
author:
type: fixed
...@@ -12,24 +12,24 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do ...@@ -12,24 +12,24 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do
let(:offset) { 30 } let(:offset) { 30 }
let(:first_new_line) { 508 } let(:first_new_line) { 508 }
let(:second_new_line) { 521 } let(:second_new_line) { 521 }
let(:third_removed_line) { 1240 }
let(:service) { described_class.new(merge_request) } let(:service) { described_class.new(merge_request) }
def build_position(new_line, diff_refs) def build_position(diff_refs, new_line: nil, old_line: nil)
path = 'files/markdown/ruby-style-guide.md' path = 'files/markdown/ruby-style-guide.md'
Gitlab::Diff::Position.new(old_path: path, new_path: path, Gitlab::Diff::Position.new(old_path: path, new_path: path,
new_line: new_line, diff_refs: diff_refs) new_line: new_line, old_line: old_line, diff_refs: diff_refs)
end end
def note_for(new_line) def note_for(new_line: nil, old_line: nil)
position = build_position(new_line, merge_request.diff_refs) position = build_position(merge_request.diff_refs, new_line: new_line, old_line: old_line)
create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request)
end end
def verify_diff_note_position!(note, line) def verify_diff_note_position!(note, new_line: nil, old_line: nil)
id, old_line, new_line = note.line_code.split('_') id, removed_line, added_line = note.line_code.split('_')
expect(new_line).to eq(line.to_s)
expect(note.diff_note_positions.size).to eq(1) expect(note.diff_note_positions.size).to eq(1)
diff_position = note.diff_note_positions.last diff_position = note.diff_note_positions.last
...@@ -38,12 +38,13 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do ...@@ -38,12 +38,13 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do
start_sha: merge_request.target_branch_sha, start_sha: merge_request.target_branch_sha,
head_sha: merge_request.merge_ref_head.sha) head_sha: merge_request.merge_ref_head.sha)
expect(diff_position.line_code).to eq("#{id}_#{old_line.to_i - offset}_#{new_line}") expect(diff_position.line_code).to eq("#{id}_#{removed_line.to_i - offset}_#{added_line}")
expect(diff_position.position).to eq(build_position(new_line.to_i, diff_refs)) expect(diff_position.position).to eq(build_position(diff_refs, new_line: new_line, old_line: old_line))
end end
let!(:first_discussion_note) { note_for(first_new_line) } let!(:first_discussion_note) { note_for(new_line: first_new_line) }
let!(:second_discussion_note) { note_for(second_new_line) } let!(:second_discussion_note) { note_for(new_line: second_new_line) }
let!(:third_discussion_note) { note_for(old_line: third_removed_line) }
let!(:second_discussion_another_note) do let!(:second_discussion_another_note) do
create(:diff_note_on_merge_request, create(:diff_note_on_merge_request,
project: project, project: project,
...@@ -57,8 +58,9 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do ...@@ -57,8 +58,9 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do
MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
service.execute service.execute
verify_diff_note_position!(first_discussion_note, first_new_line) verify_diff_note_position!(first_discussion_note, new_line: first_new_line)
verify_diff_note_position!(second_discussion_note, second_new_line) verify_diff_note_position!(second_discussion_note, new_line: second_new_line)
verify_diff_note_position!(third_discussion_note, old_line: third_removed_line - offset)
expect(second_discussion_another_note.diff_note_positions).to be_empty expect(second_discussion_another_note.diff_note_positions).to be_empty
end end
......
...@@ -76,7 +76,7 @@ module TestEnv ...@@ -76,7 +76,7 @@ module TestEnv
'png-lfs' => 'fe42f41', 'png-lfs' => 'fe42f41',
'sha-starting-with-large-number' => '8426165', 'sha-starting-with-large-number' => '8426165',
'invalid-utf8-diff-paths' => '99e4853', 'invalid-utf8-diff-paths' => '99e4853',
'compare-with-merge-head-source' => 'b5f4399', 'compare-with-merge-head-source' => 'f20a03d',
'compare-with-merge-head-target' => '2f1e176' 'compare-with-merge-head-target' => '2f1e176'
}.freeze }.freeze
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment