Commit 4e0c9091 authored by Phil Hughes's avatar Phil Hughes Committed by Mayra Cabrera

Added extended dropdown to merge requests header icon

Adds a dropdown to the merge requests count icon
in the header that will allow users to see both assigned and
reviewer merge requests.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/290355
parent 54ebd72d
......@@ -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);
};
}
}
......
......@@ -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;
......
......@@ -796,6 +796,14 @@
.navbar-gitlab {
li.dropdown {
position: static;
&.user-counter {
margin-left: 8px !important;
> a {
padding: 0 4px !important;
}
}
}
}
......
......@@ -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;
......
......@@ -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
......
......@@ -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')
......@@ -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
......
......@@ -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)
......
......@@ -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',
......
......@@ -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
......
......@@ -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 ""
......
......@@ -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)
......
......@@ -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);
});
});
......
......@@ -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
......@@ -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
......@@ -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)
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment