Merge branch 'id-capture-diff-positions-service' into 'master'

Capture diff note position on mergeability check

See merge request gitlab-org/gitlab!28623

app/models/diff_note_position.rb

@@ -28,9 +28,20 @@ class DiffNotePosition < ApplicationRecord
   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

app/models/note.rb

@@ -83,6 +83,7 @@ class Note < ApplicationRecord
   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

app/services/discussions/capture_diff_note_position_service.rb

+# 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

app/services/discussions/capture_diff_note_positions_service.rb

+# 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

app/services/merge_requests/mergeability_check_service.rb

@@ -118,11 +118,18 @@ module MergeRequests
 
       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

app/services/notes/create_service.rb

@@ -65,6 +65,10 @@ module Notes
       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)

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:

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

spec/services/discussions/capture_diff_note_position_service_spec.rb

+# 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

spec/services/discussions/capture_diff_note_positions_service_spec.rb

+# 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

spec/services/merge_requests/mergeability_check_service_spec.rb

@@ -33,6 +33,24 @@ describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shared_sta
       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

spec/services/notes/create_service_spec.rb

@@ -143,10 +143,21 @@ describe Notes::CreateService do
         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 @@ describe Notes::CreateService do
         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

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