Applies suggestions from @kushalpandya review

- Adds i18n to the new components
- Adds a load of code quality improvements
- Fixes the feedback on the MR page

ee/app/assets/javascripts/security_dashboard/components/app.vue

@@ -51,13 +51,16 @@ export default {
     ...mapState('projects', ['projects']),
     ...mapGetters('filters', ['activeFilters']),
     canCreateIssuePermission() {
-      const path = this.modal.vulnerability.vulnerability_feedback_issue_path;
+      const path = this.vulnerability.vulnerability_feedback_issue_path;
       return _.isString(path) && !_.isEmpty(path);
     },
     canCreateFeedbackPermission() {
-      const path = this.modal.vulnerability.vulnerability_feedback_dismissal_path;
+      const path = this.vulnerability.vulnerability_feedback_dismissal_path;
       return _.isString(path) && !_.isEmpty(path);
     },
+    vulnerability() {
+      return this.modal.vulnerability;
+    },
   },
   created() {
     this.setProjectsEndpoint(this.projectsEndpoint);
@@ -103,10 +106,10 @@ export default {
       :vulnerability-feedback-help-path="vulnerabilityFeedbackHelpPath"
       :can-create-issue-permission="canCreateIssuePermission"
       :can-create-feedback-permission="canCreateFeedbackPermission"
-      @createNewIssue="createIssue({ vulnerability: modal.vulnerability })"
-      @dismissIssue="dismissVulnerability({ vulnerability: modal.vulnerability })"
-      @revertDismissIssue="undoDismiss({ vulnerability: modal.vulnerability })"
-      @createMergeRequest="createMergeRequest({ vulnerability: modal.vulnerability })"
+      @createNewIssue="createIssue({ vulnerability })"
+      @dismissIssue="dismissVulnerability({ vulnerability })"
+      @revertDismissIssue="undoDismiss({ vulnerability })"
+      @createMergeRequest="createMergeRequest({ vulnerability })"
     />
   </div>
 </template>

ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/actions.js

@@ -210,17 +210,23 @@ export const receiveUndoDismissError = ({ commit }, { flashError }) => {
 };
 
 export const createMergeRequest = ({ dispatch }, { vulnerability, flashError }) => {
+  const {
+    report_type,
+    project_fingerprint,
+    vulnerability_feedback_merge_request_path,
+  } = vulnerability;
+
   dispatch('requestCreateMergeRequest');
 
   axios
-    .post(vulnerability.vulnerability_feedback_merge_request_path, {
+    .post(vulnerability_feedback_merge_request_path, {
       vulnerability_feedback: {
         feedback_type: 'merge_request',
-        category: vulnerability.report_type,
-        project_fingerprint: vulnerability.project_fingerprint,
+        category: report_type,
+        project_fingerprint,
         vulnerability_data: {
           ...vulnerability,
-          category: vulnerability.report_type,
+          category: report_type,
         },
       },
     })

ee/app/assets/javascripts/vue_shared/security_reports/components/event_item.vue

 <script>
+import { s__, sprintf } from '~/locale';
 import Icon from '~/vue_shared/components/icon.vue';
 
 export default {
@@ -40,11 +41,11 @@ export default {
   },
   typeMap: {
     issue: {
-      name: 'issue',
+      name: s__('Reports|issue'),
       icon: 'issue-created',
     },
     mergeRequest: {
-      name: 'merge request',
+      name: s__('Reports|merge request'),
       icon: 'merge-request',
     },
   },
@@ -55,6 +56,9 @@ export default {
     iconName() {
       return this.typeData.icon || 'plus';
     },
+    createdText() {
+      return sprintf(s__('ciReport|Created %{eventType}'), { eventType: this.typeData.name });
+    },
   },
 };
 </script>
@@ -70,12 +74,12 @@ export default {
         <em class="js-username">@{{ authorUsername }}</em>
       </div>
       <div>
-        <span v-if="typeData.name" class="js-created">Created {{ typeData.name }}</span>
+        <span v-if="typeData.name" class="js-created">{{ createdText }}</span>
         <a class="js-action-link" :title="actionLinkText" :href="actionLinkUrl">
           {{ actionLinkText }}
         </a>
         <template v-if="projectName">
-          <span>at </span>
+          <span>{{ __('at') }} </span>
           <a class="js-project-name" :title="projectName" :href="projectLink">{{ projectName }}</a>
         </template>
       </div>

ee/app/assets/javascripts/vue_shared/security_reports/components/modal.vue

@@ -48,52 +48,50 @@ export default {
     actionButtons() {
       const buttons = [];
       const issueButton = {
-        name: 'Create issue',
-        tagline: 'Investigate this vulnerability by creating an issue',
+        name: s__('ciReport|Create issue'),
+        tagline: s__('ciReport|Investigate this vulnerability by creating an issue'),
         isLoading: this.modal.isCreatingNewIssue,
         action: 'createNewIssue',
       };
       const MRButton = {
-        name: 'Create merge request',
-        tagline: 'Implement this solution by creating a merge request',
+        name: s__('ciReport|Create merge request'),
+        tagline: s__('ciReport|Implement this solution by creating a merge request'),
         isLoading: this.modal.isCreatingMergeRequest,
         action: 'createMergeRequest',
       };
 
-      if (!this.modal.vulnerability.hasIssue && this.canCreateIssuePermission) {
+      if (!this.vulnerability.hasIssue && this.canCreateIssuePermission) {
         buttons.push(issueButton);
       }
 
-      if (!this.modal.vulnerability.hasMergeRequest) {
+      if (!this.vulnerability.hasMergeRequest) {
         buttons.push(MRButton);
       }
 
       return buttons;
     },
     revertTitle() {
-      return this.modal.vulnerability.isDismissed
+      return this.vulnerability.isDismissed
         ? s__('ciReport|Undo dismiss')
         : s__('ciReport|Dismiss vulnerability');
     },
     hasDismissedBy() {
       return (
-        this.modal.vulnerability &&
-        this.modal.vulnerability.dismissalFeedback &&
-        this.modal.vulnerability.dismissalFeedback.pipeline &&
-        this.modal.vulnerability.dismissalFeedback.author
+        this.vulnerability &&
+        this.vulnerability.dismissalFeedback &&
+        this.vulnerability.dismissalFeedback.pipeline &&
+        this.vulnerability.dismissalFeedback.author
       );
     },
     project() {
       return this.modal.data.project || {};
     },
     solution() {
-      return this.modal.vulnerability && this.modal.vulnerability.solution;
+      return this.vulnerability && this.vulnerability.solution;
     },
     remediation() {
       return (
-        this.modal.vulnerability &&
-        this.modal.vulnerability.remediations &&
-        this.modal.vulnerability.remediations[0]
+        this.vulnerability && this.vulnerability.remediations && this.vulnerability.remediations[0]
       );
     },
     renderSolutionCard() {
@@ -108,10 +106,32 @@ export default {
         (this.canCreateFeedbackPermission || this.canCreateIssuePermission)
       );
     },
+    issueFeedback() {
+      return this.vulnerability && this.vulnerability.issue_feedback;
+    },
+    mergeRequestFeedback() {
+      return this.vulnerability && this.vulnerability.merge_request_feedback;
+    },
+    vulnerability() {
+      return this.modal.vulnerability;
+    },
+    valuedFields() {
+      const { data } = this.modal;
+      const result = {};
+
+      for (let key in data) {
+        // debugger;
+        if (data[key].value && data[key].value.length) {
+          result[key] = data[key];
+        }
+      }
+
+      return result;
+    },
   },
   methods: {
     handleDismissClick() {
-      if (this.modal.vulnerability.isDismissed) {
+      if (this.vulnerability.isDismissed) {
         this.$emit('revertDismissIssue');
       } else {
         this.$emit('dismissIssue');
@@ -147,12 +167,7 @@ export default {
   >
     <slot>
       <div class="border-white mb-0 px-3">
-        <div
-          v-for="(field, key, index) in modal.data"
-          v-if="field.value"
-          :key="index"
-          class="d-flex my-2"
-        >
+        <div v-for="(field, key, index) in valuedFields" :key="index" class="d-flex my-2">
           <label class="col-2 text-right font-weight-bold pl-0">{{ field.text }}:</label>
           <div class="col-10 pl-0 text-secondary">
             <div v-if="hasInstances(field, key)" class="info-well">
@@ -238,30 +253,27 @@ export default {
       <solution-card v-if="renderSolutionCard" :solution="solution" :remediation="remediation" />
       <hr v-else />
 
-      <ul
-        v-if="modal.vulnerability.hasIssue || modal.vulnerability.hasMergeRequest"
-        class="notes card"
-      >
-        <li v-if="modal.vulnerability.hasIssue" class="note">
+      <ul v-if="vulnerability.hasIssue || vulnerability.hasMergeRequest" class="notes card">
+        <li v-if="vulnerability.hasIssue" class="note">
           <event-item
             type="issue"
             :project-name="project.value"
             :project-link="project.url"
-            :author-name="modal.vulnerability.issue_feedback.author.name"
-            :author-username="modal.vulnerability.issue_feedback.author.username"
-            :action-link-text="`#${modal.vulnerability.issue_feedback.issue_iid}`"
-            :action-link-url="modal.vulnerability.issue_feedback.issue_url"
+            :author-name="issueFeedback.author.name"
+            :author-username="issueFeedback.author.username"
+            :action-link-text="`#${issueFeedback.issue_iid}`"
+            :action-link-url="issueFeedback.issue_url"
           />
         </li>
-        <li v-if="modal.vulnerability.hasMergeRequest" class="note">
+        <li v-if="vulnerability.hasMergeRequest" class="note">
           <event-item
             type="mergeRequest"
-            :project-name="modal.data.project.value"
-            :project-link="modal.data.project.url"
-            :author-name="modal.vulnerability.merge_request_feedback.author.name"
-            :author-username="modal.vulnerability.merge_request_feedback.author.username"
-            :action-link-text="`!${modal.vulnerability.merge_request_feedback.merge_request_iid}`"
-            :action-link-url="modal.vulnerability.merge_request_feedback.merge_request_url"
+            :project-name="project.value"
+            :project-link="project.url"
+            :author-name="mergeRequestFeedback.author.name"
+            :author-username="mergeRequestFeedback.author.username"
+            :action-link-text="`!${mergeRequestFeedback.merge_request_iid}`"
+            :action-link-url="mergeRequestFeedback.merge_request_url"
           />
         </li>
       </ul>
@@ -270,12 +282,12 @@ export default {
         <div class="col-sm-12 text-secondary">
           <template v-if="hasDismissedBy">
             {{ s__('ciReport|Dismissed by') }}
-            <a :href="modal.vulnerability.dismissalFeedback.author.web_url" class="pipeline-id">
-              @{{ modal.vulnerability.dismissalFeedback.author.username }}
+            <a :href="vulnerability.dismissalFeedback.author.web_url" class="pipeline-id">
+              @{{ vulnerability.dismissalFeedback.author.username }}
             </a>
             {{ s__('ciReport|on pipeline') }}
-            <a :href="modal.vulnerability.dismissalFeedback.pipeline.path" class="pipeline-id"
-              >#{{ modal.vulnerability.dismissalFeedback.pipeline.id }}</a
+            <a :href="vulnerability.dismissalFeedback.pipeline.path" class="pipeline-id"
+              >#{{ vulnerability.dismissalFeedback.pipeline.id }}</a
             >.
           </template>
           <a
@@ -308,6 +320,7 @@ export default {
         <split-button
           v-if="actionButtons.length > 1"
           :buttons="actionButtons"
+          class="js-split-button"
           @createMergeRequest="$emit('createMergeRequest')"
           @createNewIssue="$emit('createNewIssue')"
         />

ee/app/assets/javascripts/vue_shared/security_reports/components/split_button.vue

 <script>
 import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
+import { s__ } from '~/locale';
 
 export default {
   components: {

ee/app/assets/javascripts/vue_shared/security_reports/store/actions.js

@@ -333,6 +333,7 @@ export const createNewIssue = ({ state, dispatch }) => {
 
 export const createMergeRequest = ({ state, dispatch }) => {
   const { vulnerability } = state.modal;
+  const { category, project_fingerprint } = vulnerability;
 
   dispatch('requestCreateMergeRequest');
 
@@ -340,8 +341,8 @@ export const createMergeRequest = ({ state, dispatch }) => {
     .post(state.vulnerabilityFeedbackPath, {
       vulnerability_feedback: {
         feedback_type: 'merge_request',
-        category: vulnerability.category,
-        project_fingerprint: vulnerability.project_fingerprint,
+        category,
+        project_fingerprint,
         vulnerability_data: vulnerability,
       },
     })

ee/app/assets/javascripts/vue_shared/security_reports/store/state.js

@@ -132,6 +132,7 @@ export default () => ({
     vulnerability: {
       isDismissed: false,
       hasIssue: false,
+      hasMergeRequest: false,
     },
 
     isCreatingNewIssue: false,

ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js

@@ -24,17 +24,17 @@ const hasMatchingFix = (fixes, vulnerability) =>
 
 /**
  *
- * Returns the remediations that fix the given vulnerability or null
+ * Returns the remediations that fix the given vulnerability
  *
  * @param {Array} remediations
  * @param {Object} vulnerability
- * @returns {Array|null}
+ * @returns {Array}
  */
 export const findMatchingRemediations = (remediations, vulnerability) => {
   if (!Array.isArray(remediations)) {
-    return null;
+    return [];
   }
-  return remediations.filter(rem => hasMatchingFix(rem.fixes, vulnerability)) || null;
+  return remediations.filter(rem => hasMatchingFix(rem.fixes, vulnerability));
 };
 
 /**
@@ -59,6 +59,12 @@ function enrichVulnerabilityWithfeedback(vulnerability, feedback = []) {
           hasIssue: true,
           issue_feedback: fb,
         };
+      } else if (fb.feedback_type === 'merge_request') {
+        return {
+          ...vuln,
+          hasMergeRequest: true,
+          merge_request_feedback: fb,
+        };
       }
       return vuln;
     }, vulnerability);

ee/spec/javascripts/vue_shared/security_reports/components/modal_spec.js

@@ -97,20 +97,14 @@ describe('Security Reports modal', () => {
         expect(vm.$el.querySelector('.js-dismiss-btn')).toBe(null);
       });
 
-      // TODO: Work out how to properly test this
-      xit('renders create issue button', () => {
-        expect(vm.$el.querySelector('.js-create-issue-btn')).not.toBe(null);
-      });
-
       it('renders the footer', () => {
         expect(vm.$el.classList.contains('modal-hide-footer')).toEqual(false);
       });
 
-      // TODO: Work out how to properly test this
-      xit('emits createIssue when create issue button is clicked', () => {
+      it('emits createIssue when create issue button is clicked', () => {
         spyOn(vm, '$emit');
 
-        const button = vm.$el.querySelector('.js-create-issue-btn');
+        const button = vm.$el.querySelector('.js-split-button').querySelector('.btn-success');
         button.click();
 
         expect(vm.$emit).toHaveBeenCalledWith('createNewIssue');

ee/spec/javascripts/vue_shared/security_reports/components/split_button_spec.js

@@ -2,7 +2,7 @@ import Vue from 'vue';
 import component from 'ee/vue_shared/security_reports/components/split_button.vue';
 import mountComponent from 'spec/helpers/vue_mount_component_helper';
 
-describe('Event Item', () => {
+describe('Split Button', () => {
   const Component = Vue.extend(component);
   const buttons = [
     {
@@ -42,22 +42,15 @@ describe('Event Item', () => {
     });
 
     it('displays the first button initially', () => {
-      // TODO: Workout what the selector is
-      expect(vm.$el.querySelector(''));
-    });
-
-    it('displays the second button when selected', () => {
-      vm.$el.querySelectorAll('.dropdown-item')[1].click();
-
-      // TODO: Workout what the selector is
-      expect(vm.$el.querySelector(''));
+      expect(vm.$el.querySelector('.btn').textContent.trim()).toBe(buttons[0].name);
     });
 
     it('emits the correct event when the button is pressed', () => {
-      vm.$el.querySelector('the button').click();
+      spyOn(vm, '$emit');
+
+      vm.$el.querySelector('.btn').click();
 
-      // TODO: work out how to test the emitted event
-      expect('the event to be emmitted');
+      expect(vm.$emit).toHaveBeenCalledWith(buttons[0].action);
     });
   });
 

locale/gitlab.pot

@@ -3002,9 +3002,6 @@ msgstr ""
 msgid "Create group label"
 msgstr ""
 
-msgid "Create issue"
-msgstr ""
-
 msgid "Create lists from labels. Issues with that label appear in that list."
 msgstr ""
 
@@ -8324,6 +8321,12 @@ msgstr ""
 msgid "Reports|Vulnerability"
 msgstr ""
 
+msgid "Reports|issue"
+msgstr ""
+
+msgid "Reports|merge request"
+msgstr ""
+
 msgid "Reports|no changed test results"
 msgstr ""
 
@@ -8750,6 +8753,9 @@ msgstr ""
 msgid "Security Reports|There was an error creating the issue."
 msgstr ""
 
+msgid "Security Reports|There was an error creating the merge request."
+msgstr ""
+
 msgid "Security Reports|There was an error dismissing the vulnerability."
 msgstr ""
 
@@ -10958,9 +10964,6 @@ msgstr ""
 msgid "View in Sentry"
 msgstr ""
 
-msgid "View issue"
-msgstr ""
-
 msgid "View it on GitLab"
 msgstr ""
 
@@ -11528,6 +11531,9 @@ msgstr ""
 msgid "assign yourself"
 msgstr ""
 
+msgid "at"
+msgstr ""
+
 msgid "attach a new file"
 msgstr ""
 
@@ -11626,6 +11632,15 @@ msgstr ""
 msgid "ciReport|Container scanning detects known vulnerabilities in your docker images."
 msgstr ""
 
+msgid "ciReport|Create issue"
+msgstr ""
+
+msgid "ciReport|Create merge request"
+msgstr ""
+
+msgid "ciReport|Created %{eventType}"
+msgstr ""
+
 msgid "ciReport|DAST"
 msgstr ""
 
@@ -11668,9 +11683,15 @@ msgstr ""
 msgid "ciReport|Identifiers"
 msgstr ""
 
+msgid "ciReport|Implement this solution by creating a merge request"
+msgstr ""
+
 msgid "ciReport|Instances"
 msgstr ""
 
+msgid "ciReport|Investigate this vulnerability by creating an issue"
+msgstr ""
+
 msgid "ciReport|Learn more about interacting with security reports (Alpha)."
 msgstr ""
 
@@ -11735,6 +11756,9 @@ msgstr ""
 msgid "ciReport|There was an error creating the issue. Please try again."
 msgstr ""
 
+msgid "ciReport|There was an error creating the merge request. Please try again."
+msgstr ""
+
 msgid "ciReport|There was an error dismissing the vulnerability. Please try again."
 msgstr ""
 
@@ -12260,6 +12284,9 @@ msgstr[1] ""
 msgid "score"
 msgstr ""
 
+msgid "security Reports|There was an error creating the merge request"
+msgstr ""
+
 msgid "should be higher than %{access} inherited membership from group %{group_name}"
 msgstr ""