diff --git a/app/assets/javascripts/commons/nav/user_merge_requests.js b/app/assets/javascripts/commons/nav/user_merge_requests.js index 1e2cf756059dd97b2b478e08b3b1df7f4347aa8a..eab2b5a31d75c044fac73160fecf0498a2470fc1 100644 --- a/app/assets/javascripts/commons/nav/user_merge_requests.js +++ b/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); }; } } diff --git a/app/assets/javascripts/sidebar/components/reviewers/sidebar_reviewers.vue b/app/assets/javascripts/sidebar/components/reviewers/sidebar_reviewers.vue index aee94a55134e45d0c17ce9ce9017831f39420126..d9228450371fc557ef0e3e93ecedb23e63024dbf 100644 --- a/app/assets/javascripts/sidebar/components/reviewers/sidebar_reviewers.vue +++ b/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; diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index e2335c184b0b11809e0f8eef1586f8f6df07e2c7..c5ffcb48814d7d6f2d4c766f5f9649a7fa322894 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/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; + } + } } } diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index a6a01c7b09021df6e007e98c8931692797c59869..730e10114c3b2ae6d884c820f5813ea36c5f307d 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/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; diff --git a/app/helpers/dashboard_helper.rb b/app/helpers/dashboard_helper.rb index 195b31620391188ab9d7446d5fdce5e2125d245e..08f357916b504c9b7ac4f9ee4a9d2097d1d42f5d 100644 --- a/app/helpers/dashboard_helper.rb +++ b/app/helpers/dashboard_helper.rb @@ -11,6 +11,10 @@ def assigned_mrs_dashboard_path 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 diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 37e701c1c2b7cdc5ce1c3a624f48c24c78acaa4f..bbfd06ca06daa87535d1f2abe860f6f187d0ae86 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -159,6 +159,32 @@ def toggle_draft_merge_request_path(issuable) 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') diff --git a/app/models/user.rb b/app/models/user.rb index c735f20b92cfe0299ffa0d7fc5996bd33ba1c98b..284182797c03cc594e005b442fb9d126dc0e6a07 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1564,6 +1564,12 @@ def assigned_open_merge_requests_count(force: false) 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 @@ def invalidate_issue_cache_counts 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 diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 1d0ab403f12b22fb1f8d58d309428f40f8e0f42d..d2e5a2a1619c2ef5e114eb83b4ab62e995d6af9d 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -112,9 +112,11 @@ def handle_assignees_change(merge_request, old_assignees) 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) diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index 70ab0a5658124385966589c6da01d5f4d24b704a..1394f15cd1515c68650d994959ad3c29ea89a6ec 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/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', diff --git a/lib/api/user_counts.rb b/lib/api/user_counts.rb index 3071f08e1de108ad4b0d9cc4a2391c494b5365c7..31c923a219a48d4bb5e9d762228c45696c123cc1 100644 --- a/lib/api/user_counts.rb +++ b/lib/api/user_counts.rb @@ -12,7 +12,9 @@ class UserCounts < ::API::Base 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 88d9f3f3cbfd0055c917ee45376e7e616f4400dd..9a93cb3e8f0343ca9c953012caaa9fd4ce22cebf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3940,6 +3940,9 @@ msgstr "" msgid "Assigned to me" msgstr "" +msgid "Assigned to you" +msgstr "" + msgid "Assignee" msgid_plural "%d Assignees" msgstr[0] "" @@ -24008,6 +24011,9 @@ msgstr "" msgid "Review requested from %{name}" msgstr "" +msgid "Review requests for you" +msgstr "" + msgid "Review the changes locally" msgstr "" diff --git a/spec/features/dashboard/merge_requests_spec.rb b/spec/features/dashboard/merge_requests_spec.rb index 0e76b5478a1d17e52bf09cbf0fefe8d76ebbe1a2..26b376be660488276a273b0be9ede5fde0e45131 100644 --- a/spec/features/dashboard/merge_requests_spec.rb +++ b/spec/features/dashboard/merge_requests_spec.rb @@ -110,6 +110,12 @@ 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) diff --git a/spec/frontend/commons/nav/user_merge_requests_spec.js b/spec/frontend/commons/nav/user_merge_requests_spec.js index 4da6d53557a315bb73d98b01221a0a045fecc502..5f81067f84f5d9ea4cb61f19321d4ffdc01f59f8 100644 --- a/spec/frontend/commons/nav/user_merge_requests_spec.js +++ b/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(`
0
`); + setFixtures( + `
0
`, + ); }); 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); }); }); diff --git a/spec/helpers/dashboard_helper_spec.rb b/spec/helpers/dashboard_helper_spec.rb index 65182dcb7299a442a03ca567399433992f05b2e5..8a76771be0ab89743b42a0c23c7eb1bf23ddd709 100644 --- a/spec/helpers/dashboard_helper_spec.rb +++ b/spec/helpers/dashboard_helper_spec.rb @@ -89,4 +89,10 @@ 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 diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb index 377e2c43a72bff2740d5895aaefc29361fc34893..821faaab194a7dbdddc0cf49f46b1a5a00c259d8 100644 --- a/spec/helpers/merge_requests_helper_spec.rb +++ b/spec/helpers/merge_requests_helper_spec.rb @@ -67,4 +67,37 @@ 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fb05c9e8052ecbab79f1c10e56fb2149bca04f86..3c5bc1250110d477525a96ed289658f1b75f74fb 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -4084,6 +4084,7 @@ def add_user(access) 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 @@ def add_user(access) 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) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index be802c430ff22725827729abc2d21d1e12856ffc..fdda1515cce9429294291ad68943a41b3adf2d97 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -521,6 +521,19 @@ def update_merge_request(opts) 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