Improve after code review

10 files changed, 86 insertions, 23 deletions

diff --git a/app/assets/javascripts/merge_request_widget.js.es6 b/app/assets/javascripts/merge_request_widget.js.es6
index fb55d13a223..639859ab96f 100644
--- a/app/assets/javascripts/merge_request_widget.js.es6
+++ b/app/assets/javascripts/merge_request_widget.js.es6
@@ -18,7 +18,7 @@
            </a>
          </span>
          <span class="close-env-container js-close-env-link">
-          <a href="<%- stop_url %>" class="close-evn-link" data-method="post" rel="nofollow" data-confirm="Are you sure you want to close this environment?">
+          <a href="<%- stop_url %>" class="close-evn-link" data-method="post" rel="nofollow" data-confirm="Are you sure you want to stop this environment?">
             <i class="fa fa-stop-circle-o"/>
             Stop environment
           </a>
diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb
index efdfbd24cae..86bc17a720a 100644
--- a/app/controllers/projects/environments_controller.rb
+++ b/app/controllers/projects/environments_controller.rb
@@ -8,11 +8,8 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   def index
     @scope = params[:scope]
     @all_environments = project.environments
-    @environments =
-      case @scope
-        when 'stopped' then @all_environments.stopped
-        else @all_environments.available
-      end
+    @environments = @scope == 'stopped' ?
+      @all_environments.stopped : @all_environments.available
   end
 
   def show
@@ -45,9 +42,11 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   end
 
   def stop
+    return render_404 unless @environment.stoppable?
+
     action = @environment.stop_action
     new_action = action.active? ? action : action.play(current_user)
-    redirect_to [project.namespace.becomes(Namespace), project, new_action]
+    redirect_to polymorphic_path([project.namespace.becomes(Namespace), project, new_action])
   end
 
   private
diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index f6cccae4334..1f8c5fb3d85 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -85,13 +85,14 @@ class Deployment < ActiveRecord::Base
   end
 
   def stop_action
+    return nil unless on_stop.present?
     return nil unless manual_actions
 
     @stop_action ||= manual_actions.find_by(name: on_stop)
   end
 
   def stoppable?
-    on_stop.present? && stop_action.present?
+    stop_action.present?
   end
 
   def formatted_deployment_time
diff --git a/app/models/environment.rb b/app/models/environment.rb
index 93e7dedd6f8..20da71ccb3f 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -19,10 +19,7 @@ class Environment < ActiveRecord::Base
             allow_nil: true,
             addressable_url: true
 
-  delegate :stoppable?, :stop_action, to: :last_deployment, allow_nil: true
-
-  scope :available, -> { where(state: [:available]) }
-  scope :stopped, -> { where(state: [:stopped]) }
+  delegate :stop_action, to: :last_deployment, allow_nil: true
 
   state_machine :state, initial: :available do
     event :start do
@@ -84,4 +81,8 @@ class Environment < ActiveRecord::Base
 
     external_url.gsub(/\A.*?:\/\//, '')
   end
+
+  def stoppable?
+    available? && stop_action.present?
+  end
 end
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index 5e6745cdd4e..85c0bf72074 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -6,10 +6,11 @@ class CreateDeploymentService < BaseService
 
     ActiveRecord::Base.transaction do
       @deployable = deployable
+
       @environment = environment
       @environment.external_url = expanded_url if expanded_url
       @environment.state_event = action
-      @environment.save
+      @environment.save!
 
       return if @environment.stopped?
 
@@ -33,7 +34,7 @@ class CreateDeploymentService < BaseService
       sha: params[:sha],
       user: current_user,
       deployable: @deployable,
-      on_stop: options.fetch(:on_stop, nil))
+      on_stop: options[:on_stop])
   end
 
   def environment
diff --git a/app/views/projects/environments/_stop.html.haml b/app/views/projects/environments/_stop.html.haml
index 6ed6aee141b..c7dec086890 100644
--- a/app/views/projects/environments/_stop.html.haml
+++ b/app/views/projects/environments/_stop.html.haml
@@ -1,5 +1,5 @@
-- if environment.available? && environment.stoppable?
+- if 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 close this environment?' } do
+      class: 'btn close-env-link', rel: 'nofollow', data: { confirm: 'Are you sure you want to stop this environment?' } do
       = icon('stop', class: 'close-env-icon')
