From a40cea58207d579d7bdde28baaea26279d0eb647 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Wed, 13 Mar 2019 13:52:19 -0500
Subject: [PATCH 01/22] Sec dashboards can load with deleted issuables

The security dashboards can now load when an issue / merge request
related to a vulnerability has been deleted. This does not address the
ability to create a new issue / merge request for a vulnerability after
it has been deleted.
---
 ee/app/serializers/vulnerabilities/feedback_entity.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ee/app/serializers/vulnerabilities/feedback_entity.rb b/ee/app/serializers/vulnerabilities/feedback_entity.rb
index 18239da5c287cf..f94a60dd3427c8 100644
--- a/ee/app/serializers/vulnerabilities/feedback_entity.rb
+++ b/ee/app/serializers/vulnerabilities/feedback_entity.rb
@@ -17,19 +17,19 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
     end
   end
 
-  expose :issue_iid, if: -> (feedback, _) { feedback.issue? } do |feedback|
+  expose :issue_iid, if: -> (feedback, _) { feedback.issue? && feedback.issue_id } do |feedback|
     feedback.issue.iid
   end
 
-  expose :issue_url, if: -> (feedback, _) { feedback.issue? } do |feedback|
+  expose :issue_url, if: -> (feedback, _) { feedback.issue? && feedback.issue_id } do |feedback|
     project_issue_url(feedback.project, feedback.issue)
   end
 
-  expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
+  expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? && feedback.merge_request_id } do |feedback|
     feedback.merge_request.iid
   end
 
-  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request? } do |feedback|
+  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request? && feedback.merge_request_id } do |feedback|
     project_merge_request_path(feedback.project, feedback.merge_request)
   end
 
-- 
GitLab


From 9684f4d87f821f26625fe1b8579b795bde24b3da Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Wed, 13 Mar 2019 13:55:34 -0500
Subject: [PATCH 02/22] Allow re-creation of issues/MRs from Sec Dashboard

In the Projects::VulnerabilityFeedbackController#index, bringing back
feedback with a nil issue_id causes incorrect behaviour for showing the
New/Create links.
VulnerabilityFeedbackModule::CreateService updated due to uniquness
constraints.
---
 .../controllers/projects/vulnerability_feedback_controller.rb  | 3 +++
 .../services/vulnerability_feedback_module/create_service.rb   | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ee/app/controllers/projects/vulnerability_feedback_controller.rb b/ee/app/controllers/projects/vulnerability_feedback_controller.rb
index 3bc09f3e4c634d..b58e7235fbc46c 100644
--- a/ee/app/controllers/projects/vulnerability_feedback_controller.rb
+++ b/ee/app/controllers/projects/vulnerability_feedback_controller.rb
@@ -12,6 +12,9 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
   def index
     # TODO: Move to finder or list service
     @vulnerability_feedback = @project.vulnerability_feedback.with_associations
