diff --git a/app/services/system_notes/merge_requests_service.rb b/app/services/system_notes/merge_requests_service.rb index 47c35bf01b7b8531379375cd51d4e6dd86609151..743845cf2d1d1e009f88275e28ac6549579f677c 100644 --- a/app/services/system_notes/merge_requests_service.rb +++ b/app/services/system_notes/merge_requests_service.rb @@ -57,12 +57,13 @@ def discussion_continued_in_issue(discussion, issue) def diff_discussion_outdated(discussion, change_position) merge_request = discussion.noteable - diff_refs = change_position.diff_refs + diff_refs = change_position&.diff_refs version_index = merge_request.merge_request_diffs.viewable.count - position_on_text = change_position.on_text? + + position_on_text = change_position&.on_text? text_parts = ["changed this #{position_on_text ? 'line' : 'file'} in"] - if version_params = merge_request.version_params_for(diff_refs) + if diff_refs && version_params = merge_request.version_params_for(diff_refs) repository = project.repository anchor = position_on_text ? change_position.line_code(repository) : change_position.file_hash url = url_helpers.diffs_project_merge_request_path(project, merge_request, version_params.merge(anchor: anchor)) diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb index 274430fdccb031399fc4b24de144e1d0c462df6d..0ed51538a603c4adee6565dea94427f146edf8e2 100644 --- a/spec/services/discussions/update_diff_position_service_spec.rb +++ b/spec/services/discussions/update_diff_position_service_spec.rb @@ -223,5 +223,29 @@ include_examples 'outdated diff note' end end + + context 'when position tracer returns nil for outdated discussion' do + let(:line) { 9 } + let(:tracer_double) { instance_double(Gitlab::Diff::PositionTracer) } + + before do + allow(Gitlab::Diff::PositionTracer).to receive(:new).and_return(tracer_double) + allow(tracer_double).to receive(:trace).and_return({ position: nil, outdated: true }) + end + + it "doesn't update the position" do + subject.execute(discussion) + + expect(discussion.original_position).to eq(old_position) + expect(discussion.position).to eq(old_position) + end + + it 'creates a system discussion' do + expect(SystemNoteService).to receive(:diff_discussion_outdated).with( + discussion, project, current_user, nil) + + subject.execute(discussion) + end + end end end diff --git a/spec/services/system_notes/merge_requests_service_spec.rb b/spec/services/system_notes/merge_requests_service_spec.rb index b0dceacf81f43e475dfa216b497585d64e4b66c9..21fe625f8538a889f9191717bc09c47fd226f8b1 100644 --- a/spec/services/system_notes/merge_requests_service_spec.rb +++ b/spec/services/system_notes/merge_requests_service_spec.rb @@ -177,6 +177,19 @@ def reloaded_merge_request expect(subject.note).to eq('changed this line in version 1 of the diff') end end + + context 'when change position is nil' do + let(:change_position) { nil } + + 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 file in version 1 of the diff') + end + end end describe '.change_branch' do