Fix environments specs

8 files changed, 74 insertions, 32 deletions

diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb
index 86bc17a720a..02a659d3894 100644
--- a/app/controllers/projects/environments_controller.rb
+++ b/app/controllers/projects/environments_controller.rb
@@ -1,8 +1,9 @@
 class Projects::EnvironmentsController < Projects::ApplicationController
   layout 'project'
   before_action :authorize_read_environment!
-  before_action :authorize_create_environment!, only: [:new, :create]
-  before_action :authorize_update_environment!, only: [:edit, :update, :stop]
+  before_action :authorize_create_environment!, only: [:new, :create, :stop]
+  before_action :authorize_create_deployment!, only: [:stop]
+  before_action :authorize_update_environment!, only: [:edit, :update]
   before_action :environment, only: [:show, :edit, :update, :stop]
 
   def index
diff --git a/app/models/environment.rb b/app/models/environment.rb
index 20da71ccb3f..ff55e751f70 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -21,6 +21,9 @@ class Environment < ActiveRecord::Base
 
   delegate :stop_action, to: :last_deployment, allow_nil: true
 
+  scope :available, -> { with_state(:available) }
+  scope :stopped, -> { with_state(:stopped) }
+
   state_machine :state, initial: :available do
     event :start do
       transition stopped: :available
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index c55d2ed231a..8ae15ad32f4 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -10,8 +10,8 @@ class CreateDeploymentService < BaseService
       @environment = environment
       @environment.external_url = expanded_url if expanded_url
       @environment.fire_state_event(action)
-      @environment.save!
 
+      return unless @environment.save
       return if @environment.stopped?
 
       deploy.tap do |deployment|
diff --git a/app/views/projects/environments/_external_url.html.haml b/app/views/projects/environments/_external_url.html.haml
index 6255e4baea0..4c8fe1c271b 100644
--- a/app/views/projects/environments/_external_url.html.haml
+++ b/app/views/projects/environments/_external_url.html.haml
@@ -1,3 +1,3 @@
-- if environment.external_url
+- if environment.external_url && can?(current_user, :read_environment, environment)
   = link_to environment.external_url, target: '_blank', class: 'btn external-url' do
     = icon('external-link')
diff --git a/app/views/projects/environments/_stop.html.haml b/app/views/projects/environments/_stop.html.haml
index c7dec086890..880f8c8c62c 100644
--- a/app/views/projects/environments/_stop.html.haml
+++ b/app/views/projects/environments/_stop.html.haml
@@ -1,4 +1,4 @@
-- if environment.stoppable?
+- if can?(current_user, :create_deployment, environment) && environment.stoppable?
   .inline
     = link_to stop_namespace_project_environment_path(@project.namespace, @project, environment), method: :post,
       class: 'btn close-env-link', rel: 'nofollow', data: { confirm: 'Are you sure you want to stop this environment?' } do
diff --git a/app/views/projects/environments/show.html.haml b/app/views/projects/environments/show.html.haml
index b6a1a7fc89e..bf082a05c39 100644
--- a/app/views/projects/environments/show.html.haml
+++ b/app/views/projects/environments/show.html.haml
@@ -8,13 +8,11 @@
       %h3.page-title= @environment.name.capitalize
     .col-md-3
       .nav-controls
-        - if can?(current_user, :read_environmnet, @environment)
-          = render 'projects/environments/external_url', environment: @environment
-
+        = render 'projects/environments/external_url', environment: @environment
         - if can?(current_user, :update_environment, @environment)
           = link_to 'Edit', edit_namespace_project_environment_path(@project.namespace, @project, @environment), class: 'btn'
-          - if @environment.stoppable?
-            = link_to 'Stop', stop_namespace_project_environment_path(@project.namespace, @project, @environment), data: { confirm: 'Are you sure you want to stop this environment?' }, class: 'btn btn-danger', method: :post
+        - if can?(current_user, :create_deployment, @environment) && @environment.stoppable?
+          = link_to 'Stop', stop_namespace_project_environment_path(@project.namespace, @project, @environment), data: { confirm: 'Are you sure you want to stop this environment?' }, class: 'btn btn-danger', method: :post
 
   .deployments-container
     - if @deployments.blank?
diff --git a/spec/features/environments_spec.rb b/spec/features/environments_spec.rb
index 35f5706f920..1b51bf9ef66 100644
--- a/spec/features/environments_spec.rb
+++ b/spec/features/environments_spec.rb
@@ -35,7 +35,7 @@ feature 'Environments', feature: true do
       end
       
       scenario 'does show 0 as counter for environments in both tabs' do
-        expect(page.find('.js-avaibale-environments-count').text).to eq('0')
+        expect(page.find('.js-available-environments-count').text).to eq('0')
         expect(page.find('.js-stopped-environments-count').text).to eq('0')
       end
     end
