On Fri, Apr 01, 2011 at 04:51:57PM +0200, Tomas Sedovic wrote:
Hey Steve,
ACK, works fine. I have a few suggestions about the code style (inline). Feel free to ignore them and push anyway.
Thomas
On 04/01/2011 06:04 AM, Steve Linabery wrote:
From: Steve Linaberyslinabery@gmail.com
Added multi_stop method to instances_controller.rb, changed form to send correct params, edited routes.rb to match new path.
.../controllers/resources/instances_controller.rb | 55 ++++++++++++++------ src/app/views/resources/instances/_list.haml | 4 +- src/config/routes.rb | 2 +- src/features/instance.feature | 14 +++++ src/features/step_definitions/instance_steps.rb | 6 ++- 5 files changed, 61 insertions(+), 20 deletions(-)
diff --git a/src/app/controllers/resources/instances_controller.rb b/src/app/controllers/resources/instances_controller.rb index f857aa7..7b26383 100644 --- a/src/app/controllers/resources/instances_controller.rb +++ b/src/app/controllers/resources/instances_controller.rb @@ -1,6 +1,6 @@ class Resources::InstancesController< ApplicationController before_filter :require_user, :except => [:can_start, :can_create]
- before_filter :load_instance, :only => [:show, :remove_failed, :key, :stop]
before_filter :load_instance, :only => [:show, :key] before_filter :set_view_vars, :only => [:show, :index]
def index
@@ -106,27 +106,50 @@ class Resources::InstancesController< ApplicationController redirect_to resources_instance_path(@instance) end
- def stop
- unless @instance.valid_action?('stop')
raise ActionError.new("stop is an invalid action.")
- end
- def multi_stop
- notices = ""
- errors = ""
- Instance.find(params[:instance_selected]).each do |instance|
begin
require_privilege(Privilege::USE,instance)
unless instance.valid_action?('stop')
raise ActionError.new("stop is an invalid action.")
end
- # not sure if task is used as everything goes through condor
- #permissons check here
- @task = @instance.queue_action(@current_user, 'stop')
- unless @task
raise ActionError.new("stop cannot be performed on this instance.")
# not sure if task is used as everything goes through condor
#permissons check here
@task = instance.queue_action(@current_user, 'stop')
unless @task
raise ActionError.new("stop cannot be performed on this instance.")
end
condormatic_instance_stop(@task)
notices<< "#{instance.name}: stop action was successfully queued.<br/>"
rescue Exception => err
errors<< "#{instance.name}: " + err +"<br/>"
endend
- condormatic_instance_stop(@task)
- flash[:notice] = "#{@instance.name}: stop action was successfully queued."
- flash[:notice] = notices unless notices == ""
- flash[:error] = errors unless errors == ""
Might I suggest using notices.blank? and errors.blank? here? I think it's a tad more readable.
redirect_to resources_instances_path
end
You've added an empty line here. In case it was not intentional, could you please remove it?
def remove_failed
- raise ActionError.new("remove failed cannot be performed on this instance.") unless
@instance.state == Instance::STATE_ERROR
- condormatic_instance_reset_error(@instance)
- flash[:notice] = "#{@instance.name}: remove failed action was successfully queued."
- notices = ""
- errors = ""
- Instance.find(params[:instance_selected]).each do |instance|
begin
require_privilege(Privilege::USE,instance)
raise ActionError.new("remove failed cannot be performed on this instance.") unless
instance.state == Instance::STATE_ERROR
condormatic_instance_reset_error(instance)
notices<< "#{instance.name}: remove failed action was successfully queued."
rescue Exception => err
errors<< "#{instance.name}: " + err +"<br/>"
end
- end
- flash[:notice] = notices unless notices == ""
- flash[:error] = errors unless errors == ""
I think the `blank?` method fits here more as well.
redirect_to resources_instances_path
end
diff --git a/src/app/views/resources/instances/_list.haml b/src/app/views/resources/instances/_list.haml index b62d9e7..157dc97 100644 --- a/src/app/views/resources/instances/_list.haml +++ b/src/app/views/resources/instances/_list.haml @@ -1,5 +1,5 @@
- form_tag do
- = restful_submit_tag 'Stop', 'stop', stop_resources_instances_path, 'GET'
- = restful_submit_tag 'Stop', 'stop', multi_stop_resources_instances_path, 'GET' = restful_submit_tag 'Create', 'new', select_template_resources_instances_path, 'GET' = restful_submit_tag 'Remove failed', 'remove_failed', remove_failed_resources_instances_path, 'GET'
@@ -14,7 +14,7 @@ %tr %td - selected = params[:select] == 'all'
%input{:checked => selected, :name => 'id', :type => 'checkbox', :value => inst.id, :id => "inst_ids_#{inst.id}" }
%input{:name => 'instance_selected[]', :type => 'checkbox', :value => inst.id, :id => "instance_checkbox_#{inst.id}", :checked => selected } = link_to inst.name, resources_instance_path(inst) %td= inst.state %td= inst.template.name
diff --git a/src/config/routes.rb b/src/config/routes.rb index 313dd25..88e3f71 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -35,7 +35,7 @@ ActionController::Routing::Routes.draw do |map| map.namespace 'resources' do |r| r.resources :pools, :collection => { :multi_destroy => :delete } r.resources :deployments
- r.resources :instances, :collection => {:start => :get, :stop => :get, :select_template => :get, :remove_failed => :get, :can_start => :get, :can_create => :get }, :member => {:key => :get}
r.resources :instances, :collection => {:start => :get, :multi_stop => :get, :select_template => :get, :remove_failed => :get, :can_start => :get, :can_create => :get }, :member => {:key => :get} end
map.can_start_instance '/resources/instances/:instance_id/can_start/:provider_account_id', :controller => 'resources/instances', :action => 'can_start', :conditions => { :method => :get }
diff --git a/src/features/instance.feature b/src/features/instance.feature index 19b80bd..3b4e000 100644 --- a/src/features/instance.feature +++ b/src/features/instance.feature @@ -77,6 +77,20 @@ Feature: Mange Instances Then I should be on the instances page And I should see "mock1: stop action was successfully queued"
- Scenario: Stop multiple instances
- Given there is a "mock1" running instance
- And there is a "mock2" running instance
- And there is a "mock3" stopped instance
- And I am on the instances page
- When I check "mock1" instance
- And I check "mock2" instance
- And I check "mock3" instance
- And I press "Stop"
- Then I should be on the instances page
- And I should see "mock1: stop action was successfully queued"
- And I should see "mock2: stop action was successfully queued"
- And I should see "mock3: stop is an invalid action"
- @tag Scenario: Search for instances Given there are the following instances:
diff --git a/src/features/step_definitions/instance_steps.rb b/src/features/step_definitions/instance_steps.rb index 66e1282..b7daa18 100644 --- a/src/features/step_definitions/instance_steps.rb +++ b/src/features/step_definitions/instance_steps.rb @@ -47,6 +47,10 @@ Given /^there is a "([^"]*)" running instance$/ do |name| Factory :instance, :name => name, :state => Instance::STATE_RUNNING end
+Given /^there is a "([^"]*)" stopped instance$/ do |name|
- Factory :instance, :name => name, :state => Instance::STATE_STOPPED
+end
Given /^there is an uploaded image for a template$/ do Factory :provider_image end @@ -66,7 +70,7 @@ end
When /^I check "([^"]*)" instance$/ do |name| inst = Instance.find_by_name(name)
- check("inst_ids_#{inst.id}")
- check("instance_checkbox_#{inst.id}")
end
Given /^there are the following instances:$/ do |table|
Thanks for the review, Thomas; I will incorporate those changes before I push.
Have a great weekend, S|E