Merge branch 'refactor-stop-environments-service' into 'master'

Refactor stop environments service and fix a bug that it can't stop empty environments

See merge request gitlab-org/gitlab!67485

app/services/environments/auto_stop_service.rb

@@ -32,7 +32,7 @@ module Environments
 
       return false unless environments.exists?
 
-      Ci::StopEnvironmentsService.execute_in_batch(environments)
+      Environments::StopService.execute_in_batch(environments)
     end
   end
 end

app/services/ci/stop_environments_service.rb → app/services/environments/stop_service.rb

 # frozen_string_literal: true
 
-module Ci
-  class StopEnvironmentsService < BaseService
+module Environments
+  class StopService < BaseService
     attr_reader :ref
 
-    def execute(branch_name)
+    def execute(environment)
+      return unless can?(current_user, :stop_environment, environment)
+
+      environment.stop_with_action!(current_user)
+    end
+
+    def execute_for_branch(branch_name)
       @ref = branch_name
 
       return unless @ref.present?
 
-      environments.each { |environment| stop(environment) }
+      environments.each { |environment| execute(environment) }
     end
 
     def execute_for_merge_request(merge_request)
-      merge_request.environments.each { |environment| stop(environment) }
+      merge_request.environments.each { |environment| execute(environment) }
     end
 
     ##
@@ -39,12 +45,5 @@ module Ci
         .new(project, current_user, ref: @ref, recently_updated: true)
         .execute
     end
-
-    def stop(environment)
-      return unless environment.stop_action_available?
-      return unless can?(current_user, :stop_environment, environment)
-
-      environment.stop_with_action!(current_user)
-    end
   end
 end

app/services/git/branch_push_service.rb

@@ -58,7 +58,7 @@ module Git
     def stop_environments
       return unless removing_branch?
 
-      Ci::StopEnvironmentsService.new(project, current_user).execute(branch_name)
+      Environments::StopService.new(project, current_user).execute_for_branch(branch_name)
     end
 
     def unlock_artifacts

app/services/merge_requests/base_service.rb

@@ -61,8 +61,8 @@ module MergeRequests
     end
 
     def cleanup_environments(merge_request)
-      Ci::StopEnvironmentsService.new(merge_request.source_project, current_user)
-                                 .execute_for_merge_request(merge_request)
+      Environments::StopService.new(merge_request.source_project, current_user)
+                               .execute_for_merge_request(merge_request)
     end
 
     def cancel_review_app_jobs!(merge_request)

spec/services/ci/stop_environments_service_spec.rb → spec/services/environments/stop_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Ci::StopEnvironmentsService do
+RSpec.describe Environments::StopService do
   include CreateEnvironmentsHelpers
 
   let(:project) { create(:project, :private, :repository) }
@@ -11,6 +11,59 @@ RSpec.describe Ci::StopEnvironmentsService do
   let(:service) { described_class.new(project, user) }
 
   describe '#execute' do