+        .where("feedback_type = 0
+                or ( feedback_type = 1 and issue_id is not null )
+                or ( feedback_type = 2 and merge_request_id is not null ) ")
     if params[:category].present?
       @vulnerability_feedback = @vulnerability_feedback
         .where(category: Vulnerabilities::Feedback.categories[params[:category]])
diff --git a/ee/app/services/vulnerability_feedback_module/create_service.rb b/ee/app/services/vulnerability_feedback_module/create_service.rb
index 63667e912bb27e..c7bec18e9b091a 100644
--- a/ee/app/services/vulnerability_feedback_module/create_service.rb
+++ b/ee/app/services/vulnerability_feedback_module/create_service.rb
@@ -3,7 +3,8 @@
 module VulnerabilityFeedbackModule
   class CreateService < ::BaseService
     def execute
-      vulnerability_feedback = @project.vulnerability_feedback.new(@params)
+      vulnerability_feedback = @project.vulnerability_feedback.find_or_initialize_by(project_id: @project.id, category: @params[:category], feedback_type: @params[:feedback_type], project_fingerprint: @params[:project_fingerprint] )
+      vulnerability_feedback.vulnerability_data = @params[:vulnerability_data]
       vulnerability_feedback.author = @current_user
 
       if vulnerability_feedback.issue? &&
-- 
GitLab


From aba56d2fbc32406be18ab9131230a088a2d6cda2 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Fri, 15 Mar 2019 08:51:25 -0500
Subject: [PATCH 03/22] pipeline_id must be specified

---
 ee/app/services/vulnerability_feedback_module/create_service.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ee/app/services/vulnerability_feedback_module/create_service.rb b/ee/app/services/vulnerability_feedback_module/create_service.rb
index c7bec18e9b091a..4dabc773be0202 100644
--- a/ee/app/services/vulnerability_feedback_module/create_service.rb
+++ b/ee/app/services/vulnerability_feedback_module/create_service.rb
@@ -5,6 +5,7 @@ class CreateService < ::BaseService
     def execute
       vulnerability_feedback = @project.vulnerability_feedback.find_or_initialize_by(project_id: @project.id, category: @params[:category], feedback_type: @params[:feedback_type], project_fingerprint: @params[:project_fingerprint] )
       vulnerability_feedback.vulnerability_data = @params[:vulnerability_data]
+      vulnerability_feedback.pipeline_id = @params[:pipeline_id]
       vulnerability_feedback.author = @current_user
 
       if vulnerability_feedback.issue? &&
-- 
GitLab


From 0c916b573d2aa013491707e558c1bd2517efd24f Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Fri, 15 Mar 2019 08:54:33 -0500
Subject: [PATCH 04/22] issue_feedback must have an issue_iid to be valid

Issues that were associated with a vulnerability can be deleted, but
this leaves the feedback record behind, this puts the issue_feedback in a bad
state. This allows a new issue to be created from the row for the
vulnerability on the group security dasboard.
---
 .../components/security_dashboard_table_row.vue                 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue b/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue
index 8bee8fad9bda3b..0437d36f045baf 100644
--- a/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue
+++ b/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue
@@ -42,7 +42,7 @@ export default {
       return Boolean(this.vulnerability.dismissal_feedback);
     },
     hasIssue() {
-      return Boolean(this.vulnerability.issue_feedback);
+      return Boolean(this.vulnerability.issue_feedback && this.vulnerability.issue_feedback.issue_iid);
     },
     canDismissVulnerability() {
       const path = this.vulnerability.vulnerability_feedback_dismissal_path;
-- 
GitLab


From 8594d12bcf4821c0188f24104b00cf9c005f9f99 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 09:13:59 -0500
Subject: [PATCH 05/22] Moved to filtering to the frontend code

This wasn't the only place that the issues/MRs needed to be filtered
like this, so it was easier to do the filtering elsewhere.
---
 .../controllers/projects/vulnerability_feedback_controller.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ee/app/controllers/projects/vulnerability_feedback_controller.rb b/ee/app/controllers/projects/vulnerability_feedback_controller.rb
index b58e7235fbc46c..57064a125f91ac 100644
--- a/ee/app/controllers/projects/vulnerability_feedback_controller.rb
+++ b/ee/app/controllers/projects/vulnerability_feedback_controller.rb
@@ -12,9 +12,7 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
   def index
     # TODO: Move to finder or list service
     @vulnerability_feedback = @project.vulnerability_feedback.with_associations
-        .where("feedback_type = 0
-                or ( feedback_type = 1 and issue_id is not null )
-                or ( feedback_type = 2 and merge_request_id is not null ) ")
+
     if params[:category].present?
       @vulnerability_feedback = @vulnerability_feedback
         .where(category: Vulnerabilities::Feedback.categories[params[:category]])
-- 
GitLab


From 551641dd8a81eaf02ea3737065613e071e5b6235 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 09:17:01 -0500
Subject: [PATCH 06/22] Issue/MR feedback is valid if issue/MR is present

An issue or an MR related to a vulnerability feedback record can be
deleted but the feedback record remains. We need to handle this so that
a new issue or MR can be created and so that we don't try to link to the
non-existent issue/MR.
---
 .../store/modules/vulnerabilities/mutations.js                | 4 ++--
 .../javascripts/vue_shared/security_reports/store/utils.js    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js
index 0200dc05423e62..bb3cffcda043ce 100644
--- a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js
+++ b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js
@@ -93,11 +93,11 @@ export default {
     Vue.set(state.modal.data.reportType, 'value', vulnerability.report_type);
     Vue.set(state.modal.data.confidence, 'value', vulnerability.confidence);
     Vue.set(state.modal, 'vulnerability', vulnerability);
-    Vue.set(state.modal.vulnerability, 'hasIssue', Boolean(vulnerability.issue_feedback));
+    Vue.set(state.modal.vulnerability, 'hasIssue', Boolean(vulnerability.issue_feedback && vulnerability.issue_feedback.issue_iid));
     Vue.set(
       state.modal.vulnerability,
       'hasMergeRequest',
-      Boolean(vulnerability.merge_request_feedback),
+      Boolean(vulnerability.merge_request_feedback && vulnerability.merge_request.merge_request_iid),
     );
     Vue.set(state.modal.vulnerability, 'isDismissed', Boolean(vulnerability.dismissal_feedback));
     Vue.set(state.modal, 'error', null);
diff --git a/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js b/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js
index a01964b16bea9d..1d942fa3fda3c5 100644
--- a/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js
+++ b/ee/app/assets/javascripts/vue_shared/security_reports/store/utils.js
@@ -53,13 +53,13 @@ function enrichVulnerabilityWithfeedback(vulnerability, feedback = []) {
           isDismissed: true,
           dismissalFeedback: fb,
         };
-      } else if (fb.feedback_type === 'issue') {
+      } else if (fb.feedback_type === 'issue' && fb.issue_iid) {
         return {
           ...vuln,
           hasIssue: true,
           issue_feedback: fb,
         };
-      } else if (fb.feedback_type === 'merge_request') {
+      } else if (fb.feedback_type === 'merge_request' && fb.merge_request_iid) {
         return {
           ...vuln,
           hasMergeRequest: true,
-- 
GitLab


From cc27dee83f9725870752ad07aa3a04e24fb3a6e8 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 09:26:44 -0500
Subject: [PATCH 07/22] Pretty print Frontend files

---
 .../components/security_dashboard_table_row.vue        |  4 +++-
 .../store/modules/vulnerabilities/mutations.js         | 10 ++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue b/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue
index 0437d36f045baf..0c144e93f9f477 100644
--- a/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue
+++ b/ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue
@@ -42,7 +42,9 @@ export default {
       return Boolean(this.vulnerability.dismissal_feedback);
     },
     hasIssue() {
-      return Boolean(this.vulnerability.issue_feedback && this.vulnerability.issue_feedback.issue_iid);
+      return Boolean(
+        this.vulnerability.issue_feedback && this.vulnerability.issue_feedback.issue_iid,
+      );
     },
     canDismissVulnerability() {
       const path = this.vulnerability.vulnerability_feedback_dismissal_path;
diff --git a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js
index bb3cffcda043ce..1ba221a970ff60 100644
--- a/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js
+++ b/ee/app/assets/javascripts/security_dashboard/store/modules/vulnerabilities/mutations.js
@@ -93,11 +93,17 @@ export default {
     Vue.set(state.modal.data.reportType, 'value', vulnerability.report_type);
     Vue.set(state.modal.data.confidence, 'value', vulnerability.confidence);
     Vue.set(state.modal, 'vulnerability', vulnerability);
-    Vue.set(state.modal.vulnerability, 'hasIssue', Boolean(vulnerability.issue_feedback && vulnerability.issue_feedback.issue_iid));
+    Vue.set(
+      state.modal.vulnerability,
+      'hasIssue',
+      Boolean(vulnerability.issue_feedback && vulnerability.issue_feedback.issue_iid),
+    );
     Vue.set(
       state.modal.vulnerability,
       'hasMergeRequest',
-      Boolean(vulnerability.merge_request_feedback && vulnerability.merge_request.merge_request_iid),
+      Boolean(
+        vulnerability.merge_request_feedback && vulnerability.merge_request.merge_request_iid,
+      ),
     );
     Vue.set(state.modal.vulnerability, 'isDismissed', Boolean(vulnerability.dismissal_feedback));
     Vue.set(state.modal, 'error', null);
-- 
GitLab


From 31c2c1c43de7472a3545d21bca6d0457bfa8ab70 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 09:27:39 -0500
Subject: [PATCH 08/22] Test feedback.issue directly and add tests

---
 .../vulnerabilities/feedback_entity.rb        |  8 ++--
 .../vulnerabilities/feedback_entity_spec.rb   | 45 +++++++++++++++++++
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/ee/app/serializers/vulnerabilities/feedback_entity.rb b/ee/app/serializers/vulnerabilities/feedback_entity.rb
index f94a60dd3427c8..131698ba58c08b 100644
--- a/ee/app/serializers/vulnerabilities/feedback_entity.rb
+++ b/ee/app/serializers/vulnerabilities/feedback_entity.rb
@@ -17,19 +17,19 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
     end
   end
 
-  expose :issue_iid, if: -> (feedback, _) { feedback.issue? && feedback.issue_id } do |feedback|
+  expose :issue_iid, if: -> (feedback, _) { feedback.issue.present? } do |feedback|
     feedback.issue.iid
   end
 
-  expose :issue_url, if: -> (feedback, _) { feedback.issue? && feedback.issue_id } do |feedback|
+  expose :issue_url, if: -> (feedback, _) { feedback.issue.present? } do |feedback|
     project_issue_url(feedback.project, feedback.issue)
   end
 
-  expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request? && feedback.merge_request_id } do |feedback|
+  expose :merge_request_iid, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback|
     feedback.merge_request.iid
   end
 
-  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request? && feedback.merge_request_id } do |feedback|
+  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback|
     project_merge_request_path(feedback.project, feedback.merge_request)
   end
 
diff --git a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
index 983745cd009f57..8adc031f01be27 100644
--- a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
+++ b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
@@ -11,5 +11,50 @@
     subject { entity.as_json }
 
     it { is_expected.to include(:project_id, :author, :category, :feedback_type) }
+
+  end
+
+  context 'when issue is present' do
+    let(:feedback) { build(:vulnerability_feedback, :issue ) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+    it 'exposes issue information' do
+      expect(subject).to include(:issue_iid)
+      expect(subject).to include(:issue_url)
+    end
+  end
+
+  context 'when issue is not present' do
+    let(:feedback) { build(:vulnerability_feedback, feedback_type: 'issue', issue: nil) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+    it 'does not expose issue information' do
+      expect(subject).not_to include(:issue_iid)
+      expect(subject).not_to include(:issue_url)
+    end
+  end
+
+  context 'when merge request is present' do
+    let(:feedback) { build(:vulnerability_feedback, :merge_request ) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+    it 'exposes merge request information' do
+      expect(subject).to include(:merge_request_iid)
+      expect(subject).to include(:merge_request_path)
+    end
+  end
+
+  context 'when merge request is not present' do
+    let(:feedback) { build(:vulnerability_feedback, feedback_type: 'merge_request', merge_request: nil) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+    it 'does not expose merge request information' do
+      expect(subject).not_to include(:merge_request_iid)
+      expect(subject).not_to include(:merge_request_path)
+    end
   end
 end
-- 
GitLab


From 622e9b5da77afdac6be596bf55f3d2b190b99b25 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 09:28:19 -0500
Subject: [PATCH 09/22] Move AR logic to the model and add tests

---
 ee/app/models/vulnerabilities/feedback.rb     | 23 ++++++++++++++++
 .../create_service.rb                         |  4 +--
 .../create_service_spec.rb                    | 26 +++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/ee/app/models/vulnerabilities/feedback.rb b/ee/app/models/vulnerabilities/feedback.rb
index 019f9ed04f87b8..5f3b05d86a8144 100644
--- a/ee/app/models/vulnerabilities/feedback.rb
+++ b/ee/app/models/vulnerabilities/feedback.rb
@@ -29,5 +29,28 @@ class Feedback < ActiveRecord::Base
     scope :all_preloaded, -> do
       preload(:author, :project, :issue, :merge_request, :pipeline)
     end
+
+    def self.find_or_init_for(feedback_params)
+      validate_enums(feedback_params)
+
+      vulnerability_feedback = find_or_initialize_by(category: feedback_params[:category], feedback_type: feedback_params[:feedback_type], project_fingerprint: feedback_params[:project_fingerprint] )
+      vulnerability_feedback.vulnerability_data = feedback_params[:vulnerability_data]
+      vulnerability_feedback.pipeline_id = feedback_params[:pipeline_id]
+      vulnerability_feedback
+    end
+
+    private
+
+    # Rails 5.0 does not properly handle validation of enums in select queries such as find_or_initialize_by.
+    # This method, and calls to it can be removed when we are on Rails 5.2.
+    def self.validate_enums(feedback_params)
+      ['feedback_type', 'category'].each do |enum|
+        unless send(enum.pluralize).include? feedback_params[enum.to_sym]
+          raise ArgumentError.new("'#{feedback_params[enum.to_sym]}' is not a valid #{enum}")
+        end
+      end
+    end
+
   end
+
 end
diff --git a/ee/app/services/vulnerability_feedback_module/create_service.rb b/ee/app/services/vulnerability_feedback_module/create_service.rb
index 4dabc773be0202..b55dcc3958d1cd 100644
--- a/ee/app/services/vulnerability_feedback_module/create_service.rb
+++ b/ee/app/services/vulnerability_feedback_module/create_service.rb
@@ -3,9 +3,7 @@
 module VulnerabilityFeedbackModule
   class CreateService < ::BaseService
     def execute
-      vulnerability_feedback = @project.vulnerability_feedback.find_or_initialize_by(project_id: @project.id, category: @params[:category], feedback_type: @params[:feedback_type], project_fingerprint: @params[:project_fingerprint] )
-      vulnerability_feedback.vulnerability_data = @params[:vulnerability_data]
-      vulnerability_feedback.pipeline_id = @params[:pipeline_id]
+      vulnerability_feedback = @project.vulnerability_feedback.find_or_init_for(@params)
       vulnerability_feedback.author = @current_user
 
       if vulnerability_feedback.issue? &&
diff --git a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
index 419887154e11a3..ca1eac87540343 100644
--- a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
+++ b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
@@ -74,6 +74,16 @@
         expect(feedback.merge_request).to be_nil
       end
 
+      it 'updates the feedback when it already exists' do
+        feedback = result[:vulnerability_feedback]
+        expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count)
+      end
+
+      it 'creates an issue when feedback already exists' do
+        feedback = result[:vulnerability_feedback]
+        expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1)
+      end
+
       it 'delegates the Issue creation to CreateFromVulnerabilityDataService' do
         expect_any_instance_of(Issues::CreateFromVulnerabilityDataService)
           .to receive(:execute).once.and_call_original
@@ -191,5 +201,21 @@
         expect(result[:message]).to eq("'foo' is not a valid feedback_type")
       end
     end
+
+    context 'when category is invalid' do
+      let(:feedback_params) do
+        {
+          feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'foo',
+          project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8'
+        }
+      end
+
+      let(:result) { described_class.new(project, user, feedback_params).execute }
+
+      it 'returns error with correct message' do
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("'foo' is not a valid category")
+      end
+    end
   end
 end
-- 
GitLab


From 48f36765f4e5beb7ec01a58e03fa5ea021b4cf2f Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 13:19:08 -0500
Subject: [PATCH 10/22] Address rubocop feedback

Removing the send method adds some duplication, but the whole
validate_enums method can be removed when we move to rails 5.2
---
 ee/app/models/vulnerabilities/feedback.rb | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/ee/app/models/vulnerabilities/feedback.rb b/ee/app/models/vulnerabilities/feedback.rb
index 5f3b05d86a8144..c1c0d7aeccefe6 100644
--- a/ee/app/models/vulnerabilities/feedback.rb
+++ b/ee/app/models/vulnerabilities/feedback.rb
@@ -39,18 +39,17 @@ def self.find_or_init_for(feedback_params)
       vulnerability_feedback
     end
 
-    private
-
     # Rails 5.0 does not properly handle validation of enums in select queries such as find_or_initialize_by.
     # This method, and calls to it can be removed when we are on Rails 5.2.
     def self.validate_enums(feedback_params)
-      ['feedback_type', 'category'].each do |enum|
-        unless send(enum.pluralize).include? feedback_params[enum.to_sym]
-          raise ArgumentError.new("'#{feedback_params[enum.to_sym]}' is not a valid #{enum}")
-        end
+      unless feedback_types.include? feedback_params[:feedback_type]
+
+        raise ArgumentError.new("'#{feedback_params[:feedback_type]}' is not a valid feedback_type")
       end
-    end
 
+      unless categories.include? feedback_params[:category]
+        raise ArgumentError.new("'#{feedback_params[:category]}' is not a valid category")
+      end
+    end
   end
-
 end
-- 
GitLab


From 38611b098d75c12be997724708e50646be8e2224 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 13:22:02 -0500
Subject: [PATCH 11/22] Update mock data for specs to reference issue_iid

---
 .../vue_shared/security_reports/mock_data.js     | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ee/spec/javascripts/vue_shared/security_reports/mock_data.js b/ee/spec/javascripts/vue_shared/security_reports/mock_data.js
index ec29476dc39a49..81645ee158f310 100644
--- a/ee/spec/javascripts/vue_shared/security_reports/mock_data.js
+++ b/ee/spec/javascripts/vue_shared/security_reports/mock_data.js
@@ -1196,7 +1196,7 @@ export const sastFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'sast',
     feedback_type: 'dismissal',
@@ -1207,7 +1207,7 @@ export const sastFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'sast',
     feedback_type: 'issue',
@@ -1221,7 +1221,7 @@ export const dependencyScanningFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'dependency_scanning',
     feedback_type: 'dismissal',
@@ -1232,7 +1232,7 @@ export const dependencyScanningFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'dependency_scanning',
     feedback_type: 'issue',
@@ -1246,7 +1246,7 @@ export const dastFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'dismissal',
@@ -1257,7 +1257,7 @@ export const dastFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'issue',
@@ -1271,7 +1271,7 @@ export const containerScanningFeedbacks = [
     id: 3,
     project_id: 17,
     author_id: 1,
-    issue_id: null,
+    issue_iid: null,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'dismissal',
@@ -1282,7 +1282,7 @@ export const containerScanningFeedbacks = [
     id: 4,
     project_id: 17,
     author_id: 1,
-    issue_id: 123,
+    issue_iid: 123,
     pipeline_id: 132,
     category: 'container_scanning',
     feedback_type: 'issue',
-- 
GitLab


From 0232297ee12b8cd1e54a0438b05ae5f50de0df81 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 15:03:06 -0500
Subject: [PATCH 12/22] We new check for vulnerability feedback issue_iid

---
 .../store/vulnerabilities/mutations_spec.js              | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
index e5a41f8e76c989..8ae3169ef0b952 100644
--- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
+++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
@@ -314,7 +314,14 @@ describe('vulnerabilities module mutations', () => {
       });
 
       it('should set hasIssue when the vulnerabilitiy has a related issue', () => {
-        const payload = { vulnerability: { ...vulnerability, issue_feedback: 'I am an issue' } };
+        const payload = {
+          vulnerability: {
+            ...vulnerability,
+            issue_feedback: {
+              issue_iid: 123
+            }
+          }
+        };
         mutations[types.SET_MODAL_DATA](state, payload);
 
         expect(state.modal.vulnerability.hasIssue).toEqual(true);
-- 
GitLab


From bd44b8f11a20c0fab07fe4ed3d0e3430fa1acacd Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 15:04:55 -0500
Subject: [PATCH 13/22] Address more rubocop warnings

---
 ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb     | 1 -
 .../ee/vulnerability_feedback_module/create_service_spec.rb     | 2 --
 2 files changed, 3 deletions(-)

diff --git a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
index 8adc031f01be27..978fd49a332f82 100644
--- a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
+++ b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
@@ -11,7 +11,6 @@
     subject { entity.as_json }
 
     it { is_expected.to include(:project_id, :author, :category, :feedback_type) }
-
   end
 
   context 'when issue is present' do
diff --git a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
index ca1eac87540343..67b53f58324368 100644
--- a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
+++ b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
@@ -75,12 +75,10 @@
       end
 
       it 'updates the feedback when it already exists' do
-        feedback = result[:vulnerability_feedback]
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count)
       end
 
       it 'creates an issue when feedback already exists' do
-        feedback = result[:vulnerability_feedback]
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1)
       end
 
-- 
GitLab


From 1585ace5032517388be588e2b5b58381349e1f46 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 19 Mar 2019 16:26:24 -0500
Subject: [PATCH 14/22] Needed to run prettier on mutations_spec

---
 .../store/vulnerabilities/mutations_spec.js                 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
index 8ae3169ef0b952..ea73697e0e8422 100644
--- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
+++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
@@ -318,9 +318,9 @@ describe('vulnerabilities module mutations', () => {
           vulnerability: {
             ...vulnerability,
             issue_feedback: {
-              issue_iid: 123
-            }
-          }
+              issue_iid: 123,
+            },
+          },
         };
         mutations[types.SET_MODAL_DATA](state, payload);
 
-- 
GitLab


From 3065eaadb0188bdec29526b7773383a7325b6e48 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Wed, 20 Mar 2019 11:05:41 -0500
Subject: [PATCH 15/22] Need to reference result so it will be evaluated

---
 .../ee/vulnerability_feedback_module/create_service_spec.rb      | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
index 67b53f58324368..5049342de33855 100644
--- a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
+++ b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
@@ -75,6 +75,7 @@
       end
 
       it 'updates the feedback when it already exists' do
+        result[:vulnerability_feedback]
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count)
       end
 
-- 
GitLab


From 9ac345bb01ea7453b1bdbb8250d74743c33525df Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Thu, 21 Mar 2019 08:13:25 -0500
Subject: [PATCH 16/22] Add and improve specs

---
 .../models/vulnerabilities/feedback_spec.rb   | 53 +++++++++++++++++++
 .../create_service_spec.rb                    |  6 ++-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/ee/spec/models/vulnerabilities/feedback_spec.rb b/ee/spec/models/vulnerabilities/feedback_spec.rb
index 3214a04f7b41ba..922febed35fe60 100644
--- a/ee/spec/models/vulnerabilities/feedback_spec.rb
+++ b/ee/spec/models/vulnerabilities/feedback_spec.rb
@@ -21,4 +21,57 @@
     it { is_expected.to validate_presence_of(:category) }
     it { is_expected.to validate_presence_of(:project_fingerprint) }
   end
+
+  describe '#find_or_init_for' do
+    let(:group)   { create(:group) }
+    let(:project) { create(:project, :public, :repository, namespace: group) }
+    let(:user)    { create(:user) }
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+    let(:feedback_params) do
+      {
+        feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast',
+        project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8',
+        vulnerability_data: {
+          category: 'sast',
+          priority: 'Low', line: '41',
+          file: 'subdir/src/main/java/com/gitlab/security_products/tests/App.java',
+          cve: '818bf5dacb291e15d9e6dc3c5ac32178:PREDICTABLE_RANDOM',
+          name: 'Predictable pseudorandom number generator',
+          description: 'Description of Predictable pseudorandom number generator',
+          tool: 'find_sec_bugs'
+        }
+      }
+    end
+
+    context 'when params are valid' do
+      it 'inits the feedback' do
+        new_feedback = described_class.find_or_init_for(feedback_params)
+        expect(new_feedback.new_record?).to eq(true)
+      end
+
+      it 'finds the existing feedback' do
+        new_feedback = described_class.find_or_init_for(feedback_params)
+
+        new_feedback.author = user
+        new_feedback.project = project
+
+        new_feedback.save!
+        new_feedback.reload
+        existing_feedback = described_class.find_or_init_for(feedback_params)
+        expect(existing_feedback.new_record?).to eq(false)
+      end
+    end
+
+    context 'when params are invalid' do
+      it 'raises ArgumentError when given a bad feedback_type value' do
+        feedback_params[:feedback_type] = 'foo'
+        expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /feedback_type/)
+      end
+
+      it 'raises ArgumentError when given a bad category value' do
+        feedback_params[:category] = 'foo'
+        expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /category/)
+      end
+    end
+  end
 end
diff --git a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
index 5049342de33855..8d156abf152ede 100644
--- a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
+++ b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
@@ -75,11 +75,13 @@
       end
 
       it 'updates the feedback when it already exists' do
-        result[:vulnerability_feedback]
+        result
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count)
       end
 
-      it 'creates an issue when feedback already exists' do
+      it 'creates a new issue when feedback already exists and issue has been deleted' do
+        result
+        expect { result[:vulnerability_feedback].issue.destroy}.to change(Issue, :count).by(-1)
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1)
       end
 
-- 
GitLab


From 5196d20322b71f9cbc4e9996e136727649e9c14a Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Fri, 22 Mar 2019 09:48:07 -0500
Subject: [PATCH 17/22] Address spec feedback

Updated formatting/spacing
Make test more explicit by comparing feedback objects
---
 ee/spec/models/vulnerabilities/feedback_spec.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ee/spec/models/vulnerabilities/feedback_spec.rb b/ee/spec/models/vulnerabilities/feedback_spec.rb
index 922febed35fe60..e2a2f2afda621f 100644
--- a/ee/spec/models/vulnerabilities/feedback_spec.rb
+++ b/ee/spec/models/vulnerabilities/feedback_spec.rb
@@ -23,10 +23,11 @@
   end
 
   describe '#find_or_init_for' do
-    let(:group)   { create(:group) }
+    let(:group) { create(:group) }
     let(:project) { create(:project, :public, :repository, namespace: group) }
-    let(:user)    { create(:user) }
+    let(:user) { create(:user) }
     let(:pipeline) { create(:ci_pipeline, project: project) }
+
     let(:feedback_params) do
       {
         feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast',
@@ -58,7 +59,7 @@
         new_feedback.save!
         new_feedback.reload
         existing_feedback = described_class.find_or_init_for(feedback_params)
-        expect(existing_feedback.new_record?).to eq(false)
+        expect(existing_feedback).to eq(new_feedback)
       end
     end
 
-- 
GitLab


From a74cf1734ca4cac372b4e9c1e733d635aa3d8ba7 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Sun, 24 Mar 2019 11:43:03 -0500
Subject: [PATCH 18/22] Add JS specs for dashboard changes

---
 .../security_dashboard_table_row_spec.js      | 51 +++++++++++++
 .../data/mock_data_vulnerabilities.json       | 76 ++++++++++++++++++-
 .../store/vulnerabilities/mutations_spec.js   | 14 ++++
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js b/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js
index 9e83f27f4a74d3..08d97dcbe50658 100644
--- a/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js
+++ b/ee/spec/javascripts/security_dashboard/components/security_dashboard_table_row_spec.js
@@ -108,4 +108,55 @@ describe('Security Dashboard Table Row', () => {
       expect(vm.$el.textContent).toContain('DISMISSED');
     });
   });
+
+  describe('with valid issue feedback', () => {
+    const vulnerability = mockDataVulnerabilities[3];
+
+    beforeEach(() => {
+      props = { vulnerability };
+      vm = mountComponentWithStore(Component, { store, props });
+    });
+
+    afterEach(() => {
+      vm.$destroy();
+    });
+
+    it('should have a `ic-issue-created` class', () => {
+      expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(1);
+    });
+  });
+
+  describe('with invalid issue feedback', () => {
+    const vulnerability = mockDataVulnerabilities[6];
+
+    beforeEach(() => {
+      props = { vulnerability };
+      vm = mountComponentWithStore(Component, { store, props });
+    });
+
+    afterEach(() => {
+      vm.$destroy();
+    });
+
+    it('should not have a `ic-issue-created` class', () => {
+      expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(0);
+    });
+  });
+
+  describe('with no issue feedback', () => {
+    const vulnerability = mockDataVulnerabilities[0];
+
+    beforeEach(() => {
+      props = { vulnerability };
+      vm = mountComponentWithStore(Component, { store, props });
+    });
+
+    afterEach(() => {
+      vm.$destroy();
+    });
+
+    it('should not have a `ic-issue-created` class', () => {
+      expect(vm.$el.querySelectorAll('.ic-issue-created')).toHaveLength(0);
+    });
+  });
 });
diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json
index 44841e366f35b4..bfdd894d7fc660 100644
--- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json
+++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/data/mock_data_vulnerabilities.json
@@ -406,5 +406,79 @@
         "url": "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first"
       }
     ]
+  },
+  {
+    "id": 7,
+    "report_type": "sast",
+    "name": "Insecure variable usage",
+    "severity": "high",
+    "confidence": "low",
+    "scanner": {
+      "external_id": "find_sec_bugs",
+      "name": "Find Security Bugs"
+    },
+    "identifiers": [
+      {
+        "external_type": "CVE",
+        "external_id": "CVE-2018-1234",
+        "name": "CVE-2018-1234",
+        "url": "http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234"
+      },
+      {
+        "external_type": "CVE",
+        "external_id": "CVE-2018-1234",
+        "name": "CVE-2018-1234",
+        "url": "http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234"
+      }
+    ],
+    "project_fingerprint": "4e5b6966dd100170b4b1ad599c7058cce91b57b4",
+    "project": {
+      "id": 1,
+      "name": "project1",
+      "full_path": "/namespace1/project1",
+      "full_name": "Gitab.org / security-products / codequality"
+    },
+    "dismissal_feedback": null,
+    "issue_feedback": {
+      "id": 7,
+      "project_id": 1,
+      "author": {
+        "id": 8,
+        "name": "John Doe9",
+        "username": "user8",
+        "state": "active",
+        "avatar_url": "https://www.gravatar.com/avatar/51798cfc94af924ac2dffb7083baa6f4?s=80&d=identicon",
+        "web_url": "http://localhost/user8",
+        "status_tooltip_html": null,
+        "path": "/user8"
+      },
+      "issue_iid": null,
+      "pipeline": {
+        "id": 3,
+        "path": "/namespace6/project6/pipelines/3"
+      },
+      "issue_url": null,
+      "category": "sast",
+      "feedback_type": "issue",
+      "branch": "master",
+      "project_fingerprint": "4e5b6966dd100170b4b1ad599c7058cce91b57b4"
+    },
+    "vulnerability_feedback_issue_path": "https://example.com/vulnerability_feedback",
+    "vulnerability_feedback_dismissal_path": "https://example.com/vulnerability_feedback",
+    "description": "The cipher does not provide data integrity update 1",
+    "solution": "GCM mode introduces an HMAC into the resulting encrypted data, providing integrity of the result.",
+    "location": {
+      "file": "maven/src/main/java/com/gitlab/security_products/tests/App.java",
+      "start_line": 29,
+      "end_line": 29,
+      "class": "com.gitlab.security_products.tests.App",
+      "method": "insecureCypher"
+    },
+    "links": [
+      {
+        "name": "Cipher does not check for integrity first?",
+        "url": "https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first"
+      }
+    ]
   }
