Merge pull request #14349 from ncounter/refactoring-tab-visibility

Refactoring tab visibility - use decorator instead of helper
openSUSE · Jun 30, 2023 · 073e6f6 · 073e6f6
2 parents 71895ba + 7cf0344
commit 073e6f6
Show file tree

Hide file tree

Showing 10 changed files with 71 additions and 18 deletions.
diff --git a/src/api/app/controllers/webui/request_controller.rb b/src/api/app/controllers/webui/request_controller.rb
@@ -13,6 +13,8 @@ class Webui::RequestController < Webui::WebuiController
                                         if: -> { Flipper.enabled?(:request_show_redesign, User.session) }
   before_action :set_action_id, only: [:inline_comment, :show, :build_results, :rpm_lint, :changes, :mentioned_issues],
                                 if: -> { Flipper.enabled?(:request_show_redesign, User.session) }
+  before_action :set_bs_request_action, only: [:show, :build_results, :rpm_lint, :changes, :mentioned_issues],
+                                        if: -> { Flipper.enabled?(:request_show_redesign, User.session) }
   before_action :set_active_action, only: [:inline_comment, :show, :build_results, :rpm_lint, :changes, :mentioned_issues],
                                     if: -> { Flipper.enabled?(:request_show_redesign, User.session) }
   before_action :set_superseded_request, only: [:show, :request_action, :request_action_changes, :build_results, :rpm_lint, :changes, :mentioned_issues]
@@ -284,7 +286,7 @@ def inline_comment
   end
 
   def build_results
-    redirect_to request_show_path(params[:number], params[:request_action_id]) unless @action[:sprj] || @action[:spkg]
+    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]
@@ -296,7 +298,7 @@ def build_results
   end
 
   def rpm_lint
-    redirect_to request_show_path(params[:number], params[:request_action_id]) unless @action[:sprj] || @action[:spkg]
+    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 = {}
@@ -306,13 +308,13 @@ def rpm_lint
   end
 
   def changes
-    redirect_to request_show_path(params[:number], params[:request_action_id]) unless @action[:type].in?(@actions_for_diff)
+    redirect_to request_show_path(params[:number], params[:request_action_id]) unless @bs_request_action.tab_visibility.changes
 
     @active_tab = 'changes'
   end
 
   def mentioned_issues
-    redirect_to request_show_path(params[:number], params[:request_action_id]) unless @action[:type].in?(@actions_for_diff)
+    redirect_to request_show_path(params[:number], params[:request_action_id]) unless @bs_request_action.tab_visibility.issues
 
     @active_tab = 'mentioned_issues'
   end
@@ -488,6 +490,10 @@ def set_action_id
     @action_id = params[:request_action_id] || @supported_actions.first&.id || @actions.first.id
   end
 
+  def set_bs_request_action
+    @bs_request_action = @bs_request.bs_request_actions.find(@action_id)
+  end
+
   def set_active_action
     @active_action = @actions.find(@action_id)
   end
@@ -560,7 +566,6 @@ def prepare_request_data
 
     # Handling build results
     @staging_project = @bs_request.staging_project.name unless @bs_request.staging_project_id.nil?
-    @actions_for_diff = [:submit, :delete, :maintenance_incident, :maintenance_release]
 
     handle_notification
   end

diff --git a/src/api/app/models/bs_request_action.rb b/src/api/app/models/bs_request_action.rb
@@ -864,6 +864,10 @@ def event_parameters
     params
   end
 
+  def tab_visibility
+    BsRequestActionTabVisibility.new(self)
+  end
+
   private
 
   def cache_diffs

