Merge branch 'rendered-diffs-view-fix-sidebyside' into 'master'

Fixing showing line number as 0 on diff and blocks notes on transformed diff lines without mapping

See merge request gitlab-org/gitlab!81536

app/helpers/diff_helper.rb

@@ -60,6 +60,18 @@ module DiffHelper
     html.join.html_safe
   end
 
+  def diff_nomappinginraw_line(line, first_line_num_class, second_line_num_class, content_line_class)
+    css_class = ''
+    css_class = 'old' if line.type == 'old-nomappinginraw'
+    css_class = 'new' if line.type == 'new-nomappinginraw'
+
+    html = [content_tag(:td, '', class: [*first_line_num_class, css_class])]
+    html << content_tag(:td, '', class: [*second_line_num_class, css_class]) if second_line_num_class
+    html << content_tag(:td, diff_line_content(line.rich_text), class: [*content_line_class, 'nomappinginraw', css_class])
+
+    html.join.html_safe
+  end
+
   def diff_line_content(line)
     if line.blank?
       "&nbsp;".html_safe
@@ -74,7 +86,7 @@ module DiffHelper
   end
 
   def diff_link_number(line_type, match, text)
-    line_type == match || text == 0 ? " " : text
+    line_type == match ? " " : text
   end
 
   def parallel_diff_discussions(left, right, diff_file)

app/views/projects/diffs/_line.html.haml

@@ -10,6 +10,8 @@
   - case line.type
   - when 'match'
     = diff_match_line line.old_pos, line.new_pos, text: line.text
+  - when 'old-nomappinginraw', 'new-nomappinginraw', 'unchanged-nomappinginraw'
+    = diff_nomappinginraw_line line, %w[old_line diff-line-num], %w[new_line diff-line-num], %w[line_content]
   - when 'old-nonewline', 'new-nonewline'
     %td.old_line.diff-line-num
     %td.new_line.diff-line-num

app/views/projects/diffs/_parallel_view.html.haml

@@ -11,6 +11,8 @@
           - case left.type
           - when 'match'
             = diff_match_line left.old_pos, nil, text: left.text, view: :parallel
+          - when 'old-nomappinginraw', 'new-nomappinginraw', 'unchanged-nomappinginraw'
+            = diff_nomappinginraw_line left, %w[old_line diff-line-num], nil, %w[line_content parallel left-side]
           - when 'old-nonewline', 'new-nonewline'
             %td.old_line.diff-line-num
             %td.line_content.match.left-side= left.text
@@ -29,6 +31,8 @@
           - case right.type
           - when 'match'
             = diff_match_line nil, right.new_pos, text: left.text, view: :parallel
+          - when 'old-nomappinginraw', 'new-nomappinginraw', 'unchanged-nomappinginraw'
+            = diff_nomappinginraw_line right, %w[new_line diff-line-num], nil, %w[line_content parallel right-side]
           - when 'old-nonewline', 'new-nonewline'
             %td.new_line.diff-line-num
             %td.line_content.match.right-side= right.text

app/views/projects/diffs/_text_file.html.haml

@@ -12,6 +12,8 @@
         - case line.type
         - when 'match'
           = diff_match_line line.old_pos, line.new_pos, text: line.text
+        - when 'old-nomappinginraw', 'new-nomappinginraw', 'unchanged-nomappinginraw'
+          = diff_nomappinginraw_line line, %w[old_line diff-line-num], %w[new_line diff-line-num], %w[line_content]
         - when 'old-nonewline', 'new-nonewline'
           %td.old_line.diff-line-num
           %td.new_line.diff-line-num

lib/gitlab/diff/line.rb

@@ -9,8 +9,8 @@ module Gitlab
       SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze
 
       attr_reader :marker_ranges
-      attr_writer :text, :rich_text, :discussable
-      attr_accessor :index, :type, :old_pos, :new_pos, :line_code
+      attr_writer :text, :rich_text
+      attr_accessor :index, :old_pos, :new_pos, :line_code, :type
 
       def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil)
         @text = text
@@ -24,9 +24,7 @@ module Gitlab
         # When line code is not provided from cache store we build it
         # using the parent_file(Diff::File or Conflict::File).
         @line_code = line_code || calculate_line_code
-
         @marker_ranges = []
-        @discussable = true
       end
 
       def self.init_from_hash(hash)
@@ -81,23 +79,28 @@ module Gitlab
       end
 
       def added?
-        %w[new new-nonewline].include?(type)
+        %w[new new-nonewline new-nomappinginraw].include?(type)
       end
 
       def removed?
-        %w[old old-nonewline].include?(type)
+        %w[old old-nonewline old-nomappinginraw].include?(type)
       end
 
       def meta?
         %w[match new-nonewline old-nonewline].include?(type)
       end
 
