diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb
index dcc4cf4bb1ec54f20630b0ce7ae68b5ecbd738f9..05bdc7dec9ce10b94031767b6494e669e1e6cb25 100644
--- a/app/services/merge_requests/approval_service.rb
+++ b/app/services/merge_requests/approval_service.rb
@@ -17,19 +17,11 @@ def execute(merge_request)
# 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 }
- )
+ Gitlab::EventStore.publish(
+ MergeRequests::ApprovedEvent.new(
+ data: { current_user_id: current_user.id, merge_request_id: merge_request.id }
)
- else
- 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)
- end
+ )
success
end
@@ -49,29 +41,6 @@ def save_approval(approval)
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/config/feature_flags/development/async_after_approval.yml b/config/feature_flags/development/async_after_approval.yml
deleted file mode 100644
index db53454b88f5200eacd43d50c4394d96acaab9b2..0000000000000000000000000000000000000000
--- a/config/feature_flags/development/async_after_approval.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-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/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index c2fd157412c3866b88372bd9f9c117090aa1fde7..19d7feb0d1ad668379cf9daa1e0fa79a4060d0b6 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -30,45 +30,6 @@ def can_be_approved?(merge_request)
def reset_approvals_cache(merge_request)
merge_request.reset_approval_cache!
end
-
- override :create_event
- def create_event(merge_request)
- # Making sure MergeRequest::Metrics updates are in sync with
- # Event creation.
- ::Event.transaction do
- super
- 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
- super
- 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/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb
index 56999a4e06a35098cb312dbc7b124b674617a442..165dda1093f472c5e61ab01870c1c42fab33b837 100644
--- a/ee/spec/services/merge_requests/approval_service_spec.rb
+++ b/ee/spec/services/merge_requests/approval_service_spec.rb
@@ -33,116 +33,6 @@
service.execute(merge_request)
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
-
- 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
-
- 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
-
- 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
context 'when project requires force auth for approval' do
diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb
index ab98fad5d73f7c3ed33211d29d7ca13334c78fb7..dd6c049028423fd382c1051fc6176461314123b7 100644
--- a/spec/services/merge_requests/approval_service_spec.rb
+++ b/spec/services/merge_requests/approval_service_spec.rb
@@ -36,30 +36,6 @@
it 'does not publish MergeRequests::ApprovedEvent' do
expect { service.execute(merge_request) }.not_to publish_event(MergeRequests::ApprovedEvent)
end
-
- context 'async_after_approval feature flag is disabled' do
- before do
- stub_feature_flags(async_after_approval: false)
- end
-
- it 'does not create approve MR event' do
- expect(EventCreateService).not_to receive(:new)
-
- service.execute(merge_request)
- 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
- end
end
context 'with valid approval' do
@@ -81,51 +57,6 @@
.to publish_event(MergeRequests::ApprovedEvent)
.with(current_user_id: user.id, merge_request_id: merge_request.id)
end
-
- context 'async_after_approval feature flag is disabled' do
- let(:notification_service) { NotificationService.new }
-
- before do
- stub_feature_flags(async_after_approval: false)
- allow(service).to receive(:notification_service).and_return(notification_service)
- 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 'creates an approval note' do
- expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user)
-
- service.execute(merge_request)
- end
-
- it 'marks pending todos as done' do
- 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
-
- 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
context 'user cannot update the merge request' do