From 20312087dbd16a08f416985727e3ae26420529da Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Thu, 23 Apr 2020 15:59:23 -0700 Subject: [PATCH 1/3] Add "Approved By" in filtered MR This MR adds a new options in the filtered search: "Approved By". Allowing to select an user, none, or any. Available for starter/bronze. --- .../shared/issuable/_search_bar.html.haml | 1 + .../add_extra_tokens_for_merge_requests.js | 71 +++++++++++++++---- .../available_dropdown_mappings.js | 6 ++ .../javascripts/filtered_search/constants.js | 2 +- .../issuable/_approved_by_dropdown.html.haml | 16 +++++ ...-filter-merge-request-approved-by-user.yml | 5 ++ locale/gitlab.pot | 3 + 7 files changed, 91 insertions(+), 13 deletions(-) create mode 100644 ee/app/views/shared/issuable/_approved_by_dropdown.html.haml create mode 100644 ee/changelogs/unreleased/39042-filter-merge-request-approved-by-user.yml diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index cee8d34a881b34..34be9291f1f9ef 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -74,6 +74,7 @@ user: User.new(username: '{{username}}', name: '{{name}}'), avatar: { lazy: true, url: '{{avatar_url}}' } = render_if_exists 'shared/issuable/approver_dropdown' + = render_if_exists 'shared/issuable/approved_by_dropdown' #js-dropdown-milestone.filtered-search-input-dropdown-menu.dropdown-menu %ul{ data: { dropdown: true } } %li.filter-dropdown-item{ data: { value: 'None' } } diff --git a/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js b/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js index 86e1fa6d2705b5..0cf5b33f1330bb 100644 --- a/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js +++ b/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js @@ -1,10 +1,8 @@ import addExtraTokensForMergeRequests from '~/filtered_search/add_extra_tokens_for_merge_requests'; import { __ } from '~/locale'; -export default IssuableTokenKeys => { - addExtraTokensForMergeRequests(IssuableTokenKeys); - - const approversConditions = [ +const approvers = { + condition: [ { url: 'approver_usernames[]=None', tokenKey: 'approver', @@ -29,9 +27,8 @@ export default IssuableTokenKeys => { value: __('Any'), operator: '!=', }, - ]; - - const approversToken = { + ], + token: { formattedKey: __('Approver'), key: 'approver', type: 'array', @@ -39,10 +36,60 @@ export default IssuableTokenKeys => { symbol: '@', icon: 'approval', tag: '@approver', - }; - const approversTokenPosition = 2; + }, +}; + +const approvedBy = { + condition: [ + { + url: 'approved_by_usernames[]=None', + tokenKey: 'approved-by', + value: __('None'), + operator: '=', + }, + { + url: 'not[approved_by_usernames][]=None', + tokenKey: 'approved-by', + value: __('None'), + operator: '!=', + }, + { + url: 'approved_by_usernames[]=Any', + tokenKey: 'approved-by', + value: __('Any'), + operator: '=', + }, + { + url: 'not[approved_by_usernames][]=Any', + tokenKey: 'approved-by', + value: __('Any'), + operator: '!=', + }, + ], + token: { + formattedKey: __('Approved-By'), + key: 'approved-by', + type: 'array', + param: 'usernames[]', + symbol: '@', + icon: 'approval', + tag: '@approved-by', + }, +}; + +export default IssuableTokenKeys => { + addExtraTokensForMergeRequests(IssuableTokenKeys); + const tokenPosition = 2; + const labels = [approvers, approvedBy]; + const combinedTokens = []; + const combinedConditions = []; + + labels.forEach(({ token, condition }) => { + combinedTokens.push(token); + combinedConditions.push(...condition); + }); - IssuableTokenKeys.tokenKeys.splice(approversTokenPosition, 0, approversToken); - IssuableTokenKeys.tokenKeysWithAlternative.splice(approversTokenPosition, 0, approversToken); - IssuableTokenKeys.conditions.push(...approversConditions); + IssuableTokenKeys.tokenKeys.splice(tokenPosition, 0, ...combinedTokens); + IssuableTokenKeys.tokenKeysWithAlternative.splice(tokenPosition, 0, ...combinedTokens); + IssuableTokenKeys.conditions.push(...combinedConditions); }; diff --git a/ee/app/assets/javascripts/filtered_search/available_dropdown_mappings.js b/ee/app/assets/javascripts/filtered_search/available_dropdown_mappings.js index 98e2ec8164ce59..e23a1a94562d4b 100644 --- a/ee/app/assets/javascripts/filtered_search/available_dropdown_mappings.js +++ b/ee/app/assets/javascripts/filtered_search/available_dropdown_mappings.js @@ -47,6 +47,12 @@ export default class AvailableDropdownMappings { element: this.container.querySelector('#js-dropdown-approver'), }; + ceMappings['approved-by'] = { + reference: null, + gl: DropdownUser, + element: this.container.querySelector('#js-dropdown-approved-by'), + }; + ceMappings.weight = { reference: null, gl: DropdownWeight, diff --git a/ee/app/assets/javascripts/filtered_search/constants.js b/ee/app/assets/javascripts/filtered_search/constants.js index 339099bba20898..04524688e8690d 100644 --- a/ee/app/assets/javascripts/filtered_search/constants.js +++ b/ee/app/assets/javascripts/filtered_search/constants.js @@ -1,4 +1,4 @@ /* eslint-disable import/prefer-default-export */ import { USER_TOKEN_TYPES as CE_USER_TOKEN_TYPES } from '~/filtered_search/constants'; -export const USER_TOKEN_TYPES = [...CE_USER_TOKEN_TYPES, 'approver']; +export const USER_TOKEN_TYPES = [...CE_USER_TOKEN_TYPES, 'approver', 'approved-by']; diff --git a/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml b/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml new file mode 100644 index 00000000000000..8014545ab85265 --- /dev/null +++ b/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml @@ -0,0 +1,16 @@ +#js-dropdown-approved-by.filtered-search-input-dropdown-menu.dropdown-menu + %ul{ data: { dropdown: true } } + %li.filter-dropdown-item{ data: { value: 'None' } } + %button.btn.btn-link{ type: 'button' } + = _('None') + %li.filter-dropdown-item{ data: { value: 'Any' } } + %button.btn.btn-link{ type: 'button' } + = _('Any') + %li.divider.droplab-item-ignore + - if current_user + = render 'shared/issuable/user_dropdown_item', + user: current_user + %ul.filter-dropdown{ data: { dynamic: true, dropdown: true } } + = render 'shared/issuable/user_dropdown_item', + user: User.new(username: '{{username}}', name: '{{name}}'), + avatar: { lazy: true, url: '{{avatar_url}}' } diff --git a/ee/changelogs/unreleased/39042-filter-merge-request-approved-by-user.yml b/ee/changelogs/unreleased/39042-filter-merge-request-approved-by-user.yml new file mode 100644 index 00000000000000..ad056e0fb4ca39 --- /dev/null +++ b/ee/changelogs/unreleased/39042-filter-merge-request-approved-by-user.yml @@ -0,0 +1,5 @@ +--- +title: Add Approved By in filtered MR search +merge_request: 30335 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7cec0bbee5b2ed..ed63596d582134 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2482,6 +2482,9 @@ msgstr "" msgid "Approved the current merge request." msgstr "" +msgid "Approved-By" +msgstr "" + msgid "Approver" msgstr "" -- GitLab From f20d684cde34cca2c72a77487704b82ca73dc9e5 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Thu, 7 May 2020 17:12:32 -0700 Subject: [PATCH 2/3] Address reviewer's comments --- .../add_extra_tokens_for_merge_requests.js | 10 ++-------- .../shared/issuable/_approved_by_dropdown.html.haml | 4 ++-- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js b/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js index 0cf5b33f1330bb..5f564123a02081 100644 --- a/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js +++ b/ee/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js @@ -80,14 +80,8 @@ const approvedBy = { export default IssuableTokenKeys => { addExtraTokensForMergeRequests(IssuableTokenKeys); const tokenPosition = 2; - const labels = [approvers, approvedBy]; - const combinedTokens = []; - const combinedConditions = []; - - labels.forEach(({ token, condition }) => { - combinedTokens.push(token); - combinedConditions.push(...condition); - }); + const combinedTokens = [approvers.token, approvedBy.token]; + const combinedConditions = [approvers.condition, approvedBy.condition]; IssuableTokenKeys.tokenKeys.splice(tokenPosition, 0, ...combinedTokens); IssuableTokenKeys.tokenKeysWithAlternative.splice(tokenPosition, 0, ...combinedTokens); diff --git a/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml b/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml index 8014545ab85265..eae1ae212d2474 100644 --- a/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml +++ b/ee/app/views/shared/issuable/_approved_by_dropdown.html.haml @@ -1,9 +1,9 @@ #js-dropdown-approved-by.filtered-search-input-dropdown-menu.dropdown-menu %ul{ data: { dropdown: true } } - %li.filter-dropdown-item{ data: { value: 'None' } } + %li.filter-dropdown-item{ data: { value: _('None') } } %button.btn.btn-link{ type: 'button' } = _('None') - %li.filter-dropdown-item{ data: { value: 'Any' } } + %li.filter-dropdown-item{ data: { value: _('Any') } } %button.btn.btn-link{ type: 'button' } = _('Any') %li.divider.droplab-item-ignore -- GitLab From b6f6f8a2fef1a320c7ddf8f1e4ac39fa803bb152 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Fri, 8 May 2020 04:33:45 +0000 Subject: [PATCH 3/3] Add feature spec for "Approved by" filter MR Test the scenarios "any", "none", "@username" is working when user select the "Approved-By" Filter MR and it will display the respective MRs matching that parameter. --- .../user_filters_by_approvers_spec.rb | 152 +++++++++++++----- 1 file changed, 108 insertions(+), 44 deletions(-) diff --git a/ee/spec/features/merge_requests/user_filters_by_approvers_spec.rb b/ee/spec/features/merge_requests/user_filters_by_approvers_spec.rb index c84a76e88715b5..83f66086ff1eb8 100644 --- a/ee/spec/features/merge_requests/user_filters_by_approvers_spec.rb +++ b/ee/spec/features/merge_requests/user_filters_by_approvers_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe 'Merge Requests > User filters by approvers', :js do +describe 'Merge Requests > User filters', :js do include FilteredSearchHelpers let(:project) { create(:project, :public, :repository) } @@ -30,68 +30,132 @@ visit project_merge_requests_path(project) end - context 'filtering by approver:none' do - it 'applies the filter' do - input_filtered_search('approver:=none') + context 'by "approvers"' do + context 'filtering by approver:none' do + it 'applies the filter' do + input_filtered_search('approver:=none') - expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) - expect(page).not_to have_content 'Bugfix1' - expect(page).not_to have_content 'Bugfix2' - expect(page).not_to have_content 'Bugfix4' - expect(page).to have_content 'Bugfix3' + expect(page).not_to have_content 'Bugfix1' + expect(page).not_to have_content 'Bugfix2' + expect(page).not_to have_content 'Bugfix4' + expect(page).to have_content 'Bugfix3' + end end - end - context 'filtering by approver:any' do - it 'applies the filter' do - input_filtered_search('approver:=any') + context 'filtering by approver:any' do + it 'applies the filter' do + input_filtered_search('approver:=any') - expect(page).to have_issuable_counts(open: 3, closed: 0, all: 3) + expect(page).to have_issuable_counts(open: 3, closed: 0, all: 3) - expect(page).to have_content 'Bugfix1' - expect(page).to have_content 'Bugfix2' - expect(page).to have_content 'Bugfix4' - expect(page).not_to have_content 'Bugfix3' + expect(page).to have_content 'Bugfix1' + expect(page).to have_content 'Bugfix2' + expect(page).to have_content 'Bugfix4' + expect(page).not_to have_content 'Bugfix3' + end end - end - context 'filtering by approver:@username' do - it 'applies the filter' do - input_filtered_search("approver:=@#{first_user.username}") + context 'filtering by approver:@username' do + it 'applies the filter' do + input_filtered_search("approver:=@#{first_user.username}") - expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2) + expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2) - expect(page).to have_content 'Bugfix1' - expect(page).to have_content 'Bugfix2' - expect(page).not_to have_content 'Bugfix3' - expect(page).not_to have_content 'Bugfix4' + expect(page).to have_content 'Bugfix1' + expect(page).to have_content 'Bugfix2' + expect(page).not_to have_content 'Bugfix3' + expect(page).not_to have_content 'Bugfix4' + end + end + + context 'filtering by multiple approvers' do + it 'applies the filter' do + input_filtered_search("approver:=@#{first_user.username} approver:=@#{user.username}") + + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + + expect(page).to have_content 'Bugfix2' + expect(page).not_to have_content 'Bugfix1' + expect(page).not_to have_content 'Bugfix3' + expect(page).not_to have_content 'Bugfix4' + end end - end - context 'filtering by multiple approvers' do - it 'applies the filter' do - input_filtered_search("approver:=@#{first_user.username} approver:=@#{user.username}") + context 'filtering by an approver from a group' do + it 'applies the filter' do + input_filtered_search("approver:=@#{group_user.username}") - expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) - expect(page).to have_content 'Bugfix2' - expect(page).not_to have_content 'Bugfix1' - expect(page).not_to have_content 'Bugfix3' - expect(page).not_to have_content 'Bugfix4' + expect(page).to have_content 'Bugfix4' + expect(page).not_to have_content 'Bugfix1' + expect(page).not_to have_content 'Bugfix2' + expect(page).not_to have_content 'Bugfix3' + end end end - context 'filtering by an approver from a group' do - it 'applies the filter' do - input_filtered_search("approver:=@#{group_user.username}") + context 'by "approved by"' do + let!(:merge_request_with_first_user_approval) do + create(:merge_request, source_project: project, title: 'Bugfix5').tap do |mr| + create(:approval, merge_request: mr, user: first_user) + end + end + + let!(:merge_request_with_group_user_approved) do + group = create(:group) + group.add_developer(group_user) + + create(:merge_request, source_project: project, title: 'Bugfix6', approval_groups: [group], source_branch: 'bugfix6').tap do |mr| + create(:approval, merge_request: mr, user: group_user) + end + end + + context 'filtering by approved-by:none' do + it 'applies the filter' do + input_filtered_search('approved-by:=none') + + expect(page).to have_issuable_counts(open: 4, closed: 0, all: 4) + + expect(page).not_to have_content 'Bugfix5' + expect(page).to have_content 'Bugfix3' + end + end + + context 'filtering by approved-by:any' do + it 'applies the filter' do + input_filtered_search('approved-by:=any') + + expect(page).to have_issuable_counts(open: 2, closed: 0, all: 2) + + expect(page).to have_content 'Bugfix5' + expect(page).not_to have_content 'Bugfix3' + end + end + + context 'filtering by approved-by:@username' do + it 'applies the filter' do + input_filtered_search("approved-by:=@#{first_user.username}") + + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + + expect(page).to have_content 'Bugfix5' + expect(page).not_to have_content 'Bugfix3' + end + end + + context 'filtering by an approver from a group' do + it 'applies the filter' do + input_filtered_search("approved-by:=@#{group_user.username}") - expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) + expect(page).to have_issuable_counts(open: 1, closed: 0, all: 1) - expect(page).to have_content 'Bugfix4' - expect(page).not_to have_content 'Bugfix1' - expect(page).not_to have_content 'Bugfix2' - expect(page).not_to have_content 'Bugfix3' + expect(page).to have_content 'Bugfix6' + expect(page).not_to have_content 'Bugfix5' + expect(page).not_to have_content 'Bugfix3' + end end end end -- GitLab