-]
\ No newline at end of file
+]
diff --git a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
index ea73697e0e8422..e1561b35e065a8 100644
--- a/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
+++ b/ee/spec/javascripts/security_dashboard/store/vulnerabilities/mutations_spec.js
@@ -327,6 +327,20 @@ describe('vulnerabilities module mutations', () => {
         expect(state.modal.vulnerability.hasIssue).toEqual(true);
       });
 
+      it('should not set hasIssue when the issue_iid in null', () => {
+        const payload = {
+          vulnerability: {
+            ...vulnerability,
+            issue_feedback: {
+              issue_iid: null,
+            },
+          },
+        };
+        mutations[types.SET_MODAL_DATA](state, payload);
+
+        expect(state.modal.vulnerability.hasIssue).toEqual(false);
+      });
+
       it('should nullify the modal links', () => {
         const payload = { vulnerability: { ...vulnerability, links: [] } };
         mutations[types.SET_MODAL_DATA](state, payload);
-- 
GitLab


From becc9e736ae75a6f66de1336cc67d51161dc8485 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 26 Mar 2019 14:20:03 -0500
Subject: [PATCH 19/22] Address feedback by adding and improving specs

---
 .../models/vulnerabilities/feedback_spec.rb   | 34 ++++++++++++++-----
 .../vulnerabilities/feedback_entity_spec.rb   | 20 ++++++-----
 .../create_service_spec.rb                    |  2 ++
 3 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/ee/spec/models/vulnerabilities/feedback_spec.rb b/ee/spec/models/vulnerabilities/feedback_spec.rb
index e2a2f2afda621f..7d6fec4aa619f8 100644
--- a/ee/spec/models/vulnerabilities/feedback_spec.rb
+++ b/ee/spec/models/vulnerabilities/feedback_spec.rb
@@ -45,32 +45,48 @@
     end
 
     context 'when params are valid' do
+      subject(:feedback) { described_class.find_or_init_for(feedback_params) }
+
+      before do
+        feedback.author = user
+        feedback.project = project
+      end
+
       it 'inits the feedback' do
-        new_feedback = described_class.find_or_init_for(feedback_params)
-        expect(new_feedback.new_record?).to eq(true)
+        is_expected.to be_new_record
       end
 
       it 'finds the existing feedback' do
-        new_feedback = described_class.find_or_init_for(feedback_params)
+        feedback.save!
 
-        new_feedback.author = user
-        new_feedback.project = project
-
-        new_feedback.save!
-        new_feedback.reload
         existing_feedback = described_class.find_or_init_for(feedback_params)
-        expect(existing_feedback).to eq(new_feedback)
+
+        expect(existing_feedback).to eq(feedback)
+      end
+
+      context 'when attempting to save duplicate' do
+        it 'raises ActiveRecord::RecordInvalid' do
+          duplicate = described_class.find_or_init_for(feedback_params)
+          duplicate.author = user
+          duplicate.project = project
+
+          feedback.save!
+
+          expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid)
+        end
       end
     end
 
     context 'when params are invalid' do
       it 'raises ArgumentError when given a bad feedback_type value' do
         feedback_params[:feedback_type] = 'foo'
+
         expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /feedback_type/)
       end
 
       it 'raises ArgumentError when given a bad category value' do
         feedback_params[:category] = 'foo'
