Merge branch 'revert-8b5e1ae9' into 'master'

Add reviewer_id and reviewer_username filters to MergeRequest

See merge request gitlab-org/gitlab!47355

app/controllers/concerns/issuable_collections.rb

@@ -150,7 +150,7 @@ module IssuableCollections
                                   common_attributes + [:project, project: :namespace]
                                 when 'MergeRequest'
                                   common_attributes + [
-                                    :target_project, :latest_merge_request_diff, :approvals, :approved_by_users,
+                                    :target_project, :latest_merge_request_diff, :approvals, :approved_by_users, :reviewers,
                                     source_project: :route, head_pipeline: :project, target_project: :namespace
                                   ]
                                 end

app/finders/merge_requests_finder.rb

@@ -41,6 +41,8 @@ class MergeRequestsFinder < IssuableFinder
       :environment,
       :merged_after,
       :merged_before,
+      :reviewer_id,
+      :reviewer_username,
       :target_branch,
       :wip
     ]
@@ -54,6 +56,10 @@ class MergeRequestsFinder < IssuableFinder
     MergeRequest
   end
 
+  def params_class
+    MergeRequestsFinder::Params
+  end
+
   def filter_items(_items)
     items = by_commit(super)
     items = by_source_branch(items)
@@ -62,12 +68,14 @@ class MergeRequestsFinder < IssuableFinder
     items = by_merged_at(items)
     items = by_approvals(items)
     items = by_deployments(items)
+    items = by_reviewer(items)
 
     by_source_project_id(items)
   end
 
   def filter_negated_items(items)
     items = super(items)
+    items = by_negated_reviewer(items)
     by_negated_target_branch(items)
   end
 
@@ -186,6 +194,30 @@ class MergeRequestsFinder < IssuableFinder
 
     items.where_exists(deploys)
   end
+
+  def by_reviewer(items)
+    return items unless params.reviewer_id? || params.reviewer_username?
+
+    if params.filter_by_no_reviewer?
+      items.no_review_requested
+    elsif params.filter_by_any_reviewer?
+      items.review_requested
+    elsif params.reviewer
+      items.review_requested_to(params.reviewer)
+    else # reviewer not found
+      items.none
+    end
+  end
+
+  def by_negated_reviewer(items)
+    return items unless not_params.reviewer_id? || not_params.reviewer_username?
+
+    if not_params.reviewer.present?
+      items.no_review_requested_to(not_params.reviewer)
+    else # reviewer not found
+      items.none
+    end
+  end
 end
 
 MergeRequestsFinder.prepend_if_ee('EE::MergeRequestsFinder')

app/finders/merge_requests_finder/params.rb

+# frozen_string_literal: true
+
+class MergeRequestsFinder
+  class Params < IssuableFinder::Params
+    def filter_by_no_reviewer?
+      params[:reviewer_id].to_s.downcase == FILTER_NONE
+    end
+
+    def filter_by_any_reviewer?
+      params[:reviewer_id].to_s.downcase == FILTER_ANY
+    end
+
+    def reviewer
+      strong_memoize(:reviewer) do
+        if reviewer_id?
+          User.find_by_id(params[:reviewer_id])
+        elsif reviewer_username?
+          User.find_by_username(params[:reviewer_username])
+        else
+          nil
+        end
+      end
+    end
+  end
+end

app/models/merge_request.rb

@@ -314,6 +314,31 @@ class MergeRequest < ApplicationRecord
 
   scope :with_jira_issue_keys, -> { where('title ~ :regex OR merge_requests.description ~ :regex', regex: Gitlab::Regex.jira_issue_key_regex.source) }
 
+  scope :review_requested, -> do
+    where(reviewers_subquery.exists)
+  end
+
+  scope :no_review_requested, -> do
+    where(reviewers_subquery.exists.not)
+  end
+
+  scope :review_requested_to, ->(user) do
+    where(
+      reviewers_subquery
+        .where(Arel::Table.new("#{to_ability_name}_reviewers")[:user_id].eq(user))
+        .exists
+    )
+  end
+
+  scope :no_review_requested_to, ->(user) do
+    where(
+      reviewers_subquery
+        .where(Arel::Table.new("#{to_ability_name}_reviewers")[:user_id].eq(user))
+        .exists
+        .not
+    )
+  end
+
   after_save :keep_around_commit, unless: :importing?
 
   alias_attribute :project, :target_project
@@ -361,6 +386,12 @@ class MergeRequest < ApplicationRecord
     end
   end
 
