Refactor Issues menu

In this commit we're refactoring the Issues 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/boards_helper.rb

@@ -119,14 +119,6 @@ module BoardsHelper
     }
   end
 
-  def boards_link_text
-    if current_board_parent.multiple_issue_boards_available?
-      s_("IssueBoards|Boards")
-    else
-      s_("IssueBoards|Board")
-    end
-  end
-
   def recent_boards_path
     recent_project_boards_path(@project) if current_board_parent.is_a?(Project)
   end

app/helpers/nav_helper.rb

@@ -67,15 +67,6 @@ module NavHelper
     %w(dev_ops_report usage_trends)
   end
 
-  def group_issues_sub_menu_items
-    %w[
-      groups#issues
-      milestones#index
-      boards#index
-      boards#show
-    ]
-  end
-
   private
 
   def get_header_links

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

-- issues_count = cached_issuables_count(@group, type: :issues)
 - merge_requests_count = cached_issuables_count(@group, type: :merge_requests)
 
-- if group_sidebar_link?(:issues)
-  = nav_link(path: group_issues_sub_menu_items, unless: -> { current_path?('issues_analytics#show') }) do
-    = link_to issues_group_path(@group), data: { qa_selector: 'group_issues_item' }, class: 'has-sub-items' do
-      .nav-icon-container
-        = sprite_icon('issues')
-      %span.nav-item-name
-        = _('Issues')
-      %span.badge.badge-pill.count= issues_count
-
-    %ul.sidebar-sub-level-items{ data: { qa_selector: 'group_issues_sidebar_submenu'} }
-      = nav_link(path: group_issues_sub_menu_items, html_options: { class: "fly-out-top-item" } ) do
-        = link_to issues_group_path(@group) do
-          %strong.fly-out-top-item-name
-            = _('Issues')
-          %span.badge.badge-pill.count.issue_counter.fly-out-badge= issues_count
-
-      %li.divider.fly-out-top-item
-      = nav_link(path: 'groups#issues', html_options: { class: 'home' }) do
-        = link_to issues_group_path(@group), title: _('List') do
-          %span
-            = _('List')
-
-      - if group_sidebar_link?(:boards)
-        = nav_link(path: ['boards#index', 'boards#show']) do
-          = link_to group_boards_path(@group), title: boards_link_text, data: { qa_selector: 'group_issue_boards_link' } do
-            %span
-              = boards_link_text
-
-      - if group_sidebar_link?(:milestones)
-        = nav_link(path: 'milestones#index') do
-          = link_to group_milestones_path(@group), title: _('Milestones'), data: { qa_selector: 'group_milestones_link' } do
-            %span
-              = _('Milestones')
-
-      = render_if_exists 'layouts/nav/sidebar/group_iterations_link'
-
 - if group_sidebar_link?(:merge_requests)
   = nav_link(path: 'groups#merge_requests') do
     = link_to merge_requests_group_path(@group) do

ee/app/helpers/ee/nav_helper.rb

@@ -19,26 +19,5 @@ module EE
       controllers = %w(audit_logs)
       super.concat(controllers)
     end
-
-    override :group_issues_sub_menu_items
-    def group_issues_sub_menu_items
-      controllers = %w(issues_analytics#show)
-
-      if @group&.feature_available?(:iterations)
-        controllers = iterations_sub_menu_controllers
-      end
-
-      super.concat(controllers)
-    end
-
-    def iterations_sub_menu_controllers
-      paths = ['iterations#index', 'iterations#show']
-
-      if ::Feature.enabled?(:iteration_cadences, @group, default_enabled: :yaml)
-        paths << 'iteration_cadences#index'
-      end
-
-      paths
-    end
   end
 end

ee/app/views/layouts/nav/sidebar/_group_iterations_link.html.haml

-- if group_sidebar_link?(:iteration_cadences) || group_sidebar_link?(:iterations)
-  - iterations_path = Feature.enabled?(:iteration_cadences, @group, default_enabled: :yaml) ? group_iteration_cadences_path(@group) : group_iterations_path(@group)
-  = nav_link(path: iterations_sub_menu_controllers) do
-    = link_to iterations_path, data: { qa_selector: 'group_iterations_link' } do
-      %span
-        = _('Iterations')

ee/lib/ee/sidebars/groups/menus/issues_menu.rb

+# frozen_string_literal: true
+
+module EE
+  module Sidebars
+    module Groups
+      module Menus
+        module IssuesMenu
+          extend ::Gitlab::Utils::Override
+
+          override :configure_menu_items
+          def configure_menu_items
+            return false unless super
+
+            add_item(iterations_menu_item)
+
+            true
+          end
+
+          private
+
+          def iterations_menu_item
+            if !iterations_enabled? || !user_can_access_iterations?
+              return ::Sidebars::NilMenuItem.new(item_id: :iterations)
+            end
+
+            ::Sidebars::MenuItem.new(
+              title: _('Iterations'),
+              link: iterations_link,
+              active_routes: { path: iterations_paths },
+              item_id: :iterations
+            )
+          end
+
+          def iterations_enabled?
+            ::Feature.enabled?(:group_iterations, context.group, default_enabled: true) && context.group.licensed_feature_available?(:iterations)
+          end
+
+          def user_can_access_iterations?
+            (context.group.iteration_cadences_feature_flag_enabled? && can?(context.current_user, :read_iteration_cadence, context.group)) ||
+              can?(context.current_user, :read_iteration, context.group)
+          end
+
+          def iterations_link
+            strong_memoize(:iterations_link) do
+              context.group.iteration_cadences_feature_flag_enabled? ? group_iteration_cadences_path(context.group) : group_iterations_path(context.group)
+            end
+          end
+
+          def iterations_paths
+            strong_memoize(:iterations_paths) do
+              %w[iterations#index iterations#show iterations#new].tap do |paths|
+                paths << 'iteration_cadences#index' if context.group.iteration_cadences_feature_flag_enabled?
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/helpers/ee/nav_helper_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe NavHelper do
-  describe '#iterations_sub_menu_controllers' do
-    context 'when :iteration_cadences is turned on' do
-      it 'includes iteration_cadences#index path in the list' do
-        expect(helper.iterations_sub_menu_controllers).to include('iteration_cadences#index')
-      end
-    end
-
-    context 'when :iteration_cadences is NOT turned on' do
-      before do
-        stub_feature_flags(iteration_cadences: false)
-      end
-
-      it 'includes iteration_cadences#index path in the list' do
-        expect(helper.iterations_sub_menu_controllers).not_to include('iteration_cadences#index')
-      end
-    end
-  end
-end

ee/spec/lib/ee/sidebars/groups/menus/issues_menu_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sidebars::Groups::Menus::IssuesMenu do
+  let_it_be(:owner) { create(:user) }
+
+  let(: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) }
+
+  describe 'Menu Items' do
+    subject { described_class.new(context).renderable_items.find { |e| e.item_id == item_id} }
+
+    describe 'Iterations' do
+      let(:item_id) { :iterations }
+      let(:iterations_enabled) { true }
+
+      before do
+        stub_licensed_features(iterations: iterations_enabled)
+      end
+
+      context 'when licensed feature iterations is not enabled' do
+        let(:iterations_enabled) { false }
+
+        it 'does not include iterations menu item' do
+          is_expected.to be_nil
+        end
+      end
+
+      context 'when licensed feature iterations is enabled' do
+        context 'when user can read iterations' do
+          it 'includes iterations menu item' do
+            is_expected.to be_present
+          end
+        end
+
+        context 'when user cannot read iterations' do
+          let(:user) { nil }
+
+          it 'does not include iterations menu item' do
+            is_expected.to be_nil
+          end
+        end
+      end
+
+      context 'when iteration_cadences are not enabled' do
+        before do
+          stub_feature_flags(iteration_cadences: false)
+        end
+
+        it 'contains the interation link' do
+          expect(subject.link).to include "/groups/#{group.full_path}/-/iterations"
+        end
+
+        it 'includes iterations active routes' do
+          expect(subject.active_routes[:path]).to contain_exactly('iterations#index', 'iterations#show', 'iterations#new')
+        end
+      end
+
+      context 'when iteration_cadences is enabled' do
+        it 'contains the iteration cadences link' do
+          expect(subject.link).to include "/groups/#{group.full_path}/-/cadences"
+        end
+
+        it 'includes iteration and iteration_cadences active routes' do
+          expect(subject.active_routes[:path]).to contain_exactly('iterations#index', 'iterations#show', 'iterations#new', 'iteration_cadences#index')
+        end
+      end
+    end
+  end
+end

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

@@ -121,6 +121,60 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
     end
   end
 
+  describe 'Issues menu' do
+    describe 'iterations link' do
+      let_it_be(:current_user) { create(:user) }
+
+      before do
+        group.add_guest(current_user)
+
+        allow(view).to receive(:current_user).and_return(current_user)
+      end
+
+      context 'with iterations licensed feature available' do
+        before do
+          stub_licensed_features(iterations: true)
+        end
+
+        context 'with group iterations feature flag enabled' do
+          before do
+            stub_feature_flags(group_iterations: true)
+          end
+
+          it 'is visible' do
+            render
+
+            expect(rendered).to have_text 'Iterations'
+          end
+        end
+
+        context 'with iterations feature flag disabled' do
+          before do
+            stub_feature_flags(group_iterations: false)
+          end
+
+          it 'is not visible' do
+            render
+
+            expect(rendered).not_to have_text 'Iterations'
+          end
+        end
+      end
+
+      context 'with iterations licensed feature disabled' do
+        before do
+          stub_licensed_features(iterations: false)
+        end
+
+        it 'is not visible' do
+          render
+
+          expect(rendered).not_to have_text 'Iterations'
+        end
+      end
+    end
+  end
+
   describe 'DevOps adoption link' do
     let!(:current_user) { create(:user) }
 
@@ -409,56 +463,4 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
       end
     end
   end
-
-  describe 'iterations link' do
-    let_it_be(:current_user) { create(:user) }
-
-    before do
-      group.add_guest(current_user)
-
-      allow(view).to receive(:current_user).and_return(current_user)
-    end
-
-    context 'with iterations licensed feature available' do
-      before do
-        stub_licensed_features(iterations: true)
-      end
-
-      context 'with group iterations feature flag enabled' do
-        before do
-          stub_feature_flags(group_iterations: true)
-        end
-
-        it 'is visible' do
-          render
-
-          expect(rendered).to have_text 'Iterations'
-        end
-      end
-
-      context 'with iterations feature flag disabled' do
-        before do
-          stub_feature_flags(group_iterations: false)
-        end
-
-        it 'is not visible' do
-          render
-
-          expect(rendered).not_to have_text 'Iterations'
-        end
-      end
-    end
-
-    context 'with iterations licensed feature disabled' do
-      before do
-        stub_licensed_features(iterations: false)
-      end
-
-      it 'is not visible' do
-        render
-
-        expect(rendered).not_to have_text 'Iterations'
-      end
-    end
-  end
 end

lib/sidebars/groups/menus/issues_menu.rb

+# frozen_string_literal: true
+
+module Sidebars
+  module Groups
+    module Menus
+      class IssuesMenu < ::Sidebars::Menu
+        include Gitlab::Utils::StrongMemoize
+
+        override :configure_menu_items
+        def configure_menu_items
+          return unless can?(context.current_user, :read_group_issues, context.group)
+
+          add_item(list_menu_item)
+          add_item(boards_menu_item)
+          add_item(milestones_menu_item)
+
+          true
+        end
+
+        override :link
+        def link
+          issues_group_path(context.group)
+        end
+
+        override :title
+        def title
+          _('Issues')
+        end
+
+        override :sprite_icon
+        def sprite_icon
+          'issues'
+        end
+
+        override :has_pill?
+        def has_pill?
+          true
+        end
+
+        override :pill_count
+        def pill_count
+          strong_memoize(:pill_count) do
+            count_service = ::Groups::OpenIssuesCountService
+            count = count_service.new(context.group, context.current_user).count
+
+            format_cached_count(count_service, count)
+          end
+        end
+
+        override :pill_html_options
+        def pill_html_options
+          {
+            class: 'issue_counter'
+          }
+        end
+
+        private
+
+        def list_menu_item
+          ::Sidebars::MenuItem.new(
+            title: _('List'),
+            link: issues_group_path(context.group),
+            active_routes: { path: 'groups#issues' },
+            container_html_options: { aria: { label: _('Issues') } },
+            item_id: :issue_list
+          )
+        end
+
+        def boards_menu_item
+          unless can?(context.current_user, :read_group_boards, context.group)
+            return ::Sidebars::NilMenuItem.new(item_id: :boards)
+          end
+
+          title = context.group.multiple_issue_boards_available? ? s_('IssueBoards|Boards') : s_('IssueBoards|Board')
+
+          ::Sidebars::MenuItem.new(
+            title: title,
+            link: group_boards_path(context.group),
+            active_routes: { path: %w[boards#index boards#show] },
+            item_id: :boards
+          )
+        end
+
+        def milestones_menu_item
+          unless can?(context.current_user, :read_group_milestones, context.group)
+            return ::Sidebars::NilMenuItem.new(item_id: :milestones)
+          end
+
+          ::Sidebars::MenuItem.new(
+            title: _('Milestones'),
+            link: group_milestones_path(context.group),
+            active_routes: { path: 'milestones#index' },
+            item_id: :milestones
+          )
+        end
+      end
+    end
+  end
+end
+
+Sidebars::Groups::Menus::IssuesMenu.prepend_mod_with('Sidebars::Groups::Menus::IssuesMenu')

lib/sidebars/groups/panel.rb

@@ -8,6 +8,7 @@ module Sidebars
         set_scope_menu(Sidebars::Groups::Menus::ScopeMenu.new(context))
 
         add_menu(Sidebars::Groups::Menus::GroupInformationMenu.new(context))
+        add_menu(Sidebars::Groups::Menus::IssuesMenu.new(context))
       end
 
       override :render_raw_menus_partial

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

@@ -14,8 +14,6 @@ module QA
               prepend QA::Page::Group::SubMenus::Common
 
               view 'app/views/layouts/nav/sidebar/_group_menus.html.haml' do
-                element :group_issue_boards_link
-                element :group_issues_item
                 element :group_sidebar_submenu
                 element :group_settings
               end
@@ -47,10 +45,6 @@ module QA
                 element :group_insights_link
               end
 
-              view 'ee/app/views/layouts/nav/sidebar/_group_iterations_link.html.haml' do
-                element :group_iterations_link
-              end
-
               view 'ee/app/views/groups/sidebar/_packages.html.haml' do
                 element :group_packages_item
                 element :group_packages_link
@@ -68,17 +62,17 @@ module QA
           end
 
           def go_to_issue_boards
-            hover_element(:group_issues_item) do
-              within_submenu(:group_issues_sidebar_submenu) do
-                click_element(:group_issue_boards_link)
+            hover_issues do
+              within_submenu do
+                click_element(:sidebar_menu_item_link, menu_item: 'Boards')
               end
             end
           end
 
           def go_to_group_iterations
-            hover_element(:group_issues_item) do
-              within_submenu(:group_issues_sidebar_submenu) do
-                click_element(:group_iterations_link)
+            hover_issues do
+              within_submenu do
+                click_element(:sidebar_menu_item_link, menu_item: 'Iterations')
               end
             end
           end

qa/qa/page/group/menu.rb

@@ -8,8 +8,6 @@ module QA
 
         view 'app/views/layouts/nav/sidebar/_group_menus.html.haml' do
           element :general_settings_link
-          element :group_issues_item
-          element :group_milestones_link
           element :group_settings
         end
 
@@ -63,7 +61,7 @@ module QA
         def go_to_milestones
           hover_issues do
             within_submenu do
-              click_element(:group_milestones_link)
+              click_element(:sidebar_menu_item_link, menu_item: 'Milestones')
             end
           end
         end
@@ -81,8 +79,8 @@ module QA
 
         def hover_issues
           within_sidebar do
-            scroll_to_element(:group_issues_item)
-            find_element(:group_issues_item).hover
+            scroll_to_element(:sidebar_menu_link, menu_item: 'Issues')
+            find_element(:sidebar_menu_link, menu_item: 'Issues').hover
 
             yield
           end

spec/helpers/nav_helper_spec.rb

@@ -112,16 +112,6 @@ RSpec.describe NavHelper do
     it { is_expected.to all(be_a(String)) }
   end
 
-  describe '.group_issues_sub_menu_items' do
-    subject { helper.group_issues_sub_menu_items }
-
-    before do
-      allow(helper).to receive(:current_user).and_return(nil)
-    end
-
-    it { is_expected.to all(be_a(String)) }
-  end
-
   describe '#page_has_markdown?' do
     using RSpec::Parameterized::TableSyntax
 

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sidebars::Groups::Menus::IssuesMenu 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 'Menu Items' do
+    subject { menu.renderable_items.index { |e| e.item_id == item_id } }
+
+    shared_examples 'menu access rights' do
+      specify { is_expected.not_to be_nil }
+
+      describe 'when the user does not have access' do
+        let(:user) { nil }
+
+        specify { is_expected.to be_nil }
+      end
+    end
+
+    describe 'List' do
+      let(:item_id) { :issue_list }
+
+      specify { is_expected.not_to be_nil }
+
+      it_behaves_like 'menu access rights'
+    end
+
+    describe 'Boards' do
+      let(:item_id) { :boards }
+
+      it_behaves_like 'menu access rights'
+    end
+
+    describe 'Milestones' do
+      let(:item_id) { :milestones }
+
+      it_behaves_like 'menu access rights'
+    end
+  end
+
+  it_behaves_like 'pill_count formatted results' do
+    let(:count_service) { ::Groups::OpenIssuesCountService }
+  end
+end

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

@@ -39,4 +39,30 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
       expect(rendered).to have_link('Members', href: group_group_members_path(group))
     end
   end
+
+  describe 'Issues' do
+    it 'has a default link to the issue list path' do
+      render
+
+      expect(rendered).to have_link('Issues', href: issues_group_path(group))
+    end
+
+    it 'has a link to the issue list page' do
+      render
+
+      expect(rendered).to have_link('List', href: issues_group_path(group))
+    end
+
+    it 'has a link to the boards page' do
+      render
+
+      expect(rendered).to have_link('Board', href: group_boards_path(group))
+    end
+
+    it 'has a link to the milestones page' do
+      render
+
+      expect(rendered).to have_link('Milestones', href: group_milestones_path(group))
+    end
+  end
 end