+
         expect { described_class.find_or_init_for(feedback_params) }.to raise_error(ArgumentError, /category/)
       end
     end
diff --git a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
index 978fd49a332f82..dd2f22e48f47f8 100644
--- a/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
+++ b/ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb
@@ -18,9 +18,10 @@
     let(:entity) { described_class.represent(feedback) }
 
     subject { entity.as_json }
+
     it 'exposes issue information' do
-      expect(subject).to include(:issue_iid)
-      expect(subject).to include(:issue_url)
+      is_expected.to include(:issue_iid)
+      is_expected.to include(:issue_url)
     end
   end
 
@@ -29,9 +30,10 @@
     let(:entity) { described_class.represent(feedback) }
 
     subject { entity.as_json }
+
     it 'does not expose issue information' do
-      expect(subject).not_to include(:issue_iid)
-      expect(subject).not_to include(:issue_url)
+      is_expected.not_to include(:issue_iid)
+      is_expected.not_to include(:issue_url)
     end
   end
 
@@ -40,9 +42,10 @@
     let(:entity) { described_class.represent(feedback) }
 
     subject { entity.as_json }
+
     it 'exposes merge request information' do
-      expect(subject).to include(:merge_request_iid)
-      expect(subject).to include(:merge_request_path)
+      is_expected.to include(:merge_request_iid)
+      is_expected.to include(:merge_request_path)
     end
   end
 
@@ -51,9 +54,10 @@
     let(:entity) { described_class.represent(feedback) }
 
     subject { entity.as_json }
+
     it 'does not expose merge request information' do
-      expect(subject).not_to include(:merge_request_iid)
-      expect(subject).not_to include(:merge_request_path)
+      is_expected.not_to include(:merge_request_iid)
+      is_expected.not_to include(:merge_request_path)
     end
   end
 end
diff --git a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
index 8d156abf152ede..a3ec58147cc8ce 100644
--- a/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
+++ b/ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb
@@ -76,11 +76,13 @@
 
       it 'updates the feedback when it already exists' do
         result
+
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.not_to change(Vulnerabilities::Feedback, :count)
       end
 
       it 'creates a new issue when feedback already exists and issue has been deleted' do
         result
+
         expect { result[:vulnerability_feedback].issue.destroy}.to change(Issue, :count).by(-1)
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1)
       end
-- 
GitLab


From 4d38a26360ce662c579d3b85d80deb2522f9a71a Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Tue, 26 Mar 2019 16:07:31 -0500
Subject: [PATCH 20/22] Added parens per style guide

---
 ee/app/models/vulnerabilities/feedback.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ee/app/models/vulnerabilities/feedback.rb b/ee/app/models/vulnerabilities/feedback.rb
