Merge branch 'accept-reviewer-ids-on-merge-requests' into 'master'

Add ability to set reviewers on MRs

See merge request gitlab-org/gitlab!40488

app/controllers/projects/merge_requests/application_controller.rb

@@ -43,6 +43,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
       :discussion_locked,
       label_ids: [],
       assignee_ids: [],
+      reviewer_ids: [],
       update_task: [:index, :checked, :line_number, :line_source]
     ]
   end

app/models/concerns/issuable.rb

@@ -177,6 +177,10 @@ module Issuable
       assignees.count > 1
     end
 
+    def allows_reviewers?
+      false
+    end
+
     def supports_time_tracking?
       is_a?(TimeTrackable) && !incident?
     end

app/models/merge_request.rb

@@ -1658,6 +1658,10 @@ class MergeRequest < ApplicationRecord
     end
   end
 
+  def allows_reviewers?
+    Feature.enabled?(:merge_request_reviewers, project)
+  end
+
   private
 
   def with_rebase_lock

app/serializers/merge_request_basic_entity.rb

@@ -9,6 +9,7 @@ class MergeRequestBasicEntity < Grape::Entity
   expose :milestone, using: API::Entities::Milestone
   expose :labels, using: LabelEntity
   expose :assignees, using: API::Entities::UserBasic
+  expose :reviewers, if: -> (m) { m.allows_reviewers? }, using: API::Entities::UserBasic
   expose :task_status, :task_status_short
   expose :lock_version, :lock_version
 end

app/serializers/merge_request_reviewer_entity.rb

+# frozen_string_literal: true
+
+class MergeRequestReviewerEntity < ::API::Entities::UserBasic
+  expose :can_merge do |reviewer, options|
+    options[:merge_request]&.can_be_merged_by?(reviewer)
+  end
+end
+
+MergeRequestReviewerEntity.prepend_if_ee('EE::MergeRequestReviewerEntity')

app/serializers/merge_request_sidebar_extras_entity.rb

@@ -4,4 +4,8 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity
   expose :assignees do |merge_request|
     MergeRequestAssigneeEntity.represent(merge_request.assignees, merge_request: merge_request)
   end
+
+  expose :reviewers, if: -> (m) { m.allows_reviewers? } do |merge_request|
+    MergeRequestReviewerEntity.represent(merge_request.reviewers, merge_request: merge_request)
+  end
 end

app/services/issuable_base_service.rb

@@ -46,7 +46,7 @@ class IssuableBaseService < BaseService
       params[:assignee_ids] = params[:assignee_ids].first(1)
     end
 
-    assignee_ids = params[:assignee_ids].select { |assignee_id| assignee_can_read?(issuable, assignee_id) }
+    assignee_ids = params[:assignee_ids].select { |assignee_id| user_can_read?(issuable, assignee_id) }
 
     if params[:assignee_ids].map(&:to_s) == [IssuableFinder::Params::NONE]
       params[:assignee_ids] = []
@@ -57,15 +57,15 @@ class IssuableBaseService < BaseService
     end
   end
 
-  def assignee_can_read?(issuable, assignee_id)
-    new_assignee = User.find_by_id(assignee_id)
+  def user_can_read?(issuable, user_id)
+    user = User.find_by_id(user_id)
 
-    return false unless new_assignee
+    return false unless user
 
     ability_name = :"read_#{issuable.to_ability_name}"
     resource     = issuable.persisted? ? issuable : project
 
-    can?(new_assignee, ability_name, resource)
+    can?(user, ability_name, resource)
   end
 
   def filter_milestone

app/services/merge_requests/base_service.rb

@@ -97,6 +97,28 @@ module MergeRequests
       unless merge_request.can_allow_collaboration?(current_user)
         params.delete(:allow_collaboration)
       end
+
+      filter_reviewer(merge_request)
+    end
+
+    def filter_reviewer(merge_request)
+      return if params[:reviewer_ids].blank?
+
+      unless can_admin_issuable?(merge_request) && merge_request.allows_reviewers?
+        params.delete(:reviewer_ids)
+
+        return
+      end
+
+      reviewer_ids = params[:reviewer_ids].select { |reviewer_id| user_can_read?(merge_request, reviewer_id) }
+
+      if params[:reviewer_ids].map(&:to_s) == [IssuableFinder::Params::NONE]
+        params[:reviewer_ids] = []
+      elsif reviewer_ids.any?
+        params[:reviewer_ids] = reviewer_ids
+      else
+        params.delete(:reviewer_ids)
+      end
     end
 
     def merge_request_metrics_service(merge_request)

