Merge branch '39042-filter-merge-requests-approved-by-user-backend' into 'master'

Filter merge requests approved by user (backend)

See merge request gitlab-org/gitlab!21379

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

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

doc/api/merge_requests.md

@@ -38,7 +38,7 @@ GET /merge_requests?search=foo&in=title
 Parameters:
 
 | Attribute                       | Type           | Required | Description                                                                                                            |
-| ------------------- | --------       | -------- | ---------------------------------------------------------------------------------------------------------------------- |
+| ------------------------------- | -------------- | -------- | ---------------------------------------------------------------------------------------------------------------------- |
 | `state`                         | string         | no       | Return all merge requests or just those that are `opened`, `closed`, `locked`, or `merged`                             |
 | `order_by`                      | string         | no       | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`                                |
 | `sort`                          | string         | no       | Return requests sorted in `asc` or `desc` order. Default is `desc`                                                     |
@@ -54,6 +54,7 @@ Parameters:
 | `author_id`                     | integer        | no       | Returns merge requests created by the given user `id`. Combine with `scope=all` or `scope=assigned_to_me`              |
 | `assignee_id`                   | integer        | no       | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. |
 | `approver_ids` **(STARTER)**    | integer array  | no       | Returns merge requests which have specified all the users with the given `id`s as individual approvers. `None` returns merge requests without approvers. `Any` returns merge requests with an approver. |
+| `approved_by_ids` **(STARTER)** | integer array  | no       | Returns merge requests which have been approved by all the users with the given `id`s (Max: 5). `None` returns merge requests with no approvals. `Any` returns merge requests with an approval. |
 | `my_reaction_emoji`             | string         | no       | Return merge requests reacted by the authenticated user by the given `emoji`. `None` returns issues not given a reaction. `Any` returns issues given at least one reaction. _([Introduced][ce-14016] in GitLab 10.0)_ |
 | `source_branch`                 | string         | no       | Return merge requests with the given source branch                                                                     |
 | `target_branch`                 | string         | no       | Return merge requests with the given target branch                                                                     |
@@ -211,7 +212,7 @@ will be the same. In the case of a merge request from a fork,
 Parameters:
 
 | Attribute                       | Type           | Required | Description                                                                                                                    |
-| ------------------- | -------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ |
+| ------------------------------- | -------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ |
 | `id`                            | integer        | yes      | The ID of a project                                                                                                            |
 | `iids[]`                        | integer array  | no       | Return the request having the given `iid`                                                                                      |
 | `state`                         | string         | no       | Return all merge requests or just those that are `opened`, `closed`, `locked`, or `merged`                                     |
@@ -229,6 +230,7 @@ Parameters:
 | `author_id`                     | integer        | no       | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_                                 |
 | `assignee_id`                   | integer        | no       | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. _([Introduced][ce-13060] in GitLab 9.5)_ |
 | `approver_ids` **(STARTER)**    | integer array  | no       | Returns merge requests which have specified all the users with the given `id`s as individual approvers. `None` returns merge requests without approvers. `Any` returns merge requests with an approver. |
+| `approved_by_ids` **(STARTER)** | integer array  | no       | Returns merge requests which have been approved by all the users with the given `id`s (Max: 5). `None` returns merge requests with no approvals. `Any` returns merge requests with an approval. |
 | `my_reaction_emoji`             | string         | no       | Return merge requests reacted by the authenticated user by the given `emoji`. `None` returns issues not given a reaction. `Any` returns issues given at least one reaction. _([Introduced][ce-14016] in GitLab 10.0)_ |
 | `source_branch`                 | string         | no       | Return merge requests with the given source branch                                                                             |
 | `target_branch`                 | string         | no       | Return merge requests with the given target branch                                                                             |
@@ -373,7 +375,7 @@ GET /groups/:id/merge_requests?my_reaction_emoji=star
 Parameters:
 
 | Attribute                       | Type           | Required | Description                                                                                                                    |
-| ------------------- | -------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ |
+| ------------------------------- | -------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ |
 | `id`                            | integer        | yes      | The ID of a group                                                                                                              |
 | `state`                         | string         | no       | Return all merge requests or just those that are `opened`, `closed`, `locked`, or `merged`                                     |
 | `order_by`                      | string         | no       | Return merge requests ordered by `created_at` or `updated_at` fields. Default is `created_at`                                  |
@@ -390,6 +392,7 @@ Parameters:
 | `author_id`                     | integer        | no       | Returns merge requests created by the given user `id` _([Introduced][ce-13060] in GitLab 9.5)_                                 |
 | `assignee_id`                   | integer        | no       | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. _([Introduced][ce-13060] in GitLab 9.5)_ |
 | `approver_ids` **(STARTER)**    | integer array  | no       | Returns merge requests which have specified all the users with the given `id`s as individual approvers. `None` returns merge requests without approvers. `Any` returns merge requests with an approver. |
+| `approved_by_ids` **(STARTER)** | integer array  | no       | Returns merge requests which have been approved by all the users with the given `id`s (Max: 5). `None` returns merge requests with no approvals. `Any` returns merge requests with an approval. |
 | `my_reaction_emoji`             | string         | no       | Return merge requests reacted by the authenticated user by the given `emoji`. `None` returns issues not given a reaction. `Any` returns issues given at least one reaction. _([Introduced][ce-14016] in GitLab 10.0)_ |
 | `source_branch`                 | string         | no       | Return merge requests with the given source branch                                                                             |
 | `target_branch`                 | string         | no       | Return merge requests with the given target branch                                                                             |

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 MergeRequest collections by approvers
+  class ByApprovalsFinder
+    attr_reader :usernames, :ids
+
+    # We apply a limitation to the amount of elements that can be part of the filter condition
+    MAX_FILTER_ELEMENTS = 5
+
+    # Initialize the finder
+    #
+    # @param [Array<String>] usernames
+    # @param [Array<Integers>] ids
+    def initialize(usernames, ids)
+      # rubocop:disable CodeReuse/ActiveRecord
+      @usernames = Array(usernames).map(&:to_s).uniq.take(MAX_FILTER_ELEMENTS)
+      @ids = Array(ids).uniq.take(MAX_FILTER_ELEMENTS)
+      # rubocop:enable CodeReuse/ActiveRecord
+    end
+
+    # Filter MergeRequest collections by approvers
+    #
+    # @param [ActiveRecord::Relation] items the activerecord relation
+    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" ?
+    #
+    # @return [Boolean] whether special condition "None" is being used
+    def by_no_approvals?
+      includes_special_label?(IssuableFinder::FILTER_NONE)
+    end
+
+    # Is param using special condition: "Any" ?
+    #
+    # @return [Boolean] whether special condition "Any"" is being used
+    def by_any_approvals?
+      includes_special_label?(IssuableFinder::FILTER_ANY)
+    end
+
+    # Check if we have the special label in ids or usernames field
+    #
+    # @param [String] label the special label
+    # @return [Boolean] whether ids or usernames includes the special label
+    def includes_special_label?(label)
+      ids.first.to_s.downcase == label || usernames.map(&:downcase).include?(label)
+    end
+
+    # Merge Requests without any approval
+    #
+    # @param [ActiveRecord::Relation] items
+    def without_approvals(items)
+      items.without_approvals
+    end
+
+    # Merge Requests with any number of approvals
+    #
+    # @param [ActiveRecord::Relation] items the activerecord relation
+    def with_any_approvals(items)
+      items.select_from_union([
+        items.with_approvals
+      ])
+    end
+
+    # Merge Requests approved by given usernames
+    #
+    # @param [ActiveRecord::Relation] items the activerecord relation
+    def find_approved_by_names(items)
+      items.approved_by_users_with_usernames(*usernames)
+    end
+
+    # Merge Requests approved by given user IDs
+    #
+    # @param [ActiveRecord::Relation] items the activerecord relation
+    def find_approved_by_ids(items)
+      items.approved_by_users_with_ids(*ids)
+    end
+  end
+end

ee/app/models/approval.rb

@@ -6,4 +6,6 @@ class Approval < ApplicationRecord
 
   validates :merge_request_id, presence: true
   validates :user_id, presence: true, uniqueness: { scope: [:merge_request_id] }
+
+  scope :with_user, -> { joins(:user) }
 end

ee/app/models/ee/merge_request.rb

@@ -41,6 +41,23 @@ module EE
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
       delegate :merge_requests_author_approval?, to: :target_project, allow_nil: true
 
+      scope :without_approvals, -> { left_outer_joins(:approvals).where(approvals: { id: nil }) }
+      scope :with_approvals, -> { joins(:approvals) }
+      scope :approved_by_users_with_ids, -> (*user_ids) do
+        with_approvals
+          .merge(Approval.with_user)
+          .where(users: { id: user_ids })
+          .group(:id)
+          .having("COUNT(users.id) = ?", user_ids.size)
+      end
+      scope :approved_by_users_with_usernames, -> (*usernames) do
+        with_approvals
+          .merge(Approval.with_user)
+          .where(users: { username: usernames })
+          .group(:id)
+          .having("COUNT(users.id) = ?", usernames.size)
+      end
+
       participant :participant_approvers
 
       accepts_nested_attributes_for :approval_rules, allow_destroy: true

ee/lib/ee/api/merge_requests.rb

@@ -18,6 +18,8 @@ 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'
+            optional :approved_by_ids, types: [String, Array], array_none_any: true,
+                     desc: 'Return merge requests which have been approved by the specified 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) }
+  let(:third_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 match_array(expected_result)
+      expect(merge_requests(names: ['None'])).to match_array(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 match_array(expected_result)
+      expect(merge_requests(names: ['Any'])).to match_array(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 match_array(expected_result)
+      expect(merge_requests(names: [first_user.username])).to match_array(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
+
+    context 'limiting max conditional elements' do
+      it 'returns merge requests approved by both users, considering limit of 2 being defined' do
+        stub_const('MergeRequests::ByApprovalsFinder::MAX_FILTER_ELEMENTS', 2)
+
+        expected_result = [merge_request_with_both_approvals]
+
+        expect(merge_requests(ids: [first_user.id, second_user.id, third_user.id])).to match_array(expected_result)
+        expect(merge_requests(names: [first_user.username, second_user.username, third_user.username])).to match_array(expected_result)
+      end
+    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
 
+  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(: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) }
   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