diff --git a/app/events/merge_requests/approved_event.rb b/app/events/merge_requests/approved_event.rb new file mode 100644 index 0000000000000000000000000000000000000000..c68a002dcc378760df3f3508db109f4fe476b7f0 --- /dev/null +++ b/app/events/merge_requests/approved_event.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class ApprovedEvent < Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => %w[ + current_user_id + merge_request_id + ], + 'properties' => { + 'current_user_id' => { 'type' => 'integer' }, + 'merge_request_id' => { 'type' => 'integer' } + } + } + end + end +end diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index b8d817a15f310f6d4e00c0d0b81bab0ab7272331..f26513703055a8afea9d72ec8c68d3babd8ee3ac 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -10,13 +10,28 @@ def execute(merge_request) return success unless save_approval(approval) reset_approvals_cache(merge_request) - create_event(merge_request) + merge_request_activity_counter.track_approve_mr_action(user: current_user, merge_request: merge_request) + + # Approval side effects (things not required to be done immediately but + # should happen after a successful approval) should be done asynchronously + # utilizing the `Gitlab::EventStore`. + # + # Workers can subscribe to the `MergeRequests::ApprovedEvent`. + if Feature.enabled?(:async_after_approval, project) + Gitlab::EventStore.publish( + MergeRequests::ApprovedEvent.new( + data: { current_user_id: current_user.id, merge_request_id: merge_request.id } + ) + ) + else + create_event(merge_request) + end + stream_audit_event(merge_request) create_approval_note(merge_request) mark_pending_todos_as_done(merge_request) execute_approval_hooks(merge_request, current_user) remove_attention_requested(merge_request) - merge_request_activity_counter.track_approve_mr_action(user: current_user, merge_request: merge_request) success end @@ -27,21 +42,22 @@ def can_be_approved?(merge_request) current_user.can?(:approve_merge_request, merge_request) end + def save_approval(approval) + Approval.safe_ensure_unique do + approval.save + end + end + def reset_approvals_cache(merge_request) merge_request.approvals.reset end - def execute_approval_hooks(merge_request, current_user) - # Only one approval is required for a merge request to be approved - notification_service.async.approve_mr(merge_request, current_user) - - execute_hooks(merge_request, 'approved') + def create_event(merge_request) + event_service.approve_mr(merge_request, current_user) end - def save_approval(approval) - Approval.safe_ensure_unique do - approval.save - end + def stream_audit_event(merge_request) + # Defined in EE end def create_approval_note(merge_request) @@ -52,12 +68,11 @@ def mark_pending_todos_as_done(merge_request) todo_service.resolve_todos_for_target(merge_request, current_user) end - def create_event(merge_request) - event_service.approve_mr(merge_request, current_user) - end + def execute_approval_hooks(merge_request, current_user) + # Only one approval is required for a merge request to be approved + notification_service.async.approve_mr(merge_request, current_user) - def stream_audit_event(merge_request) - # Defined in EE + execute_hooks(merge_request, 'approved') end end end diff --git a/app/services/merge_requests/create_approval_event_service.rb b/app/services/merge_requests/create_approval_event_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..1678bf31139142010520b6aefb2af03c2975aaae --- /dev/null +++ b/app/services/merge_requests/create_approval_event_service.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module MergeRequests + class CreateApprovalEventService < MergeRequests::BaseService + def execute(merge_request) + event_service.approve_mr(merge_request, current_user) + end + end +end + +MergeRequests::CreateApprovalEventService.prepend_mod diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 22503531008b5d15a53c19c681e123b2ea1444d4..bdd50f69e6b8505230ace24ec682ae0d45d8bd30 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -2560,6 +2560,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: merge_requests_create_approval_event + :worker_name: MergeRequests::CreateApprovalEventWorker + :feature_category: :code_review + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: merge_requests_delete_source_branch :worker_name: MergeRequests::DeleteSourceBranchWorker :feature_category: :source_code_management diff --git a/app/workers/merge_requests/create_approval_event_worker.rb b/app/workers/merge_requests/create_approval_event_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..9b1a3c262e47bd4e9a0e5ff7b8e372b18bf7cac5 --- /dev/null +++ b/app/workers/merge_requests/create_approval_event_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module MergeRequests + class CreateApprovalEventWorker + include Gitlab::EventStore::Subscriber + + data_consistency :always + feature_category :code_review + urgency :low + idempotent! + + def handle_event(event) + current_user_id = event.data[:current_user_id] + merge_request_id = event.data[:merge_request_id] + current_user = User.find_by_id(current_user_id) + + unless current_user + logger.info(structured_payload(message: 'Current user not found.', current_user_id: current_user_id)) + return + end + + merge_request = MergeRequest.find_by_id(merge_request_id) + + unless merge_request + logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id)) + return + end + + ::MergeRequests::CreateApprovalEventService + .new(project: merge_request.project, current_user: current_user) + .execute(merge_request) + end + end +end diff --git a/config/feature_flags/development/async_after_approval.yml b/config/feature_flags/development/async_after_approval.yml new file mode 100644 index 0000000000000000000000000000000000000000..db53454b88f5200eacd43d50c4394d96acaab9b2 --- /dev/null +++ b/config/feature_flags/development/async_after_approval.yml @@ -0,0 +1,8 @@ +--- +name: async_after_approval +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92520 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/368098 +milestone: '15.3' +type: development +group: group::code review +default_enabled: false diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 73d0212b5fe41a981e069e86120f24596cfc86cc..b4e2071668e1eb273855611080e8d94e3f2c2c2b 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -277,6 +277,8 @@ - 1 - - merge_requests_close_issue - 1 +- - merge_requests_create_approval_event + - 1 - - merge_requests_delete_source_branch - 1 - - merge_requests_handle_assignees_change diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb index b9c7526f45aa859c56777d38cd84f52da81c86f1..c2fd157412c3866b88372bd9f9c117090aa1fde7 100644 --- a/ee/app/services/ee/merge_requests/approval_service.rb +++ b/ee/app/services/ee/merge_requests/approval_service.rb @@ -16,6 +16,11 @@ def execute(merge_request) private + def incorrect_approval_password?(merge_request) + merge_request.project.require_password_to_approve? && + !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password]) + end + override :can_be_approved? def can_be_approved?(merge_request) merge_request.can_approve?(current_user) @@ -26,22 +31,12 @@ def reset_approvals_cache(merge_request) merge_request.reset_approval_cache! end - override :execute_approval_hooks - def execute_approval_hooks(merge_request, current_user) - if merge_request.approvals_left == 0 - notification_service.async.approve_mr(merge_request, current_user) - execute_hooks(merge_request, 'approved') - else - execute_hooks(merge_request, 'approval') - end - end - override :create_event def create_event(merge_request) # Making sure MergeRequest::Metrics updates are in sync with # Event creation. ::Event.transaction do - event_service.approve_mr(merge_request, current_user) + super calculate_approvals_metrics(merge_request) end end @@ -60,13 +55,17 @@ def stream_audit_event(merge_request) ::Gitlab::Audit::Auditor.audit(audit_context) end - def incorrect_approval_password?(merge_request) - merge_request.project.require_password_to_approve? && - !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password]) + override :execute_approval_hooks + def execute_approval_hooks(merge_request, current_user) + if merge_request.approvals_left == 0 + super + else + execute_hooks(merge_request, 'approval') + end end def calculate_approvals_metrics(merge_request) - return unless merge_request.project.feature_available?(:code_review_analytics) + return unless merge_request.project.licensed_feature_available?(:code_review_analytics) ::Analytics::RefreshApprovalsData.new(merge_request).execute end diff --git a/ee/app/services/ee/merge_requests/create_approval_event_service.rb b/ee/app/services/ee/merge_requests/create_approval_event_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..fc7a719c3cc1c787a5bfbe88b23eef8af1ffa70f --- /dev/null +++ b/ee/app/services/ee/merge_requests/create_approval_event_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module CreateApprovalEventService + extend ::Gitlab::Utils::Override + + override :execute + def execute(merge_request) + # Making sure MergeRequest::Metrics updates are in sync with + # Event creation. + ::Event.transaction do + super + calculate_approvals_metrics(merge_request) + end + end + + private + + def calculate_approvals_metrics(merge_request) + return unless merge_request.project.licensed_feature_available?(:code_review_analytics) + + ::Analytics::RefreshApprovalsData.new(merge_request).execute + end + end + end +end diff --git a/ee/spec/services/ee/merge_requests/create_approval_event_service_spec.rb b/ee/spec/services/ee/merge_requests/create_approval_event_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..29bb8c9781537a43a36efd7cc1c4e607e8a77631 --- /dev/null +++ b/ee/spec/services/ee/merge_requests/create_approval_event_service_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CreateApprovalEventService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.project } + + subject(:service) { described_class.new(project: project, current_user: user) } + + describe '#execute' do + it 'creates approve MR event' do + expect_next_instance_of(EventCreateService) do |instance| + expect(instance).to receive(:approve_mr) + .with(merge_request, user) + end + + service.execute(merge_request) + end + + context 'for approvals metrics calculation' do + context 'when code_review_analytics project feature is available' do + before do + stub_licensed_features(code_review_analytics: true) + end + + it 'schedules RefreshApprovalsData' do + expect(::Analytics::RefreshApprovalsData) + .to receive_message_chain(:new, :execute) + + service.execute(merge_request) + end + 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 RefreshApprovalsData' do + expect(::Analytics::RefreshApprovalsData).not_to receive(:new) + + service.execute(merge_request) + end + end + end + end +end diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb index a47025fd1b5f306186eb48a1c986c0f9eb3ba355..f4959961357104bd8480825caf98279fe516bcf5 100644 --- a/ee/spec/services/merge_requests/approval_service_spec.rb +++ b/ee/spec/services/merge_requests/approval_service_spec.rb @@ -20,20 +20,20 @@ allow(merge_request.approvals).to receive(:new).and_return(double(save: false)) end - it 'does not create an approval note' do - expect(SystemNoteService).not_to receive(:approve_mr) + it 'does not reset approvals cache' do + expect(merge_request).not_to receive(:reset_approval_cache!) service.execute(merge_request) end + end - it 'does not mark pending todos as done' do - service.execute(merge_request) + context 'with valid approval' do + it 'resets the cache for approvals' do + expect(merge_request).to receive(:reset_approval_cache!) - expect(todo.reload).to be_pending + service.execute(merge_request) end - end - context 'with valid approval' do it 'creates an approval note' do allow(merge_request).to receive(:approvals_left).and_return(1) @@ -50,21 +50,6 @@ expect(todo.reload).to be_done end - it 'resets the cache for approvals' do - expect(merge_request).to receive(:reset_approval_cache!) - - service.execute(merge_request) - end - - it 'creates approve MR event' do - expect_next_instance_of(EventCreateService) do |instance| - expect(instance).to receive(:approve_mr) - .with(merge_request, user) - end - - service.execute(merge_request) - end - it 'sends the audit streaming event' do audit_context = { name: 'merge_request_approval_operation', @@ -117,29 +102,44 @@ end end - context 'approvals metrics calculation' do - context 'when code_review_analytics project feature is available' do - before do - stub_licensed_features(code_review_analytics: true) - end - - it 'schedules RefreshApprovalsData' do - expect(::Analytics::RefreshApprovalsData) - .to receive_message_chain(:new, :execute) + context 'async_after_approval feature flag is disabled' do + before do + stub_feature_flags(async_after_approval: false) + end - service.execute(merge_request) + it 'creates approve MR event' do + expect_next_instance_of(EventCreateService) do |instance| + expect(instance).to receive(:approve_mr) + .with(merge_request, user) end + + service.execute(merge_request) end - context 'when code_review_analytics is not available' do - before do - stub_licensed_features(code_review_analytics: false) + context 'approvals metrics calculation' do + context 'when code_review_analytics project feature is available' do + before do + stub_licensed_features(code_review_analytics: true) + end + + it 'schedules RefreshApprovalsData' do + expect(::Analytics::RefreshApprovalsData) + .to receive_message_chain(:new, :execute) + + service.execute(merge_request) + end end - it 'does not schedule for RefreshApprovalsData' do - expect(::Analytics::RefreshApprovalsData).not_to receive(:new) + context 'when code_review_analytics is not available' do + before do + stub_licensed_features(code_review_analytics: false) + end + + it 'does not schedule for RefreshApprovalsData' do + expect(::Analytics::RefreshApprovalsData).not_to receive(:new) - service.execute(merge_request) + service.execute(merge_request) + end end end end diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb index cb2cfa4b2b569e43380aa083f7c79ac185474f93..f85fff6ab4c07b18cf31bc7c7448145d067cb7e1 100644 --- a/lib/gitlab/event_store.rb +++ b/lib/gitlab/event_store.rb @@ -41,6 +41,7 @@ def self.configure!(store) store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::Projects::ProjectDeletedEvent store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::Projects::ProjectCreatedEvent store.subscribe ::Pages::InvalidateDomainCacheWorker, to: ::Projects::ProjectPathChangedEvent + store.subscribe ::MergeRequests::CreateApprovalEventWorker, to: ::MergeRequests::ApprovedEvent end private_class_method :configure! end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index e1fbb945ee3ff63055be70aebf2464fc1a2c8b40..1a0ad3118d5fee6f2db588034fdaa2f127cf1cb3 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -20,6 +20,23 @@ allow(merge_request.approvals).to receive(:new).and_return(double(save: false)) end + it 'does not reset approvals' do + expect(merge_request.approvals).not_to receive(:reset) + + service.execute(merge_request) + end + + it 'does not track merge request approve action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_approve_mr_action).with(user: user) + + service.execute(merge_request) + end + + it 'does not publish MergeRequests::ApprovedEvent' do + expect { service.execute(merge_request) }.not_to publish_event(MergeRequests::ApprovedEvent) + end + it 'does not create an approval note' do expect(SystemNoteService).not_to receive(:approve_mr) @@ -32,11 +49,16 @@ expect(todo.reload).to be_pending end - it 'does not track merge request approve action' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .not_to receive(:track_approve_mr_action).with(user: user) + context 'async_after_approval feature flag is disabled' do + before do + stub_feature_flags(async_after_approval: false) + end - service.execute(merge_request) + it 'does not create approve MR event' do + expect(EventCreateService).not_to receive(:new) + + service.execute(merge_request) + end end end @@ -47,22 +69,31 @@ 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) + it 'resets approvals' do expect(merge_request.approvals).to receive(:reset) service.execute(merge_request) + end - expect(todo.reload).to be_done + it 'tracks merge request approve action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_approve_mr_action).with(user: user, merge_request: merge_request) + + service.execute(merge_request) end - it 'creates approve MR event' do - expect_next_instance_of(EventCreateService) do |instance| - expect(instance).to receive(:approve_mr) - .with(merge_request, user) - end + it 'publishes MergeRequests::ApprovedEvent' do + expect { service.execute(merge_request) } + .to publish_event(MergeRequests::ApprovedEvent) + .with(current_user_id: user.id, merge_request_id: merge_request.id) + end + + it 'creates an approval note and marks pending todos as done' do + expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) service.execute(merge_request) + + expect(todo.reload).to be_done end it 'sends a notification when approving' do @@ -88,11 +119,20 @@ end end - it 'tracks merge request approve action' do - expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .to receive(:track_approve_mr_action).with(user: user, merge_request: merge_request) + context 'async_after_approval feature flag is disabled' do + before do + stub_feature_flags(async_after_approval: false) + allow(service).to receive(:notification_service).and_return(notification_service) + end - service.execute(merge_request) + it 'creates approve MR event' do + expect_next_instance_of(EventCreateService) do |instance| + expect(instance).to receive(:approve_mr) + .with(merge_request, user) + end + + service.execute(merge_request) + end end end diff --git a/spec/services/merge_requests/create_approval_event_service_spec.rb b/spec/services/merge_requests/create_approval_event_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..3d41ace11a7a8e090b3fd0a0a9615912e95c4074 --- /dev/null +++ b/spec/services/merge_requests/create_approval_event_service_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CreateApprovalEventService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.project } + + subject(:service) { described_class.new(project: project, current_user: user) } + + describe '#execute' do + it 'creates approve MR event' do + expect_next_instance_of(EventCreateService) do |instance| + expect(instance).to receive(:approve_mr) + .with(merge_request, user) + end + + service.execute(merge_request) + end + end +end diff --git a/spec/workers/merge_requests/create_approval_event_worker_spec.rb b/spec/workers/merge_requests/create_approval_event_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..8389949ecc9696be09b29c24b2914fa1e6ab1df3 --- /dev/null +++ b/spec/workers/merge_requests/create_approval_event_worker_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CreateApprovalEventWorker do + let!(:user) { create(:user) } + let!(:project) { create(:project) } + let!(:merge_request) { create(:merge_request, source_project: project) } + let(:data) { { current_user_id: user.id, merge_request_id: merge_request.id } } + let(:approved_event) { MergeRequests::ApprovedEvent.new(data: data) } + + it_behaves_like 'subscribes to event' do + let(:event) { approved_event } + end + + it 'calls MergeRequests::CreateApprovalEventService' do + expect_next_instance_of( + MergeRequests::CreateApprovalEventService, + project: project, current_user: user + ) do |service| + expect(service).to receive(:execute).with(merge_request) + end + + consume_event(subscriber: described_class, event: approved_event) + end + + shared_examples 'when object does not exist' do + it 'does not call MergeRequests::CreateApprovalEventService' do + expect(MergeRequests::CreateApprovalEventService).not_to receive(:new) + + expect { consume_event(subscriber: described_class, event: approved_event) } + .not_to raise_exception + end + end + + context 'when the user does not exist' do + before do + user.destroy! + end + + it_behaves_like 'when object does not exist' + end + + context 'when the merge request does not exist' do + before do + merge_request.destroy! + end + + it_behaves_like 'when object does not exist' + end +end