Skip to content
代码片段 群组 项目
提交 ffdc7bd7 编辑于 作者: Patrick Bajao's avatar Patrick Bajao
浏览文件

Enable async_after_approval feature flag 

This removes the `async_after_approval` feature flag and keep the
new code.

Old code on ApprovalService has been removed as well since they
won't be used anymore.

Changelog: changed
上级 8ee50712
No related branches found
No related tags found
无相关合并请求
隐藏空白变更内容
行内 左右并排
app/services/merge_requests/approval_service.rb
+ 4
− 35
浏览文件 @ ffdc7bd7
......@@ -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
......
config/feature_flags/development/async_after_approval.yml 已删除 100644 → 0
+ 0
− 8
浏览文件 @ 8ee50712
---
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
ee/app/services/ee/merge_requests/approval_service.rb
+ 0
− 39
浏览文件 @ ffdc7bd7
......@@ -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
ee/spec/services/merge_requests/approval_service_spec.rb
+ 0
− 110
浏览文件 @ ffdc7bd7
......@@ -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
......
spec/services/merge_requests/approval_service_spec.rb
+ 0
− 69
浏览文件 @ ffdc7bd7
......@@ -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
......
0% 或 .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑！
想要评论请 注册 或