From 4a71a493d14d7b442543ead2d3c514ffb89ef8f3 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Tue, 19 Jul 2022 13:49:38 +0800 Subject: [PATCH 1/3] Execute side effects of merge request approval async Before this, `MergeRequests::ApprovalService` will execute a lot of SQL queries. When called with other services in a single web request, that can lead 100+ SQL queries in a single queries. That exceeds our query limit of 100. This moves the side effects of approvals to be performed async via Sidekiq. As a result, this reduces the number of SQL queries executed in the service since it's offloaded to Sidekiq. `Gitlab::EventStore` is utilized to make it easier in the future to add side effects and opens the possiblity to refactor the `MergeRequests::AfterApprovalWorker` to be split into smaller subscribers. This is behind `async_after_approval` feature flag. --- app/events/merge_requests/approved_event.rb | 19 +++ .../merge_requests/after_approval_service.rb | 44 +++++++ .../merge_requests/approval_service.rb | 46 +++---- app/workers/all_queues.yml | 9 ++ .../merge_requests/after_approval_worker.rb | 37 ++++++ .../development/async_after_approval.yml | 8 ++ config/sidekiq_queues.yml | 2 + .../merge_requests/after_approval_service.rb | 51 ++++++++ .../ee/merge_requests/approval_service.rb | 40 ------ .../after_approval_service_spec.rb | 118 ++++++++++++++++++ .../merge_requests/approval_service_spec.rb | 115 +---------------- lib/gitlab/event_store.rb | 1 + .../after_approval_service_spec.rb | 60 +++++++++ .../merge_requests/approval_service_spec.rb | 80 ++++++------ .../after_approval_worker_spec.rb | 49 ++++++++ 15 files changed, 453 insertions(+), 226 deletions(-) create mode 100644 app/events/merge_requests/approved_event.rb create mode 100644 app/services/merge_requests/after_approval_service.rb create mode 100644 app/workers/merge_requests/after_approval_worker.rb create mode 100644 config/feature_flags/development/async_after_approval.yml create mode 100644 ee/app/services/ee/merge_requests/after_approval_service.rb create mode 100644 ee/spec/services/ee/merge_requests/after_approval_service_spec.rb create mode 100644 spec/services/merge_requests/after_approval_service_spec.rb create mode 100644 spec/workers/merge_requests/after_approval_worker_spec.rb diff --git a/app/events/merge_requests/approved_event.rb b/app/events/merge_requests/approved_event.rb new file mode 100644 index 00000000000000..c68a002dcc3787 --- /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/after_approval_service.rb b/app/services/merge_requests/after_approval_service.rb new file mode 100644 index 00000000000000..be039b587ece1a --- /dev/null +++ b/app/services/merge_requests/after_approval_service.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +# This service must be called in a worker as this service executes a number of +# SQL queries. When called in a web request, this can lead to slower web requests +# due to the number of SQL queries this service executes. +module MergeRequests + class AfterApprovalService < MergeRequests::BaseService + def execute(merge_request) + create_event(merge_request) + 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) + end + + private + + def create_event(merge_request) + event_service.approve_mr(merge_request, current_user) + end + + def stream_audit_event(merge_request) + # Defined in EE + end + + def create_approval_note(merge_request) + SystemNoteService.approve_mr(merge_request, current_user) + end + + def mark_pending_todos_as_done(merge_request) + todo_service.resolve_todos_for_target(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) + + execute_hooks(merge_request, 'approved') + end + end +end + +MergeRequests::AfterApprovalService.prepend_mod diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index b8d817a15f310f..ac0a5085e304db 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -10,14 +10,25 @@ def execute(merge_request) return success unless save_approval(approval) reset_approvals_cache(merge_request) - create_event(merge_request) - 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) + # 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 + MergeRequests::AfterApprovalService + .new(project: project, current_user: current_user) + .execute(merge_request) + end + success end @@ -31,34 +42,11 @@ 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') - end - def save_approval(approval) Approval.safe_ensure_unique do approval.save end end - - def create_approval_note(merge_request) - SystemNoteService.approve_mr(merge_request, current_user) - end - - 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 stream_audit_event(merge_request) - # Defined in EE - end end end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 22503531008b5d..f9ba88a399f2d4 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -2551,6 +2551,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: merge_requests_after_approval + :worker_name: MergeRequests::AfterApprovalWorker + :feature_category: :code_review + :has_external_dependencies: true + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: merge_requests_close_issue :worker_name: MergeRequests::CloseIssueWorker :feature_category: :code_review diff --git a/app/workers/merge_requests/after_approval_worker.rb b/app/workers/merge_requests/after_approval_worker.rb new file mode 100644 index 00000000000000..381a4074c18e14 --- /dev/null +++ b/app/workers/merge_requests/after_approval_worker.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module MergeRequests + class AfterApprovalWorker + include Gitlab::EventStore::Subscriber + + data_consistency :always + feature_category :code_review + urgency :low + idempotent! + + # MergeRequests:AfterApprovalService execute webhooks which are treated as external dependencies + worker_has_external_dependencies! + + 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::AfterApprovalService + .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 00000000000000..db53454b88f520 --- /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 73d0212b5fe41a..96de4f4e349f43 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -275,6 +275,8 @@ - 1 - - merge_request_reset_approvals - 1 +- - merge_requests_after_approval + - 1 - - merge_requests_close_issue - 1 - - merge_requests_delete_source_branch diff --git a/ee/app/services/ee/merge_requests/after_approval_service.rb b/ee/app/services/ee/merge_requests/after_approval_service.rb new file mode 100644 index 00000000000000..2f19ffddc1c5ec --- /dev/null +++ b/ee/app/services/ee/merge_requests/after_approval_service.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module EE + module MergeRequests + module AfterApprovalService + extend ::Gitlab::Utils::Override + + private + + 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) + calculate_approvals_metrics(merge_request) + end + end + + override :stream_audit_event + def stream_audit_event(merge_request) + audit_context = { + name: 'merge_request_approval_operation', + stream_only: true, + author: current_user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + 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 + + 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/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb index b9c7526f45aa85..c3a7a1918fd5f8 100644 --- a/ee/app/services/ee/merge_requests/approval_service.rb +++ b/ee/app/services/ee/merge_requests/approval_service.rb @@ -26,50 +26,10 @@ 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) - calculate_approvals_metrics(merge_request) - end - end - - override :stream_audit_event - def stream_audit_event(merge_request) - audit_context = { - name: 'merge_request_approval_operation', - stream_only: true, - author: current_user, - scope: merge_request.project, - target: merge_request, - message: 'Approved 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]) end - - def calculate_approvals_metrics(merge_request) - return unless merge_request.project.feature_available?(:code_review_analytics) - - ::Analytics::RefreshApprovalsData.new(merge_request).execute - end end end end diff --git a/ee/spec/services/ee/merge_requests/after_approval_service_spec.rb b/ee/spec/services/ee/merge_requests/after_approval_service_spec.rb new file mode 100644 index 00000000000000..28fa336bc3a862 --- /dev/null +++ b/ee/spec/services/ee/merge_requests/after_approval_service_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::AfterApprovalService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request, reviewers: [user]) } + let(:project) { merge_request.project } + let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } + + subject(:service) { described_class.new(project: project, current_user: user) } + + describe '#execute' do + it 'creates an approval note' do + allow(merge_request).to receive(:approvals_left).and_return(1) + + expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) + + service.execute(merge_request) + end + + it 'marks pending todos as done' do + allow(merge_request).to receive(:approvals_left).and_return(1) + + service.execute(merge_request) + + expect(todo.reload).to be_done + 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', + stream_only: true, + author: user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + + service.execute(merge_request) + end + + context 'with remaining approvals' do + it 'fires an approval webhook' do + expect(merge_request).to receive(:approvals_left).and_return(5) + expect(service).to receive(:execute_hooks).with(merge_request, 'approval') + + service.execute(merge_request) + end + + it 'does not send an email' do + expect(merge_request).to receive(:approvals_left).and_return(5) + expect(service).not_to receive(:notification_service) + + service.execute(merge_request) + end + end + + context 'with required approvals' do + let(:notification_service) { NotificationService.new } + + before do + expect(merge_request).to receive(:approvals_left).and_return(0) + allow(service).to receive(:execute_hooks) + allow(service).to receive(:notification_service).and_return(notification_service) + end + + it 'fires an approved webhook' do + expect(service).to receive(:execute_hooks).with(merge_request, 'approved') + + service.execute(merge_request) + end + + it 'sends an email' do + expect(notification_service).to receive_message_chain(:async, :approve_mr).with(merge_request, user) + + service.execute(merge_request) + 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) + + 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 a47025fd1b5f30..289c209047dabf 100644 --- a/ee/spec/services/merge_requests/approval_service_spec.rb +++ b/ee/spec/services/merge_requests/approval_service_spec.rb @@ -7,7 +7,6 @@ let(:user) { create(:user) } let(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } - let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } subject(:service) { described_class.new(project: project, current_user: user) } @@ -20,129 +19,19 @@ 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 - - it 'does not mark pending todos as done' do - service.execute(merge_request) - - expect(todo.reload).to be_pending - end end context 'with valid approval' do - it 'creates an approval note' do - allow(merge_request).to receive(:approvals_left).and_return(1) - - expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) - - service.execute(merge_request) - end - - it 'marks pending todos as done' do - allow(merge_request).to receive(:approvals_left).and_return(1) - - service.execute(merge_request) - - 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', - stream_only: true, - author: user, - scope: merge_request.project, - target: merge_request, - message: 'Approved merge request' - } - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) - - service.execute(merge_request) - end - - context 'with remaining approvals' do - it 'fires an approval webhook' do - expect(merge_request).to receive(:approvals_left).and_return(5) - expect(service).to receive(:execute_hooks).with(merge_request, 'approval') - - service.execute(merge_request) - end - - it 'does not send an email' do - expect(merge_request).to receive(:approvals_left).and_return(5) - expect(service).not_to receive(:notification_service) - - service.execute(merge_request) - end - end - - context 'with required approvals' do - let(:notification_service) { NotificationService.new } - - before do - expect(merge_request).to receive(:approvals_left).and_return(0) - allow(service).to receive(:execute_hooks) - allow(service).to receive(:notification_service).and_return(notification_service) - end - - it 'fires an approved webhook' do - expect(service).to receive(:execute_hooks).with(merge_request, 'approved') - - service.execute(merge_request) - end - - it 'sends an email' do - expect(notification_service).to receive_message_chain(:async, :approve_mr).with(merge_request, user) - - service.execute(merge_request) - 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) - - 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 context 'when project requires force auth for approval' do diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb index cb2cfa4b2b569e..cf578e78f9651b 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::AfterApprovalWorker, to: ::MergeRequests::ApprovedEvent end private_class_method :configure! end diff --git a/spec/services/merge_requests/after_approval_service_spec.rb b/spec/services/merge_requests/after_approval_service_spec.rb new file mode 100644 index 00000000000000..32afc3a7bc6a91 --- /dev/null +++ b/spec/services/merge_requests/after_approval_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::AfterApprovalService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request, reviewers: [user]) } + let(:project) { merge_request.project } + let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } + + subject(:service) { described_class.new(project: project, current_user: user) } + + describe '#execute' 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) + + service.execute(merge_request) + + expect(todo.reload).to be_done + 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 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, user: user) + .and_call_original + + service.execute(merge_request) + end + + context 'with remaining approvals' do + it 'fires an approval webhook' do + expect(service).to receive(:execute_hooks).with(merge_request, 'approved') + + service.execute(merge_request) + end + end + end +end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index e1fbb945ee3ff6..bf81e7fcc6cc4b 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -7,7 +7,6 @@ let(:user) { create(:user) } let(:merge_request) { create(:merge_request, reviewers: [user]) } let(:project) { merge_request.project } - let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } subject(:service) { described_class.new(project: project, current_user: user) } @@ -20,79 +19,72 @@ 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' do + expect(merge_request.approvals).not_to receive(:reset) service.execute(merge_request) end - it 'does not mark pending todos as done' do - service.execute(merge_request) - - 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) service.execute(merge_request) end - end - - context 'with valid approval' do - let(:notification_service) { NotificationService.new } - before do - allow(service).to receive(:notification_service).and_return(notification_service) + it 'does not publish MergeRequests::ApprovedEvent' do + expect { service.execute(merge_request) }.not_to publish_event(MergeRequests::ApprovedEvent) 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) - - service.execute(merge_request) + context 'async_after_approval feature flag is disabled' do + before do + stub_feature_flags(async_after_approval: false) + end - expect(todo.reload).to be_done - end + it 'does not call after approval service' do + expect(MergeRequests::AfterApprovalService).not_to receive(:new) - it 'creates approve MR event' do - expect_next_instance_of(EventCreateService) do |instance| - expect(instance).to receive(:approve_mr) - .with(merge_request, user) + service.execute(merge_request) end - - service.execute(merge_request) end + end - it 'sends a notification when approving' do - expect(notification_service).to receive_message_chain(:async, :approve_mr) - .with(merge_request, user) + context 'with valid approval' do + it 'resets approvals' do + expect(merge_request.approvals).to receive(:reset) 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, user: user) - .and_call_original + 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 - context 'with remaining approvals' do - it 'fires an approval webhook' do - expect(service).to receive(:execute_hooks).with(merge_request, 'approved') + 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 - service.execute(merge_request) + context 'async_after_approval feature flag is disabled' do + before do + stub_feature_flags(async_after_approval: false) 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) + it 'calls after approval service' do + expect_next_instance_of( + MergeRequests::AfterApprovalService, + project: project, + current_user: user + ) do |after_approval_service| + expect(after_approval_service).to receive(:execute).with(merge_request) + end - service.execute(merge_request) + service.execute(merge_request) + end end end diff --git a/spec/workers/merge_requests/after_approval_worker_spec.rb b/spec/workers/merge_requests/after_approval_worker_spec.rb new file mode 100644 index 00000000000000..7a9fe9c453e0b8 --- /dev/null +++ b/spec/workers/merge_requests/after_approval_worker_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::AfterApprovalWorker do + let!(:user) { create(:user) } + let!(:project) { create(:project) } + let!(:issue) { create(:issue, project: 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 the merge requests after approval service' do + expect_next_instance_of(MergeRequests::AfterApprovalService, 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 the merge requests approval service' do + expect(MergeRequests::ApprovalService).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 -- GitLab From 3981e154a571a9d86d5040743c01992b5b389ed4 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Thu, 21 Jul 2022 18:33:04 +0800 Subject: [PATCH 2/3] Implement CreateApprovalEvent worker/service This is a start of splitting approval side effects into smaller subscribers to `MergeRequests::ApprovedEvent`. The idea is to move each side effect into its own worker and ensure that it's idempotent as event store subscribers should be idempotent. --- .../merge_requests/after_approval_service.rb | 44 ------- .../merge_requests/approval_service.rb | 41 ++++-- .../create_approval_event_service.rb | 11 ++ app/workers/all_queues.yml | 10 +- ...ker.rb => create_approval_event_worker.rb} | 7 +- config/sidekiq_queues.yml | 4 +- .../merge_requests/after_approval_service.rb | 51 -------- .../ee/merge_requests/approval_service.rb | 46 ++++++- .../create_approval_event_service.rb | 27 ++++ .../after_approval_service_spec.rb | 118 ------------------ .../create_approval_event_service_spec.rb | 49 ++++++++ .../merge_requests/approval_service_spec.rb | 111 ++++++++++++++++ lib/gitlab/event_store.rb | 2 +- .../after_approval_service_spec.rb | 60 --------- .../merge_requests/approval_service_spec.rb | 66 ++++++++-- .../create_approval_event_service_spec.rb | 22 ++++ ...b => create_approval_event_worker_spec.rb} | 14 ++- 17 files changed, 372 insertions(+), 311 deletions(-) delete mode 100644 app/services/merge_requests/after_approval_service.rb create mode 100644 app/services/merge_requests/create_approval_event_service.rb rename app/workers/merge_requests/{after_approval_worker.rb => create_approval_event_worker.rb} (80%) delete mode 100644 ee/app/services/ee/merge_requests/after_approval_service.rb create mode 100644 ee/app/services/ee/merge_requests/create_approval_event_service.rb delete mode 100644 ee/spec/services/ee/merge_requests/after_approval_service_spec.rb create mode 100644 ee/spec/services/ee/merge_requests/create_approval_event_service_spec.rb delete mode 100644 spec/services/merge_requests/after_approval_service_spec.rb create mode 100644 spec/services/merge_requests/create_approval_event_service_spec.rb rename spec/workers/merge_requests/{after_approval_worker_spec.rb => create_approval_event_worker_spec.rb} (72%) diff --git a/app/services/merge_requests/after_approval_service.rb b/app/services/merge_requests/after_approval_service.rb deleted file mode 100644 index be039b587ece1a..00000000000000 --- a/app/services/merge_requests/after_approval_service.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -# This service must be called in a worker as this service executes a number of -# SQL queries. When called in a web request, this can lead to slower web requests -# due to the number of SQL queries this service executes. -module MergeRequests - class AfterApprovalService < MergeRequests::BaseService - def execute(merge_request) - create_event(merge_request) - 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) - end - - private - - def create_event(merge_request) - event_service.approve_mr(merge_request, current_user) - end - - def stream_audit_event(merge_request) - # Defined in EE - end - - def create_approval_note(merge_request) - SystemNoteService.approve_mr(merge_request, current_user) - end - - def mark_pending_todos_as_done(merge_request) - todo_service.resolve_todos_for_target(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) - - execute_hooks(merge_request, 'approved') - end - end -end - -MergeRequests::AfterApprovalService.prepend_mod diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index ac0a5085e304db..f26513703055a8 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -24,11 +24,15 @@ def execute(merge_request) ) ) else - MergeRequests::AfterApprovalService - .new(project: project, current_user: current_user) - .execute(merge_request) + 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) + success end @@ -38,15 +42,38 @@ def can_be_approved?(merge_request) current_user.can?(:approve_merge_request, merge_request) end - def reset_approvals_cache(merge_request) - merge_request.approvals.reset - 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 create_event(merge_request) + event_service.approve_mr(merge_request, current_user) + end + + def stream_audit_event(merge_request) + # Defined in EE + end + + def create_approval_note(merge_request) + SystemNoteService.approve_mr(merge_request, current_user) + end + + def mark_pending_todos_as_done(merge_request) + todo_service.resolve_todos_for_target(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) + + 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 00000000000000..1678bf31139142 --- /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 f9ba88a399f2d4..bdd50f69e6b850 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -2551,8 +2551,8 @@ :weight: 1 :idempotent: true :tags: [] -- :name: merge_requests_after_approval - :worker_name: MergeRequests::AfterApprovalWorker +- :name: merge_requests_close_issue + :worker_name: MergeRequests::CloseIssueWorker :feature_category: :code_review :has_external_dependencies: true :urgency: :low @@ -2560,10 +2560,10 @@ :weight: 1 :idempotent: true :tags: [] -- :name: merge_requests_close_issue - :worker_name: MergeRequests::CloseIssueWorker +- :name: merge_requests_create_approval_event + :worker_name: MergeRequests::CreateApprovalEventWorker :feature_category: :code_review - :has_external_dependencies: true + :has_external_dependencies: false :urgency: :low :resource_boundary: :unknown :weight: 1 diff --git a/app/workers/merge_requests/after_approval_worker.rb b/app/workers/merge_requests/create_approval_event_worker.rb similarity index 80% rename from app/workers/merge_requests/after_approval_worker.rb rename to app/workers/merge_requests/create_approval_event_worker.rb index 381a4074c18e14..9b1a3c262e47bd 100644 --- a/app/workers/merge_requests/after_approval_worker.rb +++ b/app/workers/merge_requests/create_approval_event_worker.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module MergeRequests - class AfterApprovalWorker + class CreateApprovalEventWorker include Gitlab::EventStore::Subscriber data_consistency :always @@ -9,9 +9,6 @@ class AfterApprovalWorker urgency :low idempotent! - # MergeRequests:AfterApprovalService execute webhooks which are treated as external dependencies - worker_has_external_dependencies! - def handle_event(event) current_user_id = event.data[:current_user_id] merge_request_id = event.data[:merge_request_id] @@ -29,7 +26,7 @@ def handle_event(event) return end - ::MergeRequests::AfterApprovalService + ::MergeRequests::CreateApprovalEventService .new(project: merge_request.project, current_user: current_user) .execute(merge_request) end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 96de4f4e349f43..b4e2071668e1eb 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -275,10 +275,10 @@ - 1 - - merge_request_reset_approvals - 1 -- - merge_requests_after_approval - - 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/after_approval_service.rb b/ee/app/services/ee/merge_requests/after_approval_service.rb deleted file mode 100644 index 2f19ffddc1c5ec..00000000000000 --- a/ee/app/services/ee/merge_requests/after_approval_service.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -module EE - module MergeRequests - module AfterApprovalService - extend ::Gitlab::Utils::Override - - private - - 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) - calculate_approvals_metrics(merge_request) - end - end - - override :stream_audit_event - def stream_audit_event(merge_request) - audit_context = { - name: 'merge_request_approval_operation', - stream_only: true, - author: current_user, - scope: merge_request.project, - target: merge_request, - message: 'Approved merge request' - } - - ::Gitlab::Audit::Auditor.audit(audit_context) - 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 - - 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/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb index c3a7a1918fd5f8..a3a3637e60261a 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,9 +31,44 @@ def reset_approvals_cache(merge_request) merge_request.reset_approval_cache! 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 :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) + calculate_approvals_metrics(merge_request) + end + end + + override :stream_audit_event + def stream_audit_event(merge_request) + audit_context = { + name: 'merge_request_approval_operation', + stream_only: true, + author: current_user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + 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 + + 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 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 00000000000000..94ea2cb833515e --- /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 + event_service.approve_mr(merge_request, current_user) + 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/after_approval_service_spec.rb b/ee/spec/services/ee/merge_requests/after_approval_service_spec.rb deleted file mode 100644 index 28fa336bc3a862..00000000000000 --- a/ee/spec/services/ee/merge_requests/after_approval_service_spec.rb +++ /dev/null @@ -1,118 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe MergeRequests::AfterApprovalService do - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [user]) } - let(:project) { merge_request.project } - let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } - - subject(:service) { described_class.new(project: project, current_user: user) } - - describe '#execute' do - it 'creates an approval note' do - allow(merge_request).to receive(:approvals_left).and_return(1) - - expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) - - service.execute(merge_request) - end - - it 'marks pending todos as done' do - allow(merge_request).to receive(:approvals_left).and_return(1) - - service.execute(merge_request) - - expect(todo.reload).to be_done - 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', - stream_only: true, - author: user, - scope: merge_request.project, - target: merge_request, - message: 'Approved merge request' - } - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) - - service.execute(merge_request) - end - - context 'with remaining approvals' do - it 'fires an approval webhook' do - expect(merge_request).to receive(:approvals_left).and_return(5) - expect(service).to receive(:execute_hooks).with(merge_request, 'approval') - - service.execute(merge_request) - end - - it 'does not send an email' do - expect(merge_request).to receive(:approvals_left).and_return(5) - expect(service).not_to receive(:notification_service) - - service.execute(merge_request) - end - end - - context 'with required approvals' do - let(:notification_service) { NotificationService.new } - - before do - expect(merge_request).to receive(:approvals_left).and_return(0) - allow(service).to receive(:execute_hooks) - allow(service).to receive(:notification_service).and_return(notification_service) - end - - it 'fires an approved webhook' do - expect(service).to receive(:execute_hooks).with(merge_request, 'approved') - - service.execute(merge_request) - end - - it 'sends an email' do - expect(notification_service).to receive_message_chain(:async, :approve_mr).with(merge_request, user) - - service.execute(merge_request) - 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) - - 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/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 00000000000000..29bb8c9781537a --- /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 289c209047dabf..f4959961357104 100644 --- a/ee/spec/services/merge_requests/approval_service_spec.rb +++ b/ee/spec/services/merge_requests/approval_service_spec.rb @@ -7,6 +7,7 @@ let(:user) { create(:user) } let(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } + let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } subject(:service) { described_class.new(project: project, current_user: user) } @@ -32,6 +33,116 @@ service.execute(merge_request) end + + it 'creates an approval note' do + allow(merge_request).to receive(:approvals_left).and_return(1) + + expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user) + + service.execute(merge_request) + end + + it 'marks pending todos as done' do + allow(merge_request).to receive(:approvals_left).and_return(1) + + service.execute(merge_request) + + expect(todo.reload).to be_done + end + + it 'sends the audit streaming event' do + audit_context = { + name: 'merge_request_approval_operation', + stream_only: true, + author: user, + scope: merge_request.project, + target: merge_request, + message: 'Approved merge request' + } + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + + service.execute(merge_request) + end + + context 'with remaining approvals' do + it 'fires an approval webhook' do + expect(merge_request).to receive(:approvals_left).and_return(5) + expect(service).to receive(:execute_hooks).with(merge_request, 'approval') + + service.execute(merge_request) + end + + it 'does not send an email' do + expect(merge_request).to receive(:approvals_left).and_return(5) + expect(service).not_to receive(:notification_service) + + service.execute(merge_request) + end + end + + context 'with required approvals' do + let(:notification_service) { NotificationService.new } + + before do + expect(merge_request).to receive(:approvals_left).and_return(0) + allow(service).to receive(:execute_hooks) + allow(service).to receive(:notification_service).and_return(notification_service) + end + + it 'fires an approved webhook' do + expect(service).to receive(:execute_hooks).with(merge_request, 'approved') + + service.execute(merge_request) + end + + it 'sends an email' do + expect(notification_service).to receive_message_chain(:async, :approve_mr).with(merge_request, user) + + service.execute(merge_request) + end + end + + context 'async_after_approval feature flag is disabled' do + before do + stub_feature_flags(async_after_approval: false) + 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 + + 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 + + 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 context 'when project requires force auth for approval' do diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb index cf578e78f9651b..f85fff6ab4c07b 100644 --- a/lib/gitlab/event_store.rb +++ b/lib/gitlab/event_store.rb @@ -41,7 +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::AfterApprovalWorker, to: ::MergeRequests::ApprovedEvent + store.subscribe ::MergeRequests::CreateApprovalEventWorker, to: ::MergeRequests::ApprovedEvent end private_class_method :configure! end diff --git a/spec/services/merge_requests/after_approval_service_spec.rb b/spec/services/merge_requests/after_approval_service_spec.rb deleted file mode 100644 index 32afc3a7bc6a91..00000000000000 --- a/spec/services/merge_requests/after_approval_service_spec.rb +++ /dev/null @@ -1,60 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe MergeRequests::AfterApprovalService do - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [user]) } - let(:project) { merge_request.project } - let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } - - subject(:service) { described_class.new(project: project, current_user: user) } - - describe '#execute' 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) - - service.execute(merge_request) - - expect(todo.reload).to be_done - 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 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, user: user) - .and_call_original - - service.execute(merge_request) - end - - context 'with remaining approvals' do - it 'fires an approval webhook' do - expect(service).to receive(:execute_hooks).with(merge_request, 'approved') - - service.execute(merge_request) - end - end - end -end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index bf81e7fcc6cc4b..1a0ad3118d5fee 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -7,6 +7,7 @@ let(:user) { create(:user) } let(:merge_request) { create(:merge_request, reviewers: [user]) } let(:project) { merge_request.project } + let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } subject(:service) { described_class.new(project: project, current_user: user) } @@ -36,13 +37,25 @@ 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) + + service.execute(merge_request) + end + + it 'does not mark pending todos as done' do + service.execute(merge_request) + + expect(todo.reload).to be_pending + end + context 'async_after_approval feature flag is disabled' do before do stub_feature_flags(async_after_approval: false) end - it 'does not call after approval service' do - expect(MergeRequests::AfterApprovalService).not_to receive(:new) + it 'does not create approve MR event' do + expect(EventCreateService).not_to receive(:new) service.execute(merge_request) end @@ -50,6 +63,12 @@ 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 'resets approvals' do expect(merge_request.approvals).to receive(:reset) @@ -69,18 +88,47 @@ .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 + 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, user: user) + .and_call_original + + service.execute(merge_request) + end + + context 'with remaining approvals' do + it 'fires an approval webhook' do + expect(service).to receive(:execute_hooks).with(merge_request, 'approved') + + service.execute(merge_request) + end + end + 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 - it 'calls after approval service' do - expect_next_instance_of( - MergeRequests::AfterApprovalService, - project: project, - current_user: user - ) do |after_approval_service| - expect(after_approval_service).to receive(:execute).with(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) 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 00000000000000..3d41ace11a7a8e --- /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/after_approval_worker_spec.rb b/spec/workers/merge_requests/create_approval_event_worker_spec.rb similarity index 72% rename from spec/workers/merge_requests/after_approval_worker_spec.rb rename to spec/workers/merge_requests/create_approval_event_worker_spec.rb index 7a9fe9c453e0b8..8389949ecc9696 100644 --- a/spec/workers/merge_requests/after_approval_worker_spec.rb +++ b/spec/workers/merge_requests/create_approval_event_worker_spec.rb @@ -2,10 +2,9 @@ require 'spec_helper' -RSpec.describe MergeRequests::AfterApprovalWorker do +RSpec.describe MergeRequests::CreateApprovalEventWorker do let!(:user) { create(:user) } let!(:project) { create(:project) } - let!(:issue) { create(:issue, project: 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) } @@ -14,8 +13,11 @@ let(:event) { approved_event } end - it 'calls the merge requests after approval service' do - expect_next_instance_of(MergeRequests::AfterApprovalService, project: project, current_user: user) do |service| + 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 @@ -23,8 +25,8 @@ end shared_examples 'when object does not exist' do - it 'does not call the merge requests approval service' do - expect(MergeRequests::ApprovalService).not_to receive(:new) + 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 -- GitLab From c44f93b82fc22ba61bdd61821ee69dd605de69ab Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Thu, 28 Jul 2022 15:18:56 +0800 Subject: [PATCH 3/3] Fix coverage issue This follows the guide in https://docs.gitlab.com/ee/development/ee_features.html#use-self-descriptive-wrapper-methods when overriding to extend a method in EE that exists in CE. --- ee/app/services/ee/merge_requests/approval_service.rb | 5 ++--- .../ee/merge_requests/create_approval_event_service.rb | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb index a3a3637e60261a..c2fd157412c386 100644 --- a/ee/app/services/ee/merge_requests/approval_service.rb +++ b/ee/app/services/ee/merge_requests/approval_service.rb @@ -36,7 +36,7 @@ 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 @@ -58,8 +58,7 @@ def stream_audit_event(merge_request) 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') + super else execute_hooks(merge_request, 'approval') 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 index 94ea2cb833515e..fc7a719c3cc1c7 100644 --- a/ee/app/services/ee/merge_requests/create_approval_event_service.rb +++ b/ee/app/services/ee/merge_requests/create_approval_event_service.rb @@ -10,7 +10,7 @@ def execute(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 -- GitLab