index c1c0d7aeccefe6..644c2bdecddd85 100644
--- a/ee/app/models/vulnerabilities/feedback.rb
+++ b/ee/app/models/vulnerabilities/feedback.rb
@@ -42,12 +42,12 @@ def self.find_or_init_for(feedback_params)
     # Rails 5.0 does not properly handle validation of enums in select queries such as find_or_initialize_by.
     # This method, and calls to it can be removed when we are on Rails 5.2.
     def self.validate_enums(feedback_params)
-      unless feedback_types.include? feedback_params[:feedback_type]
+      unless feedback_types.include?(feedback_params[:feedback_type])
 
         raise ArgumentError.new("'#{feedback_params[:feedback_type]}' is not a valid feedback_type")
       end
 
-      unless categories.include? feedback_params[:category]
+      unless categories.include?(feedback_params[:category])
         raise ArgumentError.new("'#{feedback_params[:category]}' is not a valid category")
       end
     end
-- 
GitLab


From 1a29f73ddbbf5291a92f729eb70b7503aaf3a5b1 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Wed, 27 Mar 2019 11:24:48 -0500
Subject: [PATCH 21/22] Incorporate feedback to clean up code

---
 ee/app/models/vulnerabilities/feedback.rb | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ee/app/models/vulnerabilities/feedback.rb b/ee/app/models/vulnerabilities/feedback.rb
