Merge branch 'ph/290355/mergeRequestHeaderDropdown' into 'master'

Added extended dropdown to merge requests header icon

See merge request gitlab-org/gitlab!50026

app/assets/javascripts/commons/nav/user_merge_requests.js

@@ -11,7 +11,17 @@ function broadcastCount(newCount) {
 }
 
 function updateUserMergeRequestCounts(newCount) {
-  const mergeRequestsCountEl = document.querySelector('.merge-requests-count');
+  const mergeRequestsCountEl = document.querySelector('.js-assigned-mr-count');
+  mergeRequestsCountEl.textContent = newCount.toLocaleString();
+}
+
+function updateReviewerMergeRequestCounts(newCount) {
+  const mergeRequestsCountEl = document.querySelector('.js-reviewer-mr-count');
+  mergeRequestsCountEl.textContent = newCount.toLocaleString();
+}
+
+function updateMergeRequestCounts(newCount) {
+  const mergeRequestsCountEl = document.querySelector('.js-merge-requests-count');
   mergeRequestsCountEl.textContent = newCount.toLocaleString();
   mergeRequestsCountEl.classList.toggle('hidden', Number(newCount) === 0);
 }
@@ -22,10 +32,14 @@ function updateUserMergeRequestCounts(newCount) {
 export function refreshUserMergeRequestCounts() {
   return Api.userCounts()
     .then(({ data }) => {
-      const count = data.merge_requests;
+      const assignedMergeRequests = data.assigned_merge_requests;
+      const reviewerMergeRequests = data.review_requested_merge_requests;
+      const fullCount = assignedMergeRequests + reviewerMergeRequests;
 
-      updateUserMergeRequestCounts(count);
-      broadcastCount(count);
+      updateUserMergeRequestCounts(assignedMergeRequests);
+      updateReviewerMergeRequestCounts(reviewerMergeRequests);
+      updateMergeRequestCounts(fullCount);
+      broadcastCount(fullCount);
     })
     .catch((ex) => {
       console.error(ex); // eslint-disable-line no-console
@@ -60,7 +74,7 @@ export function openUserCountsBroadcast() {
     if (currentUserId) {
       channel = new BroadcastChannel(`mr_count_channel_${currentUserId}`);
       channel.onmessage = (ev) => {
-        updateUserMergeRequestCounts(ev.data);
+        updateMergeRequestCounts(ev.data);
       };
     }
   }

app/assets/javascripts/sidebar/components/reviewers/sidebar_reviewers.vue

@@ -2,6 +2,7 @@
 // NOTE! For the first iteration, we are simply copying the implementation of Assignees
 // It will soon be overhauled in Issue https://gitlab.com/gitlab-org/gitlab/-/issues/233736
 import { deprecatedCreateFlash as Flash } from '~/flash';
+import { refreshUserMergeRequestCounts } from '~/commons/nav/user_merge_requests';
 import eventHub from '~/sidebar/event_hub';
 import Store from '~/sidebar/stores/sidebar_store';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
@@ -80,8 +81,7 @@ export default {
         .saveReviewers(this.field)
         .then(() => {
           this.loading = false;
-          // Uncomment once this issue has been addressed > https://gitlab.com/gitlab-org/gitlab/-/issues/237922
-          // refreshUserMergeRequestCounts();
+          refreshUserMergeRequestCounts();
         })
         .catch(() => {
           this.loading = false;

app/assets/stylesheets/framework/dropdowns.scss

@@ -796,6 +796,14 @@
   .navbar-gitlab {
     li.dropdown {
       position: static;
+
+      &.user-counter {
+        margin-left: 8px !important;
+
+        > a {
+          padding: 0 4px !important;
+        }
+      }
     }
   }
 

app/assets/stylesheets/framework/header.scss

@@ -172,7 +172,7 @@
       }
 
       li {
-        .badge.badge-pill {
+        .badge.badge-pill:not(.merge-request-badge) {
           box-shadow: none;
           font-weight: $gl-font-weight-bold;
         }
@@ -438,7 +438,7 @@
 
 .title-container,
 .navbar-nav {
-  .badge.badge-pill {
+  .badge.badge-pill:not(.merge-request-badge) {
     position: inherit;
     font-weight: $gl-font-weight-normal;
     margin-left: -6px;

app/helpers/dashboard_helper.rb

@@ -11,6 +11,10 @@ module DashboardHelper
     merge_requests_dashboard_path(assignee_username: current_user.username)
   end
 
+  def reviewer_mrs_dashboard_path
+    merge_requests_dashboard_path(reviewer_username: current_user.username)
+  end
+
   def dashboard_nav_links
     @dashboard_nav_links ||= get_dashboard_nav_links
   end

app/helpers/merge_requests_helper.rb

@@ -159,6 +159,32 @@ module MergeRequestsHelper
 
     issuable_path(issuable, { merge_request: { wip_event: wip_event } })
   end
+
+  def user_merge_requests_counts
+    @user_merge_requests_counts ||= begin
+      assigned_count = assigned_issuables_count(:merge_requests)
+      review_requested_count = review_requested_merge_requests_count
+      total_count = assigned_count + review_requested_count
+
+      {
+        assigned: assigned_count,
+        review_requested: review_requested_count,
+        total: total_count
+      }
+    end
+  end
+
+  def merge_request_reviewers_enabled?
+    Feature.enabled?(:merge_request_reviewers, default_enabled: :yaml)
+  end
+
+  private
+
+  def review_requested_merge_requests_count
+    return 0 unless merge_request_reviewers_enabled?
+
+    current_user.review_requested_open_merge_requests_count
+  end
 end
 
 MergeRequestsHelper.prepend_if_ee('EE::MergeRequestsHelper')

app/models/user.rb

@@ -1564,6 +1564,12 @@ class User < ApplicationRecord
     end
   end
 
+  def review_requested_open_merge_requests_count(force: false)
+    Rails.cache.fetch(['users', id, 'review_requested_open_merge_requests_count'], force: force, expires_in: 20.minutes) do
+      MergeRequestsFinder.new(self, reviewer_id: id, state: 'opened', non_archived: true).execute.count
+    end
+  end
+
   def assigned_open_issues_count(force: false)
     Rails.cache.fetch(['users', id, 'assigned_open_issues_count'], force: force, expires_in: 20.minutes) do
       IssuesFinder.new(self, assignee_id: self.id, state: 'opened', non_archived: true).execute.count
@@ -1607,6 +1613,7 @@ class User < ApplicationRecord
 
   def invalidate_merge_request_cache_counts
     Rails.cache.delete(['users', id, 'assigned_open_merge_requests_count'])
+    Rails.cache.delete(['users', id, 'review_requested_open_merge_requests_count'])
   end
 
   def invalidate_todos_done_count

app/services/merge_requests/update_service.rb

@@ -112,9 +112,11 @@ module MergeRequests
     end
 
     def handle_reviewers_change(merge_request, old_reviewers)
+      affected_reviewers = (old_reviewers + merge_request.reviewers) - (old_reviewers & merge_request.reviewers)
       create_reviewer_note(merge_request, old_reviewers)
       notification_service.async.changed_reviewer_of_merge_request(merge_request, current_user, old_reviewers)
       todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers)
+      invalidate_cache_counts(merge_request, users: affected_reviewers.compact)
     end
 
     def create_branch_change_note(issuable, branch_type, old_branch, new_branch)

app/views/layouts/header/_default.html.haml

@@ -47,17 +47,36 @@
                 %span.badge.badge-pill.issues-count.green-badge{ class: ('hidden' if issues_count == 0) }
                   = number_with_delimiter(issues_count)
           - if header_link?(:merge_requests)
-            = nav_link(path: 'dashboard#merge_requests', html_options: { class: "user-counter" }) do
-              = link_to assigned_mrs_dashboard_path, title: _('Merge requests'), class: 'dashboard-shortcuts-merge_requests', aria: { label: _('Merge requests') },
-                data: { qa_selector: 'merge_requests_shortcut_button', toggle: 'tooltip', placement: 'bottom',
+            - reviewers_enabled = merge_request_reviewers_enabled?
+            = nav_link(path: 'dashboard#merge_requests', html_options: { class: "user-counter #{reviewers_enabled ? 'dropdown' : ''}" }) do
+              = link_to assigned_mrs_dashboard_path, class: 'dashboard-shortcuts-merge_requests', title: _('Merge requests'), aria: { label: _('Merge requests') },
+                data: { qa_selector: 'merge_requests_shortcut_button',
+                toggle: reviewers_enabled ? "dropdown" : "tooltip",
+                placement: 'bottom',
                 track_label: 'main_navigation',
                 track_event: 'click_merge_link',
                 track_property: 'navigation',
                 container: 'body' } do
                 = sprite_icon('git-merge')
-                - merge_requests_count = assigned_issuables_count(:merge_requests)
-                %span.badge.badge-pill.merge-requests-count{ class: ('hidden' if merge_requests_count == 0) }
-                  = number_with_delimiter(merge_requests_count)
+                %span.badge.badge-pill.merge-requests-count.js-merge-requests-count{ class: ('hidden' if user_merge_requests_counts[:total] == 0) }
+                  = number_with_delimiter(user_merge_requests_counts[:total])
+                - if reviewers_enabled
+                  = sprite_icon('chevron-down', css_class: 'caret-down gl-mx-0!')
+              - if reviewers_enabled
+                .dropdown-menu.dropdown-menu-right
+                  %ul
+                    %li.dropdown-header
+                      = _('Merge requests')
+                    %li
+                      = link_to assigned_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center' do
+                        = _('Assigned to you')
+                        %span.badge.gl-badge.badge-pill.badge-muted.merge-request-badge.gl-ml-auto.js-assigned-mr-count{ class: "" }
+                          = user_merge_requests_counts[:assigned]
+                    %li
+                      = link_to reviewer_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center' do
+                        = _('Review requests for you')
+                        %span.badge.gl-badge.badge-pill.badge-muted.merge-request-badge.gl-ml-auto.js-reviewer-mr-count{ class: "" }
+                          = user_merge_requests_counts[:review_requested]
           - if header_link?(:todos)
             = nav_link(controller: 'dashboard/todos', html_options: { class: "user-counter" }) do
               = link_to dashboard_todos_path, title: _('To-Do List'), aria: { label: _('To-Do List') }, class: 'shortcuts-todos',

lib/api/user_counts.rb

@@ -12,7 +12,9 @@ module API
         unauthorized! unless current_user
 
         {
-          merge_requests: current_user.assigned_open_merge_requests_count
+          merge_requests: current_user.assigned_open_merge_requests_count, # @deprecated
+          assigned_merge_requests: current_user.assigned_open_merge_requests_count,
+          review_requested_merge_requests: current_user.review_requested_open_merge_requests_count
         }
       end
     end

locale/gitlab.pot

@@ -3943,6 +3943,9 @@ msgstr ""
 msgid "Assigned to me"
 msgstr ""
 
+msgid "Assigned to you"
+msgstr ""
+
 msgid "Assignee"
 msgid_plural "%d Assignees"
 msgstr[0] ""
@@ -24032,6 +24035,9 @@ msgstr ""
 msgid "Review requested from %{name}"
 msgstr ""
 
+msgid "Review requests for you"
+msgstr ""
+
 msgid "Review the changes locally"
 msgstr ""
 

spec/features/dashboard/merge_requests_spec.rb

@@ -110,6 +110,12 @@ RSpec.describe 'Dashboard Merge Requests' do
       visit merge_requests_dashboard_path(assignee_username: current_user.username)
     end
 
+    it 'includes assigned and reviewers in badge' do
+      expect(find('.merge-requests-count')).to have_content('3')
+      expect(find('.js-assigned-mr-count')).to have_content('2')
+      expect(find('.js-reviewer-mr-count')).to have_content('1')
+    end
+
     it 'shows assigned merge requests' do
       expect(page).to have_content(assigned_merge_request.title)
       expect(page).to have_content(assigned_merge_request_from_fork.title)

spec/frontend/commons/nav/user_merge_requests_spec.js

@@ -8,7 +8,7 @@ import Api from '~/api';
 jest.mock('~/api');
 
 const TEST_COUNT = 1000;
-const MR_COUNT_CLASS = 'merge-requests-count';
+const MR_COUNT_CLASS = 'js-merge-requests-count';
 
 describe('User Merge Requests', () => {
   let channelMock;
@@ -24,7 +24,9 @@ describe('User Merge Requests', () => {
     newBroadcastChannelMock = jest.fn().mockImplementation(() => channelMock);
 
     global.BroadcastChannel = newBroadcastChannelMock;
-    setFixtures(`<div class="${MR_COUNT_CLASS}">0</div>`);
+    setFixtures(
+      `<div><div class="${MR_COUNT_CLASS}">0</div><div class="js-assigned-mr-count"></div><div class="js-reviewer-mr-count"></div></div>`,
+    );
   });
 
   const findMRCountText = () => document.body.querySelector(`.${MR_COUNT_CLASS}`).textContent;
@@ -33,7 +35,10 @@ describe('User Merge Requests', () => {
     beforeEach(() => {
       Api.userCounts.mockReturnValue(
         Promise.resolve({
-          data: { merge_requests: TEST_COUNT },
+          data: {
+            assigned_merge_requests: TEST_COUNT,
+            review_requested_merge_requests: TEST_COUNT,
+          },
         }),
       );
     });
@@ -46,7 +51,7 @@ describe('User Merge Requests', () => {
       });
 
       it('updates the top count of merge requests', () => {
-        expect(findMRCountText()).toEqual(TEST_COUNT.toLocaleString());
+        expect(findMRCountText()).toEqual(Number(TEST_COUNT + TEST_COUNT).toLocaleString());
       });
 
       it('calls the API', () => {
@@ -54,7 +59,7 @@ describe('User Merge Requests', () => {
       });
 
       it('posts count to BroadcastChannel', () => {
-        expect(channelMock.postMessage).toHaveBeenCalledWith(TEST_COUNT);
+        expect(channelMock.postMessage).toHaveBeenCalledWith(TEST_COUNT + TEST_COUNT);
       });
     });
 

spec/helpers/dashboard_helper_spec.rb

@@ -89,4 +89,10 @@ RSpec.describe DashboardHelper do
 
     it { is_expected.to eq(false) }
   end
+
+  describe '#reviewer_mrs_dashboard_path' do
+    subject { helper.reviewer_mrs_dashboard_path }
+
+    it { is_expected.to eq(merge_requests_dashboard_path(reviewer_username: user.username)) }
+  end
 end

spec/helpers/merge_requests_helper_spec.rb

@@ -67,4 +67,37 @@ RSpec.describe MergeRequestsHelper do
       end
     end
   end
+
+  describe '#user_merge_requests_counts' do
+    let(:user) do
+      double(
+        assigned_open_merge_requests_count: 1,
+        review_requested_open_merge_requests_count: 2
+      )
+    end
+
+    subject { helper.user_merge_requests_counts }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    it "returns assigned, review requested and total merge request counts" do
+      expect(subject).to eq(
+        assigned: user.assigned_open_merge_requests_count,
+        review_requested: user.review_requested_open_merge_requests_count,
+        total: user.assigned_open_merge_requests_count + user.review_requested_open_merge_requests_count
+      )
+    end
+
+    context 'when merge_request_reviewers is disabled' do
+      before do
+        stub_feature_flags(merge_request_reviewers: false)
+      end
+
+      it 'returns review_requested as 0' do
+        expect(subject[:review_requested]).to eq(0)
+      end
+    end
+  end
 end

spec/models/user_spec.rb

@@ -4084,6 +4084,7 @@ RSpec.describe User do
       cache_mock = double
 
       expect(cache_mock).to receive(:delete).with(['users', user.id, 'assigned_open_merge_requests_count'])
+      expect(cache_mock).to receive(:delete).with(['users', user.id, 'review_requested_open_merge_requests_count'])
 
       allow(Rails).to receive(:cache).and_return(cache_mock)
 
@@ -4163,6 +4164,20 @@ RSpec.describe User do
     end
   end
 
+  describe '#review_requested_open_merge_requests_count' do
+    it 'returns number of open merge requests from non-archived projects' do
+      user    = create(:user)
+      project = create(:project, :public)
+      archived_project = create(:project, :public, :archived)
+
+      create(:merge_request, source_project: project, author: user, reviewers: [user])
+      create(:merge_request, :closed, source_project: project, author: user, reviewers: [user])
+      create(:merge_request, source_project: archived_project, author: user, reviewers: [user])
+
+      expect(user.review_requested_open_merge_requests_count(force: true)).to eq 1
+    end
+  end
+
   describe '#assigned_open_issues_count' do
     it 'returns number of open issues from non-archived projects' do
       user    = create(:user)

spec/services/merge_requests/update_service_spec.rb

@@ -521,6 +521,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           should_email(user2)
           should_email(user3)
         end
+
+        it 'updates open merge request counter for reviewers', :use_clean_rails_memory_store_caching do
+          merge_request.reviewers = [user3]
+
+          # Cache them to ensure the cache gets invalidated on update
+          expect(user2.review_requested_open_merge_requests_count).to eq(0)
+          expect(user3.review_requested_open_merge_requests_count).to eq(1)
+
+          update_merge_request(reviewer_ids: [user2.id])
+
+          expect(user2.review_requested_open_merge_requests_count).to eq(1)
+          expect(user3.review_requested_open_merge_requests_count).to eq(0)
+        end
       end
 
       context 'when the milestone is removed' do