Remove ControllerActionOverride concern7048_set_security_dashboard_as_default_view_for_groups_be-ce

Remove ControllerActionOverride and replace it
with supportive code for controlling group sidebar
nav items active state, extracting groups/details
page-specific JS into a reusable module, restoring
helpers' code responsible for JS bundling

16 files changed, 101 insertions, 129 deletions

diff --git a/app/assets/javascripts/pages/groups/details/index.js b/app/assets/javascripts/pages/groups/details/index.js
index 898afab1bbf..3bcaa0f0232 100644
--- a/app/assets/javascripts/pages/groups/details/index.js
+++ b/app/assets/javascripts/pages/groups/details/index.js
@@ -1,31 +1,5 @@
-/* eslint-disable no-new */
-
-import { getPagePath } from '~/lib/utils/common_utils';
-import { ACTIVE_TAB_SHARED, ACTIVE_TAB_ARCHIVED } from '~/groups/constants';
-import NewGroupChild from '~/groups/new_group_child';
-import notificationsDropdown from '~/notifications_dropdown';
-import NotificationsForm from '~/notifications_form';
-import ProjectsList from '~/projects_list';
-import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation';
-import GroupTabs from './group_tabs';
+import initGroupDetails from '../shared/group_details';
 
 document.addEventListener('DOMContentLoaded', () => {
-  const newGroupChildWrapper = document.querySelector('.js-new-project-subgroup');
-  const loadableActions = [ACTIVE_TAB_SHARED, ACTIVE_TAB_ARCHIVED];
-  const paths = window.location.pathname.split('/');
-  const subpath = paths[paths.length - 1];
-  let action = loadableActions.includes(subpath) ? subpath : getPagePath(1);
-  if (action === 'details') {
-    action = 'show'; // 'show' resets GroupTabs to default action through base class
-  }
-
-  new GroupTabs({ parentEl: '.groups-listing', action });
-  new ShortcutsNavigation();
-  new NotificationsForm();
-  notificationsDropdown();
-  new ProjectsList();
-
-  if (newGroupChildWrapper) {
-    new NewGroupChild(newGroupChildWrapper);
-  }
+  initGroupDetails('details');
 });
diff --git a/app/assets/javascripts/pages/groups/shared/group_details.js b/app/assets/javascripts/pages/groups/shared/group_details.js
new file mode 100644
index 00000000000..01ef3f1db2b
--- /dev/null
+++ b/app/assets/javascripts/pages/groups/shared/group_details.js
@@ -0,0 +1,31 @@
+/* eslint-disable no-new */
+
+import { getPagePath } from '~/lib/utils/common_utils';
+import { ACTIVE_TAB_SHARED, ACTIVE_TAB_ARCHIVED } from '~/groups/constants';
+import NewGroupChild from '~/groups/new_group_child';
+import notificationsDropdown from '~/notifications_dropdown';
+import NotificationsForm from '~/notifications_form';
+import ProjectsList from '~/projects_list';
+import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation';
+import GroupTabs from './group_tabs';
+
+export default function initGroupDetails(actionName = 'show') {
+  const newGroupChildWrapper = document.querySelector('.js-new-project-subgroup');
+  const loadableActions = [ACTIVE_TAB_SHARED, ACTIVE_TAB_ARCHIVED];
+  const paths = window.location.pathname.split('/');
+  const subpath = paths[paths.length - 1];
+  let action = loadableActions.includes(subpath) ? subpath : getPagePath(1);
+  if (actionName && action === actionName) {
+    action = 'show'; // 'show' resets GroupTabs to default action through base class
+  }
+
+  new GroupTabs({ parentEl: '.groups-listing', action });
+  new ShortcutsNavigation();
+  new NotificationsForm();
+  notificationsDropdown();
+  new ProjectsList();
+
+  if (newGroupChildWrapper) {
+    new NewGroupChild(newGroupChildWrapper);
+  }
+}
diff --git a/app/assets/javascripts/pages/groups/details/group_tabs.js b/app/assets/javascripts/pages/groups/shared/group_tabs.js
index c6fe61d2bd9..c6fe61d2bd9 100644
--- a/app/assets/javascripts/pages/groups/details/group_tabs.js
+++ b/app/assets/javascripts/pages/groups/shared/group_tabs.js
diff --git a/app/assets/javascripts/pages/groups/show/index.js b/app/assets/javascripts/pages/groups/show/index.js
new file mode 100644
index 00000000000..af924e74f1f
--- /dev/null
+++ b/app/assets/javascripts/pages/groups/show/index.js
@@ -0,0 +1,5 @@
+import initGroupDetails from '../shared/group_details';
+
+document.addEventListener('DOMContentLoaded', () => {
+  initGroupDetails();
+});
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 03d04c3190a..26cd5dc801f 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -12,7 +12,6 @@ class ApplicationController < ActionController::Base
   include EnforcesTwoFactorAuthentication
   include WithPerformanceBar
   include SessionlessAuthentication
-  include ControllerActionOverride
 
   before_action :authenticate_user!
   before_action :enforce_terms!, if: :should_enforce_terms?
diff --git a/app/controllers/concerns/controller_action_override.rb b/app/controllers/concerns/controller_action_override.rb
deleted file mode 100644
index 373a141e710..00000000000
--- a/app/controllers/concerns/controller_action_override.rb
+++ /dev/null
@@ -1,68 +0,0 @@
-# frozen_string_literal: true
-
-module ControllerActionOverride
-  extend ActiveSupport::Concern
-
-  class_methods do
-    def ctrl_action_override_chain
-      @_ctrl_action_override_chain ||= []
-    end
-
-    def add_controller_action_override(controller_path, action_name, initial_action:, &override_condition)
-      ctrl_action_override_chain << {
-        controller_path: controller_path,
-        controller_name: controller_path.split('/').last,
-        action_name: action_name,
-        initial_action: initial_action,
-        override_condition: override_condition
-      }
-    end
-
-    def set_controller_action_override
-      helper do
-        # rubocop:disable Lint/NestedMethodDefinition
-        def ctrl_action_override_chain
-          controller.class.ctrl_action_override_chain
-        end
-        # rubocop:enable Lint/NestedMethodDefinition
-
-        define_method :controller_action_override_spec do
-          strong_memoize(:controller_action_override_spec) do
-            override = nil
-
-            ctrl_action_override_chain.reverse.each do |o|
-              should_override = controller.action_name == o[:initial_action]
-
-              if o[:override_condition]
-                should_override &&= instance_eval(&o[:override_condition])
-              end
-
-              if should_override
-                override = o
-                break # rubocop:disable Cop/AvoidBreakFromStrongMemoize
-              end
-            end
-
-            override
-          end
-        end
-
-        define_method :controller_action_override do |key|
-          controller_action_override_spec && controller_action_override_spec[key]
-        end
-
-        define_method :controller_path do
-          controller_action_override(:controller_path) || super()
-        end
-
-        define_method :controller_name do
-          controller_action_override(:controller_name) || super()
-        end
-
-        define_method :action_name do
-          controller_action_override(:action_name) || super()
-        end
-      end
-    end
-  end
-end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 913cb5f52ff..6ccccf85043 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -30,9 +30,6 @@ class GroupsController < Groups::ApplicationController
   # project information
   skip_cross_project_access_check :show, if: -> { request.format.html? }
 
-  add_controller_action_override 'groups', 'details', initial_action: 'show'
-  set_controller_action_override
-
   layout :determine_layout
 
   def index
@@ -60,28 +57,11 @@ class GroupsController < Groups::ApplicationController
   end
 
   def show
-    # TODO: improve ControllerActionOverride to support template selection for render
-    respond_to do |format|
-      format.html do
-        render :details
-      end
-
-      format.atom do
-        load_events
-        render layout: 'xml.atom', action: :details
-      end
-    end
+    render_details_view
   end
 
   def details
-    respond_to do |format|
-      format.html
-
-      format.atom do
-        load_events
-        render layout: 'xml.atom'
-      end
-    end
+    render_details_view
   end
 
   def activity
@@ -136,6 +116,19 @@ class GroupsController < Groups::ApplicationController
 
   protected
 
+  def render_details_view
+    respond_to do |format|
+      format.html do
+        render 'groups/details'
+      end
+
+      format.atom do
+        load_events
+        render layout: 'xml.atom', template: 'groups/details'
+      end
+    end
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def authorize_create_group!
     allowed = if params[:parent_id].present?
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 9a2d85fa9dd..9efa84b02f0 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -36,7 +36,7 @@ module ApplicationHelper
   #   current_controller?('gitlab/application') # => false
   def current_controller?(*args)
     args.any? do |v|
-      v.to_s.downcase == controller_name || v.to_s.downcase == controller_path
+      v.to_s.downcase == controller.controller_name || v.to_s.downcase == controller.controller_path
     end
   end
 
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index 9d028dccad7..7ae016d6980 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -23,6 +23,18 @@ module GroupsHelper
     group_sidebar_links.include?(link)
   end
 
+  def group_view_nav_link_active?(_group_view)
+    # must be false; overridden in EE
+    # will be updated if the Group Overview content is ported to CE, see gitlab-ce/#57236
+    false
+  end
+
+  def group_view_nav_link(group_view:, **options, &block)
+    options = options.merge(active: true) if group_view_nav_link_active?(group_view)
+
+    nav_link(options, &block)
+  end
+
   def can_change_group_visibility_level?(group)
     can?(current_user, :change_visibility_level, group)
   end
diff --git a/app/helpers/tab_helper.rb b/app/helpers/tab_helper.rb
index d91f0f78db7..d058d8ba867 100644
--- a/app/helpers/tab_helper.rb
+++ b/app/helpers/tab_helper.rb
@@ -73,7 +73,9 @@ module TabHelper
   end
 
   def active_nav_link?(options)
-    if path = options.delete(:path)
+    if options.key?(:active)
+      options[:active]
+    elsif path = options.delete(:path)
       unless path.respond_to?(:each)
         path = [path]
       end
diff --git a/app/helpers/webpack_helper.rb b/app/helpers/webpack_helper.rb
index 3dbaee8b88b..345ddcf023a 100644
--- a/app/helpers/webpack_helper.rb
+++ b/app/helpers/webpack_helper.rb
@@ -8,13 +8,13 @@ module WebpackHelper
   def webpack_controller_bundle_tags
     chunks = []
 
-    action = case action_name
+    action = case controller.action_name
              when 'create' then 'new'
              when 'update' then 'edit'
-             else action_name
+             else controller.action_name
              end
 
-    route = [*controller_path.split('/'), action].compact
+    route = [*controller.controller_path.split('/'), action].compact
 
     until chunks.any? || route.empty?
       entrypoint = "pages.#{route.join('.')}"
diff --git a/app/views/groups/sidebar/_details.html.haml b/app/views/groups/sidebar/_details.html.haml
new file mode 100644
index 00000000000..ff48b04c737
--- /dev/null
+++ b/app/views/groups/sidebar/_details.html.haml
@@ -0,0 +1,4 @@
+= group_view_nav_link(group_view: :details, path: ['groups#show', 'groups#details', 'groups#subgroups'], html_options: { class: 'home' }) do
+  = link_to details_group_path(@group), title: _('Group details') do
+    %span
+      = _('Details')
diff --git a/app/views/layouts/nav/sidebar/_group.html.haml b/app/views/layouts/nav/sidebar/_group.html.haml
index 757bf11b463..bc59de04a7c 100644
--- a/app/views/layouts/nav/sidebar/_group.html.haml
+++ b/app/views/layouts/nav/sidebar/_group.html.haml
@@ -20,15 +20,13 @@
               = _('Overview')
 
           %ul.sidebar-sub-level-items
-            = nav_link(path: ['groups#show', 'groups#activity', 'groups#subgroups'], html_options: { class: "fly-out-top-item" } ) do
+            = nav_link(path: ['groups#show', 'groups#details', 'groups#activity', 'groups#subgroups'], html_options: { class: "fly-out-top-item" } ) do
               = link_to group_path(@group) do
                 %strong.fly-out-top-item-name
                   = _('Overview')
             %li.divider.fly-out-top-item
-            = nav_link(path: ['groups#details', 'groups#show', 'groups#subgroups'], html_options: { class: 'home' }) do
-              = link_to details_group_path(@group), title: _('Group details') do
-                %span
-                  = _('Details')
+
+            = render 'groups/sidebar/details'
 
             - if group_sidebar_link?(:activity)
               = nav_link(path: 'groups#activity') do
diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb
index 540a8674ec2..75af59946c5 100644
--- a/spec/helpers/groups_helper_spec.rb
+++ b/spec/helpers/groups_helper_spec.rb
@@ -227,4 +227,11 @@ describe GroupsHelper do
       expect(helper.group_sidebar_links).not_to include(*cross_project_features)
     end
   end
+
+  describe '#group_view_nav_link' do
+    it 'makes link active if eligible' do
+      allow(helper).to receive(:group_view_nav_link_active?).with(any_args).and_return(true)
+      expect(helper.group_view_nav_link(group_view: :dummy_group_view)).to match(/<li class="active">/)
+    end
+  end
 end
diff --git a/spec/helpers/tab_helper_spec.rb b/spec/helpers/tab_helper_spec.rb
index 9abf63d4bd4..43ea9a5878c 100644
--- a/spec/helpers/tab_helper_spec.rb
+++ b/spec/helpers/tab_helper_spec.rb
@@ -15,6 +15,16 @@ describe TabHelper do
       end
     end
 
+    context 'with active flag set externally' do
+      it "marks link a active disregarding other options" do
+        expect(nav_link(active: true, path: 'blah#blah')).to match(/<li class="active">/)
+      end
+
+      it "marks link as inactive disregarding other options" do
+        expect(nav_link(active: false, path: 'foo#foo')).not_to match(/<li class="active">/)
+      end
+    end
+
     context 'with controller param' do
       it "performs checks on the current controller" do
         expect(nav_link(controller: :foo)).to match(/<li class="active">/)
@@ -79,6 +89,7 @@ describe TabHelper do
     it "passes extra html options to the list element" do
       expect(nav_link(action: :foo, html_options: { class: 'home' })).to match(/<li class="home active">/)
       expect(nav_link(html_options: { class: 'active' })).to match(/<li class="active">/)
+      expect(nav_link(active: true, html_options: { class: 'link' })).to match(/<li class="link active">/)
     end
   end
 end
diff --git a/spec/routing/group_routing_spec.rb b/spec/routing/group_routing_spec.rb
index 71788028cbf..53271550e8b 100644
--- a/spec/routing/group_routing_spec.rb
+++ b/spec/routing/group_routing_spec.rb
@@ -17,6 +17,10 @@ describe "Groups", "routing" do
     expect(get("/#{group_path}")).to route_to('groups#show', id: group_path)
   end
 
+  it "to #details" do
+    expect(get("/groups/#{group_path}/-/details")).to route_to('groups#details', id: group_path)
+  end
+
   it "to #activity" do
     expect(get("/groups/#{group_path}/-/activity")).to route_to('groups#activity', id: group_path)
   end