Merge branch 'ali/refactor-deployment-approval-service-params' into 'master'

Refactor Deployments::ApprovalService interface

See merge request gitlab-org/gitlab!82455

ee/app/services/deployments/approval_service.rb

@@ -2,11 +2,11 @@
 
 module Deployments
   class ApprovalService < ::BaseService
-    def execute(deployment:, status:, comment: nil)
+    def execute(deployment, status)
       error_message = validate(deployment, status)
       return error(error_message) if error_message
 
-      approval = upsert_approval(deployment, status, comment)
+      approval = upsert_approval(deployment, status, params[:comment])
       return error(approval.errors.full_messages) if approval.errors.any?
 
       process_build!(deployment, approval)

ee/lib/ee/api/deployments.rb

@@ -22,8 +22,8 @@ module EE
           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: deployment, status: params[:status], comment: params[:comment])
+            result = ::Deployments::ApprovalService.new(user_project, current_user, declared_params(include_missing: false))
+                                                   .execute(deployment, params[:status])
 
             if result[:status] == :success
               present(result[:approval], with: ::API::Entities::Deployments::Approval, current_user: current_user)

ee/spec/services/deployments/approval_service_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe Deployments::ApprovalService do
   let_it_be(:project) { create(:project, :repository) }
 
-  let(:service) { described_class.new(project, user) }
+  let(:service) { described_class.new(project, user, params) }
+  let(:params) { { comment: comment } }
   let(:user) { create(:user) }
   let(:environment) { create(:environment, project: project) }
   let(:status) { 'approved' }
@@ -28,7 +29,7 @@ RSpec.describe Deployments::ApprovalService do
   end
 
   shared_examples_for 'reject' do
-    it 'rejects the deployment' do
+    it 'rejects the deployment', :aggregate_failures do
       expect(subject[:status]).to eq(:success)
       expect(subject[:approval].status).to eq('rejected')
       expect(subject[:approval].user).to eq(user)
@@ -39,7 +40,7 @@ RSpec.describe Deployments::ApprovalService do
   end
 
   shared_examples_for 'approve' do
-    it 'approves the deployment' do
+    it 'approves the deployment', :aggregate_failures do
       expect(subject[:status]).to eq(:success)
       expect(subject[:approval].status).to eq('approved')
       expect(subject[:approval].user).to eq(user)
@@ -61,7 +62,7 @@ RSpec.describe Deployments::ApprovalService do
   end
 
   describe '#execute' do
-    subject { service.execute(deployment: deployment, status: status, comment: comment) }
+    subject { service.execute(deployment, status) }
 
     context 'when status is approved' do
       include_examples 'approve'
@@ -76,17 +77,21 @@ RSpec.describe Deployments::ApprovalService do
     end
 
     context 'when user already approved' do
-      let(:comment) { 'Changed comment' }
+      let(:comment) { 'Original comment' }
 
       before do
-        service.execute(deployment: deployment, status: :approved, comment: 'Original comment')
+        service.execute(deployment, :approved)
       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')
+        context 'with a different comment' do
+          it 'does not change the comment' do
+            service = described_class.new(project, user, params.merge(comment: 'Changed comment'))
+
+            expect(service.execute(deployment, status)[:approval].comment).to eq('Original comment')
+          end
         end
       end
 
@@ -95,8 +100,12 @@ RSpec.describe Deployments::ApprovalService do
 
         include_examples 'reject'
 
-        it 'saves the changed comment' do
-          expect(subject[:approval].comment).to eq('Changed comment')
+        context 'with a different comment' do
+          it 'changes the comment' do
+            service = described_class.new(project, user, params.merge(comment: 'Changed comment'))
+
+            expect(service.execute(deployment, status)[:approval].comment).to eq('Changed comment')
+          end
         end
       end
     end