Merge pull request #13910 from hellcp-work/diff_component

Move new diffs to a component

src/api/app/assets/javascripts/webui/packages.js

@@ -1,3 +1,4 @@
+// Remove this after PackageController#rdiff moves to DiffListComponent
 $(function ($) {
   $('body').on('click', '.expand-diffs', function () {
     var forPackage = $(this).data('package');

src/api/app/assets/javascripts/webui/request.js

@@ -196,3 +196,17 @@ function loadChanges() { // jshint ignore:line
     }
   });
 }
+
+$(function ($) {
+  $('body').on('click', '.expand-diffs', function () {
+    var forObject = $(this).data('object');
+    var details = $('.collapse[data-object="' + forObject + '"]');
+    details.addClass('show');
+  });
+
+  $('body').on('click', '.collapse-diffs', function () {
+    var forObject = $(this).data('object');
+    var details = $('.collapse[data-object="' + forObject + '"]');
+    details.removeClass('show');
+  });
+});

src/api/app/components/diff_component.html.haml

@@ -0,0 +1,21 @@
+.pre.scroll
+  .diff{ class: "digits-#{diff.max_digits}" }>
+    - diff.lines&.each do |line|
+      - id = "diff_#{file_index}_n#{line.index}"
+      %div{ class: "line #{line.state}", id: }>
+        - case line.state
+        - when 'comment', 'range'
+          .offset
+            .text-muted.text
+              = line.content
+        - else
+          = link_to("##{id}", class: 'd-flex') do
+            .number<
+              = line.original_index
+            .number<
+              = line.changed_index
+          .value<
+            %span.character><
+              = line.content[0]
+            %span.content><
+              = line.content[1..]

src/api/app/components/diff_component.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class DiffComponent < ApplicationComponent
+  attr_reader :diff, :file_index
+
+  def initialize(diff:, file_index:)
+    super
+    @diff = parse_diff(diff)
+    @file_index = file_index
+  end
+
+  def render?
+    diff.present?
+  end
+
+  private
+
+  def parse_diff(content)
+    DiffParser.new(content: content).call
+  end
+end

src/api/app/components/diff_list_component.html.haml

@@ -0,0 +1,15 @@
+- diff_list.each_with_index do |(name, file_info), file_index|
+  - state = file_info['state']
+  .accordion.mb-2{ id: "diff-list-#{name.parameterize}" }
+    .accordion-item
+      %h2.accordion-header
+        %button.accordion-button.text-bg-light{ type: 'button', data: { 'bs-toggle': 'collapse',
+                                                                        'bs-target': "#diff-item-#{name.parameterize}" },
+                                                aria: { expanded: 'true', controls: "diff-item-#{name.parameterize}" } }
+          - if (old_file = file_info['old'])
+            = changed_filename(old_file['name'], name, state)
+          - else
+            = name
+          %span.badge.ms-1{ class: badge_for_state(state) }= state.capitalize
+      .accordion-collapse.collapse{ class: expand?(name, state) ? 'show' : '', id: "diff-item-#{name.parameterize}", 'data-object': view_id }
+        = render(DiffComponent.new(diff: file_info.dig('diff', '_content'), file_index:))

src/api/app/components/diff_list_component.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+class DiffListComponent < ApplicationComponent
+  attr_reader :diff_list, :view_id
+
+  def initialize(diff_list:, view_id: nil)
+    super
+    @diff_list = diff_list
+    @view_id = view_id
+  end
+
+  def badge_for_state(state)
+    return 'text-bg-success' if state == 'added'
+    return 'text-bg-danger' if state == 'deleted'
+
+    'text-bg-info'
+  end
+
+  def changed_filename(old_filename, new_filename, state)
+    return new_filename unless ['changed', 'renamed'].include?(state)
+    return new_filename if old_filename == new_filename
+
+    "#{old_filename} -> #{new_filename}"
+  end
+
+  def expand?(filename, state)
+    state != 'deleted' && filename.exclude?('/') && (filename == '_patchinfo' || filename.ends_with?('.spec', '.changes'))
+  end
+end

src/api/app/components/sourcediff_component.html.haml

@@ -9,9 +9,9 @@
       - (@action[:sourcediff] || []).each do |sourcediff|
         .clearfix.mb-2
           .btn-group.float-end
-            %button.btn.btn-outline-secondary.expand-diffs{ data: { package: @action[:spkg] } }
+            %button.btn.btn-outline-secondary.expand-diffs{ data: { object: @action[:spkg] } }
               Expand all
-            %button.btn.btn-outline-secondary.collapse-diffs{ data: { package: @action[:spkg] } }
+            %button.btn.btn-outline-secondary.collapse-diffs{ data: { object: @action[:spkg] } }
               Collapse all
 
         - if sourcediff[:error]
@@ -23,15 +23,7 @@
             %h4
               #{diff_label(sourcediff['new'])} – #{diff_label(sourcediff['old'])}
           - if sourcediff['filenames'].present?
-            - sourcediff['filenames'].each_with_index do |filename, file_index|
-              .mb-2
-                = render partial: 'webui/package/revision_diff_detail', locals: { file_view_path: file_view_path(filename, sourcediff),
-                                                                                  filename: filename,
-                                                                                  file: sourcediff['files'][filename],
-                                                                                  index: file_index,
-                                                                                  last: sourcediff['filenames'].last == filename,
-                                                                                  package: @action[:spkg],
-                                                                                  linkinfo: nil }
+            = render(DiffListComponent.new(diff_list: sourcediff['files'], view_id: @action[:spkg]))
           - else
             .mb-2
               %p.lead

src/api/app/components/sourcediff_component.rb

@@ -11,8 +11,6 @@ def initialize(bs_request:, action:, index:, refresh:)
     @action = action
     @index = index
     @refresh = refresh
-
-    SourcediffsParser.new(sourcediffs: action[:sourcediff]).call
   end
 
   def file_view_path(filename, sourcediff)

src/api/app/controllers/webui/package_controller.rb

@@ -301,8 +301,6 @@ def rdiff
     @not_full_diff = @files.any? { |file| file[1]['diff'].try(:[], 'shown') }
     @filenames = filenames['filenames']
 
-    SourcediffsParser.new(sourcediffs: [filenames]).call if Flipper.enabled?(:request_show_redesign, User.session)
-
     # FIXME: moved from the old view, needs refactoring
     @submit_url_opts = {}
     if @oproject && @opackage && !@oproject.find_attribute('OBS', 'RejectRequests') && !@opackage.find_attribute('OBS', 'RejectRequests')

src/api/app/services/diff_parser.rb

@@ -1,4 +1,6 @@
 class DiffParser
+  attr_reader :lines
+
   def initialize(content:)
     @content = content || ''
     @original_index = 0
@@ -18,7 +20,11 @@ def call
       @last_changed_index = @changed_index
     end
 
-    @lines
+    self
+  end
+
+  def max_digits
+    [@last_original_index, @last_changed_index].max.digits.length
   end
 
   private

src/api/app/views/webui/package/_revision_diff_detail.html.haml

@@ -8,36 +8,7 @@
         .diff-card-header
           = calculate_filename(filename, file)
           %span.badge{ class: "badge-#{file['state']}" }= file['state'].capitalize
-      -# TODO: Remove this `if` condition, and the `else` clause once request_show_redesign is rolled out
-      - if Flipper.enabled?(:request_show_redesign, User.session)
-        .pre.scroll
-          :ruby
-            diff_parsed_lines = file.dig('diff', 'parsed_lines') || []
-            last_line = diff_parsed_lines.reverse.find { |l| l.original_index || l.changed_index }
-            max_index = [last_line.original_index, last_line.changed_index].compact.max if last_line
-            index_width = max_index&.digits&.count || 6
-          .diff{ class: "digits-#{index_width}" }>
-            - diff_parsed_lines&.each do |line|
-              - id = "diff_#{index}_n#{line.index}"
-              %div{ class: "line #{line.state}", id: }>
-                - case line.state
-                - when 'comment', 'range'
-                  .offset
-                    .text-muted.text
-                      = line.content
-                - else
-                  = link_to "##{id}", class: 'd-flex' do
-                    .number<
-                      = line.original_index
-                    .number<
-                      = line.changed_index
-                  .value<
-                    %span.character><
-                      = line.content[0]
-                    %span.content><
-                      = line.content[1..]
-      - else
-        = render_diff(diff_content, file_index: index)
+      = render_diff(diff_content, file_index: index)
   - else
     .card{ id: "revision_details_#{index}" }
       .card-header.py-3