From a0295e812f26fa19509b3a8e7866cd89928f828d Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Fri, 5 Aug 2022 12:36:38 +0800
Subject: [PATCH] Implement mergeRequestSetReviewers mutation
Adds `mergeRequestSetReviewers` GraphQL mutation that will be
used by the frontend to set reviewers.
This has the same pattern of arguments and fields as the one
for assignees.
Changelog: added
---
.../mutations/merge_requests/set_reviewers.rb | 52 ++++++
app/graphql/types/mutation_type.rb | 1 +
.../update_assignees_service.rb | 23 +--
.../update_reviewers_service.rb | 44 +++++
app/services/merge_requests/update_service.rb | 36 +++-
doc/api/graphql/reference/index.md | 22 +++
.../update_assignees_service.rb | 2 +-
.../update_reviewers_service.rb | 18 ++
.../merge_requests/set_reviewers_spec.rb | 83 +++++++++
.../update_reviewers_service_spec.rb | 59 +++++++
.../merge_requests/set_reviewers_spec.rb | 140 +++++++++++++++
.../merge_requests/set_reviewers_spec.rb | 106 ++++++++++++
.../update_reviewers_service_spec.rb | 162 ++++++++++++++++++
13 files changed, 721 insertions(+), 27 deletions(-)
create mode 100644 app/graphql/mutations/merge_requests/set_reviewers.rb
create mode 100644 app/services/merge_requests/update_reviewers_service.rb
create mode 100644 ee/app/services/ee/merge_requests/update_reviewers_service.rb
create mode 100644 ee/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb
create mode 100644 ee/spec/services/ee/merge_requests/update_reviewers_service_spec.rb
create mode 100644 spec/graphql/mutations/merge_requests/set_reviewers_spec.rb
create mode 100644 spec/requests/api/graphql/mutations/merge_requests/set_reviewers_spec.rb
create mode 100644 spec/services/merge_requests/update_reviewers_service_spec.rb
diff --git a/app/graphql/mutations/merge_requests/set_reviewers.rb b/app/graphql/mutations/merge_requests/set_reviewers.rb
new file mode 100644
index 0000000000000..8d3f860159781
--- /dev/null
+++ b/app/graphql/mutations/merge_requests/set_reviewers.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+module Mutations
+ module MergeRequests
+ class SetReviewers < Base
+ graphql_name 'MergeRequestSetReviewers'
+
+ argument :reviewer_usernames,
+ [GraphQL::Types::String],
+ required: true,
+ description: 'Usernames of reviewers to assign. Replaces existing reviewers by default.'
+
+ argument :operation_mode,
+ Types::MutationOperationModeEnum,
+ required: false,
+ default_value: Types::MutationOperationModeEnum.default_mode,
+ description: 'Operation to perform. Defaults to REPLACE.'
+
+ def resolve(project_path:, iid:, reviewer_usernames:, operation_mode:)
+ resource = authorized_find!(project_path: project_path, iid: iid)
+
+ ::MergeRequests::UpdateReviewersService.new(
+ project: resource.project,
+ current_user: current_user,
+ params: { reviewer_ids: reviewer_ids(resource, reviewer_usernames, operation_mode) }
+ ).execute(resource)
+
+ {
+ resource.class.name.underscore.to_sym => resource,
+ errors: errors_on_object(resource)
+ }
+ end
+
+ private
+
+ def reviewer_ids(resource, usernames, mode)
+ new_reviewers = UsersFinder.new(current_user, username: usernames).execute.to_a
+ new_reviewer_ids = user_ids(new_reviewers)
+
+ case mode
+ when 'REPLACE' then new_reviewer_ids
+ when 'APPEND' then user_ids(resource.reviewers) | new_reviewer_ids
+ when 'REMOVE' then user_ids(resource.reviewers) - new_reviewer_ids
+ end
+ end
+
+ def user_ids(users)
+ users.map(&:id)
+ end
+ end
+ end
+end
diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb
index 47462b5e9f7a5..499c2e786bf4c 100644
--- a/app/graphql/types/mutation_type.rb
+++ b/app/graphql/types/mutation_type.rb
@@ -72,6 +72,7 @@ class MutationType < BaseObject
mount_mutation Mutations::MergeRequests::SetSubscription
mount_mutation Mutations::MergeRequests::SetDraft, calls_gitaly: true
mount_mutation Mutations::MergeRequests::SetAssignees
+ mount_mutation Mutations::MergeRequests::SetReviewers
mount_mutation Mutations::MergeRequests::ReviewerRereview
mount_mutation Mutations::Metrics::Dashboard::Annotations::Create
mount_mutation Mutations::Metrics::Dashboard::Annotations::Delete
diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb
index 5b23f69ac4a15..a6b0235c525a6 100644
--- a/app/services/merge_requests/update_assignees_service.rb
+++ b/app/services/merge_requests/update_assignees_service.rb
@@ -11,7 +11,7 @@ def execute(merge_request)
old_assignees = merge_request.assignees.to_a
old_ids = old_assignees.map(&:id)
- new_ids = new_assignee_ids(merge_request)
+ new_ids = new_user_ids(merge_request, update_attrs[:assignee_ids], :assignees)
return merge_request if merge_request.errors.any?
return merge_request if new_ids.size != update_attrs[:assignee_ids].size
@@ -32,27 +32,8 @@ def execute(merge_request)
private
- def new_assignee_ids(merge_request)
- # prime the cache - prevent N+1 lookup during authorization loop.
- user_ids = update_attrs[:assignee_ids]
- return [] if user_ids.empty?
-
- merge_request.project.team.max_member_access_for_user_ids(user_ids)
- User.id_in(user_ids).map do |user|
- if user.can?(:read_merge_request, merge_request)
- user.id
- else
- merge_request.errors.add(
- :assignees,
- "Cannot assign #{user.to_reference} to #{merge_request.to_reference}"
- )
- nil
- end
- end.compact
- end
-
def assignee_ids
- params.fetch(:assignee_ids).reject { _1 == 0 }.first(1)
+ filter_sentinel_values(params.fetch(:assignee_ids)).first(1)
end
def params
diff --git a/app/services/merge_requests/update_reviewers_service.rb b/app/services/merge_requests/update_reviewers_service.rb
new file mode 100644
index 0000000000000..8e974d756762f
--- /dev/null
+++ b/app/services/merge_requests/update_reviewers_service.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module MergeRequests
+ class UpdateReviewersService < UpdateService
+ def execute(merge_request)
+ return merge_request unless current_user&.can?(:update_merge_request, merge_request)
+
+ old_reviewers = merge_request.reviewers.to_a
+ old_ids = old_reviewers.map(&:id)
+ new_ids = new_user_ids(merge_request, update_attrs[:reviewer_ids], :reviewers)
+
+ return merge_request if merge_request.errors.any?
+ return merge_request if new_ids.size != update_attrs[:reviewer_ids].size
+ return merge_request if old_ids.to_set == new_ids.to_set # no-change
+
+ merge_request.update!(update_attrs.merge(reviewer_ids: new_ids))
+ handle_reviewers_change(merge_request, old_reviewers)
+ resolve_todos_for(merge_request)
+ execute_reviewers_hooks(merge_request, old_reviewers)
+
+ merge_request
+ end
+
+ private
+
+ def reviewer_ids
+ filter_sentinel_values(params.fetch(:reviewer_ids)).first(1)
+ end
+
+ def update_attrs
+ @attrs ||= { updated_by: current_user, reviewer_ids: reviewer_ids }
+ end
+
+ def execute_reviewers_hooks(merge_request, old_reviewers)
+ execute_hooks(
+ merge_request,
+ 'update',
+ old_associations: { reviewers: old_reviewers }
+ )
+ end
+ end
+end
+
+MergeRequests::UpdateReviewersService.prepend_mod
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 603da4ef535d5..0902b5195a139 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -155,11 +155,7 @@ def notify_if_mentions_added(merge_request, old_mentioned_users)
def resolve_todos(merge_request, old_labels, old_assignees, old_reviewers)
return unless has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees, old_reviewers: old_reviewers)
- service_user = current_user
-
- merge_request.run_after_commit_or_now do
- ::MergeRequests::ResolveTodosService.new(merge_request, service_user).async_execute
- end
+ resolve_todos_for(merge_request)
end
def handle_target_branch_change(merge_request)
@@ -296,6 +292,36 @@ def assignees_service
def add_time_spent_service
@add_time_spent_service ||= ::MergeRequests::AddSpentTimeService.new(project: project, current_user: current_user, params: params)
end
+
+ def new_user_ids(merge_request, user_ids, attribute)
+ # prime the cache - prevent N+1 lookup during authorization loop.
+ return [] if user_ids.empty?
+
+ merge_request.project.team.max_member_access_for_user_ids(user_ids)
+ User.id_in(user_ids).map do |user|
+ if user.can?(:read_merge_request, merge_request)
+ user.id
+ else
+ merge_request.errors.add(
+ attribute,
+ "Cannot assign #{user.to_reference} to #{merge_request.to_reference}"
+ )
+ nil
+ end
+ end.compact
+ end
+
+ def resolve_todos_for(merge_request)
+ service_user = current_user
+
+ merge_request.run_after_commit_or_now do
+ ::MergeRequests::ResolveTodosService.new(merge_request, service_user).async_execute
+ end
+ end
+
+ def filter_sentinel_values(param)
+ param.reject { _1 == 0 }
+ end
end
end
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index 570f35839e196..effb6d3e590d4 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -3762,6 +3762,28 @@ Input type: `MergeRequestSetMilestoneInput`
| <a id="mutationmergerequestsetmilestoneerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
| <a id="mutationmergerequestsetmilestonemergerequest"></a>`mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. |
+### `Mutation.mergeRequestSetReviewers`
+
+Input type: `MergeRequestSetReviewersInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationmergerequestsetreviewersclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationmergerequestsetreviewersiid"></a>`iid` | [`String!`](#string) | IID of the merge request to mutate. |
+| <a id="mutationmergerequestsetreviewersoperationmode"></a>`operationMode` | [`MutationOperationMode`](#mutationoperationmode) | Operation to perform. Defaults to REPLACE. |
+| <a id="mutationmergerequestsetreviewersprojectpath"></a>`projectPath` | [`ID!`](#id) | Project the merge request to mutate is in. |
+| <a id="mutationmergerequestsetreviewersreviewerusernames"></a>`reviewerUsernames` | [`[String!]!`](#string) | Usernames of reviewers to assign. Replaces existing reviewers by default. |
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationmergerequestsetreviewersclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationmergerequestsetreviewerserrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+| <a id="mutationmergerequestsetreviewersmergerequest"></a>`mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. |
+
### `Mutation.mergeRequestSetSubscription`
Input type: `MergeRequestSetSubscriptionInput`
diff --git a/ee/app/services/ee/merge_requests/update_assignees_service.rb b/ee/app/services/ee/merge_requests/update_assignees_service.rb
index 21a52394e8bdf..ba8534f485660 100644
--- a/ee/app/services/ee/merge_requests/update_assignees_service.rb
+++ b/ee/app/services/ee/merge_requests/update_assignees_service.rb
@@ -5,7 +5,7 @@ module MergeRequests
module UpdateAssigneesService
def assignee_ids
if project.licensed_feature_available?(:multiple_merge_request_assignees)
- params.fetch(:assignee_ids).reject { _1 == 0 }
+ filter_sentinel_values(params.fetch(:assignee_ids))
else
super
end
diff --git a/ee/app/services/ee/merge_requests/update_reviewers_service.rb b/ee/app/services/ee/merge_requests/update_reviewers_service.rb
new file mode 100644
index 0000000000000..530fb349f1a61
--- /dev/null
+++ b/ee/app/services/ee/merge_requests/update_reviewers_service.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module EE
+ module MergeRequests
+ module UpdateReviewersService
+ extend ::Gitlab::Utils::Override
+
+ override :reviewer_ids
+ def reviewer_ids
+ if project.licensed_feature_available?(:multiple_merge_request_reviewers)
+ filter_sentinel_values(params.fetch(:reviewer_ids))
+ else
+ super
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb b/ee/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb
new file mode 100644
index 0000000000000..53fc3210736a8
--- /dev/null
+++ b/ee/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb
@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::MergeRequests::SetReviewers do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:merge_request, reload: true) { create(:merge_request) }
+
+ subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+
+ describe '#resolve' do
+ let_it_be(:reviewers) { create_list(:user, 3) }
+
+ let(:mode) { Types::MutationOperationModeEnum.default_mode }
+ let(:reviewer_usernames) { reviewers.map(&:username) }
+ let(:mutated_merge_request) { subject[:merge_request] }
+
+ subject do
+ mutation.resolve(project_path: merge_request.project.full_path,
+ iid: merge_request.iid,
+ operation_mode: mode,
+ reviewer_usernames: reviewer_usernames)
+ end
+
+ before do
+ reviewers.each do |user|
+ merge_request.project.add_developer(user)
+ end
+ end
+
+ context 'when the user can update the merge_request' do
+ before do
+ merge_request.project.add_developer(user)
+ end
+
+ it 'sets the reviewers' do
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to match_array(reviewers)
+ expect(subject[:errors]).to be_empty
+ end
+
+ it 'removes reviewers not in the list' do
+ users = create_list(:user, 2)
+ users.each do |user|
+ merge_request.project.add_developer(user)
+ end
+ merge_request.reviewers = users
+ merge_request.save!
+
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to match_array(reviewers)
+ expect(subject[:errors]).to be_empty
+ end
+
+ context 'when passing "append" as true' do
+ subject do
+ mutation.resolve(
+ project_path: merge_request.project.full_path,
+ iid: merge_request.iid,
+ reviewer_usernames: reviewer_usernames,
+ operation_mode: Types::MutationOperationModeEnum.enum[:append]
+ )
+ end
+
+ let(:existing_reviewers) { create_list(:user, 2) }
+
+ before do
+ existing_reviewers.each do |user|
+ merge_request.project.add_developer(user)
+ end
+ merge_request.reviewers = existing_reviewers
+ merge_request.save!
+ end
+
+ it 'does not remove reviewers not in the list' do
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to match_array(reviewers + existing_reviewers)
+ expect(subject[:errors]).to be_empty
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/services/ee/merge_requests/update_reviewers_service_spec.rb b/ee/spec/services/ee/merge_requests/update_reviewers_service_spec.rb
new file mode 100644
index 0000000000000..6f85a5128ddd6
--- /dev/null
+++ b/ee/spec/services/ee/merge_requests/update_reviewers_service_spec.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::UpdateReviewersService do
+ let_it_be(:group) { create(:group, :public) }
+ let_it_be(:project) { create(:project, :private, :repository, group: group) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:user2) { create(:user) }
+ let_it_be(:user3) { create(:user) }
+
+ let_it_be_with_reload(:merge_request) do
+ create(:merge_request, :simple, :unique_branches,
+ reviewer_ids: [user.id],
+ source_project: project,
+ author: user)
+ end
+
+ before do
+ project.add_maintainer(user)
+ project.add_developer(user2)
+ project.add_developer(user3)
+ end
+
+ let(:service) { described_class.new(project: project, current_user: user, params: opts) }
+
+ describe 'execute' do
+ def set_reviewers
+ service.execute(merge_request)
+ merge_request.reload
+ end
+
+ context 'when the parameters are valid' do
+ context 'when using sentinel values' do
+ let(:opts) { { reviewer_ids: [0, 0, 0] } }
+
+ it 'removes all reviewers' do
+ expect { set_reviewers }.to change(merge_request, :reviewers).to([])
+ end
+ end
+
+ context 'when the reviewer_ids parameter is the empty list' do
+ let(:opts) { { reviewer_ids: [] } }
+
+ it 'removes all reviewers' do
+ expect { set_reviewers }.to change(merge_request, :reviewers).to([])
+ end
+ end
+
+ context 'when the reviewer_ids parameter contains both zeros and valid IDs' do
+ let(:opts) { { reviewer_ids: [0, user2.id, 0, user3.id, 0] } }
+
+ it 'ignores 0 IDs' do
+ expect { set_reviewers }.to change(merge_request, :reviewers).to(match_array([user2, user3]))
+ end
+ end
+ end
+ end
+end
diff --git a/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb b/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb
new file mode 100644
index 0000000000000..df4aa885bbff8
--- /dev/null
+++ b/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb
@@ -0,0 +1,140 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::MergeRequests::SetReviewers do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:merge_request, reload: true) { create(:merge_request) }
+ let_it_be(:reviewer) { create(:user) }
+ let_it_be(:reviewer2) { create(:user) }
+
+ subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+
+ describe '#resolve' do
+ let(:reviewer_usernames) { [reviewer.username] }
+ let(:mutated_merge_request) { subject[:merge_request] }
+ let(:mode) { described_class.arguments['operationMode'].default_value }
+
+ subject do
+ mutation.resolve(project_path: merge_request.project.full_path,
+ iid: merge_request.iid,
+ operation_mode: mode,
+ reviewer_usernames: reviewer_usernames)
+ end
+
+ it 'does not change reviewers if the merge_request is not accessible to the reviewers' do
+ merge_request.project.add_developer(user)
+
+ expect { subject }.not_to change { merge_request.reload.reviewer_ids }
+ end
+
+ it 'returns an operational error if the merge_request is not accessible to the reviewers' do
+ merge_request.project.add_developer(user)
+
+ result = subject
+
+ expect(result[:errors]).to include a_string_matching(/Cannot assign/)
+ end
+
+ context 'when the user does not have permissions' do
+ it_behaves_like 'permission level for merge request mutation is correctly verified'
+ end
+
+ context 'when the user can update the merge_request' do
+ before do
+ merge_request.project.add_developer(reviewer)
+ merge_request.project.add_developer(reviewer2)
+ merge_request.project.add_developer(user)
+ end
+
+ it 'replaces the reviewer' do
+ merge_request.reviewers = [reviewer2]
+ merge_request.save!
+
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to contain_exactly(reviewer)
+ expect(subject[:errors]).to be_empty
+ end
+
+ it 'returns errors when merge_request could not be updated' do
+ allow(merge_request).to receive(:errors_on_object).and_return(['foo'])
+
+ expect(subject[:errors]).not_to match_array(['foo'])
+ end
+
+ context 'when passing an empty reviewer list' do
+ let(:reviewer_usernames) { [] }
+
+ before do
+ merge_request.reviewers = [reviewer]
+ merge_request.save!
+ end
+
+ it 'removes all reviewers' do
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to eq([])
+ expect(subject[:errors]).to be_empty
+ end
+ end
+
+ context 'when passing "append" as true' do
+ subject do
+ mutation.resolve(
+ project_path: merge_request.project.full_path,
+ iid: merge_request.iid,
+ reviewer_usernames: reviewer_usernames,
+ operation_mode: Types::MutationOperationModeEnum.enum[:append]
+ )
+ end
+
+ before do
+ merge_request.reviewers = [reviewer2]
+ merge_request.save!
+
+ # In CE, APPEND is a NOOP as you can't have multiple reviewers
+ # We test multiple assignment in EE specs
+ stub_licensed_features(multiple_merge_request_reviewers: false)
+ end
+
+ it 'is a NO-OP in FOSS' do
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to contain_exactly(reviewer2)
+ expect(subject[:errors]).to be_empty
+ end
+ end
+
+ context 'when passing "remove" as true' do
+ before do
+ merge_request.reviewers = [reviewer]
+ merge_request.save!
+ end
+
+ it 'removes named reviewer' do
+ mutated_merge_request = mutation.resolve(
+ project_path: merge_request.project.full_path,
+ iid: merge_request.iid,
+ reviewer_usernames: reviewer_usernames,
+ operation_mode: Types::MutationOperationModeEnum.enum[:remove]
+ )[:merge_request]
+
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to eq([])
+ expect(subject[:errors]).to be_empty
+ end
+
+ it 'does not remove unnamed reviewer' do
+ mutated_merge_request = mutation.resolve(
+ project_path: merge_request.project.full_path,
+ iid: merge_request.iid,
+ reviewer_usernames: [reviewer2.username],
+ operation_mode: Types::MutationOperationModeEnum.enum[:remove]
+ )[:merge_request]
+
+ expect(mutated_merge_request).to eq(merge_request)
+ expect(mutated_merge_request.reviewers).to contain_exactly(reviewer)
+ expect(subject[:errors]).to be_empty
+ end
+ end
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_reviewers_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_reviewers_spec.rb
new file mode 100644
index 0000000000000..be786256ef223
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/merge_requests/set_reviewers_spec.rb
@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Setting reviewers of a merge request', :assume_throttled do
+ include GraphqlHelpers
+
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:current_user) { create(:user, developer_projects: [project]) }
+ let_it_be(:reviewer) { create(:user) }
+ let_it_be(:reviewer2) { create(:user) }
+ let_it_be_with_reload(:merge_request) { create(:merge_request, source_project: project) }
+
+ let(:input) { { reviewer_usernames: [reviewer.username] } }
+ let(:expected_result) do
+ [{ 'username' => reviewer.username }]
+ end
+
+ let(:mutation) do
+ variables = {
+ project_path: project.full_path,
+ iid: merge_request.iid.to_s
+ }
+ graphql_mutation(:merge_request_set_reviewers, variables.merge(input),
+ <<-QL.strip_heredoc
+ clientMutationId
+ errors
+ mergeRequest {
+ id
+ reviewers {
+ nodes {
+ username
+ }
+ }
+ }
+ QL
+ )
+ end
+
+ def mutation_response
+ graphql_mutation_response(:merge_request_set_reviewers)
+ end
+
+ def mutation_reviewer_nodes
+ mutation_response['mergeRequest']['reviewers']['nodes']
+ end
+
+ def run_mutation!
+ post_graphql_mutation(mutation, current_user: current_user)
+ end
+
+ before do
+ project.add_developer(reviewer)
+ project.add_developer(reviewer2)
+
+ merge_request.update!(reviewers: [])
+ end
+
+ it 'returns an error if the user is not allowed to update the merge request' do
+ post_graphql_mutation(mutation, current_user: create(:user))
+
+ expect(graphql_errors).not_to be_empty
+ end
+
+ context 'when the current user does not have permission to add reviewers' do
+ let(:current_user) { create(:user) }
+
+ it 'does not change the reviewers' do
+ project.add_guest(current_user)
+
+ expect { run_mutation! }.not_to change { merge_request.reset.reviewers.pluck(:id) }
+
+ expect(graphql_errors).not_to be_empty
+ end
+ end
+
+ context 'with reviewers already assigned' do
+ before do
+ merge_request.reviewers = [reviewer2]
+ merge_request.save!
+ end
+
+ it 'replaces the reviewer' do
+ run_mutation!
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(mutation_reviewer_nodes).to match_array(expected_result)
+ end
+ end
+
+ context 'when passing an empty list of reviewers' do
+ let(:input) { { reviewer_usernames: [] } }
+
+ before do
+ merge_request.reviewers = [reviewer2]
+ merge_request.save!
+ end
+
+ it 'removes reviewer' do
+ run_mutation!
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(mutation_reviewer_nodes).to eq([])
+ end
+ end
+end
diff --git a/spec/services/merge_requests/update_reviewers_service_spec.rb b/spec/services/merge_requests/update_reviewers_service_spec.rb
new file mode 100644
index 0000000000000..8920141adbb0f
--- /dev/null
+++ b/spec/services/merge_requests/update_reviewers_service_spec.rb
@@ -0,0 +1,162 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::UpdateReviewersService do
+ include AfterNextHelpers
+
+ let_it_be(:group) { create(:group, :public) }
+ let_it_be(:project) { create(:project, :private, :repository, group: group) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:user2) { create(:user) }
+ let_it_be(:user3) { create(:user) }
+
+ let_it_be_with_reload(:merge_request) do
+ create(:merge_request, :simple, :unique_branches,
+ title: 'Old title',
+ description: "FYI #{user2.to_reference}",
+ reviewer_ids: [user3.id],
+ source_project: project,
+ target_project: project,
+ author: create(:user))
+ end
+
+ before do
+ project.add_maintainer(user)
+ project.add_developer(user2)
+ project.add_developer(user3)
+ merge_request.errors.clear
+ end
+
+ let(:service) { described_class.new(project: project, current_user: user, params: opts) }
+ let(:opts) { { reviewer_ids: [user2.id] } }
+
+ describe 'execute' do
+ def set_reviewers
+ service.execute(merge_request)
+ end
+
+ def find_note(starting_with)
+ merge_request.notes.find do |note|
+ note && note.note.start_with?(starting_with)
+ end
+ end
+
+ shared_examples 'removing all reviewers' do
+ it 'removes all reviewers' do
+ expect(set_reviewers).to have_attributes(reviewers: be_empty, errors: be_none)
+ end
+ end
+
+ context 'when the parameters are valid' do
+ context 'when using sentinel values' do
+ let(:opts) { { reviewer_ids: [0] } }
+
+ it_behaves_like 'removing all reviewers'
+ end
+
+ context 'when the reviewer_ids parameter is the empty list' do
+ let(:opts) { { reviewer_ids: [] } }
+
+ it_behaves_like 'removing all reviewers'
+ end
+
+ it 'updates the MR' do
+ expect { set_reviewers }
+ .to change { merge_request.reload.reviewers }.from([user3]).to([user2])
+ .and change(merge_request, :updated_at)
+ .and change(merge_request, :updated_by).to(user)
+ end
+
+ it 'creates system note about merge_request review request' do
+ set_reviewers
+
+ note = find_note('requested review from')
+
+ expect(note).not_to be_nil
+ expect(note.note).to include "requested review from #{user2.to_reference}"
+ end
+
+ it 'creates a pending todo for new review request' do
+ set_reviewers
+
+ attributes = {
+ project: project,
+ author: user,
+ user: user2,
+ target_id: merge_request.id,
+ target_type: merge_request.class.name,
+ action: Todo::REVIEW_REQUESTED,
+ state: :pending
+ }
+
+ expect(Todo.where(attributes).count).to eq 1
+ end
+
+ it 'sends email reviewer change notifications to old and new reviewers', :sidekiq_inline, :mailer do
+ perform_enqueued_jobs do
+ set_reviewers
+ end
+
+ should_email(user2)
+ should_email(user3)
+ end
+
+ it 'updates open merge request counter for reviewers', :use_clean_rails_memory_store_caching do
+ # Cache them to ensure the cache gets invalidated on update
+ expect(user2.review_requested_open_merge_requests_count).to eq(0)
+ expect(user3.review_requested_open_merge_requests_count).to eq(1)
+
+ set_reviewers
+
+ expect(user2.review_requested_open_merge_requests_count).to eq(1)
+ expect(user3.review_requested_open_merge_requests_count).to eq(0)
+ end
+
+ it 'updates the tracking' do
+ expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+ .to receive(:track_users_review_requested)
+ .with(users: [user2])
+
+ set_reviewers
+ end
+
+ it 'tracks reviewers changed event' do
+ expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+ .to receive(:track_reviewers_changed_action).once.with(user: user)
+
+ set_reviewers
+ end
+
+ it 'calls MergeRequest::ResolveTodosService#async_execute' do
+ expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+ expect(service).to receive(:async_execute)
+ end
+
+ set_reviewers
+ end
+
+ it 'executes hooks with update action' do
+ expect(service).to receive(:execute_hooks)
+ .with(
+ merge_request,
+ 'update',
+ old_associations: {
+ reviewers: [user3]
+ }
+ )
+
+ set_reviewers
+ end
+
+ it 'does not update the reviewers if they do not have access' do
+ opts[:reviewer_ids] = [create(:user).id]
+
+ expect(set_reviewers).to have_attributes(
+ reviewers: [user3],
+ errors: be_any
+ )
+ end
+ end
+ end
+end
--
GitLab