From 9b790f1cf97157240178601c62d2e557a404503e Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 17 Oct 2016 16:13:19 +0200 Subject: [PATCH] Improve after code review --- .../javascripts/merge_request_widget.js.es6 | 2 +- .../projects/environments_controller.rb | 11 ++-- app/models/deployment.rb | 3 +- app/models/environment.rb | 9 +-- app/services/create_deployment_service.rb | 5 +- .../projects/environments/_stop.html.haml | 4 +- .../projects/environments/index.html.haml | 4 +- .../projects/environments/show.html.haml | 5 +- lib/gitlab/ci/config/node/environment.rb | 4 +- .../gitlab/ci/config/node/environment_spec.rb | 62 +++++++++++++++++++ 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 @@ - + Stop environment 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', -- GitLab