diff --git a/src/api/app/models/bs_request_action_tab_visibility.rb b/src/api/app/models/bs_request_action_tab_visibility.rb
@@ -0,0 +1,33 @@
+class BsRequestActionTabVisibility
+  CHANGES_TABS = [:submit, :maintenance_incident, :maintenance_release].freeze
+
+  def initialize(bs_request_action)
+    @action = bs_request_action
+  end
+
+  def build
+    source_package && !patchinfo_package
+  end
+
+  def rpm_lint
+    build
+  end
+
+  def changes
+    (@action.type == :delete && @action.source_package) || @action.type.in?(CHANGES_TABS)
+  end
+
+  def issues
+    @action.type.in?(CHANGES_TABS)
+  end
+
+  private
+
+  def patchinfo_package
+    @action.type.in?([:maintenance_incident, :maintenance_release]) && @action.source_package == 'patchinfo'
+  end
+
+  def source_package
+    @action.source_project && @action.source_package
+  end
+end
diff --git a/src/api/app/views/webui/request/_request_tabs.html.haml b/src/api/app/views/webui/request/_request_tabs.html.haml
@@ -3,18 +3,19 @@
     %li.nav-item.scrollable-tab-link
       = link_to('Conversation', request_show_path(bs_request.number, actions_count > 1 ? active_action : nil),
                 class: "nav-link text-nowrap #{active_tab == 'conversation' ? 'active' : ''}")
-    - if (action[:sprj] || action[:spkg]) && !(action[:type].in?([:maintenance_incident, :maintenance_release]) && action[:spkg] == 'patchinfo')
+    - if bs_request_action.tab_visibility.build
       %li.nav-item.scrollable-tab-link.active
         = link_to('Build Results', request_build_results_path(bs_request.number, actions_count > 1 ? active_action : nil),
                   class: "nav-link text-nowrap #{active_tab == 'build_results' ? 'active' : ''}")
+    - if bs_request_action.tab_visibility.rpm_lint
       %li.nav-item.scrollable-tab-link
         = link_to('RPM Lint', request_rpm_lint_path(bs_request.number, actions_count > 1 ? active_action : nil),
                   class: "nav-link text-nowrap #{active_tab == 'rpm_lint' ? 'active' : ''}")
-    - if action[:type].in?(actions_for_diff)
+    - if bs_request_action.tab_visibility.changes
       %li.nav-item.scrollable-tab-link
         = link_to('Changes', request_changes_path(bs_request.number, actions_count > 1 ? active_action : nil),
                   class: "nav-link text-nowrap #{active_tab == 'changes' ? 'active' : ''}")
-    - if action[:type].in?(actions_for_diff)
+    - if bs_request_action.tab_visibility.issues
       %li.nav-item.scrollable-tab-link
         = link_to(request_mentioned_issues_path(bs_request.number, actions_count > 1 ? active_action : nil),
                   class: "nav-link text-nowrap #{active_tab == 'mentioned_issues' ? 'active' : ''}") do

diff --git a/src/api/app/views/webui/request/beta_show.html.haml b/src/api/app/views/webui/request/beta_show.html.haml
@@ -12,8 +12,8 @@
                   prev_action: @prev_action, next_action: @next_action, supported_actions: @supported_actions,
                   diff_to_superseded_id: @diff_to_superseded_id, diff_limit: @diff_limit, page_name: '' }
     = render partial: 'request_tabs',
-        locals: { actions_for_diff: @actions_for_diff, bs_request: @bs_request, action: @action, issues: @issues,
-                  active_action: @active_action, actions_count: @supported_actions.count, active_tab: @active_tab }
+        locals: { bs_request: @bs_request, bs_request_action: @bs_request_action, issues: @issues, active_action: @active_action,
+                  actions_count: @supported_actions.count, active_tab: @active_tab }
     .container.p-4
       .row
         .col-md-12

diff --git a/src/api/app/views/webui/request/build_results.html.haml b/src/api/app/views/webui/request/build_results.html.haml
@@ -10,8 +10,8 @@
                   prev_action: @prev_action, next_action: @next_action, supported_actions: @supported_actions,
                   diff_to_superseded_id: @diff_to_superseded_id, diff_limit: @diff_limit, page_name: 'request_build_results' }
     = render partial: 'request_tabs',
-        locals: { actions_for_diff: @actions_for_diff, bs_request: @bs_request, action: @action, issues: @issues,
-                  active_action: @active_action, actions_count: @supported_actions.count, active_tab: @active_tab }
+        locals: { bs_request: @bs_request, bs_request_action: @bs_request_action, issues: @issues, active_action: @active_action,
+                  actions_count: @supported_actions.count, active_tab: @active_tab }
     .container.p-4
       - if @buildable
         .build-results-content{ data: @ajax_data }

diff --git a/src/api/app/views/webui/request/changes.html.haml b/src/api/app/views/webui/request/changes.html.haml
@@ -10,8 +10,8 @@
                   prev_action: @prev_action, next_action: @next_action, supported_actions: @supported_actions,
                   diff_to_superseded_id: @diff_to_superseded_id, diff_limit: @diff_limit, page_name: 'request_changes' }
     = render partial: 'request_tabs',
-        locals: { actions_for_diff: @actions_for_diff, bs_request: @bs_request, action: @action, issues: @issues,
-                  active_action: @active_action, actions_count: @supported_actions.count, active_tab: @active_tab }
+        locals: { bs_request: @bs_request, bs_request_action: @bs_request_action, issues: @issues, active_action: @active_action,
+                  actions_count: @supported_actions.count, active_tab: @active_tab }
     .container.p-4
       .tab-content.sourcediff
         = render SourcediffComponent.new(bs_request: @bs_request, action: @action, index: 0)

diff --git a/src/api/app/views/webui/request/mentioned_issues.html.haml b/src/api/app/views/webui/request/mentioned_issues.html.haml
@@ -10,8 +10,8 @@
                   prev_action: @prev_action, next_action: @next_action, supported_actions: @supported_actions,
                   diff_to_superseded_id: @diff_to_superseded_id, diff_limit: @diff_limit, page_name: 'request_mentioned_issues' }
     = render partial: 'request_tabs',
-        locals: { actions_for_diff: @actions_for_diff, bs_request: @bs_request, action: @action, issues: @issues,
-                  active_action: @active_action, actions_count: @supported_actions.count, active_tab: @active_tab }
+        locals: { bs_request: @bs_request, bs_request_action: @bs_request_action, issues: @issues, active_action: @active_action,
+                  actions_count: @supported_actions.count, active_tab: @active_tab }
     .container.p-4
       - if @issues.empty?
         %p No issues are mentioned for this request action.

diff --git a/src/api/app/views/webui/request/rpm_lint.html.haml b/src/api/app/views/webui/request/rpm_lint.html.haml
@@ -10,8 +10,8 @@
                   prev_action: @prev_action, next_action: @next_action, supported_actions: @supported_actions,
                   diff_to_superseded_id: @diff_to_superseded_id, diff_limit: @diff_limit, page_name: 'request_rpm_lint' }
     = render partial: 'request_tabs',
-        locals: { actions_for_diff: @actions_for_diff, bs_request: @bs_request, action: @action, issues: @issues,
-                  active_action: @active_action, actions_count: @supported_actions.count, active_tab: @active_tab }
+        locals: { bs_request: @bs_request, bs_request_action: @bs_request_action, issues: @issues, active_action: @active_action,
+                  actions_count: @supported_actions.count, active_tab: @active_tab }
     .container.p-4
       - if @action[:spkg]
         .rpm-lint-content{ data: @ajax_data }

diff --git a/src/api/spec/features/webui/requests_spec.rb b/src/api/spec/features/webui/requests_spec.rb
@@ -356,6 +356,16 @@
       visit request_show_path(delete_bs_request)
       expect(page).to have_text('Project Maintainers')
     end
+
+    it 'a delete request does not show the Changes Tab' do
+      visit request_show_path(delete_bs_request)
+      expect(page).not_to have_text('Changes')
+    end
+
+    it 'a delete request does not show the Issues Tab' do
+      visit request_show_path(delete_bs_request)
+      expect(page).not_to have_text('Issues')
+    end
   end
 
   describe 'for a request with a deleted target project' do