Add and refactor Packages menu

In this commit we're refactoring the Packages
menu in the group sidebar.

We're moving away from partial defined logic to object
contained one. Now menus are defined in objects that
will contain all the information instead of in partials.

app/helpers/groups_helper.rb

@@ -22,18 +22,6 @@ module GroupsHelper
     ]
   end
 
-  def group_packages_nav_link_paths
-    %w[
-      groups/packages#index
-      groups/container_registries#index
-    ]
-  end
-
-  def group_container_registry_nav?
-    Gitlab.config.registry.enabled &&
-      can?(current_user, :read_container_image, @group)
-  end
-
   def group_sidebar_links
     @group_sidebar_links ||= get_group_sidebar_links
   end
@@ -179,19 +167,6 @@ module GroupsHelper
     groups.to_json
   end
 
-  def group_packages_nav?
-    group_packages_list_nav? ||
-      group_container_registry_nav?
-  end
-
-  def group_dependency_proxy_nav?
-    @group.dependency_proxy_feature_available?
-  end
-
-  def group_packages_list_nav?
-    @group.packages_feature_enabled?
-  end
-
   def show_invite_banner?(group)
     can?(current_user, :admin_group, group) &&
     !just_created? &&

app/views/groups/sidebar/_packages.html.haml

-- packages_link = group_packages_list_nav? ? group_packages_path(@group) : group_container_registries_path(@group)
-
-- if group_packages_nav?
-  = nav_link(controller: ['groups/packages', 'groups/registry/repositories', 'groups/dependency_proxies']) do
-    = link_to packages_link, title: _('Packages'), class: 'has-sub-items' do
-      .nav-icon-container
-        = sprite_icon('package')
-      %span.nav-item-name
-        = _('Packages & Registries')
-    %ul.sidebar-sub-level-items
-      = nav_link(controller: [:packages, :repositories], html_options: { class: "fly-out-top-item" } ) do
-        = link_to packages_link, title: _('Packages & Registries') do
-          %strong.fly-out-top-item-name
-            = _('Packages & Registries')
-      %li.divider.fly-out-top-item
-      - if group_packages_list_nav?
-        = nav_link(controller: 'groups/packages') do
-          = link_to group_packages_path(@group), title: _('Packages') do
-            %span= _('Package Registry')
-      - if group_container_registry_nav?
-        = nav_link(controller: 'groups/registry/repositories') do
-          = link_to group_container_registries_path(@group), title: _('Container Registry') do
-            %span= _('Container Registry')
-      - if group_dependency_proxy_nav?
-        = nav_link(controller: 'groups/dependency_proxies') do
-          = link_to group_dependency_proxy_path(@group), title: _('Dependency Proxy') do
-            %span= _('Dependency Proxy')

app/views/groups/sidebar/_packages_settings.html.haml

-- if group_packages_list_nav?
+- if @group.packages_feature_enabled?
   = nav_link(controller: :packages_and_registries) do
     = link_to group_settings_packages_and_registries_path(@group), title: _('Packages & Registries'), data: { qa_selector: 'group_package_settings_link' } do
       %span

app/views/layouts/nav/sidebar/_group_menus.html.haml

-= render 'groups/sidebar/packages'
-
 = render 'layouts/nav/sidebar/analytics_links', links: group_analytics_navbar_links(@group, current_user)
 
 - if group_sidebar_link?(:wiki)

ee/app/helpers/ee/groups_helper.rb

@@ -26,24 +26,6 @@ module EE
       "Max size for repositories within this group #{show_lfs}. Can be overridden inside each project. For no limit, enter 0. To inherit the global value, leave blank."
     end
 
-    override :group_packages_nav_link_paths
-    def group_packages_nav_link_paths
-      %w[
-        groups/packages#index
-        groups/dependency_proxies#show
-        groups/container_registries#index
-      ]
-    end
-
-    override :group_packages_nav?
-    def group_packages_nav?
-      super || group_dependency_proxy_nav?
-    end
-
-    def group_dependency_proxy_nav?
-      @group.dependency_proxy_feature_available?
-    end
-
     def group_path_params(group)
       { group_id: group }
     end

ee/app/views/groups/sidebar/_packages.html.haml

