Filter merge requests by approvals

This adds the following two additional filters when searching for merge
requests:

* approved_by_ids: [int, 'None', 'Any']
* approved_by_usernames: [String, 'None', 'Any']

changelogs/unreleased/39042-filter-merge-requests-approved-by-user-backend.yml

+---
+title: Filter merge requests by approvals (API)
+merge_request: 21379
+author:
+type: added

ee/app/finders/ee/merge_requests_finder.rb

@@ -8,27 +8,35 @@ module EE
     override :filter_items
     def filter_items(items)
       items = super(items)
-
-      by_approvers(items)
+      items = by_approvers(items)
+      by_approvals(items)
     end
 
+    # Filter by merge requests approval list that contains specified user directly or as part of group membership
     def by_approvers(items)
       ::MergeRequests::ByApproversFinder
         .new(params[:approver_usernames], params[:approver_ids])
         .execute(items)
     end
 
+    # Filter by merge requests that had been approved by specific users
+    def by_approvals(items)
+      ::MergeRequests::ByApprovalsFinder
+        .new(params[:approved_by_usernames], params[:approved_by_ids])
+        .execute(items)
+    end
+
     class_methods do
       extend ::Gitlab::Utils::Override
 
       override :scalar_params
       def scalar_params
-        @scalar_params ||= super + [:approver_ids]
+        @scalar_params ||= super + [:approver_ids, :approved_by_ids]
       end
 
       override :array_params
       def array_params
-        @array_params ||= super.merge(approver_usernames: [])
+        @array_params ||= super.merge(approver_usernames: [], approved_by_usernames: [])
       end
     end
   end

ee/app/finders/merge_requests/by_approvals_finder.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  # Used to filter MergeRequests collections by approvers
+  class ByApprovalsFinder
+    attr_reader :usernames, :ids
+
+    def initialize(usernames, ids)
+      @usernames = usernames.to_a.map(&:to_s)
+      @ids = ids
+    end
+
+    def execute(items)
+      if by_no_approvals?
+        without_approvals(items)
+      elsif by_any_approvals?
+        with_any_approvals(items)
+      elsif ids.present?
+        find_approved_by_ids(items)
+      elsif usernames.present?
+        find_approved_by_names(items)
+      else
+        items
+      end
+    end
+
+    private
+
+    # Is param using special condition: "None" ?
+    def by_no_approvals?
+      includes_custom_label?(IssuableFinder::FILTER_NONE)
+    end
+
+    # Is param using special condition: "Any" ?
+    def by_any_approvals?
+      includes_custom_label?(IssuableFinder::FILTER_ANY)
+    end
+
+    def includes_custom_label?(label)
+      ids.to_s.downcase == label || usernames.map(&:downcase).include?(label)
+    end
+
+    # Merge Requests without any approval
+    def without_approvals(items)
+      items
+        .left_outer_joins(:approvals)
+        .joins('LEFT OUTER JOIN approvals ON approvals.merge_request_id = merge_requests.id')
+        .where(approvals: { id: nil })
+    end
+
+    # Merge Requests with any number of approvals
+    def with_any_approvals(items)
+      items.select_from_union([
+        items.joins(:approvals),
+        items.joins('INNER JOIN approvals ON approvals.merge_request_id = merge_requests.id')
+      ])
+    end
+
+    # Merge Requests approved by given usernames
+    def find_approved_by_names(items)
+      find_approved_by_query(items, :username, usernames)
+    end
+
+    # Merge Requests approved by given user IDs
+    def find_approved_by_ids(items)
+      find_approved_by_query(items, :id, ids)
+    end
+
+    def find_approved_by_query(items, field, values)
+      items
+        .joins(:approvals)
+        .joins(approvals: [:user])
+        .where(users: { field => values })
+        .group('merge_requests.id')
+        .having("COUNT(users.id) = ?", values.size)
+    end
+  end
+end

ee/lib/ee/api/merge_requests.rb

@@ -17,7 +17,9 @@ module EE
 
           params :optional_merge_requests_search_params do
             optional :approver_ids, types: [String, Array], array_none_any: true,
-                                    desc: 'Return merge requests which have specified the users with the given IDs as an individual approver'
+                     desc: 'Return merge requests which have specified the users with the given IDs as an individual approver'
+            optional :approved_by_ids, types: [String, Array], array_none_any: true,
+                     desc: 'Return merge requests which have been approved by the specified the users with the given IDs'
           end
         end
       end

