diff options
Diffstat (limited to 'spec/services/merge_requests')
14 files changed, 667 insertions, 60 deletions
diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 9b064da44b8..e500102a00c 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -41,6 +41,12 @@ RSpec.describe MergeRequests::ApprovalService do end context 'with valid approval' do + let(:notification_service) { NotificationService.new } + + before do + allow(service).to receive(:notification_service).and_return(notification_service) + end + it 'creates an approval note and marks pending todos as done' do expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) expect(merge_request.approvals).to receive(:reset) @@ -59,9 +65,16 @@ RSpec.describe MergeRequests::ApprovalService do service.execute(merge_request) end + it 'sends a notification when approving' do + expect(notification_service).to receive_message_chain(:async, :approve_mr) + .with(merge_request, user) + + service.execute(merge_request) + end + it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request) + .with(project: project, current_user: user, merge_request: merge_request, user: user) .and_call_original service.execute(merge_request) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index d36a2f75cfe..cd1c362a19f 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -97,7 +97,7 @@ RSpec.describe MergeRequests::CloseService do it 'clean up environments for the merge request' do expect_next_instance_of(::Environments::StopService) do |service| - expect(service).to receive(:execute_for_merge_request).with(merge_request) + expect(service).to receive(:execute_for_merge_request_pipeline).with(merge_request) end described_class.new(project: project, current_user: user).execute(merge_request) diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb index 26f53f55b0f..fa3b1614e21 100644 --- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -89,18 +89,12 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request) + .with(project: project, current_user: user, merge_request: merge_request, user: user) .and_call_original execute end - it 'updates attention requested by of assignee' do - execute - - expect(merge_request.find_assignee(assignee).updated_state_by).to eq(user) - end - it 'tracks users assigned event' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_users_assigned_to_mr).once.with(users: [assignee]) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index ecb856bd1a4..78deab64b1c 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -149,7 +149,7 @@ RSpec.describe MergeRequests::MergeService do allow(project).to receive(:default_branch).and_return(merge_request.target_branch) end - it 'closes GitLab issue tracker issues' do + it 'closes GitLab issue tracker issues', :sidekiq_inline do issue = create :issue, project: project commit = double('commit', safe_message: "Fixes #{issue.to_reference}", date: Time.current, authored_date: Time.current) allow(merge_request).to receive(:commits).and_return([commit]) diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 8d9a32c3e9e..f0885365f96 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -59,24 +59,9 @@ RSpec.describe MergeRequests::PostMergeService do expect(diff_removal_service).to have_received(:execute) end - it 'marks MR as merged regardless of errors when closing issues' do - merge_request.update!(target_branch: 'foo') - allow(project).to receive(:default_branch).and_return('foo') - - issue = create(:issue, project: project) - allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue]) - expect_next_instance_of(Issues::CloseService) do |close_service| - allow(close_service).to receive(:execute).with(issue, commit: merge_request).and_raise(RuntimeError) - end - - expect { subject }.to raise_error(RuntimeError) - - expect(merge_request.reload).to be_merged - end - it 'clean up environments for the merge request' do expect_next_instance_of(::Environments::StopService) do |stop_environment_service| - expect(stop_environment_service).to receive(:execute_for_merge_request).with(merge_request) + expect(stop_environment_service).to receive(:execute_for_merge_request_pipeline).with(merge_request) end subject @@ -88,6 +73,67 @@ RSpec.describe MergeRequests::PostMergeService do subject end + context 'when there are issues to be closed' do + let_it_be(:issue) { create(:issue, project: project) } + + before do + merge_request.update!(target_branch: 'foo') + + allow(project).to receive(:default_branch).and_return('foo') + allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue]) + end + + it 'performs MergeRequests::CloseIssueWorker asynchronously' do + expect(MergeRequests::CloseIssueWorker) + .to receive(:perform_async) + .with(project.id, user.id, issue.id, merge_request.id) + + subject + + expect(merge_request.reload).to be_merged + end + + context 'when issue is an external issue' do + let_it_be(:issue) { ExternalIssue.new('JIRA-123', project) } + + it 'executes Issues::CloseService' do + expect_next_instance_of(Issues::CloseService) do |close_service| + expect(close_service).to receive(:execute).with(issue, commit: merge_request) + end + + subject + + expect(merge_request.reload).to be_merged + end + end + + context 'when async_mr_close_issue feature flag is disabled' do + before do + stub_feature_flags(async_mr_close_issue: false) + end + + it 'executes Issues::CloseService' do + expect_next_instance_of(Issues::CloseService) do |close_service| + expect(close_service).to receive(:execute).with(issue, commit: merge_request) + end + + subject + + expect(merge_request.reload).to be_merged + end + + it 'marks MR as merged regardless of errors when closing issues' do + expect_next_instance_of(Issues::CloseService) do |close_service| + allow(close_service).to receive(:execute).with(issue, commit: merge_request).and_raise(RuntimeError) + end + + expect { subject }.to raise_error(RuntimeError) + + expect(merge_request.reload).to be_merged + end + end + end + context 'when the merge request has review apps' do it 'cancels all review app deployments' do pipeline = create(:ci_pipeline, diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 348ea9ad7d4..338057f23d5 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -20,7 +20,17 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } let(:title) { 'my title' } + let(:draft_title) { 'Draft: my title' } + let(:draft) { true } let(:description) { 'my description' } + let(:multiline_description) do + <<~MD.chomp + Line 1 + Line 2 + Line 3 + MD + end + let(:label1) { 'mylabel1' } let(:label2) { 'mylabel2' } let(:label3) { 'mylabel3' } @@ -64,6 +74,26 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can set the multiline description of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the multiline description' do + service.execute + + expect(last_mr.description).to eq(multiline_description) + end + end + + shared_examples_for 'a service that can set the draft of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the draft' do + service.execute + + expect(last_mr.draft).to eq(draft) + end + end + shared_examples_for 'a service that can set the milestone of a merge request' do subject(:last_mr) { MergeRequest.last } @@ -417,6 +447,74 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it_behaves_like 'a service that can set the description of a merge request' + + context 'with a multiline description' do + let(:push_options) { { description: "Line 1\\nLine 2\\nLine 3" } } + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the multiline description of a merge request' + end + end + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + describe '`draft` push option' do + let(:push_options) { { draft: draft } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, draft: draft } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, draft: draft } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' + end + + context 'draft title provided while `draft` push option is set to false' do + let(:push_options) { { create: true, draft: false, title: draft_title } } + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the draft of a merge request' end it_behaves_like 'with a deleted branch' diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index a47e626666b..e7aa6e74246 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -70,11 +70,13 @@ RSpec.describe MergeRequests::RebaseService do it 'logs the error' do expect(service).to receive(:log_error).with(exception: exception, message: described_class::REBASE_ERROR, save_message_on_model: true).and_call_original expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception, - class: described_class.to_s, - merge_request: merge_request_ref, - merge_request_id: merge_request.id, - message: described_class::REBASE_ERROR, - save_message_on_model: true).and_call_original + { + class: described_class.to_s, + merge_request: merge_request_ref, + merge_request_id: merge_request.id, + message: described_class::REBASE_ERROR, + save_message_on_model: true + }).and_call_original service.execute(merge_request) end diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb index ef6a0ec69bd..5a319e90a68 100644 --- a/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/spec/services/merge_requests/remove_approval_service_spec.rb @@ -21,14 +21,20 @@ RSpec.describe MergeRequests::RemoveApprovalService do context 'with a user who has approved' do let!(:approval) { create(:approval, user: user, merge_request: merge_request) } + let(:notification_service) { NotificationService.new } + + before do + allow(service).to receive(:notification_service).and_return(notification_service) + end it 'removes the approval' do expect { execute! }.to change { merge_request.approvals.size }.from(2).to(1) end - it 'creates an unapproval note and triggers web hook' do + it 'creates an unapproval note, triggers a web hook, and sends a notification' do expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved') expect(SystemNoteService).to receive(:unapprove_mr) + expect(notification_service).to receive_message_chain(:async, :unapprove_mr).with(merge_request, user) execute! end diff --git a/spec/services/merge_requests/remove_attention_requested_service_spec.rb b/spec/services/merge_requests/remove_attention_requested_service_spec.rb index 450204ebfdd..576049b9f1b 100644 --- a/spec/services/merge_requests/remove_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/remove_attention_requested_service_spec.rb @@ -3,64 +3,112 @@ require 'spec_helper' RSpec.describe MergeRequests::RemoveAttentionRequestedService do - let(:current_user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } - let(:reviewer) { merge_request.find_reviewer(current_user) } - let(:assignee) { merge_request.find_assignee(current_user) } + let_it_be(:current_user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee_user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) } + + let(:reviewer) { merge_request.find_reviewer(user) } + let(:assignee) { merge_request.find_assignee(assignee_user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + let(:result) { service.execute } before do + allow(SystemNoteService).to receive(:remove_attention_request) + project.add_developer(current_user) + project.add_developer(user) end describe '#execute' do - context 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } + context 'when current user cannot update merge request' do + let(:service) do + described_class.new( + project: project, + current_user: create(:user), + merge_request: merge_request, + user: user + ) + end it 'returns an error' do expect(result[:status]).to eq :error end end - context 'reviewer does not exist' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } + context 'when user is not a reviewer nor assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: create(:user) + ) + end it 'returns an error' do expect(result[:status]).to eq :error end end - context 'reviewer exists' do + context 'when user is a reviewer' do + before do + reviewer.update!(state: :attention_requested) + end + it 'returns success' do expect(result[:status]).to eq :success end - it 'updates reviewers state' do + it 'updates reviewer state' do service.execute reviewer.reload expect(reviewer.state).to eq 'reviewed' end + it 'creates a remove attention request system note' do + expect(SystemNoteService) + .to receive(:remove_attention_request) + .with(merge_request, merge_request.project, current_user, user) + + service.execute + end + it_behaves_like 'invalidates attention request cache' do - let(:users) { [current_user] } + let(:users) { [user] } end end - context 'assignee exists' do - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + context 'when user is an assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: assignee_user + ) + end before do - assignee.update!(state: :reviewed) + assignee.update!(state: :attention_requested) end it 'returns success' do expect(result[:status]).to eq :success end - it 'updates assignees state' do + it 'updates assignee state' do service.execute assignee.reload @@ -68,14 +116,40 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do end it_behaves_like 'invalidates attention request cache' do - let(:users) { [current_user] } + let(:users) { [assignee_user] } + end + + it 'creates a remove attention request system note' do + expect(SystemNoteService) + .to receive(:remove_attention_request) + .with(merge_request, merge_request.project, current_user, assignee_user) + + service.execute end end - context 'assignee is the same as reviewer' do - let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } - let(:assignee) { merge_request.find_assignee(current_user) } + context 'when user is an assignee and reviewer at the same time' do + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) } + + let(:assignee) { merge_request.find_assignee(user) } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + + before do + reviewer.update!(state: :attention_requested) + assignee.update!(state: :attention_requested) + end + + it 'returns success' do + expect(result[:status]).to eq :success + end it 'updates reviewers and assignees state' do service.execute @@ -86,5 +160,24 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do expect(assignee.state).to eq 'reviewed' end end + + context 'when state is already not attention_requested' do + before do + reviewer.update!(state: :reviewed) + end + + it 'does not change state' do + service.execute + reviewer.reload + + expect(reviewer.state).to eq 'reviewed' + end + + it 'does not create a remove attention request system note' do + expect(SystemNoteService).not_to receive(:remove_attention_request) + + service.execute + end + end end end diff --git a/spec/services/merge_requests/request_attention_service_spec.rb b/spec/services/merge_requests/request_attention_service_spec.rb new file mode 100644 index 00000000000..813a8150625 --- /dev/null +++ b/spec/services/merge_requests/request_attention_service_spec.rb @@ -0,0 +1,220 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::RequestAttentionService do + let_it_be(:current_user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee_user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) } + + let(:reviewer) { merge_request.find_reviewer(user) } + let(:assignee) { merge_request.find_assignee(assignee_user) } + let(:project) { merge_request.project } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + + let(:result) { service.execute } + let(:todo_svc) { instance_double('TodoService') } + let(:notification_svc) { instance_double('NotificationService') } + + before do + allow(service).to receive(:todo_service).and_return(todo_svc) + allow(service).to receive(:notification_service).and_return(notification_svc) + allow(todo_svc).to receive(:create_attention_requested_todo) + allow(notification_svc).to receive_message_chain(:async, :attention_requested_of_merge_request) + allow(SystemNoteService).to receive(:request_attention) + + project.add_developer(current_user) + project.add_developer(user) + end + + describe '#execute' do + context 'when current user cannot update merge request' do + let(:service) do + described_class.new( + project: project, + current_user: create(:user), + merge_request: merge_request, + user: user + ) + end + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + context 'when user is not a reviewer nor assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: create(:user) + ) + end + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + context 'when user is a reviewer' do + before do + reviewer.update!(state: :reviewed) + end + + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + service.execute + reviewer.reload + + expect(reviewer.state).to eq 'attention_requested' + end + + it 'adds who toggled attention' do + service.execute + reviewer.reload + + expect(reviewer.updated_state_by).to eq current_user + end + + it 'creates a new todo for the reviewer' do + expect(todo_svc).to receive(:create_attention_requested_todo).with(merge_request, current_user, user) + + service.execute + end + + it 'sends email to reviewer' do + expect(notification_svc) + .to receive_message_chain(:async, :attention_requested_of_merge_request) + .with(merge_request, current_user, user) + + service.execute + end + + it 'removes attention requested state' do + expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .and_call_original + + service.execute + end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [user] } + end + end + + context 'when user is an assignee' do + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: assignee_user + ) + end + + before do + assignee.update!(state: :reviewed) + end + + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates assignees state' do + service.execute + assignee.reload + + expect(assignee.state).to eq 'attention_requested' + end + + it 'creates a new todo for the reviewer' do + expect(todo_svc).to receive(:create_attention_requested_todo).with(merge_request, current_user, assignee_user) + + service.execute + end + + it 'creates a request attention system note' do + expect(SystemNoteService) + .to receive(:request_attention) + .with(merge_request, merge_request.project, current_user, assignee_user) + + service.execute + end + + it 'removes attention requested state' do + expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .and_call_original + + service.execute + end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [assignee_user] } + end + end + + context 'when user is an assignee and reviewer at the same time' do + let_it_be(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) } + + let(:assignee) { merge_request.find_assignee(user) } + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + merge_request: merge_request, + user: user + ) + end + + before do + reviewer.update!(state: :reviewed) + assignee.update!(state: :reviewed) + end + + it 'updates reviewers and assignees state' do + service.execute + reviewer.reload + assignee.reload + + expect(reviewer.state).to eq 'attention_requested' + expect(assignee.state).to eq 'attention_requested' + end + end + + context 'when state is attention_requested' do + before do + reviewer.update!(state: :attention_requested) + end + + it 'does not change state' do + service.execute + reviewer.reload + + expect(reviewer.state).to eq 'attention_requested' + end + + it 'does not create a new todo for the reviewer' do + expect(todo_svc).not_to receive(:create_attention_requested_todo).with(merge_request, current_user, user) + + service.execute + end + end + end +end diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 387be8471b5..9210242a11e 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -222,11 +222,13 @@ RSpec.describe MergeRequests::SquashService do it 'logs the error' do expect(service).to receive(:log_error).with(exception: exception, message: 'Failed to squash merge request').and_call_original expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception, - class: described_class.to_s, - merge_request: merge_request_ref, - merge_request_id: merge_request.id, - message: 'Failed to squash merge request', - save_message_on_model: false).and_call_original + { + class: described_class.to_s, + merge_request: merge_request_ref, + merge_request_id: merge_request.id, + message: 'Failed to squash merge request', + save_message_on_model: false + }).and_call_original service.execute end diff --git a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb index dcaac5d2699..20bc536b21e 100644 --- a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb @@ -80,7 +80,7 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) .and_call_original service.execute @@ -129,7 +129,7 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request) + .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) .and_call_original service.execute diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb index 3a0b17c2768..f5f6f0ca301 100644 --- a/spec/services/merge_requests/update_assignees_service_spec.rb +++ b/spec/services/merge_requests/update_assignees_service_spec.rb @@ -113,6 +113,49 @@ RSpec.describe MergeRequests::UpdateAssigneesService do expect { service.execute(merge_request) } .to issue_fewer_queries_than { update_service.execute(other_mr) } end + + context 'setting state of assignees' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it 'does not set state as attention_requested if feature flag is disabled' do + update_merge_request + + expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested') + end + + context 'feature flag is enabled for current_user' do + before do + stub_feature_flags(mr_attention_requests: user) + end + + it 'sets state as attention_requested' do + update_merge_request + + expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested') + expect(merge_request.merge_request_assignees[0].updated_state_by).to eq(user) + end + + it 'uses reviewers state if it is same user as new assignee' do + merge_request.reviewers << user2 + + update_merge_request + + expect(merge_request.merge_request_assignees[0].state).to eq('unreviewed') + end + + context 'when assignee_ids matches existing assignee' do + let(:opts) { { assignee_ids: [user3.id] } } + + it 'keeps original assignees state' do + update_merge_request + + expect(merge_request.find_assignee(user3).state).to eq('unreviewed') + end + end + end + end end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 30095ebeb50..7164ba8fac0 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -328,6 +328,49 @@ RSpec.describe MergeRequests::UpdateService, :mailer do update_merge_request(reviewer_ids: [user.id]) end + + context 'setting state of reviewers' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it 'does not set state as attention_requested if feature flag is disabled' do + update_merge_request(reviewer_ids: [user.id]) + + expect(merge_request.merge_request_reviewers[0].state).not_to eq('attention_requested') + end + + context 'feature flag is enabled for current_user' do + before do + stub_feature_flags(mr_attention_requests: user) + end + + it 'sets state as attention_requested' do + update_merge_request(reviewer_ids: [user2.id]) + + expect(merge_request.merge_request_reviewers[0].state).to eq('attention_requested') + expect(merge_request.merge_request_reviewers[0].updated_state_by).to eq(user) + end + + it 'keeps original reviewers state' do + merge_request.find_reviewer(user2).update!(state: :unreviewed) + + update_merge_request({ + reviewer_ids: [user2.id] + }) + + expect(merge_request.find_reviewer(user2).state).to eq('unreviewed') + end + + it 'uses reviewers state if it is same user as new assignee' do + merge_request.assignees << user + + update_merge_request(reviewer_ids: [user.id]) + + expect(merge_request.merge_request_reviewers[0].state).to eq('unreviewed') + end + end + end end it 'creates a resource label event' do @@ -1066,6 +1109,53 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end end + + context 'setting state of assignees' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it 'does not set state as attention_requested if feature flag is disabled' do + update_merge_request({ + assignee_ids: [user2.id] + }) + + expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested') + end + + context 'feature flag is enabled for current_user' do + before do + stub_feature_flags(mr_attention_requests: user) + end + + it 'sets state as attention_requested' do + update_merge_request({ + assignee_ids: [user2.id] + }) + + expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested') + expect(merge_request.merge_request_assignees[0].updated_state_by).to eq(user) + end + + it 'keeps original assignees state' do + update_merge_request({ + assignee_ids: [user3.id] + }) + + expect(merge_request.find_assignee(user3).state).to eq('unreviewed') + end + + it 'uses reviewers state if it is same user as new assignee' do + merge_request.reviewers << user2 + + update_merge_request({ + assignee_ids: [user2.id] + }) + + expect(merge_request.merge_request_assignees[0].state).to eq('unreviewed') + end + end + end end context 'when adding time spent' do |