Merge branch '331001-code-coverage-approval-rule-frontend' into 'master'

Make coverage approval rule frontend-configurable

See merge request gitlab-org/gitlab!64816

doc/ci/pipelines/settings.md

@@ -241,6 +241,26 @@ you can view a graph or download a CSV file with this data. From your project:
 
 Code coverage data is also [available at the group level](../../user/group/repositories_analytics/index.md).
 
+### Coverage check approval rule **(PREMIUM)**
+
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/15765) in GitLab 14.0.
+> - [Made configurable in Project Settings](https://gitlab.com/gitlab-org/gitlab/-/issues/331001) in GitLab 14.1.
+
+You can implement merge request approvals to require approval by selected users or a group
+when merging a merge request would cause the project's test coverage to decline.
+
+Follow these steps to enable the `Coverage-Check` MR approval rule:
+
+1. Go to your project and select **Settings > General**.
+1. Expand **Merge request approvals**.
+1. Select **Enable** next to the `Coverage-Check` approval rule.
+1. Select the **Target branch**.
+1. Set the number of **Approvals required** to greater than zero.
+1. Select the users or groups to provide approval.
+1. Select **Add approval rule**.
+
+![Coverage-Check approval rule](img/coverage_check_approval_rule_14_1.png)
+
 ### Removing color codes
 
 Some test coverage tools output with ANSI color codes that aren't

doc/user/project/merge_requests/approvals/index.md

@@ -104,6 +104,7 @@ Without the approvals, the work cannot merge. Required approvals enable multiple
   database, for all proposed code changes.
 - Use the [code owners of changed files](rules.md#code-owners-as-eligible-approvers),
   to determine who should review the work.
+- Require an [approval before merging code that causes test coverage to decline](../../../../ci/pipelines/settings.md#coverage-check-approval-rule)
 - [Require approval from a security team](../../../application_security/index.md#security-approvals-in-merge-requests)
   before merging code that could introduce a vulnerability. **(ULTIMATE)**
 

ee/app/assets/javascripts/approvals/components/rule_form.vue

@@ -4,14 +4,21 @@ import { groupBy, isEqual, isNumber } from 'lodash';
 import { mapState, mapActions } from 'vuex';
 import ProtectedBranchesSelector from 'ee/vue_shared/components/branches_selector/protected_branches_selector.vue';
 import { sprintf, __, s__ } from '~/locale';
-import { ANY_BRANCH, TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from '../constants';
+import {
+  ANY_BRANCH,
+  TYPE_USER,
+  TYPE_GROUP,
+  TYPE_HIDDEN_GROUPS,
+  LICENSE_CHECK_NAME,
+  VULNERABILITY_CHECK_NAME,
+  COVERAGE_CHECK_NAME,
+} from '../constants';
 import ApproversList from './approvers_list.vue';
 import ApproversSelect from './approvers_select.vue';
 
 const DEFAULT_NAME = 'Default';
-const DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check';
-const DEFAULT_NAME_FOR_VULNERABILITY_CHECK = 'Vulnerability-Check';
-const READONLY_NAMES = [DEFAULT_NAME_FOR_LICENSE_REPORT, DEFAULT_NAME_FOR_VULNERABILITY_CHECK];
+
+export const READONLY_NAMES = [LICENSE_CHECK_NAME, VULNERABILITY_CHECK_NAME, COVERAGE_CHECK_NAME];
 
 function mapServerResponseToValidationErrors(messages) {
   return Object.entries(messages).flatMap(([key, msgs]) => msgs.map((msg) => `${key} ${msg}`));

ee/app/assets/javascripts/approvals/components/rule_name.vue

@@ -3,6 +3,7 @@ import { GlLink, GlPopover, GlIcon } from '@gitlab/ui';
 import {
   LICENSE_CHECK_NAME,
   VULNERABILITY_CHECK_NAME,
+  COVERAGE_CHECK_NAME,
   APPROVAL_RULE_CONFIGS,
 } from 'ee/approvals/constants';
 
@@ -14,11 +15,12 @@ export default {
   },
   inject: {
     vulnerabilityCheckHelpPagePath: {
-      from: 'vulnerabilityCheckHelpPagePath',
       default: '',
     },
     licenseCheckHelpPagePath: {
-      from: 'licenseCheckHelpPagePath',
+      default: '',
+    },
+    coverageCheckHelpPagePath: {
       default: '',
     },
   },
@@ -39,6 +41,10 @@ export default {
           description: APPROVAL_RULE_CONFIGS[LICENSE_CHECK_NAME].popoverText,
           linkPath: this.licenseCheckHelpPagePath,
         },
+        [COVERAGE_CHECK_NAME]: {
+          description: APPROVAL_RULE_CONFIGS[COVERAGE_CHECK_NAME].popoverText,
+          linkPath: this.coverageCheckHelpPagePath,
+        },
       };
     },
     description() {

ee/app/assets/javascripts/approvals/components/security_configuration/unconfigured_security_rules.vue

@@ -5,6 +5,7 @@ import {
   LICENSE_CHECK_NAME,
   VULNERABILITY_CHECK_NAME,
   LICENSE_SCANNING,
+  COVERAGE_CHECK_NAME,
 } from 'ee/approvals/constants';
 import { s__ } from '~/locale';
 import UnconfiguredSecurityRule from './unconfigured_security_rule.vue';
@@ -16,11 +17,12 @@ export default {
   },
   inject: {
     vulnerabilityCheckHelpPagePath: {
-      from: 'vulnerabilityCheckHelpPagePath',
       default: '',
     },
     licenseCheckHelpPagePath: {
-      from: 'licenseCheckHelpPagePath',
+      default: '',
+    },
+    coverageCheckHelpPagePath: {
       default: '',
     },
   },
@@ -56,6 +58,16 @@ export default {
           ),
           docsPath: this.licenseCheckHelpPagePath,
         },
+        {
+          name: COVERAGE_CHECK_NAME,
+          description: s__(
+            'SecurityApprovals|Test coverage must be enabled. %{linkStart}Learn more%{linkEnd}.',
+          ),
+          enableDescription: s__(
+            'SecurityApprovals|Requires approval for decreases in test coverage. %{linkStart}More information%{linkEnd}',
+          ),
+          docsPath: this.coverageCheckHelpPagePath,
+        },
       ];
     },
     unconfiguredRules() {

ee/app/assets/javascripts/approvals/constants.js

-import { __ } from '~/locale';
+import { __, s__ } from '~/locale';
 
 export const TYPE_USER = 'user';
 export const TYPE_GROUP = 'group';
@@ -19,23 +19,31 @@ export const RULE_NAME_ANY_APPROVER = 'All Members';
 
 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 APPROVAL_RULE_CONFIGS = {
   [VULNERABILITY_CHECK_NAME]: {
-    title: __('Vulnerability-Check'),
-    popoverText: __(
-      'A merge request approval is required when a security report contains a new vulnerability of high, critical, or unknown severity.',
+    title: s__('SecurityApprovals|Vulnerability-Check'),
+    popoverText: s__(
+      'SecurityApprovals|A merge request approval is required when a security report contains a new vulnerability of high, critical, or unknown severity.',
     ),
-    documentationText: __('Learn more about Vulnerability-Check'),
+    documentationText: s__('SecurityApprovals|Learn more about Vulnerability-Check'),
   },
   [LICENSE_CHECK_NAME]: {
-    title: __('License-Check'),
-    popoverText: __(
-      'A merge request approval is required when the license compliance report contains a denied license.',
+    title: s__('SecurityApprovals|License-Check'),
+    popoverText: s__(
+      'SecurityApprovals|A merge request approval is required when the license compliance report contains a denied license.',
     ),
-    documentationText: __('Learn more about License-Check'),
+    documentationText: s__('SecurityApprovals|Learn more about License-Check'),
+  },
+  [COVERAGE_CHECK_NAME]: {
+    title: s__('SecurityApprovals|Coverage-Check'),
+    popoverText: s__(
+      'SecurityApprovals|A merge request approval is required when test coverage declines.',
+    ),
+    documentationText: s__('SecurityApprovals|Learn more about Coverage-Check'),
   },
 };
 

ee/app/assets/javascripts/approvals/mount_project_settings.js

@@ -9,7 +9,11 @@ export default function mountProjectSettingsApprovals(el) {
     return null;
   }
 
-  const { vulnerabilityCheckHelpPagePath, licenseCheckHelpPagePath } = el.dataset;
+  const {
+    vulnerabilityCheckHelpPagePath,
+    licenseCheckHelpPagePath,
+    coverageCheckHelpPagePath,
+  } = el.dataset;
 
   const store = createStore(projectSettingsModule(), {
     ...el.dataset,
@@ -24,6 +28,7 @@ export default function mountProjectSettingsApprovals(el) {
     provide: {
       vulnerabilityCheckHelpPagePath,
       licenseCheckHelpPagePath,
+      coverageCheckHelpPagePath,
     },
     render(h) {
       return h(ProjectSettingsApp);

ee/app/helpers/ee/projects_helper.rb

@@ -80,7 +80,8 @@ module EE
           'security_approvals_help_page_path': help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
           'security_configuration_path': project_security_configuration_path(project),
           'vulnerability_check_help_page_path': help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
-          'license_check_help_page_path': help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project')
+          'license_check_help_page_path': help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
+          'coverage_check_help_page_path': help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
         }
       }
     end

ee/spec/frontend/approvals/components/rule_form_spec.js

@@ -4,7 +4,7 @@ import Vue, { nextTick } from 'vue';
 import Vuex from 'vuex';
 import ApproversList from 'ee/approvals/components/approvers_list.vue';
 import ApproversSelect from 'ee/approvals/components/approvers_select.vue';
-import RuleForm from 'ee/approvals/components/rule_form.vue';
+import RuleForm, { READONLY_NAMES } from 'ee/approvals/components/rule_form.vue';
 import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from 'ee/approvals/constants';
 import { createStoreOptions } from 'ee/approvals/stores';
 import projectSettingsModule from 'ee/approvals/stores/modules/project_settings';
@@ -515,6 +515,7 @@ describe('EE Approvals RuleForm', () => {
         defaultRuleName          | expectedDisabledAttribute
         ${'Vulnerability-Check'} | ${true}
         ${'License-Check'}       | ${true}
+        ${'Coverage-Check'}      | ${true}
         ${'Foo Bar Baz'}         | ${false}
       `(
         'with defaultRuleName set to $defaultRuleName',
@@ -536,52 +537,30 @@ describe('EE Approvals RuleForm', () => {
       );
     });
 
-    describe('with new License-Check rule', () => {
-      beforeEach(() => {
-        createComponent({
-          initRule: { ...TEST_RULE, id: null, name: 'License-Check' },
+    describe('with read-only rule name', () => {
+      describe.each(READONLY_NAMES)('with new %s rule', (ruleName) => {
+        beforeEach(() => {
+          createComponent({
+            initRule: { ...TEST_RULE, id: null, name: ruleName },
+          });
         });
-      });
 
-      it('does not disable the name text field', () => {
-        expect(findNameInput().props('disabled')).toBe(false);
-      });
-    });
-
-    describe('with new Vulnerability-Check rule', () => {
-      beforeEach(() => {
-        createComponent({
-          initRule: { ...TEST_RULE, id: null, name: 'Vulnerability-Check' },
+        it(`it does not disable the name text field`, () => {
+          expect(findNameInput().props('disabled')).toBe(false);
         });
       });
 
-      it('does not disable the name text field', () => {
-        expect(findNameInput().props('disabled')).toBe(false);
-      });
-    });
-
-    describe('with editing the License-Check rule', () => {
-      beforeEach(() => {
-        createComponent({
-          initRule: { ...TEST_RULE, name: 'License-Check' },
+      describe.each(READONLY_NAMES)('with editing the %s rule', (ruleName) => {
+        beforeEach(() => {
+          createComponent({
+            initRule: { ...TEST_RULE, name: ruleName },
+          });
         });
-      });
 
-      it('disables the name text field', () => {
-        expect(findNameInput().props('disabled')).toBe(true);
-      });
-    });
-
-    describe('with editing the Vulnerability-Check rule', () => {
-      beforeEach(() => {
-        createComponent({
-          initRule: { ...TEST_RULE, name: 'Vulnerability-Check' },
+        it(`it disables the name text field`, () => {
+          expect(findNameInput().props('disabled')).toBe(true);
         });
       });
-
-      it('disables the name text field', () => {
-        expect(findNameInput().props('disabled')).toBe(true);
-      });
     });
   });
 

ee/spec/frontend/approvals/components/security_configuration/unconfigured_security_rule_spec.js

@@ -2,7 +2,11 @@ import { GlSprintf, GlButton } from '@gitlab/ui';
 import { mount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
 import UnconfiguredSecurityRule from 'ee/approvals/components/security_configuration/unconfigured_security_rule.vue';
-import { LICENSE_CHECK_NAME, VULNERABILITY_CHECK_NAME } from 'ee/approvals/constants';
+import {
+  LICENSE_CHECK_NAME,
+  VULNERABILITY_CHECK_NAME,
+  COVERAGE_CHECK_NAME,
+} from 'ee/approvals/constants';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -28,6 +32,13 @@ describe('UnconfiguredSecurityRule component', () => {
     docsPath: 'docs/license-check',
   };
 
+  const coverageCheckRule = {
+    name: COVERAGE_CHECK_NAME,
+    description: 'coverage-check description without enable button',
+    enableDescription: 'coverage-check description with enable button',
+    docsPath: 'docs/coverage-check',
+  };
+
   const createWrapper = (props = {}, options = {}) => {
     wrapper = mount(UnconfiguredSecurityRule, {
       localVue,
@@ -44,9 +55,10 @@ describe('UnconfiguredSecurityRule component', () => {
   });
 
   describe.each`
-    rule                | ruleName                 | descriptionText
-    ${licenseCheckRule} | ${licenseCheckRule.name} | ${licenseCheckRule.enableDescription}
-    ${vulnCheckRule}    | ${vulnCheckRule.name}    | ${vulnCheckRule.enableDescription}
+    rule                 | ruleName                  | descriptionText
+    ${licenseCheckRule}  | ${licenseCheckRule.name}  | ${licenseCheckRule.enableDescription}
+    ${vulnCheckRule}     | ${vulnCheckRule.name}     | ${vulnCheckRule.enableDescription}
+    ${coverageCheckRule} | ${coverageCheckRule.name} | ${coverageCheckRule.enableDescription}
   `('with a configured job that is eligible for $ruleName', ({ rule, descriptionText }) => {
     beforeEach(() => {
       createWrapper({
@@ -68,9 +80,10 @@ describe('UnconfiguredSecurityRule component', () => {
   });
 
   describe.each`
-    rule                | ruleName                 | descriptionText
-    ${licenseCheckRule} | ${licenseCheckRule.name} | ${licenseCheckRule.description}
-    ${vulnCheckRule}    | ${vulnCheckRule.name}    | ${vulnCheckRule.description}
+    rule                 | ruleName                  | descriptionText
+    ${licenseCheckRule}  | ${licenseCheckRule.name}  | ${licenseCheckRule.description}
+    ${vulnCheckRule}     | ${vulnCheckRule.name}     | ${vulnCheckRule.description}
+    ${coverageCheckRule} | ${coverageCheckRule.name} | ${coverageCheckRule.description}
   `('with a unconfigured job that is eligible for $ruleName', ({ rule, descriptionText }) => {
     beforeEach(() => {
       createWrapper({

ee/spec/frontend/approvals/components/security_configuration/unconfigured_security_rules_spec.js

@@ -54,7 +54,7 @@ describe('UnconfiguredSecurityRules component', () => {
     });
 
     it('should render a unconfigured-security-rule component for every security rule ', () => {
-      expect(wrapper.findAll(UnconfiguredSecurityRule).length).toBe(2);
+      expect(wrapper.findAll(UnconfiguredSecurityRule).length).toBe(3);
     });
 
     describe('when license_scanning is set to true', () => {

ee/spec/helpers/projects_helper_spec.rb

@@ -397,7 +397,8 @@ RSpec.describe ProjectsHelper do
         security_approvals_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
         security_configuration_path: project_security_configuration_path(project),
         vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
-        license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project')
+        license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
+        coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
       })
     end
   end

locale/gitlab.pot

@@ -1459,12 +1459,6 @@ msgstr ""
 msgid "A member of the abuse team will review your report as soon as possible."
 msgstr ""
 
-msgid "A merge request approval is required when a security report contains a new vulnerability of high, critical, or unknown severity."
-msgstr ""
-
-msgid "A merge request approval is required when the license compliance report contains a denied license."
-msgstr ""
-
 msgid "A merge request hasn't yet been merged"
 msgstr ""
 
@@ -19293,15 +19287,9 @@ msgstr ""
 msgid "Learn more about Auto DevOps"
 msgstr ""
 
-msgid "Learn more about License-Check"
-msgstr ""
-
 msgid "Learn more about Needs relationships"
 msgstr ""
 
-msgid "Learn more about Vulnerability-Check"
-msgstr ""
-
 msgid "Learn more about Web Terminal"
 msgstr ""
 
@@ -19497,9 +19485,6 @@ msgstr ""
 msgid "License overview"
 msgstr ""
 
-msgid "License-Check"
-msgstr ""
-
 msgid "LicenseCompliance|%{docLinkStart}License Approvals%{docLinkEnd} are active"
 msgstr ""
 
@@ -28934,18 +28919,51 @@ msgstr ""
 msgid "Security report is out of date. Run %{newPipelineLinkStart}a new pipeline%{newPipelineLinkEnd} for the target branch (%{targetBranchName})"
 msgstr ""
 
+msgid "SecurityApprovals|A merge request approval is required when a security report contains a new vulnerability of high, critical, or unknown severity."
+msgstr ""
+
+msgid "SecurityApprovals|A merge request approval is required when test coverage declines."
+msgstr ""
+
+msgid "SecurityApprovals|A merge request approval is required when the license compliance report contains a denied license."
+msgstr ""
+
 msgid "SecurityApprovals|Configurable if security scanners are enabled. %{linkStart}Learn more.%{linkEnd}"
 msgstr ""
 
+msgid "SecurityApprovals|Coverage-Check"
+msgstr ""
+
+msgid "SecurityApprovals|Learn more about Coverage-Check"
+msgstr ""
+
+msgid "SecurityApprovals|Learn more about License-Check"
+msgstr ""
+
+msgid "SecurityApprovals|Learn more about Vulnerability-Check"
+msgstr ""
+
 msgid "SecurityApprovals|License Scanning must be enabled. %{linkStart}Learn more%{linkEnd}."
 msgstr ""
 
+msgid "SecurityApprovals|License-Check"
+msgstr ""
+
 msgid "SecurityApprovals|Requires approval for Denied licenses. %{linkStart}More information%{linkEnd}"
 msgstr ""
 
+msgid "SecurityApprovals|Requires approval for decreases in test coverage. %{linkStart}More information%{linkEnd}"
+msgstr ""
+
 msgid "SecurityApprovals|Requires approval for vulnerabilities of Critical, High, or Unknown severity. %{linkStart}Learn more.%{linkEnd}"
 msgstr ""
 
+msgid "SecurityApprovals|Test coverage must be enabled. %{linkStart}Learn more%{linkEnd}."
+msgstr ""
+
+msgid "SecurityApprovals|Vulnerability-Check"
+msgstr ""
+
 msgid "SecurityConfiguration|%{featureName} merge request creation mutation failed"
 msgstr ""
 
@@ -36406,9 +36424,6 @@ msgstr ""
 msgid "Vulnerability resolved in the default branch"
 msgstr ""
 
-msgid "Vulnerability-Check"
-msgstr ""
-
 msgid "VulnerabilityChart|%{formattedStartDate} to today"
 msgstr ""