Commit ca89b8ba authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Allow MR authors to approve their MRs

parent 0526e96a
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20180826111825) do
ActiveRecord::Schema.define(version: 20180831152625) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -2231,6 +2231,7 @@ ActiveRecord::Schema.define(version: 20180826111825) do
t.boolean "pages_https_only", default: true
t.string "external_webhook_token"
t.boolean "packages_enabled"
t.boolean "merge_requests_author_approval"
end
add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
......
......@@ -52,7 +52,7 @@ group approvers will be restricted.
If a user is added as an individual approver and is also part of a group approver,
then that user is just counted once. The merge request author does not count as
an eligible approver.
an eligible approver, unless [self-approval] is explicitly enabled on the project settings.
Let's say that `m` is the number of required approvals, and `Ω` is the set of
explicit approvers. Depending on their number, there are different cases:
......@@ -91,7 +91,8 @@ the following is possible:
![Remove approval](img/remove_approval.png)
NOTE: **Note:**
The merge request author is not allowed to approve their own merge request.
The merge request author is only allowed to approve their own merge request
if [self-approval] is enabled on the project settings.
For the given merge request, if the required number of approvals has been met
(i.e., the number of approvals given to the merge request is greater or equal
......@@ -172,6 +173,18 @@ However, approvals will be reset if the target branch is changed.
If you want approvals to persist, independent of changes to the merge request,
turn this setting to off by unchecking the box and saving the changes.
## Allowing merge request authors to approve their own merge requests
You can allow merge request authors to self-approve merge requests by
enabling it [at the project level](#editing-approvals). Authors
also need to be included in the approvers list in order to be able to
approve their merge request.
1. Navigate to your project's **Settings > General** and expand the
**Merge requests settings**
1. Tick the "Enable self approval of merge requests" checkbox
1. Click **Save changes**
## Merge requests with different source branch and target branch projects
If the merge request source branch and target branch belong to different
......@@ -182,3 +195,5 @@ branch's project, the relevant settings are the target project's. The source
branch's project settings are not applicable. Even if you start the merge
request from the source branch's project UI, pay attention to the created merge
request itself. It belongs to the target branch's project.
[self-approval]: #allowing-merge-request-authors-to-approve-their-own-merge-requests
......@@ -23,6 +23,7 @@ module EE
ci_cd_only
use_custom_template
packages_enabled
merge_requests_author_approval
]
if allow_mirror_params?
......
......@@ -16,13 +16,14 @@ module Approvable
# Number of approvals remaining (excluding existing approvals) before the MR is
# considered approved. If there are fewer potential approvers than approvals left,
# choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
# users should either reduce the number of approvers on projects and/or merge
# requests settings and/or allow MR authors to approve their own merge
# requests (in case only one approval is needed).
#
def approvals_left
[
[approvals_required - approvals.size, number_of_potential_approvers].min,
0
].max
approvals_left_count = approvals_required - approvals.size
[approvals_left_count, 0].max
end
def approvals_required
......@@ -35,41 +36,6 @@ module Approvable
super
end
# An MR can potentially be approved by:
# - anyone in the approvers list
# - any other project member with developer access or higher (if there are no approvers
# left)
#
# It cannot be approved by:
# - a user who has already approved the MR
# - the MR author
#
def number_of_potential_approvers
has_access = ['access_level > ?', Member::REPORTER]
users_with_access = { id: project.project_authorizations.where(has_access).select(:user_id) }
all_approvers = all_approvers_including_groups
users_relation = User.active.where.not(id: approvals.select(:user_id))
users_relation = users_relation.where.not(id: author.id) if author
# This is an optimisation for large instances. Instead of getting the
# count of all users who meet the conditions in a single query, which
# produces a slow query plan, we get the union of all users with access
# and all users in the approvers list, and count them.
if all_approvers.any?
specific_approvers = { id: all_approvers.map(&:id) }
union = Gitlab::SQL::Union.new([
users_relation.where(users_with_access).select(:id),
users_relation.where(specific_approvers).select(:id)
])
User.from("(#{union.to_sql}) subquery").count
else
users_relation.where(users_with_access).count
end
end
# Users in the list of approvers who have not already approved this MR.
#
def approvers_left
......@@ -79,14 +45,18 @@ module Approvable
end
# The list of approvers from either this MR (if they've been set on the MR) or the
# target project. Excludes the author by default.
# target project. Excludes the author if 'self-approval' isn't explicitly
# enabled on project settings.
#
# Before a merge request has been created, author will be nil, so pass the current user
# on the MR create page.
#
def overall_approvers
approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
approvers_relation = approvers_relation.where.not(user_id: author.id) if author
if author && !authors_can_approve?
approvers_relation = approvers_relation.where.not(user_id: author.id)
end
approvers_relation.includes(:user)
end
......@@ -121,7 +91,7 @@ module Approvable
group_approvers.flatten!
group_approvers.delete(author)
group_approvers.delete(author) unless authors_can_approve?
group_approvers
end
......@@ -132,7 +102,10 @@ module Approvable
def can_approve?(user)
return false unless user
# The check below considers authors being able to approve the MR. That is,
# they're included/excluded from that list accordingly.
return true if approvers_left.include?(user)
# We can safely unauthorize authors if it reaches this guard clause.
return false if user == author
return false unless user.can?(:update_merge_request, self)
......@@ -155,6 +128,10 @@ module Approvable
remaining_approvals.zero? || remaining_approvals > approvers_left.count
end
def authors_can_approve?
target_project.merge_requests_author_approval?
end
def approver_ids=(value)
::Gitlab::Utils.ensure_array_from_string(value).each do |user_id|
next if author && user_id == author.id
......
......@@ -46,6 +46,7 @@ module EE
delegate :expose_sast_container_data?, to: :head_pipeline, allow_nil: true
delegate :expose_container_scanning_data?, to: :head_pipeline, allow_nil: true
delegate :expose_dast_data?, to: :head_pipeline, allow_nil: true
delegate :merge_requests_author_approval?, to: :target_project, allow_nil: true
participant :participant_approvers
end
......
......@@ -73,3 +73,11 @@
= form.label :reset_approvals_on_push do
%strong Remove all approvals in a merge request when new commits are pushed to its source branch
.form-group.self-approval
.form-check
= form.check_box :merge_requests_author_approval, class: 'form-check-input'
= form.label :merge_requests_author_approval do
%strong Enable self approval of merge requests
= link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals",
anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
......@@ -4,7 +4,8 @@
- return unless issuable.is_a?(MergeRequest)
- return unless issuable.requires_approve?
- ineligible_approver = issuable.author || current_user
- author = issuable.author || current_user
- authors_can_approve = issuable.merge_requests_author_approval?
- can_update_approvers = can?(current_user, :update_approvers, issuable)
.form-group.row
......@@ -12,7 +13,15 @@
Approvers
.col-sm-10
- if can_update_approvers
= users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups + [ineligible_approver], project: issuable.target_project)
- skip_users = [*issuable.all_approvers_including_groups, (author unless authors_can_approve)].compact
= users_select_tag("merge_request[approver_ids]",
multiple: true,
class: 'input-large',
email_user: true,
skip_users: skip_users,
project: issuable.target_project)
.form-text.text-muted
This merge request must be approved by these users.
You can override the project settings by setting your own list of approvers.
......
---
title: Allow MR authors to approve their MRs
merge_request:
author:
type: other
# frozen_string_literal: true
class AddMergeRequestsAuthorApprovalToProjects < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :projects, :merge_requests_author_approval, :boolean
end
end
......@@ -479,6 +479,7 @@ Project:
- merge_requests_template
- merge_requests_rebase_enabled
- approvals_before_merge
- merge_requests_author_approval
- reset_approvals_on_push
- disable_overriding_approvers_per_merge_request
- merge_requests_ff_only_enabled
......
This diff is collapsed.
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