index 644c2bdecddd85..6bf91d6fe60143 100644
--- a/ee/app/models/vulnerabilities/feedback.rb
+++ b/ee/app/models/vulnerabilities/feedback.rb
@@ -33,10 +33,9 @@ class Feedback < ActiveRecord::Base
     def self.find_or_init_for(feedback_params)
       validate_enums(feedback_params)
 
-      vulnerability_feedback = find_or_initialize_by(category: feedback_params[:category], feedback_type: feedback_params[:feedback_type], project_fingerprint: feedback_params[:project_fingerprint] )
-      vulnerability_feedback.vulnerability_data = feedback_params[:vulnerability_data]
-      vulnerability_feedback.pipeline_id = feedback_params[:pipeline_id]
-      vulnerability_feedback
+      record = find_or_initialize_by(feedback_params.slice(:category, :feedback_type, :project_fingerprint))
+      record.assign_attributes(feedback_params)
+      record
     end
 
     # Rails 5.0 does not properly handle validation of enums in select queries such as find_or_initialize_by.
-- 
GitLab


From e0cda960059d23cc8f7769fdbb03838fcc796614 Mon Sep 17 00:00:00 2001
From: rossfuhrman <rfuhrman@gitlab.com>
Date: Wed, 27 Mar 2019 11:29:31 -0500
Subject: [PATCH 22/22] Add changelog

---
 ...shes-when-vulnerability-associated-issuables-deleted.yml | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml

diff --git a/ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml b/ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml
new file mode 100644
index 00000000000000..ecd8baffd50a73
--- /dev/null
+++ b/ee/changelogs/unreleased/9462-security-report-crashes-when-vulnerability-associated-issuables-deleted.yml
@@ -0,0 +1,6 @@
+---
+title: Resolve Deletion of vulnerability-associated issuables prevents security report
+  from loading
+merge_request: 10016
+author:
+type: fixed
-- 
GitLab