Add comment to Deployment Approvals

- Expose comments to internal and public APIs
- Allow comment to be saved with an Approval

Changelog: added
EE: true

db/migrate/20220303190555_add_comment_to_deployment_approvals.rb

+# frozen_string_literal: true
+
+class AddCommentToDeploymentApprovals < Gitlab::Database::Migration[1.0]
+  # rubocop:disable Migration/AddLimitToTextColumns
+  # limit is added in 20220303191047_add_text_limit_to_deployment_approvals_comment
+  def change
+    add_column :deployment_approvals, :comment, :text
+  end
+  # rubocop:enable Migration/AddLimitToTextColumns
+end

db/migrate/20220303191047_add_text_limit_to_deployment_approvals_comment.rb

+# frozen_string_literal: true
+
+class AddTextLimitToDeploymentApprovalsComment < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :deployment_approvals, :comment, 255
+  end
+
+  def down
+    remove_text_limit :deployment_approvals, :comment
+  end
+end

db/schema_migrations/20220303190555

+f8fa8b83da24bf98c97447a2940c8ca801532c80395b6a65c11f83515f811652
\ No newline at end of file

db/schema_migrations/20220303191047

+19566152e16a92263dd5dcfd66299e3b9d8b82acd4edb4bba21f6b9b06fc8070
\ No newline at end of file

db/structure.sql

@@ -14262,7 +14262,9 @@ CREATE TABLE deployment_approvals (
     user_id bigint NOT NULL,
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
-    status smallint NOT NULL
+    status smallint NOT NULL,
+    comment text,
+    CONSTRAINT check_e2eb6a17d8 CHECK ((char_length(comment) <= 255))
 );
 
 CREATE SEQUENCE deployment_approvals_id_seq

doc/api/deployments.md

@@ -465,9 +465,10 @@ POST /projects/:id/deployments/:deployment_id/approval
 | `id`            | integer/string | yes      | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user. |
 | `deployment_id` | integer        | yes      | The ID of the deployment.                                                                                       |
 | `status`        | string         | yes      | The status of the approval (either `approved` or `rejected`).                                                   |
+| `comment`       | string         | no       | A comment to go with the approval                                                                               |
 
 ```shell
-curl --data "status=approved" \
+curl --data "status=approved&comment=Looks good to me" \
      --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/deployments/1/approval"
 ```
 
@@ -484,6 +485,7 @@ Example response:
     "web_url": "http://localhost:3000/root"
   },
   "status": "approved",
-  "created_at": "2022-02-24T20:22:30.097Z"
+  "created_at": "2022-02-24T20:22:30.097Z",
+  "comment":"Looks good to me"
 }
 ```

doc/ci/environments/deployment_approvals.md

@@ -89,7 +89,7 @@ Using the [Deployments API](../../api/deployments.md#approve-or-reject-a-blocked
 Example:
 
 ```shell
