From da59ce8b217f67707b391d9fb3503dbdf8c4e511 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 12 Apr 2021 18:12:15 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- spec/services/draft_notes/publish_service_spec.rb | 2 +- .../handle_assignees_change_service_spec.rb | 114 +++++++++++++++++++++ .../update_assignees_service_spec.rb | 29 ++---- .../services/merge_requests/update_service_spec.rb | 77 +++----------- 4 files changed, 141 insertions(+), 81 deletions(-) create mode 100644 spec/services/merge_requests/handle_assignees_change_service_spec.rb (limited to 'spec/services') diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index f83e91b683f..f93622dc25a 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -229,7 +229,7 @@ RSpec.describe DraftNotes::PublishService do expect(DraftNote.count).to eq(2) end - context 'with quick actions' do + context 'with quick actions', :sidekiq_inline do it 'performs quick actions' do other_user = create(:user) project.add_developer(other_user) diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb new file mode 100644 index 00000000000..cc595aab04b --- /dev/null +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -0,0 +1,114 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::HandleAssigneesChangeService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:assignee) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, author: user, source_project: project, assignees: [assignee]) } + let_it_be(:old_assignees) { create_list(:user, 3) } + + let(:options) { {} } + let(:service) { described_class.new(project, user) } + + before_all do + project.add_maintainer(user) + project.add_developer(assignee) + + old_assignees.each do |old_assignee| + project.add_developer(old_assignee) + end + end + + describe '#async_execute' do + def async_execute + service.async_execute(merge_request, old_assignees, options) + end + + it 'performs MergeRequests::HandleAssigneesChangeWorker asynchronously' do + expect(MergeRequests::HandleAssigneesChangeWorker) + .to receive(:perform_async) + .with( + merge_request.id, + user.id, + old_assignees.map(&:id), + options + ) + + async_execute + end + + context 'when async_handle_merge_request_assignees_change feature is disabled' do + before do + stub_feature_flags(async_handle_merge_request_assignees_change: false) + end + + it 'calls #execute' do + expect(service).to receive(:execute).with(merge_request, old_assignees, options) + + async_execute + end + end + end + + describe '#execute' do + def execute + service.execute(merge_request, old_assignees, options) + end + + it 'creates assignee note' do + execute + + note = merge_request.notes.last + + expect(note).not_to be_nil + expect(note.note).to include "assigned to #{assignee.to_reference} and unassigned #{old_assignees.map(&:to_reference).to_sentence}" + end + + it 'sends email notifications to old and new assignees', :mailer, :sidekiq_inline do + perform_enqueued_jobs do + execute + end + + should_email(assignee) + old_assignees.each do |old_assignee| + should_email(old_assignee) + end + end + + it 'creates pending todo for assignee' do + execute + + todo = assignee.todos.last + + expect(todo).to be_pending + end + + it 'tracks users assigned event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_users_assigned_to_mr).once.with(users: [assignee]) + + execute + end + + it 'tracks assignees changed event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_assignees_changed_action).once.with(user: user) + + execute + end + + context 'when execute_hooks option is set to true' do + let(:options) { { execute_hooks: true } } + + it 'execute hooks and services' do + expect(merge_request.project).to receive(:execute_hooks).with(anything, :merge_request_hooks) + expect(merge_request.project).to receive(:execute_services).with(anything, :merge_request_hooks) + expect(service).to receive(:enqueue_jira_connect_messages_for).with(merge_request) + + execute + end + end + end +end diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb index aa6aad854a6..de03aab5418 100644 --- a/spec/services/merge_requests/update_assignees_service_spec.rb +++ b/spec/services/merge_requests/update_assignees_service_spec.rb @@ -37,9 +37,11 @@ RSpec.describe MergeRequests::UpdateAssigneesService do context 'when the parameters are valid' do it 'updates the MR, and queues the more expensive work for later' do - expect(MergeRequests::AssigneesChangeWorker) - .to receive(:perform_async) - .with(merge_request.id, user.id, [user3.id]) + expect_next(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user3], execute_hooks: true) + end expect { update_merge_request } .to change(merge_request, :assignees).to([user2]) @@ -54,9 +56,11 @@ RSpec.describe MergeRequests::UpdateAssigneesService do end it 'is more efficient than using the full update-service' do - allow(MergeRequests::AssigneesChangeWorker) - .to receive(:perform_async) - .with(merge_request.id, user.id, [user3.id]) + allow_next(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user3], execute_hooks: true) + end other_mr = create(:merge_request, :simple, :unique_branches, title: merge_request.title, @@ -72,17 +76,4 @@ RSpec.describe MergeRequests::UpdateAssigneesService do end end end - - describe '#handle_assignee_changes' do - subject { service.handle_assignee_changes(merge_request, [user2]) } - - it 'calls UpdateService#handle_assignee_changes and executes hooks' do - expect(service).to receive(:handle_assignees_change).with(merge_request, [user2]) - expect(merge_request.project).to receive(:execute_hooks).with(anything, :merge_request_hooks) - expect(merge_request.project).to receive(:execute_services).with(anything, :merge_request_hooks) - expect(service).to receive(:enqueue_jira_connect_messages_for).with(merge_request) - - subject - end - end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 579d274d910..24b7c3b933b 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -205,30 +205,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) end - context 'assignees' do - context 'when assignees changed' do - it 'tracks assignees changed event' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .to receive(:track_assignees_changed_action).once.with(user: user) - - opts[:assignees] = [user2] - - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) - end - end - - context 'when assignees did not change' do - it 'does not track assignees changed event' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .not_to receive(:track_assignees_changed_action) - - opts[:assignees] = merge_request.assignees - - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) - end - end - end - context 'reviewers' do context 'when reviewers changed' do it 'tracks reviewers changed event' do @@ -326,21 +302,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do ) end - it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment', :sidekiq_might_not_need_inline do - deliveries = ActionMailer::Base.deliveries - email = deliveries.last - recipients = deliveries.last(2).flat_map(&:to) - expect(recipients).to include(user2.email, user3.email) - expect(email.subject).to include(merge_request.title) - end - - it 'creates system note about merge_request reassign' do - note = find_note('assigned to') - - expect(note).not_to be_nil - expect(note.note).to include "assigned to #{user.to_reference} and unassigned #{user3.to_reference}" - end - context 'with reviewers' do let(:opts) { { reviewer_ids: [user2.id] } } @@ -664,20 +625,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'marks previous assignee pending todos as done' do expect(pending_todo.reload).to be_done end - - it 'creates a pending todo for new assignee' do - attributes = { - project: project, - author: user, - user: user2, - target_id: merge_request.id, - target_type: merge_request.class.name, - action: Todo::ASSIGNED, - state: :pending - } - - expect(Todo.where(attributes).count).to eq 1 - end end context 'when reviewers gets changed' do @@ -999,6 +946,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'does not update assignee when assignee_id is invalid' do merge_request.update!(assignee_ids: [user.id]) + expect(MergeRequests::HandleAssigneesChangeService).not_to receive(:new) + update_merge_request(assignee_ids: [-1]) expect(merge_request.reload.assignees).to eq([user]) @@ -1007,29 +956,35 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'unassigns assignee when user id is 0' do merge_request.update!(assignee_ids: [user.id]) + expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user]) + end + update_merge_request(assignee_ids: [0]) expect(merge_request.assignee_ids).to be_empty end it 'saves assignee when user id is valid' do + expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect(service) + .to receive(:async_execute) + .with(merge_request, [user3]) + end + update_merge_request(assignee_ids: [user.id]) expect(merge_request.assignee_ids).to eq([user.id]) end - it 'updates the tracking when user ids are valid' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .to receive(:track_users_assigned_to_mr) - .with(users: [user]) - - update_merge_request(assignee_ids: [user.id]) - end - it 'does not update assignee_id when user cannot read issue' do non_member = create(:user) original_assignees = merge_request.assignees + expect(MergeRequests::HandleAssigneesChangeService).not_to receive(:new) + update_merge_request(assignee_ids: [non_member.id]) expect(merge_request.reload.assignees).to eq(original_assignees) -- cgit v1.2.3