config/feature_flags/development/merge_request_reviewers.yml

+---
+name: merge_request_reviewers
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40488
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/245190
+group: group::source code
+type: development
+default_enabled: false

ee/app/serializers/ee/merge_request_reviewer_entity.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequestReviewerEntity
+    extend ActiveSupport::Concern
+
+    prepended do
+      expose :gitlab_employee?, as: :is_gitlab_employee, if: proc { ::Gitlab.com? && ::Feature.enabled?(:gitlab_employee_badge) }
+    end
+  end
+end

spec/fixtures/api/schemas/entities/merge_request_basic.json

@@ -15,6 +15,13 @@
         "$ref": "../public_api/v4/user/basic.json"
       }
     },
+    "reviewers": {
+      "type": ["array"],
+      "items": {
+        "type": "object",
+        "$ref": "../public_api/v4/user/basic.json"
+      }
+    },
     "milestone": {
       "type": [ "object", "null" ]
     },

spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json

@@ -17,6 +17,10 @@
     "assignees": {
       "type": "array",
       "items": { "$ref": "../public_api/v4/user/basic.json" }
+    },
+    "reviewers": {
+      "type": "array",
+      "items": { "$ref": "../public_api/v4/user/basic.json" }
     }
   },
   "additionalProperties": false

spec/models/issue_spec.rb

@@ -1212,4 +1212,14 @@ RSpec.describe Issue do
       end
     end
   end
+
+  describe '#allows_reviewers?' do
+    it 'returns false as issues do not support reviewers feature' do
+      stub_feature_flags(merge_request_reviewers: true)
+
+      issue = build_stubbed(:issue)
+
+      expect(issue.allows_reviewers?).to be(false)
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -4145,4 +4145,22 @@ RSpec.describe MergeRequest, factory_default: :keep do
         .with_arguments(allow_nil: true)
     end
   end
+
+  describe '#allows_reviewers?' do
+    it 'returns false without merge_request_reviewers feature' do
+      stub_feature_flags(merge_request_reviewers: false)
+
+      merge_request = build_stubbed(:merge_request)
+
+      expect(merge_request.allows_reviewers?).to be(false)
+    end
+
+    it 'returns true with merge_request_reviewers feature' do
+      stub_feature_flags(merge_request_reviewers: true)
+
+      merge_request = build_stubbed(:merge_request)
+
+      expect(merge_request.allows_reviewers?).to be(true)
+    end
+  end
 end

spec/serializers/merge_request_basic_entity_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequestBasicEntity do
-  let(:resource) { build(:merge_request) }
+  let(:resource) { build(:merge_request, params) }
+  let(:params) { {} }
 
   subject do
     described_class.new(resource).as_json
@@ -14,4 +15,28 @@ RSpec.describe MergeRequestBasicEntity do
 
     expect(subject[:merge_status]).to eq 'checking'
   end
+
+  describe '#reviewers' do
+    let(:params) { { reviewers: [reviewer] } }
+    let(:reviewer) { build(:user) }
+
+    context 'when merge_request_reviewers feature is disabled' do
+      it 'does not contain assignees attributes' do
+        stub_feature_flags(merge_request_reviewers: false)
+
+        expect(subject[:reviewers]).to be_nil
+      end
+    end
+
+    context 'when merge_request_reviewers feature is enabled' do
+      it 'contains reviewers attributes' do
+        stub_feature_flags(merge_request_reviewers: true)
+
+        expect(subject[:reviewers].count).to be 1
+        expect(subject[:reviewers].first.keys).to include(
+          :id, :name, :username, :state, :avatar_url, :web_url
+        )
+      end
+    end
+  end
 end

