Backend work to comment on vulnerability dismissal

This change adds columns to the vulnerability_feedback table to support
adding a single plain-text comment for vulnerability feedback items and
specifically for commenting on dismissals of vulnerabilities.
It also adds support for submitting a comment when dismissing a
vulnerability as well as returning the comment-related attributes when
retrieving vulnerability_feedback records.

db/schema.rb

@@ -3316,7 +3316,11 @@ ActiveRecord::Schema.define(version: 20190426180107) do
     t.integer "issue_id"
     t.string "project_fingerprint", limit: 40, null: false
     t.integer "merge_request_id"
+    t.integer "comment_author_id"
+    t.text "comment"
+    t.datetime_with_timezone "comment_timestamp"
     t.index ["author_id"], name: "index_vulnerability_feedback_on_author_id", using: :btree
+    t.index ["comment_author_id"], name: "index_vulnerability_feedback_on_comment_author_id", using: :btree
     t.index ["issue_id"], name: "index_vulnerability_feedback_on_issue_id", using: :btree
     t.index ["merge_request_id"], name: "index_vulnerability_feedback_on_merge_request_id", using: :btree
     t.index ["pipeline_id"], name: "index_vulnerability_feedback_on_pipeline_id", using: :btree
@@ -3755,6 +3759,7 @@ ActiveRecord::Schema.define(version: 20190426180107) do
   add_foreign_key "vulnerability_feedback", "merge_requests", name: "fk_563ff1912e", on_delete: :nullify
   add_foreign_key "vulnerability_feedback", "projects", on_delete: :cascade
   add_foreign_key "vulnerability_feedback", "users", column: "author_id", on_delete: :cascade
+  add_foreign_key "vulnerability_feedback", "users", column: "comment_author_id", name: "fk_94f7c8a81e", on_delete: :nullify
   add_foreign_key "vulnerability_identifiers", "projects", on_delete: :cascade
   add_foreign_key "vulnerability_occurrence_identifiers", "vulnerability_identifiers", column: "identifier_id", on_delete: :cascade
   add_foreign_key "vulnerability_occurrence_identifiers", "vulnerability_occurrences", column: "occurrence_id", on_delete: :cascade

ee/app/controllers/projects/vulnerability_feedback_controller.rb

@@ -69,6 +69,7 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
       feedback_type
       pipeline_id
       project_fingerprint
+      comment
       ] + [
         vulnerability_data: vulnerability_data_params_attributes
       ]

ee/app/models/vulnerabilities/feedback.rb

@@ -5,11 +5,13 @@ module Vulnerabilities
     self.table_name = 'vulnerability_feedback'
 
     belongs_to :project
-    belongs_to :author, class_name: "User"
+    belongs_to :author, class_name: 'User'
     belongs_to :issue
     belongs_to :merge_request
     belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
 
+    belongs_to :comment_author, class_name: 'User'
+
     attr_accessor :vulnerability_data
 
     enum feedback_type: { dismissal: 0, issue: 1, merge_request: 2 }
@@ -17,6 +19,7 @@ module Vulnerabilities
 
     validates :project, presence: true
     validates :author, presence: true
+    validates :comment_timestamp, :comment_author, presence: true, if: :comment?
     validates :issue, presence: true, if: :issue?
     validates :merge_request, presence: true, if: :merge_request?
     validates :vulnerability_data, presence: true, unless: :dismissal?
@@ -24,10 +27,10 @@ module Vulnerabilities
     validates :category, presence: true
     validates :project_fingerprint, presence: true, uniqueness: { scope: [:project_id, :category, :feedback_type] }
 
-    scope :with_associations, -> { includes(:pipeline, :issue, :merge_request, :author) }
+    scope :with_associations, -> { includes(:pipeline, :issue, :merge_request, :author, :comment_author) }
 
     scope :all_preloaded, -> do
-      preload(:author, :project, :issue, :merge_request, :pipeline)
+      preload(:author, :comment_author, :project, :issue, :merge_request, :pipeline)
     end
 
     def self.find_or_init_for(feedback_params)

ee/app/serializers/vulnerabilities/feedback_entity.rb

