Commit ed947a89 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'create-approval-todos-on-update' into 'master'

Create approval todos on update

See merge request gitlab-org/gitlab!26077
parents 3f63ffaf fc8821ae
---
title: Create approval todos on update
merge_request: 26077
author:
type: fixed
......@@ -35,6 +35,28 @@ module EE
super
end
def reset_approvals?(merge_request, _newrev)
merge_request.target_project.reset_approvals_on_push
end
def reset_approvals(merge_request, newrev = nil)
return unless reset_approvals?(merge_request, newrev)
merge_request.approvals.delete_all
create_new_approval_todos_for_all_approvers(merge_request)
end
def all_approvers(merge_request)
merge_request.overall_approvers(exclude_code_owners: true)
end
def create_new_approval_todos_for_all_approvers(merge_request)
return unless ::Feature.enabled?(:create_approval_todos_on_mr_update, merge_request.project, default_enabled: true)
return if merge_request.closed?
todo_service.add_merge_request_approvers(merge_request, all_approvers(merge_request))
end
end
end
end
......@@ -22,13 +22,7 @@ module EE
merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
merge_requests.each do |merge_request|
target_project = merge_request.target_project
if target_project.reset_approvals_on_push &&
merge_request.rebase_commit_sha != newrev
merge_request.approvals.delete_all
end
reset_approvals(merge_request, newrev)
end
end
......@@ -54,6 +48,10 @@ module EE
.execute(project, @push.branch_name, @push.newrev)
end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def reset_approvals?(merge_request, newrev)
super && merge_request.rebase_commit_sha != newrev
end
end
end
end
......@@ -24,7 +24,7 @@ module EE
return merge_request if update_task_event?
new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers
new_approvers = all_approvers(merge_request) - old_approvers
if new_approvers.any?
todo_service.add_merge_request_approvers(merge_request, new_approvers)
......@@ -46,12 +46,6 @@ module EE
reset_approvals(merge_request)
end
def reset_approvals(merge_request)
target_project = merge_request.target_project
merge_request.approvals.delete_all if target_project.reset_approvals_on_push
end
end
end
end
......@@ -16,12 +16,6 @@ module EE
def new_issuable(issuable, author)
if issuable.is_a?(MergeRequest)
approvers = issuable.overall_approvers(exclude_code_owners: true)
issuable.project.team.max_member_access_for_user_ids(approvers.map(&:id))
approvers = approvers.select do |approver|
approver.can?(:approve_merge_request, issuable)
end
create_approval_required_todos(issuable, approvers, author)
end
......@@ -51,6 +45,14 @@ module EE
def create_approval_required_todos(merge_request, approvers, author)
attributes = attributes_for_todo(merge_request.project, merge_request, author, ::Todo::APPROVAL_REQUIRED)
# Preload project_authorizations to prevent n+1 queries
merge_request.project.team.max_member_access_for_user_ids(approvers.map(&:id))
approvers = approvers.select do |approver|
approver.can?(:approve_merge_request, merge_request)
end
create_todos(approvers, attributes)
end
end
......
......@@ -66,7 +66,7 @@ describe MergeRequests::RefreshService do
let(:service) { described_class.new(project, current_user) }
let(:enable_code_owner) { true }
let(:enable_report_approver_rules) { true }
let(:todo_service) { double(:todo_service) }
let(:todo_service) { double(:todo_service, add_merge_request_approvers: true) }
let(:notification_service) { double(:notification_service) }
before do
......
......@@ -37,6 +37,18 @@ describe MergeRequests::RefreshService do
@oldrev = @commits.last.id
@newrev = @commits.first.id
@approver = create(:user)
@project.add_developer(@approver)
perform_enqueued_jobs do
@merge_request.update(approver_ids: [@approver].map(&:id).join(','))
@fork_merge_request.update(approver_ids: [@approver].map(&:id).join(','))
end
end
def approval_todos(merge_request)
Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request)
end
context 'push to origin repo source branch' do
......@@ -54,6 +66,8 @@ describe MergeRequests::RefreshService do
expect(@merge_request.approvals).to be_empty
expect(@fork_merge_request.approvals).not_to be_empty
expect(approval_todos(@merge_request).map(&:user)).to contain_exactly(@approver)
expect(approval_todos(@fork_merge_request)).to be_empty
end
end
......@@ -67,6 +81,8 @@ describe MergeRequests::RefreshService do
it 'does not reset approvals' do
expect(@merge_request.approvals).not_to be_empty
expect(@fork_merge_request.approvals).not_to be_empty
expect(approval_todos(@merge_request)).to be_empty
expect(approval_todos(@fork_merge_request)).to be_empty
end
end
end
......@@ -86,6 +102,8 @@ describe MergeRequests::RefreshService do
expect(@merge_request.approvals).not_to be_empty
expect(@fork_merge_request.approvals).to be_empty
expect(approval_todos(@merge_request)).to be_empty
expect(approval_todos(@fork_merge_request).map(&:user)).to contain_exactly(@approver)
end
end
......@@ -99,6 +117,8 @@ describe MergeRequests::RefreshService do
expect(@merge_request.approvals).not_to be_empty
expect(@fork_merge_request.approvals).to be_empty
expect(approval_todos(@merge_request)).to be_empty
expect(approval_todos(@fork_merge_request)).to be_empty
end
end
end
......@@ -113,6 +133,8 @@ describe MergeRequests::RefreshService do
it 'does not reset approvals', :sidekiq_might_not_need_inline do
expect(@merge_request.approvals).not_to be_empty
expect(@fork_merge_request.approvals).not_to be_empty
expect(approval_todos(@merge_request)).to be_empty
expect(approval_todos(@fork_merge_request)).to be_empty
end
end
end
......@@ -127,6 +149,8 @@ describe MergeRequests::RefreshService do
it 'does not reset approvals' do
expect(@merge_request.approvals).not_to be_empty
expect(@fork_merge_request.approvals).not_to be_empty
expect(approval_todos(@merge_request)).to be_empty
expect(approval_todos(@fork_merge_request)).to be_empty
end
end
......@@ -142,6 +166,7 @@ describe MergeRequests::RefreshService do
it 'resets approvals' do
expect(@merge_request.approvals).to be_empty
expect(approval_todos(@merge_request).map(&:user)).to contain_exactly(@approver)
end
end
......@@ -156,6 +181,7 @@ describe MergeRequests::RefreshService do
it 'does not reset approvals' do
expect(@merge_request.approvals).not_to be_empty
expect(approval_todos(@merge_request)).to be_empty
end
end
......@@ -170,6 +196,7 @@ describe MergeRequests::RefreshService do
it 'does not reset approvals' do
expect(@merge_request.approvals).not_to be_empty
expect(approval_todos(@merge_request)).to be_empty
end
end
......@@ -185,6 +212,7 @@ describe MergeRequests::RefreshService do
it 'resets the approvals' do
expect(@merge_request.approvals).to be_empty
expect(approval_todos(@merge_request)).to be_empty
end
end
......@@ -198,6 +226,7 @@ describe MergeRequests::RefreshService do
it 'resets the approvals' do
expect(@merge_request.approvals).to be_empty
expect(approval_todos(@merge_request).map(&:user)).to contain_exactly(@approver)
end
end
end
......
......@@ -172,14 +172,30 @@ describe MergeRequests::UpdateService, :mailer do
end
context 'updating target_branch' do
it 'resets approvals when target_branch is changed' do
let(:existing_approver) { create(:user) }
let(:new_approver) { create(:user) }
before do
project.add_developer(existing_approver)
project.add_developer(new_approver)
perform_enqueued_jobs do
update_merge_request(approver_ids: "#{existing_approver.id},#{new_approver.id}")
end
merge_request.target_project.update(reset_approvals_on_push: true)
merge_request.approvals.create(user_id: user2.id)
merge_request.approvals.create(user_id: existing_approver.id)
end
it 'resets approvals when target_branch is changed' do
update_merge_request(target_branch: 'video')
expect(merge_request.reload.approvals).to be_empty
end
it 'creates new todos for the approvers' do
expect(Todo.where(action: Todo::APPROVAL_REQUIRED).map(&:user)).to contain_exactly(new_approver, existing_approver)
end
end
context 'updating blocking merge requests' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment