From 27c2524880c71b98385ba4a1aed09587d5ae9d98 Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Thu, 4 Mar 2021 16:05:50 +1300 Subject: [PATCH 1/5] Add the ability to use != approved-by in merge request filter MR: gitlab.com/gitlab-org/gitlab/-/merge_requests/55025 Issue: gitlab.com/gitlab-org/gitlab/-/issues/218570 --- app/finders/merge_requests_finder.rb | 7 +++++++ app/models/concerns/approvable_base.rb | 4 ++++ spec/finders/merge_requests_finder_spec.rb | 22 ++++++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index 19fcd91a5b8a81..e23fa3f7f689e9 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 c2d94b50f8d93d..bcba7dfdd9e0b1 100644 --- a/app/models/concerns/approvable_base.rb +++ b/app/models/concerns/approvable_base.rb @@ -24,6 +24,10 @@ module ApprovableBase .group(:id) .having("COUNT(users.id) = ?", usernames.size) end + + scope :not_approved_by_users_with_usernames, -> (*usernames) do + MergeRequest.from_union([without_approvals, with_approvals.merge(Approval.with_user).where.not(users: { username: usernames })]) + end end class_methods do diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 3b835d366dbec7..63d0f9b42a20c0 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -520,6 +520,28 @@ 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 not created 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 created by that user' do + merge_requests = described_class.new(user, params).execute + expect(merge_requests).to_not include(merge_request3) + end + end + end + context 'filtering by created_at/updated_at' do let(:new_project) { create(:project, forked_from_project: project1) } -- GitLab From c80a7cf239f41d161b85dd503a3bbd72fc6fcee1 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 22 Jun 2021 16:33:48 +0100 Subject: [PATCH 2/5] Fixed frontend not showing the token correctly --- .../add_extra_tokens_for_merge_requests.js | 13 ++++++++++++- .../add_extra_tokens_for_merge_requests.js | 13 ++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) 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 38a5bdd4a71576..d00e6e59cf5934 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/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 84274d90b93c63..518f3a779e6e74 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); }; -- GitLab From f8004a6df3f17bbab7c48a6d8d4ef0253cc63526 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 22 Jun 2021 16:34:01 +0100 Subject: [PATCH 3/5] Fixed failing rubocop --- spec/finders/merge_requests_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 63d0f9b42a20c0..79ca3c91a6b31f 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -537,7 +537,7 @@ def find(label_name) it 'returns merge requests not created by that user' do merge_requests = described_class.new(user, params).execute - expect(merge_requests).to_not include(merge_request3) + expect(merge_requests).not_to include(merge_request3) end end end -- GitLab From f5cdf3fdb0234d4f7342f70b62c824a27ee179b6 Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Wed, 23 Jun 2021 17:50:47 +0200 Subject: [PATCH 4/5] Add test for not approved by and author --- app/models/concerns/approvable_base.rb | 4 ++-- spec/finders/merge_requests_finder_spec.rb | 22 +++++++++++++++++--- spec/models/concerns/approvable_base_spec.rb | 17 +++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/approvable_base.rb b/app/models/concerns/approvable_base.rb index bcba7dfdd9e0b1..e5197df8f057e1 100644 --- a/app/models/concerns/approvable_base.rb +++ b/app/models/concerns/approvable_base.rb @@ -25,8 +25,8 @@ module ApprovableBase .having("COUNT(users.id) = ?", usernames.size) end - scope :not_approved_by_users_with_usernames, -> (*usernames) do - MergeRequest.from_union([without_approvals, with_approvals.merge(Approval.with_user).where.not(users: { username: usernames })]) + scope :not_approved_by_users_with_usernames, -> (usernames) do + left_outer_joins(:approvals).and(where(approvals: { id: nil }).or(where.not(approvals: { user_id: User.where(username: usernames).pluck(:id) }))) end end diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 79ca3c91a6b31f..c2ea918449c512 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -527,17 +527,33 @@ def find(label_name) create(:approval, merge_request: merge_request3, user: user2) end - it 'returns merge requests not created by that user' do + 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 created by that user' do + 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).not_to include(merge_request3) + + expect(merge_requests).to contain_exactly(merge_request1, merge_request2, merge_request5) end end end diff --git a/spec/models/concerns/approvable_base_spec.rb b/spec/models/concerns/approvable_base_spec.rb index a9e944cf220429..887237ca6d0735 100644 --- a/spec/models/concerns/approvable_base_spec.rb +++ b/spec/models/concerns/approvable_base_spec.rb @@ -59,4 +59,21 @@ 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(:user2) { create(:user) } + + before do + create(:approval, merge_request: merge_request, user: user) + create(:approval, merge_request: merge_request2, user: user2) + end + + it 'has the merge request that is not approved at all and not approved by user' do + expect(subject).to contain_exactly(merge_request3) + end + end end -- GitLab From f39c9ded0b5f4391b37db6915803bd6fabc77f90 Mon Sep 17 00:00:00 2001 From: Marc Shaw Date: Fri, 25 Jun 2021 17:08:48 +0200 Subject: [PATCH 5/5] Improve query and test coverage for edge cases --- app/models/concerns/approvable_base.rb | 11 ++++++++++- spec/models/concerns/approvable_base_spec.rb | 8 ++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/approvable_base.rb b/app/models/concerns/approvable_base.rb index e5197df8f057e1..ef7ba7b10899d2 100644 --- a/app/models/concerns/approvable_base.rb +++ b/app/models/concerns/approvable_base.rb @@ -26,7 +26,16 @@ module ApprovableBase end scope :not_approved_by_users_with_usernames, -> (usernames) do - left_outer_joins(:approvals).and(where(approvals: { id: nil }).or(where.not(approvals: { user_id: User.where(username: usernames).pluck(:id) }))) + 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 diff --git a/spec/models/concerns/approvable_base_spec.rb b/spec/models/concerns/approvable_base_spec.rb index 887237ca6d0735..c7ea2631a24ffc 100644 --- a/spec/models/concerns/approvable_base_spec.rb +++ b/spec/models/concerns/approvable_base_spec.rb @@ -65,15 +65,19 @@ 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 user' do - expect(subject).to contain_exactly(merge_request3) + 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 -- GitLab