Implement code review feedback

* Update unit tests
* Update vue components

ee/app/assets/javascripts/approvals/components/project_settings/project_rules.vue

@@ -2,7 +2,8 @@
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import { mapState, mapActions } from 'vuex';
 import { s__, n__, sprintf } from '~/locale';
-import { RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR } from '../../constants';
+import { RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR , LICENSE_CHECK_NAME, VULNERABILITY_CHECK_NAME } from '../../constants';
+
 import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
 import Rules from '../rules.vue';
 import RuleControls from '../rule_controls.vue';
@@ -66,7 +67,7 @@ export default {
     securityRules() {
       return [
         {
-          name: 'Vulnerability-Check',
+          name: VULNERABILITY_CHECK_NAME,
           description: s__(
             'SecurityApprovals|One or more of the security scanners must be enabled %{linkStart}more information%{linkEnd}',
           ),
@@ -76,7 +77,7 @@ export default {
           docsPath: this.vulnerabilityCheckHelpPagePath,
         },
         {
-          name: 'License-Check',
+          name: LICENSE_CHECK_NAME,
           description: s__(
             'SecurityApprovals|License Scanning must be enabled %{linkStart}more information%{linkEnd}',
           ),
@@ -105,7 +106,7 @@ export default {
       immediate: true,
     },
   },
-  mounted() {
+  created() {
     // TODO: Remove feature flag in https://gitlab.com/gitlab-org/gitlab/-/issues/235114
     if (this.isApprovalSuggestionsEnabled) {
       this.setSecurityConfigurationEndpoint(this.securityConfigurationPath);
@@ -212,7 +213,7 @@ export default {
           :rules="rules"
           :is-loading="isRulesLoading"
           :match-rule="securityRule"
-          @enable-btn-clicked="openCreateModal({ name: securityRule.name, initRuleField: true })"
+          @enable="openCreateModal({ name: securityRule.name, initRuleField: true })"
         />
       </template>
     </template>

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

 <script>
 import { camelCase } from 'lodash';
 import { GlButton, GlLink, GlSprintf, GlSkeletonLoading } from '@gitlab/ui';
+import { LICENSE_CHECK_NAME, VULNERABILITY_CHECK_NAME, JOB_TYPES } from 'ee/approvals/constants';
 
 export default {
   components: {
@@ -11,15 +12,15 @@ export default {
   },
   featureTypes: {
     vulnerabilityCheck: [
-      'sast',
-      'dast',
-      'dependency_scanning',
-      'secret_detection',
-      'coverage_fuzzing',
+      JOB_TYPES.SAST,
+      JOB_TYPES.DAST,
+      JOB_TYPES.DEPENDENCY_SCANNING,
+      JOB_TYPES.SECRET_DETECTION,
+      JOB_TYPES.COVERAGE_FUZZING,
     ],
-    licenseCheck: ['license_scanning'],
+    licenseCheck: [JOB_TYPES.LICENSE_SCANNING],
   },
-  securityRules: ['Vulnerability-Check', 'License-Check'],
+  securityRules: [VULNERABILITY_CHECK_NAME, LICENSE_CHECK_NAME],
   props: {
     configuration: {
       type: Object,
@@ -45,13 +46,11 @@ export default {
       }, this);
     },
     hasConfiguredJob() {
-      const { features } = this.configuration;
+      const { features = [] } = this.configuration;
       return this.$options.featureTypes[camelCase(this.matchRule.name)].some(featureType => {
-        return Boolean(
-          features?.some(feature => {
-            return feature.type === featureType && feature.configured;
-          }),
-        );
+        return features.some(feature => {
+          return feature.type === featureType && feature.configured;
+        });
       });
     },
   },
@@ -89,7 +88,7 @@ export default {
           </div>
         </td>
         <td class="gl-px-2! gl-text-right">
-          <gl-button @click="$emit('enable-btn-clicked')">
+          <gl-button @click="$emit('enable')">
             {{ s__('Enable') }}
           </gl-button>
         </td>

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

@@ -14,6 +14,15 @@ export const RULE_NAME_ANY_APPROVER = 'All Members';
 export const VULNERABILITY_CHECK_NAME = 'Vulnerability-Check';
 export const LICENSE_CHECK_NAME = 'License-Check';
 
+export const JOB_TYPES = {
+  SAST: 'sast',
+  DAST: 'dast',
+  DEPENDENCY_SCANNING: 'dependency_scanning',
+  SECRET_DETECTION: 'secret_detection',
+  COVERAGE_FUZZING: 'coverage_fuzzing',
+  LICENSE_SCANNING: 'license_scanning',
+};
+
 export const APPROVAL_RULE_CONFIGS = {
   [VULNERABILITY_CHECK_NAME]: {
     title: __('Vulnerability-Check'),

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

@@ -14,6 +14,11 @@ localVue.use(Vuex);
 describe('Approvals ModalRuleCreate', () => {
   let createModalState;
   let wrapper;
+  let modal;
+  let form;
+
+  const findModal = () => wrapper.find(GlModalVuex);
+  const findForm = () => wrapper.find(RuleForm);
 
   const factory = (options = {}) => {
     const store = new Vuex.Store({
@@ -49,15 +54,13 @@ describe('Approvals ModalRuleCreate', () => {
   describe('without data', () => {
     beforeEach(() => {
       createModalState.data = null;
+      factory();
+      modal = findModal();
+      form = findForm();
     });
 
     it('renders modal', () => {
-      factory();
-
-      const modal = wrapper.find(GlModalVuex);
-
       expect(modal.exists()).toBe(true);
-
       expect(modal.props('modalModule')).toEqual(MODAL_MODULE);
       expect(modal.props('modalId')).toEqual(TEST_MODAL_ID);
       expect(modal.attributes('title')).toEqual('Add approval rule');
@@ -65,22 +68,12 @@ describe('Approvals ModalRuleCreate', () => {
     });
 
     it('renders form', () => {
-      factory();
-
-      const modal = wrapper.find(GlModalVuex);
-      const form = modal.find(RuleForm);
-
       expect(form.exists()).toBe(true);
       expect(form.props('initRule')).toEqual(null);
     });
 
     it('when modal emits ok, submits form', () => {
-      factory();
-
-      const form = wrapper.find(RuleForm);
       form.vm.submit = jest.fn();
-
-      const modal = wrapper.find(GlModalVuex);
       modal.vm.$emit('ok', new Event('ok'));
 
       expect(form.vm.submit).toHaveBeenCalled();
@@ -90,25 +83,18 @@ describe('Approvals ModalRuleCreate', () => {
   describe('with data', () => {
     beforeEach(() => {
       createModalState.data = TEST_RULE;
+      factory();
+      modal = findModal();
+      form = findForm();
     });
 
     it('renders modal', () => {
-      factory();
-
-      const modal = wrapper.find(GlModalVuex);
-
       expect(modal.exists()).toBe(true);
-
       expect(modal.attributes('title')).toEqual('Update approval rule');
       expect(modal.attributes('ok-title')).toEqual('Update approval rule');
     });
 
     it('renders form', () => {
-      factory();
-
-      const modal = wrapper.find(GlModalVuex);
-      const form = modal.find(RuleForm);
-
       expect(form.exists()).toBe(true);
       expect(form.props('initRule')).toEqual(TEST_RULE);
     });
@@ -123,31 +109,24 @@ describe('Approvals ModalRuleCreate', () => {
           glFeatures: { approvalSuggestions: true },
         },
       });
+      modal = findModal();
+      form = findForm();
     });
 
     it('renders add rule modal', () => {
-      const modal = wrapper.find(GlModalVuex);
-
       expect(modal.exists()).toBe(true);
-
       expect(modal.attributes('title')).toEqual('Add approval rule');
       expect(modal.attributes('ok-title')).toEqual('Add approval rule');
     });
 
     it('renders form with initRuleFieldName', () => {
-      const modal = wrapper.find(GlModalVuex);
-      const form = modal.find(RuleForm);
-
       expect(form.props().initRuleFieldName).toBe('Vulnerability-Check');
       expect(form.exists()).toBe(true);
     });
 
     it('renders the form without passing in an existing rule', () => {
-      const modal = wrapper.find(GlModalVuex);
-      const form = modal.find(RuleForm);
-
       expect(form.exists()).toBe(true);
-      expect(form.props('initRule')).toEqual(null);
+      expect(form.props('initRule')).toBeNull();
     });
   });
 });

ee/spec/frontend/approvals/components/project_settings/project_rules_spec.js

@@ -124,9 +124,8 @@ describe('Approvals ProjectRules', () => {
       expect(nameCell.find('.js-help').exists()).toBeFalsy();
     });
 
-    it('should not the unconfigured-security-rule component', () => {
-      const unconfiguredRules = wrapper.find(UnconfiguredSecurityRule);
-      expect(unconfiguredRules.exists()).toBe(false);
+    it('should not render the unconfigured-security-rule component', () => {
+      expect(wrapper.contains(UnconfiguredSecurityRule)).toBe(false);
     });
   });
 
@@ -150,8 +149,7 @@ describe('Approvals ProjectRules', () => {
     });
 
     it('should render the unconfigured-security-rule component', () => {
-      const unconfiguredRules = wrapper.find(UnconfiguredSecurityRule);
-      expect(unconfiguredRules.exists()).toBe(true);
+      expect(wrapper.contains(UnconfiguredSecurityRule)).toBe(true);
     });
   });
 });

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

 import Vuex from 'vuex';
+import { LICENSE_CHECK_NAME, VULNERABILITY_CHECK_NAME } from 'ee/approvals/constants';
 import UnconfiguredSecurityRule from 'ee/approvals/components/security_configuration/unconfigured_security_rule.vue';
 import createStore from 'ee/security_dashboard/store';
 import { mount, createLocalVue } from '@vue/test-utils';
@@ -10,27 +11,31 @@ localVue.use(Vuex);
 describe('UnconfiguredSecurityRule component', () => {
   let wrapper;
   let store;
+  let description;
+
+  const findDescription = () => wrapper.find(GlSprintf);
+  const findButton = () => wrapper.find(GlButton);
 
   const vulnCheckMatchRule = {
-    name: 'Vulnerability-Check',
+    name: VULNERABILITY_CHECK_NAME,
     description: 'vuln-check description without enable button',
     enableDescription: 'vuln-check description with enable button',
     docsPath: 'docs/vuln-check',
   };
 
   const licenseCheckMatchRule = {
-    name: 'License-Check',
+    name: LICENSE_CHECK_NAME,
     description: 'license-check description without enable button',
     enableDescription: 'license-check description with enable button',
     docsPath: 'docs/license-check',
   };
 
   const licenseCheckRule = {
-    name: 'License-Check',
+    name: LICENSE_CHECK_NAME,
   };
 
   const vulnCheckRule = {
-    name: 'Vulnerability-Check',
+    name: VULNERABILITY_CHECK_NAME,
   };
 
   const features = [
@@ -115,7 +120,7 @@ describe('UnconfiguredSecurityRule component', () => {
     });
 
     it('should render the loading skeleton', () => {
-      expect(wrapper.find(GlSkeletonLoading).exists()).toBe(true);
+      expect(wrapper.contains(GlSkeletonLoading)).toBe(true);
     });
   });
 
@@ -131,7 +136,7 @@ describe('UnconfiguredSecurityRule component', () => {
       });
 
       it('should not render the loading skeleton', () => {
-        expect(wrapper.find(GlSkeletonLoading).exists()).toBe(false);
+        expect(wrapper.contains(GlSkeletonLoading)).toBe(false);
       });
 
       it('should not render the row', () => {
@@ -147,16 +152,17 @@ describe('UnconfiguredSecurityRule component', () => {
           matchRule: vulnCheckMatchRule,
           isLoading: false,
         });
+        description = findDescription();
       });
 
       it('should not render the loading skeleton', () => {
-        expect(wrapper.find(GlSkeletonLoading).exists()).toBe(false);
+        expect(wrapper.contains(GlSkeletonLoading)).toBe(false);
       });
 
       it('should render the row with the enable decription and enable button', () => {
-        expect(wrapper.find(GlSprintf).exists()).toBe(true);
-        expect(wrapper.find(GlSprintf).text()).toBe(vulnCheckMatchRule.enableDescription);
-        expect(wrapper.find(GlButton).exists()).toBe(true);
+        expect(description.exists()).toBe(true);
+        expect(description.text()).toBe(vulnCheckMatchRule.enableDescription);
+        expect(findButton().exists()).toBe(true);
       });
     });
   });
@@ -170,16 +176,17 @@ describe('UnconfiguredSecurityRule component', () => {
           matchRule: licenseCheckMatchRule,
           isLoading: false,
         });
+        description = findDescription();
       });
 
       it('should not render the loading skeleton', () => {
-        expect(wrapper.find(GlSkeletonLoading).exists()).toBe(false);
+        expect(wrapper.contains(GlSkeletonLoading)).toBe(false);
       });
 
       it('should render the row with the decription and no button', () => {
-        expect(wrapper.find(GlSprintf).exists()).toBe(true);
-        expect(wrapper.find(GlSprintf).text()).toBe(licenseCheckMatchRule.description);
-        expect(wrapper.find(GlButton).exists()).toBe(false);
+        expect(description.exists()).toBe(true);
+        expect(description.text()).toBe(licenseCheckMatchRule.description);
+        expect(findButton().exists()).toBe(false);
       });
     });
 
@@ -191,16 +198,17 @@ describe('UnconfiguredSecurityRule component', () => {
           matchRule: licenseCheckMatchRule,
           isLoading: false,
         });
+        description = findDescription();
       });
 
       it('should not render the loading skeleton', () => {
-        expect(wrapper.find(GlSkeletonLoading).exists()).toBe(false);
+        expect(wrapper.contains(GlSkeletonLoading)).toBe(false);
       });
 
       it('should render the row with the decription and no button', () => {
-        expect(wrapper.find(GlSprintf).exists()).toBe(true);
-        expect(wrapper.find(GlSprintf).text()).toBe(licenseCheckMatchRule.description);
-        expect(wrapper.find(GlButton).exists()).toBe(false);
+        expect(description.exists()).toBe(true);
+        expect(description.text()).toBe(licenseCheckMatchRule.description);
+        expect(findButton().exists()).toBe(false);
       });
     });
   });