10:36 a.m.
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 Linabery<slinabery(a)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/>"
>+ end
> end
>- condormatic_instance_stop(@task)
>- flash[:notice] = "#{(a)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] = "#{(a)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