+  def self.reviewers_subquery
+    MergeRequestReviewer.arel_table
+      .project('true')
+      .where(Arel::Nodes::SqlLiteral.new("#{to_ability_name}_id = #{to_ability_name}s.id"))
+  end
+
   def rebase_in_progress?
     rebase_jid.present? && Gitlab::SidekiqStatus.running?(rebase_jid)
   end

app/models/merge_request_reviewer.rb

@@ -2,5 +2,5 @@
 
 class MergeRequestReviewer < ApplicationRecord
   belongs_to :merge_request
-  belongs_to :reviewer, class_name: "User", foreign_key: :user_id, inverse_of: :merge_request_assignees
+  belongs_to :reviewer, class_name: 'User', foreign_key: :user_id, inverse_of: :merge_request_reviewers
 end

app/models/user.rb

@@ -166,6 +166,7 @@ class User < ApplicationRecord
 
   has_many :issue_assignees, inverse_of: :assignee
   has_many :merge_request_assignees, inverse_of: :assignee
+  has_many :merge_request_reviewers, inverse_of: :reviewer
   has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue
   has_many :assigned_merge_requests, class_name: "MergeRequest", through: :merge_request_assignees, source: :merge_request
 

ee/spec/requests/api/merge_requests_spec.rb

@@ -267,12 +267,20 @@ RSpec.describe API::MergeRequests do
     end
 
     context 'filter merge requests by approval IDs' do
-      let!(:merge_request_with_approval) do
+      let_it_be(:user3) { create(:user) }
+      let_it_be(: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
 
+      let_it_be(:merge_request_with_multiple_approvals) do
+        create(:merge_request, author: user, source_project: project, target_project: project, source_branch: 'another-branch').tap do |mr|
+          create(:approval, merge_request: mr, user: user2)
+          create(:approval, merge_request: mr, user: user3)
+        end
+      end
+
       before do
         get api('/merge_requests', user), params: { approved_by_ids: approvals_param, scope: :all }
       end
@@ -281,7 +289,25 @@ RSpec.describe API::MergeRequests 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.id)
+          expect_response_contain_exactly(merge_request_with_approval.id, merge_request_with_multiple_approvals.id)
+        end
+      end
+
+      context 'with multiple specified approved_by ids' do
+        context 'when approved by all users' do
+          let(:approvals_param) { [user2.id, user3.id] }
+
+          it 'returns an array of merge requests which have specified the user as an approver' do
+            expect_response_contain_exactly(merge_request_with_multiple_approvals.id)
+          end
+        end
+
+        context 'when not approved by all users' do
+          let(:approvals_param) { [user.id, user2.id] }
+
+          it 'does not return any merge request' do
+            expect_empty_array_response
+          end
         end
       end
 
@@ -297,7 +323,7 @@ RSpec.describe API::MergeRequests do
         let(:approvals_param) { 'Any' }
 
         it 'returns an array of merge requests with any approver' do
-          expect_response_contain_exactly(merge_request_with_approval.id)
+          expect_response_contain_exactly(merge_request_with_approval.id, merge_request_with_multiple_approvals.id)
         end
       end
 

spec/features/dashboard/merge_requests_spec.rb

@@ -52,20 +52,29 @@ RSpec.describe 'Dashboard Merge Requests' do
   end
 
   context 'merge requests exist' do
+    let_it_be(:author_user) { create(:user) }
     let(:label) { create(:label) }
 
     let!(:assigned_merge_request) do
       create(:merge_request,
         assignees: [current_user],
         source_project: project,
-        author: create(:user))
+        author: author_user)
+    end
+
+    let!(:review_requested_merge_request) do
+      create(:merge_request,
+        reviewers: [current_user],
+        source_branch: 'review',
+        source_project: project,
+        author: author_user)
     end
 
     let!(:assigned_merge_request_from_fork) do
       create(:merge_request,
               source_branch: 'markdown', assignees: [current_user],
               target_project: public_project, source_project: forked_project,
-              author: create(:user))
+              author: author_user)
     end
 
     let!(:authored_merge_request) do
@@ -94,7 +103,7 @@ RSpec.describe 'Dashboard Merge Requests' do
       create(:merge_request,
               source_branch: 'fix',
               source_project: project,
-              author: create(:user))
+              author: author_user)
     end
 
     before do
@@ -111,6 +120,10 @@ RSpec.describe 'Dashboard Merge Requests' do
       expect(page).not_to have_content(labeled_merge_request.title)
     end
 
+    it 'does not show review requested merge requests' do
+      expect(page).not_to have_content(review_requested_merge_request.title)
+    end
+
     it 'shows authored merge requests', :js do
       reset_filters
       input_filtered_search("author:=#{current_user.to_reference}")