@@ -48,7 +48,7 @@ feature 'Environments', feature: true do
       end
       
       scenario 'does show number of opened environments in Availabe tab' do
-        expect(page.find('.js-avaibale-environments-count').text).to eq('1')
+        expect(page.find('.js-available-environments-count').text).to eq('1')
       end
 
       scenario 'does show number of closed environments in Stopped tab' do
@@ -92,6 +92,14 @@ feature 'Environments', feature: true do
           scenario 'does show build name and id' do
             expect(page).to have_link("#{build.name} (##{build.id})")
           end
+
+          scenario 'does not show stop button' do
+            expect(page).not_to have_selector('.close-env-link')
+          end
+
+          scenario 'does not show external link button' do
+            expect(page).not_to have_css('external-url')
+          end
           
           context 'with external_url' do
             given(:environment) { create(:environment, project: project, external_url: 'https://git.gitlab.com') }
@@ -103,14 +111,27 @@ feature 'Environments', feature: true do
             end
           end
 
-          scenario 'does show close button' do 
-            # TODO: Add test to verify if close button is visible
-            # This needs to be true: if local_assigns.fetch(:allow_close, false) && deployment.closeable?
-          end
-          
-          scenario 'does allow to close environment' do 
-            # TODO: Add test to verify if close environment works
-            # This needs to be true: if local_assigns.fetch(:allow_close, false) && deployment.closeable?
+          context 'with stop action' do
+            given(:manual) { create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') }
+            given(:deployment) { create(:deployment, environment: environment, deployable: build, on_stop: 'close_app') }
+
+            scenario 'does show stop button' do
+              expect(page).to have_selector('.close-env-link')
+            end
+
+            scenario 'does allow to stop environment' do
+              first('.close-env-link').click
+
+              expect(page).to have_content('close_app')
+            end
+
+            context 'for reporter' do
+              let(:role) { :reporter }
+
+              scenario 'does not show stop button' do
+                expect(page).not_to have_selector('.close-env-link')
+              end
+            end
           end
         end
       end
@@ -160,6 +181,10 @@ feature 'Environments', feature: true do
           expect(page).to have_link('Re-deploy')
         end
 
+        scenario 'does not show stop button' do
+          expect(page).not_to have_link('Stop')
+        end
+
         context 'with manual action' do
           given(:manual) { create(:ci_build, :manual, pipeline: pipeline, name: 'deploy to production') }
 
@@ -178,20 +203,33 @@ feature 'Environments', feature: true do
             given(:environment) { create(:environment, project: project, external_url: 'https://git.gitlab.com') }
             given(:build) { create(:ci_build, pipeline: pipeline) }
             given(:deployment) { create(:deployment, environment: environment, deployable: build) }
-            
+
             scenario 'does show an external link button' do
               expect(page).to have_link(nil, href: environment.external_url)
             end
           end
 
-          scenario 'does show close button' do 
-            # TODO: Add test to verify if close button is visible
-            # This needs to be true: if local_assigns.fetch(:allow_close, false) && deployment.closeable?
-          end
-          
-          scenario 'does allow to close environment' do 
-            # TODO: Add test to verify if close environment works
-            # This needs to be true: if local_assigns.fetch(:allow_close, false) && deployment.closeable?
+          context 'with stop action' do
+            given(:manual) { create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') }
+            given(:deployment) { create(:deployment, environment: environment, deployable: build, on_stop: 'close_app') }
+
+            scenario 'does show stop button' do
+              expect(page).to have_link('Stop')
+            end
+
+            scenario 'does allow to stop environment' do
+              click_link('Stop')
+
+              expect(page).to have_content('close_app')
+            end
+
+            context 'for reporter' do
+              let(:role) { :reporter }
+
+              scenario 'does not show stop button' do
+                expect(page).not_to have_link('Stop')
+              end
+            end
           end
         end
       end
diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb
index d4ceb83caf8..5a4562b939b 100644
--- a/spec/services/create_deployment_service_spec.rb
+++ b/spec/services/create_deployment_service_spec.rb
@@ -54,8 +54,8 @@ describe CreateDeploymentService, services: true do
             expect(environment.reload).to be_available
           end
 
-          it 'does not create a deployment' do
-            expect(subject).not_to be_persisted
+          it 'does create a deployment' do
+            expect(subject).to be_persisted
           end
         end
       end
@@ -95,7 +95,7 @@ describe CreateDeploymentService, services: true do
       end
 
       it 'does not create a deployment' do
-        expect(subject).not_to be_persisted
+        expect(subject).to be_nil
       end
     end
 
@@ -127,6 +127,8 @@ describe CreateDeploymentService, services: true do
       end
 
       context 'and environment exist' do
+        let!(:environment) { create(:environment, project: project, name: 'review-apps/feature-review-apps') }
+
         it 'does not create a new environment' do
           expect { subject }.not_to change { Environment.count }
         end