Merge pull request #15794 from ncounter/deprecate-action-details

src/api/.rubocop_todo.yml

@@ -230,6 +230,7 @@ Metrics/ClassLength:
     - 'app/models/bs_request_action.rb'
     - 'app/models/bs_request_action_maintenance_incident.rb'
     - 'app/models/bs_request_action_maintenance_release.rb'
+    - 'app/models/bs_request_action_submit.rb'
     - 'app/models/bs_request_permission_check.rb'
     - 'app/models/buildresult.rb'
     - 'app/models/channel.rb'

src/api/app/components/bs_request_action_description_component.rb

@@ -8,61 +8,54 @@ class BsRequestActionDescriptionComponent < ApplicationComponent
   delegate :requester_str, to: :helpers
   delegate :creator_intentions, to: :helpers
 
-  def initialize(action:, creator:)
+  def initialize(action:)
     super
     @action = action
-    @creator = creator
   end
 
   # rubocop:disable Metrics/CyclomaticComplexity
   # rubocop:disable Rails/OutputSafety
   # rubocop:disable Style/FormatString
   def description
-    source_project_hash = { project: action[:sprj], package: action[:spkg], trim_to: nil }
+    creator = action.bs_request.creator
+    source_project_hash = { project: action.source_project, package: action.source_package, trim_to: nil }
+    target_project_hash = { project: action.target_project, package: action.target_package, trim_to: nil }
 
-    description = case action[:type]
-                  when :submit
-                    'Submit %{source_container} to %{target_container}' % {
-                      source_container: project_or_package_link(source_project_hash),
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg])
-                    }
-                  when :delete
-                    target_repository = "repository #{link_to(action[:trepo], repositories_path(project: action[:tprj], repository: action[:trepo]))} for " if action[:trepo]
+    source_container = project_or_package_link(source_project_hash)
+    target_container = project_or_package_link(target_project_hash)
 
-                    'Delete %{target_repository}%{target_container}' % {
-                      target_repository: target_repository,
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg])
-                    }
-                  when :add_role, :set_bugowner
+    description = case action.type
+                  when 'submit'
+                    'Submit %{source_container} to %{target_container}' %
+                    { source_container: source_container, target_container: target_container }
+                  when 'delete'
+                    target_repository = "repository #{link_to(action.target_repository, repositories_path(target_project_hash))} for " if action.target_repository
+
+                    'Delete %{target_repository}%{target_container}' %
+                    { target_repository: target_repository, target_container: target_container }
+                  when 'add_role', 'set_bugowner'
                     '%{creator} wants %{requester} to %{task} for %{target_container}' % {
-                      creator: user_with_realname_and_icon(@creator),
-                      requester: requester_str(@creator, action[:user], action[:group]),
-                      task: creator_intentions(action[:role]),
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg])
-                    }
-                  when :change_devel
-                    'Set %{source_container} to be devel project/package of %{target_container}' % {
-                      source_container: project_or_package_link(source_project_hash),
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg])
-                    }
-                  when :maintenance_incident
-                    source_project_hash.update(homeproject: @creator)
-                    'Submit update from %{source_container} to %{target_container}' % {
-                      source_container: project_or_package_link(source_project_hash),
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg], trim_to: nil)
-                    }
-                  when :maintenance_release
-                    'Maintenance release %{source_container} to %{target_container}' % {
-                      source_container: project_or_package_link(source_project_hash),
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg], trim_to: nil)
-                    }
-                  when :release
-                    'Release %{source_container} to %{target_container}' % {
-                      source_container: project_or_package_link(source_project_hash),
-                      target_container: project_or_package_link(project: action[:tprj], package: action[:tpkg])
+                      creator: user_with_realname_and_icon(creator),
+                      requester: requester_str(creator, action.person_name, action.group_name),
+                      task: creator_intentions(action.role),
+                      target_container: target_container
                     }
+                  when 'change_devel'
+                    'Set %{source_container} to be devel project/package of %{target_container}' %
+                    { source_container: source_container, target_container: target_container }
+                  when 'maintenance_incident'
+                    'Submit update from %{source_container} to %{target_container}' %
+                    { source_container: source_container, target_container: target_container }
+                  when 'maintenance_release'
+                    'Maintenance release %{source_container} to %{target_container}' %
+                    { source_container: source_container, target_container: target_container }
+                  when 'release'
+                    'Release %{source_container} to %{target_container}' %
+                    { source_container: source_container, target_container: target_container }
                   end
 
+    # HACK: this is just a porting of the already existing way of passing the string to the view
+    # TODO: refactor in order to get rid of the `html_safe` tagging
     description.html_safe
   end
   # rubocop:enable Metrics/CyclomaticComplexity

src/api/app/components/bs_request_activity_timeline_component.rb

@@ -11,8 +11,7 @@ def initialize(bs_request:, request_reviews_for_non_staging_projects: [])
     @bs_request = bs_request
     @creator = User.find_by_login(bs_request.creator) || User.nobody
     action_comments = Comment.on_actions_for_request(@bs_request).without_parent.includes(:user)
-    commented_actions = action_comments.map { |c| c.commentable.id }.uniq.compact
-    @diffs = commented_actions.flat_map { |a| @bs_request.webui_actions(action_id: a, diffs: true, cacheonly: 1) }
+    @commented_actions = action_comments.map(&:commentable).uniq.compact
 
     # IDEA: Forge the first item to remove the need of having the hand-crafted "created request" haml fragment
     @timeline = (
@@ -27,7 +26,7 @@ def diff_for_ref(comment)
     return unless (ref = comment.diff_ref&.match(/diff_([0-9]+)/))
 
     file_index = ref.captures.first
-    sourcediff = @diffs.find { |d| d[:id] == comment.commentable.id }[:sourcediff].first
+    sourcediff = @commented_actions.find(comment.commentable.id).first.webui_sourcediff.first
     filename = sourcediff.dig('filenames', file_index.to_i)
     sourcediff.dig('files', filename)
   end

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

@@ -5,16 +5,17 @@
     - if single_action_request && @is_target_maintainer && @bs_request.state.in?([:new, :review])
       - if show_add_submitter_as_maintainer_option?
         .form-check.mb-2
-          = check_box_tag('add_submitter_as_maintainer_0', "#{@action[:tprj]}_#_#{@action[:tpkg]}", false, class: 'form-check-input')
+          = check_box_tag('add_submitter_as_maintainer_0', "#{@action.target_project}_#_#{@action.target_package}", false, class: 'form-check-input')
           %label.form-check-label{ for: 'add_submitter_as_maintainer_0' }
             Add #{link_to(@bs_request.creator, user_path(@bs_request.creator))} as maintainer for
-            #{helpers.project_or_package_link(project: @action[:tprj], package: @action[:tpkg], short: true)}
-      - (@action[:forward] || []).each_with_index do |forward, index|
-        .form-check.mb-2
-          = check_box_tag("forward-#{index}", "#{forward[:project]}_#_#{forward[:package]}", true, class: 'form-check-input')
-          %label.form-check-label{ for: "forward-#{index}" }
-            Forward submit request to
-            #{helpers.project_or_package_link(project: forward[:project], package: forward[:package], short: true)}
+            #{helpers.project_or_package_link(project: @action.target_project, package: @action.target_package, short: true)}
+      - if @action.type == 'submit'
+        - (@action.forward || []).each_with_index do |forward, index|
+          .form-check.mb-2
+            = check_box_tag("forward-#{index}", "#{forward[:project]}_#_#{forward[:package]}", true, class: 'form-check-input')
+            %label.form-check-label{ for: "forward-#{index}" }
+              Forward submit request to
+              #{helpers.project_or_package_link(project: forward[:project], package: forward[:package], short: true)}
     - if policy(@bs_request).revoke_request?
       = submit_tag 'Revoke request', name: 'revoked', class: 'btn btn-danger me-2'
     - if can_reopen_request?

src/api/app/components/request_decision_component.rb

@@ -24,7 +24,7 @@ def confirmation
   end
 
   def show_add_submitter_as_maintainer_option?
-    !@action[:creator_is_target_maintainer] && @action[:type] == :submit
+    @action.type == 'submit' && !@action.creator_is_target_maintainer
   end
 
   # TODO: Move all those "can_*" checks to a pundit policy

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

@@ -1,31 +1,31 @@
 .row
   .col
-    - if @action[:diff_not_cached]
+    - if @action.diff_not_cached
       .clearfix.mb-2.text-center
-        .btn.btn-outline-primary.cache-refresh{ title: 'Refresh results', onclick: "loadChanges(#{@bs_request.number}, #{@action[:id]});" }
+        .btn.btn-outline-primary.cache-refresh{ title: 'Refresh results', onclick: "loadChanges(#{@bs_request.number}, #{@action.id});" }
           Crunching the latest data. Refresh again in a few seconds
           %i.fas.fa-sync-alt{ id: "cache#0-reload" }
     - else
-      - (@action[:sourcediff] || []).each do |sourcediff|
+      - (@action.webui_sourcediff).each do |sourcediff|
         .clearfix.mb-2
           .btn-group.float-end
-            %button.btn.btn-outline-secondary.expand-diffs{ data: { object: @action[:spkg] } }
+            %button.btn.btn-outline-secondary.expand-diffs{ data: { object: @action.source_package } }
               Expand all
-            %button.btn.btn-outline-secondary.collapse-diffs{ data: { object: @action[:spkg] } }
+            %button.btn.btn-outline-secondary.collapse-diffs{ data: { object: @action.source_package } }
               Collapse all
 
         - if sourcediff[:error]
           %p
             %i.error
               = sourcediff[:error]
         - else
-          - if @action[:sourcediff].length > 1
+          - if @action.webui_sourcediff.length > 1
             %h4
               #{diff_label(sourcediff['new'])} – #{diff_label(sourcediff['old'])}
           - if sourcediff['filenames'].present?
             - diff_list = sourcediff['files'].sort_by { |k, _v| sourcediff['filenames'].find_index(k) }.to_h
-            = render(DiffListComponent.new(diff_list:, view_id: @action[:spkg], commentable:, source_package:,
-                                           target_package:, source_rev: @action[:srev]))
+            = render(DiffListComponent.new(diff_list:, view_id: @action.source_package, commentable:, source_package:,
+                                           target_package:, source_rev: @action.source_rev))
           - else
             .mb-2
               %p.lead

src/api/app/components/sourcediff_component.rb

@@ -12,19 +12,19 @@ def initialize(bs_request:, action:)
   end
 
   def commentable
-    BsRequestAction.find(@action[:id])
+    BsRequestAction.find(@action.id)
   end
 
   def source_package
-    Package.get_by_project_and_name(@action[:sprj], @action[:spkg], { follow_multibuild: true })
+    Package.get_by_project_and_name(@action.source_project, @action.source_package, { follow_multibuild: true })
   rescue Package::UnknownObjectError, Project::Errors::UnknownObjectError
   end
 
   def target_package
     # For not accepted maintenance incident requests, the package is not there.
-    return nil unless @action[:tpkg]
+    return nil unless @action.target_package
 
-    Package.get_by_project_and_name(@action[:tprj], @action[:tpkg], { follow_multibuild: true })
+    Package.get_by_project_and_name(@action.target_project, @action.target_package, { follow_multibuild: true })
   rescue Package::UnknownObjectError, Project::Errors::UnknownObjectError
   end
 end

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

@@ -60,6 +60,8 @@ def show
       @actions = @bs_request.webui_actions(filelimit: @diff_limit, tarlimit: @diff_limit, diff_to_superseded: @diff_to_superseded, diffs: false)
       @action = @actions.first
       @active = @action[:name]
+      # TODO: this is the last instance of the @not_full_diff variable in the request scope, once request_workflow_redesign beta is rolled out,
+      # let's get rid of this variable and also of `BsRequest.truncated_diffs` as it is no longer used anywhere else
       # print a hint that the diff is not fully shown (this only needs to be verified for submit actions)
       @not_full_diff = BsRequest.truncated_diffs?(@actions)
 
@@ -176,12 +178,7 @@ def request_action
   end
 
   def request_action_changes
-    # TODO: Change @diff_limit to a local variable
-    @diff_limit = params[:full_diff] ? 0 : nil
-    # TODO: Change @actions to a local variable
-    @actions = @bs_request.webui_actions(filelimit: @diff_limit, tarlimit: @diff_limit, diff_to_superseded: @diff_to_superseded, diffs: true,
-                                         action_id: params['id'].to_i, cacheonly: 1)
-    @action = @actions.find { |action| action[:id] == params['id'].to_i }
+    @action = @bs_request.bs_request_actions.where(id: params['id'].to_i).first
 
     cache_diff_data
 
@@ -287,17 +284,17 @@ def build_results
     redirect_to request_show_path(params[:number], params[:request_action_id]) unless @bs_request_action.tab_visibility.build
 
     @active_tab = 'build_results'
-    @project = @staging_project || @action[:sprj]
-    @buildable = @action[:spkg] || @project
+    @project = @staging_project || @action.source_project
+    @buildable = @action.source_package || @project
   end
 
   def rpm_lint
     redirect_to request_show_path(params[:number], params[:request_action_id]) unless @bs_request_action.tab_visibility.rpm_lint
 
     @active_tab = 'rpm_lint'
     @ajax_data = {}
-    @ajax_data['project'] = @action[:sprj] if @action[:sprj]
-    @ajax_data['package'] = @action[:spkg] if @action[:spkg]
+    @ajax_data['project'] = @action.source_project if @action.source_project
+    @ajax_data['package'] = @action.source_package if @action.source_package
     @ajax_data['is_staged_request'] = true if @staging_project.present?
   end
 
@@ -513,11 +510,10 @@ def staging_status(request, target_project)
   end
 
   def cache_diff_data
-    return unless @action[:diff_not_cached]
+    return unless @action.diff_not_cached
 
-    bs_request_action = BsRequestAction.find(@action[:id])
-    job = Delayed::Job.where('handler LIKE ?', "%job_class: BsRequestActionWebuiInfosJob%#{bs_request_action.to_global_id.uri}%").count
-    BsRequestActionWebuiInfosJob.perform_later(bs_request_action) if job.zero?
+    job = Delayed::Job.where('handler LIKE ?', "%job_class: BsRequestActionWebuiInfosJob%#{@action.to_global_id.uri}%").count
+    BsRequestActionWebuiInfosJob.perform_later(@action) if job.zero?
   end
 
   def handle_notification
@@ -534,8 +530,8 @@ def prepare_request_data
     @diff_to_superseded_id = params[:diff_to_superseded]
 
     # Handling request actions
-    @action = @bs_request.webui_actions(diff_to_superseded: @diff_to_superseded, diffs: true, action_id: @action_id.to_i, cacheonly: 1).first
-    active_action_index = @supported_actions.index(@active_action)
+    @action = @active_action || @bs_request.bs_request_actions.first
+    active_action_index = @supported_actions.index(@action)
     if active_action_index
       @prev_action = @supported_actions[active_action_index - 1] unless active_action_index.zero?
       @next_action = @supported_actions[active_action_index + 1] if active_action_index + 1 < @supported_actions.length
@@ -546,7 +542,7 @@ def prepare_request_data
     @staging_status = staging_status(@bs_request, target_project) if Staging::Workflow.find_by(project: target_project)
 
     # Collecting all issues in a hash. Each key is the issue name and the value is a hash containing all the issue details.
-    @issues = (@action.fetch(:sourcediff, []) || []).reduce({}) { |accumulator, sourcediff| accumulator.merge(sourcediff.fetch('issues', {})) }
+    @issues = @action.webui_sourcediff({ diff_to_superseded: @diff_to_superseded_id, cacheonly: 1 }).reduce({}) { |accumulator, sourcediff| accumulator.merge(sourcediff.fetch('issues', {})) }
 
     # retrieve a list of all package maintainers that are assigned to at least one target package
     @package_maintainers = target_package_maintainers

src/api/app/models/bs_request.rb

@@ -230,6 +230,8 @@ def self.new_from_hash(hashed)
     request
   end
 
+  # [DEPRECATED] TODO: there is only one instance of the @not_full_diff variable in the request scope which is using this method.
+  # Once request_workflow_redesign beta is rolled out, let's drop this method
   # TODO: refactor this method as soon as the request_show_redesign feature is rolled out.
   # Now it expects an array of action hashes we'll never display more than one action at a time.
   def self.truncated_diffs?(actions)
@@ -917,6 +919,7 @@ def notify
     end
   end
 
+  # [DEPRECATED] TODO: drop this after request_workflow_redesign beta is rolled_out
   def webui_actions(opts = {})
     actions = []
     action_id = opts.delete(:action_id)
@@ -985,6 +988,7 @@ def conclusive?
     FINAL_REQUEST_STATES.include?(state)
   end
 
+  # [DEPRECATED] TODO: drop this after request_workflow_redesign beta is rolled_out
   def action_details(opts = {}, xml:)
     with_diff = opts.delete(:diffs)
     action = { type: xml.action_type }

src/api/app/models/bs_request_action.rb

@@ -269,6 +269,15 @@ def webui_sourcediff(opts = {})
     sorted_filenames_from_sourcediff(sd)
   end
 
+  def diff_not_cached
+    sourcediff_results = webui_sourcediff({ cacheonly: 1 })
+
+    return false if sourcediff_results.present?
+
+    errors = sourcediff_results.pluck(:error).compact
+    errors.any? { |e| e.include?('diff not yet in cache') }
+  end
+
   def find_action_with_same_target(other_bs_request)
     return nil if other_bs_request.blank?
 
@@ -810,6 +819,10 @@ def name
     uniq_key
   end
 
+  def short_name
+    ''
+  end
+
   def commit_details
     package = Package.find_by_project_and_name(source_project, source_package)
 

src/api/app/models/bs_request_action_add_role.rb

@@ -55,6 +55,10 @@ def name
     uniq_key.gsub('add_role/', 'Add Role ')
   end
 
+  def short_name
+    'Add Role'
+  end
+
   #### Alias of methods
 end
 

src/api/app/models/bs_request_action_change_devel.rb

@@ -34,6 +34,10 @@ def name
     uniq_key.gsub('changedevel/', 'Change Devel ')
   end
 
+  def short_name
+    'Change Devel'
+  end
+
   #### Alias of methods
 end
 

src/api/app/models/bs_request_action_delete.rb

@@ -79,6 +79,10 @@ def name
     end
   end
 
+  def short_name
+    name
+  end
+
   private
 
   def remove_repository(opts)

src/api/app/models/bs_request_action_maintenance_incident.rb

@@ -111,6 +111,10 @@ def name
     "Incident #{uniq_key}"
   end
 
+  def short_name
+    "Incident #{source_package}"
+  end
+
   private
 
   def _merge_pkg_into_maintenance_incident(incident_project)