Merge branch 'osw-multi-line-suggestions-creation-strategy' into 'master'

Prepares suggestion implementation for multi-line support

See merge request gitlab-org/gitlab-ce!26057

8 files changed, 99 insertions, 60 deletions

diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 87755cf3f3d..feabea9b8ba 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -77,8 +77,8 @@ class DiffNote < Note
   def supports_suggestion?
     return false unless noteable.supports_suggestion? && on_text?
     # We don't want to trigger side-effects of `diff_file` call.
-    return false unless file = fetch_diff_file
-    return false unless line = file.line_for_position(self.original_position)
+    return false unless file = latest_diff_file
+    return false unless line = file.line_for_position(self.position)
 
     line&.suggestible?
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2c0692e1b45..19557fd476e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -66,6 +66,7 @@ class MergeRequest < ActiveRecord::Base
 
   has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
   has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
+  has_many :suggestions, through: :notes
 
   has_many :merge_request_assignees
   # Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457
diff --git a/app/models/suggestion.rb b/app/models/suggestion.rb
index 09034646bff..22e2f11230d 100644
--- a/app/models/suggestion.rb
+++ b/app/models/suggestion.rb
@@ -1,11 +1,19 @@
 # frozen_string_literal: true
 
 class Suggestion < ApplicationRecord
+  include Suggestible
+
   belongs_to :note, inverse_of: :suggestions
   validates :note, presence: true
   validates :commit_id, presence: true, if: :applied?
 
-  delegate :original_position, :position, :noteable, to: :note
+  delegate :position, :noteable, to: :note
+
+  scope :active, -> { where(outdated: false) }
+
+  def diff_file
+    note.latest_diff_file
+  end
 
   def project
     noteable.source_project
@@ -19,37 +27,37 @@ class Suggestion < ApplicationRecord
     position.file_path
   end
 
-  # For now, suggestions only serve as a way to send patches that
-  # will change a single line (being able to apply multiple in the same place),
-  # which explains `from_line` and `to_line` being the same line.
-  # We'll iterate on that in https://gitlab.com/gitlab-org/gitlab-ce/issues/53310
-  # when allowing multi-line suggestions.
-  def from_line
-    position.new_line
-  end
-  alias_method :to_line, :from_line
-
-  def from_original_line
-    original_position.new_line
-  end
-  alias_method :to_original_line, :from_original_line
-
   # `from_line_index` and `to_line_index` represents diff/blob line numbers in
   # index-like way (N-1).
   def from_line_index
     from_line - 1
   end
-  alias_method :to_line_index, :from_line_index
 
-  def appliable?
-    return false unless note.supports_suggestion?
+  def to_line_index
+    to_line - 1
+  end
 
+  def appliable?(cached: true)
     !applied? &&
       noteable.opened? &&
+      !outdated?(cached: cached) &&
+      note.supports_suggestion? &&
       different_content? &&
       note.active?
   end
 
+  # Overwrites outdated column
+  def outdated?(cached: true)
+    return super() if cached
+    return true unless diff_file
+
+    from_content != fetch_from_content
+  end
+
+  def target_line
+    position.new_line
+  end
+
   private
 
   def different_content?
diff --git a/app/services/concerns/suggestible.rb b/app/services/concerns/suggestible.rb
new file mode 100644
index 00000000000..0b9822b1909
--- /dev/null
+++ b/app/services/concerns/suggestible.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Suggestible
+  extend ActiveSupport::Concern
+
+  # This translates into limiting suggestion changes to `suggestion:-100+100`.
+  MAX_LINES_CONTEXT = 100.freeze
+
+  def fetch_from_content
+    diff_file.new_blob_lines_between(from_line, to_line).join
+  end
+
+  def from_line
+    real_above = [lines_above, MAX_LINES_CONTEXT].min
+    [target_line - real_above, 1].max
+  end
+
+  def to_line
+    real_below = [lines_below, MAX_LINES_CONTEXT].min
+    target_line + real_below
+  end
+
+  def diff_file
+    raise NotImplementedError
+  end
+
+  def target_line
+    raise NotImplementedError
+  end
+end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index f712b8863cd..f5dc5a0256d 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -20,6 +20,7 @@ module MergeRequests
       close_upon_missing_source_branch_ref
       post_merge_manually_merged
       reload_merge_requests
