Refactor Merge requests menu

In this commit we're refactoring the Merge Requests 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

@@ -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