From 58ae394a49eb4dcb98b3440ff430dcf5a787fd88 Mon Sep 17 00:00:00 2001 From: Igor Drozdov <idrozdov@gitlab.com> Date: Thu, 2 Apr 2020 04:10:23 +0300 Subject: [PATCH] Capture diff note position on mergeability check When a merge request is checked whether it's mergeable merge_ref_head is updated and we need to update diff note positions within it --- app/models/diff_note_position.rb | 15 ++++- app/models/note.rb | 1 + .../capture_diff_note_position_service.rb | 65 ++++++++++++++++++ .../capture_diff_note_positions_service.rb | 34 ++++++++++ .../mergeability_check_service.rb | 7 ++ app/services/notes/create_service.rb | 4 ++ spec/lib/gitlab/import_export/all_models.yml | 4 ++ spec/models/diff_note_position_spec.rb | 37 +++++++--- ...capture_diff_note_position_service_spec.rb | 31 +++++++++ ...apture_diff_note_positions_service_spec.rb | 67 +++++++++++++++++++ .../mergeability_check_service_spec.rb | 18 +++++ spec/services/notes/create_service_spec.rb | 13 ++++ spec/support/helpers/test_env.rb | 4 +- 13 files changed, 289 insertions(+), 11 deletions(-) create mode 100644 app/services/discussions/capture_diff_note_position_service.rb create mode 100644 app/services/discussions/capture_diff_note_positions_service.rb create mode 100644 spec/services/discussions/capture_diff_note_position_service_spec.rb create mode 100644 spec/services/discussions/capture_diff_note_positions_service_spec.rb diff --git a/app/models/diff_note_position.rb b/app/models/diff_note_position.rb index 78e4fbc49eb00f19..716a56c64300599e 100644 --- a/app/models/diff_note_position.rb +++ b/app/models/diff_note_position.rb @@ -28,9 +28,20 @@ def position end def position=(position) + assign_attributes(self.class.position_to_attrs(position)) + end + + def self.create_or_update_for(note, params) + attrs = position_to_attrs(params[:position]) + attrs.merge!(params.slice(:diff_type, :line_code)) + attrs[:note_id] = note.id + + upsert(attrs, unique_by: [:note_id, :diff_type]) + end + + def self.position_to_attrs(position) position_attrs = position.to_h position_attrs[:diff_content_type] = position_attrs.delete(:position_type) - - assign_attributes(position_attrs) + position_attrs end end diff --git a/app/models/note.rb b/app/models/note.rb index 251a75e60251d595..e6ad7c2227f0bff1 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -83,6 +83,7 @@ def value?(val) has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_one :system_note_metadata has_one :note_diff_file, inverse_of: :diff_note, foreign_key: :diff_note_id + has_many :diff_note_positions delegate :gfm_reference, :local_reference, to: :noteable delegate :name, to: :project, prefix: true diff --git a/app/services/discussions/capture_diff_note_position_service.rb b/app/services/discussions/capture_diff_note_position_service.rb new file mode 100644 index 0000000000000000..273a60f7e55da39c --- /dev/null +++ b/app/services/discussions/capture_diff_note_position_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Discussions + class CaptureDiffNotePositionService + def initialize(merge_request, paths) + @project = merge_request.project + @tracer = build_tracer(merge_request, paths) + end + + def execute(discussion) + # The service has been implemented for text only + # The impact of image notes on this service is being investigated in + # https://gitlab.com/gitlab-org/gitlab/-/issues/213989 + return unless discussion.on_text? + + result = tracer&.trace(discussion.position) + return unless result + + position = result[:position] + + # Currently position data is copied across all notes of a discussion + # It makes sense to store a position only for the first note instead + # Within the newly introduced table we can start doing just that + DiffNotePosition.create_or_update_for(discussion.notes.first, + diff_type: :head, + position: position, + line_code: position.line_code(project.repository)) + end + + private + + attr_reader :tracer, :project + + def build_tracer(merge_request, paths) + return if paths.blank? + + old_diff_refs, new_diff_refs = build_diff_refs(merge_request) + + return unless old_diff_refs && new_diff_refs + + Gitlab::Diff::PositionTracer.new( + project: project, + old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + paths: paths.uniq) + end + + def build_diff_refs(merge_request) + merge_ref_head = merge_request.merge_ref_head + return unless merge_ref_head + + start_sha, base_sha = merge_ref_head.parent_ids + new_diff_refs = Gitlab::Diff::DiffRefs.new( + base_sha: base_sha, + start_sha: start_sha, + head_sha: merge_ref_head.id) + + old_diff_refs = merge_request.diff_refs + + return if new_diff_refs == old_diff_refs + + [old_diff_refs, new_diff_refs] + end + end +end diff --git a/app/services/discussions/capture_diff_note_positions_service.rb b/app/services/discussions/capture_diff_note_positions_service.rb new file mode 100644 index 0000000000000000..3684a3f679a242bb --- /dev/null +++ b/app/services/discussions/capture_diff_note_positions_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Discussions + class CaptureDiffNotePositionsService + def initialize(merge_request) + @merge_request = merge_request + end + + def execute + return unless merge_request.has_complete_diff_refs? + + discussions, paths = build_discussions + + service = Discussions::CaptureDiffNotePositionService.new(merge_request, paths) + + discussions.each do |discussion| + service.execute(discussion) + end + end + + private + + attr_reader :merge_request + + def build_discussions + active_diff_discussions = merge_request.notes.new_diff_notes.discussions.select do |discussion| + discussion.active?(merge_request.diff_refs) + end + paths = active_diff_discussions.flat_map { |n| n.diff_file.paths } + + [active_diff_discussions, paths] + end + end +end diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 5b79e4d01f2fc618..d3d661a3b75fcb85 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -118,11 +118,18 @@ def update_merge_status if can_git_merge? && merge_to_ref merge_request.mark_as_mergeable + update_diff_discussion_positions! else merge_request.mark_as_unmergeable end end + def update_diff_discussion_positions! + return if Feature.disabled?(:merge_ref_head_comments, merge_request.target_project) + + Discussions::CaptureDiffNotePositionsService.new(merge_request).execute + end + def recheck! if !merge_request.recheck_merge_status? && outdated_merge_ref? merge_request.mark_as_unchecked diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 80bc44859887c7c5..6c1f52ec8669143c 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -65,6 +65,10 @@ def when_saved(note) if Feature.enabled?(:notes_create_service_tracking, project) Gitlab::Tracking.event('Notes::CreateService', 'execute', tracking_data_for(note)) end + + if Feature.enabled?(:merge_ref_head_comments, project) && note.for_merge_request? && note.diff_note? && note.start_of_discussion? + Discussions::CaptureDiffNotePositionService.new(note.noteable, note.diff_file&.paths).execute(note.discussion) + end end def do_commands(note, update_params, message, only_commands) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 515d72add92608b7..5d5e2fe2a3336e44 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -58,6 +58,7 @@ notes: - system_note_metadata - note_diff_file - suggestions +- diff_note_positions - review label_links: - target @@ -134,6 +135,7 @@ merge_requests: - pipelines_for_merge_request - merge_request_assignees - suggestions +- diff_note_positions - unresolved_notes - assignees - reviews @@ -517,6 +519,8 @@ error_tracking_setting: - project suggestions: - note +diff_note_positions: +- note metrics_setting: - project protected_environments: diff --git a/spec/models/diff_note_position_spec.rb b/spec/models/diff_note_position_spec.rb index a00ba35feef787dc..dedb8a8da4d41f1e 100644 --- a/spec/models/diff_note_position_spec.rb +++ b/spec/models/diff_note_position_spec.rb @@ -3,14 +3,35 @@ require 'spec_helper' describe DiffNotePosition, type: :model do - it 'has a position attribute' do - diff_position = build(:diff_position) - line_code = 'bd4b7bfff3a247ccf6e3371c41ec018a55230bcc_534_521' - diff_note_position = build(:diff_note_position, line_code: line_code, position: diff_position) - - expect(diff_note_position.position).to eq(diff_position) - expect(diff_note_position.line_code).to eq(line_code) - expect(diff_note_position.diff_content_type).to eq('text') + describe '.create_or_update_by' do + context 'when a diff note' do + let(:note) { create(:diff_note_on_merge_request) } + let(:diff_position) { build(:diff_position) } + let(:line_code) { 'bd4b7bfff3a247ccf6e3371c41ec018a55230bcc_534_521' } + let(:diff_note_position) { note.diff_note_positions.first } + let(:params) { { diff_type: :head, line_code: line_code, position: diff_position } } + + context 'does not have a diff note position' do + it 'creates a diff note position' do + described_class.create_or_update_for(note, params) + + expect(diff_note_position.position).to eq(diff_position) + expect(diff_note_position.line_code).to eq(line_code) + expect(diff_note_position.diff_content_type).to eq('text') + end + end + + context 'has a diff note position' do + it 'updates the existing diff note position' do + create(:diff_note_position, note: note) + described_class.create_or_update_for(note, params) + + expect(note.diff_note_positions.size).to eq(1) + expect(diff_note_position.position).to eq(diff_position) + expect(diff_note_position.line_code).to eq(line_code) + end + end + end end it 'unique by note_id and diff type' do diff --git a/spec/services/discussions/capture_diff_note_position_service_spec.rb b/spec/services/discussions/capture_diff_note_position_service_spec.rb new file mode 100644 index 0000000000000000..fced2eb7fce6222a --- /dev/null +++ b/spec/services/discussions/capture_diff_note_position_service_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Discussions::CaptureDiffNotePositionService do + context 'image note on diff' do + let!(:note) { create(:image_diff_note_on_merge_request) } + + subject { described_class.new(note.noteable, ['files/images/any_image.png']) } + + it 'is note affected by the service' do + expect(Gitlab::Diff::PositionTracer).not_to receive(:new) + + expect(subject.execute(note.discussion)).to eq(nil) + expect(note.diff_note_positions).to be_empty + end + end + + context 'when empty paths are passed as a param' do + let!(:note) { create(:diff_note_on_merge_request) } + + subject { described_class.new(note.noteable, []) } + + it 'does not calculate positons' do + expect(Gitlab::Diff::PositionTracer).not_to receive(:new) + + expect(subject.execute(note.discussion)).to eq(nil) + expect(note.diff_note_positions).to be_empty + end + end +end diff --git a/spec/services/discussions/capture_diff_note_positions_service_spec.rb b/spec/services/discussions/capture_diff_note_positions_service_spec.rb new file mode 100644 index 0000000000000000..7b1e207f3eb1ffab --- /dev/null +++ b/spec/services/discussions/capture_diff_note_positions_service_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Discussions::CaptureDiffNotePositionsService do + context 'when merge request has a discussion' do + let(:source_branch) { 'compare-with-merge-head-source' } + let(:target_branch) { 'compare-with-merge-head-target' } + let(:merge_request) { create(:merge_request, source_branch: source_branch, target_branch: target_branch) } + let(:project) { merge_request.project } + + let(:offset) { 30 } + let(:first_new_line) { 508 } + let(:second_new_line) { 521 } + + let(:service) { described_class.new(merge_request) } + + def build_position(new_line, diff_refs) + path = 'files/markdown/ruby-style-guide.md' + Gitlab::Diff::Position.new(old_path: path, new_path: path, + new_line: new_line, diff_refs: diff_refs) + end + + def note_for(new_line) + position = build_position(new_line, merge_request.diff_refs) + create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) + end + + def verify_diff_note_position!(note, line) + id, old_line, new_line = note.line_code.split('_') + + expect(new_line).to eq(line.to_s) + expect(note.diff_note_positions.size).to eq(1) + + diff_position = note.diff_note_positions.last + diff_refs = Gitlab::Diff::DiffRefs.new( + base_sha: merge_request.target_branch_sha, + start_sha: merge_request.target_branch_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.position).to eq(build_position(new_line.to_i, diff_refs)) + end + + let!(:first_discussion_note) { note_for(first_new_line) } + let!(:second_discussion_note) { note_for(second_new_line) } + let!(:second_discussion_another_note) do + create(:diff_note_on_merge_request, + project: project, + position: second_discussion_note.position, + discussion_id: second_discussion_note.discussion_id, + noteable: merge_request) + end + + context 'and position of the discussion changed on target branch head' do + it 'diff positions are created for the first notes of the discussions' do + MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + service.execute + + verify_diff_note_position!(first_discussion_note, first_new_line) + verify_diff_note_position!(second_discussion_note, second_new_line) + + expect(second_discussion_another_note.diff_note_positions).to be_empty + end + end + end +end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 8f17e8083e39681a..45519ddf3d38edca 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -33,6 +33,24 @@ expect(merge_request.merge_status).to eq('can_be_merged') end + it 'update diff discussion positions' do + expect_next_instance_of(Discussions::CaptureDiffNotePositionsService) do |service| + expect(service).to receive(:execute) + end + + subject + end + + context 'when merge_ref_head_comments is disabled' do + it 'does not update diff discussion positions' do + stub_feature_flags(merge_ref_head_comments: false) + + expect(Discussions::CaptureDiffNotePositionsService).not_to receive(:new) + + subject + end + end + it 'updates the merge ref' do expect { subject }.to change(merge_request, :merge_ref_head).from(nil) end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index a03b78a9a7a16aa4..c461dd700ecbab10 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -143,10 +143,21 @@ end it 'note is associated with a note diff file' do + MergeRequests::MergeToRefService.new(merge_request.project, merge_request.author).execute(merge_request) + note = described_class.new(project_with_repo, user, new_opts).execute expect(note).to be_persisted expect(note.note_diff_file).to be_present + expect(note.diff_note_positions).to be_present + end + + it 'does not create diff positions merge_ref_head_comments is disabled' do + stub_feature_flags(merge_ref_head_comments: false) + + expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new) + + described_class.new(project_with_repo, user, new_opts).execute end end @@ -160,6 +171,8 @@ end it 'note is not associated with a note diff file' do + expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new) + note = described_class.new(project_with_repo, user, new_opts).execute expect(note).to be_persisted diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index fd41c5c8fe38db3f..47d69ca1f6a6937a 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -73,7 +73,9 @@ module TestEnv 'submodule_inside_folder' => 'b491b92', 'png-lfs' => 'fe42f41', '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-target' => '2f1e176' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily -- GitLab