ee/spec/finders/merge_requests/by_approvals_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::ByApprovalsFinder do
+  set(:first_user) { create(:user) }
+  set(:second_user) { create(:user) }
+
+  set(:merge_request_without_approvals) { create(:merge_request) }
+  set(:merge_request_with_first_user_approval) do
+    create(:merge_request).tap do |mr|
+      create(:approval, merge_request: mr, user: first_user)
+    end
+  end
+  set(:merge_request_with_both_approvals) do
+    create(:merge_request).tap do |mr|
+      create(:approval, merge_request: mr, user: first_user)
+      create(:approval, merge_request: mr, user: second_user)
+    end
+  end
+
+  def merge_requests(ids: nil, names: [])
+    described_class.new(names, ids).execute(MergeRequest.all)
+  end
+
+  context 'filter by no approvals' do
+    it 'returns merge requests without approvals' do
+      expected_result = [merge_request_without_approvals]
+
+      expect(merge_requests(ids: 'None')).to eq(expected_result)
+      expect(merge_requests(names: ['None'])).to eq(expected_result)
+    end
+  end
+
+  context 'filter by any approvals' do
+    it 'returns merge requests approved by at least one user' do
+      expected_result = [merge_request_with_first_user_approval, merge_request_with_both_approvals]
+
+      expect(merge_requests(ids: 'Any')).to eq(expected_result)
+      expect(merge_requests(names: ['Any'])).to eq(expected_result)
+    end
+  end
+
+  context 'filter by specific user approval' do
+    it 'returns merge requests approved by specific user' do
+      expected_result = [merge_request_with_first_user_approval, merge_request_with_both_approvals]
+
+      expect(merge_requests(ids: [first_user.id])).to eq(expected_result)
+      expect(merge_requests(names: [first_user.username])).to eq(expected_result)
+    end
+  end
+
+  context 'filter by multiple user approval' do
+    it 'returns merge requests approved by both users' do
+      expected_result = [merge_request_with_both_approvals]
+
+      expect(merge_requests(ids: [first_user.id, second_user.id])).to match_array(expected_result)
+      expect(merge_requests(names: [first_user.username, second_user.username])).to match_array(expected_result)
+    end
+  end
+
+  context 'with empty params' do
+    it 'returns all merge requests' do
+      expected_result = [merge_request_without_approvals, merge_request_with_first_user_approval, merge_request_with_both_approvals]
+
+      expect(merge_requests(ids: [])).to match_array(expected_result)
+      expect(merge_requests(names: [])).to match_array(expected_result)
+    end
+  end
+end

ee/spec/requests/api/merge_requests_spec.rb

@@ -5,17 +5,16 @@ require "spec_helper"
 describe API::MergeRequests do
   include ProjectForksHelper
 
-  let(:base_time)   { Time.now }
-  let(:user)        { create(:user) }
-  let(:user2)       { create(:user) }
-  let!(:project)    { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
-  let(:milestone)   { create(:milestone, title: '1.0.0', project: project) }
-  let(:milestone1) { create(:milestone, title: '0.9', project: project) }
+  set(:user)        { create(:user) }
+  set(:user2)       { create(:user) }
+  set(:project)     { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
+  set(:milestone)   { create(:milestone, title: '1.0.0', project: project) }
+  set(:milestone1)  { create(:milestone, title: '0.9', project: project) }
+  set(:label)       { create(:label, title: 'label', color: '#FFAABB', project: project) }
+  set(:label2)      { create(:label, title: 'a-test', color: '#FFFFFF', project: project) }
+
+  let(:base_time) { Time.now }
   let!(:merge_request) { create(:merge_request, :simple, milestone: milestone1, author: user, assignees: [user, user2], source_project: project, target_project: project, title: "Test", created_at: base_time) }
-  let!(:label) do
-    create(:label, title: 'label', color: '#FFAABB', project: project)
-  end
-  let!(:label2) { create(:label, title: 'a-test', color: '#FFFFFF', project: project) }
 
   before do
     project.add_reporter(user)
@@ -215,12 +214,8 @@ describe API::MergeRequests do
   context 'when authenticated' do
     def expect_response_contain_exactly(*items)
       expect(response).to have_gitlab_http_status(200)
-      expect(json_response.length).to eq(items.size)
       expect(json_response.map { |element| element['id'] }).to contain_exactly(*items.map(&:id))
-    end
-
-    let!(:merge_request_with_approver) do
-      create(:merge_request_with_approver, :simple, author: user, source_project: project, target_project: project, source_branch: 'other-branch')
+      expect(json_response.length).to eq(items.size)
     end
 
     context 'filter merge requests by assignee ID' do
@@ -236,6 +231,10 @@ describe API::MergeRequests do
     end
 
     context 'filter merge requests by approver IDs' do
+      let!(:merge_request_with_approver) do
+        create(:merge_request_with_approver, :simple, author: user, source_project: project, target_project: project, source_branch: 'other-branch')
+      end
+
       before do
         get api('/merge_requests', user), params: { approver_ids: approvers_param, scope: :all }
       end
@@ -273,5 +272,50 @@ describe API::MergeRequests do
         end
       end
     end
+
+    context 'filter merge requests by approval IDs' do
+      let!(:merge_request_with_approval) do
+        create(:merge_request, author: user, source_project: project, target_project: project, source_branch: 'other-branch').tap do |mr|
+          create(:approval, merge_request: mr, user: user2)
+        end
+      end
+
+      before do
+        get api('/merge_requests', user), params: { approved_by_ids: approvals_param, scope: :all }
+      end
+
+      context 'with specified approved_by id' do
+        let(:approvals_param) { [user2.id] }
+
+        it 'returns an array of merge requests which have specified the user as an approver' do
+          expect_response_contain_exactly(merge_request_with_approval)
+        end
+      end
+
+      context 'with specified None as a param' do
+        let(:approvals_param) { 'None' }
+
+        it 'returns an array of merge requests with no approvers' do
+          expect_response_contain_exactly(merge_request)
+        end
+      end
+
+      context 'with specified Any as a param' do
+        let(:approvals_param) { 'Any' }
+
+        it 'returns an array of merge requests with any approver' do
+          expect_response_contain_exactly(merge_request_with_approval)
+        end
+      end
+
+      context 'with any other string as a param' do
+        let(:approvals_param) { 'any-other-string' }
+
+        it 'returns a validation error' do
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['error']).to eq("approved_by_ids should be an array, 'None' or 'Any'")
+        end
+      end
+    end
   end
 end