diff --git a/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js b/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js index 38a5bdd4a71576de60bf80a1ab7c16e95769b253..d00e6e59cf59348088f07b6e51315d79a2624a4b 100644 --- a/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js +++ b/app/assets/javascripts/filtered_search/add_extra_tokens_for_merge_requests.js @@ -75,6 +75,13 @@ export default (IssuableTokenKeys, disableTargetBranchFilter = false) => { icon: 'approval', tag: '@approved-by', }, + tokenAlternative: { + formattedKey: __('Approved-By'), + key: 'approved-by', + type: 'string', + param: 'usernames', + symbol: '@', + }, condition: [ { url: 'approved_by_usernames[]=None', @@ -105,7 +112,11 @@ export default (IssuableTokenKeys, disableTargetBranchFilter = false) => { const tokenPosition = 3; IssuableTokenKeys.tokenKeys.splice(tokenPosition, 0, ...[approvedBy.token]); - IssuableTokenKeys.tokenKeysWithAlternative.splice(tokenPosition, 0, ...[approvedBy.token]); + IssuableTokenKeys.tokenKeysWithAlternative.splice( + tokenPosition, + 0, + ...[approvedBy.token, approvedBy.tokenAlternative], + ); IssuableTokenKeys.conditions.push(...approvedBy.condition); const environmentToken = { diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index 19fcd91a5b8a812edd37869811d165d1764586a7..e23fa3f7f689e92f59bb3e7382a5f1a69b850dff 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -76,6 +76,7 @@ def filter_items(_items) def filter_negated_items(items) items = super(items) items = by_negated_reviewer(items) + items = by_negated_approved_by(items) by_negated_target_branch(items) end @@ -119,6 +120,12 @@ def by_negated_target_branch(items) end # rubocop: enable CodeReuse/ActiveRecord + def by_negated_approved_by(items) + return items unless not_params[:approved_by_usernames] + + items.not_approved_by_users_with_usernames(not_params[:approved_by_usernames]) + end + def source_project_id @source_project_id ||= params[:source_project_id].presence end diff --git a/app/models/concerns/approvable_base.rb b/app/models/concerns/approvable_base.rb index c2d94b50f8d93d0050b22d2786a16bb5ca58d6c6..ef7ba7b10899d2db9fe42a6dd48fbd6be40a9996 100644 --- a/app/models/concerns/approvable_base.rb +++ b/app/models/concerns/approvable_base.rb @@ -24,6 +24,19 @@ module ApprovableBase .group(:id) .having("COUNT(users.id) = ?", usernames.size) end + + scope :not_approved_by_users_with_usernames, -> (usernames) do + users = User.where(username: usernames).select(:id) + self_table = self.arel_table + app_table = Approval.arel_table + + where( + Approval.where(approvals: { user_id: users }) + .where(app_table[:merge_request_id].eq(self_table[:id])) + .select('true') + .arel.exists.not + ) + end end class_methods do 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 84274d90b93c63980477c8936f2d9ad51cbe8b2b..518f3a779e6e7417d2134e73e08ee3cc252e51f0 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 @@ -37,6 +37,13 @@ const approvers = { icon: 'approval', tag: '@approver', }, + tokenAlternative: { + formattedKey: __('Approver'), + key: 'approver', + type: 'string', + param: 'usernames', + symbol: '@', + }, }; export default (IssuableTokenKeys, disableTargetBranchFilter = false) => { @@ -44,6 +51,10 @@ export default (IssuableTokenKeys, disableTargetBranchFilter = false) => { const tokenPosition = 3; IssuableTokenKeys.tokenKeys.splice(tokenPosition, 0, ...[approvers.token]); - IssuableTokenKeys.tokenKeysWithAlternative.splice(tokenPosition, 0, ...[approvers.token]); + IssuableTokenKeys.tokenKeysWithAlternative.splice( + tokenPosition, + 0, + ...[approvers.token, approvers.tokenAlternative], + ); IssuableTokenKeys.conditions.push(...approvers.condition); }; diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 3b835d366dbec7f7e805da3a3eb4b2a7d94dc8da..c2ea918449c512f96fe168a475319f5c59fef2ae 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -520,6 +520,44 @@ def find(label_name) end end + context 'filtering by approved by' do + let(:params) { { approved_by_usernames: user2.username } } + + before do + create(:approval, merge_request: merge_request3, user: user2) + end + + it 'returns merge requests approved by that user' do + merge_requests = described_class.new(user, params).execute + + expect(merge_requests).to contain_exactly(merge_request3) + end + + context 'not filter' do + let(:params) { { not: { approved_by_usernames: user2.username } } } + + it 'returns merge requests not approved by that user' do + merge_requests = described_class.new(user, params).execute + + expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request4, merge_request5) + end + end + + context 'when filtering by author and not approved by' do + let(:params) { { not: { approved_by_usernames: user2.username }, author_username: user.username } } + + before do + merge_request4.update!(author: user2) + end + + it 'returns merge requests authored by user and not approved by user2' do + merge_requests = described_class.new(user, params).execute + + expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request5) + end + end + end + context 'filtering by created_at/updated_at' do let(:new_project) { create(:project, forked_from_project: project1) } diff --git a/spec/models/concerns/approvable_base_spec.rb b/spec/models/concerns/approvable_base_spec.rb index a9e944cf220429fb453f8ca4eb32934de46d5776..c7ea2631a24ffc44cd90e1412e80aa71a4ff1515 100644 --- a/spec/models/concerns/approvable_base_spec.rb +++ b/spec/models/concerns/approvable_base_spec.rb @@ -59,4 +59,25 @@ end end end + + describe '.not_approved_by_users_with_usernames' do + subject { MergeRequest.not_approved_by_users_with_usernames([user.username, user2.username]) } + + let!(:merge_request2) { create(:merge_request) } + let!(:merge_request3) { create(:merge_request) } + let!(:merge_request4) { create(:merge_request) } + let(:user2) { create(:user) } + let(:user3) { create(:user) } + + before do + create(:approval, merge_request: merge_request, user: user) + create(:approval, merge_request: merge_request2, user: user2) + create(:approval, merge_request: merge_request2, user: user3) + create(:approval, merge_request: merge_request4, user: user3) + end + + it 'has the merge request that is not approved at all and not approved by either user' do + expect(subject).to contain_exactly(merge_request3, merge_request4) + end + end end