Commit ba3ed8a1 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Enforce code owner approval

In EEP code owner approval can be enforced at the project level.
This can be turned on in the project settings.

When this is enabled in combination with the feature flag
`multiple_code_owner_rules` (enabled by default) then a user per code
owner entry needs to approve.
parent e0391009
...@@ -155,6 +155,23 @@ are other conditions that may block it, such as merge conflicts, ...@@ -155,6 +155,23 @@ are other conditions that may block it, such as merge conflicts,
[pending discussions](../../discussions/index.md#l#only-allow-merge-requests-to-be-merged-if-all-discussions-are-resolved) [pending discussions](../../discussions/index.md#l#only-allow-merge-requests-to-be-merged-if-all-discussions-are-resolved)
or a [failed CI/CD pipeline](merge_when_pipeline_succeeds.md). or a [failed CI/CD pipeline](merge_when_pipeline_succeeds.md).
## Code Owners approvals **[PREMIUM]**
> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/4418) in [GitLab Premium](https://about.gitlab.com/pricing/) 11.9.
It is possible to require at least one approval for each entry in the
[`CODEOWNERS` file](../code_owners.md) that matches a file changed in
the merge request. To enable this feature:
1. Navigate to your project's **Settings > General** and expand
**Merge request approvals**.
1. Tick the **Require approval from code owners** checkbox
checkbox.
1. Click **Save changes**.
When this feature is enabled, all merge requests will need approval
from one code owner per matched rule before it can be merged.
## Overriding the merge request approvals default settings ## Overriding the merge request approvals default settings
> Introduced in GitLab Enterprise Edition 9.4. > Introduced in GitLab Enterprise Edition 9.4.
......
<script> <script>
import _ from 'underscore';
import { sprintf, __ } from '~/locale'; import { sprintf, __ } from '~/locale';
import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue'; import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
import { RULE_TYPE_CODE_OWNER } from 'ee/approvals/constants';
import ApprovedIcon from './approved_icon.vue'; import ApprovedIcon from './approved_icon.vue';
export default { export default {
...@@ -14,6 +16,24 @@ export default { ...@@ -14,6 +16,24 @@ export default {
required: true, required: true,
}, },
}, },
computed: {
sections() {
return [
{
id: _.uniqueId(),
title: '',
rules: this.approvalRules.filter(rule => rule.rule_type !== RULE_TYPE_CODE_OWNER),
},
{
id: _.uniqueId(),
title: __('Code Owners'),
rules: this.approvalRules
.filter(rule => rule.rule_type === RULE_TYPE_CODE_OWNER)
.map(rule => ({ ...rule, nameClass: 'monospace' })),
},
].filter(x => x.rules.length);
},
},
methods: { methods: {
pendingApprovalsText(rule) { pendingApprovalsText(rule) {
if (!rule.approvals_required) { if (!rule.approvals_required) {
...@@ -58,11 +78,17 @@ export default { ...@@ -58,11 +78,17 @@ export default {
<th>{{ s__('MRApprovals|Approved by') }}</th> <th>{{ s__('MRApprovals|Approved by') }}</th>
</tr> </tr>
</thead> </thead>
<tbody> <tbody v-for="{ id, title, rules } in sections" :key="id" class="border-top-0">
<tr v-for="rule in approvalRules" :key="rule.id"> <tr v-if="title" class="js-section-title">
<td class="w-0"></td>
<td colspan="99">
<strong>{{ title }}</strong>
</td>
</tr>
<tr v-for="rule in rules" :key="rule.id">
<td class="w-0"><approved-icon :is-approved="rule.approved" /></td> <td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
<td :colspan="rule.fallback ? 2 : 1"> <td :colspan="rule.fallback ? 2 : 1">
<div class="d-none d-sm-block js-name">{{ rule.name }}</div> <div class="d-none d-sm-block js-name" :class="rule.nameClass">{{ rule.name }}</div>
<div class="d-flex d-sm-none flex-column js-summary"> <div class="d-flex d-sm-none flex-column js-summary">
<span>{{ summaryText(rule) }}</span> <span>{{ summaryText(rule) }}</span>
<user-avatar-list <user-avatar-list
......
import _ from 'underscore';
import { __ } from '~/locale'; import { __ } from '~/locale';
import { RULE_TYPE_REGULAR, RULE_TYPE_FALLBACK } from 'ee/approvals/constants'; import {
RULE_TYPE_REGULAR,
RULE_TYPE_FALLBACK,
RULE_TYPE_CODE_OWNER,
} from 'ee/approvals/constants';
function mapApprovalRule(rule, settings) { function mapApprovalRule(rule, settings) {
if (rule.rule_type === RULE_TYPE_FALLBACK) { if (rule.rule_type === RULE_TYPE_FALLBACK) {
...@@ -20,6 +25,23 @@ function mapApprovalRule(rule, settings) { ...@@ -20,6 +25,23 @@ function mapApprovalRule(rule, settings) {
return rule; return rule;
} }
function getApprovalRuleNamesLeft(data) {
if (!data.multiple_approval_rules_available) {
return [];
}
const rulesLeft = _.groupBy(data.approval_rules_left, x => x.rule_type);
// Filter out empty names (fallback rule has no name) because the empties would look weird.
const regularRules = (rulesLeft[RULE_TYPE_REGULAR] || []).map(x => x.name).filter(x => x);
// If there are code owners that need to approve, only mention that once.
// As the names of code owner rules are patterns that don't mean much out of context.
const codeOwnerRules = rulesLeft[RULE_TYPE_CODE_OWNER] ? [__('Code Owners')] : [];
return [...regularRules, ...codeOwnerRules];
}
/** /**
* Map the approval rules response for use by the MR widget * Map the approval rules response for use by the MR widget
*/ */
...@@ -33,10 +55,6 @@ export function mapApprovalRulesResponse(rules, settings) { ...@@ -33,10 +55,6 @@ export function mapApprovalRulesResponse(rules, settings) {
export function mapApprovalsResponse(data) { export function mapApprovalsResponse(data) {
return { return {
...data, ...data,
// Filter out empty names (fallback rule has no name) because approvalRuleNamesLeft: getApprovalRuleNamesLeft(data),
// the empties would look weird.
approvalRuleNamesLeft: data.multiple_approval_rules_available
? data.approval_rules_left.map(x => x.name).filter(x => x)
: [],
}; };
} }
...@@ -42,6 +42,7 @@ module EE ...@@ -42,6 +42,7 @@ module EE
use_custom_template use_custom_template
packages_enabled packages_enabled
merge_requests_author_approval merge_requests_author_approval
merge_requests_require_code_owner_approval
group_with_project_templates_id group_with_project_templates_id
] ]
......
# frozen_string_literal: true # frozen_string_literal: true
class ApprovalMergeRequestRule < ApplicationRecord class ApprovalMergeRequestRule < ApplicationRecord
include Gitlab::Utils::StrongMemoize
include ApprovalRuleLike include ApprovalRuleLike
DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner' DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner'
...@@ -43,13 +44,18 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -43,13 +44,18 @@ class ApprovalMergeRequestRule < ApplicationRecord
# enabled on project settings. # enabled on project settings.
# @return [Array<User>] # @return [Array<User>]
def approvers def approvers
scope = super strong_memoize(:approvers) do
scope_or_array = super
if merge_request.author && !project.merge_requests_author_approval? next scope_or_array unless merge_request.author
scope = scope.where.not(id: merge_request.author) next scope_or_array if project.merge_requests_author_approval?
end
scope if scope_or_array.respond_to?(:where)
scope_or_array.where.not(id: merge_request.author)
else
scope_or_array - [merge_request.author]
end
end
end end
def sync_approved_approvers def sync_approved_approvers
......
...@@ -5,10 +5,14 @@ class ApprovalWrappedRule ...@@ -5,10 +5,14 @@ class ApprovalWrappedRule
extend Forwardable extend Forwardable
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
REQUIRED_APPROVALS_PER_CODE_OWNER_RULE = 1
attr_reader :merge_request attr_reader :merge_request
attr_reader :approval_rule attr_reader :approval_rule
def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule, :rule_type def_delegators(:@approval_rule,
:id, :name, :users, :groups, :code_owner, :code_owner?, :source_rule,
:rule_type)
def initialize(merge_request, approval_rule) def initialize(merge_request, approval_rule)
@merge_request = merge_request @merge_request = merge_request
...@@ -72,4 +76,22 @@ class ApprovalWrappedRule ...@@ -72,4 +76,22 @@ class ApprovalWrappedRule
def unactioned_approvers def unactioned_approvers
approvers - approved_approvers approvers - approved_approvers
end end
def approvals_required
if code_owner?
code_owner_approvals_required
else
approval_rule.approvals_required
end
end
private
def code_owner_approvals_required
strong_memoize(:code_owner_approvals_required) do
next 0 unless project.merge_requests_require_code_owner_approval?
approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0
end
end
end end
...@@ -13,12 +13,18 @@ module ApprovalRuleLike ...@@ -13,12 +13,18 @@ module ApprovalRuleLike
validates :name, presence: true validates :name, presence: true
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 }
scope :with_users, -> { preload(:users, :group_users) }
end end
# Users who are eligible to approve, including specified group members. # Users who are eligible to approve, including specified group members.
# @return [Array<User>] # @return [Array<User>]
def approvers def approvers
@approvers ||= User.from_union([users, group_users]) @approvers ||= if users.loaded? && group_users.loaded?
users | group_users
else
User.from_union([users, group_users])
end
end end
def add_member(member) def add_member(member)
......
...@@ -229,6 +229,10 @@ module EE ...@@ -229,6 +229,10 @@ module EE
feature_available?(:multiple_approval_rules) feature_available?(:multiple_approval_rules)
end end
def code_owner_approval_required_available?
feature_available?(:code_owner_approval_required)
end
def service_desk_enabled def service_desk_enabled
::EE::Gitlab::ServiceDesk.enabled?(project: self) && super ::EE::Gitlab::ServiceDesk.enabled?(project: self) && super
end end
...@@ -338,6 +342,10 @@ module EE ...@@ -338,6 +342,10 @@ module EE
end end
end end
def merge_requests_require_code_owner_approval?
super && code_owner_approval_required_available?
end
def find_path_lock(path, exact_match: false, downstream: false) def find_path_lock(path, exact_match: false, downstream: false)
path_lock_finder = strong_memoize(:path_lock_finder) do path_lock_finder = strong_memoize(:path_lock_finder) do
::Gitlab::PathLocksFinder.new(self) ::Gitlab::PathLocksFinder.new(self)
......
...@@ -70,7 +70,7 @@ class License < ActiveRecord::Base ...@@ -70,7 +70,7 @@ class License < ActiveRecord::Base
protected_environments protected_environments
custom_project_templates custom_project_templates
packages packages
code_owner_as_approver_suggestion code_owner_approval_required
feature_flags feature_flags
batch_comments batch_comments
issues_analytics issues_analytics
......
...@@ -39,6 +39,10 @@ module EE ...@@ -39,6 +39,10 @@ module EE
merge_request.target_project.present(current_user: current_user) merge_request.target_project.present(current_user: current_user)
end end
def code_owner_rules_with_users
@code_owner_rules ||= merge_request.approval_rules.code_owner.with_users.to_a
end
def approver_groups def approver_groups
::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user) ::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
end end
......
...@@ -8,7 +8,7 @@ module MergeRequests ...@@ -8,7 +8,7 @@ module MergeRequests
end end
def execute def execute
if ::Feature.enabled?(:multiple_code_owner_rules) if ::Feature.enabled?(:multiple_code_owner_rules, default_enabled: true)
sync_rules sync_rules
else else
merge_request.sync_code_owners_with_approvers merge_request.sync_code_owners_with_approvers
......
...@@ -5,23 +5,31 @@ ...@@ -5,23 +5,31 @@
- else - else
= render 'shared/merge_request_approvals_settings/single_rule_form', form: form, project: project = render 'shared/merge_request_approvals_settings/single_rule_form', form: form, project: project
- if project.code_owner_approval_required_available?
.form-group.require-code-owner-approval
.form-check
= form.check_box(:merge_requests_require_code_owner_approval, class: 'form-check-input')
= form.label :merge_requests_require_code_owner_approval, class: 'form-check-label' do
%strong= _('Require approval from code owners')
= link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'editing-approvals-premium'), target: '_blank'
.form-group .form-group
.form-check .form-check
= form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input' }, false, true) = form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input' }, false, true)
= form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do = form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do
%strong= _("Can override approvers and approvals required per merge request") %strong= _('Can override approvers and approvals required per merge request')
= link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", anchor: 'overriding-the-merge-request-approvals-default-settings'), target: '_blank' = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'overriding-the-merge-request-approvals-default-settings'), target: '_blank'
.form-group.reset-approvals-on-push .form-group.reset-approvals-on-push
.form-check .form-check
= form.check_box :reset_approvals_on_push, class: 'form-check-input' = form.check_box :reset_approvals_on_push, class: 'form-check-input'
= form.label :reset_approvals_on_push, class: 'form-check-label' do = form.label :reset_approvals_on_push, class: 'form-check-label' do
%strong= _("Remove all approvals in a merge request when new commits are pushed to its source branch") %strong= _('Remove all approvals in a merge request when new commits are pushed to its source branch')
.form-group.self-approval .form-group.self-approval
.form-check .form-check
= form.check_box :merge_requests_author_approval, class: 'form-check-input' = form.check_box :merge_requests_author_approval, class: 'form-check-input'
= form.label :merge_requests_author_approval, class: 'form-check-label' do = form.label :merge_requests_author_approval, class: 'form-check-label' do
%strong= _("Enable self approval of merge requests") %strong= _('Enable self approval of merge requests')
= link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", = 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' anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
- return unless @project.merge_requests_require_code_owner_approval?
- code_owner_rules = merge_request.code_owner_rules_with_users
- return unless code_owner_rules.any?
.prepend-top-20
%strong= _('Code owner approval is required')
%p
= _('At least one approval from a code owner is required to change files matching the respective CODEOWNER rules.')
= link_to(_('Read more'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'code-owners-approvals-premium'))
.border-bottom
%table.table.m-0
%thead.thead-white.text-nowrap
%tr.d-none.d-sm-table-row
%th.w-25= s_('CodeOwner|Pattern')
%th= _('Members')
%tbody
- code_owner_rules.each do |code_owner_approval_rule|
%tr
%td.monospace= code_owner_approval_rule.name
%td.d-none.d-sm-table-cell
- code_owner_approval_rule.approvers.each do |approver|
= user_avatar(user: approver)
...@@ -26,3 +26,4 @@ ...@@ -26,3 +26,4 @@
Tip: add a Tip: add a
= link_to 'CODEOWNERS', help_page_path('user/project/code_owners'), target: '_blank', tabindex: -1 = link_to 'CODEOWNERS', help_page_path('user/project/code_owners'), target: '_blank', tabindex: -1
to automatically add approvers based on file paths and file types. to automatically add approvers based on file paths and file types.
= render 'projects/merge_requests/code_owner_approval_rules', merge_request: @mr_presenter
---
title: Enforce merge request approvals from code owners
merge_request: 9656
author:
type: added
# frozen_string_literal: true # frozen_string_literal: true
class AddMergeRequestsRequireCodeownerApprovalToProjects < ActiveRecord::Migration[5.0] class AddMergeRequestsRequireCodeownerApprovalToProjects < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false DOWNTIME = false
def change def change
......
require 'rails_helper' require 'rails_helper'
describe 'Merge request > User sees approval widget', :js do describe 'Merge request > User sees approval widget', :js do
let(:project) { create(:project, :public, :repository, approvals_before_merge: 1) } let(:project) { create(:project, :public, :repository) }
let(:user) { project.creator } let(:user) { project.creator }
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
before do before do
stub_feature_flags(approval_rules: false) stub_feature_flags(approval_rules: false)
sign_in(user)
end end
context 'when merge when discussions resolved is active' do context 'when merge when discussions resolved is active' do
...@@ -17,8 +19,6 @@ describe 'Merge request > User sees approval widget', :js do ...@@ -17,8 +19,6 @@ describe 'Merge request > User sees approval widget', :js do
end end
before do before do
sign_in(user)
visit project_merge_request_path(project, merge_request) visit project_merge_request_path(project, merge_request)
end end
...@@ -27,4 +27,111 @@ describe 'Merge request > User sees approval widget', :js do ...@@ -27,4 +27,111 @@ describe 'Merge request > User sees approval widget', :js do
expect(find('.js-mr-approvals')).to have_selector('.approvals-body') expect(find('.js-mr-approvals')).to have_selector('.approvals-body')
end end
end end
context 'when rules are enabled' do
before do
stub_feature_flags(approval_rules: true)
end
context 'merge request approvers enabled' do
let(:project) { create(:project, :public, :repository, approvals_before_merge: 3) }
before do
stub_licensed_features(merge_request_approvers: true)
visit project_merge_request_path(project, merge_request)
end
it 'the renders the number of required approvals' do
wait_for_requests
expect(page).to have_content('Requires 3 more approvals.')
end
end
context 'multiple approval rules enabled' do
let(:members) { create_list(:user, 2) }
let!(:rule) do
create(:approval_merge_request_rule,
merge_request: merge_request,
users: members,
approvals_required: 1)
end
before do
stub_licensed_features(multiple_approval_rules: true)
members.each { |user| project.add_developer(user) }
end
it 'shows the approval rule' do
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).to have_content("Requires approval from #{rule.name}")
click_on 'View eligible approvers'
wait_for_requests
within('.mr-widget-workflow table') do
expect(page).to have_content(rule.name)
end
end
context 'for code owner rules' do
let(:code_owners) { create_list(:user, 2) }
let!(:code_owner_rule) do
create(:code_owner_rule,
merge_request: merge_request,
users: code_owners,
name: '*.js')
end
before do
code_owners.each { |user| project.add_developer(user) }
end
it 'shows the code owner rule as optional' do
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).to have_content("Requires approval from #{rule.name}.")
click_on 'View eligible approvers'
wait_for_requests
within('.mr-widget-workflow table .monospace') do
code_owner_row = find(:xpath, "//tr[td[contains(.,'#{code_owner_rule.name}')]]")
expect(code_owner_row).to have_content('Optional')
end
end
context 'when code owner approval is required' do
before do
stub_licensed_features(code_owner_approval_required: true, multiple_approval_rules: true)
project.update!(merge_requests_require_code_owner_approval: true)
end
it 'shows the code owner rule as required' do
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).to have_content("Requires 2 more approvals from #{rule.name} and Code Owners")
click_on 'View eligible approvers'
wait_for_requests
within('.mr-widget-workflow table .monospace') do
code_owner_row = find(:xpath, "//tr[td[contains(.,'#{code_owner_rule.name}')]]")
expect(code_owner_row).to have_content('0 of 1')
end
end
end
end
end
end
end end
require 'spec_helper'
describe 'Projects > Merge Requests > User edits a merge request' do
let(:user) { create(:user) }
before do
stub_licensed_features(licensed_features)
project.add_maintainer(user)
sign_in(user)
end
context 'when the merge request has matching code owners', :js do
let(:licensed_features) do
{ code_owners: true, code_owner_approval_required: true }
end
let(:project) do
create(:project, :custom_repo,
merge_requests_require_code_owner_approval: true,
files: { 'docs/CODEOWNERS' => "*.rb @ruby-owner\n*.js @js-owner" })
end
let(:merge_request) do
create(:merge_request,
source_project: project,
target_project: project,
target_branch: 'master',
source_branch: 'feature')
end
let(:ruby_owner) { create(:user, username: 'ruby-owner') }
before do
project.add_developer(ruby_owner)
project.repository.create_file(user, 'ruby.rb', '# a ruby file',
message: 'Add a ruby file',
branch_name: 'feature')
# To make sure the rules are created for the merge request, the services
# that do that aren't triggered from factories
MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
end
it 'shows the matching code owner rules' do
visit(edit_project_merge_request_path(project, merge_request))
expect(page).to have_content('*.rb')
expect(page).to have_link(href: user_path(ruby_owner))
end
end
end
require 'spec_helper'
describe 'EE > Projects > Settings > User manages approval rule settings' do
let(:project) { create(:project) }
let(:user) { project.owner }
before do
sign_in(user)
stub_licensed_features(licensed_features)
visit edit_project_path(project)
end
context 'when `code_owner_approval_required` is available' do
let(:licensed_features) { { code_owner_approval_required: true } }
it 'allows the user to enforce code owner approval' do
within('.require-code-owner-approval') do
check('Require approval from code owners')
end
within('.merge-request-approval-settings-form') do
click_on('Save changes')
end
expect(project.reload.merge_requests_require_code_owner_approval?).to be_truthy
end
end
context 'when `code_owner_approval_required` is not available' do
let(:licensed_features) { { code_owner_approval_required: false } }
it 'does not allow the user to require code owner approval' do
expect(page).not_to have_content('Require approval from code owners')
end
end
end
...@@ -40,6 +40,16 @@ const testRuleFallback = () => ({ ...@@ -40,6 +40,16 @@ const testRuleFallback = () => ({
approvers: [], approvers: [],
approved: false, approved: false,
}); });
const testRuleCodeOwner = () => ({
id: '*.js',
name: '',
fallback: true,
approvals_required: 3,
approved_by: [{ id: 1 }, { id: 2 }],
approvers: [],
approved: false,
rule_type: 'code_owner',
});
const testRules = () => [testRuleApproved(), testRuleUnapproved(), testRuleOptional()]; const testRules = () => [testRuleApproved(), testRuleUnapproved(), testRuleOptional()];
describe('EE MRWidget approvals list', () => { describe('EE MRWidget approvals list', () => {
...@@ -77,6 +87,28 @@ describe('EE MRWidget approvals list', () => { ...@@ -77,6 +87,28 @@ describe('EE MRWidget approvals list', () => {
expect(rows.length).toEqual(expected.length); expect(rows.length).toEqual(expected.length);
expect(names).toEqual(expected.map(x => x.name)); expect(names).toEqual(expected.map(x => x.name));
}); });
it('does not render a code owner subtitle', () => {
expect(wrapper.find('.js-section-title').exists()).toBe(false);
});
describe('when a code owner rule is included', () => {
let rulesWithCodeOwner;
beforeEach(() => {
rulesWithCodeOwner = testRules().concat([testRuleCodeOwner()]);
createComponent({
approvalRules: rulesWithCodeOwner,
});
});
it('renders a code owner subtitle', () => {
const rows = findRows();
expect(wrapper.find('.js-section-title').exists()).toBe(true);
expect(rows.length).toEqual(rulesWithCodeOwner.length + 1);
});
});
}); });
describe('when approved rule', () => { describe('when approved rule', () => {
...@@ -255,4 +287,29 @@ describe('EE MRWidget approvals list', () => { ...@@ -255,4 +287,29 @@ describe('EE MRWidget approvals list', () => {
expect(lists.at(0).props('items')).toEqual(rule.approved_by); expect(lists.at(0).props('items')).toEqual(rule.approved_by);
}); });
}); });
describe('when code owner rule', () => {
const rule = testRuleCodeOwner();
let row;
beforeEach(() => {
createComponent({
approvalRules: [rule],
});
row = findRows().at(1);
});
it('renders the code owner title row', () => {
const titleRow = findRows().at(0);
expect(titleRow.text()).toEqual('Code Owners');
});
it('renders the name in a monospace font', () => {
const codeOwnerRow = findRowElement(row, 'name');
expect(codeOwnerRow.hasClass('monospace')).toEqual(true);
expect(codeOwnerRow.text()).toEqual(rule.name);
});
});
}); });
import { mapApprovalsResponse } from 'ee/vue_merge_request_widget/mappers';
import { RULE_TYPE_REGULAR, RULE_TYPE_CODE_OWNER } from 'ee/approvals/constants';
describe('EE MR Widget mappers', () => {
let data;
beforeEach(() => {
data = {
approval_rules_left: [
{ name: 'Lorem', rule_type: RULE_TYPE_REGULAR },
{ name: '', rule_type: RULE_TYPE_REGULAR },
{ name: 'Ipsum', rule_type: RULE_TYPE_REGULAR },
],
};
});
describe('mapApprovalsResponse', () => {
describe('with multiple approval rules allowed', () => {
beforeEach(() => {
data.multiple_approval_rules_available = true;
});
it('approvalRuleNamesLeft does not include empty names', () => {
const result = mapApprovalsResponse(data);
expect(result).toEqual(
jasmine.objectContaining({
approvalRuleNamesLeft: ['Lorem', 'Ipsum'],
}),
);
});
it('approvalRuleNamesLeft includes "Code Owners" if any', () => {
data.approval_rules_left.push(
{ name: 'src/foo', rule_type: RULE_TYPE_CODE_OWNER },
{ name: 'src/bar', rule_type: RULE_TYPE_CODE_OWNER },
);
const result = mapApprovalsResponse(data);
expect(result).toEqual(
jasmine.objectContaining({
approvalRuleNamesLeft: ['Lorem', 'Ipsum', 'Code Owners'],
}),
);
});
it('approvalRuleNamesLeft is empty with no rules left', () => {
const result = mapApprovalsResponse({
...data,
approval_rules_left: [],
});
expect(result).toEqual(
jasmine.objectContaining({
approvalRuleNamesLeft: [],
}),
);
});
});
describe('with single approval rule allowed', () => {
beforeEach(() => {
data.multiple_approval_rules_available = false;
});
it('approvalRuleNamesLeft is empty', () => {
const result = mapApprovalsResponse(data);
expect(result).toEqual(
jasmine.objectContaining({
approvalRuleNamesLeft: [],
}),
);
});
});
});
});
--- ---
Project:
- merge_requests_require_code_owner_approval
ProjectTracingSetting: ProjectTracingSetting:
- external_url - external_url
Note: Note:
......
...@@ -124,11 +124,28 @@ describe ApprovalMergeRequestRule do ...@@ -124,11 +124,28 @@ describe ApprovalMergeRequestRule do
end end
context 'when project merge_requests_author_approval is false' do context 'when project merge_requests_author_approval is false' do
it 'contains author' do before do
merge_request.project.update(merge_requests_author_approval: false) merge_request.project.update(merge_requests_author_approval: false)
end
it 'does not contain author' do
expect(subject.approvers).to be_empty expect(subject.approvers).to be_empty
end end
context 'when the rules users have already been loaded' do
before do
subject.users
subject.group_users
end
it 'does not cause queries' do
expect { subject.approvers }.not_to exceed_query_limit(0)
end
it 'does not contain the author' do
expect(subject.approvers).to be_empty
end
end
end end
end end
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe ApprovalWrappedRule do describe ApprovalWrappedRule do
using RSpec::Parameterized::TableSyntax
let(:merge_request) { create(:merge_request) } let(:merge_request) { create(:merge_request) }
let(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: approvals_required) } let(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: approvals_required) }
let(:approvals_required) { 0 } let(:approvals_required) { 0 }
...@@ -161,4 +163,37 @@ describe ApprovalWrappedRule do ...@@ -161,4 +163,37 @@ describe ApprovalWrappedRule do
end end
end end
end end
describe '#approvals_required' do
context 'for regular rules' do
let(:rule) { create(:approval_merge_request_rule, approvals_required: 19) }
it 'returns the attribute saved on the model' do
expect(subject.approvals_required).to eq(19)
end
end
context 'for code owner rules' do
where(:feature_enabled, :approver_count, :expected_required_approvals) do
true | 0 | 0
true | 2 | 1
false | 2 | 0
false | 0 | 0
end
with_them do
let(:rule) do
create(:code_owner_rule,
merge_request: merge_request,
users: create_list(:user, approver_count))
end
it 'returns the correct number of approvals' do
allow(subject.project).to receive(:merge_requests_require_code_owner_approval?).and_return(feature_enabled)
expect(subject.approvals_required).to eq(expected_required_approvals)
end
end
end
end
end end
...@@ -59,8 +59,25 @@ describe ApprovalRuleLike do ...@@ -59,8 +59,25 @@ describe ApprovalRuleLike do
group2.add_guest(group2_user) group2.add_guest(group2_user)
end end
it 'contains users as direct members and group members' do shared_examples 'approvers contains the right users' do
expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user) it 'contains users as direct members and group members' do
expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
end
end
it_behaves_like 'approvers contains the right users'
context 'when the user relations are already loaded' do
before do
subject.users
subject.group_users
end
it 'does not perform any queries when all users are loaded already' do
expect { subject.approvers }.not_to exceed_query_limit(0)
end
it_behaves_like 'approvers contains the right users'
end end
context 'when user is both a direct member and a group member' do context 'when user is both a direct member and a group member' do
......
...@@ -1010,6 +1010,28 @@ describe Project do ...@@ -1010,6 +1010,28 @@ describe Project do
end end
end end
describe '#merge_requests_require_code_owner_approval?' do
let(:project) { build(:project) }
where(:feature_available, :feature_enabled, :approval_required) do
true | true | true
false | true | false
true | false | false
true | nil | false
end
with_them do
before do
stub_licensed_features(code_owner_approval_required: feature_available)
project.merge_requests_require_code_owner_approval = feature_enabled
end
it 'requires code owner approval when needed' do
expect(project.merge_requests_require_code_owner_approval?).to eq(approval_required)
end
end
end
shared_examples 'project with disabled services' do shared_examples 'project with disabled services' do
it 'has some disabled services' do it 'has some disabled services' do
stub_const('License::ANY_PLAN_FEATURES', []) stub_const('License::ANY_PLAN_FEATURES', [])
......
# frozen_string_literal: true
require 'spec_helper'
describe Projects::MergeRequestsController do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:user) { merge_request.author }
before do
login_as(user)
end
describe 'GET #edit' do
def get_edit
get edit_project_merge_request_path(project, merge_request)
end
context 'when the project requires code owner approval' do
before do
stub_licensed_features(code_owners: true, code_owner_approval_required: true)
project.update!(merge_requests_require_code_owner_approval: true)
get_edit # Warm the cache
end
it 'does not cause an extra queries when code owner rules are present' do
control = ActiveRecord::QueryRecorder.new { get_edit }
create(:code_owner_rule, merge_request: merge_request)
# Threshold of 2 because we load the users & group users for all rules
expect { get_edit }.not_to exceed_query_limit(control).with_threshold(2)
end
it 'does not cause extra queries when multiple code owner rules are present' do
create(:code_owner_rule, merge_request: merge_request)
control = ActiveRecord::QueryRecorder.new { get_edit }
create(:code_owner_rule, merge_request: merge_request)
expect { get_edit }.not_to exceed_query_limit(control)
end
end
end
end
...@@ -1203,6 +1203,9 @@ msgstr "" ...@@ -1203,6 +1203,9 @@ msgstr ""
msgid "Assignee(s)" msgid "Assignee(s)"
msgstr "" msgstr ""
msgid "At least one approval from a code owner is required to change files matching the respective CODEOWNER rules."
msgstr ""
msgid "Attach a file" msgid "Attach a file"
msgstr "" msgstr ""
...@@ -2550,9 +2553,18 @@ msgstr "" ...@@ -2550,9 +2553,18 @@ msgstr ""
msgid "Code" msgid "Code"
msgstr "" msgstr ""
msgid "Code Owners"
msgstr ""
msgid "Code owner approval is required"
msgstr ""
msgid "Code owners" msgid "Code owners"
msgstr "" msgstr ""
msgid "CodeOwner|Pattern"
msgstr ""
msgid "Cohorts" msgid "Cohorts"
msgstr "" msgstr ""
...@@ -8249,6 +8261,9 @@ msgstr "" ...@@ -8249,6 +8261,9 @@ msgstr ""
msgid "Require all users to accept Terms of Service and Privacy Policy when they access GitLab." msgid "Require all users to accept Terms of Service and Privacy Policy when they access GitLab."
msgstr "" msgstr ""
msgid "Require approval from code owners"
msgstr ""
msgid "Requires approval from %{names}." msgid "Requires approval from %{names}."
msgid_plural "Requires %{count} more approvals from %{names}." msgid_plural "Requires %{count} more approvals from %{names}."
msgstr[0] "" msgstr[0] ""
......
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