-- packages_link = group_packages_list_nav? ? group_packages_path(@group) : group_container_registries_path(@group)
-
-- if group_packages_nav?
-  = nav_link(controller: ['groups/packages', 'groups/registry/repositories', 'groups/dependency_proxies']) do
-    = link_to packages_link, title: _('Packages'), data: { qa_selector: 'group_packages_item' }, class: 'has-sub-items' do
-      .nav-icon-container
-        = sprite_icon('package')
-      %span.nav-item-name
-        = _('Packages & Registries')
-    %ul.sidebar-sub-level-items{ data: { qa_selector: 'group_packages_submenu' } }
-      = nav_link(controller: [:packages, :repositories], html_options: { class: "fly-out-top-item" } ) do
-        = link_to packages_link, title: _('Packages & Registries') do
-          %strong.fly-out-top-item-name
-            = _('Packages & Registries')
-      %li.divider.fly-out-top-item
-      - if group_packages_list_nav?
-        = nav_link(controller: 'groups/packages') do
-          = link_to group_packages_path(@group), title: _('Packages'), data: { qa_selector: 'group_packages_link' } do
-            %span= _('Package Registry')
-      - if group_container_registry_nav?
-        = nav_link(controller: 'groups/registry/repositories') do
-          = link_to group_container_registries_path(@group), title: _('Container Registry') do
-            %span= _('Container Registry')
-      - if group_dependency_proxy_nav?
-        = nav_link(controller: 'groups/dependency_proxies') do
-          = link_to group_dependency_proxy_path(@group), title: _('Dependency Proxy') do
-            %span= _('Dependency Proxy')

lib/sidebars/groups/menus/packages_registries_menu.rb

+# frozen_string_literal: true
+
+module Sidebars
+  module Groups
+    module Menus
+      class PackagesRegistriesMenu < ::Sidebars::Menu
+        override :configure_menu_items
+        def configure_menu_items
+          add_item(packages_registry_menu_item)
+          add_item(container_registry_menu_item)
+          add_item(dependency_proxy_menu_item)
+
+          true
+        end
+
+        override :link
+        def link
+          renderable_items.first.link
+        end
+
+        override :title
+        def title
+          _('Packages & Registries')
+        end
+
+        override :sprite_icon
+        def sprite_icon
+          'package'
+        end
+
+        private
+
+        def packages_registry_menu_item
+          unless context.group.packages_feature_enabled?
+            return ::Sidebars::NilMenuItem.new(item_id: :packages_registry)
+          end
+
+          ::Sidebars::MenuItem.new(
+            title: _('Package Registry'),
+            link: group_packages_path(context.group),
+            active_routes: { controller: 'groups/packages' },
+            item_id: :packages_registry
+          )
+        end
+
+        def container_registry_menu_item
+          if !::Gitlab.config.registry.enabled || !can?(context.current_user, :read_container_image, context.group)
+            return ::Sidebars::NilMenuItem.new(item_id: :container_registry)
+          end
+
+          ::Sidebars::MenuItem.new(
+            title: _('Container Registry'),
+            link: group_container_registries_path(context.group),
+            active_routes: { controller: 'groups/registry/repositories' },
+            item_id: :container_registry
+          )
+        end
+
+        def dependency_proxy_menu_item
+          unless context.group.dependency_proxy_feature_available?
+            return ::Sidebars::NilMenuItem.new(item_id: :dependency_proxy)
+          end
+
+          ::Sidebars::MenuItem.new(
+            title: _('Dependency Proxy'),
+            link: group_dependency_proxy_path(context.group),
+            active_routes: { controller: 'groups/dependency_proxies' },
+            item_id: :dependency_proxy
+          )
+        end
+      end
+    end
+  end
+end

lib/sidebars/groups/panel.rb

@@ -12,6 +12,7 @@ module Sidebars
         add_menu(Sidebars::Groups::Menus::MergeRequestsMenu.new(context))
         add_menu(Sidebars::Groups::Menus::CiCdMenu.new(context))
         add_menu(Sidebars::Groups::Menus::KubernetesMenu.new(context))
+        add_menu(Sidebars::Groups::Menus::PackagesRegistriesMenu.new(context))
       end
 
       override :render_raw_menus_partial

