Commit 9b790f1c authored by Kamil Trzcinski's avatar Kamil Trzcinski

Improve after code review

parent 50d3cc2b
...@@ -18,7 +18,7 @@ ...@@ -18,7 +18,7 @@
</a> </a>
</span> </span>
<span class="close-env-container js-close-env-link"> <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"/> <i class="fa fa-stop-circle-o"/>
Stop environment Stop environment
</a> </a>
......
...@@ -8,11 +8,8 @@ class Projects::EnvironmentsController < Projects::ApplicationController ...@@ -8,11 +8,8 @@ class Projects::EnvironmentsController < Projects::ApplicationController
def index def index
@scope = params[:scope] @scope = params[:scope]
@all_environments = project.environments @all_environments = project.environments
@environments = @environments = @scope == 'stopped' ?
case @scope @all_environments.stopped : @all_environments.available
when 'stopped' then @all_environments.stopped
else @all_environments.available
end
end end
def show def show
...@@ -45,9 +42,11 @@ class Projects::EnvironmentsController < Projects::ApplicationController ...@@ -45,9 +42,11 @@ class Projects::EnvironmentsController < Projects::ApplicationController
end end
def stop def stop
return render_404 unless @environment.stoppable?
action = @environment.stop_action action = @environment.stop_action
new_action = action.active? ? action : action.play(current_user) 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 end
private private
......
...@@ -85,13 +85,14 @@ class Deployment < ActiveRecord::Base ...@@ -85,13 +85,14 @@ class Deployment < ActiveRecord::Base
end end
def stop_action def stop_action
return nil unless on_stop.present?
return nil unless manual_actions return nil unless manual_actions
@stop_action ||= manual_actions.find_by(name: on_stop) @stop_action ||= manual_actions.find_by(name: on_stop)
end end
def stoppable? def stoppable?
on_stop.present? && stop_action.present? stop_action.present?
end end
def formatted_deployment_time def formatted_deployment_time
......
...@@ -19,10 +19,7 @@ class Environment < ActiveRecord::Base ...@@ -19,10 +19,7 @@ class Environment < ActiveRecord::Base
allow_nil: true, allow_nil: true,
addressable_url: true addressable_url: true
delegate :stoppable?, :stop_action, to: :last_deployment, allow_nil: true delegate :stop_action, to: :last_deployment, allow_nil: true
scope :available, -> { where(state: [:available]) }
scope :stopped, -> { where(state: [:stopped]) }
state_machine :state, initial: :available do state_machine :state, initial: :available do
event :start do event :start do
...@@ -84,4 +81,8 @@ class Environment < ActiveRecord::Base ...@@ -84,4 +81,8 @@ class Environment < ActiveRecord::Base
external_url.gsub(/\A.*?:\/\//, '') external_url.gsub(/\A.*?:\/\//, '')
end end
def stoppable?
available? && stop_action.present?
end
end end
...@@ -6,10 +6,11 @@ class CreateDeploymentService < BaseService ...@@ -6,10 +6,11 @@ class CreateDeploymentService < BaseService
ActiveRecord::Base.transaction do ActiveRecord::Base.transaction do
@deployable = deployable @deployable = deployable
@environment = environment @environment = environment
@environment.external_url = expanded_url if expanded_url @environment.external_url = expanded_url if expanded_url
@environment.state_event = action @environment.state_event = action
@environment.save @environment.save!
return if @environment.stopped? return if @environment.stopped?
...@@ -33,7 +34,7 @@ class CreateDeploymentService < BaseService ...@@ -33,7 +34,7 @@ class CreateDeploymentService < BaseService
sha: params[:sha], sha: params[:sha],
user: current_user, user: current_user,
deployable: @deployable, deployable: @deployable,
on_stop: options.fetch(:on_stop, nil)) on_stop: options[:on_stop])
end end
def environment def environment
......
- if environment.available? && environment.stoppable? - if environment.stoppable?
.inline .inline
= link_to stop_namespace_project_environment_path(@project.namespace, @project, environment), method: :post, = 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') = icon('stop', class: 'close-env-icon')
...@@ -17,8 +17,8 @@ ...@@ -17,8 +17,8 @@
%span.badge.js-stopped-environments-count %span.badge.js-stopped-environments-count
= number_with_delimiter(@all_environments.stopped.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 = link_to new_namespace_project_environment_path(@project.namespace, @project), class: 'btn btn-create' do
New environment New environment
......
...@@ -13,9 +13,8 @@ ...@@ -13,9 +13,8 @@
- if can?(current_user, :update_environment, @environment) - if can?(current_user, :update_environment, @environment)
= link_to 'Edit', edit_namespace_project_environment_path(@project.namespace, @project, @environment), class: 'btn' = link_to 'Edit', edit_namespace_project_environment_path(@project.namespace, @project, @environment), class: 'btn'
- if @environment.available? && @environment.stoppable? - if @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 '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
= 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
.deployments-container .deployments-container
- if @deployments.blank? - if @deployments.blank?
......
...@@ -37,10 +37,10 @@ module Gitlab ...@@ -37,10 +37,10 @@ module Gitlab
allow_nil: true allow_nil: true
validates :action, 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 allow_nil: true
validates :on_stop, string: true, allow_nil: true validates :on_stop, type: String, allow_nil: true
end end
end end
......
...@@ -87,6 +87,68 @@ describe Gitlab::Ci::Config::Node::Environment do ...@@ -87,6 +87,68 @@ describe Gitlab::Ci::Config::Node::Environment do
end end
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 context 'when variables are used for environment' do
let(:config) do let(:config) do
{ name: 'review/$CI_BUILD_REF_NAME', { name: 'review/$CI_BUILD_REF_NAME',
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment