diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 0bb4e0fb5ee35ac54502de147e04cc8aada0312d..921da788ad2616ed18d6aa393e2a0c8603a1590a 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -43,6 +43,7 @@ def merge_request_params_attributes :discussion_locked, label_ids: [], assignee_ids: [], + reviewer_ids: [], update_task: [:index, :checked, :line_number, :line_source] ] end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 78161a3b4462cf240502c639bbe5a029dee0ab4e..69e95fb4e0498f8269b786222f85f56988fcd7f3 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -177,6 +177,10 @@ def has_multiple_assignees? assignees.count > 1 end + def allows_reviewers? + false + end + def supports_time_tracking? is_a?(TimeTrackable) && !incident? end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 88062240fb30122b45d95aa373098a1fffbdf428..7431314b261af61e02b8fc5a103b66fee8126159 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1658,6 +1658,10 @@ def ensure_metrics end end + def allows_reviewers? + Feature.enabled?(:merge_request_reviewers, project) + end + private def with_rebase_lock diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index 82baf4a4a78b0ffcdad7b9a9435a623e4c094c91..ef1177e9967f9c789a2c62e9e86a4042cc5779d8 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/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 diff --git a/app/serializers/merge_request_reviewer_entity.rb b/app/serializers/merge_request_reviewer_entity.rb new file mode 100644 index 0000000000000000000000000000000000000000..fefd116014fbaf795cae2f873973ccfdb883fead --- /dev/null +++ b/app/serializers/merge_request_reviewer_entity.rb @@ -0,0 +1,9 @@ +# 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') diff --git a/app/serializers/merge_request_sidebar_extras_entity.rb b/app/serializers/merge_request_sidebar_extras_entity.rb index 7276509c363bcc286418a02b131fb7d8499f4012..9db8e52abef629727f081aea6bc998477b365347 100644 --- a/app/serializers/merge_request_sidebar_extras_entity.rb +++ b/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 diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 65a73dadc2e7c511dc3ec0b063bff267332645f6..51c161a5e6046f3d7cdf0b26e59db76f8e8c183e 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -46,7 +46,7 @@ def filter_assignee(issuable) 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 @@ def filter_assignee(issuable) 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 diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 3dc9a5cd2276b357d634adfeb43e7a59f93ddc10..abc3f99797dc1427e70302abfe43c9d010e87391 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -97,6 +97,28 @@ def filter_params(merge_request) 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) diff --git a/config/feature_flags/development/merge_request_reviewers.yml b/config/feature_flags/development/merge_request_reviewers.yml new file mode 100644 index 0000000000000000000000000000000000000000..2180662b9df3d5b14de910677a552b7b1ed36985 --- /dev/null +++ b/config/feature_flags/development/merge_request_reviewers.yml @@ -0,0 +1,7 @@ +--- +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 diff --git a/ee/app/serializers/ee/merge_request_reviewer_entity.rb b/ee/app/serializers/ee/merge_request_reviewer_entity.rb new file mode 100644 index 0000000000000000000000000000000000000000..20e3ea5fb3a907ab7768c09d6a702304fe1bae24 --- /dev/null +++ b/ee/app/serializers/ee/merge_request_reviewer_entity.rb @@ -0,0 +1,11 @@ +# 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 diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index ac0a031445558c88367bb7352a30e4a3bd1f3322..3c19528d71bdebf124cb16b8da7b9048482bca6b 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/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" ] }, diff --git a/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json b/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json index 11076ec73ded711d7d812592a65516d335197790..c54ae551d6a8c0a6167aafa0ca38c6728d75c495 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json +++ b/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 diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index f869b586fbee964de010c60ff331d94257400efd..f1902d199932821b1e0888c8f17897c3ec808597 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1212,4 +1212,14 @@ 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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c9389ad7f439546f30b06d24cf268ba8699ddcc4..0a5c4241abb5bbac14e2a0b2fa7dfe060b1a61f5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -4145,4 +4145,22 @@ def create_build(pipeline, coverage, name) .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 diff --git a/spec/serializers/merge_request_basic_entity_spec.rb b/spec/serializers/merge_request_basic_entity_spec.rb index 1cddd87e91732c3c4b38d463bc480c79306880ea..4a8bcd72d9cc80f7d68d8631898dc58e9abd4233 100644 --- a/spec/serializers/merge_request_basic_entity_spec.rb +++ b/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 @@ 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 diff --git a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..74e9956c8a0305810f174cb4b7c7179b9628a951 --- /dev/null +++ b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb @@ -0,0 +1,57 @@ +# 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 diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 5ccefe3d129cf979034317c3e4e4ca194739179d..941e1f20999d489c9ea17a3f31b7cfd9bc1215e0 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -339,6 +339,10 @@ 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 diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 5f5a94da400c7aba26a35600fe036bad0961cd0c..097115af4ef432121de4020203563fc91bbf7b1c 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -161,6 +161,29 @@ def update_merge_request(opts) 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 diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..a703264021705a0c1c48d228d78ec9885fc6e7a1 --- /dev/null +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -0,0 +1,62 @@ +# 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