Merge branch 'revert-253da9ae' into 'master'

Redo reverted Assignee filter change for MergeRequest

See merge request gitlab-org/gitlab!45738

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/issuable_finder.rb

@@ -406,7 +406,7 @@ class IssuableFinder
     elsif params.filter_by_any_assignee?
       items.assigned
     elsif params.assignee
-      items.assigned_to(params.assignee)
+      items_assigned_to(items, params.assignee)
     elsif params.assignee_id? || params.assignee_username? # assignee not found
       items.none
     else
@@ -414,6 +414,10 @@ class IssuableFinder
     end
   end
 
+  def items_assigned_to(items, user)
+    items.assigned_to(user)
+  end
+
   def by_negated_assignee(items)
     # We want CE users to be able to say "Issues not assigned to either PersonA nor PersonB"
     if not_params.assignees.present?

app/finders/merge_requests_finder.rb

@@ -164,6 +164,13 @@ class MergeRequestsFinder < IssuableFinder
   end
   # rubocop: enable CodeReuse/Finder
 
+  # rubocop: disable CodeReuse/ActiveRecord
+  def items_assigned_to(items, user)
+    assignee_or_reviewer = MergeRequest.from_union([super, items.reviewer_assigned_to(user)])
+    items.where(id: assignee_or_reviewer)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+
   def by_deployments(items)
     env = params[:environment]
     before = params[:deployed_before]

app/models/merge_request.rb

@@ -303,6 +303,19 @@ class MergeRequest < ApplicationRecord
     includes(:metrics)
   end
 
+  scope :reviewer_assigned_to, ->(user) do
+    mr_reviewers_table = MergeRequestReviewer.arel_table
+
+    inner_sql = mr_reviewers_table
+                  .project(Arel::Nodes::True.new)
+                  .where(
+                    mr_reviewers_table[:merge_request_id].eq(MergeRequest.arel_table[:id])
+                      .and(mr_reviewers_table[:user_id].eq(user.id))
+                  ).exists
+
+    where(inner_sql)
+  end
+
   after_save :keep_around_commit, unless: :importing?
 
   alias_attribute :project, :target_project

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 approved by only one user' do
+          let(:approvals_param) { [user.id, user2.id] }
+
+          it 'does not returns 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 'shows review requested merge requests' do
+      expect(page).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,7 +367,6 @@ 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
 
@@ -374,30 +374,54 @@ RSpec.describe MergeRequestsFinder do
           let(:expected_issuables) { [merge_request1, merge_request2, merge_request3] }
         end
 
-        context 'filtering by group milestone' do
-          let(:group_milestone) { create(:milestone, group: group) }
+        context 'with just reviewers' do
+          it_behaves_like 'assignee username filter' do
+            before do
+              merge_request4.reviewers = [user3]
+              merge_request4.assignees = []
+            end
 
-          before do
-            merge_request1.update!(milestone: group_milestone)
-            merge_request2.update!(milestone: group_milestone)
+            let(:params) { { assignee_username: [user3.username] } }
+            let(:expected_issuables) { [merge_request4] }
           end
+        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
+        context 'with an additional reviewer' do
+          it_behaves_like 'assignee username filter' do
+            before do
+              merge_request3.assignees = [user3]
+              merge_request4.reviewers = [user3]
+            end
 
-            expect(merge_requests).to contain_exactly(merge_request1, merge_request2)
+            let(:params) { { assignee_username: [user3.username] } }
+            let(:expected_issuables) { [merge_request3, merge_request4] }
           end
+        end
+      end
 
-          context 'using NOT' do
-            let(:params) { { not: { milestone_title: group_milestone.title } } }
+      context 'filtering by group milestone' do
+        let(:group_milestone) { create(:milestone, group: group) }
 
-            it 'returns MRs not assigned to that group milestone' do
-              merge_requests = described_class.new(user, params).execute
+        before do
+          merge_request1.update!(milestone: group_milestone)
+          merge_request2.update!(milestone: group_milestone)
+        end
 
-              expect(merge_requests).to contain_exactly(merge_request3, merge_request4, merge_request5)
-            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 +587,27 @@ 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,
+          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