spec/serializers/merge_request_sidebar_extras_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequestSidebarExtrasEntity do
+  let_it_be(:assignee) { build(:user) }
+  let_it_be(:reviewer) { build(:user) }
+  let_it_be(:user) { build(:user) }
+  let_it_be(:project) { create :project, :repository }
+
+  let(:params) do
+    {
+      source_project: project,
+      target_project: project,
+      assignees: [assignee],
+      reviewers: [reviewer]
+    }
+  end
+
+  let(:resource) do
+    build(:merge_request, params)
+  end
+
+  let(:request) { double('request', current_user: user, project: project) }
+
+  let(:entity) { described_class.new(resource, request: request).as_json }
+
+  describe '#assignees' do
+    it 'contains assignees attributes' do
+      expect(entity[:assignees].count).to be 1
+      expect(entity[:assignees].first.keys).to include(
+        :id, :name, :username, :state, :avatar_url, :web_url, :can_merge
+      )
+    end
+  end
+
+  describe '#reviewers' do
+    context 'when merge_request_reviewers feature is disabled' do
+      it 'does not contain reviewers attributes' do
+        stub_feature_flags(merge_request_reviewers: false)
+
+        expect(entity[:reviewers]).to be_nil
+      end
+    end
+
+    context 'when merge_request_reviewers feature is enabled' do
+      it 'contains reviewers attributes' do
+        stub_feature_flags(merge_request_reviewers: true)
+
+        expect(entity[:reviewers].count).to be 1
+        expect(entity[:reviewers].first.keys).to include(
+          :id, :name, :username, :state, :avatar_url, :web_url, :can_merge
+        )
+      end
+    end
+  end
+end

spec/services/merge_requests/create_service_spec.rb

@@ -339,6 +339,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           end
         end
       end
+
+      it_behaves_like 'reviewer_ids filter' do
+        let(:execute) { service.execute }
+      end
     end
 
     it_behaves_like 'issuable record that supports quick actions' do

spec/services/merge_requests/update_service_spec.rb

@@ -161,6 +161,29 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           expect(@merge_request.merge_params["force_remove_source_branch"]).to eq("1")
         end
       end
+
+      it_behaves_like 'reviewer_ids filter' do
+        let(:opts) { {} }
+        let(:execute) { update_merge_request(opts) }
+      end
+
+      context 'with an existing reviewer' do
+        let(:merge_request) do
+          create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id])
+        end
+
+        context 'when merge_request_reviewer feature is enabled' do
+          before do
+            stub_feature_flags(merge_request_reviewer: true)
+          end
+
+          let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } }
+
+          it 'removes reviewers' do
+            expect(update_merge_request(opts).reviewers).to eq []
+          end
+        end
+      end
     end
 
     context 'after_save callback to store_mentions' do

spec/support/shared_examples/services/merge_request_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'reviewer_ids filter' do
+  context 'filter_reviewer' do
+    let(:opts) { super().merge(reviewer_ids_param) }
+
+    context 'without reviewer_ids' do
+      let(:reviewer_ids_param) { {} }
+
+      it 'contains no reviewer_ids' do
+        expect(execute.reviewers).to eq []
+      end
+    end
+
+    context 'with reviewer_ids' do
+      let(:reviewer_ids_param) { { reviewer_ids: [reviewer1.id, reviewer2.id, reviewer3.id] } }
+
+      let(:reviewer1) { create(:user) }
+      let(:reviewer2) { create(:user) }
+      let(:reviewer3) { create(:user) }
+
+      context 'when the current user can admin the merge_request' do
+        context 'when merge_request_reviewer feature is enabled' do
+          before do
+            stub_feature_flags(merge_request_reviewer: true)
+          end
+
+          context 'with reviewers who can read the merge_request' do
+            before do
+              project.add_developer(reviewer1)
+              project.add_developer(reviewer2)
+            end
+
+            it 'contains reviewers who can read the merge_request' do
+              expect(execute.reviewers).to contain_exactly(reviewer1, reviewer2)
+            end
+          end
+        end
+
+        context 'when merge_request_reviewer feature is disabled' do
+          before do
+            stub_feature_flags(merge_request_reviewer: false)
+          end
+
+          it 'contains no reviewers' do
+            expect(execute.reviewers).to eq []
+          end
+        end
+      end
+
+      context 'when the current_user cannot admin the merge_request' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'contains no reviewers' do
+          expect(execute.reviewers).to eq []
+        end
+      end
+    end
+  end
+end