diff --git a/app/assets/javascripts/users_select/index.js b/app/assets/javascripts/users_select/index.js index 94b4ee77e7e1e188f5704b6df3fddb9209897d7f..2fface2729f75a64a7fc4e0bf134c2800c9687ee 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 88592efcec75514e880eb5221ddaf9d72bd8c13c..45585ab84b49b88f3f47c72166f81f1ab5d5f6ac 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 f2e24f54391cd062b79357ef5b2b3bee309dae4c..e8539f5997e473f029c2a943c8764189ff76aecd 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 a57cb97e9360dbcb7a4d2e5b8f5b2c26d7cbe4a8..161076f99b97ebd39abfb8f1cbf4265e50fa4d80 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 5530ebe0f7aa6454e50e370143ec303b10449171..3387a55f20d917f36e9b3c5acd0f06e47a069a22 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 36825d140621318bc3a39594079bd5a82ec06aa9..caf2e4c89b64fa22c1b5b6a90574edda05812e72 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 5e7dab31e8a1da81164ecd6435927353a38c932b..5082b84978a83056d47217796289afad146d0ebf 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 3f78f29ea2482f860c11070fa95521306e2dea7c..89efd5c11d18ea3b95dfb8b34a15b4cc2446363b 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 a4782a8f6e331701530fbd53a554a9d832784b9e..3b39dcfcfa1cadee21c6a90daf939c23f0df4dad 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 315947e743d17c48afdb62010786a3f12fa3ebfa..db9714e3d136d83ada7000c988d86e3f2259b178 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 83e47dd6243789a0023fcb70d8b1b0b9331e0487..5908e8d0ef3a269800631aa0a56ed7f6f8e822c6 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 79d778f2cd73611cc4d5ec12b5bb644b5d0c47b9..ba50fa31d9daaaee23cf7dcc5663290b20635a70 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 23d3064cdcb34b1cc561459f390ab428a1d66baf..9a822089e23d9fb0bb986c791ef1d0684022bb2e 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 d0aa879290c27c4a0b5cd0297bb94a7eabe67760..9c555fe13e777c0d87bc7703ca794f52f9022aa2 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 f27f3b749b1a5f77885b80c25f1e44b6d1ca007e..54af9d922a58c14e11b5f7913ae405e61f6cf51c 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