Commit 8ac59c7d authored by Mayra Cabrera's avatar Mayra Cabrera

Merge branch 'add_report_type_into_approval_project_rules' into 'master'

Add report_type into approval_project_rules

See merge request gitlab-org/gitlab!70221
parents 7928d956 fbc9b54b
# frozen_string_literal: true
class AddReportTypeIntoApprovalProjectRules < Gitlab::Database::Migration[1.0]
def up
add_column :approval_project_rules, :report_type, :integer, limit: 2
end
def down
remove_column :approval_project_rules, :report_type
end
end
# frozen_string_literal: true
class AddReportTypeIndexIntoApprovalProjectRules < Gitlab::Database::Migration[1.0]
disable_ddl_transaction!
INDEX_NAME = 'index_approval_project_rules_report_type'
def up
add_concurrent_index :approval_project_rules, :report_type, name: INDEX_NAME
end
def down
remove_concurrent_index_by_name :approval_project_rules, name: INDEX_NAME
end
end
# frozen_string_literal: true
class UpdateReportTypeForExistingApprovalProjectRules < Gitlab::Database::Migration[1.0]
def up
# 1. We only want to consider when rule_type is set to :report_approver (i.e., 2):
# enum rule_type: {
# regular: 0,
# code_owner: 1, # currently unused
# report_approver: 2,
# any_approver: 3
# }
# 2. Also we want to change only the folowing names and respective values:
# DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check'
# DEFAULT_NAME_FOR_VULNERABILITY_REPORT = 'Vulnerability-Check'
# DEFAULT_NAME_FOR_COVERAGE = 'Coverage-Check'
# enum report_type: {
# vulnerability: 1,
# license_scanning: 2,
# code_coverage: 3
# }
execute <<~SQL
UPDATE approval_project_rules
SET report_type = converted_values.report_type
FROM
( values
(1, 'Vulnerability-Check'),
(2, 'License-Check'),
(3, 'Coverage-Check')
) AS converted_values(report_type, name)
WHERE approval_project_rules.name = converted_values.name
AND approval_project_rules.rule_type = 2;
SQL
end
def down
# no-op
end
end
48b5f8e614117ac5f1e7af4ab85a8835c3b3b34103154565dd7ef6f541dd3c37
\ No newline at end of file
b805a0d3143f968343694ec864b38ba4991931131e8a5082dcd719420328a3ef
\ No newline at end of file
8fa4dbfc075036ca379f153e97b7afd2b7600d129f7fb5294dcf4574be9dd7d1
\ No newline at end of file
......@@ -10462,7 +10462,8 @@ CREATE TABLE approval_project_rules (
rule_type smallint DEFAULT 0 NOT NULL,
scanners text[],
vulnerabilities_allowed smallint DEFAULT 0 NOT NULL,
severity_levels text[] DEFAULT '{}'::text[] NOT NULL
severity_levels text[] DEFAULT '{}'::text[] NOT NULL,
report_type smallint
);
CREATE TABLE approval_project_rules_groups (
......@@ -24243,6 +24244,8 @@ CREATE INDEX index_approval_project_rules_on_rule_type ON approval_project_rules
CREATE INDEX index_approval_project_rules_protected_branches_pb_id ON approval_project_rules_protected_branches USING btree (protected_branch_id);
CREATE INDEX index_approval_project_rules_report_type ON approval_project_rules USING btree (report_type);
CREATE UNIQUE INDEX index_approval_project_rules_users_1 ON approval_project_rules_users USING btree (approval_project_rule_id, user_id);
CREATE INDEX index_approval_project_rules_users_2 ON approval_project_rules_users USING btree (user_id);
......@@ -4,7 +4,7 @@ import { mapState, mapActions } from 'vuex';
import {
LICENSE_CHECK_NAME,
VULNERABILITY_CHECK_NAME,
LICENSE_SCANNING,
REPORT_TYPE_LICENSE_SCANNING,
COVERAGE_CHECK_NAME,
} from 'ee/approvals/constants';
import { s__ } from '~/locale';
......@@ -98,7 +98,7 @@ export default {
return (
matchRule.name !== LICENSE_CHECK_NAME ||
features.some((feature) => {
return feature.type === LICENSE_SCANNING && feature.configured;
return feature.type === REPORT_TYPE_LICENSE_SCANNING && feature.configured;
})
);
},
......
......@@ -21,7 +21,9 @@ export const VULNERABILITY_CHECK_NAME = 'Vulnerability-Check';
export const LICENSE_CHECK_NAME = 'License-Check';
export const COVERAGE_CHECK_NAME = 'Coverage-Check';
export const LICENSE_SCANNING = 'license_scanning';
export const REPORT_TYPE_LICENSE_SCANNING = 'license_scanning';
export const REPORT_TYPE_VULNERABILITY = 'vulnerability';
export const REPORT_TYPE_CODE_COVERAGE = 'code_coverage';
export const APPROVAL_RULE_CONFIGS = {
[VULNERABILITY_CHECK_NAME]: {
......@@ -30,6 +32,7 @@ export const APPROVAL_RULE_CONFIGS = {
'SecurityApprovals|A merge request approval is required when a security report contains a new vulnerability.',
),
documentationText: s__('SecurityApprovals|Learn more about Vulnerability-Check'),
reportType: REPORT_TYPE_VULNERABILITY,
},
[LICENSE_CHECK_NAME]: {
title: s__('SecurityApprovals|License-Check'),
......@@ -37,6 +40,7 @@ export const APPROVAL_RULE_CONFIGS = {
'SecurityApprovals|A merge request approval is required when the license compliance report contains a denied license.',
),
documentationText: s__('SecurityApprovals|Learn more about License-Check'),
reportType: REPORT_TYPE_LICENSE_SCANNING,
},
[COVERAGE_CHECK_NAME]: {
title: s__('SecurityApprovals|Coverage-Check'),
......@@ -44,6 +48,7 @@ export const APPROVAL_RULE_CONFIGS = {
'SecurityApprovals|A merge request approval is required when test coverage declines.',
),
documentationText: s__('SecurityApprovals|Learn more about Coverage-Check'),
reportType: REPORT_TYPE_CODE_COVERAGE,
},
};
......
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import { RULE_TYPE_REGULAR, RULE_TYPE_ANY_APPROVER } from './constants';
import {
RULE_TYPE_REGULAR,
RULE_TYPE_ANY_APPROVER,
APPROVAL_RULE_CONFIGS,
RULE_TYPE_REPORT_APPROVER,
} from './constants';
const visibleTypes = new Set([RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR]);
......@@ -25,6 +30,14 @@ function withDefaultEmptyRule(rules = []) {
];
}
function ruleTypeFromName(ruleName) {
return ruleName in APPROVAL_RULE_CONFIGS ? RULE_TYPE_REPORT_APPROVER : undefined;
}
function reportTypeFromName(ruleName) {
return APPROVAL_RULE_CONFIGS[ruleName]?.reportType;
}
export const mapApprovalRuleRequest = (req) => ({
name: req.name,
approvals_required: req.approvalsRequired,
......@@ -35,6 +48,8 @@ export const mapApprovalRuleRequest = (req) => ({
scanners: req.scanners,
vulnerabilities_allowed: req.vulnerabilitiesAllowed,
severity_levels: req.severityLevels,
report_type: reportTypeFromName(req.name),
rule_type: ruleTypeFromName(req.name),
});
export const mapApprovalFallbackRuleRequest = (req) => ({
......
......@@ -29,7 +29,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
validates :name, uniqueness: { scope: [:merge_request_id, :rule_type, :section] }
validates :rule_type, uniqueness: { scope: :merge_request_id, message: proc { _('any-approver for the merge request already exists') } }, if: :any_approver?
validates :report_type, presence: true, if: :report_approver?
belongs_to :merge_request, inverse_of: :approval_rules
......@@ -54,12 +53,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
alias_method :regular, :regular?
alias_method :code_owner, :code_owner?
enum report_type: {
vulnerability: 1,
license_scanning: 2,
code_coverage: 3
}
scope :vulnerability_report, -> { report_approver.vulnerability }
scope :license_compliance, -> { report_approver.license_scanning }
scope :coverage, -> { report_approver.code_coverage }
......
......@@ -50,7 +50,6 @@ class ApprovalProjectRule < ApplicationRecord
end
def apply_report_approver_rules_to(merge_request)
report_type = report_type_for(self)
rule = merge_request
.approval_rules
.report_approver
......@@ -69,10 +68,6 @@ class ApprovalProjectRule < ApplicationRecord
private
def report_type_for(rule)
ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME[rule.name]
end
def attributes_to_apply_for(report_type)
attributes
.slice('approvals_required', 'name')
......
......@@ -7,11 +7,6 @@ module ApprovalRuleLike
DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check'
DEFAULT_NAME_FOR_VULNERABILITY_REPORT = 'Vulnerability-Check'
DEFAULT_NAME_FOR_COVERAGE = 'Coverage-Check'
REPORT_TYPES_BY_DEFAULT_NAME = {
DEFAULT_NAME_FOR_LICENSE_REPORT => :license_scanning,
DEFAULT_NAME_FOR_VULNERABILITY_REPORT => :vulnerability,
DEFAULT_NAME_FOR_COVERAGE => :code_coverage
}.freeze
APPROVALS_REQUIRED_MAX = 100
ALL_MEMBERS = 'All Members'
......@@ -23,8 +18,15 @@ module ApprovalRuleLike
after_add: :audit_add, after_remove: :audit_remove
has_many :group_users, -> { distinct }, through: :groups, source: :users
enum report_type: {
vulnerability: 1,
license_scanning: 2,
code_coverage: 3
}
validates :name, presence: true
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 }
validates :report_type, presence: true, if: :report_approver?
scope :with_users, -> { preload(:users, :group_users) }
scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) }
......
......@@ -9,12 +9,6 @@ module ApprovalRules
@rule = target.approval_rules.build
@params = params
# report_approver rule_type is currently auto-set according to rulename
# Techdebt to be addressed with: https://gitlab.com/gitlab-org/gitlab/issues/12759
if target.is_a?(Project) && ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.key?(params[:name])
params.reverse_merge!(rule_type: :report_approver)
end
# If merge request approvers are specified, they take precedence over project
# approvers.
copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest)
......
......@@ -15,6 +15,7 @@ module API
optional :scanners, type: Array[String], desc: 'The security scanners to be considered by the approval rule'
optional :vulnerabilities_allowed, type: Integer, desc: 'The number of vulnerabilities allowed for this rule'
optional :severity_levels, type: Array[String], desc: 'The security levels to be considered by the approval rule'
optional :report_type, type: String, desc: 'The type of the report required when rule type equals to report_approver'
end
params :update_project_approval_rule do
......
......@@ -61,6 +61,7 @@ FactoryBot.define do
trait :vulnerability_report do
rule_type { :report_approver }
name { ApprovalRuleLike::DEFAULT_NAME_FOR_VULNERABILITY_REPORT }
report_type { :vulnerability }
end
trait :vulnerability do
......@@ -70,11 +71,13 @@ FactoryBot.define do
trait :license_scanning do
name { ApprovalRuleLike::DEFAULT_NAME_FOR_LICENSE_REPORT }
rule_type { :report_approver }
report_type { :license_scanning }
end
trait :code_coverage do
name { ApprovalRuleLike::DEFAULT_NAME_FOR_COVERAGE }
rule_type { :report_approver }
report_type { :code_coverage }
end
end
end
......@@ -3,6 +3,7 @@ import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
import UnconfiguredSecurityRule from 'ee/approvals/components/security_configuration/unconfigured_security_rule.vue';
import UnconfiguredSecurityRules from 'ee/approvals/components/security_configuration/unconfigured_security_rules.vue';
import { VULNERABILITY_CHECK_NAME, LICENSE_CHECK_NAME } from 'ee/approvals/constants';
import { createStoreOptions } from 'ee/approvals/stores';
import projectSettingsModule from 'ee/approvals/stores/modules/project_settings';
......@@ -65,7 +66,7 @@ describe('UnconfiguredSecurityRules component', () => {
});
it('returns true', () => {
expect(wrapper.vm.hasConfiguredJob({ name: 'License-Check' })).toBe(true);
expect(wrapper.vm.hasConfiguredJob({ name: LICENSE_CHECK_NAME })).toBe(true);
});
});
......@@ -77,7 +78,7 @@ describe('UnconfiguredSecurityRules component', () => {
});
it('returns false', () => {
expect(wrapper.vm.hasConfiguredJob({ name: 'License-Check' })).toBe(false);
expect(wrapper.vm.hasConfiguredJob({ name: LICENSE_CHECK_NAME })).toBe(false);
});
});
......@@ -89,7 +90,7 @@ describe('UnconfiguredSecurityRules component', () => {
});
it('returns true', () => {
expect(wrapper.vm.hasConfiguredJob({ name: 'Vulnerability-Check' })).toBe(true);
expect(wrapper.vm.hasConfiguredJob({ name: VULNERABILITY_CHECK_NAME })).toBe(true);
});
});
});
......
import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
import {
VULNERABILITY_CHECK_NAME,
LICENSE_CHECK_NAME,
COVERAGE_CHECK_NAME,
REPORT_TYPE_VULNERABILITY,
REPORT_TYPE_LICENSE_SCANNING,
REPORT_TYPE_CODE_COVERAGE,
RULE_TYPE_REPORT_APPROVER,
} from 'ee/approvals/constants';
import { mergeRequestApprovalSettingsMappers, mapApprovalRuleRequest } from 'ee/approvals/mappers';
import { createGroupApprovalsPayload, createGroupApprovalsState } from './mocks';
describe('approvals mappers', () => {
......@@ -25,4 +34,21 @@ describe('approvals mappers', () => {
);
});
});
describe('mapApprovalRuleRequest', () => {
describe.each`
ruleName | expectedReportType | expectedRuleType
${VULNERABILITY_CHECK_NAME} | ${REPORT_TYPE_VULNERABILITY} | ${RULE_TYPE_REPORT_APPROVER}
${LICENSE_CHECK_NAME} | ${REPORT_TYPE_LICENSE_SCANNING} | ${RULE_TYPE_REPORT_APPROVER}
${COVERAGE_CHECK_NAME} | ${REPORT_TYPE_CODE_COVERAGE} | ${RULE_TYPE_REPORT_APPROVER}
${'Test Name'} | ${undefined} | ${undefined}
`('with rule name set to $ruleName', ({ ruleName, expectedRuleType, expectedReportType }) => {
it(`it returns ${expectedRuleType} rule_type for ${ruleName}`, () => {
expect(mapApprovalRuleRequest({ name: ruleName }).rule_type).toBe(expectedRuleType);
});
it(`it returns ${expectedReportType} report_type for ${ruleName}`, () => {
expect(mapApprovalRuleRequest({ name: ruleName }).report_type).toBe(expectedReportType);
});
});
});
});
......@@ -124,15 +124,24 @@ RSpec.describe ApprovalProjectRule do
subject.groups << group
end
ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.each do |name, value|
context "when the project rule is for a `#{name}`" do
subject { create(:approval_project_rule, value, :requires_approval, project: project) }
where(:default_name, :report_type) do
'Vulnerability-Check' | :vulnerability
'License-Check' | :license_scanning
'Coverage-Check' | :code_coverage
end
context "when there is a project rule for each report type" do
with_them do
subject { create(:approval_project_rule, report_type, :requires_approval, project: project) }
let!(:result) { subject.apply_report_approver_rules_to(merge_request) }
specify { expect(merge_request.reload.approval_rules).to match_array([result]) }
specify { expect(result.users).to match_array([user]) }
specify { expect(result.groups).to match_array([group]) }
specify { expect(result.name).to be(:default_name) }
specify { expect(result.rule_type).to be(:report_approver) }
specify { expect(result.report_type).to be(:report_type) }
end
end
end
......
......@@ -209,18 +209,34 @@ RSpec.describe ApprovalRules::CreateService do
end
end
ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
context "when the rule name is `#{rule_name}`" do
subject { described_class.new(target, user, { name: rule_name, approvals_required: 1 }) }
context 'with rule_type set to :report_approver' do
let(:result) { subject.execute }
context 'without report_type' do
subject { described_class.new(target, user, { name: 'Vulnerability-Check', approvals_required: 1, rule_type: :report_approver }) }
specify { expect(result[:status]).to eq(:error) }
end
context 'with report_type set to any of valid value' do
using RSpec::Parameterized::TableSyntax
subject { described_class.new(target, user, { name: rule_name, approvals_required: 1, rule_type: :report_approver, report_type: report_type }) }
where(:rule_name, :report_type) do
'Vulnerability-Check' | :vulnerability
'License-Check' | :license_scanning
'Coverage-Check' | :code_coverage
end
with_them do
specify { expect(result[:status]).to eq(:success) }
specify { expect(result[:rule].approvals_required).to eq(1) }
specify { expect(result[:rule].rule_type).to eq('report_approver') }
end
end
end
end
context 'when target is merge request' do
let(:target) { create(:merge_request, source_project: project, target_project: project) }
......
......@@ -8,13 +8,20 @@ RSpec.describe MergeRequests::SyncReportApproverApprovalRules do
let(:merge_request) { create(:merge_request) }
describe '#execute' do
using RSpec::Parameterized::TableSyntax
before do
stub_licensed_features(report_approver_rules: true)
end
ApprovalRuleLike::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |default_name|
context "when a project has a single `#{default_name}` approval rule" do
let(:report_type) { ApprovalRuleLike::REPORT_TYPES_BY_DEFAULT_NAME[default_name] }
where(:default_name, :report_type) do
'Vulnerability-Check' | :vulnerability
'License-Check' | :license_scanning
'Coverage-Check' | :code_coverage
end
context 'when a project has a single approval rule for each report_type' do
with_them do
let!(:report_approval_project_rule) { create(:approval_project_rule, report_type, project: merge_request.target_project, approvals_required: 2) }
let!(:regular_approval_project_rule) { create(:approval_project_rule, project: merge_request.target_project) }
......
......@@ -61,17 +61,41 @@ RSpec.shared_examples 'an API endpoint for creating project approval rule' do
end
end
ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
context "when creating a '#{rule_name}' approval rule" do
it 'specifies a `rule_type` of `report_approver`' do
context 'with rule_type set to report_approver' do
before do
params.merge!(rule_type: :report_approver)
end
context 'without report_type' do
it 'returns a error http status' do
expect do
post api(url, current_user), params: params.merge(name: rule_name)
post api(url, current_user), params: params.merge(name: 'Vulnerability-Check')
end.not_to change { ApprovalProjectRule.report_approver.count }
expect(response).to have_gitlab_http_status(:bad_request)
end
end
context 'when creating a approval rule for each report_type' do
using RSpec::Parameterized::TableSyntax
where(:rule_name, :report_type) do
'Vulnerability-Check' | :vulnerability
'License-Check' | :license_scanning
'Coverage-Check' | :code_coverage
end
with_them do
it 'specifies `report_rule` and `report_type`' do
expect do
post api(url, current_user), params: params.merge(name: rule_name, report_type: report_type)
end.to change { ApprovalProjectRule.report_approver.count }.from(0).to(1)
expect(response).to have_gitlab_http_status(:created)
end
end
end
end
context 'with valid scanners' do
let(:scanners) { ['sast'] }
......
# frozen_string_literal: true
require 'spec_helper'
require_migration!('update_report_type_for_existing_approval_project_rules')
RSpec.describe UpdateReportTypeForExistingApprovalProjectRules, :migration do
using RSpec::Parameterized::TableSyntax
let(:group) { table(:namespaces).create!(name: 'user', path: 'user') }
let(:project) { table(:projects).create!(namespace_id: group.id) }
let(:approval_project_rule) { table(:approval_project_rules).create!(name: rule_name, rule_type: rule_type, project_id: project.id) }
let(:rule_type) { 2 }
let(:rule_name) { 'Vulnerability-Check' }
context 'with rule_type set to :report_approver' do
where(:rule_name, :report_type) do
[
['Vulnerability-Check', 1],
['License-Check', 2],
['Coverage-Check', 3]
]
end
with_them do
context "with names associated with report type" do
it 'updates report_type' do
expect { migrate! }.to change { approval_project_rule.reload.report_type }.from(nil).to(report_type)
end
end
end
end
context 'with rule_type set to another value (e.g., :regular)' do
let(:rule_type) { 0 }
it 'does not update report_type' do
expect { migrate! }.not_to change { approval_project_rule.reload.report_type }
end
end
context 'with the rule name set to another value (e.g., Test Rule)' do
let(:rule_name) { 'Test Rule'}
it 'does not update report_type' do
expect { migrate! }.not_to change { approval_project_rule.reload.report_type }
end
end
end
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