@@ -8,6 +8,12 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
   expose :created_at
   expose :project_id
   expose :author, using: UserEntity
+  expose :comment_details, if: -> (feedback, _) { feedback.comment.present? } do
+    expose :comment
+    expose :comment_timestamp
+    expose :comment_author, using: UserEntity
+  end
+
   expose :pipeline, if: -> (feedback, _) { feedback.pipeline.present? } do
     expose :id do |feedback|
       feedback.pipeline.id

ee/app/services/vulnerability_feedback_module/create_service.rb

@@ -3,8 +3,7 @@
 module VulnerabilityFeedbackModule
   class CreateService < ::BaseService
     def execute
-      vulnerability_feedback = @project.vulnerability_feedback.find_or_init_for(@params)
-      vulnerability_feedback.author = @current_user
+      vulnerability_feedback = @project.vulnerability_feedback.find_or_init_for(create_params)
 
       if vulnerability_feedback.issue? &&
           !vulnerability_feedback.vulnerability_data.blank?
@@ -33,6 +32,20 @@ module VulnerabilityFeedbackModule
 
     private
 
+    def create_params
+      @params[:author] = @current_user
+      @params.merge(comment_params)
+    end
+
+    def comment_params
+      return {} unless @params[:comment].present?
+
+      {
+        comment_author: @current_user,
+        comment_timestamp: Time.zone.now
+      }
+    end
+
     def success(vulnerability_feedback)
       super().merge(vulnerability_feedback: vulnerability_feedback)
     end

ee/db/migrate/20190414185432_add_comment_to_vulnerability_feedback.rb

+# frozen_string_literal: true
+
+class AddCommentToVulnerabilityFeedback < ActiveRecord::Migration[5.1]
+  DOWNTIME = false
+
+  def up
+    add_column :vulnerability_feedback, :comment_author_id, :integer
+    add_column :vulnerability_feedback, :comment, :text
+    add_column :vulnerability_feedback, :comment_timestamp, :datetime_with_timezone
+  end
+
+  def down
+    remove_column :vulnerability_feedback, :comment_author_id
+    remove_column :vulnerability_feedback, :comment
+    remove_column :vulnerability_feedback, :comment_timestamp
+  end
+end

ee/db/migrate/20190418132750_add_foreign_key_from_vulnerability_feedback_to_users.rb

+# frozen_string_literal: true
+
+class AddForeignKeyFromVulnerabilityFeedbackToUsers < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :vulnerability_feedback, :users, column: :comment_author_id, on_delete: :nullify
+    add_concurrent_index :vulnerability_feedback, :comment_author_id
+  end
+
+  def down
+    remove_foreign_key :vulnerability_feedback, column: :comment_author_id
+    remove_concurrent_index :vulnerability_feedback, :comment_author_id
+  end
+end

ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb

@@ -85,6 +85,7 @@ describe Projects::VulnerabilityFeedbackController do
     let(:create_params) do
       {
         feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast',
+        comment: 'a dismissal comment',
         project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8',
         vulnerability_data: {
           category: 'sast',

ee/spec/factories/vulnerabilities/feedback.rb

@@ -20,6 +20,12 @@ FactoryBot.define do
       feedback_type 'dismissal'
     end
 
+    trait :comment do
+      comment 'a dismissal comment'
+      comment_timestamp { Time.zone.now }
+      comment_author { author }
+    end
+
     trait :issue do
       feedback_type 'issue'
       issue { create(:issue, project: project) }

ee/spec/fixtures/api/schemas/vulnerability_feedback.json

@@ -13,6 +13,11 @@
     "created_at": { "type": "date" },
     "project_id": { "type": "integer" },
     "author": { "$ref": "../../../../../spec/fixtures/api/schemas/entities/user.json" },
+    "comment_details": {
+      "comment": { "type": ["string", "null"] },
+      "comment_author": { "$ref": "../../../../../spec/fixtures/api/schemas/entities/user.json" },
+      "comment_timestamp": { "type": ["timestamp", "null"] }
+    },
     "pipeline": {
       "id": { "type": ["integer", "null"] },
       "path": { "type": ["string", "null"] }

ee/spec/models/vulnerabilities/feedback_spec.rb

@@ -9,6 +9,7 @@ describe Vulnerabilities::Feedback do
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:author).class_name('User') }
+    it { is_expected.to belong_to(:comment_author).class_name('User') }
     it { is_expected.to belong_to(:issue) }
     it { is_expected.to belong_to(:merge_request) }
     it { is_expected.to belong_to(:pipeline).class_name('Ci::Pipeline').with_foreign_key('pipeline_id') }
@@ -20,6 +21,18 @@ describe Vulnerabilities::Feedback do
     it { is_expected.to validate_presence_of(:feedback_type) }
     it { is_expected.to validate_presence_of(:category) }
     it { is_expected.to validate_presence_of(:project_fingerprint) }
+
+    context 'comment is set' do
+      let(:feedback) { build(:vulnerability_feedback, comment: 'a comment' ) }
+
+      it 'validates presence of comment_timestamp' do
+        expect(feedback).to validate_presence_of(:comment_timestamp)
+      end
+
+      it 'validates presence of comment_author' do
+        expect(feedback).to validate_presence_of(:comment_author)
+      end
+    end
   end
 
   describe '#find_or_init_for' do
@@ -32,6 +45,7 @@ describe Vulnerabilities::Feedback do
       {
         feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast',
         project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8',
+        author: user,
         vulnerability_data: {
           category: 'sast',
           priority: 'Low', line: '41',
@@ -48,7 +62,6 @@ describe Vulnerabilities::Feedback do
       subject(:feedback) { described_class.find_or_init_for(feedback_params) }
 
       before do
-        feedback.author = user
         feedback.project = project
       end
 
@@ -67,7 +80,6 @@ describe Vulnerabilities::Feedback do
       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!

ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb

@@ -13,6 +13,26 @@ describe Vulnerabilities::FeedbackEntity do
     it { is_expected.to include(:created_at, :project_id, :author, :category, :feedback_type) }
   end
 
+  context 'when comment is not present' do
+    subject { entity.as_json }
+
+    it { is_expected.not_to include(:comment_details) }
+  end
+
+  context 'when comment is present' do
+    let(:feedback) { build(:vulnerability_feedback, :comment) }
+    let(:entity) { described_class.represent(feedback) }
+
+    subject { entity.as_json }
+
+    it 'exposes comment information' do
+      expect(subject).to include(:comment_details)
+      expect(subject[:comment_details]).to include(:comment)
+      expect(subject[:comment_details]).to include(:comment_timestamp)
+      expect(subject[:comment_details]).to include(:comment_author)
+    end
+  end
+
   context 'when issue is present' do
     let(:feedback) { build(:vulnerability_feedback, :issue ) }
     let(:entity) { described_class.represent(feedback) }

ee/spec/services/ee/vulnerability_feedback_module/create_service_spec.rb

@@ -15,6 +15,7 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
       {
         feedback_type: 'dismissal', pipeline_id: pipeline.id, category: 'sast',
         project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8',
+        comment: 'a dismissal comment',
         vulnerability_data: {
           category: 'sast',
           priority: 'Low', line: '41',
@@ -46,6 +47,30 @@ describe VulnerabilityFeedbackModule::CreateService, '#execute' do
         expect(feedback.merge_request?).to eq(false)
         expect(feedback.merge_request).to be_nil
       end
+
+      context 'when feedback params has a comment' do
+        it 'sets the comment attributes' do
+          feedback = result[:vulnerability_feedback]
+
+          expect(feedback.comment).to eq('a dismissal comment')
+          expect(feedback.comment_author).to eq(user)
+          expect(feedback.comment_timestamp).not_to be_nil
+        end
+      end
+
+      context 'when feedback params does not have a comment' do
+        before do
+          feedback_params[:comment] = nil
+        end
+
+        it 'does not set comment attributes' do
+          feedback = result[:vulnerability_feedback]
+
+          expect(feedback.comment).to be_nil
+          expect(feedback.comment_author).to be_nil
+          expect(feedback.comment_timestamp).to be_nil
+        end
+      end
     end
 
     context 'when feedback_type is issue' do