From 1acc47aad2206fa7c0e218ff036acf5231c8e14d Mon Sep 17 00:00:00 2001
From: Alper Akgun <aakgun@gitlab.com>
Date: Fri, 23 Sep 2022 17:59:07 +0000
Subject: [PATCH] Merge request suggested reviewer dropdown
For ultimate projects on gitlab.com
---
app/assets/javascripts/users_select/index.js | 30 +++++++
app/controllers/autocomplete_controller.rb | 20 ++++-
app/helpers/form_helper.rb | 6 ++
app/models/merge_request.rb | 7 ++
app/models/project.rb | 5 ++
app/serializers/merge_request_user_entity.rb | 4 +
app/serializers/user_serializer.rb | 2 +-
.../issuable/_sidebar_reviewers.html.haml | 3 +
.../controllers/ee/autocomplete_controller.rb | 21 +++++
ee/app/helpers/ee/form_helper.rb | 9 ++
ee/app/models/ee/project.rb | 10 +++
.../autocomplete_controller_spec.rb | 82 +++++++++++++++++++
.../user_edits_multiple_reviewers_mr_spec.rb | 33 ++++++++
locale/gitlab.pot | 6 ++
spec/models/merge_request_spec.rb | 72 ++++++++++++++++
15 files changed, 308 insertions(+), 2 deletions(-)
diff --git a/app/assets/javascripts/users_select/index.js b/app/assets/javascripts/users_select/index.js
index 94b4ee77e7e1..2fface2729f7 100644
--- a/app/assets/javascripts/users_select/index.js
+++ b/app/assets/javascripts/users_select/index.js
@@ -50,6 +50,7 @@ function UsersSelect(currentUser, els, options = {}) {
options.iid = $dropdown.data('iid');
options.issuableType = $dropdown.data('issuableType');
options.targetBranch = $dropdown.data('targetBranch');
+ options.showSuggested = $dropdown.data('showSuggested');
const showNullUser = $dropdown.data('nullUser');
const defaultNullUser = $dropdown.data('nullUserDefault');
const showMenuAbove = $dropdown.data('showMenuAbove');
@@ -340,6 +341,16 @@ function UsersSelect(currentUser, els, options = {}) {
if ($dropdown.hasClass('js-multiselect')) {
const selected = getSelected().filter((i) => i !== 0);
+ if ($dropdown.data('showSuggested')) {
+ const suggested = this.suggestedUsers(users);
+ if (suggested.length) {
+ users = users.filter(
+ (u) => !u.suggested || (u.suggested && selected.indexOf(u.id) !== -1),
+ );
+ users.splice(showDivider + 1, 0, ...suggested);
+ }
+ }
+
if (selected.length > 0) {
if ($dropdown.data('dropdownHeader')) {
showDivider += 1;
@@ -370,6 +381,21 @@ function UsersSelect(currentUser, els, options = {}) {
$dropdown.data('deprecatedJQueryDropdown').positionMenuAbove();
}
},
+ suggestedUsers(users) {
+ const selected = getSelected().filter((i) => i !== 0);
+ const suggestedUsers = users
+ .filter((u) => u.suggested && selected.indexOf(u.id) === -1)
+ .sort((a, b) => a.name > b.name);
+
+ if (!suggestedUsers.length) return [];
+
+ const items = [
+ { type: 'header', content: $dropdown.data('suggestedReviewersHeader') },
+ ...suggestedUsers,
+ { type: 'header', content: $dropdown.data('allMembersHeader') },
+ ];
+ return items;
+ },
filterable: true,
filterRemote: true,
search: {
@@ -760,6 +786,10 @@ UsersSelect.prototype.users = function (query, options, callback) {
params.approval_rules = true;
}
+ if (isMergeRequest && options.showSuggested) {
+ params.show_suggested = true;
+ }
+
if (isNewMergeRequest) {
params.target_branch = options.targetBranch || null;
}
diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb
index 88592efcec75..45585ab84b49 100644
--- a/app/controllers/autocomplete_controller.rb
+++ b/app/controllers/autocomplete_controller.rb
@@ -25,7 +25,20 @@ def users
.new(params: params, current_user: current_user, project: project, group: group)
.execute
- render json: UserSerializer.new(params.merge({ current_user: current_user })).represent(users, project: project)
+ presented_users = UserSerializer
+ .new(params.merge({ current_user: current_user }))
+ .represent(users, project: project)
+
+ extra_users = presented_suggested_users
+
+ if extra_users.present?
+ presented_users.reject! do |user|
+ extra_users.any? { |suggested_user| suggested_user[:id] == user[:id] }
+ end
+ presented_users += extra_users
+ end
+
+ render json: presented_users
end
def user
@@ -80,6 +93,11 @@ def project
def target_branch_params
params.permit(:group_id, :project_id).select { |_, v| v.present? }
end
+
+ # overridden in EE
+ def presented_suggested_users
+ []
+ end
end
AutocompleteController.prepend_mod_with('AutocompleteController')
diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb
index f2e24f54391c..e8539f5997e4 100644
--- a/app/helpers/form_helper.rb
+++ b/app/helpers/form_helper.rb
@@ -117,9 +117,15 @@ def reviewers_dropdown_options(issuable_type, iid = nil, target_branch = nil)
dropdown_data = multiple_reviewers_dropdown_options(dropdown_data)
end
+ dropdown_data[:data].merge!(reviewers_dropdown_options_for_suggested_reviewers)
dropdown_data
end
+ # Overwritten
+ def reviewers_dropdown_options_for_suggested_reviewers
+ {}
+ end
+
# Overwritten
def issue_supports_multiple_assignees?
false
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index a57cb97e9360..161076f99b97 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -73,6 +73,13 @@ class MergeRequest < ApplicationRecord
belongs_to :latest_merge_request_diff, class_name: 'MergeRequestDiff'
manual_inverse_association :latest_merge_request_diff, :merge_request
+ def suggested_reviewer_users
+ return [] unless predictions && predictions.suggested_reviewers.is_a?(Hash)
+
+ usernames = Array.wrap(suggested_reviewers["reviewers"])
+ project.authorized_users.active.by_username(usernames)
+ end
+
# This is the same as latest_merge_request_diff unless:
# 1. There are arguments - in which case we might be trying to force-reload.
# 2. This association is already loaded.
diff --git a/app/models/project.rb b/app/models/project.rb
index 5530ebe0f7aa..3387a55f20d9 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -3027,6 +3027,11 @@ def can_create_custom_domains?
pages_domains.count < Gitlab::CurrentSettings.max_pages_custom_domains_per_project
end
+ # overridden in EE
+ def suggested_reviewers_available?
+ false
+ end
+
private
# overridden in EE
diff --git a/app/serializers/merge_request_user_entity.rb b/app/serializers/merge_request_user_entity.rb
index 36825d140621..caf2e4c89b64 100644
--- a/app/serializers/merge_request_user_entity.rb
+++ b/app/serializers/merge_request_user_entity.rb
@@ -25,6 +25,10 @@ def self.satisfies(*methods)
# makes one query per merge request, whereas #approved_by? makes one per user
options[:merge_request].approvals.any? { |app| app.user_id == user.id }
end
+
+ expose :suggested, if: satisfies(:present?) do |user, options|
+ options[:suggested]
+ end
end
MergeRequestUserEntity.prepend_mod_with('MergeRequestUserEntity')
diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb
index 5e7dab31e8a1..5082b84978a8 100644
--- a/app/serializers/user_serializer.rb
+++ b/app/serializers/user_serializer.rb
@@ -8,7 +8,7 @@ def represent(resource, opts = {}, entity = nil)
merge_request = opts[:project].merge_requests.find_by_iid!(params[:merge_request_iid])
preload_max_member_access(merge_request.project, Array(resource))
- super(resource, opts.merge(merge_request: merge_request), MergeRequestUserEntity)
+ super(resource, opts.merge(merge_request: merge_request, suggested: params[:suggested]), MergeRequestUserEntity)
else
super
end
diff --git a/app/views/shared/issuable/_sidebar_reviewers.html.haml b/app/views/shared/issuable/_sidebar_reviewers.html.haml
index 3f78f29ea248..89efd5c11d18 100644
--- a/app/views/shared/issuable/_sidebar_reviewers.html.haml
+++ b/app/views/shared/issuable/_sidebar_reviewers.html.haml
@@ -36,6 +36,9 @@
- data[:multi_select] = true
- data['dropdown-title'] = title
- data['dropdown-header'] = dropdown_options[:data][:'dropdown-header']
+ - data[:suggested_reviewers_header] = dropdown_options[:data][:suggested_reviewers_header]
+ - data[:all_members_header] = dropdown_options[:data][:all_members_header]
+ - data[:show_suggested] = dropdown_options[:data][:show_suggested]
- data['max-select'] = dropdown_max_select(dropdown_options[:data])
- options[:data].merge!(data)
diff --git a/ee/app/controllers/ee/autocomplete_controller.rb b/ee/app/controllers/ee/autocomplete_controller.rb
index a4782a8f6e33..3b39dcfcfa1c 100644
--- a/ee/app/controllers/ee/autocomplete_controller.rb
+++ b/ee/app/controllers/ee/autocomplete_controller.rb
@@ -41,5 +41,26 @@ def namespace_routes
render json: RouteSerializer.new.represent(routes)
end
+
+ private
+
+ def suggested_reviewers_available?
+ project.suggested_reviewers_available?
+ end
+
+ def presented_suggested_users
+ return [] unless params[:search].blank? && params[:merge_request_iid].present?
+ return [] unless suggested_reviewers_available?
+
+ merge_request = project.merge_requests.find_by_iid!(params[:merge_request_iid])
+ return [] unless merge_request&.open?
+
+ suggested_users = merge_request.suggested_reviewer_users
+ return [] if suggested_users.empty?
+
+ ::UserSerializer
+ .new(params.merge({ current_user: current_user, suggested: true }))
+ .represent(suggested_users, project: project)
+ end
end
end
diff --git a/ee/app/helpers/ee/form_helper.rb b/ee/app/helpers/ee/form_helper.rb
index 315947e743d1..db9714e3d136 100644
--- a/ee/app/helpers/ee/form_helper.rb
+++ b/ee/app/helpers/ee/form_helper.rb
@@ -2,6 +2,15 @@
module EE
module FormHelper
+ # Overwritten
+ def reviewers_dropdown_options_for_suggested_reviewers
+ {
+ show_suggested: true,
+ suggested_reviewers_header: _('Suggestion(s)'),
+ all_members_header: _('All project members')
+ }
+ end
+
def issue_supports_multiple_assignees?
current_board_parent.feature_available?(:multiple_issue_assignees)
end
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index 83e47dd62437..5908e8d0ef3a 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -911,6 +911,16 @@ def epic_ids_referenced_by_issues
epic_ids.to_a
end
+ def suggested_reviewers_available?
+ strong_memoize(:suggested_reviewers_available) do
+ next false unless ::Gitlab.com? &&
+ ::Feature.enabled?(:suggested_reviewers, self) &&
+ licensed_feature_available?(:suggested_reviewers)
+
+ true
+ end
+ end
+
private
def update_legacy_open_source_license_available
diff --git a/ee/spec/controllers/autocomplete_controller_spec.rb b/ee/spec/controllers/autocomplete_controller_spec.rb
index 79d778f2cd73..ba50fa31d9da 100644
--- a/ee/spec/controllers/autocomplete_controller_spec.rb
+++ b/ee/spec/controllers/autocomplete_controller_spec.rb
@@ -54,6 +54,88 @@
expect(json_response.map { |u| u["username"] }).to match_array([user.username])
end
end
+
+ describe 'GET #users with suggested users' do
+ let_it_be(:suggested_user) { create(:user) }
+ let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+
+ let(:request_common_params) do
+ {
+ active: 'true',
+ project_id: project.id,
+ merge_request_iid: merge_request.iid,
+ current_user: true
+ }
+ end
+
+ let(:request_params) { request_common_params }
+
+ before do
+ project.add_developer(suggested_user)
+ merge_request.build_predictions
+ merge_request.predictions.update!(suggested_reviewers: { reviewers: [suggested_user.username] })
+
+ allow(controller).to receive(:suggested_reviewers_available?).and_return(true)
+ end
+
+ shared_examples 'feature available' do
+ it 'returns the suggested reviewers' do
+ get(:users, params: request_params)
+
+ expect(json_response).to be_kind_of(Array)
+ expect(json_response.size).to eq(3)
+ expect(json_response.map { |user| user['suggested'] }).to match_array([nil, nil, true])
+ end
+ end
+
+ shared_examples 'feature unavailable' do
+ it 'returns no suggested reviewers' do
+ get(:users, params: request_params)
+
+ expect(json_response).to be_kind_of(Array)
+ expect(json_response.size).to eq(3)
+ expect(json_response.map { |user| user['suggested'] }).to match_array([nil, nil, nil])
+ end
+ end
+
+ include_examples 'feature available'
+
+ context 'when suggested reviewers is unavailable for project' do
+ before do
+ allow(controller).to receive(:suggested_reviewers_available?).and_return(false)
+ end
+
+ include_examples 'feature unavailable'
+ end
+
+ context 'when search param is not blank' do
+ let(:request_params) { request_common_params.merge(search: suggested_user.username) }
+
+ it 'returns no suggested reviewers' do
+ get(:users, params: request_params)
+
+ expect(json_response.map { |user| user['suggested'] }).to match_array([nil])
+ end
+ end
+
+ context 'when merge_request_iid is blank' do
+ let(:request_params) { request_common_params.except(:merge_request_iid) }
+
+ include_examples 'feature unavailable'
+ end
+
+ context 'when merge_request is closed' do
+ let_it_be(:merge_request) { create(:merge_request, :closed, source_project: project) }
+
+ include_examples 'feature unavailable'
+ end
+
+ context 'when merge_request has been merged' do
+ let_it_be(:merge_request) { create(:merge_request, :merged, source_project: project) }
+
+ include_examples 'feature unavailable'
+ end
+ end
end
end
diff --git a/ee/spec/features/merge_request/user_edits_multiple_reviewers_mr_spec.rb b/ee/spec/features/merge_request/user_edits_multiple_reviewers_mr_spec.rb
index 23d3064cdcb3..9a822089e23d 100644
--- a/ee/spec/features/merge_request/user_edits_multiple_reviewers_mr_spec.rb
+++ b/ee/spec/features/merge_request/user_edits_multiple_reviewers_mr_spec.rb
@@ -62,4 +62,37 @@
end
end
end
+
+ context 'suggested reviewers', :js, :saas do
+ let_it_be(:suggested_user) { create(:user) }
+
+ before do
+ stub_licensed_features(suggested_reviewers: true)
+ stub_feature_flags(suggested_reviewers: merge_request.project)
+ merge_request.project.add_developer(suggested_user)
+ merge_request.build_predictions
+ merge_request.predictions.update!(suggested_reviewers: { reviewers: [suggested_user.username] })
+ end
+
+ it 'displays suggested reviewers in reviewer dropdown', :aggregate_failures do
+ find('.js-reviewer-search').click
+ wait_for_requests
+
+ page.within '.dropdown-menu-reviewer' do
+ expect(page).to have_content('Suggestion(s)')
+ expect(page).to have_content(suggested_user.name)
+ expect(page).to have_content(suggested_user.username)
+ end
+ end
+
+ it 'removes headers in reviewer dropdown', :aggregate_failures do
+ find('.js-reviewer-search').click
+ wait_for_requests
+
+ page.within '.dropdown-menu-reviewer' do
+ click_on suggested_user.name
+ expect(page).not_to have_content('Suggestion(s)')
+ end
+ end
+ end
end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index d0aa879290c2..9c555fe13e77 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -3885,6 +3885,9 @@ msgstr ""
msgid "All paths are relative to the GitLab URL. Do not include %{relative_url_link_start}relative URLs%{relative_url_link_end}."
msgstr ""
+msgid "All project members"
+msgstr ""
+
msgid "All projects"
msgstr ""
@@ -38756,6 +38759,9 @@ msgstr ""
msgid "Suggestion is not applicable as the suggestion was not found."
msgstr ""
+msgid "Suggestion(s)"
+msgstr ""
+
msgid "Suggestions are not applicable as one or more suggestions were not found."
msgstr ""
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index f27f3b749b1a..54af9d922a58 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -5171,4 +5171,76 @@ def create_build(pipeline, coverage, name)
end
end
end
+
+ describe '#suggested_reviewer_users' do
+ let_it_be(:merge_request) { build(:merge_request, project: project) }
+
+ subject(:suggested_reviewer_users) { merge_request.suggested_reviewer_users }
+
+ shared_examples 'blank suggestions' do
+ it 'returns blank' do
+ expect(suggested_reviewer_users).to eq([])
+ end
+ end
+
+ context 'predictions is nil' do
+ it_behaves_like 'blank suggestions'
+ end
+
+ context 'predictions is not nil' do
+ before do
+ merge_request.build_predictions
+ end
+
+ context 'a non hash' do
+ before do
+ merge_request.build_predictions
+ merge_request.predictions.suggested_reviewers = 1
+ end
+
+ it_behaves_like 'blank suggestions'
+ end
+
+ context 'an empty hash' do
+ before do
+ merge_request.predictions.suggested_reviewers = {}
+ end
+
+ it_behaves_like 'blank suggestions'
+ end
+
+ context 'suggests a user which is not a member' do
+ let_it_be(:non_member) { create(:user) }
+
+ before do
+ merge_request.predictions.suggested_reviewers = { 'reviewers' => [non_member.username] }
+ end
+
+ it_behaves_like 'blank suggestions'
+ end
+
+ context 'suggests a user which is a non member' do
+ let_it_be(:member) { create(:user) }
+
+ before do
+ project.add_developer(member)
+ merge_request.predictions.suggested_reviewers = { 'reviewers' => [member.username] }
+ end
+
+ context 'user is nonactive' do
+ before do
+ member.deactivate
+ end
+
+ it_behaves_like 'blank suggestions'
+ end
+
+ context 'user is active' do
+ it 'returns the user' do
+ expect(suggested_reviewer_users).to eq([member])
+ end
+ end
+ end
+ end
+ end
end
--
GitLab