Merge branch '328506-fj-add-merge-requests-menu' into 'master'

Add Merge Requests menu

See merge request gitlab-org/gitlab!66444

app/helpers/groups_helper.rb

@@ -76,14 +76,6 @@ module GroupsHelper
       .count
   end
 
-  def cached_issuables_count(group, type: nil)
-    count_service = issuables_count_service_class(type)
-    return unless count_service.present?
-
-    issuables_count = count_service.new(group, current_user).count
-    format_issuables_count(count_service, issuables_count)
-  end
-
   def group_dependency_proxy_url(group)
     # The namespace path can include uppercase letters, which
     # Docker doesn't allow. The proxy expects it to be downcased.
@@ -318,26 +310,6 @@ module GroupsHelper
   def group_url_error_message
     s_('GroupSettings|Please choose a group URL with no special characters or spaces.')
   end
-
-  def issuables_count_service_class(type)
-    if type == :issues
-      Groups::OpenIssuesCountService
-    elsif type == :merge_requests
-      Groups::MergeRequestsCountService
-    end
-  end
-
-  def format_issuables_count(count_service, count)
-    if count > count_service::CACHED_COUNT_THRESHOLD
-      ActiveSupport::NumberHelper
-        .number_to_human(
-          count,
-          units: { thousand: 'k', million: 'm' }, precision: 1, significant: false, format: '%n%u'
-        )
-    else
-      number_with_delimiter(count)
-    end
-  end
 end
 
 GroupsHelper.prepend_mod_with('GroupsHelper')

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

-- merge_requests_count = cached_issuables_count(@group, type: :merge_requests)
-
-- if group_sidebar_link?(:merge_requests)
-  = nav_link(path: 'groups#merge_requests') do
-    = link_to merge_requests_group_path(@group) do
-      .nav-icon-container
-        = sprite_icon('git-merge')
-      %span.nav-item-name
-        = _('Merge requests')
-      %span.badge.badge-pill.count= merge_requests_count
-    %ul.sidebar-sub-level-items.is-fly-out-only
-      = nav_link(path: 'groups#merge_requests', html_options: { class: "fly-out-top-item" } ) do
-        = link_to merge_requests_group_path(@group) do
-          %strong.fly-out-top-item-name
-            = _('Merge requests')
-          %span.badge.badge-pill.count.merge_counter.js-merge-counter.fly-out-badge= merge_requests_count
-
 = render_if_exists "layouts/nav/ee/security_link" # EE-specific
 
 = render_if_exists "layouts/nav/ee/push_rules_link" # EE-specific

ee/app/helpers/ee/groups_helper.rb

@@ -4,13 +4,6 @@ module EE
   module GroupsHelper
     extend ::Gitlab::Utils::Override
 
-    override :issuables_count_service_class
-    def issuables_count_service_class(type)
-      return super unless type == :epics
-
-      ::Groups::EpicsCountService
-    end
-
     def group_nav_link_paths
       %w[saml_providers#show usage_quotas#index billings#index]
     end

ee/spec/helpers/ee/groups_helper_spec.rb

@@ -16,34 +16,6 @@ RSpec.describe GroupsHelper do
     group.add_owner(owner)
   end
 
-  describe '#cached_issuables_count' do
-    context 'with epics type' do
-      let(:type) { :epics }
-      let(:count_service) { ::Groups::EpicsCountService }
-
-      it_behaves_like 'cached issuables count'
-
-      context 'with subgroup epics' do
-        before do
-          stub_licensed_features(epics: true)
-          allow(helper).to receive(:current_user) { owner }
-          allow(count_service).to receive(:new).and_call_original
-        end
-
-        it 'counts also epics from subgroups not visible to user' do
-          parent_group = create(:group, :public)
-          subgroup = create(:group, :private, parent: parent_group)
-          create(:epic, :opened, group: parent_group)
-          create(:epic, :opened, group: subgroup)
-
-          expect(Ability.allowed?(owner, :read_epic, parent_group)).to be_truthy
-          expect(Ability.allowed?(owner, :read_epic, subgroup)).to be_falsey
-          expect(helper.cached_issuables_count(parent_group, type: type)).to eq('2')
-        end
-      end
-    end
-  end
-
   describe '#group_sidebar_links' do
     before do
       allow(helper).to receive(:can?) { |*args| Ability.allowed?(*args) }

lib/sidebars/groups/menus/merge_requests_menu.rb

+# frozen_string_literal: true
+
+module Sidebars
+  module Groups
+    module Menus
+      class MergeRequestsMenu < ::Sidebars::Menu
+        include Gitlab::Utils::StrongMemoize
+
+        override :link
+        def link
+          merge_requests_group_path(context.group)
+        end
+
+        override :title
+        def title
+          _('Merge requests')
+        end
+
+        override :sprite_icon
+        def sprite_icon
+          'git-merge'
+        end
+
+        override :render?
+        def render?
+          can?(context.current_user, :read_group_merge_requests, context.group)
+        end
+
+        override :has_pill?
+        def has_pill?
+          true
+        end
+
+        override :pill_count
+        def pill_count
+          strong_memoize(:pill_count) do
+            count_service = ::Groups::MergeRequestsCountService
+            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: 'merge_counter js-merge-counter'
+          }
+        end
+
+        override :active_routes
+        def active_routes
+          { path: 'groups#merge_requests' }
+        end
+      end
+    end
+  end
+end

lib/sidebars/groups/panel.rb

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

spec/helpers/groups_helper_spec.rb

@@ -554,23 +554,4 @@ RSpec.describe GroupsHelper do
       expect(helper.render_setting_to_allow_project_access_token_creation?(group)).to be_falsy
     end
   end
-
-  describe '#cached_issuables_count' do
-    let_it_be(:current_user) { create(:user) }
-    let_it_be(:group) { create(:group, name: 'group') }
-
-    context 'with issues type' do
-      let(:type) { :issues }
-      let(:count_service) { Groups::OpenIssuesCountService }
-
-      it_behaves_like 'cached issuables count'
-    end
-
-    context 'with merge requests type' do
-      let(:type) { :merge_requests }
-      let(:count_service) { Groups::MergeRequestsCountService }
-
-      it_behaves_like 'cached issuables count'
-    end
-  end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sidebars::Groups::Menus::MergeRequestsMenu 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 user can read merge requests' do
+      it 'returns true' do
+        expect(menu.render?).to eq true
+      end
+    end
+
+    context 'when user cannot read merge requests' do
+      let(:user) { nil }
+
+      it 'returns false' do
+        expect(menu.render?).to eq false
+      end
+    end
+  end
+
+  it_behaves_like 'pill_count formatted results' do
+    let(:count_service) { ::Groups::MergeRequestsCountService }
+  end
+end

spec/support/shared_examples/helpers/groups_shared_examples.rb

-# frozen_string_literal: true
-
-# This shared_example requires the following variables:
-# - current_user
-# - group
-# - type, the issuable type (ie :issues, :merge_requests)
-# - count_service, the Service used by the specified issuable type
-
-RSpec.shared_examples 'cached issuables count' do
-  subject { helper.cached_issuables_count(group, type: type) }
-
-  before do
-    allow(helper).to receive(:current_user) { current_user }
-    allow(count_service).to receive(:new).and_call_original
-  end
-
-  it 'calls the correct service class' do
-    subject
-    expect(count_service).to have_received(:new).with(group, current_user)
-  end
-
-  it 'returns all digits for count value under 1000' do
-    allow_next_instance_of(count_service) do |service|
-      allow(service).to receive(:count).and_return(999)
-    end
-
-    expect(subject).to eq('999')
-  end
-
-  it 'returns truncated digits for count value over 1000' do
-    allow_next_instance_of(count_service) do |service|
-      allow(service).to receive(:count).and_return(2300)
-    end
-
-    expect(subject).to eq('2.3k')
-  end
-
-  it 'returns truncated digits for count value over 10000' do
-    allow_next_instance_of(count_service) do |service|
-      allow(service).to receive(:count).and_return(12560)
-    end
-
-    expect(subject).to eq('12.6k')
-  end
-
-  it 'returns truncated digits for count value over 100000' do
-    allow_next_instance_of(count_service) do |service|
-      allow(service).to receive(:count).and_return(112560)
-    end
-
-    expect(subject).to eq('112.6k')
-  end
-end

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

@@ -65,4 +65,18 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
       expect(rendered).to have_link('Milestones', href: group_milestones_path(group))
     end
   end
+
+  describe 'Merge Requests' do
+    it 'has a link to the merge request list path' do
+      render
+
+      expect(rendered).to have_link('Merge requests', href: merge_requests_group_path(group))
+    end
+
+    it 'shows pill with the number of merge requests' do
+      render
+
+      expect(rendered).to have_css('span.badge.badge-pill.merge_counter.js-merge-counter')
+    end
+  end
 end