From 3bf388888882aca098659d71fb6e84ba3daa371f Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Mon, 2 Nov 2020 16:51:12 -0800 Subject: [PATCH 1/3] Display which approval rules match a given reviewer Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/233736 --- app/assets/javascripts/users_select/index.js | 55 +++++++++++++++++-- app/helpers/form_helper.rb | 10 +++- .../_metadata_issuable_reviewer.html.haml | 2 +- .../233736-add-reviewer-approval-rules.yml | 5 ++ .../user_edits_multiple_reviewers_mr_spec.rb | 24 ++++++++ locale/gitlab.pot | 3 + 6 files changed, 92 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/233736-add-reviewer-approval-rules.yml diff --git a/app/assets/javascripts/users_select/index.js b/app/assets/javascripts/users_select/index.js index 971c9978865719..af53d11cf644b1 100644 --- a/app/assets/javascripts/users_select/index.js +++ b/app/assets/javascripts/users_select/index.js @@ -49,6 +49,7 @@ function UsersSelect(currentUser, els, options = {}) { options.todoStateFilter = $dropdown.data('todoStateFilter'); options.iid = $dropdown.data('iid'); options.issuableType = $dropdown.data('issuableType'); + options.targetBranch = $dropdown.data('targetBranch'); const showNullUser = $dropdown.data('nullUser'); const defaultNullUser = $dropdown.data('nullUserDefault'); const showMenuAbove = $dropdown.data('showMenuAbove'); @@ -582,7 +583,14 @@ function UsersSelect(currentUser, els, options = {}) { img = ``; } - return userSelect.renderRow(options.issuableType, user, selected, username, img); + return userSelect.renderRow( + options.issuableType, + user, + selected, + username, + img, + elsClassName, + ); }, }); }); @@ -746,8 +754,16 @@ UsersSelect.prototype.users = function(query, options, callback) { ...getAjaxUsersSelectParams(options, AJAX_USERS_SELECT_PARAMS_MAP), }; - if (options.issuableType === 'merge_request') { + if ( + options.issuableType === 'merge_request' || + (!options.issuableType && (options.iid || options.targetBranch)) + ) { params.merge_request_iid = options.iid || null; + params.approval_rules = true; + + if (!options.iid) { + params.target_branch = options.targetBranch || null; + } } return axios.get(url, { params }).then(({ data }) => { @@ -762,7 +778,14 @@ UsersSelect.prototype.buildUrl = function(url) { return url; }; -UsersSelect.prototype.renderRow = function(issuableType, user, selected, username, img) { +UsersSelect.prototype.renderRow = function( + issuableType, + user, + selected, + username, + img, + elsClassName, +) { const tooltip = issuableType === 'merge_request' && !user.can_merge ? __('Cannot merge') : ''; const tooltipClass = tooltip ? `has-tooltip` : ''; const selectedClass = selected === true ? 'is-active' : ''; @@ -776,10 +799,15 @@ UsersSelect.prototype.renderRow = function(issuableType, user, selected, usernam ${this.renderRowAvatar(issuableType, user, img)} - + ${escape(user.name)} - ${username ? `${username}` : ''} + ${ + username + ? `${username}` + : '' + } + ${this.renderApprovalRules(elsClassName, user.applicable_approval_rules)} @@ -802,4 +830,21 @@ UsersSelect.prototype.renderRowAvatar = function(issuableType, user, img) { `; }; +UsersSelect.prototype.renderApprovalRules = function(elsClassName, approvalRules = []) { + if (!elsClassName?.includes('reviewer')) { + return ''; + } + const count = approvalRules.length; + const [rule] = approvalRules; + const countText = sprintf(__('(+%{count} rules)'), { count }); + const renderApprovalRulesCount = count > 1 ? `${countText}` : ''; + + return count + ? `
+ ${rule.name} + ${renderApprovalRulesCount} +
` + : ''; +}; + export default UsersSelect; diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 8a8d708b0b24dc..d0276c91316af3 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -55,7 +55,7 @@ def assignees_dropdown_options(issuable_type) dropdown_data end - def reviewers_dropdown_options(issuable_type) + def reviewers_dropdown_options(issuable_type, iid = nil, target_branch = nil) dropdown_data = { toggle_class: 'js-reviewer-search js-multiselect js-save-user-data', title: 'Request review from', @@ -78,6 +78,14 @@ def reviewers_dropdown_options(issuable_type) } } + if iid + dropdown_data[:data][:iid] = iid + end + + if target_branch + dropdown_data[:data][:target_branch] = target_branch + end + if merge_request_supports_multiple_reviewers? dropdown_data = multiple_reviewers_dropdown_options(dropdown_data) end diff --git a/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml b/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml index a4e2ce035cc79a..a0df007f8ca07c 100644 --- a/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml +++ b/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml @@ -7,6 +7,6 @@ - if issuable.reviewers.empty? = hidden_field_tag "#{issuable.to_ability_name}[reviewer_ids][]", 0, id: nil, data: { meta: '' } - = dropdown_tag(users_dropdown_label(issuable.reviewers), options: reviewers_dropdown_options(issuable.to_ability_name)) + = dropdown_tag(users_dropdown_label(issuable.reviewers), options: reviewers_dropdown_options(issuable.to_ability_name, issuable.iid, issuable.target_branch)) - if Feature.enabled?(:mr_collapsed_approval_rules, @project) = render_if_exists 'shared/issuable/approver_suggestion', issuable: issuable, presenter: presenter diff --git a/changelogs/unreleased/233736-add-reviewer-approval-rules.yml b/changelogs/unreleased/233736-add-reviewer-approval-rules.yml new file mode 100644 index 00000000000000..c09f5fb96d7802 --- /dev/null +++ b/changelogs/unreleased/233736-add-reviewer-approval-rules.yml @@ -0,0 +1,5 @@ +--- +title: Display approval rules in reviewer dropdown +merge_request: 46738 +author: +type: added 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 2ebb76b01a29d9..98693c6e030151 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 @@ -10,4 +10,28 @@ end it_behaves_like 'multiple reviewers merge request', 'updates', 'Save changes' + + context 'user approval rules', :js do + let(:rule_name) { 'some-custom-rule' } + let(:user) { create(:admin) } + let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [user], name: rule_name, approvals_required: 1 )} + + it 'is not shown in assignee dropdown' do + find('.js-assignee-search').click + wait_for_requests + + page.within '.dropdown-menu-assignee' do + expect(page).not_to have_content(rule_name) + end + end + + it 'is shown in reviewer dropdown' do + find('.js-reviewer-search').click + wait_for_requests + + page.within '.dropdown-menu-reviewer' do + expect(page).to have_content(rule_name) + end + end + end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f0336bb3af631d..50cb411f38e512 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -947,6 +947,9 @@ msgstr "" msgid "(%{value}) has already been taken" msgstr "" +msgid "(+%{count} rules)" +msgstr "" + msgid "(No changes)" msgstr "" -- GitLab From d94ffb928ccb4de75b89393deb3155c7a3d6d302 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Mon, 14 Dec 2020 20:31:24 -0800 Subject: [PATCH 2/3] Hide behind feature flag Add reviewer_approval_rules FF --- app/assets/javascripts/users_select/index.js | 2 +- .../merge_requests/creations_controller.rb | 1 + .../projects/merge_requests_controller.rb | 1 + .../development/reviewer_approval_rules.yml | 8 ++++++++ .../user_edits_multiple_reviewers_mr_spec.rb | 19 ++++++++++++++++++- 5 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 config/feature_flags/development/reviewer_approval_rules.yml diff --git a/app/assets/javascripts/users_select/index.js b/app/assets/javascripts/users_select/index.js index af53d11cf644b1..8309cdf7059b76 100644 --- a/app/assets/javascripts/users_select/index.js +++ b/app/assets/javascripts/users_select/index.js @@ -831,7 +831,7 @@ UsersSelect.prototype.renderRowAvatar = function(issuableType, user, img) { }; UsersSelect.prototype.renderApprovalRules = function(elsClassName, approvalRules = []) { - if (!elsClassName?.includes('reviewer')) { + if (!gon.features?.reviewerApprovalRules || !elsClassName?.includes('reviewer')) { return ''; } const count = approvalRules.length; diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 453928e251f424..14fd26f1c4372b 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -14,6 +14,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap before_action do push_frontend_feature_flag(:merge_request_reviewers, @project) push_frontend_feature_flag(:mr_collapsed_approval_rules, @project) + push_frontend_feature_flag(:reviewer_approval_rules, @project) end def new diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ee25a2006ad810..6d0dbc655852af 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -53,6 +53,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:vue_issuable_sidebar, @project.group) push_frontend_feature_flag(:merge_request_reviewers, @project) push_frontend_feature_flag(:mr_collapsed_approval_rules, @project) + push_frontend_feature_flag(:reviewer_approval_rules, @project) end around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions] diff --git a/config/feature_flags/development/reviewer_approval_rules.yml b/config/feature_flags/development/reviewer_approval_rules.yml new file mode 100644 index 00000000000000..97181ef2a36467 --- /dev/null +++ b/config/feature_flags/development/reviewer_approval_rules.yml @@ -0,0 +1,8 @@ +--- +name: reviewer_approval_rules +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46738 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/293742 +milestone: '13.7' +type: development +group: group::code review +default_enabled: false 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 98693c6e030151..53f001034b2e44 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 @@ -13,7 +13,6 @@ context 'user approval rules', :js do let(:rule_name) { 'some-custom-rule' } - let(:user) { create(:admin) } let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [user], name: rule_name, approvals_required: 1 )} it 'is not shown in assignee dropdown' do @@ -34,4 +33,22 @@ end end end + + context 'when reviewer_approval_rules feature flag off' do + let(:rule_name) { 'some-custom-rule' } + let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [user], name: rule_name, approvals_required: 1 )} + + before do + stub_feature_flags(reviewer_approval_rules: false) + end + + it 'is not shown in reviewer dropdown' do + find('.js-reviewer-search').click + wait_for_requests + + page.within '.dropdown-menu-reviewer' do + expect(page).not_to have_content(rule_name) + end + end + end end -- GitLab From ef89ff91fedfdfbcc44e2cc05c8c336645614172 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Tue, 15 Dec 2020 17:09:32 -0800 Subject: [PATCH 3/3] Address reviewer & attempt to solve confusing code --- app/assets/javascripts/users_select/index.js | 18 ++++++++++-------- .../233736-add-reviewer-approval-rules.yml | 5 ----- 2 files changed, 10 insertions(+), 13 deletions(-) delete mode 100644 changelogs/unreleased/233736-add-reviewer-approval-rules.yml diff --git a/app/assets/javascripts/users_select/index.js b/app/assets/javascripts/users_select/index.js index 8309cdf7059b76..e693c3e90a4d36 100644 --- a/app/assets/javascripts/users_select/index.js +++ b/app/assets/javascripts/users_select/index.js @@ -754,16 +754,17 @@ UsersSelect.prototype.users = function(query, options, callback) { ...getAjaxUsersSelectParams(options, AJAX_USERS_SELECT_PARAMS_MAP), }; - if ( - options.issuableType === 'merge_request' || - (!options.issuableType && (options.iid || options.targetBranch)) - ) { + const isMergeRequest = options.issuableType === 'merge_request'; + const isEditMergeRequest = !options.issuableType && (options.iid && options.targetBranch); + const isNewMergeRequest = !options.issuableType && (!options.iid && options.targetBranch); + + if (isMergeRequest || isEditMergeRequest || isNewMergeRequest) { params.merge_request_iid = options.iid || null; params.approval_rules = true; + } - if (!options.iid) { - params.target_branch = options.targetBranch || null; - } + if (isNewMergeRequest) { + params.target_branch = options.targetBranch || null; } return axios.get(url, { params }).then(({ data }) => { @@ -804,7 +805,7 @@ UsersSelect.prototype.renderRow = function( ${ username - ? `${username}` + ? `${username}` : '' } ${this.renderApprovalRules(elsClassName, user.applicable_approval_rules)} @@ -834,6 +835,7 @@ UsersSelect.prototype.renderApprovalRules = function(elsClassName, approvalRules if (!gon.features?.reviewerApprovalRules || !elsClassName?.includes('reviewer')) { return ''; } + const count = approvalRules.length; const [rule] = approvalRules; const countText = sprintf(__('(+%{count} rules)'), { count }); diff --git a/changelogs/unreleased/233736-add-reviewer-approval-rules.yml b/changelogs/unreleased/233736-add-reviewer-approval-rules.yml deleted file mode 100644 index c09f5fb96d7802..00000000000000 --- a/changelogs/unreleased/233736-add-reviewer-approval-rules.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Display approval rules in reviewer dropdown -merge_request: 46738 -author: -type: added -- GitLab