locale/gitlab.pot

@@ -23790,9 +23790,6 @@ msgstr ""
 msgid "PackageRegistry|published by %{author}"
 msgstr ""
 
-msgid "Packages"
-msgstr ""
-
 msgid "Packages & Registries"
 msgstr ""
 

qa/qa/ee/page/group/menu.rb

@@ -36,12 +36,6 @@ module QA
               view 'ee/app/views/layouts/nav/_group_insights_link.html.haml' do
                 element :group_insights_link
               end
-
-              view 'ee/app/views/groups/sidebar/_packages.html.haml' do
-                element :group_packages_item
-                element :group_packages_link
-                element :group_packages_submenu
-              end
             end
           end
 
@@ -123,14 +117,6 @@ module QA
             end
           end
 
-          def go_to_group_packages
-            hover_element(:group_packages_item) do
-              within_submenu(:group_packages_submenu) do
-                click_element(:group_packages_link)
-              end
-            end
-          end
-
           def click_group_wiki_link
             within_sidebar do
               click_element(:wiki_link)

qa/qa/page/group/menu.rb

@@ -75,6 +75,14 @@ module QA
           end
         end
 
+        def go_to_group_packages
+          hover_group_packages do
+            within_submenu do
+              click_element(:sidebar_menu_item_link, menu_item: 'Package Registry')
+            end
+          end
+        end
+
         private
 
         def hover_issues
@@ -101,6 +109,15 @@ module QA
             yield
           end
         end
+
+        def hover_group_packages
+          within_sidebar do
+            scroll_to_element(:sidebar_menu_link, menu_item: 'Packages & Registries')
+            find_element(:sidebar_menu_link, menu_item: 'Packages & Registries').hover
+
+            yield
+          end
+        end
       end
     end
   end

spec/helpers/groups_helper_spec.rb

@@ -263,42 +263,6 @@ RSpec.describe GroupsHelper do
     end
   end
 
-  describe '#group_container_registry_nav' do
-    let_it_be(:group) { create(:group, :public) }
-    let_it_be(:user) { create(:user) }
-
-    before do
-      stub_container_registry_config(enabled: true)
-      allow(helper).to receive(:current_user) { user }
-      allow(helper).to receive(:can?).with(user, :read_container_image, group) { true }
-      helper.instance_variable_set(:@group, group)
-    end
-
-    subject { helper.group_container_registry_nav? }
-
-    context 'when container registry is enabled' do
-      it { is_expected.to be_truthy }
-
-      it 'is disabled for guest' do
-        allow(helper).to receive(:can?).with(user, :read_container_image, group) { false }
-        expect(subject).to be false
-      end
-    end
-
-    context 'when container registry is not enabled' do
-      before do
-        stub_container_registry_config(enabled: false)
-      end
-
-      it { is_expected.to be_falsy }
-
-      it 'is disabled for guests' do
-        allow(helper).to receive(:can?).with(user, :read_container_image, group) { false }
-        expect(subject).to be false
-      end
-    end
-  end
-
   describe '#group_sidebar_links' do
     let_it_be(:group) { create(:group, :public) }
     let_it_be(:user) { create(:user) }