spec/finders/merge_requests_finder_spec.rb

@@ -333,6 +333,8 @@ RSpec.describe MergeRequestsFinder do
       end
 
       context 'assignee filtering' do
+        let_it_be(:user3) { create(:user) }
+
         let(:issuables) { described_class.new(user, params).execute }
 
         it_behaves_like 'assignee ID filter' do
@@ -351,7 +353,6 @@ RSpec.describe MergeRequestsFinder do
             merge_request3.assignees = [user2, user3]
           end
 
-          let_it_be(:user3) { create(:user) }
           let(:params) { { assignee_username: [user2.username, user3.username] } }
           let(:expected_issuables) { [merge_request3] }
         end
@@ -366,38 +367,95 @@ RSpec.describe MergeRequestsFinder do
         end
 
         it_behaves_like 'no assignee filter' do
-          let_it_be(:user3) { create(:user) }
           let(:expected_issuables) { [merge_request4, merge_request5] }
         end
 
         it_behaves_like 'any assignee filter' do
           let(:expected_issuables) { [merge_request1, merge_request2, merge_request3] }
         end
+      end
 
-        context 'filtering by group milestone' do
-          let(:group_milestone) { create(:milestone, group: group) }
+      context 'reviewer filtering' do
+        subject { described_class.new(user, params).execute }
 
-          before do
-            merge_request1.update!(milestone: group_milestone)
-            merge_request2.update!(milestone: group_milestone)
-          end
+        context 'by reviewer_id' do
+          let(:params) { { reviewer_id: user2.id } }
+          let(:expected_mr) { [merge_request1, merge_request2] }
 
-          it 'returns merge requests assigned to that group milestone' do
-            params = { milestone_title: group_milestone.title }
+          it { is_expected.to contain_exactly(*expected_mr) }
+        end
 
-            merge_requests = described_class.new(user, params).execute
+        context 'by NOT reviewer_id' do
+          let(:params) { { not: { reviewer_id: user2.id } } }
+          let(:expected_mr) { [merge_request3, merge_request4, merge_request5] }
 
-            expect(merge_requests).to contain_exactly(merge_request1, merge_request2)
-          end
+          it { is_expected.to contain_exactly(*expected_mr) }
+        end
 
-          context 'using NOT' do
-            let(:params) { { not: { milestone_title: group_milestone.title } } }
+        context 'by reviewer_username' do
+          let(:params) { { reviewer_username: user2.username } }
+          let(:expected_mr) { [merge_request1, merge_request2] }
 
-            it 'returns MRs not assigned to that group milestone' do
-              merge_requests = described_class.new(user, params).execute
+          it { is_expected.to contain_exactly(*expected_mr) }
+        end
 
-              expect(merge_requests).to contain_exactly(merge_request3, merge_request4, merge_request5)
-            end
+        context 'by NOT reviewer_username' do
+          let(:params) { { not: { reviewer_username: user2.username } } }
+          let(:expected_mr) { [merge_request3, merge_request4, merge_request5] }
+
+          it { is_expected.to contain_exactly(*expected_mr) }
+        end
+
+        context 'by reviewer_id=None' do
+          let(:params) { { reviewer_id: 'None' } }
+          let(:expected_mr) { [merge_request4, merge_request5] }
+
+          it { is_expected.to contain_exactly(*expected_mr) }
+        end
+
+        context 'by reviewer_id=Any' do
+          let(:params) { { reviewer_id: 'Any' } }
+          let(:expected_mr) { [merge_request1, merge_request2, merge_request3] }
+
+          it { is_expected.to contain_exactly(*expected_mr) }
+        end
+
+        context 'by reviewer_id with unknown user' do
+          let(:params) { { reviewer_id: 99999 } }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'by NOT reviewer_id with unknown user' do
+          let(:params) { { not: { reviewer_id: 99999 } } }
+
+          it { is_expected.to be_empty }
+        end
+      end
+
+      context 'filtering by group milestone' do
+        let(:group_milestone) { create(:milestone, group: group) }
+
+        before do
+          merge_request1.update!(milestone: group_milestone)
+          merge_request2.update!(milestone: group_milestone)
+        end
+
+        it 'returns merge requests assigned to that group milestone' do
+          params = { milestone_title: group_milestone.title }
+
+          merge_requests = described_class.new(user, params).execute
+
+          expect(merge_requests).to contain_exactly(merge_request1, merge_request2)
+        end
+
+        context 'using NOT' do
+          let(:params) { { not: { milestone_title: group_milestone.title } } }
+
+          it 'returns MRs not assigned to that group milestone' do
+            merge_requests = described_class.new(user, params).execute
+
+            expect(merge_requests).to contain_exactly(merge_request3, merge_request4, merge_request5)
           end
         end
       end