+      def has_mapping_in_raw?
+        # Used for rendered diff, when the displayed line doesn't have a matching line in the raw diff
+        !type&.ends_with?('nomappinginraw')
+      end
+
       def match?
         type == :match
       end
 
       def discussable?
-        @discussable && !meta?
+        has_mapping_in_raw? && !meta?
       end
 
       def suggestible?

lib/gitlab/diff/parallel_diff.rb

@@ -44,7 +44,7 @@ module Gitlab
               free_right_index = nil
               i += 1
             end
-          elsif line.meta? || line.unchanged?
+          elsif line.meta? || line.unchanged? || !line.has_mapping_in_raw?
             # line in the right panel is the same as in the left one
             lines << {
               left: line,

lib/gitlab/diff/rendered/notebook/diff_file.rb

@@ -87,10 +87,7 @@ module Gitlab
             line.new_pos = removal_line_maps[line.old_pos] if line.new_pos == 0 && line.old_pos != 0
 
             # Lines that do not appear on the original diff should not be commentable
-
-            unless addition_line_maps[line.new_pos] || removal_line_maps[line.old_pos]
-              line.discussable = false
-            end
+            line.type = "#{line.type || 'unchanged'}-nomappinginraw" unless addition_line_maps[line.new_pos] || removal_line_maps[line.old_pos]
 
             line.line_code = line_code(line)
             line
@@ -113,8 +110,8 @@ module Gitlab
             additions = {}
 
             source_diff.highlighted_diff_lines.each do |line|
-              removals[line.old_pos] = line.new_pos
-              additions[line.new_pos] = line.old_pos
+              removals[line.old_pos] = line.new_pos unless source_diff.new_file?
+              additions[line.new_pos] = line.old_pos unless source_diff.deleted_file?
             end
 
             [removals, additions]

spec/features/projects/commits/multi_view_diff_spec.rb

@@ -27,17 +27,11 @@ RSpec.describe 'Multiple view Diffs', :js do
 
   context 'when :rendered_diffs_viewer is off' do
     context 'and diff does not have ipynb' do
-      include_examples "no multiple viewers", 'ddd0f15ae83993f5cb66a927a28673882e99100b'
+      it_behaves_like "no multiple viewers", 'ddd0f15ae83993f5cb66a927a28673882e99100b'
     end
 
     context 'and diff has ipynb' do
-      include_examples "no multiple viewers", '5d6ed1503801ca9dc28e95eeb85a7cf863527aee'
-
-      it 'shows the transformed diff' do
-        diff = page.find('.diff-file, .file-holder', match: :first)
-
-        expect(diff['innerHTML']).to include('%% Cell type:markdown id:0aac5da7-745c-4eda-847a-3d0d07a1bb9b tags:')
-      end
+      it_behaves_like "no multiple viewers", '5d6ed1503801ca9dc28e95eeb85a7cf863527aee'
     end
   end
 
@@ -45,14 +39,28 @@ RSpec.describe 'Multiple view Diffs', :js do
     let(:feature_flag_on) { true }
 
     context 'and diff does not include ipynb' do
-      include_examples "no multiple viewers", 'ddd0f15ae83993f5cb66a927a28673882e99100b'
+      it_behaves_like "no multiple viewers", 'ddd0f15ae83993f5cb66a927a28673882e99100b'
+
+      context 'and in inline diff' do
+        let(:ref) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
+
+        it 'does not change display for non-ipynb' do
+          expect(page).to have_selector line_with_content('new', 1)
+        end
       end
 
-    context 'and opening a diff with ipynb' do
-      context 'but the changes are not renderable' do
-        include_examples "no multiple viewers", 'a867a602d2220e5891b310c07d174fbe12122830'
+      context 'and in parallel diff' do
+        let(:ref) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
+
+        it 'does not change display for non-ipynb' do
+          page.find('#parallel-diff-btn').click
+
+          expect(page).to have_selector line_with_content('new', 1)
+        end
+      end
     end
 
+    context 'and opening a diff with ipynb' do
       it 'loads the rendered diff as hidden' do
         diff = page.find('.diff-file, .file-holder', match: :first)
 
@@ -76,10 +84,55 @@ RSpec.describe 'Multiple view Diffs', :js do
         expect(classes_for_element(diff, 'toHideBtn')).not_to include('selected')
         expect(classes_for_element(diff, 'toShowBtn')).to include('selected')
       end
+
+      it 'transforms the diff' do
+        diff = page.find('.diff-file, .file-holder', match: :first)
+
+        expect(diff['innerHTML']).to include('%% Cell type:markdown id:0aac5da7-745c-4eda-847a-3d0d07a1bb9b tags:')
+      end
+
+      context 'on parallel view' do
+        before do
+          page.find('#parallel-diff-btn').click
+        end
+
+        it 'lines without mapping cannot receive comments' do
+          expect(page).not_to have_selector('td.line_content.nomappinginraw ~ td.diff-line-num > .add-diff-note')
+          expect(page).to have_selector('td.line_content:not(.nomappinginraw) ~ td.diff-line-num > .add-diff-note')
+        end
+
+        it 'lines numbers without mapping are empty' do
+          expect(page).not_to have_selector('td.nomappinginraw + td.diff-line-num')
+          expect(page).to have_selector('td.nomappinginraw + td.diff-line-num', visible: false)
+        end
+
+        it 'transforms the diff' do
+          diff = page.find('.diff-file, .file-holder', match: :first)
+
+          expect(diff['innerHTML']).to include('%% Cell type:markdown id:0aac5da7-745c-4eda-847a-3d0d07a1bb9b tags:')
+        end
+      end
+
+      context 'on inline view' do
+        it 'lines without mapping cannot receive comments' do
+          expect(page).not_to have_selector('tr.line_holder[class$="nomappinginraw"] > td.diff-line-num > .add-diff-note')
+          expect(page).to have_selector('tr.line_holder:not([class$="nomappinginraw"]) > td.diff-line-num > .add-diff-note')
+        end
+
+        it 'lines numbers without mapping are empty' do
+          elements = page.all('tr.line_holder[class$="nomappinginraw"] > td.diff-line-num').map { |e| e.text(:all) }
+
+          expect(elements).to all(be == "")
+        end
+      end
     end
   end
 
   def classes_for_element(node, data_diff_entity, visible: true)
     node.find("[data-diff-toggle-entity=\"#{data_diff_entity}\"]", visible: visible)[:class]
   end
+
+  def line_with_content(old_or_new, line_number)
+    "td.#{old_or_new}_line.diff-line-num[data-linenumber=\"#{line_number}\"] > a[data-linenumber=\"#{line_number}\"]"
+  end
 end

spec/helpers/diff_helper_spec.rb

@@ -290,6 +290,53 @@ RSpec.describe DiffHelper do
     end
   end
 
+  describe "#diff_nomappinginraw_line" do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:line) { double("line") }
+    let(:line_type) { 'line_type' }
+
+    before do
+      allow(line).to receive(:rich_text).and_return('line_text')
+      allow(line).to receive(:type).and_return(line_type)
+    end
+
+    it 'generates only single line num' do
+      output = diff_nomappinginraw_line(line, ['line_num_1'], nil, ['line_content'])
+
+      expect(output).to be_html_safe
+      expect(output).to have_css 'td:nth-child(1).line_num_1'
+      expect(output).to have_css 'td:nth-child(2).line_content', text: 'line_text'
+      expect(output).not_to have_css 'td:nth-child(3)'
+    end
+
+    it 'generates only both line nums' do
+      output = diff_nomappinginraw_line(line, ['line_num_1'], ['line_num_2'], ['line_content'])
+
+      expect(output).to be_html_safe
+      expect(output).to have_css 'td:nth-child(1).line_num_1'
+      expect(output).to have_css 'td:nth-child(2).line_num_2'
+      expect(output).to have_css 'td:nth-child(3).line_content', text: 'line_text'
+    end
+
+    where(:line_type, :added_class) do
+      'old-nomappinginraw'       | '.old'
+      'new-nomappinginraw'       | '.new'
+      'unchanged-nomappinginraw' | ''
+    end
+
+    with_them do
+      it "appends the correct class" do
+        output = diff_nomappinginraw_line(line, ['line_num_1'], ['line_num_2'], ['line_content'])
+
+        expect(output).to be_html_safe
+        expect(output).to have_css 'td:nth-child(1).line_num_1' + added_class
+        expect(output).to have_css 'td:nth-child(2).line_num_2' + added_class
+        expect(output).to have_css 'td:nth-child(3).line_content' + added_class, text: 'line_text'
+      end
+    end
+  end
+
   describe '#render_overflow_warning?' do
     using RSpec::Parameterized::TableSyntax
 

spec/lib/gitlab/diff/file_spec.rb

@@ -66,6 +66,12 @@ RSpec.describe Gitlab::Diff::File do
       it 'does not have renderable viewer' do
         expect(diff_file.has_renderable?).to be_falsey
       end
+
+      it 'does not create a Notebook DiffFile' do
+        expect(diff_file.rendered).to be_nil
+
+        expect(::Gitlab::Diff::Rendered::Notebook::DiffFile).not_to receive(:new)
+      end
     end
   end