Commit edccd047 authored by Patrick Bajao's avatar Patrick Bajao

Handle assignee changes side effects asynchronously

This is to reduce the number of queries that get executed by
running `MergeRequests::UpdateService`.

This is based on the solution made in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57523.

Added a new worker/service to extend the capability of the worker
added in that merge request. It now accepts a `execute_hooks`
option so it can be re-used by both `UpdateService` and
`UpdateAssigneesService`.
parent 3f139e5d
# frozen_string_literal: true
module MergeRequests
class HandleAssigneesChangeService < MergeRequests::BaseService
def async_execute(merge_request, old_assignees, options = {})
if Feature.enabled?(:async_handle_merge_request_assignees_change, merge_request.target_project, default_enabled: :yaml)
MergeRequests::HandleAssigneesChangeWorker
.perform_async(
merge_request.id,
current_user.id,
old_assignees.map(&:id),
options
)
else
execute(merge_request, old_assignees, options)
end
end
def execute(merge_request, old_assignees, options = {})
create_assignee_note(merge_request, old_assignees)
notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees.to_a)
todo_service.reassigned_assignable(merge_request, current_user, old_assignees)
new_assignees = merge_request.assignees - old_assignees
merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees)
merge_request_activity_counter.track_assignees_changed_action(user: current_user)
execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks]
end
private
def execute_assignees_hooks(merge_request, old_assignees)
execute_hooks(
merge_request,
'update',
old_associations: { assignees: old_assignees }
)
end
end
end
MergeRequests::HandleAssigneesChangeService.prepend_if_ee('EE::MergeRequests::HandleAssigneesChangeService')
......@@ -9,7 +9,8 @@ module MergeRequests
def execute(merge_request)
return merge_request unless current_user&.can?(:update_merge_request, merge_request)
old_ids = merge_request.assignees.map(&:id)
old_assignees = merge_request.assignees
old_ids = old_assignees.map(&:id)
new_ids = new_assignee_ids(merge_request)
return merge_request if new_ids.size != update_attrs[:assignee_ids].size
return merge_request if old_ids.to_set == new_ids.to_set # no-change
......@@ -18,22 +19,13 @@ module MergeRequests
merge_request.update!(**attrs)
# Defer the more expensive operations (handle_assignee_changes) to the background
MergeRequests::AssigneesChangeWorker.perform_async(merge_request.id, current_user.id, old_ids)
MergeRequests::HandleAssigneesChangeService
.new(project, current_user)
.async_execute(merge_request, old_assignees, execute_hooks: true)
merge_request
end
def handle_assignee_changes(merge_request, old_assignees)
# exposes private method from super-class
users = old_assignees.to_a
handle_assignees_change(merge_request, users)
execute_hooks(
merge_request,
'update',
old_associations: { assignees: users }
)
end
private
def new_assignee_ids(merge_request)
......
......@@ -213,13 +213,9 @@ module MergeRequests
end
def handle_assignees_change(merge_request, old_assignees)
create_assignee_note(merge_request, old_assignees)
notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees)
todo_service.reassigned_assignable(merge_request, current_user, old_assignees)
new_assignees = merge_request.assignees - old_assignees
merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees)
merge_request_activity_counter.track_assignees_changed_action(user: current_user)
MergeRequests::HandleAssigneesChangeService
.new(project, current_user)
.async_execute(merge_request, old_assignees)
end
def handle_reviewers_change(merge_request, old_reviewers)
......
......@@ -1892,6 +1892,14 @@
:weight: 1
:idempotent: true
:tags: []
- :name: merge_requests_handle_assignees_change
:feature_category: :code_review
:has_external_dependencies:
:urgency: :high
:resource_boundary: :unknown
:weight: 1
:idempotent: true
:tags: []
- :name: metrics_dashboard_prune_old_annotations
:feature_category: :metrics
:has_external_dependencies:
......
......@@ -18,12 +18,9 @@ class MergeRequests::AssigneesChangeWorker
return if users.blank?
service = ::MergeRequests::UpdateAssigneesService.new(
merge_request.target_project,
current_user
)
service.handle_assignee_changes(merge_request, users)
::MergeRequests::HandleAssigneesChangeService
.new(merge_request.target_project, current_user)
.execute(merge_request, users, execute_hooks: true)
rescue ActiveRecord::RecordNotFound
end
end
# frozen_string_literal: true
class MergeRequests::HandleAssigneesChangeWorker
include ApplicationWorker
feature_category :code_review
urgency :high
deduplicate :until_executed
idempotent!
def perform(merge_request_id, user_id, old_assignee_ids, options = {})
merge_request = MergeRequest.find(merge_request_id)
user = User.find(user_id)
old_assignees = User.id_in(old_assignee_ids)
::MergeRequests::HandleAssigneesChangeService
.new(merge_request.target_project, user)
.execute(merge_request, old_assignees, options)
rescue ActiveRecord::RecordNotFound
end
end
---
title: Handle assignee changes side effects asynchronously
merge_request: 58783
author:
type: performance
---
name: async_handle_merge_request_assignees_change
introduced_by_url:
rollout_issue_url:
milestone: '13.11'
type: development
group: group::code review
default_enabled: false
......@@ -214,6 +214,8 @@
- 1
- - merge_requests_delete_source_branch
- 1
- - merge_requests_handle_assignees_change
- 1
- - metrics_dashboard_prune_old_annotations
- 1
- - metrics_dashboard_sync_dashboards
......
# frozen_string_literal: true
module EE
module MergeRequests
module HandleAssigneesChangeService
extend ::Gitlab::Utils::Override
override :execute
def execute(merge_request, _old_assignees, _options = {})
super
return unless merge_request.project.licensed_feature_available?(:code_review_analytics)
::Analytics::RefreshReassignData.new(merge_request).execute_async
end
end
end
end
......@@ -49,15 +49,6 @@ module EE
reset_approvals(merge_request)
end
override :handle_assignees_change
def handle_assignees_change(merge_request, _old_assignees)
super
return unless merge_request.project.feature_available?(:code_review_analytics)
::Analytics::RefreshReassignData.new(merge_request).execute_async
end
def reset_approval_rules(merge_request)
return unless merge_request.project.can_override_approvers?
......
# 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(:merge_request) { create(:merge_request, author: user, source_project: project) }
let(:old_assignees) { [] }
let(:options) { {} }
let(:service) { described_class.new(project, user) }
describe '#execute' do
def execute
service.execute(merge_request, old_assignees, options)
end
it 'schedules for analytics metric update' do
expect(Analytics::CodeReviewMetricsWorker)
.to receive(:perform_async).with('Analytics::RefreshReassignData', merge_request.id)
execute
end
context 'when code_review_analytics is not available' do
before do
stub_licensed_features(code_review_analytics: false)
end
it 'does not schedule for analytics metric update' do
expect(Analytics::CodeReviewMetricsWorker).not_to receive(:perform_async)
execute
end
end
end
end
......@@ -226,27 +226,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
end
end
context 'when reassigned' do
it 'schedules for analytics metric update' do
expect(Analytics::CodeReviewMetricsWorker)
.to receive(:perform_async).with('Analytics::RefreshReassignData', merge_request.id)
update_merge_request({ assignee_ids: [user2.id] })
end
context 'when code_review_analytics is not available' do
before do
stub_licensed_features(code_review_analytics: false)
end
it 'does not schedule for analytics metric update' do
expect(Analytics::CodeReviewMetricsWorker).not_to receive(:perform_async)
update_merge_request({ assignee_ids: [user2.id] })
end
end
end
context 'reset_approval_rules_to_defaults param' do
let!(:existing_any_rule) { create(:any_approver_rule, merge_request: merge_request) }
let!(:existing_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
......@@ -331,16 +310,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
update_merge_request(title: 'Title')
end
context 'updating assignee_ids' do
it 'updates the tracking when user ids are valid' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_users_assigned_to_mr)
.with(users: [user, user2])
update_merge_request(assignee_ids: [user.id, user2.id])
end
end
context 'updating reviewers_ids' do
it 'updates the tracking when user ids are valid' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
......
......@@ -297,7 +297,7 @@ RSpec.describe Projects::MergeRequests::DraftsController do
expect { post :publish, params: params }.to change { Note.count }.by(0).and change { DraftNote.count }.by(0)
end
it 'publishes a draft note with quick actions and applies them' do
it 'publishes a draft note with quick actions and applies them', :sidekiq_inline do
project.add_developer(user2)
create(:draft_note, merge_request: merge_request, author: user,
note: "/assign #{user2.to_reference}")
......
......@@ -35,7 +35,7 @@ RSpec.describe 'User sees user popover', :js do
end
end
it "displays user popover in system note" do
it 'displays user popover in system note', :sidekiq_inline do
add_note("/assign @#{user.username}")
find('.system-note-message .js-user-link').hover
......
......@@ -251,7 +251,7 @@ RSpec.describe API::Notes do
expect { subject }.not_to change { Note.where(system: false).count }
end
it 'does however create a system note about the change' do
it 'does however create a system note about the change', :sidekiq_inline do
expect { subject }.to change { Note.system.count }.by(1)
end
......
......@@ -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)
......
# 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
......@@ -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
......@@ -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)
......
......@@ -19,7 +19,7 @@ RSpec.describe MergeRequests::AssigneesChangeWorker do
describe '#perform' do
context 'with a non-existing merge request' do
it 'does nothing' do
expect(::MergeRequests::UpdateAssigneesService).not_to receive(:new)
expect(::MergeRequests::HandleAssigneesChangeService).not_to receive(:new)
worker.perform(non_existing_record_id, user.id, user_ids)
end
......@@ -27,7 +27,7 @@ RSpec.describe MergeRequests::AssigneesChangeWorker do
context 'with a non-existing user' do
it 'does nothing' do
expect(::MergeRequests::UpdateAssigneesService).not_to receive(:new)
expect(::MergeRequests::HandleAssigneesChangeService).not_to receive(:new)
worker.perform(merge_request.id, non_existing_record_id, user_ids)
end
......@@ -35,7 +35,7 @@ RSpec.describe MergeRequests::AssigneesChangeWorker do
context 'when there are no changes' do
it 'does nothing' do
expect(::MergeRequests::UpdateAssigneesService).not_to receive(:new)
expect(::MergeRequests::HandleAssigneesChangeService).not_to receive(:new)
worker.perform(merge_request.id, user.id, merge_request.assignee_ids)
end
......@@ -43,15 +43,15 @@ RSpec.describe MergeRequests::AssigneesChangeWorker do
context 'when the old users cannot be found' do
it 'does nothing' do
expect(::MergeRequests::UpdateAssigneesService).not_to receive(:new)
expect(::MergeRequests::HandleAssigneesChangeService).not_to receive(:new)
worker.perform(merge_request.id, user.id, [non_existing_record_id])
end
end
it 'gets MergeRequests::UpdateAssigneesService to handle the changes' do
expect_next(::MergeRequests::UpdateAssigneesService)
.to receive(:handle_assignee_changes).with(merge_request, match_array(old_assignees))
expect_next(::MergeRequests::HandleAssigneesChangeService)
.to receive(:execute).with(merge_request, match_array(old_assignees), execute_hooks: true)
worker.perform(merge_request.id, user.id, user_ids)
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe MergeRequests::HandleAssigneesChangeWorker do
include AfterNextHelpers
let_it_be(:merge_request) { create(:merge_request) }
let_it_be(:user) { create(:user) }
let_it_be(:old_assignees) { create_list(:user, 3) }
let(:user_ids) { old_assignees.map(&:id).to_a }
let(:options) { {} }
let(:worker) { described_class.new }
it_behaves_like 'an idempotent worker' do
let(:job_args) { [merge_request.id, user.id, user_ids, options] }
end
describe '#perform' do
it 'calls MergeRequests::HandleAssigneesChangeService#execute to handle the changes' do
expect_next(::MergeRequests::HandleAssigneesChangeService)
.to receive(:execute).with(merge_request, old_assignees, options)
worker.perform(merge_request.id, user.id, user_ids, options)
end
context 'when there are no changes' do
it 'still calls MergeRequests::HandleAssigneesChangeService#execute' do
expect_next(::MergeRequests::HandleAssigneesChangeService)
.to receive(:execute).with(merge_request, [], options)
worker.perform(merge_request.id, user.id, merge_request.assignee_ids, options)
end
end
context 'when the old assignees cannot be found' do
it 'still calls MergeRequests::HandleAssigneesChangeService#execute' do
expect_next(::MergeRequests::HandleAssigneesChangeService)
.to receive(:execute).with(merge_request, [], options)
worker.perform(merge_request.id, user.id, [non_existing_record_id], options)
end
end
context 'with a non-existing merge request' do
it 'does nothing' do
expect(::MergeRequests::HandleAssigneesChangeService).not_to receive(:new)
worker.perform(non_existing_record_id, user.id, user_ids, options)
end
end
context 'with a non-existing user' do
it 'does nothing' do
expect(::MergeRequests::HandleAssigneesChangeService).not_to receive(:new)
worker.perform(merge_request.id, non_existing_record_id, user_ids, options)
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment