Merge branch 'osw-revert-comment-in-any-diff-line' into 'master'

Revert "Merge branch 'osw-comment-on-any-line-on-diffs' into 'master'"

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

Author: smcgivern

app/assets/javascripts/diffs/components/diff_line_gutter_content.vue

@@ -55,6 +55,11 @@ export default {
       required: false,
       default: false,
     },
+    isContextLine: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
     isHover: {
       type: Boolean,
       required: false,
@@ -76,6 +81,7 @@ export default {
         this.showCommentButton &&
         this.isHover &&
         !this.isMatchLine &&
+        !this.isContextLine &&
         !this.isMetaLine &&
         !this.hasDiscussions
       );

app/assets/javascripts/diffs/components/diff_table_cell.vue

@@ -3,6 +3,7 @@ import { mapGetters } from 'vuex';
 import DiffLineGutterContent from './diff_line_gutter_content.vue';
 import {
   MATCH_LINE_TYPE,
+  CONTEXT_LINE_TYPE,
   EMPTY_CELL_TYPE,
   OLD_LINE_TYPE,
   OLD_NO_NEW_LINE_TYPE,
@@ -70,6 +71,9 @@ export default {
     isMatchLine() {
       return this.line.type === MATCH_LINE_TYPE;
     },
+    isContextLine() {
+      return this.line.type === CONTEXT_LINE_TYPE;
+    },
     isMetaLine() {
       const { type } = this.line;
 
@@ -84,7 +88,11 @@ export default {
         [type]: type,
         [LINE_UNFOLD_CLASS_NAME]: this.isMatchLine,
         [LINE_HOVER_CLASS_NAME]:
-          this.isLoggedIn && this.isHover && !this.isMatchLine && !this.isMetaLine,
+          this.isLoggedIn &&
+          this.isHover &&
+          !this.isMatchLine &&
+          !this.isContextLine &&
+          !this.isMetaLine,
       };
     },
     lineNumber() {

app/assets/javascripts/diffs/components/inline_diff_table_row.vue

@@ -4,6 +4,8 @@ import DiffTableCell from './diff_table_cell.vue';
 import {
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
+  CONTEXT_LINE_TYPE,
+  CONTEXT_LINE_CLASS_NAME,
   PARALLEL_DIFF_VIEW_TYPE,
   LINE_POSITION_LEFT,
   LINE_POSITION_RIGHT,
@@ -39,9 +41,13 @@ export default {
   },
   computed: {
     ...mapGetters('diffs', ['isInlineView']),
+    isContextLine() {
+      return this.line.type === CONTEXT_LINE_TYPE;
+    },
     classNameMap() {
       return {
         [this.line.type]: this.line.type,
+        [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
         [PARALLEL_DIFF_VIEW_TYPE]: this.isParallelView,
       };
     },

app/assets/javascripts/diffs/components/parallel_diff_table_row.vue

@@ -5,6 +5,8 @@ import DiffTableCell from './diff_table_cell.vue';
 import {
   NEW_LINE_TYPE,
   OLD_LINE_TYPE,
+  CONTEXT_LINE_TYPE,
+  CONTEXT_LINE_CLASS_NAME,
   OLD_NO_NEW_LINE_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
   NEW_NO_NEW_LINE_TYPE,
@@ -41,8 +43,12 @@ export default {
     };
   },
   computed: {
+    isContextLine() {
+      return this.line.left && this.line.left.type === CONTEXT_LINE_TYPE;
+    },
     classNameMap() {
       return {
+        [CONTEXT_LINE_CLASS_NAME]: this.isContextLine,
         [PARALLEL_DIFF_VIEW_TYPE]: true,
       };
     },

app/assets/javascripts/diffs/constants.js

@@ -3,6 +3,7 @@ export const PARALLEL_DIFF_VIEW_TYPE = 'parallel';
 export const MATCH_LINE_TYPE = 'match';
 export const OLD_NO_NEW_LINE_TYPE = 'old-nonewline';
 export const NEW_NO_NEW_LINE_TYPE = 'new-nonewline';
+export const CONTEXT_LINE_TYPE = 'context';
 export const EMPTY_CELL_TYPE = 'empty-cell';
 export const COMMENT_FORM_TYPE = 'commentForm';
 export const DIFF_NOTE_TYPE = 'DiffNote';
@@ -21,6 +22,7 @@ export const LINE_SIDE_RIGHT = 'right-side';
 export const DIFF_VIEW_COOKIE_NAME = 'diff_view';
 export const LINE_HOVER_CLASS_NAME = 'is-over';
 export const LINE_UNFOLD_CLASS_NAME = 'unfold js-unfold';
+export const CONTEXT_LINE_CLASS_NAME = 'diff-expanded';
 
 export const UNFOLD_COUNT = 20;
 export const COUNT_OF_AVATARS_IN_GUTTER = 3;

app/assets/javascripts/diffs/store/mutations.js

@@ -65,13 +65,7 @@ export default {
     const { highlightedDiffLines, parallelDiffLines } = diffFile;
 
     removeMatchLine(diffFile, lineNumbers, bottom);
-
-    const lines = addLineReferences(contextLines, lineNumbers, bottom).map(line => ({
-      ...line,
-      lineCode: line.lineCode || `${fileHash}_${line.oldLine}_${line.newLine}`,
-      discussions: line.discussions || [],
-    }));
-
+    const lines = addLineReferences(contextLines, lineNumbers, bottom);
     addContextLines({
       inlineLines: highlightedDiffLines,
       parallelLines: parallelDiffLines,

app/controllers/projects/blob_controller.rb

@@ -122,7 +122,7 @@ def render_diff_lines
     @lines.map! do |line|
       # These are marked as context lines but are loaded from blobs.
       # We also have context lines loaded from diffs in other places.
-      diff_line = Gitlab::Diff::Line.new(line, nil, nil, nil, nil)
+      diff_line = Gitlab::Diff::Line.new(line, 'context', nil, nil, nil)
       diff_line.rich_text = line
       diff_line
     end

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -22,12 +22,6 @@ def diff_for_path
 
   def render_diffs
     @environment = @merge_request.environments_for(current_user).last
-    notes_grouped_by_path = @notes.group_by { |note| note.position.file_path }
-
-    @diffs.diff_files.each do |diff_file|
-      notes = notes_grouped_by_path.fetch(diff_file.file_path, [])
-      notes.each { |note| diff_file.unfold_diff_lines(note.position) }
-    end
 
     @diffs.write_cache
 

app/models/diff_note.rb

@@ -66,10 +66,6 @@ def created_at_diff?(diff_refs)
     self.original_position.diff_refs == diff_refs
   end
 
-  def discussion_first_note?
-    self == discussion.first_note
-  end
-
   private
 
   def enqueue_diff_file_creation_job
@@ -82,33 +78,26 @@ def enqueue_diff_file_creation_job
   end
 
   def should_create_diff_file?
-    on_text? && note_diff_file.nil? && discussion_first_note?
+    on_text? && note_diff_file.nil? && self == discussion.first_note
   end
 
   def fetch_diff_file
-    file =
-      if note_diff_file
-        diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
-        Gitlab::Diff::File.new(diff,
-                               repository: project.repository,
-                               diff_refs: original_position.diff_refs)
-      elsif created_at_diff?(noteable.diff_refs)
-        # We're able to use the already persisted diffs (Postgres) if we're
-        # presenting a "current version" of the MR discussion diff.
-        # So no need to make an extra Gitaly diff request for it.
-        # As an extra benefit, the returned `diff_file` already
-        # has `highlighted_diff_lines` data set from Redis on
-        # `Diff::FileCollection::MergeRequestDiff`.
-        noteable.diffs(original_position.diff_options).diff_files.first
-      else
-        original_position.diff_file(self.project.repository)
-      end
-
-    # Since persisted diff files already have its content "unfolded"
-    # there's no need to make it pass through the unfolding process.
-    file&.unfold_diff_lines(position) unless note_diff_file
-
-    file
+    if note_diff_file
+      diff = Gitlab::Git::Diff.new(note_diff_file.to_hash)
+      Gitlab::Diff::File.new(diff,
+                             repository: project.repository,
+                             diff_refs: original_position.diff_refs)
+    elsif created_at_diff?(noteable.diff_refs)
+      # We're able to use the already persisted diffs (Postgres) if we're
+      # presenting a "current version" of the MR discussion diff.
+      # So no need to make an extra Gitaly diff request for it.
+      # As an extra benefit, the returned `diff_file` already
+      # has `highlighted_diff_lines` data set from Redis on
+      # `Diff::FileCollection::MergeRequestDiff`.
+      noteable.diffs(original_position.diff_options).diff_files.first
+    else
+      original_position.diff_file(self.project.repository)
+    end
   end
 
   def supported?

app/services/merge_requests/reload_diffs_service.rb

@@ -29,6 +29,10 @@ def update_diff_discussion_positions(old_diff_refs)
 
     # rubocop: disable CodeReuse/ActiveRecord
     def clear_cache(new_diff)
+      # Executing the iteration we cache highlighted diffs for each diff file of
+      # MergeRequestDiff.
+      cacheable_collection(new_diff).write_cache
+
       # Remove cache for all diffs on this MR. Do not use the association on the
       # model, as that will interfere with other actions happening when
       # reloading the diff.

app/services/notes/base_service.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module Notes
-  class CreateService < ::Notes::BaseService
+  class CreateService < ::BaseService
     def execute
       merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
 
@@ -35,7 +35,6 @@ def execute
 
       if !only_commands && note.save
         todo_service.new_note(note, current_user)
-        clear_noteable_diffs_cache(note)
       end
 
       if command_params.present?

app/services/notes/create_service.rb

@@ -1,13 +1,11 @@
 # frozen_string_literal: true
 
 module Notes
-  class DestroyService < ::Notes::BaseService
+  class DestroyService < BaseService
     def execute(note)
       TodoService.new.destroy_target(note) do |note|
         note.destroy
       end
-
-      clear_noteable_diffs_cache(note)
     end
   end
 end

app/services/notes/destroy_service.rb

@@ -26,7 +26,6 @@ def initialize(diff, repository:, diff_refs: nil, fallback_diff_refs: nil, stats
         @repository = repository
         @diff_refs = diff_refs
         @fallback_diff_refs = fallback_diff_refs
-        @unfolded = false
 
         # Ensure items are collected in the the batch
         new_blob_lazy
@@ -136,24 +135,6 @@ def diff_lines
           Gitlab::Diff::Parser.new.parse(raw_diff.each_line, diff_file: self).to_a
       end
 
-      # Changes diff_lines according to the given position. That is,
-      # it checks whether the position requires blob lines into the diff
-      # in order to be presented.
-      def unfold_diff_lines(position)
-        return unless position
-
-        unfolder = Gitlab::Diff::LinesUnfolder.new(self, position)
-
-        if unfolder.unfold_required?
-          @diff_lines = unfolder.unfolded_diff_lines
-          @unfolded = true
-        end
-      end
-
-      def unfolded?
-        @unfolded
-      end
-
       def highlighted_diff_lines
         @highlighted_diff_lines ||=
           Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight

changelogs/unreleased/osw-comment-on-any-line-on-diffs.yml

@@ -3,9 +3,9 @@ module Diff
     class Line
       SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze
 
-      attr_reader :line_code, :type, :old_pos, :new_pos
+      attr_reader :line_code, :type, :index, :old_pos, :new_pos
       attr_writer :rich_text
-      attr_accessor :text, :index
+      attr_accessor :text
 
       def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil)
         @text, @type, @index = text, type, index
@@ -19,14 +19,7 @@ def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code:
       end
 
       def self.init_from_hash(hash)
-        new(hash[:text],
-            hash[:type],
-            hash[:index],
-            hash[:old_pos],
-            hash[:new_pos],
-            parent_file: hash[:parent_file],
-            line_code: hash[:line_code],
-            rich_text: hash[:rich_text])
+        new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code], rich_text: hash[:rich_text])
       end
 
       def to_hash