+      outdate_suggestions
       reset_merge_when_pipeline_succeeds
       mark_pending_todos_done
       cache_merge_requests_closing_issues
@@ -125,6 +126,14 @@ module MergeRequests
         merge_request.source_branch == @push.branch_name
     end
 
+    def outdate_suggestions
+      outdate_service = Suggestions::OutdateService.new
+
+      merge_requests_for_source_branch.each do |merge_request|
+        outdate_service.execute(merge_request)
+      end
+    end
+
     def reset_merge_when_pipeline_succeeds
       merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
     end
diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb
index f778c5aa5f5..8ba50e22b09 100644
--- a/app/services/suggestions/apply_service.rb
+++ b/app/services/suggestions/apply_service.rb
@@ -7,7 +7,7 @@ module Suggestions
     end
 
     def execute(suggestion)
-      unless suggestion.appliable?
+      unless suggestion.appliable?(cached: false)
         return error('Suggestion is not appliable')
       end
 
@@ -15,7 +15,7 @@ module Suggestions
         return error('The file has been changed')
       end
 
-      diff_file = suggestion.note.latest_diff_file
+      diff_file = suggestion.diff_file
 
       unless diff_file
         return error('The file was not found')
diff --git a/app/services/suggestions/create_service.rb b/app/services/suggestions/create_service.rb
index c7ac2452c53..1d3338c1b45 100644
--- a/app/services/suggestions/create_service.rb
+++ b/app/services/suggestions/create_service.rb
@@ -9,52 +9,24 @@ module Suggestions
     def execute
       return unless @note.supports_suggestion?
 
-      diff_file = @note.latest_diff_file
-
-      return unless diff_file
-
-      suggestions = Banzai::SuggestionsParser.parse(@note.note)
-
-      # For single line suggestion we're only looking forward to
-      # change the line receiving the comment. Though, in
-      # https://gitlab.com/gitlab-org/gitlab-ce/issues/53310
-      # we'll introduce a ```suggestion:L<x>-<y>, so this will
-      # slightly change.
-      comment_line = @note.position.new_line
+      suggestions = Gitlab::Diff::SuggestionsParser.parse(@note.note,
+                                                          project: @note.project,
+                                                          position: @note.position)
 
       rows =
         suggestions.map.with_index do |suggestion, index|
-          from_content = changing_lines(diff_file, comment_line, comment_line)
+          creation_params =
+            suggestion.to_hash.slice(:from_content,
+                                     :to_content,
+                                     :lines_above,
+                                     :lines_below)
 
-          # The parsed suggestion doesn't have information about the correct
-          # ending characters (we may have a line break, or not), so we take
-          # this information from the last line being changed (last
-          # characters).
-          endline_chars = line_break_chars(from_content.lines.last)
-          to_content = "#{suggestion}#{endline_chars}"
-
-          {
-            note_id: @note.id,
-            from_content: from_content,
-            to_content: to_content,
-            relative_order: index
-          }
+          creation_params.merge!(note_id: @note.id, relative_order: index)
         end
 
       rows.in_groups_of(100, false) do |rows|
         Gitlab::Database.bulk_insert('suggestions', rows)
       end
     end
-
-    private
-
-    def changing_lines(diff_file, from_line, to_line)
-      diff_file.new_blob_lines_between(from_line, to_line).join
-    end
-
-    def line_break_chars(line)
-      match = /\r\n|\r|\n/.match(line)
-      match[0] if match
-    end
   end
 end
diff --git a/app/services/suggestions/outdate_service.rb b/app/services/suggestions/outdate_service.rb
new file mode 100644
index 00000000000..a33aac9f6b5
--- /dev/null
+++ b/app/services/suggestions/outdate_service.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Suggestions
+  class OutdateService
+    def execute(merge_request)
+      # rubocop: disable CodeReuse/ActiveRecord
+      suggestions = merge_request.suggestions.active.includes(:note)
+
+      suggestions.find_in_batches(batch_size: 100) do |group|
+        outdatable_suggestion_ids = group.select do |suggestion|
+          suggestion.outdated?(cached: false)
+        end.map(&:id)
+
+        Suggestion.where(id: outdatable_suggestion_ids).update_all(outdated: true)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+    end
+  end
+end