+    subject { service.execute(environment) }
+
+    let_it_be(:project) { create(:project, :private, :repository) }
+    let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } }
+    let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } }
+
+    let(:user) { developer }
+
+    context 'with a deployment' do
+      let!(:environment) { review_job.persisted_environment }
+      let!(:pipeline) { create(:ci_pipeline, project: project) }
+      let!(:review_job) { create(:ci_build, :with_deployment, :start_review_app, pipeline: pipeline, project: project) }
+      let!(:stop_review_job) { create(:ci_build, :with_deployment, :stop_review_app, :manual, pipeline: pipeline, project: project) }
+
+      before do
+        review_job.success!
+      end
+
+      it 'stops the environment' do
+        expect { subject }.to change { environment.reload.state }.from('available').to('stopped')
+      end
+
+      it 'plays the stop action' do
+        expect { subject }.to change { stop_review_job.reload.status }.from('manual').to('pending')
+      end
+
+      context 'when an environment has already been stopped' do
+        let!(:environment) { create(:environment, :stopped, project: project) }
+
+        it 'does not play the stop action' do
+          expect { subject }.not_to change { stop_review_job.reload.status }
+        end
+      end
+    end
+
+    context 'without a deployment' do
+      let!(:environment) { create(:environment, project: project) }
+
+      it 'stops the environment' do
+        expect { subject }.to change { environment.reload.state }.from('available').to('stopped')
+      end
+
+      context 'when the actor is a reporter' do
+        let(:user) { reporter }
+
+        it 'does not stop the environment' do
+          expect { subject }.not_to change { environment.reload.state }
+        end
+      end
+    end
+  end
+
+  describe '#execute_for_branch' do
     context 'when environment with review app exists' do
       before do
         create(:environment, :with_review_app, project: project,
@@ -48,8 +101,9 @@ RSpec.describe Ci::StopEnvironmentsService do
 
         context 'when environment is not stopped' do
           before do
-            allow_any_instance_of(Environment)
-              .to receive(:state).and_return(:stopped)
+            allow_next_found_instance_of(Environment) do |environment|
+              allow(environment).to receive(:state).and_return(:stopped)
+            end
           end
 
           it 'does not stop environment' do
@@ -101,7 +155,7 @@ RSpec.describe Ci::StopEnvironmentsService do
 
     context 'when environment does not exist' do
       it 'does not raise error' do
-        expect { service.execute('master') }
+        expect { service.execute_for_branch('master') }
           .not_to raise_error
       end
     end
@@ -238,16 +292,12 @@ RSpec.describe Ci::StopEnvironmentsService do
   end
 
   def expect_environment_stopped_on(branch)
-    expect_any_instance_of(Environment)
-      .to receive(:stop!)
-
-    service.execute(branch)
+    expect { service.execute_for_branch(branch) }
+      .to change { Environment.last.state }.from('available').to('stopped')
   end
 
   def expect_environment_not_stopped_on(branch)
-    expect_any_instance_of(Environment)
-      .not_to receive(:stop!)
-
-    service.execute(branch)
+    expect { service.execute_for_branch(branch) }
+      .not_to change { Environment.last.state }
   end
 end

spec/services/git/branch_push_service_spec.rb

@@ -597,7 +597,7 @@ RSpec.describe Git::BranchPushService, services: true do
       let(:oldrev) { blankrev }
 
       it 'does nothing' do
-        expect(::Ci::StopEnvironmentsService).not_to receive(:new)
+        expect(::Environments::StopService).not_to receive(:new)
 
         execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
       end
@@ -605,7 +605,7 @@ RSpec.describe Git::BranchPushService, services: true do
 
     context 'update branch' do
       it 'does nothing' do
-        expect(::Ci::StopEnvironmentsService).not_to receive(:new)
+        expect(::Environments::StopService).not_to receive(:new)
 
         execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
       end
@@ -615,10 +615,10 @@ RSpec.describe Git::BranchPushService, services: true do
       let(:newrev) { blankrev }
 
       it 'stops environments' do
-        expect_next_instance_of(::Ci::StopEnvironmentsService) do |stop_service|
+        expect_next_instance_of(::Environments::StopService) do |stop_service|
           expect(stop_service.project).to eq(project)
           expect(stop_service.current_user).to eq(user)
-          expect(stop_service).to receive(:execute).with(branch)
+          expect(stop_service).to receive(:execute_for_branch).with(branch)
         end
 
         execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)

spec/services/merge_requests/close_service_spec.rb

@@ -92,7 +92,7 @@ RSpec.describe MergeRequests::CloseService do
     end
 
     it 'clean up environments for the merge request' do
-      expect_next_instance_of(Ci::StopEnvironmentsService) do |service|
+      expect_next_instance_of(::Environments::StopService) do |service|
         expect(service).to receive(:execute_for_merge_request).with(merge_request)
       end
 

spec/services/merge_requests/post_merge_service_spec.rb

@@ -75,7 +75,7 @@ RSpec.describe MergeRequests::PostMergeService do
     end
 
     it 'clean up environments for the merge request' do
-      expect_next_instance_of(Ci::StopEnvironmentsService) do |stop_environment_service|
+      expect_next_instance_of(::Environments::StopService) do |stop_environment_service|
         expect(stop_environment_service).to receive(:execute_for_merge_request).with(merge_request)
       end