Merge branch '218570-not-filter-operator-does-not-work-for-several-filters' into 'master'

Add the ability to use != approved-by in merge request filter

See merge request gitlab-org/gitlab!55025

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 = {

app/finders/merge_requests_finder.rb

@@ -76,6 +76,7 @@ class MergeRequestsFinder < IssuableFinder
   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 @@ class MergeRequestsFinder < IssuableFinder
   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

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

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);
 };

spec/finders/merge_requests_finder_spec.rb

@@ -520,6 +520,44 @@ RSpec.describe MergeRequestsFinder do
         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) }
 

spec/models/concerns/approvable_base_spec.rb

@@ -59,4 +59,25 @@ RSpec.describe ApprovableBase do
       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