-curl --data "status=approved" \
+curl --data "status=approved&comment=Looks good to me" \
      --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/deployments/1/approval"
 ```
 

ee/app/models/deployments/approval.rb

@@ -10,6 +10,7 @@ module Deployments
     validates :user, presence: true, uniqueness: { scope: :deployment_id }
     validates :deployment, presence: true
     validates :status, presence: true
+    validates :comment, length: { maximum: 255 }
 
     enum status: {
       approved: 0,

ee/app/services/deployments/approval_service.rb

@@ -2,11 +2,11 @@
 
 module Deployments
   class ApprovalService < ::BaseService
-    def execute(deployment, status)
+    def execute(deployment:, status:, comment: nil)
       error_message = validate(deployment, status)
       return error(error_message) if error_message
 
-      approval = upsert_approval(deployment, status)
+      approval = upsert_approval(deployment, status, comment)
       return error(approval.errors.full_messages) if approval.errors.any?
 
       process_build!(deployment, approval)
@@ -16,13 +16,13 @@ module Deployments
 
     private
 
-    def upsert_approval(deployment, status)
+    def upsert_approval(deployment, status, comment)
       if (approval = deployment.approvals.find_by_user_id(current_user.id))
         return approval if approval.status == status
 
-        approval.tap { |a| a.update(status: status) }
+        approval.tap { |a| a.update(status: status, comment: comment) }
       else
-        deployment.approvals.create(user: current_user, status: status)
+        deployment.approvals.create(user: current_user, status: status, comment: comment)
       end
     end
 

ee/lib/api/entities/deployments/approval.rb

@@ -7,6 +7,7 @@ module API
         expose :user, using: Entities::UserBasic
         expose :status
         expose :created_at
+        expose :comment
       end
     end
   end

ee/lib/ee/api/deployments.rb

@@ -17,12 +17,13 @@ module EE
           params do
             requires :deployment_id, type: String, desc: 'The Deployment ID'
             requires :status, type: String, values: ::Deployments::Approval.statuses.keys
+            optional :comment, type: String, desc: 'A comment to go with the approval'
           end
           post ':id/deployments/:deployment_id/approval' do
             deployment = user_project.deployments.find(params[:deployment_id])
 
             result = ::Deployments::ApprovalService.new(user_project, current_user)
-                                                   .execute(deployment, params[:status])
+                                                   .execute(deployment: deployment, status: params[:status], comment: params[:comment])
 
             if result[:status] == :success
               present(result[:approval], with: ::API::Entities::Deployments::Approval, current_user: current_user)

ee/spec/factories/deployments/approval.rb

@@ -5,6 +5,7 @@ FactoryBot.define do
     user
     deployment
     status { 'approved' }
+    comment { 'Looks good to me!' }
 
     trait :rejected do
       status { 'rejected' }

ee/spec/fixtures/api/schemas/public_api/v4/deployment_approval.json

@@ -2,11 +2,17 @@
   "type": "object",
   "required": [
     "status",
-    "user"
+    "user",
+    "created_at",
+    "comment"
   ],
   "properties": {
     "status": {
-      "type": "string"
+      "type": "string",
+      "enum": [
+        "approved",
+        "rejected"
+      ]
     },
     "user": {
       "type": "object",
@@ -17,6 +23,10 @@
     "created_at": {
       "type": "string",
       "format": "date-time"
+    },
+    "comment": {
+      "type": ["string", "null"],
+      "maxLength": 255
     }
   },
   "additionalProperties": false

ee/spec/lib/api/entities/deployments/approval_spec.rb

@@ -8,6 +8,6 @@ RSpec.describe API::Entities::Deployments::Approval do
   let(:approval) { build(:deployment_approval) }
 
   it 'exposes correct attributes' do
-    expect(subject.keys).to contain_exactly(:user, :status, :created_at)
+    expect(subject.keys).to contain_exactly(:user, :status, :created_at, :comment)
   end
 end

ee/spec/models/deployments/approval_spec.rb

@@ -15,5 +15,6 @@ RSpec.describe Deployments::Approval do
     it { is_expected.to validate_uniqueness_of(:user).scoped_to(:deployment_id) }
     it { is_expected.to validate_presence_of(:deployment) }
     it { is_expected.to validate_presence_of(:status) }
+    it { is_expected.to validate_length_of(:comment).is_at_most(255) }
   end
 end

ee/spec/requests/api/deployments_spec.rb

@@ -267,6 +267,20 @@ RSpec.describe API::Deployments do
             expect(json_response['status']).to eq('approved')
             expect(json_response.dig('user', 'id')).to eq(user.id)
           end
+
+          it 'creates a rejection' do
+            expect { post(api(path, user), params: { status: 'rejected' }) }.to change { Deployments::Approval.count }.by(1)
+
+            expect(response).to have_gitlab_http_status(:success)
+            expect(json_response['status']).to eq('rejected')
+          end
+
+          it 'creates an approval with a comment' do
+            expect { post(api(path, user), params: { status: 'approved', comment: 'LGTM!' }) }.to change { Deployments::Approval.count }.by(1)
+
+            expect(response).to have_gitlab_http_status(:success)
+            expect(json_response['comment']).to eq('LGTM!')
+          end
         end
 
         context 'and user is not authorized to update deployment' do

ee/spec/services/deployments/approval_service_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe Deployments::ApprovalService do
   let(:user) { create(:user) }
   let(:environment) { create(:environment, project: project) }
   let(:status) { 'approved' }
+  let(:comment) { nil }
   let(:required_approval_count) { 2 }
   let(:build) { create(:ci_build, :manual, project: project) }
   let(:deployment) { create(:deployment, :blocked, project: project, environment: environment, deployable: build) }
@@ -48,32 +49,55 @@ RSpec.describe Deployments::ApprovalService do
     end
   end
 
+  shared_examples_for 'comment' do
+    context 'with a comment' do
+      let(:comment) { 'LGTM!' }
+
+      it 'saves the comment' do
+        expect(subject[:status]).to eq(:success)
+        expect(subject[:approval].comment).to eq(comment)
+      end
+    end
+  end
+
   describe '#execute' do
-    subject { service.execute(deployment, status) }
+    subject { service.execute(deployment: deployment, status: status, comment: comment) }
 
     context 'when status is approved' do
       include_examples 'approve'
+      include_examples 'comment'
     end
 
     context 'when status is rejected' do
       let(:status) { 'rejected' }
 
       include_examples 'reject'
+      include_examples 'comment'
     end
 
     context 'when user already approved' do
+      let(:comment) { 'Changed comment' }
+
       before do
-        service.execute(deployment, :approved)
+        service.execute(deployment: deployment, status: :approved, comment: 'Original comment')
       end
 
       context 'and is approving again' do
         include_examples 'approve'
+
+        it 'does not change the comment' do
+          expect(subject[:approval].comment).to eq('Original comment')
+        end
       end
 
       context 'and is rejecting' do
         let(:status) { 'rejected' }
 
         include_examples 'reject'
+
+        it 'saves the changed comment' do
+          expect(subject[:approval].comment).to eq('Changed comment')
+        end
       end
     end