diff --git a/app/views/projects/environments/index.html.haml b/app/views/projects/environments/index.html.haml
index 70185176222..705a1360ec5 100644
--- a/app/views/projects/environments/index.html.haml
+++ b/app/views/projects/environments/index.html.haml
@@ -17,8 +17,8 @@
           %span.badge.js-stopped-environments-count
             = number_with_delimiter(@all_environments.stopped.count)
 
-    .nav-controls
-      - if can?(current_user, :create_environment, @project) && !@all_environments.blank?
+    - if can?(current_user, :create_environment, @project) && !@all_environments.blank?
+      .nav-controls
         = link_to new_namespace_project_environment_path(@project.namespace, @project), class: 'btn btn-create' do
           New environment
 
diff --git a/app/views/projects/environments/show.html.haml b/app/views/projects/environments/show.html.haml
index 3b4d0395db0..b6a1a7fc89e 100644
--- a/app/views/projects/environments/show.html.haml
+++ b/app/views/projects/environments/show.html.haml
@@ -13,9 +13,8 @@
 
         - if can?(current_user, :update_environment, @environment)
           = link_to 'Edit', edit_namespace_project_environment_path(@project.namespace, @project, @environment), class: 'btn'
-          - if @environment.available? && @environment.stoppable?
-            = link_to 'Stop', stop_namespace_project_environment_path(@project.namespace, @project, @environment), data: { confirm: 'Are you sure you want to close this environment?' }, class: 'btn btn-danger', method: :post
-          = link_to 'Destroy', namespace_project_environment_path(@project.namespace, @project, @environment), data: { confirm: 'Are you sure you want to delete this environment?' }, class: 'btn btn-danger', method: :delete
+          - 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
 
   .deployments-container
     - if @deployments.blank?
diff --git a/lib/gitlab/ci/config/node/environment.rb b/lib/gitlab/ci/config/node/environment.rb
index 1c1d07843b1..b392f272bd6 100644
--- a/lib/gitlab/ci/config/node/environment.rb
+++ b/lib/gitlab/ci/config/node/environment.rb
@@ -37,10 +37,10 @@ module Gitlab
                         allow_nil: true
 
               validates :action,
-                        inclusion: { in: %w[start stop], message: 'should be start or stop, ' },
+                        inclusion: { in: %w[start stop], message: 'should be start or stop' },
                         allow_nil: true
 
-              validates :on_stop, string: true, allow_nil: true
+              validates :on_stop, type: String, allow_nil: true
             end
           end
 
diff --git a/spec/lib/gitlab/ci/config/node/environment_spec.rb b/spec/lib/gitlab/ci/config/node/environment_spec.rb
index df453223da7..430e18a816f 100644
--- a/spec/lib/gitlab/ci/config/node/environment_spec.rb
+++ b/spec/lib/gitlab/ci/config/node/environment_spec.rb
@@ -87,6 +87,68 @@ describe Gitlab::Ci::Config::Node::Environment do
     end
   end
 
+  context 'when valid action is used' do
+    let(:config) do
+      { name: 'production',
+        action: 'start' }
+    end
+
+    it 'is valid' do
+      expect(entry).to be_valid
+    end
+  end
+
+  context 'when invalid action is used' do
+    let(:config) do
+      { name: 'production',
+        action: false }
+    end
+
+    describe '#valid?' do
+      it 'is not valid' do
+        expect(entry).not_to be_valid
+      end
+    end
+
+    describe '#errors' do
+      it 'contains error about invalid action' do
+        expect(entry.errors)
+          .to include 'environment action should be start or stop'
+      end
+    end
+  end
+
+  context 'when on_stop is used' do
+    let(:config) do
+      { name: 'production',
+        on_stop: 'close_app' }
+    end
+
+    it 'is valid' do
+      expect(entry).to be_valid
+    end
+  end
+
+  context 'when invalid on_stop is used' do
+    let(:config) do
+      { name: 'production',
+        on_stop: false }
+    end
+
+    describe '#valid?' do
+      it 'is not valid' do
+        expect(entry).not_to be_valid
+      end
+    end
+
+    describe '#errors' do
+      it 'contains error about invalid action' do
+        expect(entry.errors)
+          .to include 'environment action should be start or stop'
+      end
+    end
+  end
+
   context 'when variables are used for environment' do
     let(:config) do
       { name: 'review/$CI_BUILD_REF_NAME',