spec/lib/sidebars/groups/menus/packages_registries_menu_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sidebars::Groups::Menus::PackagesRegistriesMenu do
+  let_it_be(:owner) { create(:user) }
+  let_it_be(:group) do
+    build(:group, :private).tap do |g|
+      g.add_owner(owner)
+    end
+  end
+
+  let(:user) { owner }
+  let(:context) { Sidebars::Groups::Context.new(current_user: user, container: group) }
+  let(:menu) { described_class.new(context) }
+
+  describe '#render?' do
+    context 'when menu has menu items to show' do
+      it 'returns true' do
+        expect(menu.render?).to eq true
+      end
+    end
+
+    context 'when menu does not have any menu item to show' do
+      it 'returns false' do
+        stub_container_registry_config(enabled: false)
+        stub_config(packages: { enabled: false })
+        stub_config(dependency_proxy: { enabled: false })
+
+        expect(menu.render?).to eq false
+      end
+    end
+  end
+
+  describe '#link' do
+    let(:registry_enabled) { true }
+    let(:packages_enabled) { true }
+
+    before do
+      stub_container_registry_config(enabled: registry_enabled)
+      stub_config(packages: { enabled: packages_enabled })
+      stub_config(dependency_proxy: { enabled: true })
+    end
+
+    subject { menu.link }
+
+    context 'when Packages Registry is visible' do
+      it 'menu link points to Packages Registry page' do
+        expect(subject).to eq find_menu(menu, :packages_registry).link
+      end
+    end
+
+    context 'when Packages Registry is not visible' do
+      let(:packages_enabled) { false }
+
+      it 'menu link points to Container Registry page' do
+        expect(subject).to eq find_menu(menu, :container_registry).link
+      end
+
+      context 'when Container Registry is not visible' do
+        let(:registry_enabled) { false }
+
+        it 'menu link points to Dependency Proxy page' do
+          expect(subject).to eq find_menu(menu, :dependency_proxy).link
+        end
+      end
+    end
+  end
+
+  describe 'Menu items' do
+    subject { find_menu(menu, item_id) }
+
+    describe 'Packages Registry' do
+      let(:item_id) { :packages_registry }
+
+      context 'when user can read packages' do
+        before do
+          stub_config(packages: { enabled: packages_enabled })
+        end
+
+        context 'when config package setting is disabled' do
+          let(:packages_enabled) { false }
+
+          it 'the menu item is not added to list of menu items' do
+            is_expected.to be_nil
+          end
+        end
+
+        context 'when config package setting is enabled' do
+          let(:packages_enabled) { true }
+
+          it 'the menu item is added to list of menu items' do
+            is_expected.not_to be_nil
+          end
+        end
+      end
+    end
+
+    describe 'Container Registry' do
+      let(:item_id) { :container_registry }
+
+      context 'when user can read container images' do
+        before do
+          stub_container_registry_config(enabled: container_enabled)
+        end
+
+        context 'when config registry setting is disabled' do
+          let(:container_enabled) { false }
+
+          it 'the menu item is not added to list of menu items' do
+            is_expected.to be_nil
+          end
+        end
+
+        context 'when config registry setting is enabled' do
+          let(:container_enabled) { true }
+
+          it 'the menu item is added to list of menu items' do
+            is_expected.not_to be_nil
+          end
+
+          context 'when user cannot read container images' do
+            let(:user) { nil }
+
+            it 'the menu item is not added to list of menu items' do
+              is_expected.to be_nil
+            end
+          end
+        end
+      end
+    end
+
+    describe 'Dependency Proxy' do
+      let(:item_id) { :dependency_proxy }
+
+      before do
+        stub_config(dependency_proxy: { enabled: dependency_enabled })
+      end
+
+      context 'when config dependency_proxy is enabled' do
+        let(:dependency_enabled) { true }
+
+        it 'the menu item is added to list of menu items' do
+          is_expected.not_to be_nil
+        end
+      end
+
+      context 'when config dependency_proxy is not enabled' do
+        let(:dependency_enabled) { false }
+
+        it 'the menu item is not added to list of menu items' do
+          is_expected.to be_nil
+        end
+      end
+    end
+  end
+
+  private
+
+  def find_menu(menu, item)
+    menu.renderable_items.find { |i| i.item_id == item }
+  end
+end

spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb

@@ -108,4 +108,30 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
       expect(rendered).to have_link('Kubernetes', href: group_clusters_path(group))
     end
   end
+
+  describe 'Packages & Registries' do
+    it 'has a link to the package registry page' do
+      stub_config(packages: { enabled: true })
+
+      render
+
+      expect(rendered).to have_link('Package Registry', href: group_packages_path(group))
+    end
+
+    it 'has a link to the container registry page' do
+      stub_container_registry_config(enabled: true)
+
+      render
+
+      expect(rendered).to have_link('Container Registry', href: group_container_registries_path(group))
+    end
+
+    it 'has a link to the dependency proxy page' do
+      stub_config(dependency_proxy: { enabled: true })
+
+      render
+
+      expect(rendered).to have_link('Dependency Proxy', href: group_dependency_proxy_path(group))
+    end
+  end
 end