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