@@ -563,6 +621,28 @@ RSpec.describe MergeRequestsFinder do
           expect(mrs).to eq([mr2])
         end
       end
+
+      it 'does not raise any exception with complex filters' do
+        # available filters from MergeRequest dashboard UI
+        params = {
+          project_id: project1.id,
+          scope: 'authored',
+          state: 'opened',
+          author_username: user.username,
+          assignee_username: user.username,
+          reviewer_username: user.username,
+          approver_usernames: [user.username],
+          approved_by_usernames: [user.username],
+          milestone_title: 'none',
+          release_tag: 'none',
+          label_names: 'none',
+          my_reaction_emoji: 'none',
+          draft: 'no'
+        }
+
+        merge_requests = described_class.new(user, params).execute
+        expect { merge_requests.load }.not_to raise_error
+      end
     end
 
     describe '#row_count', :request_store do

spec/models/merge_request_reviewer_spec.rb

@@ -9,6 +9,6 @@ RSpec.describe MergeRequestReviewer do
 
   describe 'associations' do
     it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') }
-    it { is_expected.to belong_to(:reviewer).class_name('User') }
+    it { is_expected.to belong_to(:reviewer).class_name('User').inverse_of(:merge_request_reviewers) }
   end
 end

spec/models/merge_request_spec.rb

@@ -92,6 +92,39 @@ RSpec.describe MergeRequest, factory_default: :keep do
     it { is_expected.not_to include(mr_without_jira_reference) }
   end
 
+  context 'scopes' do
+    let_it_be(:user1) { create(:user) }
+    let_it_be(:user2) { create(:user) }
+
+    let_it_be(:merge_request1) { create(:merge_request, :unique_branches, reviewers: [user1])}
+    let_it_be(:merge_request2) { create(:merge_request, :unique_branches, reviewers: [user2])}
+    let_it_be(:merge_request3) { create(:merge_request, :unique_branches, reviewers: [])}
+
+    describe '.review_requested' do
+      it 'returns MRs that has any review requests' do
+        expect(described_class.review_requested).to eq([merge_request1, merge_request2])
+      end
+    end
+
+    describe '.no_review_requested' do
+      it 'returns MRs that has no review requests' do
+        expect(described_class.no_review_requested).to eq([merge_request3])
+      end
+    end
+
+    describe '.review_requested_to' do
+      it 'returns MRs that the user has been requested to review' do
+        expect(described_class.review_requested_to(user1)).to eq([merge_request1])
+      end
+    end
+
+    describe '.no_review_requested_to' do
+      it 'returns MRs that the user has been requested to review' do
+        expect(described_class.no_review_requested_to(user1)).to eq([merge_request2, merge_request3])
+      end
+    end
+  end
+
   describe '#squash_in_progress?' do
     let(:repo_path) do
       Gitlab::GitalyClient::StorageSettings.allow_disk_access do

spec/models/user_spec.rb

@@ -99,6 +99,8 @@ RSpec.describe User do
     it { is_expected.to have_many(:releases).dependent(:nullify) }
     it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:user) }
     it { is_expected.to have_many(:reviews).inverse_of(:author) }
+    it { is_expected.to have_many(:merge_request_assignees).inverse_of(:assignee) }
+    it { is_expected.to have_many(:merge_request_reviewers).inverse_of(:reviewer) }
 
     describe "#user_detail" do
       it 'does not persist `user_detail` by default' do

spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb

@@ -54,19 +54,19 @@ RSpec.shared_context 'MergeRequestsFinder multiple projects with merge requests
   let!(:label2) { create(:label, project: project1) }
 
   let!(:merge_request1) do
-    create(:merge_request, assignees: [user], author: user,
+    create(:merge_request, assignees: [user], author: user, reviewers: [user2],
            source_project: project2, target_project: project1,
            target_branch: 'merged-target')
   end
 
   let!(:merge_request2) do
-    create(:merge_request, :conflict, assignees: [user], author: user,
+    create(:merge_request, :conflict, assignees: [user], author: user, reviewers: [user2],
            source_project: project2, target_project: project1,
            state: 'closed')
   end
 
   let!(:merge_request3) do
-    create(:merge_request, :simple, author: user, assignees: [user2],
+    create(:merge_request, :simple, author: user, assignees: [user2], reviewers: [user],
            source_project: project2, target_project: project2,
            state: 'locked',
            title: 'thing WIP thing')